Skip to content

Conversation

acordiner
Copy link
Contributor

This change permits cookie values to be quoted and contain equal signs, e.g.:

_cookie_session="hello=world"

@coveralls
Copy link

coveralls commented Feb 7, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 88eadc4 on acordiner:cookies-with-quotes into fb323b8 on TracyWebTech:master.

@acordiner
Copy link
Contributor Author

Any chance to get this merged?

@seocam
Copy link
Contributor

seocam commented Feb 24, 2017

Thanks @acordiner, that's really interesting!

I was checking the complete cookies specs and looks like we are missing a bunch of stuff: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Directives

@acordiner
Copy link
Contributor Author

There is actually a built-in Python library for handling cookies: https://docs.python.org/2/library/cookie.html
It takes care of these sort of decoding issues.

@seocam
Copy link
Contributor

seocam commented Mar 1, 2017

Looks like for some reason that the http.cookies library doesn't work for every cookie. Take a look in this example:

This code:

from http.cookies import Cookie

cookie = "_cookie_session=1266bb13c139cfba3ed1c9c68110bae9;" \
         "expires=Thu, 29 Jan 2015 13:51:41 -0000; httponly;" \
         "secure;Path=/gitlab"

dict(Cookie(cookie).values()[0])

Outputs:

{'comment': '',
 'domain': '',
 'expires': 'Thu,',
 'httponly': True,
 'max-age': '',
 'path': '/gitlab',
 'secure': True,
 'version': ''}

While using cookie_from_string:

from revproxy.utils import cookie_from_string
cookie_from_string(cookie)

Outputs:

{'expires': 'Thu, 29 Jan 2015 13:51:41 -0000',
 'httponly': True,
 'key': '_cookie_session',
 'path': '/gitlab',
 'secure': True,
 'value': '1266bb13c139cfba3ed1c9c68110bae9'}

@acordiner acordiner force-pushed the cookies-with-quotes branch 2 times, most recently from 735cb46 to ea97417 Compare April 24, 2017 10:12
@acordiner
Copy link
Contributor Author

acordiner commented Apr 24, 2017

I believe that the "expires" attribute must always end with "GMT" to comply with the RFC. If you change -0000 to GMT in your cookie string then it should work. I've pushed another change to my pull request that uses the http.cookies library. What do you think?

@seocam
Copy link
Contributor

seocam commented May 3, 2017

@acordiner the cookie in the test came from a real life example. Unfortunately even if the RFC says it needs to end with GMT if the browsers accept the -0000 and some web applications actually generate the cookie this way your change will break compatibility for current users of django-revproxy.

I understand that your PR is quite important and useful but we need to take care of backward compatibility as well.

@acordiner
Copy link
Contributor Author

How about if there is a setting for "strict" mode: if enabled it uses my code, if disabled it uses the previous code?

The problem with the current code is that it is rejecting valid cookies. Specifically I am trying to use it with JupyterHub, but it doesn't work because JupyterHub uses cookies containing quotes.

@seocam
Copy link
Contributor

seocam commented May 8, 2017

The strict mode seems a viable alternative but I would prefer to try to have the code working for both cases.

If that helps you we can release a version with the strict arg (maybe a private property?) and then we revisit the implementation after.

@acordiner acordiner force-pushed the cookies-with-quotes branch 2 times, most recently from 5d8f45c to 1ba9b3b Compare May 19, 2017 10:37
@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5d8f45c on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1ba9b3b on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner acordiner force-pushed the cookies-with-quotes branch from 1ba9b3b to 2e3ffb3 Compare May 19, 2017 12:33
@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2e3ffb3 on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2e3ffb3 on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner acordiner force-pushed the cookies-with-quotes branch from 2e3ffb3 to 62fef6c Compare May 19, 2017 13:58
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 62fef6c on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 62fef6c on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 62fef6c on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner acordiner force-pushed the cookies-with-quotes branch from 62fef6c to 612e8c7 Compare June 27, 2017 04:49
@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 612e8c7 on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner acordiner force-pushed the cookies-with-quotes branch from 612e8c7 to 39bc69a Compare June 27, 2017 04:57
@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling 39bc69a on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner acordiner force-pushed the cookies-with-quotes branch from 39bc69a to b214847 Compare June 27, 2017 05:24
@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-100.0%) to 0.0% when pulling b214847 on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2b60229 on acordiner:cookies-with-quotes into b0e7be6 on TracyWebTech:master.

@acordiner
Copy link
Contributor Author

@seocam - For now I've added the strict mode as discussed.

@acordiner
Copy link
Contributor Author

Any chance to get this merged?

@seocam
Copy link
Contributor

seocam commented Jun 30, 2017

I'm gonna merge it but not release a version yet because it's missing docs. Could you please work on that? Thanks for the contribution!

@seocam seocam merged commit 4da98bd into jazzband:master Jun 30, 2017
@acordiner
Copy link
Contributor Author

@seocam - I've added some documentation to proxyview.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants