sec: narrow CSRF exemption for approve/cancel POST endpoints (closes #404)#888
sec: narrow CSRF exemption for approve/cancel POST endpoints (closes #404)#888cristim wants to merge 1 commit into
Conversation
…404) The blanket "/api/purchases/approve/" and "/api/purchases/cancel/" entries in requiresCSRFValidation's exempt list were correct for the original token-only (email-link) flow but became incorrect once the three-mode dispatch (issues #46/#286) introduced a session-authed branch. A logged-in user's browser could be CSRF-forged into a POST to these endpoints because the middleware skipped CSRF validation entirely. Fix: remove the blanket middleware exemption and instead call validateCSRF() at the top of approvePurchaseViaSession and cancelPurchaseViaSession -- the two functions that mutate state under session authentication. The token-only path (authorizeApprovalAction) is stateless and never calls these functions, so it remains unaffected. These routes are already listed in isPublicEndpoint(), so validateSecurity() short-circuits before requiresCSRFValidation() is reached on the email-link path; the middleware exemption was therefore doubly redundant for that path and actively harmful for the session path. Tests: 4 new boundary tests in middleware_test.go - TestApproveViaSession_RequiresCSRF: session POST without CSRF -> 403 - TestApproveViaSession_PassesCSRF: session POST with CSRF -> 200 - TestCancelViaSession_RequiresCSRF: session POST without CSRF -> 403 - TestTokenOnlyApprove_BypassesCSRF: token path never calls CSRF check Updated 6 existing tests to supply ValidateCSRFToken expectations on the session-authed branches they exercise. No migration needed.
|
Warning Review limit reached
More reviews will be available in 31 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
/api/purchases/approve/and/api/purchases/cancel/entries fromrequiresCSRFValidation's exemption list. These routes are already covered byisPublicEndpoint(), so the middleware never reached CSRF validation on the token-only path anyway -- but the exemption was leaving session-authenticated POSTs unprotected.validateCSRF()calls at the entry ofapprovePurchaseViaSessionandcancelPurchaseViaSession-- the two functions that mutate state under session authentication (issues feat(history): inline Cancel button for pending purchase rows #46/feat(api,history): add Approve button + approve-{any,own} RBAC for in-dashboard purchase approval #286 added these session branches). The token-only (email-link) path never calls these functions and is unaffected.ValidateCSRFTokenexpectations on the session-authed branches they exercise, and updates one test whose expected error message changed (CSRF fires beforerequireSession, so tokenless requests now get 403 instead of 401).Attack scenario closed
A logged-in user's browser could previously be CSRF-forged into approving or cancelling a purchase if the attacker knew the execution ID. The session branch added in issues #46/#286 bypassed CSRF because the blanket middleware exemption applied unconditionally. This fix closes that gap.
Test plan
TestApproveViaSession_RequiresCSRF: session POST without CSRF token -> 403 "CSRF validation failed"TestApproveViaSession_PassesCSRF: session POST with valid CSRF token -> 200 "completed"TestCancelViaSession_RequiresCSRF: session POST without CSRF token -> 403 "CSRF validation failed"TestTokenOnlyApprove_BypassesCSRF: email-link token path never callsValidateCSRFToken(no mock registered; unexpected call would panic)internal/apitests passgo build ./...succeedsCloses #404