[IAmJSD]

Summary

DELETE /api/purchases/planned/{id} is authorized by authorizePlannedPurchaseCancel (internal/api/handler_purchases.go). Its update-any:purchases branch granted full scope on the verb alone: it never evaluated the granting permission's Constraints (Providers, Services, Regions, AccountIDs). A session whose update-any was bounded to one provider could therefore cancel any user's scheduled purchase for any provider. This is the same SEC-01 fail-open class as #678 and #404. Per the maintainer's scope update on the issue, #978 covered the cancel-any and creator branches, and this PR covers the update-any branch.

Fix

This builds on #975, #978 and #1001 and reuses their helpers unchanged:

• A new authorizePlannedFullScope tries update-any and then cancel-any. For each one the session holds (sessionHoldsVerb, so a user API key is probed on the key, as in fix(api): evaluate planned-purchase full-scope verbs on the API key, not its owner #1001), it calls requirePermissionConstraints with revokeExecutionConstraintSets(execution). Those are the strict per-recommendation sets built from the stored row, never from the request.
• Fallback: a Constraints 403 on update-any falls through to cancel-any, then to the creator branch (authorizePlannedCreatorCancel). That is the sec(auth): enforce per-permission Constraints on cancel-any/cancel-own (SEC-01 follow-up to #404) #678 fallback, now with one more verb in front.
• Terminal denial: a non-creator gets the constraint message, not the ownership 403. The last constraint denial is passed to the creator branch, the same mechanism as before.
• Fail closed: a probe error or a constraint-store error is returned as-is. It is never read as "not held" or as a denial, so it cannot fall through to cancel-any or to the creator branch.
• authorizePlannedPurchaseCancel now loads the row before the full-scope checks, because update-any needs it too. Previously an update-any holder skipped that read. A missing row still maps to errNotFound.
• Unchanged: the stateless admin API key bypass, the entry gate (requireDeleteOrCancelPurchasePermission), 404/403 concealment for callers without a verb, and the creator branch.

No migration and no DB write path changed. This is a read-then-CAS authorization check, the same shape as the cancel-any check from #978.

Closes #961

Tests

The new file is internal/api/handler_purchases_planned_update_any_test.go. Bearer-session answers come from the real matcher via grantPermissionsScoped (auth.PermissionsAllowForConstraintSets). The API-key cases run Handler.HandleRequest against a real auth.Service (key intersected with owner) through the existing #979 serveKeyScope harness.

• TestDeletePlannedPurchase_UpdateAnyConstraintDimensions: one deny case per dimension (provider, service, region, account) asserts the 403 and that TransitionExecutionStatus is never called. One allow control per dimension, plus an unconstrained control.
• TestDeletePlannedPurchase_UpdateAnyStrictScopeAndRecommendations:
  • an unknown region is refused by a region-bounded grant;
  • every recommendation must be in scope;
  • a row with no recommendations is refused by a bounded grant and allowed by an unbounded one.
• TestDeletePlannedPurchase_UpdateAnyFallback:
  • out-of-scope update-any falls back to unbounded or in-scope cancel-any, to a creator holding delete, and to a creator with in-scope cancel-own;
  • it is denied, with the constraint message and no write, for a non-creator with delete, a non-creator with bounded cancel-any, a legacy NULL-creator row, and a creator with out-of-scope cancel-own.
• TestDeletePlannedPurchase_UpdateAnyAdministratorsGroup: unbounded {admin,*} still cancels. {admin,*} bounded to another provider is refused, because admin matches update-any through the strict path.
• TestDeletePlannedPurchase_UpdateAnyConstraintErrorDoesNotFallBack: an update-any constraint-store error propagates. This holds even though the caller is the creator and holds cancel-any.
• TestDeletePlannedPurchase_APIKeyUpdateAnyFollowsKeyConstraints: real auth.Service. A key whose update-any is bounded to another provider gets a 403 and never reaches the mutation. An in-scope key is allowed.

Changes to existing tests:

• TestDeletePlannedPurchase_CancelConstraints/"update-any is unchanged and unconstrained" asserted the bug. It now asserts the denial.
• Five strict-mock admin tests in handler_purchases_test.go (TestHandler_deletePlannedPurchase, _DisablesPlan, _AlreadyDisabledPlan, _BackendErrorReturns5xx, _NilExecution) now stub the authorization read of the row with .Once(). In _BackendErrorReturns5xx the .Once() keeps its "no recovery-path read" assertion meaningful. Their other assertions are unchanged.

Verification (local, macOS)

Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, because this machine's default AWS profile is invalid.

• Fail-before / pass-after (unit tests on mocks and the real matcher): I reverted only internal/api/handler_purchases.go to origin/main (8caed39) and kept the new tests, then ran go test ./internal/api/ -count=1 -v -run 'UpdateAny|TestDeletePlannedPurchase_CancelConstraints'.
  • Before the fix: 15 deny cases failed (14 subtests plus TestDeletePlannedPurchase_UpdateAnyConstraintErrorDoesNotFallBack), and all 19 allow-control subtests passed.
  • With the fix: all pass.
• Mutation probes on handler_purchases.go, all killed:
  • grant a held update-any without its Constraints (the original bug);
  • evaluate update-any's Constraints under the cancel-any verb;
  • non-strict (empty) constraint sets;
  • fall back on any error, not only a 403;
  • stop at the first denial with no fallback;
  • drop the terminal constraint denial (ownership message instead);
  • treat a probe error as "not held";
  • remove update-any from the full-scope verbs.
• go build ./... and go vet ./...: clean.
• go test -race -count=1 ./internal/api/: ok.
• go test -count=1 ./internal/api/... ./internal/auth/... ./internal/server/... ./internal/mocks/: ok.
• go test -tags integration ./internal/api/ -run 'TestDeletePlannedPurchase_RecordsCancelActor|TestNotifiedRow_' against real Postgres (testcontainers): ok. These are the existing DELETE-planned integration tests on a real store.
• golangci-lint v2.10.1 run --allow-parallel-runners --build-tags integration ./internal/api/...: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/api/: no output.
• gosec v2.28.0 via scripts/gosec-hook.sh on the changed files: clean.
• git-secrets --scan over the changed files, in a fresh temp repo with an isolated HOME: clean. bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• Not verified against a real deployment. The authorization decision runs the real matcher and a real auth.Service over mocked stores.

Out of scope (noted, not changed)

• Pause, resume and run: authorizeExecutionManagement (POST /api/purchases/planned/{id}/pause|resume|run) also grants full scope on update-any without its Constraints. sec(api): planned-purchase DELETE ignores the granting permission's Constraints #961 names only DELETE. /run re-checks execute Constraints before buying (fix(purchase): re-check purchase constraints on the claimed row before buying #1036), but pause and resume do not. This may deserve its own issue.
• delete:purchases Constraints: the creator branch still accepts a creator match alone for delete:purchases holders and does not evaluate delete's Constraints. The maintainer's scope update keeps sec(api): planned-purchase DELETE ignores the granting permission's Constraints #961 to the update-any branch.
• Creator fallback: like the cancel-any fallback from fix(api): enforce cancel permission constraints on planned purchase cancel #978, a constrained update-any denial falls back to the creator branch. A creator whose only verb is an out-of-scope update-any can therefore still delete their own row. I kept this to match the "same fallback semantics" the issue asks for. Tightening it would also change fix(api): enforce cancel permission constraints on planned purchase cancel #978's cancel-any behavior.
• TOCTOU: the Constraints are evaluated on the row read before the status CAS, as on the cancel-any path from fix(api): enforce cancel permission constraints on planned purchase cancel #978. SavePurchaseExecution's upsert can rewrite recommendations, so a concurrent writer could in principle change a row's scope between the check and the cancel. I did not audit every caller for that. The window is the same one fix(api): enforce cancel permission constraints on planned purchase cancel #978's cancel-any path already has, and closing it would mean moving the check into the CAS, which is a wider change. I did not add a race test for it.
• sec(auth): an admin-scoped API key ignores its account and amount constraints on non-strict checks #956 (admin API key Constraints on non-strict checks) is handled separately. This PR does not touch sessionHoldsVerb or the auth helpers.

Labels

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

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 8caed39
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2b94e8a1-03f7-4357-a783-9e2b95e88af7

📥 Commits

Reviewing files that changed from the base of the PR and between 8caed39 and 7c6b0b0.

📒 Files selected for processing: 4

• internal/api/handler_purchases.go
• internal/api/handler_purchases_cancel_constraints_test.go
• internal/api/handler_purchases_planned_update_any_test.go
• internal/api/handler_purchases_test.go

+---------------------------------------------+
| Rabbit in the repo, here to fix that bug 🎶 |
+---------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• 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 7c6b0b0

Reviewed the full diff of #1097 (one commit) against origin/main 56e226a. The base is 8caed39, 39 commits behind. git merge-tree against current main is clean (tree c68b1a85). The main commits that touch this route since the base are #1158 (daf8713, requirePlannedCancelPlanScope and disablePlanAfterCancel) and #1134 (e0215b0, the bounded update-any check for pause and resume). #1097 also merges cleanly with #1192 on top.

Findings (none blocking):

• nit internal/api/handler_purchases.go:479 (authorizePlannedFullScope) - main now has a second update-any-on-stored-row check in authorizeExecutionPauseResume (fix: rate-limiter worker context, pause/resume update-any constraints, IAM parity wildcards #1134), and sec(api): bound update-any by its Constraints on planned-purchase run #1192 extracts it as authorizeUpdateAnyOnRow. The semantics agree: the check uses the strict revokeExecutionConstraintSets of the stored row, a 403 falls back to the creator, and a non-403 error is returned as-is. Two differences remain:
  • DELETE wraps the denial with wrapConstraintDenied; pause, resume and run do not.
  • DELETE's creator fallback is also bounded by cancel-own when the creator holds it; run's is the creator match alone.
    Both differences are defensible per route. Consolidate the duplicated update-any step after sec(api): bound update-any by its Constraints on planned-purchase run #1192 lands, as sec(api): bound update-any by its Constraints on planned-purchase run #1192 proposes.
• nit internal/api/handler_purchases.go:454 - the planned-cancel path now reads the row up to three times: authorizePlannedPurchaseCancel, requirePlannedCancelPlanScope for scoped sessions, and then the CAS. That is correct, but the row read in this function could be passed through. Optional.
• nit process - the largest commit changes 324 lines (286+/38-), within the 400-line cap.

Plan interaction (merged tree): deletePlannedPurchase runs authorizePlannedPurchaseCancel, then requirePlannedCancelPlanScope, then the cancel CAS, then disablePlanAfterCancel. A bounded update-any holder who passes the new check is still refused with a 403 by #1158's account check when the plan spans accounts outside their scope, and nothing is written. Unscoped callers are unchanged.

Local results (merged tree, macOS):

• go vet and go vet -tags integration on internal/api: clean.
• go test ./internal/api/: ok.
• go test -tags integration ./internal/api/ -run 'DeletePlanned|NotifiedRow|PlannedPurchase|Cancel' (testcontainers Postgres): ok.

Attacked (mutation probes on handler_purchases.go in a scratch copy, all killed):

• M1, held update-any granted without Constraints: CancelConstraints/update-any, APIKeyUpdateAny, AdministratorsGroup and ConstraintErrorDoesNotFallBack failed.
• M2, a non-403 error falls through to the next verb: ConstraintStoreErrorDoesNotFallBack failed.
• M3, update-any removed from the verb list: PlannedPurchaseRoutes_FullScopeFollowsTheAPIKey/delete and APIKeyUpdateAny failed.
• M4, terminal denial dropped: StrictScope and AdministratorsGroup failed.
• M5, probe error read as "not held": APIKeyFullScopeProbeErrorRefuses/delete failed.
• M6, non-strict empty constraint set: StrictScope and APIKeyUpdateAny failed.

Also checked:

• Unbounded update-any still cancels.
• Cancel-own still works through the creator fallback (existing CancelConstraints tests pass).
• The admin API key bypass is unchanged.
• A row with no recommendations is refused by a bounded grant.
• Updating the existing test that had asserted the bug ("update-any is unchanged and unconstrained") was correct.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 7c6b0b0

Findings (none blocking):

• [non-blocking] internal/api/handler_purchases.go:410-418 - The doc on requireDeleteOrCancelPurchasePermission still says "update-any already grants full-scope cancel one step later". After this PR, update-any is bounded by its Constraints, so the comment overstates the entry gate.
• [non-blocking] The PR body's "Out of scope: Pause, resume" is stale. origin/main e0215b0 (sec(api): planned-purchase pause/resume ignore the update-any permission's Constraints #1098) already added authorizeExecutionPauseResume with update-any Constraints. The body needs correcting, not the code.
• [non-blocking] Duplication: the creator check in authorizePlannedCreatorCancel (handler_purchases.go:522) duplicates isExecutionCreator, which main added in e0215b0. Main's version also rejects an empty session UserID. Reusing the helper after a rebase would be a reasonable follow-up.
• [non-blocking] In the merged tree, DELETE reads the row up to 3 times for a scoped session: requireExecutionAccess, the new authorization read and main's requirePlannedCancelPlanScope (daf8713). An unrestricted admin now costs 1 read where it used to cost 0. This is a performance cost only, and the result is correct.

Against #961: this fixes the root cause.

• In authorizePlannedFullScope (handler_purchases.go:479-500), update-any now goes through the same strict per-recommendation check as cancel-any: requirePermissionConstraints plus revokeExecutionConstraintSets on the stored row.
• Fallback and terminal-denial semantics match sec(auth): enforce per-permission Constraints on cancel-any/cancel-own (SEC-01 follow-up to #404) #678.
• Refuted: a (nil, nil) from GetExecutionByID. The Postgres store returns ErrNotFound for a missing row (internal/config/store_postgres.go:1896).
• Refuted: isForbidden missing the wrapped constraint error. IsClientError uses errors.As (handler_router.go:102).
• Blast radius: deletePlannedPurchase (handler_purchases.go:575) is the only caller. No exported or route changes, and no migration.

Attacked:

• Mutation probes on a scratch copy, each restored and confirmed identical with cmp. Each was caught:
  • Constraints evaluated under cancel-any instead of the held verb: 4 tests FAIL.
  • Fallback on any error, not only a 403: ConstraintStoreErrorDoesNotFallBack FAILS.
  • Terminal denial dropped: CancelConstraints and StrictScope FAIL.
  • Non-strict {{}} constraint sets: APIKey, AdministratorsGroup and CancelConstraints FAIL.
  • Verb order swapped: ConstraintErrorDoesNotFallBack FAILS.
  • A held update-any granted unconstrained (the original bug): APIKey, ConstraintError and StrictScope FAIL.
• Covered by tests and consistent with the code: a bounded {admin,*}, the API-key path, empty recommendations and the legacy NULL creator.

Verified locally, on git archive exports with no cloud credentials, under the heavy lock:

• Head: go build ./... and go vet ./internal/api/: OK. gofmt -l on the 4 changed files: clean. go test -count=1 ./internal/api/: ok.
• Merged tree:
  • git merge-tree --write-tree origin/main 7c6b0b0 is clean. Main changed handler_purchases.go in e0215b0 and daf8713.
  • Build, vet and go test -count=1 ./internal/api/ on that tree: ok.
• Fail-before:
  • With the head's test files on base, all 6 new UpdateAny tests and CancelConstraints FAIL.
  • The 3 admin fixture tests also fail, as expected, on their now-unmet GetExecutionByID stubs.
• Integration tests were not run. There is no store change.

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 7c6b0b07b909dd93f5b9b141dc5d805f55b3bf6b merged onto current main (61bf6070; merge-tree 1ed3bc87; 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 7c6b0b07b909dd93f5b9b141dc5d805f55b3bf6b).