fix: use segment-based domain matching for CSRF origin wildcard patterns#821
fix: use segment-based domain matching for CSRF origin wildcard patterns#821james-elicx merged 1 commit intomainfrom
Conversation
The wildcard origin matching in isOriginAllowed() used endsWith() for suffix matching, so a pattern like *.example.com would match any origin whose hostname ended with .example.com, including attacker-controlled domains like evil.example.com.attacker.com. Replace with segment-by-segment domain matching ported from Next.js: - * matches exactly one DNS label - ** matches one or more DNS labels - wildcards are blocked from matching entire domains - matching is case-insensitive per RFC 1035 Ported from Next.js: packages/next/src/server/app-render/csrf-protection.ts https://github.com/vercel/next.js/blob/canary/packages/next/src/server/app-render/csrf-protection.ts Tests ported from Next.js csrf-protection.test.ts plus an explicit regression test for the attacker-controlled suffix domain case.
|
/bigbonk review this PR |
commit: |
|
|
@southpolesteve Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix for a real vulnerability. The old String.endsWith() approach would match evil.example.com.attacker.com against *.example.com, which is a CSRF bypass.
The port from Next.js's csrf-protection.ts is faithful — the matchWildcardDomain implementation matches the upstream logic correctly, and the test cases are comprehensive.
Two minor nits (neither blocking):
-
Stale JSDoc comment at line 243-246 — the old
isOriginAllowedcomment (Check if an origin matches any pattern...) was left as a dangling block immediately before the newmatchWildcardDomainJSDoc. It should be removed or merged into the new comment since it's now orphaned. -
Missing
undefinedcase label in switch — Next.js hascase undefined: default:in their switch statement, while this port only hasdefault:. Functionally equivalent sinceundefinedfalls through todefault, but mentioning for completeness. Not worth changing.
The case-insensitive comparison in isOriginAllowed using .toLowerCase() (line 294) differs from matchWildcardDomain's ASCII-only replace(/[A-Z]/g, ...), but this matches the Next.js upstream where isCsrfOriginAllowed also uses the regex approach for both paths. Either way, for DNS names (ASCII-only), .toLowerCase() and the regex approach are equivalent — no issue here.
Tests are well-structured and cover all the important cases including the regression case (attacker-controlled suffix domains).
| @@ -244,13 +244,54 @@ export async function validateServerActionPayload( | |||
| * Check if an origin matches any pattern in the allowed origins list. | |||
| * Supports wildcard subdomains (e.g. `*.example.com`). | |||
| */ | |||
There was a problem hiding this comment.
Nit: This old JSDoc block is now orphaned — it was the comment for the previous isOriginAllowed function, but now it sits above matchWildcardDomain which has its own JSDoc immediately below. Should be removed to avoid confusion.
|
The review has been posted. Here's a summary of my review: PR #821 Review: Approved This PR fixes a real CSRF vulnerability where the old The fix properly ports Next.js's segment-by-segment domain matching from
All 12 ported tests pass. One minor nit was left as an inline comment: there's a stale JSDoc block (the old |
2 participants
Summary
Replace suffix-based wildcard origin matching with proper segment-by-segment DNS domain matching for server action CSRF origin validation.
Details
isOriginAllowed()usedString.endsWith()for wildcard patterns, so*.example.comwould match any hostname ending with.example.com, includingevil.example.com.attacker.com.The new
matchWildcardDomain()implementation splits both domain and pattern by.and compares segment-by-segment from right to left:*matches exactly one DNS label**matches one or more DNS labels*or**alone)Ported from Next.js:
csrf-protection.tscsrf-protection.test.tsTests
12 unit tests ported from Next.js covering:
*)**)