This is an automated email from the ASF dual-hosted git repository. rzo1 pushed a commit to branch fix/cookie-host-scope in repository https://gitbox.apache.org/repos/asf/stormcrawler.git
commit 25836d63e15c88c7912e0a9ad36a7b8885f027bd Author: Richard Zowalla <[email protected]> AuthorDate: Thu Aug 27 14:20:10 2026 +0200 Scope cookies to the host that set them CookieConverter only checked the domain when the cookie carried a Domain attribute, so a cookie without one was sent to any target URL, a single label Domain such as "com" matched every host under it, and checkDomainMatchToUrl returned true when it threw. getCookies now takes the originating URL, keeps a cookie without a Domain attribute for that host only, rejects single label domains and fails closed on error. Behaviour change: the protocols do not record the host that set a cookie, so they pass no origin and cookies without a Domain attribute are no longer sent. The metadata.transfer example in internals.adoc named set-cookie instead of protocol.set-cookie and is corrected too. --- .../protocol/httpclient/HttpProtocol.java | 4 +- .../stormcrawler/protocol/okhttp/HttpProtocol.java | 4 +- .../apache/stormcrawler/util/CookieConverter.java | 53 ++++++++- .../stormcrawler/util/CookieConverterTest.java | 120 ++++++++++++++++++--- docs/src/main/asciidoc/internals.adoc | 2 +- 5 files changed, 164 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java b/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java index 6c4a137d..12d7118e 100644 --- a/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java +++ b/core/src/main/java/org/apache/stormcrawler/protocol/httpclient/HttpProtocol.java @@ -291,7 +291,9 @@ public class HttpProtocol extends AbstractHttpProtocol if (cookieStrings != null && cookieStrings.length > 0) { List<Cookie> cookies; try { - cookies = CookieConverter.getCookies(cookieStrings, request.getURI().toURL()); + // the host whose response set the cookies is not kept in the metadata, + // hence the null origin: cookies without a domain attribute are dropped + cookies = CookieConverter.getCookies(cookieStrings, null, request.getURI().toURL()); for (Cookie c : cookies) { request.addHeader("Cookie", c.getName() + "=" + c.getValue()); } diff --git a/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java b/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java index ba60d7f9..49716a14 100644 --- a/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java +++ b/core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java @@ -286,8 +286,10 @@ public class HttpProtocol extends AbstractHttpProtocol { return; } try { + // the host whose response set the cookies is not kept in the metadata, + // hence the null origin: cookies without a domain attribute are dropped final List<Cookie> cookies = - CookieConverter.getCookies(cookieStrings, URLUtil.toURL(url)); + CookieConverter.getCookies(cookieStrings, null, URLUtil.toURL(url)); for (Cookie c : cookies) { rb.addHeader("Cookie", c.getName() + "=" + c.getValue()); } diff --git a/core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java b/core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java index 5592fd50..ef294436 100644 --- a/core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java +++ b/core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java @@ -33,13 +33,35 @@ public class CookieConverter { /** * Get a list of cookies based on the cookies string taken from response header and the target - * url. + * url. As the host which set the cookies is unknown, cookies without a domain attribute are + * dropped instead of being sent to the target url. * * @param cookiesStrings the value(s) of the http header for "Cookie" in the http response. * @param targetURL the url for which we wish to pass the cookies in the request. * @return List off cookies to add to the request. + * @deprecated use {@link #getCookies(String[], URL, URL)} instead */ + @Deprecated public static List<Cookie> getCookies(String[] cookiesStrings, URL targetURL) { + return getCookies(cookiesStrings, null, targetURL); + } + + /** + * Get a list of cookies based on the cookies string taken from response header, the url which + * set them and the target url. + * + * <p>A cookie without a domain attribute is only returned when the target url has the same host + * as the url which set the cookie, as required by RFC 6265. A cookie with a domain attribute is + * only returned when both urls match that domain. No public suffix list is used: a domain made + * of a single label such as "com" is rejected, but multi label suffixes such as "co.uk" are + * not. + * + * @param cookiesStrings the value(s) of the http header for "Cookie" in the http response. + * @param originURL the url whose response set the cookies, or null when it is unknown. + * @param targetURL the url for which we wish to pass the cookies in the request. + * @return List off cookies to add to the request. + */ + public static List<Cookie> getCookies(String[] cookiesStrings, URL originURL, URL targetURL) { ArrayList<Cookie> list = new ArrayList<>(); for (String cs : cookiesStrings) { @@ -78,11 +100,28 @@ public class CookieConverter { // check domain if (domain != null) { + // a domain made of a single label covers every host under that + // suffix and is not usable as a scope + if (isSingleLabel(domain)) { + continue; + } + cookie.setDomain(domain); if (!checkDomainMatchToUrl(domain, targetURL.getHost())) { continue; } + + // the host which set the cookie must be covered by the domain too + if (originURL != null && !checkDomainMatchToUrl(domain, originURL.getHost())) { + continue; + } + } else { + // host only cookie: valid for the host which set it and nothing else + if (originURL == null + || !originURL.getHost().equalsIgnoreCase(targetURL.getHost())) { + continue; + } } // check path @@ -129,6 +168,16 @@ public class CookieConverter { return list; } + /** Checks whether a cookie domain is made of a single label, e.g. "com". */ + private static boolean isSingleLabel(String domain) { + String d = domain; + if (d.startsWith(".")) { + d = d.substring(1); + } + int dot = d.indexOf('.'); + return dot < 1 || dot == d.length() - 1; + } + /** * Helper method to check if url matches a cookie domain. * @@ -156,7 +205,7 @@ public class CookieConverter { } return true; } catch (Exception e) { - return true; + return false; } } } diff --git a/core/src/test/java/org/apache/stormcrawler/util/CookieConverterTest.java b/core/src/test/java/org/apache/stormcrawler/util/CookieConverterTest.java index 57b95f45..b9111240 100644 --- a/core/src/test/java/org/apache/stormcrawler/util/CookieConverterTest.java +++ b/core/src/test/java/org/apache/stormcrawler/util/CookieConverterTest.java @@ -40,7 +40,9 @@ class CookieConverterTest { String dummyCookieString = buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, null, null); cookiesStrings[0] = dummyCookieString; - List<Cookie> result = CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl)); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl)); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -60,7 +62,9 @@ class CookieConverterTest { null, null); cookiesStrings[0] = dummyCookieString; - List<Cookie> result = CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl)); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl)); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -80,7 +84,9 @@ class CookieConverterTest { null, null); cookiesStrings[0] = dummyCookieString; - List<Cookie> result = CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl)); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl)); Assertions.assertEquals( 0, result.size(), "Should have 0 cookies, since cookie was expired"); } @@ -92,7 +98,8 @@ class CookieConverterTest { buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, "/", null); cookiesStrings[0] = dummyCookieString; List<Cookie> result = - CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl + "/somepage")); + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl + "/somepage")); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -106,7 +113,9 @@ class CookieConverterTest { String dummyCookieString = buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, "/", null); cookiesStrings[0] = dummyCookieString; - List<Cookie> result = CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl)); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl)); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -122,7 +131,8 @@ class CookieConverterTest { dummyCookieHeader, dummyCookieValue, null, null, "/someFolder", null); cookiesStrings[0] = dummyCookieString; List<Cookie> result = - CookieConverter.getCookies(cookiesStrings, getUrl(unsecuredUrl + "/someFolder")); + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl + "/someFolder")); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -139,7 +149,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -156,7 +168,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(unsecuredUrl + "/someOtherFolder/SomeFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(unsecuredUrl + "/someOtherFolder/SomeFolder")); Assertions.assertEquals(0, result.size(), "path mismatch, should have 0 cookies"); } @@ -169,7 +183,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -186,7 +202,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(0, result.size(), "Domain is not valid - Should have 0 cookies"); } @@ -199,7 +217,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(unsecuredUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals( 0, result.size(), "Target url is not secured - Should have 0 cookies"); } @@ -213,7 +233,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(securedUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(securedUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(1, result.size(), "Target url is secured - Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -235,7 +257,9 @@ class CookieConverterTest { cookiesStrings[0] = dummyCookieString; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(securedUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(securedUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -266,7 +290,9 @@ class CookieConverterTest { cookiesStrings[1] = dummyCookieString2; List<Cookie> result = CookieConverter.getCookies( - cookiesStrings, getUrl(securedUrl + "/someFolder/SomeOtherFolder")); + cookiesStrings, + getUrl(unsecuredUrl), + getUrl(securedUrl + "/someFolder/SomeOtherFolder")); Assertions.assertEquals(2, result.size(), "Should have 2 cookies"); Assertions.assertEquals( dummyCookieHeader, result.get(0).getName(), "Cookie header should be as defined"); @@ -282,6 +308,66 @@ class CookieConverterTest { "Cookie value should be as defined"); } + @Test + void cookieWithoutDomainIsSentToSameHost() { + String[] cookiesStrings = new String[1]; + cookiesStrings[0] = + buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, null, null); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl + "/somepage")); + Assertions.assertEquals(1, result.size(), "Should have 1 cookie"); + } + + @Test + void cookieWithoutDomainIsNotSentToOtherHost() { + String[] cookiesStrings = new String[1]; + cookiesStrings[0] = + buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, null, null); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl("http://someotherurl.com";)); + Assertions.assertEquals( + 0, result.size(), "Cookie without domain is bound to the host which set it"); + } + + @Test + void cookieWithoutDomainIsNotSentWhenOriginIsUnknown() { + String[] cookiesStrings = new String[1]; + cookiesStrings[0] = + buildCookieString(dummyCookieHeader, dummyCookieValue, null, null, null, null); + List<Cookie> result = + CookieConverter.getCookies(cookiesStrings, null, getUrl(unsecuredUrl)); + Assertions.assertEquals( + 0, result.size(), "Cookie without domain needs the host which set it"); + } + + @Test + void cookieWithSingleLabelDomainIsNotSent() { + String[] cookiesStrings = new String[1]; + cookiesStrings[0] = + buildCookieString(dummyCookieHeader, dummyCookieValue, "com", null, null, null); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, getUrl(unsecuredUrl), getUrl(unsecuredUrl)); + Assertions.assertEquals(0, result.size(), "Domain com must not match someurl.com"); + } + + @Test + void cookieWithDomainIsNotSentWhenOriginIsOutsideTheDomain() { + String[] cookiesStrings = new String[1]; + cookiesStrings[0] = + buildCookieString( + dummyCookieHeader, dummyCookieValue, "someurl.com", null, null, null); + List<Cookie> result = + CookieConverter.getCookies( + cookiesStrings, + getUrl("http://someotherurl.com";), + getUrl(unsecuredUrl + "/somepage")); + Assertions.assertEquals( + 0, result.size(), "Domain does not cover the host which set the cookie"); + } + @Test void testDomainsChecker() { boolean result = CookieConverter.checkDomainMatchToUrl(".example.com", "www.example.com"); @@ -306,6 +392,12 @@ class CookieConverterTest { Assertions.assertFalse(result, "domain is not valid"); } + @Test + void testDomainsChecker5() { + boolean result = CookieConverter.checkDomainMatchToUrl("example.com", null); + Assertions.assertFalse(result, "domain can not be checked"); + } + private URL getUrl(String urlString) { try { return URLUtil.toURL(urlString); diff --git a/docs/src/main/asciidoc/internals.adoc b/docs/src/main/asciidoc/internals.adoc index 262e7c07..84edac88 100644 --- a/docs/src/main/asciidoc/internals.adoc +++ b/docs/src/main/asciidoc/internals.adoc @@ -520,5 +520,5 @@ metadata.persist: [source,yaml] ---- metadata.transfer: - - set-cookie + - protocol.set-cookie ----
