fix: nonce #5522
fix: nonce #5522
Conversation
fixes #5511
|
Caution Review failedThe pull request is closed. WalkthroughRemoves explicit nonce prop from React Asset script handling and adds per-request CSP nonce extraction and propagation: HeadContent and Scripts in both React and Solid routers now derive nonce from router.options.ssr, attach it to generated head/link/style/script attrs, and ssr-client reads nonce from a meta[csp-nonce] during hydration. Changes
Sequence Diagram(s)sequenceDiagram
participant Server as SSR Server
participant Head as HeadContent (React/Solid)
participant Scripts as Scripts (React/Solid)
participant Asset as Asset Renderer
participant Browser as Browser DOM
participant Client as ssr-client.ts
Server->>Head: render HTML (router.options.ssr may include nonce)
Head->>Head: const nonce = router.options.ssr?.nonce
Head->>Head: include meta[property="csp-nonce"] if nonce
Head->>Asset: render head assets with attrs + nonce
Head->>Scripts: render scripts with attrs + nonce
Scripts->>Asset: render script assets (attrs include nonce)
Browser->>Client: hydration begins
Client->>Browser: query meta[property="csp-nonce"]
Browser-->>Client: return content (nonce)
Client->>Client: set router.options.ssr = { nonce }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 6m 17s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 3s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-17 23:17:07 UTC
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router/src/HeadContent.tsx (1)
16-70: Missingnoncein useMemo dependency array.The
useMemousesnonce(line 47) androuter.options.ssr?.nonce(lines 58-63) but only includes[routeMeta]in its dependency array (line 70). If the nonce value changes between renders, the memoized meta tags will contain a stale nonce.Apply this diff to fix the dependency array:
return resultMeta - }, [routeMeta]) + }, [routeMeta, nonce, router.options.ssr?.nonce])Alternatively, since
nonceis derived fromrouter.options.ssr?.nonce, you could use just one:return resultMeta - }, [routeMeta]) + }, [routeMeta, nonce])
🧹 Nitpick comments (2)
packages/solid-router/src/Scripts.tsx (1)
8-8: LGTM: Nonce extraction looks good.The nonce is correctly extracted from router options using optional chaining. When the nonce is undefined, Solid.js will skip rendering the attribute, which is the expected behavior.
For slightly cleaner code, you could conditionally spread the nonce only when defined:
- attrs: { ...asset.attrs, nonce }, + attrs: { ...asset.attrs, ...(nonce && { nonce }) },And similarly at line 47:
attrs: { ...script, - nonce, + ...(nonce && { nonce }), },However, the current implementation is simpler and Solid handles undefined attributes correctly, so this is purely optional.
packages/react-router/src/HeadContent.tsx (1)
188-195: Consider reusing nonce from useTags hook.The
nonceis read twice: once inuseTags(line 9) and again inHeadContent(line 191). While not incorrect, this is redundant. Consider threading the nonce through fromuseTagsor deriving tags with nonce already embedded.However, since
Assetneeds the nonce prop for link and style tags (lines 27, 33 in Asset.tsx), and the current approach is clear and explicit, this is a minor optimization opportunity rather than a defect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-router/src/Asset.tsx(2 hunks)packages/react-router/src/HeadContent.tsx(6 hunks)packages/react-router/src/Scripts.tsx(4 hunks)packages/router-core/src/ssr/ssr-client.ts(1 hunks)packages/solid-router/src/HeadContent.tsx(7 hunks)packages/solid-router/src/Scripts.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/src/Scripts.tsxpackages/react-router/src/HeadContent.tsxpackages/react-router/src/Asset.tsxpackages/react-router/src/Scripts.tsxpackages/solid-router/src/HeadContent.tsxpackages/router-core/src/ssr/ssr-client.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/src/Scripts.tsxpackages/react-router/src/HeadContent.tsxpackages/react-router/src/Asset.tsxpackages/react-router/src/Scripts.tsxpackages/solid-router/src/HeadContent.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/ssr/ssr-client.ts
🧬 Code graph analysis (1)
packages/react-router/src/Scripts.tsx (1)
packages/react-router/src/Asset.tsx (1)
Asset(11-41)
⏰ 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 (5)
packages/solid-router/src/HeadContent.tsx (3)
10-10: LGTM! Nonce correctly extracted and applied to meta tags.The nonce is properly extracted from router options and threaded into meta tag attributes.
Also applies to: 49-49
144-144: LGTM! Nonce correctly applied to styles and scripts.The nonce is properly added to both style and script tags, which are the critical elements for Content Security Policy enforcement. This directly addresses the issue where inline scripts lacked nonce attributes.
Also applies to: 161-161
60-68: No issues found — this is an intentional framework pattern.Verification confirms the custom meta tag with
property: 'csp-nonce'is intentional. The client-side code inpackages/router-core/src/ssr/ssr-client.tsexplicitly queries this meta tag (document.querySelector('meta[property="csp-nonce"]')) and extracts the nonce value during hydration. This pattern is consistent across both solid-router and react-router and serves as the framework's mechanism for passing the nonce from server to client during SSR.packages/solid-router/src/Scripts.tsx (1)
43-50: LGTM: Inline script nonce propagation is correct.The nonce is correctly added to inline scripts, ensuring CSP compliance for all dynamically generated script tags. The spread order ensures the nonce from router options takes precedence, which is the correct behavior.
packages/react-router/src/Asset.tsx (1)
37-37: LGTM: Script nonce handling simplified.The nonce is correctly threaded through
attrsrather than as a separate prop, aligning with the Script component's implementation that reads fromattrs.nonce(line 98).
| const meta = document.querySelector('meta[property="csp-nonce"]') as | ||
| | HTMLMetaElement | ||
| | undefined | ||
| const nonce = meta?.content | ||
| router.options.ssr = { | ||
| nonce, | ||
| } |
There was a problem hiding this comment.
Potential data loss: entire router.options.ssr object is overwritten.
Assigning router.options.ssr = { nonce } discards any pre-existing properties on router.options.ssr. If other parts of the hydration flow or user configuration set additional properties on this object, they will be lost.
Apply this diff to preserve existing properties:
const meta = document.querySelector('meta[property="csp-nonce"]') as
| HTMLMetaElement
| undefined
const nonce = meta?.content
- router.options.ssr = {
- nonce,
- }
+ router.options.ssr = {
+ ...router.options.ssr,
+ nonce,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const meta = document.querySelector('meta[property="csp-nonce"]') as | |
| | HTMLMetaElement | |
| | undefined | |
| const nonce = meta?.content | |
| router.options.ssr = { | |
| nonce, | |
| } | |
| const meta = document.querySelector('meta[property="csp-nonce"]') as | |
| | HTMLMetaElement | |
| | undefined | |
| const nonce = meta?.content | |
| router.options.ssr = { | |
| ...router.options.ssr, | |
| nonce, | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/ssr-client.ts around lines 89 to 95, the code
unconditionally assigns router.options.ssr = { nonce } which overwrites any
existing properties; instead merge the nonce into the existing object
(preserving existing properties) by shallow-merging into router.options.ssr (or
initializing it if undefined) so the final object contains prior keys plus the
nonce.
| tag: 'link', | ||
| attrs: { | ||
| ...link, | ||
| nonce, |
There was a problem hiding this comment.
Nonce applied to constructed links, but missing on preload and asset links.
While constructed links receive the nonce attribute (line 84), both:
- Preload links (modulepreload) at lines 119-125
- Manifest assets (link tags) at lines 97-102
do not receive the nonce. This inconsistency may cause CSP violations if those links contain resources that require nonce validation.
Apply this diff to add nonce to preload links:
router.ssr?.manifest?.routes[route.id]?.preloads
?.filter(Boolean)
.forEach((preload) => {
preloadMeta.push({
tag: 'link',
attrs: {
rel: 'modulepreload',
href: preload,
+ nonce,
},
})
}),And this diff to add nonce to manifest assets:
.map(
(asset) =>
({
tag: 'link',
- attrs: asset.attrs,
+ attrs: {
+ ...asset.attrs,
+ nonce,
+ },
}) satisfies RouterManagedTag,
)| .forEach((asset) => { | ||
| assetScripts.push({ | ||
| tag: 'script', | ||
| attrs: asset.attrs, | ||
| attrs: { ...asset.attrs, nonce }, | ||
| children: asset.children, | ||
| } as any) | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the type assertion and ensure type safety.
The as any cast at line 28 suggests a type mismatch between the constructed object and RouterManagedTag. This bypasses TypeScript's type checking and could hide issues.
Please verify:
- Does the
RouterManagedTagtype properly account for thenonceattribute in script tags? - Can the type assertion be removed by updating the type definition?
Run the following script to check the RouterManagedTag type definition:
🏁 Script executed:
#!/bin/bash
# Description: Find the RouterManagedTag type definition to verify if nonce is properly typed
# Search for RouterManagedTag type definition
ast-grep --pattern 'type RouterManagedTag = $$$'
# Also search for interface definition
ast-grep --pattern 'interface RouterManagedTag {
$$$
}'
# Search for nonce in type definitions
rg -n --type=ts 'RouterManagedTag' -A 10 -B 2Length of output: 30501
Replace as any with type-safe pattern for consistency.
The RouterManagedTag type properly supports the nonce attribute through its generic attrs?: Record<string, any>. The as any assertion is unnecessary and bypasses type checking. The codebase already uses the satisfies RouterManagedTag pattern elsewhere (e.g., HeadContent.tsx lines 86, 102). For consistency and type safety per the coding guidelines, replace the as any with:
} satisfies RouterManagedTag)Alternatively, remove the assertion entirely, since other scripts in react-router follow this approach without needing individual assertions.
🤖 Prompt for AI Agents
In packages/solid-router/src/Scripts.tsx around lines 23 to 29, the object
pushed into assetScripts is asserted with "as any", which bypasses type
checking; replace that assertion with a type-safe pattern by using "satisfies
RouterManagedTag" on the object literal (or remove the assertion entirely like
other script entries) so the nonce attribute is correctly type-checked and the
codebase remains consistent.
fixes #5511
Summary by CodeRabbit