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

Don't strip trailing slash if it's escaped in StripSlashes middlware #790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nuuls
Copy link

@nuuls nuuls commented Jan 30, 2023

This fixes an edge case where a path ending in a trailing encoded slash %2F is incorrectly stripped and rctx.RoutePath is set to the unescaped Path, resulting in routing errors.

The specific edge case in my project is:

  • rctx is nil
  • rctx.RoutePath is empty
  • r.URL.RawPath contains multiple escaped slashes in various places
  • rctx.RoutePath is set to the unescaped values contained in r.URL.Path
  • The Mux no longer matches the route /link_resolver/{url} because all %2F are now /

A similar issue also exists in RedirectSlashes, I can open a second PR for that as well if you'd like.

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

LGTM

Can you please force-push to the branch to kick off CI?

@brasic
Copy link

brasic commented Jan 17, 2025

@VojtechVitek it seems like the author may have abandoned this PR, but it's needed: the current behavior strips trailing %2F strings from the URL inappropriately. I've tested this code in our project and it fixed the issue. If @nuuls isn't around to kick-off CI I'm happy to open a new PR with this content to do so.

Although this might properly be considered a breaking change since in my experience it changes the routing of plenty of real-world URLs that apps might have depended on.

@nuuls nuuls force-pushed the fix-escaped-path-strip branch from bb53683 to f152ebd Compare January 17, 2025 20:32
@nuuls
Copy link
Author

nuuls commented Jan 17, 2025

Ah sorry, completely forgot about this one 😄 Pushed it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants