-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
CookieError on invalid cookie names #2683
Comments
The question is what to do with invalid cookies.
|
Problem with acceptance is that some user will spend quite a lot of time figuring out why his cookies get ignored. |
I vote for 2nd option.. :) |
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. |
They are not disappearing, you still can get invalid cookies from |
Similar issue: playframework/playframework#6140 |
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}' 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. |
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. |
Note that as of httpwg/http-extensions@5469d51, RFC6265bis defines Cookie as the following:
…where all other symbols are defined in RFC5234 (ABNF) and RFC7230 (HTTP Message Syntax and Routing) This therefore matches |
When I used real cookies:
log:
|
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 |
Thank you for this monkey patch! |
Also, this issue is connected with invalid cookie value, e.g. json object - in this case http.cookies.SimpleCookie('json={"key": "value"};valid-key=valid-value;')
<SimpleCookie: >
http.cookies.SimpleCookie('valid-key=valid-value;')
<SimpleCookie: valid-key='valid-value'> In |
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
:RFC2616 defines
token
as: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:
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:Your environment
Tested on Ubuntu 16.04, aiohttp version 2.3.9 installed through pip.
The text was updated successfully, but these errors were encountered: