CookieError on invalid cookie names

[irmatov]

Long story short

aiohttp servers fail with 500 Internal error on requests where Cookie header is present and cookie name contains invalid symbols for cookie name. aiohttp uses SimpleCookie from python's http.cookies package. Although SimpleCookie follows standards and supposedly correctly rejects such cookie names, real world is much messier and such cookies are seen frequently.

For the reference, RFC2109 states that cookie attribute (name) is a token:

The two state management headers, Set-Cookie and Cookie, have common
syntactic properties involving attribute-value pairs.  The following
grammar uses the notation, and tokens DIGIT (decimal digits) and
token (informally, a sequence of non-special, non-white space
characters) from the HTTP/1.1 specification [RFC 2068] to describe
their syntax.

av-pairs        =       av-pair *(";" av-pair)
av-pair         =       attr ["=" value]        ; optional value
attr            =       token

RFC2616 defines token as:

token          = 1*<any CHAR except CTLs or separators>
separators     = "(" | ")" | "<" | ">" | "@"
               | "," | ";" | ":" | "\" | <">
               | "/" | "[" | "]" | "?" | "="
               | "{" | "}" | SP | HT

So any cookie attribute containing one of ( ) < > @ , ; : \ " / [ ] ? = { } SP HT is considered invalid by SimpleCookie. Django uses own cookie parsing because of similar reasons.

Expected behaviour

Such invalid cookie should still be accepted as they are tolerated by browsers and most web servers.

Actual behaviour

500 Internal server error caused by exception CookieError.

Steps to reproduce

Given sample http server:

from aiohttp import web

async def handle(request):
    text = ""
    for k, v in request.cookies:
        text += '\n{}\t{}'.format(k, v)
    return web.Response(text=text)

app = web.Application()
app.router.add_get('/', handle)
app.router.add_get('/{name}', handle)

web.run_app(app)

When one executes curl -H 'Cookie: ISAWPLB{DB45DF86-F806-407C-932C-D52A60E4019E}=x' -v http://localhost:8080/ against this server it receives 500 internal server error. On the server side a traceback is visible:

Error handling request
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/helpers.py", line 554, in __get__
    return inst._cache[self.name]
KeyError: 'cookies'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/web_protocol.py", line 410, in start
    resp = yield from self._request_handler(request)
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/web.py", line 325, in _handle
    resp = yield from handler(request)
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/web_middlewares.py", line 93, in impl
    return (yield from handler(request))
  File "./test.py", line 7, in handle
    for k, v in request.cookies:
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/helpers.py", line 556, in __get__
    val = self.wrapped(inst)
  File "/usr/local/lib/python3.5/dist-packages/aiohttp/web_request.py", line 413, in cookies
    parsed = SimpleCookie(raw)
  File "/usr/lib/python3.5/http/cookies.py", line 507, in __init__
    self.load(input)
  File "/usr/lib/python3.5/http/cookies.py", line 556, in load
    self.__parse_string(rawdata)
  File "/usr/lib/python3.5/http/cookies.py", line 620, in __parse_string
    self.__set(key, rval, cval)
  File "/usr/lib/python3.5/http/cookies.py", line 512, in __set
    M.set(key, real_value, coded_value)
  File "/usr/lib/python3.5/http/cookies.py", line 380, in set
    raise CookieError('Illegal key %r' % (key,))
http.cookies.CookieError: Illegal key 'ISAWPLB{DB45DF86-F806-407C-932C-D52A60E4019E}'

Your environment

Tested on Ubuntu 16.04, aiohttp version 2.3.9 installed through pip.

[asvetlov]

The question is what to do with invalid cookies. There are two options:

1. Ignore invalid names
2. Return them as is

[kxepal]

3. Ignore and generate warnings that they were ignored.
4. Throw HttpBadRequest instead - that's not a server fault.

Problem with acceptance is that some user will spend quite a lot of time figuring out why his cookies get ignored.

[irmatov]

I vote for 2nd option.. :)

[irmatov]

Seriously speaking, ignoring invalid names leads to cookies just disappearing. 3 just adds warnings about it. 4 is, technically, the best scenario. Unfortunately, there are two many clients and servers that generate such cookies, and insisting on being technically correct is not a realistic choice.

[asvetlov]

They are not disappearing, you still can get invalid cookies from Cookies HTTP header hard way.

[kxepal]

Similar issue: https://github.com/playframework/playframework/issues/6140 It seems this problem happens, when aiohttp service is behind those forefront thing which adds those malformed cookies on proxy. Raising exception, even 400 wouldn't be cool here, though I think that's the right thing. Filter them out is better idea here - unlikely someone would happy if his aiohttp application will eventually broken when someone put forefront in front of it. Though, he wouldn't be happy because of it anyway (:

[wumpus]

Using aiohttp as a client in a web crawler, here are some invalid cookies I observed being sent out by top-million websites:

WARNING:aiohttp.client:Can not load response cookies: Illegal key 'ISAWPLB{381DEC7D-8336-4B7A-B144-62C8A8EBBC2A}' WARNING:aiohttp.client:Can not load response cookies: Illegal key 'fcnt(s)index(d)php' WARNING:aiohttp.client:Can not load response cookies: Illegal key 'balancer://bnn6cluster' WARNING:aiohttp.client:Can not load response cookies: Illegal key '_csrf/link' WARNING:aiohttp.client:Can not load response cookies: Illegal key '/UserPreferenceLang' WARNING:aiohttp.client:Can not load response cookies: Illegal key 'karikachi.org/' WARNING:aiohttp.client:Can not load response cookies: Illegal key 'Z6BB5yDGnXPSBQVT4Vry8y27fcg60A@@'

This is the way the web is, it would be nice if aiohttp worked with it. I recommend that (2) be possible for both client and server, but I don't strongly think it should be the default.

[rosstex]

Seconding the need for allowing the response to be returned even with invalid keys. I'm running into issues with a crawler because of this.

[gsnedders]

Note that as of https://github.com/httpwg/http-extensions/commit/5469d515d8876196900078806d07813765aa9a49, RFC6265bis defines Cookie as the following:

cookie-header = "Cookie:" SP cookie-string
cookie-string = cookie-pair *( ";" SP cookie-pair )
cookie-pair       = cookie-name BWS "=" BWS cookie-value
cookie-name       = 1*cookie-octet
cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E 
                    / %x80-FF
                      ; octets excluding CTLs,
                      ; whitespace DQUOTE, comma, semicolon,
                      ; and backslash

…where all other symbols are defined in RFC5234 (ABNF) and RFC7230 (HTTP Message Syntax and Routing)

This therefore matches Cookie: ISAWPLB{DB45DF86-F806-407C-932C-D52A60E4019E}=x from OP, though it is still necessary to handle (somehow!) cases that don't match (500 is certainly wrong, 400 might be workable under the new grammar).

[whg517]

When I used aiohttp to log in to a web page, there was a warning that cookies were illegal, which directly caused that I could not use valid cookies to access other pages during login, and I would be redirected to the login page every time. Websites are out of my control, so I think aiohttp should be tolerant of accepting illegal cookies. When I used requests to mock the request, everything was fine. I think we should make some compatibility, because most sites are not written by visitors and they have no control over this.

real cookies:

Cookie: DotNetOpenAuth.WebServerClient.XSRF-Session=r3DbFHfeMakPf7gYWbO0Tw; lhc_per={%22vid%22:%22kahg621ol5g80izbqa3j%22}; /.ASPXAUTH=55BCA3AA1E7745E1D6F7163B235EEF99036CF6D4FE92C9F8547BEB71148EF37C044A00F672DBD6FAB221A79544BC4C023114C6F6D4AB61F51AA27A9E212431E7E699BB04DF6AAEB7691C26ADB6FC3A7A106A8CDF9B9B9768272EBC31A99C8756D1250AE3C013D22B0782621B5A382E61550E44CDADDF5BFA1D0FF4701793BD4EF80E34950EB5D24D9C241B0EC2E0598C; ASP.NET_SessionId=cqfo4dcuk43mnhnwgczuk1ay

log:

WARNING:aiohttp.client:Can not load response cookies: Illegal key '/.ASPXAUTH'

[whg517]

Now I use a patch monkeypatch ref CookieError: Illegal Key, and it's work. There are drawbacks, but I have to fix this first, and I hope the maintainers will pay attention to it.

eg:

import sys
if "http" in sys.modules:
    raise ImportError("Crawler must be imported before http module")
import http.cookies
http.cookies._is_legal_key = lambda _: True

[wumpus]

Thank you for this monkey patch!

[belegnar]

Also, this issue is connected with invalid cookie value, e.g. json object - in this case SimpleCookie just ignores all next cookies (and the malformed one). FMPV, it'd nice to have possibility to replace SimpleCookie with other parser before runtime

http.cookies.SimpleCookie('json={"key": "value"};valid-key=valid-value;')
<SimpleCookie: >

http.cookies.SimpleCookie('valid-key=valid-value;')
<SimpleCookie: valid-key='valid-value'>

In 3.8.6 the only possible way to replace cookie parser is to intrude into Request._cache what is not really convinient

[bdraco]

I ended up writing a cookie parser in https://github.com/aio-libs/aiohttp/pull/11106#issuecomment-2927246152 but I'm too afraid to move it forward as I'm not sure what will break as currently we need bug for bug compat with SimpleCookie except for some cases