dpol1 commented on code in PR #2077:
URL: https://github.com/apache/stormcrawler/pull/2077#discussion_r3878556400
##########
core/src/main/java/org/apache/stormcrawler/protocol/HttpRobotRulesParser.java:
##########
@@ -149,7 +170,18 @@ public BaseRobotRules getRobotRulesSet(Protocol http, URL
url) {
String redirection =
response.getMetadata().getFirstValue(HttpHeaders.LOCATION);
LOG.debug("Redirected from {} to {}", redir, redirection);
if (StringUtils.isNotBlank(redirection)) {
- redir = URLUtil.resolveUrl(redir, redirection);
+ URL target = URLUtil.resolveUrl(redir, redirection);
+ if (!followRedirect(redir, target)) {
+ LOG.debug(
+ "Robots for {} redirected to {} which is not
fetched "
+ + "(not on the same host as {})",
+ url,
+ target,
+ redir);
+ // handled like any other response which does not
provide rules
+ break;
Review Comment:
The new tests make rejected redirect -> allow-all explicit, and that 3xx is
then cached as a successful `EMPTY_RULES` result for 6h. I don't think an
egress restriction should silently turn into permission to crawl without rules.
Could we at least keep it out of the success cache and decide explicitly
between fail-closed and allow-all?
##########
core/src/main/java/org/apache/stormcrawler/protocol/HttpRobotRulesParser.java:
##########
@@ -44,6 +44,8 @@ public class HttpRobotRulesParser extends RobotRulesParser {
protected boolean allow5xx = false;
+ protected boolean allowCrossHostRedirects = false;
Review Comment:
Two cases hide behind `false`. `getCacheKey` treats `http:h:80` and
`https:h:443` as different authorities, so the scheme upgrade on the same host
dies on the first hop; no other host is involved, I'd always follow that one.
Apex -> www and CDNs are a real host change: the robots RFC says a crawler
should follow those (hence our five-hop cap), but it's a SHOULD, and an egress
restriction is defensible under our security model. Keep it opt-in if you want,
but exempt the same-host scheme upgrade?
##########
core/src/test/java/org/apache/stormcrawler/protocol/HttpRobotRulesParserRedirectTest.java:
##########
@@ -79,6 +79,8 @@ void setUp() {
mockServer7.start();
mockServer8.start();
conf.put("http.agent.name", "this_is_only_a_test");
+ // the redirect chains tested below point at other ports of the same
host
+ conf.put("http.robots.redirect.crosshost.allow", true);
Review Comment:
With the flag on here nothing runs end to end with the default, and
`localhost:8090 -> :8091` is exactly the chain that now ends as allow-all. One
case without the flag? Same host `http -> https` in the new test would be good
too.
--
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]
