Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: do not encode slashes in catch-all routes #2291

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ll3006
Copy link

@ll3006 ll3006 commented Jul 4, 2024

This fixes #1638

Encoding of the parameters is now done by the matcher and is dependend on the matcher keys: every key has a keepSlash property that reflects if it should encode the slash character or not.

The keepSlash currently applies to all custom regex that either match a slash or contain one in their literal part, excluding lookaheads, lookbehinds and negative ranges. This is very simplistic but should be enough to cover both catch-all (e.g. "(.*)" routes) and explicit path regexes (e.g. "(deep/path/[a-z]*/)" )

I have also added a few tests to ensure everyting is working properly.

The Router > 'can redirect to a star route when encoding the param' test is currently failing as it sets a parameter with an urlencoded slash (%2F) and expects to find it as it is in the route. However, since this PR allows literal slashes in star routes, the actual url contains the sequence as a literal (%252F) rather than keeping the escaped slash.

I could edit the test, but I am unsure if this is behaivour that is being relied upon. In case it is, more work is needed to distinguish user-set urlencoded path segments from the normal usecase.

To test this PR I have updated the sandbox from #1638 here: codesandbox.io

This applies to all custom regex routes which match a slash or
contain one in their literal part

fixes vuejs#1638
Copy link

netlify bot commented Jul 4, 2024

Deploy Preview for vue-router ready!

Name Link
🔨 Latest commit e683d7e
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/6748ffcd441bb400085effc2
😎 Deploy Preview https://deploy-preview-2291--vue-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 95 (🔴 down 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (🟢 up 8 from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

ll3006 added a commit to ridamoe/rida that referenced this pull request Aug 1, 2024
The patched version fixes the issue where url parameters specified
with catch-all regex url-encode the slash character. The patched
version correctly handles this case while also preserving encoding
on standard parameters.
A PR for this patch has been submitted upstream [here](vuejs/router#2291).
TODO: if this is merged, switch to npm registry version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slashes should not be encoded for catch all regexes
1 participant