Puma 6 hard-codes "allowed" HTTP methods

[kyledrake]

Puma 6 has introduced a SUPPORTED_HTTP_METHODS constant and a check which fails other methods:

puma/lib/puma/request.rb

Line 94 in 3d33475

status, headers, app_body = [501, {}, ["#{env[REQUEST_METHOD]} method is not supported"]]

This will break, at a bare minimum, WebDAV implementations behind Puma, which use methods like PROPFIND and MKCOL.

I'm not sure what the "correct" approach here is, but we did have a working WebDAV implementation that is no longer working with Puma 6 and I've had to downgrade for now to figure out how to deal with this for the time being.

[nateberkopec]

I think we could add a note about this in the upgrade guide/history under breaking changes. However, any support of non-standard HTTP verbs prior to Puma 6 was completely accidental.

When considering how Puma should react, RFC 9110 lists the 8 standard methods, so that's how we implemented this.

IMO HTTP methods are not a 100% Rack-level concern so it's Puma's job to make sure that we handle HTTP methods correctly. Supporting arbitrary methods is therefore not good.

[dentarg]

What about allowing the user to specify methods that should also be considered supported? If the user knows their rack app can handle it.

I actually had the thought that this was a breaking change but I failed to come up with a valid use case so I never said anything in the PR.

[kyledrake]

The RFC as I interpret it doesn't say that the HTTP methods listed are required to be the only methods (only that GET and HEAD are required and the rest are optional). It says I should respond with 501 only if the origin server does not recognize or does not implement the method.

I think the ambiguity is who's in charge of making that determination, puma or the web app. Since puma is primarily used for highly customized programming, I feel like it should be the responsibility of the web application, not the HTTP server, to make this determination because in my use case, I do recognize the method and can handle it.

If this was causing a major security problem I might have a different opinion (but would probably still do the implementation upstream with something like nginx), but failing that check I think it's reasonable to move back to the original behavior and allow the web application to decide how to handle irregular methods.

[MSP-Greg]

The RFC as I interpret it doesn't say that the HTTP methods listed are required to be the only methods

Correct. See Hypertext Transfer Protocol (HTTP) Method Registry, note that there are RFC's for all the additional methods.

Both PROPFIND and MKCOL are listed there.

See RFC9110 16.1 Method Extensibility

Maybe a good option would be to leave the code as is, and add an option like 'http-methods-extended', which would add the methods in the registry?

@kyledrake are you using methods that are not defined in the registry?

[kyledrake]

I'm not using anything "non-standard" that WebDAV wouldn't use. And frankly if I had designed WebDAV it would have used the common HTTP methods and/or been designed a lot better. 🙃

[MSP-Greg]

To clarify, all the "non-standard" methods you're using are listed in Hypertext Transfer Protocol (HTTP) Method Registry?

[kyledrake]

All of the methods I am using are in that list, correct.

[francois-ferrandis]

I would like to work on a PR for this issue. 😉 Which option would work best in your opinion?

1. Add a boolean option to allow "extended methods" (the ones listed here)
2. Add an option to specify the allowed HTTP methods (defaults to %w[HEAD GET POST PUT DELETE OPTIONS TRACE PATCH])
3. Add a boolean option to disable method checking entirely

I started working on tests and implementation, but I would love your opinion on which design would be the best. 🙏

[dentarg]

I like 2 best

[MSP-Greg]

I just found the code I started for this, which defines two constants in lib/puma/const.rb (see below), and adds a DSL method http_methods_extended, which switches to allowing all the methods listed at https://www.iana.org/assignments/http-methods/http-methods.xhtml.

Thoughts? Not sure about method/const naming, or point 2 above by @francois-ferrandis

I think I wanted to bench whether it was better to create hashes or arrays. Or, what's faster, Hash.key?, Array#include?, or one of the Array bsearch methods...

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

# see https://www.iana.org/assignments/http-methods/http-methods.xhtml
# updated 07-Nov-2022
 EXTENDED_HTTP_METHODS = %w[
  # list from the registry
]

[nateberkopec]

I'm not sure I was 100% right here. Now that I'm seeing the alternative in #3041, I think maybe I made a mistake saying we should not accept all HTTP methods. Adding this extra config point seems pretty fiddly, and I guess I was surprised with how many people depended on the non-standard HTTP methods.

[dentarg]

I think it is useful to keep the config option, in order to address what was requested in #1441, to reject requests that your app doesn't support

The default could be changed though, to accept any method. Would we have to wait until Puma 7 for that?

[francois-ferrandis]

I think maybe I made a mistake saying we should not accept all HTTP methods.

I have to agree, I feel like puma should accept any method by default. I will gladly close PR #3041 if all that complexity is not needed. 🧹

I think it is useful to keep the config option, in order to address what was requested in #1441, to reject requests that your app doesn't support

I can't seem to find where that need was expressed, is it really puma's responsibility or could it be left to puma users to handle that themselves? Genuine question here. 🙃

The default could be changed though, to accept any method. Would we have to wait until Puma 7 for that?

I am also curious about versioning and communication. It is unclear to me how these decisions are made, where can I find more info on the project's governance?

Thank you all for your time. 🙏

[dentarg]

I think it is useful to keep the config option, in order to address what was requested in #1441, to reject requests that your app doesn't support

I can't seem to find where that need was expressed, is it really puma's responsibility or could it be left to puma users to handle that themselves? Genuine question here. 🙃

It comes from #1441, an issue reported in 2017 for Puma 3.10.0. In 2021, Nate commented on it and PR #2932 followed in 2022.

You can trigger the issue like this:

arm64 $ gem install -v 5.2.1 puma
Fetching puma-5.2.1.gem
Building native extensions. This could take a while...
Successfully installed puma-5.2.1
1 gem installed

~
arm64 $ echo 'app { [200, {}, ["OK"]] }' | puma --config /dev/stdin
Puma starting in single mode...
* Puma version: 5.2.1 (ruby 3.1.2-p20) ("Fettisdagsbulle")
*  Min threads: 0
*  Max threads: 5
*  Environment: development
*          PID: 23483
* Listening on http://0.0.0.0:9292
Use Ctrl-C to stop

Make the PROXY request (using wget as suggested in #1441 did not trigger it for me (wget version 1.21.3))

arm64 $ curl --proxytunnel --proxy http://localhost:9292 http://example.com/
curl: (56) Received HTTP code 500 from proxy after CONNECT

Puma 5 will log

2023-01-12 13:40:18 +0100 Read: #<RuntimeError: No REQUEST PATH>

Puma 6 will log

Unsupported HTTP method used: CONNECT

Puma 6 also logs Unsupported HTTP method used for any unsupported HTTP method where as Puma 5 did not log anything (as the request was passed down to your app). (Test that with curl --request FOO localhost:9292 for example).

I think I misunderstood #1441, I thought it was much easier to trigger the RuntimeError. I'm fine with going back to Puma 5 behaviour, or only doing what Nate suggested initially: #1441 (comment)

[nateberkopec]

That context is very helpful @dentarg. Let me think about it some more...

[dentarg]

@nateberkopec Another thing to consider is that Puma has now turned curl --request FOO ... (or any "unsupported" method) into a hard-coded 501 response where-as before, for this simple Rack/Sinatra app it was an harmless 404:

$ bundle add puma -v '~>5'
$ bundle add sinatra

$ cat config.ru
require "sinatra/base"

class App < Sinatra::Base
  get "/" do
    "OK"
  end
end

run App

$ bundle e puma --port 8080

$ curl -s -v --request FOO localhost:8080
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> FOO / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.81.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< X-Cascade: pass
< Content-Type: text/html;charset=utf-8
< X-XSS-Protection: 1; mode=block
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< Content-Length: 493
<
<!DOCTYPE html>
<html>
<head>
  <style type="text/css">
  body { text-align:center;font-family:helvetica,arial;font-size:22px;
    color:#888;margin:20px}
  #c {margin:0 auto;width:500px;text-align:left}
  </style>
</head>
<body>
  <h2>Sinatra doesn’t know this ditty.</h2>
  <img src='http://localhost:8080/__sinatra__/404.png'>
  <div id="c">
    Try this:
    <pre># in config.ru
class App
  foo &#x27;&#x2F;&#x27; do
    &quot;Hello World&quot;
  end
end
</pre>
  </div>
</body>
</html>
* Connection #0 to host localhost left intact

[MSP-Greg]

@Fjan

See comment #3106 (comment)

[nateberkopec]

our app needs to support Javascript CORS requests. Those requests use a PROPFIND method as part of their handshake

Hm? Not sure what you're referring too there, as PROPFIND is a method for WEBDAV. It's possible you have to preflight that, but the PROPFIND method itself is a WEBDAV thing.

[Fjan]

@nateberkopec You are right! I got PROPFIND mixed up with OPTIONS, I'll remove my comment to avoid confusion.