Failure introduced from 3.8.5 --> 3.8.6; Passing on 3.9+ about aiohttp

[webknjaz]

Looks relate to the security fixes. Do you have a reproducer. Also, the current stream is 3.9.x. We won't be resurrecting 3.8. So I'm going to close this as wontfix.

[ShaneHarvey]

Here is a repro script:

import asyncio
import datetime
import email
import hashlib
import sys
import time

import aiohttp
import aiohttp.web


def format_date(d):
    return time.strftime("%a, %d %b %Y %H:%M:%S GMT", d.utctimetuple())


def parse_date(d):
    date_tuple = email.utils.parsedate(d)
    return datetime.datetime.fromtimestamp(time.mktime(date_tuple))


async def start_app(loop):
    app = aiohttp.web.Application()
    resource = app.router.add_resource("/fs/{filename}")
    handler = AioHttpHandler({"foo": {"data": b"foo", "upload_date": datetime.datetime.now()}})
    resource.add_route("GET", handler)
    resource.add_route("HEAD", handler)

    app_handler = app.make_handler()
    server = loop.create_server(app_handler, host="localhost", port=8088)

    srv, _ = await asyncio.gather(server, app.startup())
    return app, app_handler, srv


async def request(method, path, if_modified_since=None, headers=None):
    headers = headers or {}
    if if_modified_since:
        headers["If-Modified-Since"] = format_date(if_modified_since)

    session = aiohttp.ClientSession()

    try:
        method = getattr(session, method)

        resp = await method("http://localhost:8088%s" % path, headers=headers)
        await resp.read()
        return resp
    finally:
        await session.close()


class AioHttpHandler:
    def __init__(self, files):
        self.files = files

    async def __call__(self, request):
        """Send filepath to client using request."""
        try:
            filename = request.match_info["filename"]
        except KeyError:
            raise aiohttp.web.HTTPInternalServerError(text="Missing filename on request")

        if request.method not in ("GET", "HEAD"):
            raise aiohttp.web.HTTPMethodNotAllowed(
                method=request.method, allowed_methods={"GET", "HEAD"}
            )

        try:
            file = self.files[filename]
        except KeyError:
            raise aiohttp.web.HTTPNotFound(text=request.path)

        data = file["data"]
        upload_date = file["upload_date"]

        resp = aiohttp.web.StreamResponse()

        checksum = hashlib.sha256(data).hexdigest()

        resp.last_modified = upload_date
        resp.headers["Etag"] = '"%s"' % checksum
        resp.headers["Cache-Control"] = "public"

        # Check the If-Modified-Since, and don't send the result if the
        # content has not been modified
        ims_value = request.if_modified_since
        if ims_value is not None:
            # If our MotorClient is tz-aware, assume the naive ims_value is in
            # its time zone.
            if_since = ims_value.replace(tzinfo=upload_date.tzinfo)
            modified = upload_date.replace(microsecond=0)
            if if_since >= modified:
                resp.set_status(304)
                return resp

        # Same for Etag
        etag = request.headers.get("If-None-Match")
        if etag is not None and etag.strip('"') == checksum:
            resp.set_status(304)
            return resp

        resp.content_length = len(data)
        await resp.prepare(request)

        if request.method == "GET":
            await resp.write(data)
        return resp


async def test():
    # First request
    print('request: get "/fs/foo"')
    response = await request("get", "/fs/foo")

    assert response.status == 200
    assert (await response.read()) == b"foo"
    assert int(response.headers["Content-Length"]) == len(b"foo")
    assert response.headers["Cache-Control"] == "public"
    assert "Expires" not in response.headers

    last_mod_dt = parse_date(response.headers["Last-Modified"])

    # If-Modified-Since in the past, get whole response back
    past = last_mod_dt - datetime.timedelta(seconds=10)
    print(f'request: get "/fs/foo" if_modified_since={past}')
    response = await request("get", "/fs/foo", if_modified_since=past)
    assert response.status == 200
    assert (await response.read()) == b"foo"

    # Now check we get 304 NOT MODIFIED responses as appropriate
    future = last_mod_dt + datetime.timedelta(seconds=10)
    print(f'request: get "/fs/foo" if_modified_since={future}')
    response = await request("get", "/fs/foo", if_modified_since=future)
    assert response.status == 304
    assert (await response.read()) == b""


async def main():
    loop = asyncio.get_running_loop()
    app, app_handler, srv = await start_app(loop)
    await test()
    # aiohttp.rtfd.io/en/stable/web.html#aiohttp-web-graceful-shutdown
    srv.close()
    await srv.wait_closed()
    await app.shutdown()
    await app_handler.shutdown(timeout=1)
    await app.cleanup()


if __name__ == "__main__":
    print(f"Running with python: {sys.version}, aiohttp: {aiohttp.__version__}")
    asyncio.run(main())

Output on 3.8.6:

$ python repro-if-modified-since.py                 
Running with python: 3.10.11 (v3.10.11:7d4cc5aa85, Apr  4 2023, 19:05:19) [Clang 13.0.0 (clang-1300.0.29.30)], aiohttp: 3.8.6
/Users/shane/git/motor/repro-if-modified-since.py:28: DeprecationWarning: Application.make_handler(...) is deprecated, use AppRunner API instead
  app_handler = app.make_handler()
request: get "/fs/foo"
request: get "/fs/foo" if_modified_since=2024-01-31 16:10:35
request: get "/fs/foo" if_modified_since=2024-01-31 16:10:55
Traceback (most recent call last):
  File "/Users/shane/work/pycharm/motor-py310/lib/python3.10/site-packages/aiohttp/client_reqrep.py", line 905, in start
    message, payload = await protocol.read()  # type: ignore[union-attr]
  File "/Users/shane/work/pycharm/motor-py310/lib/python3.10/site-packages/aiohttp/streams.py", line 616, in read
    await self._waiter
  File "/Users/shane/work/pycharm/motor-py310/lib/python3.10/site-packages/aiohttp/client_proto.py", line 213, in data_received
    messages, upgraded, tail = self._parser.feed_data(data)
  File "aiohttp/_http_parser.pyx", line 557, in aiohttp._http_parser.HttpParser.feed_data
aiohttp.http_exceptions.BadHttpMessage: 400, message:
  Expected HTTP/:

    b'0'
      ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/shane/git/motor/repro-if-modified-since.py", line 152, in <module>
    asyncio.run(main())
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/Users/shane/git/motor/repro-if-modified-since.py", line 141, in main
    await test()
  File "/Users/shane/git/motor/repro-if-modified-since.py", line 133, in test
    response = await request("get", "/fs/foo", if_modified_since=future)
  File "/Users/shane/git/motor/repro-if-modified-since.py", line 45, in request
    resp = await method("http://localhost:8088%s" % path, headers=headers)
  File "/Users/shane/work/pycharm/motor-py310/lib/python3.10/site-packages/aiohttp/client.py", line 586, in _request
    await resp.start(conn)
  File "/Users/shane/work/pycharm/motor-py310/lib/python3.10/site-packages/aiohttp/client_reqrep.py", line 907, in start
    raise ClientResponseError(
aiohttp.client_exceptions.ClientResponseError: 400, message="Expected HTTP/:\n\n  b'0'\n    ^", url=URL('http://localhost:8088/fs/foo')

On 3.8.5 or >=3.9.0

$ python repro-if-modified-since.py
Running with python: 3.10.11 (v3.10.11:7d4cc5aa85, Apr  4 2023, 19:05:19) [Clang 13.0.0 (clang-1300.0.29.30)], aiohttp: 3.8.5
/Users/shane/git/motor/repro-if-modified-since.py:28: DeprecationWarning: Application.make_handler(...) is deprecated, use AppRunner API instead
  app_handler = app.make_handler()
request: get "/fs/foo"
request: get "/fs/foo" if_modified_since=2024-01-31 16:11:53
request: get "/fs/foo" if_modified_since=2024-01-31 16:12:13

It appears the bug is related to if_modified_since and HTTP 304 status responses.

Is there a aiohttp test to ensure this bug doesn't regress?

[webknjaz]

No idea. But feel free to submit PRs improving the test coverage.

[Dreamsorcerer]

Maybe to do with empty body? #7756
Otherwise, no idea, llhttp hasn't been updated since then..