Skip to content
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.

Sign up for GitHub

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

unquote double-quotes cookie values #1440

Merged
merged 3 commits into from
Jul 15, 2013
Merged

unquote double-quotes cookie values #1440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cdab4fa
unquote double-quotes cookie values
fcurella Jul 1, 2013
a8cf5b8
keep the double quotes, but don't escape them
fcurella Jul 1, 2013
7da16c5
Added myself to AUTHORS
fcurella Jul 1, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,4 @@ Patches and Suggestions
- Dave Shawley <[email protected]>
- James Clarke (jam)
- Kevin Burke <[email protected]>
- Flavio Curella
5 changes: 5 additions & 0 deletions requests/cookies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,11 @@ def __delitem__(self, name):
"""Deletes a cookie given a name. Wraps cookielib.CookieJar's remove_cookie_by_name()."""
remove_cookie_by_name(self, name)

def set_cookie(self, cookie, *args, **kwargs):
if cookie.value.startswith('"') and cookie.value.endswith('"'):
cookie.value = cookie.value.replace('\\"', '')
return super(RequestsCookieJar, self).set_cookie(cookie, *args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this code doing now? It looks like you're going through cookies and removing all instances of \"...is that the right behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa going by what you quoted, I suspect not. Going one step further, the *cookie-octent you mention looks to be a URL-encoded string that doesn't allow ", controls, whitespace, ,, ;, and \. The real problem then becomes, how do we add back the "s (when necessary)?

Also @fcurella do you have an example of where the current implementation breaks (or rather a server breaks due to it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa currently, the cookie.value will be '"\\"bar:baz\\""' (to clarify: double-quote, slash, double-quote, cookie_value, slash, double-quote, double-quote).

If we want to keep the quotes, we can't use .strip(). Maybe we can use a regex like re.sub(r'"\\"(.*)\\""', '"$1"', cookie.value), to make sure we remove only the two \" at the ends of the string.

@sigmavirus24 the problem came up on a django project i'm working on, where one service would use request.set_cookie(key, "param1:param2"), and we're using requests on a different service and we need to forward the cookie to the browser.

I've already verified that keeping the quotes (but not escaping them) produces the correct cookie.

Moreover, when removing the quotes altogether (my previous solution), django will recognize the : and will add the quotes back again. I've done a quick test with Flask, and in this last scenario (unquoted cookie containing :) it doesn't quote the cookie value automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, that makes sense to me. It would be awesome if you could add a test just to verify that this is actuall what happens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fcurella you mean request.set_cookie(key, '"param1:param2"') right? If so that makes sense. But yes, adding a test and ensuring this doesn't break anything else would be awesome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmavirus24 no. if you call response.set_cookie(key, 'param1:param2') django will add the double-quotes implicitly because of the : (and other special characters, see https://github.com/django/django/blob/master/django/http/cookie.py#L23)

Looking at the django code and its comments, this seems to be the desired behaviour and not due to a bug in django.

Re tests: I'm already adding a new test for the parsing. Do you want me to include additional weirder cookie values?

I'd also like your and @Lukasa's opinion on .replace() vs re.sub().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also like your and @Lukasa's opinion on replace vs regex.

Mine has always been straight-forward, never use regex. Replace works well and
it it is all we need currently. re is not the best implementation of regular
expressions in python and the others are not in the standard library. Beyond
that, string methods are fast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmavirus24 +1. Regexes are only worth using if you're actually defining a complicated grammar. If you don't use a single regex special character, you didn't need a regex. =)


def update(self, other):
"""Updates this jar with cookies from another CookieJar or dict-like"""
if isinstance(other, cookielib.CookieJar):
Expand Down
5 changes: 5 additions & 0 deletions test_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ def test_cookie_removed_on_expire(self):
)
assert 'foo' not in s.cookies

def test_cookie_quote_wrapped(self):
s = requests.session()
s.get(httpbin('cookies/set?foo="bar:baz"'))
self.assertTrue(s.cookies['foo'] == '"bar:baz"')

def test_request_cookie_overrides_session_cookie(self):
s = requests.session()
s.cookies['foo'] = 'bar'
Expand Down