Skip to content

static: clean the path URL before redirecting#199

Merged
unknwon merged 5 commits intogo-macaron:masterfrom
humaidq:302vuln
May 3, 2020
Merged

static: clean the path URL before redirecting#199
unknwon merged 5 commits intogo-macaron:masterfrom
humaidq:302vuln

Conversation

@humaidq
Copy link
Contributor

@humaidq humaidq commented Apr 30, 2020

This prevents a malicious redirect with a crafted URL. Resolves #198.

It prevents links such as:
http://localhost:4000/%2fhumaidq.ae%2f..
http://localhost:4000//humaidq.ae%2f..
from giving a 302 Found redirecting to humaidq.ae

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #199 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #199   +/-   ##
=======================================
  Coverage   91.77%   91.77%           
=======================================
  Files          11       11           
  Lines        1556     1557    +1     
=======================================
+ Hits         1428     1429    +1     
  Misses         96       96           
  Partials       32       32           
Impacted Files Coverage Δ
static.go 90.00% <100.00%> (+0.08%) ⬆️

@unknwon
Copy link
Contributor

unknwon commented May 2, 2020

Thanks for the PR! But I am not confident this is the right change to fix #198.

I think we should instead do this on line 126 (not verified):

-file := ctx.Req.URL.Path
+file := path.Clean(ctx.Req.URL.Path)

Besides, can you add some tests to catch this case?

@unknwon
Copy link
Contributor

unknwon commented May 2, 2020

I can't see your reply here. Why path.Clean can't solve the problem? https://play.golang.org/p/AB5Ih30w0bc

@humaidq
Copy link
Contributor Author

humaidq commented May 2, 2020

I don't know why it doesn't solve it. Let me push with the test and check it out.
Edit: https://github.com/go-macaron/macaron/pull/199/checks?check_run_id=638633200#step:4:884
Edit 2: My bad, realised what the issue was. Pushing a fix.

@humaidq humaidq marked this pull request as draft May 2, 2020 10:59
This prevents a malicious redirect with a crafted URL.
@humaidq humaidq marked this pull request as ready for review May 2, 2020 11:16
@humaidq humaidq changed the title static: check static file prefix before redirecting static: clean the path URL before redirecting May 2, 2020
Copy link
Contributor

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Nice,

static.go Outdated
}

file := ctx.Req.URL.Path
file := path.Clean(ctx.Req.URL.Path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the call of path.Clean here is ineffective and needless, so we can remove it.

@unknwon
Copy link
Contributor

unknwon commented May 3, 2020

Sorry about noises 😅 Waiting for CI to merge.

@unknwon unknwon merged commit addc746 into go-macaron:master May 3, 2020
@mattdavis90
Copy link

Hi, this seems to have started causing issues when StaticOptions.Prefix != "". The path.Clean call only ends in "/" when ctx.Req.URL.Path == "/" (or similar) which isn't the case if Prefix != "", hence you get into a redirect-spiral and no content can be served. Thanks

@humaidq
Copy link
Contributor Author

humaidq commented May 29, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

vulnerability: open redirect in static handler

3 participants