cookies module allows commas in keys

[jaraco]

BPO	26302
Nosy	@jaraco, @vadmium, @serhiy-storchaka, @demianbrecht, @AnishShah
Files	issue26302_20160206.patch issue26302_20160206-2.patch issue26302_20160207.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 2016-02-24.13:54:01.325>
created_at = <Date 2016-02-06.05:18:50.253>
labels = ['easy', 'type-bug', 'library']
title = 'cookies module allows commas in keys'
updated_at = <Date 2016-02-24.13:54:01.323>
user = 'https://github.com/jaraco'

bugs.python.org fields:

activity = <Date 2016-02-24.13:54:01.323>
actor = 'jaraco'
assignee = 'none'
closed = True
closed_date = <Date 2016-02-24.13:54:01.325>
closer = 'jaraco'
components = ['Library (Lib)']
creation = <Date 2016-02-06.05:18:50.253>
creator = 'jaraco'
dependencies = []
files = ['41836', '41837', '41841']
hgrepos = []
issue_num = 26302
keywords = ['patch', 'easy', '3.5regression']
message_count = 20.0
messages = ['259718', '259721', '259726', '259742', '259744', '259745', '259746', '259750', '259762', '259765', '259778', '259780', '259781', '259782', '259817', '260399', '260443', '260766', '260801', '260802']
nosy_count = 7.0
nosy_names = ['jaraco', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'demian.brecht', 'anish.shah', 'Kunal Grover']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26302'
versions = ['Python 3.5', 'Python 3.6']

[jaraco]

Commas aren't legal characters in cookie keys, yet in Python 3.5, they're allowed:

>>> bool(http.cookies._is_legal_key(','))
True

The issue lies in the use of _LegalChars constructing a regular expression.

"Some people, when confronted with a problem, think 'I know, I'll use regular expressions.' Now they have two problems."

The issue arises in this line:

_is_legal_key = re.compile('[%s]+' % _LegalChars).fullmatch

Which was added in 88e1151e8e0242 referencing bpo-2211.

The problem is that in a regular expression, and in a character class in particular, the '-' character has a special meaning if not the first character in the class, which is "span all characters between the leading and following characters". As a result, the pattern has the unintended effect of including the comma in the pattern:

>>> http.cookies._is_legal_key.__self__
re.compile("[abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!#$%&'*+-.^_`|~:]+")
>>> pattern = _
>>> pattern.fullmatch(',')
<_sre.SRE_Match object; span=(0, 1), match=','>
>>> ord('+')
43
>>> ord('.')
46
>>> ''.join(map(chr, range(43,47)))
'+,-.'

That's how the comma creeped in.

This issue is the underlying cause of https://bitbucket.org/cherrypy/cherrypy/issues/1405/testcookies-fails-on-python-35 and possible other cookie-related bugs in Python.

While I jest about regular expressions, I like the implementation. It just needs to account for the extraneous comma, perhaps by escaping the dash:

_is_legal_key = re.compile('[%s]+' % _LegalChars.replace('-', '\\-')).fullmatch

Also, regression tests for keys containing invalid characters should be added as well.

[serhiy-storchaka]

Ah, how can I missed this catch?

The simplest way is just move "-" to the start or the end of character list. The most error-proof way is to use re.escape().

[KunalGrover]

Hi, I am a newcomer here, I am interested to work on this issue.

[AnishShah]

We just need to use '\-' instead of '-'.

>>> regex = re.compile("[a-z]")
>>> bool(regex.match('b'))
True
>>> regex = re.compile("[a\-z]")
>>> bool(regex.match('b'))
False

I have uploaded a patch.
Let me know if this needs some tests too?

[jaraco]

A test would be much appreciated. It's worthwhile capturing the rejection of at least one invalid character, if not several common ones.

[serhiy-storchaka]

No, _LegalChars shouldn't include "\".

[AnishShah]

@serhiy.storchaka OK, I have used re.escape instead of '\'. And I have added a test too.

Also, may I know why '\' can not be in _LegalChars, so that I can remember this for future purpose?

[serhiy-storchaka]

_LegalChars contained only characters which don't require quoting, as documented in the comment above. If _LegalChars was only used to create _is_legal_key, we would just wrote the regular expression. But it is used also in other places. In this particular case adding "\" to _LegalChars doesn't lead to visible bug (except inconsistency with the comment), but we can't be sure.

_is_legal_key() is implementation detail. It would be better to test public API.

[vadmium]

Another option might be to do away with the regular expression (personally I like to avoid REs and code generation where practical):

def _is_legal_key(key):
    return key and set(_LegalChars).issuperset(key)

[serhiy-storchaka]

The regular expression is used for performance.

[AnishShah]

I ran regex and issuperset version on a random string. The regex one gives better performance. So, I have included the re.escape in the patch.

>>> random_str = ''.join(random.choice(_LegalChars) for _ in range(10 ** 8))
>>> is_legal_key = re.compile('[%s]+' % re.escape(_LegalChars)).fullmatch
>>> Timer("is_legal_key(random_str)", setup="from __main__ import random_str, is_legal_key").timeit(1)
0.3168252399998437
>>> def is_legal_key(key):
...     return key and set(_LegalChars).issuperset(key)
... 
>>> Timer("is_legal_key(random_str)", setup="from __main__ import random_str, is_legal_key").timeit(1)
4.3335622880001665

Also, I have updated the patch. Can you please review it? :)

[vadmium]

The same bug is in the _CookiePattern regular expression. For illegal characters other than a comma, the CookieError does not actually seem to be raised.