🐛 bug: Fix CSRF subdomain wildcard boundary #3694
Conversation
WalkthroughRefactors CSRF trusted-origin parsing to trim and normalize entries, adds explicit wildcard base-origin handling, and hardens subdomain matching logic. Updates CSRF and helper tests to align with new suffix semantics and to cover malformed subdomain cases. Public API unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant CSRF as CSRF Middleware
participant Parser as TrustedOrigins Parser
participant Matcher as subdomain.match
rect rgba(230,245,255,0.6)
note over Parser: Init (on startup/config)
Parser->>Parser: Trim cfg.TrustedOrigins
Parser->>Parser: If wildcard, strip "*" and normalize base
Parser->>Parser: Store exact or wildcard-base entry
end
Client->>CSRF: HTTP Request (Origin/Referer, Cookie/Token)
CSRF->>CSRF: Validate CSRF token
alt Origin present
CSRF->>Matcher: Is request origin in trusted origins?
opt Wildcard entry
CSRF->>Matcher: prefix/suffix dot-aware check
Matcher-->>CSRF: true/false
end
alt Match
CSRF-->>Client: Proceed
else No match
CSRF-->>Client: 403 Forbidden
end
else No Origin
CSRF-->>Client: Proceed (per existing rules)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3694 +/- ##
==========================================
- Coverage 91.81% 91.80% -0.02%
==========================================
Files 114 114
Lines 11498 11517 +19
==========================================
+ Hits 10557 10573 +16
- Misses 681 683 +2
- Partials 260 261 +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:
|
![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: 57a9f29 | Previous: 9ed35c9 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes CSRF subdomain wildcard boundary validation by hardening the subdomain matching logic to prevent bypass attacks and ensuring proper parsing of trusted origins. The changes align CSRF subdomain validation with CORS logic for consistency.
Key changes:
- Enhanced subdomain matching with proper boundary validation and malformed input rejection
- Fixed trusted origin parsing to correctly handle wildcard patterns
- Added comprehensive test coverage for edge cases and malformed inputs
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| middleware/csrf/helpers.go | Replaces simple string matching with robust boundary validation logic |
| middleware/csrf/csrf.go | Fixes trusted origin parsing to properly extract scheme and host components |
| middleware/csrf/helpers_test.go | Updates test cases to reflect new suffix format and adds edge case coverage |
| middleware/csrf/csrf_test.go | Adds test for malformed subdomain attack vector |
![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 addresses a security vulnerability in the CSRF subdomain wildcard matching by hardening the logic and expanding test coverage for edge cases. The refactoring of the trusted origin parsing in middleware/csrf/csrf.go improves clarity and robustness. The new subdomain.match logic in middleware/csrf/helpers.go correctly handles various malformed origin formats, and the new tests are a valuable addition. I have a couple of suggestions to further refine the code.
![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
🧹 Nitpick comments (4)
middleware/csrf/csrf_test.go (1)
853-866: Good addition: malformed subdomain origin is correctly rejectedThe case "http://evil.comdomain-1.com" while host is "domain-1.com" should indeed be denied. This test guards against suffix substring tricks and aligns with the hardened Subdomain.match.
Consider adding:
- The same malformed subdomain with https scheme.
- A referer-based counterpart to ensure parity when Origin is absent on HTTPS.
If you want, I can draft those tests.
middleware/csrf/helpers.go (1)
55-84: Subdomain matching hardened and precise; logic LGTMThe guarded checks are clear and correct:
- Length guard prevents trivial underflows.
- Combined HasPrefix/HasSuffix assures scheme and base host alignment.
- Dot-before-suffix constraint + non-empty label validation block suffix-substring attacks (e.g., evil.comexample.com) and empty labels (., ..).
No functional gaps spotted for wildcard host semantics; ports are naturally handled since they’re part of the suffix.
Optional: micro-clarify expectations in a short doc comment above match, e.g., that prefix normally ends at "://", and suffix is the base host (possibly with port).
middleware/csrf/csrf.go (1)
63-79: Wildcard parsing mirrors CORS and feeds the stronger matcher; one tiny trim nitThe approach of stripping "://*." to derive a base origin, normalizing, then building subdomain{prefix: scheme://, suffix: host[:port]} is sound and avoids false positives.
Minor nit: utils.Trim(origin, ' ') only trims spaces, not other whitespace. Using TrimSpace is more robust without changing semantics here.
Apply within this hunk:
- trimmedOrigin := utils.Trim(origin, ' ') + trimmedOrigin := strings.TrimSpace(origin)middleware/csrf/helpers_test.go (1)
66-67: Tests aligned with new suffix semantics; edge cases covered
- Updating suffixes from ".example.com" to "example.com" matches the new matcher contract.
- New negatives for malformed subdomain and empty/malformed labels are valuable and map directly to the new guards.
- Benchmark updated accordingly.
One small naming nit: there are two cases labeled "match with different scheme" (Line 65 and Line 71). Renaming for uniqueness improves failure reporting (e.g., "no match with different scheme (prefix anchored)" vs "no match with different scheme (scheme mismatch)").
Would you like me to propose exact renames?
Also applies to: 72-73, 78-79, 84-85, 91-92, 97-98, 103-104, 107-112, 115-116, 119-130, 145-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
middleware/csrf/csrf.go(1 hunks)middleware/csrf/csrf_test.go(1 hunks)middleware/csrf/helpers.go(1 hunks)middleware/csrf/helpers_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/csrf/csrf_test.go (2)
constants.go (2)
MethodPost(7-7)HeaderOrigin(206-206)middleware/csrf/config.go (2)
HeaderName(120-120)ConfigDefault(123-130)
⏰ 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). (7)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: Analyse
- GitHub Check: Compare
- GitHub Check: lint
Summary