rzo1 commented on code in PR #2125:
URL: https://github.com/apache/stormcrawler/pull/2125#discussion_r3944167684
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -103,22 +118,19 @@ public void execute(Tuple tuple) {
LOG.debug("Processing {}", url);
- boolean looksLikeSitemap = sniff(content);
- // can force the mimetype as we know it is XML
- if (looksLikeSitemap) {
+ String isSitemap = metadata.getFirstValue(isSitemapKey);
+
+ // only sniff when the operator asked for it: a page deciding how the
+ // pipeline treats it must not depend on a string in its body, and a
+ // sniffed document also needs a sitemap compatible content type
+ if (isSitemap == null && sniffContent && sniffsAsSitemap(ct, content))
{
Review Comment:
Regression here.
On `main`, `sniff()` ran unconditionally and set `ct = "application/xml"`
whenever the namespace was found, **including** for documents already marked
`isSitemap=true`. Now the override only happens when `isSitemap == null &&
sniffContent`, and `sniffContent` defaults to false.
`parseSiteMap` passes `ct` straight to `parser.parseSiteMap(contentType,
content, url)` unless it is blank or `octet-stream`. So a sitemap served as
`text/html`, which is common and the reason the sniff existed, now throws
`UnknownFormatException` and becomes `FETCH_ERROR` where it used to parse.
Sniffing to *confirm* a document the crawl already declared a sitemap is not
attacker-driven classification. Only sniffing to *promote* an unmarked document
is.
```suggestion
if (isSitemap == null && sniffContent && sniffsAsSitemap(ct,
content)) {
LOG.info("{} detected as sitemap based on content and content
type", url);
ct = "application/xml";
isSitemap = "true";
} else if (Boolean.parseBoolean(isSitemap) && sniff(content)) {
// already declared a sitemap: the namespace only confirms the
type,
// it does not decide how the pipeline treats the document, so a
// sitemap served with the wrong content type still parses
ct = "application/xml";
}
```
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -86,6 +86,21 @@ public class SiteMapParserBolt extends StatusEmitterBolt {
private int maxOffsetGuess = 300;
+ /**
+ * Whether a document without the {@code isSitemap} key is classified as a
sitemap by searching
+ * the first bytes for the sitemaps.org namespace. Any page that carries
the namespace string
+ * early enough is reclassified as a sitemap and never reaches the parser
bolt, so this defaults
+ * to false, like {@code feed.sniffContent} does for feeds.
+ */
+ private boolean sniffContent = false;
+
+ /**
+ * Whether the parser rejects documents which are not well formed
sitemaps. Strict parsing keeps
Review Comment:
This describes `strictNamespace`, not `strict`.
crawler-commons `SiteMapParser` (1.6) has two separate flags:
```
protected boolean strict; // strict URL checking: same host and
path prefix
protected boolean strictNamespace; // namespace / well-formedness
```
The constructor argument used on line 360 is `strict`, which is URL scoping.
So the javadoc, the yaml comment and the PR description all describe the other
flag.
If the namespace check is what you want, call `setStrictNamespace(true)` and
leave URL scoping off. If URL scoping is what you want, say that, and see my
comment on `crawler-default.yaml:290`.
##########
core/src/main/resources/crawler-default.yaml:
##########
@@ -275,6 +275,20 @@ config:
# filters URLs in sitemaps based on their modified Date (if any)
sitemap.filter.hours.since.modified: -1
+ # whether a document without the isSitemap key is classified as a sitemap
+ # by searching the first bytes of its content for the sitemaps.org
+ # namespace. Off by default: any page carrying the namespace string early
+ # enough would be reclassified as a sitemap and never reach the parser
+ # bolt. When enabled, a content type which rules a sitemap out (a page
+ # served as HTML) stops the sniffing.
+ sitemap.sniffContent: false
+
+ # whether the sitemap parser rejects documents which are not well formed
+ # sitemaps. Strict parsing also discards URLs a sitemap lists on hosts
+ # other than its own, and keeps an ordinary HTML page which mentions the
+ # sitemap namespace from being parsed leniently into half a sitemap.
+ sitemap.strict: true
Review Comment:
Defaulting this to `true` silently shrinks existing crawls.
crawler-commons `strict` is strict URL checking, so a sitemap may only list
URLs under its own host and path. `www.example.com` vs `example.com`, CDN
hosts, and a sitemap at `/sitemaps/foo.xml` covering `/` are all common and all
spec violations. The two test fixtures this PR had to rewrite
(`stormcrawler.sitemap.extensions.*.xml`, `http://www.example.com/` to
`https://stormcrawler.apache.org/with-image.html`) are a direct consequence.
Cross-host enrolment from a sitemap is a genuine concern, but under the
security model this is a hardening decision, and this one costs users URLs
without warning.
Either default to `false` and document it as recommended, or ship it on and
make it a headline item in the upgrade note. Also note the yaml comment here
repeats the incorrect description of what `strict` does; see my comment on
`SiteMapParserBolt.java:98`.
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -140,12 +152,21 @@ public void execute(Tuple tuple) {
// exception while parsing the sitemap
String errorMessage = "Exception while parsing " + url + ": " + e;
LOG.error(errorMessage);
- // send to status stream in case another component wants to update
- // its status
+ /*
+ * A document which does not parse as a sitemap is most likely an
+ * ordinary page whose persisted metadata carried isSitemap=true.
+ * Dropping the marking and emitting it as FETCH_ERROR keeps it
+ * schedulable: a terminal ERROR would remove it from the crawl for
+ * good when fetchInterval.error is negative, which lets whoever
+ * controls the content remove URLs from the corpus. The document
+ * goes on to the parser bolt on its next fetch, like any other
+ * page.
+ */
+ metadata.remove(isSitemapKey);
metadata.setValue(Constants.STATUS_ERROR_SOURCE, "sitemap
parsing");
metadata.setValue(Constants.STATUS_ERROR_MESSAGE, errorMessage);
collector.emit(
- Constants.StatusStreamName, tuple, new Values(url,
metadata, Status.ERROR));
+ Constants.StatusStreamName, tuple, new Values(url,
metadata, Status.FETCH_ERROR));
Review Comment:
This line exceeds the limit and the formatter will rewrite it. CI runs the
format check (`-Dskip.format.code=false`).
--
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]
