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

Redirect to external website #26

Closed
pierre-elie opened this Issue Jan 3, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@pierre-elie

pierre-elie commented Jan 3, 2015

Stumbled upon a weird behavior where serve-static would redirect to an external website when "asked nicely".

Reproduction Steps

Using express 4.10.6 and static-serve 1.7.1 on node 0.10.33.

1. Simple app.js

var app = require('express')();
app.use(require('serve-static')('assets'));
app.listen(80);

2. Start server

$ sudo node app.js

3. Open in Firefox http://localhost//www.google.com/%2e%2e

Request
GET //www.google.com/%2e%2e HTTP/1.1
Host: localhost
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:33.0) Gecko/20100101 Firefox/33.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: fr,fr-fr;q=0.8,en-us;q=0.5,en;q=0.3
Accept-Encoding: gzip, deflate
Connection: keep-alive
Response
HTTP/1.1 303 See Other
X-Powered-By: Express
Content-Type: text/html; charset=utf-8
Location: //www.google.com/%2e%2e/
Date: Sat, 03 Jan 2015 01:13:49 GMT
Connection: keep-alive
Transfer-Encoding: chunked

Redirecting to <a href="//www.google.com/%2e%2e/">//www.google.com/%2e%2e/</a>

4. You get redirected to Google...


It works in Firefox, Safari and probably IE, not in Chrome.
Setting static-serve’s option redirect: false seems to fix it (but redirect: true is the default).

It looks like many applications could be affected.
A quick test on apps listed on http://expressjs.com/resources/applications.html does not disappoint:

send emits directory in that case, which triggers the redirection.

@dougwilson dougwilson added the bug label Jan 3, 2015

@dougwilson dougwilson self-assigned this Jan 3, 2015

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 3, 2015

Member

Great, I'll fix this as soon as I get home. The caveat is that you need to mount this middleware at the root.

Member

dougwilson commented Jan 3, 2015

Great, I'll fix this as soon as I get home. The caveat is that you need to mount this middleware at the root.

@dougwilson dougwilson closed this in 0399e39 Jan 3, 2015

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 3, 2015

Member

Hi @pierre-elie ! If you have the time and are willing, I would love it if you could verify the fix that is currently on master.

Member

dougwilson commented Jan 3, 2015

Hi @pierre-elie ! If you have the time and are willing, I would love it if you could verify the fix that is currently on master.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 3, 2015

Member

I went ahead and published it as 1.7.2, but I would still love to hear your assessment on the change as well :)

Member

dougwilson commented Jan 3, 2015

I went ahead and published it as 1.7.2, but I would still love to hear your assessment on the change as well :)

@pierre-elie

This comment has been minimized.

Show comment
Hide comment
@pierre-elie

pierre-elie Jan 5, 2015

Hey, thanks a lot for the fast change!
It definitely fixes it, though I was wondering if there would be an advantage to use path.normalize()
(in this specific case it would "convert" //www.google.com/../ to /)

pierre-elie commented Jan 5, 2015

Hey, thanks a lot for the fast change!
It definitely fixes it, though I was wondering if there would be an advantage to use path.normalize()
(in this specific case it would "convert" //www.google.com/../ to /)

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 5, 2015

Member

The reason I didn't use path.normalize is a couple reasons: 1) that's not a file system path, so it wouldn't even work right when run on Windows (path.normalize would change all the slashes to backslashes on Windows) and 2) consider that this module was mounted at /some/path and a user requested /some/path/.., we would redirect the user outside of this module's control, which would probably lead to some other kind of unexpected behavior.

Member

dougwilson commented Jan 5, 2015

The reason I didn't use path.normalize is a couple reasons: 1) that's not a file system path, so it wouldn't even work right when run on Windows (path.normalize would change all the slashes to backslashes on Windows) and 2) consider that this module was mounted at /some/path and a user requested /some/path/.., we would redirect the user outside of this module's control, which would probably lead to some other kind of unexpected behavior.

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 5, 2015

Member

P.S., if you haven't done so already, please feel free to report this to https://nodesecurity.io/ , where the affected versions are all < 1.7.2 and fixed is 1.7.2.

Member

dougwilson commented Jan 5, 2015

P.S., if you haven't done so already, please feel free to report this to https://nodesecurity.io/ , where the affected versions are all < 1.7.2 and fixed is 1.7.2.

@pierre-elie

This comment has been minimized.

Show comment
Hide comment
@pierre-elie

pierre-elie Jan 5, 2015

Right. Looks good to me then! Thanks again :)

pierre-elie commented Jan 5, 2015

Right. Looks good to me then! Thanks again :)

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jan 5, 2015

Member

And thank you soo much for bringing this to me attention :)! Go community!

Member

dougwilson commented Jan 5, 2015

And thank you soo much for bringing this to me attention :)! Go community!

@pierre-elie

This comment has been minimized.

Show comment
Hide comment
@pierre-elie

pierre-elie Jan 5, 2015

Reported by a researcher from https://bugcrowd.com/

pierre-elie commented Jan 5, 2015

Reported by a researcher from https://bugcrowd.com/

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 24, 2015

Member

Hey guys, this exact bug has existed in Python's SimpleHTTPServer since 2006. Feel free to attack them for it :)

Member

dougwilson commented Feb 24, 2015

Hey guys, this exact bug has existed in Python's SimpleHTTPServer since 2006. Feel free to attack them for it :)

@expressjs expressjs locked and limited conversation to collaborators Apr 27, 2015

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