bpo-38976: Add support for HTTP Only flag in MozillaCookieJar by jacobneiltaylor · Pull Request #17471 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits13 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

jacobneiltaylor

This PR adds support for the HttpOnly flag as encoded in CURL cookiejars.

This PR was mainly designed to allow the MozillaCookieJar to parse in the cookies, as previously they were considered comments and ignored.

As HttpOnly is considered a non-standard attribute, the nonstandard attribute dict was considered the most appropriate place to persist this information.

https://bugs.python.org/issue38976

@jacobneiltaylor

@blurb-it

@jacobneiltaylor

For more information:

orsenthil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, @jacobneiltaylor

I only had two brief comments. Please see if you like to address them.

I also wonder why a very old issue got resurfaced again? Is there a practical change in the curl and other clients that we missed noticing? (ref: https://bugs.python.org/msg300920)

@@ -2004,6 +2004,7 @@ class MozillaCookieJar(FileCookieJar):
header by default (Mozilla can cope with that).
"""
httponly_prefix = "#HttpOnly_"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't visited this module often. Do you think, we could move this class attributes to a __init__ method - The new httponly_prefix as well as the previous magic_re and header.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, you are asking for the class variables to be removed and the values moved into the constructor as literals?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think, we could move this class attributes to a __init__ method - The new httponly_prefix as well as the previous magic_re and header.

Hi Jacob,

My original thinking was - we add a __init__ method and we move those variables to the __init__ method, then our usage with self.attr_name becomes consistent with instance variable usages.

Move them to module-level headers is fine too.

I notice that moving.compile` to the module header will add a slight performance to the module which was not present. I am fine with it, but I will try to measure the performance of the test suite, before and after to ensure there is no drastic difference with the change.

if line == "": break
# detect httponly flag if present

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will a comment or a reference to an RFC help to explain why we are doing this?

@jacobneiltaylor

Thanks for the feedback @orsenthil - I'll work on those code changes.

With regard to the "why", I can share some limited details on the specific use case that prompted this contribution. An in-house SSO system I work with allows users to periodically vend libcurl-compatible cookiejar files. These files are explicitly for use with console scripts that interface with internal web APIs, usually for purposes of testing, experimentation and automation. They currently work with cURL-based BASH scripts, however there is increased interest in using them with more sophisticated Python-based automation.

The cookiejar files are generated with the #HttpOnly_ prefix, which are ignored as comments under the current implementation. We have an internal workaround, but we thought it would be prudent to share a fix with the community in case others have the same or a similar use case (as niche as the use case may be).

@jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

@blurb-it @jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

@jacobneiltaylor

…ython into mozcookiejar-httponly

@orsenthil

Azure Pipelines PR — #20200402.24 failed

this is make patchcheck failure, perhaps there is a extra-space or a newline some in the patch. Running it locally could help us figure it out.

@jacobneiltaylor

@jacobneiltaylor

Thanks Senthil, that is now fixed by the looks of things.

With regard to the changes made, I opted to move all the class variables to module constants instead of literals. This was because they were either relatively complex declarations (multiline strings/compiled regex objects) or were referenced in multiple locations.

Additionally it is important to note that while HTTPOnly is a standard attribute in RFC6265, these modules are designed to model the cookies as defined in RFC2965.

As such, I still believe the nonstandard attributes dictionary is the correct location to persist this information.

orsenthil

if line == "": break
# httponly is a cookie flag as defined in rfc6265
# when encoded in a netscape cookie file,
# the line is prepended with "#HttpOnly_"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

orsenthil

@bedevere-bot

@orsenthil: Please replace # with GH- in the commit message next time. Thanks!

@dlenski

This issue is a duplicate of bpo-2190, which means that my PR (#22798) for that one is probably now unnecessary.

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 13, 2021

…#17471)

Add support for HTTP Only flag in MozillaCookieJar

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

dlenski

@@ -50,10 +50,18 @@ def _debug(*args):
logger = logging.getLogger("http.cookiejar")
return logger.debug(*args)
HTTPONLY_ATTR = "HTTPOnly"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is similar to my alternative approach in #22798 (and see bpo-2190), except: