🐛 fix(csrf): strip path from referer before matching trusted origins#4204
Conversation
refererMatchesHost compared the full referer URL (including path and query) against trustedOrigins and trustedSubOrigins. Since trusted origins are stored without paths (e.g. https://example.com), a referer like https://example.com/form/submit never matched. Extract only scheme://host from the parsed referer URL before comparing, matching the behavior of originMatchesHost which already works correctly because Origin headers omit the path per the HTTP spec. Added a test case for trusted referer with path component.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughrefererMatchesHost now derives a normalized origin (scheme://host) from the Referer and uses that origin for trusted-origin and subdomain checks. Tests were extended to ensure Referer values with paths and queries are accepted when matching trusted origins. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CSRF middleware to extract and compare only the origin (scheme and host) from the Referer header, ensuring that paths or query parameters do not cause false negatives when validating against trusted origins. A test case was added to verify this fix. Feedback was provided to remove redundant string lowercasing operations to optimize performance and reduce allocations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
middleware/csrf/csrf_test.go (1)
1340-1354: Add one more case for wildcard trusted referer with path.This new case validates exact trusted origin + path, but the same regression path also exists for wildcard trusted referers. Please add a
https://safe.domain-1.com/some/path?...case to exercise the wildcard branch too.🧪 Suggested additional test case
+ // Test Trusted Referer Wildcard with path + ctx.Request.Reset() + ctx.Response.Reset() + ctx.Request.Header.SetMethod(fiber.MethodPost) + ctx.Request.Header.Set(fiber.HeaderXForwardedProto, "https") + ctx.Request.URI().SetScheme("https") + ctx.Request.URI().SetHost("domain-1.com") + ctx.Request.Header.SetProtocol("https") + ctx.Request.Header.SetHost("domain-1.com") + ctx.Request.Header.Set(fiber.HeaderReferer, "https://safe.domain-1.com/some/path?q=1") + ctx.Request.Header.Set(HeaderName, token) + ctx.Request.Header.SetCookie(ConfigDefault.CookieName, token) + h(ctx) + require.Equal(t, 200, ctx.Response.StatusCode())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/csrf/csrf_test.go` around lines 1340 - 1354, Add a new test case mirroring the existing "Trusted Referer with path" block but using a wildcard-trusted origin host (e.g. set ctx.Request.Header.Set(fiber.HeaderReferer, "https://safe.domain-1.com/some/path?q=1")) so the wildcard matching branch is exercised; keep the same request setup (Reset(), MethodPost, X-Forwarded-Proto=https, URI scheme/host, Header Host, HeaderName and ConfigDefault.CookieName set to token), call h(ctx) and assert require.Equal(t, 200, ctx.Response.StatusCode()) to validate the wildcard referer path handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@middleware/csrf/csrf_test.go`:
- Around line 1340-1354: Add a new test case mirroring the existing "Trusted
Referer with path" block but using a wildcard-trusted origin host (e.g. set
ctx.Request.Header.Set(fiber.HeaderReferer,
"https://safe.domain-1.com/some/path?q=1")) so the wildcard matching branch is
exercised; keep the same request setup (Reset(), MethodPost,
X-Forwarded-Proto=https, URI scheme/host, Header Host, HeaderName and
ConfigDefault.CookieName set to token), call h(ctx) and assert require.Equal(t,
200, ctx.Response.StatusCode()) to validate the wildcard referer path handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b9743f6b-a903-4a0e-93ef-bfc924350258
📒 Files selected for processing (2)
middleware/csrf/csrf.gomiddleware/csrf/csrf_test.go
gaby
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes CSRF trusted-origin matching when browsers send a Referer header that includes a path/query by comparing only scheme://host (aligned with how Origin is matched).
Changes:
- Normalize
Referertoscheme://hostbefore checkingtrustedOriginsandtrustedSubOrigins. - Add a regression test ensuring a trusted referer containing a path/query is accepted (200 vs 403).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/csrf/csrf.go | Strips path/query from parsed referer URL by comparing only scheme://host to trusted origin allowlists. |
| middleware/csrf/csrf_test.go | Adds a test case covering trusted referer matching when the referer includes a path/query. |
referer is already lowercased at line 376 before parsing, so the per-component ToLower calls are unnecessary. Also added a test for wildcard trusted subdomain with path+query in the referer to cover the subdomain.match path.
sixcolors
left a comment
sixcolors
left a comment
There was a problem hiding this comment.
LGTM. Normalizing Referer to an origin (scheme://host) before checking trustedOrigins/wildcards is the right behavior, and the new tests cover the regression nicely.
|
Merge pending fixes to Fiber. The CI failing are related to a recently merged PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4204 +/- ##
==========================================
+ Coverage 91.15% 91.21% +0.06%
==========================================
Files 123 123
Lines 11855 11855
==========================================
+ Hits 10806 10814 +8
+ Misses 660 654 -6
+ Partials 389 387 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
What happened
refererMatchesHostcompares the full referer URL (including path and query string) againsttrustedOriginsandtrustedSubOrigins. Since trusted origins are stored without paths (e.g.https://example.com), a referer likehttps://example.com/form/submitnever matches viaslices.Containsorsubdomain.match(which usesstrings.HasSuffix).originMatchesHostdoesn't have this problem because Origin headers omit the path per the HTTP spec.This means CSRF protection falls back to rejecting all cross-origin requests from trusted origins when the browser sends a Referer instead of an Origin header.
Fix
Extract
scheme://hostfrom the parsed referer URL before comparing against trusted origins, matching howoriginMatchesHostalready works.Test
Added a test case that sends a trusted referer with a path component and verifies it returns 200 instead of 403.