Skip to content

test(api): classify the approval-details route in the AuthPublic CSRF guard (fixes red main) - #1003

Merged
cristim merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/csrf-guard-approval-details
Oct 16, 2026
Merged

cristim merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/csrf-guard-approval-details

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1002.

Why main is red: TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt fails, because two of my PRs merged independently:

Change: a test-only, one-entry addition to authPublicCSRFExempt. The route is read-only and moves no money, so it doesn't fit the guard's money-moving rows. It still runs validateCSRF before the execution lookup, via requireSessionBeforeLookup, and TestHandleRequest_ApprovalDetails_SessionAndCSRFBeforeLookup from #990 pins that. The rest of the diff is gofmt re-aligning the map.

Verification (run on fresh main with AWS config set to /dev/null):

  • Before: go test ./internal/api/ -run TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt fails with "route POST /api/purchases/approve/*/details is not classified".
  • After: go test ./internal/api/ ./internal/mocks/ -count=1 is ok.
  • go vet ./internal/api/ is clean, and golangci-lint v2.10.1 reports 0 issues.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Purchase approval detail requests now follow the expected handling for read-only requests, while retaining CSRF protection before any purchase lookup. No other purchase approval behavior is changed.

… guard

LeanerCloud#990 added POST /api/purchases/approve/*/details and LeanerCloud#993 added the
class-level guard that requires every AuthPublic route to be classified;
they merged independently, so main fails
TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt. The route is
read-only and already runs validateCSRF before the lookup, so it goes in
the exempt list with that reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged effort/s Hours labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 63f094b1-6814-4a4d-b98b-847eb90b7faa
📥 Commits

Reviewing files that changed from the base of the PR and between 0ed85aa and ed54450.

📒 Files selected for processing: 1
  • internal/api/authpublic_csrf_guard_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

The CSRF guard test now classifies POST /api/purchases/approve/*/details as exempt and documents that the route is read-only.

Changes: AuthPublic CSRF classification

Layer / File(s) Summary
Approval details route exemption
internal/api/authpublic_csrf_guard_test.go
The exempt-route list now includes POST /api/purchases/approve/*/details with a read-only explanation.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to ed544

This classifies the read-only approval-details route in the guard test; its handler still checks session and CSRF before lookup, leaving no material merge risk.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the API route classification change in the AuthPublic CSRF guard and states the related test failure fix.
Linked Issues check Passed Issue #1002 requires classification of POST /api/purchases/approve/*/details. The whole-PR diff adds that exact key to authPublicCSRFExempt with a reason. The guard test checks this map and theref…
Out of Scope Changes check Passed The diff changes only internal/api/authpublic_csrf_guard_test.go. It adds the required exemption entry and re-aligns the map entries with formatting. These changes directly support issue #1002 and i…
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Full details: Linked Issues check

Explanation

Issue #1002 requires classification of POST /api/purchases/approve/*/details. The whole-PR diff adds that exact key to authPublicCSRFExempt with a reason. The guard test checks this map and therefore no longer reports the route as unclassified. The change also documents the existing requireSessionBeforeLookup CSRF behavior.

Full details: Out of Scope Changes check

Explanation

The diff changes only internal/api/authpublic_csrf_guard_test.go. It adds the required exemption entry and re-aligns the map entries with formatting. These changes directly support issue #1002 and introduce no unrelated behavior.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ 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

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Independent review verdict: CLEAN at ed54450

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief, acting as a security reviewer. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Main is red: on origin/main, TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt fails with "route POST /api/purchases/approve/*/details is not classified". The route came in with #990 (a74e6ea).

The classification is accurate:

  • Route: router.go:168 registers POST /api/purchases/approve/{id}/details as AuthPublic, handled by approvalDetailsHandler (:612), which is rate-limited by approve_cancel_public.
  • Read-only: getApprovalDetails (handler_purchases_approval_details.go:30) validates the UUID, then calls requireSessionBeforeLookup (handler_purchases.go:763). That returns 401 with no session and runs validateCSRF (403) before any lookup.
  • Authorization: authorizeApprovalDetailsRead requires either view:purchases plus account scope, or a valid token plus a contact-approver session email. Every denial is concealed as a 404.
  • Result: it does not act on a session without its own CSRF check, has no unauthenticated data leak, and is safe as AuthPublic.

The diff does not weaken the guard: git diff -w shows one added map entry. The rest is gofmt realignment, with no entries removed and no matching logic or assertions changed.

What was attacked and how:

  • go vet ./internal/api/... is clean.
  • -run 'CSRF|AuthPublic' with -race passes 56, and ./internal/api/... passes 3522.
  • Mutation probes, both caught:
    • Removing the new entry makes the guard fail.
    • Adding a fake AuthPublic POST route makes it fail with "not classified".
  • The cited TestHandleRequest_ApprovalDetails_SessionAndCSRFBeforeLookup (handler_purchases_approval_details_test.go:174) exists and passes.

Gate: labels are mirrored from #1002. Merge once CI is green at this SHA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(api): main fails TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt (approval-details route unclassified)

2 participants