search

expressjs / serve-static

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

incase of escaped characters in path #52

Closed stormslowly closed 8 years ago

stormslowly commented 8 years ago

if there is a space in the filename , will got a 404 error.

dougwilson commented 8 years ago

Hi! The tests are now failing, probably because this PR makes no sense; decodeURI is already called, just inside the send call, so your call here will cause a double decoding.

Please look into the test failures. In addition, to accept the PR, the PR needs to include a test that failed without your change and passes with your change, to prove that it is fixing an issue.

dougwilson commented 8 years ago

For now, I'm going to close this PR as invalid for the following reasons:

  1. The change is causing uncaught exception to occur, which introduces a huge security hole allowing one to DoS a server with this change on it.
  2. The change will now cause double-decoding of URLs, i.e. it makes the URL /foo%2525bar.txt load the file foo bar.txt when it should be loading the file foo%25bar.txt.
  3. There are no tests with the change.
  4. There is already a test that should prove loading things with a space works (https://github.com/expressjs/serve-static/blob/v1.10.0/test/test.js#L61-L65).
  5. This now incorrectly calls the send module. You can read the documentation at https://github.com/pillarjs/send#sendreq-path-options , which states "the path is a urlencoded path to send (urlencoded, not the actual file-system path)". You cannot decode the path before handing to send, or it's in violation of the API contact.

Please feel free to either open a new PR with a fix along with a test that fails without the change and passes with the change, or open an issue instead providing all the details for how we can reproduce thew issue.

stormslowly commented 8 years ago

sorry for my stupid mistake...