Follow-up from PR #1796 (#1757). Deliberately not done in that PR: #1757 was a live money-path vulnerability on main, test hygiene does not reduce that exposure, and every extra commit there was another CI cycle plus a CodeRabbit round on a p0.
Two changes, and the second is a matched pair that must land in one commit.
1. Add the guard that does not exist
A table-driven test driving Router.Route over every AuthPublic route with a session-authed branch that reaches a state-changing or money-moving call, with a valid session and no CSRF header, asserting refusal.
This is the only artifact that would have caught #1757, and it guards the class: the next AuthPublic route someone registers is covered without them having heard of this issue. Roughly 40 lines for the whole matrix.
Drive Router.Route, not the handlers. That distinction is the entire reason #1757 survived, and it decided #1773 as well.
Assert on the money-moving call (TransitionRIExchangeStatus, ApproveAndExecute, and their siblings), not merely on the error. An error assertion alone does not prove the state transition was prevented.
2. Delete TestRequiresCSRFValidation_TokenBasedPathsWithSession together with the dead csrfExemptWhenTokenOnly block
That test asserts requiresCSRFValidation(...) returns true for /api/ri-exchange/approve/... and passes — while calling the predicate directly and bypassing the isPublicEndpoint gate that stops the real dispatch from ever consulting it. Its name and its passing assertion both read as proof of enforcement; neither touches the enforced path.
But deleting the test on its own is worse than leaving it. It is the only thing pinning the csrfExemptWhenTokenOnly block's behaviour. Remove the test alone and unreachable production code is left with zero coverage; today the table is at least pinned if someone ever makes those routes non-public.
They are a matched pair: dead code, and the test that makes it look alive. One commit removing both, with the unreachability demonstrated in the message — isPublicEndpoint returns at handler.go:762 for all four prefixes, so requiresCSRFValidation at :784 is never consulted for them — is reviewable in a way that either half alone is not.
3. Minor, fold in here
middleware_test.go:519 and :577 both use execution ID ...446655440021. One character; not worth its own CI cycle.
Verification
For change 1, both directions per route: no CSRF token refused before the money-moving call, and a valid CSRF token still succeeds. A matrix that only asserts refusal passes against a change that breaks every session-authed approve.
Mutation-verify by removing the validateCSRF call from one handler and confirming that route's row fails — and that it fails by assertion rather than by panic, which requires the .Maybe() fixture discipline used in TestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken.
For change 2, confirm the block is genuinely unreachable before deleting it, rather than inferring it from the test's existence.
Related
Follow-up from PR #1796 (#1757). Deliberately not done in that PR: #1757 was a live money-path vulnerability on
main, test hygiene does not reduce that exposure, and every extra commit there was another CI cycle plus a CodeRabbit round on a p0.Two changes, and the second is a matched pair that must land in one commit.
1. Add the guard that does not exist
A table-driven test driving
Router.Routeover everyAuthPublicroute with a session-authed branch that reaches a state-changing or money-moving call, with a valid session and no CSRF header, asserting refusal.This is the only artifact that would have caught #1757, and it guards the class: the next
AuthPublicroute someone registers is covered without them having heard of this issue. Roughly 40 lines for the whole matrix.Drive
Router.Route, not the handlers. That distinction is the entire reason #1757 survived, and it decided #1773 as well.Assert on the money-moving call (
TransitionRIExchangeStatus,ApproveAndExecute, and their siblings), not merely on the error. An error assertion alone does not prove the state transition was prevented.2. Delete
TestRequiresCSRFValidation_TokenBasedPathsWithSessiontogether with the deadcsrfExemptWhenTokenOnlyblockThat test asserts
requiresCSRFValidation(...)returnstruefor/api/ri-exchange/approve/...and passes — while calling the predicate directly and bypassing theisPublicEndpointgate that stops the real dispatch from ever consulting it. Its name and its passing assertion both read as proof of enforcement; neither touches the enforced path.But deleting the test on its own is worse than leaving it. It is the only thing pinning the
csrfExemptWhenTokenOnlyblock's behaviour. Remove the test alone and unreachable production code is left with zero coverage; today the table is at least pinned if someone ever makes those routes non-public.They are a matched pair: dead code, and the test that makes it look alive. One commit removing both, with the unreachability demonstrated in the message —
isPublicEndpointreturns athandler.go:762for all four prefixes, sorequiresCSRFValidationat:784is never consulted for them — is reviewable in a way that either half alone is not.3. Minor, fold in here
middleware_test.go:519and:577both use execution ID...446655440021. One character; not worth its own CI cycle.Verification
For change 1, both directions per route: no CSRF token refused before the money-moving call, and a valid CSRF token still succeeds. A matrix that only asserts refusal passes against a change that breaks every session-authed approve.
Mutation-verify by removing the
validateCSRFcall from one handler and confirming that route's row fails — and that it fails by assertion rather than by panic, which requires the.Maybe()fixture discipline used inTestApproveRIExchange_CSRFFailureDoesNotFallThroughToToken.For change 2, confirm the block is genuinely unreachable before deleting it, rather than inferring it from the test's existence.
Related
/api/infoprefix-matched as public, found in the same review sweep.