Cookies sent in 302 redirect not handled correctly anymore

[scranen]

Long story short

When a 302 sets or clears a cookie, newer versions of aiohttp do not correctly process this.

Expected behaviour

1. If a 302 response sets a cookie, the resulting request will use the new value.
2. If a 302 response clears a cookie, the resulting request will omit that cookie.

Actual behaviour

In aiohttp 3.4.4:

1. As expected.
2. As expected.

In aiohttp 3.5.0:

1. If a 302 sets a cookie, the old value remains.
2. As expected.

In aiohttp 3.5.3:

1. If a 302 sets a cookie, the old value remains.
2. If a 302 clears a cookie, the old value remains.

Steps to reproduce

import logging
import pytest
import socket
import sys
from aiohttp import ClientSession, log, web


@pytest.mark.asyncio
async def test_302_cookie(host_and_session):
    """
    Baseline: check that /200 returns the content of the cookie set by the
    client
    """
    host, session = host_and_session
    async with session.get(host + '/200') as response:
        assert await response.text() == ''
        assert response.cookies.get('c').value == 'something'
    async with session.get(host + '/200') as response:
        assert await response.text() == 'something'
        assert response.cookies.get('c').value == 'something'


@pytest.mark.asyncio
async def test_302_cookie(host_and_session):
    """
    Check that /302_overwrite overwrites the cookie, and that the client sends
    the new value.

    Passes in aiohttp 3.4.4, fails in 3.5.0
    """
    host, session = host_and_session
    # Trigger Set-Cookie response
    async with session.get(host + '/200') as response:
        assert await response.text() == ''
        assert response.cookies.get('c').value == 'something'
    async with session.get(host + '/302_overwrite') as response:
        assert response.cookies.get('c').value == 'something'
        assert await response.text() == 'nothing'  # <-- fails


@pytest.mark.asyncio
async def test_302_cookie(host_and_session):
    """
    Check that /302_clear clears the cookie, and that the client does not send
    the cookie either.

    Passes in aiohttp 3.5.2, fails in 3.5.3
    """
    host, session = host_and_session
    async with session.get(host + '/200') as response:
        assert await response.text() == ''
        assert response.cookies.get('c').value == 'something'
    async with session.get(host + '/302_clear') as response:
        assert response.cookies.get('c').value == 'something'
        assert await response.text() == ''  # <-- fails


async def _server_200(request: web.Request):
    """
    Returns the cookie value as a text body and sets the cookie to "something".
    """
    return web.Response(
        body=request.cookies.get('c', ''),
        headers={'Set-Cookie': 'c="something"; Path=/'})


async def _server_302_overwrite(_request):
    """
    Sets the cookie to "nothing" and redirects to /200.
    """
    raise web.HTTPFound(
        location='/200',
        headers={'Set-Cookie': 'c="nothing"; Path=/'})


async def _server_302_clear(_request):
    """
    Clears the cookie and redirects to /200.
    """
    clear = 'c=""; expires=Thu, 01 Jan 1970 00:00:00 GMT; Max-Age=0; Path=/'
    raise web.HTTPFound(
        location='/200',
        headers={'Set-Cookie': clear})


def access_logger_class():
    """
    Helper to enable verbose logging for aiohttp >= 3.5.0.
    """
    try:
        class AccessLogger(web.AbstractAccessLogger):
            def log(self, request, response, time):
                def join(headers):
                    pairs = [f'{k}: {v}' for k, v in headers.items()]
                    return '\n  '.join([''] + pairs)

                self.logger.info(f'Request: {request.method} {request.path}'
                                 f'{join(request.headers)}')
                self.logger.info(f'Response: {response.status}'
                                 f'{join(response.headers)}')

        return {'access_log_class': AccessLogger}
    except AttributeError:
        return {}


@pytest.fixture(name="host_and_session")
async def run_server():
    """
    Fixture that spins up a sample server, and returns a hostname and a
    client session that can be used to connect to the server as a tuple.
    """
    log.access_logger.setLevel(logging.DEBUG)
    log.access_logger.addHandler(logging.StreamHandler(sys.stdout))
    with socket.socket() as sock:
        sock.bind(('127.0.0.1', 0))
        port = sock.getsockname()[1]
    app = web.Application()
    app.add_routes([
        web.get('/302_clear', _server_302_clear),
        web.get('/302_overwrite', _server_302_overwrite),
        web.get('/200', _server_200)
    ])
    runner = web.AppRunner(app, **access_logger_class())
    await runner.setup()
    site = web.TCPSite(runner, port=port)
    host = 'http://localhost:{}'.format(port)
    await site.start()

    async with ClientSession() as session:
        yield host, session

    await site.stop()
    await runner.shutdown()

[betatim]

I also ran into this bug and tried out ad42bdd where this has been fixed/started working again.

Nice test case! Should that be added to the test suite to stop the bug from coming back?

[scranen]

Nice, did not see that. Looks like this issue was addressed in #3576.

[betatim]

Let's close this issue then?

[blueyed]

This can be closed.