Skip to content

feat(cookie): expose storage host in Jar::get_all - #1269

Open
sqdshguy wants to merge 5 commits into
0x676e67:mainfrom
sqdshguy:feat/jar-get-all-scoped
Open

feat(cookie): expose storage host in Jar::get_all#1269
sqdshguy wants to merge 5 commits into
0x676e67:mainfrom
sqdshguy:feat/jar-get-all-scoped

Conversation

@sqdshguy

Copy link
Copy Markdown
Contributor

get_all() loses the origin host of host-only cookies. It has to leave Domain absent so re-importing doesn't widen the scope, and the host itself only exists as the store's map key, so a snapshot can't be put back where it came from. The existing jar_get_all_export_import_keeps_host_only_scope_and_effective_path test hardcodes the origin URI for that reason.

get_all_scoped() returns the same cookies plus domain() and host_only(). domain() is in URI authority form, so it can be passed straight back to add. get_all() is now a map over it and behaves exactly as before.

Three tests added. cargo test --lib --features cookies and both doctests pass, cargo +nightly fmt --check and clippy are clean.

I hit this adding jar import to wreq-js: domain cookies restore, host-only ones can't.

@0x676e67

Copy link
Copy Markdown
Owner

wreq/src/cookie.rs

Lines 259 to 266 in 18436d1

if cookie.domain().is_none() {
cookie.set_domain(domain.to_owned());
}
if cookie.path().is_none() {
cookie.set_path(path.to_owned());
}

This appears to be a regression after refactoring. This issue was supposedly fixed before, but the code got reverted. Could we restore this piece of code implemented in 6.0.0‑rc29?

@sqdshguy

Copy link
Copy Markdown
Contributor Author

I had a look at the history on this. As far as I can tell that backfill changed in #1216, which swapped jar_get_all_backfills_domain_and_path for tests asserting the opposite, so restoring it fails four of them.

I think the problem is that writing the domain onto a host-only cookie widens its scope. If I export a cookie set for example.com and import it into a fresh jar, it starts going to api.example.com too:

source   -> api.example.com: Empty
restored -> api.example.com: Compressed("session=abc")

Path looks fine already, add() stores the effective path when the cookie goes in.

I might be missing what you actually need here though. What should get_all() be giving back, and what are you using it for?

@0x676e67

Copy link
Copy Markdown
Owner

In my use case, get_all is not actually required. Some users need it, and that’s why it exists.

@sqdshguy

Copy link
Copy Markdown
Contributor Author

So should I keep get_all_scoped() with the host separate, or put the rc.29 backfill back in get_all() and drop the tests blocking it? The backfill is what causes the subdomain leak I mentioned above.

@0x676e67

Copy link
Copy Markdown
Owner

A new host method has been added to Cookie to resolve this issue. The API documentation comments for this method still need improvements.

@0x676e67 0x676e67 changed the title feat(cookie): add Jar::get_all_scoped to report stored cookie scope feat(cookie): expose storage host in Jar::get_all Aug 31, 2026
@0x676e67 0x676e67 changed the title feat(cookie): expose storage host in Jar::get_all feat(cookie): expose storage host in Jar::get_all Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants