fix(robots): the gate did not fail closed on a soft 200 or an encoded Disallow (#286)

Found by an adversarial review run over the merged checker, prompted to falsify rather than confirm. Both are pinned in tests/test_robots_check.py. A soft 200 granted permission. A host answering /robots.txt with an HTML error page at status 200 produces a body that parses to zero rules, and zero rules read as "allowed" - so the browser-header retry ran on permission that was never given: rc._fetch = lambda url, ua: ("<html>404 Not Found</html>", 200) rc.gate("https://x.example/jobs") # -> (0, 'ALLOWED - robots.txt permits this path') A non-empty body carrying no recognised directive is now treated as unreadable. A genuinely empty file stays allow-all per RFC 9309, so this does not over-correct. Disallow patterns were never percent-decoded while the request path was, so "Disallow: /foo%20bar" never matched "/foo bar" and the rule was silently skipped. Also adds the "--" terminator before the URL in the curl argv, plus an explicit --max-redirs 5. gate() rebuilds the target as scheme://host/robots.txt before calling _fetch, so the gate path was never exposed to a dash-leading URL - this is hardening for direct callers. Three tests pin it: the terminator is present, a dash-leading argument fails closed end to end, and gate() never passes a caller-supplied URL through to curl. 187 tests pass. Claude-Session: https://claude.ai/code/session_01XTtiXab1yUFF2aL4s3fVY1 Co-authored-by: kgb <kevingblackman@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-09-17 00:26:26 +00:00 · 2026-08-06 07:59:34 +02:00
parent f89728e52f
commit 8ffe987f09
3 changed files with 166 additions and 4 deletions
@@ -13,6 +13,28 @@ per-file diff commands.

 ## [Unreleased]

+### Fixed
+
+- **The robots gate did not fail closed** (`tools/robots_check.py`, #277). Found by an
+  adversarial review run over the merged file, not by inspection. Both cases are pinned
+  in `tests/test_robots_check.py` as FAIL-OPEN REGRESSIONs:
+
+  - **A soft `200` granted permission.** A host answering `/robots.txt` with an HTML
+    error page at status 200 produces a body that parses to zero rules, and zero rules
+    read as "allowed" - so the browser-header retry ran on permission that was never
+    given. A non-empty body carrying no recognised directive is now treated as
+    unreadable. A genuinely empty file stays allow-all, per RFC 9309.
+  - **`Disallow` patterns were never percent-decoded** while the request path was, so
+    `Disallow: /foo%20bar` never matched `/foo bar` and the rule was silently skipped -
+    a fail-open on any site that encodes its own rules.
+
+- **`curl` argument hardening** (`tools/robots_check.py`). The curl argv had no `--`
+  terminator before the URL. `gate()` rebuilds the target as `scheme://host/robots.txt`
+  before calling `_fetch`, so the gate path was never exposed; this is hardening for
+  direct callers, with a test pinning the terminator, that a dash-leading argument fails
+  closed end to end, and that `gate()` never passes a caller-supplied URL through to
+  curl. `--max-redirs 5` is set explicitly rather than left to curl's default.
+
 ### Added

 - **Spec-pinning tests for the Language Gate's `/rank` contract** (#278) - four regression
@@ -17,7 +17,7 @@ from pathlib import Path
 REPO_ROOT = Path(__file__).resolve().parents[1]
 sys.path.insert(0, str(REPO_ROOT / "tools"))

-from robots_check import allowed  # noqa: E402
+from robots_check import allowed, is_robots_body  # noqa: E402


 # Real body served by privatebank.barclays.com: blank lines sit between the
@@ -103,5 +103,112 @@ class TestCli(unittest.TestCase):
        self.assertNotEqual(out.returncode, 0)


+class TestSoftTwoHundred(unittest.TestCase):
+    """A 200 whose body is not a robots.txt used to grant permission.
+
+    Found by adversarial review, not inspection. A misconfigured host answering
+    /robots.txt with an HTML error page at status 200 parses to zero rules, and
+    zero rules read as "allowed" - so the browser retry ran on permission that
+    was never given. FAIL-OPEN REGRESSION.
+    """
+
+    def test_html_error_page_is_not_a_robots_file(self):
+        self.assertFalse(is_robots_body("<html><body>404 Not Found</body></html>"))
+
+    def test_json_error_body_is_not_a_robots_file(self):
+        self.assertFalse(is_robots_body('{"error":"not found"}'))
+
+    def test_soft_200_is_unconfirmed_not_allowed(self):
+        import robots_check
+
+        original = robots_check._fetch
+        robots_check._fetch = lambda url, ua: ("<html>404</html>", 200)
+        try:
+            rc, msg = robots_check.gate("https://x.example/jobs")
+        finally:
+            robots_check._fetch = original
+        self.assertEqual(rc, 1)
+        self.assertIn("not a robots.txt", msg)
+
+    def test_a_genuinely_empty_robots_is_still_allow_all(self):
+        """RFC 9309: an empty file permits everything. Do not over-correct."""
+        self.assertTrue(is_robots_body(""))
+        self.assertTrue(is_robots_body("\n\n   \n"))
+
+    def test_a_real_policy_is_recognised(self):
+        self.assertTrue(is_robots_body(BARCLAYS))
+        self.assertTrue(is_robots_body(JOBUP))
+
+    def test_sitemap_only_file_counts(self):
+        self.assertTrue(is_robots_body("Sitemap: https://x.example/sitemap.xml\n"))
+
+
+class TestPercentEncodedRules(unittest.TestCase):
+    """Rule patterns are percent-decoded to match the decoded request path.
+
+    FAIL-OPEN REGRESSION: without this, a site that percent-encodes its own
+    Disallow patterns has them silently skipped.
+    """
+
+    def test_encoded_space_in_disallow_now_matches(self):
+        self.assertFalse(allowed("User-agent: *\nDisallow: /foo%20bar\n", "*", "/foo bar"))
+
+    def test_encoded_rule_does_not_overmatch(self):
+        self.assertTrue(allowed("User-agent: *\nDisallow: /foo%20bar\n", "*", "/foobar"))
+
+    def test_plain_rules_are_unaffected(self):
+        self.assertFalse(allowed(JOBUP, "*", "/api/x"))
+        self.assertTrue(allowed(JOBUP, "*", "/en/jobs/x"))
+
+
+
+class TestArgumentHardening(unittest.TestCase):
+    """A URL can never be read by curl as an option.
+
+    gate() rebuilds the target as scheme://host/robots.txt, so the gate path was
+    never exposed; this pins the "--" terminator for direct _fetch callers and
+    confirms a dash-leading argument fails closed end to end.
+    """
+
+    def test_curl_argv_ends_with_a_double_dash_before_the_url(self):
+        import inspect
+
+        import robots_check
+
+        src = inspect.getsource(robots_check._fetch)
+        self.assertIn("'--', url", src)
+
+    def test_a_dash_leading_argument_fails_closed(self):
+        script = REPO_ROOT / "tools" / "robots_check.py"
+        out = subprocess.run(
+            [sys.executable, str(script), "--help"],
+            capture_output=True,
+            text=True,
+            timeout=60,
+        )
+        self.assertEqual(out.returncode, 1)
+        self.assertNotIn("Usage: curl", out.stdout)
+
+    def test_gate_never_passes_the_caller_url_through_to_curl(self):
+        """The robots target is rebuilt from scheme+host, never the raw input."""
+        import robots_check
+
+        seen = []
+
+        original = robots_check._fetch
+
+        def spy(url, ua):
+            seen.append(url)
+            return "User-agent: *\nAllow: /\n", 200
+
+        robots_check._fetch = spy
+        try:
+            robots_check.gate("https://x.example/-o/evil?q=1")
+        finally:
+            robots_check._fetch = original
+        self.assertEqual(seen[0], "https://x.example/robots.txt")
+
+
+
 if __name__ == "__main__":
    unittest.main()
@@ -30,15 +30,39 @@ BROWSER = ('Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 '
 def _fetch(url, ua):
    """curl, not urllib: some hosts (jobup.ch) hang urllib indefinitely while
    answering curl in under a second, and --max-time is a hard ceiling."""
+    # "--" terminates option parsing, so a URL beginning with a dash can never
+    # be read by curl as a flag. gate() rebuilds the target as
+    # scheme://host/robots.txt before calling here, so this is hardening for
+    # direct callers rather than a hole in the gate path itself.
    r = subprocess.run(
-        ['curl', '-sS', '-L', '--max-time', '12', '-A', ua,
-         '-H', 'Accept: text/plain,*/*', '-w', '\n%{http_code}', url],
+        ['curl', '-sS', '-L', '--max-redirs', '5', '--max-time', '12', '-A', ua,
+         '-H', 'Accept: text/plain,*/*', '-w', '\n%{http_code}', '--', url],
        capture_output=True, text=True, timeout=20)
    if r.returncode != 0:
        raise RuntimeError('curl exit %d' % r.returncode)
    body, _, code = r.stdout.rpartition('\n')
    return body, int(code or 0)

+
+def is_robots_body(text):
+    """Does this actually look like a robots.txt?
+
+    A misconfigured host can answer /robots.txt with 200 and an HTML error page.
+    That body parses to zero rules, and zero rules read as "allowed" - so a
+    soft-200 granted permission that was never given. An empty or whitespace-only
+    body IS a valid allow-all under RFC 9309 and stays allowed; a non-empty body
+    with no recognised directive is treated as unreadable.
+    """
+    if not text.strip():
+        return True
+    for raw in text.splitlines():
+        line = raw.split('#', 1)[0].strip().lower()
+        if ':' in line and line.split(':', 1)[0].strip() in (
+            'user-agent', 'allow', 'disallow', 'sitemap', 'crawl-delay', 'host',
+        ):
+            return True
+    return False
+
 def _groups(text):
    """user-agent -> [(is_allow, pattern)], tolerating blank lines inside a record."""
    out, agents, expect = {}, [], True
@@ -60,9 +84,15 @@ def _groups(text):
    return out

 def _match(pattern, path):
-    """RFC 9309 wildcard match; returns match length or -1."""
+    """RFC 9309 wildcard match; returns match length or -1.
+
+    The pattern is percent-decoded to match the already-decoded path. Without
+    this, "Disallow: /foo%20bar" never matched "/foo bar" and the rule was
+    silently skipped - a fail-open on any site that encodes its own rules.
+    """
    if pattern == '':
        return -1
+    pattern = unquote(pattern)
    rx = '^' + ''.join('.*' if c == '*' else ('$' if c == '$' else re.escape(c)) for c in pattern)
    return len(pattern) if re.match(rx, path) else -1

@@ -91,6 +121,9 @@ def gate(url):
        if code == 404:
            return 0, 'ALLOWED - no robots.txt published'
        if code == 200:
+            if not is_robots_body(text):
+                last = 'HTTP 200 but the body is not a robots.txt'
+                continue
            body = text; break
        last = 'HTTP %d' % code
    if body is None: