Repository navigation
sec(api): enforce CSRF on the RI-exchange session-authed approve path - #1796
Conversation
approveRIExchangeViaSession had no CSRF check at all. The endpoint is
AuthPublic, so the generic middleware CSRF gate never runs for it
(validateSecurityContext returns before requiresCSRFValidation is
reached), same as purchases approve/cancel -- but unlike purchases,
nothing enforced CSRF inside the handler either. A comment at
middleware.go:288-289 claimed parity with the purchases path; it
didn't hold.
Confirmed live by execution with a purchases control in the same
probe run (not committed): same shape, valid session, zero CSRF
headers -- purchases correctly refused ("CSRF validation failed"),
RI-exchange transitioned the record from pending to processing with
no CSRF token at all. Filed as #1757.
Adds h.validateCSRF(ctx, req) at the top of approveRIExchangeViaSession,
mirroring approvePurchaseViaSession / cancelPurchaseViaSession exactly
(including the explanatory comment about AuthPublic routes).
Adds TestApproveRIExchangeViaSession_RequiresCSRF and its PassesCSRF
companion, both driving the real handler.approveRIExchange entry
point (not requiresCSRFValidation in isolation). Fixes four existing
tests (TestApproveRIExchange_SessionAdmin, both subtests of
TestApproveRIExchange_SessionApproveOwn, TestApproveRIExchange_
SessionActorStamped) that never supplied a CSRF token and would now
fail against the fix.
Corrects the middleware.go:288-289 comment, which asserted parity
that didn't exist and is why this survived -- the next reader now
sees where CSRF is actually enforced for this path.
Adds a scope-limit comment to TestRequiresCSRFValidation_
TokenBasedPathsWithSession (kept, not deleted: it's the only test
pinning requiresCSRFValidation's own per-path table, which would
matter again if any of these routes stopped being AuthPublic) -- it
calls requiresCSRFValidation directly, bypassing the isPublicEndpoint
gate that keeps the real dispatch from ever consulting it for these
paths, so a passing assertion there was never proof of end-to-end
enforcement, and was misread as exactly that here.
Systemic check: enumerated all 26 AuthPublic routes in router.go and
read every handler with a session-authed branch. approvePurchase /
cancelPurchase (protected via validateCSRF), revokeViaEmailToken
(protected on its only session-authed mutating path), rejectRIExchange
(no session-authed path at all -- unconditionally token-gated),
unsubscribeHandler (no session concept -- pure signed-token, RFC 8058),
submitRegistrationHandler (no session concept -- public submission,
separate AuthAdmin approval step). approveRIExchange was the only gap.
Fixes #1757.
TestApproveRIExchangeViaSession_RequiresCSRF only mocked ValidateCSRFToken; under mutation (CSRF guard removed) it failed via a panic on the first unmocked call (GetRIExchangeRecord) rather than through its own AssertNotCalled assertions, which never ran. A panic on an unrelated unmocked call proves the record wasn't fetched, not that the state transition (the actual money-moving call) was prevented -- the same class of fiction-vs-mechanism gap flagged on PR #1758. Adds the full happy-path fixture (.Maybe(), never meant to be consumed under the fix) so a reintroduced bug can run all the way to TransitionRIExchangeStatus instead of crashing early, and reorders the state-changing AssertNotCalled checks ahead of the error-shape assertions so they are the primary, always-run check. Re-verified by mutation (inverse-edit revert of the CSRF guard, not git checkout): fails cleanly with "TransitionRIExchangeStatus was called 1 time(s)... expected none" and the recorded call showing the pending->processing transition -- the assertion the test needs to protect is now the one that actually fires.
cristim
added
triaged
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe session-authenticated RI exchange approval handler now validates CSRF before processing. CSRF failures cannot fall through to token approval. Tests cover rejected and successful approval paths. ChangesRI exchange approval protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SessionClient
participant approveRIExchange
participant approveRIExchangeViaSession
participant TransitionRIExchangeStatus
SessionClient->>approveRIExchange: Submit session-authenticated approval
approveRIExchange->>approveRIExchangeViaSession: Route session approval
approveRIExchangeViaSession->>approveRIExchangeViaSession: Validate CSRF token
alt CSRF validation fails
approveRIExchangeViaSession-->>approveRIExchange: Return errCSRFRejected
approveRIExchange-->>SessionClient: HTTP 403
else CSRF validation succeeds
approveRIExchangeViaSession->>TransitionRIExchangeStatus: Transition exchange status
TransitionRIExchangeStatus-->>SessionClient: Approval result
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Independent adversarial review — head
|
| Gate | Result |
|---|---|
go test ./internal/api/ -count=1 |
exit 0, ok ... 18.4s |
golangci-lint run ./... at CI-pinned v2.10.1 |
exit 0 and a genuine 0 issues. line |
gocyclo -over 10 -ignore "_test\.go" |
clean (approveRIExchangeViaSession at 9, one below the pre-commit ceiling) |
| CI on the PR | 20 passed, 0 failed; MERGEABLE / CLEAN |
Out of scope for this PR — filing separately, not blockers
/api/infois prefix-matched inisPublicEndpoint(middleware.go:22) while/api/info/deploymentis registeredAuthUser(router.go:363).validateSecurityContexttherefore skips authentication entirely for an endpoint that returns the AWS account ID and Secrets Manager ARN; it is saved only byRouter.Route's defense-in-depthAuthUsercheck. The file's own comment says exact-match exists to stop precisely this. Pre-existing, currently mitigated, and the only such violation across all 121 routes.csrfExemptWhenTokenOnly(middleware.go:311-325) is unreachable dead code.isPublicEndpointreturns athandler.go:762for all four of its prefixes, sorequiresCSRFValidationat:784is never consulted for them. It reads as live protection — the same prose-vs-code divergence that produced sec(api): RI-exchange session-authed approve has no CSRF enforcement despite a comment claiming parity with purchases #1757. This PR adds 8 lines of (accurate) comment to that dead block.- RI-exchange approval emails emit a
GETlink (internal/email/templates.go:308,366) to aPOST-only route (router.go:318), with no RI-exchange deeplink page in the frontend. Clicking Approve in the email should 404. Relevant here because it means the session+token combination F1 depends on is not producible by the current frontend, making F1 latent rather than active.
Verdict
The p0 is genuinely fixed and the fix is correct for the reachable flow. Reproduced by execution in both directions, with a control; the regression test is non-vacuous; the completeness sweep proves no sibling gap; the rebase replayed faithfully; all gates green.
F1 should be addressed before merge. It is a three-line change inside the fix's own control flow, it makes a claim the PR states in two places actually true, and it is the kind of latent conflation that becomes a live bypass the moment anything else on that path returns 403.
Delta review —
|
| # | Scenario | err |
transitioned | executed | approved_by |
|---|---|---|---|---|---|
| D1 | session, no CSRF, valid ?token= — the F1 case |
CSRF validation failed |
false | false | false |
| D2 | no session at all, valid ?token= — pure email-link |
<nil> → status:completed |
true | true | – |
| D3 | session w/ approve-own but not the creator, valid CSRF, valid ?token= |
<nil> → status:completed |
true | true | – |
| D4 | session + valid CSRF, no ?token= — dashboard |
<nil> → status:completed |
true | true | true |
| D5 | session with no approve right, valid ?token= |
<nil> → status:completed |
true | true | – |
D1 is the finding, closed. D2 answers your second question directly — the legitimate email-link holder with no session is unaffected; the narrowing did not touch that path, because with no session the dispatch never enters approveRIExchangeViaSession at all.
D3 is the one I'd have most expected to break and it doesn't. It is the case the fallthrough actually exists for: a session that holds an approve right in general but is denied at record level (approve-own, not the creator, handler_ri_exchange.go:2251-2253). That denial is a genuine 403 that must still fall through to the token flow, and it does. Narrowing on the sentinel rather than widening isPermissionDenied is what preserved it — your reasoning about blast radius holds up under execution, not just on inspection.
Your first question: the sentinel vs isPermissionDenied on every path
Measured contract:
errors.Is(sentinel, sentinel) = true
errors.Is(wrapped, sentinel) = true
errors.Is(freshEqualMsg, sentinel) = false <- the mutation is genuinely caught
isPermissionDenied(sentinel) = true <- note this
isPermissionDenied(wrapped) = false
IsClientError(sentinel) / (wrapped) = true / true (IsClientError uses errors.As, so it unwraps)
Worth stating precisely, because it differs from how the commit message frames it: the sentinel is not invisible to isPermissionDenied — isPermissionDenied(errCSRFRejected) is true. What protects it is the short-circuit ordering of || at handler_ri_exchange.go:2068. The code comment says exactly this ("Checked first so the sentinel is never reached by the 403 test below"), so the intent is recorded; I just want the property named accurately, since "unreachable" and "reached first" have different failure modes. Reordering that boolean expression silently reopens F1 — but your new test drives that exact path, so a reorder fails it. The guard is itself guarded.
Flow is contained: errCSRFRejected is produced at exactly one site (:2121) and consumed at exactly one (:2068). approveRIExchangeViaSession has exactly two callers, both in approveRIExchange — :2054 (guarded) and :2082 (returned directly, no 403 test). The other three isPermissionDenied call sites are in handler_purchases.go and the sentinel cannot reach them.
Your third question: wrapping
Safe in both directions, and doubly so. errors.Is(wrapped, sentinel) is true, so the guard holds if validateCSRF's error is ever wrapped. And isPermissionDenied is a strict (non-unwrapping) assertion by design (handler_purchases.go:2013), so a wrapped sentinel returns false there — meaning even if the errors.Is term were deleted, a wrapped sentinel would still take the !isPermissionDenied branch and return. The dangerous direction is the opposite one — returning a fresh NewClientError(403, ...) instead of the sentinel — and that is precisely what your mutation exercises.
Purchases has no sibling instance of this defect
Checked, since the fix is scoped to RI-exchange. cancelPurchase (:1066) and tryRevokeViaSession (:1329) both switch isPermissionDenied on the RBAC pre-check (authorizeSessionCancel), and the CSRF check runs after that switch has already selected the == nil branch. Revoke returns (nil, true, err) on CSRF failure — handled=true, so the caller does not fall through. RI-exchange was the only place where a ViaSession result, CSRF error included, was fed back into isPermissionDenied. No follow-up needed there.
D-F1 — blocker: ci_gate_check.sh was committed
ba3d34f36 adds ci_gate_check.sh at the repo root, tracked, mode 100755, not in .gitignore, not present on fcdc681f9. It is a local CI-reproduction scratch script, unrelated to the CSRF fix, and it hardcodes a machine- and worktree-specific path:
log="/tmp/claude/wt-1757/ci-unit-${tag}.log"— a path that does not exist on any other machine, and points at a different worktree than the one this fix was made in. Almost certainly an accidental git add. It should come out of the commit before merge; if the script is worth keeping, it belongs in .claude/scripts/ with the log path derived rather than hardcoded.
D-F2 — nit: duplicate fixture UUID
middleware_test.go:519 (the new TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken) and :577 (TestApproveRIExchangeViaSession_PassesCSRF) both use 550e8400-e29b-41d4-a716-446655440021. Harmless today — separate mock instances — but it is a copy-paste artifact and the neighbouring tests otherwise increment. One character.
Gates at ba3d34f36
| Gate | Result |
|---|---|
go test ./internal/api/ -count=1 |
exit 0, ok ... 9.3s |
golangci-lint run ./... at CI-pinned v2.10.1 |
exit 0 and a genuine 0 issues. |
gocyclo -over 10 -ignore "_test\.go" . (repo-wide) |
empty |
One thing to watch, not a finding: approveRIExchange went 8 → 9 with the added || term, and approveRIExchangeViaSession is also at 9. Two functions in this file now sit one point below the pre-commit ceiling of 10, so the next case or condition added to either will redden pre-commit --all-files.
Priority 4 — my view, since you asked me to decide
Add an end-to-end test, rename the existing one, and file the dead-code removal separately. Not "fix or delete or replace" — the first two together, and explicitly not the third as a replacement.
Add (the item that actually matters). The guard that would have caught #1757 does not exist in any form, and it is cheap: a table-driven test that drives Router.Route for each AuthPublic money route with a valid session and no CSRF header, asserting refusal. My probes are exactly this shape and took ~40 lines for the whole matrix. That converts a property currently proven only by this review into a permanent regression guard — and it guards the class, so the next AuthPublic route someone adds is covered without anyone remembering #1757. Everything else here is bookkeeping by comparison.
Rename. TestRequiresCSRFValidation_TokenBasedPathsWithSession → something like TestRequiresCSRFValidation_PerPathTable_NotEndToEnd. The test misled through its name, and names are what appear in CI output, go test -v, and a grep for CSRF coverage. The scope-limit comment you added is accurate and well-argued, but it only reaches someone who already opened the file — which is not how the original misreading happened.
Do not delete the csrfExemptWhenTokenOnly block in this PR. It is genuinely dead — isPublicEndpoint returns at handler.go:762 for all four of its prefixes, so requiresCSRFValidation at :784 is never consulted for them — and dead code that reads as live protection is the root cause of #1757, so it should go. But it is a separate concern from the CSRF fix, it touches a predicate shared with live non-AuthPublic routes, and removing it before the end-to-end test lands would briefly trade real coverage for none. File it next to LeanerCloud/cloud-commitments-platform#192.
This is a shift in emphasis from what I said last round. Asked narrowly what to do with that one test, "rename it" was the answer; asked whether an end-to-end test should exist, that is plainly the higher-value item, and it is additive rather than a substitute.
Verdict
F1 is closed and the fix is correct. The narrowing is minimal, the fallback it had to preserve is intact under execution, the sentinel's flow is contained to one producer and one consumer, wrapping is safe in both directions, purchases has no sibling instance, and all gates are green.
Blocking on D-F1 only — drop ci_gate_check.sh from the commit. That is a git rm --cached and a reword, not a code change. D-F2 is a one-character nit worth folding into the same amend. Once ci_gate_check.sh is gone, this is ready to merge.
…path
Review finding F1. approveRIExchange falls back to the token flow when the
session path returns 403, so email-link holders can still approve. That test
is isPermissionDenied, which compares the status code alone, so a CSRF
rejection was indistinguishable from a legitimate approve-own denial and the
request proceeded on the token.
Measured before fixing, session with no CSRF headers and a valid ?token=:
err = <nil>
TransitionRIExchangeStatus called = true
ExecuteExchange called = true money moved
transition actor was NIL = true audit attribution lost
StampRIExchangeApprovedBy called = false
Not a reopened CSRF hole: the caller still needs the unguessable ApprovalToken,
which is constant-time compared and is sufficient authorization by design. But
a legitimate session-authed approve carrying a token degraded to an
unattributed system approval on a money path, and the code comment claimed
parity with approvePurchaseViaSession, which returns its CSRF 403 directly
rather than swallowing it. Shipping a #1757 fix whose comment overstates what
the code does is the failure mode #1757 was about.
CSRF rejection is now the errCSRFRejected sentinel. Note precisely what
protects it: isPermissionDenied(errCSRFRejected) is still true, because the
sentinel is a 403. What keeps it out of the fallback is the short-circuit
ordering of || at the dispatch, with the errors.Is term first. "Reached first"
rather than "unreachable", and the two have different failure modes: reordering
that expression reopens F1. The new test drives that exact path, so a reorder
fails it.
Chose the sentinel over widening isPermissionDenied because that predicate has
three other call sites in handler_purchases.go, and narrowing its meaning
globally is a larger blast radius than this finding warrants. Verified the
record-level approve-own denial still falls through, which is the case the
fallback exists for.
The new test drives the dispatch with a valid token, which the existing
empty-token test cannot reach, and asserts the money-moving call is never made.
Mutation-verified: replacing the sentinel with a plain NewClientError(403, ...)
makes it fail on both the TransitionRIExchangeStatus assertion and "an error is
expected but got nil", by assertion rather than by panic. Restored afterwards
and confirmed byte-identical.
Refs #1757
ba3d34f to
6026bb3
Compare
Sign-off —
|
|
Merging. Closes #1757 ( Recovered work. These commits were written a day ago, committed to a worktree, and never pushed because the authoring agent hit a quota limit between committing and pushing. Rebased onto current The vulnerabilityThe session-authed RI-exchange approve path had no CSRF enforcement at all. Reproduced independently in review through The control differentiating within the same run is what rules out "RI-exchange happens to fail for an unrelated reason". A logged-in user visiting an attacker-controlled page that fired a cross-site POST to Why it survived, and both reasons are the same defectA comment asserted the protection existed. A test asserted it was required, and passed. A comment claiming the control and a green test naming the exact property. Both reassuring, both wrong. Review found a second defect inside the fix
Not a reopened CSRF hole — the caller still needs the unguessable Fixed with an Chosen over widening Verified
Filed from this review, not folded in
Test hygiene was kept out of this PR deliberately: it does not reduce the exposure, and every extra commit on a p0 is another CI cycle and CodeRabbit round while the vulnerability is live. |
1 participant
Closes #1757 (
p0/severity/critical).Recovered work. These two commits were written a day ago, committed to a worktree, and never pushed because the authoring agent hit a quota limit mid-task. Rebased onto current
main(13 commits behind), zero conflicts, and re-verified after the rebase rather than carrying the earlier gate results forward.The vulnerability
The session-authed RI-exchange approve path had no CSRF enforcement at all. Confirmed by execution with a purchases control in the same probe run:
A logged-in user visiting an attacker-controlled page that fires a cross-site POST to
/api/ri-exchange/approve/<id>had their exchange approved and executed. This is exactly the threat model #404 fixed for purchases; it was never applied here.Why it survived
Two compounding reasons, both worth recording.
A comment asserted the protection existed.
middleware.go:288-289stated that/api/ri-exchange/approve/and/api/ri-exchange/reject/areAuthPublicand "therefore exempted by isPublicEndpoint(), not here" — the same sentence structure used two lines above for purchases approve/cancel, where "not here" means enforced inside the handler instead. For purchases that is true (approvePurchaseViaSessioncallsh.validateCSRFdirectly). For RI-exchange it was not:h.validateCSRFappeared nowhere inhandler_ri_exchange.go.A test asserted it was required, and passed.
TestRequiresCSRFValidation_TokenBasedPathsWithSessionassertsrequiresCSRFValidation(...)returnstruefor this exact path — but calls the predicate directly, bypassing theisPublicEndpointgate that stops the real dispatch from ever consulting it. The test name and its passing assertion both read as "CSRF is required and enforced here"; neither touches the enforced path.The repos own test suite had zero real CSRF assertions for this handler: the only CSRF-related line was a stub
ValidateCSRFTokenreturningnilunconditionally, never exercised to prove rejection.The fix
h.validateCSRF(ctx, req)at the top ofapproveRIExchangeViaSession, mirroringapprovePurchaseViaSession, with the explanatory comment aboutAuthPublicroutes skipping the outer middleware so the next reader sees why it is in-handler.rejectRIExchangeneeds nothing: it has no session-authed branch, being unconditionally token-gated with asubtle.ConstantTimeCompareagainst the stored approval token.The regression test asserts the money-moving call
The second commit exists because asserting on the error alone would not prove the state transition was prevented. The test asserts
TransitionRIExchangeStatusis not reached, which is the property that matters. Both directions: no CSRF token is refused before the transition, and a valid token still succeeds — a fix that refused every session-authed approve would pass a rejection-only test while breaking the feature.Post-rebase verification
validateCSRFpresent inhandler_ri_exchange.go(was 0 occurrences, now 1),go build ./...exit 0, andgo test ./internal/api/ -run CSRF|RIExchangepasses. Full gates run by CI.Related, filed separately from this investigation
approvePurchase/cancelPurchasediscard an authorization denial and fall through. Verified latent, not live: safe today on two independent facts (inner and outer call the sameauthorizeSessionApprove, and thisAuthPublicroute family never caches a context principal), either of which could change alone.approvePurchaseViaSessionreturns a 409 carryingstatus=before authorization, letting a caller probe existence and status of an execution whose UUID it knows.Summary by CodeRabbit
Bug Fixes
Tests