Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Feature/fix client side cookie gen #6

Closed
wants to merge 2 commits into from

2 participants

@abadger
Owner

We've been having trouble with python-request's handling of a client-side generated cookie that has domain set to "localhost". This issue did not occur with our pycurl code. Inspection of the previous code showed that we did not set the domain (or any cookie attributes) on the client-generated cookie that we were returning to the server. This commit removes the setting of attributes on the client-generated cookie in our new code.

This seems to be correct because attributes on cookies (path, domain, secure, httponly, etc) are only set on a Set-Cookie header that is sent by the server to the client. The client uses those attributes when it determines whether a particular cookie should be sent back to the server when a new url is requested but it does not include them when it passes the cookie name and values back in a Cookie header.

Wikipedia section with that in plain English: http://en.wikipedia.org/wiki/HTTP_cookie#Cookie_attributes

Relevant section from the Cookie RFC: http://tools.ietf.org/html/rfc6265#section-5.4 The fourth step in creating the cookie header is to "Output the cookie's name, the %x3D ("=") character, and the cookie's value." The cookie's attributes were only used to determine if the data would be set; it isn't set in the Cookie header. Also contrast with http://tools.ietf.org/html/rfc6265#section-5.2 which talks about the Set-Cookie header that is sent by the server.

@ralphbean
Owner

I tried using this in a test case and ended up getting those tracebacks again:

Traceback (most recent call last):
  File "testit.py", line 40, in <module>
    print c.query(mine=True)
  File "/home/fedora/ralph/testing-stuff/lib/python2.6/site-packages/fedora/client/bodhi.py", line 147, in query
    return self.send_request('list', req_params=params, auth=auth)
  File "/home/fedora/ralph/testing-stuff/lib/python2.6/site-packages/fedora/client/baseclient.py", line 346, in send_request
    auth_params=auth_params, retries=retries)
  File "/home/fedora/ralph/testing-stuff/lib/python2.6/site-packages/fedora/client/proxyclient.py", line 381, in send_request
    new_session = response.cookies.get(self.session_name, '')
  File "/home/fedora/ralph/testing-stuff/lib/python2.6/site-packages/requests/cookies.py", line 160, in get
    return self._find_no_duplicates(name, domain, path)
  File "/home/fedora/ralph/testing-stuff/lib/python2.6/site-packages/requests/cookies.py", line 281, in _find_no_duplicates
    raise CookieConflictError('There are multiple cookies with name, %r' % (name))
requests.cookies.CookieConflictError: There are multiple cookies with name, 'tg-visit'

Here's the script I used for testing. (It is a little awkward because I was trying to model what /usr/bin/bodhi was doing).

import fedora.client
import logging
import sys
import getpass

logging.basicConfig(
    level=logging.DEBUG,
    stream=sys.stdout,
)
c = fedora.client.BodhiClient(
        "http://localhost/updates",        # I'm running this on releng04
        username="ralph",                   # Change this to your username
)
while True:
        try:
                print c.query(mine=True)
                print "Trying a second time..."
                print c.query(mine=True)
                print "Third time's a charm..."
                print c.query(mine=True)
        except fedora.client.AuthError:
                c.password = getpass.getpass("pw plz")

It is useful for debugging to add print statements on response.cookies after the call to requests.post(...). If you add a print response.cookies line, you see the following::

<<class 'requests.cookies.RequestsCookieJar'> [
   <Cookie tg-visit=a23f6dac037b13c4314c36e856f51e33c4b81483 for />,
   <Cookie tg-visit=a23f6dac037b13c4314c36e856f51e33c4b81483 for localhost.local/>
]>

The first cookie is the one that we set in the beginning on our request. The second is the one set by the server. python-requests then gets "confused" about what to do and we get the traceback at the top of this comment.

@abadger
Owner

Worked on the broken test case today and found out that this is a bug in python-requests < 1.0.0. When requests posts to a url, you may set some cookies on the Request so that they are sent to the server. When the server replies python-requests sets up a response object. In 0.14.x, the cookies that are set on the Response object are taken from both the cookies that were returned from the server and from the cookies that the Request object holds. This is incorrect behaviour. Many pieces of code want to know specifically what cookies were returned from the server as these will be the most current and may contain information that should replace what was given before.

I've generated a patch for python-requests to do that. We'll be testing it in infrastructure along with this patch to python-fedora tonight.

@abadger
Owner

Reviewed by threebean. Merged to devel via git flow

@abadger abadger closed this
@ralphbean
Owner

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 10 deletions.
  1. +4 −10 fedora/client/proxyclient.py
View
14 fedora/client/proxyclient.py
@@ -40,17 +40,11 @@
from urllib.parse import urlparse
try:
- import simplejson as json
-except ImportError:
- import json as json
-
-try:
from hashlib import sha1 as sha_constructor
except ImportError:
from sha import new as sha_constructor
from bunch import bunchify
-from kitchen.iterutils import isiterable
from kitchen.text.converters import to_bytes
from fedora import __version__, b_
@@ -304,10 +298,10 @@ def send_request(self, method, req_params=None, auth_params=None,
# If we have a session_id, send it
if session_id:
# Anytime the session_id exists, send it so that visit tracking
- # works. Will also authenticate us if there's a need.
- cookies.set(self.session_name, session_id,
- secure=True, domain=self.domain,
- rest={'HttpOnly': True})
+ # works. Will also authenticate us if there's a need. Note that
+ # there's no need to set other cookie attributes because this is a
+ # cookie generated client-side.
+ cookies.set(self.session_name, session_id)
complete_params = req_params or {}
if session_id:
Something went wrong with that request. Please try again.