Skip to content

Commit 924b38f

Browse files
ok2cok2c
committed
HTTPCLIENT-2365, regression: corrected handling of private domains by PublicSuffixMatcher
1 parent 93a87be commit 924b38f

File tree

6 files changed

+134
-108
lines changed

6 files changed

+134
-108
lines changed

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/PublicSuffixDomainFilter.java‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@
3939
import org.apache.hc.core5.annotation.Contract;
4040
import org.apache.hc.core5.annotation.ThreadingBehavior;
4141
import org.apache.hc.core5.util.Args;
42+
import org.slf4j.Logger;
43+
import org.slf4j.LoggerFactory;
4244

4345
/**
4446
* Wraps a {@link org.apache.hc.client5.http.cookie.CookieAttributeHandler} and leverages
@@ -54,6 +56,8 @@
5456
@Contract(threading = ThreadingBehavior.STATELESS)
5557
public class PublicSuffixDomainFilter implements CommonCookieAttributeHandler {
5658

59+
private static final Logger LOG = LoggerFactory.getLogger(PublicSuffixDomainFilter.class);
60+
5761
private final CommonCookieAttributeHandler handler;
5862
private final PublicSuffixMatcher publicSuffixMatcher;
5963
private final Map<String, Boolean> localDomainMap;
@@ -84,6 +88,17 @@ public PublicSuffixDomainFilter(
8488
this.localDomainMap = createLocalDomainMap();
8589
}
8690

91+
private boolean verifyHost(final String host) {
92+
if (this.publicSuffixMatcher.verify(host)) {
93+
return true;
94+
} else {
95+
if (LOG.isDebugEnabled()) {
96+
LOG.debug("Public Suffix List verification failed for host '{}'", host);
97+
}
98+
return false;
99+
}
100+
}
101+
87102
/**
88103
* Never matches if the cookie's domain is from the blacklist.
89104
*/
@@ -97,13 +112,13 @@ public boolean match(final Cookie cookie, final CookieOrigin origin) {
97112
if (i >= 0) {
98113
final String domain = host.substring(i);
99114
if (!this.localDomainMap.containsKey(domain)) {
100-
if (this.publicSuffixMatcher.matches(host)) {
115+
if (!verifyHost(host)) {
101116
return false;
102117
}
103118
}
104119
} else {
105120
if (!host.equalsIgnoreCase(origin.getHost())) {
106-
if (this.publicSuffixMatcher.matches(host)) {
121+
if (!verifyHost(host)) {
107122
return false;
108123
}
109124
}

‎httpclient5/src/main/java/org/apache/hc/client5/http/psl/PublicSuffixMatcher.java‎

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434

3535
import org.apache.hc.client5.http.utils.DnsUtils;
3636
import org.apache.hc.core5.annotation.Contract;
37+
import org.apache.hc.core5.annotation.Internal;
3738
import org.apache.hc.core5.annotation.ThreadingBehavior;
3839
import org.apache.hc.core5.util.Args;
3940

@@ -137,21 +138,36 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
137138
if (domain.startsWith(".")) {
138139
return null;
139140
}
141+
final DomainRootInfo match = resolveDomainRoot(domain, expectedType);
142+
return match != null ? match.root : null;
143+
}
144+
145+
static final class DomainRootInfo {
146+
147+
final String root;
148+
final String matchingKey;
149+
final DomainType domainType;
150+
151+
DomainRootInfo(final String root, final String matchingKey, final DomainType domainType) {
152+
this.root = root;
153+
this.matchingKey = matchingKey;
154+
this.domainType = domainType;
155+
}
156+
}
157+
158+
DomainRootInfo resolveDomainRoot(final String domain, final DomainType expectedType) {
140159
String segment = DnsUtils.normalize(domain);
141160
String result = null;
142161
while (segment != null) {
143162
// An exception rule takes priority over any other matching rule.
144163
final String key = IDN.toUnicode(segment);
145164
final DomainType exceptionRule = findEntry(exceptions, key);
146165
if (match(exceptionRule, expectedType)) {
147-
return segment;
166+
return new DomainRootInfo(segment, key, exceptionRule);
148167
}
149168
final DomainType domainRule = findEntry(rules, key);
150169
if (match(domainRule, expectedType)) {
151-
// Prior to version 5.4 the result for "private" rules was different. However, the
152-
// PSL algorithm doesn't have any rules changing the result based on "domain type"
153-
// see https://github.com/publicsuffix/list/wiki/Format#formal-algorithm
154-
return result;
170+
return new DomainRootInfo(result, key, domainRule);
155171
}
156172

157173
final int nextdot = segment.indexOf('.');
@@ -161,15 +177,15 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
161177
final String wildcardKey = (nextSegment == null) ? "*" : "*." + IDN.toUnicode(nextSegment);
162178
final DomainType wildcardDomainRule = findEntry(rules, wildcardKey);
163179
if (match(wildcardDomainRule, expectedType)) {
164-
return result;
180+
return new DomainRootInfo(result, wildcardKey, wildcardDomainRule);
165181
}
166182

167183
// If we're out of segments, and we're not looking for a specific type of entry,
168184
// apply the default `*` rule.
169185
// This wildcard rule means any final segment in a domain is a public suffix,
170186
// so the current `result` is the desired public suffix plus 1
171187
if (nextSegment == null && (expectedType == null || expectedType == DomainType.UNKNOWN)) {
172-
return result;
188+
return new DomainRootInfo(result, null, null);
173189
}
174190

175191
result = segment;
@@ -178,7 +194,7 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
178194

179195
// If no expectations then this result is good.
180196
if (expectedType == null || expectedType == DomainType.UNKNOWN) {
181-
return result;
197+
return new DomainRootInfo(result, null, null);
182198
}
183199

184200
// If we did have expectations apparently there was no match
@@ -210,4 +226,38 @@ public boolean matches(final String domain, final DomainType expectedType) {
210226
return domainRoot == null;
211227
}
212228

229+
/**
230+
* Verifies if the given domain does not represent a public domain root and is
231+
* allowed to set cookies, have an identity represented by a certificate, etc.
232+
* <p>
233+
* This method tolerates a leading dot in the domain name, for example '.example.com'
234+
* which will be automatically stripped.
235+
* </p>
236+
*/
237+
@Internal
238+
public boolean verify(final String domain) {
239+
if (domain == null) {
240+
return false;
241+
}
242+
return verifyStrict(domain.startsWith(".") ? domain.substring(1) : domain);
243+
}
244+
245+
/**
246+
* Verifies if the given domain does not represent a public domain root and is
247+
* allowed to set cookies, have an identity represented by a certificate, etc.
248+
*/
249+
@Internal
250+
public boolean verifyStrict(final String domain) {
251+
Args.notNull(domain, "Domain");
252+
if (domain.startsWith(".")) {
253+
return false;
254+
}
255+
final DomainRootInfo domainRootInfo = resolveDomainRoot(domain, null);
256+
if (domainRootInfo == null) {
257+
return false;
258+
}
259+
return domainRootInfo.root != null ||
260+
(domainRootInfo.domainType == DomainType.PRIVATE && domainRootInfo.matchingKey != null);
261+
}
262+
213263
}

‎httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java‎

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
import javax.net.ssl.SSLSession;
4545
import javax.security.auth.x500.X500Principal;
4646

47-
import org.apache.hc.client5.http.psl.DomainType;
4847
import org.apache.hc.client5.http.psl.PublicSuffixMatcher;
4948
import org.apache.hc.client5.http.utils.DnsUtils;
5049
import org.apache.hc.core5.annotation.Contract;
@@ -227,7 +226,6 @@ static boolean matchDomainRoot(final String host, final String domainRoot) {
227226

228227
private static boolean matchIdentity(final String host, final String identity,
229228
final PublicSuffixMatcher publicSuffixMatcher,
230-
final DomainType domainType,
231229
final boolean strict) {
232230

233231
final String normalizedIdentity;
@@ -240,7 +238,10 @@ private static boolean matchIdentity(final String host, final String identity,
240238

241239
// Public suffix check on the Unicode identity
242240
if (publicSuffixMatcher != null && host.contains(".")) {
243-
if (publicSuffixMatcher.getDomainRoot(normalizedIdentity, domainType) == null) {
241+
if (!publicSuffixMatcher.verifyStrict(normalizedIdentity)) {
242+
if (LOG.isDebugEnabled()) {
243+
LOG.debug("Public Suffix List verification failed for identity '{}'", identity);
244+
}
244245
return false;
245246
}
246247
}
@@ -278,32 +279,20 @@ private static boolean matchIdentity(final String host, final String identity,
278279

279280
static boolean matchIdentity(final String host, final String identity,
280281
final PublicSuffixMatcher publicSuffixMatcher) {
281-
return matchIdentity(host, identity, publicSuffixMatcher, null, false);
282+
return matchIdentity(host, identity, publicSuffixMatcher, false);
282283
}
283284

284285
static boolean matchIdentity(final String host, final String identity) {
285-
return matchIdentity(host, identity, null, null, false);
286+
return matchIdentity(host, identity, null, false);
286287
}
287288

288289
static boolean matchIdentityStrict(final String host, final String identity,
289290
final PublicSuffixMatcher publicSuffixMatcher) {
290-
return matchIdentity(host, identity, publicSuffixMatcher, null, true);
291+
return matchIdentity(host, identity, publicSuffixMatcher, true);
291292
}
292293

293294
static boolean matchIdentityStrict(final String host, final String identity) {
294-
return matchIdentity(host, identity, null, null, true);
295-
}
296-
297-
static boolean matchIdentity(final String host, final String identity,
298-
final PublicSuffixMatcher publicSuffixMatcher,
299-
final DomainType domainType) {
300-
return matchIdentity(host, identity, publicSuffixMatcher, domainType, false);
301-
}
302-
303-
static boolean matchIdentityStrict(final String host, final String identity,
304-
final PublicSuffixMatcher publicSuffixMatcher,
305-
final DomainType domainType) {
306-
return matchIdentity(host, identity, publicSuffixMatcher, domainType, true);
295+
return matchIdentity(host, identity, null, true);
307296
}
308297

309298
static String extractCN(final String subjectPrincipal) throws SSLException {

‎httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,6 @@ class TestPublicSuffixMatcher {
4343
private PublicSuffixMatcher matcher;
4444
private PublicSuffixMatcher pslMatcher;
4545

46-
/**
47-
* Create a matcher using the public suffix list file provided by publicsuffix.org (Mozilla).
48-
*
49-
* This test uses a copy of https://publicsuffix.org/list/effective_tld_names.dat in
50-
* src/main/resources/org/publicsuffix/list/effective_tld_names.dat
51-
*/
5246
@BeforeEach
5347
void setUp() throws Exception {
5448
final ClassLoader classLoader = getClass().getClassLoader();
@@ -58,6 +52,7 @@ void setUp() throws Exception {
5852
final List<PublicSuffixList> lists = PublicSuffixListParser.INSTANCE.parseByType(new InputStreamReader(in, StandardCharsets.UTF_8));
5953
matcher = new PublicSuffixMatcher(lists);
6054
}
55+
// Create a matcher using the public suffix list file provided by publicsuffix.org (Mozilla).
6156
pslMatcher = PublicSuffixMatcherLoader.getDefault();
6257
}
6358

@@ -87,6 +82,8 @@ void testGetDomainRootAnyType() {
8782
Assertions.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX"));
8883
Assertions.assertEquals(null, matcher.getDomainRoot("appspot.com"));
8984
Assertions.assertEquals("example.appspot.com", matcher.getDomainRoot("example.appspot.com"));
85+
Assertions.assertEquals(null, matcher.getDomainRoot("s3.amazonaws.com"));
86+
Assertions.assertEquals(null, matcher.getDomainRoot("blah.s3.amazonaws.com"));
9087
// Too short
9188
Assertions.assertNull(matcher.getDomainRoot("jp"));
9289
Assertions.assertNull(matcher.getDomainRoot("ac.jp"));
@@ -122,6 +119,8 @@ void testGetDomainRootOnlyPRIVATE() {
122119
Assertions.assertNull(matcher.getDomainRoot("garbage.garbage", DomainType.PRIVATE));
123120
Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage", DomainType.PRIVATE));
124121
Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage.garbage", DomainType.PRIVATE));
122+
Assertions.assertNull(matcher.getDomainRoot("s3.amazonaws.com"));
123+
Assertions.assertNull(matcher.getDomainRoot("blah.s3.amazonaws.com"));
125124
}
126125

127126
@Test
@@ -146,6 +145,33 @@ void testGetDomainRootOnlyICANN() {
146145
Assertions.assertNull(matcher.getDomainRoot("*.garbage.garbage.garbage", DomainType.ICANN));
147146
}
148147

148+
@Test
149+
void testMaySetCookies() {
150+
Assertions.assertTrue(matcher.verify("foo.com"));
151+
152+
Assertions.assertFalse(matcher.verify("bar.foo.com"));
153+
Assertions.assertTrue(matcher.verify("example.bar.foo.com"));
154+
155+
Assertions.assertTrue(matcher.verify("foo.bar.jp"));
156+
Assertions.assertFalse(matcher.verify("bar.jp"));
157+
158+
Assertions.assertTrue(matcher.verify("foo.bar.hokkaido.jp"));
159+
Assertions.assertFalse(matcher.verify("bar.hokkaido.jp"));
160+
161+
Assertions.assertTrue(matcher.verify("foo.bar.tokyo.jp"));
162+
Assertions.assertFalse(matcher.verify("bar.tokyo.jp"));
163+
164+
Assertions.assertTrue(matcher.verify("pref.hokkaido.jp")); // exception from a wildcard rule
165+
Assertions.assertTrue(matcher.verify("metro.tokyo.jp")); // exception from a wildcard rule
166+
}
167+
168+
@Test
169+
void testVerifyPrivate() {
170+
Assertions.assertTrue(matcher.verify("s3.amazonaws.com"));
171+
Assertions.assertTrue(matcher.verify("blah.s3.amazonaws.com"));
172+
Assertions.assertTrue(matcher.verify("blah.xxx.uk"));
173+
}
174+
149175
@Test
150176
void testMatch() {
151177
Assertions.assertTrue(matcher.matches(".jp"));

0 commit comments

Comments
 (0)