fix: open redirect #6309
fix: open redirect #6309
Conversation
📝 WalkthroughWalkthroughThis pull request adds open redirect prevention by introducing path sanitization logic that removes control characters (CR, LF) and collapses leading double slashes across the history and router-core packages, along with comprehensive E2E and unit tests validating the prevention of CRLF injection and protocol-relative redirect attacks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit d3450ff
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/history/tests/parseHref.test.ts (1)
6-6: Minor type cast adjustment for test parameter.The
as anycast allows passing an empty object as the state parameter. While this works for testing, consider whetherundefinedwould be more semantically accurate (as used in the new tests below).packages/history/src/index.ts (1)
626-643: Consider consolidating sanitization logic to reduce duplication.The
sanitizePathimplementation duplicates the control character removal regex and leading slash collapsing logic found inpackages/router-core/src/utils.ts(lines 492-502 and 535-540). While this duplication may be intentional given the package separation, it creates a maintenance burden—changes to the security logic would need to be applied in multiple places.Consider:
- Extract shared sanitization logic to a common utility package, or
- Document the intentional duplication with a comment referencing the parallel implementation
Note: The Biome warning about control characters in the regex is a false positive—the regex intentionally matches control characters for security purposes.
e2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.ts (2)
5-64: Consider extracting the common assertion pattern.The four CRLF injection tests share an identical assertion pattern. Extracting this to a helper function would improve maintainability and reduce duplication.
🔎 Proposed refactor with helper function
Add a helper function before the test suite:
async function assertSameOrigin(page: Page, baseURL: string) { await page.waitForLoadState('networkidle') expect(page.url().startsWith(baseURL)).toBe(true) const url = new URL(page.url()) expect(url.origin).toBe(new URL(baseURL).origin) }Then simplify each test:
test('should not redirect to external site via CR injection (%0d)', async ({ page, baseURL, }) => { // This URL attempts to exploit CRLF injection to redirect to google.com // %0d = \r (carriage return) // Without the fix, /\r/google.com/ would be interpreted as //google.com/ // which browsers resolve as a protocol-relative URL to http://google.com/ await page.goto('/%0d/google.com/') - await page.waitForLoadState('networkidle') - - // Should stay on the same origin - the path should be treated as a local path - // not as an external redirect - expect(page.url().startsWith(baseURL!)).toBe(true) - // The origin should be localhost, not google.com - const url = new URL(page.url()) - expect(url.origin).toBe(new URL(baseURL!).origin) + await assertSameOrigin(page, baseURL!) })Additionally, consider verifying the HTTP status code or actual page content to ensure the application is handling these paths correctly (e.g., rendering a 404 or the expected route), not just checking the URL.
76-84: Consider making the pathname assertion more specific.The regex
/^\/test-path\/?$/allows both/test-pathand/test-path/. If the sanitization logic has predictable behavior regarding trailing slashes, consider making the assertion more specific to validate the exact expected output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tspackages/history/src/index.tspackages/history/tests/parseHref.test.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/history/tests/parseHref.test.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.tse2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.tspackages/history/src/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/history/tests/parseHref.test.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.tse2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.tspackages/history/src/index.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/history/tests/parseHref.test.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.tse2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.tspackages/history/src/index.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.tspackages/router-core/tests/utils.test.tse2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.ts
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/utils.tspackages/router-core/tests/utils.test.tspackages/history/src/index.ts
📚 Learning: 2025-09-22T00:56:53.426Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
Applied to files:
packages/router-core/src/utils.ts
📚 Learning: 2025-09-22T00:56:49.237Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Applied to files:
packages/router-core/src/utils.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/utils.ts
🧬 Code graph analysis (4)
packages/history/tests/parseHref.test.ts (1)
packages/history/src/index.ts (1)
parseHref(645-677)
packages/router-core/src/utils.ts (1)
packages/router-core/src/route.ts (1)
path(1553-1555)
packages/router-core/tests/utils.test.ts (1)
packages/router-core/src/utils.ts (1)
decodePath(521-543)
packages/history/src/index.ts (2)
packages/router-core/src/route.ts (1)
path(1553-1555)packages/router-core/src/router.ts (1)
state(1099-1101)
🪛 Biome (2.1.2)
packages/router-core/src/utils.ts
[error] 501-501: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 501-501: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
packages/history/src/index.ts
[error] 634-634: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 634-634: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (12)
packages/router-core/src/utils.ts (3)
492-502: LGTM! Security control for open redirect prevention.The implementation correctly removes ASCII control characters including CR, LF, and DEL. The static analysis warnings from Biome about control characters in the regex are false positives—the regex is intentionally designed to match and remove these dangerous characters for security purposes.
504-519: LGTM! Proper sanitization after decoding.The refactored logic correctly decodes URI segments first, then applies sanitization to remove control characters. The intermediate
decodedvariable improves readability while maintaining the existing error handling for malformed encodings.
521-543: LGTM! Protocol-relative URL protection is properly implemented.The post-processing step correctly collapses leading double slashes to prevent protocol-relative URL interpretation. After control character sanitization converts paths like
/\r/evil.comto//evil.com, this logic ensures the final result is/evil.com, preventing the browser from treating it as an external redirect.packages/router-core/tests/utils.test.ts (1)
616-665: LGTM! Comprehensive test coverage for open redirect prevention.The test suite thoroughly validates the security fixes:
- CR, LF, and CRLF injection scenarios
- Multiple control character handling
- Protocol-relative URL collapsing
- Safe origin resolution with localhost
- Edge cases like direct
//input- Normal path preservation
All test assertions correctly verify the expected sanitized behavior.
packages/history/tests/parseHref.test.ts (1)
12-55: LGTM! Thorough validation of parseHref sanitization.The test suite comprehensively validates that
parseHrefcorrectly sanitizes all returned fields (href, pathname, search, hash) and prevents open redirect vulnerabilities. The safe origin resolution test (lines 42-45) effectively confirms that sanitized paths stay on the expected origin.packages/history/src/index.ts (1)
645-677: LGTM! Consistent sanitization across all URL components.The refactored
parseHrefcorrectly applies sanitization upfront and consistently usessanitizedHreffor all computations. This ensures that control characters are removed from all returned fields (href, pathname, hash, search), preventing any path through which an open redirect could occur.e2e/react-start/basic/tests/open-redirect-prevention.spec.ts (4)
4-8: LGTM! Appropriate error whitelisting for open redirect tests.The 404 error whitelist is sensible for these tests, as navigating to crafted paths like
/google.com/will legitimately result in 404 responses when no matching route exists. This configuration reduces test noise while still catching genuine failures.
11-72: LGTM! Comprehensive E2E validation of CRLF injection prevention.The test suite thoroughly validates that all CRLF injection vectors are mitigated:
- Individual CR and LF characters
- Combined CRLF sequences
- Multiple control characters
The dual assertions (URL prefix and origin verification) effectively confirm that navigation stays on the same origin rather than being hijacked to external domains. The inline comments clearly explain each attack vector.
74-93: LGTM! Protocol-relative URL prevention validated end-to-end.This test effectively verifies the complete fix chain: control character sanitization followed by leading slash collapsing. The pathname assertion (line 91) appropriately allows for optional trailing slashes, accounting for potential route normalization behavior.
95-116: LGTM! Essential regression coverage for normal navigation.These tests ensure the sanitization logic doesn't break standard routing behavior. Testing both simple paths (
/posts) and paths with URL-encoded parameters confirms that legitimate navigation remains unaffected by the security fixes.e2e/react-router/basic-file-based/tests/open-redirect-prevention.spec.ts (2)
1-110: Well-structured security test suite.The test file is comprehensive and well-organized, covering the key attack vectors for open redirect vulnerabilities. The grouping of tests by attack type makes the suite easy to understand and maintain.
88-109: Good regression coverage for normal navigation.These tests appropriately verify that the security fix doesn't break normal navigation functionality. The inclusion of URL-encoded characters is a nice touch.
fixes #6306
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.