Repository navigation
Conversation
…ing slash AddTrailingSlash and RemoveTrailingSlash built the redirect Location from req.URL.Path, which is already percent-decoded. An escaped slash inside a path parameter therefore became a real path separator, so following the redirect matched a different route (404) or delivered a different parameter value. Build the Location from URL.EscapedPath() instead, and keep RawPath in step for the forward path. sanitizeURI now also treats a leading %2F/%5C as a slash so the existing open-redirect normalisation still applies to the escaped form. The percent-encoded location means one existing expectation changes: /%09%5Cevil.example/x keeps its backslash encoded. It is the same URL.
labstack#3134 already keeps RawPath in sync for forward mode, so this change must not duplicate it. Forward mode is left exactly as it is on master; only the redirect Location is now built from EscapedPath().
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.
Fixes #3206.
AddTrailingSlashWithConfigandRemoveTrailingSlashWithConfigbuilt the redirectLocationfromreq.URL.Path, which is already percent-decoded. An escaped slash inside a path parameter was therefore promoted to a real path separator, so following the redirect matched a different route.Change
Build the redirect
LocationfromURL.EscapedPath()so percent-encoded segments survive the redirect.sanitizeURIonly collapsed a leading/or\. Now that the location stays escaped it also treats a leading%2F/%5Cas a slash, so the existing open-redirect normalisation still applies to/%5Cexample.comand friends.Scope
Forward mode is deliberately untouched. Keeping
RawPathin sync when forwarding is a separate concern and is already covered by #3134 (open), so this PR does not duplicate or conflict with it — it changes only the redirect branch.One existing expectation changed
/%09%5Cevil.example/xnow redirects to/%09%5Cevil.example/x/rather than/%09\evil.example/x/. That is the same URL with the backslash left percent-encoded, which follows from keeping the location escaped. Every other open-redirect case inslash_test.goproduces the same locations as before.Tests
Added cases for
%2F,%3Fand%20in both middlewares, plus two follow-the-redirect tests that assert the original route is reached. All four fail againstmasterand pass with the change.