Fix documentation/cookie handling in response.set_cookie() #172

Closed
wants to merge 27 commits into
from

Projects

None yet

3 participants

@bertjwregeer
Member

This updates response.set_cookie() with proper documentation, and refactors it so that it uses webob.cookies.make_cookie() instead of it's own implementation, code-reuse for the win!

However, this does break backwards compatibility because we no longer allow unicode cookie values, these were technically contra-spec in the first place, and may not have been accepted by all browsers.

We also change from key to name because cookies have names.

This fixes #166 and #171.

@bertjwregeer
Member

This additionally fixes: #129

@mmerickel
Member

I'm definitely +1 on utilizing make_cookie here. However is make_cookie actually correct? In #171 you mention that it encodes things to latin1 but the code shows that it encodes things to ascii (which would fail on higher-order bytes).

@bertjwregeer
Member

@mmerickel Seems I made a mistake when looking at the code, ASCII is correct, high-order bytes are NOT allowed in cookies. So make_cookie() and by extension Morsel() is correct.

Technically we should restrict the allowed characters even further to the allowed set as set in RFC6265:

0x21: !
0x23-2B: #$%&'()*+
0x2D-3A: -./0123456789: 
0x3C-5B: <=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[
0x5D-7E: ]^_`abcdefghijklmnopqrstuvwxyz{|}~

So that WebOb does not allow the creation of cookies that fall outside of that spec.

@mmerickel mmerickel and 1 other commented on an outdated diff Nov 13, 2014
docs/news.txt
@@ -4,12 +4,33 @@ News
Unreleased
----------
+Backwards Incompatibilities
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- ``Morsel`` will no longer accept a cookie value that does not meet RFC6265's
+ cookie-octet specification. Upon calling ``Morsel.serialize`` a warning will
+ be issued, in the future this raise a ``ValueError``, please update your
@mmerickel
mmerickel Nov 13, 2014 Member

"this raise" grammar

@bertjwregeer
bertjwregeer Nov 13, 2014 Member

Fixed. Squashed.

@mmerickel mmerickel commented on an outdated diff Nov 13, 2014
docs/news.txt
@@ -4,12 +4,33 @@ News
Unreleased
----------
+Backwards Incompatibilities
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- ``Morsel`` will no longer accept a cookie value that does not meet RFC6265's
+ cookie-octet specification. Upon calling ``Morsel.serialize`` a warning will
+ be issued, in the future this raise a ``ValueError``, please update your
+ cookie handling code.
+
+- ``response.set_cookie`` now uses the ``cookies.make_cookie`` API, which will
@mmerickel
mmerickel Nov 13, 2014 Member

webob.cookies.make_cookie or possibly "the internal make_cookie api"

@mmerickel mmerickel commented on an outdated diff Nov 13, 2014
docs/news.txt
@@ -4,12 +4,33 @@ News
Unreleased
----------
+Backwards Incompatibilities
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- ``Morsel`` will no longer accept a cookie value that does not meet RFC6265's
+ cookie-octet specification. Upon calling ``Morsel.serialize`` a warning will
+ be issued, in the future this raise a ``ValueError``, please update your
+ cookie handling code.
+
+- ``response.set_cookie`` now uses the ``cookies.make_cookie`` API, which will
+ issue warnings if cookies are set with invalid bytes.
@mmerickel
mmerickel Nov 13, 2014 Member

reference this PR

@mmerickel mmerickel and 1 other commented on an outdated diff Nov 13, 2014
tests/test_cookies.py
import unittest
from webob.compat import native_
from webob.compat import PY3
+cookies._should_raise = True
@mmerickel
mmerickel Nov 13, 2014 Member

you don't want to set this at module scope do you?

@bertjwregeer
bertjwregeer Nov 13, 2014 Member

There's only one test that doesn't require that _value_quote() raise an error instead of warning. So in test_cookies_bw.py I explicitly disable/enable that flag.

I don't know if this is the best way to do it, or if there is a better way to turn that flag on/off.

@mmerickel
Member

+1, LGTM

This is a nice cleanup of the cookie apis.

@mcdonc mcdonc commented on the diff Nov 14, 2014
docs/news.txt
@@ -4,12 +4,34 @@ News
Unreleased
----------
+Backwards Incompatibilities
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+- ``Morsel`` will no longer accept a cookie value that does not meet RFC6265's
+ cookie-octet specification. Upon calling ``Morsel.serialize`` a warning will
+ be issued, in the future this will raise a ``ValueError``, please update your
+ cookie handling code. See https://github.com/Pylons/webob/pull/172
+
@mcdonc
mcdonc Nov 14, 2014 Member

Can you say a few words about what RFC6265 allows here so folks don't need to look up the spec, and say under what circumstances a warning will be issued?

@mcdonc
mcdonc Nov 14, 2014 Member

We should also push out this code in the form of an alpha release, if only to let people try it and tell us "hey I was successfully using WebOb to store cookies with high-order names/values and you broke me!"

@bertjwregeer
bertjwregeer Nov 14, 2014 Member

Currently the API will just issue a RuntimeWarning, which hopefully means we don't break anyone, but give everyone time to update their code.

I will update this file with your suggestions for what RFC6265 allows, and I am all for doing an alpha release/beta release to make sure I didn't break anything... at least too terribly ;-)

@mcdonc
Member
mcdonc commented Nov 14, 2014

This all looks reasonable to me; did confirm that the interpretation of RFC6265 that Bert had is more or less correct. As I mentioned above we should push this out in some sort of "major alpha" to see if it hurts anyone.

@bertjwregeer
Member

@mcdonc

Information has been added about the cookie-octet allowed characters in docs/news.txt.

@mmerickel @mcdonc:

Thank you both for reviewing this :-)

@bertjwregeer bertjwregeer added this to the Version 1.5 milestone Mar 23, 2015
bertjwregeer added some commits Nov 9, 2014
@bertjwregeer bertjwregeer Documentation states expires can be timedelta
The documentation for expires incorrectly states that expires can be a
timedelta, let's add a test for that.
0c2890c
@bertjwregeer bertjwregeer Update documentation for set_cookie expires
The documentation erroneously states that that expires can be a
datetime.timedelta, this is incorrect, but since it has been documented
as such, we should continue accepting it.

Add the fact that it can be a datetime.datetime, and probably should be.
6f37ba9
@bertjwregeer bertjwregeer Cookies have names, not keys... 7cc8094
@bertjwregeer bertjwregeer Refactor the respnse.set_cookie
Start using the make_cookie call that is available in webob.cookies,
this way we don't duplicate the same behavior.

There are a couple of backwards compatible fixes:

 - If expires is set to a timedelta, and max_age is not set, we set
   max_age to expires a timedelta

 - expires can also be a datetime, however this was not documented. So
   if it is a datetime, we want to get a timedelta, by taking the
   existing expires value, and removing datetime.utcnow() from the
   value.
8e1db14
@bertjwregeer bertjwregeer Update the documentation for response.set_cookie max_age
We need to document that max_age can be an integer, timedelta or None,
and what that means.

max_age takes precedence over an expires argument.
93ca34a
@bertjwregeer bertjwregeer Change test_cookies() to not include unicode
Unicode values in cookies are invalid.
2bf7746
@bertjwregeer bertjwregeer Verify unicode values raise an exception b1486dc
@bertjwregeer bertjwregeer Update news.txt with latest fixes/documentation e14a4df
@bertjwregeer bertjwregeer Update comments that had false information f3ba7a8
@bertjwregeer bertjwregeer Add new base64 serializer f8379ef
@bertjwregeer bertjwregeer Set new Base64Serializer as default on CookieProfile
The previous standard was the JSON serializer, however with upcoming
changes that limit what characters are allowed in cookies, bare JSON no
longer worked because it could return values that are invalid to be
stored in cookies.
e5726bf
@bertjwregeer bertjwregeer Raise ValueError if cookie value has invalid chars
WebOb will now raise a ValueError if an attempt is made to set the
cookie to an invalid value. According to RFC6265 a cookie-octet has a
specific subset of allowed ASCII characters, and that subset does not
change whether the value is DQUOT'ed or not.

WebOb will still accept all other cookies, it just won't be able to
create them.
95890ab
@bertjwregeer bertjwregeer Document cookie name requirements explicitly 3bf0ca5
@bertjwregeer bertjwregeer Import assert_raises 6d61105
@bertjwregeer bertjwregeer Remove utf-8 strings from testing e5bd763
@bertjwregeer bertjwregeer Update test strings to match what is now returned e25cd58
@bertjwregeer bertjwregeer Verify decode of old cookies, and failure of encoding same cookies f3c1175
@bertjwregeer bertjwregeer Remove useless test a671bef
@bertjwregeer bertjwregeer Add new test for CookieProfile
We want to make sure we have at least one test for CookieProfile that
fetches a cookie from our request object, and then verifies it returns
the appropriate answer.
da7e8a4
@bertjwregeer bertjwregeer Until such a time that we can deprecate this, warn
Someday we can get rid of this mess and just be strict about what we
send, but until such a day we want to warn people that it is going to
happen in the future.
6804b13
@bertjwregeer bertjwregeer Don't calculate the same result twice 664041d
@bertjwregeer bertjwregeer Update news about Morsel/set_cookie changes 966dd64
@bertjwregeer bertjwregeer Update test to use eq_ instead of assert 933b6c5
@bertjwregeer bertjwregeer Use RuntimeWarning instead of DeprecationWarning
This makes the warning more noticeable to people and will be less likely
to be ignored.
0e56c53
@bertjwregeer bertjwregeer Re-instate utf-8 cookie values (b/w) 62cf2d9
@bertjwregeer bertjwregeer Add setup/teardown module functions
Sets up the cookies._should_raise using a module setup/teardown
functions instead of setting it at module scope per suggestions from
Michael Merickel.
5d0e7b9
@bertjwregeer bertjwregeer Add better/more information about RFC6265 cookie-octet
We want to provide people an easy to find resource on what is now going
to be valid in a WebOb cookie.
cebf4b8
@bertjwregeer bertjwregeer referenced this pull request Mar 23, 2015
Merged

Fix.cookie handling #188

@bertjwregeer
Member

Superseded by #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment