test(cors,csrf): cover remaining middleware branches#4462
Merged
Conversation
Add targeted tests to push CORS and CSRF middleware coverage to their practical maximum (CORS 96.2% -> 98.5%, CSRF 94.3% -> 99.6%). CORS: - setSimpleHeaders nil-config guard - setSimpleHeaders wildcard origin with AllowCredentials (warning branch) - setPreflightHeaders nil-config guard CSRF: - validateExtractorSecurity nil-config guard - raw origin surfaced in panic when DisableValueRedaction is set (regular and wildcard trusted-origin branches) - non-ErrNotFound extractor error forwarded to the error handler - storage fetch error after a successful double-submit comparison - DeleteToken: missing cookie and storage delete failure - DeleteToken with the session loaded by the session middleware - storage manager: memory unexpected-type assertion and storage success paths - session manager: store load and save error branches for getRaw/setRaw/delRaw The only lines left uncovered are unreachable defensive branches in subdomain.match, guarded by an earlier length check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MW3kyDqPzUUQ9iuRqWP94Y
Add dedicated security_test.go files covering attack/defense cases that were not yet asserted directly. CSRF (middleware/csrf/security_test.go): - constant-time token/string comparison logical correctness - Sec-Fetch-Site normalization: case-insensitive and whitespace-trimmed acceptance, with unknown/embedded-space values still rejected - double-submit mismatch (valid header token vs different valid cookie token) rejected with ErrTokenInvalid - forged token consistent across header/cookie but absent from storage is rejected and the stale cookie is expired - cookie security attributes (HttpOnly, Secure, SameSite, Path) are reflected on the Set-Cookie response - HTTPS request with an http:// Origin for the same host is rejected (scheme is part of the origin) - trusting an origin under one scheme does not trust the other scheme CORS (middleware/cors/security_test.go): - AllowCredentials with a wildcard-subdomain pattern reflects a matching origin verbatim (never "*") with credentials, and denies non-matching, apex, and scheme-downgraded origins - credentialed-origin reflection also applies to simple (non-preflight) requests - a disallowed origin is never reflected back even with multiple configured origins Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MW3kyDqPzUUQ9iuRqWP94Y
Move the coverage and security tests out of the standalone coverage_test.go / security_test.go files and into the existing test files, matching each test to the source file it exercises: - CORS: all tests (cors.go helpers and security scenarios) -> cors_test.go - CSRF: validateExtractorSecurity nil guard -> config_test.go; constant-time comparison helpers -> helpers_test.go; remaining coverage and security tests (extractor, storage/session managers, DeleteToken, cookie attributes, Sec-Fetch-Site, double submit, etc.) -> csrf_test.go No test logic changed; only relocation plus the required imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MW3kyDqPzUUQ9iuRqWP94Y
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds CORS tests for nil-config helper behavior and credentialed origin handling. Adds CSRF tests for nil-config validation, constant-time comparison helpers, storage and session error paths, and security checks for fetch-site, tokens, cookies, and origin matching. ChangesMiddleware test coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 #4462 +/- ##
==========================================
+ Coverage 91.73% 92.02% +0.28%
==========================================
Files 138 138
Lines 13486 13486
==========================================
+ Hits 12371 12410 +39
+ Misses 705 682 -23
+ Partials 410 394 -16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
middleware/csrf/csrf_test.go (1)
2804-2854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the session-backed token was actually deleted.
Line 2853 only checks the status code. If the in-context
delRawbranch regresses to a no-op, this test still passes. Reusing the same session/token for a second unsafe request, or asserting the response expires the CSRF cookie, would validate the behavior instead of just the code 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 `@middleware/csrf/csrf_test.go` around lines 2804 - 2854, The CSRF delete-token test only verifies the handler returns OK, so it can miss a regression where DeleteToken is a no-op in the session-backed path. Update Test_CSRF_DeleteToken_WithSessionMiddleware to assert the token was actually removed by reusing the same session/token for a second unsafe request or by checking the CSRF cookie is expired/cleared after calling HandlerFromContext(c).DeleteToken(c), so the test validates the deletion behavior rather than just the status code.
🤖 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 `@middleware/cors/cors_test.go`:
- Around line 1726-1729: The test
`Test_CORS_setSimpleHeaders_WildcardWithCredentials` is changing package-global
logger state and restoring it to `os.Stderr`, which can clobber another test’s
redirected output. Update the setup/cleanup around `log.SetOutput` to use the
repo’s log-capture helper or save and restore the exact previous writer instead
of hardcoding `os.Stderr`. Keep the fix localized to this test so later
assertions don’t inherit the wrong logger destination.
In `@middleware/csrf/csrf_test.go`:
- Around line 3133-3162: The CSRF cookie attribute test in
Test_CSRF_Security_CookieAttributes should also assert the __Host- prefix
host-only requirement. Update the test to verify the cookie returned by
setCSRFCookie has an empty Domain field, alongside the existing HttpOnly,
Secure, SameSite, Path, and value checks, so regressions from copying
Config.CookieDomain into the response cookie are caught.
---
Nitpick comments:
In `@middleware/csrf/csrf_test.go`:
- Around line 2804-2854: The CSRF delete-token test only verifies the handler
returns OK, so it can miss a regression where DeleteToken is a no-op in the
session-backed path. Update Test_CSRF_DeleteToken_WithSessionMiddleware to
assert the token was actually removed by reusing the same session/token for a
second unsafe request or by checking the CSRF cookie is expired/cleared after
calling HandlerFromContext(c).DeleteToken(c), so the test validates the deletion
behavior rather than just the status code.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5c63b30-3ab6-44d4-a8ad-4a0613e0b8e9
📒 Files selected for processing (4)
middleware/cors/cors_test.gomiddleware/csrf/config_test.gomiddleware/csrf/csrf_test.gomiddleware/csrf/helpers_test.go
…-only - Reorder the Sec-Fetch-Site table struct fields (error first) to satisfy the govet fieldalignment linter that was failing CI. - Assert the CSRF cookie has an empty Domain in the cookie-attributes test so a regression that scopes the __Host- cookie to a domain is caught (addresses PR review feedback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MW3kyDqPzUUQ9iuRqWP94Y
ReneWerner87
approved these changes
Jun 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add targeted tests to push CORS and CSRF middleware coverage to their
practical maximum (CORS 96.2% -> 98.5%, CSRF 94.3% -> 99.6%).
CORS:
CSRF:
(regular and wildcard trusted-origin branches)
success paths
getRaw/setRaw/delRaw
The only lines left uncovered are unreachable defensive branches in
subdomain.match, guarded by an earlier length check.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01MW3kyDqPzUUQ9iuRqWP94Y