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.

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

Conversation

Projects
None yet
3 participants
@bertjwregeer
Copy link
Member

bertjwregeer commented Dec 10, 2013

When we are retrieving the value, and the SignedSerializer fails because
of invalid signature for example we want to simply return None, as if
there was no cookie set in the first place.

Bert JW Regeer
Squash ValueError from SignedSerializer in CookieProfile
When we are retrieving the value, and the SignedSerializer fails because
of invalid signature for example we want to simply return None, as if
there was no cookie set in the first place.
return self.serializer.loads(bytes_(cookie))
try:
return self.serializer.loads(bytes_(cookie))
except ValueError as e:

This comment has been minimized.

@mmerickel

mmerickel Dec 11, 2013

Member

don't name the exception variable unless you're gonna use it

This comment has been minimized.

@bertjwregeer
@mcdonc

This comment has been minimized.

Copy link
Member

mcdonc commented Dec 11, 2013

Cool, can you add a note to docs/news.txt?

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Dec 11, 2013

Done :-)

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Dec 12, 2013

This is a show-stopper bug for Pyramid 1.5 because the new session class and authtkt use CookieProfile.

Right now without this change CookieProfile will raise an exception if the serializer failed to deserialize the data (such as missing signature, invalid signature or bad data), this is not caught anywhere in Pyramid (nor should it) when get_value() is used. This means that if an attacker changes the cookie value they can cause an exception to occur that the user may not be expecting.

This would require implementors to either try/except get_value() or to set up a ValueError exception handler so that their web app doesn't return a 500 error to the user.

This patch simply squashes all of the possible exceptions, this fixes the issue and is a much saner solution.

return self.serializer.loads(bytes_(cookie))
try:
return self.serializer.loads(bytes_(cookie))
except:

This comment has been minimized.

@mcdonc

mcdonc Dec 12, 2013

Member

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.

This comment has been minimized.

@mcdonc

mcdonc Dec 12, 2013

Member

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.

This comment has been minimized.

@mmerickel

mmerickel Dec 12, 2013

Member

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

This comment has been minimized.

@bertjwregeer

bertjwregeer Dec 13, 2013

Member

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

This comment has been minimized.

@mmerickel

mmerickel Dec 13, 2013

Member

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

This comment has been minimized.

@bertjwregeer

bertjwregeer Dec 13, 2013

Member

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.

@mcdonc mcdonc merged commit 7d0931f into Pylons:master Dec 13, 2013

@mcdonc

This comment has been minimized.

Copy link
Member

mcdonc commented Dec 13, 2013

Merged with a few modifications (only catch valueerror instead of all errors, add test, etc).

@bertjwregeer bertjwregeer deleted the bertjwregeer:bugfix.squash-valueerror branch Dec 13, 2013

mamash pushed a commit to joyent/pkgsrc-wip that referenced this pull request Aug 20, 2014

thomasklausner
Update to 1.4:
1.4 (2013-05-14)
----------------

Features
~~~~~~~~

- Remove ``webob.__version__``, the version number had not been kept in sync
  with the official pkg version.  To obtain the WebOb version number, use
  ``pkg_resources.get_distribution('webob').version`` instead.

Bug Fixes
~~~~~~~~~

- Fix a bug in ``EmptyResponse`` that prevents it from setting self.close as
  appropriate due to testing truthiness of object rather than if it is
  something other than ``None``.

- Fix a bug in ``SignedSerializer`` preventing secrets from containing
  higher-order characters. See Pylons/webob#136

- Use the ``hmac.compare_digest`` method when available for constant-time
  comparisons.

1.3.1 (2013-12-13)
------------------

Bug Fixes
~~~~~~~~~

- Fix a bug in ``SignedCookieProfile`` whereby we didn't keep the original
  serializer around, this would cause us to have ``SignedSerializer`` be added on
  top of a ``SignedSerializer`` which would cause it to be run twice when
  attempting to verify a cookie.  See Pylons/webob#127

Backwards Incompatibilities
~~~~~~~~~~~~~~~~~~~~~~~~~~~

- When ``CookieProfile.get_value`` and ``SignedCookieProfile.get_value`` fails
  to deserialize a badly encoded value, we now return ``None`` as if the cookie
  was never set in the first place instead of allowing a ``ValueError`` to be
  raised to the calling code.  See Pylons/webob#126

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

Features
~~~~~~~~

- Added a read-only ``domain`` property to ``BaseRequest``.  This property
  returns the domain portion of the host value.  For example, if the
  environment contains an ``HTTP_HOST`` value of ``foo.example.com:8000``,
  ``request.domain`` will return ``foo.example.com``.

- Added five new APIs: ``webob.cookies.CookieProfile``,
  ``webob.cookies.SignedCookieProfile``, ``webob.cookies.JSONSerializer`` and
  ``webob.cookies.SignedSerializer``, and ``webob.cookies.make_cookie``.  These
  APIs are convenience APIs for generating and parsing cookie headers as well
  as dealing with signing cookies.

- Cookies generated via webob.cookies quoted characters in cookie values that
  did not need to be quoted per RFC 6265.  The following characters are no
  longer quoted in cookie values: ``~/=<>()[]{}?@`` .  The full set of
  non-letter-or-digit unquoted cookie value characters is now
  ``!#$%&'*+-.^_`|~/: =<>()[]{}?@``.  See
  http://tools.ietf.org/html/rfc6265#section-4.1.1 for more information.

- Cookie names are now restricted to the set of characters expected by RFC
  6265.  Previously they could contain unsupported characters such as ``/``.

- Older versions of Webob escaped the doublequote to ``\"`` and the backslash
  to ``\\`` when quoting cookie values.  Now, instead, cookie serialization
  generates ``\042`` for the doublequote and ``\134`` for the backslash. This
  is what is expected as per RFC 6265.  Note that old cookie values that do
  have the older style quoting in them will still be unquoted correctly,
  however.

- Added support for draft status code 451 ("Unavailable for Legal Reasons").
  See http://tools.ietf.org/html/draft-tbray-http-legally-restricted-status-00

- Added status codes 428, 429, 431 and 511 to ``util.status_reasons`` (they
  were already present in a previous release as ``webob.exc`` exceptions).

Bug Fixes
~~~~~~~~~

- MIMEAccept happily parsed malformed wildcard strings like "image/pn*" at
  parse time, but then threw an AssertionError during matching.  See
  Pylons/webob#83 .

- Preserve document ordering of GET and POST request data when POST data passed
  to Request.blank is a MultiDict.  See Pylons/webob#96

- Allow query strings attached to PATCH requests to populate request.params.
  See Pylons/webob#106

- Added Python 3.3 trove classifier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment