🔥 feat: Add support for Sec-Fetch-Site header in CSRF middleware #3913
Conversation
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's security posture by introducing centralized management for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded exported header constant Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CSRF as CSRF Middleware
participant ErrH as Error Handler
participant App as Application Handler
Client->>CSRF: HTTP request (method, Sec-Fetch-Site?, Origin, cookies, token)
alt method is unsafe
CSRF->>CSRF: validateSecFetchSite(header)
alt invalid Sec-Fetch-Site
CSRF->>ErrH: ErrFetchSiteInvalid
ErrH-->>Client: 403 Forbidden
else valid or absent Sec-Fetch-Site
CSRF->>CSRF: origin / scheme / token checks
alt any check fails
CSRF->>ErrH: relevant CSRF error
ErrH-->>Client: 403 Forbidden
else all checks pass
CSRF->>App: forward request
App-->>Client: 200 OK
end
end
else method considered safe
CSRF->>App: forward request
App-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3913 +/- ##
==========================================
- Coverage 91.66% 91.60% -0.06%
==========================================
Files 119 119
Lines 10096 10107 +11
==========================================
+ Hits 9254 9259 +5
- Misses 534 539 +5
- Partials 308 309 +1
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:
|
![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 effectively centralizes the Sec-Fetch-Site header constant and integrates its validation into the CSRF middleware, which is a good security enhancement. The changes are logical and the accompanying tests are thorough. I have one suggestion to optimize the new validation function for better performance by avoiding string allocations, which is in line with the Fiber framework's performance goals.
There was a problem hiding this comment.
Pull request overview
This PR centralizes the Sec-Fetch-Site header constant definition by adding it to the shared constants file, updating the CSRF middleware to reference this shared constant, and enhancing CSRF protection with early validation of the Sec-Fetch-Site header to reject cross-site requests before performing more expensive origin validation checks.
Key changes:
- Added
HeaderSecFetchSiteconstant to centralize header name definition - Implemented Sec-Fetch-Site header validation in CSRF middleware for improved security
- Added comprehensive test coverage for Sec-Fetch-Site validation scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| constants.go | Added HeaderSecFetchSite constant definition alongside other Sec-* header constants |
| middleware/csrf/csrf.go | Implemented validateSecFetchSite function and integrated Sec-Fetch-Site validation into the CSRF middleware flow, added ErrFetchSiteInvalid error constant |
| middleware/csrf/csrf_test.go | Added comprehensive test suite for Sec-Fetch-Site header validation covering various scenarios including same-origin, cross-site, same-site, and missing header cases |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gaby
changed the title
gaby
changed the title
|
Do not merge yet, forgot to update documentation. |
|
can we include this hint in the agent.md ? |
Will do |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
middleware/csrf/csrf.go (1)
322-335: Consider usingutils.EqualFoldfor zero-allocation comparison.The current implementation allocates a new string via
utils.ToLower. For better alignment with Fiber's zero-allocation philosophy, consider usingutils.EqualFold:func validateSecFetchSite(c fiber.Ctx) error { secFetchSite := utils.Trim(c.Get(fiber.HeaderSecFetchSite), ' ') if secFetchSite == "" { return nil } - switch utils.ToLower(secFetchSite) { - case "same-origin", "none", "cross-site", "same-site": + if utils.EqualFold(secFetchSite, "same-origin") || + utils.EqualFold(secFetchSite, "none") || + utils.EqualFold(secFetchSite, "cross-site") || + utils.EqualFold(secFetchSite, "same-site") { return nil - default: - return ErrFetchSiteInvalid } + + return ErrFetchSiteInvalid }
🧹 Nitpick comments (1)
middleware/csrf/csrf_test.go (1)
851-861: Useintinstead ofint16for HTTP status codes.Using
int16forexpectedStatusis unusual and requires an explicit cast on line 994. Standard Go HTTP libraries useintfor status codes.tests := []struct { name string method string secFetchSite string origin string - expectedStatus int16 + expectedStatus int https bool expectFetchSiteInvalid bool }{Then update line 994:
- require.Equal(t, int(tt.expectedStatus), c.Response.StatusCode()) + require.Equal(t, tt.expectedStatus, c.Response.StatusCode())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/csrf/csrf.go(3 hunks)middleware/csrf/csrf_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
middleware/csrf/csrf_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/csrf/csrf_test.gomiddleware/csrf/csrf.go
🧠 Learnings (5)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/csrf/csrf_test.gomiddleware/csrf/csrf.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
middleware/csrf/csrf_test.gomiddleware/csrf/csrf.go
📚 Learning: 2025-07-19T14:06:29.884Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Applied to files:
middleware/csrf/csrf.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
middleware/csrf/csrf.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
middleware/csrf/csrf.go
🧬 Code graph analysis (1)
middleware/csrf/csrf_test.go (2)
middleware/csrf/csrf.go (3)
New(48-220)Handler(31-35)ErrFetchSiteInvalid(19-19)middleware/csrf/config.go (3)
Config(16-126)HeaderName(129-129)ConfigDefault(132-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: lint
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
🔇 Additional comments (3)
middleware/csrf/csrf.go (2)
16-27: LGTM!The error constants are well-organized and the new
ErrFetchSiteInvaliderror follows the existing naming convention.
131-135: LGTM!The early Sec-Fetch-Site validation correctly rejects only malformed header values while allowing legitimate cross-origin requests to proceed to origin/referer validation. This addresses the previous concern about TrustedOrigins bypass.
middleware/csrf/csrf_test.go (1)
826-1000: LGTM!The test provides comprehensive coverage for the Sec-Fetch-Site header validation, including:
- All valid header values (same-origin, none, cross-site, same-site)
- Invalid header rejection
- Missing header scenarios
- Safe vs unsafe HTTP methods
- Origin matching behavior
The use of
t.Parallel()at both test and subtest levels follows the coding guidelines. As per coding guidelines for**/*_test.go,t.Parallel()is correctly invoked.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 38324a8 | Previous: ed2be1c | Ratio |
|---|---|---|---|
Benchmark_NewError |
71.45 ns/op 24 B/op 1 allocs/op |
40.61 ns/op 24 B/op 1 allocs/op |
1.76 |
Benchmark_NewError - ns/op |
71.45 ns/op |
40.61 ns/op |
1.76 |
Benchmark_Compress_Levels/Zstd_LevelDefault - B/op |
1 B/op |
0 B/op |
+∞ |
Benchmark_Compress_Parallel/Zstd - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request enhances CSRF protection by adding support for the Sec-Fetch-Site header. The changes are well-implemented and include comprehensive tests. My review includes two suggestions for improvement: one to clarify the documentation for the new feature, and another to refactor hardcoded string values into constants for better maintainability. Overall, this is a solid security enhancement.
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/middleware/csrf.md (1)
41-41: Use lowercase SameSite values to align with Fiber v3 constants.The documentation examples use capitalized
"Lax"and"Strict"for theCookieSameSiteproperty, but Fiber v3 constants define these as lowercase:"lax","strict","none". The code should use either lowercase string values or the typed constants for consistency.Based on learnings, update the CookieSameSite values from capitalized to lowercase throughout the documentation:
- CookieSameSite: "Lax", + CookieSameSite: "lax",This should be applied to lines 41, 80, 94 in code examples, and line 406 in the Config Properties table (change
"Lax"to"lax"and"Strict"to"strict").Also update line 55 in the Best Practices section:
- CookieSameSite: "Lax"` or `"Strict"` + CookieSameSite: "lax"` or `"strict"`Also applies to: 55-55, 80-80, 94-94, 406-406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/middleware/csrf.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/csrf.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/middleware/csrf.md
🧠 Learnings (1)
📚 Learning: 2025-07-19T14:06:29.884Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3598
File: docs/middleware/csrf.md:37-42
Timestamp: 2025-07-19T14:06:29.884Z
Learning: In Fiber v3, the CookieSameSite constants use lowercase values: CookieSameSiteLaxMode = "lax", CookieSameSiteStrictMode = "strict", CookieSameSiteNoneMode = "none". Documentation examples should use lowercase string values or the typed constants, not capitalized strings like "Lax".
Applied to files:
docs/middleware/csrf.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
|
@sixcolors can you also check @gaby should we add this feature to whatsNew.md ? |
CSRF middleware now inspects the Sec-Fetch-Site header for unsafe methods, rejecting requests with invalid values. Also, added redaction options for CSRF and Idempotency middleware.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/whats_new.md (1)
1289-1290: Improve clarity and fix comma splice in CSRF Sec-Fetch-Site documentation.Line 1289 has a comma splice ("...unsafe methods, the middleware inspects..."). Additionally, the documentation repeats the core idea twice—first stating the middleware "handles" the header, then immediately restating it "inspects" the header. The final explanatory sentence also somewhat duplicates information already conveyed.
Consider restructuring for better flow:
-The CSRF now handles **Sec-Fetch-Site** header for unsafe methods, the middleware inspects the [`Sec-Fetch-Site`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site) header when present. If the header value is not one of "same-origin", "none", "same-site", or "cross-site", the request is rejected with `ErrFetchSiteInvalid`. If the header is valid or absent, the request proceeds to the standard origin and token validation checks. This provides an early check to block requests with invalid `Sec-Fetch-Site` values, while allowing legitimate same-site and cross-site requests to be validated by the existing mechanisms. +The CSRF middleware now validates the [`Sec-Fetch-Site`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site) header for unsafe HTTP methods. When present, requests with invalid `Sec-Fetch-Site` values (not one of "same-origin", "none", "same-site", or "cross-site") are rejected with `ErrFetchSiteInvalid`. Valid or absent headers proceed to standard origin and token validation checks, providing an early gate to catch malformed requests while maintaining compatibility with legitimate cross-site traffic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/whats_new.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/whats_new.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (1)
docs/whats_new.md (1)
1-6: Remember to run markdown linting before finalizing.Per the coding guidelines, run
make markdownto lint all Markdown files when modifying documentation.
The CSRF middleware now validates the Sec-Fetch-Site header for unsafe HTTP methods, rejecting requests with invalid values. Additionally, the Expiration field has been renamed to IdleTimeout, and the default value has been changed from 1 hour to 30 minutes.
@ReneWerner87 done |
gaby
added
the
📜 RFC Compliance
ReneWerner87
deleted the
implement-header-evaluation-for-csrf-middleware
branch
Summary
Highlights
Related #3912