Skip to content

Accept cookies from IP address origins as host-only cookies - #13586

Draft
dylanpulver wants to merge 4 commits into
aio-libs:masterfrom
dylanpulver:fix-ip-address-cookies
Draft

Accept cookies from IP address origins as host-only cookies#13586
dylanpulver wants to merge 4 commits into
aio-libs:masterfrom
dylanpulver:fix-ip-address-cookies

Conversation

@dylanpulver

@dylanpulver dylanpulver commented Aug 30, 2026

Copy link
Copy Markdown

What do these changes do?

_is_domain_match() already implements RFC 6265 section 5.1.3 correctly: identical strings match, and only the suffix rules are restricted to non-address hosts. The two blanket is_ip_address() gates in update_cookies() and filter_cookies() sat in front of it and dropped address-origin cookies outright.

Removing those gates alone is not safe, and that is most of this patch. Domain=1.2.3.4 set from 1.2.3.4 currently stores a domain cookie, and a.1.2.3.4 is a legal DNS name that suffix-matches it, so the cookie reaches a host somebody else can register; the reverse also holds. Both are reproducible today under unsafe=True, and making that the default would have shipped them to everyone. So address origins are now host-only in both directions, and a DNS name may not claim an address as its parent domain.

Are there changes in behavior for the user?

Yes, and one is beyond the issue. Cookies now work against address origins by default, including IPv6 and zone-id forms. unsafe is retained but I measured 135 (host x Domain) combinations before and after: it changed 24 cells beforehand, all of them the identical-match case, and 0 after. Every other door it looked like it opened was already shut by _is_domain_match(). It is now a no-op — flagging that rather than burying it; say the word if you would prefer a deprecation warning.

Second, the scope guards also apply to existing unsafe=True users, so a cookie that currently crosses between 1.2.3.4 and a.1.2.3.4 will stop. THREAT_MODEL.md has no cookie-scoping entry, so I did not add one; happy to if you want it recorded.

Is it a substantial burden for the maintainers to support this?

No new public API or option, and the rules sit next to the existing Domain handling rather than in a new code path.

Related issue number

Fixes #1183

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES/ folder
Test runs

Full suite via PYTHONPATH='.' pytest --numprocesses=auto, Cython extensions built, same machine and session:

  • master: 5096 passed, 1 failed
  • this branch: 5126 passed, 1 failed (+30, exactly the new cases)

The two failures are different tests each run (test_read_timeout_between_chunks on master, test_close_timeout here), both pass in isolation on both trees, and neither touches cookies. flake8 clean. mypy reports the same 14 pre-existing errors in 5 unrelated files on both trees and 0 in cookiejar.py.

Mutation testing of the 31 new assertions:

  • revert cookiejar.py to master: 11 fail
  • delete the two gates and nothing else: 6 fail, i.e. the two scope guards
  • keep the fix but drop the DNS-claims-an-address clause: 2 fail, isolating that clause

The other 20 assertions survive every mutant, and I would rather name that than imply otherwise: 4 pin unsafe=True behaviour that must not change, and 16 are negative cases that stay true whenever nothing is stored or _is_domain_match() already rejects the Domain.

Drafted with Claude Opus 5 as a coding agent in this repository; opened as a draft pending the human review your AGENTS.md asks for before it goes out of draft.

RFC 6265 section 5.1.3 permits a cookie on an origin addressed by IP
when the domain string and the host are identical; only the suffix
matching rules are restricted to non-address hosts. CookieJar dropped
these cookies entirely unless constructed with unsafe=True.

Store and return them as host-only cookies. A Domain attribute is
accepted only when it names the same address, and a DNS name may no
longer claim an address as its parent domain, so 1.2.3.4 and a.1.2.3.4
cannot reach each other's cookies.

Signed-off-by: Dylan Pulver <dylanpulver@users.noreply.github.com>
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Aug 30, 2026
Signed-off-by: Dylan Pulver <dylanpulver@users.noreply.github.com>
@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.03%. Comparing base (9e08ba0) to head (389b257).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13586   +/-   ##
=======================================
  Coverage   99.03%   99.03%           
=======================================
  Files         135      135           
  Lines       50940    50986   +46     
  Branches     2677     2679    +2     
=======================================
+ Hits        50446    50492   +46     
  Misses        370      370           
  Partials      124      124           
Flag Coverage Δ
Autobahn 21.96% <30.76%> (+0.01%) ⬆️
CI-GHA 98.92% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.70% <100.00%> (-0.01%) ⬇️
OS-Windows 97.31% <100.00%> (+<0.01%) ⬆️
OS-macOS 98.19% <100.00%> (+<0.01%) ⬆️
Py-3.10 98.12% <100.00%> (+<0.01%) ⬆️
Py-3.11 98.35% <100.00%> (+<0.01%) ⬆️
Py-3.12 98.44% <100.00%> (-0.01%) ⬇️
Py-3.13 98.43% <100.00%> (+<0.01%) ⬆️
Py-3.14 98.46% <100.00%> (+<0.01%) ⬆️
Py-3.14t 97.83% <100.00%> (-0.01%) ⬇️
Py-pypy-3.11 97.39% <100.00%> (-0.01%) ⬇️
VM-macos 98.19% <100.00%> (+<0.01%) ⬆️
VM-ubuntu 98.70% <100.00%> (-0.01%) ⬇️
VM-windows 97.31% <100.00%> (+<0.01%) ⬆️
cython-coverage 83.17% <30.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@codspeed-hq

codspeed-hq Bot commented Aug 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 97 untouched benchmarks
⏩ 83 skipped benchmarks1


Comparing dylanpulver:fix-ip-address-cookies (389b257) with master (9e08ba0)

Open in CodSpeed

Footnotes

  1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@noqt noqt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Disclosure: I work on NOQT's Lumi Trace. I ran Lumi Trace 0.10.0 against this exact head (d0774dcaa124194febfbeed5bcc3d62f11f8d8f9); it ranked CookieJar.__init__ first, then I checked the behavior-facing docs.

One TestClient example still teaches the flag this PR makes a no-op:

CookieJar(unsafe=True, treat_as_secure_origin="http://127.0.0.1")

That's at docs/testing.rst:671-674. Since treat_as_secure_origin is the remaining useful part, this should read CookieJar(treat_as_secure_origin="http://127.0.0.1"). Otherwise the 4.0 docs conflict: client_reference.rst says unsafe is ignored while the testing guide keeps recommending it.

The Lumi evidence bundle verified locally, and the exact PR checkout remained clean.

dylanpulver and others added 2 commits September 4, 2026 15:00
The testing guide still told readers to build a jar with
CookieJar(unsafe=True, treat_as_secure_origin=...), and TestClient itself
still defaulted to CookieJar(unsafe=True), while client_reference now
documents unsafe as having no effect. treat_as_secure_origin is the part
that still does something, so keep only that.

Both call sites are inert once cookies from an address origin are stored
host-only, so this changes no behaviour.

Signed-off-by: Dylan Pulver <dylanpulver@users.noreply.github.com>
Resolves the conflict in aiohttp/cookiejar.py. Upstream aio-libs#13674 widened
_host_only_cookies from (domain, name) to (domain, path, name) and moved
the host-only block below the path computation, which is where the two
sides diverged.

The three hunks of this branch were re-applied onto master's ordering
rather than merged line by line. The IP scoping block now sits after the
leading-dot strip instead of before it, so the lstrip(".") calls are no
longer needed and were dropped. Behaviour is unchanged: an IP origin
falls through with an empty domain and is picked up by master's
host-only branch, which is what this branch wanted in the first place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClientSession cookies does not work with IP

2 participants