search

aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
15.11k stars 2.02k forks source link

aiohttp may respond to requests sent after the client asks for the connection to be closed #8087

Closed kenballus closed 1 month ago

kenballus commented 9 months ago

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 aiohttp receives a pipeline with a request containing Connection: close, followed by an invalid request, aiohttp responds only to the second (invalid) request, even though the standard requires that aiohttp respond only to the first one.

To Reproduce

  1. Start the following web werver: 
    
from aiohttp import web

async def respond(request): return web.Response(text="hello world")

app = web.Application() app.add_routes([web.route("", "/{whatever:.}", respond)]) web.run_app(app, port=8080)

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
  1. Observe that the only response received is intended for the invalid request: 
    
HTTP/1.0 400 Bad Request
Content-Type: text/plain; charset=utf-8
Content-Length: 25
Date: Mon, 29 Jan 2024 03:20:29 GMT
Server: Python/3.11 aiohttp/4.0.0a2.dev0

Bad status line 'Invalid'


### Expected behavior

The server should respond only to the first request, and then close the connection.

### Logs/tracebacks

```python-traceback
Error handling request
Traceback (most recent call last):
  File "/app/aiohttp/env/lib/python3.11/site-packages/aiohttp/web_protocol.py", line 366, in data_received
    messages, upgraded, tail = self._request_parser.feed_data(data)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/aiohttp/env/lib/python3.11/site-packages/aiohttp/http_parser.py", line 325, in feed_data
    msg: _MsgT = self.parse_message(self._lines)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/aiohttp/env/lib/python3.11/site-packages/aiohttp/http_parser.py", line 558, in parse_message
    raise BadStatusLine(line) from None
aiohttp.http_exceptions.BadStatusLine: 400, message:
  Bad status line 'Invalid'

Python Version

$ python --version
Python 3.11.2

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 4.0.0a2.dev0
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author:
Author-email:
License: Apache 2
Location: /app/aiohttp/env/lib/python3.11/site-packages
Requires: aiohappyeyeballs, aiosignal, frozenlist, multidict, yarl
Required-by:

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.0.4
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /app/aiohttp/env/lib/python3.11/site-packages
Requires:
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.4
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache-2.0
Location: /app/aiohttp/env/lib/python3.11/site-packages
Requires: idna, multidict
Required-by: aiohttp

OS

Debian 12 (running in Docker on Arch Linux) Linux 6.7.2

Related component

Server

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, Uvicorn

Code of Conduct

Dreamsorcerer commented 2 months ago

That error is from the Python parser. With llhttp, you get the more accurate: Data after Connection: close:.

Dreamsorcerer commented 2 months ago

A client that sends a "close" connection option MUST NOT send further requests on that connection (after the one containing the "close") https://www.rfc-editor.org/rfc/rfc9112#section-9.6-5

For the server, it merely says that it should not process further requests. It does not say it should process the previous request. So, I think tweaking the Python parser to match the llhttp error is fine.

kenballus commented 1 month ago

For the server, it merely says that it should not process further requests. It does not say it should process the previous request.

I suppose this might be technically allowable, but it's pretty unintuitive in my opinion. I would assume that data received after a connection has been declared closed should not invalidate previous valid messages sent on that connection.

I've since tested this on many more HTTP implementations.

These ones reject the second request without responding to the first:

These ones respond to the first request and ignore the second because the connection is closed: (Mongoose and Uvicorn have changed sides on this because of reports that I submitted, so feel free to ignore those two)

Given the behavior of other implementations, it's probably worth at least documenting that AIOHTTP handles things differently.

Dreamsorcerer commented 1 month ago

As we use llhttp, if you convince Node.js to change behaviour, then we'll update the Python parser to match. But, it'd be weird to have different behaviour depending on the parser used.

Given that the client in this case has violated the HTTP protocol, I don't think it really matters whether the behaviour is intuitive or not, it should never be encountered by an HTTP client.