From c71c270765471f0fc9e6028b70bf88babbec6401 Mon Sep 17 00:00:00 2001 From: abdulwasea89 <165668178+abdulwasea89@users.noreply.github.com> Date: Wed, 21 Oct 2026 10:06:40 +0500 Subject: [PATCH] fix(fetch-page): check robots policy before each redirect (#680) * fix(fetch-page): check robots policy before each redirect * changelog: place the #680 entry under [Unreleased] Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Mads Lorentzen Co-authored-by: Claude Opus 5.5 --- .../09-web-research.md | 6 +- CHANGELOG.md | 15 ++ SECURITY.md | 2 +- tests/test_fetch_page.py | 131 +++++++++++++++- tests/test_posting_gate_wiring.py | 2 +- tools/fetch_page.py | 141 ++++++++++++------ 6 files changed, 247 insertions(+), 50 deletions(-) diff --git a/.claude/skills/job-application-assistant/09-web-research.md b/.claude/skills/job-application-assistant/09-web-research.md index d9aae98a..56c8021c 100644 --- a/.claude/skills/job-application-assistant/09-web-research.md +++ b/.claude/skills/job-application-assistant/09-web-research.md @@ -1,5 +1,5 @@ --- -framework_version: 1.1.6 +framework_version: 1.1.7 --- # Web Research and Fetching @@ -54,7 +54,7 @@ Two details worth knowing, both covered by `tests/test_robots_check.py`: python3 tools/fetch_page.py '' --out "${SCRATCHPAD:?set this to the session scratchpad directory from your system prompt}/page.html" --text ``` -One command does the whole retry: it runs the `robots.txt` gate above (exit `1` and no fetch when the policy says no), GETs the page with browser headers, saves the body to `--out`, prints `HTTP size= saved=`, and with `--text` prints the tag-stripped text (script, style, noscript, svg and comments removed, entities unescaped) capped at `--max-chars` (default 6000). It is GET-only with a fixed argv, http(s) only, bounded to 45 s, 5 redirects and 10 MB, and it refuses an `--out` inside the repo - fetched content is untrusted and belongs in the scratchpad. That fixed shape is why the workflow pre-approves `python3 tools/fetch_page.py` and not `curl`. +One command does the whole retry: it runs the `robots.txt` gate above before the first GET **and before each redirect target is fetched** (exit `1` and no fetch when the policy says no), GETs the page with browser headers, saves the body to `--out`, prints `HTTP size= saved=`, and with `--text` prints the tag-stripped text (script, style, noscript, svg and comments removed, entities unescaped) capped at `--max-chars` (default 6000). It is GET-only with a fixed argv, http(s) only, bounded to 45 s, 5 redirects and 10 MB, and it refuses an `--out` inside the repo - fetched content is untrusted and belongs in the scratchpad. That fixed shape is why the workflow pre-approves `python3 tools/fetch_page.py` and not `curl`. The HTTP status is information, not a verdict: a `403` body still tells you what the site answered. Only exit `2` (unsupported scheme, transport failure, oversized response) means nothing was retrieved. @@ -72,7 +72,7 @@ Try these in order and stop at the first that yields real content: ``` This retrieves the posting directly from the portal's API or resilient detail route, bypassing bot-detection 403s and login walls entirely. 2. **`WebFetch`** on the target URL (for non-portal URLs, or if the portal CLI is missing or failed). Cheapest for general web pages, returns clean markdown. -3. **`python3 tools/fetch_page.py '' --out "$SCRATCHPAD/page.html" --text`** (above). It checks `robots.txt` itself, retries with browser headers and strips the tags. Fixes the 403 class of failure. An exit of `1` means the site's policy disallows the path for `*` or `Claude-User`: the retry did not run, so go to step 4. +3. **`python3 tools/fetch_page.py '' --out "$SCRATCHPAD/page.html" --text`** (above). It checks `robots.txt` on every hop, retries with browser headers and strips the tags. Fixes the 403 class of failure. An exit of `1` means the starting URL or a redirect target is disallowed for `*` or `Claude-User`: go to step 4. 4. **`WebSearch`** for the company or role by name, to find an alternative canonical URL: the employer's own careers portal is almost always richer than the aggregator that surfaced the posting, and it carries the reference ID and grade that aggregators drop. 5. **Declare it genuinely unavailable** only after 1 to 4 have failed. In `/rank` that means marking the entry `unreachable`, which later runs retry (`expired` is only for a posting shown to be closed - see `rank.md` Step 2); in `/apply` it means telling the user the posting could not be retrieved and stopping rather than drafting from the title. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d3feded..9c0607c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -313,6 +313,21 @@ per-file diff commands. salary indices of 1 and 0. Numeric 0 and 1 remain valid, and a count/index pair retains its numeric half when the other cell is logical. +- **`fetch_page.py` checks `robots.txt` before every redirect it follows** (#680, `tools/fetch_page.py`, + `tests/test_fetch_page.py`, `SECURITY.md`, `tests/test_posting_gate_wiring.py`, + `.claude/skills/job-application-assistant/09-web-research.md` `framework_version: 1.1.6 -> 1.1.7`) - + the gate ran once, on the URL it was given, and `curl -L` or urllib's redirect handler then followed + up to five redirects unchecked, so an allowed posting URL that redirected to a path the site's + `robots.txt` disallows, or to another host whose policy says no, was fetched and saved anyway. The + script now follows redirects itself: each hop must be http(s) and passes the gate for its own host + before it is requested, and a refusal exits 1 without requesting it. The 5-redirect cap and the 45 s + budget now hold across all hops on both the curl and the urllib path, and the body goes to a temp file + that replaces `--out` only on success. A non-conformant `Location` (a raw space or raw UTF-8) is + percent-encoded the way urllib's own redirect handler does it. This also closes three gaps: curl + followed an `ftp:` redirect, a size failure left a partial file at `--out`, and the urllib path had + no total deadline. `size=` in the status line is now the saved file's size on both paths, and step 3 + of `09-web-research.md` says an exit 1 can come from a redirect target's policy. + ### Security - **CI no longer leaves the job token in `.git/config` or expands a PR-controlled name into a diff --git a/SECURITY.md b/SECURITY.md index f539ab10..23e015e8 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -31,7 +31,7 @@ Every grant the template ships, where it lives, how long it lasts, and what chec | Hooks (`.claude/settings.json` `hooks`) | None. The template ships no hooks: a hook runs with no prompt the moment its event fires, which is the vector the Shai-Hulud worm used in August 2026. | - | Any hook fails CI | | Compiling (`lualatex`, `xelatex`, a custom template's command) | Not pre-approved: a `lualatex:*` grant would also approve `-shell-escape`. In default mode each compile prompts. Answering "don't ask again" would persist exactly that `lualatex` rule to your `settings.local.json`, so answer per call. | - | - | | Portal CLI manifests (`.agents/**/package.json`) | No lifecycle scripts (`preinstall`, `postinstall`, ...) and no `trustedDependencies`, so `bun install` runs no code | - | Fails CI; `tools/install_portal_clis.py` also skips a manifest that carries one | -| The browser-header retry (`tools/fetch_page.py`) | A GET with a fixed argv, http(s) only, 45 s / 5 redirects / 10 MB, `robots.txt` enforced with no bypass flag, output refused inside the repo | - | Pinned by `tests/test_fetch_page.py`; the grant names the script, not `curl` | +| The browser-header retry (`tools/fetch_page.py`) | A GET with a fixed argv, http(s) only, 45 s / 5 redirects / 10 MB, `robots.txt` enforced before each redirect target with no bypass flag, output refused inside the repo | - | Pinned by `tests/test_fetch_page.py`; the grant names the script, not `curl` | ### What this does not do diff --git a/tests/test_fetch_page.py b/tests/test_fetch_page.py index bd72905a..c1d591fe 100644 --- a/tests/test_fetch_page.py +++ b/tests/test_fetch_page.py @@ -39,12 +39,52 @@ CONTENT_TYPES = {"/euc-kr.html": "text/html; charset=euc-kr"} class _Handler(http.server.BaseHTTPRequestHandler): def do_GET(self): # noqa: N802 (http.server naming) - if self.path == "/ok.html": + self.server.paths.append(self.path) + if self.path in ("/ok.html", "/ok.html?q=a%20b", "/ok.html?t=%C3%A6%C3%B8%C3%A5") or ( + self.path.startswith("/unicode/") and self.path.endswith("?page=2") + ): body = PAGE.encode("utf-8") self.send_response(200) + elif self.path.startswith("/unicode/"): + self.send_response(302) + self.send_header("Location", "?page=2") + self.send_header("Content-Length", "0") + self.end_headers() + return + elif self.path == "/redirect-utf8": + # http.server writes header values as latin-1; preserve raw UTF-8 bytes. + self.send_response(302) + self.send_header("Location", "/ok.html?t=" + "æøå".encode("utf-8").decode("latin-1")) + self.send_header("Content-Length", "0") + self.end_headers() + return elif self.path == "/euc-kr.html": body = KOREAN_PAGE.encode("cp949") self.send_response(200) + elif self.path == "/robots.txt": + body = b"User-agent: *\nAllow: /redirect\nDisallow: /blocked\n" + self.send_response(200) + elif self.path in ("/redirect", "/redirect-allowed"): + self.send_response(302) + self.send_header("Location", "/blocked" if self.path == "/redirect" else "/ok.html") + self.send_header("Content-Length", "0") + self.end_headers() + return + elif self.path == "/redirect-space": + self.send_response(302) + self.send_header("Location", "/ok.html?q=a b") + self.send_header("Content-Length", "0") + self.end_headers() + return + elif self.path == "/redirect-other": + self.send_response(302) + self.send_header("Location", self.server.redirect_to) + self.send_header("Content-Length", "0") + self.end_headers() + return + elif self.path == "/blocked": + body = b"blocked posting" + self.send_response(200) elif self.path == "/forbidden": body = b"denied" self.send_response(403) @@ -75,6 +115,7 @@ class FetchPageFixture(unittest.TestCase): @classmethod def setUpClass(cls): cls.server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _Handler) + cls.server.paths = [] cls.thread = threading.Thread(target=cls.server.serve_forever, daemon=True) cls.thread.start() cls.base = f"http://127.0.0.1:{cls.server.server_address[1]}" @@ -134,6 +175,90 @@ class RobotsGate(FetchPageFixture): self.run_main(f"{self.base}/ok.html", "--out", str(self.out)) self.assertEqual(self.gate_calls, [f"{self.base}/ok.html"]) + def test_redirect_target_is_checked_before_fetching(self): + # The documented retry follows redirects. An allowed posting URL can + # redirect to a path this site's robots.txt explicitly disallows. + fetch_page.gate = self._orig_gate + for path in ("curl", "urllib"): + with self.subTest(path=path): + if path == "urllib": + original_which = fetch_page.shutil.which + fetch_page.shutil.which = lambda name: None + self.addCleanup(setattr, fetch_page.shutil, "which", original_which) + self.server.paths.clear() + code, out, err = self.run_main(f"{self.base}/redirect", "--out", str(self.out)) + self.assertEqual(code, 1, out + err) + self.assertIn("DISALLOWED", out) + self.assertNotIn("/blocked", self.server.paths) + self.assertFalse(self.out.exists()) + + def test_allowed_redirect_still_fetches_the_posting(self): + fetch_page.gate = self._orig_gate + for path in ("curl", "urllib"): + with self.subTest(path=path): + if path == "urllib": + original_which = fetch_page.shutil.which + fetch_page.shutil.which = lambda name: None + self.addCleanup(setattr, fetch_page.shutil, "which", original_which) + self.server.paths.clear() + code, out, err = self.run_main(f"{self.base}/redirect-allowed", "--out", str(self.out)) + self.assertEqual(code, 0, out + err) + self.assertEqual(self.out.read_text(encoding="utf-8"), PAGE) + self.assertIn("/ok.html", self.server.paths) + self.assertGreaterEqual(self.server.paths.count("/robots.txt"), 2) + self.out.unlink() + + def test_raw_space_in_location_is_encoded_on_both_paths(self): + for path in ("curl", "urllib"): + with self.subTest(path=path): + if path == "urllib": + original_which = fetch_page.shutil.which + fetch_page.shutil.which = lambda name: None + self.addCleanup(setattr, fetch_page.shutil, "which", original_which) + self.server.paths.clear() + code, out, err = self.run_main(f"{self.base}/redirect-space", "--out", str(self.out)) + self.assertEqual(code, 0, out + err) + self.assertIn("/ok.html?q=a%20b", self.server.paths) + self.assertEqual(self.out.read_text(encoding="utf-8"), PAGE) + self.out.unlink() + + def test_raw_utf8_in_location_keeps_its_bytes_on_both_paths(self): + for path in ("curl", "urllib"): + with self.subTest(path=path): + if path == "urllib": + original_which = fetch_page.shutil.which + fetch_page.shutil.which = lambda name: None + self.addCleanup(setattr, fetch_page.shutil, "which", original_which) + self.server.paths.clear() + code, out, err = self.run_main(f"{self.base}/redirect-utf8", "--out", str(self.out)) + self.assertEqual(code, 0, out + err) + self.assertIn("/ok.html?t=%C3%A6%C3%B8%C3%A5", self.server.paths) + self.out.unlink() + + @unittest.skipUnless(shutil.which("curl"), "urllib cannot send a URL with raw non-ASCII characters") + def test_relative_redirect_from_a_unicode_url(self): + self.server.paths.clear() + code, out, err = self.run_main(f"{self.base}/unicode/日本", "--out", str(self.out)) + self.assertEqual(code, 0, out + err) + self.assertTrue(any(p.endswith("?page=2") for p in self.server.paths), self.server.paths) + + def test_cross_origin_redirect_uses_destination_policy(self): + destination = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _Handler) + destination.paths = [] + thread = threading.Thread(target=destination.serve_forever, daemon=True) + thread.start() + self.addCleanup(destination.server_close) + self.addCleanup(destination.shutdown) + self.server.redirect_to = f"http://127.0.0.1:{destination.server_address[1]}/blocked" + fetch_page.gate = self._orig_gate + + code, out, err = self.run_main(f"{self.base}/redirect-other", "--out", str(self.out)) + + self.assertEqual(code, 1, out + err) + self.assertIn("/robots.txt", destination.paths) + self.assertNotIn("/blocked", destination.paths) + self.assertFalse(self.out.exists()) + class Fetching(FetchPageFixture): def test_fetches_and_reports_status(self): @@ -232,7 +357,7 @@ class CurlArgvShape(unittest.TestCase): def test_curl_argv_is_get_only_and_terminates_options(self): url = "-https://example.com/x" # a URL that would read as a flag - argv = fetch_page.build_curl_argv(url, Path("out.html")) + argv = fetch_page.build_curl_argv(url, Path("out.html"), Path("headers.txt"), 45) self.assertEqual(argv[0], "curl") self.assertEqual(argv[-2:], ["--", url]) for forbidden in ("-d", "--data", "--data-binary", "--data-raw", "-F", "--form", @@ -240,6 +365,8 @@ class CurlArgvShape(unittest.TestCase): self.assertNotIn(forbidden, argv[:-1]) self.assertIn("--max-filesize", argv) self.assertIn("--max-redirs", argv) + self.assertEqual(argv[argv.index("--max-redirs") + 1], "0") + self.assertNotIn("-L", argv) self.assertIn("-A", argv) self.assertEqual(argv[argv.index("-A") + 1], fetch_page.BROWSER) diff --git a/tests/test_posting_gate_wiring.py b/tests/test_posting_gate_wiring.py index e64aa9c8..7dfcbd7d 100644 --- a/tests/test_posting_gate_wiring.py +++ b/tests/test_posting_gate_wiring.py @@ -78,7 +78,7 @@ class TrustBoundaryWiring(unittest.TestCase): text = RESEARCH.read_text(encoding="utf-8") boundary = re.search(r"## Trust boundary(.*?)(?=\n## )", text, re.DOTALL).group(1) self.assertIn("posting_guard", boundary) - self.assertIn("framework_version: 1.1.6", text) + self.assertIn("framework_version: 1.1.7", text) if __name__ == "__main__": diff --git a/tools/fetch_page.py b/tools/fetch_page.py index 95db9bb2..e27c04c9 100644 --- a/tools/fetch_page.py +++ b/tools/fetch_page.py @@ -15,10 +15,10 @@ Properties, each pinned by tests/test_fetch_page.py: never be parsed as an option. The urllib fallback (used only when curl is absent) has the same shape. * http(s) only. Any other scheme exits 2 before anything else happens. - * robots.txt is enforced, not advised. robots_check.gate() runs first and a - non-zero gate exits 1 with its message; no flag skips it. The retry is for - firewalls on sites whose published policy allows access, never for sites - that said no. + * robots.txt is enforced, not advised. robots_check.gate() runs before each + GET, including redirect targets, and a non-zero gate exits 1 with its + message; no flag skips it. The retry is for firewalls on sites whose + published policy allows access, never for sites that said no. * Fetched content never lands in the repo. --out defaults to the system temp directory and a path that resolves inside the repo root is refused. * Bounded: 45 s, 5 redirects, 10 MB - on both the curl and the urllib path, @@ -42,15 +42,18 @@ import argparse import gzip import hashlib import html +import os import re import shutil +import string import subprocess import sys import tempfile +import time import zlib from pathlib import Path from urllib.error import HTTPError -from urllib.parse import urlsplit +from urllib.parse import quote, urljoin, urlsplit from urllib.request import HTTPRedirectHandler, Request, build_opener sys.path.insert(0, str(Path(__file__).resolve().parent)) @@ -65,41 +68,66 @@ MAX_REDIRECTS = 5 MAX_BYTES = 10 * 1024 * 1024 -def build_curl_argv(url: str, out: Path) -> list: - """The one curl invocation this script ever runs. GET, bounded, `--` last.""" +def build_curl_argv(url: str, out: Path, headers: Path, timeout: float) -> list: + """One GET without automatic redirects, so the next hop can be gated.""" return [ - "curl", "-sSL", - "--max-redirs", str(MAX_REDIRECTS), - "--max-time", str(TIMEOUT_SECONDS), + "curl", "-sS", + "--max-redirs", "0", + "--max-time", str(timeout), "--max-filesize", str(MAX_BYTES), "--compressed", "-A", BROWSER, "-H", f"Accept: {ACCEPT}", "-H", f"Accept-Language: {ACCEPT_LANGUAGE}", + "-D", str(headers), "-o", str(out), - "-w", "HTTP %{http_code} size=%{size_download}", + "-w", "%{http_code}", "--", url, ] -def fetch_with_curl(url: str, out: Path) -> str: - proc = subprocess.run( - build_curl_argv(url, out), - capture_output=True, text=True, encoding="utf-8", errors="replace", - timeout=TIMEOUT_SECONDS + 15, - ) - if proc.returncode != 0: - raise RuntimeError(f"curl exit {proc.returncode}: {proc.stderr.strip()}") - return proc.stdout.strip() +def _location(header_text: str) -> str | None: + """Read Location from the final response (a proxy may add an earlier one).""" + for block in reversed(re.split(r"\r?\n\r?\n", header_text)): + if not block.startswith("HTTP/"): + continue + for line in block.splitlines(): + if line.lower().startswith("location:"): + return line.partition(":")[2].strip() + break + return None -class _BoundedRedirects(HTTPRedirectHandler): - """urllib's default is 10 hops; match curl's --max-redirs.""" - max_redirections = MAX_REDIRECTS +def fetch_with_curl(url: str, out: Path, timeout: float) -> tuple[int, str | None]: + fd, header_name = tempfile.mkstemp(prefix=".fetch_page_headers.") + os.close(fd) # curl must be able to open it on Windows too + headers = Path(header_name) + try: + proc = subprocess.run( + build_curl_argv(url, out, headers, timeout), + capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=timeout + 15, + ) + if proc.returncode != 0: + raise RuntimeError(f"curl exit {proc.returncode}: {proc.stderr.strip()}") + code = int(proc.stdout.strip()) + location = _location(headers.read_text(encoding="iso-8859-1")) + finally: + headers.unlink(missing_ok=True) + if out.stat().st_size > MAX_BYTES: + raise RuntimeError(f"response exceeds {MAX_BYTES} bytes after decompression") + return code, location -def fetch_with_urllib(url: str, out: Path) -> str: - """Fallback for machines without curl. Same headers and bounds, gzip/deflate only.""" +class _NoRedirects(HTTPRedirectHandler): + """Return the 3xx response to the caller for a robots check first.""" + + def redirect_request(self, request, response, code, message, headers, new_url): + return None + + +def fetch_with_urllib(url: str, out: Path, timeout: float) -> tuple[int, str | None]: + """Fallback for machines without curl. One GET, gzip/deflate only.""" request = Request(url, headers={ "User-Agent": BROWSER, "Accept": ACCEPT, @@ -107,15 +135,14 @@ def fetch_with_urllib(url: str, out: Path) -> str: "Accept-Encoding": "gzip, deflate", }) try: - response = build_opener(_BoundedRedirects()).open(request, timeout=TIMEOUT_SECONDS) # noqa: S310 + response = build_opener(_NoRedirects()).open(request, timeout=timeout) # noqa: S310 except HTTPError as exc: - if 300 <= exc.code < 400: - # Redirects are followed transparently; a 3xx surfacing here means - # the hop limit was hit (urllib reports a loop this way). - raise RuntimeError(f"more than {MAX_REDIRECTS} redirects") from exc response = exc with response: code = response.status if hasattr(response, "status") else response.code + location = response.headers.get("Location") + if code in (301, 302, 303, 307, 308) and location: + return code, location encoding = (response.headers.get("Content-Encoding") or "").lower() body = response.read(MAX_BYTES + 1) if len(body) > MAX_BYTES: @@ -127,7 +154,7 @@ def fetch_with_urllib(url: str, out: Path) -> str: if len(body) > MAX_BYTES: raise RuntimeError(f"response exceeds {MAX_BYTES} bytes after decompression") out.write_bytes(body) - return f"HTTP {code} size={len(body)}" + return code, location def strip_tags(markup: str, limit: int) -> str: @@ -195,19 +222,47 @@ def main(argv=None) -> int: ) return 2 - rc, message = gate(args.url) - print(message) - if rc != 0: - print( - "fetch_page: refused - robots.txt does not permit this path, so the " - "browser-header retry does not run. Go to escalation step 4: search for " - "the employer's own posting." - ) - return 1 - try: - out.parent.mkdir(parents=True, exist_ok=True) - status = fetch_with_curl(args.url, out) if shutil.which("curl") else fetch_with_urllib(args.url, out) + current = args.url + deadline = None + for redirects in range(MAX_REDIRECTS + 1): + scheme = urlsplit(current).scheme.lower() + if scheme not in ("http", "https"): + raise ValueError(f"redirect target must be http(s), got scheme {scheme!r}") + rc, message = gate(current) + print(message) + if rc != 0: + print( + "fetch_page: refused - robots.txt does not permit this path, so the " + "browser-header retry does not run. Go to escalation step 4: search for " + "the employer's own posting." + ) + return 1 + if deadline is None: + deadline = time.monotonic() + TIMEOUT_SECONDS + remaining = deadline - time.monotonic() + if remaining <= 0: + raise TimeoutError("fetch exceeded the time limit") + out.parent.mkdir(parents=True, exist_ok=True) + fd, body_name = tempfile.mkstemp(dir=out.parent, prefix=".fetch_page.", suffix=".tmp") + os.close(fd) # curl/urllib may reopen the path on Windows + body = Path(body_name) + try: + code, location = ( + fetch_with_curl(current, body, remaining) + if shutil.which("curl") else fetch_with_urllib(current, body, remaining) + ) + if code in (301, 302, 303, 307, 308) and location: + if redirects == MAX_REDIRECTS: + raise RuntimeError(f"more than {MAX_REDIRECTS} redirects") + # Encode the header's raw bytes before resolving; preserve Unicode in the base URL. + current = urljoin(current, quote(location, encoding="iso-8859-1", safe=string.punctuation)) + continue + os.replace(body, out) + status = f"HTTP {code} size={out.stat().st_size}" + break + finally: + body.unlink(missing_ok=True) except Exception as exc: # transport failures of any shape: report, exit 2 print(f"fetch_page: {type(exc).__name__}: {exc}", file=sys.stderr) return 2