fix: centralize protocol-relative URL guard to handle percent-encoded delimiters#888
Conversation
… delimiters
Six places in the request pipeline inlined a variation of:
pathname.replaceAll("\\", "/").startsWith("//")
to reject protocol-relative paths before they reach the trailing-slash
redirect emitter (which would otherwise echo them into a Location header).
The check handles literal delimiters but not their percent-encoded forms —
`/%5Cevil.com/` survives the guard, then `normalizePathnameForRouteMatchStrict`
re-encodes the decoded backslash back to `%5C` (preserving it as part of a
single path segment), so the 308 trailing-slash redirect ends up with
`Location: /%5Cevil.com`. Browsers percent-decode Location headers and
WHATWG URL treats backslash as forward slash, so the browser resolves that
to a protocol-relative host.
Factor the leading-segment shape check into a new `isOpenRedirectShaped`
helper in `server/request-pipeline.ts` that recognises literal (`//`, `/\\`)
and percent-encoded (`%5C`, `%2F`) variants, and swap each call site over
to it. Also add a defense-in-depth check in `normalizeTrailingSlash` so a
future caller that skips the upstream guard still can't emit a bad Location.
Updated call sites:
- server/request-pipeline.ts — guardProtocolRelativeUrl + normalizeTrailingSlash
- server/prod-server.ts — App Router and Pages Router Node handlers
- server/app-router-entry.ts — default Cloudflare Worker entry
- server/app-ssr-entry.ts — SSR environment entry
- index.ts — Pages Router Vite dev middleware
- deploy.ts — generated Pages Router Cloudflare Worker (inlined, kept in sync)
Regression tests in tests/request-pipeline.test.ts cover the encoded
variants plus the defense-in-depth path. Updates the pinned string
assertion in tests/deploy.test.ts that was checking for the old inlined
pattern.
commit: |
|
|
/bigbonk review please. Also can you be sure this fix is comprehensive. Did we get it everywhere? |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The fix is correct, well-tested, and appropriately defensive. Approving.
Root cause check ✓
Walked through the attack on main (commit before this PR):
GET /%5Cevil.com/arrives- Existing guard:
pathname.replaceAll("\\", "/").startsWith("//")— literal backslash replacement does not touch%5C, so/%5Cevil.com/still starts with/%, guard passes normalizePathnameForRouteMatchStrictdecodes%5Cto\segment-wise, then re-encodes unsafe bytes inside a segment, leaving/%5Cevil.com/as a single segmenttrailingSlash: false+ path ends with/→ 308 withLocation: /%5Cevil.com- Browser percent-decodes the Location header; WHATWG URL treats
\as/; resolves to//evil.com→http://evil.com/
isOpenRedirectShaped correctly catches this at step 2 by examining the leading bytes before decode, and the defense-in-depth check in normalizeTrailingSlash catches it at step 4 even if an upstream caller is skipped.
Correctness of isOpenRedirectShaped ✓
- Case sensitivity:
.toLowerCase()onafterSlash.slice(0, 3)handles both%5C/%5cand%2F/%2f. RFC 3986 §2.1 confirms percent-encoding is case-insensitive. - Length guard:
afterSlash.length >= 3means a truncated/%or/%Zcorrectly falls through so the downstream decode path can return 400 rather than 404. Testreturns null for malformed percent-encodinglocks this in. - No false positives: UTF-8 encoded paths like
/%E4%B8%ADhaveafterSlash.slice(0, 3) === "%e4", which matches neither%5cnor%2f. Locked in byreturns null when % appears after the leading slash but not as a delimiter. - Leading-segment only:
/foo/%5Cbarcorrectly passes because the check only examines position 0 after the leading/. Legitimate (if unusual) encoded paths elsewhere still resolve normally.
Call-site coverage ✓
All six inlined copies migrated, plus deploy.ts gets an inlined mirror with a "keep in sync" comment (necessary because the generated worker can't import from local source at build time). Spot-checked rg 'startsWith\("//"\)' for stragglers — remaining hits are in unrelated contexts (image URL validation, client-side router redirect guards, full-URL detection for http:///https://), none of which emit Location headers for path-shaped open redirects. The image-optimization validator has its own independent resolved.origin !== base defense-in-depth check that already blocks the encoded-backslash variant.
Also verified the reorder in prod-server.ts and app-router-entry.ts: the guard now runs before normalizePathnameForRouteMatchStrict / decodeURIComponent, which is the key correctness property. Previously the literal-only guard could run after decode since decode doesn't introduce //, but the encoded-check must run pre-decode.
Test coverage ✓
tests/request-pipeline.test.ts covers:
- Literal
//,/\ - Encoded
%5C,%5c,%2F,%2f - Double-encoded
%5C%5C - Negative cases: UTF-8 chars, single encoded ASCII (
/%61dmin), encoded delimiters after the first segment, malformed percent-encoding - Defense-in-depth via
normalizeTrailingSlashdirectly, asserting both the 404 status and absentLocationheader for bothtrailingSlash: trueandfalse
tests/deploy.test.ts verifies the generated worker actually inlines the new helper with the %5c/%2f constants and calls it against pathname.
Local verification
vp test run tests/request-pipeline.test.ts tests/deploy.test.ts tests/shims.test.ts→ 1100/1100 passvp linton all 6 modified source files → cleanvp check— only pre-existing unrelated error (benchmarks/vinext/vite.config.tsworkspace link), no issues introduced by this PR
Minor observation (non-blocking)
deploy.ts keeps an inlined copy of isOpenRedirectShaped. The comment explicitly calls out the "keep in sync" requirement and the deploy test now asserts the inlined implementation contains %5c/%2f, which is a reasonable guard against drift. If the two copies diverge in the future, extracting a shared template string or a codegen helper would remove this footgun — but that's a refactor for another PR, not a blocker here.
LGTM.
|
Posted an approving review on PR #888. Summary of the analysis: Verdict: Approved. The fix is correct. Key findings:
Minor non-blocking note: the inlined copy in |
southpolesteve
deleted the
fix/normalize-encoded-delimiters-in-protocol-guard
branch
1 participant
Summary
Six places in the request pipeline inlined a variation of:
to reject protocol-relative paths before they reach the trailing-slash redirect emitter (which would otherwise echo them back into a
Locationheader). The check handles literal delimiters but not their percent-encoded forms —/%5Cevil.com/survives the guard, thennormalizePathnameForRouteMatchStrictre-encodes the decoded backslash back to%5C(preserving it as part of a single path segment), so the 308 trailing-slash redirect ends up withLocation: /%5Cevil.com. Browsers percent-decodeLocationheaders and WHATWG URL treats\as/, so the browser resolves that to a protocol-relative host.Changes
Factor the leading-segment shape check into a new
isOpenRedirectShapedhelper inserver/request-pipeline.tsthat recognises literal (//,/\) and percent-encoded (%5C,%2F) variants, and swap each call site over to it. Also add a defense-in-depth check innormalizeTrailingSlashso a future caller that skips the upstream guard still can't emit a badLocation.Updated call sites:
server/request-pipeline.ts—guardProtocolRelativeUrl+normalizeTrailingSlashserver/prod-server.ts— App Router and Pages Router Node handlersserver/app-router-entry.ts— default Cloudflare Worker entryserver/app-ssr-entry.ts— SSR environment entryindex.ts— Pages Router Vite dev middlewaredeploy.ts— generated Pages Router Cloudflare Worker (inlined copy kept in sync)Tests
Regression tests in
tests/request-pipeline.test.tscover the encoded variants plus the defense-in-depth path. Updates the pinned string assertion intests/deploy.test.tsthat was checking for the old inlined pattern.