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

Jump to bottom

static: clean the path URL before redirecting #199

Merged
merged 5 commits into from May 3, 2020
Merged

static: clean the path URL before redirecting #199

merged 5 commits into from May 3, 2020

Conversation

humaidq
Copy link
Contributor

@humaidq humaidq commented Apr 30, 2020 •
edited

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 •
edited

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 •
edited

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 •
edited

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
@humaidq
static: clean the path URL before redirecting.
6bd9385 
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
unknwon
unknwon approved these changes May 3, 2020
View reviewed changes
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
@@ -123,7 +123,7 @@ func staticHandler(ctx *Context, log *log.Logger, opt StaticOptions) bool {
return false
}

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.

static.go Outdated Show resolved Hide resolved
unknwon
unknwon reviewed May 3, 2020
View reviewed changes
static_test.go Show resolved Hide resolved
unknwon
unknwon reviewed May 3, 2020
View reviewed changes
static_test.go Outdated Show resolved Hide resolved
unknwon
unknwon reviewed May 3, 2020
View reviewed changes
static_test.go Outdated Show resolved Hide resolved
@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

@mattdavis90 mattdavis90 mentioned this pull request May 29, 2020
Closed
@simonpasquier simonpasquier mentioned this pull request Jun 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unknwon unknwon unknwon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vulnerability: open redirect in static handler
3 participants
@humaidq @unknwon @mattdavis90