Correctly handle Upgrade-requests for HTTP/2 by ignoring them

[hajs]

(Fix for #1501)

MDN (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Upgrade) says:

The server can choose to ignore the request, for any reason, in which case it should just respond as though the Upgrade header had not been sent (for example, with a 200 OK).

The previous implementation could only upgrade requests for WebSockets and generated errors for "Upgrade: h2c".

Example for uvicorn example:app:

async def app(scope, receive, send):
    assert scope['type'] == 'http'

    await send({
        'type': 'http.response.start',
        'status': 200,
        'headers': [
            [b'content-type', b'text/plain'],
        ],
    })
    await send({
        'type': 'http.response.body',

With this patch the server will send the correct result:

$ curl -v --http2 localhost:8000
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.81.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< date: Sun, 11 Sep 2022 21:14:15 GMT
< server: uvicorn
< content-type: text/plain
< transfer-encoding: chunked
<
* Connection #0 to host localhost left intact

Previous behaviour:

$ curl -v --http2 localhost:8000
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET / HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.81.0
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQCAAAAAAIAAAAA
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 400 Bad Request
< content-type: text/plain; charset=utf-8
< connection: close
< Transfer-Encoding: chunked
<
* Closing connection 0
Unsupported upgrade request.

[Kludex]

If we look for should_upgrade on the httptools implementation, why is the same logic not needed on on_body and on_message_complete?

[Kludex]

We can add a self.is_http2, and set as True on the on_header method.

[hajs]

Oh indeed, the code looks like, we should also replace the self.parser.should_upgrade() in on_body and on_message_complete with (self.parser.should_upgrade() and not self.is_http2).
Sorry, I had overlooked that because no unit test failed for this edge case.

[Kludex]

We can probably benefit from more tests that consider those conditionals 🙌

[Kludex]

The drop on coverage threshold shouldn't be needed if we add the necessary tests here. Would you like to work on those, @hajs ?

[hajs]

Yeah this is normally not a good idea. The coverage declined only after re-formatting this commit 0cffedc into multiple lines.

I am not sure how to test for different request-response-cycles. Do you have any idea?

[Kludex]

Thanks for the PR @hajs! 🙏

I'll be closing this, and continue on #1661, as it's the changes are a superset of these here.