fix(routing): re-escape '#' and '?' left over from localePath's path normalization - #4099
fix(routing): re-escape '#' and '?' left over from localePath's path normalization#4099dargmuesli wants to merge 2 commits into
Conversation
…normalization
resolveRoute() resolves an already-resolved route object a second time to
attach the localized route name. Vue Router decodes percent-encoded
characters in `path` on that second resolve (kept for readable URLs, e.g.
`%20` becomes a literal space), but a literal '#' or '?' surviving that
decode is read as an actual fragment/query delimiter when the resulting
`fullPath` is later used as a real navigation target (navigateTo,
router.replace({ path }), <NuxtLink :to>), silently truncating the path.
`query`/`hash` are resolved separately and unaffected — only `path` (and
the `fullPath` string built from it) needs the two characters re-escaped.
Fixes nuxt-modules#4098
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds resolved-route sanitization that percent-encodes literal hash and query delimiters in Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/routing/routing.ts`:
- Around line 68-75: Update sanitizeResolvedPath to also refresh resolved.href
after encoding literal # and ? characters, using createHref(resolved.fullPath)
or the equivalent normalized value. Preserve the existing path and fullPath
updates, and add a regression assertion covering localeRoute(...).href for
decoded routes containing these characters.
In `@test/kit.test.ts`:
- Around line 165-170: Extend the localePath test case around the existing
encoded “#” assertion to use an encoded “?” within the path, such as
`/path/as%20a%20test%3Fone?foo=bar`. Assert that `%3F` remains encoded in the
result for each of the four normalization strategies, while preserving the
actual `?foo=bar` query delimiter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ce2f2b8-eae7-4a05-b704-d5d6b0b0350a
📒 Files selected for processing (2)
src/runtime/routing/routing.tstest/kit.test.ts
- sanitizeResolvedPath() now also recomputes href via router.options.history.createHref(fullPath), matching how vue-router itself derives href from fullPath — it was left stale (still showing the decoded, unsafe characters) after the previous commit only patched path/fullPath. - Extend the localePath regression case to cover an encoded '?' within the path (distinct from a real trailing '?query' delimiter), and add a localeRoute(...).href assertion.
|
Thanks! I'm a bit cautious with changes like these, ideally we leave encoding/decoding up to vue-router as much as possible as we have ran into issues trying to fix this in the past. Going to explore some other approaches as well. |
|
Yes, I'd expect stuff like this to be vue (extension) logic instead of nuxt (extension) logic too of course. I just stumbled upon this and want to point to the finding :) |
Resolves #4098
Summary
resolveRoute()(used bylocalePath,localeRoute, andswitchLocalePath) resolves an already-resolved route object a second time to attach the localized route name (ctx.router.resolve(ctx.resolveLocalizedRouteObject(normalized, locale))). For theprefix_except_default/prefix_and_defaultstrategies,resolveLocalizedRouteObject's path-based branch has already gone through onerouter.resolve()call (createLocalizedRouteByPathResolver), which decodes percent-encoded characters — kept intentionally, for readable URLs (%20→ literal space, already covered by an existing test intest/kit.test.ts).The second
router.resolve()call re-parses that decodedpathas a fresh location string. A literal#or?left over from decoding is read as an actual fragment/query delimiter rather than path text — confirmed directly against vue-router alone, no i18n module involved:query/hashare populated separately during resolution and are unaffected by this — verified they stay structurally correct ({"foo":"bar"}, not swallowed into the corrupted path) even whilepath/fullPathare wrong. Any caller that uses thelocalePath()/switchLocalePath()output directly as a navigation target —navigateTo(...)(SSR redirects),router.replace({ path }),<NuxtLink :to>— ends up navigating to a silently truncated path with a bogus fragment attached.This is the same underlying double-resolve pattern #4079 fixed for
switchLocalePath'srouteCopy(by omittingpathso the second resolve takes the name+params branch, which does correctly re-encode viaencodeParam). That approach doesn't work here without regressing the "readable URL" behaviortest/kit.test.tsalready covers (localePath('/path/as%20a%20test...')decoding to/path/as a test...) — using name+params always fully re-encodes viaencodeURI, including turning spaces into%20. So rather than avoiding the second resolve, this re-escapes only the two characters that are genuinely unsafe to leave literal in a path string (#→%23,?→%3F) on the already-resolved object, after both resolves, right before the caller gets it.Fix
Added
sanitizeResolvedPath()insrc/runtime/routing/routing.ts, applied at both return points ofresolveRoute()(the shared functionlocalePath/localeRoute/switchLocalePathall funnel through). No-ops whenpathdoesn't contain#/?(the common case).Test plan
localePathtest intest/kit.test.ts, covering all four strategies, alongside the existing "readable URL" assertion it must not regresspnpm lint— cleantsc --noEmit— cleanpnpm test:unit— 407 passed, 1 skipped (unchanged from main + the 1 new assertion)pnpm build— cleanpnpm test:e2e— 201 passed (33 files)pnpm test:types'sspecs/fixtures/typed_routessub-check fails in my local sandbox with unrelatedCannot find module '#app'/__I18N_*__errors — reproduces identically on a clean, unmodifiedmaincheckout (verified viagit stash), so it's a pre-existing local environment issue, not something this change introduces.Summary by CodeRabbit
%23and%3F) from being treated as fragment/query separators on subsequent resolutions.fullPath/hrefare correctly recomputed to preserve the expected escaping behavior.%23and%3Fremain encoded for both path-based and route-based localization resolution.