Skip to content

sec: narrow CSRF exemption for approve/cancel to token-only requests - #531

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
sec/csrf-approve-cancel-scope
Jun 6, 2026
Merged

sec: narrow CSRF exemption for approve/cancel to token-only requests#531
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
sec/csrf-approve-cancel-scope

Conversation

@cristim

@cristim cristim commented May 19, 2026

Copy link
Copy Markdown
Member

Fixes #404

Summary by CodeRabbit

  • Bug Fixes

    • CSRF validation now correctly distinguishes token-only vs. session-auth requests, ensuring sensitive operations (plan changes, payment approvals/cancellations) require CSRF when a session token is present while keeping login/registration and token-only flows appropriately exempt.
  • Tests

    • Added regression tests covering CSRF exemption rules across token and session scenarios to prevent regressions.

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ba44d24c-b2c8-4b1c-b6de-e2feb50b6d49

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Middleware requiresCSRFValidation now accepts the request and makes approve/cancel/reject exemptions conditional on bearer-token absence. The handler passes the request into the function. Tests and an existing coverage test were updated to validate token-only exemptions and session-authenticated CSRF requirements.

Changes

Token-aware CSRF exemption for purchase endpoints

Layer / File(s) Summary
Token-aware CSRF exemption middleware
internal/api/middleware.go
requiresCSRFValidation now accepts the request object and splits exemption logic: auth/register paths remain unconditionally exempt, while approve/cancel/reject prefixes are exempt only when extractBearerToken(req) is empty.
Handler wiring
internal/api/handler.go
validateSecurity now calls requiresCSRFValidation(method, path, req) so the middleware can inspect the incoming request for a bearer token.
Regression and coverage tests
internal/api/middleware_test.go, internal/api/coverage_gaps_test.go
Adds tests asserting token-only approve/cancel/reject paths are exempt when no session bearer token is present and require CSRF when a session bearer token exists; updates coverage test to pass a request object and adjust expectations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I checked the token, the headers, the line,
A tiny guard now knows when to decline.
Approve paths whisper when token is near,
But sessions must prove they truly are clear.
Hooray — fewer gaps, the code hops in rhyme.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: narrowing CSRF exemption for approve/cancel endpoints to token-only (non-session) requests.
Linked Issues check ✅ Passed The PR directly addresses issue #404 by implementing the suggested fix: removing the blanket approve/cancel exemption and making it conditional on whether a session bearer token is present.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing issue #404: modifying CSRF validation logic and adding corresponding regression tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/csrf-approve-cancel-scope

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/s Hours type/security Security finding triaged Item has been triaged labels May 19, 2026
@cristim

cristim commented May 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 19, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 27, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 54 minutes and 9 seconds before sending another message.

@cristim
cristim force-pushed the sec/csrf-approve-cancel-scope branch from a76882c to e1bee49 Compare June 5, 2026 08:49
@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Rebased on feat/multicloud-web-frontend. Resolved conflicts in:

  • internal/api/middleware.go (comment block around csrfExemptAlways)
  • internal/api/middleware_test.go (kept both HEAD integration tests and PR middleware unit tests)
  • internal/api/coverage_gaps_test.go (used PR's correct call signature with noSessionReq)

All tests pass (1444 passed).

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@cristim Thanks for the update! Triggering a fresh review of the rebased changes now.


🧠 Learnings used
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Fixes #404: the approve/cancel/reject paths were unconditionally exempt
from CSRF validation, meaning a session-authenticated user could be
CSRF-attacked into approving a purchase from a malicious page.

requiresCSRFValidation now accepts the request and checks for a bearer
token: if the request carries a session, CSRF is required (session flow);
if there is no session, CSRF is skipped (pure email-link/token flow where
the approval token in the URL serves as the authenticator).

Unconditionally-exempt paths (login, setup-admin, forgot/reset-password,
register) are unchanged.

Updated the call site in validateSecurity and fixed existing tests in
coverage_gaps_test.go to pass the request argument.

Regression tests added covering both the token-only and session cases.
@cristim
cristim force-pushed the sec/csrf-approve-cancel-scope branch from e1bee49 to d8eabd6 Compare June 5, 2026 12:08
@cristim
cristim merged commit 3405060 into feat/multicloud-web-frontend Jun 6, 2026
4 checks passed
@cristim
cristim deleted the sec/csrf-approve-cancel-scope branch July 27, 2026 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/security Security finding urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant