search

roadrunner-server / roadrunner

🤯 High-performance PHP application server, process manager written in Go and powered with plugins
https://docs.roadrunner.dev
MIT License
7.85k stars 408 forks source link

[RFC] Add "allow" option to static middleware to specify allowed file extensions #645

Closed edsrzf closed 3 years ago

edsrzf commented 3 years ago

The static middleware currently has a forbid option to forbid certain file extensions. I think it would be more consistent with security best practices to keep a list of allowed file extensions.

As is, I have to be very careful about:

Failure could result in a security vulnerability.

To address this, I propose adding a new allow option to the static middleware, listing file extensions the middleware is allowed to serve. The allow option would be applied before forbid, so that if a file extension is listed in both, it is forbidden. (I can't think of any reason it would be useful to do this.)

An empty allow array would mean allowing all file extensions (except those forbidden by forbid). By default, allow would be an empty array, so it would be backward-compatible with existing configurations.

Here is an example configuration that only allows .css and .js file extensions:

http:
  address: 127.0.0.1:44933
  # ...
  middleware: [ "static" ]
  # ...
  static:
    dir: "assets"
    allow: [".css", ".js"]

I could imagine wanting to add similar functionality to the uploads configuration, but I'm not using uploads myself (yet?).

rustatian commented 3 years ago

Hello @edsrzf . Sure, I completely agree with you. Will add this in the 2.1.0 release.

rustatian commented 3 years ago

Unfortunately, I found some inconsistencies with the current implementation, so, the update will arrive a little bit later. Expected by the next sprint end.

edsrzf commented 3 years ago

No worries. Let me know if I can help. I'd originally intended to implement the option myself. It sounds like you've noticed something deeper that I would not be aware of.

rustatian commented 3 years ago

Yeah, I found, that this plugin should be totally rewritten because initially it was moved from RR1 w/o any changes. I'll use here new features from the go 1.16 like io/fs and implement etag #16 (long-awaited feature), thus I just didn't have time to make changes to this release 😞

edsrzf commented 3 years ago

Okay, that makes sense. Thanks for the clear update!

There's no rush on my end. This was just something I found challenging as I was prototyping moving an existing application to use Roadrunner.