/% causes Bad Request

[moll]

Hey,

The serve-static middleware, when failing to decode a path URL, fails with a bad request error. I'd say it should merely ignore the request and pass it along to the app's final middleware.

Reproduce it by requesting /%.

Offending lines:

  var path = decode(this.path)
  if (path === -1) return this.error(400)

[moll]

Oh, right, forgot to mention those lines were from the send module serve-static uses.

[dougwilson]

I agree, it should. send should continue to operate the same, but this middleware should be smarter with pass throughs, like it does for 404.

[dougwilson]

Looks like this would automatically be addressable when send starts taking file system paths, but until them to avoid multiple decoding attempts, I just need to adjust send so I can determine what the reason for the 400 is. Basically right now send will 400 if there was a decoding issue or the URL contained %00 somewhere in it. We may want to next() in both those cases anyway, but it would be nice to just make the next()ing logic robust.

In fact, what do you think about this idea:

If send encountered any 4xx error before determining a file to actually send (i.e. encounter a 4xx condition before it successfully stat()d a file, just next(). This would mean we would next() on 404s, on URL decoding failures, on 0x00 being in the URL, and if the URL had a bunch of ../ going above the root path.

Does that sound reasonable?

[dougwilson]

@moll thoughts? :)

[moll]

Whoops, slow me. :innocent:

I think I'm in agreement. I can't think of a more elegant solution right now and probably YAGNI anyways. Invalid and malicious requests come in many forms and tend to be business as usual regardless, so handling (400ing) a subset in a static asset middleware is not its concern. So I'm opposable thumbs erect for next()ing.

[dougwilson]

From what I can tell, both Apache and nginx will 400 on a malformed URL right away (live example: https://www.mozilla.org/%). I'm just noting this while I'm researching prior art on this right now :)

[dougwilson]

Basically, it looks like replying with a 400 is the right thing to do, but there are two views with how this fits into the Express flow:

1) This module is expected to only reply with a file/error if the file actually exists. This would mean that no, we should not reply with a 400, but we should also not 403 or anything, either. Basically we say "if send didn't actually stat a file, then whatever the response is should be next().

or

2) This module is expected to be the endpoint for the path it is mounted on and as such, it should not fall through unless it is sure no file exists.

In fact, the more I think about it, the more I could see this module falling into a duality: adding in # 1 as a new option, and taking what currently exists and tweaking it a bit to make it into # 2. Thoughts, if you're still around :)?

[dougwilson]

The more I think about it, the more I think this is the best and most flexible option :)

[dougwilson]

Alright, this is finally implemented! By default, /% will just next() instead of 404 and people can always configure this otherwise :)

[moll]

Superb. Thanks. Confirmed that /% passes through to the next middleware out of the box.

[dougwilson]

:+1:

[Arezohayeman]

😊