Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.5k
feat(nuxt): experimental page component name normalisation#33513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
|
Uh oh!
There was an error while loading.Please reload this page.
pkg-pr-newbot commentedOct 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@nuxt/kit@nuxt/nitro-servernuxt@nuxt/rspack-builder@nuxt/schema@nuxt/vite-builder@nuxt/webpack-buildercommit: |
codspeed-hqbot commentedOct 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#33513 willnot alter performanceComparing Summary
|
WalkthroughThis pull request introduces a new experimental feature Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple packages with consistent patterns: a new experimental configuration option with schema definitions, conditional logic to augment component metadata, and dedicated tests for the new feature. The implementation logic is relatively straightforward (wrapping imports with Object.assign or promise chains), but review requires verification that the conditional logic correctly handles both sync and async import paths, proper integration with the experimental flag, and validation that test coverage adequately reflects the new behaviour. The heterogeneity is moderate—whilst most changes follow the same pattern (adding the flag and implementation), the different import method handling adds some complexity. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2024-12-12T12:36:34.871ZApplied to files:
🧬 Code graph analysis (2)packages/nuxt/src/pages/utils.ts (2)
packages/nuxt/test/pages.test.ts (3)
🪛 GitHub Check: CodeQLpackages/nuxt/src/pages/utils.ts[warning] 598-598: Improper code sanitization 🔇 Additional comments (9)
Thanks for usingCodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
packages/nuxt/src/pages/utils.ts Outdated
| :pageImport, | ||
| } | ||
| if(nuxt.options.experimental.normalizePageNames){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could we maybe split this intonormalizeComponent andnormalizeComponentWithName functions, so we only assign it once, rather than manipulating an existing string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you mean anormalizeComponentName at runtime ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no. I mean, at the moment it's created a few lines above and then changed here
we can instead create it just once, which will tidy up the logic
| } | ||
| letcomponent=page.mode==='server' | ||
| ?`() => createIslandPage(${route.name})` |
Check warning
Code scanning / CodeQL
Improper code sanitization Medium
improperly sanitized value
Uh oh!
There was an error while loading.Please reload this page.
Show autofix suggestionHide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the improper code sanitization, dangerous characters that may break out of a JavaScript string or script context and allow code injection should be escaped from values inserted into constructed JavaScript code. The best approach is to escape additional risky characters, even afterJSON.stringify, by mapping all characters listed in the prompt'scharMap. This is done by defining anescapeUnsafeChars function that applies the mapping after the value is stringified. The fix should be applied whereserializeRouteValue is used in code interpolation, specifically forroute.name (and possibly other similar cases). TheescapeUnsafeChars helper can be added to this file, and applied in the necessary locations (either withinserializeRouteValue or as a wrapper on its output in line 629 and similar code constructions).
Implementation details:
- Add
escapeUnsafeChars(per the prompt) topackages/nuxt/src/pages/utils.ts. - When interpolating
route.nameinto a template string (e.g., in line 629), wrap it withescapeUnsafeChars. - Ensure minimal changes to functionality: this escapes only the dangerous characters and preserve original semantics.
| @@ -9,7 +9,6 @@ | ||
| import{filename}from'pathe/utils' | ||
| import{hash}from'ohash' | ||
| import{defu}from'defu' | ||
| import{klona}from'klona' | ||
| import{parseAndWalk}from'oxc-walker' | ||
| import{parseSync}from'oxc-parser' | ||
| @@ -626,7 +625,7 @@ | ||
| } | ||
| letcomponent=page.mode==='server' | ||
| ?`() => createIslandPage(${route.name})` | ||
| ?`() => createIslandPage(${escapeUnsafeChars(route.name)})` | ||
| :page.mode==='client' | ||
| ?`() => createClientPage(${pageImport})` | ||
| :pageImport |
| if(isSyncImport){ | ||
| component=`Object.assign(${pageImportName}, { __name:${metaRouteName} })` | ||
| }else{ | ||
| component=`${component}.then((m) => Object.assign(m.default, { __name:${metaRouteName} }))` |
Check warning
Code scanning / CodeQL
Improper code sanitization Medium
improperly sanitized value
Uh oh!
There was an error while loading.Please reload this page.
Show autofix suggestionHide autofix suggestion
Copilot Autofix
AI 1 day ago
General fix:
Whenever generating JavaScript code by interpolating user-controllable data into template strings, you must perform additional escaping on top ofJSON.stringify to neutralize dangerous characters that could break out of the string literal or script context.
Best fix for this code:
Add an escaping function for potentially dangerous unicode and script-breaking characters (as per the background), and use it to sanitize any interpolated JSON strings used in the generated code, specifically in values likemetaRouteName (and any similar fields, as appropriate).
- Define an
escapeUnsafeChars(str: string)function modeled after the example in the background to escape<,>,/, and other dangerous characters. - Use this function when interpolating values like
metaRouteNameinto code, specifically on the assignment tocomponentwheremetaRouteNameis interpolated (line 636, 638). - Consider exporting the sanitize function or placing it at file scope.
- Only add the function and calls within shown snippet regions; do not assume code beyond those lines.
Lines to change:
- Insert
escapeUnsafeCharsfunction before usage, within the shown file. - On lines 636 and 638 (and any similar string interpolations), wrap the variable injected into code with
escapeUnsafeChars(...).
Methods/imports needed:
- Only the helper function, no external packages required.
| @@ -541,6 +541,28 @@ | ||
| } | ||
| } | ||
| /** | ||
| *EscapeunsafeJavaScriptcharactersforsafeinsertionintoJSsource(scriptcontext). | ||
| *Escapes<,>,/,\\,null,andotherspecialorline/paragraphseparatorchars. | ||
| */ | ||
| constcharMap:Record<string,string>={ | ||
| '<':'\\u003C', | ||
| '>':'\\u003E', | ||
| '/':'\\u002F', | ||
| '\\':'\\\\', | ||
| '\b':'\\b', | ||
| '\f':'\\f', | ||
| '\n':'\\n', | ||
| '\r':'\\r', | ||
| '\t':'\\t', | ||
| '\0':'\\0', | ||
| '\u2028':'\\u2028', | ||
| '\u2029':'\\u2029' | ||
| } | ||
| functionescapeUnsafeChars(str:string){ | ||
| returnstr.replace(/[<>\b\f\n\r\t\0\u2028\u2029/\\]/g,x=>charMap[x]) | ||
| } | ||
| functiongetRouteFromNuxtPage(page:NuxtPage,metaImports:Set<string>,options:NormalizeRoutesOptions):NormalizedRoute{ | ||
| constmetaFiltered:Record<string,any>={} | ||
| letskipMeta=true | ||
| @@ -633,9 +655,9 @@ | ||
| if(nuxt.options.experimental.normalizePageNames){ | ||
| if(isSyncImport){ | ||
| component=`Object.assign(${pageImportName}, { __name:${metaRouteName} })` | ||
| component=`Object.assign(${pageImportName}, { __name:${escapeUnsafeChars(metaRouteName)} })` | ||
| }else{ | ||
| component=`${component}.then((m) => Object.assign(m.default, { __name:${metaRouteName} }))` | ||
| component=`${component}.then((m) => Object.assign(m.default, { __name:${escapeUnsafeChars(metaRouteName)} }))` | ||
| } | ||
| } | ||
📚 Description
resolves#33254
this PR adds an experimental feature to normalize page components names using the route name.