Repository: crawler-commons/crawler-commons (java)

Issue:
[Robots.txt] Path analyse bug with url-decode if allow/disallow path contains escaped wild-card characters
The problem is with url-decode:
https://github.com/crawler-commons/crawler-commons/blob/4b13456bc954f97ae209446b3736976b2a9ea298/src/main/java/crawlercommons/robots/SimpleRobotRulesParser.java#L631
Google robots.txt specification (https://developers.google.com/search/reference/robots_txt?hl=en) says:
"Non-7-bit ASCII characters in a path may be included as UTF-8 characters or as percent-escaped UTF-8 encoded characters per RFC 3986."
So for example, if we have rule ``Disallow: /*%2F*`` (blocks all rules with url-encoded urls in it), SimpleRobotRulesParser interprets it like ``Disallow: /*/*``, and this rule kills much more urls.

So, parser should url-decode not the whole path, but only url-encoded Non-7-bit ASCII characters in a path

Pull request description:
[Robots.txt] Path analyse bug with url-decode if allow/disallow path contains escaped wild-card characters
(fixes #195)
- add unit tests to catch the issue
- percent-encode allow/disallow paths and URL paths before/during rule matching
- decode characters where necessary
- match paths containing `*` or `$`: percent-encoded in rules because of the special meaning. See also google/robotstxt#57 for clarification about the correct behavior.
- minor changes in BasicURLNormalizer to be able to use the provided path encoding/decoding methods

Patch:
diff --git a/src/main/java/crawlercommons/filters/basic/BasicURLNormalizer.java b/src/main/java/crawlercommons/filters/basic/BasicURLNormalizer.java
index 09d6c4f..8f73f25 100644
--- a/src/main/java/crawlercommons/filters/basic/BasicURLNormalizer.java
+++ b/src/main/java/crawlercommons/filters/basic/BasicURLNormalizer.java
@@ -540,7 +540,11 @@ public class BasicURLNormalizer extends URLFilter {
      * characters which should be escaped according to <a
      * href="https://tools.ietf.org/html/rfc3986#section-2.2">RFC3986</a>.
      */
-    private static String escapePath(String path) {
+    public static String escapePath(String path) {
+        return escapePath(path, null);
+    }
+
+    public static String escapePath(String path, boolean[] extraEscapedBytes) {
         StringBuilder sb = new StringBuilder(path.length());
 
         // Traverse over all bytes in this URL
@@ -548,7 +552,7 @@ public class BasicURLNormalizer extends URLFilter {
         for (int i = 0; i < bytes.length; i++) {
             byte b = bytes[i];
             // Is this a control character?
-            if (b < 0 || escapedCharacters[b]) {
+            if (b < 0 || escapedCharacters[b] || (extraEscapedBytes != null && extraEscapedBytes[b])) {
                 // Start escape sequence
                 sb.append('%');
 
diff --git a/src/main/java/crawlercommons/robots/SimpleRobotRules.java b/src/main/java/crawlercommons/robots/SimpleRobotRules.java
index c359d77..4a74fc6 100644
--- a/src/main/java/crawlercommons/robots/SimpleRobotRules.java
+++ b/src/main/java/crawlercommons/robots/SimpleRobotRules.java
@@ -18,11 +18,12 @@ package crawlercommons.robots;
 
 import java.io.Serializable;
 import java.net.URL;
-import java.net.URLDecoder;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
+import crawlercommons.filters.basic.BasicURLNormalizer;
+
 /**
  * Result from parsing a single robots.txt file - which means we get a set of
  * rules, and an optional crawl-delay, and an optional sitemap URL. Note that we
@@ -121,6 +122,13 @@ public class SimpleRobotRules extends BaseRobotRules {
     protected ArrayList<RobotRule> _rules;
     protected RobotRulesMode _mode;
 
+    /** Special characters which require percent-encoding for path matching */
+    protected final static boolean[] specialCharactersPathMatching = new boolean[128];
+    static {
+        specialCharactersPathMatching['*'] = true;
+        specialCharactersPathMatching['$'] = true;
+    }
+
     public SimpleRobotRules() {
         this(RobotRulesMode.ALLOW_SOME);
     }
@@ -200,6 +208,24 @@ public class SimpleRobotRules extends BaseRobotRules {
         }
     }
 
+    /**
+     * Encode/decode (using percent-encoding) all characters where necessary:
+     * encode Unicode/non-ASCII characters) and decode printable ASCII
+     * characters without special semantics.
+     * 
+     * @param urlPathQuery
+     *            path and query component of the URL
+     * @param additionalEncodedBytes
+     *            boolean array to request bytes (ASCII characters) to be
+     *            percent-encoded in addition to other characters requiring
+     *            encoding (Unicode/non-ASCII and characters not allowed in
+     *            URLs).
+     * @return properly percent-encoded URL path and query
+     */
+    public static String escapePath(String urlPathQuery, boolean[] additionalEncodedBytes) {
+        return BasicURLNormalizer.escapePath(BasicURLNormalizer.unescapePath(urlPathQuery), additionalEncodedBytes);
+    }
+
     private String getPath(String url, boolean getWithQuery) {
 
         try {
@@ -214,9 +240,17 @@ public class SimpleRobotRules extends BaseRobotRules {
                 path += "?" + query;
             }
 
-            // We used to lower-case the path, but Google says we need to do
-            // case-sensitive matching.
-            return URLDecoder.decode(path, "UTF-8");
+            /*
+             * We used to lower-case the path, but Google says we need to do
+             * case-sensitive matching.
+             * 
+             * However, we need to properly decode percent-encoded characters,
+             * but preserve those escaped characters which have special
+             * semantics in path matching, e.g. slash `/`. However, for the
+             * implementation of the path matching requires that asterisk `*`
+             * and dollar `$` are exceptionally percent-encoded.
+             */
+            return escapePath(path, specialCharactersPathMatching);
         } catch (Exception e) {
             // If the URL is invalid, we don't really care since the fetch
             // will fail, so return the root.
diff --git a/src/main/java/crawlercommons/robots/SimpleRobotRulesParser.java b/src/main/java/crawlercommons/robots/SimpleRobotRulesParser.java
index 2f6eb80..646d5ef 100644
--- a/src/main/java/crawlercommons/robots/SimpleRobotRulesParser.java
+++ b/src/main/java/crawlercommons/robots/SimpleRobotRulesParser.java
@@ -21,7 +21,6 @@ import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
-import java.net.URLDecoder;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
@@ -778,13 +777,23 @@ public class SimpleRobotRulesParser extends BaseRobotsParser {
 
     /**
      * Add any uniform rules to clean up path directives
+     * <ul>
+     * <li>trim leading and trailing white space and control characters not
+     * handled by the tokenizer</li>
+     * <li>properly percent-encode all characters where necessary</li>
+     * <li>but make sure that characters with special semantics for path
+     * matching (asterisk <code>*</code>, slash <code>/</code>, dollar
+     * <code>$</code>, etc.) are left as is (do not decode if percent-encoded).
+     * </ul>
+     * 
+     * This method uses {@link SimpleRobotRules#escapePath(String, boolean[])}
+     * to normalize the URL path before matching against allow/disallow rules.
+     * 
      * @param path
-     * @return clean path
+     * @return clean and encoded path
      */
     private String normalizePathDirective(String path) {
-        path = path.trim();
-
-        return path;
+        return SimpleRobotRules.escapePath(path.trim(), null);
     }
 
     /**
@@ -803,7 +812,6 @@ public class SimpleRobotRulesParser extends BaseRobotsParser {
         String path = token.getData();
 
         try {
-            path = URLDecoder.decode(path, "UTF-8");
             path = normalizePathDirective(path);
             if (path.length() == 0) {
                 // Disallow: <nothing> => allow all.
@@ -832,7 +840,6 @@ public class SimpleRobotRulesParser extends BaseRobotsParser {
         String path = token.getData();
 
         try {
-            path = URLDecoder.decode(path, "UTF-8");
             path = normalizePathDirective(path);
         } catch (Exception e) {
             reportWarning(state, "Error parsing robots rules - can't decode path: {}", path);


Test patch:
diff --git a/src/test/java/crawlercommons/robots/SimpleRobotRulesParserTest.java b/src/test/java/crawlercommons/robots/SimpleRobotRulesParserTest.java
index 3fb6dc4..0962f32 100644
--- a/src/test/java/crawlercommons/robots/SimpleRobotRulesParserTest.java
+++ b/src/test/java/crawlercommons/robots/SimpleRobotRulesParserTest.java
@@ -265,6 +265,49 @@ public class SimpleRobotRulesParserTest {
         // assertFalse(rules.isAllowed("https://www.example.com/bücher/book1.html"));
     }
 
+    @ParameterizedTest
+    @CsvSource({ // Tests for percent-encoded characters with special semantics
+                 // in allow/disallow statements:
+                 // (a) must not trim percent-encoded white space
+                    "True, /*%20, https://www.example.com/", //
+                    "False, /*%20, https://www.example.com/foobar%20/", //
+                    "True, /*%20, https://www.example.com/foobar/", //
+                    // (b) match literal %2F in URL path, but do not match a
+                    // slash
+                    "True, /*%2F*, https://www.example.com/path/index.html", //
+                    "False, /*%2F*, https://www.example.com/topic/9%2F11/index.html", //
+                    "False, /topic/9%2F11/, https://www.example.com/topic/9%2F11/index.html", //
+                    "False, /topic/9%2F11/, https://www.example.com/topic/9%2f11/index.html", //
+                    "False, /q?*mime=application%2Fpdf, https://www.example.com/q?mime=application%2Fpdf", //
+                    // (c) percent-encoded dollar and asterisk (*)
+                    "False, /$, https://www.example.com/", //
+                    "True, /$, https://www.example.com/foobar", //
+                    "True, /%24, https://www.example.com/", //
+                    "False, /%24, https://www.example.com/%24100", //
+                    "False, /%24, https://www.example.com/$100", //
+                    "True, /search/%2A/, https://www.example.com/search/foobar/", //
+                    "False, /search/%2A/, https://www.example.com/search/%2A/", //
+                    "False, /search/%2A/, https://www.example.com/search/%2a/", //
+                    "False, /search/%2a/, https://www.example.com/search/%2a/", //
+                    "False, /search/%2a/, https://www.example.com/search/*/", //
+                    "False, /search/*/, https://www.example.com/search/foobar/", //
+                    // examples from RFC 9309
+                    "False, /path/file-with-a-%2A.html, https://www.example.com/path/file-with-a-*.html", //
+                    "True, /path/file-with-a-%2A.html, https://www.example.com/path/file-with-a-foo.html", //
+                    "False, /path/file-with-a-%2A.html, https://www.example.com/path/file-with-a-%2A.html", //
+                    "False, /path/foo-%24, https://www.example.com/path/foo-$", //
+                    "True, /path/foo-%24, https://www.example.com/path/foo-", //
+                    "False, /path/foo-%24, https://www.example.com/path/foo-%24", //
+    })
+    void testEscapedPaths(boolean isAllowed, String disallowPath, String urlStr) {
+        final String simpleRobotsTxt = "User-agent: *" + CRLF //
+                        + "Disallow: " + disallowPath + CRLF //
+                        + "Allow: /";
+        BaseRobotRules rules = createRobotRules("mybot", simpleRobotsTxt);
+        String msg = urlStr + " should " + (isAllowed ? "not" : "") + " be disallowed by rule Disallow: " + disallowPath;
+        assertEquals(isAllowed, rules.isAllowed(urlStr), msg);
+    }
+
     @Test
     void testSimplestAllowAll() {
         final String simpleRobotsTxt = "User-agent: *" + CRLF //
diff --git a/src/test/resources/normalizer/weirdToNormalizedUrls.csv b/src/test/resources/normalizer/weirdToNormalizedUrls.csv
index d3c64b7..ad38772 100644
--- a/src/test/resources/normalizer/weirdToNormalizedUrls.csv
+++ b/src/test/resources/normalizer/weirdToNormalizedUrls.csv
@@ -18,6 +18,8 @@ http://foo.com/%66oo.htm%1A, http://foo.com/foo.htm%1A
 
 # check that % decoder converts to upper case letters
 http://foo.com/%66oo.htm%c0, http://foo.com/foo.htm%C0
+https://www.example.com/search/%2a/, https://www.example.com/search/%2A/
+https://www.example.com/topic/9%2f11/, https://www.example.com/topic/9%2F11/
 
 # check that % decoder leaves encoded spaces alone
 http://foo.com/you%20too.html, http://foo.com/you%20too.html
@@ -210,4 +212,8 @@ http://example.com/?, http://example.com/
 
 # Should not decode URL query data
 https://foo.com/?one/valid_query/without_%2F_params, https://foo.com/?one/valid_query/without_%2F_params
-http://foo.com/asdf/page.php?article%2F1234, http://foo.com/asdf/page.php?article%2F1234
\ No newline at end of file
+http://foo.com/asdf/page.php?article%2F1234, http://foo.com/asdf/page.php?article%2F1234
+
+# examples from the robots.txt RFC 9309 - * and $ should be unchanged
+https://www.example.com/path/file-with-a-*.html, https://www.example.com/path/file-with-a-*.html
+https://www.example.com/path/foo-$, https://www.example.com/path/foo-$
\ No newline at end of file