Skip to content

Fix trailing redirection with query parameters #936

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

Merged

Conversation

calavera
Copy link
Contributor

@calavera calavera commented Apr 16, 2022

Motivation

When the request URI matches a route that needs a trailing slash, or has an extra trailing slash, the redirect URI is not generated correctly.

This change adds or removes a trailing slash to the path part of the URI, instead of the full URI, preserving query parameters during redirection.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
isaacs isaacs
When the request URI matches a route that need a trailing slash, or has an extra trailing slash, the redirect URI is not generated correctly.

This change adds or removes a trailing slash to the path part of the URI, instead of the full URI, preserving query parameters during redirection.

Signed-off-by: David Calavera <david.calavera@gmail.com>
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

str::replace replaces all matches of a pattern, so /foo?bar=/foo would get rewritten to /foo/?bar=/foo/. Please use something like .find('?') + .insert(idx, '/') instead.

@davidpdrsn
Copy link
Member

There are also methods to take apart the path and query. Can't quite put it back together as easily but I think that's safer to use. We do that already in StripPrefix afaik.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
isaacs isaacs
Extract parts from Uri and recreate it, so it doesn't bump
into corner cases with string manipulation.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Copy link
Contributor Author

@jplatte, @davidpdrsn I've updated it to make it safer

Verified

This commit was signed with the committer’s verified signature. The key has expired.
isaacs isaacs
Signed-off-by: David Calavera <david.calavera@gmail.com>
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this! I had one minor comment.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
isaacs isaacs
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@calavera
Copy link
Contributor Author

Thanks for noticing this! I had one minor comment.

Updated

@calavera calavera requested review from davidpdrsn and jplatte April 17, 2022 17:19

Verified

This commit was signed with the committer’s verified signature. The key has expired.
isaacs isaacs
@davidpdrsn davidpdrsn enabled auto-merge (squash) April 17, 2022 21:06
@davidpdrsn davidpdrsn merged commit 83e1a15 into tokio-rs:main Apr 17, 2022
@calavera calavera deleted the fix_trailing_redirection_with_query branch April 17, 2022 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants