encode / uvicorn

An ASGI web server, for Python. 🦄
https://www.uvicorn.org/
BSD 3-Clause "New" or "Revised" License
8.57k stars 745 forks source link

uvicorn may respond to requests sent after the client asks for the connection to be closed #2238

Closed Kludex closed 3 months ago

Kludex commented 9 months ago

Discussed in https://github.com/encode/uvicorn/discussions/2234

Originally posted by **kenballus** January 28, 2024 ### Describe the bug From RFC 9112, section 9.6: > A server that receives a "close" connection option MUST initiate closure of the connection (see below) after it sends the final response to the request that contained the "close" connection option. The server SHOULD send a "close" connection option in its final response on that connection. The server MUST NOT process any further requests received on that connection. When uvicorn receives a pipeline with a request containing `Connection: close`, followed by an invalid request, uvicorn responds only to the second (invalid) request, even though the standard requires that uvicorn respond only to the first one. ### To Reproduce 1. Start the example server from the README. 2. Send it a pipeline consisting of a valid request with `Connection: close` set, followed by an invalid request: ```sh printf 'GET / HTTP/1.1\r\nConnection: close\r\n\r\nInvalid\r\n\r\n' | nc localhost 8080 ``` 3. Observe that the only response received is intended for the invalid request: ``` HTTP/1.1 400 Bad Request content-type: text/plain; charset=utf-8 Transfer-Encoding: chunked Connection: close 1e Invalid HTTP request received. 0 ``` ### Expected behavior The server should respond only to the first request, and then close the connection. ### Logs/tracebacks ```python-traceback INFO: 127.0.0.1:51922 - "GET / HTTP/1.1" 200 OK ERROR: Exception in ASGI application Traceback (most recent call last): File "/usr/local/lib/python3.11/dist-packages/uvicorn/protocols/http/h11_impl.py", line 404, in run_asgi result = await app( # type: ignore[func-returns-value] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__ return await self.app(scope, receive, send) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/example.py", line 4, in app await send({ File "/usr/local/lib/python3.11/dist-packages/uvicorn/protocols/http/h11_impl.py", line 486, in send output = self.conn.send(event=response) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/h11/_connection.py", line 512, in send data_list = self.send_with_data_passthrough(event) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/local/lib/python3.11/dist-packages/h11/_connection.py", line 537, in send_with_data_passthrough self._process_event(self.our_role, event) File "/usr/local/lib/python3.11/dist-packages/h11/_connection.py", line 272, in _process_event self._cstate.process_event(role, type(event), server_switch_event) File "/usr/local/lib/python3.11/dist-packages/h11/_state.py", line 293, in process_event self._fire_event_triggered_transitions(role, _event_type) File "/usr/local/lib/python3.11/dist-packages/h11/_state.py", line 311, in _fire_event_triggered_transitions raise LocalProtocolError( h11._util.LocalProtocolError: can't handle event type Response when role=SERVER and state=MUST_CLOSE ``` ### Python Version ```console $ python --version Python 3.11.2 ``` ### uvicorn Version ```console $ python -m pip show uvicorn Name: uvicorn Version: 0.27.0 Summary: The lightning-fast ASGI server. Home-page: Author: Author-email: Tom Christie License: Location: /usr/local/lib/python3.11/dist-packages Requires: click, h11 Required-by: ``` ### h11 Version ```console $ python -m pip show h11 Name: h11 Version: 0.14.0 Summary: A pure-Python, bring-your-own-I/O implementation of HTTP/1.1 Home-page: https://github.com/python-hyper/h11 Author: Nathaniel J. Smith Author-email: njs@pobox.com License: MIT Location: /usr/local/lib/python3.11/dist-packages Requires: Required-by: uvicorn ``` ### OS Debian 12 (running in Docker on Arch Linux) Linux 6.7.2 ### Additional context Some other HTTP implementations that handle this correctly: Apache httpd, Boost::Beast, Daphne, H2O, Lighttpd, Nginx, Tornado, OpenWrt uhttpd, Waitress Some other HTTP implementations that also have this bug: Mongoose, aiohttp

[!IMPORTANT]

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.

Fund with Polar

kenballus commented 9 months ago

The repro that I provided above has a typo (the first request is missing a Host header).

Here's a better pipeline that actually demonstrates the bug:

printf 'GET / HTTP/1.1\r\nConnection: close\r\nHost: a\r\n\r\nGET / HTTP/1.1\r\n\r\n' | nc localhost 8080

There are two requests in this pipeline; the first is valid and should close the connection. The second is invalid (missing Host header) and should receive no response at all.

What Uvicorn does is respond to the second request and not the first.

liuguanlin commented 9 months ago

same error

ERROR: Exception in ASGI application Traceback (most recent call last): File "/data/anaconda3/lib/python3.11/site-packages/uvicorn/protocols/http/h11_impl.py", line 404, in run_asgi result = await app( # type: ignore[func-returns-value] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/uvicorn/middleware/proxy_headers.py", line 84, in __call__ return await self.app(scope, receive, send) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/fastapi/applications.py", line 1054, in __call__ await super().__call__(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/applications.py", line 123, in __call__ await self.middleware_stack(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/middleware/errors.py", line 186, in __call__ raise exc File "/data/anaconda3/lib/python3.11/site-packages/starlette/middleware/errors.py", line 164, in __call__ await self.app(scope, receive, _send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/middleware/cors.py", line 83, in __call__ await self.app(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/middleware/exceptions.py", line 62, in __call__ await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app raise exc File "/data/anaconda3/lib/python3.11/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app await app(scope, receive, sender) File "/data/anaconda3/lib/python3.11/site-packages/starlette/routing.py", line 762, in __call__ await self.middleware_stack(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/routing.py", line 782, in app await route.handle(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/routing.py", line 297, in handle await self.app(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/routing.py", line 77, in app await wrap_app_handling_exceptions(app, request)(scope, receive, send) File "/data/anaconda3/lib/python3.11/site-packages/starlette/_exception_handler.py", line 64, in wrapped_app raise exc File "/data/anaconda3/lib/python3.11/site-packages/starlette/_exception_handler.py", line 53, in wrapped_app await app(scope, receive, sender) File "/data/anaconda3/lib/python3.11/site-packages/starlette/routing.py", line 72, in app response = await func(request) ^^^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/fastapi/routing.py", line 299, in app raise e File "/data/anaconda3/lib/python3.11/site-packages/fastapi/routing.py", line 294, in app raw_response = await run_endpoint_function( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/fastapi/routing.py", line 191, in run_endpoint_function return await dependant.call(**values) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/ChatGLM-6B/visualglm-6b/api.py", line 38, in visual_glm json_post_raw = await request.json() ^^^^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/starlette/requests.py", line 249, in json body = await self.body() ^^^^^^^^^^^^^^^^^ File "/data/anaconda3/lib/python3.11/site-packages/starlette/requests.py", line 242, in body async for chunk in self.stream(): File "/data/anaconda3/lib/python3.11/site-packages/starlette/requests.py", line 236, in stream raise ClientDisconnect() starlette.requests.ClientDisconnect

kenballus commented 9 months ago

same error

I can't tell whether you're supporting this issue or trying to poke a hole in it :)

That error isn't necessarily confirmation of the bug; you'd have to look at the server's HTTP response in order to see the bad behavior.

Kludex commented 9 months ago

Confirmed. PR welcome.

Massakera commented 9 months ago

@Kludex do you have a guess about where's the file that needs to be changed? I was thinking on h11_impl.py but looking at it

        if self.response_complete:
            if self.conn.our_state is h11.MUST_CLOSE or not self.keep_alive:
                self.conn.send(event=h11.ConnectionClosed())
                self.transport.close()
            self.on_response()

it looks ok to me so I'm bit confused

berkio3x commented 9 months ago

@kenballus

The repro that I provided above has a typo (the first request is missing a Host header).

Here's a better pipeline that actually demonstrates the bug:

printf 'GET / HTTP/1.1\r\nConnection: close\r\nHost: a\r\n\r\nGET / HTTP/1.1\r\n\r\n' | nc localhost 8080

There are two requests in this pipeline; the first is valid and should close the connection. The second is invalid (missing Host header) and should receive no response at all.

What Uvicorn does is respond to the second request and not the first.

I get below in response. has this issue been fixed already?

date: Sun, 18 Feb 2024 17:34:35 GMT
server: uvicorn
content-type: text/plain
connection: close
transfer-encoding: chunked

d
Hello, world!
0
kenballus commented 9 months ago

The issue has not been fixed. I just reproduced it again on the current master branch (latest commit is 1e5f1be at the time of writing).

When I send that pipeline, I get the following:

HTTP/1.1 400 Bad Request
content-type: text/plain; charset=utf-8
Transfer-Encoding: chunked
Connection: close

1e
Invalid HTTP request received.
0
nayanmanojgupta commented 8 months ago

+1 to what @berkio3x said, got below response when I tried to run it.

HTTP/1.1 200 OK
date: Wed, 28 Feb 2024 18:09:27 GMT
server: uvicorn
content-type: text/plain
connection: close
transfer-encoding: chunked

d
Hello, world!
0
Kludex commented 8 months ago

We don't need further confirmation.

kenballus commented 8 months ago

We don't need further confirmation.

Agreed, though I am curious about why others are not able to reproduce the issue.

JustAPyro commented 8 months ago

I'd be willing to take a shot at fixing this, if nobody else is working on it. I haven't contributed to Uvicorn before though, would there be anything to keep in mind apart from what's in https://www.uvicorn.org/contributing/?

theyashl commented 4 months ago

I was able to reproduce the above issue. Also went through the code of uvicorn and h11. I do have RCA for this problem. NGL this bug is kinda funny lol. Because it'll only occur if you have installed uvicon with pip. It does not occur with manual installation using repo code.

Issue: When passed more than one HTTP requests within one single TCP stream, where first request has Connection: close header, the server should only respond to first request and close the connection. Instead in current case, server is returning Invalid HTTP request received. response for second request which is invalid.

Observations: This issue only occurs for pip installed uvicorn. Works totally fine with manually installed version. Unless you explicitly set the http parameter to h11 while booting your application.

Reason: pip installed uvicorn uses h11 HTTP protocol implementation because httptools is not on PyPi hence which did not got installed. Now in h11 protocol, h11 sets the client_state to MUST_CLOSE after processing first request because of Connection header which is set to close (please refer to h11 state machines here). But since the TCP stream contains two requests, while processing the second request h11 checks the state of the client, which is currently set to MUST_CLOSE, but the request buffer still has data. Which is not expected, and hence it raises LocalProtocolError("Got data when expecting EOF").

Possible fix: In the H11Protocol implementation, we need to check if the client's state is set to MUST_CLOSE and if some data is still there in the received request buffer then we need to override that remaining buffer with an empty buffer.

Kludex commented 3 months ago

Fixed on #2375.

Uvicorn 0.30.4 contains the fix.