feat(amber): reject mismatched hub access queries - #8138
Conversation
Backport auto-label reportThis
|
Automated Reviewer SuggestionsBased on the
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🟢 | bs=10 sw=10 sl=64 | 443 | 0.27 | 21,470/32,290/32,290 us | 🟢 -13.0% / 🔴 +103.1% |
| 🔴 | bs=100 sw=10 sl=64 | 883 | 0.539 | 108,729/146,049/146,049 us | 🔴 +9.9% / 🔴 +32.0% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,059 | 0.646 | 935,682/1,095,610/1,095,610 us | 🔴 +10.3% / 🟢 -7.6% |
Baseline details
Latest main b28faee from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 443 tuples/sec | 451 tuples/sec | 750.65 tuples/sec | -1.8% | -41.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.27 MB/s | 0.275 MB/s | 0.458 MB/s | -1.8% | -41.1% |
| bs=10 sw=10 sl=64 | p50 | 21,470 us | 20,694 us | 13,049 us | +3.7% | +64.5% |
| bs=10 sw=10 sl=64 | p95 | 32,290 us | 37,105 us | 15,902 us | -13.0% | +103.1% |
| bs=10 sw=10 sl=64 | p99 | 32,290 us | 37,105 us | 19,184 us | -13.0% | +68.3% |
| bs=100 sw=10 sl=64 | throughput | 883 tuples/sec | 938 tuples/sec | 960.3 tuples/sec | -5.9% | -8.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.539 MB/s | 0.573 MB/s | 0.586 MB/s | -5.9% | -8.0% |
| bs=100 sw=10 sl=64 | p50 | 108,729 us | 102,777 us | 104,199 us | +5.8% | +4.3% |
| bs=100 sw=10 sl=64 | p95 | 146,049 us | 132,860 us | 110,645 us | +9.9% | +32.0% |
| bs=100 sw=10 sl=64 | p99 | 146,049 us | 132,860 us | 120,888 us | +9.9% | +20.8% |
| bs=1000 sw=10 sl=64 | throughput | 1,059 tuples/sec | 1,078 tuples/sec | 993.27 tuples/sec | -1.8% | +6.6% |
| bs=1000 sw=10 sl=64 | MB/s | 0.646 MB/s | 0.658 MB/s | 0.606 MB/s | -1.8% | +6.6% |
| bs=1000 sw=10 sl=64 | p50 | 935,682 us | 918,519 us | 1,012,191 us | +1.9% | -7.6% |
| bs=1000 sw=10 sl=64 | p95 | 1,095,610 us | 993,247 us | 1,054,057 us | +10.3% | +3.9% |
| bs=1000 sw=10 sl=64 | p99 | 1,095,610 us | 993,247 us | 1,081,313 us | +10.3% | +1.3% |
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,451.74,200,128000,443,0.270,21469.62,32290.48,32290.48
1,100,10,64,20,2265.51,2000,1280000,883,0.539,108729.35,146049.09,146049.09
2,1000,10,64,20,18883.06,20000,12800000,1059,0.646,935682.01,1095610.40,1095610.40
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8138 +/- ##
============================================
- Coverage 94.07% 94.07% -0.01%
+ Complexity 4811 4809 -2
============================================
Files 1193 1193
Lines 48573 48579 +6
Branches 5854 5855 +1
============================================
+ Hits 45696 45699 +3
Misses 1428 1428
- Partials 1449 1452 +3
*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:
|
There was a problem hiding this comment.
🔴 2 must-fix · 0 advisory · 1 polish — the guard itself is right; both blockers are about what the change leaves untouched a few lines away.
Correctness (1)
HubResource.scala:649—isLikedHelperzips the same two lists unguarded, so #8137's own reproduction still works on/hub/isLiked(must-fix, see inline)
Design & architecture (1)
HubResource.scala:642-648—userAccessdeclares neither@Authnor@RolesAllowed, where its siblings on this resource declare both; the method returns per-entity access data and performs no ownership check of its own (must-fix)
Polish: 1 quick touch-up (see inline comment).
Verification trace
Checked the guard rather than assuming it: it sits before any DB access, tests both mismatch directions, and reuses the peer's proven pattern instead of a new one. It cannot regress the UI either — the only frontend caller builds both arrays in one lockstep loop, so it never sends a mismatched pair.
Then looked for the same contract elsewhere. Three sites zip these two lists: isLikedHelper (:104), getCounts (:523) and userAccess (:649). After this PR two of the three reject a malformed request and the third does not, which is the state the PR is trying to leave behind.
The annotation gap came out of the same read: isLiked (:338-341) and postLike (:349-350) both carry the resource's authorization annotations and userAccess (:642-648) carries none, while returning access data for whatever (entityType, entityId) pairs the caller names. It predates this PR and the diff does not touch it — raised here because this is the method the PR is editing, and the asymmetry with its own siblings is what makes it look unintended rather than a decision.
Two things were deliberately not raised, and are recorded as dismissals rather than dropped: the null and empty-list guards the peer carries. Neither is reachable over HTTP — JAX-RS injects an empty list, never null — and 400-on-empty is not the house rule here, since isLikedHelper is empty-in/empty-out on purpose with a test pinning it.
|
Addressed the list mismatch and documentation. Authorization changes are outside this PR's scope. |
|
The non-security feedback is addressed and CI is green. Ready for another review. |
carloea2
changed the title
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 2 resolved · 0 open · 3 new (3 new = 0 newly introduced · 3 late catches) — the code is done; the one blocker left is the title.
Round 1's design must-fix on userAccess is withdrawn: its premise was wrong. HubResource declares no @RolesAllowed anywhere, and five of its eight HTTP methods carry no @Auth either, so nothing is owed there. Details in the trace below.
Conventions (2)
- Retitle →
fix(amber): reject mismatched hub access queries—featclaims a new capability, but this corrects a defect, and the branch and both commits already sayfix(must-fix) - Description: name the
isLikedendpoint in "What changes were proposed" and "How was this PR tested?" — the second commit guards it too (advisory)
Polish: 1 quick touch-up (see inline comment).
Verification trace
Verified both round-1 findings against the tree rather than taking the replies. isLikedHelper now rejects at :105-108, ahead of the zip at :112 and the first query at :124, and only the length half of the peer's guard was copied — so the deliberate empty-in/empty-out contract that HubResourceSpec.scala:558-561 pins still holds. Both mismatch directions are tested.
Then checked the guard breaks nothing reachable. recordLikeAction (:196) builds both lists as singletons, so the branch is unreachable from POST /hub/like and /hub/unlike; all three frontend callers of /hub/isLiked send lockstep arrays or a single pair. All three zip sites now reject a mismatch (:105, :529, :656), and no resource outside this file declares the paired query params.
The withdrawal came out of re-reading the whole resource rather than the two methods round 1 compared against. HubResource has no @RolesAllowed at all, and five of eight HTTP methods carry no @Auth — getCount, postView, getTops, getCounts, userAccess — against isLiked, postLike and postUnlike, which are exactly the three needing a caller identity. amber never registers RoleAnnotationEnforcer. userAccess sits with the read-only group, so the asymmetry was mine to correct, not yours.
483a9b6 to
3e11b39
Compare
|
I kept this as feat after checking the history against the definition you shared. Mismatched lists have been silently truncated since the endpoint was introduced, so this adds validation rather than restoring previously working behavior. |
|
@Yicong-Huang, all review comments are addressed and CI is green. Could you take another look? |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟢 2 resolved · 0 open · 0 new (0 new = 0 newly introduced · 0 late catches) — nothing left on my side.
Round 3's must-fix retitle is withdrawn: feat is correct and my fix call was wrong. The decision table gives fix one row, "A functionality worked before and no longer does" (CONTRIBUTING.md:65), and nothing regressed here — at the base b28faee, isLikedHelper opens at :104 with the zip and userAccess at :650 with the zip, neither preceded by a guard. Two feat rows apply instead: support that never existed (:66), and a rework that intentionally changes user-facing behavior (:68), since both endpoints now answer 400 where they answered 200. CONTRIBUTING.md:71 settles it outright — "A functionality that was never implemented does not exist, so implementing it is a feat."
A correction on the evidence, too. Round 3 cited fix(amber): reject duplicate worker initialization (#8083) as "the same shape". It is not: its whole production diff is !workerExecutions.contains(workerId) to !workerExecutions.containsKey(workerId), repairing an assert that was already there and silently inert because contains on a java.util.Map tests values, not keys. A guard repaired is the fix row; a guard that never existed is not.
Verification trace
Verified all three threads you resolved against the tree rather than accepting them. isLikedHelper rejects at :105-108, ahead of the zip at :112 and the first query at :124; userAccess at :657-660, ahead of :664 and :674. Both Scaladoc blocks now state the precondition and the @throws (:98, :648), and :92 reads as the batch contract its tags describe.
Then checked the guards break nothing reachable, since a new early-throw can only regress by rejecting traffic that used to work. recordLikeAction (:196-200) passes two singletons, so POST /hub/like and /hub/unlike cannot reach the branch; search.service.ts:201-210 pushes both arrays inside one loop; the three single-pair callers send arrays of one. The empty-in/empty-out contract that HubResourceSpec.scala:557-561 pins still holds, because only the length half of the peer's guard was copied.
Coverage of the contract is complete: the three zip sites are :112, :539 and :664, and all three are now guarded (:105, :531, :657); entityId is declared as a paired query param only at :347, :527 and :655, all in this file, so there is no fourth site. One thing I checked and did not flag — both methods emit responses grouped by entity type, so response order does not track request order, but no consumer reads positionally: search.service.ts:231-238 builds keyed maps, and the two detail pages read [0] from a one-element response.
5 participants
What changes were proposed in this PR?
Reject hub user-access and isLiked requests whose entityType and entityId query lists have different lengths. Cover both mismatch directions while preserving empty and equal-length requests.
Before: unmatched query values were silently dropped.
After: mismatched lists return 400 with a clear message.
Any related issues, documentation, discussions?
Closes #8137
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex