diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ba8aed0..49c45ff1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,25 @@ per-file diff commands. ### Fixed +- **`robots_check` no longer reads a leading BOM or an undecodable rule as permission** + (`tools/robots_check.py`, `tests/test_robots_check.py`) - two decoding edge cases failed + open, both flagged as follow-ups in #506. A robots.txt saved with a UTF-8 byte-order mark + decodes to a body that starts with U+FEFF, so its first field was not `user-agent`: a + leading `User-agent: *` went unseen, every rule in that group was dropped for want of an + agent, and the gate read the file as allow-all. Reproduced through the CLI on real hosts: + cnnturk.com and sakarya.edu.tr serve `EF BB BF` + `User-agent: *`, and master printed + `ALLOWED` for their disallowed `/hesap/` and `/bin/`. `_groups()` and `is_robots_body()` + now skip one leading U+FEFF, as Google's reference parser does, so every caller of + `allowed()` is covered (a BOM-only body is now the empty file it is, allow-all, rather than + a soft 200). Separately, #506's `errors='replace'` turns raw non-ASCII bytes in a + non-conformant robots.txt saved in a legacy code page (cp1254, ISO-8859-9) into U+FFFD, so + a rule such as `Disallow: /şirket/` could never match and was silently skipped. A + User-agent, Allow or Disallow line holding U+FFFD now makes the body unreadable, and the + gate prints `UNCONFIRMED`. The field is named with U+FFFD removed, so a stray byte before + `Disallow` cannot hide the line either; U+FFFD in a comment or another field stays + harmless, and valid UTF-8 rules decide as before. Nine new tests; the seven that pin the + fail-opens fail on master. + - **`verify_layout.py` finds an orphaned entry header by where the text starts** (`tools/verify_layout.py`, `tests/test_verify_layout.py`) - the orphan rule compared line left edges against the document margin, and a list marker moves a line's left edge without diff --git a/tests/test_robots_check.py b/tests/test_robots_check.py index 66787481..6c0044f2 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, is_robots_body # noqa: E402 +from robots_check import allowed, has_undecodable_rule, is_robots_body # noqa: E402 # Real body served by privatebank.barclays.com: blank lines sit between the @@ -211,6 +211,134 @@ class TestPercentEncodedRules(unittest.TestCase): self.assertTrue(allowed(JOBUP, "*", "/en/jobs/x")) +class TestByteOrderMark(unittest.TestCase): + """One leading UTF-8 byte-order mark is skipped before parsing. + + FAIL-OPEN REGRESSION: a robots.txt saved with a BOM decodes to a body that + starts with U+FEFF, so its first field was U+FEFF + "user-agent", which is + not "user-agent". A leading "User-agent: *" went unseen, every rule in its + group was dropped for want of an agent, and "Disallow: /" read as + allow-all. Google's reference parser skips the BOM; Python's robotparser + does not. + """ + + BODY = "\ufeffUser-agent: *\nDisallow: /\n" + + def test_bom_does_not_hide_the_first_group(self): + self.assertFalse(allowed(self.BODY, "*", "/x")) + self.assertFalse(allowed(self.BODY, "Claude-User", "/x")) + + def test_gate_obeys_a_policy_saved_with_a_bom(self): + import robots_check + + original = robots_check._fetch + robots_check._fetch = lambda url, ua: (self.BODY, 200) + try: + rc, msg = robots_check.gate("https://bom.example/jobs") + finally: + robots_check._fetch = original + self.assertEqual(rc, 1) + self.assertIn("DISALLOWED", msg) + + def test_is_robots_body_skips_the_bom_too(self): + """A lone directive behind a BOM read as a soft 200 (fail-closed, but + wrong). A BOM-only file is an empty file: allow-all, as RFC 9309 says.""" + self.assertTrue(is_robots_body("\ufeffSitemap: https://x.example/sitemap.xml\n")) + self.assertTrue(is_robots_body("\ufeff")) + + +class TestUndecodableRules(unittest.TestCase): + """A User-agent, Allow or Disallow line holding U+FFFD is unreadable. + + FAIL-OPEN REGRESSION: read as the UTF-8 that RFC 9309 requires, with bad + bytes replaced, a non-conformant robots.txt saved in cp1254 turns + "Disallow: /şirket/" into a pattern holding U+FFFD. It can never match, so + the rule was silently skipped and the path read as allowed. The gate now + leaves permission unconfirmed instead. U+FFFD in a comment or an unrelated + line decides nothing and must stay harmless. + """ + + @staticmethod + def _legacy(text): + """`text` saved in cp1254, then read as UTF-8 with bad bytes replaced.""" + return text.encode("cp1254").decode("utf-8", "replace") + + def _gate(self, body, url): + import robots_check + + original = robots_check._fetch + robots_check._fetch = lambda url, ua: (body, 200) + try: + return robots_check.gate(url) + finally: + robots_check._fetch = original + + def test_undecodable_disallow_is_unconfirmed_not_allowed(self): + body = self._legacy("User-agent: *\nDisallow: /şirket/\n") + self.assertIn("\ufffd", body) + rc, msg = self._gate(body, "https://legacy.example/şirket/x") + self.assertEqual(rc, 1, msg) + self.assertIn("UNCONFIRMED", msg) + self.assertIn("not valid UTF-8", msg) + + def test_undecodable_star_agent_is_unconfirmed(self): + """A trailing 0xA0 is a no-break space in cp1254, which strip() removes, + but U+FFFD in UTF-8, which it does not: "*" became another agent and + the group's Disallow stopped applying.""" + body = self._legacy("User-agent: *\xa0\nDisallow: /\n") + rc, msg = self._gate(body, "https://legacy.example/jobs") + self.assertEqual(rc, 1, msg) + self.assertIn("UNCONFIRMED", msg) + + def test_undecodable_byte_before_the_field_is_unconfirmed(self): + """A 0xA0 indent garbled the field name, so the Disallow went unseen.""" + body = self._legacy("User-agent: *\n\xa0Disallow: /jobs\n") + rc, msg = self._gate(body, "https://legacy.example/jobs") + self.assertEqual(rc, 1, msg) + self.assertIn("UNCONFIRMED", msg) + + def test_every_rule_line_is_checked(self): + """User-agent, Allow and Disallow alike, and a first line behind a BOM: + _groups skips the BOM, so the check must too or it misses that line.""" + for body in ( + self._legacy("User-agent: Bot\xa0\n"), + self._legacy("Allow: /ş/\n"), + self._legacy("Disallow: /ş/\n"), + "\ufeff" + self._legacy("User-agent: *\xa0\nDisallow: /\n"), + ): + with self.subTest(body=body): + self.assertTrue(has_undecodable_rule(body)) + + def test_fffd_in_a_comment_or_an_unrelated_line_is_harmless(self): + body = self._legacy( + "# Şirket politikası\n" + "User-agent: *\n" + "Disallow: /api/ # özel\n" + "Sitemap: https://legacy.example/şirket.xml\n" + "Açıklama: iş ilanları\n" + ) + # Every line but the User-agent holds U+FFFD, so the case is not vacuous. + clean = [line for line in body.splitlines() if "\ufffd" not in line] + self.assertEqual(clean, ["User-agent: *"]) + self.assertFalse(has_undecodable_rule(body)) + rc, msg = self._gate(body, "https://legacy.example/jobs") + self.assertEqual(rc, 0, msg) + rc, msg = self._gate(body, "https://legacy.example/api/x") + self.assertEqual(rc, 1, msg) + self.assertIn("DISALLOWED", msg) + + def test_valid_utf8_non_ascii_rules_are_still_obeyed(self): + """Real UTF-8 never decodes to U+FFFD. The first rule is tr.indeed.com's.""" + body = "User-agent: *\nDisallow: /職涯貼士/\nDisallow: /şirket/\n" + self.assertFalse(has_undecodable_rule(body)) + for path in ("/職涯貼士/x", "/şirket/x"): + rc, msg = self._gate(body, "https://tr.indeed.example" + path) + self.assertEqual(rc, 1, msg) + self.assertIn("DISALLOWED", msg) + rc, msg = self._gate(body, "https://tr.indeed.example/cmp/x/reviews") + self.assertEqual(rc, 0, msg) + + class TestArgumentHardening(unittest.TestCase): """A URL can never be read by curl as an option. diff --git a/tools/robots_check.py b/tools/robots_check.py index c17cab65..3587c8cf 100644 --- a/tools/robots_check.py +++ b/tools/robots_check.py @@ -13,9 +13,12 @@ Rules implemented (RFC 9309), deliberately on the cautious side: * a Disallow for either "*" or "Claude-User" blocks the retry * blank lines inside a record do not end it (Python's robotparser drops rules in that case, which fails open - see tests) + * one leading UTF-8 byte-order mark is skipped (left in, it hides a first + "User-agent" line and drops that whole group, which fails open) * 404 means no published policy, which is permission * any other failure to read robots.txt leaves permission unconfirmed, - and the retry does not happen + and the retry does not happen - including a User-agent, Allow or + Disallow line holding U+FFFD, i.e. a byte that is not valid UTF-8 Usage: python3 tools/robots_check.py Exit 0 = the retry may proceed. Exit 1 = do not retry; go to escalation step 3. @@ -56,6 +59,7 @@ def is_robots_body(text): body IS a valid allow-all under RFC 9309 and stays allowed; a non-empty body with no recognised directive is treated as unreadable. """ + text = text.removeprefix('\ufeff') # byte-order mark, see _groups if not text.strip(): return True for raw in text.splitlines(): @@ -66,10 +70,36 @@ def is_robots_body(text): return True return False +def has_undecodable_rule(text): + """Does a User-agent, Allow or Disallow line hold U+FFFD? + + U+FFFD stands in for a byte that did not decode as UTF-8. A non-conformant + robots.txt saved in a legacy code page (cp1254, ISO-8859-9) with raw + non-ASCII bytes in a rule yields a pattern that can never match: the rule + was silently skipped and the path read as allowed. Such a body is + unreadable, not permissive. The field is named with U+FFFD removed, so a + stray byte before "Disallow" cannot hide the line. U+FFFD in a comment or + in any other field (Sitemap, Crawl-delay, ...) decides nothing and is + ignored. + """ + for raw in text.removeprefix('\ufeff').splitlines(): + line = raw.split('#', 1)[0] + if '\ufffd' in line and ':' in line: + field = line.split(':', 1)[0].replace('\ufffd', '').strip().lower() + if field in ('user-agent', 'allow', 'disallow'): + return True + return False + def _groups(text): - """user-agent -> [(is_allow, pattern)], tolerating blank lines inside a record.""" + """user-agent -> [(is_allow, pattern)], tolerating blank lines inside a record. + + One leading U+FEFF is skipped, as Google's reference parser does. A file + saved with a UTF-8 byte-order mark otherwise hid its first field: a leading + "User-agent: *" went unseen, every rule in its group was dropped for want + of an agent, and "Disallow: /" read as allow-all. + """ out, agents, expect = {}, [], True - for raw in text.splitlines(): + for raw in text.removeprefix('\ufeff').splitlines(): line = raw.split('#', 1)[0].strip() if not line or ':' not in line: continue @@ -127,6 +157,9 @@ def gate(url): if not is_robots_body(text): last = 'HTTP 200 but the body is not a robots.txt' continue + if has_undecodable_rule(text): + last = 'HTTP 200 but a User-agent/Allow/Disallow line is not valid UTF-8' + continue body = text; break last = 'HTTP %d' % code if body is None: