HTTPCLIENT-2365, regression: corrected handling of private domains by PublicSuffixMatcher

ok2c · ok2c · commit 924b38fb1c48 · 2025-03-31T13:50:09.000+02:00
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/PublicSuffixDomainFilter.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/PublicSuffixDomainFilter.java
@@ -39,6 +39,8 @@
 import org.apache.hc.core5.annotation.Contract;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 import org.apache.hc.core5.util.Args;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Wraps a {@link org.apache.hc.client5.http.cookie.CookieAttributeHandler} and leverages
@@ -54,6 +56,8 @@
 @Contract(threading = ThreadingBehavior.STATELESS)
 public class PublicSuffixDomainFilter implements CommonCookieAttributeHandler {
 
+    private static final Logger LOG = LoggerFactory.getLogger(PublicSuffixDomainFilter.class);
+
     private final CommonCookieAttributeHandler handler;
     private final PublicSuffixMatcher publicSuffixMatcher;
     private final Map<String, Boolean> localDomainMap;
@@ -84,6 +88,17 @@ public PublicSuffixDomainFilter(
         this.localDomainMap = createLocalDomainMap();
     }
 
+    private boolean verifyHost(final String host) {
+        if (this.publicSuffixMatcher.verify(host)) {
+            return true;
+        } else {
+            if (LOG.isDebugEnabled()) {
+                LOG.debug("Public Suffix List verification failed for host '{}'", host);
+            }
+            return false;
+        }
+    }
+
     /**
      * Never matches if the cookie's domain is from the blacklist.
      */
@@ -97,13 +112,13 @@ public boolean match(final Cookie cookie, final CookieOrigin origin) {
         if (i >= 0) {
             final String domain = host.substring(i);
             if (!this.localDomainMap.containsKey(domain)) {
-                if (this.publicSuffixMatcher.matches(host)) {
+                if (!verifyHost(host)) {
                     return false;
                 }
             }
         } else {
             if (!host.equalsIgnoreCase(origin.getHost())) {
-                if (this.publicSuffixMatcher.matches(host)) {
+                if (!verifyHost(host)) {
                     return false;
                 }
             }
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/psl/PublicSuffixMatcher.java b/httpclient5/src/main/java/org/apache/hc/client5/http/psl/PublicSuffixMatcher.java
@@ -34,6 +34,7 @@
 
 import org.apache.hc.client5.http.utils.DnsUtils;
 import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.Internal;
 import org.apache.hc.core5.annotation.ThreadingBehavior;
 import org.apache.hc.core5.util.Args;
 
@@ -137,21 +138,36 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
         if (domain.startsWith(".")) {
             return null;
         }
+        final DomainRootInfo match = resolveDomainRoot(domain, expectedType);
+        return match != null ? match.root : null;
+    }
+
+    static final class DomainRootInfo {
+
+        final String root;
+        final String matchingKey;
+        final DomainType domainType;
+
+        DomainRootInfo(final String root, final String matchingKey, final DomainType domainType) {
+            this.root = root;
+            this.matchingKey = matchingKey;
+            this.domainType = domainType;
+        }
+    }
+
+    DomainRootInfo resolveDomainRoot(final String domain, final DomainType expectedType) {
         String segment = DnsUtils.normalize(domain);
         String result = null;
         while (segment != null) {
             // An exception rule takes priority over any other matching rule.
             final String key = IDN.toUnicode(segment);
             final DomainType exceptionRule = findEntry(exceptions, key);
             if (match(exceptionRule, expectedType)) {
-                return segment;
+                return new DomainRootInfo(segment, key, exceptionRule);
             }
             final DomainType domainRule = findEntry(rules, key);
             if (match(domainRule, expectedType)) {
-                // Prior to version 5.4 the result for "private" rules was different. However, the
-                // PSL algorithm doesn't have any rules changing the result based on "domain type"
-                // see https://github.com/publicsuffix/list/wiki/Format#formal-algorithm
-                return result;
+                return new DomainRootInfo(result, key, domainRule);
             }
 
             final int nextdot = segment.indexOf('.');
@@ -161,15 +177,15 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
             final String wildcardKey = (nextSegment == null) ? "*" : "*." + IDN.toUnicode(nextSegment);
             final DomainType wildcardDomainRule = findEntry(rules, wildcardKey);
             if (match(wildcardDomainRule, expectedType)) {
-                return result;
+                return new DomainRootInfo(result, wildcardKey, wildcardDomainRule);
             }
 
             // If we're out of segments, and we're not looking for a specific type of entry,
             // apply the default `*` rule.
             // This wildcard rule means any final segment in a domain is a public suffix,
             // so the current `result` is the desired public suffix plus 1
             if (nextSegment == null && (expectedType == null || expectedType == DomainType.UNKNOWN)) {
-                return result;
+                return new DomainRootInfo(result, null, null);
             }
 
             result = segment;
@@ -178,7 +194,7 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
 
         // If no expectations then this result is good.
         if (expectedType == null || expectedType == DomainType.UNKNOWN) {
-            return result;
+            return new DomainRootInfo(result, null, null);
         }
 
         // If we did have expectations apparently there was no match
@@ -210,4 +226,38 @@ public boolean matches(final String domain, final DomainType expectedType) {
         return domainRoot == null;
     }
 
+    /**
+     * Verifies if the given domain does not represent a public domain root and is
+     * allowed to set cookies, have an identity represented by a certificate, etc.
+     * <p>
+     * This method tolerates a leading dot in the domain name, for example '.example.com'
+     * which will be automatically stripped.
+     * </p>
+     */
+     @Internal
+     public boolean verify(final String domain) {
+         if (domain == null) {
+             return false;
+         }
+         return verifyStrict(domain.startsWith(".") ? domain.substring(1) : domain);
+     }
+
+    /**
+     * Verifies if the given domain does not represent a public domain root and is
+     * allowed to set cookies, have an identity represented by a certificate, etc.
+     */
+    @Internal
+    public boolean verifyStrict(final String domain) {
+        Args.notNull(domain, "Domain");
+        if (domain.startsWith(".")) {
+            return false;
+        }
+        final DomainRootInfo domainRootInfo = resolveDomainRoot(domain, null);
+        if (domainRootInfo == null) {
+            return false;
+        }
+        return domainRootInfo.root != null ||
+                (domainRootInfo.domainType == DomainType.PRIVATE && domainRootInfo.matchingKey != null);
+    }
+
 }
diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
@@ -44,7 +44,6 @@
 import javax.net.ssl.SSLSession;
 import javax.security.auth.x500.X500Principal;
 
-import org.apache.hc.client5.http.psl.DomainType;
 import org.apache.hc.client5.http.psl.PublicSuffixMatcher;
 import org.apache.hc.client5.http.utils.DnsUtils;
 import org.apache.hc.core5.annotation.Contract;
@@ -227,7 +226,6 @@ static boolean matchDomainRoot(final String host, final String domainRoot) {
 
     private static boolean matchIdentity(final String host, final String identity,
                                          final PublicSuffixMatcher publicSuffixMatcher,
-                                         final DomainType domainType,
                                          final boolean strict) {
 
         final String normalizedIdentity;
@@ -240,7 +238,10 @@ private static boolean matchIdentity(final String host, final String identity,
 
         // Public suffix check on the Unicode identity
         if (publicSuffixMatcher != null && host.contains(".")) {
-            if (publicSuffixMatcher.getDomainRoot(normalizedIdentity, domainType) == null) {
+            if (!publicSuffixMatcher.verifyStrict(normalizedIdentity)) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Public Suffix List verification failed for identity '{}'", identity);
+                }
                 return false;
             }
         }
@@ -278,32 +279,20 @@ private static boolean matchIdentity(final String host, final String identity,
 
     static boolean matchIdentity(final String host, final String identity,
                                  final PublicSuffixMatcher publicSuffixMatcher) {
-        return matchIdentity(host, identity, publicSuffixMatcher, null, false);
+        return matchIdentity(host, identity, publicSuffixMatcher, false);
     }
 
     static boolean matchIdentity(final String host, final String identity) {
-        return matchIdentity(host, identity, null, null, false);
+        return matchIdentity(host, identity, null, false);
     }
 
     static boolean matchIdentityStrict(final String host, final String identity,
                                        final PublicSuffixMatcher publicSuffixMatcher) {
-        return matchIdentity(host, identity, publicSuffixMatcher, null, true);
+        return matchIdentity(host, identity, publicSuffixMatcher, true);
     }
 
     static boolean matchIdentityStrict(final String host, final String identity) {
-        return matchIdentity(host, identity, null, null, true);
-    }
-
-    static boolean matchIdentity(final String host, final String identity,
-                                 final PublicSuffixMatcher publicSuffixMatcher,
-                                 final DomainType domainType) {
-        return matchIdentity(host, identity, publicSuffixMatcher, domainType, false);
-    }
-
-    static boolean matchIdentityStrict(final String host, final String identity,
-                                       final PublicSuffixMatcher publicSuffixMatcher,
-                                       final DomainType domainType) {
-        return matchIdentity(host, identity, publicSuffixMatcher, domainType, true);
+        return matchIdentity(host, identity, null, true);
     }
 
     static String extractCN(final String subjectPrincipal) throws SSLException {
diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java b/httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java
@@ -43,12 +43,6 @@ class TestPublicSuffixMatcher {
     private PublicSuffixMatcher matcher;
     private PublicSuffixMatcher pslMatcher;
 
-    /**
-     * Create a matcher using the public suffix list file provided by publicsuffix.org (Mozilla).
-     *
-     * This test uses a copy of https://publicsuffix.org/list/effective_tld_names.dat in
-     * src/main/resources/org/publicsuffix/list/effective_tld_names.dat
-     */
     @BeforeEach
     void setUp() throws Exception {
         final ClassLoader classLoader = getClass().getClassLoader();
@@ -58,6 +52,7 @@ void setUp() throws Exception {
             final List<PublicSuffixList> lists = PublicSuffixListParser.INSTANCE.parseByType(new InputStreamReader(in, StandardCharsets.UTF_8));
             matcher = new PublicSuffixMatcher(lists);
         }
+        // Create a matcher using the public suffix list file provided by publicsuffix.org (Mozilla).
         pslMatcher = PublicSuffixMatcherLoader.getDefault();
     }
 
@@ -87,6 +82,8 @@ void testGetDomainRootAnyType() {
         Assertions.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX"));
         Assertions.assertEquals(null, matcher.getDomainRoot("appspot.com"));
         Assertions.assertEquals("example.appspot.com", matcher.getDomainRoot("example.appspot.com"));
+        Assertions.assertEquals(null, matcher.getDomainRoot("s3.amazonaws.com"));
+        Assertions.assertEquals(null, matcher.getDomainRoot("blah.s3.amazonaws.com"));
         // Too short
         Assertions.assertNull(matcher.getDomainRoot("jp"));
         Assertions.assertNull(matcher.getDomainRoot("ac.jp"));
@@ -122,6 +119,8 @@ void testGetDomainRootOnlyPRIVATE() {
         Assertions.assertNull(matcher.getDomainRoot("garbage.garbage", DomainType.PRIVATE));
         Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage", DomainType.PRIVATE));
         Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage.garbage", DomainType.PRIVATE));
+        Assertions.assertNull(matcher.getDomainRoot("s3.amazonaws.com"));
+        Assertions.assertNull(matcher.getDomainRoot("blah.s3.amazonaws.com"));
     }
 
     @Test
@@ -146,6 +145,33 @@ void testGetDomainRootOnlyICANN() {
         Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage.garbage", DomainType.ICANN));
     }
 
+    @Test
+    void testMaySetCookies() {
+        Assertions.assertTrue(matcher.verify("foo.com"));
+
+        Assertions.assertFalse(matcher.verify("bar.foo.com"));
+        Assertions.assertTrue(matcher.verify("example.bar.foo.com"));
+
+        Assertions.assertTrue(matcher.verify("foo.bar.jp"));
+        Assertions.assertFalse(matcher.verify("bar.jp"));
+
+        Assertions.assertTrue(matcher.verify("foo.bar.hokkaido.jp"));
+        Assertions.assertFalse(matcher.verify("bar.hokkaido.jp"));
+
+        Assertions.assertTrue(matcher.verify("foo.bar.tokyo.jp"));
+        Assertions.assertFalse(matcher.verify("bar.tokyo.jp"));
+
+        Assertions.assertTrue(matcher.verify("pref.hokkaido.jp")); // exception from a wildcard rule
+        Assertions.assertTrue(matcher.verify("metro.tokyo.jp")); // exception from a wildcard rule
+    }
+
+    @Test
+    void testVerifyPrivate() {
+        Assertions.assertTrue(matcher.verify("s3.amazonaws.com"));
+        Assertions.assertTrue(matcher.verify("blah.s3.amazonaws.com"));
+        Assertions.assertTrue(matcher.verify("blah.xxx.uk"));
+    }
+
     @Test
     void testMatch() {
         Assertions.assertTrue(matcher.matches(".jp"));
diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestDefaultHostnameVerifier.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestDefaultHostnameVerifier.java
diff --git a/httpclient5/src/test/resources/suffixlistmatcher.txt b/httpclient5/src/test/resources/suffixlistmatcher.txt
