expressjs / serve-static

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

allow root to be a function that returns the actual root for each request #74

Closed simonfan closed 7 years ago

simonfan commented 7 years ago

Hey people,

I've made a version of the middleware that allows using a function that returns the root for each request (instead of being only the same root for all request).

I find it useful for serving multiple websites on the same host (e.g. based on the server's hostname). In the tests I've simulated the usage for multiple users (very simplified though).

I'd like to hear your opinion on this. Thanks.

simonfan commented 7 years ago

@dougwilson thanks for the comments. I've made the changes you requested and linted the code as well.

Pull try-catch statements outside of main function so v8 can optimize it.

I've put the logic of try-catch in an auxiliary function that takes the request object and the next function. The main thing is that the try catch logic must be run in every request. https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax

dougwilson commented 7 years ago

Hi @simonfan I know this is a couple months old, but just wanted to circle back around. Other members of the Express project didn't have too much opinion about this expect for the general reservations from myself about what aspects are just part of a middleware instance and which are dynamic. I am under the feeling that the root path is an aspect that is directly coupled with the middleware instance and shouldn't be anything but static w.r.t. the instance. Besides philosophical, the nature of the root path is also of a security nature, and not having at least some fixed part to prevent accidental reads outside of the root may not go down well, as can be seen by the change of Express' res.sendFile to require a root path for a security audit fix.

You may be interested in either using res.sendFile, just using the send module directly, or rewriting req.url (which would be similar to how it would be done in traditional Apache / nginx) as a solution. Or even publish a fork under your npm account's namespace and come back with fork usage as evidence of feature demand :)

simonfan commented 7 years ago

Hey @dougwilson thank you for the follow up ;). I agree with your concerns on the security issues this may bring up and as it does not seem to be a major feature, I'll try rewriting req.url (didn't think of that before) and report the results later here.

Thank you