feat(amber): guard cloneWorkflow with a read-access check - #7605
Merged
aglinxinyuan merged 1 commit intoAug 13, 2026
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 372 | 0.227 | 25,144/37,901/37,901 us | 🔴 -10.3% / 🔴 +132.4% |
| 🔴 | bs=100 sw=10 sl=64 | 774 | 0.472 | 128,999/146,692/146,692 us | 🔴 +12.4% / 🔴 +35.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 915 | 0.558 | 1,088,222/1,179,927/1,179,927 us | ⚪ within ±5% / 🔴 +13.2% |
Baseline details
Latest main dd7d813 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 372 tuples/sec | 414 tuples/sec | 775.33 tuples/sec | -10.1% | -52.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.227 MB/s | 0.253 MB/s | 0.473 MB/s | -10.3% | -52.0% |
| bs=10 sw=10 sl=64 | p50 | 25,144 us | 23,129 us | 12,743 us | +8.7% | +97.3% |
| bs=10 sw=10 sl=64 | p95 | 37,901 us | 34,520 us | 16,310 us | +9.8% | +132.4% |
| bs=10 sw=10 sl=64 | p99 | 37,901 us | 34,520 us | 18,926 us | +9.8% | +100.3% |
| bs=100 sw=10 sl=64 | throughput | 774 tuples/sec | 841 tuples/sec | 1,001 tuples/sec | -8.0% | -22.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.472 MB/s | 0.514 MB/s | 0.611 MB/s | -8.2% | -22.7% |
| bs=100 sw=10 sl=64 | p50 | 128,999 us | 114,809 us | 101,399 us | +12.4% | +27.2% |
| bs=100 sw=10 sl=64 | p95 | 146,692 us | 138,779 us | 108,206 us | +5.7% | +35.6% |
| bs=100 sw=10 sl=64 | p99 | 146,692 us | 138,779 us | 118,195 us | +5.7% | +24.1% |
| bs=1000 sw=10 sl=64 | throughput | 915 tuples/sec | 918 tuples/sec | 1,026 tuples/sec | -0.3% | -10.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.558 MB/s | 0.56 MB/s | 0.626 MB/s | -0.4% | -10.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,088,222 us | 1,085,309 us | 996,304 us | +0.3% | +9.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,179,927 us | 1,165,966 us | 1,042,531 us | +1.2% | +13.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,179,927 us | 1,165,966 us | 1,074,934 us | +1.2% | +9.8% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,537.86,200,128000,372,0.227,25143.98,37901.31,37901.31
1,100,10,64,20,2583.66,2000,1280000,774,0.472,128999.13,146691.94,146691.94
2,1000,10,64,20,21869.06,20000,12800000,915,0.558,1088222.06,1179926.63,1179926.63
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7605 +/- ##
============================================
- Coverage 88.63% 87.40% -1.24%
+ Complexity 4346 4342 -4
============================================
Files 1177 1177
Lines 46894 46782 -112
Branches 5229 5191 -38
============================================
- Hits 41566 40890 -676
- Misses 3568 4107 +539
- Partials 1760 1785 +25
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cloneWorkflow fetched the source by wid and copied its content into a workflow owned by the caller, with no access check on the way in. Any authenticated REGULAR user could POST /workflow/clone/<wid> for a wid they hold no privilege on and receive a full copy of a private workflow's content. Every sibling guards: retrieveWorkflow and duplicateWorkflow call hasReadAccess directly, and the version endpoint cloneVersion inherits the check through retrieveWorkflowVersion. cloneWorkflow was the one that did not, which reads as an oversight rather than a decision -- SECURITY.md states that REGULAR users cannot access other users' private resources without granted permissions. Add the same guard the siblings use. hasReadAccess already returns true for public workflows, so the hub's clone button -- the only caller, and always on a published workflow -- is unaffected; a caller holding an explicit READ grant is likewise still able to clone, which the new tests pin alongside the rejection.
aglinxinyuan
force-pushed
the
fix/clone-workflow-read-access
branch
from
August 13, 2026 03:58
cf8d403 to
522abef
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala:1242
- The clone-rejection test asserts that no
WORKFLOW_USER_CLONESrow exists for the sourcewidat all, but the behavior being pinned is that the caller (user2) did not get a clone record. Filtering only byWIDcan make this test fail spuriously if another user ever clones the same workflow in the same DB lifecycle (or if fixtures change). Prefer matching the earlier clone test by filtering on bothWIDandUID.
// no copy reached the caller, and the rejected attempt was not recorded as a clone
assert(workflowNamesOf(sessionUser2).isEmpty)
assert(getDSLContext.fetchCount(WORKFLOW_USER_CLONES, WORKFLOW_USER_CLONES.WID.eq(wid)) == 0)
Contributor
|
Backport PR opened: draft #7625 (#7625) to |
aglinxinyuan
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
cloneWorkflowfetched the source workflow bywidand copied its content into a workflow owned by the caller, with no access check on the way in. Any authenticated REGULAR user couldPOST /workflow/clone/<wid>for a wid they hold no privilege on and receive a full copy of a private workflow's content — operator configurations, file paths and all.Root cause. Every sibling on this path guards; this one endpoint did not.
retrieveWorkflowhasReadAccessdirectlyduplicateWorkflowhasReadAccessdirectlycloneVersion(/version/clone/{vid})retrieveWorkflowVersioncloneWorkflowThat reads as an oversight rather than a decision. SECURITY.md states that REGULAR users "cannot access other users' private resources without granted permissions", so the endpoint contradicted the project's own declared model.
Before → after
The fix adds the same three lines the siblings use.
hasReadAccessalready returns true for public workflows, so the hub's clone button — the only caller, and always acting on a published workflow — is unaffected. A caller holding an explicit READ grant can still clone.Any related issues, documentation, discussions?
Found while reviewing the clone-endpoint test in #7592, now merged; the new cases here build on the spec helpers that landed with it. Not filed as an issue, because SECURITY.md asks that security bugs not be reported through public issues.
The
release/v1.2backport preflight comes back grey: the guard itself applies, but the two new test cases depend on helpers that arrived with #7592, which was not backported. The backport needs those cases rewritten self-contained, so it will have to be resolved by hand rather than pushed straight through.How was this PR tested?
Two cases added to the existing
WorkflowResourceSpec, and the pre-existing success case in #7592 now publishes its source first so it exercises the public path.clone a private workflow the caller has been granted read access toreject a caller with no access to the source workflowWORKFLOW_USER_CLONESrow is written for the rejected attemptRed before the guard, green after — with the guard reverted the rejection case fails and the other 69 pass, so it is the guard the test is pinning and not a fixture.
scalafmtCheckandscalafix --checkpass for bothCompileandTest.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)