Express: Express 4.16.0 express.static broken for filename

[dougwilson]

I'm opening this issue to discuss and / or talk about an issue that has been reported twice (that I know of, at the time of opening this issue) so far regarding a change of behavior in express.static in Express 4.16.0 release.

In Express.js 4.16.0 there is a change in express.static that shouldn't cause issues (assuming correct API usage) but mitigates Express.js apps from the Node.js path validation vulnerability.

After tracking it down, it turns out there may be some apps (I don't know how common / uncommon yet) that were providing a file name as the first argument of express.static instead of a file path. This combined with a app.use path that would match the entire URL (like /* for example) seems to have been accidentally serving that file to all URLs in prior Express.js versions.

If this change broke you app, first, we apologize; the API has never worked or been documented this way so we had no idea that this is something that worked and thus also had zero tests for this kind of combination.

The first argument for express.static (which is the serve-static module) is document as the name root with the following description:

The root argument refers to the root directory from which the static assets are to be served.

I 100% understand that documentation can be terse and that working is working, so please don't take that as an excuse, just a post-mortem to the situation.

If you are in a situation where you can modify your app, the best thing you can do is use res.sendFile in a middleware instead of express.static for this specific case.

For example, change

app.use('/*', express.static('path/to/file.html'))

to

app.use('/*', (req, res) => res.sendFile(path.resolve('path/to/file.html'))

We would like to better understand the impact of this (if any) to evaluate if there needs to be a follow up patch release to get the above use-case back to working. It's possible there could be another odd use-case that doesn't work as well, so it may be whack-a-mole, which is why using the correct APIs is definitely the best course of action.

If you were impacted by this change, let us know here. Even if you took the action of changing to res.sendFile, feel free to also let us know anyway (noting that you changed your app), which can help act as a proxy for the silent users as well for impact evaluation.

[jakearchibald]

https://github.com/jakearchibald/wittr/pull/14 was impacted by this. It's a demo for an online course.

[dotLou]

If you were impacted by this change, let us know here. Even if you took the action of changing to res.sendFile, feel free to also let us know anyway (noting that you changed your app), which can help act as a proxy for the silent users as well for impact evaluation.

We've got several private repositories impacted by this change, but we've already begun moving to the suggested res.sendFile syntax above.

[RyotaSugawara]

If you were impacted by this change, let us know here.

I was impacted by this change too, and I will rewrite to res.sendFile my code.

[calebolin]

This issue has impacted my team. We support dozens of Express apps that service tens of millions of users every day. Our deployments are currently dead in the water until we can go into every app and pin Express to a previous version.

PLEASE consider releasing a hotfix for this ASAP.

[dougwilson]

Thanks for the feedback so far, everyone. We are evaluating a patch version that would restore the behavior, and code in tests around this to make it a "thing". It looks like when Express 4 first launched, this wouldn't have worked, but became possible accidentally in a change in 4.5.0, which means that it's been possible for the majority of the Express 4 lifetime.

[dougwilson]

If we could simply _revert_ the change, we would, but that would undo the security vulnerability patch it brings. Without the patch, a remote user can access _any file on your file system_. We are looking into an alternative method of fixing this that would restore the API mis-use of express.static and still keep your servers protected.

[dougwilson]

And to further the above comment: the thinking is to get a patch release out today for everyone. Potentially we'll look into detecting this pattern and deprecate it, but new deprecations are a semver minor, so 4.16.1 would continue to silently work with this code. We may not deprecate this at all, as it's not been discussed, just thinking out loud.

[LinusU]

Just a quick thought, we could potentially do fs.statSync when calling .static and switch to serving only a single file if the call returns .isFile() => true. That way the old behaviour would work, but it wouldn't expose any other file...

Our deployments are currently dead in the water until we can go into every app and pin Express to a previous version.

I would really really recommend using a package-lock.json (npm 5) or yarn.lock (yarn) file to combat this. It's the only way to consistently get reproducible builds...

[dougwilson]

Just a quick thought, we could potentially do fs.statSync when calling .static and switch to serving only a single file if the call returns .isFile() => true. That way the old behaviour would work, but it wouldn't expose any other file...

Yea, I've considered that, but it wouldn't be quite the same if that file wasn't there on the middleware creation but was added later. The stat would be what would trigger a potential deprecation warning in the future, but I think for now we should just get the old behavior back completely. I almost got a working solution now.

[calebolin]

RE: locks. We do that for production builds (well, similar but that's beside the point).

For anyone following along, it looks like the path validation vulnerability current being discussed only impacts Node v8.5.0.

[adamldoyle]

I created pillarjs/send#148 due to the impact to us but am commenting here as well to help with accurate counts. From an impact standpoint, we should be able to move over to res.sendFile with no issue, although I don't know if there was some reason that we originally went with express.static instead.

[dougwilson]

Yes, there is no difference, but in the end, a minor release should ideally not break anything. Of course, any change could break something, particularly bug fixes, but this seems like one that doesn't look like just a random single user and looks like there is a way to get it back to working exactly as it was, which in the end is good for reduced friction.

[dougwilson]

A fix is on the way through the dependencies and an Express 4.16.1 will be released today with the fix. It should function exactly as it used to, we're not going to warn or anything at this time, just get it back to working as-is.

[dougwilson]

Waiting for the CI to come back green and then 4.16.1 will be released. Thanks again for the feedback everyone. I'm sorry this d=caused some disruption during the upgrade, and hope you can see that we value the stability of Express, and take even disruptions of weird edge cases like this seriously :) I hope you continue to enjoy Express!

[calebolin]

Thank you! Appreciate the attention given to this issue.

[LinusU]

Great work @dougwilson! 👍