diff --git a/.claude/settings.json b/.claude/settings.json index 61871a64..513fc026 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -20,7 +20,6 @@ "Bash(python3 tools/verify_pdf.py:*)", "Bash(python tools/verify_layout.py:*)", "Bash(python3 tools/verify_layout.py:*)", - "Bash(pdftotext:*)", "Bash(python tools/robots_check.py:*)", "Bash(python3 tools/robots_check.py:*)", "Bash(python tools/fetch_page.py:*)", diff --git a/CHANGELOG.md b/CHANGELOG.md index 78362902..20eeba14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -493,6 +493,18 @@ per-file diff commands. template ships no hooks; only settings.json hooks were checked). Check 5 refuses an agent `permissionMode` that removes prompts and agent `hooks` or `mcpServers`. +- **Pre-approved PDF tooling can no longer write to arbitrary paths** (`tools/verify_pdf.py`, + `.claude/settings.json`, `tools/security_guards.py`, `tests/test_verify_pdf.py`) - + `settings.json` pre-approves `verify_pdf.py` with any arguments, and `--dump-text` wrote the + extracted text layer to whatever path it was given, creating missing parent directories and + overwriting an existing file: a prompt-free write anywhere on disk, with content taken from a + PDF that can carry posting-derived text. The CLI now writes only a `.txt` file under the repo's + `cv/` or `cover_letters/` (the place `apply.md` Step 5d documents) and refuses a symlinked + target. `Bash(pdftotext:*)` is dropped from the allowlist for the same reason - Poppler writes + to any output path and `pdftotext -` prints any PDF on disk into the session - and + `verify_pdf.py` already wraps it. The documented manual `pdftotext` fallback in Step 5d still + works; it now asks first. + ## [1.7.3] - 2026-10-19 ### Added diff --git a/SECURITY.md b/SECURITY.md index 4a807490..c7acd5ec 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -20,7 +20,7 @@ Every grant the template ships, where it lives, how long it lasts, and what chec | Surface | What is granted | Lifetime | Guard | |---|---|---|---| -| `.claude/settings.json` `permissions.allow` | Each shipped portal CLI by exact path, `salary_lookup.py`, the `tools/` scripts (`rank_state`, `job_key`, `verify_pdf`, `verify_layout`, `robots_check`, `fetch_page`), `pdftotext`, and `Skill(job-application-assistant)` | Whole session | Exact allowlist; any addition fails CI unless added to `ALLOWED_PERMISSIONS` in the same PR | +| `.claude/settings.json` `permissions.allow` | Each shipped portal CLI by exact path, `salary_lookup.py`, the `tools/` scripts (`rank_state`, `job_key`, `verify_pdf`, `verify_layout`, `robots_check`, `fetch_page`), and `Skill(job-application-assistant)` | Whole session | Exact allowlist; any addition fails CI unless added to `ALLOWED_PERMISSIONS` in the same PR | | `.claude/settings.json` `permissions.ask` | Always prompt, even over an allow rule or a skill grant: edits to `settings.json`, `settings.local.json`, any `.claude/skills/*/SKILL.md`, any `.claude/commands/*.md`, `.claude/agents/**`, `.github/**`, `tools/security_guards.py`; edits to the files the allow list runs by path (`tools/**`, `.agents/**`, `salary_lookup.py`); and `git push` | Whole session | Each rule is required; removal fails CI | | `.claude/settings.json` `permissions.deny` | Blocked in every mode, `bypassPermissions` included: `curl`/`wget` with data, form or upload flags, `scp`, `sftp`, `ssh`, `nc`; reads of `.env`, `.env.*`, `~/.ssh/**`, `~/.aws/**`, `~/.gnupg/**` (Read denies also cover Bash `cat`/`head`/`tail`/`sed`) | Whole session | Each rule is required; removal fails CI | | Skill `allowed-tools` (`.claude/skills/*/SKILL.md`) | `job-application-assistant`: Read, Glob, Grep, WebFetch, WebSearch, Edit, Write, AskUserQuestion. `scrape`: the same plus Agent, `bun --version`, `tools/job_key.py`. `upskill`: Read, Write, Glob, Grep, WebFetch, WebSearch. No bare `Bash` anywhere. | The turn that invokes the skill; clears on your next message. Not gated by workspace trust. | Exact allowlist (`ALLOWED_SKILL_TOOLS`); bare tools and wildcards fail CI | diff --git a/SETUP.md b/SETUP.md index 67592068..6aa053f6 100644 --- a/SETUP.md +++ b/SETUP.md @@ -344,7 +344,7 @@ Make sure Bun is installed and you ran `bun install` in each CLI directory. The The cover letter template expects fonts in `cover_letters/OpenFonts/fonts/`. Make sure this directory exists and contains the Lato and Raleway font files. ### Stale `.claude/settings.local.json` from an older clone -Shared Claude Code permissions now live in `.claude/settings.json`: an `allow` list scoped to the exact commands the workflow runs (each portal CLI by path, the `tools/` scripts, `salary_lookup.py`, `pdftotext`), an `ask` list that always prompts before the permission-defining files are edited or `git push` runs, and a `deny` list that blocks upload-shaped shell and reads of credential files in every mode. `SECURITY.md` has the full table. Earlier versions of this repo committed a broader `.claude/settings.local.json` that pre-approved `Bash(curl:*)`, `Bash(python:*)` and `Bash(bun:*)`. If you cloned before that change, git leaves the old file behind in your working copy, and its permissions still apply on top of `settings.json`. Delete it (or trim it to your own personal overrides): +Shared Claude Code permissions now live in `.claude/settings.json`: an `allow` list scoped to the exact commands the workflow runs (each portal CLI by path, the `tools/` scripts and `salary_lookup.py`), an `ask` list that always prompts before the permission-defining files are edited or `git push` runs, and a `deny` list that blocks upload-shaped shell and reads of credential files in every mode. `SECURITY.md` has the full table. Earlier versions of this repo committed a broader `.claude/settings.local.json` that pre-approved `Bash(curl:*)`, `Bash(python:*)` and `Bash(bun:*)`. If you cloned before that change, git leaves the old file behind in your working copy, and its permissions still apply on top of `settings.json`. Delete it (or trim it to your own personal overrides): ```bash rm .claude/settings.local.json diff --git a/tests/test_verify_pdf.py b/tests/test_verify_pdf.py index 6df55b86..84c776d8 100644 --- a/tests/test_verify_pdf.py +++ b/tests/test_verify_pdf.py @@ -5,12 +5,15 @@ from pathlib import Path from unittest.mock import patch from tools.verify_pdf import ( + ROOT, VerificationError, build_parser, + check_dump_target, extract_text_layer, find_non_ascii_date_ranges, normalize_text, parse_page_count, + main, run_tool, verify_pdf, ) @@ -331,5 +334,48 @@ class RunToolTests(unittest.TestCase): run_tool(["pdfinfo", "example.pdf"]) +class DumpTargetTests(unittest.TestCase): + """`.claude/settings.json` pre-approves verify_pdf.py with any arguments, so + `--dump-text` was a prompt-free write to any path on disk (it also created + missing parent directories). The CLI now writes only a .txt under the + repo's cv/ or cover_letters/, the place apply.md Step 5d documents.""" + + def setUp(self): + self.temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.temp_dir.cleanup) + self.root = Path(self.temp_dir.name) + (self.root / "cv").mkdir() + + def test_documented_target_is_allowed(self): + self.assertIsNone(check_dump_target(self.root / "cv" / "main_acme_analyst.txt", self.root)) + self.assertIsNone(check_dump_target(ROOT / "cv" / "main_acme_analyst.txt")) + + def test_targets_outside_the_output_dirs_are_refused(self): + outside = Path(self.temp_dir.name).parent / "elsewhere.txt" + for path, reason in [ + (outside, "inside this repo's cv/"), + (self.root / "tools" / "notes.txt", "inside this repo's cv/"), + (self.root / "cv" / ".." / "tools" / "x.txt", "inside this repo's cv/"), + (self.root / "cv" / "main_acme.py", "must name a .txt file"), + (Path.home() / ".zshrc", "must name a .txt file"), + ]: + with self.subTest(path=path): + self.assertIn(reason, check_dump_target(path, self.root)) + + def test_symlink_out_of_cv_is_refused(self): + link = self.root / "cv" / "main_acme.txt" + link.symlink_to(self.root / "outside.txt") + self.assertEqual(check_dump_target(link, self.root), "must not be a symlink") + + @patch("tools.verify_pdf.verify_pdf") + def test_cli_refuses_before_extracting_or_writing(self, mock_verify): + target = Path(self.temp_dir.name) / "victim" / "rc.txt" + with patch("sys.stderr"): + code = main(["cv/main_example.pdf", "--dump-text", str(target)]) + self.assertEqual(code, 1) + mock_verify.assert_not_called() + self.assertFalse(target.parent.exists(), "the CLI must not create the parent directory") + + if __name__ == "__main__": unittest.main() diff --git a/tools/security_guards.py b/tools/security_guards.py index f3ff4e40..d9635bef 100644 --- a/tools/security_guards.py +++ b/tools/security_guards.py @@ -82,7 +82,8 @@ ALLOWED_PERMISSIONS = { "Bash(python3 tools/verify_pdf.py:*)", "Bash(python tools/verify_layout.py:*)", "Bash(python3 tools/verify_layout.py:*)", - "Bash(pdftotext:*)", + # No bare Bash(pdftotext:*): it writes its output to any path it is given, + # and tools/verify_pdf.py already wraps it with that path confined. # The browser-header retry in 09-web-research.md. fetch_page.py is GET-only # with a fixed argv and refuses to write inside the repo, which is why the # grant names the script and not `curl`. diff --git a/tools/verify_pdf.py b/tools/verify_pdf.py index 9b757f5f..f6bd7975 100644 --- a/tools/verify_pdf.py +++ b/tools/verify_pdf.py @@ -24,10 +24,33 @@ import unicodedata from pathlib import Path +ROOT = Path(__file__).resolve().parent.parent + +# `.claude/settings.json` pre-approves this tool with any arguments, so where +# `--dump-text` may write is part of that permission. The workflow only dumps a +# CV's text layer beside it (`apply.md` Step 5d writes `cv/main__.txt`); +# a path anywhere else - a shell rc file, a script the allowlist runs - would be +# an overwrite nobody was asked about. +DUMP_TEXT_DIRS = ("cv", "cover_letters") + + class VerificationError(Exception): """Raised when a generated PDF does not satisfy its checks.""" +def check_dump_target(path, root=ROOT): + """Return why `--dump-text` may not write to `path`, or None if it may.""" + path = Path(path) + if path.suffix != ".txt": + return "must name a .txt file" + if path.is_symlink(): + return "must not be a symlink" + parent = path.resolve().parent + if not any(parent.is_relative_to((root / d).resolve()) for d in DUMP_TEXT_DIRS): + return "must be inside this repo's " + " or ".join(f"{d}/" for d in DUMP_TEXT_DIRS) + return None + + def run_tool(command): try: return subprocess.run( @@ -302,7 +325,7 @@ def build_parser(): parser.add_argument( "--dump-text", type=Path, - help="write the extracted text layer to this path (UTF-8)", + help="write the extracted text layer to this .txt path under cv/ or cover_letters/ (UTF-8)", ) parser.add_argument( "--ascii-dates", @@ -332,6 +355,11 @@ def _force_utf8_output() -> None: def main(argv=None): _force_utf8_output() args = build_parser().parse_args(argv) + if args.dump_text is not None: + reason = check_dump_target(args.dump_text) + if reason: + print(f"Error: --dump-text {args.dump_text}: {reason}", file=sys.stderr) + return 1 try: extractor, text, pages = verify_pdf( args.pdf,