-
Notifications
You must be signed in to change notification settings - Fork 30.4k
fix: CSRF origin matching should be case-insensitive #89127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level: Yes, this looks like a good change.
I'm a bit concerned about unicode support though, so I'm requesting changes. See my inline comment.
| const normalizedDomain = domain.toLowerCase().replace(/\.$/, '') | ||
| const normalizedPattern = pattern.toLowerCase().replace(/\.$/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this handle punycode domain names? Is it possible that this gets a unicode-encoded domain and tries to lowercase that, because that would be a security vulnerability as RFC 1035 only covers 7-bit ASCII. It's hard for me to know for certain if this function gets the punycode representation or the unicode representation.
You can address this in one of two ways:
- Write your own
toLowerCaseimplementation that's careful to only modify A-Z. - Add an e2e test to tests/e2e that proves this does not try to lower-case unicode characters. I'm not sure how you'd mock the DNS lookup though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Implemented custom asciiLowerCase() that only modifies A-Z (65-90), leaving all other characters unchanged per RFC 1035.
Added tests for unicode/punycode handling. Also updated baseline-browser-mapping to 2.9.19 to fix the test failures.
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Tests Passed |
|
The test failures appear to be unrelated to the CSRF changes in this PR. All failing tests are showing The warning appears in multiple test suites:
Would you like me to update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the trailing dot: After some further research, it does look like browsers do sometimes treat it as a separate, non-equivalent, hostname:
The example.com and example.com. domains are not equivalent and typically treated as distinct.
-- https://url.spec.whatwg.org/#concept-host
Though apparently the whole spec is a mess:
Because this is security critical code where being overly restrictive isn't a security issue, but not being restrictive enough could result in a CVE, let's be conservative treat the trailing dot as a separate host/domain, like browsers do.
I don't see any practical way the trailing dot could be abused (and I had a chat with Claude to double-check), but I also don't think anyone was running into problems with that. I can believe people were running into case sensitivity issues, but that's more straightforward because all the specs are clear and agree about ASCII case-insensitive matching.
Would you like me to update baseline-browser-mapping to the latest version as part of this PR, or should this be handled separately?
No thanks, just rebase on top of canary. #89175 should've fixed that issue.
| // ASCII portion should still be case-insensitive | ||
| expect(isCsrfOriginAllowed('münchen.COM', ['münchen.com'])).toBe(true) | ||
| expect(isCsrfOriginAllowed('MÜNCHEN.COM', ['MÜNCHEN.com'])).toBe(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does a better job of testing what the comment suggests:
| // ASCII portion should still be case-insensitive | |
| expect(isCsrfOriginAllowed('münchen.COM', ['münchen.com'])).toBe(true) | |
| expect(isCsrfOriginAllowed('MÜNCHEN.COM', ['MÜNCHEN.com'])).toBe(true) | |
| // ASCII portion should still be case-insensitive | |
| expect(isCsrfOriginAllowed('MüNCHEN.COM', ['münchen.com'])).toBe(true) | |
| expect(isCsrfOriginAllowed('mÜnchen.COM', ['MÜNCHEN.com'])).toBe(true) |
| "@next/env": "16.2.0-canary.13", | ||
| "@swc/helpers": "0.5.15", | ||
| "baseline-browser-mapping": "^2.8.3", | ||
| "baseline-browser-mapping": "^2.9.19", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this. The CI failures should be fixed by #89175
|
Looks like you need to run |
Fixes RFC 1035 compliance by making DNS name matching case-insensitive. Uses ASCII-only character replacement instead of toLowerCase() to avoid potential unicode normalization issues. Tests added for case-insensitive matching across exact matches and wildcard patterns.
19df7c6 to
6eebd5f
Compare
|
Updated PR based on feedback: ✅ Rebased on latest canary The PR now only addresses case-insensitive DNS name matching per RFC 1035, using ASCII-only character replacement to avoid unicode issues. |
|
Thanks for the contribution, @jaffarkeikei! Sorry about all the extra back-and-forth. Because this is code is security-related, I had to be careful that we're not screwing anything up. |
Summary
This PR fixes two bugs in the
isCsrfOriginAllowedfunction used for Server Actions CSRF protection:Case sensitivity bug: Domain matching was case-sensitive, but DNS names are case-insensitive per RFC 1035. This caused legitimate requests to fail CSRF checks when the Origin header contained uppercase characters.
Trailing dot bug: FQDNs with trailing dots (e.g.,Edit: Reverted, see comments on PRexample.com.) weren't matched. The trailing dot is valid DNS notation representing the root zone.The Problem
Before this fix:
Both of these should return
truebecause:The Fix
Normalize both domain and pattern before comparison:
Testing
Added new test cases for:
All existing tests continue to pass.
Related
serverActions.allowedOriginsinnext.config.js