Pull requests

#55 Open
cherrypy/CherryPy CherryPy

_cprequest cookie bug fix

Bitbucket cannot automatically merge this request.

The commits that make up this pull request have been removed.

Bitbucket cannot automatically merge this request due to conflicts.

Review the conflicts on the Overview tab. You can then either decline the request or merge it manually on your local system using the following commands:

hg update default
hg pull -r default https://bitbucket.org/dimdog/cherrypy-_cprequest-cookie-bug-fix
hg merge bd64ee804750
hg commit -m 'Merged in dimdog/cherrypy-_cprequest-cookie-bug-fix (pull request #55)'
  1. dimdog

I found that when using _cprequest.run, even if a cookie is already attached to the request object, the cookie is still blown away. This patch causes it to check if a cookie is already there, and keep the preexisiting cookie if it exists, else the old behavior takes over.

  • Learn about pull requests

Comments (8)

  1. Jason R. Coombs

    Under what conditions is a cookie already attached to a request object?

    The proposed patch seems particularly dangerous given that Request.cookie is a SimpleCookie() as well (so the class instance could end up becoming the basis for every request, leaking cookies).

    1. dimdog author

      Imagine a scenario where a user logs into a web page, he passes in a username / password, and is handed back an authentication token(in the form of a cookie). On the subsequent request, the user includes the token in his request to indicate to the server that he is currently logged in. Without this pull request, it is impossible to ever attach a cookie on an outgoing request because it always attaches a fresh blank cookie just before sending off. This PR provides the user the ability to choose whether or not to attach a cookie to their request.

      1. Jason R. Coombs

        dimdog, I'm afraid I'm not following (probably due to my lack of experience in the code base). It sounds like from what you're describing, cookies wouldn't work at all in CherryPy. I'm certain that's not the case, so help me bridge the gap.

        Can you provide an example that demonstrates what you're trying to accomplish? Alternatively (and perhaps even better) would be to provide a test in the suite that captures the use case, failing without your patch, and passing with it.

  2. Joel Rivera

    I don't really see the use case of this change, and I think that you are trying to set a cookie on the request headers not on the response headers.

    To further explain the situation:

    On each new request a new pair of objects gets created (Request, Response).

    You are suppose to set the cookies on the form:

    cherrypy.response.cookie['name'] = 'value'

    (Notice the response)

    That is an abstraction over the threadlocals making a "proxy" through the cherrypy namespace.

    And to read the one that the client has sent, you just do:


    (Notice the request)

    You could make the mental mapping of:

    Response => HTTP header: Set-Cookie

    Request => HTTP header: Cookie

    The cookies are stored on the client, not the server, the workflow of Cookie/Set-Cookie on each request/response or more like a session with a keep a live threshold.

    Here is more information related to the cookies on cherrypy.

  3. Joel Rivera

    dimdog maybe this PR is already irrelevant to you, given the time on which you submit the request, could you do us the favor on giving an update of your position related to this PR?

  4. dimdog author

    I forked cherrypy a while ago to incorporate this change, though obviously I would like to not be on my own fork that is growing more and more out of date.

    The scenario this came up in is when I was testing code, and I was using the CherryPy request object to communicate with a cherryPy server. I would log in, get handed back a cookie on the response, then try to include the cookie in my new request - showing that I was logged in, essentially - and it would always be overridden by SimpleCookie's default values. This change is designed to allow, if you want, to set a cookie on a request object before firing it off.

    Does that make sense? Essentially, without this change, I couldn't make an authenticated request.

    1. Joel Rivera

      I see, so it's testing. That's a reasonable use-case.

      There are ways to do that, but it will probably require some extra-lines on your testing (basically fill the request cookie from the response cookie).

      Do you have any comments on this Jason R. Coombs?