mirror of
https://github.com/MadsLorentzen/ai-job-search.git
synced 2026-11-02 13:12:50 +00:00
fix(robots_check): match wildcard rules in linear time and hand curl only http(s) URLs (#779)
Two findings on tools/robots_check.py, reported privately by Martin Vlach
(@Huge, BudgetScan).
GHSA-xhqf-8q8x-4jwm: _match turned every '*' into a regex '.*', so a rule
with repeated wildcard segments that does not match the path
("Disallow: /*a*a*a...*b$") sent Python's backtracking engine into
exponential time - minutes at 16 segments - in the main process, where
no curl timeout applies, on a file the site being checked controls. The
matcher is now the reporter's linear table walk in O(len(pattern) *
len(path)) with no regex and no backtracking; consecutive '*' match as
one. A 200,000-case differential fuzz against the old regex matcher
found no change in answers, and three tests pin the prefix, anchored and
slash-spanning semantics.
GHSA-h8vc-753p-wpmh: gate() rebuilt the target as scheme://host/robots.txt
without checking the scheme, so a direct call with file:, gopher: or ftp:
passed that scheme to curl -L, which could also follow a redirect to any
protocol it was built with. gate() now fails closed with UNCONFIRMED on
any scheme but http(s) and on a URL without a host, before curl runs, and
curl gets --proto =http,https --proto-redir =http,https.
Seven new tests, all failing before this change.
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
d83945f136
commit
4f2083bfbd
3 files changed
+178
-5
No files matched your search
@@ -479,6 +479,25 @@ per-file diff commands.
|
||||
the `texlive/texlive:latest` leg exists to track current TeX Live, and a digest pin without
|
||||
automated updates would freeze both legs. The test reads the workflow text with the stdlib, so
|
||||
it needs neither zizmor nor PyYAML.
|
||||
- **`robots_check.py` matches wildcard rules in linear time** (`tools/robots_check.py` `_match`,
|
||||
`tests/test_robots_check.py`) - every `*` in a robots.txt rule became a regex `.*`, so a rule with
|
||||
repeated wildcard segments that does not match the path (`Disallow: /*a*a*a...*b$`) sent Python's
|
||||
backtracking engine into exponential time: 16 segments took minutes of CPU, on a file the site
|
||||
being checked controls, in the main process where no curl timeout applies. The matcher is now a
|
||||
table walk in `O(len(pattern) * len(path))` with no regex and no backtracking; consecutive `*`
|
||||
match as one, and three tests pin the old matcher's answers for prefix, anchored and
|
||||
slash-spanning rules. Reported privately by @Huge (Martin Vlach), whose linear matcher this is.
|
||||
|
||||
- **`robots_check.py` only ever hands http(s) URLs to curl** (`tools/robots_check.py` `gate`,
|
||||
`_fetch`, `tests/test_robots_check.py`) - `gate()` rebuilt the target as
|
||||
`scheme://host/robots.txt` without checking the scheme, so a direct call with `file:`, `gopher:`
|
||||
or `ftp:` passed that scheme to `curl -L`, which was also free to follow a redirect to any
|
||||
protocol it was built with. Reach was small (the response is never printed, and the main fetch
|
||||
path in `fetch_page.py` already allowed only http(s) per hop), but the tool is pre-approved.
|
||||
`gate()` now fails closed with `UNCONFIRMED` on any scheme but http(s) and on a URL without a
|
||||
host, before curl runs, and curl itself gets `--proto =http,https --proto-redir =http,https`.
|
||||
Reported privately by @Huge (Martin Vlach).
|
||||
|
||||
|
||||
- **The ask list now covers the files the allow list runs, and the guard reads load-time commands**
|
||||
(`.claude/settings.json`, `tools/security_guards.py` checks 4 and 5, `SECURITY.md`,
|
||||
|
||||
@@ -73,6 +73,47 @@ class TestPathRules(unittest.TestCase):
|
||||
self.assertFalse(allowed(body, "*", "/tag/$var"))
|
||||
self.assertFalse(allowed(body, "*", "/tag/$var/sub"))
|
||||
|
||||
def test_repeated_wildcards_do_not_catastrophically_backtrack(self):
|
||||
# GHSA-xhqf-8q8x-4jwm: every '*' became a regex '.*', so a rule with
|
||||
# repeated wildcard segments that does not match took exponential time
|
||||
# in Python's backtracking engine (16 segments: minutes). The matcher
|
||||
# must run in O(len(pattern) * len(path)) whatever the rule says.
|
||||
import time
|
||||
|
||||
pat = "/" + "*a" * 13 + "*b$"
|
||||
body = f"User-agent: *\nDisallow: {pat}\n"
|
||||
path = "/" + "a" * 28 + "X"
|
||||
start = time.perf_counter()
|
||||
res = allowed(body, "*", path)
|
||||
elapsed = time.perf_counter() - start
|
||||
self.assertTrue(res)
|
||||
self.assertLess(elapsed, 0.5, f"_match took {elapsed:.2f}s (catastrophic backtracking)")
|
||||
|
||||
def test_consecutive_wildcards_match_identically_to_single_wildcard(self):
|
||||
body = "User-agent: *\nDisallow: /docs/*******.pdf$\n"
|
||||
self.assertFalse(allowed(body, "*", "/docs/cv.pdf"))
|
||||
self.assertFalse(allowed(body, "*", "/docs/sub/folder/cv.pdf"))
|
||||
self.assertTrue(allowed(body, "*", "/docs/sub/folder/cv.html"))
|
||||
|
||||
def test_wildcard_semantics_survive_the_matcher_change(self):
|
||||
# Pinned against the regex matcher's answers: prefix match without '$',
|
||||
# full match with it, a wildcard spanning slashes and the empty string,
|
||||
# and a trailing wildcard before the anchor.
|
||||
body = (
|
||||
"User-agent: *\n"
|
||||
"Disallow: /a*b\n"
|
||||
"Disallow: /x/*/y$\n"
|
||||
"Disallow: /end*$\n"
|
||||
)
|
||||
self.assertFalse(allowed(body, "*", "/ab"))
|
||||
self.assertFalse(allowed(body, "*", "/a/long/path/b/and-more"))
|
||||
self.assertTrue(allowed(body, "*", "/a"))
|
||||
self.assertFalse(allowed(body, "*", "/x/anything/at/all/y"))
|
||||
self.assertTrue(allowed(body, "*", "/x/anything/y/more"))
|
||||
self.assertFalse(allowed(body, "*", "/end"))
|
||||
self.assertFalse(allowed(body, "*", "/ending/here"))
|
||||
self.assertTrue(allowed(body, "*", "/en"))
|
||||
|
||||
def test_percent_encoded_query_string_matches_rule_in_gate(self):
|
||||
import robots_check
|
||||
|
||||
@@ -462,3 +503,78 @@ class TestNonAsciiCurlOutput(unittest.TestCase):
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
|
||||
class TestSchemeHardening(unittest.TestCase):
|
||||
"""GHSA-h8vc-753p-wpmh: only http(s) reaches curl, on the first hop and on
|
||||
every redirect.
|
||||
|
||||
gate() rebuilt the target as scheme://host/robots.txt but never checked the
|
||||
scheme, so a direct call with file:, gopher: or ftp: handed that scheme to
|
||||
curl, and curl -L was free to follow a redirect to any protocol it supports.
|
||||
"""
|
||||
|
||||
def _gate_without_curl(self, url):
|
||||
# Record rather than fail inside the spy: gate() swallows every
|
||||
# exception from _fetch into an UNCONFIRMED verdict, which would pass.
|
||||
import robots_check
|
||||
|
||||
fetched = []
|
||||
original = robots_check._fetch
|
||||
robots_check._fetch = lambda url, ua: fetched.append(url) or ("", 404)
|
||||
try:
|
||||
result = robots_check.gate(url)
|
||||
finally:
|
||||
robots_check._fetch = original
|
||||
self.assertEqual(fetched, [], f"curl was asked for {fetched}")
|
||||
return result
|
||||
|
||||
def test_non_http_scheme_is_unconfirmed_without_a_fetch(self):
|
||||
for url in ("file:///etc/passwd", "gopher://127.0.0.1:6379/_", "ftp://x.example/robots.txt", "x.example/path"):
|
||||
with self.subTest(url=url):
|
||||
rc, msg = self._gate_without_curl(url)
|
||||
self.assertEqual(rc, 1)
|
||||
self.assertTrue(msg.startswith("UNCONFIRMED"), msg)
|
||||
self.assertIn("scheme", msg)
|
||||
|
||||
def test_missing_host_is_unconfirmed_without_a_fetch(self):
|
||||
rc, msg = self._gate_without_curl("http:///robots.txt")
|
||||
self.assertEqual(rc, 1)
|
||||
self.assertTrue(msg.startswith("UNCONFIRMED"), msg)
|
||||
|
||||
def test_curl_is_pinned_to_http_and_https_on_every_hop(self):
|
||||
import robots_check
|
||||
|
||||
seen = []
|
||||
|
||||
class Done:
|
||||
returncode, stdout, stderr = 0, "\n404", ""
|
||||
|
||||
def fake_run(argv, **kwargs):
|
||||
seen.append(argv)
|
||||
return Done()
|
||||
|
||||
original = robots_check.subprocess.run
|
||||
robots_check.subprocess.run = fake_run
|
||||
try:
|
||||
body, code = robots_check._fetch("https://x.example/robots.txt", "Claude-User")
|
||||
finally:
|
||||
robots_check.subprocess.run = original
|
||||
self.assertEqual(code, 404)
|
||||
argv = seen[0]
|
||||
self.assertIn("--proto", argv)
|
||||
self.assertEqual(argv[argv.index("--proto") + 1], "=http,https")
|
||||
self.assertIn("--proto-redir", argv)
|
||||
self.assertEqual(argv[argv.index("--proto-redir") + 1], "=http,https")
|
||||
self.assertLess(argv.index("--proto-redir"), argv.index("--"))
|
||||
|
||||
def test_real_curl_refuses_a_file_url_through_fetch(self):
|
||||
import shutil
|
||||
|
||||
import robots_check
|
||||
|
||||
if not shutil.which("curl"):
|
||||
self.skipTest("curl is not installed")
|
||||
target = (REPO_ROOT / "tools" / "robots_check.py").resolve().as_uri()
|
||||
with self.assertRaises(RuntimeError):
|
||||
robots_check._fetch(target, "Claude-User")
|
||||
+43
-5
@@ -19,6 +19,9 @@ Rules implemented (RFC 9309), deliberately on the cautious side:
|
||||
* any other failure to read robots.txt leaves permission unconfirmed,
|
||||
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
|
||||
* only an http or https URL with a host is checked at all; anything else
|
||||
is unconfirmed without a request, and curl itself is pinned to http(s)
|
||||
on the first hop and on every redirect
|
||||
|
||||
Usage: python3 tools/robots_check.py <url>
|
||||
Exit 0 = the retry may proceed. Exit 1 = do not retry; go to escalation step 3.
|
||||
@@ -37,8 +40,12 @@ def _fetch(url, ua):
|
||||
# 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.
|
||||
# --proto pins the first hop to http(s) and --proto-redir every redirect
|
||||
# -L follows, so neither the caller nor the site can steer curl to file:,
|
||||
# gopher:, ftp: or any other protocol it was built with (GHSA-h8vc-753p-wpmh).
|
||||
r = subprocess.run(
|
||||
['curl', '-sS', '-L', '--max-redirs', '5', '--max-time', '12', '-A', ua,
|
||||
['curl', '-sS', '-L', '--proto', '=http,https', '--proto-redir', '=http,https',
|
||||
'--max-redirs', '5', '--max-time', '12', '-A', ua,
|
||||
'-H', 'Accept: text/plain,*/*', '-w', '\n%{http_code}', '--', url],
|
||||
# robots.txt is UTF-8 (RFC 9309). Unpinned, Turkish Windows decoded it as
|
||||
# cp1254, where bytes like 0x81 are undefined: the pipe reader thread died,
|
||||
@@ -124,14 +131,39 @@ def _match(pattern, path):
|
||||
silently skipped - a fail-open on any site that encodes its own rules.
|
||||
Under RFC 9309 section 2.2.2, '$' anchors the end of the URL only when
|
||||
at the end of the pattern; mid-pattern '$' is matched literally.
|
||||
|
||||
Matching is a table walk in O(len(pattern) * len(path)), never a regex.
|
||||
Each '*' used to become '.*', and a non-matching rule with repeated
|
||||
wildcard segments ("/*a*a*a...*b$") sent Python's backtracking engine into
|
||||
exponential time - minutes at 16 segments - on a file the site controls
|
||||
(GHSA-xhqf-8q8x-4jwm). Consecutive '*' match as one, as RFC 9309 implies.
|
||||
"""
|
||||
if pattern == '':
|
||||
return -1
|
||||
pattern = unquote(pattern)
|
||||
end_anchor = pattern.endswith('$')
|
||||
pat = pattern[:-1] if end_anchor else pattern
|
||||
rx = '^' + ''.join('.*' if c == '*' else re.escape(c) for c in pat) + ('$' if end_anchor else '')
|
||||
return len(pattern) if re.match(rx, path) else -1
|
||||
pat = re.sub(r'\*{2,}', '*', pattern[:-1] if end_anchor else pattern)
|
||||
# reach[j] is True when the first j pattern characters match the path
|
||||
# consumed so far. A '*' extends a match over any character (reach[j]
|
||||
# carries forward) and also matches nothing (reach[j-1] on the same row).
|
||||
reach = [True] + [False] * len(pat)
|
||||
for j, c in enumerate(pat, 1):
|
||||
if c != '*':
|
||||
break
|
||||
reach[j] = True
|
||||
if reach[-1] and not end_anchor:
|
||||
return len(pattern)
|
||||
for ch in path:
|
||||
step = [False] * (len(pat) + 1)
|
||||
for j, c in enumerate(pat, 1):
|
||||
if c == '*':
|
||||
step[j] = step[j - 1] or reach[j]
|
||||
elif c == ch:
|
||||
step[j] = reach[j - 1]
|
||||
reach = step
|
||||
if reach[-1] and not end_anchor:
|
||||
return len(pattern) # a prefix match is enough without '$'
|
||||
return len(pattern) if reach[-1] else -1
|
||||
|
||||
def allowed(text, agent, path):
|
||||
g = _groups(text)
|
||||
@@ -144,7 +176,13 @@ def allowed(text, agent, path):
|
||||
return True if best_len < 0 else best_allow
|
||||
|
||||
def gate(url):
|
||||
parts = urlsplit(url)
|
||||
parts = urlsplit(url) # urlsplit lower-cases the scheme
|
||||
# Only http(s) with a host is a page this tool can have a policy for; any
|
||||
# other input fails closed before curl is asked anything.
|
||||
if parts.scheme not in ('http', 'https'):
|
||||
return 1, f'UNCONFIRMED (unsupported URL scheme {parts.scheme!r}) - do not retry, go to step 3'
|
||||
if not parts.netloc:
|
||||
return 1, 'UNCONFIRMED (no host in the URL) - do not retry, go to step 3'
|
||||
path = unquote(parts.path) or '/'
|
||||
if parts.query:
|
||||
path += '?' + unquote(parts.query)
|
||||
|
||||
Reference in new issue
Block a user