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

Squash ValueError from SignedSerializer in CookieProfile #126

Merged
merged 4 commits into from
Dec 13, 2013
Merged

Squash ValueError from SignedSerializer in CookieProfile #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
726b5af
Squash ValueError from SignedSerializer in CookieProfile
Dec 10, 2013
2867ff8
Don't name the exception
Dec 11, 2013
f1ea046
Add notes regarding squashing
Dec 11, 2013
7d0931f
Squash any exceptions raised in the deserializer
Dec 12, 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
9 changes: 9 additions & 0 deletions docs/news.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
News
====

Unreleased
----------

Bug Fixes
~~~~~~~~~

- Squash a ValueError when SignedSerializer fails. We simply return None as if
the cookie was never set in the first place.

1.3 (2013-12-10)
----------------

Expand Down
10 changes: 7 additions & 3 deletions tests/test_cookies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ def test_make_cookie_path(self):

self.assertTrue('test_cookie=value' in cookie)
self.assertTrue('Path=/foo/bar/baz' in cookie)

class CommonCookieProfile(unittest.TestCase):
def makeDummyRequest(self, **kw):
class Dummy(object):
Expand Down Expand Up @@ -510,7 +510,9 @@ def test_with_bad_cookie_invalid_base64(self):
"SHrRkd3lyE8c4w5ruxAKOyj2h5oF69Ix7ERZv_")
cookie = self.makeOne(request=request)

self.assertRaises(ValueError, cookie.get_value)
val = cookie.get_value()

self.assertEqual(val, None)

def test_with_bad_cookie_invalid_signature(self):
request = self.makeOneRequest()
Expand All @@ -519,7 +521,9 @@ def test_with_bad_cookie_invalid_signature(self):
"+CNeVKpWksQa0ktMhuQDdjzmDwgzbptg==")
cookie = self.makeOne(secret='sekrit!', request=request)

self.assertRaises(ValueError, cookie.get_value)
val = cookie.get_value()

self.assertEqual(val, None)

def test_with_domain(self):
cookie = self.makeOne(domains=("testing.example.net",))
Expand Down
5 changes: 4 additions & 1 deletion webob/cookies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,10 @@ def get_value(self):
cookie = self.request.cookies.get(self.cookie_name)

if cookie:
return self.serializer.loads(bytes_(cookie))
try:
return self.serializer.loads(bytes_(cookie))
except:
Copy link
Member

Choose a reason for hiding this comment

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

I think the serializer should be responsible for handling its own error conditions, and the bare except should be limited to catching the exceptions that might happen trying to turn the cookie value into bytes.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I should amend that. This should be except (ValueError, <other_exceptions_that_might_be_raised_by_bytes_(cookie)). Rationale: it is part of the serializer API that serializers must raise ValueError on malformed inputs. They should never raise a different error. If they do. it's a bug in the serializer. Of course we should probably check that existing serializers don't raise anything but ValueError roo.

Copy link
Member

Choose a reason for hiding this comment

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

This should not be a bare except. It is documented that a serializer must raise ValueError on malformed inputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this because you (@mmerickel) mentioned adding another exception that would return more information to the user.

Copy link
Member

Choose a reason for hiding this comment

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

In the SignedSerializer, not the CookieProfile. And again anything the SignedSerializer raises should be a subclass of ValueError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I understand SignedSerializer would possibly return other exceptions. Did not realise that it should be a subclass of ValueError.

I wasn't suggesting that the CookieProfile return another exception.

Sorry. Talked to @mcdonc he said he would look into it, along with the other issue I just ran into and fixed.

return None

def set_cookies(self, response, value, domains=_default, max_age=_default,
path=_default, secure=_default, httponly=_default):
Expand Down