Conversation
The router matches req.URL.RawPath when it is set, but AddTrailingSlash and RemoveTrailingSlash only changed req.URL.Path. For a path with an escape that differs from Go's default encoding (e.g. %40 from encodeURIComponent) the added slash was ignored (404) and the removed slash stayed in the last path parameter.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The router matches
req.URL.RawPathwhen it is set, butAddTrailingSlashandRemoveTrailingSlash(in rewrite mode, withoutRedirectCode) only changereq.URL.Path.Go sets
RawPathwhenever the client's escaping differs from Go's default, which is common:encodeURIComponent("jane@example.com")givesjane%40example.com. For such requests the slash middlewares do nothing the router can see:e.Pre(middleware.AddTrailingSlash())withe.GET("/users/:email/", h):GET /users/jane%40example.comreturns 404.e.Pre(middleware.RemoveTrailingSlash())withe.GET("/users/:email", h):GET /users/jane%40example.com/reaches the handler withc.Param("email") == "jane%40example.com/". The slash that should have been removed ends up in the last path parameter.Requests without escapes are unaffected, which is why this is easy to miss.
The fix updates
RawPathnext toPathin both middlewares when it is set (add or trim the trailing/). Redirect mode is unchanged.Tests:
TestAddTrailingSlash_escapedPathIsRoutedandTestRemoveTrailingSlash_escapedPathIsRoutedgo throughServeHTTP. Without the fix the first gets 404 and the second getsjane%40example.com/. With the fix both pass, and so dogo test -race ./middleware/andgo vet ./middleware/.middleware/slash.goonv4has the same code, if you want a backport.This change was found and written with AI assistance, and the failing and passing test runs above were run locally.