mirror of
https://github.com/MadsLorentzen/ai-job-search.git
synced 2026-11-11 17:37:59 +00:00
feat(security-guards): fork-local negations via .github/security-guards-local.txt (#618)
A fork that tracks its own file under a personal-data ignore rule can list the negation in .github/security-guards-local.txt instead of editing ALLOWED_IGNORE_NEGATIONS, so merging upstream never conflicts there. Review fixes (MadsLorentzen, #618): - a local negation can no longer cancel a REQUIRED_IGNORE_RULES entry (`!job_search_tracker.csv`, also as `/x` or `./x`); it is reported and not added to the allowlist - UPSTREAM_REPO now names ai-job-search-dev/ai-job-search, so the "file must not exist upstream" check fires in CI again - rebased on current master; the frozen CHANGELOG.md hunk is dropped Claude-Session: https://claude.ai/code/session_01Mv8AZjCnuGt98qsM9zuCFZ Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
e9ab29dcc2
commit
71c49f3d83
3 files changed
+192
-5
No files matched your search
@@ -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).
|
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.
|
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
|
## Troubleshooting
|
||||||
|
|
||||||
### "salary_data.json not found"
|
### "salary_data.json not found"
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import json
|
import json
|
||||||
|
import os
|
||||||
import shutil
|
import shutil
|
||||||
import subprocess
|
import subprocess
|
||||||
import sys
|
import sys
|
||||||
@@ -13,11 +14,22 @@ sys.path.insert(0, str(REPO_ROOT / "tools"))
|
|||||||
import security_guards # noqa: E402 (imported for its allowlist constants)
|
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(
|
return subprocess.run(
|
||||||
[sys.executable, str(root / "tools" / "security_guards.py")],
|
[sys.executable, str(root / "tools" / "security_guards.py")],
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
text=True,
|
text=True,
|
||||||
|
env=env,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -851,6 +863,99 @@ class SettingsKeyAllowlistTests(GuardRepoFixture):
|
|||||||
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
|
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):
|
class ManifestGuardTests(GuardRepoFixture):
|
||||||
def test_each_lifecycle_script_fails(self):
|
def test_each_lifecycle_script_fails(self):
|
||||||
for script in sorted(security_guards.FORBIDDEN_SCRIPTS):
|
for script in sorted(security_guards.FORBIDDEN_SCRIPTS):
|
||||||
|
|||||||
@@ -53,6 +53,7 @@ Stdlib only. Exit 0 on success, 1 with a failure list otherwise.
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
import json
|
import json
|
||||||
|
import os
|
||||||
import re
|
import re
|
||||||
import subprocess
|
import subprocess
|
||||||
import sys
|
import sys
|
||||||
@@ -228,6 +229,16 @@ ALLOWED_IGNORE_NEGATIONS = {
|
|||||||
"!documents/**/.gitkeep",
|
"!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 "<Event>:<command>" strings.
|
# Hook commands the template legitimately ships, as "<Event>:<command>" strings.
|
||||||
# Empty by design - the template ships no hooks at all.
|
# Empty by design - the template ships no hooks at all.
|
||||||
#
|
#
|
||||||
@@ -578,14 +589,71 @@ def _gitignore_lines(path: Path) -> list[str]:
|
|||||||
|
|
||||||
def _other_gitignores():
|
def _other_gitignores():
|
||||||
"""Every .gitignore below the root, skipping .git and installed dependencies."""
|
"""Every .gitignore below the root, skipping .git and installed dependencies."""
|
||||||
import os
|
|
||||||
|
|
||||||
for dirpath, dirnames, filenames in os.walk(ROOT):
|
for dirpath, dirnames, filenames in os.walk(ROOT):
|
||||||
dirnames[:] = sorted(d for d in dirnames if d not in (".git", "node_modules"))
|
dirnames[:] = sorted(d for d in dirnames if d not in (".git", "node_modules"))
|
||||||
if ".gitignore" in filenames and Path(dirpath) != ROOT:
|
if ".gitignore" in filenames and Path(dirpath) != ROOT:
|
||||||
yield Path(dirpath) / ".gitignore"
|
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:
|
def check_gitignore() -> None:
|
||||||
path = ROOT / ".gitignore"
|
path = ROOT / ".gitignore"
|
||||||
try:
|
try:
|
||||||
@@ -602,14 +670,17 @@ def check_gitignore() -> None:
|
|||||||
"or was renamed intentionally, update REQUIRED_IGNORE_RULES in "
|
"or was renamed intentionally, update REQUIRED_IGNORE_RULES in "
|
||||||
"tools/security_guards.py in the same PR."
|
"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:
|
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(
|
errors.append(
|
||||||
f".gitignore: negation rule not in the reviewed allowlist: {line!r}. "
|
f".gitignore: negation rule not in the reviewed allowlist: {line!r}. "
|
||||||
"A negation re-includes a path an earlier rule excluded and can silently "
|
"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 "
|
"re-expose personal data (a required ignore rule stays present but stops "
|
||||||
"taking effect). If this negation is intentional, add it to "
|
"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,
|
# 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.
|
# so a negation there re-includes personal data the root rules excluded.
|
||||||
|
|||||||
Reference in new issue
Block a user