[IAmJSD]

Root cause

Since #594, a caller with no session gets the same 401 on approve, cancel and revoke whether or not the execution exists. A signed-in caller could still tell a missing ID from an existing one:

Caller (signed in)	Missing ID	Existing ID (before)
no approve/cancel RBAC, no token	404 execution not found	403 permission denied: requires ...
approve-own/cancel-own, not the creator	404	403 cannot approve/cancel another user's ...
outside the account scope	404 execution not found	404 Not found (different body)
token present, not the contact email	404	403 not the authorized approver / no per-account contact email configured
any caller, Azure/GCP orphan (approve)	404	409 references an account that no longer exists (ran before authorization)
authorized, or a contact-email match, with no CSRF header	404	403 CSRF validation failed, or success on the token branch

CSRF only ran inside the session sub-paths, after the lookup. The orphan preflight ran in loadApproveExecution, before any authorization.

Fix (internal/api/handler_purchases.go)

The decision the issue asks for: yes, for signed-in callers too, "you may not act on this" now answers the same as "this does not exist".

• CSRF before the lookup. requireSessionBeforeLookup now also validates CSRF, so any session caller without a valid header gets the same 403 CSRF validation failed (errCSRFRejected) for every ID. This includes the email-link token branch. Every caller there now has a session (sec(api): unauthenticated cancel/revoke distinguish missing (404) from existing (401) executions #435), and the session bearer is the credential being protected. This matches the intent of sec(auth): enforce per-permission Constraints on ladder/config/RI-exchange-config/marketplace/revoke (SEC-01 follow-up to #60) #404's "session present means CSRF applies". The frontend apiRequest already sends X-CSRF-Token on every POST, the deep-link flow included.
• Denials concealed. concealExecutionDenial maps a final authorization denial to the same 404 execution not found that getExecutionOr404 returns for a missing ID. "Final" means a scope-miss errNotFound, an RBAC 403, or a contact_email 401/403 from authorizeApprovalAction. The mapping is applied at the approve, cancel and revoke session-denial branches and at the three token-branch authorizeApprovalAction calls. It returns a concealedDenialError that responds exactly like the missing case and keeps the hidden cause, for an Info log and for tests.
• Not concealed, on purpose: wrapped backend failures (500s still propagate), constraintDeniedError (approve-any/own Constraints) and the 4-eyes 403. Those only reach callers who already hold the approve verb for the execution and are in scope.
• Orphan 409 after authorization. loadApproveExecution is replaced by getExecutionOr404. The fix(plans): a plan with no matching recommendations schedules zero-count purchases instead of failing #609 orphan check (rejectOrphanExecution) now runs in approvePurchaseViaSession once the session is authorized. The token branch needs no copy: an orphan has no per-account contact_email, so authorizeApprovalAction never authorizes it, and purchase.ApproveExecution runs the same predicate itself.

Legitimate flows are unchanged: an admin or RBAC holder with CSRF, and the contact-email recipient on the email link with CSRF.

Behavior changes reviewers should weigh

1. The token branch now needs CSRF when a session is present. TestTokenOnlyApprove_BypassesCSRF pinned the old behavior and is replaced by TestTokenApprove_SessionRequiresCSRF. That test checks that no header gives 403 and the execution is never read, and that a valid header still approves. Non-browser callers that use a session bearer without a CSRF token on these three routes would now get 403. API-key callers are unaffected: tryGetSession returns nil for them, so they already got 401 (sec(api): unauthenticated cancel/revoke distinguish missing (404) from existing (401) executions #435).
2. Less helpful error messages. Users who are denied now see execution not found instead of the specific reason, for example "set the cloud account's contact_email". The reason is logged at Info (purchase action denied, answered as execution not found: ...).

Tests

• New: TestHandleRequest_TokenActions_SignedInCannotTellMissingFromExisting (handler_purchases_token_oracle_test.go). It runs through HandleRequest and covers approve, cancel and revoke × {pending, completed, Azure orphan} × token {none, valid, wrong} × 6 signed-in callers: no RBAC with CSRF, no RBAC without CSRF, approve-own/cancel-own non-creator, scoped-out, admin without CSRF, contact email without CSRF. That is 153 subtests. Each one asserts identical status, headers and body for the existing and the missing ID. Every status write in that fixture loses its CAS, so even a regression cannot reach a purchase.
• New: TestTokenApprove_SessionRequiresCSRF (described above).
• Updated (assertion shape only):
  • Tests that pinned a specific 403 message now use requireConcealedDenial(t, err, "<original reason>"). It asserts the 404 body and that the hidden cause is still the gate the test is about. These are: session_denial_final_test.go (approve/cancel/revoke), the RejectsMismatchedSession / RejectsMissingContactEmail / RejectsGlobalNotifyWhenContactSet approve tests, the CancelOwn / NoVerb / LegacyNullCreator / DeepLink contact-gate cancel tests, revokePurchase_SessionNoPermissionNoToken and approvePurchase_OutOfScope.
  • The scope matrix accepts either 404 form via isNotFound404.
  • The orphan 409 tests use an approve-capable principal, since the guard is now post-authorization.
  • internal/server TestHandleLambdaHTTPEvent_DecodesBase64FormBody now proves body-token decoding with the post-authorization invalid revocation token 403, because the two 403s it used to tell apart are now both concealed.
  • About 30 existing tests gained a ValidateCSRFToken mock because CSRF now runs before the lookup.

Verification

All of this is unit/mock evidence: testify mocks plus the real purchase.Manager over a mock store. No real cloud calls or purchases were made. I did not run the frontend against a deployed backend, so the deep-link flow sending X-CSRF-Token is verified by reading frontend/src/api/client.ts, not end to end.

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

go build ./... && go vet ./...                                   # OK
go test ./...                                                    # all ok (internal/mocks guard re-run after a fix: ok)
go test -short -race ./internal/api/ ./internal/server/          # ok
golangci-lint v2.10.1 run --allow-parallel-runners ./...         # 0 issues
gocyclo -over 10 $(git ls-files '*.go' | grep -v _test.go)       # silent
bash scripts/gosec-hook.sh internal/api/handler_purchases.go     # clean
gofmt -l internal/                                               # clean

Fail-before / pass-after: I put the main handler_purchases.go back with the new tests. A one-line stub type was needed so the test helper compiles.

• Before: SignedInCannotTellMissingFromExisting: 153/153 subtests FAIL. TestTokenApprove_SessionRequiresCSRF/no_CSRF_header FAIL.
• After: 153/153 PASS, and both CSRF subtests PASS.

Out of scope

• sec(api): delayed token approval validates the execution snapshot, not the token hash at the status change #590 (delayed token approval validates the snapshot, not the token hash) is untouched. approveViaToken only changed its authorizeApprovalAction error mapping.
• Other execution-keyed routes (pause, resume, run, delete, retry) already use the scope gate's errNotFound. I did not audit them for the same RBAC-403 vs 404 difference. That may deserve a follow-up issue.

Closes #597

Maintainer: please mirror the issue's triage labels on this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Note

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

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6043b4aa-0336-4d00-bde9-5c4ba560e7a5

📥 Commits

Reviewing files that changed from the base of the PR and between 4b4526a and 8441e6f.

📒 Files selected for processing: 11

• internal/api/coverage_extras_test.go
• internal/api/executed_notification_flow_test.go
• internal/api/execution_scope_test.go
• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go
• internal/api/handler_purchases_token_errors_test.go
• internal/api/handler_purchases_token_oracle_test.go
• internal/api/handler_test.go
• internal/api/middleware_test.go
• internal/api/session_denial_final_test.go
• internal/server/lambda_coverage_test.go

+-----------------------------------------------------------------------------------------------------------------------------------------------------------+
| Don't be a slave to formal methods. Don't blindly adopt any technique without putting it into the context of your development practices and capabilities. |
+-----------------------------------------------------------------------------------------------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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]

Independent review of 8441e6f: clean

Reviewer: fresh cold-briefed agent (non-author), dispatched by claude-cc-pr-merge-2. This is a security review of an auth + purchase-approval path.

Findings (none blocking)

1. Stale comments at internal/api/handler_purchases.go:660-662 and :1386 ("with no session ... returns 401"). That branch is unreachable now that requireSessionBeforeLookup guarantees a session.
2. CSRF is validated twice per request (:703 and :871, :1459, :1665). auth.ValidateCSRFToken (internal/auth/service.go:369) is idempotent, so the cost is one extra session lookup. The second check is redundant defence in depth (probe p9).
3. Residual oracle for callers holding the approve verb: the constraint-denied 403 and the 4-eyes 403 are deliberately not concealed. These callers already passed the scope gate and can list executions. Acceptable, and consistent with the PR's design.
4. Timing and transient errors: an existing ID triggers extra GetCloudAccount/GetGlobalConfig calls, so a transient failure gives 500 vs 404, for token-bearing callers only.
5. Test gap: the oracle matrix never pairs a valid CSRF header with the contact-email caller. That success path is covered only by TestTokenApprove_SessionRequiresCSRF/valid_CSRF_header.
6. Pause, resume, run, delete and retry routes were not audited; the PR lists them as out of scope.

Authorization matrix checked (approve / cancel / revoke; missing ID is 404 execution not found both before and after)

Signed-in caller	Before	After
No session	401	unchanged
No CSRF header	403 if exists / 404 if missing	403 CSRF validation failed for every ID, before lookup (:699-705)
No RBAC, no token	403 permission denied	404 concealed
approve-own/cancel-own, non-creator	403	404 concealed
Scoped out	404 Not found (different body)	404 execution not found
Token, not the contact email / no contact email	403	404 concealed
Orphan, unauthorized (approve)	409 before authz	404 (orphan check at :880, after authz)
Orphan, authorized	409	409
Authorized with CSRF	success	success
Contact-email recipient with token + CSRF	success	success
Contact-email recipient with token, no CSRF	success (old bypass)	403 CSRF
Backend errors	500	500

Authorization is not weakened: every RBAC, scope, contact-email and 4-eyes denial still errors, and concealExecutionDenial rewrites only the code and body. API-key callers still get 401. The frontend sends X-CSRF-Token on every apiRequest (frontend/src/api/client.ts:183-184, :211). Purchases go through it (purchases.ts:57,70,112), as does the deep link (purchases-deeplink.ts:121).

Removed test assertions audited

All removals are intentional behaviour changes:

• TestTokenOnlyApprove_BypassesCSRF is replaced by the stronger TestTokenApprove_SessionRequiresCSRF.
• Forbidden assertions became requireConcealedDenial, which still pins the hidden cause.
• IsNotFoundError became isNotFound404, slightly looser but still a 404 with a fixed message.
• The Lambda form-body test still proves the token is decoded.
• The roughly 30 added CSRF mocks are additive.

What I attacked

• go vet is clean. go test ./internal/api/... ./internal/server/... passes 3876.
• Fail-before: with main's handler_purchases.go plus a stub type, the new tests show 156 --- FAIL lines.

#	Mutation (reverted)	Result
p1	drop pre-lookup CSRF	38 fail
p2	drop approve-own RBAC	1 fail (thin)
p3	drop scope check	11 fail
p4	token branch raw 403	22 fail
p5	skip contact-email match	1 fail (thin)
p6	orphan preflight back before authz	10 fail
p7	revoke denial unconcealed	12 fail
p8	cancel denial unconcealed	14 fail
p9	drop second CSRF check	0 fail (redundant, finding 2)
p10	revoke token denial unconcealed	19 fail

Worktree cleaned: yes.

[cristim]

Gate record for 8441e6f (claude-cc-pr-merge-2):

• Independent adversarial security review: clean at this SHA (PR review above). It covers an authorization matrix of 12 cases, an audit of every removed assertion, and 10 hand mutation probes on the auth gates; all were caught except a redundant second CSRF check. Six non-blocking notes.
• Local verification on macOS (fixture/mock level through the real handlers): at this SHA, go test ./internal/api/... ./internal/server/... passes 3876. Fail-before: main's handler with the new tests gives 156 --- FAIL lines. Because main moved after this PR's base (including fix(purchase): refuse cancel on scheduled executions with an accurate revoke hint #927 in handler_purchases.go), I also merged this SHA into current main (7138188) locally. The auto-merge was clean; on that tree go build ./... is ok and the api/server tests pass 3912. No real-account verification: the change is response shaping on existing auth paths.
• CI: 25/25 green on this SHA.
• Labels mirrored from sec(api): authenticated callers can still tell a missing execution from an existing one on approve, cancel and revoke #597. CodeRabbit: no actionable review (quota).
• Marking ready and merging with squash and --match-head-commit.