Opened 3 years ago

Closed 3 years ago

#5175 defect closed fixed (fixed)

CookieAgent fails when it encounters "secure" cookies

Reported by: exarkun Owned by: therve
Priority: high Milestone: Twisted-11.1
Component: web Keywords: httpclient
Cc: jonathanj, therve, jknight, jesstess Branch: branches/secure-cookie-5175
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

Cookies can be marked as "secure" which basically means they should only be transmitted over an HTTPS connection. CookieAgent passes a fake for urllib2.Request to the cookielib.CookieJar it uses, and this fails when the jar has a secure cookie and wants to know if the transport is going to be secure:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/trunk/twisted/web/client.py", line 1046, in request
    self.cookieJar.add_cookie_header(lastRequest)
  File "/usr/lib/python2.6/cookielib.py", line 1323, in add_cookie_header
    cookies = self._cookies_for_request(request)
  File "/usr/lib/python2.6/cookielib.py", line 1249, in _cookies_for_request
    cookies.extend(self._cookies_for_domain(domain, request))
  File "/usr/lib/python2.6/cookielib.py", line 1238, in _cookies_for_domain
    if not self._policy.return_ok(cookie, request):
  File "/usr/lib/python2.6/cookielib.py", line 1070, in return_ok
    if not fn(cookie, request):
  File "/usr/lib/python2.6/cookielib.py", line 1096, in return_ok_secure
    if cookie.secure and request.get_type() != "https":
AttributeError: '_FakeUrllib2Request' object has no attribute 'get_type'

Since CookieAgent is new, it would be nice to fix this before it's included in a release.

Change History (10)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 3 years ago by therve

  • Owner set to therve

comment:3 Changed 3 years ago by therve

  • Author set to therve
  • Branch set to branches/secure-cookie-5175

(In [32219]) Branching to 'secure-cookie-5175'

comment:4 Changed 3 years ago by therve

  • Keywords review added
  • Owner therve deleted

This is ready to review in the attached branch. I spotted another problem with the port key of cookies, which I handled as well.

comment:5 Changed 3 years ago by jonathanj

  • Keywords review removed
  • Owner set to therve
  1. It's unfortunate that the methods of _FakeUrllib2Request do not have docstrings but perhaps you could add a "@see" link to <http://docs.python.org/library/urllib2.html#request-objects> to the class docstring?
  1. Looking at the source for cookielib there is only one method left on urllib2.Request that _FakeUrllib2Request does not implement and that is get_origin_req_host. It would be nice to finish off the implementation of _FakeUrllib2Request. (Though this might be more suited to the branch adding redirect support.)
  1. You're missing a newsfile.

Looks good to merge after these have been fixed and the buildbot is green.

comment:6 Changed 3 years ago by exarkun

There's no test for the negative case. Since this is a security-related feature, it's probably doubly important to exercise the code path that causes secure cookies for a particular request to not be sent over an insecure transport (though even for non-security related cases, it's important to test positive and negative paths).

comment:7 Changed 3 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • I added a link to the python.org doc
  • Yeah I'll postpone that
  • Added a (empty) news file
  • I added tests for the negative case. It feels like testing cookielib, but anyway.

Back to review!

comment:8 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:9 Changed 3 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to therve

Thanks for working on this, therve. The test additions look great; I have a couple of small comments if you're willing to indulge them:

  • http://tools.ietf.org/html/rfc3986 uses "scheme" and "schemes" to describe things like http and https, instead of "schema" as in the _FakeUrllib2Request docstring.
  • The use of "secure=1" in the test cookies seems to work anyway, but http://www.ietf.org/rfc/rfc2965.txt section 3.2.2 says that the "secure" attribute has no value.
  • In test_portCookie, transfered ==> transferred.

Other than that, looks great, please merge!

build results

comment:10 Changed 3 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [32335]) Merge secure-cookie-5175

Author: therve
Reviewers: jonathanj, exarkun, jesstess
Fixes: #5175

Fix the fake urllib2.Request used by CookieAgent so that secure cookies can be
used properly.

Note: See TracTickets for help on using tickets.