Opened 8 years ago

Closed 8 years ago

#5175 defect closed fixed (fixed)

CookieAgent fails when it encounters "secure" cookies

Reported by: Jean-Paul Calderone Owned by: therve
Priority: high Milestone: Twisted-11.1
Component: web Keywords: httpclient
Cc: Jonathan Jacobs, therve, jknight, jesstess Branch: branches/secure-cookie-5175
branch-diff, diff-cov, branch-cov, buildbot
Author: therve


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/", line 1046, in request
  File "/usr/lib/python2.6/", line 1323, in add_cookie_header
    cookies = self._cookies_for_request(request)
  File "/usr/lib/python2.6/", line 1249, in _cookies_for_request
    cookies.extend(self._cookies_for_domain(domain, request))
  File "/usr/lib/python2.6/", line 1238, in _cookies_for_domain
    if not self._policy.return_ok(cookie, request):
  File "/usr/lib/python2.6/", line 1070, in return_ok
    if not fn(cookie, request):
  File "/usr/lib/python2.6/", line 1096, in return_ok_secure
    if 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 8 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 8 years ago by therve

Owner: set to therve

comment:3 Changed 8 years ago by therve

Author: therve
Branch: branches/secure-cookie-5175

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

comment:4 Changed 8 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 8 years ago by Jonathan Jacobs

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 <> 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 8 years ago by Jean-Paul Calderone

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 8 years ago by therve

Keywords: review added
Owner: therve deleted
  • I added a link to the 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 8 years ago by jesstess

Owner: set to jesstess

comment:9 Changed 8 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:

  • 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 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 8 years ago by therve

Resolution: fixed
Status: newclosed

(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.