From 36462e356ee1740914ee306b21a12336d5a59baa Mon Sep 17 00:00:00 2001 From: Thejesh Reddy <35212698+thejesh23@users.noreply.github.com> Date: Mon, 20 Jul 2026 09:46:58 -0700 Subject: [PATCH] fix(security_guards): reject un-allowlisted .gitignore negations (#195) check_gitignore() verified each required personal-data rule was present via set membership, but .gitignore is order-sensitive: a later !pattern re-includes a file an earlier rule excluded, so the required line stays physically present while the file is no longer ignored - the guard failed open on exactly the weakening its docstring claims to catch. Keeps the required-rules-present check and additionally rejects any negation line outside a small reviewed ALLOWED_IGNORE_NEGATIONS allowlist (same explicit-widening pattern as ALLOWED_PERMISSIONS). Fixes #194. By @thejesh23. Verified: allowlist matches the four negations currently in .gitignore; guard test suite passes locally (17 tests) and in CI. Closes #194 --- tests/test_security_guards.py | 23 +++++++++++++++++++++++ tools/security_guards.py | 33 +++++++++++++++++++++++++++++---- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/tests/test_security_guards.py b/tests/test_security_guards.py index 47eacbc..7ec094e 100644 --- a/tests/test_security_guards.py +++ b/tests/test_security_guards.py @@ -125,6 +125,29 @@ class GitignoreGuardTests(GuardRepoFixture): self.assertEqual(result.returncode, 0, result.stdout + result.stderr) +class GitignoreNegationTests(GuardRepoFixture): + def test_negation_reincluding_personal_data_fails(self): + # .gitignore is order-sensitive: `!salary_data.json` after the + # `salary_data.json` rule re-includes the file, so the required rule is + # still present but no longer takes effect. Set membership on the + # required rules cannot see this, so the negation must be rejected. + self.write_gitignore(list(security_guards.REQUIRED_IGNORE_RULES) + ["!salary_data.json"]) + result = run_guards(self.root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("negation rule not in the reviewed allowlist", result.stdout) + self.assertIn("!salary_data.json", result.stdout) + + def test_allowlisted_negations_pass(self): + # The template's own benign negations (example CV/cover letter, fonts, + # .gitkeep placeholders) must keep passing. + self.write_gitignore( + list(security_guards.REQUIRED_IGNORE_RULES) + + sorted(security_guards.ALLOWED_IGNORE_NEGATIONS) + ) + result = run_guards(self.root) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + class ManifestGuardTests(GuardRepoFixture): def test_each_lifecycle_script_fails(self): for script in sorted(security_guards.FORBIDDEN_SCRIPTS): diff --git a/tools/security_guards.py b/tools/security_guards.py index 4eb783f..e9102f6 100644 --- a/tools/security_guards.py +++ b/tools/security_guards.py @@ -13,9 +13,10 @@ Checks: 1. .claude/settings.json — every permissions.allow entry must be in the exact allowlist below. Catches permission widening (e.g. Bash(*), Bash(curl:*)), which would auto-approve commands on every fork. -2. .gitignore — the personal-data ignore rules must all still be present. - Catches weakening that would make future users silently commit their - tracker, profile exports, or application archives. +2. .gitignore — the personal-data ignore rules must all still be present, + and no un-allowlisted negation (!pattern) may re-include them. Catches + weakening that would make future users silently commit their tracker, + profile exports, or application archives. 3. .agents/**/package.json — no npm/bun lifecycle scripts (preinstall, install, postinstall, prepare, prepack) and no trustedDependencies. Catches code execution smuggled into `bun install`. @@ -55,6 +56,20 @@ REQUIRED_IGNORE_RULES = [ "job_search_tracker.csv", ] +# Negation (re-include) rules the template legitimately ships. .gitignore is +# order-sensitive: a later `!pattern` re-includes a path an earlier rule +# excluded, so a rule can be physically present in REQUIRED_IGNORE_RULES yet +# no longer ignored (e.g. adding `!salary_data.json`). Set membership on the +# required rules cannot see that. Any negation outside this allowlist is a +# failure - add an intentional one here in the same PR, exactly as with +# ALLOWED_PERMISSIONS, so the widening is explicit and reviewable. +ALLOWED_IGNORE_NEGATIONS = { + "!cover_letters/OpenFonts/fonts/**", + "!cv/main_example.tex", + "!cover_letters/cover_example.tex", + "!documents/**/.gitkeep", +} + FORBIDDEN_SCRIPTS = {"preinstall", "install", "postinstall", "prepare", "prepack"} @@ -93,10 +108,11 @@ def check_permissions() -> None: def check_gitignore() -> None: path = ROOT / ".gitignore" try: - rules = {line.strip() for line in path.read_text(encoding="utf-8").splitlines()} + lines = [line.strip() for line in path.read_text(encoding="utf-8").splitlines()] except OSError as exc: errors.append(f".gitignore: unreadable: {exc}") return + rules = set(lines) for rule in REQUIRED_IGNORE_RULES: if rule not in rules: errors.append( @@ -105,6 +121,15 @@ def check_gitignore() -> None: "or was renamed intentionally, update REQUIRED_IGNORE_RULES in " "tools/security_guards.py in the same PR." ) + for line in lines: + if line.startswith("!") and line not in ALLOWED_IGNORE_NEGATIONS: + errors.append( + f".gitignore: negation rule not in the reviewed allowlist: {line!r}. " + "A negation re-includes a path an earlier rule excluded and can silently " + "re-expose personal data (a required ignore rule stays present but stops " + "taking effect). If this negation is intentional, add it to " + "ALLOWED_IGNORE_NEGATIONS in tools/security_guards.py in the same PR." + ) def check_package_manifests() -> None: