fix(frontend): fix intermittent csrf 403s on mutations #422
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 15 seconds. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR migrates CSRF token handling from server-side preload (via meta tag in the layout) to a client-side async cache warmed in a provider useEffect. All mutating endpoints await the cached token. Middleware logging for CSRF validation and API access is enhanced for observability. ChangesClient-side CSRF token caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend-v2/README.md (1)
50-50: 💤 Low valueConsider clarifying the fetch source.
The phrase "fetches
api/csrf-tokenfrom the browser" could be misread. The function runs in the browser but fetches the token from the server endpointapi/csrf-token.Consider rephrasing to: "Before any state-changing request,
ApiClient.getCsrfToken()fetches a token from the server endpointapi/csrf-token."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend-v2/README.md` at line 50, The README wording is ambiguous; update the sentence that references ApiClient.getCsrfToken() so it clarifies the source by stating that ApiClient.getCsrfToken() (which runs in the browser) fetches a token from the server endpoint `api/csrf-token` before any state-changing request; locate the sentence mentioning ApiClient.getCsrfToken() in README.md and replace "fetches `api/csrf-token` from the browser" with the clearer phrasing (e.g., "fetches a token from the server endpoint `api/csrf-token`").server/src/main.go (1)
243-246: ⚡ Quick winCSRF error handler: API is valid; response detail is a policy choice
server/src/main.go:243-246usescsrf.ErrorHandlerandcsrf.FailureReason(r)correctly forgorilla/csrf v1.7.3—the library’s default 403 handler already includesFailureReason(r)in the response body.- If you want to reduce information disclosure, keep
csrf.FailureReason(r)in logs but return only a generic 403 message to clients.Optional: don’t return the failure reason in the response body
csrf.ErrorHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log.Printf("csrfMiddleware: validation failed for %s %s: %v", r.Method, r.URL.Path, csrf.FailureReason(r)) - http.Error(w, fmt.Sprintf("%s - %s", http.StatusText(http.StatusForbidden), csrf.FailureReason(r)), http.StatusForbidden) + http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main.go` around lines 243 - 246, The CSRF error handler currently logs and returns the detailed failure reason; keep logging csrf.FailureReason(r) but stop echoing it to clients: inside the csrf.ErrorHandler(http.HandlerFunc(...)) change the http.Error call to return a generic http.StatusText(http.StatusForbidden) (or a short "Forbidden" message) while preserving the log.Printf that includes csrf.FailureReason(r); this touches the csrf.ErrorHandler block and uses csrf.FailureReason(r) and http.Error to locate and update the response body only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/main.go`:
- Line 486: The log statement in apiAccessMiddleware currently includes PII
(user.Email); remove user.Email from the log and only log non-PII identifiers
like user.ID and user.Roles (update the log.Printf call that mentions "access
denied for user %d (%s) roles=%v on %s %s" to omit the email placeholder),
ensuring the message still contains sufficient context (user.ID, user.Roles,
r.Method, r.URL.Path) for correlation and debugging.
---
Nitpick comments:
In `@frontend-v2/README.md`:
- Line 50: The README wording is ambiguous; update the sentence that references
ApiClient.getCsrfToken() so it clarifies the source by stating that
ApiClient.getCsrfToken() (which runs in the browser) fetches a token from the
server endpoint `api/csrf-token` before any state-changing request; locate the
sentence mentioning ApiClient.getCsrfToken() in README.md and replace "fetches
`api/csrf-token` from the browser" with the clearer phrasing (e.g., "fetches a
token from the server endpoint `api/csrf-token`").
In `@server/src/main.go`:
- Around line 243-246: The CSRF error handler currently logs and returns the
detailed failure reason; keep logging csrf.FailureReason(r) but stop echoing it
to clients: inside the csrf.ErrorHandler(http.HandlerFunc(...)) change the
http.Error call to return a generic http.StatusText(http.StatusForbidden) (or a
short "Forbidden" message) while preserving the log.Printf that includes
csrf.FailureReason(r); this touches the csrf.ErrorHandler block and uses
csrf.FailureReason(r) and http.Error to locate and update the response body
only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3fb9754-bbc9-47d1-802f-21a3a1770dcb
📒 Files selected for processing (5)
frontend-v2/README.mdfrontend-v2/src/app/layout.tsxfrontend-v2/src/app/providers.tsxfrontend-v2/src/lib/api.tsserver/src/main.go
💤 Files with no reviewable changes (1)
- frontend-v2/src/app/layout.tsx
| } | ||
|
|
||
| if !hasAccess(user) { | ||
| log.Printf("apiAccessMiddleware: access denied for user %d (%s) roles=%v on %s %s", user.ID, user.Email, user.Roles, r.Method, r.URL.Path) |
There was a problem hiding this comment.
Remove PII from log statement.
Logging user.Email creates a compliance/privacy risk. User emails are personally identifiable information (PII) and should not be logged. The user.ID is sufficient for correlation and debugging without exposing PII.
🛡️ Proposed fix to remove PII
- log.Printf("apiAccessMiddleware: access denied for user %d (%s) roles=%v on %s %s", user.ID, user.Email, user.Roles, r.Method, r.URL.Path)
+ log.Printf("apiAccessMiddleware: access denied for user %d roles=%v on %s %s", user.ID, user.Roles, r.Method, r.URL.Path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Printf("apiAccessMiddleware: access denied for user %d (%s) roles=%v on %s %s", user.ID, user.Email, user.Roles, r.Method, r.URL.Path) | |
| log.Printf("apiAccessMiddleware: access denied for user %d roles=%v on %s %s", user.ID, user.Roles, r.Method, r.URL.Path) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/main.go` at line 486, The log statement in apiAccessMiddleware
currently includes PII (user.Email); remove user.Email from the log and only log
non-PII identifiers like user.ID and user.Roles (update the log.Printf call that
mentions "access denied for user %d (%s) roles=%v on %s %s" to omit the email
placeholder), ensuring the message still contains sufficient context (user.ID,
user.Roles, r.Method, r.URL.Path) for correlation and debugging.
Fix by fetching token client-side.
Previously the CSRF token was fetched in the layout.tsx server component
and injected into a <meta name="csrf-token"> tag, then read via
document.querySelector. This caused intermittent 403s: gorilla/csrf ties
the token to the _gorilla_csrf cookie it sets via Set-Cookie, but when
fetched server-side the cookie was sent to the Next.js server, not the
browser — so the browser's cookie never matched the token.
This commit moves CSRF token fetching entirely to the browser:
Removes the SSR fetchCsrfToken() call and <meta name="csrf-token">
injection from layout.tsx — this also eliminates the previous
DOM-scraping approach (document.querySelector('meta[name="csrf-token"]')),
which was fragile and a mild XSS risk if meta tag content were ever
attacker-influenced.
Adds a module-level getCsrfToken() with in-flight deduplication and a
session cache, so only one round-trip is made per browser session.
Exports preloadCsrfToken(), called from a useEffect in Providers, to
eagerly warm the cache before any mutation fires.
alexsapps
force-pushed
the
alex/auth-errors
branch
from
a7fb0a9 to
07bf829
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation