uvicorn icon indicating copy to clipboard operation
uvicorn copied to clipboard

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

Open hajs opened this issue 2 years ago • 1 comments

(Fix for https://github.com/encode/uvicorn/issues/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.

hajs avatar Sep 11 '22 21:09 hajs

Yeah this is normally not a good idea. The coverage declined only after re-formatting this commit https://github.com/encode/uvicorn/pull/1642/commits/0cffedcbc1c392fc1edbc5c6e59e8298578fcfbb into multiple lines.

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

hajs avatar Sep 20 '22 16:09 hajs

Thanks for the PR @hajs! 🙏

I'll be closing this, and continue on https://github.com/encode/uvicorn/pull/1661, as it's the changes are a superset of these here.

Kludex avatar Sep 29 '22 16:09 Kludex