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 Issue with decoding URLs #11138

Closed
wants to merge 11 commits into from

Conversation

labkey-nicka
Copy link

Rationale

This addresses #10814 by switching from using safelyDecodeURI() to using safelyDecodeURIComponent() in matchRoutes(). The reason using decodeURIComponent is necessary is because it does not assume is is operating on a full URI. From MDN:

decodeURI assumes the input is a full URI, so it does not decode characters that are part of the URI syntax.

The result is when decodeURI it can lead to portions of the string not being decoded properly.

Example

This example shows what is currently happening when decodeURI is used and is borrowed from #11109:

// Route definition
<Route path=":myParam" element={<MyPage />} />

// In MyPage
const myParam = useParams().myParam;
console.log(myParam);

Then navigate to the following URL: /bad%20%26%20encoding%20%25%20here

Expected: The parameter should be decoded and "bad & encoding % here" is logged.
Actual: The parameter is not decoded properly and "bad %26 encoding % here" is logged.

This PR fixes this and adds regression tests.

Changes

  • Switch matchRoutes() utility to use safelyDecodeURIComponent().
  • Only decode the pathname once rather than per call to matchRouteBranch as the pathname does not change.
  • Update tests to ensure coverage of decoding behavior.
  • Update safelyDecodeURIComponent to optionally take the paramName argument.
  • Removes safelyDecodeURI as this utility is no longer used. Happy to walk this back if the maintainers feel different about leaving code like this around.
  • Fix a typo.

Copy link

changeset-bot bot commented Dec 24, 2023

🦋 Changeset detected

Latest commit: b242c77

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router Patch
@remix-run/router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Dec 24, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

// Here we encode the second slash to allow for pattern matching against the second slash.
// The encoded variant can be thought of as a placeholder during this matching process.
// When this process is complete we undo this encoding iff it was applied.
const encodeDoubleSlash = pattern.end && pathname.startsWith("//");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this may seem odd I think it is cleaner then trying to refactor the regular expressions generated by compilePath() to handle "//". This should maintain backwards compatibility support for #8072 in light of the new decoding strategy introduced by this PR. @mjackson I'd appreciate it if you could take a look since you're familiar with this issue and the matching semantics in general.

@brophdawg11
Copy link
Contributor

Thank you for the PR @labkey-nicka! I had actually also been looking into this and had a local branch with some changes that I finally got completed and pushed up in #11199 so I'm going to close this one. I think this approach also had some minor issues with descendant routes and the way we do the remainingPathname/parentPathnameBase stuff which should be handled by the approach in #11999 as well.

@labkey-nicka labkey-nicka deleted the decode_uri_component branch February 8, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants