fix(v2): prevent origin bypass via suffix match in wildcardPatternToRegex#5074
fix(v2): prevent origin bypass via suffix match in wildcardPatternToRegex#5074leaanthony wants to merge 1 commit intomasterfrom
Conversation
…egex Wildcards in BindingsAllowedOrigins patterns are now only expanded when they represent a full domain component (after "://", ".", or ":"). A wildcard appended to a partial label (e.g. "com*") is stripped, preventing attacker-controlled domains like "myapp.community" from bypassing origin checks when "https://myapp.com*" is configured. Fixes GHSA-47hv-j4px-h3c9 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes refine wildcard pattern matching in origin validation by restricting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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. 🔧 OpenGrep (1.16.5)v2/internal/frontend/originvalidator/originValidator.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m v2/internal/frontend/originvalidator/originValidator_test.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m 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.
🧹 Nitpick comments (2)
v2/internal/frontend/originvalidator/originValidator.go (1)
95-105: Solid implementation of separator-anchored wildcard expansion.The ordered replacements correctly handle overlapping patterns (
://before:) and the{0,}quantifier avoids collision with the cleanup step. The character class[^.:/]effectively prevents cross-boundary matching.Consider documenting the single-level subdomain constraint.
The
[^.:/]constraint meanshttps://*.myapp.commatcheshttps://api.myapp.combut nothttps://a.b.myapp.com. Users requiring multi-level subdomain matching would needhttps://*.*.myapp.com. This is a reasonable security-first tradeoff, but adding a brief note in the docstring (lines 80-85) would help users configure patterns correctly.📝 Optional: Add documentation note about multi-level subdomains
// wildcardPatternToRegex converts wildcard pattern to regex. // // Wildcards are only expanded when they represent a full domain component, // i.e. immediately after "://", ".", or ":". A wildcard appended to a // partial label (e.g. "com*") is stripped so that it cannot match across // domain boundaries (GHSA-47hv-j4px-h3c9). +// +// Note: A single wildcard matches a single domain level only. For example, +// "https://*.myapp.com" matches "https://api.myapp.com" but not +// "https://a.b.myapp.com". Use "https://*.*.myapp.com" for multi-level matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/internal/frontend/originvalidator/originValidator.go` around lines 95 - 105, Add a brief docstring note in the originvalidator package (near the comment that introduces the wildcard expansion) explaining that the regex uses [^.:/] to anchor wildcards to a single path/host component so patterns like "https://*.myapp.com" match "https://api.myapp.com" but not "https://a.b.myapp.com"; mention that multi-level subdomains must be expressed explicitly (e.g., "https://*.*.myapp.com"). Reference the code performing the replacements on the variable escaped (the "//*", "\\.*", ":*" and trailing "*" cleanup) so readers can see why the single-level constraint exists.v2/internal/frontend/originvalidator/originValidator_test.go (1)
16-94: Good coverage of the vulnerability vectors.The test cases effectively validate the security fix, covering trailing wildcard bypass attempts, exact matching, and boundary separators.
Consider adding these edge cases for completeness:
- Multi-level subdomain non-match — Verifies the single-level constraint is enforced:
{ name: "subdomain wildcard does not match nested subdomain", allowedOrigins: "https://*.myapp.com", origin: "https://a.b.myapp.com", expected: false, },
- Port wildcard — Verifies port-level wildcard works correctly:
{ name: "port wildcard matches different port", allowedOrigins: "https://localhost:*", origin: "https://localhost:8080", expected: true, },
- Base domain with subdomain wildcard — Verifies
*.domainrequires an actual subdomain:{ name: "subdomain wildcard does not match base domain", allowedOrigins: "https://*.myapp.com", origin: "https://myapp.com", expected: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/internal/frontend/originvalidator/originValidator_test.go` around lines 16 - 94, Add three edge-case table entries to TestWildcardPatternDoesNotCrossDomainBoundaries to cover multi-level subdomain rejection, port wildcard matching, and that a subdomain wildcard does not match the base domain: add a test case named "subdomain wildcard does not match nested subdomain" with allowedOrigins "https://*.myapp.com", origin "https://a.b.myapp.com", expected false; add "port wildcard matches different port" with allowedOrigins "https://localhost:*", origin "https://localhost:8080", expected true; and add "subdomain wildcard does not match base domain" with allowedOrigins "https://*.myapp.com", origin "https://myapp.com", expected false so the TestWildcardPatternDoesNotCrossDomainBoundaries table covers these edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/internal/frontend/originvalidator/originValidator_test.go`:
- Around line 16-94: Add three edge-case table entries to
TestWildcardPatternDoesNotCrossDomainBoundaries to cover multi-level subdomain
rejection, port wildcard matching, and that a subdomain wildcard does not match
the base domain: add a test case named "subdomain wildcard does not match nested
subdomain" with allowedOrigins "https://*.myapp.com", origin
"https://a.b.myapp.com", expected false; add "port wildcard matches different
port" with allowedOrigins "https://localhost:*", origin
"https://localhost:8080", expected true; and add "subdomain wildcard does not
match base domain" with allowedOrigins "https://*.myapp.com", origin
"https://myapp.com", expected false so the
TestWildcardPatternDoesNotCrossDomainBoundaries table covers these edge cases.
In `@v2/internal/frontend/originvalidator/originValidator.go`:
- Around line 95-105: Add a brief docstring note in the originvalidator package
(near the comment that introduces the wildcard expansion) explaining that the
regex uses [^.:/] to anchor wildcards to a single path/host component so
patterns like "https://*.myapp.com" match "https://api.myapp.com" but not
"https://a.b.myapp.com"; mention that multi-level subdomains must be expressed
explicitly (e.g., "https://*.*.myapp.com"). Reference the code performing the
replacements on the variable escaped (the "//*", "\\.*", ":*" and trailing "*"
cleanup) so readers can see why the single-level constraint exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17e09bc8-a9ea-42fd-82fa-d9c39e711757
📒 Files selected for processing (2)
v2/internal/frontend/originvalidator/originValidator.gov2/internal/frontend/originvalidator/originValidator_test.go
There was a problem hiding this comment.
Pull request overview
Addresses a security issue in the v2 frontend origin validation used by BindingsAllowedOrigins, preventing wildcard patterns from being abused as suffix matches to bypass origin restrictions (GHSA-47hv-j4px-h3c9).
Changes:
- Tightens
wildcardPatternToRegexso*only expands when used as a full component (after://,., or:) and strips other*usage to prevent suffix-based origin bypass. - Adds a new table-driven unit test suite covering wildcard boundary behavior, trailing-wildcard rejection, exact matches, and empty-origin rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
v2/internal/frontend/originvalidator/originValidator.go |
Updates wildcard-to-regex conversion to prevent suffix-based origin bypass. |
v2/internal/frontend/originvalidator/originValidator_test.go |
Adds unit tests validating the new wildcard/origin matching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // component. Order matters: handle :// before : to avoid partial overlap. | ||
| // Use {0,} instead of * as the regex quantifier to avoid collision with | ||
| // the literal * cleanup in the next step. | ||
| escaped = strings.ReplaceAll(escaped, "//*", "//[^.:/]{0,}") |
There was a problem hiding this comment.
The comment says wildcards are expanded only immediately after "://", but the implementation replaces the substring "//". That also expands wildcards after any "//" (e.g., in a path) and makes the following note about handling "://" before ":" a bit misleading. Consider changing the replacement to target exactly "://" (or "http(s)://*") to match the documented rule, or adjust the comment to reflect the broader matching behavior.
| escaped = strings.ReplaceAll(escaped, "//*", "//[^.:/]{0,}") | |
| escaped = strings.ReplaceAll(escaped, "://*", "://[^.:/]{0,}") |
| func TestWildcardPatternDoesNotCrossDomainBoundaries(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| allowedOrigins string | ||
| origin string | ||
| expected bool | ||
| }{ | ||
| // Legitimate subdomain wildcard — should still work |
There was a problem hiding this comment.
The PR description mentions 12 new test cases, but this table-driven test currently defines 11 entries. Either add the missing case or update the description so it matches what was actually added.
Deploying wails with
|
| Latest commit: |
35437f8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d46166fd.wails.pages.dev |
| Branch Preview URL: | https://fix-origin-validator-bypass.wails.pages.dev |
Saku0512
left a comment
Saku0512
left a comment
There was a problem hiding this comment.
Thanks for the fix! The implementation looks correct — restricting wildcard
expansion to separator boundaries and stripping trailing wildcards properly
addresses the bypass.
One minor note: the inline comment says wildcards are only expanded
"immediately after ://" but the implementation also handles "." and ":"
separators. The comment should be updated to reflect that.
Summary
wildcardPatternToRegexallowed origin bypass via suffix match inBindingsAllowedOrigins*) are now only expanded when preceded by a separator (://,., or:), ensuring they match full domain components onlycom*) is stripped, preventinghttps://myapp.communityfrom matchinghttps://myapp.com*Risk Assessment
https://*.myapp.comworks identicallymyapp.com*matchingmyapp.community)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests