rzo1 commented on code in PR #2122:
URL: https://github.com/apache/stormcrawler/pull/2122#discussion_r3944167262
##########
core/src/main/java/org/apache/stormcrawler/util/URLUtil.java:
##########
@@ -253,6 +254,31 @@ public static String getHost(String url) {
}
}
+ /**
+ * Returns the host in the form the HTTP client will connect to it:
percent-escapes decoded,
+ * lowercased and without a trailing dot. Host strings which only differ
in escaping or case
+ * reach the same server, so politeness queues and robots.txt caches must
key on the same
+ * value, otherwise one server is fetched under several queue ids and its
robots.txt is
+ * downloaded once per spelling.
+ *
+ * @param url The url to check.
+ * @return String The canonical host for the url, or null if the url is
not well formed or has
+ * no host.
+ */
+ public static String getCanonicalHost(URL url) {
+ String host = url.getHost();
+ if (host == null) {
+ return null;
+ }
+ // okhttp percent-decodes the host when it parses the URL; do the same
+ // so keys derived from the URL string agree with what it connects to
+ String decoded = URLDecoder.decode(host, StandardCharsets.UTF_8);
Review Comment:
`URLDecoder.decode` throws on a malformed escape and maps `+` to a space.
Both are reachable from crawled content, which the security model treats as
hostile. Verified on JDK 25:
```
new URL("http://exa%zz.org/";).getHost() -> "exa%zz.org"
URLDecoder.decode("exa%zz.org", UTF_8) -> IllegalArgumentException
URLDecoder.decode("a+b.example.org") -> "a b.example.org"
```
The exception propagates out of `HttpRobotRulesParser.getCacheKey` and
`FetchItem.create`, both of which take a URL from a fetched page.
```suggestion
String decoded;
try {
decoded = new URI(url.getProtocol(), null, host, -1, "/", null,
null).getHost();
} catch (URISyntaxException e) {
decoded = host;
}
if (decoded == null) {
decoded = host;
}
```
A hand-rolled percent-decoder that leaves `+` alone and falls back to the
raw host also works. The requirement is that it never throws.
##########
core/src/main/java/org/apache/stormcrawler/util/URLUtil.java:
##########
@@ -253,6 +254,31 @@ public static String getHost(String url) {
}
}
+ /**
+ * Returns the host in the form the HTTP client will connect to it:
percent-escapes decoded,
Review Comment:
This says "the host in the form the HTTP client will connect to it", but
`HostAliasCacheKeyTest.okhttpCollapsesHostAliases` in this same PR asserts that
okhttp keeps the trailing dot (`example.org.`), which this method strips.
Keep the behaviour, it is the better politeness key. Reword to something
like "the form used to key politeness queues and the robots.txt cache: what
okhttp connects to, with the root label normalised away".
##########
core/src/main/java/org/apache/stormcrawler/bolt/FetcherBolt.java:
##########
@@ -186,7 +186,7 @@ public static FetchItem create(URL u, String url, Tuple t,
String queueMode) {
key = u.getHost();
}
} else {
- key = u.getHost();
+ key = URLUtil.getCanonicalHost(u);
Review Comment:
Only the `byHost` branch is canonicalised. `byIP` above still calls
`InetAddress.getByName(u.getHost())` and `byDomain` still calls
`PaidLevelDomain.getPLD(u.getHost())` on the raw escaped host, so
`http://%65xample.org/` still gets a separate queue from `http://example.org/`
in those modes.
Compute it once before the `if` and use it in all three branches:
```java
final String canonicalHost = URLUtil.getCanonicalHost(u);
if (FetchItemQueues.QUEUE_MODE_IP.equalsIgnoreCase(queueMode)) {
... InetAddress.getByName(canonicalHost) ...
} else if (FetchItemQueues.QUEUE_MODE_DOMAIN.equalsIgnoreCase(queueMode)) {
key = PaidLevelDomain.getPLD(canonicalHost);
...
} else {
key = canonicalHost;
}
```
##########
core/src/main/java/org/apache/stormcrawler/bolt/SimpleFetcherBolt.java:
##########
@@ -628,7 +628,7 @@ private String getPolitenessKey(URL u) {
key = u.getHost();
}
} else {
- key = u.getHost();
+ key = URLUtil.getCanonicalHost(u);
Review Comment:
Same as `FetcherBolt`: `byIP` (line 615) and `byDomain` (line 625) still use
the raw `u.getHost()`, so those two modes keep the behaviour the issue is
about. Canonicalise once at the top of the method and use it in all three
branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
