Repository navigation
sec(api): apply account-scope gate to approve/cancel/retry/revoke purchase endpoints - #381
Conversation
|
Warning Review limit reached
This review includes 11 billable files and costs up to $2.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 45 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (11)
Comment |
cristim
added
urgency/this-sprint
|
@coderabbitai review |
|
…chase endpoints approvePurchase, cancelPurchase, retryPurchase, the session-authed branch of revokeViaEmailToken, and revokeScheduledExecution relied only on the approve/cancel/retry/revoke -any/-own RBAC matrix and never consulted the session's allowed_accounts, unlike the sibling pause/resume/run/delete-planned endpoints which all call requireExecutionAccess. A session scoped to one account but holding an -any permission could approve, cancel, retry, or revoke a purchase execution belonging to a different account, including firing the real SDK purchase. Adds a requireExecutionAccess call to each session-authed branch, after the existing RBAC check so a caller who fails RBAC still falls through to the per-account contact_email token path unaffected. Folds the new check plus the existing RBAC/4-eyes checks into per-endpoint authorize*Session helpers (authorizeApproveSession, authorizeCancelSession, authorizeRetrySession) to keep the callers under the project's gocyclo budget. Also covers audit finding A16-009: authorizeSessionRevokeExecution's hasAny allow, the !hasOwn 403, and both permission-lookup error paths had no test coverage; adds a table-driven test for all four branches. Regression tests: TestHandler_approvePurchase_OutOfScope and TestHandler_retryPurchase_OutOfScope drive the real HTTP handler with a session scoped to allowed_accounts=[Production] holding approve-any/ retry-any against an execution scoped to a different account, and assert the same 404 requireExecutionAccess already returns for pause/resume/ delete-planned. Verified both fail pre-fix (one panics reaching the nil purchase manager, the other hits an unmocked downstream call) via `git stash` on the production files only, and pass post-fix. Docker is unavailable on this host; testcontainers-based DB tests were not run locally. All internal/api unit tests (2223) pass under -race -short, go vet is clean, go mod tidy -diff is empty, and golangci-lint v2.10.1 (pinned in ci.yml) run with the exact CI invocation reports 0 issues. Closes #92
…first
requireExecutionAccess only checked that the execution's plan accounts
intersect the session's allowed_accounts. With an operator scoped to A, a
plan spanning {A, B} and an execution for B, the check passed, so approve,
cancel, retry, revoke and run-now all let the operator act on B's money.
It now also requires the session to be allowed the execution's own
CloudAccountID when one is set (via requireAccountAccess, which resolves
the account name for name-based allow-lists). A nil CloudAccountID keeps
the plan-only check, and an empty PlanID still fails closed.
The scope gate now runs before RBAC and Constraints on every session path:
the approve, cancel and revoke dispatchers, approvePurchaseViaSession,
cancelPurchaseViaSession, tryRevokeViaSession and revokeScheduledExecution.
Previously an in-RBAC but out-of-scope caller could get a Constraints 403
before the scope 404, confirming the execution exists; after #378 the
approve dispatcher also evaluated Constraints before the gate. A scope miss
in a dispatcher falls through to the email-token branch like an RBAC 403,
so the contact_email token path stays unscoped as issue #92 intends;
without a token the session branch re-runs the gate and returns 404 (401
from the revoke dispatcher, as for an RBAC denial).
Tests: TestExecutionEndpoints_OutOfScopeExecutionRefused drives approve,
cancel, retry, both revoke paths and run-now with a Production-scoped
caller holding every money verb, against a plan-out-of-scope execution
and a cross-account execution, asserting refusal and no mutation.
TestRequireExecutionAccess_InScopeCloudAccountAllowed is the positive
control. Tests reaching the dispatchers with an unscoped session now mock
GetAllowedAccountsAPI, since the gate runs first.
163a441 to
7643708
Compare
|
Rebased onto main (was 110 behind) and force-pushed, new head 7643708. Kept every guard from #375, #378, #392, #405, #406 and #409. Also added: the scope gate now runs before RBAC/Constraints on every session path (which fixes the #378 interaction), |
Replace the ad-hoc plan-intersection and CloudAccountID checks in requireExecutionAccess with one helper, executionAccounts, which returns the full set of cloud accounts an execution can buy in: its CloudAccountID when set; otherwise every account of its plan (a plan-level parent fans out to all of them); otherwise, for an ad-hoc purchase with no plan, every recommendation's account, refusing any recommendation without one. A scoped session must hold every account in that set, and an empty set is a deny. - A session scoped to A can no longer approve, retry or run-now a plan parent that buys in B. - Ad-hoc executions no longer call GetPlanAccounts with an empty plan ID, which 500ed for scoped sessions and token holders. - An out-of-scope session falls through to the email-token branch only when a token is present; otherwise it gets the same 404 as a missing row instead of status-revealing 409/410/401 answers. - Purchase details and the planned/upcoming lists use the same rule. Refs #92
revokeViaEmailToken returned the 409 status guard before any session or token check, so an out-of-scope session or an unauthenticated caller learned that a pending/notified/failed execution exists and its status. The status check now runs after the session scope/RBAC and CSRF checks, and after the token is validated. An authorized caller still gets 409. The scope matrix now asserts every mutation is absent on deny with literal method names and resolvable mock types, so TestNoUnfailableMockAssertions can check each site.
|
Independent adversarial review (Opus 5.5), 4 passes, final head 226ae97: MERGE. A single executionAccounts helper returns the execution's own CloudAccountID, else all plan accounts, else every recommendation's account (nil/'' deny, no GetPlanAccounts(''), an empty set denies); every execution action, detail and list requires ALL of them in scope, and scope runs before RBAC/constraints. A scoped-out session falls through to the token path only when a token is present; revoke's status check now runs after authorization, so there's no status leak. Mutations: plan-intersection logic fails 20 cases, main's plan-only 53, dropping token!='' 9; with the gate opened, all 54 deny subtests fail on AssertNotCalled alone. Local: go test -race -short api/server/mocks 2851 pass, golangci-lint v2.10.1 clean; CI green. Follow-ups: #432, #433. |
Summary
Two gaps in the account scoping of purchase executions:
approvePurchase,cancelPurchase,retryPurchase, the session-authed branch ofrevokeViaEmailToken, andrevokeScheduledExecutionrelied only on theapprove/cancel/retry/revoke-any/ownRBAC matrix and never consulted the session'sallowed_accounts, unlike pause/resume/run/delete-planned, which callrequireExecutionAccess. An operator scoped to one account but holding an-anypermission could approve (firing the real SDK purchase), cancel, retry, or revoke an execution belonging to another account.requireExecutionAccessitself scoped by plan-account intersection, not by the accounts the execution actually buys in. With an operator scoped to A and a plan spanning {A, B}, both a child execution for B and the plan-level parent (nilCloudAccountID, which fans out to A and B) passed the gate.Root cause
requireExecutionAccess(internal/api/scoping.go) was only wired into pause/resume/run/delete-planned, and it only asked "does any of the plan's accounts intersect the scope?", which is true for a multi-account plan even when the execution spends an out-of-scope account's money. For ad-hoc web purchases (plan_idNULL) it calledGetPlanAccounts(""), which errors on the UUID column, so scoped users got a 500 on their own purchases.Fix
executionAccounts(exec), returns every cloud account an execution can buy in: itsCloudAccountIDwhen set; otherwise all of its plan's accounts (a plan-level parent fans out to every one,purchase/execution.go); otherwise, for an ad-hoc purchase, every recommendation'sCloudAccountID, refusing the whole execution when any is nil or empty.GetPlanAccountsis never called with an empty plan ID.requireExecutionAccess/requireExecutionInScoperequire every account in that set to be in the session's scope (matched by ID or name). An empty set is a deny.approvePurchaseViaSession/cancelPurchaseViaSession,tryRevokeViaSession,revokeScheduledExecution, retry, and pause/resume/run-now/delete. Purchase details and the planned/upcoming lists now use the same rule instead of the plan-only check, so a B child row is neither readable nor listed for an A-scoped user.fallsThroughToToken(err, token)). Thecontact_emailtoken path stays unscoped, as the issue intends. Without a token an out-of-scope session gets the same 404 as a missing row, so the token/session branches' status guards (409/410/401) cannot confirm the execution exists or leak its status.checkRevokableStatus, 409) now runs only after authorization: intryRevokeViaSessionafter the scope/RBAC and CSRF checks, and in the token path aftervalidateRevokeToken. Before, it ran first, so an out-of-scope session or an unauthenticated caller got "still pending" / "cannot be revoked" for a non-completed execution. An authorized caller still gets the 409.Also covers audit finding A16-009 (
TestAuthorizeSessionRevokeExecution_Matrix): therevoke-anyallow path, the!hasOwn403 and both permission-lookup error paths ofauthorizeSessionRevokeExecutionhad no tests.Regression tests
internal/api/execution_scope_test.go, existing mocks only:TestExecutionEndpoints_ScopeMatrix: {plan-level parent with nilCloudAccountID, fan-out child, ad-hoc withPlanID""} x {all accounts in scope: allow, partial scope: deny, no scope: deny} x {approve, cancel, retry, revoke completed via session, revoke scheduled, run now, pause, resume, delete, details}, 90 subtests. The caller holds admin plus every carved-out money verb, so only the scope gate can refuse. Deny cases assert the 404 and that none of the mutations (all four store methods and all three purchase-manager methods, literal names, full matcher arity) run; allow cases assert the mutation is reached.GetPlanAccounts("")errors in the mock, as Postgres does.TestExecutionEndpoints_OutOfScopeNoTokenIs404: an out-of-scope session with no token gets 404 on approve/cancel (wrong status) and on revoke of completed, pending, notified and failed executions, not 409/401. The pending/notified/failed revoke cases fail on5907bb80(409 from the status guard) and pass now.TestRevoke_AuthorizedNonRevocableStatusIs409: an authorized caller, via the session and via the email token, still gets 409 for pending/notified/failed and no mutation runs.TestHandler_revokePurchase_PendingExecution(existing) asserted the leak: a caller with no session got 409 for a pending execution. It now expects 401.TestCancel_OutOfScopeSessionWithTokenUsesContactGate: an out-of-scope session carrying a valid token reaches the token branch and succeeds as the account's contact email.TestExecutionAccounts_AdHocUnattributedDenied: nil and empty recommendation accounts deny.TestPlannedPurchases_HidesPartiallyScopedParent: the list hides a parent for a partially-scoped session and shows it when fully scoped.Mutation proofs (helper replaced, tests re-run):
CloudAccountID)fallsThroughToTokenignoring the tokenThe "no scope" deny cases for the parent and child also pass under the old logic, since plan intersection already refuses them; they are kept as matrix coverage.
Verification
With
GOTOOLCHAIN=go1.26.6:go build ./internal/...: cleango test -race -short ./internal/api ./internal/server ./internal/mocks: ok, ok, ok (includesTestNoUnfailableMockAssertions, which failed on5907bb80on the matrix's non-literalAssertNotCallednames and the unresolvable mock type returned by the helper; the test now constructs the mocks itself)golangci-lintv2.10.1 on./internal/api/...: 0 issuesgocyclo -over 10on the changed files: no hitsThe previous head's Lint Code failure (
internal/schedulercould not importproviders/azure) was a proxy.golang.orgstream error ... INTERNAL_ERRORwhile downloadingarmcosmos/v2@v2.7.0, not a code issue; Lint Code passes on main (ca4b0b21).Docker is unavailable on this host, so testcontainers-based DB integration tests were not run locally; they run in CI.
Related
Follow-ups found in review: #432 (approve's orphan-preflight 409 runs before any session or scope check), #433 (
AccountScope.Allowsmatches display names, which are not unique).Per-permission Constraints (
LeanerCloud/cloud-commitments-cli#1300) is a different mechanism, handled separately.Closes #92