diff --git a/SETUP.md b/SETUP.md index 52235765..35e4d091 100644 --- a/SETUP.md +++ b/SETUP.md @@ -327,6 +327,17 @@ Upstream keeps improving the methodology files your fork has personalized, so pl Forks also inherit a `.github/workflows/upstream-watch.yml` that runs this weekly and writes the result into a single rolling issue (it no-ops on the upstream template itself, and stays disabled on a fork until you enable Actions). 3. **Merge normally.** `git merge upstream/master` (or `git pull`) three-way-merges upstream's edits around your personalization; because methodology edits rarely touch the lines `/setup` filled in, most updates land cleanly. A conflict in a personalized file is a *feature*, not a failure — it means upstream changed methodology in a section you customized, and the version marker plus its changelog commit tell you why. Resolve by keeping your data and adopting the methodology change around it. +### Tracking your own directory without editing `security_guards.py` + +If your fork keeps a tracked placeholder under a personal-data ignore rule (for example a `leads/` folder with a `.gitkeep`), `tools/security_guards.py` rejects the `!leads/.gitkeep` negation because it is not in `ALLOWED_IGNORE_NEGATIONS`. Editing that set works, but every upstream change near it then conflicts on merge. Instead, list the negation in `.github/security-guards-local.txt`: + +```text +# fork-only tracked placeholders +!leads/.gitkeep +``` + +One negation per line; blank lines and `#` comments are ignored. The file can only *extend* the allowlist: any other line (a plain ignore rule, say) fails the guard, and a negation not listed here or in `ALLOWED_IGNORE_NEGATIONS` still fails. If the file is absent nothing changes. The upstream template never ships it, and the guard fails if it appears there (CI decides which side it is on from `GITHUB_REPOSITORY`). + ## Troubleshooting ### "salary_data.json not found" diff --git a/tests/test_security_guards.py b/tests/test_security_guards.py index ccadd728..50aa318c 100644 --- a/tests/test_security_guards.py +++ b/tests/test_security_guards.py @@ -1,4 +1,5 @@ import json +import os import shutil import subprocess import sys @@ -13,11 +14,22 @@ sys.path.insert(0, str(REPO_ROOT / "tools")) import security_guards # noqa: E402 (imported for its allowlist constants) -def run_guards(root: Path) -> subprocess.CompletedProcess: +def run_guards(root: Path, repository=None) -> subprocess.CompletedProcess: + """Run the guard script in ``root``. + + ``repository`` pins GITHUB_REPOSITORY for the subprocess so results do not + depend on where the suite runs (CI on upstream sets it to the upstream slug). + """ + env = dict(os.environ) + if repository is None: + env.pop("GITHUB_REPOSITORY", None) + else: + env["GITHUB_REPOSITORY"] = repository return subprocess.run( [sys.executable, str(root / "tools" / "security_guards.py")], capture_output=True, text=True, + env=env, ) @@ -851,6 +863,99 @@ class SettingsKeyAllowlistTests(GuardRepoFixture): self.assertEqual(result.returncode, 0, result.stdout + result.stderr) +class LocalNegationsTests(GuardRepoFixture): + """`.github/security-guards-local.txt`: a fork-only extension of the negation allowlist.""" + + FORK = "someone-else/ai-job-search" + + def write_local(self, text): + path = self.root / ".github" / "security-guards-local.txt" + path.parent.mkdir(exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + def with_negation(self, negation): + self.write_gitignore(list(security_guards.REQUIRED_IGNORE_RULES) + [negation]) + + def test_listed_negation_passes(self): + self.with_negation("!leads/.gitkeep") + self.write_local("# fork-only tracked placeholder\n\n!leads/.gitkeep\n") + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_unlisted_negation_still_fails(self): + self.with_negation("!salary_data.json") + self.write_local("!leads/.gitkeep\n") + result = run_guards(self.root, self.FORK) + 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_line_that_is_not_a_negation_is_rejected(self): + self.write_local("salary_data.json\n") + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("is not a negation", result.stdout) + self.assertIn("security-guards-local.txt:1", result.stdout) + + def test_local_negation_cannot_cancel_a_required_rule(self): + # Every required rule is a personal-data guard; none may be re-included + # locally, whatever the spelling git would fold to the same pattern. + for rule in security_guards.REQUIRED_IGNORE_RULES: + if rule.startswith("!"): + continue + for spelling in (rule, "/" + rule, "./" + rule): + with self.subTest(rule=rule, spelling=spelling): + self.with_negation("!" + spelling) + self.write_local("!" + spelling + "\n") + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("negates a required personal-data ignore rule", result.stdout) + self.assertIn("security-guards-local.txt:1", result.stdout) + + def test_rejected_required_negation_is_not_also_allowlisted(self): + # The refusal must also leave the .gitignore line itself failing, so a + # rejected local entry cannot widen the allowlist as a side effect. + self.with_negation("!job_search_tracker.csv") + self.write_local("!job_search_tracker.csv\n") + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("negation rule not in the reviewed allowlist", result.stdout) + self.assertIn("!job_search_tracker.csv", result.stdout) + + def test_negation_of_a_path_under_a_required_rule_is_still_allowed(self): + # The extension point exists for exactly this: a fork-owned tracked file + # inside a directory a required glob ignores. + self.with_negation("!documents/applications/fork-notes.md") + self.write_local("!documents/applications/fork-notes.md\n") + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_file_absent_means_no_change(self): + self.write_gitignore( + list(security_guards.REQUIRED_IGNORE_RULES) + + sorted(security_guards.ALLOWED_IGNORE_NEGATIONS) + ) + result = run_guards(self.root, self.FORK) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_file_must_be_absent_on_upstream(self): + self.write_local("!leads/.gitkeep\n") + self.with_negation("!leads/.gitkeep") + result = run_guards(self.root, security_guards.UPSTREAM_REPO) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("must not exist on the upstream template", result.stdout) + + def test_upstream_checkout_ships_no_local_file(self): + # Direct assertion on the real tree: this repo, when it is the upstream + # template, must not carry the fork-only file. + sys.path.insert(0, str(REPO_ROOT)) + from tests._upstream import is_upstream_checkout + if not is_upstream_checkout(): + self.skipTest("fork checkout: the local negations file is allowed here") + self.assertFalse(security_guards.LOCAL_NEGATIONS_FILE.exists()) + + 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 ebdc7859..77c5bee2 100644 --- a/tools/security_guards.py +++ b/tools/security_guards.py @@ -53,6 +53,7 @@ Stdlib only. Exit 0 on success, 1 with a failure list otherwise. """ import json +import os import re import subprocess import sys @@ -228,6 +229,16 @@ ALLOWED_IGNORE_NEGATIONS = { "!documents/**/.gitkeep", } +# Fork-local extension of the negation allowlist. A fork that adds its own tracked +# directory under a personal-data ignore rule (say, `!leads/.gitkeep`) lists the +# negation here instead of editing this file, so `git merge upstream/master` never +# conflicts on ALLOWED_IGNORE_NEGATIONS. One negation per line, `#` comments and blank +# lines allowed. The file can only EXTEND the negation allowlist: any other +# non-comment line is a failure. Upstream never ships it (see +# check_local_negations_absent_upstream). +LOCAL_NEGATIONS_FILE = ROOT / ".github" / "security-guards-local.txt" +UPSTREAM_REPO = "ai-job-search-dev/ai-job-search" + # Hook commands the template legitimately ships, as ":" strings. # Empty by design - the template ships no hooks at all. # @@ -578,14 +589,71 @@ def _gitignore_lines(path: Path) -> list[str]: def _other_gitignores(): """Every .gitignore below the root, skipping .git and installed dependencies.""" - import os - for dirpath, dirnames, filenames in os.walk(ROOT): dirnames[:] = sorted(d for d in dirnames if d not in (".git", "node_modules")) if ".gitignore" in filenames and Path(dirpath) != ROOT: yield Path(dirpath) / ".gitignore" +def _normalized_pattern(pattern: str) -> str: + """A .gitignore pattern with the spellings git treats as the same rule folded. + + A leading ``/`` only anchors the pattern to the .gitignore's directory, and the + root ``.gitignore`` is that directory, so ``/salary_data.json`` and + ``salary_data.json`` are the same rule for a file at the repo root. + """ + pattern = pattern.strip() + while pattern.startswith("./"): + pattern = pattern[2:] + return pattern.lstrip("/") + + +_REQUIRED_IGNORE_PATTERNS = { + _normalized_pattern(rule) for rule in REQUIRED_IGNORE_RULES if not rule.startswith("!") +} + + +def load_local_negations() -> set[str]: + """Negations a fork lists in .github/security-guards-local.txt (empty if absent).""" + if not LOCAL_NEGATIONS_FILE.is_file(): + return set() + try: + raw = LOCAL_NEGATIONS_FILE.read_text(encoding="utf-8").splitlines() + except OSError as exc: + errors.append(f".github/security-guards-local.txt: unreadable: {exc}") + return set() + extra: set[str] = set() + for number, line in enumerate(raw, start=1): + line = line.strip() + if not line or line.startswith("#"): + continue + if not line.startswith("!"): + errors.append( + f".github/security-guards-local.txt:{number}: {line!r} is not a negation. " + "The file may only list `!pattern` lines that extend the .gitignore " + "negation allowlist." + ) + continue + if _normalized_pattern(line[1:]) in _REQUIRED_IGNORE_PATTERNS: + errors.append( + f".github/security-guards-local.txt:{number}: {line!r} negates a required " + "personal-data ignore rule (REQUIRED_IGNORE_RULES). A fork may re-include " + "its own tracked paths, never the rule that keeps personal data out of git." + ) + continue + extra.add(line) + return extra + + +def check_local_negations_absent_upstream() -> None: + """The local-negations file must never exist on the upstream template itself.""" + if os.environ.get("GITHUB_REPOSITORY") == UPSTREAM_REPO and LOCAL_NEGATIONS_FILE.exists(): + errors.append( + ".github/security-guards-local.txt must not exist on the upstream template: " + "it is a fork-only extension point. Remove it from this PR." + ) + + def check_gitignore() -> None: path = ROOT / ".gitignore" try: @@ -602,14 +670,17 @@ def check_gitignore() -> None: "or was renamed intentionally, update REQUIRED_IGNORE_RULES in " "tools/security_guards.py in the same PR." ) + check_local_negations_absent_upstream() + allowed_negations = ALLOWED_IGNORE_NEGATIONS | load_local_negations() for line in lines: - if line.startswith("!") and line not in ALLOWED_IGNORE_NEGATIONS: + if line.startswith("!") and line not in allowed_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." + "ALLOWED_IGNORE_NEGATIONS in tools/security_guards.py in the same PR " + "(on a fork, list it in .github/security-guards-local.txt instead)." ) # A .gitignore in a subdirectory outranks the root file for paths under it, # so a negation there re-includes personal data the root rules excluded.