Repository navigation
sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned - #378
Merged
Conversation
Contributor
|
Warning Review limit reached
This review includes 8 billable files and costs up to $2.00.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 30 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 58 included PR review attempts over the past 7 days set your current allowance at 2 reviews 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 (8)
Comment |
cristim
added
urgency/this-sprint
Member
Author
|
@coderabbitai review |
Contributor
|
Member
Author
|
@coderabbitai full review |
Contributor
|
cristim
added a commit
that referenced
this pull request
Sep 28, 2026
…ypass Independent review of #378 found that approvePurchase's session-vs-token dispatch treated authorizeSessionApprove's Constraints-exceeded 403 the same as a bare missing-verb 403 (isPermissionDenied matches any 403 ClientError), so it fell through to approveViaToken -- which never evaluates Constraints. A session with a capped approve-any/approve-own permission who was also the account's registered contact_email could approve past its own cap via the email deep-link (which always carries a session, since the frontend forces a login first). Fix: wrap a Constraints-exceeded 403 in a new constraintDeniedError type so isPermissionDenied's deliberately strict, unwrapped *clientError assertion no longer recognizes it, forcing the dispatch's default (propagate, terminal) branch instead of the fall-through one. IsClientError still resolves it via errors.As + Unwrap, so HTTP response mapping is unaffected. approveViaToken also gets its own independent Constraints check (approveConstraintsForSession) as defense in depth, in case it is ever reached some other way with such a session present. Also: authorizeSessionApprove and runPlannedPurchase previously skipped requireNonZeroCommitment, so an execution whose recommendations summed to /bin/zsh would read as an uncapped MaxPurchaseAmount. All five constrained call sites (execute-any, execute-own, approve-any, approve-own, runPlannedPurchase) now route through enforcePurchaseConstraints, which threads an explicit action parameter and always runs requireNonZeroCommitment before the Constraints check. Filed #404 to track the five remaining call sites named in issue #60's review comments (ladder config, global config, RI-exchange config, marketplace list, revoke) that carry the same fail-open shape but are out of scope for this PR. Regression tests: TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenied proves the dispatch-level fix with a non-empty token (the exact shape the review flagged); TestHandler_approveViaToken_SessionApproveAny_ConstraintsDenied drives approveViaToken directly to prove its own independent check. Both fail against the pre-fix code (verified via git stash) and pass against the fix.
…rove-any/own, runPlanned SEC-01 (issue #1141, PR cli-monorepo#1210) enforced per-permission Constraints (MaxPurchaseAmount, Providers, Services, Regions, AccountIDs) on the base execute:purchases permission. The same fail-open class remained on three sibling money-mutating call sites: the direct-execute gate (authorizeSessionExecuteDirect, execute-any/execute-own), the session-approve gate (authorizeSessionApprove, approve-any/approve-own, which hands off to ApproveAndExecute), and runPlannedPurchase's pending/paused -> running transition. Each ran only a bare HasPermissionAPI verb check, so a session capped tighter on one of these verbs than on its base execute:purchases permission could still mutate past that cap. requirePermissionConstraints previously hardcoded its action parameter to "execute" since every existing caller checked exactly that action. Threaded action through explicitly now that execute-any, execute-own, approve-any and approve-own are real distinct values, and wired all three new call sites to build the same purchaseConstraintSets already used by the web execute path and re-check them against the verb that actually authorized the mutation. Regression tests mirror TestHandler_executePurchase_PermissionConstraintsDenied for each new call site (execute-any, execute-own, approve-any, approve-own), asserting a 403 before any state mutation or SDK call. Verified with a standalone repro: a session with execute:purchases (uncapped) and execute-any:purchases capped at $100 could direct-execute a $50,000 purchase pre-fix; post-fix the same request is denied with a 403 naming the exceeded constraints. Co-Authored-By: claude-flow <ruv@ruv.net>
…ypass Independent review of #378 found that approvePurchase's session-vs-token dispatch treated authorizeSessionApprove's Constraints-exceeded 403 the same as a bare missing-verb 403 (isPermissionDenied matches any 403 ClientError), so it fell through to approveViaToken -- which never evaluates Constraints. A session with a capped approve-any/approve-own permission who was also the account's registered contact_email could approve past its own cap via the email deep-link (which always carries a session, since the frontend forces a login first). Fix: wrap a Constraints-exceeded 403 in a new constraintDeniedError type so isPermissionDenied's deliberately strict, unwrapped *clientError assertion no longer recognizes it, forcing the dispatch's default (propagate, terminal) branch instead of the fall-through one. IsClientError still resolves it via errors.As + Unwrap, so HTTP response mapping is unaffected. approveViaToken also gets its own independent Constraints check (approveConstraintsForSession) as defense in depth, in case it is ever reached some other way with such a session present. Also: authorizeSessionApprove and runPlannedPurchase previously skipped requireNonZeroCommitment, so an execution whose recommendations summed to /bin/zsh would read as an uncapped MaxPurchaseAmount. All five constrained call sites (execute-any, execute-own, approve-any, approve-own, runPlannedPurchase) now route through enforcePurchaseConstraints, which threads an explicit action parameter and always runs requireNonZeroCommitment before the Constraints check. Filed #404 to track the five remaining call sites named in issue #60's review comments (ladder config, global config, RI-exchange config, marketplace list, revoke) that carry the same fail-open shape but are out of scope for this PR. Regression tests: TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenied proves the dispatch-level fix with a non-empty token (the exact shape the review flagged); TestHandler_approveViaToken_SessionApproveAny_ConstraintsDenied drives approveViaToken directly to prove its own independent check. Both fail against the pre-fix code (verified via git stash) and pass against the fix.
cristim
force-pushed
the
fix/60-enforce-approve-runplanned-constraints
branch
from
September 28, 2026 13:48
da30659 to
8efd142
Compare
Member
Author
|
Rebased onto current |
Member
Author
|
Independent adversarial review, final head 225a829: MERGE. Earlier passes approved the code: the approve-any token-flow bypass is closed (terminal constraintDeniedError plus approveConstraintsForSession), requireNonZeroCommitment runs on all 5 call sites, and the rebase resolution runs the constraints check before RunPlannedPurchaseNow's 4-eyes/CAS funnel with both sides' guards intact. Final pass (Opus 5.5, tests delta 8efd142..225a829): only handler_purchases_test.go changed. With the runPlannedPurchase constraint block disabled, TestHandler_runPlannedPurchase_ConstraintDenied and _ZeroCommitment both fail; they pass restored. TestNoUnfailableMockAssertions passes; CI 25/25. TOCTOU follow-up: #415. |
cristim
added a commit
that referenced
this pull request
Sep 28, 2026
…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.
cristim
added a commit
that referenced
this pull request
Sep 28, 2026
…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.
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.
Summary
PR cli-monorepo#1210 fixed SEC-01 (issue #1141) by enforcing per-permission
Constraints(MaxPurchaseAmount, Providers, Services, Regions, AccountIDs)on the base
execute:purchasespermission before a web-submitted purchaseexecutes. The same fail-open class remained on three sibling
money-mutating call sites named in this issue:
execute-any/execute-own(direct-execute),
approve-any/approve-own(session approve, which firesthe purchase synchronously via
ApproveAndExecute), andrunPlannedPurchase(the scheduler run-now transition).
Root cause
authorizeSessionExecuteDirect,authorizeSessionApproveandrunPlannedPurchaseeach ran only a bareHasPermissionAPIverb check fortheir respective verb, never consulting that permission's own
Constraints.A session whose
execute-any/execute-own/approve-any/approve-ownpermission was configured with a tighter cap than its base
execute:purchasespermission could mutate past that tighter cap, becausethe verb check alone (with nil request-side constraints) always passes for a
constrained permission.
requirePermissionConstraintsalso had itsactionparameter hardcoded to"execute", since every existing caller checked exactly that action. Thatmade it structurally impossible to reuse for
execute-any/execute-own/approve-any/approve-ownwithout threading a real parameter back in(exactly what the removed constant's own doc comment called for).
An independent review of this PR (see thread comments) found a second gap
in the initial fix:
approvePurchase's session-vs-token dispatch treatedany 403 from
authorizeSessionApprovethe same, whether it was a baremissing-verb denial or a Constraints-exceeded denial, and fell through to
approveViaToken(the email deep-link flow) in both cases.approveViaTokennever evaluates Constraints, so a session with a cappedapprove-any/approve-ownwho was also the account's registeredcontact_emailcould approve past its own cap via the email link (whichalways carries a session, since the frontend forces a login first). The
review also found that
authorizeSessionApproveandrunPlannedPurchaseskipped
requireNonZeroCommitment, so an execution whose recommendationssummed to $0 would read as an uncapped
MaxPurchaseAmount.Fix
actionthroughrequirePermissionConstraintsas an explicitparameter (
internal/api/handler.go), updating the three existingexecute:purchases/execute:ri-exchangecall sites to pass"execute"explicitly.
authorizeSessionExecuteDirect(internal/api/handler_purchases.go) nowtakes the execution's recommendation set and, once the
execute-any/execute-ownverb branch is chosen, builds the samepurchaseConstraintSetsthe web execute path already builds and calls
requirePermissionConstraints(session, "execute-any"|"execute-own", "purchases", sets).authorizeSessionApprovedoes the same forapprove-any/approve-ownagainst the already-loaded execution's
Recommendations(no extra fetchneeded: both callers already hold the full row).
runPlannedPurchasere-hydrates the execution viaGetExecutionByIDandcalls the same constraint check before the
pending|paused -> runningtransition.
constraintDeniedErrorwrapper type so aConstraints-exceeded 403 is no longer recognized by
isPermissionDenied'sdeliberately strict, unwrapped
*clientErrorassertion, forcingapprovePurchase's dispatch into its terminaldefault:(propagate)branch instead of falling through to the token flow.
IsClientErrorstill resolves it to the right HTTP code/message viaerrors.As+Unwrap, so response mapping is unaffected.approveViaTokenalso gets its own independent Constraints check(
approveConstraintsForSession) as defense in depth, in case it's everreached some other way with such a session present. All five constrained
call sites now route through a shared
enforcePurchaseConstraints(action, recs)helper that always runsrequireNonZeroCommitmentfirst.Regression tests
Mirrors of
TestHandler_executePurchase_PermissionConstraintsDeniedforevery new call site, each proving a session that passes the bare-verb gate
but whose per-permission
Constraintsreject the request gets a 403before any state mutation or SDK call:
TestHandler_authorizeSessionExecuteDirect_ExecuteAny_ConstraintsDeniedTestHandler_authorizeSessionExecuteDirect_ExecuteOwn_ConstraintsDeniedTestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied(end-to-end via
approvePurchase, assertsApproveAndExecute/ApproveExecutionare never called)
TestHandler_authorizeSessionApprove_ApproveOwn_ConstraintsDeniedTestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenied(review follow-up: drives
approvePurchasewith a non-empty token,the exact shape the review flagged, and asserts no
ValidateCSRFToken/approveViaToken-only mock is ever consulted)TestHandler_approveViaToken_SessionApproveAny_ConstraintsDenied(reviewfollow-up: drives
approveViaTokendirectly, with thecontact_emailgate wired to succeed, to isolate
approveConstraintsForSession's owncheck)
Proof the fix closes a real gap (not committed, used to validate locally):
a standalone repro built a session holding
execute:purchaseswith no capand
execute-any:purchasescapped atMaxPurchaseAmount: 100, then calledexecutePurchasewithexecute_mode: "direct"for a $50,000 recommendation.origin/main(pre-fix): the call succeeds (err == nil), i.e. the$50,000 purchase executes despite the $100 execute-any cap.
permission denied: this request exceeds the constraints configured on your execute-any permission for purchases.The two review-follow-up tests above were verified against the pre-review-fix
commit (
git stashisolating the two production files) and confirmed tofail there (reaching
approveViaToken/its contact_email gate, or hitting amissing-mock panic proving the same code path was reached) before passing
against the current commit.
Verification
go build ./...— passesgo vet ./...— cleango test -race ./internal/api/...— 2222 passedgo test ./...(full module) — 6140 passed in 33 packagesgo mod tidy -diff— emptygolangci-lint runat the exact CI-pinned version (v2.10.1, underGOTOOLCHAIN=go1.26.6to matchci.yml'sGO_VERSION: 1.26.6) — 0 issuesgocyclo -over 10on the changed files — cleanDocker is unavailable on this host (daemon hung), so testcontainers-based
DB integration tests were not run locally; CI covers those.
Follow-up
Issue #60's own review comments widened the fail-open census beyond the
three verbs above to five more call sites (
upsertLadderConfig,updateConfig,updateRIExchangeConfig,marketplaceList,revokePurchase). Those are tracked separately in #404 rather than foldedinto this PR, since each needs its own money-relevant
Constraintsdimensions worked out (not a mechanical copy of
purchaseConstraintSets).Closes #60