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.

Sign up for GitHub

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

Jump to bottom

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

Open
1 task done
kenballus opened this issue Jan 29, 2024 · 4 comments · May be fixed by #9018
Open
1 task done

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

kenballus opened this issue Jan 29, 2024 · 4 comments · May be fixed by #9018
Labels
bug

Comments

@kenballus
Copy link
Contributor

kenballus commented Jan 29, 2024 •
edited
Loading

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)
  1. 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.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

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

  • I agree to follow the aio-libs Code of Conduct
@kenballus kenballus added the bug label Jan 29, 2024
@kenballus kenballus changed the title aiohttp prematurely terminates some connections when Connection: close is present aiohttp may respond to requests sent after the client requests for the connection to be closed Jan 29, 2024
@kenballus kenballus changed the title aiohttp may respond to requests sent after the client requests for the connection to be closed aiohttp may respond to requests sent after the client asks for the connection to be closed Jan 29, 2024
@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Sep 4, 2024 •
edited
Loading

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

@Dreamsorcerer
Copy link
Member

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.

@Dreamsorcerer Dreamsorcerer linked a pull request Sep 4, 2024 that will close this issue
Open
@kenballus
Copy link
Contributor Author

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:

  • Hypercorn
  • Node.js

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)

  • Apache httpd
  • Cheroot
  • cpp-httplib
  • Dart stdlib
  • FastHTTP
  • Eclipse GlassFish
  • Go net/http
  • Gunicorn
  • H2O
  • HAProxy
  • Hyper
  • Microsoft IIS
  • JetBrains Ktor
  • Eclipse Jetty
  • Libevent
  • Libmicrohttpd
  • Libsoup
  • Lighttpd
  • Mongoose
  • Netty
  • Nginx
  • OpenBSD httpd
  • OpenLiteSpeed
  • Phusion Passenger
  • PHP stdlib
  • protocol-http1
  • Puma
  • Apple ServiceTalk
  • Apache Tomcat
  • Tornado
  • Twisted
  • OpenWrt uhttpd
  • Unicorn
  • Uvicorn
  • Waitress
  • WEBrick
  • Akamai GHost
  • Apache Traffic Server
  • AWS Application Load Balancer
  • AWS Classic Load Balancer
  • Cloudflare
  • Google Classic Application Load Balancer
  • Envoy
  • OpenBSD relayd
  • Pingora
  • Pound
  • Squid
  • Varnish

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

@Dreamsorcerer
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reject data after close message aio-libs/aiohttp
2 participants
@Dreamsorcerer @kenballus