plack / Plack

PSGI toolkit and server adapters
http://plackperl.org/
Other
486 stars 214 forks source link

Support restricting request methods for static files and add OPTIONS support #661

Closed robrwo closed 3 years ago

robrwo commented 3 years ago

This modifies Plack::App::File to support restricting the allowed request methods. If these are set, then it will reject other request methods with HTTP 405.

If it receives an OPTIONS request (assuming it is an allowed method), then it will return a simple OPTIONS response (without CORS support).

This also modifies the Static middleware to pass through the methods attribute to Plack::App::File.

Note that I consider Plack::App::File to be the proper place for handling OPTIONS requests, since it knows about the resource.

CORS support is not added here, since that can be handled with other packages.

This closes #660.

miyagawa commented 3 years ago

As I mentioned in #660 I suggest you to turn this into a generic Plack::Middleware so you can add it to any PSGI app, not just Plack::App::File.

miyagawa commented 3 years ago

Note that I consider Plack::App::File to be the proper place for handling OPTIONS requests, since it knows about the resource.

OPTIONS is a preflight request and it doesn't necessarily have to know whether the resource exists or not in its response.

Edit: if it's important for you to respond with 404 to OPTIONS, I guess the middleware can only handle OPTIONS for 204/405 when the response from the app is not 200.

robrwo commented 3 years ago

Ok, I can remove OPTIONS support. But it seems there should be some way to restrict methods other than GET or HEAD. It doesn't make sense to respond to POST, PUT or DELETE requests for static documents, for instance.

Putting a filter in another layer is also a bit messy: the Static middleware knows what resources there are, whereas putting a filter in another layer that has to know what different layers support and whether they are valid resources kind of breaks the point about keeping things in separate layers.

miyagawa commented 3 years ago

It doesn't make sense to respond to POST, PUT or DELETE requests for static documents, for instance.

Yeah that makes sense. Right now I guess it responds to whatever method as if it were a GET request.

whereas putting a filter in another layer that has to know what different layers support and whether they are valid resources kind of breaks the point about keeping things in separate layers.

It can be argued as being more flexible and reusable, as a beauty of PSGI middleware.

As I noted in my edited comment, I think the middleware can work in a response phase (rather than a request phase), if handling OPTIONS for non-existent resource would be a concern for you.

robrwo commented 3 years ago

Ok, I've removed OPTIONS support. It just adds an optional methods attribute for restricting request methods.

miyagawa commented 3 years ago

To be honest I'm not sure if you need a backward compatibility here, I'd argue that the current behavior is incorrect, and it should only respond to GET/HEAD.

robrwo commented 3 years ago

I'm fine with that. How best is it to set defaults? Using the prepare_app method?

miyagawa commented 3 years ago

Oh, what I meant was that we don't need an option and just reject requests other than GET/HEAD. Whoever wishes to retain the current behavior would be able to write a middleware to change the request method to GET.

robrwo commented 3 years ago

Ok, I'm going to close this PR and create another.