Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Kludex opened this issue Jan 29, 2024 Discussed in #2234 · 11 comments
Open

Comments

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 29, 2024

Discussed in #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:
printf 'GET / HTTP/1.1\r\nConnection: close\r\n\r\nInvalid\r\n\r\n' | nc localhost 8080
  1. 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

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

$ python --version
Python 3.11.2

uvicorn Version

$ python -m pip show uvicorn
Name: uvicorn
Version: 0.27.0
Summary: The lightning-fast ASGI server.
Home-page:
Author:
Author-email: Tom Christie <tom@tomchristie.com>
License:
Location: /usr/local/lib/python3.11/dist-packages
Requires: click, h11
Required-by:

h11 Version

$ 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
Copy link

kenballus commented Jan 29, 2024

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

This comment was marked as spam.

@kenballus
Copy link

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
Copy link
Sponsor Member Author

Kludex commented Feb 10, 2024

Confirmed. PR welcome.

@Massakera
Copy link

@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
Copy link

@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
Copy link

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
Copy link

+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
Copy link
Sponsor Member Author

Kludex commented Feb 28, 2024

We don't need further confirmation.

@kenballus
Copy link

We don't need further confirmation.

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

@JustAPyro
Copy link

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/?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants