Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

bpo-2190: make MozillaCookieJar handle HTTP-only cookies #22798

Closed
wants to merge 3 commits into from
Closed

bpo-2190: make MozillaCookieJar handle HTTP-only cookies #22798

wants to merge 3 commits into from

Conversation

dlenski
Copy link

@dlenski dlenski commented Oct 20, 2020 •
edited
Loading

This resolves a longstanding issue with http.cookiejar.MozillaCookieJar, namely the fact that it does not know how to load or save cookies marked HTTP-only in the on-disk file format.

As discussed in bpo-2190, this file format is not obsolete (despite the fact that Mozilla/Firefox browsers no longer store cookies in it).

cURL's -b/-c options still load and store cookies in this format, including marking cookies as HTTP-only in this way.

With these changes applied, MozillaCookieJar will…

  1. Correctly .load HTTP-only cookies,
  2. Add a Cookie.httponly attribute to mark them as such, and
  3. Correctly .save cookies with httponly == True

https://bugs.python.org/issue2190

@dlenski dlenski force-pushed the patch-1 branch 4 times, most recently from 9be98f0 to a3b7f8b Compare October 20, 2020 02:05
jstasiak
jstasiak reviewed Oct 20, 2020
View reviewed changes
Copy link
Contributor

@jstasiak jstasiak left a comment

Choose a reason for hiding this comment

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

Two minor comments but LGTM otherwise, the relevant paths are being tested and everything else seems in order.

@dlenski
Copy link
Author

dlenski commented Oct 20, 2020

Other than unnecessarily pushing a new commit… is there any way to retrigger the failed builds (probably an Azure issue?) so that this PR doesn't have the scarlet ❌ next to it?

@python python deleted a comment from the-knights-who-say-ni Oct 23, 2020
terryjreedy
terryjreedy reviewed Oct 23, 2020
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Doc change is good. Blurb could use minor change. You can add yourself to misc.ACKS, but that is best done when merge is close. I need to look more before I have an opinion on .py files.

ZackerySpytz
ZackerySpytz reviewed Nov 15, 2020
View reviewed changes
@ZackerySpytz
Copy link
Contributor

Other than unnecessarily pushing a new commit… is there any way to retrigger the failed builds (probably an Azure issue?) so that this PR doesn't have the scarlet x next to it?

Closing and reopening the PR.

@terryjreedy
Copy link
Member

Sometimes coredevs, at least, can trigger a rerun of just one group of tests. But, after 23 days, this is not possible for me for Pipelines. And this does not matter as commits are needed first to resolve the merge conflicts (due, I believe, to another cookiejar patch being merged) and second for Zackery's suggestions.

dlenski and others added 3 commits November 15, 2020 12:15
@dlenski
bpo-2190: load #HttpOnly_ cookies in MozillaCookieJar
a41f289 
This enabled MozillaCookieJar to correctly .load cookies
marked as HTTP-only. (See
https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies
for the meaning of this attribute.)

As discussed in bpo-2190, this format is no longer used by
Firefox itself, but is still in common use due to its adoption
by cURL.
@dlenski
bpo-2190: store HTTP-only attribute, so it can be saved back to Mozil…
614ac02 
…laCookieJar

Renamed from 'http_only' → 'httponly', due to preexisting support for this name
in 'http.cookies' and its tests; the names of the Python properties match the
lowercase names of the keywords in the 'Set-Cookie' HTTP header.
@dlenski
add News blurb and update http.cookiejar docs
39dbe9e
@dlenski
Copy link
Author

dlenski commented Nov 15, 2020 •
edited by bedevere-bot
Loading

… and second for Zackery's suggestions.

Updated the PR to comply with PEP-8 per his suggestions.

And this does not matter as commits are needed first to resolve the merge conflicts (due, I believe, to another cookiejar patch being merged)

It's a bit surprising, but it appears that the other patch (#17471) addressed the exact same issue… or rather, it addressed bpo-38976, which is essentially a duplicate of this bpo-2190.

@dlenski dlenski mentioned this pull request Nov 15, 2020
Merged
@orsenthil
Copy link
Member

HTTP-only cookies for MozillaCookieJar was introduced in #17471
Closing this PR as described in https://bugs.python.org/issue2190

@orsenthil orsenthil closed this Nov 18, 2020
@orsenthil
Copy link
Member

Thanks for your contribution @dlenski. I am not sure how we couldn't notice the overlap of both the issue and independent work earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terryjreedy terryjreedy terryjreedy left review comments

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

@jstasiak jstasiak jstasiak approved these changes

@orsenthil orsenthil Awaiting requested review from orsenthil

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dlenski @ZackerySpytz @terryjreedy @orsenthil @jstasiak @the-knights-who-say-ni @bedevere-bot