Skip to content

Commit 40ed683

Browse files
authored
fix(start-server-core): redirect misinterpreted protocol-relative URLs (#6600)
* add test to verify redirect * handle redirect * update tests * cleanup unused code in decodePath * use 308 instead of 302 * redirect is not relevant in spa mode
1 parent 172fa05 commit 40ed683

File tree

8 files changed

+71
-113
lines changed

8 files changed

+71
-113
lines changed

e2e/react-start/basic/tests/open-redirect-prevention.spec.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { expect } from '@playwright/test'
22
import { test } from '@tanstack/router-e2e-utils'
3+
import { isSpaMode } from './utils/isSpaMode'
34

45
test.use({
56
whitelistErrors: [
@@ -80,7 +81,7 @@ test.describe('Open redirect prevention', () => {
8081
// the result could be //evil.com/ which is a protocol-relative URL
8182
// Our fix collapses these to /evil.com/ to prevent external redirects
8283
// This is already tested above, but we verify the collapsed path works
83-
await page.goto('/%0d/test-path/')
84+
const res = await page.goto('/%0d/test-path/')
8485
await page.waitForLoadState('networkidle')
8586

8687
// Should stay on the same origin
@@ -89,6 +90,9 @@ test.describe('Open redirect prevention', () => {
8990
expect(url.origin).toBe(new URL(baseURL!).origin)
9091
// Path should be collapsed to /test-path (not //test-path/)
9192
expect(url.pathname).toMatch(/^\/test-path\/?$/)
93+
if (!isSpaMode) {
94+
expect(res?.request().redirectedFrom()?.url()).not.toBe(undefined)
95+
}
9296
})
9397
})
9498

packages/router-core/src/router.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1268,14 +1268,14 @@ export class RouterCore<
12681268
return {
12691269
href: pathname + searchStr + hash,
12701270
publicHref: href,
1271-
pathname: decodePath(pathname),
1271+
pathname: decodePath(pathname).path,
12721272
external: false,
12731273
searchStr,
12741274
search: replaceEqualDeep(
12751275
previousLocation?.search,
12761276
parsedSearch,
12771277
) as any,
1278-
hash: decodePath(hash.slice(1)),
1278+
hash: decodePath(hash.slice(1)).path,
12791279
state: replaceEqualDeep(previousLocation?.state, state),
12801280
}
12811281
}
@@ -1297,11 +1297,11 @@ export class RouterCore<
12971297
return {
12981298
href: fullPath,
12991299
publicHref: href,
1300-
pathname: decodePath(url.pathname),
1300+
pathname: decodePath(url.pathname).path,
13011301
external: !!this.rewrite && url.origin !== this.origin,
13021302
searchStr,
13031303
search: replaceEqualDeep(previousLocation?.search, parsedSearch) as any,
1304-
hash: decodePath(url.hash.slice(1)),
1304+
hash: decodePath(url.hash.slice(1)).path,
13051305
state: replaceEqualDeep(previousLocation?.state, state),
13061306
}
13071307
}
@@ -1879,7 +1879,7 @@ export class RouterCore<
18791879
decoder: this.pathParamsDecoder,
18801880
server: this.isServer,
18811881
}).interpolatedPath,
1882-
)
1882+
).path
18831883

18841884
// Resolve the next search
18851885
let nextSearch = fromSearch

packages/router-core/src/ssr/createRequestHandler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ export function createRequestHandler<TRouter extends AnyRouter>({
3535
})
3636

3737
// normalizing and sanitizing the pathname here for server, so we always deal with the same format during SSR.
38-
const url = getNormalizedURL(request.url, 'http://localhost')
38+
const { url } = getNormalizedURL(request.url, 'http://localhost')
3939
const origin = getOrigin(request)
4040
const href = url.href.replace(url.origin, '')
4141

packages/router-core/src/ssr/ssr-server.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -398,13 +398,18 @@ export function getNormalizedURL(url: string | URL, base?: string | URL) {
398398
if (typeof url === 'string') url = url.replace('\\', '%5C')
399399

400400
const rawUrl = new URL(url, base)
401-
const decodedPathname = decodePath(rawUrl.pathname)
401+
const { path: decodedPathname, handledProtocolRelativeURL } = decodePath(
402+
rawUrl.pathname,
403+
)
402404
const searchParams = new URLSearchParams(rawUrl.search)
403405
const normalizedHref =
404406
decodedPathname +
405407
(searchParams.size > 0 ? '?' : '') +
406408
searchParams.toString() +
407409
rawUrl.hash
408410

409-
return new URL(normalizedHref, rawUrl.origin)
411+
return {
412+
url: new URL(normalizedHref, rawUrl.origin),
413+
handledProtocolRelativeURL,
414+
}
410415
}

packages/router-core/src/utils.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -593,21 +593,19 @@ export function escapeHtml(str: string): string {
593593
return str.replace(HTML_ESCAPE_REGEX, (match) => HTML_ESCAPE_LOOKUP[match]!)
594594
}
595595

596-
export function decodePath(path: string, decodeIgnore?: Array<string>): string {
597-
if (!path) return path
596+
export function decodePath(path: string) {
597+
if (!path) return { path, handledProtocolRelativeURL: false }
598598

599599
// Fast path: most paths are already decoded and safe.
600600
// Only fall back to the slower scan/regex path when we see a '%' (encoded),
601601
// a backslash (explicitly handled), a control character, or a protocol-relative
602602
// prefix which needs collapsing.
603603
// eslint-disable-next-line no-control-regex
604604
if (!/[%\\\x00-\x1f\x7f]/.test(path) && !path.startsWith('//')) {
605-
return path
605+
return { path, handledProtocolRelativeURL: false }
606606
}
607607

608-
const re = decodeIgnore
609-
? new RegExp(`${decodeIgnore.join('|')}`, 'gi')
610-
: /%25|%5C/gi
608+
const re = /%25|%5C/gi
611609
let cursor = 0
612610
let result = ''
613611
let match
@@ -620,11 +618,13 @@ export function decodePath(path: string, decodeIgnore?: Array<string>): string {
620618
// Prevent open redirect via protocol-relative URLs (e.g. "//evil.com")
621619
// After sanitizing control characters, paths like "/\r/evil.com" become "//evil.com"
622620
// Collapse leading double slashes to a single slash
621+
let handledProtocolRelativeURL = false
623622
if (result.startsWith('//')) {
623+
handledProtocolRelativeURL = true
624624
result = '/' + result.replace(/^\/+/, '')
625625
}
626626

627-
return result
627+
return { path: result, handledProtocolRelativeURL }
628628
}
629629

630630
/**

packages/router-core/tests/getNormalizedURL.test.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@ describe('getNormalizedURL', () => {
99
const normalizedUrl1 = getNormalizedURL(url1)
1010
const normalizedUrl2 = getNormalizedURL(url2)
1111

12-
expect(normalizedUrl1.pathname).toBe('/%EB%8C%80|/path')
13-
expect(normalizedUrl1.pathname).toBe(normalizedUrl2.pathname)
12+
expect(normalizedUrl1.url.pathname).toBe('/%EB%8C%80|/path')
13+
expect(normalizedUrl1.url.pathname).toBe(normalizedUrl2.url.pathname)
1414
expect(new URL(url1).pathname).not.toBe(new URL(url2).pathname)
1515

16-
expect(normalizedUrl1.search).toBe(`?query=%EB%8C%80%7C`)
17-
expect(normalizedUrl1.search).toBe(normalizedUrl2.search)
16+
expect(normalizedUrl1.url.search).toBe(`?query=%EB%8C%80%7C`)
17+
expect(normalizedUrl1.url.search).toBe(normalizedUrl2.url.search)
1818
expect(new URL(url1).search).not.toBe(new URL(url2).search)
1919
})
2020

@@ -120,9 +120,9 @@ describe('getNormalizedURL', () => {
120120
'should treat encoded URL specific characters correctly',
121121
({ url, expectedPathName, expectedHash, expectedSearchParams }) => {
122122
const normalizedUrl = getNormalizedURL(url)
123-
expect(normalizedUrl.pathname).toBe(expectedPathName)
124-
expect(normalizedUrl.search).toBe(expectedSearchParams)
125-
expect(normalizedUrl.hash).toBe(expectedHash)
123+
expect(normalizedUrl.url.pathname).toBe(expectedPathName)
124+
expect(normalizedUrl.url.search).toBe(expectedSearchParams)
125+
expect(normalizedUrl.url.hash).toBe(expectedHash)
126126
},
127127
)
128128
})

packages/router-core/tests/utils.test.ts

Lines changed: 32 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -501,116 +501,47 @@ describe('deepEqual', () => {
501501
})
502502

503503
describe('decodePath', () => {
504-
it('should decode a path segment with no ignored items existing', () => {
505-
const itemsToCheck = ['%25', '%5C']
506-
const stringToCheck =
507-
'https://mozilla.org/?x=%D1%88%D0%B5%D0%BB%D0%BB%D1%8B'
508-
const expectedResult = 'https://mozilla.org/?x=шеллы'
509-
510-
const result = decodePath(stringToCheck, itemsToCheck)
511-
512-
expect(result).toBe(expectedResult)
513-
})
514-
515-
it('should decode a path segment with one ignored character and one ignored item existing', () => {
516-
const itemsToCheck = ['%25']
517-
const stringToCheck =
518-
'https://mozilla.org/?x=%25%D1%88%D0%B5%5C%D0%BB%D0%BB%D1%8B'
519-
const expectedResult = 'https://mozilla.org/?x=%25ше\\ллы'
520-
521-
const result = decodePath(stringToCheck, itemsToCheck)
522-
523-
expect(result).toBe(expectedResult)
524-
})
525-
526-
it('should decode a path segment with multiple ignored characters and first ignored item existing', () => {
527-
const itemsToCheck = ['%25', '%5C']
528-
let stringToCheck =
529-
'https://mozilla.org/?x=%25%D1%88%D0%B5%D0%BB%D0%BB%D1%8B'
530-
let expectedResult = 'https://mozilla.org/?x=%25шеллы'
531-
532-
let result = decodePath(stringToCheck, itemsToCheck)
533-
534-
expect(result).toBe(expectedResult)
535-
536-
stringToCheck = 'https://mozilla.org/?x=%D1%88%D0%B5%5C%D0%BB%D0%BB%D1%8B'
537-
expectedResult = 'https://mozilla.org/?x=ше%5Cллы'
538-
539-
result = decodePath(stringToCheck, itemsToCheck)
540-
541-
expect(result).toBe(expectedResult)
542-
})
543-
544-
it('should decode a path segment with multiple ignored characters and other ignored item existing', () => {
545-
const itemsToCheck = ['%25', '%5C']
546-
let stringToCheck =
547-
'https://mozilla.org/?x=%5C%D1%88%D0%B5%D0%BB%D0%BB%D1%8B'
548-
let expectedResult = 'https://mozilla.org/?x=%5Cшеллы'
549-
550-
let result = decodePath(stringToCheck, itemsToCheck)
551-
552-
expect(result).toBe(expectedResult)
553-
554-
stringToCheck = 'https://mozilla.org/?x=%D1%88%D0%B5%5C%D0%BB%D0%BB%D1%8B'
555-
expectedResult = 'https://mozilla.org/?x=ше%5Cллы'
556-
557-
result = decodePath(stringToCheck, itemsToCheck)
558-
559-
expect(result).toBe(expectedResult)
560-
})
561-
562-
it('should decode a path segment with multiple ignored characters and multiple ignored items existing', () => {
563-
const itemsToCheck = ['%25', '%5C']
564-
const stringToCheck =
565-
'https://mozilla.org/?x=%25%D1%88%D0%B5%5C%D0%BB%D0%BB%D1%8B'
566-
const expectedResult = 'https://mozilla.org/?x=%25ше%5Cллы'
567-
568-
const result = decodePath(stringToCheck, itemsToCheck)
569-
570-
expect(result).toBe(expectedResult)
571-
})
572-
573504
it('should decode a path segment, ignoring `%` and `\\` by default, with multiple ignored items existing', () => {
574505
const stringToCheck =
575506
'https://mozilla.org/?x=%25%D1%88%D0%B5%5C%D0%BB%D0%BB%D1%8B%2F'
576507
const expectedResult = 'https://mozilla.org/?x=%25ше%5Cллы%2F'
577508

578-
const result = decodePath(stringToCheck)
509+
const result = decodePath(stringToCheck).path
579510

580511
expect(result).toBe(expectedResult)
581512
})
582513

583514
it('should handle malformed percent-encodings gracefully', () => {
584515
const stringToCheck = 'path%ZZ%D1%88test%5C%C3%A9'
585516
// Malformed sequences should remain as-is, valid ones decoded
586-
const result = decodePath(stringToCheck)
517+
const result = decodePath(stringToCheck).path
587518
expect(result).toBe(`path%ZZ%D1%88test%5Cé`)
588519
})
589520

590521
it('should return empty string unchanged', () => {
591-
expect(decodePath('')).toBe('')
522+
expect(decodePath('').path).toBe('')
592523
})
593524

594525
it('should return strings without encoding unchanged', () => {
595526
const stringToCheck = 'plain-text-path'
596-
expect(decodePath(stringToCheck)).toBe(stringToCheck)
527+
expect(decodePath(stringToCheck).path).toBe(stringToCheck)
597528
})
598529

599530
it('should handle consecutive ignored characters', () => {
600531
const stringToCheck = 'test%25%25end'
601532
const expectedResult = 'test%25%25end'
602-
expect(decodePath(stringToCheck)).toBe(expectedResult)
533+
expect(decodePath(stringToCheck).path).toBe(expectedResult)
603534
})
604535

605536
it('should handle multiple ignored items of the same type with varying case', () => {
606537
const stringToCheck = '/params-ps/named/foo%2Fabc/c%2Fh'
607538
const expectedResult = '/params-ps/named/foo%2Fabc/c%2Fh'
608-
expect(decodePath(stringToCheck)).toBe(expectedResult)
539+
expect(decodePath(stringToCheck).path).toBe(expectedResult)
609540

610541
const stringToCheckWithLowerCase = '/params-ps/named/foo%2Fabc/c%5C%2f%5cAh'
611542
const expectedResultWithLowerCase =
612543
'/params-ps/named/foo%2Fabc/c%5C%2f%5cAh'
613-
expect(decodePath(stringToCheckWithLowerCase)).toBe(
544+
expect(decodePath(stringToCheckWithLowerCase).path).toBe(
614545
expectedResultWithLowerCase,
615546
)
616547
})
@@ -620,49 +551,61 @@ describe('decodePath', () => {
620551
// /%0d/google.com/ decodes to /\r/google.com/ which becomes //google.com/
621552
// This must be sanitized to prevent protocol-relative URL interpretation
622553
const result = decodePath('/%0d/google.com/')
623-
expect(result).toBe('/google.com/')
624-
expect(result).not.toMatch(/^\/\//)
554+
expect(result.path).toBe('/google.com/')
555+
expect(result.path).not.toMatch(/^\/\//)
556+
expect(result.handledProtocolRelativeURL).toBe(true)
625557
})
626558

627559
it('should strip LF (%0a) to prevent open redirect', () => {
628560
const result = decodePath('/%0a/evil.com/')
629-
expect(result).toBe('/evil.com/')
630-
expect(result).not.toMatch(/^\/\//)
561+
expect(result.path).toBe('/evil.com/')
562+
expect(result.path).not.toMatch(/^\/\//)
563+
expect(result.handledProtocolRelativeURL).toBe(true)
631564
})
632565

633566
it('should strip CRLF (%0d%0a) to prevent open redirect', () => {
634567
const result = decodePath('/%0d%0a/evil.com/')
635-
expect(result).toBe('/evil.com/')
636-
expect(result).not.toMatch(/^\/\//)
568+
expect(result.path).toBe('/evil.com/')
569+
expect(result.path).not.toMatch(/^\/\//)
570+
expect(result.handledProtocolRelativeURL).toBe(true)
637571
})
638572

639573
it('should strip multiple control characters to prevent open redirect', () => {
640574
const result = decodePath('/%0d%0d%0d/evil.com/')
641-
expect(result).toBe('/evil.com/')
642-
expect(result).not.toMatch(/^\/\//)
575+
expect(result.path).toBe('/evil.com/')
576+
expect(result.path).not.toMatch(/^\/\//)
577+
expect(result.handledProtocolRelativeURL).toBe(true)
643578
})
644579

645580
it('should strip null bytes and other control characters', () => {
646581
const result = decodePath('/%00/test/')
647-
expect(result).toBe('/test/')
582+
expect(result.path).toBe('/test/')
583+
expect(result.handledProtocolRelativeURL).toBe(true)
648584
})
649585

650586
it('should collapse leading double slashes to prevent protocol-relative URLs', () => {
651587
// After stripping control chars, ensure we don't end up with //evil.com
652588
const result = decodePath('/%0d%0a/evil.com/path')
653589
// Should resolve to localhost, not evil.com
654-
const url = new URL(result, 'http://localhost:3000')
590+
const url = new URL(result.path, 'http://localhost:3000')
655591
expect(url.origin).toBe('http://localhost:3000')
592+
expect(result.handledProtocolRelativeURL).toBe(true)
656593
})
657594

658595
it('should handle normal paths unchanged', () => {
659-
expect(decodePath('/users/profile/')).toBe('/users/profile/')
660-
expect(decodePath('/api/v1/data')).toBe('/api/v1/data')
596+
expect(decodePath('/users/profile/').path).toBe('/users/profile/')
597+
expect(decodePath('/users/profile/').handledProtocolRelativeURL).toBe(
598+
false,
599+
)
600+
expect(decodePath('/api/v1/data').path).toBe('/api/v1/data')
601+
expect(decodePath('/api/v1/data').handledProtocolRelativeURL).toBe(false)
661602
})
662603

663604
it('should handle double slash only input', () => {
664605
// Direct // input should also be collapsed
665-
expect(decodePath('//')).toBe('/')
606+
const result = decodePath('//')
607+
expect(result.path).toBe('/')
608+
expect(result.handledProtocolRelativeURL).toBe(true)
666609
})
667610
})
668611
})

packages/start-server-core/src/createStartHandler.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,10 +218,16 @@ export function createStartHandler<TRegister = Register>(
218218

219219
try {
220220
// normalizing and sanitizing the pathname here for server, so we always deal with the same format during SSR.
221-
const url = getNormalizedURL(request.url)
221+
// during normalization paths like '//posts' are flattened to '/posts'.
222+
// in these cases we would prefer to redirect to the new path
223+
const { url, handledProtocolRelativeURL } = getNormalizedURL(request.url)
222224
const href = url.pathname + url.search + url.hash
223225
const origin = getOrigin(request)
224226

227+
if (handledProtocolRelativeURL) {
228+
return Response.redirect(url, 308)
229+
}
230+
225231
const entries = await getEntries()
226232
const startOptions: AnyStartInstanceOptions =
227233
(await entries.startEntry.startInstance?.getOptions()) ||

0 commit comments

Comments
 (0)