[IAmJSD]

Root cause

runPlannedPurchase (POST /api/purchases/planned/{id}/run) authorized through authorizeExecutionManagement. For a caller who isn't the row's creator, that only checks that update-any:purchases is held. It never evaluates update-any's Constraints. A user with update-any bounded to provider=aws and an unbounded execute:purchases could therefore run another user's Azure scheduled purchase. #1134 (which closed #1098) fixed the same gap for pause and resume, but run still used the unbounded helper.

Fix

• Run now calls the constraint-aware helper that fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134 merged for pause and resume. That helper was authorizeExecutionPauseResume. It's renamed to authorizeBoundedExecutionManagement because it now serves pause, resume and run. Its behavior is unchanged:
  • update-any is checked against the strict per-recommendation scope of the stored row (revokeExecutionConstraintSets).
  • A Constraints denial falls back to the creator match.
  • A constraint-store error is returned as-is. It's never treated as a denial, so it can't fall through to the creator branch.
  • Sessions without update-any keep the existing authorizeExecutionManagement creator check.
• The row-based tail is extracted as authorizeUpdateAnyOnRow.
• New authorizeClaimedRowManagement re-runs the same decision on the row that the claim CAS returns. runNowClaimCheck calls it before the existing execute:purchases Constraints check. Without it, an edit that lands between the handler's last pre-claim read and the claim (the sec(purchase): constraint checks read recs before the CAS; execution buys from the post-CAS row (TOCTOU) #415 window) could move the row out of the bounded update-any's scope, and the row would still be bought.
• The execute:purchases Constraints check on the recommendations (sec(auth): enforce per-permission Constraints on execute-any/own, approve-any/own, runPlanned (SEC-01 follow-up to #1210) #60, sec(purchase): constraint checks read recs before the CAS; execution buys from the post-CAS row (TOCTOU) #415) is unchanged and still applies after this check.

Closes #1129

Related work

• sec(api): planned-purchase pause/resume ignore the update-any permission's Constraints #1098's helper is merged on main (via fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134), and this PR reuses it.
• fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 is still an open draft. It adds authorizePlannedFullScope for planned DELETE (update-any, then cancel-any, both bounded by Constraints). This PR doesn't touch authorizePlannedPurchaseCancel or authorizePlannedFullScope, so the two PRs edit different parts of handler_purchases.go. Once both land, the update-any-on-stored-row check is implemented in two places: authorizeUpdateAnyOnRow here and the update-any step of authorizePlannedFullScope in fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097. They should be consolidated into one helper, as the issue asks.
• sec(api): enforce CSRF on PATCH and every other non-safe method #1104 (CSRF) is unrelated to this route's authorization.

Verification

All commands were run from the repo root at the head of this branch. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's ~/.aws/config default profile is invalid. The tests make no AWS calls.

Unit tests (mock store, real auth matcher via grantPermissions, real auth.Service for API keys): internal/api/handler_purchases_run_update_any_test.go

• update-any bounded on each of providers, services, regions and account IDs: an out-of-scope non-creator is denied with 403, an in-scope one is allowed, and an unbounded grant is allowed.
• Strict scope: if one recommendation is out of scope, the run is denied.
• admin:* is unaffected.
• A creator holding an out-of-scope update-any is still allowed. A non-creator with the same grant gets the constraint 403.
• A constraint-store error is returned (ErrorIs). The creator branch doesn't swallow it, and nothing runs.
• A user API key is bounded by the key's update-any Constraints, not its owner's (end to end through HandleRequest).
• The claimed row is re-checked: the snapshot is in scope, the claimed row is out of scope, the run is refused with "nothing was bought", and the row is marked failed.
• Direct branch coverage of authorizeClaimedRowManagement: the admin API key, a creator and a non-creator without update-any, a NULL creator, and a probe error.

Integration tests (real Postgres via testcontainers, real handler and purchase manager, counting fake provider, no cloud API): internal/api/handler_purchases_run_update_any_integration_test.go

• TestRunNow_Postgres_RowMovedOutOfUpdateAnyScopeBeforeClaimIsRefused: a non-creator holds update-any bounded to aws. A second writer runs a raw UPDATE that moves the row to gcp after the handler's last pre-claim check and before the claim. Expected result: 403 "nothing was bought", zero provider calls, row failed with no_submission, and no purchase_history.
• TestRunNow_Postgres_InScopeUpdateAnyNonCreatorPurchases is the control: the same grant on an unmoved row buys once.

Fail before, pass after. Production files were reset to origin/main (75219c0). The test that only compiles against the new helper (TestAuthorizeClaimedRowManagement_WithoutUpdateAny) was removed for this run, and the rest were run:

go test ./internal/api/ -run 'TestRunPlanned_' -count=1 -v
--- FAIL: TestRunPlanned_APIKeyUpdateAnyFollowsKeyConstraints/key_bounded_to_another_provider_is_refused
--- FAIL: TestRunPlanned_UpdateAnyStrictScope/every_recommendation_must_be_in_scope
--- FAIL: TestRunPlanned_UpdateAnyConstraintErrorDoesNotFallBack
--- FAIL: TestRunPlanned_UpdateAnyBoundRecheckedOnClaimedRow
--- FAIL: TestRunPlanned_UpdateAnyConstraintDimensions/deny/{providers,services,regions,account_ids}
--- FAIL: TestRunPlanned_UpdateAnyDenialFallsBackToCreatorOnly/non-creator_denied
(all allow / admin / creator-allowed subtests PASS on main too)

go test -tags integration ./internal/api/ -run 'TestRunNow_Postgres' -count=1 -v
--- FAIL: TestRunNow_Postgres_RowMovedOutOfUpdateAnyScopeBeforeClaimIsRefused   (expected a client-facing error, got <nil>: the moved row was bought)
--- PASS: TestRunNow_Postgres_InScopeUpdateAnyNonCreatorPurchases

With the fix, all of these pass. A mutation probe was also run: with only the authorizeClaimedRowManagement call removed from runNowClaimCheck, TestRunPlanned_UpdateAnyBoundRecheckedOnClaimedRow fails.

CI-equivalent checks:

go build ./...
go vet ./internal/api/ && go vet -tags integration ./internal/api/
go test ./internal/api/ -count=1                                   # ok
go test -tags integration ./internal/api/ -run 'TestRunNow_|TestClaimedRow|Pause' -count=1   # ok (real Postgres)
go test ./internal/mocks/ -count=1                                 # ok (TestNoUnfailableMockAssertions)
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] ./internal/api/   # 0 issues
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_purchases.go internal/api/claimed_row_check.go   # silent
bash scripts/gosec-hook.sh internal/api/handler_purchases.go internal/api/claimed_row_check.go             # exit 0
git-secrets --scan on the changed files in a fresh repo with an isolated HOME                            # clean
bash scripts/test-git-secrets-allowlist.sh                                                     # 42 PASS, 0 FAIL

Coverage gaps. All of this is unit, mock and local-Postgres evidence. No real cloud account or deployed environment was exercised, and nothing was purchased.

Out of scope

• authorizeExecutionManagement is now reached only as the fallback for sessions without update-any. Its doc comment gets one added sentence; it isn't otherwise restructured.
• MaxPurchaseAmount on update-any isn't evaluated for run. The stored-row sets (revokeExecutionConstraintSets) carry no amount, which matches pause, resume and cancel. The spend cap stays on execute:purchases, as before.
• Consolidation with fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097's authorizePlannedFullScope is described under "Related work".

Could a maintainer mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/few, effort/s, type/security?

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 75219c0
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b80cb7bf-1912-46c0-8c7b-dd1ce4ffe9fe

📥 Commits

Reviewing files that changed from the base of the PR and between 75219c0 and 9a5c4dc.

📒 Files selected for processing: 4

• internal/api/claimed_row_check.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_run_update_any_integration_test.go
• internal/api/handler_purchases_run_update_any_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE 9a5c4dc

Base: origin/main 9fceda3. git merge-tree is clean (merged tree e05460bb). The merged tree builds, and vet passes with and without -tags integration.

The fix addresses #1129. Run now goes through authorizeBoundedExecutionManagement (internal/api/handler_purchases.go:215), so a non-creator holding update-any is held to the strict per-recommendation scope of the stored row, and execute:purchases Constraints still apply afterwards. The post-claim re-check (claimed_row_check.go:29 -> handler_purchases.go:258) closes the #415 window for the update-any bound.

Consistency:

• Matches fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134 pause/resume exactly. It is the same helper, renamed: the denial is not wrapped with wrapConstraintDenied, a Constraints 403 falls back to the creator match, and a store or probe error is returned without falling back.
• fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 (DELETE) is still OPEN (head 7c6b0b0), not merged. It wraps with wrapConstraintDenied and gives the creator branch the extra cancel-own Constraints. Neither difference is a gap for run: run has no email-token fall-through, so wrapConstraintDenied has nothing to stop, and execute:purchases Constraints bound the creator. Once fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 lands, authorizeUpdateAnyOnRow (:243) and authorizePlannedFullScope duplicate the update-any-on-stored-row step. Consolidating them is sec(api): bound update-any by its Constraints on planned-purchase run #1129's own follow-up ask. The PR body says so.

Findings (all nits):

• nit internal/api/handler_purchases.go:258 - authorizeClaimedRowManagement repeats the update-any branch of authorizeBoundedExecutionManagement. The bounded helper could load the row and delegate to it, apart from the no-update-any path's own fetch. It also has no h.auth == nil guard, which cannot be reached because the pre-claim helper returns 500 first.
• nit internal/api/handler_purchases.go:434 - the run handler now reads the row three times before the claim (requireExecutionAccess, loadPlannedExecution and this call).
• nit (process) the PR body's "fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134 (which closed sec(api): planned-purchase pause/resume ignore the update-any permission's Constraints #1098)" is parsed as a closing keyword, so closingIssuesReferences lists sec(api): planned-purchase pause/resume ignore the update-any permission's Constraints #1098 next to sec(api): bound update-any by its Constraints on planned-purchase run #1129. sec(api): planned-purchase pause/resume ignore the update-any permission's Constraints #1098 is already CLOSED, so this does no harm, but rephrase it.
• nit (process) the PR has a single commit, 9a5c4dc, at 415 changed lines (396+, 19-), over the 400 cap.

Attacked:

• Merged tree: go test ./internal/api/ -count=1: 3968 passed. Integration (-tags integration, testcontainers Postgres) TestRunNow_|TestClaimedRow|Pause: 89 passed, including both TestRunNow_Postgres_*.
• Mutation probes (merged tree, scratch copy):
  • run reverted to authorizeExecutionManagement: 5 TestRunPlanned_* FAIL.
  • claimed-row call removed: unit TestRunPlanned_UpdateAnyBoundRecheckedOnClaimedRow FAIL, and integration TestRunNow_Postgres_RowMovedOutOfUpdateAnyScopeBeforeClaimIsRefused FAIL.
  • creator fallback removed: run and pause/resume creator tests FAIL.
  • non-403 errors treated as denial: both ConstraintErrorDoesNotFallBack tests FAIL.
  • claimed-row creator check removed (no update-any): TestAuthorizeClaimedRowManagement_WithoutUpdateAny FAIL.
  • claimed-row update-any short-circuit: TestRunPlanned_UpdateAnyBoundRecheckedOnClaimedRow FAIL.
• Edge cases: a row with no recommendations gets one empty strict set (handler_purchases_revoke.go:366), so a bounded grant denies it. A legacy NULL creator never matches. A user API key is evaluated with the key's own Constraints (test present).

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 9a5c4dc

Findings (none blocking):

• [non-blocking] internal/api/handler_purchases.go:215-234 and :258-273 - authorizeBoundedExecutionManagement and authorizeClaimedRowManagement make the same decision twice: the admin sentinel, then the update-any probe, then a row or creator check. Only the row source differs.
  • The bounded helper could load the row and delegate to the claimed-row helper.
  • That would also remove a redundant second sessionHoldsVerb(update-any) probe on the no-update-any path. Today that path probes again through authorizeExecutionManagement (:186).
  • The body defers consolidation until fix(api): bound update-any by its Constraints on planned-purchase DELETE #1097 lands.
• [non-blocking] handler_purchases.go:258 - authorizeClaimedRowManagement has no h.auth == nil guard, unlike its sibling at :219. It is unreachable, because the pre-claim call at :417 already returns 500 when auth is nil.
• No migration.

Against #1129: the issue holds at base, where run used authorizeExecutionManagement. That returns nil for any update-any holder (base ~:183-189).

• The fix is at the root: run now goes through the constraint-aware helper (head :417).
• It also closes the window between the pre-claim read and the claim (sec(purchase): constraint checks read recs before the CAS; execution buys from the post-CAS row (TOCTOU) #415), by re-checking the row the CAS returns (claimed_row_check.go:29).
• Execute Constraints still run before and after the claim (:438, claimed_row_check.go:32).
• Pause and resume behave the same after the rename.

Attacked:

• Blast radius:
  • authorizeExecutionPauseResume has no remaining references.
  • authorizeExecutionManagement is now called only from the bounded helper's fallback (:227) and by existing tests, which still compile.
• Merge: git merge-tree --write-tree origin/main <head> is clean.
  • Main changed handler_purchases.go (daf8713) and internal/auth (dec9ad7), and neither overlaps the changed functions.
  • Re-run on a merged-tree export.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. All 6 were caught:
  • M1, run's pre-claim check reverted to authorizeExecutionManagement: the dimension-deny, strict-scope, error-fallback, creator-fallback and API-key tests FAIL.
  • M2, a constraint-store error treated as a denial: the run, pause and resume ErrorDoesNotFallBack tests FAIL.
  • M3, the claim re-check skips update-any: UpdateAnyBoundRecheckedOnClaimedRow FAILS.
  • M4, the claim re-check's creator check removed: TestAuthorizeClaimedRowManagement_WithoutUpdateAny FAILS.
  • M5, the creator fallback removed from authorizeUpdateAnyOnRow: the creator_allowed subtests for run, pause and resume FAIL.
  • M6, the admin sentinel removed from the claimed-row helper: WithoutUpdateAny FAILS.
• Edge cases checked:
  • a NULL creator;
  • an empty UserID;
  • a row with no recommendations;
  • an API key's own Constraints vs its owner's;
  • admin:*;
  • a probe error.

Verified locally, with no cloud credentials, under the heavy lock, using the repo's testcontainers fixture:

• Head and merged tree:
  • go build ./..., go vet ./internal/api/ and go vet -tags integration ./internal/api/: rc 0.
  • gofmt -l on the 4 changed files: clean.
  • go test -count=1 ./internal/api/ ./internal/mocks/: ok. All 7 new tests pass.
  • go test -tags integration -count=1 -run 'TestRunNow_Postgres|TestPauseClaim|Pause|TestClaimedRow' ./internal/api/: ok.
• Head: the full go test -tags integration -count=1 ./internal/api/: ok.
• Fail-before, with the new tests on base (minus the one needing the new helper):
  • Every deny, strict-scope, error-fallback, claimed-row and API-key subtest FAILS. The allow, admin and creator subtests pass.
  • Integration: RowMovedOutOfUpdateAnyScopeBeforeClaimIsRefused FAILS ("expected a client-facing error, got "), while the in-scope control passes.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at 9a5c4dc7daadb78dd54957fa776e9e922ad9f476 merged onto current main (e43ba7c7; merge-tree 0e0e59d8; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/api.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at 9a5c4dc7daadb78dd54957fa776e9e922ad9f476).