Add dsl method supported_http_methods

[MSP-Greg]

Description

Adds Puma::DSL#supported_http_methods, which allows overriding defaults in Puma::Const::SUPPORTED_HTTP_METHODS.

Turbo-Rails CI is currently broken.

Closes #3014

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[MSP-Greg]

I've converted this to a draft, as it needs changes.

1. Adding CONNECT to SUPPORTED_HTTP_METHODS is a breaking change, and I don't think it's a common http method.

2. The following will remain the Puma default:
   

   puma/lib/puma/const.rb

   Line 150 in e26b13e

   SUPPORTED_HTTP_METHODS = %w[HEAD GET POST PUT DELETE OPTIONS TRACE PATCH].freeze

3. One should be able to remove methods, if desired. Hence, Puma::DSL#additional_http_methods should be changed to Puma::DSL#supported_http_methods. Using supported_http_methods in a config file will replace the default methods specified in SUPPORTED_HTTP_METHODS. If one simply wants to add more methods, one can use SUPPORTED_HTTP_METHODS in the DSL method call. Example:

   supported_http_methods(Puma::Const:SUPPORTED_HTTP_METHODS + ['PROPFIND'])
   

4. If it's possible to check this before the request body is parsed, we should do so.

Thoughts?

EDIT: Updated per the above, Re fifth point, it would require passing a value into Client, not sure if the API changes are worth it?

[MSP-Greg]

After #3107, looked over open PR's tagged 'feature', and noticed #3041, which is very similar. Not sure what to do, but in the meantime I'll add @francois-ferrandis as a co-author, and add some more comments.

[MSP-Greg]

I've added more code comments, and added text to the upgrade doc Welcome to Puma 6: Sunflower. These additions are at the bottom of the doc.

I'm not sure we should be basing our default (SUPPORTED_HTTP_METHODS) on standards, but more so on what HTTP methods are in use by frameworks Puma is used with. I also think users should be able to choose exactly which HTTP methods are allowed.

This can lead to problems if Puma::DSL#supported_http_methods is used incorrectly, but we need to allow knowledgeable users full control over this...

EDIT:* Removed 'the' from 6.0-Upgrade.md...

[francois-ferrandis]

After #3107, looked over open PR's tagged 'feature', and noticed #3041, which is very similar. Not sure what to do, but in the meantime I'll add @francois-ferrandis as a co-author, and add some more comments.

It's OK for me to close my PR and let you take it from here. 🤝

I will be glad to be credited, thank you very much! 🙏

[nateberkopec]

I want some more time to think about this one so I'll leave it out from 6.2

[dentarg]

👍

[dentarg]

This seems like a good compromise if we are not going back to the Puma 5 behaviour of accepting any HTTP method (see #3014 (comment) and #3014 (comment)). We should probably assume some users do like the new behaviour introduced with Puma 6...

[MSP-Greg]

@dentarg

We should probably assume some users do like the new behaviour introduced with Puma 6

So, I guess this is your favorite new feature? Sorry...

This seems like a good compromise

I think one feature Puma that has always had is something similar to 'just start it and go'. That's fine for many apps, but high volume/capacity sites need a fair amount of fine-grained control/configuration. Obviously, stopping invalid request methods in Puma helps. So, allowing configuration of exactly what http methods are allowed seemed like the only way to handle it.

Having options like the below just weren't flexible enough...

1. Pick set A or set B
   -- or --
2. Methods from set A are always allowed, and one can only add to them

[MSP-Greg]

@nateberkopec

Good day!

I want some more time to think about this one

Any thoughts?

Assuming you agree with "high volume/capacity sites need a fair amount of fine-grained control/configuration", one can certainly screw things up, but that requires changing the default configuration...

[dentarg]

@MSP-Greg Hehe, it is not my favorite feature. I'm actually questioning why we did this to being with.

I think we should go so far to (provide the option) to restore the Puma 5 behavior of accepting arbitrary value for the HTTP method. I don't like that Puma will log a row because it thinks the HTTP method isn't supported. Anyone can send any value to my Puma app. With Puma 5 that was just a 404 (not logged unless I have request logging on): #3014 (comment)

[MSP-Greg]

Maybe we should change the logging, so it only is done so with 'request logging on'?

[MSP-Greg]

@dentarg

I don't like that Puma will log a row because it thinks the HTTP method isn't supported.

Maybe use LogWriter#debug`, or just remove the log liine

Anyone can send any value to my Puma app.

So how does it respond to a "PUMA-IS-GREAT" method? I'm thinking of adding a const Puma::Const::IANA_METHODS_REGISTRY, which would include all IANA registry methods

[dentarg]

@nateberkopec wdyt about this that I suggested a few weeks ago

I think we should go so far to (provide the option) to restore the Puma 5 behavior of accepting arbitrary value for the HTTP method.

[MSP-Greg]

@dentarg I updated this (forgot to rebase), below are four examples in the DSL# docs, I think the last one is what you'd like...

# Adds 'PROPFIND' to existing supported methods
supported_http_methods(Puma::Const::SUPPORTED_HTTP_METHODS + ['PROPFIND'])

# Restricts methods to the array elements
supported_http_methods %w[HEAD GET POST PUT DELETE OPTIONS PROPFIND]

# Restricts methods to the methods in the IANA Registry
supported_http_methods Puma::Const::IANA_HTTP_METHODS

# Allows any method
supported_http_methods :any

[dentarg]

That sounds great (haven't looked at the code here). Only comment is that just seeing supported_http_methods [] in a config make could make you think there's none allowed. Perhaps it would be possible to support something like supported_http_methods :any?

[MSP-Greg]

supported_http_methods :any

Good idea. I'll update...

[nateberkopec]

@MSP-Greg

Restricts methods to the methods in the IANA Registry
supported_http_methods Puma::Const::IANA_HTTP_METHODS

That would be the default you're proposing then, right?

[MSP-Greg]

@dentarg Code is now using :any (dsl code: supported_http_methods :any) to allow any http method, similar to Puma 5.

@nateberkopec

That would be the default you're proposing then, right?

Not really. I added the constant Puma::Const::IANA_HTTP_METHODS so people could easily use it, as it's unlikely that 'standard' frameworks would use methods that were not defined in the IANA registry.

We could certainly change the default, not sure whether that means releasing Puma 7?