feat(nuxt): experimental page component name normalisation #33513
Conversation
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33513 will not 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 using CodeRabbit! 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.
could we maybe split this into normalizeComponent and normalizeComponentWithName functions, so we only assign it once, rather than manipulating an existing string?
There was a problem hiding this comment.
Do you mean a normalizeComponentName at runtime ?
There was a problem hiding this comment.
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
| } | ||
|
|
||
| let component = page.mode === 'server' | ||
| ? `() => createIslandPage(${route.name})` |
Check warning
Code scanning / CodeQL
Improper code sanitization Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days 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 after JSON.stringify, by mapping all characters listed in the prompt's charMap. This is done by defining an escapeUnsafeChars function that applies the mapping after the value is stringified. The fix should be applied where serializeRouteValue is used in code interpolation, specifically for route.name (and possibly other similar cases). The escapeUnsafeChars helper can be added to this file, and applied in the necessary locations (either within serializeRouteValue 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 @@ | ||
| } | ||
|
|
||
| let component = 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
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
General fix:
Whenever generating JavaScript code by interpolating user-controllable data into template strings, you must perform additional escaping on top of JSON.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 like metaRouteName (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 @@ | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Escape unsafe JavaScript characters for safe insertion into JS source (script context). | ||
| * Escapes <, >, /, \\, null, and other special or line/paragraph separator chars. | ||
| */ | ||
| const charMap: Record<string, string> = { | ||
| '<': '\\u003C', | ||
| '>': '\\u003E', | ||
| '/': '\\u002F', | ||
| '\\': '\\\\', | ||
| '\b': '\\b', | ||
| '\f': '\\f', | ||
| '\n': '\\n', | ||
| '\r': '\\r', | ||
| '\t': '\\t', | ||
| '\0': '\\0', | ||
| '\u2028': '\\u2028', | ||
| '\u2029': '\\u2029' | ||
| } | ||
| function escapeUnsafeChars(str: string) { | ||
| return str.replace(/[<>\b\f\n\r\t\0\u2028\u2029/\\]/g, x => charMap[x]) | ||
| } | ||
|
|
||
| function getRouteFromNuxtPage (page: NuxtPage, metaImports: Set<string>, options: NormalizeRoutesOptions): NormalizedRoute { | ||
| const metaFiltered: Record<string, any> = {} | ||
| let skipMeta = 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.