Skip to content

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

Closed
dlenski wants to merge 3 commits into
python:masterfrom
dlenski:patch-1
Closed

bpo-2190: make MozillaCookieJar handle HTTP-only cookies#22798
dlenski wants to merge 3 commits into
python:masterfrom
dlenski:patch-1

Conversation

@dlenski

@dlenski dlenski commented Oct 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@jstasiak jstasiak left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread Lib/http/cookiejar.py Outdated
Comment thread Lib/http/cookiejar.py Outdated
@dlenski

dlenski commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

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

@terryjreedy terryjreedy left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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.

Comment thread Doc/library/http.cookiejar.rst Outdated
Comment thread Lib/http/cookiejar.py Outdated
terryjreedy
terryjreedy reviewed Oct 23, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2020-10-20-00-34-00.bpo-2190.WNIr6.rst Outdated
ZackerySpytz
ZackerySpytz reviewed Nov 15, 2020
View reviewed changes
Comment thread Lib/http/cookiejar.py Outdated
Comment thread Lib/test/test_http_cookiejar.py Outdated
Comment thread Lib/test/test_http_cookiejar.py Outdated
Comment thread Lib/http/cookiejar.py Outdated
Comment thread Misc/NEWS.d/next/Library/2020-10-20-00-34-00.bpo-2190.WNIr6.rst Outdated
@ZackerySpytz

ZackerySpytz commented Nov 15, 2020

Copy link
Copy Markdown
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

terryjreedy commented Nov 15, 2020

Copy link
Copy Markdown
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

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

Copy link
Copy Markdown
Contributor Author

… 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

orsenthil commented Nov 18, 2020

Copy link
Copy Markdown
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

orsenthil commented Nov 18, 2020

Copy link
Copy Markdown
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
@orsenthil orsenthil Awaiting requested review from orsenthil
+2 more reviewers
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
@jstasiak jstasiak jstasiak approved these changes
Reviewers whose approvals may not affect merge requirements

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