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: allow nested splat routes to begin with "special" url-safe characters #8563

Merged
merged 5 commits into from Feb 28, 2022

Conversation

shamsup
Copy link
Contributor

@shamsup shamsup commented Jan 6, 2022

closes #8525 and #8561

Fixes route matching to allow valid, but non-word-boundary characters(., -, ~) and url-encoded entities to appear at the beginning of a nested * route.

I pulled this character set from RFC 3986, section 2.3 Unreserved Characters:

2.3. Unreserved Characters

Characters that are allowed in a URI but do not have a reserved
purpose are called unreserved. These include uppercase and lowercase
letters, decimal digits, hyphen, period, underscore, and tilde.

  unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

I extended this to include url-encoded entities (ie %20), but I am not 100% confident I made the character range wide enough. ([0-7][0-9A-F]). It is probably safe to expand this to any 2-digit hex code, but I wasn't able to find any concrete resources.

Let me know if additional test cases are needed for this, since I'm not too familiar with the codebase and possible untested behaviors.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 6, 2022

Hi @shamsup,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 6, 2022

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

// Additionally, allow paths starting with `.`, `-`, `~`, and url-encoded entities,
// but do not consume the character in the matched path so they can match against
// nested paths.
"(?:(?=[.~-]|%[0-7][0-9A-F])|(?:\\b|\\/|$))";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(?=...) is a positive lookahead that doesn't consume the characters. Using this, we can use the characters to match, but they won't be part of the match string that gets removed to make the remaining path for child routes.

ie

(/(?:(?=a)|b)/).exec('a'); // => ['']
(/(?:(?=a)|b)/).exec('b'); // => ['b']

@shamsup
Copy link
Contributor Author

shamsup commented Jan 21, 2022

Is there anything outstanding here or anything I could improve?

@doiali
Copy link

doiali commented Feb 9, 2022

nice work.
I'm waiting for this fix

@egerrek
Copy link

egerrek commented Feb 11, 2022

Please accept this fix, routes with Cyrillic characters do not work

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

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

Thank you for the fix @shamsup!

@theostavrides
Copy link
Contributor

theostavrides commented Mar 4, 2022

@shamsup I found a case where the splat routes are still not working. This is when the route is wrapped in a layout route.
https://reactrouter.com/docs/en/v6/getting-started/concepts#layout-routes

Here is my code:

<Routes>
    <Route element={<Layout />}>
        <Route path=":id" element={<Chat />} />
    </Route>
</Routes>

/@tom does match

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.

None yet

7 participants