Skip to content

Scope cookies to the host that set them - #2075

Open
rzo1 wants to merge 1 commit into
mainfrom
fix/cookie-host-scope
Open

Scope cookies to the host that set them#2075
rzo1 wants to merge 1 commit into
mainfrom
fix/cookie-host-scope

Conversation

@rzo1

@rzo1 rzo1 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

CookieConverter only checked applicability for cookies carrying a Domain attribute. A cookie without one, which is the usual session cookie, skipped the check and was sent to whatever host was being fetched. The domain check also passed on exception and accepted a Domain covering every host under it, such as com or co.uk.

A three-argument getCookies taking the originating URL is added; the two-argument method is deprecated and kept. A Domain attribute is now normalised with IDN.toASCII and validated before use, so the unicode and punycode forms of a name are interchangeable and a malformed value such as com.. is rejected instead of matching every host under com. The public suffix list already shipped with crawler-commons, private section included, decides whether a domain may cover subdomains at all; a domain the list does not know about, e.g. an internal name under .local, falls back to the plain suffix match so unlisted domains keep working. A domain which cannot own subdomains is ignored and the cookie is bound to the host, per RFC 6265 5.3. An address is matched only by itself, per RFC 6265 5.1.3, where Domain=2.3.4 previously covered 9.2.3.4.

Two smaller ones found on the way: a Set-Cookie header whose first token has no = threw StringIndexOutOfBoundsException out of getCookies and failed the whole fetch, and is now skipped; cookies dropped on the path, secure and expiry checks are logged like the other ones.

The protocols record which host set the cookies. When a response carries Set-Cookie, okhttp and playwright write its URL alongside as set-cookie-origin, dropping any inbound key of that name first so a response cannot choose where its cookies are sent. That key has to travel with protocol.set-cookie for host-only cookies to be sent back, so it is added to the metadata.persist and metadata.transfer examples and a warning naming both keys is logged once when cookies arrive without it. Also corrects the metadata.transfer example in internals.adoc, which named set-cookie where the key is protocol.set-cookie.

For all changes

  • Is there a issue associated with this PR? Is it referenced in the commit message? - no issue

  • Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve? - no issue

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit?

  • Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

  • Have you ensured that the full suite of tests is executed via mvn clean verify? - core module tests only
  • Have you written or updated unit tests to verify your changes? - CookieConverterTest extended
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0? - no new dependencies
  • If applicable, have you updated the LICENSE file, including the main LICENSE file? - n/a
  • If applicable, have you updated the NOTICE file, including the main NOTICE file? - n/a

@rzo1 rzo1 added this to the 4.0.0 milestone Aug 27, 2026
@rzo1
rzo1 force-pushed the fix/cookie-host-scope branch from 25836d6 to fd5035f Compare August 27, 2026 12:51

@dpol1 dpol1 left a comment

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.

Two things before merging: the new domain guard is bypassable, and the null origin disables every host-only cookie (no Domain, in practice most session cookies) for anyone running http.use.cookies. Threads.

Side note on "both protocol implementations": httpclient went with #2071, only okhttp calls this now.

Comment thread core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java Outdated
Comment thread core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java Outdated
@rzo1 rzo1 unassigned jnioche and dpol1 Aug 28, 2026
@rzo1
rzo1 requested a review from jnioche August 28, 2026 07:28
@rzo1 rzo1 self-assigned this Aug 28, 2026
@rzo1
rzo1 force-pushed the fix/cookie-host-scope branch from fd5035f to 15e9338 Compare August 28, 2026 08:10
CookieConverter only checked the domain when the cookie carried a Domain
attribute, so a cookie without one was sent to any target URL, a Domain
which covers every host under it such as "com" or "co.uk" was accepted as
a scope, and checkDomainMatchToUrl returned true when it threw. getCookies
now takes the URL whose response set the cookies, keeps a cookie without a
Domain attribute for that host only, and fails closed on error.

A Domain attribute is normalised with IDN.toASCII and validated before it
is used, so the unicode and punycode forms of a name are interchangeable
and a malformed value such as "com.." is rejected rather than matching
every host under "com". The public suffix list shipped with
crawler-commons, its private section included, decides whether a domain
may cover subdomains at all; a domain the list does not know about, e.g.
an internal name under ".local", falls back to the plain suffix match so
that unlisted domains keep working. A domain which can not own subdomains
is ignored and the cookie is bound to the host, as RFC 6265 5.3 requires,
so a single label intranet domain keeps its cookies while "com" no longer
scopes anything. An address is only matched by itself, per RFC 6265 5.1.3,
instead of covering the hosts under it as "2.3.4" covered "9.2.3.4".

The protocols now record which host set the cookies. When a response
carries Set-Cookie, okhttp and playwright write its url next to the header
as set-cookie-origin, dropping any key of that name from the response
first so that a response can not choose where its cookies are sent. The
key has to travel with protocol.set-cookie for host-only cookies to be
sent back, so it is added to the metadata.persist and metadata.transfer
examples, and a warning naming both keys is logged once when cookies are
present without it. The metadata.transfer example in internals.adoc named
set-cookie instead of protocol.set-cookie and is corrected too.

A Set-Cookie header whose first token has no "=" threw
StringIndexOutOfBoundsException out of getCookies, failing the whole
fetch, and is now skipped. Cookies dropped on the path, secure and expiry
checks are logged like the other ones.
@rzo1
rzo1 force-pushed the fix/cookie-host-scope branch from 15e9338 to 8f00119 Compare August 28, 2026 08:30
@rzo1
rzo1 requested a review from dpol1 August 28, 2026 08:30
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.

3 participants