cookies with square brackets in value

[WaldemarParzonka]

BPO	22931
Nosy	@gvanrossum, @birkenfeld, @larryhastings, @benjaminp, @bitdancer, @berkerpeksag, @demianbrecht, @timgraham
Files	issue22931.patch issue22931_1.patch issue22931_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-05-23.15:48:16.649>
created_at = <Date 2014-11-24.15:43:24.320>
labels = ['type-bug', 'library', 'release-blocker']
title = 'cookies with square brackets in value'
updated_at = <Date 2015-06-03.20:48:40.359>
user = 'https://bugs.python.org/WaldemarParzonka'

bugs.python.org fields:

activity = <Date 2015-06-03.20:48:40.359>
actor = 'Tim Pierce'
assignee = 'none'
closed = True
closed_date = <Date 2015-05-23.15:48:16.649>
closer = 'python-dev'
components = ['Library (Lib)']
creation = <Date 2014-11-24.15:43:24.320>
creator = 'Waldemar.Parzonka'
dependencies = []
files = ['37340', '38525', '38773']
hgrepos = []
issue_num = 22931
keywords = ['patch']
message_count = 17.0
messages = ['231605', '231847', '231970', '231972', '231973', '231982', '232021', '234913', '238205', '238233', '238297', '239271', '239302', '241948', '243136', '243604', '243924']
nosy_count = 14.0
nosy_names = ['gvanrossum', 'georg.brandl', 'larry', 'benjamin.peterson', 'r.david.murray', 'python-dev', 'berker.peksag', 'demian.brecht', 'Tim.Graham', 'Waldemar.Parzonka', 'dlamotte', 'Mark Hughes', 'twpierce', 'Tim Pierce']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22931'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4', 'Python 3.5']

[WaldemarParzonka]

There seems to be weird behaviour in BaseCookie.load() when cookie that has '[' in one of the values is being loaded.

There is no exception being thrown as the key is still legal but the cookie is not getting loaded properly and everything that was after the '[' valued cookie is being silently ignored.

>>> dd = SimpleCookie()
>>> dd
<SimpleCookie: >
>>> s = 'a=b; c=[; d=r; f=h'
>>> dd.load(s)
>>> dd
<SimpleCookie: a='b'>
>>>

[demianbrecht]

There could be some history behind this that I'm unaware of that I'm not familiar with.

From what I can tell, this issue is simply due to the "[" character not being in _LegalCharsPatt (http/cookies.py). _LegalCharsPatt actually seems quite a bit more restrictive than it really should be. It's set to r"[\w\d!#%&'~_`><@,:/\$\*\+\-\.\^\|\)\(\?\}\{\=]", where RFC 6265 specifies:

cookie-pair = cookie-name "=" cookie-value
cookie-name = token
cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
; US-ASCII characters excluding CTLs,
; whitespace DQUOTE, comma, semicolon,
; and backslash
token = <token, defined in [RFC2616], Section 2.2>

_LegalCharsPatt is used for regex matching on the cookie value, not the key (there is a distinction made between the two).

The omission of those characters is correct for the cookie keys, but not the values (RFC 2965 is a little less verbose, but nothing ruling out those characters for values).

[demianbrecht]

Err, sorry, I entirely misunderstood the problem. The invalid characters are correct ([ = 5B, which indeed is illegal, I wasn't paying close enough attention to the hex values in the ABNF). It's the fact that the valid key/value pairs after the invalid one are ignored. I'll dig into the RFC and see if there's an expected behavior here and whether or not it's currently handled as expected.

[demianbrecht]

Now I've confused myself and my first impression was correct. For some reason, my brain was thinking "%x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E" was the exclusion list for some reason (which is obviously horribly wrong).

So my first observation was correct in that they should simply be added to the valid character list and I'll get a patch together for that.

[demianbrecht]

Attached patch to fix the issue as reported.

Something interesting that came out of this though is that due to the regex expression, if there's an invalid character in one of the cookie-octets, the rest of the cookie is ignored. I would assume that it should either a) ignore the entire cookie string or b) ignore the invalid cookie pair and accept valid pairs following. I've been unable to find that defined in any of the RFCs though.

[WaldemarParzonka]

Thanks for taking a look into that.

And yes the behaviour when invalid value is encountered is bit weird as the rest of the cookie is being silently ignored which is probably less than ideal in most cases.

Just wonder if there is any easy way of making the matching more aware as browsers may allow various things as cookie values I guess.

[demianbrecht]

I do think it should be a little more permissive when parsing cookies. I've created bpo-22983 to address that as to not conflate this issue, which the attached patch does address.

[demianbrecht]

Ping for review/commit.

[MarkHughes]

This is also an issue with Python 2.7.9 but not 2.7.8. There were various cookie related fixes in 2.7.9 which could have revealed this issue. Maybe this one?

https://hg.python.org/cpython/rev/9e765e65e5cb

[MarkHughes]

We experimented with a version of the patch for 2.7.9.

One issue we immediately noticed is that even though disallowed by the spec the use of commas in cookie values is widespread so we needed to add \, to the _LEGAL_VALUES_PATT.

[demianbrecht]

Thanks for the report Mark, updating this patch to be more backwards compatible was on my to-do list. I've attached a new patch that simply adds the new characters to the legal value set.

It does look like that's the commit that introduced this issue, but the change was made for good reason.

[timgraham]

Will this regression be fixed in Python 2.7, 3.2, and 3.3? If not, Django may need to vendor Python's cookie class to workaround this bug to prevent users from losing sessions and/or being unable to login to Django powered sites as reported in https://code.djangoproject.com/ticket/24492.

[demianbrecht]

As I understand it, the change should also be applied to security releases
as the regression manifested by a security related patch being applied.
That said, there may be some debate as there apparently isn't much (if
anything) in the way of precedence here.