dpol1 commented on code in PR #2075:
URL: https://github.com/apache/stormcrawler/pull/2075#discussion_r3878556553
##########
core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java:
##########
@@ -286,8 +286,10 @@ private void addCookiesToRequest(Builder rb, String url,
Metadata md) {
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));
Review Comment:
With `null` here every host-only cookie is dropped, in practice most session
cookies, login via 302 + Set-Cookie included, and nothing is logged. Could we
record the origin instead? Only when a response actually carries `Set-Cookie`,
write a sibling `set-cookie-origin` = fetched URL next to it in
`responsemetadata`, and it has to travel with `protocol.set-cookie` in
`metadata.transfer` (`putAll` alone won't do that). Then `getFirstValue(origin,
protocolMetadataPrefix)` here. Should be about 15 lines. If not in this PR: a
WARN at configure time, fix the cookies paragraph in `internals.adoc` and open
a follow-up, or someone will spend an afternoon looking for their cookies.
##########
core/src/main/java/org/apache/stormcrawler/util/CookieConverter.java:
##########
@@ -129,6 +168,16 @@ public static List<Cookie> getCookies(String[]
cookiesStrings, URL targetURL) {
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;
Review Comment:
`com..` gets through: first dot at 3, `length - 1` is 4, so it counts as
multi-label; then `split("\\.")` in `checkDomainMatchToUrl` drops the empty
tokens and it matches every `.com` host. `getCookies({"sid=x; Domain=com.."},
http://evil.com, http://victim.com)` returns the cookie. `Domain=co.uk` has the
same shape, `evil.co.uk` reaches `bank.co.uk`. Could we reject malformed
domains and known public suffixes with the PSL support already in
crawler-commons, with a test for unlisted/internal domains so those keep
working?
--
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]
