diff --git a/core/src/main/java/org/apache/stormcrawler/filtering/basic/BasicURLNormalizer.java b/core/src/main/java/org/apache/stormcrawler/filtering/basic/BasicURLNormalizer.java index af4eb5a2a..2105e5234 100644 --- a/core/src/main/java/org/apache/stormcrawler/filtering/basic/BasicURLNormalizer.java +++ b/core/src/main/java/org/apache/stormcrawler/filtering/basic/BasicURLNormalizer.java @@ -336,23 +336,32 @@ public int compare(NameValuePair p1, NameValuePair p2) { }; /** - * A common error to find is a query string that starts with an & instead of a ? This will fix - * that error. So http://foo.com&a=b will be changed to http://foo.com?a=b. + * Repairs a query string that starts with an & instead of a ?. The query check ignores the + * fragment, and the repair only checks the last path segment so an & in an earlier path segment + * is not mistaken for a query separator. * * @param urlToFilter the url to filter * @return corrected url */ private String unmangleQueryString(String urlToFilter) { - String[] pathElements = urlToFilter.split("/"); - final String lastPathElement = pathElements[pathElements.length - 1]; - int firstAmp = lastPathElement.indexOf('&'); + if (urlToFilter.indexOf('&') == -1) { + return urlToFilter; + } + + int fragmentStart = urlToFilter.indexOf('#'); + String fragment = fragmentStart == -1 ? "" : urlToFilter.substring(fragmentStart); + String url = fragmentStart == -1 ? urlToFilter : urlToFilter.substring(0, fragmentStart); + if (url.indexOf('?') != -1) { + return urlToFilter; + } + + int lastSlash = url.lastIndexOf('/'); + int firstAmp = url.indexOf('&', lastSlash + 1); if (firstAmp == -1) { return urlToFilter; } - int firstQuestionMark = lastPathElement.indexOf('?'); - if (firstQuestionMark == -1 && lastPathElement.indexOf("=") > 0) { - pathElements[pathElements.length - 1] = lastPathElement.replaceFirst("&", "?"); - return String.join("/", pathElements); + if (url.indexOf('=', firstAmp + 1) > firstAmp) { + return url.substring(0, firstAmp) + '?' + url.substring(firstAmp + 1) + fragment; } return urlToFilter; } diff --git a/core/src/test/java/org/apache/stormcrawler/filtering/BasicURLNormalizerTest.java b/core/src/test/java/org/apache/stormcrawler/filtering/BasicURLNormalizerTest.java index 1791bbcc3..712521a85 100644 --- a/core/src/test/java/org/apache/stormcrawler/filtering/BasicURLNormalizerTest.java +++ b/core/src/test/java/org/apache/stormcrawler/filtering/BasicURLNormalizerTest.java @@ -35,6 +35,8 @@ import org.apache.stormcrawler.util.URLUtil; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; /** * Utility class which encapsulates the filtering of URLs based on the hostname or domain of the @@ -243,6 +245,76 @@ void testFixMangledQueryString() throws MalformedURLException { assertEquals(expectedResult, normalizedUrl, "Failed to filter query string"); } + @ParameterizedTest + @CsvSource({ + "http://a.example/article.pl?sid=01/10/23/1816257&mode=thread&tid=107", + "http://a.example/p?u=http://b.example/x&z=1", + "http://a.example/login/?next=/a/b/&utm_source=ref&z=1" + }) + void testDoesNotMangleQueryWhenValueContainsSlashes(String inputUrl) + throws MalformedURLException { + URLFilter urlFilter = createFilter(new ObjectNode(JsonNodeFactory.instance)); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + assertEquals( + inputUrl, + urlFilter.filter(testSourceUrl, new Metadata(), inputUrl), + "An existing query must not be unmangled because a value contains slashes"); + } + + @Test + void testUnmanglesQueryBeforeFragmentQuestionMark() throws MalformedURLException { + URLFilter urlFilter = createFilter(false, false); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + String inputUrl = "http://a.example/p&a=b#frag?x"; + assertEquals( + "http://a.example/p?a=b#frag?x", + urlFilter.filter(testSourceUrl, new Metadata(), inputUrl), + "A question mark in the fragment must not suppress query repair"); + } + + @ParameterizedTest + @CsvSource({"false, http://a.example/x&a=b/#frag", "true, http://a.example/x&a=b/"}) + void testDoesNotDropTrailingSlashWhenUnmangling(boolean removeAnchor, String inputUrl) + throws MalformedURLException { + URLFilter urlFilter = createFilter(removeAnchor, true, List.of()); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + assertEquals( + inputUrl, + urlFilter.filter(testSourceUrl, new Metadata(), inputUrl), + "The URL must remain unchanged when the last path segment has no query parameters"); + } + + @ParameterizedTest + @CsvSource({ + "http://a.example/p&a=b, http://a.example/p?a=b", + "http://a.example/p&a=b&c=d, http://a.example/p?a=b&c=d" + }) + void testRepairsMangledQueryString(String inputUrl, String expectedUrl) + throws MalformedURLException { + URLFilter urlFilter = createFilter(false, true, List.of()); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + assertEquals(expectedUrl, urlFilter.filter(testSourceUrl, new Metadata(), inputUrl)); + } + + @Test + void testDoesNotRewriteAmpersandInFragment() throws MalformedURLException { + URLFilter urlFilter = createFilter(false, true, List.of()); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + String inputUrl = "http://a.example/p#a&b=c"; + assertEquals(inputUrl, urlFilter.filter(testSourceUrl, new Metadata(), inputUrl)); + } + + @Test + void testRemovesQueryElementWhenValueContainsSlashes() throws MalformedURLException { + URLFilter urlFilter = createFilter(List.of("utm_source")); + URL testSourceUrl = URLUtil.toURL("http://a.example/"); + String inputUrl = "http://a.example/login/?next=/a/b/&utm_source=ref&z=1"; + assertEquals( + "http://a.example/login/?next=%2Fa%2Fb%2F&z=1", + urlFilter.filter(testSourceUrl, new Metadata(), inputUrl), + "Query elements following a slash-containing value must remain removable"); + } + @Test void testProperURLEncodingWithoutQueryParameter() throws MalformedURLException { URLFilter urlFilter = createFilter(queryParamsToFilter);