fix(api): answer a missing RI exchange like a wrong token on approve and reject by IAmJSD · Pull Request #1175 · LeanerCloud/cloud-commitments-platform

IAmJSD · 2026-10-20T01:07:42Z

Stacked on #1172. This branch contains #1172's commit (fea837d, fixes #587) plus this PR's change. Review only the commits after fea837d: fea837d1...8999bc2f. Once #1172 merges, main will be merged in here and the diff will shrink to #601's change.

Root cause

POST /api/ri-exchange/approve/{id} and /reject/{id} are AuthPublic: the emailed token alone authorizes them, with no session. In token mode they gave different answers for an existing and a missing exchange id to a caller with a wrong token:

	before (mock store)	before (real Postgres)	after
existing exchange, wrong token	403 `invalid approval token`	403	403 `invalid approval token`
existing exchange with no stored token	403 `this exchange record does not support approval`	403	403 `invalid approval token`
missing exchange id	404 `exchange record not found`	500	403 `invalid approval token`

(Same for reject with rejection.) On Postgres the oracle was 500 vs 403, because PostgresStore.GetRIExchangeRecord returns a bare error for no row, never (nil, nil), so the handler's record == nil -> 404 branch was dead and the missing id fell into "failed to look up exchange record". That is also why a session approve of a missing exchange returned 500 although openapi.yaml says 404.

Decision: one answer, not a 401

The issue asks whether to answer 401 before the lookup, as the purchase routes do (#435/#584/#589/#594). That does not fit here: on the purchase routes the token never authorizes on its own, so every caller needs a session anyway. On the RI exchange routes the token alone approves or rejects (the pure email-client flow, transitioned_by NULL), so a 401 would break the documented flow. Instead, token mode now answers every caller who does not hold the token the same way: 403 with the same body for a wrong token, a record with no token, and a missing id. That keeps #587's token mapping as it is (#1172: 403 invalid, then 410 expired once the token matches, then the transition's typed 409/410, all before the 4-eyes check).

Fix (commit after `fea837d`)

checkExchangeToken (from fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172) takes a possibly nil record: a nil record, or one without a token, gets the same 403 as a wrong token. The separate "does not support approval/rejection" text is gone; ApprovalTokenMatches never matches an empty stored digest, so it needed no extra branch.
PostgresStore.GetRIExchangeRecord wraps config.ErrNotFound for no row (documented on StoreInterface). A new Handler.getRIExchangeRecord maps ErrNotFound, or a nil record from the other store implementations, to "missing". Every other store error stays a 500.
The session approve path (fetchAndAuthorizeRIExchange) uses the same helper, so a missing exchange there is the documented 404 on Postgres instead of 500. Session callers have already passed CSRF, the scope check and the approve-* RBAC pre-check, so this 404 is not open to unauthenticated callers.
openapi.yaml: approve documents 404 as session mode only, and that token mode answers an unknown id like a wrong token. 404 is removed from reject, which is token-only. The 403 descriptions say the three cases share one answer.

No DB writes or schema changes. The only store change is the error a read returns for a missing row, so there is no new concurrency surface. #1172's race test (TestPostgresStoreDB_TransitionRIExchangeStatus_ConcurrentLosersGetNotInExpectedStatus) still passes.

Verification

All local, on macOS, run with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. No cloud calls: the integration test only rejects, and the approve probes are all refused before execution.

Regression tests (fail before, pass after):

internal/api/ri_exchange_approve_reject_status_test.go (unit, through HandleRequest with mock store and auth):
- TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken: for approve and reject, and for no session, an invalid session, and a session without approve permission (which falls through to the token), it compares status, body and headers of a missing id (nil record, and a wrapped ErrNotFound as Postgres reports it) against a pending record with a wrong token, a record with no token, and an expired record with a wrong token. Before the fix all 36 subtests failed; after, all pass.
- The existing status tables gain riRecMissingNotFound (missing as Postgres reports it): token mode gives 403, and session approve gives 404 (500 before).
internal/api/ri_exchange_token_oracle_integration_test.go (-tags integration, real Postgres 16 via testcontainers, real PostgresStore, through HandleRequest):
- TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken: a missing id against pending, no-token and expired rows, for approve and reject. Before: missing id 500 Internal server error vs existing 403. After: identical 403 responses, rows left pending, plus a control where the real token holder's reject still returns 200.
- TestApproveRIExchange_Integration_SessionMissingIs404: before 500, after 404 exchange record not found.
internal/config/store_postgres_pgxmock_test.go: TestPGXMock_GetRIExchangeRecord_NotFound (no row gives ErrorIs ErrNotFound) and ..._QueryErrorIsNotErrNotFound (a failed query is not ErrNotFound).
Updated four older unit tests that pinned the 404 and "does not support" texts (coverage_extras_test.go, handler_ri_exchange_test.go, router_handlers_test.go).

Commands run:

go build ./...
go test ./internal/api/ ./internal/config/ ./internal/mocks/ -count=1                       # ok
go test -race ./internal/api/ -count=1 -run RIExchange                                     # ok
go test -tags integration ./internal/api/ -count=1 -run RIExchange                         # ok (real Postgres)
go test -tags integration ./internal/config/ -count=1 -run RIExchange                      # ok (real Postgres, incl. #1172's race test)
go vet ./internal/api/ ./internal/config/ && go vet -tags integration ./internal/api/ ./internal/config/
golangci-lint v2.10.1 run --build-tags integration ./internal/api/... ./internal/config/...   # 0 issues (also without the tag)
gocyclo -over 10 -ignore "_test\.go" internal/api/handler_ri_exchange.go internal/config/   # silent
bash scripts/gosec-hook.sh <changed non-test files>                                         # clean
git-secrets --scan (isolated temp repo with .gitallowed) + scripts/test-git-secrets-allowlist.sh   # clean, 42 PASS

Fail-before evidence came from reverting only internal/api/handler_ri_exchange.go and rerunning the same tests.

Not covered: no deployed or real-AWS run. Timing still differs slightly between a missing id (one indexed query) and an existing one (query plus a SHA-256 and a constant-time compare). That is microseconds against network and DB jitter, so it is not addressed here.

Out of scope (noted, not changed)

Session approve-own status leak: in fetchAndAuthorizeRIExchange, a signed-in session holding approve-own gets 409 cannot be approved (status=...) for a non-pending exchange before the creator check runs, so it learns another user's exchange status. That is the session path for authenticated callers, not the unauthenticated missing-vs-existing oracle in sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601, so it is left for a separate issue.
The session approve path for an authorized session still answers 404 for a missing exchange and 409 for a non-pending one, as documented. Those callers have passed RBAC and the deployment-account scope check.

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

Closes #601

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Token-based approval and rejection requests now return the same 403 response for unknown exchanges as for missing or invalid tokens.
- Session-authenticated approval still returns 404 when an exchange is not found.
Documentation
- Updated API documentation to clarify the response codes for token-based and session-based approval and rejection requests.

…and RI exchange Approve and cancel already answer an emailed-token action with 403 for an invalid token, 410 for an expired one and 409 for a wrong status. Revoke answered an expired link with 409, and read the expiry before matching the token. The RI exchange approve and reject links never checked expires_at: the transition CAS refused an expired or already-moved record with a plain error, so both came back as 500. - validateRevokeToken reuses purchase.ValidateApprovalToken: token matched first, then 410 for an expired link, then the revocation window (403). - checkExchangeToken gives RI approve and reject the same 403/410 order. - TransitionRIExchangeStatus wraps ErrRIExchangeExpired and ErrRIExchangeNotInExpectedStatus; riExchangeTransitionError maps them to 410 and 409 on the approve (token and session) and reject paths. - openapi.yaml documents the 410s and the 409 race. Closes LeanerCloud#587 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…and reject The RI exchange approve and reject email links are open to callers with no session, since the token alone authorizes them. They answered a wrong token on an existing exchange with 403 but a missing exchange id with 404 (with the in-memory mocks) or 500 (on Postgres, whose GetRIExchangeRecord returned a bare error for no row), so anyone could probe which exchange ids exist. A 401 before the lookup, the purchase routes' fix (LeanerCloud#435), does not fit: it would break the token-only email flow. Instead token mode now gives one answer to every caller who does not hold the token: - checkExchangeToken treats a missing record and a record without a token like a wrong one: 403 "invalid approval token" / "invalid rejection token". The LeanerCloud#587 order is unchanged: the token match comes first, then 410 for an expired exchange, then the transition's 409/410. - PostgresStore.GetRIExchangeRecord wraps ErrNotFound for no row. getRIExchangeRecord maps it (or a nil record) to "missing", so the session approve path answers a missing exchange with the documented 404 instead of 500. Other store errors stay 500. - openapi.yaml: 404 is session mode only on approve and is gone from reject; the 403s say an unknown id answers like a wrong token. Closes LeanerCloud#601 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-20T01:07:49Z

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: LeanerCloud/cloud-commitments-platform
Branch: main
File: .coderabbit.yaml
Commit: 89e1140
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: c741ce52-3f9c-4964-8dd5-c431fbb24be8

Commits

Reviewing files that changed from the base of the PR and between 89e1140 and 6903a8e.

Files selected for processing: 10

internal/api/coverage_extras_test.go
internal/api/handler_ri_exchange.go
internal/api/handler_ri_exchange_test.go
internal/api/openapi.yaml
internal/api/ri_exchange_approve_reject_status_test.go
internal/api/ri_exchange_token_oracle_integration_test.go
internal/api/router_handlers_test.go
internal/config/interfaces.go
internal/config/store_postgres.go
internal/config/store_postgres_pgxmock_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

RI-exchange token approval and rejection return the same 403 invalid-token response for missing records and invalid or absent tokens. Session-authorized approval retains a 404 response for a missing record. The PostgreSQL store wraps missing-record errors with config.ErrNotFound, and the API documentation and tests reflect these responses.

Changes: RI exchange token responses

Layer / File(s)	Summary
Missing-record lookup contract `internal/config/interfaces.go`, `internal/config/store_postgres.go`, `internal/config/store_postgres_pgxmock_test.go`	`GetRIExchangeRecord` wraps `ErrNotFound` when a record is missing. Tests distinguish this from query failures.
Token route lookup and validation `internal/api/handler_ri_exchange.go`	A shared lookup helper maps `ErrNotFound` to a nil record. Token validation returns 403 for a missing record or token mismatch. Session-authorized lookup retains its 404 behavior.
Response contract and regression coverage `internal/api/openapi.yaml`, `internal/api/ri_exchangetest.go`, `internal/api/coverage_extras_test.go`, `internal/api/router_handlers_test.go`	Documentation and tests cover matching responses for missing records and invalid tokens, including session-mode approval and requests that leave records unchanged.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: cristim

Merge Risk: Minimal · up to 6903a

The documented token responses appear consistent for missing exchanges and invalid tokens. No merge-blocking issue is established.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 9 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely summarizes the main API change: missing RI exchanges receive the same response as wrong tokens for approve and reject operations.
Linked Issues check		`#601` is the only active directly linked issue. The handler now gives token-mode approve and reject requests one 403 response for a missing record, a record without a token, and a wrong token. The impl…
Out of Scope Changes check		The changed handler logic, Postgres ErrNotFound mapping, interface documentation, OpenAPI updates, and unit, mock-store, and integration tests support `#601`'s response-consistency objective. The sessio…

Full details: Linked Issues check

Explanation

#601 is the only active directly linked issue. The handler now gives token-mode approve and reject requests one 403 response for a missing record, a record without a token, and a wrong token. The implementation preserves token-only authorization. The session approve path maps a missing record to 404. The added HandleRequest regression tests compare missing and wrong-token responses, including status, body, and headers. The OpenAPI definitions document the shared 403 response and the session-only 404. #587 is closed and supplies historical context only.

Full details: Out of Scope Changes check

Explanation

The changed handler logic, Postgres ErrNotFound mapping, interface documentation, OpenAPI updates, and unit, mock-store, and integration tests support #601's response-consistency objective. The session approve 404 fix prevents a related Postgres lookup error from producing 500. No unrelated implementation, schema change, or database write is shown.

Full details: Docstring Coverage

Explanation

Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 9 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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 · 2026-10-20T20:26:38Z

Independent review 2 (claude-fleet, cold): APPROVE 8999bc2

Scope: the head is stacked on #1172 (fea837d is not on origin/main), so the full merge-base..head diff was reviewed with the focus on fea837d..8999bc2. The head merges cleanly onto current origin/main. No migration.

Findings (none blocking; related low-severity notes were shared privately with the maintainers per SECURITY.md):

[non-blocking] internal/config/store_postgres.go:3432 (diagnoseTransitionFailure) - This still returns a bare "ri exchange record not found" when the row disappears between the GetRIExchangeRecord read and the transition. That case gets a 500 rather than the new ErrNotFound contract. It needs a concurrent delete of a pending row, so the window is theoretical.
[non-blocking] getRIExchangeRecord repeats the getExecutionOr404 pattern (handler_purchases.go:1786) for a different type. Acceptable.

Against #601: on Postgres, a missing id used to answer 500, because a bare error made the nil branch at handler.go dead, while an existing id answered 403. The fix maps ErrNotFound to nil at a single choke point, and checkExchangeToken treats a nil record as a wrong token. It is complete for both token routes.

Attacked:

Blast radius: GetRIExchangeRecord has one non-test caller (the new helper, handler_ri_exchange.go:2607) and two implementations (PostgresStore and MockConfigStore). The changed 404 and "does not support" texts have no frontend or email consumers.
Session fall-through: the 36-subtest oracle test compares status, body and headers. It covers no session, an invalid session, a session without approve permission, and approve-own by a non-creator.
Ordering: the token is matched before expiry, so a wrong token on an expired record gets 403. This is tested.
Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
- P1, the token-mode 404 for a nil record restored: 3 tests FAIL.
- P2, the ErrNotFound mapping dropped: the approve and reject status tables and the oracle test FAIL.
- P3, the store wrap reverted: the pgxmock test and both integration tests on real Postgres FAIL.
- P4, the "does not support rejection" branch reintroduced: 3 tests FAIL.
- P5, every store error mapped to missing (hiding 500s): FAILS.
Fail-before: P1+P2+P3 together reproduce the pre-PR code, and the PR's tests fail against it.

Verified locally, on a git archive export of HEAD with no cloud credentials:

go build ./... ok. go vet on api and config, with and without -tags integration: ok. gofmt -l: clean.
go test -count=1 ./internal/api/ ./internal/config/ ./internal/mocks/: ok.
go test -tags integration -count=1 -run RIExchange ./internal/api/ ./internal/config/ with testcontainers: ok. TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken and TestApproveRIExchange_Integration_SessionMissingIs404 both ran and PASS.

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

cristim · 2026-10-20T20:47:20Z

VERDICT: APPROVE 8999bc2

Scope reviewed: the full diff origin/main...8999bc2f (fea837d from #1172 plus 8999bc2). The issue fix is 8999bc2 alone. #1172's head is still fea837d and still open, so this PR is a strict descendant of it. git merge-tree against origin/main (cb25d99) and against #1172's head is clean. When #1172 lands as a squash, rebase this branch onto main and drop fea837d. That rebase restarts the gate.

Findings (none blocking):

nit internal/api/handler_ri_exchange.go:2437-2440 - an authenticated session with approve-own and a token still gets 404 for a missing id but 403 (via fall-through to the token) for another user's exchange, and 409 with the status for a non-pending one (:2442). The caller is authenticated and RBAC-gated, and the PR body already lists the 409 status leak as out of scope. Track both in one follow-up issue so the 404 half is not lost.
nit internal/api/handler_ri_exchange.go:2659 - a missing id skips the SHA-256 and the constant-time compare. That is a gap of microseconds, smaller than DB and network jitter. The PR body discloses it. Hashing the supplied token on the nil path would close it for almost nothing; optional.
nit (process) - the largest commit on the branch is 376 changed lines (8999bc2; fea837d is 370). Both are under the 400-line cap.

Issue #601 acceptance: met. One status and one body for missing, no-token and wrong-token records in token mode. A regression test runs through HandleRequest for no session, an invalid session and a session without approve rights. openapi.yaml is updated. The PR's choice not to answer 401 is argued correctly: the token alone authorizes this route.

Attacked:

The only non-test caller of GetRIExchangeRecord is the handler (git grep). The only other implementation is MockConfigStore. No other caller depends on the old bare-error text.
ApprovalTokenMatches (internal/config/approval_token.go:34) returns false for an empty stored digest or an empty supplied token. Dropping the "does not support" branch therefore cannot let an empty token match.
Session fall-through: an out-of-scope session gets the errNotFound sentinel and falls through to the token, so missing and existing ids get the same 403. An in-scope approve-any/own session gets a ClientError 404, which does not fall through. See the nit above.
The token holder keeps useful errors: 410 when the token is expired and matches, and the transition's 409/410 (from fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172). The integration control shows a real reject returning 200.
Mutation probes (scratch export):
M1: nil record -> 404 in checkExchangeToken. 4 tests fail (MissingAnswersLikeWrongToken, Approve/Reject StatusCodes, rejectRIExchange_RecordNotFound).
M2: restore the bare "not found" error in PostgresStore.GetRIExchangeRecord. TestPGXMock_GetRIExchangeRecord_NotFound and both integration tests (MissingAnswersLikeWrongToken, SessionMissingIs404) fail.
M3: remove the ErrNotFound -> (nil, nil) mapping in getRIExchangeRecord. 3 tests fail.
M4: reinstate the distinct "does not support" 403 for an empty stored token. 4 tests fail, including TestRejectRIExchange_EmptyStoredToken.
All probes were restored.
Tests run at the head: go test -run 'RIExchange|ExchangeToken|GetRIExchangeRecord' ./internal/api/ ./internal/config/ (275 passed). go vet -tags integration clean. go test -tags integration -run RIExchange ./internal/api/ ./internal/config/ on real Postgres under heavy.lock: ok.

IAmJSD · 2026-10-26T02:44:27Z

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

Blocking:

none

Non-blocking:

Merge state: this head contains fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's commit and CONFLICTS with main the same way fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 does (internal/api/handler_purchases.go validateRevokeToken, after sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242). Merge fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 first, then merge main into this branch. The resulting head needs its own review.
internal/api/handler_ri_exchange.go (fetchAndAuthorizeRIExchange): an authenticated in-scope session with approve-own still gets 404 for a missing id but 403/409 for an existing exchange owned by someone else. The 409 also names that exchange's status. The caller is authenticated and RBAC-gated, and the body lists this as out of scope. It is worth one follow-up issue.
internal/config/store_postgres.go diagnoseTransitionFailure: a row deleted between the read and the CAS still returns a bare "not found", which is a 500 and not ErrNotFound. This needs a concurrent delete of a pending row, so it is theoretical.
Timing: a missing id skips the SHA-256 and the constant-time compare (microseconds). The body discloses this. Hashing the supplied token on the nil path would close it cheaply.

Attacked:

Oracle: in token mode, the missing id, the record with no token, the wrong token, and the wrong token on an expired record all go through one checkExchangeToken branch, with the same 403 status and body. ApprovalTokenMatches returns false for an empty stored digest or an empty supplied token, so dropping the "does not support" branch can't let an empty token through. The only non-test caller of GetRIExchangeRecord is the new getRIExchangeRecord helper, and no other caller relied on the old bare-error text. Other store errors stay 500. With no token, approve still answers CSRF 403 and reject still answers 400, the same for missing and existing ids.
Fail-before: I restored handler_ri_exchange.go and store_postgres.go from fea837d (the fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 commit) and kept this PR's tests. Then TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken, TestHandleRequest_{Approve,Reject}RIExchange_StatusCodes and TestPGXMock_GetRIExchangeRecord_NotFound failed. They passed after restoring.
Ran go build ./..., go vet (plain and -tags integration) on internal/{api,config}, go test -race -short -count=1 on internal/{api,config} (ok), golangci-lint with and without the integration tag (0 issues), gocyclo -over 10 (silent), and targeted go test -race -tags integration -run 'RIExchange|ExchangeToken|Revoke|revoke' on internal/{api,config} (ok on real Postgres, including TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken, TestApproveRIExchange_Integration_SessionMissingIs404 and fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's two transition tests). A full-package -race -tags integration ./internal/api/ run hit the 45-min timeout under host load, with only one unrelated container-startup failure.

Body-stale:

none beyond fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's (the revoke wrong-token-on-expired row is already on main via sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242).

_{Reviewer: rB24. 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.}

Conflict in validateRevokeToken: LeanerCloud#1242 (on main) moved the token match ahead of the expiry and window checks but kept 409 for an expired link; this branch routes revoke through purchase.ValidateApprovalToken for the 403/410 mapping. Kept this branch's switch, which already matches the token (constant time, against the stored digest) before the expiry and the window, and merged the two doc comments. LeanerCloud#1242's test for that order (TestHandleRequest_RevokeViaEmail_WrongTokenOnLapsedRowIsInvalid) now expects 410 for the right token on an expired link, matching approve and cancel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…e 409 text Review follow-ups on LeanerCloud#1172. riExchangeTransitionError turns the store's error, which names the exchange and its current status, into a bare ClientError, and handleRequestError never logs ClientErrors, so a lost approve/reject race or a mid-flight expiry left no server log line. Log the store error at info level in the 409 and 410 branches. Expiry is answered with 410 now, so the 409 conflict messages no longer list "expired" as a cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Conflicts in internal/api/handler_purchases.go and its test come only from LeanerCloud#1172's commit, which this branch is stacked on; this branch does not touch those files. Took LeanerCloud#1172's resolution of the same merge (c1919cc): its 403/410 validateRevokeToken switch, which keeps LeanerCloud#1242's token-first order, and LeanerCloud#1242's order test expecting 410 for an expired link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Bring in LeanerCloud#1172's new head (main merge plus its review follow-ups) so the two stacked PRs agree. The one conflict is the RI approve/reject status table: kept this branch's LeanerCloud#601 rows (missing record answers 403 like a wrong token in token mode) with LeanerCloud#1172's 409 text, which no longer lists "expired". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

IAmJSD · 2026-10-26T06:22:12Z

Merged current main (c8bb869) and #1172's new head (c85ef50) into the branch, so the two stacked PRs agree. New head: f461ebafd231412b326fcd52c758d67baee8325b.

Resolution

Main merge (64ff307). The conflicts were in internal/api/handler_purchases.go and its test. Both come only from fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's commit; this branch doesn't touch those files. I took fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's resolution of the same merge (c1919cc): its 403/410 validateRevokeToken switch, which keeps sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242's token-first order, and sec(purchase): pin the approval token in the token approve/cancel CAS; compare revoke token first #1242's order test updated to expect 410 for an expired link.
Merge of fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's head (77db31a, then f461eba). One conflict, in the RI approve/reject status table (ri_exchange_approve_reject_status_test.go). I kept this branch's sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601 rows: in token mode a missing record answers 403 like a wrong token, and the session mode stays 404. I used fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's new 409 text, which no longer lists "expired". This branch also picks up fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's new log line for a lost RI exchange transition.

Checks on the merged tree, with no cloud credentials:

go build ./..., go vet ./..., go vet -tags integration ./... and the e2e vet all pass.
golangci-lint v2.10.1 (with and without the integration tag) and gocyclo -over 10 are clean on internal/{api,config}.
go test -race -short passes on internal/{api,config}.
go test -race -tags integration passes on internal/config.
On internal/api, every test passed except TestIntegration_HistorySummary_MatchesListedRowsUnderTheCap, whose Postgres container did not start ("database system is ready" was never logged). That test is unrelated to this PR, and it passed when rerun once.
The secrets scan prints "clean; planted key flagged", and the allowlist test passes.

#1172 should merge first. If it does, this PR's diff shrinks to its own commit.

IAmJSD · 2026-10-26T07:01:24Z

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

Blocking:

none

Non-blocking:

Merge order: this head contains fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 (fea837d + ff88e48, merged in at c85ef50), so fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 should merge first. I reviewed this PR's own delta as git diff c85ef50d..f461ebaf (fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's current head to this head). I reviewed fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 itself separately (APPROVE c85ef50).
internal/api/handler_ri_exchange.go fetchAndAuthorizeRIExchange: an authenticated, in-scope session holding approve-own still gets 404 for a missing id but 403 (it falls through to the token) or 409-with-status for someone else's exchange. Only callers who are signed in, in scope and approve-capable can see this. The body leaves it out of scope. It came up in earlier reviews too, and I agree it is not sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601's unauthenticated oracle.
Timing: a missing id skips the SHA-256 and the constant-time compare. The difference is microseconds and the body discloses it. Hashing the supplied token on the nil path would close it cheaply.

Attacked:

Oracle: in token mode, a missing id (nil record, or wrapped ErrNotFound from Postgres), a record with no token, a wrong token, and a wrong token on an expired record all hit the single record == nil || !ApprovalTokenMatches branch, with the same status and body. ApprovalTokenMatches is false for an empty stored digest, so dropping the "does not support" branch cannot let an empty token match. getRIExchangeRecord maps only ErrNotFound to missing, and every other store error stays a 500. The handler is the only non-test caller of GetRIExchangeRecord, so no other caller depended on the old bare error. An out-of-scope session gets errNotFound before the lookup.
Ran in a scratch worktree at f461eba: go build ./...; go vet with and without -tags integration on internal/{api,config}; go test -race -short -count=1 on internal/{api,config,mocks} (ok); go test -race -tags integration -count=1 -run 'RIExchange|Revoke|TokenActions' on internal/{config,api} (ok, real Postgres via testcontainers, including the new oracle test); golangci-lint (0 issues). git merge-tree against origin/main 8368330: clean.
Fail-before 1: I reverse-applied this PR's handler_ri_exchange.go delta. TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken then fails in all 36 subtests, both *_StatusCodes tables fail, and both TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken and TestApproveRIExchange_Integration_SessionMissingIs404 fail on real Postgres.
Fail-before 2: I reverse-applied only the store_postgres.go delta. Both integration tests fail, and so does TestPGXMock_GetRIExchangeRecord_NotFound. Restored afterwards.
After writing this verdict I skimmed the earlier reviews. The approve-own and timing points match cristim's earlier nits.

Body-stale:

The stacked-review link ("Review only the commits after fea837d: fea837d...8999bc2") points at an old head. The branch has since merged main and fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's head (c85ef50), so this PR's delta is now c85ef50..f461eba.

_{Reviewer: rC5. 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 · 2026-10-26T07:17:36Z

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

Blocking:

none

Non-blocking:

Merge order: this PR contains all of fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 (its head c85ef50d is an ancestor here), so the two must land in that order. Merging this one first would also merge fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's change without that PR being marked merged.
Not a 401: the issue's acceptance asks for an owner decision on "401 before the lookup vs one answer". The PR answers every non-holder with the same 403 and explains why a 401 would break the pure email-client flow. I agree, but the acting owner should confirm.
fetchAndAuthorizeRIExchange: an in-scope session with only approve-own, sending a wrong token, still sees 404 for a missing id and 403 for someone else's existing exchange (the session error is 404, so it doesn't fall through to the token). That caller is authenticated and scope-checked, so it isn't the unauthenticated oracle from sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601. The PR body lists the related approve-own 409 status leak as out of scope. A follow-up issue covering both would be good.
TransitionRIExchangeStatus and diagnoseTransitionFailure still return a plain "not found" error, so an exchange deleted between the lookup and the CAS answers 500. This is unreachable in practice because no code deletes pending rows.

Attacked:

Reviewed the diff against fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's head (git diff c85ef50d..HEAD), which is this PR's own change. GetRIExchangeRecord has exactly one caller (the new getRIExchangeRecord helper), so wrapping ErrNotFound changes nothing elsewhere. A nil record and an empty stored digest both go through the same ApprovalTokenMatches false branch, and that function returns false for an empty digest. Status, body and headers are identical for missing, no-token, wrong-token and expired-wrong-token. Real store errors still answer 500, and the pgxmock test pins that.
Ran go build ./..., go vet with and without -tags integration on internal/api and internal/config, and go test -race -short -count=1 ./internal/api/ ./internal/config/ (ok). Ran go test -race -tags integration -count=1 -run RIExchange ./internal/api/ ./internal/config/ (ok). The two new API integration tests ran on testcontainers Postgres, as did fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's race test.
Fail-before: restored fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's handler_ri_exchange.go and store_postgres.go and kept the new tests. All 36 MissingAnswersLikeWrongToken subtests failed, 48 FAIL lines in total including the StatusCodes additions. Both integration tests and TestPGXMock_GetRIExchangeRecord_NotFound failed too. With the fix restored, all pass.

Body-stale:

The stacking note's compare link stops at 8999bc2f. The branch has since merged fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's later commit ff88e481 and main. The note is still right that only the commits after fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 belong to this PR.

_{Reviewer: rC6. 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 · 2026-10-26T07:45:56Z

Local verification at f461ebafd231412b326fcd52c758d67baee8325b merged onto main (f3498548), together with #1172, #1175, #1191, #1257, #1259 in that order (combined tree 2be2745a, 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.

LeanerCloud#1172, which this branch carries, merged as squash 89e1140. For both conflicted files (internal/api/handler_ri_exchange.go and ri_exchange_approve_reject_status_test.go), main's content is byte-identical to LeanerCloud#1172's head c85ef50 and hasn't changed since, so the conflicts are only this branch's own additions on top: kept this branch's side. The diff against main is now only LeanerCloud#1175's own change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

IAmJSD · 2026-10-26T07:56:38Z

Merged origin/main (now 6903a8e7) after #1172 squash-merged. For both conflicted files, main's content is byte-identical to #1172's head and unchanged since, so I kept this branch's side; the diff against main is now only #1175's own change. Build, vet, race unit tests (api, config), the RI exchange integration tests and golangci-lint pass; secrets scan clean. Needs fresh reviews at this head.

IAmJSD · 2026-10-26T08:14:55Z

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

Blocking:

none

Non-blocking:

internal/api/handler_ri_exchange.go:2554 (fetchAndAuthorizeRIExchange): a missing in-scope record returns a fresh NewClientError(404, ...), not the errNotFound sentinel, so fallsThroughToToken does not fall through for it. A signed-in, in-scope session that holds approve-own and sends a wrong token still sees 404 for a missing id and 403 invalid approval token for someone else's exchange (403 from the creator check, then the token fall-through). This needs an authenticated user with approve rights in the deployment scope, and the PR body keeps it out of scope on purpose, together with the approve-own 409 status leak. Worth adding to the follow-up issue the body promises, so both session-path leaks are tracked in one place.
(Raised in an earlier review, and I agree.) internal/config/store_postgres.go diagnoseTransitionFailure still returns a bare "ri exchange record not found" when the row is deleted between the read and the transition. The caller answers that with 500, not ErrNotFound. It only happens if a pending row is deleted concurrently, so the risk is theoretical.
The response time still differs a little between a missing id and an existing one (the PR body says so). I agree it is not worth fixing here.

Attacked:

Checked every caller of GetRIExchangeRecord. The only non-test caller is the new Handler.getRIExchangeRecord, so wrapping ErrNotFound in PostgresStore changes no other route. The pgxmock test shows a failed query does not become ErrNotFound, so it still answers 500. ApprovalTokenMatches returns false for an empty stored digest or an empty supplied token, so dropping the "does not support" branch opens no bypass. A missing token is still 400 before the lookup on both routes, so there is no oracle there. A matching token on an expired exchange still gets 410, so only the token holder learns the exchange expired. The CSRF no-fall-through guard and the 4-eyes order are unchanged.
Fail-before: I reverted handler_ri_exchange.go and store_postgres.go to the merge-base. -run 'MissingAnswersLikeWrongToken|RIExchange_StatusCodes' gave 48 FAIL lines, 37 of them from the new oracle test (36 subtests plus the parent), and TestPGXMock_GetRIExchangeRecord_NotFound failed. With only the handler reverted, on real Postgres, the oracle test showed a missing id 500 Internal server error against 403 invalid approval token for an existing one, and SessionMissingIs404 got 500 instead of 404. Both files restored afterwards.
Commands: go build ./...; go vet with and without -tags integration on internal/api and internal/config; go test -race -short -count=1 on internal/api, internal/config and internal/mocks (all ok); go test -race -tags integration -count=1 -run 'RIExchange|Oracle' on internal/api and internal/config (testcontainers Postgres, ok, including fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172's concurrent-loser test); golangci-lint v2.10.1 with and without the integration tag (0 issues); gocyclo -over 10 on the changed non-test files (silent).
Local merge of current origin/main into the head: no conflicts. Build, vet and short tests for api and config pass, and main has added no new caller of GetRIExchangeRecord and no new code that depends on the old "does not support" text.
openapi.yaml now matches the code. Reject documents no 404, and approve documents 404 as session mode only.

Body-stale:

The "Stacked on fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 ... review only the commits after fea837d ... once fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 merges the diff will shrink" note is out of date: fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 merged on 2026-10-26 and main has been merged in, so origin/main...head is now only sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601's change (10 files, +323/-53).

_{Reviewer: rC8. 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 · 2026-10-26T08:14:55Z

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

Blocking:

none

Non-blocking:

internal/api/handler_ri_exchange.go:2553 (fetchAndAuthorizeRIExchange): a session that holds approve-own/approve-any and also sends a token still tells a missing id (404 "exchange record not found", which is not the errNotFound sentinel, so it doesn't fall through) apart from an existing exchange it can't approve (403 falls through to the token, which then gives 403 invalid token). An out-of-scope deployment returns errNotFound, which does fall through, so it answers 403. Only signed-in, RBAC-checked callers can see any of this, so it's outside sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601's unauthenticated oracle and the PR body says so. Worth folding into the follow-up issue the body proposes for the approve-own status leak.
The 403-not-401 call is a judgement on an issue that asked for a decision ("decide whether..."). I agree with it: the token alone authorizes these routes, so a 401 before the lookup would break the email-client flow, where transitioned_by is NULL. The acting owner may still want to sign off on it explicitly.
internal/config/store_postgres.go:3463 (diagnoseTransitionFailure), raised in an earlier review: if the row is deleted between the read and the transition, the error is still a bare "ri exchange record not found", so the request gets 500. That only happens after the token matched, so it's not an oracle, and the window needs a concurrent delete of a pending row. Cosmetic.

Attacked:

I checked that GetRIExchangeRecord has exactly one production caller (getRIExchangeRecord). So switching the Postgres no-row error to a wrapped config.ErrNotFound changes no other path, and no test or frontend code pins the old "not found" or "does not support" texts. ApprovalTokenMatches returns false for an empty stored digest, so dropping the no-token branch can't let a tokenless record through. Real store errors still give 500 (the dbError case plus the pgxmock QueryError test). Session + CSRF + RBAC ordering in the approve dispatch is unchanged, and so is the CSRF no-fall-through.
Ran: go build ./...; go vet on api/config with and without -tags integration; go vet -tags e2e in tests/e2e; go test -race -short -count=1 on internal/{api,config,mocks,server}, all ok; go test -race -tags integration -count=1 -run RIExchange on internal/api and internal/config against real Postgres via testcontainers, ok, and -v shows the two new integration tests ran rather than skipped; golangci-lint v2.10.1 with and without --build-tags integration gives 0 issues; gocyclo -over 10 on the changed files is silent.
Fail-before: I reverted handler_ri_exchange.go and store_postgres.go to the merge-base. 48 unit subtests failed, including all of MissingAnswersLikeWrongToken, and TestPGXMock_GetRIExchangeRecord_NotFound failed. With only the handler reverted, the integration oracle test showed missing id 500 vs existing 403 ("invalid approval token" / "this exchange record does not support approval"), and the session-missing test returned 500 instead of 404. Restored afterwards.

Body-stale:

The "Stacked on fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172" header, and its "review only commits after fea837d" compare link to 8999bc2, are out of date. fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 has merged and main has been merged in, so origin/main...6903a8e7 is now only sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601's change.

_{Reviewer: rC7. 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 · 2026-10-26T08:20:04Z

Local verification at 6903a8e761c6934b5e1075ff56d7780623576b07 merged onto main (5ddcae7d), together with #1175 in that order (combined tree 1b9ce205, 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 · 2026-10-26T08:20:09Z

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 6903a8e7 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 6903a8e761c6934b5e1075ff56d7780623576b07.

cristim · 2026-10-26T08:24:40Z

Independent review 1, delta since 8999bc2 (cold, fresh context):

VERDICT: APPROVE 6903a8e
Delta review since 8999bc2 (PR OPEN). New own commit ff88e48; merges 64ff307, 77db31a, c85ef50, f461eba, 6903a8e. ff88e48 has since landed on main via #1172 (89e1140), so the PR diff vs origin/main no longer contains it. The diff is the #601 change only (10 files, +323/-53).

Findings:

nit internal/api/handler_ri_exchange.go:2786-2788 - the comment says the store error "names the exchange and its current status". The 409 branch's error (internal/config/store_postgres.go:3471) carries only the expected and current status, not the id. So the "lost to a concurrent writer" log line cannot be tied to an exchange. Fix: log the id beside err (the caller has it), or reword the comment. The comment arrived through main (fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172), so this can be a follow-up.
nit internal/api/handler_ri_exchange.go:2791,2794 - no test asserts the two new Infof lines. Mutation P3 (both lines deleted) survived. They are not on the money path; non-blocking.
nit (pre-existing, not in diff) internal/config/store_postgres.go:3461-3462 - if the row is deleted between the token check and the transition, diagnoseTransitionFailure returns an unwrapped "not found" error, and the response is a 500. Only a caller holding the valid token can reach this, and it is a narrow race, so it is not an oracle for non-holders.

Conflict resolutions (git show --remerge-diff):

64ff307: handler_purchases.go validateRevokeToken resolved to the purchase.ValidateApprovalToken switch with the merged comment, and handler_purchases_test.go kept the commented hash fixture. Both match what main now has, and neither file appears in the final PR diff.
77db31a / 6903a8e: the status-test tables kept the sec(api): RI-exchange approve/reject distinguish a missing record (404) from an existing one (403 wrong token) for unauthenticated callers #601 rows (403 "invalid token" for missing / ErrNotFound / no-token records, and 404 only in session mode) together with ff88e48's new 409 text. In 6903a8e, handler_ri_exchange.go dropped main's duplicate checkExchangeToken call, dropped main's record == nil -> 404 in rejectRIExchange, and kept the record == nil || form of checkExchangeToken. That is correct: each path makes one token check, and nil is handled inside it.
c85ef50 / f461eba: clean, no remerge diff.

Oracle check of the whole PR, from a caller that does not hold the token:

missing id (nil record, or a wrapped ErrNotFound from PostgresStore), existing record with a wrong token, record with no token, expired record with a wrong token, and already-processed record with a wrong token all return 403 with an identical body and headers. checkExchangeToken (handler_ri_exchange.go:2773) runs before any expiry or status read. The unit table TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken compares StatusCode, Body and Headers across no session, invalid session and a session without approve rights, for both approve and reject. The integration test against real Postgres does the same and passes.
410 (expired) and 409 (lost race / processed) are reachable only after a token match. Lost-race 409 and pre-check 409 share one message constant. Since ff88e48 the 409 text no longer says "expired", so 409 and 410 no longer overlap.
Timing: both paths make exactly one SELECT. A missing record skips the SHA-256 plus constant-time compare in ApprovalTokenMatches (record == nil || short-circuits). That is sub-microsecond, well below DB and network jitter, so I see no practical timing oracle. No extra DB call exists on either branch for non-holders. diagnoseTransitionFailure's second query runs only after a valid token.
Logs: the new Infof lines print the store error, which holds the exchange id and status strings. They never contain the raw token or its digest, and only token holders can trigger them, so callers without a token cannot use them to flood the logs.
Session mode still answers 404 for a missing id. Only an authenticated, in-scope approver can see that, and openapi.yaml documents it.
GetRIExchangeRecord now wraps ErrNotFound. Its only non-test caller is getRIExchangeRecord (handler_ri_exchange.go:2722), and the mock in internal/mocks still returns (nil, nil). No other consumer changes behaviour.
No migrations.

Attacked:

go test -race -count=1 ./internal/api/ ./internal/config/ : ok.
go test -tags integration -run TestRIExchangeTokenRoutes_Integration ./internal/api/ (real Postgres container, under heavy.lock): 8/8 subtests pass.
P1, getRIExchangeRecord no longer maps ErrNotFound to nil: 3 tests FAIL (approve and reject status tables, oracle table). Restored, cmp identical.
P2, checkExchangeToken returns 404 for a nil record again: 5 tests FAIL. Restored, cmp identical.
P3, both new transition log lines deleted: survived (nit above). Restored, cmp identical.

None. I found no way for a caller without the token to tell a missing id, a wrong token, expiry or a lost race apart from the HTTP status, body or headers.

cristim · 2026-10-26T08:45:49Z

Independent review 2 (claude-fleet, cold): APPROVE at 6903a8e

Context: PR #1175 has already landed on main as the squash 2a11540 ("... (#1175)"). git merge-tree --write-tree origin/main 6903a8e gives tree 3527dcb, and that tree has an empty diff against origin/main. This review is post-merge, so any fix is a follow-up PR.

Blocking findings: none.

Non-blocking findings:

internal/api/ri_exchange_approve_reject_status_test.go:293-300: the full-response oracle test has no probe for an already-processed record with a wrong token, and none for a malformed ID. I read the code instead: the token check at handler_ri_exchange.go:2774 runs before any status use, and validateUUID runs before the lookup. So the behaviour is right, but nothing pins it.
internal/config/approval_token.go:38 (unchanged): no test pins the constant-time compare. Probe M3 survived.
internal/email/templates.go:316-317,374-375 (pre-existing, untouched): emails still link a GET to /api/ri-exchange/... The deep-link comment (frontend/src/riexchange-deeplink.ts:6-8) says emails should target the SPA paths instead. The answer to that GET does not depend on the ID.
golangci-lint 2.11.4 reports one prealloc in internal/config/store_postgres_history_page_integration_test.go:141. That file is not touched by this PR, and base reports the same issue.

Response comparison (token mode, reject and approve; X = approval or rejection):
| case | status | body | headers | same path? |
| missing id, any token | 403 | {"error":"invalid X token"} | static set (handler.go:723) | token path |
| existing, wrong token | 403 | identical | identical | token path |
| existing, no stored token | 403 | identical | identical | token path |
| existing, right token, expired | 410 | exchange has expired | identical | only the token holder sees this |
| existing, right token, processed or canceled | 409 | conflict message | identical | only the token holder sees this |
| right token, success | 200 | {"status":"canceled"} (reject) or execution result | identical | |
| malformed id | 400 | invalid ID format | identical | no lookup |
| another tenant's exchange | 403 | same as wrong token | identical | the token alone authorizes; no tenant check on this path |
| DB error during lookup | 500 | Internal server error | identical | does not depend on the ID |

The other reviewer's three claims:

ff88e48 is on main via fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172: verified. git diff ff88e481 89e1140 on the two files it touches is empty.
The merges are clean: verified (see the merge check below).
No unauthenticated response oracle: verified.

Should a DB error look like a wrong token? No. A primary-key lookup fails for reasons unrelated to the ID (connection, timeout), so a 500 reveals nothing about which IDs exist. Hiding it as a 403 would mask outages, against fail-loud. Probe M6 shows the tests enforce this.

Entry points:

POST /api/ri-exchange/approve/{id} and /reject/{id} (router.go:322-323), rate limited before the lookup.
Session approve via approveRIExchangeViaSession.
Frontend riexchange-deeplink.ts and riexchange.ts:2093 show the backend message as-is, so they add no oracle. Frontend is untouched.
No MCP or internal/server path approves or rejects.
The mock store (mocks/stores.go:964) is the only other store implementation. GetRIExchangeRecord has no callers outside the new helper (handler_ri_exchange.go:2722).

Merge resolution check:

Read the remerge-diff of every merge: 64ff307, 77db31a, c1919cc, f461eba and 6903a8e show hand resolutions; c85ef50 is clean.
Each resolution keeps this PR's side, or main's text for fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172 and chore(config): tie suppression slug aliases to the library service constants #1425 / sec(purchase): token approve/cancel race on rotation, and revoke reveals expiry to a wrong token #1190. Nothing was dropped or duplicated.
The net PR diff (89e1140..6903a8e) has the same +/- lines as the original commit 8999bc2.

Probes (head export; each restored, cmp OK, the three source files also match git show):

M1, missing record gets a distinct message: 29 failures.
M2, distinct status (404): 35 failures.
M3, non-constant-time compare: 0 failures (gap; not testable functionally).
M5, leak the exchange ID in the reject 403 for existing records: 15 failures.
M6, DB error treated as missing: 5 failures.
M7, drop the ErrNotFound mapping: 18 failures.
M8, Postgres returns the old bare error: 1 failure (pgxmock).
M9, session missing record returns 403: 3 failures.
M10, separate no-token message: 13 failures.

Commands (GOTOOLCHAIN=go1.26.10, AWS config and credential files set to /dev/null):

go build ./...: OK.
go vet on api and config, with and without -tags integration: OK. gofmt -l: clean.
go test on ./internal/api, ./internal/config and ./internal/mocks: all pass.
Integration tests with -tags integration, matching RIExchange, under lockf on real Postgres via testcontainers: api passes (including both new tests), config passes.
Fail-before (head tests on a base copy): unit tests fail, including all 36 oracle subtests, the pgxmock NotFound test and the status-table rows. Integration SessionMissingIs404: expected 404, got 500. The oracle integration test, with its fixture require removed: missing ID gives 500 "Internal server error", existing gives 403.
golangci-lint, with and without the integration tag: only the pre-existing prealloc above.
git merge-tree: clean, identical to origin/main.

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

…RI exchange approve (#1454) A session holding only approve-own on purchases reached fetchAndAuthorizeRIExchange, which answered a missing exchange id with 404 and another user's non-pending exchange with 409 (naming its status) before running the ownership check. That let the caller learn, for any id, whether the exchange exists and read its status, even for exchanges it did not create: the session-path half of the oracle #1175 closed for token-only callers. Run authorizeSessionApproveRIExchange right after the lookup and before the 404/409 answers. For an approve-own session a missing record now gets the same 403 as another user's record, so it also falls through to the email token like any other ownership refusal. The admin key and approve-any sessions may approve any exchange, so they keep 404/409, as does approve-own on an exchange the caller created. A table test through HandleRequest pins one answer for a missing id and for another user's pending, completed, failed and expired exchanges, with and without a token. openapi.yaml documents the change. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

IAmJSD and others added 2 commits October 20, 2026 01:49

IAmJSD added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/bug Defect labels Oct 20, 2026

This was referenced Oct 20, 2026

fix(ri-exchange): exclude convertible RIs with no pricing from cost math #1191

Merged

fix(ri-exchange): answer the go exchange refusals with 422 instead of 500 #1212

Merged

chore(test): point Azure RI-exchange credential test comments at cli#932 #1225

Merged

This was referenced Oct 22, 2026

fix(history): set cloud_account_id on every purchase_history insert #1244

Merged

refactor(ri-exchange): share the convertible RI monthly cost formula #1260

Merged

sec(purchase): refuse a capped purchase whose price is not provably USD #1296

Merged

IAmJSD and others added 4 commits October 26, 2026 04:30

IAmJSD added 2 commits October 26, 2026 05:38

Merge remote-tracking branch 'origin/main' into fix/issue-587

c85ef50

Merge branch 'fix/issue-587' into fix/issue-601

f461eba

IAmJSD mentioned this pull request Oct 26, 2026

fix(api): answer expired and lost-race token actions alike on revoke and RI exchange #1172

Merged

IAmJSD marked this pull request as ready for review October 26, 2026 07:45

IAmJSD merged commit 2a11540 into LeanerCloud:main Oct 26, 2026
41 checks passed

IAmJSD mentioned this pull request Oct 26, 2026

fix(api): check approve-own ownership before existence and status on RI exchange approve #1454

Merged

Conversation

IAmJSD commented Oct 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Decision: one answer, not a 401

Fix (commit after fea837d)

Verification

Out of scope (noted, not changed)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Oct 20, 2026

Uh oh!

cristim commented Oct 20, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

Uh oh!

IAmJSD commented Oct 26, 2026

Uh oh!

cristim commented Oct 26, 2026

Uh oh!

cristim commented Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IAmJSD commented Oct 20, 2026 •

edited by coderabbitai Bot

Loading

Fix (commit after `fea837d`)

coderabbitai Bot commented Oct 20, 2026 •

edited

Loading