Repository navigation
sec(auth): carve execute:ri-exchange out of admin:* and seed the granting group - #1758
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
Independent adversarial review, head
|
| symbol | references |
|---|---|
RI_EXCHANGER_GROUP_ID |
1 — its own declaration |
PURCHASER_GROUP_ID |
32, including isPurchaser() and the userActions.ts:106-110 membership affordance |
isRIExchanger |
0 — no such helper exists |
The constant was added for symmetry with Purchaser but never wired into the predicate that needed it.
Scope, stated precisely so this is not overclaimed: this is the fallback path, used while effectivePermissions is still loading. Once the server-provided set is populated the primary branch is correct, because it keys off the permission itself rather than group membership. So F2 is a loading-race window, not a permanent mis-gate. It is still wrong in both directions during that window, and the fix is what the dead constant was evidently intended for.
F3 — admin:* can still cause RI exchanges, through a verb this PR deliberately leaves alone
Every link read at its source. Issue #1644 is "admin:* executes RI exchange today". After this PR that is closed for the two interactive routes, and still open through configuration:
PUT /api/ri-exchange/config
handler_ri_exchange.go:1943 requirePermission(ctx, req, "update", "config") <-- NOT carved out
handler_ri_exchange.go:1965 existing.RIExchangeEnabled = body.AutoExchangeEnabled
handler_ri_exchange.go:1966 existing.RIExchangeMode = body.Mode
|
internal/server/handler_ri_exchange.go:45 handleRIExchangeReshape (scheduled)
| gated only on cfg.RIExchangeEnabled
internal/server/handler_ri_exchange.go:95 exchange.RunAutoExchange(...)
|
pkg/exchange/auto.go:248 if params.Config.Mode == "manual" { ...pending, needs approval... }
pkg/exchange/auto.go:257 processAutoExchange(...) <-- executes directly, money moves
update:config is not merely uncarved — this PR's own control test asserts it must stay with admin:* (TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs includes {ActionUpdate, ResourceConfig}). So a principal refused at POST /api/ri-exchange/execute can instead send {"mode":"auto","auto_exchange_enabled":true} and have the platform perform exchanges with no principal ever holding execute:ri-exchange.
RunAutoExchange is live, not theoretical: reached from providers/aws/ladder/reshape.go:96 and internal/server/handler_ri_exchange.go:95. Note providers/* is one of the trees CI's test job does not cover (#1751).
I would not call this a defect in the diff — the config path predates it, and the carve-out does what it says on the two routed endpoints. But Closes LeanerCloud/cloud-commitments-cli#1644 reads as "an admin can no longer cause an RI exchange", and that is not what the change buys. Either narrow the claim or file the config path.
F4 — the migration's idempotency subtest is vacuous
Execution-verified against a real Postgres container. RunMigrations calls m.Up(), which returns ErrNoChange on an already-migrated database (migrate.go:46-49), so the subtest's second RunMigrations never re-executes the 000096 DO block. The count == 1 assertion is satisfied by the test's own INSERT.
Proof by mutation — I stripped both idempotency devices from the migration (the DISTINCT and the AND NOT (v_uuid = ANY(...)) guard), leaving a backfill that would duplicate on every re-run:
== mutation applied: 1 file changed, 1 insertion(+), 2 deletions(-)
UPDATE users SET group_ids = ARRAY(SELECT unnest(COALESCE(group_ids,'{}') || ARRAY[v_uuid]))
WHERE v_admin_uuid = ANY(COALESCE(group_ids,'{}')) AND EXISTS (...);
--- PASS: TestMigration_SeedRIExchangerGroup/backfill_is_idempotent_and_leaves_no_duplicate_entry
The subtest passes with the idempotency removed. To actually exercise it, the test has to run the DO block twice — e.g. MigrateToVersion(95) then RunMigrations twice, or execute the migration body directly a second time.
To be clear about what this is and is not: the migration itself IS idempotent — the guards are present and correct. This is a test-quality finding, not a correctness bug.
What holds up
Priority 1 — the refusal is real, at the route, and it comes from the carve-out. The PR's three tests call h.requirePermission(...) directly, which asserts the predicate rather than the dispatch — the #1757 shape. So I drove Router.Route with the real method and path instead, using an invalid body as the discriminator (the permission check runs before parsing in both handlers):
| principal | route | result |
|---|---|---|
admin:* only |
POST /api/ri-exchange/execute |
403 permission denied: requires execute on ri-exchange |
admin:* + execute:ri-exchange |
same | 400 invalid request body — gate passed, parser reached |
admin:* only |
POST /api/ri-exchange/azure-instances/exchange |
403, same message |
admin:* + execute:ri-exchange |
same | 400, gate passed |
admin:* only |
GET /api/ri-exchange/history |
passed the gate (reaches the store) — the carve-out removed one pair, not the resource |
And the #1744 check — is the 403 the carve-out or something else? Mutation says the carve-out. Removing {ActionExecute, ResourceRIExchange} from adminCarvedOuts:
KILLED : TestRIExchangeCarveOut_PlainAdminIsRefused
SURVIVED: TestRIExchangeCarveOut_ExchangerIsAllowed (expected -- passes either way)
SURVIVED: TestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs (expected)
routed probe under the mutation: admin-only now gets 400, not 403 <-- the live bypass reproduced at the route
rest of internal/api under the mutation: ok (no collateral, and nothing else detects the bypass)
Priority 2 — the migration is sound, and I ran the integration test the author could not. Docker was available here:
go test -tags=integration -run TestMigration_SeedRIExchangerGroup ./internal/database/postgres/migrations/
--- PASS RI Exchanger seeded, system-managed, granting execute:ri-exchange
--- PASS admin-backfill: Administrators members land in RI Exchanger
--- PASS backfill is idempotent and leaves no duplicate entry (but see F4)
ok 30.920s
The no-op question, answered: the seed cannot silently skip. groups.name is NOT NULL UNIQUE (000001:162), so the name <> 'RI Exchanger' occupant guard cannot be defeated by a NULL name, and a name collision on a different id is caught by the second guard. ON CONFLICT (id) DO NOTHING is reachable only on a genuine re-run, where skipping is correct. The backfill's EXISTS (SELECT 1 FROM groups WHERE id = v_uuid) leaves no orphaned array entries if the insert was skipped. Subtests 1 and 2 do genuinely fail if the seed does nothing — subtest 2 pins to version 95 first, so the backfill is observed firing rather than inferred.
Numbering, derived in Python over both trees rather than by eye: highest on origin/main is 95, this branch adds 96 only, no same-number-different-slug collision, and no new gap (the pre-existing gaps 61/62/68/69/82/84/85 are already on main). UUID …0008 appears in no migration other than 000096.
Priority 4 — the constraint disclosure is accurate and does not overclaim. Measured against real permissionsAllow, never through MockAuthService:
seeded RI Exchanger, UNCONSTRAINED -> azure/eastus/$999999 allowed=true
custom group, aws/us-east-1/$100 -> azure/eastus/$999999 allowed=false
admin:* alone -> azure/eastus/$999999 allowed=false
Both disclosed rows reproduce exactly. The third row is mine and strengthens the PR: the carve-out also bites at the constraint layer, so admin:* is not merely gated at the handler.
A third divergence in the same mock. grantPermissionsScoped wires HasPermissionForConstraintsAPI to decide(action, resource), discarding the constraint arguments, and justifies it with "a principal holding only admin:* ... holds none of the carved-out verbs". This PR invalidates that justification — it introduces a principal that does hold a carved-out verb, and a constrained one diverges:
constrained exchanger (aws), azure request: production=false mockWouldSay=true divergent=true
After #1744's empty-constraintSets divergence, that is two known and now a third. The comment at mocks_test.go:410-412 is stale and should at minimum stop asserting an invariant that no longer holds.
Priority 3 — the two sets agree exactly, and nothing enforces it. Parsed both files in Python (no grep counts):
backend adminCarvedOuts (4): approve-any:purchases, execute:purchases, execute:ri-exchange, retry-any:purchases
frontend ADMIN_CARVED_OUTS (4): approve-any:purchases, execute:purchases, execute:ri-exchange, retry-any:purchases
only in backend: [] only in frontend: [] EXACT MATCH: True
ADMIN_CARVED_OUTS is genuinely consumed by canAccess (permissions.ts:268), so the mirror is functional rather than decorative. But no test, codegen step or CI check reads both files — the string ADMIN_CARVED_OUTS appears only in permissions.ts, and nothing on the Go side parses the TS. The sets agree today by hand. F2 is what that costs: the set was mirrored and the behaviour was not, and nothing caught it.
Gates at 92acc0ab1
| gate | result |
|---|---|
go build ./... |
exit 0 |
go vet ./... |
exit 0 |
go vet -tags=integration ./internal/database/postgres/migrations/ |
exit 0 |
go test ./... (root module) |
exit 0, 32 packages ok, 0 FAIL |
go test ./... in pkg/ (separate module, NOT covered by CI per #1751) |
exit 0 |
gocyclo -over 10 -ignore "_test\.go|node_modules" . |
0 findings |
golangci-lint run at CI-pinned v2.10.1 |
exit 0, output 0 issues. (non-empty, so not the exit-3 tool-error shape) |
npm run typecheck |
exit 0 |
npm test (frontend) |
exit 1 — 1 failed / 2800 passed (F1) |
integration TestMigration_SeedRIExchangerGroup vs real Postgres |
PASS, 3/3 subtests |
One self-inflicted false positive worth recording so nobody chases it: my first gocyclo run reported 2 findings in frontend/node_modules/flatted/golang/pkg/flatted/flatted.go — a vendored Go file inside an npm package, present only because I ran npm ci. Excluding node_modules gives 0. It is not a PR finding, though it does mean any CI job that installs npm deps before running gocyclo -over 10 . in the same workspace would fail on that file.
providers/ is not a separate module here, so it is covered by the root go test ./... run above.
Suggested disposition
F1 and F2 are blocking and share one fix: wire isRIExchanger() (the dead RI_EXCHANGER_GROUP_ID is already there for it), map carved-out verbs to their granting group rather than to isPurchaser() unconditionally, and replace the stale assertion with the new expectation plus the two directions in F2. A parity check that fails when the two sets diverge would stop the next one. F4 is a small test change. F3 is a scope/claim decision for the author.
Addendum — head moved to
|
Re-derivation of the fixture-discrimination fix, and the residue it leavesAsked to confirm independently that The fix is real. The fixture is the genuine But it is worth being precise about which assertion carries the discrimination, because it is not the one the test's comment emphasises. The mutated error is: That is still an error, so So the test's entire discriminating power rests on those two The live-cloud-call hazard is structural, not environmentalThe store stub cannot prevent it.
The shipped test is unaffected — I ran it with no AWS environment containment at all and it is refused at the gate in 1.18s without touching AWS. The hazard exists only under mutation, but mutation testing is routine on this repo, so it is worth closing. Two cheap options: set Neither is blocking. The test as written does catch the regression it was added for. Rebase and collision claims — confirmedBoth claims hold: not behind Unchanged from my earlier commentsF1 (frontend suite red) and F2 (the fallback maps |
…ting group execute:ri-exchange was absent from adminCarvedOuts, so permissionsAllow short-circuited on the admin:* wildcard and returned true unconditionally. Both routed execute endpoints, POST /api/ri-exchange/execute and POST /api/ri-exchange/azure-instances/exchange, were reachable by any admin with no explicit grant, and the provider/region/MaxPurchaseAmount dimensions were skipped along with the verb. An exchange consumes existing commitments and buys replacements with no rollback path, which is the same rationale that put execute:purchases behind separation of duties in #923. The carve-out alone would have been an outage rather than a partial fix. PR #1737's grant ceiling refuses to add a carved-out verb to any group through the API, and no migration seeded one, so the verb would have been grantable to nobody and both endpoints would 403 for every principal. execute:purchases survives its own carve-out only because 000059/000064 seed Purchaser and backfill admins into it. Migration 000096 does the same here: it seeds a system-managed RI Exchanger group at the next free namespace UUID and backfills every Administrators member, so no admin loses the capability on upgrade. The seeded grant is deliberately unconstrained, because a migration cannot know an operator's accounts, regions or spend ceiling and an over-narrow seed would refuse legitimate exchanges. Verified by execution against permissionsAllow: an unconstrained grant still short-circuits the constraint dimensions, so backfilled members bypass them exactly as admin:* does today. What the carve-out does buy is that the verb can no longer be granted through the API, that new principals need a deliberate grant instead of inheriting it from the wildcard, and that membership is revocable and auditable independently of the admin role. Constraint enforcement for the seeded population remains open on #1644. Coverage runs both directions, because a refusal-only test passes equally well against a handler that refuses everyone: a plain admin:* principal is refused, a holder of an explicit execute:ri-exchange grant is allowed, and a scope control pins that admin:* still grants the neighbouring non-carved verbs including view:ri-exchange. Mutation-verified per test, run alone: removing the pair from adminCarvedOuts fails the refusal test and leaves the other two passing, which is the correct dependency shape. The frontend ADMIN_CARVED_OUTS mirror is updated in the same change; drift there would offer an action in the UI that the backend then refuses. Closes #1644
…predicate The three existing carve-out tests call requirePermission directly. That proves adminCarvedOuts refuses the pair; it does not prove the endpoint refuses, and those are different claims. #1757 is the standing example of the gap in this same file: a test named "...MUST be required" passes while calling a predicate the real dispatch never consults. Adds coverage that exercises executeExchange itself, the handler behind POST /api/ri-exchange/execute, with a plain admin:* principal. mockStore is stubbed with no expectations so a regression reaches the store and panics rather than quietly returning 200, and the AssertNotCalled passes two matchers because SaveRIExchangeRecord hands two arguments to m.Called (a name-only form could never fail: #1595/#1740). The request body is the real ExchangeExecuteRequestBody shape. The first draft used invented field names, and the mutation run exposed it: with the carve-out removed the handler stopped at "ri_ids is required" rather than proceeding, so the no-state-change assertion was satisfied by body validation instead of by the guard under test -- the semantically-inert fixture shape from #1735. With a valid body the mutation now carries execution past the gate and into the handler proper, so the assertion discriminates. Refs #1644
…ts it back canAccess()'s loading-race fallback (effectivePermissions not yet loaded) hardcoded every carved-out verb to isPurchaser(), so it agreed with the backend for the three money-spending verbs but not for execute:ri-exchange (issue #1644): admin+Purchaser (not RI Exchanger) wrongly passed, and admin+RI-Exchanger (not Purchaser) wrongly failed. Adds isRIExchanger(), mirroring isPurchaser()'s shape, and routes both through a CARVE_OUT_FALLBACK_CHECK map keyed by carved-out verb instead of a single hardcoded predicate. Also fixes isPurchaser() itself, which iterated the full ADMIN_CARVED_OUTS set (now including execute:ri-exchange) instead of the three money-spending verbs it's actually meant to gate -- holding execute:ri-exchange alone would have wrongly satisfied the "can spend money" predicate the no-Purchaser first-run prompt keys off. Fixes the stale permissions.test.ts assertion that still expected execute:ri-exchange to pass admin:* during the fallback (pre-carve-out behaviour), and adds both-direction coverage for the fix plus regression tests pinning the two carve-outs' groups as disjoint. UX-only gate; the backend enforces on every request regardless of this fallback's answer.
… the migration The "backfill is idempotent" subtest called migrations.RunMigrations a second time expecting it to re-exercise the DO block's guards. It does not: m.Up() returns migrate.ErrNoChange once the database is already at the latest version, so the migration body never runs again and the subtest passed regardless of whether the guards worked. Proved by mutation: stripping both idempotency devices from the up migration (the WHERE NOT guard and the DISTINCT dedup on the backfill UPDATE) still left the old test green. Rewrites the subtest to read 000096_seed_ri_exchanger_group.up.sql and execute its SQL directly a second time, the same pattern 000095_purchase_history_account_id_width_test.go already uses for its re-run assertion. Re-verified by the same mutation: with the fix, stripping the guards now fails the subtest (duplicate group_ids entry), and restoring them passes it again. The migration itself was already correctly idempotent; only the test coverage was empty.
…xecute:ri-exchange handler test
TestExecuteExchange_PlainAdminIsRefused discriminated a dropped carve-out by
checking that the error message did NOT contain "execute"/"ri-exchange".
Under mutation the error actually returned is an AWS credential/STS failure
from exchange.ExecuteExchange (reached only once the carve-out stops
refusing), whose wording has nothing to do with permissions -- the test
caught the regression by accident, because that unrelated error string
happened not to contain those two tokens. If the wording of that AWS error
ever changed, the test would stop discriminating while staying green.
Replaces the substring assertions with an identity check: the carve-out
denial from requirePermission is a *clientError with code 403
(requireSessionPermission in handler.go), returned unwrapped by
executeExchange, so asserting IsClientError + code 403 fails for the actual
reason -- refused at the gate vs. failed downstream for something else.
Verified by mutation, run in a hard-sandboxed AWS environment (nulled
credentials/config files, disabled profile, IMDS disabled) so the mutated
test cannot reach live AWS: with adminCarvedOuts stripped of the
execute:ri-exchange pair, the new 403-identity assertion is the one that
fails (0.00s, no AWS contact); restoring the pair passes again.
Also corrects the docstring, which claimed the unstubbed mockStore backstop
would panic if the carve-out regressed. It cannot: executeExchange's success
path never calls SaveRIExchangeRecord at all (only the scheduled
auto-exchange path in pkg/exchange/auto.go does), so AssertNotCalled holds
unconditionally here and does not discriminate this test today. Kept as
defense-in-depth for a future change that routes this handler through the
store, documented as such.
Adds t.Setenv("AWS_EC2_METADATA_DISABLED", "true") so the credential
resolution failure this test depends on under mutation is fast and
deterministic regardless of what AWS credentials happen to be configured in
whichever environment re-runs it later, rather than depending on IMDS being
unreachable by chance. executeExchange builds its AWS clients from ambient
credentials with no injected seam (unlike internal/server's
riExchangeClients) -- that structural gap is tracked separately as #1760;
this is containment for the test, not a fix to the seam.
1caaf18 to
4eab2b5
Compare
Independent delta review — head
|
| mutation | result |
|---|---|
CARVE_OUT_FALLBACK_CHECK maps execute:ri-exchange → isPurchaser (the original bug) |
3 failed / 50 passed — both directions, incl. Expected: false, Received: true |
PURCHASER_CARVED_OUTS gains execute:ri-exchange (the second original bug) |
1 failed / 52 passed — exactly the regression test written for it |
isRIExchanger() fallback uses PURCHASER_GROUP_ID |
5 failed / 48 passed |
| baseline | 53 passed |
Priority 2 — the F5 fix
403 identity path — verified, with one correction to the mechanism. requireSessionPermission (handler.go:306-308) returns NewClientError(403, …), and executeExchange (handler_ri_exchange.go:1713-1716) returns it unwrapped. Worth noting IsClientError is errors.As (handler_router.go:83-89), so it unwraps %w anyway — the "returned unwrapped" property is not what the assertion depends on.
Ran the mutation in a fully sandboxed environment (credentials file and config file to /dev/null, nonexistent profile, key/secret/session blanked, IMDS disabled, -timeout 60s). The mutated test fails at line 167 — require.True(t, ok, "a carve-out denial must be a client error…"), i.e. IsClientError returns false — not at the assert.Equal(403, …) on line 168. Under mutation the handler reaches resolveReshapeCloudAccountID, whose STS failure is wrapped as a plain fmt.Errorf, so there is no *clientError to find. The pair of lines is the discriminator; line 167 is the one that fires. Failure was immediate (0.00s), no AWS contact.
TestRIExchangeCarveOut_PlainAdminIsRefused also fails under the same mutation (line 54); the other two correctly stay green.
The AssertNotCalled claim — verified, and it is the stronger result. Independent grep (POSIX ERE, negatives sanity-checked) finds the only non-test callers of SaveRIExchangeRecord are pkg/exchange/auto.go:390,456,629 (scheduled path) and the internal/server adapter at handler_ri_exchange.go:291,293. internal/api's executeExchange cannot reach any of them, so the assertion holds unconditionally and does not discriminate the mutation. The earlier attribution to "execution dying at STS" was wrong; this is the real reason.
One thing that improves on the docstring: AssertNotCalled is shadowed on MockConfigStore (internal/mocks/assertions.go:154-168) to read the complete call log rather than mock.Mock.Calls, and SaveRIExchangeRecord (stores.go:536-540) calls m.record before m.Called with no isExpected short-circuit. So the "defense-in-depth if a future change routes this handler through the store" rationale genuinely holds — this is not one of the vacuous-AssertNotCalled cases from #1595.
Priority 3 — mutation safety
t.Setenv("AWS_EC2_METADATA_DISABLED", "true") is present (line 145). The shipped test makes no AWS contact.
Finding 1 (nit — comment accuracy), ri_exchange_carveout_test.go:136-143. The docstring says the setenv bounds client construction "to fail fast against an unreachable credential source instead of depending on what credentials happen to be configured in whichever environment re-runs this test". That overstates it: AWS_EC2_METADATA_DISABLED closes the IMDS provider only. On a machine with a populated ~/.aws/credentials — the normal developer case, and the case on this machine — the shared-credentials provider resolves first and IMDS is never consulted. Traced the mutated path: STS succeeds, ListCloudAccounts is unstubbed and returns (nil, nil) rather than panicking (internal/mocks/stores.go:983-985), so cloudAccountID becomes unattributedAccountConstraint, requirePermissionConstraints passes (mutated admin:* grants the verb), and execution reaches exchange.ExecuteExchange — live EC2. The bogus ri-1 / off-1 ids mean AWS rejects it rather than anything being purchased, but it is real API traffic against whatever account is configured.
Evidence it is the external sandbox doing the work, not the setenv: with the credentials file neutralised the mutated test dies in 0.00s.
Suggested wording: say the setenv closes the IMDS source only, that re-running this test under mutation requires an externally sandboxed environment, and point at LeanerCloud/cloud-commitments-platform#175 for the actual fix (no injected client seam).
Also verified
- F1 —
permissions.test.ts:161assertsfalse. Full jest suite re-run from a cleannpm ci: 90 suites passed, 2815 passed / 1 skipped / 2816 total, exit 0. Matches the claim exactly. - F4 — Docker was available, so this was run rather than reasoned about. Baseline: all three subtests pass (7.90s). Mutation stripping both idempotency devices (the
NOT (…)WHERE guard and theDISTINCT): the idempotency subtest fails withexpected: 1, actual: 2at000096_seed_ri_exchanger_group_test.go:140, exactly as claimed. Stripping only one is not enough — either device alone suffices, which is worth knowing but does not weaken the test. - F3 / scope narrowing — the body does not overclaim. sec(exchange): scheduled auto-exchange bypasses the execute:ri-exchange carve-out via update:config #1765 exists, is OPEN, and carries
priority/p1 severity/high type/security triaged. The unconstrained-seed consequence is stated plainly in both the body and the migration comment rather than glossed. - Rebase — base is
27355f334; 5 commits sit on it (4 replayed +4eab2b599added after). Rather than take "gates re-run post-rebase" on trust, I re-ran them all myself at4eab2b599. - Migration number —
000096is contiguous withorigin/mainhead (000095), no gap, and no other open PR claims a0000 9xnumber.
Gate exit codes at 4eab2b599: go build ./... 0 · go vet ./... 0 · go vet -tags=integration ./... 0 · go test ./internal/... ./cmd/... 0 (zero FAIL lines) · golangci-lint v2.10.1 (the CI-pinned version) exit 0 with a genuine 0 issues. line · gocyclo -over 10 -ignore "_test\.go" 0 · jest 0 · 000096 integration 0. All six go.work modules build+vet clean (./tests/e2e vet reports "matched no packages", pre-existing and unrelated).
Remaining findings
Finding 2 (nit — coverage asymmetry). The test file header (lines 17-20) names both routed endpoints, but only executeExchange got a real-principal handler test. executeAzureExchange (handler_ri_exchange.go:1172) carries the identical requirePermission(ctx, req, "execute", "ri-exchange") gate, and its nearest existing test, TestExecuteAzureExchange_MissingExecutePermission (handler_ri_exchange_azure_test.go:727-744), stubs HasPermissionAPI(…).Return(false, nil) — it proves the handler honours a denial, not that a plain admin:* principal is denied, which is the #1644 claim.
Verified rather than assumed: with the carve-out removed, every *AzureExchange* test still passes (ok … 1.583s, identical to baseline). The Azure money path has no regression guard for this change. One test mirroring TestExecuteExchange_PlainAdminIsRefused but driving executeAzureExchange with grantPermissions([]auth.Permission{{ActionAdmin, ResourceAll}}) would close it.
Finding 3 (nit — down migration half-rollback), 000096_…down.sql:11-17. The UPDATE strips v_uuid from every user unconditionally, but the DELETE is guarded on name = 'RI Exchanger' ("a group an operator renamed onto this id is not ours to drop"). If that guard ever fires, the rollback has already detached every member from a group it then refuses to delete — a half-rollback that is worse than either doing all of it or none of it. Mirroring the guard on the UPDATE makes the two consistent. Unlikely path (the group is system_managed, so only direct SQL could rename it), but down migrations are exactly where unexercised asymmetries hide.
Finding 4 (optional simplification), permissions.ts:165-182. ADMIN_CARVED_OUTS is fully derivable from the new map: new Set(CARVE_OUT_FALLBACK_CHECK.keys()). The comment at lines 169-171 states the invariant in prose — "every entry here must also appear in [CARVE_OUT_FALLBACK_CHECK]" — that the code could simply enforce, and nothing currently does. The two are aligned today (verified above), so this is not a live bug; it removes one hand-maintained list, which is the same class of drift this PR just fixed one instance of. isPurchaser/isRIExchanger are hoisted function declarations, so the map can be declared first.
Informational — no RI Exchanger first-run prompt. #923 shipped the Purchaser carve-out and the "add yourself to Purchaser" prompt (userActions.ts:105-125). #1644 ships the carve-out without an analog. The backfill covers every existing admin, so this is invisible on upgrade; but an admin created after the migration is not auto-added to RI Exchanger and will hit a bare 403 with no guidance, which is strictly worse than the Purchaser case it is modelled on. Follow-up, not a blocker.
Verdict
Mergeable. All five fixes do what they claim, and the two frontend bugs — including the isPurchaser() one found while in there — are correctly diagnosed and correctly fixed, with tests that genuinely fail when the logic is reverted. The four items above are nits: two comment/coverage accuracy, one unexercised rollback path, one optional simplification. None needs to land before merge.
One process note, not a finding: the CodeRabbit check currently reads pass — Review rate limited, which is the false-clean shape — a throttled pass reports success without having reviewed this head. Worth a @coderabbitai full review (not the incremental form) before merge if CR sign-off is wanted. The branch is also 1 commit behind main and mergeStateStatus is BLOCKED with Security Scanning still pending.
|
|
|
Merging. Closes the direct-handler half of #1644 — Merging without a CodeRabbit verdict, deliberately. CR has produced no non-empty review at any head here; its check currently reads The carve-out alone would have bricked the feature. Once #1737 lands, the verb would be ungranted by What the carve-out buys, stated without inflation: the verb cannot be granted through the group API; new principals need a deliberate grant rather than inheriting it from Four defects were found in the fix by review, plus two the author found while fixing them:
Mutation safety: under mutation this test reaches live AWS, because Scope stated honestly rather than implied. Two items deferred to a follow-up rather than blocking a live-bypass fix: Gates at |
…1772) TestGrantCeiling_ConstraintContainment used execute:ri-exchange as its fixture verb to exercise general grant-ceiling constraint containment (narrower-allowed, cap-drop-refused, unheld-provider-refused). PR #1758 then added {execute, ri-exchange} to adminCarvedOuts. checkGrantCeiling (internal/auth/group_ceiling.go) checks adminCarvedOuts before the containment logic in grantCeilingAllows, so once the fixture's verb became carved out, the test's target group (no existing permissions) made every subtest refuse with ErrPermissionNotGrantable before the containment logic it exists to exercise ever ran. Both #1737 and #1758 were correct in isolation; the conflict is emergent from their merge order. Swap the fixture verb for view:plans, already used elsewhere in this file, and hoist it to named constants with an assertion that it is not in adminCarvedOuts -- so the next carve-out addition that collides fails loudly at the fixture instead of three subtests dying somewhere that looks unrelated. checkGrantCeiling's carve-out-before-containment ordering is untouched; it is correct and deliberate. Refs #1737, #1758
execute:ri-exchange is carved out of admin:* (#1644, PR #1758) so a compromised admin account alone cannot drain commitments. update:config is deliberately NOT carved out -- but a single update:config write could set ri_exchange_mode "auto" plus ri_exchange_enabled, after which the scheduled TaskRIExchangeReshape -> RunAutoExchange -> processAutoExchange executed exchanges against the provider with no execute:ri-exchange check anywhere on that path. The carve-out's guarantee did not hold via that route. Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold execute:ri-exchange. The verb is reused, not reinvented. Both write paths are gated through one helper. PUT /api/config unmarshals the body onto the whole GlobalConfig, so it reaches ri_exchange_mode and ri_exchange_enabled exactly as effectively as the dedicated PUT /api/ri-exchange/config; a gate on the dedicated endpoint alone would have looked complete and would not have been. Both already resolved a session, one merely discarded it. "Armed" is defined as enabled AND mode != "manual", mirroring the consumer rather than the field's nominal domain. processRecommendation routes to processManualExchange only on the literal "manual" and sends every other value to processAutoExchange, and GlobalConfig.Validate never constrains RIExchangeMode -- so via PUT /api/config a mode of "Auto", "automatic" or "" arms unattended execution just as effectively as "auto". A gate written as mode == "auto" would have been open on precisely the route that matters. Only the escalation is gated. An update:config-only operator keeps view:config, mode "manual", the utilization and lookback tunables, and ri_exchange_enabled while mode stays manual -- that only has the scheduler raise Pending approvals, which move no money. Raising a spend cap while already armed is also gated, since the caps are plain fields on the same struct and the same actor would otherwise set both the switch and its bound. Lowering a cap, disarming, and an idempotent re-save of an already-armed config are de-escalations and stay available. The check runs inside the UpdateGlobalConfigAtomic closure so it compares against the same advisory-locked row the write lands on; checking beforehand would race a concurrent writer between the check and the write. A refusal is returned as its own 403 rather than wrapped in "failed to save config", which would have blamed the store for an authorization decision. Refs #1765
execute:ri-exchange is carved out of admin:* (#1644, PR #1758) so a compromised admin account alone cannot drain commitments. update:config is deliberately NOT carved out -- but a single update:config write could set ri_exchange_mode "auto" plus ri_exchange_enabled, after which the scheduled TaskRIExchangeReshape -> RunAutoExchange -> processAutoExchange executed exchanges against the provider with no execute:ri-exchange check anywhere on that path. The carve-out's guarantee did not hold via that route. Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold execute:ri-exchange. The verb is reused, not reinvented. Authorization goes through requirePermission with the request the caller was authenticated from, so the verb is evaluated against THE CREDENTIAL PRESENTED rather than the owning user's group permissions. For a user API key that routes to authorizeAPIKey and checks the KEY's effective permissions. Resolving it against the session's user id instead let a CI key that is correctly denied execute:ri-exchange arm the scheduler to execute every exchange -- the same permission-inheritance bug requirePermissionConstraints already guards against one function above, reintroduced one function later. It also gives the stateless admin API key the same answer here as on the direct execute handlers, rather than failing a group lookup on a sentinel user id and surfacing a 500 that blamed the store for an authorization outcome. Both write paths are gated through one helper. PUT /api/config unmarshals the body onto the whole GlobalConfig, so it reaches ri_exchange_mode and ri_exchange_enabled exactly as effectively as the dedicated PUT /api/ri-exchange/config. The two differ in reach, though: /api/config is AuthAdmin and unreachable to a user API key, so the dedicated route carries the credential axis alone -- covering both handlers was necessary and not sufficient. "Armed" is defined as enabled AND mode != "manual", mirroring the consumer rather than the field's nominal domain. processRecommendation routes to processManualExchange only on the literal "manual" and sends every other value to processAutoExchange, and GlobalConfig.Validate never constrains RIExchangeMode -- so via PUT /api/config a mode of "Auto", "automatic" or "" arms unattended execution just as effectively as "auto". A gate written as mode == "auto" would have been open on precisely the route that matters. The compared state is a four-field value snapshot rather than a copy of the whole struct: GlobalConfig carries slices and maps that a shallow copy would alias, so narrowing to the fields the decision reads removes the question. Only the escalation is gated. An update:config-only operator keeps view:config, mode "manual", the utilization and lookback tunables, and ri_exchange_enabled while mode stays manual -- that only has the scheduler raise Pending approvals, which move no money. Raising a spend cap while already armed is also gated, since the caps are plain fields on the same struct and the same actor would otherwise set both the switch and its bound. Lowering a cap, disarming, and an idempotent re-save of an already-armed config are de-escalations and stay available. The check runs inside the UpdateGlobalConfigAtomic closure so it compares against the same advisory-locked row the write lands on; checking beforehand would race a concurrent writer between the check and the write. Tests drive Router.Route rather than the handlers: the credential axis is invisible below the router, which is why a handler-level suite passed while the key bypass was live. Refs #1765
execute:ri-exchange is carved out of admin:* (#1644, PR #1758) so a compromised admin account alone cannot drain commitments. update:config is deliberately NOT carved out -- but a single update:config write could set ri_exchange_mode "auto" plus ri_exchange_enabled, after which the scheduled TaskRIExchangeReshape -> RunAutoExchange -> processAutoExchange executed exchanges against the provider with no execute:ri-exchange check anywhere on that path. The carve-out's guarantee did not hold via that route. Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold execute:ri-exchange. The verb is reused, not reinvented. Authorization goes through requirePermission with the request the caller was authenticated from, so the verb is evaluated against THE CREDENTIAL PRESENTED rather than the owning user's group permissions. For a user API key that routes to authorizeAPIKey and checks the KEY's effective permissions. Resolving it against the session's user id instead let a CI key that is correctly denied execute:ri-exchange arm the scheduler to execute every exchange -- the same permission-inheritance bug requirePermissionConstraints already guards against one function above, reintroduced one function later. It also gives the stateless admin API key the same answer here as on the direct execute handlers, rather than failing a group lookup on a sentinel user id and surfacing a 500 that blamed the store for an authorization outcome. Both write paths are gated through one helper. PUT /api/config unmarshals the body onto the whole GlobalConfig, so it reaches ri_exchange_mode and ri_exchange_enabled exactly as effectively as the dedicated PUT /api/ri-exchange/config. The two differ in reach, though: /api/config is AuthAdmin and unreachable to a user API key, so the dedicated route carries the credential axis alone -- covering both handlers was necessary and not sufficient. "Armed" is defined as enabled AND mode != "manual", mirroring the consumer rather than the field's nominal domain. processRecommendation routes to processManualExchange only on the literal "manual" and sends every other value to processAutoExchange, and GlobalConfig.Validate never constrains RIExchangeMode -- so via PUT /api/config a mode of "Auto", "automatic" or "" arms unattended execution just as effectively as "auto". A gate written as mode == "auto" would have been open on precisely the route that matters. The compared state is a four-field value snapshot rather than a copy of the whole struct: GlobalConfig carries slices and maps that a shallow copy would alias, so narrowing to the fields the decision reads removes the question. Only the escalation is gated. An update:config-only operator keeps view:config, mode "manual", the utilization and lookback tunables, and ri_exchange_enabled while mode stays manual -- that only has the scheduler raise Pending approvals, which move no money. Raising a spend cap while already armed is also gated, since the caps are plain fields on the same struct and the same actor would otherwise set both the switch and its bound. Lowering a cap, disarming, and an idempotent re-save of an already-armed config are de-escalations and stay available. The check runs inside the UpdateGlobalConfigAtomic closure so it compares against the same advisory-locked row the write lands on; checking beforehand would race a concurrent writer between the check and the write. Tests drive Router.Route rather than the handlers: the credential axis is invisible below the router, which is why a handler-level suite passed while the key bypass was live. Refs #1765
…#1773) execute:ri-exchange is carved out of admin:* (#1644, PR #1758) so a compromised admin account alone cannot drain commitments. update:config is deliberately NOT carved out -- but a single update:config write could set ri_exchange_mode "auto" plus ri_exchange_enabled, after which the scheduled TaskRIExchangeReshape -> RunAutoExchange -> processAutoExchange executed exchanges against the provider with no execute:ri-exchange check anywhere on that path. The carve-out's guarantee did not hold via that route. Arming auto mode is functionally equivalent to pre-authorizing every future exchange, so the transition into that state now requires the caller to also hold execute:ri-exchange. The verb is reused, not reinvented. Authorization goes through requirePermission with the request the caller was authenticated from, so the verb is evaluated against THE CREDENTIAL PRESENTED rather than the owning user's group permissions. For a user API key that routes to authorizeAPIKey and checks the KEY's effective permissions. Resolving it against the session's user id instead let a CI key that is correctly denied execute:ri-exchange arm the scheduler to execute every exchange -- the same permission-inheritance bug requirePermissionConstraints already guards against one function above, reintroduced one function later. It also gives the stateless admin API key the same answer here as on the direct execute handlers, rather than failing a group lookup on a sentinel user id and surfacing a 500 that blamed the store for an authorization outcome. Both write paths are gated through one helper. PUT /api/config unmarshals the body onto the whole GlobalConfig, so it reaches ri_exchange_mode and ri_exchange_enabled exactly as effectively as the dedicated PUT /api/ri-exchange/config. The two differ in reach, though: /api/config is AuthAdmin and unreachable to a user API key, so the dedicated route carries the credential axis alone -- covering both handlers was necessary and not sufficient. "Armed" is defined as enabled AND mode != "manual", mirroring the consumer rather than the field's nominal domain. processRecommendation routes to processManualExchange only on the literal "manual" and sends every other value to processAutoExchange, and GlobalConfig.Validate never constrains RIExchangeMode -- so via PUT /api/config a mode of "Auto", "automatic" or "" arms unattended execution just as effectively as "auto". A gate written as mode == "auto" would have been open on precisely the route that matters. The compared state is a four-field value snapshot rather than a copy of the whole struct: GlobalConfig carries slices and maps that a shallow copy would alias, so narrowing to the fields the decision reads removes the question. Only the escalation is gated. An update:config-only operator keeps view:config, mode "manual", the utilization and lookback tunables, and ri_exchange_enabled while mode stays manual -- that only has the scheduler raise Pending approvals, which move no money. Raising a spend cap while already armed is also gated, since the caps are plain fields on the same struct and the same actor would otherwise set both the switch and its bound. Lowering a cap, disarming, and an idempotent re-save of an already-armed config are de-escalations and stay available. The check runs inside the UpdateGlobalConfigAtomic closure so it compares against the same advisory-locked row the write lands on; checking beforehand would race a concurrent writer between the check and the write. Tests drive Router.Route rather than the handlers: the credential axis is invisible below the router, which is why a handler-level suite passed while the key bypass was live. Refs #1765
TestApproveRIExchangeViaSession_RequiresCSRF only mocked ValidateCSRFToken; under mutation (CSRF guard removed) it failed via a panic on the first unmocked call (GetRIExchangeRecord) rather than through its own AssertNotCalled assertions, which never ran. A panic on an unrelated unmocked call proves the record wasn't fetched, not that the state transition (the actual money-moving call) was prevented -- the same class of fiction-vs-mechanism gap flagged on PR #1758. Adds the full happy-path fixture (.Maybe(), never meant to be consumed under the fix) so a reintroduced bug can run all the way to TransitionRIExchangeStatus instead of crashing early, and reorders the state-changing AssertNotCalled checks ahead of the error-shape assertions so they are the primary, always-run check. Re-verified by mutation (inverse-edit revert of the CSRF guard, not git checkout): fails cleanly with "TransitionRIExchangeStatus was called 1 time(s)... expected none" and the recorded call showing the pending->processing transition -- the assertion the test needs to protect is now the one that actually fires.
…#1796) * sec(api): enforce CSRF on RI-exchange session-authed approve approveRIExchangeViaSession had no CSRF check at all. The endpoint is AuthPublic, so the generic middleware CSRF gate never runs for it (validateSecurityContext returns before requiresCSRFValidation is reached), same as purchases approve/cancel -- but unlike purchases, nothing enforced CSRF inside the handler either. A comment at middleware.go:288-289 claimed parity with the purchases path; it didn't hold. Confirmed live by execution with a purchases control in the same probe run (not committed): same shape, valid session, zero CSRF headers -- purchases correctly refused ("CSRF validation failed"), RI-exchange transitioned the record from pending to processing with no CSRF token at all. Filed as #1757. Adds h.validateCSRF(ctx, req) at the top of approveRIExchangeViaSession, mirroring approvePurchaseViaSession / cancelPurchaseViaSession exactly (including the explanatory comment about AuthPublic routes). Adds TestApproveRIExchangeViaSession_RequiresCSRF and its PassesCSRF companion, both driving the real handler.approveRIExchange entry point (not requiresCSRFValidation in isolation). Fixes four existing tests (TestApproveRIExchange_SessionAdmin, both subtests of TestApproveRIExchange_SessionApproveOwn, TestApproveRIExchange_ SessionActorStamped) that never supplied a CSRF token and would now fail against the fix. Corrects the middleware.go:288-289 comment, which asserted parity that didn't exist and is why this survived -- the next reader now sees where CSRF is actually enforced for this path. Adds a scope-limit comment to TestRequiresCSRFValidation_ TokenBasedPathsWithSession (kept, not deleted: it's the only test pinning requiresCSRFValidation's own per-path table, which would matter again if any of these routes stopped being AuthPublic) -- it calls requiresCSRFValidation directly, bypassing the isPublicEndpoint gate that keeps the real dispatch from ever consulting it for these paths, so a passing assertion there was never proof of end-to-end enforcement, and was misread as exactly that here. Systemic check: enumerated all 26 AuthPublic routes in router.go and read every handler with a session-authed branch. approvePurchase / cancelPurchase (protected via validateCSRF), revokeViaEmailToken (protected on its only session-authed mutating path), rejectRIExchange (no session-authed path at all -- unconditionally token-gated), unsubscribeHandler (no session concept -- pure signed-token, RFC 8058), submitRegistrationHandler (no session concept -- public submission, separate AuthAdmin approval step). approveRIExchange was the only gap. Fixes #1757. * test(api): make the CSRF regression test assert on the money-moving call TestApproveRIExchangeViaSession_RequiresCSRF only mocked ValidateCSRFToken; under mutation (CSRF guard removed) it failed via a panic on the first unmocked call (GetRIExchangeRecord) rather than through its own AssertNotCalled assertions, which never ran. A panic on an unrelated unmocked call proves the record wasn't fetched, not that the state transition (the actual money-moving call) was prevented -- the same class of fiction-vs-mechanism gap flagged on PR #1758. Adds the full happy-path fixture (.Maybe(), never meant to be consumed under the fix) so a reintroduced bug can run all the way to TransitionRIExchangeStatus instead of crashing early, and reorders the state-changing AssertNotCalled checks ahead of the error-shape assertions so they are the primary, always-run check. Re-verified by mutation (inverse-edit revert of the CSRF guard, not git checkout): fails cleanly with "TransitionRIExchangeStatus was called 1 time(s)... expected none" and the recorded call showing the pending->processing transition -- the assertion the test needs to protect is now the one that actually fires. * sec(api): stop a CSRF rejection falling through to the token approve path Review finding F1. approveRIExchange falls back to the token flow when the session path returns 403, so email-link holders can still approve. That test is isPermissionDenied, which compares the status code alone, so a CSRF rejection was indistinguishable from a legitimate approve-own denial and the request proceeded on the token. Measured before fixing, session with no CSRF headers and a valid ?token=: err = <nil> TransitionRIExchangeStatus called = true ExecuteExchange called = true money moved transition actor was NIL = true audit attribution lost StampRIExchangeApprovedBy called = false Not a reopened CSRF hole: the caller still needs the unguessable ApprovalToken, which is constant-time compared and is sufficient authorization by design. But a legitimate session-authed approve carrying a token degraded to an unattributed system approval on a money path, and the code comment claimed parity with approvePurchaseViaSession, which returns its CSRF 403 directly rather than swallowing it. Shipping a #1757 fix whose comment overstates what the code does is the failure mode #1757 was about. CSRF rejection is now the errCSRFRejected sentinel. Note precisely what protects it: isPermissionDenied(errCSRFRejected) is still true, because the sentinel is a 403. What keeps it out of the fallback is the short-circuit ordering of || at the dispatch, with the errors.Is term first. "Reached first" rather than "unreachable", and the two have different failure modes: reordering that expression reopens F1. The new test drives that exact path, so a reorder fails it. Chose the sentinel over widening isPermissionDenied because that predicate has three other call sites in handler_purchases.go, and narrowing its meaning globally is a larger blast radius than this finding warrants. Verified the record-level approve-own denial still falls through, which is the case the fallback exists for. The new test drives the dispatch with a valid token, which the existing empty-token test cannot reach, and asserts the money-moving call is never made. Mutation-verified: replacing the sentinel with a plain NewClientError(403, ...) makes it fail on both the TransitionRIExchangeStatus assertion and "an error is expected but got nil", by assertion rather than by panic. Restored afterwards and confirmed byte-identical. Refs #1757
MockAuthService discarded the constraint arguments entirely grantPermissionsScoped wired both HasPermissionAPI and HasPermissionForConstraintsAPI to the same decide(action, resource) closure, which never read the constraint arguments. The constraint dimension was therefore not modelled at all, so a handler test asserting a constraint refusal was measuring the mock. Measured before changing anything, real permissionsAllow against the mock's own closure for the same principal, ten cases: six diverge, and they cover all five constraint dimensions rather than the one the issue cites. Providers, Regions, AccountIDs, Services and MaxPurchaseAmount all diverge; the money case is a cap of 1000 failing to stop a request for 5000. Divergence is strictly one-directional: the mock never denied where production allowed, so it was uniformly over-permissive, which is the worse direction for an authorization double. The fix is delegation rather than a second implementation. PermissionsAllowForConstraintSets is the decision half of HasPermissionForConstraintsAPI with the permission fetch removed, exported so a caller holding an already-resolved permission set answers from this code instead of a copy of it. permissionsAllow becomes a package function rather than a Service method because the decision reads no Service state. The mock now calls it. A mock that re-implements the rule diverges again the next time the rule changes, which is how this defect arose. Registering a constraint-blind func(action, resource) bool on that method now panics, naming this issue, so the divergence cannot be reintroduced silently by a future inline registration. Two corrections to the issue, both recorded on it. Its supporting argument does not hold: #1758 adding execute:ri-exchange to adminCarvedOuts makes the quoted justification more true, not false, because AuthContext HasPermission and permissionsAllow both short-circuit on the wildcard before any constraint comparison and fall through identically on a carve-out hit, measured as real=false mock=false. What actually made the comment wrong is that its premise describes grantAdmin while it sits on grantPermissionsScoped, which grantPermissions also drives with an arbitrary permission set, so it was false from the day it was written. Migration 000096_seed_ri_exchanger_group.up.sql:48-56 documents the seeded grant as deliberately unconstrained. The issue's "every handler test that appears to exercise it is measuring the mock" overstates the blast radius. The divergence was latent: the handler tests that do exercise constraints use explicit mock.On with MatchedBy, which wins over the generic .Maybe() registration. Prior art at grantadmin_carveout_test.go:176 records someone reaching the same conclusion during #1596 review. Verified: new coverage fails pre-fix by assertion rather than panic, and asserts both directions, since a refusal-only test passes against a mock that refuses everyone. Empty constraint sets fail closed at both entry points, asserted by leaving the store mock with no registered expectation so a call that reached the store would fail loudly. Full repository suite clean, golangci-lint at the CI-pinned v2.10.1 reports 0 issues with a non-empty body and exit 0. Deferred: #1858, roughly twenty handler tests including newAzureRegionScopedHandler at handler_ri_exchange_azure_test.go:1910 hand-write a permits closure re-implementing the all-match region rule, so they assert the handler submits the constraint set they expect rather than that the real matcher denies it. A distinct flavour of measuring the mock.
1 participant
Closes #1644
Scope of the close, stated precisely (review finding): this closes the direct
execute:ri-exchangepath -- the two routed handlers (executeExchange,executeAzureExchange) now refuse a plainadmin:*principal, which is the literal bypass #1644 described. It does not close every way an admin can cause an RI exchange to execute. The scheduled auto-exchange path (TaskRIExchangeReshape->RunAutoExchange->processAutoExchange,pkg/exchange/auto.go:477) executes exchanges automatically wheneverPUT /api/ri-exchange/confighas setmode: "auto"andauto_exchange_enabled: true-- and that config write is gated only onupdate:config, whichadmin:*keeps (seeTestRIExchangeCarveOut_AdminKeepsNonCarvedVerbs). So anadmin:*principal who is NOT in RI Exchanger can still cause exchanges to run, just via config instead of the direct handler. Tracked separately in #1765; out of scope here.Established before fixing
The issue reads as a paperwork gap, so I checked whether it was one. It is not.
executeExchange(POST /api/ri-exchange/execute) andexecuteAzureExchange(POST /api/ri-exchange/azure-instances/exchange), both callingrequirePermission(ctx, req, "execute", "ri-exchange").admin:*principal passes today. Probed withgrantAdmin()(the real-principal helper from fix(test): make grantAdmin model the principal instead of stubbing the authorization decision #1744):permissionsAllow:admin:*is allowedazure/eastus/$999999, so the provider, region andMaxPurchaseAmountdimensions are bypassed with the verb.Worth recording, because it nearly inverted the verdict:
auth.ResourceRIExchangeappears in non-test code only in its own declaration — the handlers demand the bare literal"ri-exchange". A constant-only grep yields "zero non-test references", which is the conclusion that would have re-triaged this as an enforcement gap.Why the one-line carve-out would have been an outage
Adding the verb to
adminCarvedOutsalone leaves it grantable to nobody:admin:*no longer grants it (this change), andgroup_ceiling.go:85-92, whose own comment cites sec(auth): execute:ri-exchange missing from adminCarvedOuts; admin:* bypasses every exchange guardrail #1644), andri_exchangeappears in migrations only as table names.Both endpoints would then 403 for every principal.
execute:purchasessurvives its carve-out only because 000059/000064 seed Purchaser and backfill admins into it. Migration 000096 does the same here.What landed
internal/auth/types.go{ActionExecute, ResourceRIExchange}added toadminCarvedOuts;DefaultRIExchangerGroupID000096_seed_ri_exchanger_group.up.sql…000008, backfills every Administrators member…down.sqlgroup_idshas no FK, so the reverse order leaves dangling ids)frontend/src/permissions.tsADMIN_CARVED_OUTSmirror +RI_EXCHANGER_GROUP_IDThe migration guards the UUID and the name with
RAISE EXCEPTIONrather thanON CONFLICT DO NOTHING, because a bareDO NOTHINGonto an occupied id is precisely how 000059 silently no-op'd and required 000064 to repair it (#942).What this actually buys — stated plainly
The seeded grant is unconstrained, and the backfill gives it to every existing admin. So for the current admin population this changes little operationally, and I would rather say that than imply otherwise:
An unconstrained grant still short-circuits the constraint dimensions, exactly as
admin:*does. The constraint machinery works, but only when the grant carries constraints — so the constraint bypass survives this fix for backfilled members, and that remains open on #1644.The seed is unconstrained deliberately: a migration cannot know an operator's accounts, regions or spend ceiling, and an over-narrow seed would refuse legitimate exchanges on upgrade. Operators who want those dimensions enforced should grant
execute:ri-exchangethrough a custom group with constraints rather than relying on the seed.What the carve-out does buy:
The backfill is a deliberate trade: without it every existing admin loses RI-exchange execute on deploy, discovered in production, and the two carve-outs in the same set would behave differently — which is what makes a security model unreviewable.
Verification
Coverage runs both directions, since a refusal-only test passes equally well against a handler that refuses everyone. Mutation-verified per test, run alone (removing the pair from
adminCarvedOuts):TestRIExchangeCarveOut_PlainAdminIsRefusedTestRIExchangeCarveOut_ExchangerIsAllowedTestRIExchangeCarveOut_AdminKeepsNonCarvedVerbsview:ri-exchangeGates:
go build ./...,go vet ./...(and-tags=integration),gocyclo -over 10all clean;./internal/... ./cmd/...green.Not run locally: the 000096 integration test needs a live Postgres container (
//go:build integration). It compiles undergo vet -tags=integrationand will be exercised by the Integration Tests job.Scope
Untouched:
handler_ri_exchange.go(fix-1732 is working there on LeanerCloud/cloud-commitments-platform#173) and theapproveRIExchangedispatch. This change is confined to the carve-out set, the seed migration, and the frontend mirror. The config-driven scheduled auto-exchange path is a separate, not-yet-closed bypass -- see the scope note at the top and #1765.Review round (post-open findings)
An independent review of this PR found four issues, all addressed in follow-up commits on this branch:
permissions.test.tsstill asserted the pre-carve-outexecute:ri-exchangebehaviour. Fixed to assert the carved-out (refused-by-default) behaviour.canAccess()'s loading-race fallback routed every carved-out verb throughisPurchaser(), so an admin+Purchaser (not RI Exchanger) user wrongly passedexecute:ri-exchange, and an admin+RI-Exchanger (not Purchaser) user wrongly failed it. Fixed with a per-verb fallback map (CARVE_OUT_FALLBACK_CHECK) and a newisRIExchanger()helper mirroringisPurchaser()'s shape. Also fixedisPurchaser()itself, which was iterating the full carved-out set (includingexecute:ri-exchange) instead of just the three money-spending verbs -- a user holding onlyexecute:ri-exchangewould have wrongly satisfied the "can spend money" predicate the no-Purchaser banner keys off. This is a UI-correctness fix (wrong actions offered/hidden during the loading race), not a security bypass -- the backend enforces on every request regardless of what the frontend fallback shows.Closes LeanerCloud/cloud-commitments-cli#1644overreach: narrowed above and in a new tracked issue, sec(exchange): scheduled auto-exchange bypasses the execute:ri-exchange carve-out via update:config #1765.RunMigrationstwice, butm.Up()returnsErrNoChangeonce the DB is at the latest version, so the migration body never re-ran and the subtest passed regardless of whether the guards worked. Rewritten to read the.up.sqlfile and execute it directly a second time, and verified by mutation: stripping theNOT (...)WHERE guard and theDISTINCTfrom the backfillUPDATEmakes the rewritten subtest fail (duplicate group membership), and restoring them makes it pass again.