fix: hardening #6337
fix: hardening #6337
Conversation
The getOrigin function was trusting the Origin header sent by clients, which could be spoofed by attackers. This could lead to SSRF-like vulnerabilities where the router's origin is set to a malicious value, affecting how redirects are processed. Now we derive the origin solely from request.url, which is set by the server infrastructure and not directly controllable by clients. For applications behind proxies that need custom origins, the router's 'origin' option should be used to explicitly configure a trusted origin.
📝 WalkthroughWalkthroughAdds URL-protocol guards and HTML-escaping utilities across router packages, normalizes/validates prerender and SPA mask paths to prevent SSRF, derives origin from request URL for SSR, and injects escaped JSON-LD in head components. Tests cover protocol detection, escaping, origin, redirects, and prerender validation. Changes
Sequence Diagram(s)(Skipped — changes span multiple unrelated flows and do not form a single concise multi-actor sequential diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 72bb09d
☁️ 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: 2
🤖 Fix all issues with AI agents
In @packages/router-core/tests/dangerous-protocols.test.ts:
- Around line 5-99: Add targeted tests to
packages/router-core/tests/dangerous-protocols.test.ts to cover URL-encoded
schemes, null/undefined inputs, protocol-relative URLs, and malformed/very long
inputs: extend the existing describe blocks (or add a new "additional edge
cases" block) and add expectations calling isDangerousProtocol() with encoded
payloads like '%6a%61%76%61%73%63%72%69%70%74:alert(1)', with null and undefined
(expect false), with protocol-relative URLs like '//example.com' (expect false
for safe resolution), and with malformed/very long strings (e.g., extremely long
repeated 'javascript:' or non-printable bytes) to assert the function fails-safe
(true for dangerous encoded or malformed javascript, false for benign
long/unknown safe patterns); reference isDangerousProtocol to locate where to
add these new it() cases.
In @packages/start-plugin-core/src/prerender.ts:
- Around line 152-162: The URL constructor is being given routerBasePath (a path
like '/base') as the base which must be an absolute URL; update the
normalization in the startConfig.pages mapping to construct the base as an
absolute origin plus the routerBasePath (e.g., 'http://localhost' +
routerBasePath, ensuring routerBasePath starts with '/'), then create new
URL(page.path, absoluteBase) and continue enforcing origin ===
'http://localhost' and returning path: url.pathname + url.search + url.hash;
this prevents the TypeError while preserving the relative-path enforcement logic
in the startConfig.pages normalization.
🧹 Nitpick comments (2)
packages/router-core/tests/dangerous-protocols.test.ts (1)
101-132: Consider testing additional dangerous protocols in redirect suite.The redirect tests cover the primary threat vectors (javascript:, data:) but don't test all dangerous protocols identified in the
isDangerousProtocolsuite. For consistency and completeness, consider adding:
- vbscript: and file: protocols in redirect tests
- Relative paths with href to ensure they work correctly (currently only tests
tofield without href)📋 Suggested additional redirect test cases
it('should throw when href uses data: protocol', () => { expect(() => redirect({ href: 'data:text/html,<script>alert(1)</script>' }), ).toThrow(/unsafe protocol/) }) + it('should throw when href uses vbscript: protocol', () => { + expect(() => redirect({ href: 'vbscript:msgbox(1)' })).toThrow( + /unsafe protocol/, + ) + }) + + it('should throw when href uses file: protocol', () => { + expect(() => redirect({ href: 'file:///etc/passwd' })).toThrow( + /unsafe protocol/, + ) + }) + it('should allow safe protocols', () => { expect(() => redirect({ href: 'https://example.com' })).not.toThrow() expect(() => redirect({ href: 'http://example.com' })).not.toThrow() expect(() => redirect({ href: 'mailto:[email protected]' })).not.toThrow() }) + it('should allow relative paths with href', () => { + expect(() => redirect({ href: '/home' })).not.toThrow() + expect(() => redirect({ href: './relative' })).not.toThrow() + }) + it('should allow redirects without href', () => { expect(() => redirect({ to: '/home' })).not.toThrow() })packages/start-plugin-core/tests/post-server-build.test.ts (1)
28-45: Add positive test case for valid relative paths.The test correctly validates rejection of absolute URLs, but consider adding a test to ensure valid relative paths (e.g.,
'/spa','/shell') are accepted without errors. This provides confidence that the validation doesn't inadvertently block legitimate configurations.✅ Suggested test case
it('accepts relative SPA maskPath', async () => { const startConfig = { spa: { enabled: true, maskPath: '/spa-shell', prerender: {}, }, pages: [], router: { basepath: '' }, serverFns: { base: '' }, prerender: { enabled: true }, sitemap: { enabled: false }, } as any await expect(postServerBuild({ builder, startConfig })).resolves.not.toThrow() })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/react-router/src/HeadContent.tsxpackages/react-router/src/link.tsxpackages/react-router/src/scroll-restoration.tsxpackages/router-core/src/index.tspackages/router-core/src/redirect.tspackages/router-core/src/router.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/src/utils.tspackages/router-core/tests/dangerous-protocols.test.tspackages/router-core/tests/getOrigin.test.tspackages/router-core/tests/load.test.tspackages/router-core/tests/utils.test.tspackages/solid-router/src/HeadContent.tsxpackages/solid-router/src/link.tsxpackages/solid-router/src/scroll-restoration.tsxpackages/start-plugin-core/src/post-server-build.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/vue-router/src/HeadContent.tsxpackages/vue-router/src/link.tsxpackages/vue-router/src/scroll-restoration.tsx
🧰 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/router-core/src/redirect.tspackages/router-core/tests/load.test.tspackages/solid-router/src/HeadContent.tsxpackages/vue-router/src/HeadContent.tsxpackages/router-core/src/index.tspackages/solid-router/src/scroll-restoration.tsxpackages/react-router/src/HeadContent.tsxpackages/router-core/src/router.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/router-core/tests/getOrigin.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/router-core/tests/dangerous-protocols.test.tspackages/router-core/src/utils.tspackages/start-plugin-core/src/prerender.tspackages/router-core/tests/utils.test.tspackages/router-core/src/ssr/ssr-server.tspackages/react-router/src/scroll-restoration.tsxpackages/vue-router/src/scroll-restoration.tsxpackages/start-plugin-core/src/post-server-build.tspackages/react-router/src/link.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/redirect.tspackages/router-core/tests/load.test.tspackages/solid-router/src/HeadContent.tsxpackages/vue-router/src/HeadContent.tsxpackages/router-core/src/index.tspackages/solid-router/src/scroll-restoration.tsxpackages/react-router/src/HeadContent.tsxpackages/router-core/src/router.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/router-core/tests/getOrigin.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/router-core/tests/dangerous-protocols.test.tspackages/router-core/src/utils.tspackages/start-plugin-core/src/prerender.tspackages/router-core/tests/utils.test.tspackages/router-core/src/ssr/ssr-server.tspackages/react-router/src/scroll-restoration.tsxpackages/vue-router/src/scroll-restoration.tsxpackages/start-plugin-core/src/post-server-build.tspackages/react-router/src/link.tsx
🧠 Learnings (8)
📓 Common learnings
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.
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.
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.
📚 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/router-core/src/redirect.tspackages/router-core/tests/load.test.tspackages/router-core/src/index.tspackages/solid-router/src/scroll-restoration.tsxpackages/router-core/src/router.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/router-core/tests/getOrigin.test.tspackages/router-core/tests/dangerous-protocols.test.tspackages/router-core/src/utils.tspackages/start-plugin-core/src/prerender.tspackages/router-core/src/ssr/ssr-server.tspackages/start-plugin-core/src/post-server-build.tspackages/react-router/src/link.tsx
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/router-core/src/redirect.tspackages/react-router/src/HeadContent.tsxpackages/router-core/src/router.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/react-router/src/link.tsx
📚 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:
packages/router-core/tests/load.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/router-core/tests/getOrigin.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/router-core/tests/dangerous-protocols.test.tspackages/router-core/tests/utils.test.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/tests/load.test.tspackages/solid-router/src/HeadContent.tsxpackages/router-core/src/index.tspackages/router-core/src/router.tspackages/solid-router/src/link.tsxpackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/post-server-build.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/tests/load.test.tspackages/router-core/src/index.tspackages/router-core/src/router.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/start-plugin-core/src/prerender.tspackages/react-router/src/link.tsx
📚 Learning: 2025-08-30T09:12:13.852Z
Learnt from: Sheraff
Repo: TanStack/router PR: 5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.852Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.
Applied to files:
packages/router-core/src/router.tspackages/vue-router/src/link.tsxpackages/solid-router/src/link.tsxpackages/react-router/src/link.tsx
📚 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:
packages/start-plugin-core/tests/post-server-build.test.ts
🧬 Code graph analysis (9)
packages/react-router/src/HeadContent.tsx (1)
packages/router-core/src/utils.ts (1)
escapeHtml(575-577)
packages/router-core/src/router.ts (2)
packages/router-core/src/utils.ts (1)
isDangerousProtocol(541-554)packages/router-core/src/redirect.ts (1)
redirect(75-118)
packages/start-plugin-core/tests/prerender-ssrf.test.ts (2)
packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)packages/start-plugin-core/src/prerender.ts (1)
prerender(11-298)
packages/router-core/tests/getOrigin.test.ts (1)
packages/router-core/src/ssr/ssr-server.ts (1)
getOrigin(345-350)
packages/start-plugin-core/tests/post-server-build.test.ts (2)
packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)packages/start-plugin-core/src/post-server-build.ts (1)
postServerBuild(8-73)
packages/router-core/tests/utils.test.ts (1)
packages/router-core/src/utils.ts (1)
escapeHtml(575-577)
packages/react-router/src/scroll-restoration.tsx (2)
packages/router-core/src/index.ts (2)
restoreScroll(380-380)escapeHtml(276-276)packages/router-core/src/utils.ts (1)
escapeHtml(575-577)
packages/vue-router/src/scroll-restoration.tsx (2)
packages/router-core/src/index.ts (1)
escapeHtml(276-276)packages/router-core/src/utils.ts (1)
escapeHtml(575-577)
packages/react-router/src/link.tsx (2)
packages/router-core/src/index.ts (1)
isDangerousProtocol(277-277)packages/router-core/src/utils.ts (1)
isDangerousProtocol(541-554)
🪛 Biome (2.1.2)
packages/solid-router/src/scroll-restoration.tsx
[error] 41-41: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
packages/react-router/src/scroll-restoration.tsx
[error] 41-41: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
packages/vue-router/src/scroll-restoration.tsx
[error] 69-69: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (24)
packages/router-core/src/ssr/ssr-server.ts (1)
331-350: Excellent SSRF protection implementation.The security approach is sound: deriving origin from
request.url(server-controlled) rather than theOriginheader (client-controlled) correctly prevents SSRF vulnerabilities. The comprehensive security comment is valuable documentation for future maintainers.One minor observation: The fallback to
'http://localhost'when URL parsing fails is safe but might mask severe infrastructure issues whererequest.urlitself is malformed. This is likely acceptable for resilience, but consider logging such failures if not already handled upstream.packages/router-core/src/utils.ts (2)
521-554: Robust dangerous protocol detection.The implementation correctly uses a whitelist approach (fail-safe) and leverages the URL constructor for protocol normalization, which automatically handles bypass attempts like mixed case (
JavaScript:), embedded whitespace (java\nscript:), and other encoding tricks per the WHATWG URL specification.The logic correctly treats relative URLs (URL constructor throws) as safe since they cannot contain dangerous protocols.
556-577: Proper HTML escaping for JSON in script tags.The implementation correctly escapes HTML-sensitive characters using Unicode escape sequences. When the escaped JSON string is embedded in a
<script>tag:
- The HTML parser treats sequences like
\u003cas literal text (not special in HTML)- The JavaScript parser interprets
\u003cback as<within the string context- This prevents XSS: even if input contains
</script>, it becomes\u003c/script\u003ewhich won't close the script tagThe inclusion of line/paragraph separators (U+2028, U+2029) is important as these can break JavaScript string literals.
packages/vue-router/src/scroll-restoration.tsx (1)
69-69: Correct XSS protection for embedded JSON.The use of
escapeHtmlonJSON.stringify(restoreScrollOptions)correctly prevents XSS vulnerabilities if user-controlled data flows into the scroll restoration options. The escaped JSON is safe to embed in the inline script tag.Note: The Biome static analysis warning about the
childrenprop is a false positive.ScriptOncerequires a string (script content), not JSX children, so passingchildrenas a prop is intentional and correct for this use case.packages/solid-router/src/scroll-restoration.tsx (1)
41-41: Correct XSS protection for embedded JSON.The use of
escapeHtmlonJSON.stringify(restoreScrollOptions)correctly prevents XSS vulnerabilities if user-controlled data flows into the scroll restoration options.Note: The Biome static analysis warning about the
childrenprop is a false positive.ScriptOncerequires a string (script content), not JSX children, so this usage is intentional and correct.packages/react-router/src/scroll-restoration.tsx (1)
41-41: Correct XSS protection for embedded JSON.The use of
escapeHtmlonJSON.stringify(restoreScrollOptions)correctly prevents XSS vulnerabilities if user-controlled data flows into the scroll restoration options. The implementation is consistent across all three framework adapters (React, Vue, Solid), which aids maintainability.Note: The Biome static analysis warning about the
childrenprop is a false positive.ScriptOncerequires a string (script content), not JSX children.packages/router-core/src/index.ts (1)
276-277: LGTM! New security utilities exposed via public API.The addition of
escapeHtmlandisDangerousProtocolto the public API enables consumers to leverage these security utilities in their own code, which aligns well with the PR's security hardening objectives.packages/router-core/src/redirect.ts (1)
86-91: LGTM! Critical security guard for dangerous protocols.The validation correctly blocks dangerous protocols before any URL processing occurs. Note that this security check always throws an Error (not a Response), which means it bypasses the
opts.throwoption—this is the correct behavior since security violations should not be bypassable.packages/router-core/tests/utils.test.ts (1)
669-725: LGTM! Comprehensive test coverage for HTML escaping.The test suite thoroughly validates
escapeHtmlfunctionality, including:
- All targeted special characters (
<,>,&, Unicode line/paragraph separators)- XSS prevention scenarios (script tag injection in JSON)
- Edge cases (empty strings, strings without special characters)
- Mixed content handling
The tests correctly verify that escaped output uses Unicode escape sequences and doesn't contain literal dangerous characters.
packages/router-core/tests/getOrigin.test.ts (1)
1-62: LGTM! Excellent SSRF-prevention test coverage.This test suite effectively validates that
getOriginmitigates SSRF-like attacks by:
- Deriving origin exclusively from
request.url, not spoofable headers- Providing safe fallback behavior (
http://localhost) for invalid URLs- Never trusting the
Originheader, even when URL parsing failsThe tests directly address CVE-2024-34351 concerns and provide strong confidence in the security posture of origin detection.
packages/vue-router/src/HeadContent.tsx (1)
33-47: LGTM! Secure JSON-LD handling with proper escaping.The implementation correctly:
- Applies
escapeHtmlafterJSON.stringifyto prevent XSS injection viainnerHTML- Uses Unicode escape sequences (
\u003c,\u003e) which are valid in JSON strings- Creates properly typed script tags (
application/ld+json)- Silently skips invalid JSON-LD objects to fail safely
The try/catch pattern prioritizes security by preventing crashes, though it may make debugging slightly harder for developers with malformed JSON-LD data.
packages/react-router/src/HeadContent.tsx (1)
38-52: LGTM! JSON-LD handling with proper XSS mitigation.The implementation correctly escapes HTML entities in the serialized JSON to prevent XSS when the content is injected into the DOM. The try/catch appropriately handles edge cases like circular references in JSON-LD objects.
packages/router-core/tests/load.test.ts (1)
16-47: LGTM! Good test coverage for redirect resolution behavior.This test correctly validates the same-origin normalization logic in
resolveRedirect. The assertions ensure both theLocationheader andoptions.hrefare normalized to path-only format, which aligns with the intentional design to strip the router's origin from redirect URLs when they match.packages/router-core/src/router.ts (2)
2052-2061: LGTM! Critical security guard for document navigation.The dangerous protocol check correctly prevents XSS attacks via
javascript:,data:, orvbscript:URLs before they can be assigned towindow.location. The warning in non-production helps developers identify misuse.
2451-2474: LGTM! Correct same-origin normalization for redirects.The enhanced
resolveRedirectlogic properly normalizes same-origin absolute URLs in the Location header to path-only format (e.g.,https://example.com/foo→/foo), consistent with the intentional design. The try/catch correctly handles invalid URLs without breaking the flow.packages/vue-router/src/link.tsx (1)
253-285: LGTM! Dangerous protocol guard for external links.The implementation correctly blocks dangerous protocols by returning props without the
hrefattribute, which prevents navigation while maintaining the element's other properties for accessibility. The dev-only warning helps identify misuse.packages/solid-router/src/link.tsx (1)
161-182: LGTM! Comprehensive dangerous protocol guards.The implementation correctly guards both external links (via
hrefOption.href) and internal navigation targets (viaoptions.to) against dangerous protocols. Returningundefinedprevents navigation while the dev-only warnings help identify misuse.packages/solid-router/src/HeadContent.tsx (1)
37-51: LGTM! JSON-LD handling consistent with React router.The implementation correctly mirrors the React router's JSON-LD handling with proper HTML escaping to prevent XSS. The try/catch appropriately handles invalid JSON-LD objects.
packages/react-router/src/link.tsx (1)
146-171: LGTM! Comprehensive dangerous protocol guards for React.The implementation correctly guards both external links (line 149) and internal navigation targets (line 162) against dangerous protocols like
javascript:,data:, andvbscript:. The dev-only warnings andundefinedreturns effectively block navigation attempts.packages/router-core/tests/dangerous-protocols.test.ts (1)
1-4: LGTM! Clean test setup.The imports are well-organized and appropriate for testing dangerous protocol validation.
packages/start-plugin-core/src/post-server-build.ts (1)
36-39: LGTM - Effective SSRF prevention.The validation correctly prevents absolute URLs from being used as
spa.maskPathby leveraging the URL constructor's behavior: relative paths preserve the base origin while absolute URLs replace it. This guards against external prerendering attacks.packages/start-plugin-core/tests/prerender-ssrf.test.ts (2)
50-59: LGTM - Comprehensive SSRF protection test.The test effectively validates that absolute external URLs are rejected before any network requests are made, preventing potential SSRF attacks. The
fetchMockassertion confirms no side effects occur.
61-66: LGTM - Validates legitimate paths work correctly.The positive test case ensures that the SSRF protection doesn't inadvertently block valid relative paths, providing balanced test coverage.
packages/start-plugin-core/src/prerender.ts (1)
43-64: LGTM - Robust path normalization and SSRF protection.The initial path validation correctly:
- Creates a proper base URL for parsing
- Catches and wraps URL parsing errors with context
- Validates origin to ensure paths are relative
- Normalizes paths to pathname + search + hash
schiller-manuel
force-pushed
the
hardening
branch
from
c8b8f09 to
d32bf01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/start-plugin-core/src/prerender.ts:
- Around line 152-172: Extract the duplicated page validation/normalization
logic into a helper function named validateAndNormalizePrerenderPages that
accepts pages and a routerBaseUrl (URL) and returns the normalized Page[]; move
the try/catch that constructs new URL(page.path, routerBaseUrl), the origin
check for 'http://localhost', and the return of {...page, path: url.pathname +
url.search + url.hash} into that helper (preserve the Error with cause on URL
construction). Replace both inline usages (the array.map in prerender.ts and the
nearly identical block at the other occurrence) by calling
validateAndNormalizePrerenderPages(startConfig.pages, new URL(absoluteBase)) (or
equivalent) to keep behavior identical while removing duplication.
🧹 Nitpick comments (1)
packages/start-plugin-core/src/prerender.ts (1)
128-141: Consider applying URL validation to crawled links for defense-in-depth.While
extractLinksfilters to relative paths (/or./prefixes), these crawled links bypass the URL validation applied to initial pages (lines 46-64). This creates an inconsistency in security posture.Notably, protocol-relative URLs like
//evil.com/pathwould pass thehref.startsWith('/')check, potentially allowing SSRF if not caught downstream.Consider applying the same validation when adding crawled pages:
// In addCrawlPageTask or when calling it at line 288 for (const link of links) { try { const url = new URL(link, routerBaseUrl) if (url.origin !== 'http://localhost') { continue // Skip external links discovered during crawl } const normalizedPath = url.pathname + url.search + url.hash addCrawlPageTask({ path: normalizedPath, fromCrawl: true }) } catch { // Skip invalid URLs } }Alternatively, update
extractLinksto be more restrictive:-if (href && (href.startsWith('/') || href.startsWith('./'))) { +if (href && href.startsWith('/') && !href.startsWith('//')) { links.push(href) }Also applies to: 288-288
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/tests/dangerous-protocols.test.tspackages/start-plugin-core/src/prerender.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/router-core/tests/dangerous-protocols.test.tspackages/start-plugin-core/src/prerender.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/tests/dangerous-protocols.test.tspackages/start-plugin-core/src/prerender.ts
🧠 Learnings (5)
📓 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/router-core/tests/dangerous-protocols.test.tspackages/start-plugin-core/src/prerender.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:
packages/router-core/tests/dangerous-protocols.test.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/start-plugin-core/src/prerender.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/start-plugin-core/src/prerender.ts
🧬 Code graph analysis (1)
packages/router-core/tests/dangerous-protocols.test.ts (2)
packages/router-core/src/utils.ts (1)
isDangerousProtocol(541-554)packages/router-core/src/redirect.ts (1)
redirect(75-118)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/tests/dangerous-protocols.test.ts (2)
5-234: Excellent comprehensive test coverage for dangerous protocol detection.This test suite thoroughly covers the security-critical
isDangerousProtocolfunction with:
- All major dangerous protocols (javascript:, data:, vbscript:, file:, custom)
- Bypass attempt variations (newlines, mixed case, leading whitespace)
- Safe protocols verification (http:, https:, mailto:, tel:)
- Edge cases (empty input, null/undefined, relative URLs, query strings, hash fragments)
- Advanced edge cases (URL-encoded protocols, protocol-relative URLs, unicode, non-printable characters, long payloads)
The comments effectively explain nuanced behavior, particularly around URL-encoded protocols (lines 112-119) where the URL constructor correctly doesn't decode percent-encoding in protocols, matching browser behavior.
The type assertions for null/undefined (lines 103, 107) appropriately test runtime behavior beyond TypeScript's type system.
236-267: LGTM! Redirect security tests cover the critical paths.The redirect tests appropriately verify:
- Dangerous protocols (javascript:, data:) are blocked with error
- Bypass attempts are caught
- Safe protocols are allowed
- Redirects using the
toparameter (withouthref) work correctlyThe error message matcher
/unsafe protocol/provides good balance between specificity and maintainability.packages/start-plugin-core/src/prerender.ts (1)
43-64: Effective SSRF prevention through URL validation.The validation logic correctly enforces relative paths by:
- Constructing URLs with a localhost base to resolve relative paths
- Rejecting any URL with a non-localhost origin
- Normalizing to pathname + search + hash to strip protocol/host
This prevents malicious absolute URLs from being used in prerendering.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/start-plugin-core/src/prerender.ts:
- Around line 313-347: validateAndNormalizePrerenderPages: replace the hardcoded
origin check and guard against decodeURIComponent throwing; specifically, use
routerBaseUrl.origin for the origin comparison (i.e., check url.origin !==
routerBaseUrl.origin) and wrap the decodeURIComponent(url.pathname) call in a
try-catch that throws a clear Error (including the original error as cause) on
URIError/malformed percent-encoding so malformed paths are rejected safely while
preserving the original behavior of returning page objects with decoded pathname
+ url.search + url.hash.
🧹 Nitpick comments (2)
packages/start-plugin-core/src/prerender.ts (1)
138-143: Consider removing redundant normalization.The pages are already normalized at lines 47-50 after merging discovered pages. This second normalization call happens before any new pages are added from crawling (line 259), so it appears redundant.
If the intent is to re-validate pages added during crawling, consider moving this validation to
addCrawlPageTaskwhen pages are added withfromCrawl: true.packages/start-plugin-core/tests/prerender-ssrf.test.ts (1)
75-92: Consider expanding test coverage for edge cases.The current tests effectively validate the core SSRF protection (absolute URLs rejected, relative paths allowed). Consider adding tests for additional edge cases to ensure comprehensive protection:
- Protocol-relative URLs:
//attacker.test/leak- Other potentially dangerous protocols:
file:///etc/passwd,data:text/html,<script>...- Unicode paths to verify decoding:
/대한민국or/café- Malformed percent-encoding:
/path%2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/prerender-ssrf.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/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/prerender-ssrf.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/prerender-ssrf.test.ts
🧠 Learnings (4)
📓 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-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/start-plugin-core/src/prerender.ts
📚 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/start-plugin-core/src/prerender.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:
packages/start-plugin-core/tests/prerender-ssrf.test.ts
🧬 Code graph analysis (1)
packages/start-plugin-core/src/prerender.ts (1)
packages/start-plugin-core/src/schema.ts (1)
Page(202-202)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/start-plugin-core/src/prerender.ts (1)
43-50: LGTM! Good SSRF protection for prerender pages.The validation ensures that all prerender pages are relative paths and rejects absolute URLs that could lead to SSRF vulnerabilities.
|
|
||
| /** | ||
| * Validates and normalizes prerender page paths to ensure they are relative | ||
| * (no protocol/host) and returns normalized Page objects with cleaned paths. | ||
| * Preserves unicode characters by decoding the pathname after URL validation. | ||
| */ | ||
| function validateAndNormalizePrerenderPages( | ||
| pages: Array<Page>, | ||
| routerBaseUrl: URL, | ||
| ): Array<Page> { | ||
| return pages.map((page) => { | ||
| let url: URL | ||
| try { | ||
| url = new URL(page.path, routerBaseUrl) | ||
| } catch (err) { | ||
| throw new Error(`prerender page path must be relative: ${page.path}`, { | ||
| cause: err, | ||
| }) | ||
| } | ||
|
|
||
| if (url.origin !== 'http://localhost') { | ||
| throw new Error(`prerender page path must be relative: ${page.path}`) | ||
| } | ||
|
|
||
| // Decode the pathname to preserve unicode characters (e.g., /대한민국) | ||
| // The URL constructor encodes non-ASCII characters, but we want to keep | ||
| // the original unicode form for filesystem paths | ||
| const decodedPathname = decodeURIComponent(url.pathname) | ||
|
|
||
| return { | ||
| ...page, | ||
| path: decodedPathname + url.search + url.hash, | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Handle potential decoding errors and prefer dynamic origin check.
The validation logic is sound for SSRF protection. Two improvements to consider:
-
Line 340 - Handle malformed percent-encoding: The
decodeURIComponentcall could throw aURIErrorif the pathname contains malformed percent-encoding (e.g., incomplete escape sequences). Wrap it in a try-catch for robustness. -
Line 333 - Use dynamic origin check: The hardcoded
'http://localhost'check works but is brittle. UserouterBaseUrl.origininstead to make the validation more maintainable and robust to future changes.
🔧 Proposed improvements
function validateAndNormalizePrerenderPages(
pages: Array<Page>,
routerBaseUrl: URL,
): Array<Page> {
return pages.map((page) => {
let url: URL
try {
url = new URL(page.path, routerBaseUrl)
} catch (err) {
throw new Error(`prerender page path must be relative: ${page.path}`, {
cause: err,
})
}
- if (url.origin !== 'http://localhost') {
+ if (url.origin !== routerBaseUrl.origin) {
throw new Error(`prerender page path must be relative: ${page.path}`)
}
// Decode the pathname to preserve unicode characters (e.g., /대한민국)
// The URL constructor encodes non-ASCII characters, but we want to keep
// the original unicode form for filesystem paths
- const decodedPathname = decodeURIComponent(url.pathname)
+ let decodedPathname: string
+ try {
+ decodedPathname = decodeURIComponent(url.pathname)
+ } catch (err) {
+ throw new Error(
+ `prerender page path contains invalid percent-encoding: ${page.path}`,
+ { cause: err },
+ )
+ }
return {
...page,
path: decodedPathname + url.search + url.hash,
}
})
}🤖 Prompt for AI Agents
In @packages/start-plugin-core/src/prerender.ts around lines 313 - 347,
validateAndNormalizePrerenderPages: replace the hardcoded origin check and guard
against decodeURIComponent throwing; specifically, use routerBaseUrl.origin for
the origin comparison (i.e., check url.origin !== routerBaseUrl.origin) and wrap
the decodeURIComponent(url.pathname) call in a try-catch that throws a clear
Error (including the original error as cause) on URIError/malformed
percent-encoding so malformed paths are rejected safely while preserving the
original behavior of returning page objects with decoded pathname + url.search +
url.hash.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/prerender.ts (1)
114-127: Protocol-relative URLs bypass origin validation.The filter at line 121 allows protocol-relative URLs like
//evil.com/pathbecause they start with/. When passed tolocalFetchat line 182-183, these becomehttp://evil.com/pathafter URL construction (line 83), enabling SSRF to external hosts during prerendering.Crawled links are not validated through
validateAndNormalizePrerenderPages, leaving this vector unprotected.🔒 Proposed fix to filter protocol-relative URLs
function extractLinks(html: string): Array<string> { const linkRegex = /<a[^>]+href=["']([^"']+)["'][^>]*>/g const links: Array<string> = [] let match while ((match = linkRegex.exec(html)) !== null) { const href = match[1] - if (href && (href.startsWith('/') || href.startsWith('./'))) { + // Exclude protocol-relative URLs (//host/path) and only allow single-slash or relative paths + if (href && (href.startsWith('./') || (href.startsWith('/') && !href.startsWith('//')))) { links.push(href) } } return links }
🧹 Nitpick comments (3)
packages/start-plugin-core/src/prerender.ts (3)
138-143: Consider removing redundant validation.This validation call processes the same pages already validated at lines 47-50, as
startConfig.pagesisn't modified between those points. Pages discovered during crawling (line 159) are added after this validation, so they're never validated anyway.If
prerenderPagesmight be called independently in the future, consider documenting that intent; otherwise, this appears redundant.
333-335: Hard-coded origin check reduces flexibility.Comparing against
'http://localhost'couples the validation to a specific base URL. IfrouterBaseUrlever uses a different origin (e.g., in testing scenarios), this would incorrectly reject valid relative paths.♻️ Proposed refactor
- if (url.origin !== 'http://localhost') { + if (url.origin !== routerBaseUrl.origin) { throw new Error(`prerender page path must be relative: ${page.path}`) }
340-344: Inconsistent URI decoding.Only
pathnameis decoded (line 340), whilesearchandhashremain percent-encoded. This creates paths that are partially decoded. While probably intentional for filesystem compatibility, consider whether search/hash should also be decoded for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/src/prerender.tspackages/start-plugin-core/tests/post-server-build.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/tests/post-server-build.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/start-plugin-core/src/prerender.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/src/prerender.ts
🧠 Learnings (3)
📓 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/start-plugin-core/src/prerender.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/start-plugin-core/src/prerender.ts
🧬 Code graph analysis (1)
packages/start-plugin-core/src/prerender.ts (1)
packages/start-plugin-core/src/schema.ts (1)
Page(202-202)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/start-plugin-core/src/prerender.ts (1)
43-50: Good: SSRF protection for initial prerender pages.The validation correctly ensures that user-defined prerender pages are relative paths without external protocols or hosts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/start-plugin-core/tests/prerender-ssrf.test.ts (2)
13-18: Consider removing the unnecessary prerender mock.This mock simply re-exports the actual implementation without any modifications. Since the
prerenderfunction is what's being tested (imported on line 2), this mock serves no purpose and can be safely removed.♻️ Simplify by removing the mock
-vi.mock('../src/prerender', async () => { - const actual = await vi.importActual<any>('../src/prerender') - return { - ...actual, - } -}) -
94-99: Strengthen the positive test case with additional assertions.The test verifies that relative paths don't throw an error, but doesn't confirm that prerender actually performs its work. Consider adding assertions to verify expected behavior, such as:
fetchMockwas called with the correct URL- Filesystem operations were invoked (if observable through the mocks)
♻️ Example: Add positive assertions
it('allows relative paths', async () => { resetFetch() const startConfig = makeStartConfig('/about') await expect(prerender({ startConfig, builder })).resolves.not.toThrow() + expect(fetchMock).toHaveBeenCalledWith( + expect.stringContaining('/about'), + expect.any(Object), + ) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-plugin-core/tests/prerender-ssrf.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/start-plugin-core/tests/prerender-ssrf.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/tests/prerender-ssrf.test.ts
🧠 Learnings (2)
📓 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-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:
packages/start-plugin-core/tests/prerender-ssrf.test.ts
⏰ 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 (1)
packages/start-plugin-core/tests/prerender-ssrf.test.ts (1)
83-92: Excellent SSRF protection test coverage.The test properly validates that absolute external URLs are rejected before any network calls occur. The assertion that
fetchMockis not called (line 91) is particularly important for confirming SSRF prevention. This aligns well with the security hardening objectives of this PR.
Summary by CodeRabbit
New Features
Security Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.