expressjs / serve-static

Serve static files
MIT License
1.39k stars 228 forks source link

Redirect to external website #26

Closed pierre-elie closed 9 years ago

pierre-elie commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

dougwilson commented 9 years ago

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

pierre-elie commented 9 years ago

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

dougwilson commented 9 years ago

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