[IAmJSD]

Closes #1428

Root cause

POST /api/ri-exchange/approve/{id} with a session that holds approve rights goes through fetchAndAuthorizeRIExchange. It answered a missing id with 404 and an exchange that is no longer pending with 409 (naming its status) before it ran authorizeSessionApproveRIExchange, the approve-own ownership check. A session holding only approve-own on purchases could therefore tell, for any exchange id, whether it exists and what its status is, including exchanges it did not create. #1175 closed this for token-only callers but not for this session path.

Fix

• fetchAndAuthorizeRIExchange now runs the ownership check right after the lookup, before the 404 and the 409.
• authorizeSessionApproveRIExchange accepts a nil record. An approve-own session gets the same 403 (permission denied: cannot approve another user's pending exchange) for a missing id as for another user's exchange in any status. Because it is the same 403, it also falls through to the email token, as another user's record already did. With a wrong token, every case then gets 403 invalid approval token.
• The admin key and approve-any sessions may approve any exchange, so they keep 404/409. Approve-own on an exchange the caller created also keeps the 409.
• openapi.yaml documents the approve-own behaviour on the route, its 403, 404 and 409.

Verification

• New TestHandleRequest_ApproveRIExchange_ApproveOwnSessionNoOracle runs through HandleRequest, in the style of fix(api): answer a missing RI exchange like a wrong token on approve and reject #1175's oracle test. For an approve-own session, with no token and with a wrong token, a missing id (nil record and wrapped ErrNotFound) and another user's completed, failed and expired exchanges must answer with exactly the same status, body and headers as another user's pending exchange. On origin/main, 8 subtests fail: the missing id gets 404 and the non-pending exchanges get 409. With the fix they all pass.
• New cases in TestHandleRequest_ApproveRIExchange_StatusCodes: approve-any gets 409 for a completed exchange, the approve-own creator gets 409 for its own completed exchange, and the approve-own creator can approve its own pending exchange (200). The existing 404 cases for an approve-any session are unchanged.
• go build, go vet (default, integration, e2e), golangci-lint v2.10.1 on internal/api with and without --build-tags integration: 0 issues. gocyclo -over 10 on handler_ri_exchange.go: clean.
• go test -race -short ./internal/api/: ok.
• go test -race -tags integration -run 'RIExchange|Exchange' ./internal/api/: all pass, including TestApproveRIExchange_Integration_SessionMissingIs404 on real Postgres. Three tests first timed out starting their Postgres containers on the shared Docker host and passed when re-run.
• The secrets scan and scripts/test-git-secrets-allowlist.sh pass.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 58 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: 2a11540
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: dda5dd94-61e8-4b34-9f69-bffbbaa0070a

Commits

Reviewing files that changed from the base of the PR and between 2a11540 and 9e01651.

Files selected for processing: 3

• internal/api/handler_ri_exchange.go
• internal/api/openapi.yaml
• internal/api/ri_exchange_approve_reject_status_test.go

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

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

[IAmJSD]

Opened at 9e01651: approve-own sessions now get the same 403 for a missing RI exchange and for another user's exchange in any status (ownership checked before the 404/409). Local checks are listed in the PR body.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 9e01651086ca2b75bece8131da0a4873813bd297

Blocking:

• none

Non-blocking:

• handler_ri_exchange.go:2652: an approve-own session probing a missing id now gets "permission denied: cannot approve another user's pending exchange", which reads oddly for an id that doesn't exist or an exchange that isn't pending. Keeping the text identical is the point of the fix, so I would leave it. A neutral wording ("cannot approve this exchange") would read better for every case if anyone touches it later. It is pinned by tests, so changing it means updating the test strings too.

Attacked: Traced the whole dispatch in approveRIExchange. sessionHasApproveRight routes approve-own to approveRIExchangeViaSession. CSRF is checked first and never falls through. The scope check (reshapeCloudAccountInScope) depends only on the session, not the id, so its 404 is no per-id oracle. getRIExchangeRecord maps config.ErrNotFound to (nil, nil), so both missing-id shapes reach the new nil-safe branch in authorizeSessionApproveRIExchange. Its 403 satisfies isPermissionDenied, so with a token the request goes to validateExchangeApproval, which answers a wrong token with the same 403 whether the id exists or not. Approve-any and the admin key return before the nil check, keep 404 (a fresh ClientError, not the errNotFound sentinel, so no new fall-through) and 409. A DB error on lookup is still a 500 before any authorization, so nothing changed there. Reject is token-only (rejectRIExchange), so it has no session oracle to fix.
Fail-before: git checkout origin/main -- internal/api/handler_ri_exchange.go, then ran the new tests. 8 subtests of TestHandleRequest_ApproveRIExchange_ApproveOwnSessionNoOracle fail (missing x2 and completed/failed, for both no-token and wrong-token), matching the body. Restored the file and they pass.
Ran: go build ./..., go vet plain and -tags integration on ./internal/api/, golangci-lint v2.10.1 on ./internal/api/... with and without --build-tags integration (0 issues), gocyclo -over 10 handler_ri_exchange.go (clean), go test -race -short -count=1 ./internal/api/ (ok), and go test -race -tags integration -count=1 -run 'RIExchange|Exchange' ./internal/api/ (ok, includes TestApproveRIExchange_Integration_SessionMissingIs404).

Body-stale: none

_{Reviewer: rD2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 9e01651086ca2b75bece8131da0a4873813bd297

Blocking:

• none

Non-blocking:

• internal/api/handler_ri_exchange.go: there is one behaviour change the body doesn't mention, and it's harmless. An approve-own session that also holds a valid email token for another user's exchange that is no longer pending used to get the session-path 409 with the status in the message. Now it gets the 403, falls through to the token path, and gets the token path's 409 ("already processed or was canceled..."). Whoever holds that token already knows the exchange, so nothing leaks. I'm noting it so nobody is surprised by the different 409 message.
• The expired-exchange probe passes on main too: the record is still pending, and expiry is only checked in token mode. It does no harm as a guard, but it doesn't add fail-before coverage.

Attacked:

• Checked every nil-record path. authorizeSessionApproveRIExchange reads record only on the approve-own branch, which is now nil-guarded. Admin key and approve-any return before that point, so they reach the 404/409 below as before. getRIExchangeRecord maps ErrNotFound to nil, so both missing-record shapes take this path. A DB error returns before the auth check, as before. The scope check (sec(api): executeExchange skips the allowed_accounts scoping all three sibling RI-exchange reads enforce #93) still runs before the fetch. CSRF rejection still can't fall through (errCSRFRejected is checked first in approveRIExchange). The 4-eyes check still runs after. Reject is token-only, so there's no matching session oracle there.
• Fail-before: I reverted handler_ri_exchange.go to origin/main and the new ApproveOwnSessionNoOracle test fails on exactly the 8 subtests the body lists (missing, missing-as-ErrNotFound, completed, failed, each with no token and with a wrong token). Restoring the fix makes them pass.
• Ran: go build ./...; go vet on internal/api with and without -tags integration; go test -race -short -count=1 ./internal/api/ passed; go test -race -tags integration -count=1 -run RIExchange ./internal/api/ passed.
• All three acceptance criteria in sec(api): RI exchange session approve still leaks missing (404) vs not-pending (409) to approve-own callers #1428 are met (same status, body and headers across probes, with and without a token; the creator gets 409; approve-any gets 404/409; a table test through HandleRequest pins it).

Body-stale:

• none

_{Reviewer: rE1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at 9e01651086ca2b75bece8131da0a4873813bd297 merged onto main (2baf23d5), together with #1240, #1302, #1309, #1454, #1456, #1463 in that order (combined tree d882e3bf, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on 9e016510 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 9e01651086ca2b75bece8131da0a4873813bd297.