URLFilters.filter() returns the URL as accepted when a filter in the chain throws

[rzo1]

What happens

URLFilters.filter() wraps the whole loop over the configured filters in a single try/catch. When one filter throws, the catch logs the exception and execution falls through to return normalizedUrl, which still holds the value produced by the last filter that completed. That value is not null, so callers such as StatusEmitterBolt.filterOutlink treat the URL as accepted. Every filter positioned after the one that threw is skipped, including the regex exclusions and SitemapFilter, which the archetype places last.

Where

core/src/main/java/org/apache/stormcrawler/filtering/URLFilters.java:111-130. The same pattern is repeated in the main() helper at line 209.

} catch (Exception e) {
    LOG.error("URL filtering threw exception", e);
}
return normalizedUrl;

Why it matters

One filter that throws on one URL disables the rest of the chain for that URL, and the only sign is a log line per URL. The exclusions an operator relies on to keep the crawler off private ranges and off the file: and ftp: schemes sit at the end of the archetype chain, so they are the ones that get skipped. The shipped chain has no demonstrated thrower today, so this needs a filter that can throw: FastURLFilter is the known one, since Rule(String) at FastURLFilter.java:337-356 leaves type null when the rule line has no recognised prefix, the file loads without complaint, and r.getType().toString() at line 266 then throws when that rule is evaluated.

Reproduction

Save as core/src/test/java/org/apache/stormcrawler/filtering/URLFiltersExceptionTest.java.

/*
 * Licensed to the Apache Software Foundation (ASF) under one or more
 * contributor license agreements.  See the NOTICE file distributed with
 * this work for additional information regarding copyright ownership.
 * The ASF licenses this file to you under the Apache License, Version 2.0
 * (the "License"); you may not use this file except in compliance with
 * the License.  You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.apache.stormcrawler.filtering;

import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.HashMap;
import java.util.Map;
import org.apache.stormcrawler.Metadata;
import org.apache.stormcrawler.util.URLUtil;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

/** Behaviour of the filter chain when one of its filters throws. */
class URLFiltersExceptionTest {

    /** Stands in for any filter that throws at evaluation time. */
    public static class ThrowingURLFilter extends URLFilter {
        @Override
        public @Nullable String filter(
                @Nullable URL sourceUrl,
                @Nullable Metadata sourceMetadata,
                @NotNull String urlToFilter) {
            throw new NullPointerException("filter blew up");
        }
    }

    /** Stands in for an exclusion rule placed after it, such as the private-range regexes. */
    public static class RejectEverythingURLFilter extends URLFilter {
        @Override
        public @Nullable String filter(
                @Nullable URL sourceUrl,
                @Nullable Metadata sourceMetadata,
                @NotNull String urlToFilter) {
            return null;
        }
    }

    @Test
    void urlIsRejectedWhenAnEarlierFilterThrows() throws IOException, MalformedURLException {
        Map<String, Object> conf = new HashMap<>();
        URLFilters filters = new URLFilters(conf, "urlfilters-throwing.json");
        URL source = URLUtil.toURL("http://www.example.com/index.html");
        Assertions.assertNull(
                filters.filter(source, new Metadata(), "http://www.example.com/outlink.html"));
    }
}

Save as core/src/test/resources/urlfilters-throwing.json.

{
	"org.apache.stormcrawler.filtering.URLFilters": [
		{
			"class": "org.apache.stormcrawler.filtering.URLFiltersExceptionTest$ThrowingURLFilter",
			"name": "ThrowingURLFilter"
		},
		{
			"class": "org.apache.stormcrawler.filtering.URLFiltersExceptionTest$RejectEverythingURLFilter",
			"name": "RejectEverythingURLFilter"
		}
	]
}

Run it:

mvn -pl core test -Dtest=URLFiltersExceptionTest

The chain is a filter that throws followed by a filter that rejects everything, so a correct chain returns null. The test fails on main.

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[ERROR]   URLFiltersExceptionTest.urlIsRejectedWhenAnEarlierFilterThrows:62
    expected: <null> but was: <http://www.example.com/outlink.html>

The exception from the filter appears in the surefire output as the logged URL filtering threw exception line, and the URL is still returned.

Suggested fix

Move the try/catch inside the loop in URLFilters.filter() and treat an exception from any filter as a rejection: log which filter threw, and return null. Do the same in main() so the command line tool reports the same verdict as the topology. Add a counter so operators can see how often it happens. This changes behaviour for topologies that currently have a throwing filter and did not notice: those URLs start being dropped instead of emitted, which belongs in the release notes. Separately, make FastURLFilter.Rule reject an unrecognised rule prefix at load time instead of building a rule with a null type.

[abhinav-phi]

Hi! I'd like to work on this.
Plan: I'll make URLFilters.filter() fail-closed — if a filter in the chain throws, the URL is rejected instead of silently accepted — and add tests for the exception path.
/assign take