ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Add allow_http10_connect in http_opts #304

Closed JesseStimpson closed 1 year ago

JesseStimpson commented 1 year ago

When true, this option permits out-of-spec proxy servers that respond with HTTP/1.0 on the response to the CONNECT. Default behaviour is to only allow HTTP/1.1.

This PR is submitted to address #303 .

These changes pass my local testing, which includes:

  1. A successful tunnel through goproxy with allow_http10_connect => true, which responds with HTTP/1.0 on the CONNECT response. goproxy issue[1]
  2. An new error response[0] with allow_http10_connect => false when HTTP/1.0 is provided.
  3. make eunit
  4. make ct

I've not attempted to add any test cases or documentation yet. The test cases for tunneling are quite comprehensive, so I would need some guidance on the best way to approach this.

[0] New error response:

** exception error: no match of right hand side value {error,{stream_error,{badstate,"CONNECT cannot be used over an HTTP/1.0 connection by default. See http_opts `allow_http10_connect`."}}}

[1] Edit: I opened the referenced goproxy issue under the wrong project by mistake, so please ignore this particular link for now. If I am able to open the correct issue, I will edit this message later on.

essen commented 1 year ago

After thinking some time about this I think I will enable HTTP/1.0 responses without the flag. While RFC7231 doesn't say anything about the version in responses to CONNECT requests, RFC7230 establishes version rules and I don't think they prevent the use of HTTP/1.0. Also in practice because of how old CONNECT is I don't think there would be any HTTP/1.0 proxies that response positively and yet do not implement it properly (not that there is much it can fail on). I will do the change (just removing the version argument) and add a test.

essen commented 1 year ago

Pushed, please confirm it works for you (but probably does considering the change). Thanks!

JesseStimpson commented 1 year ago

I will check our usage, but based on the commit I'm sure it will work as expected. Thanks for the fix!

PS: It looks like #277 can be closed as well.