fix(approval): let email-link approvers load purchase details without view:purchases

[IAmJSD]

Root cause

#548 fixed the toast half of #529: 403/404 no longer say "retry". The backend half was still open. The approval deep link's confirm modal (buildApprovalDetailsBody) loads GET /api/purchases/{id}, which requires view:purchases and account scope. The email-link approve is authorized by the account's contact_email instead (authorizeApprovalAction). So a contact approver without view:purchases, or outside the account scope, could approve but never load the details. Since #497 the modal refuses to open without them.

Fix

Backend: a new read-only route, POST /api/purchases/approve/{id}/details (internal/api/handler_purchases_approval_details.go). It is authorized like the approve it precedes, without the write:

1. validateUUID, then requireSessionBeforeLookup. This means 401 without a session and 403 CSRF validation failed without a valid CSRF header, both before the execution is read (sec(api): unauthenticated cancel/revoke distinguish missing (404) from existing (401) executions #435, sec(api): authenticated callers can still tell a missing execution from an existing one on approve, cancel and revoke #597).
2. getExecutionOr404.
3. A session with view:purchases and the execution in its account scope gets the response. This is the same gate and the same response body as GET /api/purchases/{id}, so nothing new is exposed to these callers.
4. Otherwise, only when a token is present (fallsThroughToToken), the request needs both:
   • the session email is one of the execution's contact approvers (authorizeApprovalAction, unchanged), and
   • the token matches this execution's stored hash and has not expired (purchase.ValidateApprovalToken).
5. Every final denial goes through concealExecutionDenial, so it is the same 404 execution not found a missing ID gets (sec(api): authenticated callers can still tell a missing execution from an existing one on approve, cancel and revoke #597). The exception is an expired token: a caller who passed the contact gate with the correct token gets 410, the same as the approve path.

The route is POST so the token goes in the body and stays out of the Function URL access logs (#398). The issue's suggested GET /api/purchases/approve/{id} path is already the side-effecting approve. The route goes in the existing /api/purchases/approve/ AuthPublic prefix, is registered before the approve route (suffix /details), and shares the approve_cancel_public rate-limit bucket (30/min/IP). The handler does not write anything (the tests assert this).

Frontend (small and isolated):

• api.getApprovalDetails(id, token) is new.
• buildApprovalDetailsBody(id, token?) uses it when a token is passed.
• purchases-deeplink.ts passes dl.token.
• History still calls buildApprovalDetailsBody(id) with no token, so it keeps using GET /api/purchases/{id}.
• isPermanentDetailsError also treats 410 as permanent, with the text "The approval link has expired." Without this, the new 410 would get the "retry" advice the issue complains about.

Authorization matrix: reading details on the approval link

All callers are signed in, unless noted. "Valid token" means the token from this execution's email, not expired.

Caller	Before (GET /api/purchases/{id})	After (POST …/approve/{id}/details)
No session	401	401, before lookup
Session, no/invalid CSRF header	n/a (GET)	403 CSRF, before lookup
view:purchases, in scope, any/no token	200	200
view:purchases, scoped out, no token	404	404 (concealed)
view:purchases, scoped out, is contact, valid token	404	200 (mirrors approve's fall-through, #92)
No view:purchases, is contact, valid token	403 (the bug)	200
No view:purchases, is contact, wrong/missing token	403	404 (concealed)
No view:purchases, is contact, execution has no token	403	404 (concealed)
No view:purchases, is contact, expired token	403	410
No view:purchases, not the contact, valid token	403	404 (concealed)
approve-own, not creator, not contact, any token	403	404 (concealed)
Missing execution ID (any signed-in caller with CSRF)	404	404, identical status/headers/body

The token alone never authorizes. The contact email alone never authorizes. The approve, cancel and revoke routes are unchanged.

Verification

Everything here is unit/mock evidence: testify mocks plus the oracleHandler fixture from #935, all driven through Handler.HandleRequest. Status writes in that fixture lose their CAS anyway. No real cloud calls, purchases, emails or deploys. Go tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

go build ./... && go vet ./...                                        # OK
go test ./internal/api/... ./internal/server/... -count=1             # ok (api, server, server/scheduledauth)
golangci-lint v2.10.1 run --allow-parallel-runners ./internal/api/... # 0 issues
gocyclo -over 10 internal/api/handler_purchases_approval_details.go internal/api/router.go  # silent
bash scripts/gosec-hook.sh <changed .go files>                        # clean
gofmt -l internal/                                                    # clean
git secrets --scan <changed files>  (temp repo, --register-aws)       # exit 0
bash scripts/test-git-secrets-allowlist.sh                            # 42 PASS, 0 FAIL
cd frontend && npm ci
npx jest purchases-deeplink approval-details history                  # 14 suites, 203 tests passed
npm run lint                                                          # 0 errors (125 existing warnings)
npx tsc --noEmit                                                      # clean
npm run build                                                         # compiled
CI=1 npx playwright test tests-e2e/history-action-confirmation.spec.ts --project=chromium  # 14 passed

New Go tests are in internal/api/handler_purchases_approval_details_test.go:

• ContactApproverWithoutViewPurchases: the generic GET still gives 403, and the new route gives 200 with the recommendations and writes nothing.
• TokenAndContactBothRequired: wrong token, no token, execution without a token, expired token, and not-the-contact with a valid token.
• ViewPurchasesSession: in scope; scoped out and not the contact; scoped out and the contact.
• SessionAndCSRFBeforeLookup: asserts GetExecutionByID is never called.
• SignedInCannotTellMissingFromExisting: status {pending, completed, Azure orphan} × token {none, valid, wrong} × the six fix(api): answer a hidden execution like a missing one on approve, cancel and revoke #935 callers. Each case asserts identical status, headers and body for an existing and a missing ID, and asserts no writes.
• TestRouter_ApprovalDetailsDoesNotShadowApprove.

Fail-before / pass-after (run):

• Go: I removed the new handler file and reverted router.go, keeping the tests. All 6 new test functions FAIL. The key case fails with expected: 200, actual: 400 {"error":"invalid ID format: must be a valid UUID"}, because the path fell through to the approve route. After the fix all pass.
• The oracle-matrix test fails before only because the route did not exist. It guards the new route; it is not a regression of existing behavior.
• Jest: I reverted the four frontend source files and kept the updated test. The purchases-deeplink suite fails to run (0 tests; I did not inspect the exact error, presumably the missing getApprovalDetails export). After the fix: 18/18 pass.
• Playwright: the approval-link 403/404/410 cases now stub …/approve/{id}/details. I did not run them against the old bundle.

Mutation probes on the new handler. Each was applied alone, then the new tests were run:

Mutation	Result
drop the contact-email gate	killed
drop token validation	killed
drop session+CSRF-before-lookup	killed
return the raw denial instead of concealing it	killed
drop the account-scope check on the view branch	killed
drop the view:purchases check	killed
fall through to the token branch even with no token	survived: equivalent mutant. With an empty token, ValidateApprovalToken always fails and that failure is concealed as the same 404, so no response can differ. fallsThroughToToken is kept for parity with approvePurchase and to skip the contact lookup.

Out of scope / for the maintainer

• Toast wording on the deep link. A 403 there now really means CSRF (or the rate limit), not missing view:purchases, so the existing 403 text from fix(approval): stop telling permanent 403/404 details failures to retry #548 is less accurate on this path. A 404 now also covers "you are not this purchase's contact approver". I left the wording alone because you are doing UI work.
• Response shape. The token branch returns the same body as GET /api/purchases/{id}: full recommendation records plus plan_name and error. That is what the modal's PurchaseDetails type expects, and it is roughly what the approval email already shows that contact. Trimming it would be a separate decision.
• The token branch does not restrict by execution status. The token's 7-day TTL and the contact gate bound it. A non-pending execution still fails at approve (409), as before.
• getPurchaseDetails still has its own copy of the plan-name lookup. I did not refactor it.

Closes #529

Maintainer: please mirror the issue's triage labels on this PR: triaged, priority/p2, severity/medium, urgency/this-sprint, impact/few, effort/m, 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: 5d993bea-fc51-4c10-905b-d9b90ef8e46f

📥 Commits

Reviewing files that changed from the base of the PR and between adcd80c and 1366784.

📒 Files selected for processing: 9

• frontend/src/__tests__/purchases-deeplink.test.ts
• frontend/src/api/index.ts
• frontend/src/api/purchases.ts
• frontend/src/approval-details.ts
• frontend/src/purchases-deeplink.ts
• frontend/tests-e2e/history-action-confirmation.spec.ts
• internal/api/handler_purchases_approval_details.go
• internal/api/handler_purchases_approval_details_test.go
• internal/api/router.go

+-----------------------------------------------------------------------------------------+
| If you don't make mistakes, you're not working on hard enough problems. - Frank Wilczek |
+-----------------------------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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 badc86a: draft: blocks ready

No BLOCKING code findings. Details below.

Findings

1. NON-BLOCKING, internal/api/handler_purchases_approval_details.go:63-66 + internal/purchase/approvals.go:503: the approval token hash is only cleared on cancel, and the new route does not check execution status. A contact approver holding an unexpired token can still read details of an already-approved or completed execution until the 7-day expiry. They are already an authorized approver and see the same data in the email, so no cross-tenant exposure. The PR body already states this. Optional follow-up: restrict the token branch to pending executions.
2. NON-BLOCKING, internal/api/handler_purchases.go:2569-2587 (buildPurchaseDetailsResponse): the token branch returns error (internal execution error text) and every recommendation of a multi-account execution, not just the contact's account. This matches the approve scope (authorizeApprovalAction accepts the union of contact emails) and the email. Trimming error for this branch would be a follow-up product decision.
3. NON-BLOCKING, frontend/src/purchases-deeplink.ts:84-100: the token stays in window.location.search while the confirm modal is open and is only removed by replaceState afterwards. This is the existing behavior, not new in this PR. The new request carries the token in the POST body, not the URL.

What I attacked

• Authorization: token alone never authorizes. getApprovalDetails needs a session plus CSRF before the lookup (requireSessionBeforeLookup), then either view:purchases in scope, or contact-email match (authorizeApprovalAction) AND ValidateApprovalToken (hash match, then expiry). Same checks as approve for token and contact. An unknown token plus contact gets a concealed 404.
• IDOR: the token hash is read from the same execution row that is fetched by the path ID, so a token for execution A cannot read B. The matrix tests cover wrong token, no token, execution without token, and not-the-contact.
• Oracle (#935/#597): every denial goes through concealExecutionDenial to the missing-ID 404. The only differing answer is 410, which needs a correct token for that execution plus the contact gate, so it leaks nothing to a caller without the secret. SignedInCannotTellMissingFromExisting compares status, headers and body across 6 callers. I did not find a gap.
• Rate limit: approvalDetailsHandler calls checkRateLimit(..., "approve_cancel_public") before anything else (internal/api/router.go:612-617). Route is registered before the approve prefix route, and TestRouter_ApprovalDetailsDoesNotShadowApprove covers shadowing.
• Writes: the handler does not write.
• Frontend token: it goes in the POST body (frontend/src/api/purchases.ts), History keeps the tokenless GET, and 410 is classed permanent. The existing console.error logs the error object, not the token.
• Mutation probes (auth path, 3): drop contact gate, killed (3 test funcs FAIL); drop token validation, killed (TokenAndContactBothRequired FAIL); an earlier unrelated probe failed to build, so it was discarded. All reverted, worktree clean.
• Fail-before/pass-after: jest purchases-deeplink against origin/main frontend sources fails to compile (TS2339 getApprovalDetails missing); at the SHA 2 suites / 46 tests pass. Go: go test ./internal/api -run ApprovalDetails passes at the SHA (67 passing test lines). The PR's own Go fail-before run (route missing, 400 UUID error) is plausible; I did not re-run it. Playwright not run by me.

Gate status (origin/main Review gate)

• Independent adversarial review of the full diff naming the SHA: this comment.
• CI on the SHA: NOT YET. 17 jobs pending, 6 pass (pre-commit passes), none failed or action_required at the time of review.
• CodeRabbit: skipped (draft). No actionable CR threads.
• Labels: match #529 (triaged, priority/p2, severity/medium, urgency/this-sprint, impact/few, effort/m, type/bug).
• Local verification: unit/mock only (testify mocks plus the #935 oracle fixture), labeled as such in the PR body. No real-account evidence.

Blocks ready

• Mark the PR ready for review (it is still a draft).
• Wait for the pending CI jobs to go green on this SHA.
• Rebase and re-review if main moves the router or handler_purchases.go.
• Optional: decide on the three NON-BLOCKING items (status gate, error field, token left in URL while the modal is open), as follow-up issues rather than in this PR.

[cristim]

CI on badc86a is red because of this PR's new test, not flakiness. Unit and Integration both fail TestNoUnfailableMockAssertions (internal/mocks):

• internal/api/handler_purchases_approval_details_test.go:184:3: <expr>.AssertNotCalled(t, "") -- receiver is not a plain id[entifier]
• internal/api/handler_purchases_approval_details_test.go:193:3: same

The guard only accepts AssertNotCalled on a plain mock variable with a named method. Otherwise the assertion cannot fail. Assign the mock to a local (e.g. store := ...) and assert the specific method name, e.g. store.AssertNotCalled(t, "GetPurchaseExecution"). Then check that go test ./internal/mocks/ ./internal/api/ passes locally.

The independent review (no blocking code findings) covers badc86a. A new commit gets a delta review.

[IAmJSD]

CI fix at 1366784. The red Unit and Integration jobs were internal/mocks TestNoUnfailableMockAssertions. It rejected two h.config.(*MockConfigStore).AssertNotCalled(...) calls in handler_purchases_approval_details_test.go (lines 184 and 193) because the receiver isn't a plain identifier. Both now bind store := h.config.(*MockConfigStore) first. This is test-only, and the assertions are unchanged.

go test ./internal/mocks/ -run TestNoUnfailableMockAssertions passes, as does go test ./internal/api/ -run ApprovalDetails. golangci-lint reports 0 issues, and the branch still merges cleanly with main. These tests ran with AWS config set to /dev/null. Sorry: the local run missed this guard because I didn't run ./internal/mocks.

🤖 Generated with Claude Code

[cristim]

Independent delta review of 1366784 (previous: badc86a): no actionable findings

Delta: OLD is an ancestor of NEW; one new commit (1366784, test-only, +4/-2 in internal/api/handler_purchases_approval_details_test.go). No production, router or frontend change, so the earlier auth review of the handler (session + CSRF, view:purchases or contact + token, 404 concealment, shared approve_cancel_public bucket) carries over unchanged. Its three non-blocking notes (no status gate on the token branch, response includes error and all recommendations, token stays in the URL while the modal is open) are unchanged and remain non-blocking follow-ups.

Test fix: h.config.(*MockConfigStore).AssertNotCalled is now store := h.config.(*MockConfigStore); store.AssertNotCalled(...) at test lines 184-185 and 194-195, so TestNoUnfailableMockAssertions (CI failure at OLD) is satisfied.

What I attacked (temporary edits in a scratch worktree, each reverted):

• Handler calls h.config.GetExecutionByID before requireSessionBeforeLookup: both SessionAndCSRFBeforeLookup subtests (no session, contact without CSRF) fail with "GetExecutionByID was called 1 time(s)", so the not-called checks can still fail.
• Contact gate (authorizeApprovalAction) removed: TokenAndContactBothRequired, ViewPurchasesSession and SignedInCannotTellMissingFromExisting fail.
• Token validation (ValidateApprovalToken) removed: TokenAndContactBothRequired fails.
• go test ./internal/mocks/ ./internal/api/ pass; jest purchases-deeplink.test.ts 18/18 pass.
• Full diff vs origin/main: merge-tree against current main is conflict-free; 395 additions (under the 400 cap).

Gate status (origin/main Review gate):

• Independent review: this comment names 1366784; the first review covered the code and the delta is test-only.
• CI on 1366784: all green (CI - Build & Test, pre-commit, Frontend E2E, Frontend build; Unit Tests, Lint, Integration, E2E, Security pass; CodeRabbit skipped for draft; Snyk skipped). No action_required runs.
• Local verification: handler tests exercise the real HTTP path through HandleRequest with mocked stores (mock evidence, not a real account); mutation probes above show fail-before/pass-after behavior.
• Remaining blocks: PR is still a draft (author must mark it ready); labels against #529 were not re-checked in this delta pass.

[cristim]

Gate record for 1366784 (origin/main CLAUDE.md "Review gate"):

• Independent review of badc86a: no blocking code findings.
  • The token alone never authorizes. The route requires session and CSRF before the execution lookup, then either view:purchases or the contact-approver email plus ValidateApprovalToken (hash match, then expiry).
  • There is no cross-execution read. Denials are concealed as the missing-ID 404, and the route shares the approve_cancel_public rate-limit bucket.
  • Non-blocking notes: there is no status gate on the token branch, the response includes error and all recommendations, and the token stays in the URL while the modal is open.
• Independent delta review of 1366784: no actionable findings. One test-only commit binds the mock to a local before AssertNotCalled, which fixes the TestNoUnfailableMockAssertions CI failure.
• CI on 1366784: CI Success, Run pre-commit hooks, Frontend E2E and Frontend build all pass.
• Local verification (macOS, mocks):
  • Calling GetExecutionByID before the session check fails both SessionAndCSRFBeforeLookup subtests.
  • Removing the contact gate or the token validation fails TokenAndContactBothRequired.
  • go test ./internal/mocks/ ./internal/api/ passes. Jest purchases-deeplink passes 18/18.
  • No real account.
• CR skipped: draft and quota-limited; covered by the independent reviews plus local verification.
• Labels mirrored from fix(approval): email-link approvers without view:purchases cannot approve, and the retry toast is wrong for permanent errors #529.

Marking ready (owner-authorized) and merging at 1366784 with --match-head-commit.