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 trailing redirection with query parameters #936

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.

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.

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

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.

axum/src/routing/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@calavera
Copy link
Contributor Author

Thanks for noticing this! I had one minor comment.

Updated

@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