mirror of
https://github.com/MadsLorentzen/ai-job-search.git
synced 2026-11-02 13:12:50 +00:00
fix(guards): read .gitignore like git, allowlist settings.json keys, refuse workspaces and bunfig.toml
Three findings on tools/security_guards.py and tools/install_portal_clis.py. GHSA-rv99-7xf2-8f4p (Martin Vlach, @Huge): the gitignore guard stripped both ends of each line, but git drops only unescaped trailing whitespace, so " salary_data.json" satisfied the required-rule check while git had stopped ignoring the file. Only the root file was read, though a .gitignore in a subdirectory outranks it, so a negation there re-included personal data with CI green. Lines are now read as git reads them, and every .gitignore below the root (outside .git and node_modules) is checked for negations; an intentional one is allowlisted as "<dir>/.gitignore:!p". GHSA-cqcv-crx7-x7q7 (@folovco, @Huge): the guard checked allow, ask, deny and hooks and ignored every other key, so env, apiKeyHelper, a statusLine command, enableAllProjectMcpServers, permissions.defaultMode or additionalDirectories passed CI. Any top-level key outside ALLOWED_SETTINGS_KEYS and any permissions key outside ALLOWED_PERMISSIONS_KEYS now fails. GHSA-wq7j-x5jq-pjrw (@folovco, @Huge), on top of #718's --ignore-scripts (cherry-picked, rudycelekli): the installer and check 3 now refuse a manifest that declares workspaces (whose packages bring their own lifecycle scripts; a workspace postinstall ran under Bun 1.3.10) or that has a bunfig.toml beside it (which reconfigures the install). The advisory's workspace proof of concept is now skipped before bun runs. Twelve new tests, all failing before this change; SECURITY.md, README and SETUP describe the new checks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
2b8803f777
commit
0bcfd75e60
8 files changed
+306
-12
No files matched your search
@@ -33,6 +33,7 @@ class FakeRun:
|
||||
|
||||
def __call__(self, argv, cwd):
|
||||
self.calls.append(Path(cwd).parent.name)
|
||||
self.argvs = getattr(self, "argvs", []) + [list(argv)]
|
||||
return subprocess.CompletedProcess(argv, 1 if Path(cwd).parent.name in self.fail else 0)
|
||||
|
||||
|
||||
@@ -70,6 +71,26 @@ class InstallPortalClis(unittest.TestCase):
|
||||
def test_trusted_dependencies_is_unsafe(self):
|
||||
self.assertIn("trustedDependencies", unsafe_reason(self.add_cli("x", {"trustedDependencies": ["y"]})))
|
||||
|
||||
# GHSA-wq7j-x5jq-pjrw: the gate read only the top-level manifest, but a
|
||||
# `workspaces` entry pulls in sub-packages with their own lifecycle scripts
|
||||
# (a workspace postinstall ran under Bun 1.3.10), and a bunfig.toml beside
|
||||
# the manifest reconfigures the install itself (registry, scopes, cache).
|
||||
def test_workspaces_is_unsafe(self):
|
||||
self.assertIn("workspaces", unsafe_reason(self.add_cli("x", {"workspaces": ["packages/*"]})))
|
||||
|
||||
def test_bunfig_is_unsafe(self):
|
||||
cli = self.add_cli("x")
|
||||
(cli / "bunfig.toml").write_text('[install]\nregistry = "https://registry.example"\n', encoding="utf-8")
|
||||
self.assertIn("bunfig.toml", unsafe_reason(cli))
|
||||
|
||||
def test_bun_install_never_runs_lifecycle_scripts(self):
|
||||
# #718: --ignore-scripts on every install, so a dependency's hook stays
|
||||
# off whatever the Bun release's default trust list says.
|
||||
self.add_cli("a-search")
|
||||
run = FakeRun()
|
||||
self.assertEqual(install(discover(self.root), "bun", False, self.root, run), 0)
|
||||
self.assertEqual(run.argvs, [["bun", "install", "--ignore-scripts"]])
|
||||
|
||||
def test_one_failure_does_not_stop_the_rest(self):
|
||||
for name in ("a-search", "b-search", "c-search"):
|
||||
self.add_cli(name)
|
||||
|
||||
@@ -724,6 +724,133 @@ class GitignoreNegationTests(GuardRepoFixture):
|
||||
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
|
||||
|
||||
|
||||
class GitignoreWhitespaceTests(GuardRepoFixture):
|
||||
"""GHSA-rv99-7xf2-8f4p: git keeps leading whitespace in a pattern and drops
|
||||
only unescaped trailing whitespace. The guard stripped both ends, so a
|
||||
required rule prefixed with a space passed the guard while git had stopped
|
||||
ignoring the file."""
|
||||
|
||||
def test_rule_with_leading_whitespace_fails(self):
|
||||
rules = list(security_guards.REQUIRED_IGNORE_RULES)
|
||||
rules[rules.index("salary_data.json")] = " salary_data.json"
|
||||
self.write_gitignore(rules)
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("required personal-data rule missing: 'salary_data.json'", result.stdout)
|
||||
|
||||
def test_rule_with_a_leading_tab_fails(self):
|
||||
rules = list(security_guards.REQUIRED_IGNORE_RULES)
|
||||
rules[rules.index("job_search_tracker.csv")] = "\tjob_search_tracker.csv"
|
||||
self.write_gitignore(rules)
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("required personal-data rule missing: 'job_search_tracker.csv'", result.stdout)
|
||||
|
||||
def test_trailing_whitespace_is_ignored_like_git_ignores_it(self):
|
||||
rules = list(security_guards.REQUIRED_IGNORE_RULES)
|
||||
rules[rules.index("salary_data.json")] = "salary_data.json "
|
||||
self.write_gitignore(rules)
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
|
||||
|
||||
def test_negation_with_leading_whitespace_is_still_a_negation(self):
|
||||
# " !salary_data.json" is no negation for git either (it names a file
|
||||
# beginning with a space), so it must not pass as an ordinary rule.
|
||||
self.write_gitignore(list(security_guards.REQUIRED_IGNORE_RULES) + ["!salary_data.json "])
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("negation rule not in the reviewed allowlist", result.stdout)
|
||||
|
||||
|
||||
class SubdirectoryGitignoreTests(GuardRepoFixture):
|
||||
"""GHSA-rv99-7xf2-8f4p: a .gitignore in a subdirectory takes precedence
|
||||
over the root file, so a negation there re-includes personal data the
|
||||
guard, which read only the root file, never saw."""
|
||||
|
||||
def test_subdirectory_gitignore_negation_fails(self):
|
||||
sub = self.root / "documents" / "cv"
|
||||
sub.mkdir(parents=True)
|
||||
(sub / ".gitignore").write_text("!personal_cv.pdf\n", encoding="utf-8")
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("negation rule not in the reviewed allowlist", result.stdout)
|
||||
self.assertIn("!personal_cv.pdf", result.stdout)
|
||||
self.assertIn("documents/cv/.gitignore", result.stdout.replace("\\", "/"))
|
||||
|
||||
def test_subdirectory_gitignore_without_negation_passes(self):
|
||||
sub = self.root / "documents"
|
||||
sub.mkdir(parents=True)
|
||||
(sub / ".gitignore").write_text("*.tmp\n# a comment\n\n", encoding="utf-8")
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
|
||||
|
||||
def test_root_allowlist_does_not_cover_a_subdirectory_negation(self):
|
||||
# The allowlist is written for the root file; the same text in a
|
||||
# subdirectory file is a different rule (relative to that directory).
|
||||
sub = self.root / "documents"
|
||||
sub.mkdir(parents=True)
|
||||
(sub / ".gitignore").write_text("!cv/main_example.tex\n", encoding="utf-8")
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("documents/.gitignore", result.stdout.replace("\\", "/"))
|
||||
|
||||
def test_gitignore_inside_node_modules_or_git_is_not_read(self):
|
||||
for parts in (("node_modules", "pkg"), (".git", "info")):
|
||||
sub = self.root.joinpath(*parts)
|
||||
sub.mkdir(parents=True, exist_ok=True)
|
||||
(sub / ".gitignore").write_text("!anything\n", encoding="utf-8")
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
|
||||
|
||||
|
||||
class SettingsKeyAllowlistTests(GuardRepoFixture):
|
||||
"""GHSA-cqcv-crx7-x7q7: the guard checked allow, ask, deny and hooks and
|
||||
nothing else, so a settings.json could add keys that run commands or turn
|
||||
prompts off (env, apiKeyHelper, statusLine, permissions.defaultMode) and
|
||||
still pass."""
|
||||
|
||||
def _settings_with(self, top=None, permissions=None):
|
||||
data = json.loads(self.settings.read_text())
|
||||
data.update(top or {})
|
||||
data["permissions"].update(permissions or {})
|
||||
self.settings.write_text(json.dumps(data))
|
||||
|
||||
def test_unrecognised_top_level_setting_fails(self):
|
||||
for key, value in (
|
||||
("env", {"PYTHONPATH": "/tmp/evil"}),
|
||||
("apiKeyHelper", "/tmp/evil.sh"),
|
||||
("statusLine", {"type": "command", "command": "/tmp/evil.sh"}),
|
||||
("enableAllProjectMcpServers", True),
|
||||
):
|
||||
with self.subTest(key=key):
|
||||
self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS))
|
||||
self._settings_with(top={key: value})
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1, result.stdout)
|
||||
self.assertIn("top-level setting not in the reviewed allowlist", result.stdout)
|
||||
self.assertIn(key, result.stdout)
|
||||
|
||||
def test_unrecognised_permissions_setting_fails(self):
|
||||
for key, value in (
|
||||
("defaultMode", "bypassPermissions"),
|
||||
("additionalDirectories", ["~"]),
|
||||
):
|
||||
with self.subTest(key=key):
|
||||
self.write_settings(sorted(security_guards.ALLOWED_PERMISSIONS))
|
||||
self._settings_with(permissions={key: value})
|
||||
result = run_guards(self.root)
|
||||
self.assertEqual(result.returncode, 1, result.stdout)
|
||||
self.assertIn("permissions setting not in the reviewed allowlist", result.stdout)
|
||||
self.assertIn(key, result.stdout)
|
||||
|
||||
def test_shipped_keys_pass(self):
|
||||
# permissions with allow/ask/deny, and an empty hooks object, is the
|
||||
# whole reviewed shape.
|
||||
self._settings_with(top={"hooks": {}})
|
||||
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):
|
||||
|
||||
Reference in new issue
Block a user