Skip to content

fix(gin): keep encoded slashes in trailing slash redirects - #4918

Open
b84330808 wants to merge 2 commits into
gin-gonic:masterfrom
b84330808:fix-raw-path-trailing-slash-redirect
Open

b84330808 wants to merge 2 commits into
gin-gonic:masterfrom
b84330808:fix-raw-path-trailing-slash-redirect

Conversation

@b84330808

Copy link
Copy Markdown

With UseRawPath or UseEscapedPath enabled, routing matches the escaped path, but redirectTrailingSlash builds the Location from the decoded URL.Path. An encoded slash then becomes a real separator:

r.UseRawPath = true
r.GET("/files/:name", h)
// GET /files/a%2Fb/  ->  301 Location: /files/a/b   (404s, should be /files/a%2Fb)

The same happens in the other direction (/dirs/a%2Fb -> /dirs/a/b/ for a /dirs/:name/ route).

This builds the redirect from the escaped path when routing used it and keeps URL.RawPath in sync, so the encoding survives. The default configuration is unchanged.

  • Open your pull request against the master branch.
  • All tests pass in available continuous integration systems (e.g., GitHub Actions).
  • Tests are added or modified as needed to cover code changes.
  • If the pull request introduces a new feature, the feature is documented in the docs/doc.md.

I used Claude Code to write this change.

With UseRawPath or UseEscapedPath the router matches the escaped path,
but redirectTrailingSlash built the Location from the decoded URL.Path.
A request for /files/a%2Fb/ was redirected to /files/a/b, which no
longer matches /files/:name. Build the redirect from the escaped path
when routing used it, and keep URL.RawPath in sync so the encoding
survives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.71%. Comparing base (0f09c3a) to head (5793a43).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4918      +/-   ##
==========================================
+ Coverage   98.65%   98.71%   +0.06%     
==========================================
  Files          48       48              
  Lines        3188     3195       +7     
==========================================
+ Hits         3145     3154       +9     
+ Misses         43       41       -2     
Flag Coverage Δ
--ldflags="-checklinkname=0" -tags sonic 98.64% <100.00%> (+<0.01%) ⬆️
-tags go_json 98.58% <100.00%> (+<0.01%) ⬆️
-tags nomsgpack 98.63% <100.00%> (+<0.01%) ⬆️
go-1.26 98.34% <100.00%> (+<0.01%) ⬆️
go-1.27 98.75% <100.00%> (+0.08%) ⬆️
macos-latest 98.71% <100.00%> (+0.06%) ⬆️
ubuntu-latest 98.65% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tales-of-zuna Tales-of-zuna left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for this! I reproduced the bug on master with a test of my own (route /files/:name, plus /dirs/:name/ for the other direction):

Mode Request Location on master
UseRawPath GET /files/a%2Fb/ /files/a/b
UseRawPath GET /dirs/a%2Fb /dirs/a/b/
UseEscapedPath both of the above same as UseRawPath
UseRawPath + X-Forwarded-Prefix: /api GET /files/a%2Fb/ /api/files/a/b
default GET /files/a%2Fb/ 404, the route doesn't match the decoded path, so this PR doesn't touch it

So the description is accurate, and the default configuration really is unaffected.

Suggestion: add an X-Forwarded-Prefix case to the test. The PR also changes the prefix line (p = prefix + "/" + p), but the new test doesn't go through it. From reading the code, I'd expect /api/files/a%2Fb:

  • sanitizePathChars strips % from the prefix, so the url.PathUnescape step can't decode anything in it.
  • The // between prefix and path is collapsed by http.Redirect's path.Clean, the same as on master.

I traced this by hand and didn't run it on the PR branch, so a test case would lock it in.

I also checked /x/%2F%2Fevil.com/ against a /x/:name route. It can't become a //host redirect on master (http.Redirect cleans it), and with this change the slashes stay encoded, so there's no open-redirect concern.

…irect

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@b84330808

Copy link
Copy Markdown
Author

Thanks for checking it so carefully. Added the X-Forwarded-Prefix: /api case in 5793a43: it redirects /files/a%2Fb/ to /api/files/a%2Fb as you expected.

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.

2 participants