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: generatePath incorrectly applying parameters #9051 #10078

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

Obi-Dann
Copy link
Contributor

@Obi-Dann Obi-Dann commented Feb 9, 2023

generatePath was doing multiple passes on the path using string replace, the first two passes were applying parameters, the third pass was doing a cleanup and the fourth path was applying the splat. It was possible to get incorrect results while applying splat when the last parameter value ended with *:

const path = generatePath("/route/:name", {
    name: encodeURIComponent("includes *asterisk at the end*"),
})
Expected: "/route/includes *asterisk at the end*"
Received: "/route/includes *asterisk at the end"

results of the first two passes return the value of /route/*asterisk at the end* which was later treated as path with the splat resulting in the last asterisk removed.

it was also possible to inject the splat value unintentionally

generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })
Expected: "/courses/foo*"
Received: "/courses/foosplat_should_not_be_added"

A safer option, instead of mutating a global path multiple times, is to split the path into segments, process each segment in isolation and then join them back together.

fixes #9051

@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2023

⚠️ No Changeset found

Latest commit: 2a1892e3a7be6df93f8158d64f1e12f629a8e60c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 9, 2023

Hi @Obi-Dann,

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

generatePath was doing multiple passes on the `path` using string replace, the first two passes were applying parameters, the third pass was doing a cleanup and the fourth path was applying the `splat`.
It was possible to get incorrect results while applying `splat` when the last parameter value ended with `*`:

```ts
const path = generatePath("/route/:name", {
    name: encodeURIComponent("includes *asterisk at the end*"),
})
```
```
    Expected: "/route/includes *asterisk at the end*"
    Received: "/route/includes *asterisk at the end"
```
results of the first two passes return the value of `/route/*asterisk at the end*` which was later treated as path with the splat resulting in the last asterisk removed.

it was also possible to inject the splat value unintentionally
```ts
generatePath("/courses/:name", { name: "foo*", "*": "splat_should_not_be_added" })
```
```
    Expected: "/courses/foo*"
    Received: "/courses/foosplat_should_not_be_added"
```

A safer option, instead of mutating a global path multiple times, is to split the path into segments, process each segment in isolation and then join them back together.

fixes remix-run#9051
paramName for the splat wasn't correctly resolved when the pas is `/*`
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Feb 9, 2023

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

@Obi-Dann
Copy link
Contributor Author

Obi-Dann commented Feb 9, 2023

⚠️ No Changeset found

Latest commit: 2a1892e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

The changeset is there, I added it and rebased. I guess the bot does not handle rebases well :)

@brophdawg11
Copy link
Contributor

split the path into segments, process each segment in isolation and then join them back together.

Thanks for this! I've actually got this approach sitting in a stash somewhere 😂

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.

3 participants