🐛 bug: validate wildcard CORS origins before matching#4438
Conversation
|
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)
Walkthrough
ChangesCORS subdomain match normalization guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
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 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 |
![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
🤖 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/utils_test.go`:
- Around line 206-211: The table-driven test in this file needs to be
parallelized according to repository guidelines. Add t.Parallel() at the start
of the parent test function that contains this subdomain test table, and also
add t.Parallel() as the first statement within the subtest callback passed to
t.Run(). This applies to the test function that defines the table containing the
test case with the "no match with malformed origin port before suffix" name.
🪄 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: efae52bf-39de-4b2a-ab1c-e48f9d6d0156
📒 Files selected for processing (2)
middleware/cors/utils.gomiddleware/cors/utils_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils_test.go`:
- Around line 212-219: The test case entry for "no match with empty label before
suffix" is missing its opening brace, making the slice literal syntactically
invalid. Add an opening brace `{` before the `name` field of this test case
entry to properly define the struct literal. The entry contains fields `name`,
`sub` with subdomain prefix and suffix values, `origin`, and `expected`, so
ensure the opening brace is placed before `name` to correctly wrap the complete
test case definition.
🪄 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: 87da7c9b-d40c-4b1d-82b8-cde596e5ec37
📒 Files selected for processing (2)
middleware/cors/utils.gomiddleware/cors/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/cors/utils.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/utils_test.go`:
- Around line 212-219: The test case entry for "no match with empty label before
suffix" is missing its opening brace, making the slice literal syntactically
invalid. Add an opening brace `{` before the `name` field of this test case
entry to properly define the struct literal. The entry contains fields `name`,
`sub` with subdomain prefix and suffix values, `origin`, and `expected`, so
ensure the opening brace is placed before `name` to correctly wrap the complete
test case definition.
🪄 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: 87da7c9b-d40c-4b1d-82b8-cde596e5ec37
📒 Files selected for processing (2)
middleware/cors/utils.gomiddleware/cors/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/cors/utils.go
🛑 Comments failed to post (1)
middleware/cors/utils_test.go (1)
212-219:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix malformed table entry that breaks compilation.
At Line 214, the next test case starts without an opening
{, so the slice literal is syntactically invalid and causes the parser errors reported in CI.Suggested patch
{ name: "no match with malformed origin port before suffix", sub: subdomain{prefix: "https://", suffix: "example.com"}, origin: "https://evil.com:any.example.com", - expected: false, - }, + expected: false, + }, + { name: "no match with empty label before suffix", sub: subdomain{prefix: "https://", suffix: "example.com"}, origin: "https://foo..example.com", expected: false, },📝 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.expected: false, }, { name: "no match with empty label before suffix", sub: subdomain{prefix: "https://", suffix: "example.com"}, origin: "https://foo..example.com", expected: false, }, }🧰 Tools
🪛 GitHub Actions: Linter / 0_lint _ lint.txt
[error] 219-219: golangci-lint reported a syntax error: unexpected }, expected expression
🪛 GitHub Actions: Linter / lint _ lint
[error] 219-219: golangci-lint reported a syntax error: "unexpected }, expected expression" at utils_test.go:219:2.
🪛 GitHub Check: lint / lint
[failure] 219-219:
expected operand, found '}' (typecheck)
[failure] 219-219:
syntax error: unexpected }, expected expression🪛 GitHub Check: unit (1.25.x, macos-latest)
[failure] 219-219:
expected operand, found '}'🪛 GitHub Check: unit (1.25.x, ubuntu-latest)
[failure] 219-219:
expected operand, found '}'🪛 GitHub Check: unit (1.25.x, windows-latest)
[failure] 219-219:
expected operand, found '}'🪛 GitHub Check: unit (1.26.x, windows-latest)
[failure] 219-219:
expected operand, found '}'🤖 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/cors/utils_test.go` around lines 212 - 219, The test case entry for "no match with empty label before suffix" is missing its opening brace, making the slice literal syntactically invalid. Add an opening brace `{` before the `name` field of this test case entry to properly define the struct literal. The entry contains fields `name`, `sub` with subdomain prefix and suffix values, `origin`, and `expected`, so ensure the opening brace is placed before `name` to correctly wrap the complete test case definition.Source: Linters/SAST tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4438 +/- ##
=======================================
Coverage 91.59% 91.59%
=======================================
Files 134 134
Lines 13515 13518 +3
=======================================
+ Hits 12379 12382 +3
Misses 722 722
Partials 414 414
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:
|
2 participants
Motivation
https://*.example.comfrom matching malformed Origin headers such ashttps://evil.com:any.example.comthat are not valid serialized origins, which could be reflected inAccess-Control-Allow-OriginwhenAllowCredentialsis enabled.Description
normalizeOriginand requiring the normalized form to equal the raw Origin before applying the existing prefix/suffix subdomain matching insubdomain.match(changed inmiddleware/cors/utils.go).https://evil.com:any.example.comdoes not matchhttps://*.example.com(added tomiddleware/cors/utils_test.go).Testing
go test ./middleware/cors -run Test_CORS_SubdomainMatch -count=1, which passed (ok github.com/gofiber/fiber/v3/middleware/cors 0.019s).make generate,make betteralign,make format,make lint, andmake testall completed successfully;make testran the full suite (DONE 3643 tests, 1 skipped).make auditreported failures fromgovulnchecktargeting the installed Go standard library (25 vulnerabilities reported for the toolchain), causingmake auditto fail; this is an environment/toolchain vulnerability report and not a regression in the patch itself.