gh-135768: fix allowed/blocked IPv6 domains in `http.cookiejar` #135771

LamentXU123 · 2025-06-20T17:00:47Z

PR for #135768, Thanks!

Issue: Correctly block IPv6 domains in http.cookiejar #135768

picnixz

Please add tests.

Lib/http/cookiejar.py

bedevere-app · 2025-06-20T17:15:56Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Bénédikt Tran <[email protected]>

Lib/http/cookiejar.py

Misc/NEWS.d/next/Library/2025-06-20-17-03-51.gh-issue-135768.DhUJWf.rst

…hUJWf.rst Co-authored-by: Bénédikt Tran <[email protected]>

Lib/http/cookiejar.py

LamentXU123 · 2025-06-20T18:18:08Z

Please add tests.

Done.

Lib/http/cookiejar.py

Lib/test/test_http_cookiejar.py

Lib/http/cookiejar.py

Co-authored-by: Bénédikt Tran <[email protected]>

LamentXU123 · 2025-06-21T11:54:31Z

In a follow-up PR I think we can refactor the tests because there's lot of duplicated code but not in this one yet.

I'll work on this since this PR is done.

picnixz · 2025-06-21T12:19:24Z

I'll work on this since this PR is done.

No, wait until this is merged. We may need a new issue but this is not pressing right now. Focus on this one first.

picnixz

Please, be more careful with your choices. First, answer my question about .local (why are we adding it in the first place?)

Second, detecting whether something is an IP or not needs to be more robust. For instance, we could block domains without using [] notation and only ::. Indeed, an IPv6 is not enclosed by [].

Finally, it would be good if we had more RFC-backed choices.

Lib/http/cookiejar.py

picnixz · 2025-06-21T12:22:46Z

Lib/http/cookiejar.py

+        if text.startswith('[') and text.endswith(']'):
+            try:
+                IPv6Address(text.removeprefix('[').removesuffix(']'))
+            except ValueError:
+                return False
+        else:
+            return False # not a IPv6 address in []


Why are IPv6 addressed not surrounded [] rejected? when making a request, we would use http://[::1] for instance, but we could decide to reject the domain ::1 itself. Please be more careful. Instead, we should make it allow both valid IPv6 addresses as well as IPv6 addresses enclosed in '[]'. But then, we need to be careful with the matching. Should [::1] be matched against ::1 or [::1] only?

Try to look at the RFC as well to see if there are more information about IPv6 domains.

picnixz · 2025-06-21T12:27:16Z

Lib/test/test_http_cookiejar.py

        self.assertFalse(is_HDN(""))
        self.assertFalse(is_HDN("."))
        self.assertFalse(is_HDN(".foo.bar.com"))
        self.assertFalse(is_HDN("..foo"))
        self.assertFalse(is_HDN("foo."))

+    def test_is_ip(self):
+        self.assertTrue(is_ip('[::1]'))


Strictly speaking, it's not an IPv6. It's a netloc with an IPv6 inside it. We should perhaps name is_ip_like instead and also treat ::1 as a valid IP (for blocked domains). I don't have an answer to that question so I'd prefer to know if there is some recommendations done by RFCs.

bedevere-app · 2025-06-21T12:28:26Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

LamentXU123 · 2025-06-21T12:43:05Z

Please, answer to this. Why should we avoid adding '.local'? until this is answered, I won't review it more.

because using .local in IPv6 doesn't make any sense. It will change the input IPv6 to something like [::1].local.

The additional .local is designed for HDNs not IP addresses. Before this PR. We assumed that there are only two types of input: IPv4 addr and HDN. IPv4 indeed contains dots. Some abnormal HDN do not contain dot, than we are adding a .local at the end of it. This is the expected case.

But, IPv6 is a edge case. It do not contains dot. And it is not a HDN. Adding .local after a IPv6 address is meaningless and, the most important thing, will cause error in logic when we are blocking hosts. Since the IPv6 is [::1].local and the input is [::1], every IPv6 is not blocked.

It is the .local at the end of the address that cause the bug.

picnixz · 2025-06-21T12:48:29Z

My question was more: why did we add .local for IPv4 in the first place. There is a bunch of logic involving local domains in set_ok_domain and I believe this also needs an upgrade if we're handling IPv6 now. FQDN and IPv4 look the same but IPv6 is entirely different. Therefore, we should be also careful with the changes that could affect the logic of set_ok_domain().

LamentXU123 · 2025-06-21T13:13:49Z

My question was more: why did we add .local for IPv4 in the first place. There is a bunch of logic involving local domains in set_ok_domain and I believe this also needs an upgrade if we're handling IPv6 now. FQDN and IPv4 look the same but IPv6 is entirely different. Therefore, we should be also careful with the changes that could affect the logic of set_ok_domain().

I might be stupid here, but I think we are actually not adding .local for IPv4. RFC 2965 quotes:

If a host name
contains no dots, the effective host name is that name with the
string .local appended to it.

And the implementation is correct. IPv4 contains dots so .local will not be added. The .local will only be added to host name without dots. like http://acme, the requested host name will be changed to acme.local, but http://127.0.0.1 will not

so sorry in advanced if I misunderstand the code

LamentXU123 · 2025-06-21T13:16:26Z

you can locally test this by adding breakpoints in func eff_request_host to see what is returned of these two code:

import urllib.request
from http.cookiejar import CookieJar, DefaultCookiePolicy
class FakeResponse:
    def __init__(self, headers=[], url=None):
        """
        headers: list of RFC822-style 'Key: value' strings
        """
        import email
        self._headers = email.message_from_string("\n".join(headers))
        self._url = url
    def info(self): return self._headers
pol = DefaultCookiePolicy(
            rfc2965=True, blocked_domains=[])
c = CookieJar(policy=pol)
headers = ["Set-Cookie: CUSTOMER=WILE_E_COYOTE; path=/"]
pol.set_blocked_domains(['acme'])
req = urllib.request.Request("http://acme")
res = FakeResponse(headers, "http://acme")
c.extract_cookies(res, req)

# And

c.clear()
pol.set_blocked_domains(['127.0.0.1'])
req = urllib.request.Request("http://127.0.0.1")
res = FakeResponse(headers, "http://127.0.0.1")
c.extract_cookies(res, req)

LamentXU123 · 2025-06-21T13:21:40Z

And for the issue of [::1] and ::1. I quote this from RFC 2396 3.2.2

The host is a domain name of a network host, or its IPv4 address as a
set of four decimal digit groups separated by ".". Literal IPv6
addresses are not supported.

I chose to use only [::1] because of this statement, hum..... I'll find some clearer statements now

picnixz · 2025-06-21T13:24:22Z

I might be stupid here, but I think we are actually not adding .local for IPv4. RFC 2965 quotes:

Oups my bad. Ok, so this is an RFC thing. Now, is there a similar RFC for IPv6?
What I meant with set_ok_domain is that we have some 'strict' policy and under the non-strict policy parsing, I don't know whether [::1] should be treated the same as ::1 (and whether strict policy parsing means that [::1] can only be blocked if we blocked it explicitly, not just ::1) (see https://docs.python.org/3/library/http.cookiejar.html#http.cookiejar.DefaultCookiePolicy.DomainStrictNonDomain).

(It appears that there is a lot of code that is IPv-4 only but if we're adding support to IPv6, we need to make it work everywhere actually).

so sorry in advanced if I misunderstand the code

no worries!

you can locally test this by adding breakpoints in func eff_request_host to see what is returned of these two code:

Sorry but I don't have time for that.

LamentXU123 · 2025-06-21T13:33:43Z

(It appears that there is a lot of code that is IPv-4 only but if we're adding support to IPv6, we need to make it work everywhere actually).

I think we could treat them in seperate issues and PRs, I think we could just focus on fixing allowed/blocked IPv6 domains in http.cookiejar now, and in the future we need to change a lot on that(I think I can handle this), including tests, docs, and logic everywhere. But yeah, I think we could just make sure the blocking and allowing work for IPv6 in this PR. I will make some changes based on what you've commented now.

Sorry but I don't have time for that.

I really thank you so much for your time. My coding skills and experiences have improved by all this.

LamentXU123 · 2025-06-21T13:38:53Z

And for the [::1] issue. Well IMO RFC states that Literal IPv6 addresses are not supported as host name. So to be strict, only [::1] is a legel hostname while ::1 is a legel IPv6 address. I think we could just support [::1] in any case.

But hey, users are likely to use ::1 if they don't read the docs very carefully, I actually don't know how to deal with this problem very properly. I need your suggestions tho @picnixz. Thanks!

LamentXU123 · 2025-06-21T13:50:18Z

No worries of set_ok_domain. This PR do not change any logic of it. We can change the logic in the future PRs. I think we could just focus on allowed/blocked issues in this PR now.

The only related change of other funcs is eff_request_host, which is used in set_ok_domain. But the only thing it did is just to treat IPv6 as a IP address and not adding .local on it. Since the original one don't support IPv6, this PR indeed also don't supprt IPv6 in the logic of set_ok_domain. We could take care of it later.

(There are indeed a hole bunch of things to change if we are going to support IPv6 entirely)

What I meant with set_ok_domain is that we have some 'strict' policy and under the non-strict policy parsing, I don't know whether [::1] should be treated the same as ::1 (and whether strict policy parsing means that [::1] can only be blocked if we blocked it explicitly, not just ::1) (see https://docs.python.org/3/library/http.cookiejar.html#http.cookiejar.DefaultCookiePolicy.DomainStrictNonDomain).

I'll take a look at this after this PR is merged.

Update cookiejar.py

e3cfeb6

bedevere-app bot added the awaiting review label Jun 20, 2025

bedevere-app bot mentioned this pull request Jun 20, 2025

Correctly block IPv6 domains in http.cookiejar #135768

Open

blurb-it bot and others added 4 commits June 20, 2025 17:03

📜🤖 Added by blurb_it.

8c5320a

Update 2025-06-20-17-03-51.gh-issue-135768.DhUJWf.rst

4c452f9

Update 2025-06-20-17-03-51.gh-issue-135768.DhUJWf.rst

60304ce

Update 2025-06-20-17-03-51.gh-issue-135768.DhUJWf.rst

714d4d8

picnixz requested changes Jun 20, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Jun 20, 2025

LamentXU123 and others added 5 commits June 21, 2025 01:19

Update Lib/http/cookiejar.py

4c9ab02

Co-authored-by: Bénédikt Tran <[email protected]>

Update Lib/http/cookiejar.py

2c9f6a4

Co-authored-by: Bénédikt Tran <[email protected]>

Update Lib/http/cookiejar.py

a5198ee

Co-authored-by: Bénédikt Tran <[email protected]>

Update Lib/http/cookiejar.py

baf62d1

Co-authored-by: Bénédikt Tran <[email protected]>

Update cookiejar.py

0783fc1

picnixz reviewed Jun 20, 2025

View reviewed changes

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

picnixz reviewed Jun 20, 2025

View reviewed changes

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

add IPV6 regex

dffb204

picnixz reviewed Jun 20, 2025

View reviewed changes

Misc/NEWS.d/next/Library/2025-06-20-17-03-51.gh-issue-135768.DhUJWf.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2025-06-20-17-03-51.gh-issue-135768.D…

056b44d

…hUJWf.rst Co-authored-by: Bénédikt Tran <[email protected]>

picnixz reviewed Jun 20, 2025

View reviewed changes

Lib/http/cookiejar.py Show resolved Hide resolved

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

picnixz changed the title ~~gh-135768: Support IPv6 in http.cookiejar~~ gh-135768: fix allowed/blocked IPv6 domains in http.cookiejar Jun 20, 2025

LamentXU123 added 4 commits June 21, 2025 01:41

add string method instead of regex to detect IPV6

37c1933

update is_ip

779e443

update comments

1c00c39

Update test_http_cookiejar.py

4c12cec

LamentXU123 requested a review from picnixz June 20, 2025 18:18

picnixz reviewed Jun 20, 2025

View reviewed changes

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

Lib/http/cookiejar.py Outdated Show resolved Hide resolved

Lib/test/test_http_cookiejar.py Show resolved Hide resolved

Lib/http/cookiejar.py Show resolved Hide resolved

LamentXU123 and others added 9 commits June 21, 2025 19:06

Update Lib/test/test_http_cookiejar.py

af9d29e

Co-authored-by: Bénédikt Tran <[email protected]>

Update cookiejar.py

56ac545

Update test_http_cookiejar.py

f90b354

Update cookiejar.py

3d1cc91

update is_ip

0879750

delete test for ::1

7731e7f

Merge branch 'main' into support-IPv6-in-cookiejar

35f8b9e

Update test_http_cookiejar.py

9c4a91f

Update test_http_cookiejar.py

1b0f228

LamentXU123 requested a review from picnixz June 21, 2025 12:01

picnixz requested changes Jun 21, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting change review labels Jun 21, 2025

picnixz added the DO-NOT-MERGE label Jun 21, 2025

LamentXU123 added 2 commits June 21, 2025 21:46

Change is_ip to is_ip_like

8f2c9c3

Update test script for is_ip_like

e01652e

LamentXU123 requested a review from picnixz June 21, 2025 13:58

Update cookiejar.py

3d59ed9

Uh oh!

gh-135768: fix allowed/blocked IPv6 domains in http.cookiejar #135771

Are you sure you want to change the base?

gh-135768: fix allowed/blocked IPv6 domains in http.cookiejar #135771

Conversation

LamentXU123 commented Jun 20, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented Jun 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LamentXU123 commented Jun 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LamentXU123 commented Jun 21, 2025

Uh oh!

picnixz commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

picnixz Jun 21, 2025

Choose a reason for hiding this comment

Uh oh!

picnixz Jun 21, 2025

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Jun 21, 2025

Uh oh!

LamentXU123 commented Jun 21, 2025

Uh oh!

picnixz commented Jun 21, 2025

Uh oh!

LamentXU123 commented Jun 21, 2025

Uh oh!

LamentXU123 commented Jun 21, 2025

Uh oh!

LamentXU123 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LamentXU123 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LamentXU123 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LamentXU123 commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gh-135768: fix allowed/blocked IPv6 domains in `http.cookiejar` #135771

gh-135768: fix allowed/blocked IPv6 domains in `http.cookiejar` #135771

LamentXU123 commented Jun 20, 2025 •

edited by bedevere-app bot

Loading

picnixz commented Jun 21, 2025 •

edited

Loading

LamentXU123 commented Jun 21, 2025 •

edited

Loading

picnixz commented Jun 21, 2025 •

edited

Loading

LamentXU123 commented Jun 21, 2025 •

edited

Loading

LamentXU123 commented Jun 21, 2025 •

edited

Loading

LamentXU123 commented Jun 21, 2025 •

edited

Loading