diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0b2f7a7c..8a6974bd 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -22,6 +22,24 @@ repos: - repo: local hooks: + # Runs at BOTH stages, and first: while the shared config is corrupted, + # `git status` and `git revert` lie, so every later hook is reasoning about a + # tree that is not the one on disk. + # + # Deliberately the ONLY hook here without `cd "$(git rev-parse + # --show-toplevel)"`: `core.worktree`, one of the values this gate detects, + # REDIRECTS --show-toplevel, so that prologue would cd the check out of the + # repository whenever it had something to find. `mise run` locates mise.toml by + # walking the filesystem, and the script locates the shared config the same + # way, so neither needs git to answer correctly. See #855. + - id: git-config-clean + name: shared .git/config uncorrupted (#855) + entry: bash -lc 'mise run check:git-config-clean' + language: system + pass_filenames: false + always_run: true + stages: [pre-commit, pre-push] + - id: gitleaks name: gitleaks (staged) entry: bash -lc 'cd "$(git rev-parse --show-toplevel)" && mise run security:secrets:staged' diff --git a/agent/tests/conftest.py b/agent/tests/conftest.py index 0ea21a8b..dea0770b 100644 --- a/agent/tests/conftest.py +++ b/agent/tests/conftest.py @@ -9,6 +9,13 @@ import pytest from models import TaskConfig +from tests.git_env import ( + GIT_LOCATION_VARS, + TEST_IDENTITY_EMAIL, + TEST_IDENTITY_NAME, + fingerprint_git_config, + shared_git_config_path, +) # Session-wide hang backstop. SIGALRM (pytest-timeout method="signal") fires only # in the MAIN thread during a test's *call* phase, so a deadlock in a WORKER @@ -58,15 +65,93 @@ def _reap_on_hang() -> None: _hang_watchdog.start() +# Layer 2 of the #855 git-config guard: DETECT. Captured at session start and +# re-read at session finish. `None` means there is nothing to protect (no git, or +# not inside a checkout — e.g. the built container image), which is a real +# no-risk case rather than a failure to look. +_SHARED_GIT_CONFIG: tuple[str, tuple[str, frozenset[str]]] | None = None + + +def pytest_sessionstart(session): + """Fingerprint the repository-shared ``.git/config`` before any test runs (#855). + + This is the backstop for the autouse fixture below, and it is deliberately + mechanism-INDEPENDENT: it does not care *how* the file was written, so it also + catches routes the fixture does not anticipate. Four previous fixes for this leak + were each scoped to one file and each was defeated by the next file added; a + whole-session before/after comparison cannot be outrun that way. + """ + global _SHARED_GIT_CONFIG + path = shared_git_config_path() + if path is None: + return + fingerprint = fingerprint_git_config(path) + if fingerprint is None: + return + _SHARED_GIT_CONFIG = (path, fingerprint) + + +def _report_shared_git_config_mutation(session) -> None: + """Fail the session if the shared ``.git/config`` changed during the run (#855). + + Reports key NAMES only, never values: a ``.git/config`` may hold a remote URL + with embedded credentials, and this text goes to CI logs. + + Does not repair the file. A test suite that silently rewrites ``.git/config`` + would be the same class of surprise as the bug it is guarding against — so this + prints the exact remedy and leaves the decision to a human. + """ + if _SHARED_GIT_CONFIG is None: + return + path, (digest_before, names_before) = _SHARED_GIT_CONFIG + current = fingerprint_git_config(path) + if current is None: + detail = "the file is now unreadable or gone" + else: + digest_after, names_after = current + if digest_after == digest_before: + return + added = sorted(names_after - names_before) + removed = sorted(names_before - names_after) + changed = sorted(names_after & names_before) + parts = [] + if added: + parts.append(f"keys added: {', '.join(added)}") + if removed: + parts.append(f"keys removed: {', '.join(removed)}") + if not added and not removed: + parts.append(f"value(s) changed among: {', '.join(changed)}") + detail = "; ".join(parts) + + print( + f"\nSHARED GIT CONFIG MUTATED — {path}\n" + f" {detail}\n" + " A test wrote into the repository's shared config. This is the #855 leak: a\n" + " fixture shelling out to git while a GIT_DIR is inherited from the environment\n" + " (which git exports to hooks in a linked worktree) escapes cwd, --local and the\n" + " GIT_CONFIG_* pins alike.\n" + " Fix the fixture: pass env=isolated_git_env(repo) from tests/git_env.py.\n" + f" Clean up the repo: git config --file {path} --unset-all core.worktree\n" + f" git config --file {path} --remove-section user", + file=sys.stderr, + flush=True, + ) + session.exitstatus = pytest.ExitCode.TESTS_FAILED + + def pytest_sessionfinish(session, exitstatus): - """Cancel the hang watchdog on a clean session finish. + """Cancel the hang watchdog on a clean session finish, then run the #855 check. - Without this, a legitimately slow-but-passing suite that finishes just after + Without the cancel, a legitimately slow-but-passing suite that finishes just after the 600s deadline (e.g. during teardown / coverage write) would be hard-exited by ``_reap_on_hang`` and turn green red with a thread-dump uncorrelated to any failed test. ``Timer.cancel()`` is a no-op if the timer already fired (a true - hang), so this only prevents the false-positive kill.""" + hang), so this only prevents the false-positive kill. + + The config check runs here rather than as a test because no test can observe a + mutation made by a test that runs after it.""" _hang_watchdog.cancel() + _report_shared_git_config_mutation(session) class FakeRunCmd: @@ -163,6 +248,46 @@ def make_task_config(**overrides) -> TaskConfig: ] +@pytest.fixture(autouse=True) +def _isolate_git_location(monkeypatch, tmp_path): + """Layer 1 of the #855 guard: PREVENT. Applies to every test, unconditionally. + + Placement is the whole point. #720/#731 got the *content* of this right but put it + in a per-class fixture inside ``test_post_hooks.py``, so #665 was free to add a + fresh unguarded ``_git()`` helper in ``test_registry_loader.py`` seven days later + and reopen the leak. An autouse fixture in ``conftest.py`` is the only placement + that also covers test files nobody has written yet. + + Two distinct jobs: + + 1. **Strip the repo-LOCATION vars.** While any of them is set, ``git -C ``, + ``cwd=``, ``--local`` and the ``GIT_CONFIG_*`` pins are all bypassed, because + an explicit ``GIT_DIR`` overrides repository discovery outright. Git exports + these to hooks in a linked worktree, which is exactly how this suite runs as a + pre-push gate from ``.worktrees/``. + + 2. **Pin config resolution and identity.** So that a fixture which shells out to + git *without* using ``isolated_git_env`` still cannot reach the developer's + ``~/.gitconfig``, and any commit it makes is attributed to the reserved test + identity rather than to whoever happens to be running the suite. + + Production code is a beneficiary too, not just fixtures: ``post_hooks`` and + ``repo`` shell out to git with the ambient environment, so an inherited ``GIT_DIR`` + would point the code under test at the real repository and the assertions would + silently describe the wrong one. + """ + for var in GIT_LOCATION_VARS: + monkeypatch.delenv(var, raising=False) + + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_path / ".gitconfig-test")) + monkeypatch.setenv("GIT_CONFIG_SYSTEM", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + monkeypatch.setenv("GIT_AUTHOR_NAME", TEST_IDENTITY_NAME) + monkeypatch.setenv("GIT_AUTHOR_EMAIL", TEST_IDENTITY_EMAIL) + monkeypatch.setenv("GIT_COMMITTER_NAME", TEST_IDENTITY_NAME) + monkeypatch.setenv("GIT_COMMITTER_EMAIL", TEST_IDENTITY_EMAIL) + + @pytest.fixture(autouse=True) def _clean_env(monkeypatch): """Remove agent-related env vars and reset the AWS session cache each test. diff --git a/agent/tests/git_env.py b/agent/tests/git_env.py new file mode 100644 index 00000000..53016575 --- /dev/null +++ b/agent/tests/git_env.py @@ -0,0 +1,152 @@ +"""Single source of truth for isolating test git invocations (#855). + +Four earlier fixes for the same leak (#622/#623, #695, #720/#731, #665) were each +placed in the file where the leak was observed, so none of them could protect the +next test file to shell out to git — #665 added a fresh unguarded helper seven days +after #731 hardened a different file. This module exists so there is exactly one +definition to import, and ``tests/conftest.py`` applies it to every test whether or +not the test author knew to ask. + +The mechanism, because it is not obvious from any single call site: + +An explicit ``GIT_DIR`` overrides repository **discovery** outright. That beats +``git -C ``, ``cwd=``, ``HOME=``, ``--local``, and the ``GIT_CONFIG_*`` pins +*simultaneously* — ``--local`` in particular resolves relative to ``GIT_DIR``, so it +is no defence. Git exports ``GIT_DIR``/``GIT_COMMON_DIR`` to hooks **only in a linked +worktree** (they are unset in a normal checkout), which is exactly how this suite runs +as a pre-push gate from ``.worktrees/``. Under that environment +``git -C config user.email t@t`` writes into the *real* shared ``.git/config`` +and ``git -C init`` re-inits the *real* repository instead of creating one in +````. + +That is why the bug reads as unreproducible: run the same tests by hand from the main +checkout and nothing leaks. +""" + +from __future__ import annotations + +import hashlib +import os +import subprocess + +# Repo-LOCATION vars, as distinct from config-CONTENT vars. Stripping these is +# load-bearing, not tidiness: while any one of them is set, every other containment +# measure below is bypassed. Keep this tuple as the only copy in the tree. +GIT_LOCATION_VARS: tuple[str, ...] = ( + "GIT_DIR", + "GIT_COMMON_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_PREFIX", + "GIT_CEILING_DIRECTORIES", +) + +# RFC-2606 reserved TLD: unroutable by construction, and recognisable in a stray +# commit. #720 was filed because the literal `t ` from a fixture was transcribed +# into a real repo's config and then into real commits. +TEST_IDENTITY_NAME = "ABCA Test" +TEST_IDENTITY_EMAIL = "abca-test@example.invalid" + +# `git config` timeout. Bounded so a wedged git cannot stall the session-level +# fingerprint and burn the suite's wall-clock budget. +_GIT_TIMEOUT_S = 30 + + +def isolated_git_env(repo, base: dict[str, str] | None = None) -> dict[str, str]: + """Return an environment in which git cannot reach outside *repo*. + + Order matters. The location vars are removed **first**, because the pins added + afterwards are all ineffective while a ``GIT_DIR`` is still present. + + *repo* doubles as ``HOME``, so a fixture that transcribes a bare + ``git config user.email ...`` (no ``--local``) lands in a throwaway file rather + than the developer's ``~/.gitconfig``. + """ + env = {k: v for k, v in (base or os.environ).items() if k not in GIT_LOCATION_VARS} + env.update( + { + "HOME": str(repo), + "XDG_CONFIG_HOME": str(repo), + "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), + "GIT_CONFIG_SYSTEM": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + # Identity via env, not config: these outrank every config file, so a + # commit is correctly attributed even if a config write is missed. + "GIT_AUTHOR_NAME": TEST_IDENTITY_NAME, + "GIT_AUTHOR_EMAIL": TEST_IDENTITY_EMAIL, + "GIT_COMMITTER_NAME": TEST_IDENTITY_NAME, + "GIT_COMMITTER_EMAIL": TEST_IDENTITY_EMAIL, + } + ) + return env + + +def shared_git_config_path() -> str | None: + """Absolute path of the repository-shared ``.git/config``, or None if unavailable. + + Resolved via ``--git-common-dir`` rather than ``--show-toplevel`` **on purpose**. + ``core.worktree`` — one of the values this leak writes — changes what + ``--show-toplevel`` returns, so an already-polluted repo would make this function + compute a path that does not exist and report "nothing to protect": the pollution + would disable its own detector. ``--git-common-dir`` is answered from the gitdir + alone and also resolves to the *shared* ``.git`` when called from a linked + worktree, which is the file actually at risk. Requires git >= 2.31 for + ``--path-format``. + + Returns None when there is no repository to protect (no git on PATH, or running + outside a checkout — e.g. inside the built container image). That is a genuine + "no risk" case, not a failure to look. + """ + try: + result = subprocess.run( + ["git", "rev-parse", "--path-format=absolute", "--git-common-dir"], + capture_output=True, + text=True, + check=False, + timeout=_GIT_TIMEOUT_S, + ) + except (OSError, subprocess.SubprocessError): + return None + if result.returncode != 0: + return None + common_dir = result.stdout.strip() + if not common_dir: + return None + config = os.path.join(common_dir, "config") + return config if os.path.isfile(config) else None + + +def fingerprint_git_config(path: str) -> tuple[str, frozenset[str]] | None: + """Digest *path* plus its key names, or None if it cannot be read. + + Deliberately returns key **names** and not values. A ``.git/config`` can legally + hold a remote URL with embedded credentials, so a change report built from this + can name what moved without printing anything secret. + """ + try: + with open(path, "rb") as handle: + raw = handle.read() + except OSError: + return None + digest = hashlib.sha256(raw).hexdigest() + names = frozenset(_config_key_names(path)) + return digest, names + + +def _config_key_names(path: str) -> list[str]: + """Config key names in *path*, via git itself so the parse matches git's.""" + try: + result = subprocess.run( + ["git", "config", "--file", path, "--list", "--name-only"], + capture_output=True, + text=True, + check=False, + timeout=_GIT_TIMEOUT_S, + ) + except (OSError, subprocess.SubprocessError): + return [] + if result.returncode != 0: + return [] + return [line for line in result.stdout.splitlines() if line] diff --git a/agent/tests/test_git_fixture_isolation.py b/agent/tests/test_git_fixture_isolation.py new file mode 100644 index 00000000..a9e49faf --- /dev/null +++ b/agent/tests/test_git_fixture_isolation.py @@ -0,0 +1,312 @@ +"""Tests for the git-fixture isolation guard (#855). + +The point of this file is that the guard is *proven live* rather than assumed. The +central test is differential: the **same** git command is run twice, once with an +inherited ``GIT_DIR`` and once through ``isolated_git_env``, and it is asserted to +escape in the first case and be contained in the second. A test that only checked the +contained case would still pass if ``isolated_git_env`` were quietly reduced to +``dict(os.environ)``. + +Every repository these tests touch is built inside ``tmp_path``. Nothing here writes +to the real repository — the "leak" half of the differential test leaks into a +purpose-built fake shared repo. +""" + +from __future__ import annotations + +import os +import subprocess +from types import SimpleNamespace + +import pytest + +from tests.git_env import ( + GIT_LOCATION_VARS, + TEST_IDENTITY_EMAIL, + TEST_IDENTITY_NAME, + fingerprint_git_config, + isolated_git_env, + shared_git_config_path, +) + + +def _git(repo, *args, env=None, check=True) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", "-C", str(repo), *args], + capture_output=True, + text=True, + check=check, + env=env if env is not None else isolated_git_env(repo), + timeout=60, + ) + + +def _config_get(config_path, key) -> str | None: + """Read *key* from *config_path*, or None when absent.""" + result = subprocess.run( + ["git", "config", "--file", str(config_path), "--get", key], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + return result.stdout.strip() if result.returncode == 0 else None + + +@pytest.fixture +def shared_repo(tmp_path): + """A real repo with a real-looking identity, plus a linked worktree. + + Stands in for the developer's checkout. The linked worktree matters because that is + the only configuration in which git exports ``GIT_DIR``/``GIT_COMMON_DIR`` to a + hook — which is why this leak never reproduces from a normal checkout. + """ + repo = tmp_path / "shared" + repo.mkdir() + _git(repo, "init", "-q") + _git(repo, "config", "--local", "user.name", "RealDev") + _git(repo, "config", "--local", "user.email", "real@dev.example") + _git(repo, "commit", "-q", "--allow-empty", "-m", "base") + _git(repo, "worktree", "add", "-q", str(tmp_path / "wt"), "-b", "probe") + return repo + + +class TestIsolatedGitEnv: + def test_strips_every_location_var(self, tmp_path): + base = dict.fromkeys(GIT_LOCATION_VARS, "/somewhere/else") + env = isolated_git_env(tmp_path, base=base) + assert not [var for var in GIT_LOCATION_VARS if var in env] + + def test_pins_config_resolution_and_identity(self, tmp_path): + env = isolated_git_env(tmp_path, base={}) + assert env["HOME"] == str(tmp_path) + assert env["GIT_CONFIG_GLOBAL"] == os.path.join(str(tmp_path), ".gitconfig-test") + assert env["GIT_CONFIG_SYSTEM"] == os.devnull + assert env["GIT_CONFIG_NOSYSTEM"] == "1" + assert env["GIT_AUTHOR_EMAIL"] == TEST_IDENTITY_EMAIL + assert env["GIT_COMMITTER_NAME"] == TEST_IDENTITY_NAME + + def test_an_inherited_git_dir_escapes_but_isolated_env_contains(self, tmp_path, shared_repo): + """The differential test. Same command, two environments, opposite outcomes. + + Half A reproduces the bug against a fake shared repo: with ``GIT_DIR`` present, + ``git -C config user.name`` still finds the shared repository and + writes there. Note what this defeats — ``-C`` pointing at a directory that is + not a repository at all, plus ``HOME``/``XDG_CONFIG_HOME``/``GIT_CONFIG_GLOBAL`` + all pinned to a throwaway path. Repository *discovery* is what ``GIT_DIR`` + overrides, so none of those pins are consulted. + + Half B is the same write through ``isolated_git_env``, which lands in the + sandbox's own config and leaves the shared repo byte-identical. + """ + shared_config = shared_repo / ".git" / "config" + before = shared_config.read_bytes() + + # --- Half A: the leak, witnessed --- + escapes = tmp_path / "escapes" + escapes.mkdir() + leaky_env = isolated_git_env(escapes) + leaky_env["GIT_DIR"] = str(shared_repo / ".git" / "worktrees" / "wt") + leaky_env["GIT_COMMON_DIR"] = str(shared_repo / ".git") + + _git(escapes, "config", "user.name", "leaked", env=leaky_env) + + assert not (escapes / ".git").exists(), "the write should not have landed locally" + assert _config_get(shared_config, "user.name") == "leaked", ( + "expected the inherited GIT_DIR to redirect this write into the shared " + "config — if this assertion fails the mechanism has changed and the guard " + "may no longer be guarding anything" + ) + + # Restore, so Half B starts from the original bytes. + shared_config.write_bytes(before) + + # --- Half B: the same write, contained --- + contained = tmp_path / "contained" + contained.mkdir() + _git(contained, "init", "-q") + _git(contained, "config", "user.name", "contained") + + assert _config_get(contained / ".git" / "config", "user.name") == "contained" + assert shared_config.read_bytes() == before, "shared config must be untouched" + + +class TestAutouseFixture: + def test_ambient_location_vars_are_stripped(self): + """``conftest._isolate_git_location`` has already run for this test. + + Asserted on ``os.environ`` rather than on a passed-in env because the risk is a + fixture that shells out with the *inherited* environment. + """ + assert not [var for var in GIT_LOCATION_VARS if var in os.environ] + + def test_ambient_config_resolution_is_pinned(self): + assert os.environ["GIT_CONFIG_SYSTEM"] == os.devnull + assert os.environ["GIT_CONFIG_NOSYSTEM"] == "1" + assert os.environ["GIT_AUTHOR_EMAIL"] == TEST_IDENTITY_EMAIL + # Pinned to a per-test tmp path, so a bare `git config user.email` cannot reach + # the developer's ~/.gitconfig even from a fixture that forgot isolated_git_env. + assert os.environ["GIT_CONFIG_GLOBAL"].endswith(".gitconfig-test") + assert os.path.expanduser("~") not in (os.environ["GIT_CONFIG_GLOBAL"],) + + +class TestSharedConfigResolution: + def test_resolves_through_git_common_dir_not_show_toplevel(self, tmp_path, monkeypatch): + """``core.worktree`` must not be able to disable the detector. + + This is the failure the resolution choice exists to avoid: ``core.worktree`` + redirects ``--show-toplevel``, so a detector built on it computes a path that + does not exist in a polluted repo and reports "nothing to protect" — the + pollution switching off its own alarm. ``--git-common-dir`` is answered from the + gitdir alone. + """ + repo = tmp_path / "polluted" + repo.mkdir() + _git(repo, "init", "-q") + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + _git(repo, "config", "--local", "core.worktree", str(elsewhere)) + + monkeypatch.chdir(repo) + + # The rejected approach: redirected away from the real repo. + toplevel = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + check=False, + timeout=60, + ) + assert toplevel.stdout.strip() != str(repo) + + # The chosen approach: still the real shared config. + assert shared_git_config_path() == str(repo / ".git" / "config") + + def test_returns_none_outside_a_repository(self, tmp_path, monkeypatch): + outside = tmp_path / "not-a-repo" + outside.mkdir() + monkeypatch.chdir(outside) + # GIT_CEILING_DIRECTORIES stops discovery from walking up into whatever + # repository happens to contain tmp_path on this machine. + monkeypatch.setenv("GIT_CEILING_DIRECTORIES", str(tmp_path)) + assert shared_git_config_path() is None + + +class TestFingerprint: + @staticmethod + def _fingerprint(config) -> tuple[str, frozenset[str]]: + """``fingerprint_git_config`` narrowed to non-None. + + It returns ``None`` for an unreadable path — a real case, covered by its own + test below — so unpacking the result directly is a type error (ty + ``not-iterable``). Asserting here keeps that contract visible instead of + annotating it away, and a None would fail the assertion rather than raise an + opaque unpacking error further down. + """ + result = fingerprint_git_config(str(config)) + assert result is not None, f"expected {config} to be readable" + return result + + def test_detects_an_added_key_and_names_it(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + + digest_before, names_before = self._fingerprint(config) + _git(repo, "config", "--local", "core.worktree", str(tmp_path)) + digest_after, names_after = self._fingerprint(config) + + assert digest_after != digest_before + assert names_after - names_before == {"core.worktree"} + + def test_detects_a_value_change_without_capturing_the_value(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + config = repo / ".git" / "config" + # A remote URL with a userinfo segment: the realistic reason `.git/config` + # must never be echoed. Synthetic — reserved domain (RFC 2606), and the + # userinfo is the literal word `placeholder`. Named for what it is (a URL) + # rather than `secret`, which made ruff S105 read it as a hardcoded + # credential; nothing here is one. + url_with_credential = "https://user:placeholder@example.invalid/repo.git" + _git(repo, "config", "--local", "remote.origin.url", "https://example.invalid/a.git") + + digest_before, names_before = self._fingerprint(config) + _git(repo, "config", "--local", "remote.origin.url", url_with_credential) + digest_after, names_after = self._fingerprint(config) + + assert digest_after != digest_before, "a value-only change must still be detected" + assert names_after == names_before, "no key was added, so the name set is stable" + # The reason names-not-values: this data is printed into CI logs on failure. + assert url_with_credential not in str(names_after) + + def test_returns_none_for_an_unreadable_path(self, tmp_path): + assert fingerprint_git_config(str(tmp_path / "nope" / "config")) is None + + +class TestMutationReport: + """The session-level detector's decision logic (``conftest``, Layer 2). + + Unit-tested here because the hook itself cannot be exercised from inside the + session it guards: no test can observe a mutation made by a test that runs after + it, which is precisely why the check lives in ``pytest_sessionfinish``. + """ + + @staticmethod + def _repo_with_config(tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + return repo, repo / ".git" / "config" + + def _run_report(self, monkeypatch, config, fingerprint): + from tests import conftest + + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", (str(config), fingerprint)) + session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) + conftest._report_shared_git_config_mutation(session) + return session + + def test_fails_the_session_when_the_config_changed(self, tmp_path, monkeypatch, capsys): + repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + _git(repo, "config", "--local", "user.email", "t@t") + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + message = capsys.readouterr().err + assert "SHARED GIT CONFIG MUTATED" in message + # The remedy must be copy-pasteable, not a description of one. + assert f"git config --file {config} --remove-section user" in message + assert "user.email" in message + + def test_leaves_a_clean_session_alone(self, tmp_path, monkeypatch): + _repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.OK + + def test_is_inert_when_there_was_nothing_to_protect(self, monkeypatch): + """No repository (e.g. running inside the built container image) must not fail + the suite — that is a genuine no-risk case, not a failure to look.""" + from tests import conftest + + monkeypatch.setattr(conftest, "_SHARED_GIT_CONFIG", None) + session = SimpleNamespace(exitstatus=pytest.ExitCode.OK) + conftest._report_shared_git_config_mutation(session) + assert session.exitstatus == pytest.ExitCode.OK + + def test_reports_a_config_that_vanished(self, tmp_path, monkeypatch, capsys): + _repo, config = self._repo_with_config(tmp_path) + fingerprint = fingerprint_git_config(str(config)) + config.unlink() + + session = self._run_report(monkeypatch, config, fingerprint) + + assert session.exitstatus == pytest.ExitCode.TESTS_FAILED + assert "unreadable or gone" in capsys.readouterr().err diff --git a/agent/tests/test_post_hooks.py b/agent/tests/test_post_hooks.py index 3768aec1..6ffc6b2f 100644 --- a/agent/tests/test_post_hooks.py +++ b/agent/tests/test_post_hooks.py @@ -6,15 +6,13 @@ ``shell.run_cmd`` (mutating git/gh commands) — both faked with recorders. """ -import os import subprocess from types import SimpleNamespace -import pytest - import post_hooks from models import RepoSetup from tests.conftest import FakeRunCmd, make_task_config +from tests.git_env import isolated_git_env # post_hooks.py keys scripted results off the exact label (FakeRunCmd's default # exact-match mode), so e.g. returncodes={"push": 1} does not bleed into the @@ -279,81 +277,16 @@ class TestReconcileAgentBranch: higher confidence than faking subprocess. The two seams (subprocess.run for the branch read, run_cmd for the mutating ops) both hit the tmp repo.""" - # Repo-LOCATION vars. An explicit GIT_DIR overrides repository discovery - # outright, so it beats cwd, HOME, the GIT_CONFIG_* pins and `--local` - # alike. Git exports these to hooks in a LINKED WORKTREE (unset in a normal - # repo), which is exactly how this suite runs as a pre-push gate from - # .worktrees/. - _GIT_LOCATION_VARS = ( - "GIT_DIR", - "GIT_COMMON_DIR", - "GIT_WORK_TREE", - "GIT_INDEX_FILE", - "GIT_OBJECT_DIRECTORY", - "GIT_ALTERNATE_OBJECT_DIRECTORIES", - "GIT_PREFIX", - "GIT_CEILING_DIRECTORIES", - ) - - @pytest.fixture(autouse=True) - def _clear_ambient_git_location(self, monkeypatch): - """Strip repo-location vars for the whole class (#720). - - Not just for the fixture helpers: ``post_hooks`` itself shells out to - git with the ambient environment (e.g. ``_current_branch``), so an - inherited GIT_DIR would point PRODUCTION code at the real repo instead - of the tmp one — the assertions would silently describe the wrong - repository. - """ - for var in self._GIT_LOCATION_VARS: - monkeypatch.delenv(var, raising=False) - - @staticmethod - def _isolated_env(repo): - # Hard-isolate from the developer's real git identity (#720). `cwd` alone - # is NOT containment: a bare `git config` walks up to the nearest - # enclosing repo, and `git init` at a linked-worktree root re-inits the - # SHARED .git rather than creating a nested one — so both can write - # straight into the real .git/config. Pinning the HOME/config env vars - # means even a transcribed `git config user.email` cannot escape tmp. - # - # Dropping the repo-LOCATION vars first is load-bearing, not tidiness. - # An explicit GIT_DIR overrides repository discovery outright, so it - # defeats cwd, HOME and the GIT_CONFIG_* pins together — and `--local` - # resolves relative to it, so that is no defence either. Git exports - # GIT_DIR to hooks in a LINKED WORKTREE (it is unset in a normal repo), - # which is exactly how this suite runs as a pre-push gate from - # .worktrees/: inheriting it re-opens #720 and additionally stamps - # `bare = true` on the real repo. - env = { - k: v - for k, v in os.environ.items() - if k - not in { - "GIT_DIR", - "GIT_COMMON_DIR", - "GIT_WORK_TREE", - "GIT_INDEX_FILE", - "GIT_OBJECT_DIRECTORY", - "GIT_ALTERNATE_OBJECT_DIRECTORIES", - "GIT_PREFIX", - "GIT_CEILING_DIRECTORIES", - } - } - env.update( - { - "HOME": str(repo), - "XDG_CONFIG_HOME": str(repo), - "GIT_CONFIG_GLOBAL": os.path.join(str(repo), ".gitconfig-test"), - "GIT_CONFIG_SYSTEM": os.devnull, - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_AUTHOR_NAME": "ABCA Test", - "GIT_AUTHOR_EMAIL": "abca-test@example.invalid", - "GIT_COMMITTER_NAME": "ABCA Test", - "GIT_COMMITTER_EMAIL": "abca-test@example.invalid", - } - ) - return env + # Containment comes from ``tests/git_env.isolated_git_env`` (#855), applied + # explicitly below AND to every test by the ``_isolate_git_location`` autouse + # fixture in ``tests/conftest.py``. This class used to carry its own copies of + # the location-var tuple and the env builder; they were correct but reachable + # from nowhere else, so #665 added a fresh unguarded helper in + # test_registry_loader.py and reopened the leak. One definition, imported. + # + # Kept explicit here on top of the autouse fixture because ``post_hooks`` + # itself shells out to git: an env passed per call documents at the call site + # that these fixtures must never touch a repo outside ``tmp_path``. def _git(self, repo, *args): subprocess.run( @@ -362,7 +295,7 @@ def _git(self, repo, *args): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ) def _make_repo(self, tmp_path): @@ -391,15 +324,25 @@ def test_fixture_cannot_touch_an_outer_repo_even_with_git_dir_set(self, tmp_path _git ever stops stripping those vars.""" outer = tmp_path / "outer" outer.mkdir() - # Build the stand-in "real" repo with the location vars still cleared by - # the autouse fixture, so this setup lands in tmp and not the actual repo. - subprocess.run(["git", "init", "-q"], cwd=outer, check=True, capture_output=True) + # Build the stand-in "real" repo through isolated_git_env rather than the + # ambient environment. The test is *about* a hostile ambient env, so its own + # setup must not depend on one being clean — otherwise a regression in the + # conftest fixture would make this guard build its sentinel in the actual + # repository, i.e. cause the very leak it exists to detect. + subprocess.run( + ["git", "init", "-q"], + cwd=outer, + check=True, + capture_output=True, + env=isolated_git_env(outer), + ) sentinel_config = outer / ".git" / "config" subprocess.run( ["git", "config", "--local", "user.email", "sentinel@example.invalid"], cwd=outer, check=True, capture_output=True, + env=isolated_git_env(outer), ) before = sentinel_config.read_text() @@ -423,7 +366,7 @@ def _head_sha(self, repo): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ).stdout.strip() def _sha_of(self, repo, ref): @@ -433,7 +376,7 @@ def _sha_of(self, repo, ref): check=True, capture_output=True, text=True, - env=self._isolated_env(repo), + env=isolated_git_env(repo), ).stdout.strip() def test_reconciles_when_agent_on_own_branch(self, tmp_path): diff --git a/agent/tests/test_registry_loader.py b/agent/tests/test_registry_loader.py index 21717ecb..ce3c963e 100644 --- a/agent/tests/test_registry_loader.py +++ b/agent/tests/test_registry_loader.py @@ -13,6 +13,7 @@ apply_resolved_assets, build_skill_prompt_fragment, ) +from tests.git_env import isolated_git_env def _read_mcp(repo_dir) -> dict: @@ -295,17 +296,27 @@ class TestMcpJsonNotCommittable: @staticmethod def _git(repo, *args) -> subprocess.CompletedProcess: + # ``env=`` is load-bearing (#855). Without it, an inherited GIT_DIR — which + # git exports to hooks in a linked worktree, i.e. whenever this suite runs + # as a pre-push gate from .worktrees/ — overrides repository discovery, so + # `-C ` is ignored and every command below operates on the REAL + # repository. ``check=False`` is why that stayed invisible: re-initing the + # real repo and rewriting its config both exit 0. return subprocess.run( ["git", "-C", str(repo), *args], capture_output=True, text=True, check=False, + env=isolated_git_env(repo), ) def _init_repo(self, tmp_path): + # No `git config user.*` here on purpose: isolated_git_env supplies the + # identity through GIT_AUTHOR_*/GIT_COMMITTER_*, which outrank every config + # file, so the commit below is attributed without any config write at all. + # The two writes this replaces are the literal source of #720 — `t ` + # was transcribed into a real repository's config and then into real commits. self._git(tmp_path, "init", "-q") - self._git(tmp_path, "config", "user.email", "t@t") - self._git(tmp_path, "config", "user.name", "t") (tmp_path / "README.md").write_text("x") self._git(tmp_path, "add", "README.md") self._git(tmp_path, "commit", "-qm", "init") diff --git a/cdk/test/scripts/check-git-config-clean.test.ts b/cdk/test/scripts/check-git-config-clean.test.ts new file mode 100644 index 00000000..4cec828a --- /dev/null +++ b/cdk/test/scripts/check-git-config-clean.test.ts @@ -0,0 +1,393 @@ +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Tests for `scripts/check-git-config-clean.mjs` — Layer 3 of the #855 git-config + * guard, the pre-commit/pre-push gate. + * + * WHY THESE EXIST: it is a GATE, and a gate's worst failure is a false pass. This + * one has two independent ways to reach one: a detection rule that stops matching, + * and a config-path resolution that quietly points somewhere harmless. The second is + * not hypothetical — the first draft resolved the path with + * `git rev-parse --git-common-dir`, which ABORTS when `core.worktree` names a + * missing directory, so on the most common real shape of this corruption it could + * only say "could not check". So every rule is asserted by making it fire, and the + * hostile-resolution cases have tests of their own. + * + * WHY THIS LIVES UNDER `cdk/test/` for a ROOT-level script: same reason as + * `check-constants-sync.test.ts` — there is no test tree at the repo root, and + * `cdk/` is the only workspace with a Jest runner that can reach `../../scripts`. + * Deliberate placement, not misrouting. The suite exercises a subprocess, so it + * contributes nothing to `cdk/src` coverage. + * + * NOTE ON THIS FILE'S OWN GIT CALLS: they go through `isolatedGitEnv`, a TypeScript + * mirror of `agent/tests/git_env.py`. That is not ceremony. Jest here may itself be + * running under the pre-push hook, where git has exported `GIT_DIR` — and an + * inherited `GIT_DIR` would make `git init ` re-init the REAL repository. A + * test suite for this gate that caused the leak while setting up would be a poor + * joke, so the isolation is applied and then asserted on (see the last describe). + */ + +import { execFileSync, spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const REPO_ROOT = path.resolve(__dirname, '../../..'); +const SCRIPT = path.join(REPO_ROOT, 'scripts/check-git-config-clean.mjs'); + +/** + * The shared config of the checkout this suite is running in. + * + * NOT `join(REPO_ROOT, '.git', 'config')`: in a linked worktree — which is how this + * repo's own contribution flow works — `.git` is a FILE pointing elsewhere, so that + * path does not exist. Asked of git rather than hand-resolved because the script under + * test resolves it without git, and a hand-rolled copy here would agree with the + * script's bugs instead of catching them. + */ +function realSharedConfigPath(): string { + const commonDir = execFileSync( + 'git', + ['-C', REPO_ROOT, 'rev-parse', '--path-format=absolute', '--git-common-dir'], + { encoding: 'utf-8' }, + ).trim(); + return path.join(commonDir, 'config'); +} + +/** Repo-location vars — mirrors `GIT_LOCATION_VARS` in `agent/tests/git_env.py`. */ +const GIT_LOCATION_VARS = [ + 'GIT_DIR', + 'GIT_COMMON_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_PREFIX', + 'GIT_CEILING_DIRECTORIES', +]; + +/** An environment in which git cannot reach outside `repo`. */ +function isolatedGitEnv(repo: string): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = { ...process.env }; + // Removed FIRST: while any is set, every pin below is bypassed. + for (const key of GIT_LOCATION_VARS) delete env[key]; + return { + ...env, + HOME: repo, + XDG_CONFIG_HOME: repo, + GIT_CONFIG_GLOBAL: path.join(repo, '.gitconfig-test'), + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_CONFIG_NOSYSTEM: '1', + GIT_AUTHOR_NAME: 'ABCA Test', + GIT_AUTHOR_EMAIL: 'abca-test@example.invalid', + GIT_COMMITTER_NAME: 'ABCA Test', + GIT_COMMITTER_EMAIL: 'abca-test@example.invalid', + }; +} + +function git(repo: string, args: readonly string[]): void { + const result = spawnSync('git', ['-C', repo, ...args], { + encoding: 'utf-8', + env: isolatedGitEnv(repo), + }); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed (${result.status}): ${result.stderr}`); + } +} + +interface RunResult { + readonly status: number; + readonly stdout: string; + readonly stderr: string; +} + +/** Run the gate with `cwd` (and optionally extra env), capturing the outcome. */ +function runGate(cwd: string, extraEnv: NodeJS.ProcessEnv = {}): RunResult { + try { + const stdout = execFileSync(process.execPath, [SCRIPT], { + cwd, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...isolatedGitEnv(cwd), ...extraEnv }, + }); + return { status: 0, stdout, stderr: '' }; + } catch (err) { + const e = err as { status?: number; stdout?: string; stderr?: string }; + return { status: e.status ?? -1, stdout: e.stdout ?? '', stderr: e.stderr ?? '' }; + } +} + +let scratch: string; + +/** A fresh, clean repository under the scratch dir. */ +function freshRepo(name: string): string { + const repo = path.join(scratch, name); + fs.mkdirSync(repo, { recursive: true }); + git(repo, ['init', '-q']); + return repo; +} + +/** Set a local config key, bypassing git (which may refuse on a broken repo). */ +function appendConfig(repo: string, section: string, lines: readonly string[]): void { + const configPath = path.join(repo, '.git', 'config'); + fs.appendFileSync(configPath, `[${section}]\n${lines.map((l) => `\t${l}\n`).join('')}`); +} + +describe('check-git-config-clean', () => { + // A handful of subprocess spawns plus git inits. + jest.setTimeout(60_000); + + /** Digest of the real shared config, captured before any test body runs. */ + let sharedConfigDigestAtStart: string; + + beforeAll(() => { + // os.tmpdir() honours TMPDIR, which the pre-push hook points at + // ~/.cache/cdk-tmp — so this does not land in a RAM-backed /tmp there. + // + // realpathSync because assertions below compare against paths the SCRIPT + // printed, and the script derives them from `process.cwd()`, which Node reports + // physically. On a machine where $HOME is a symlink (e.g. /home/x → + // /local/home/x) the logical and physical spellings differ, and the remedy + // strings would never match. + scratch = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'abca-git-config-clean-'))); + sharedConfigDigestAtStart = execFileSync('git', ['hash-object', realSharedConfigPath()], { + encoding: 'utf-8', + }).trim(); + }); + + afterAll(() => { + fs.rmSync(scratch, { recursive: true, force: true }); + }); + + describe('the clean cases', () => { + test('a fresh repository passes, and says what it checked', () => { + const result = runGate(freshRepo('clean')); + + expect(result.status).toBe(0); + // The rule list is the anti-vacuity assertion: a gate that inspected NOTHING + // would also exit 0. Naming them means a dropped rule shows up here. + expect(result.stdout).toContain('core.worktree'); + expect(result.stdout).toContain('core.bare'); + expect(result.stdout).toContain('user.name'); + expect(result.stdout).toContain('user.email'); + expect(result.stdout).toMatch(/OK — 4 rule\(s\)/); + }); + + test('THIS repository passes', () => { + // Not a self-test for its own sake: this is a third detection surface, after + // the conftest fixture (prevent) and the session hook (detect). If a + // contributor's shared config is polluted, the cdk suite says so here. + const result = runGate(REPO_ROOT); + + expect(result.stderr).toBe(''); + expect(result.status).toBe(0); + }); + + test('a real per-repo identity is NOT flagged', () => { + // The false-positive side, and the reason the rules match the leak's + // SIGNATURE rather than the mere presence of a [user] section. Per-repo + // identities are common; a gate that failed on them would be switched off + // instead of fixed. + const repo = freshRepo('real-identity'); + git(repo, ['config', '--local', 'user.name', 'Ada Lovelace']); + git(repo, ['config', '--local', 'user.email', 'ada@example-corp.dev']); + + expect(runGate(repo).status).toBe(0); + }); + + test('a GitHub noreply address is NOT flagged', () => { + const repo = freshRepo('noreply'); + git(repo, ['config', '--local', 'user.email', '1234+ada@users.noreply.github.com']); + + expect(runGate(repo).status).toBe(0); + }); + + test('core.bare = false is NOT flagged', () => { + // `git init` writes this itself, so flagging it would fail every repository. + const repo = freshRepo('bare-false'); + git(repo, ['config', '--local', 'core.bare', 'false']); + + expect(runGate(repo).status).toBe(0); + }); + }); + + describe('core.worktree — the #622/#720 signature', () => { + test('is rejected, with a copy-pasteable remedy', () => { + const repo = freshRepo('worktree'); + const elsewhere = path.join(scratch, 'elsewhere'); + fs.mkdirSync(elsewhere, { recursive: true }); + git(repo, ['config', '--local', 'core.worktree', elsewhere]); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.worktree'); + // The remedy must be runnable as printed, not a description of one. + expect(result.stderr).toContain( + `git config --file ${path.join(repo, '.git', 'config')} --unset-all core.worktree`, + ); + }); + + test('is rejected even when it points at a path that no longer EXISTS', () => { + // The case that drove the design. `core.worktree` left behind by a fixture + // names a pytest tmp_path, which is deleted at the end of the session — and + // `git rev-parse` (any form) then aborts with `fatal: Invalid path`, as does + // `git config` run from inside the repo. A gate that resolved its own target + // through git could only report "could not check" on the most common real + // shape of this corruption. Written with fs.appendFileSync because git itself + // refuses to set the second key once the first has broken the repo. + const repo = freshRepo('worktree-missing'); + appendConfig(repo, 'core', [`worktree = ${path.join(scratch, 'deleted-tmp-path')}`]); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.worktree'); + expect(result.stderr).toContain('deleted-tmp-path'); + }); + + test('is found in the SHARED config when run from a linked worktree', () => { + // Linked worktrees are where this leak happens, so resolution has to follow + // the `commondir` pointer rather than stopping at the per-worktree gitdir. + const repo = freshRepo('shared'); + fs.writeFileSync(path.join(repo, 'f.txt'), 'x\n'); + git(repo, ['add', '-A']); + git(repo, ['commit', '-qm', 'base']); + const linked = path.join(scratch, 'linked-wt'); + git(repo, ['worktree', 'add', '-q', linked, '-b', 'probe']); + + appendConfig(repo, 'user', ['name = t', 'email = t@t']); + + const result = runGate(linked); + + expect(result.status).toBe(1); + expect(result.stderr).toContain(path.join(repo, '.git', 'config')); + expect(result.stderr).toContain('user.name'); + }); + }); + + describe('core.bare on a checkout', () => { + test('is rejected', () => { + const repo = freshRepo('bare-true'); + git(repo, ['config', '--local', 'core.bare', 'true']); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('core.bare'); + expect(result.stderr).toContain('--unset-all core.bare'); + }); + }); + + describe('fixture identities — the #720 sighting', () => { + test.each([ + ['user.name = t', 'user', ['name = t']], + ['user.email = t@t (no dot in the domain)', 'user', ['email = t@t']], + ['a reserved .invalid domain', 'user', ['email = abca-test@example.invalid']], + ['example.com', 'user', ['email = someone@example.com']], + ['an empty value', 'user', ['name = ']], + ])('%s is rejected', (_label, section, lines) => { + const repo = freshRepo(`identity-${_label.replace(/[^a-z0-9]+/gi, '-')}`); + appendConfig(repo, section, lines); + + const result = runGate(repo); + + expect(result.status).toBe(1); + expect(result.stderr).toContain('--remove-section user'); + }); + + test('names the offending value so the human can see what replaced theirs', () => { + const repo = freshRepo('identity-named'); + appendConfig(repo, 'user', ['email = t@t']); + + expect(runGate(repo).stderr).toContain('user.email = t@t'); + }); + }); + + describe('cannot-check is a FAILURE, not a pass', () => { + test('outside any repository, exits 2 and says why', () => { + // Fail-closed. Silently exiting 0 here would make a mis-wired hook look like a + // clean repo forever. + // + // Run from `/` rather than a scratch dir: resolution walks UP for `.git`, so a + // scratch dir's verdict would depend on where TMPDIR points (inside a checkout + // on some machines, outside on others). `/` has no parent, so the walk + // terminates immediately and the outcome is the same everywhere. + expect(fs.existsSync('/.git')).toBe(false); // the one assumption `/` makes + + const result = runGate('/'); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('no `.git` found'); + }); + + test('a missing .git/config exits 2 rather than reporting clean', () => { + const repo = freshRepo('no-config'); + fs.rmSync(path.join(repo, '.git', 'config')); + + const result = runGate(repo); + + expect(result.status).toBe(2); + expect(result.stderr).toContain('does not exist'); + }); + }); + + describe('an inherited GIT_DIR — the hook environment', () => { + test('is honoured for locating the repo, and does not blind the check', () => { + // Git exports GIT_DIR to hooks in a linked worktree. For a WRITE that is the + // hazard this whole issue is about; for the gate's READ it is the accurate + // answer, so it is used — and must still find the corruption. + const repo = freshRepo('git-dir-env'); + appendConfig(repo, 'user', ['email = t@t']); + const unrelated = path.join(scratch, 'unrelated-cwd'); + fs.mkdirSync(unrelated, { recursive: true }); + + const result = runGate(unrelated, { GIT_DIR: path.join(repo, '.git') }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain(path.join(repo, '.git', 'config')); + }); + }); + + describe("this suite's own git isolation", () => { + test('isolatedGitEnv strips every location var and pins config resolution', () => { + // Asserted because the isolation is what stops these tests from re-creating + // the bug while setting up: with a GIT_DIR inherited from the pre-push hook, + // `git init ` re-inits the real repository. + const env = isolatedGitEnv('/somewhere'); + + for (const key of GIT_LOCATION_VARS) { + expect(env[key]).toBeUndefined(); + } + expect(env.HOME).toBe('/somewhere'); + expect(env.GIT_CONFIG_GLOBAL).toBe('/somewhere/.gitconfig-test'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); + }); + + test('the real repository config is byte-identical after this suite has run', () => { + // The blunt instrument, and the one that would actually have caught #622, + // #695, #720 and #665. Declared last so it runs last in file order. + const digest = execFileSync('git', ['hash-object', realSharedConfigPath()], { + encoding: 'utf-8', + }).trim(); + + expect(digest).toBe(sharedConfigDigestAtStart); + }); + }); +}); diff --git a/mise.toml b/mise.toml index 34c8b787..4c40349b 100644 --- a/mise.toml +++ b/mise.toml @@ -106,6 +106,10 @@ run = "yarn knip" description = "Dead-code ratchet (#282): fail only if knip's issue count rises above knip-baseline.json. Advisory in CI for now; flips to blocking once the baseline is driven to zero." run = "node scripts/check-deadcode-ratchet.mjs" +[tasks."check:git-config-clean"] +description = "Shared .git/config corruption gate (#855): fail if the repository's shared config carries the test-fixture leak signature — core.worktree, core.bare on a checkout, or a fixture identity in [user]. Local-only by design (a CI runner's config is ephemeral); wired into pre-commit and pre-push." +run = "node scripts/check-git-config-clean.mjs" + [tasks."check:transitive-pin-sync"] description = "Transitive-pin sync guard (#712): fail if a package pinned in root `resolutions` resolves below that floor in integrations/jira-forge-app's npm lockfile — the standalone project root `resolutions` can't reach." run = "node scripts/check-transitive-pin-sync.mjs" diff --git a/scripts/check-git-config-clean.mjs b/scripts/check-git-config-clean.mjs new file mode 100644 index 00000000..1bc6460f --- /dev/null +++ b/scripts/check-git-config-clean.mjs @@ -0,0 +1,318 @@ +#!/usr/bin/env node +/** + * MIT No Attribution + * + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of + * the Software, and to permit persons to whom the Software is furnished to do so. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/** + * Shared `.git/config` corruption gate (issue #855; recurrences #622, #695, #720, #665). + * + * Layer 3 of three. Layer 1 (`agent/tests/git_env.py` + the `_isolate_git_location` + * autouse fixture in `agent/tests/conftest.py`) PREVENTS the leak; Layer 2 + * (`pytest_sessionstart`/`pytest_sessionfinish` in the same conftest) DETECTS a + * mutation during a test run. This layer REFUSES: it runs at pre-commit and + * pre-push and blocks the operation while the repository's shared config carries + * the leak's signature, no matter which tool wrote it. + * + * Three layers rather than one because the same bug has now been "fixed" four + * times. Each earlier fix hardened the one test file where the leak was observed, + * and each was defeated by the next file to shell out to git. A gate outside the + * test suite entirely cannot be outrun that way. + * + * WHAT IT LOOKS FOR — the signature, not merely unusual settings: + * + * 1. `core.worktree` — never legitimate in a normal checkout. Set, it pins EVERY + * linked worktree to one directory, so the root reads as dirty, the root's own + * untracked files disappear from `git status`, and `git revert` silently + * no-ops. Written when a fixture runs `git init` with both GIT_DIR and + * GIT_WORK_TREE inherited from the environment. + * 2. `core.bare = true` on a repo that has a working tree — the other stamp the + * same `git init` leaves behind. + * 3. `user.name`/`user.email` holding a value no human would have: a reserved + * documentation domain (RFC 2606), a domain with no dot, or one of the literal + * fixture identities used in this repo. A real per-repo identity is COMMON and + * deliberately NOT flagged — a gate that fired on legitimate configuration + * would be switched off rather than fixed. + * + * WHY THE CONFIG PATH IS RESOLVED WITHOUT GIT AT ALL: no `git rev-parse` form + * survives the state being detected. `--show-toplevel` is redirected by + * `core.worktree` outright — the corruption disabling its own alarm — and + * `--git-common-dir` merely fails differently: when `core.worktree` names a path + * that no longer exists (a deleted pytest `tmp_path`, i.e. the shape this leak + * actually leaves behind), rev-parse aborts with `fatal: Invalid path`, so a check + * built on it can only report "could not check" and never name the cause. Walking + * the filesystem for `.git` is deterministic and reads no config, so it answers + * correctly on a repository too broken for git to describe. The + * `.pre-commit-config.yaml` entry for this hook is likewise the only one in the file + * that does NOT `cd "$(git rev-parse --show-toplevel)"`. + * + * Reads are delegated to `git config --file ` so the parse is git's own, and + * because `--file` involves no repository discovery — the one git operation this + * corruption cannot reach. + * + * Exit codes: 0 clean · 1 corruption found (with remedy) · 2 could not check. + * Case 2 is a failure, not a pass: an unreadable config or a git that cannot answer + * is exactly the state in which a leak would go unnoticed. + * + * Known limitation: submodules. Git legitimately sets `core.worktree` in a + * submodule's own config, and `--git-common-dir` resolves to whichever repository + * cwd belongs to — so committing from inside a submodule would flag rule 1. This + * repo has no submodules; if that changes, exempt them explicitly rather than + * dropping the rule. + */ + +import { existsSync, readFileSync, statSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { dirname, isAbsolute, join, resolve } from 'node:path'; + +/** Literal identities used by fixtures in this tree. `t ` is the #720 sighting. */ +const FIXTURE_NAMES = new Set(['t', 'test', 'abca test', 'test user', 'your name']); + +/** + * Reserved / documentation domains (RFC 2606 + RFC 6761). An address here can never + * be a real deliverable identity, so finding one in a repo config means a fixture + * put it there. + */ +const RESERVED_EMAIL_SUFFIXES = [ + '.invalid', + '.test', + '.example', + '.localhost', + '@example.com', + '@example.net', + '@example.org', +]; + +/** + * Repo-location vars, mirroring `GIT_LOCATION_VARS` in `agent/tests/git_env.py`. + * Stripped before reading, for the same reason the fixtures strip them: while any is + * set, git resolves a repository from the environment instead of from what we asked. + */ +const GIT_LOCATION_VARS = [ + 'GIT_DIR', + 'GIT_COMMON_DIR', + 'GIT_WORK_TREE', + 'GIT_INDEX_FILE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_PREFIX', +]; + +/** + * Run git against an explicit config file from OUTSIDE any repository. + * + * The cwd and the env pins are both load-bearing, and the reason is unobvious: + * `git config --file ` reads only that file, but git still performs + * REPOSITORY SETUP for its working directory first — so run inside a repo whose + * `core.worktree` names a missing directory, it aborts with `fatal: Invalid path` + * before reading anything. That is the exact state this gate has to report on, so + * the read cannot happen from inside the repository. cwd `/` plus + * `GIT_CEILING_DIRECTORIES` leaves discovery nothing to find, and the location vars + * are dropped so an inherited `GIT_DIR` (git sets one for hooks in a linked + * worktree) cannot put the broken repository back. + * + * Never throws, never uses a shell. + */ +function gitConfigRead(args) { + const env = { ...process.env }; + for (const key of GIT_LOCATION_VARS) delete env[key]; + env.GIT_CEILING_DIRECTORIES = '/'; + env.GIT_CONFIG_NOSYSTEM = '1'; + + const result = spawnSync('git', args, { encoding: 'utf8', cwd: '/', env }); + if (result.error) { + return { status: null, stdout: '', stderr: String(result.error.message) }; + } + return { + status: result.status, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + }; +} + +function bail(message) { + console.error(`check-git-config-clean: ${message}`); + process.exit(2); +} + +/** + * The gitdir for the tree we are operating on, found without consulting any config. + * + * `GIT_DIR` is honoured when present because git sets it for hooks and it names the + * exact tree being committed to. Note the asymmetry with the leak itself: an + * inherited `GIT_DIR` is dangerous for a WRITE aimed at somewhere else, and + * authoritative for a READ that wants this repository. + */ +function findGitDir() { + if (process.env.GIT_DIR) return resolve(process.env.GIT_DIR); + + let dir = process.cwd(); + for (;;) { + const candidate = join(dir, '.git'); + if (existsSync(candidate)) { + const stat = statSync(candidate); + if (stat.isDirectory()) return candidate; + if (stat.isFile()) { + // Linked worktree (or a submodule): `gitdir: `, possibly relative to + // the directory holding the `.git` file. + const match = /^gitdir:\s*(.+)$/m.exec(readFileSync(candidate, 'utf8')); + if (!match) { + bail(`${candidate} is a file but has no \`gitdir:\` line — cannot locate the repository.`); + } + const pointed = match[1].trim(); + return isAbsolute(pointed) ? pointed : resolve(dir, pointed); + } + bail(`${candidate} is neither a file nor a directory.`); + } + const parent = dirname(dir); + if (parent === dir) { + bail( + 'no `.git` found in this directory or any parent, so there is no shared ' + + 'config to check. This is a git-hook gate — run it from a checkout.', + ); + } + dir = parent; + } +} + +/** Absolute path of the repository-shared config, or exit 2 explaining why not. */ +function sharedConfigPath() { + const gitDir = findGitDir(); + + // A linked worktree's gitdir holds a `commondir` pointer to the SHARED `.git`, + // which is the file at risk — a per-worktree config would not be. + let commonDir = gitDir; + const commonDirFile = join(gitDir, 'commondir'); + if (existsSync(commonDirFile)) { + const pointed = readFileSync(commonDirFile, 'utf8').trim(); + if (pointed) commonDir = isAbsolute(pointed) ? pointed : resolve(gitDir, pointed); + } + + const config = join(commonDir, 'config'); + if (!existsSync(config) || !statSync(config).isFile()) { + bail( + `${config} does not exist or is not a file. Every git repository has one, so ` + + 'this repository is in an unexpected state — check it by hand.', + ); + } + return config; +} + +/** All values of `key` in `configPath` (empty array when unset). */ +function configValues(configPath, key) { + const result = gitConfigRead(['config', '--file', configPath, '--get-all', key]); + // rc 1 is git's "key not present" — the normal, clean case. + if (result.status === 1) return []; + if (result.status !== 0) { + bail( + `cannot read ${key} from ${configPath} ` + + `(${result.stderr.trim() || `git exited ${result.status}`}).`, + ); + } + // Strip only the ONE trailing newline git ends its output with, rather than + // filtering empty lines out: `name =` with no value is a real state (a fixture + // interpolating an unset variable writes it) and prints as an empty line, so a + // blanket filter would drop the very value that has to be reported. rc 0 means + // at least one value was found, so the result is never an empty list here. + const stdout = result.stdout.endsWith('\n') ? result.stdout.slice(0, -1) : result.stdout; + return stdout.split('\n'); +} + +/** True when this identity value could not belong to a real contributor. */ +function isFixtureIdentity(key, value) { + const v = value.trim().toLowerCase(); + if (v === '') return true; + if (key === 'user.name') return FIXTURE_NAMES.has(v); + if (RESERVED_EMAIL_SUFFIXES.some((suffix) => v.endsWith(suffix))) return true; + // No dot in the domain means it is not a resolvable FQDN — `t@t`, `a@b`. + const domain = v.split('@')[1]; + return domain !== undefined && !domain.includes('.'); +} + +const configPath = sharedConfigPath(); +const problems = []; +const rulesChecked = []; + +// --- Rule 1: core.worktree --------------------------------------------------- +rulesChecked.push('core.worktree'); +for (const value of configValues(configPath, 'core.worktree')) { + problems.push({ + what: `core.worktree = ${value}`, + why: + 'pins every linked worktree to one directory: the root reads as dirty, its ' + + 'own untracked files vanish from `git status`, and `git revert` no-ops.', + fix: `git config --file ${configPath} --unset-all core.worktree`, + }); +} + +// --- Rule 2: core.bare on a repo that has a working tree --------------------- +rulesChecked.push('core.bare'); +for (const value of configValues(configPath, 'core.bare')) { + if (value.trim().toLowerCase() !== 'true') continue; + problems.push({ + what: `core.bare = ${value}`, + why: + 'this repository has a working tree, so it is not bare. The same stray ' + + '`git init` that writes core.worktree stamps this.', + fix: `git config --file ${configPath} --unset-all core.bare`, + }); +} + +// --- Rule 3: fixture identities ---------------------------------------------- +for (const key of ['user.name', 'user.email']) { + rulesChecked.push(key); + for (const value of configValues(configPath, key)) { + if (!isFixtureIdentity(key, value)) continue; + problems.push({ + what: `${key} = ${value === '' ? '(empty)' : value}`, + why: + 'not a value a contributor would set — a reserved domain, a domain with no ' + + 'dot, or a literal fixture identity. Commits made under it are ' + + 'unattributable, and it silently replaced whatever was configured before.', + fix: `git config --file ${configPath} --remove-section user`, + }); + } +} + +if (problems.length > 0) { + console.error(`check-git-config-clean: ${configPath} carries the #855 leak signature.\n`); + for (const { what, why, fix } of problems) { + console.error(` ✖ ${what}`); + console.error(` ${why}`); + console.error(` fix: ${fix}\n`); + } + console.error( + 'A test or script shelled out to git with a GIT_DIR inherited from the ' + + 'environment (git exports one to hooks in a linked worktree), which overrides ' + + 'repository discovery and so defeats cwd, --local and the GIT_CONFIG_* pins ' + + 'alike. In agent/tests, build the environment with ' + + 'isolated_git_env() from tests/git_env.py.\n', + ); + console.error( + `Found ${problems.length} problem(s). Repair the config with the command(s) ` + + 'above, then re-run. Do not bypass this hook: the state it is reporting ' + + 'makes `git status` and `git revert` lie to you.', + ); + process.exit(1); +} + +// The counts are the anti-vacuity signal: a check that inspected nothing would +// also exit 0. +console.log( + `check-git-config-clean: OK — ${rulesChecked.length} rule(s) ` + + `(${rulesChecked.join(', ')}) clean in ${configPath}.`, +);