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(routing): supports encoded forward slashes in URI params #9696

Closed
wants to merge 4 commits into from

Conversation

asdfjkalsdfla
Copy link

@asdfjkalsdfla asdfjkalsdfla commented Jan 14, 2024

Changes

Fix for #7962

Testing

Ran unit test & added new unit test

Docs

If the user had a URI encoded , ? : @ & = + $ # / in the path, it will no longer be decoded automatically.

/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Jan 14, 2024

⚠️ No Changeset found

Latest commit: b0a2a4b

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 14, 2024
@asdfjkalsdfla asdfjkalsdfla changed the title fix(routing): supports encoded forward slashes in URI fix(routing): supports encoded forward slashes in URI params Jan 14, 2024
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

I think #7962 is a valid issue, but it's not totally clear to me if this is the most robust fix. Would you mind adding some additional tests for the cases that are potentially broken by this change?

@@ -64,7 +64,7 @@ function getRouteParams(route: RouteData, pathname: string): Params | undefined
if (route.params.length) {
// The RegExp pattern expects a decoded string, but the pathname is encoded
// when the URL contains non-English characters.
const paramsMatch = route.pattern.exec(decodeURIComponent(pathname));
const paramsMatch = route.pattern.exec(decodeURI(pathname));
Copy link
Member

Choose a reason for hiding this comment

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

We've had many URI encoding issues in the past, so we tend to be very cautious when modifying any logic related to this.

Seems like using decodeURI here would break users who have encoded , ? : @ & = + $ # but potentially fix just users who have encoded /?

Copy link
Author

@asdfjkalsdfla asdfjkalsdfla Jan 20, 2024

Choose a reason for hiding this comment

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

Yes, the change the behavior for users who have those symbols encoded in their parameter. But, it prevents encoded components from being interrupted as query params, uri params, location hash, etc... My gut is the change here is the right long-term strategy to prevent that class of bugs since this is really decoding a uri and not a component of the URI. (I also think may be issues with cache misses for these cases.)

I did think about adding automatic uri component decoding when accessing the parameter itself. However, I couldn't find a great spot to do that. The params object are used as part of the URI cache today, so, I can't encode it there and still have the cache work. There might be a spot to add this when accessing the value, but I don't know if that would cause other inconsistencies.

The behavior around this one was changed in a minor release in the past (i.e. from not decoded to decoded), so I leaned towards not making a more major refactor to keep backwards compatibility.

What do you think?

Copy link
Member

@ematipico ematipico Jan 24, 2024

Choose a reason for hiding this comment

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

Yes, the change the behavior for users who have those symbols encoded in their parameter

This will cause a breaking change for users who rely on , ? : @ & = + $ #. Not sure we can land this PR as is 🤔

Also, if we decided to apply this change using an experimental flag. Eventually, these users won't be able to use these characters.

@asdfjkalsdfla
Copy link
Author

Also, happy to add the additional test coverage for these other cases. Will try to get those over tomorrow.

@natemoo-re
Copy link
Member

Thanks again for the PR! I need to do a bit more digging on the original issue. The bug is totally valid but after testing this PR's decodeURI logic, I'm confident that this is not the correct fix.

We are technically decoding a URI Component rather than an entire URI here. The problem is probably higher up in the routing logic, decoding the / before we are able to match it to a route.

I'm going to close this PR, but I have put #7962 on my board and am hoping to investigate and fix it very soon.

@natemoo-re natemoo-re closed this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants