From 3c317470163b62345935125aecdeb037cba88dd8 Mon Sep 17 00:00:00 2001 From: Juan Garibay Date: Thu, 17 Sep 2026 16:55:11 -0400 Subject: [PATCH] fix(hooks): resolve the venv path before asking git whether it is tracked The guard added in 9cdc40e6 was incomplete. `git ls-files` reports paths as they are indexed and does not follow symlinks, so a repository that commits `.venv` as a symlink to its own root alongside a tracked `bin/python` gets asked about `.venv/bin/python` -- a path git has never heard of -- and the answer is "untracked". The interpreter then runs. Measured on that shape: the planted executable logged two invocations against 9cdc40e6 and none against this commit. `repo_ships_interpreter` now resolves the bin directory with `cd -P`/`pwd -P`, resolves the worktree root the same way, and asks git about the resolved path relative to it. The three cases that matter all hold: a plainly committed venv is still refused, the symlink shape is now refused, and a developer's own untracked venv still resolves and runs. `cd -P`/`pwd -P` rather than `realpath` or `readlink -f`, because neither is portable to a stock macOS. --- scripts/codex-git-hooks/pre-push | 37 ++++++++++++++++++++++--------- tests/scripts/codex-hooks.test.js | 24 +++++++++++++++++++- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/scripts/codex-git-hooks/pre-push b/scripts/codex-git-hooks/pre-push index 6f04c2680..dfeeab4fd 100755 --- a/scripts/codex-git-hooks/pre-push +++ b/scripts/codex-git-hooks/pre-push @@ -147,6 +147,31 @@ is_pytest() { grep -qiE 'pytest[[:space:]]+(version[[:space:]]+)?[0-9]' <<<"$version" } +# Does the repository itself ship this interpreter? +# +# A virtualenv is never committed -- it is platform-specific binaries, and every +# Python project gitignores it. One that IS tracked is the repository handing this +# hook an executable and asking it to run. The hook is installed globally, so +# cloning a hostile repository and pushing it to your own fork would be enough, +# and on a machine with no pytest on PATH this arm is the only thing that would +# run at all. A developer's own venv is untracked, so nothing legitimate is lost. +# +# The path is resolved through symlinks before git is asked, because `git ls-files` +# reports paths as indexed and does not follow links. A repository that commits +# `.venv` as a symlink to `.` next to a tracked `bin/python` would otherwise be +# queried for `.venv/bin/python`, a path git has never heard of, and the answer +# would be "untracked". Measured: that shape ran the planted binary twice. +repo_ships_interpreter() { + local bindir real top + bindir="$(cd -P -- "$1" 2>/dev/null && pwd -P)" || return 1 + [[ -n "$bindir" ]] || return 1 + real="$bindir/python" + top="$(git rev-parse --show-toplevel 2>/dev/null)" || return 1 + top="$(cd -P -- "$top" 2>/dev/null && pwd -P)" || return 1 + [[ -n "$top" && "$real" == "$top/"* ]] || return 1 + git ls-files --error-unmatch -- "${real#"$top"/}" >/dev/null 2>&1 +} + resolve_pytest() { # `${VAR+set}` rather than `-n "${VAR:-}"`, so that a variable set to nothing is # still an override: `ECC_PYTEST_CMD=` and `ECC_PYTEST_CMD=" "` now behave @@ -175,16 +200,8 @@ resolve_pytest() { local venv for venv in "${VIRTUAL_ENV:-}" .venv venv env; do if [[ -n "$venv" && -x "$venv/bin/python" ]]; then - # A virtualenv is never committed -- it is platform-specific binaries, and - # every Python project gitignores it. One that IS tracked is the repository - # handing this hook an executable and asking it to run. The hook is installed - # globally, so cloning a hostile repository and pushing it to your own fork - # would be enough, and on a machine with no pytest on PATH this arm is the - # only thing that would run at all. Skipping costs nothing legitimate, - # because a developer's own venv is untracked -- and it says why rather than - # going quiet about it. - if git ls-files --error-unmatch -- "$venv/bin/python" >/dev/null 2>&1; then - log "Ignoring $venv/bin/python: it is tracked in this repository." + if repo_ships_interpreter "$venv/bin"; then + log "Ignoring $venv/bin/python: the repository ships it." log " A committed virtualenv is an executable the repository controls, and" log " this hook runs on every push in every repository." continue diff --git a/tests/scripts/codex-hooks.test.js b/tests/scripts/codex-hooks.test.js index 42bdbda68..11f8d0115 100644 --- a/tests/scripts/codex-hooks.test.js +++ b/tests/scripts/codex-hooks.test.js @@ -319,6 +319,7 @@ function runHermeticPythonPrePush({ venvName = null, venvExit = 0, trackVenv = false, + trackedSymlinkVenv = false, pytestCmd = null, overrideStub = false, pathPytestVersionLine = null, @@ -350,6 +351,16 @@ function runHermeticPythonPrePush({ } } + // The shape that defeats a naive `git ls-files -- .venv/bin/python` check: the + // repository commits `.venv` as a symlink to its own root plus a tracked + // `bin/python`, so git is asked about a path it has never indexed. + if (trackedSymlinkVenv) { + writeExecutable(path.join(projectDir, 'bin', 'python'), `#!/bin/sh\n${record}\nexit 0\n`); + fs.symlinkSync('.', path.join(projectDir, '.venv')); + const added = spawnSync('git', ['add', '-f', '--', 'bin/python', '.venv'], { cwd: projectDir }); + assert.strictEqual(added.status, 0, added.stderr?.toString()); + } + // Deliberately does NOT special-case --version: an operator's wrapper would not // either, and the recorded calls are what prove the hook never probed it. const overrideStubPath = overrideStub ? path.join(tempDir, 'bin', 'wrapper') : null; @@ -416,7 +427,18 @@ if ( const { result, calls } = runHermeticPythonPrePush({ venvName: '.venv', trackVenv: true }); assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); assert.deepStrictEqual(calls, [], JSON.stringify(calls)); - assert.match(result.stdout, /it is tracked in this repository/); + assert.match(result.stdout, /the repository ships it/); + }) +) + passed++; +else failed++; + +if ( + test('pre-push refuses a tracked interpreter reached through a committed symlink', () => { + const { result, calls } = runHermeticPythonPrePush({ trackedSymlinkVenv: true }); + assert.strictEqual(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.deepStrictEqual(calls, [], JSON.stringify(calls)); + assert.match(result.stdout, /the repository ships it/); }) ) passed++;