diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d24dcf..66ad5f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/tests/test_robots_check.py b/tests/test_robots_check.py index 08983f9..08c7860 100644 --- a/tests/test_robots_check.py +++ b/tests/test_robots_check.py @@ -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("404 Not Found")) + + 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: ("404", 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() diff --git a/tools/robots_check.py b/tools/robots_check.py index 2e875f1..b59c972 100644 --- a/tools/robots_check.py +++ b/tools/robots_check.py @@ -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: