Add focused security regression for sdk-cli allowlisting and document
IOC scan + allowlist probe output under .pr/security-evidence-3171.md.
Signed-off-by: Frank_zhu <58329837+Frank-zhu0404@users.noreply.github.com>
Windows: compare repo identity via normalizeRepoPath/sameRepoIdentity (8.3 short names, case, separators; inode fallback). All nine windows-latest jobs green on 52587005.
The real-PTY test piped answers on fixed sleeps, typing them ahead of
readline. Under CI load the first answer could land before the interface
listened, shifting every later answer onto the wrong question: the
ubuntu-latest Node 18.x npm job installed Claude only, exited 0, and never
printed the Kimi profile prompt while the sibling yarn, pnpm, and bun jobs
on the same Node version passed. Answer each prompt once it appears on
screen instead; spawned stdio goes through cat because the macOS script(1)
refuses a socket stdin.
Root package.json declared no main or exports, so OpenCode npm plugin
resolution (import.meta.resolve) failed and the plugin was silently
skipped (#3127). The .opencode TypeScript sources imported siblings
with .js specifiers that only exist after compilation, so the home
install, which loads the .ts files in place, crashed the tool registry
with ERR_MODULE_NOT_FOUND (#3112).
Declare main/types/exports on the root package pointing at the
compiled plugin entry, switch the sources to .ts specifiers, and
enable allowImportingTsExtensions with rewriteRelativeImportExtensions
so the emitted dist keeps working .js specifiers. Add smoke tests that
build the package, resolve and import the entry by name from a temp
install, and verify every in-place relative import resolves.
Fixes#3127Fixes#3112
hooks/hooks.json already ships only schema-valid keys with metadata in the
hooks.metadata.json sidecar. Add scripts/ci/check-hooks-schema-keys.js, a
strict allowlist check that fails when hooks/hooks.json or
hooks/codex-hooks.json carry any key outside their loader's documented set,
wire it into the npm test chain, and cover it with fixture tests.
Refs #3138, #3114
A first-touch Edit/Write denial marks the file checked so the retry
passes. Sibling edits to the same file in the same parallel batch are
therefore judged against post-denial state and silently apply, leaving
the file in a state neither version intended.
Hooks see tool calls one at a time, so a batch-wide lock is not
possible. Instead make the partial application explicit: the Edit,
Write, MultiEdit, and condensed denials now name the file and warn
that other edits from the same batch may already have been applied,
and SKILL.md tells agents to send dependent edits sequentially and
re-read the file after a gated batch.
Red-teaming the guard from 3c317470 found two more ways to get a repository's own
code executed. Both are demonstrated by a planted binary that appends to a witness
file, counted before and after.
Case folding. git matches index pathspecs case-sensitively even where
core.ignorecase is set, but APFS does not -- so a repository that commits
`.venv/bin/Python` gets `$venv/bin/python` opening and running that file while the
guard's lowercase query finds nothing in the index and reports it untracked. The
witness logged two invocations. It applies to `venv` and `env` as well, and to any
folding of the name. The query now uses a `:(icase)` pathspec; all nine
directory-by-spelling combinations are refused, and an untracked venv still runs.
Module shadowing. `python -c "import pytest"` puts the working directory first on
sys.path, so a repository that commits a `pytest.py` in its root has that file
imported, and executed, by a check whose only job is to answer whether pytest is
installed. The probe is now `python -I -c "import pytest"` on the virtualenv, uv
and poetry paths alike. Isolation does not hide a real pytest -- it lives in the
interpreter's own site-packages, confirmed against a venv holding pytest 9.1.1.
Still true, and not something this hook can fix: running the repository's declared
suite runs the repository's code. `pytest` imports conftest.py, and the Node arm
runs package.json scripts. That is what a pre-push verification hook is for. The
line this guard draws is narrower and worth keeping -- a capability probe, and the
choice of which interpreter to trust, should not be things the pushed repository
gets to decide.
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.
This branch taught the hook to run `.venv/bin/python`, and that is a binary the
repository can supply. On main the Python arm only ever ran `pytest` from PATH --
the developer's own -- and on a machine without one it ran nothing at all, which
is exactly the machine this branch was written for. So the exposure is new, and
it arrived with the fix.
The hook is installed globally through core.hooksPath. Cloning a hostile
repository, committing nothing, and pushing it to your own fork is enough: the
pre-push hook finds the committed `.venv/bin/python`, runs it once to probe for
pytest and again to run the suite. Reproduced -- the planted executable logged
two invocations under the previous commit and none under this one.
A virtualenv is never committed. It is platform-specific binaries and every
Python project gitignores it, so `git ls-files --error-unmatch` separates the
two cases exactly: a developer's own venv is untracked and still resolves, a
tracked one is skipped with the reason printed. An absolute $VIRTUAL_ENV outside
the worktree reads as untracked, as it should.
Not addressed here, and worth a maintainer's view: `uv run` and `poetry run`
resolve from the repository's own lockfile, so they carry the same shape of
trust in a form this check cannot see. They are gated behind a lockfile being
present, and changing their semantics is a larger decision than this fix.
`[[ -n "${ECC_PYTEST_CMD:-}" ]]` asked whether the variable had a value, not
whether it was set, so `ECC_PYTEST_CMD=` fell through to virtualenv discovery
while `ECC_PYTEST_CMD=" "` failed the push. Two spellings of the same mistake,
two behaviours. Falling through is the wrong one: an override that evaluated to
nothing -- a command substitution that found no pytest, say -- then silently ran
a different runner than the operator named, which is exactly the substitution
this resolver refuses to make anywhere else. Both now fail closed.
`${ECC_PYTEST_CMD+set}` rather than `[[ -v ECC_PYTEST_CMD ]]`, because `-v` is
bash 4.2 and a stock macOS /bin/bash is 3.2, where it is not a false but a
syntax error. The hook runs under whatever `env bash` resolves to.
The override is still not probed -- probing runs the operator's command, and a
wrapper that ignores `--version` executes the whole suite and is then rejected
for not printing a version. What the gate can honestly do about a stale override
is refuse to be quiet about it, so a push that uses one now says so, every time,
and says the hook has not checked that it is pytest. A bypass that announces
itself is not the silent gate this resolver exists to prevent.
The fixture env is built from nothing instead of inheriting process.env with two
keys blanked. Blanking is no longer neutral: a blanked ECC_PYTEST_CMD is now an
override, and every one of these tests would have taken that branch.
Three defects, found by reviewing this branch against a running pytest rather
than by reading it.
Exit 5 is not a failure. pytest reserves it for NO_TESTS_COLLECTED, and
`|| fail "pytest failed"` collapsed it into a blocked push. The `|| fail`
predates this branch, but this branch is what makes it reachable: a repository
whose pyproject.toml only configures ruff or black, with pytest in its venv and
no test files, used to hit the "pytest is not installed" skip and now gets
gated. $VIRTUAL_ENV is the first candidate, so merely having a venv activated in
the pushing shell drags any requirements.txt repository into this path, and the
hook is installed globally. Reproduced with pytest 9.1.1. Exit 5 is now
non-blocking but loud -- a bad rootdir, testpaths or an unimportable conftest
also collects nothing, and swallowing that silently would reopen the hole this
resolver exists to close. Other non-zero codes now carry the code, because 1
(tests failed) and 4 (usage error) call for different responses.
The ECC_PYTEST_CMD probe ran the operator's command. Validating the override
with `--version` assumed it would answer like pytest. A wrapper that sets an
environment variable and execs pytest ignores the flag and runs the whole suite,
so the probe executed the tests, then rejected the command for not printing a
version, then blocked the push -- with the suite green. That is worse than the
silent gate the probe was added to close, so the override is taken as given
again: it is a deliberate setting, the hook cannot inspect it without running
it, and pointing it at something that is not pytest is the operator's call.
`is_pytest` still guards the PATH candidate, which this script composes itself,
where `pytest --version` is harmless. An empty override still fails closed.
The tests inherited the ambient environment. `runHermeticPythonPrePush` passed
process.env through, so an exported ECC_PYTEST_CMD or an activated virtualenv
resolved a pytest the fixture never created and the venv test failed for anyone
who runs the suite that way. Both variables are now neutralised in the base env.
Coverage: the gate had no test proving it blocks. Changing the run line to
`|| true` left all three previous tests green. Seven now cover a spaced venv
path, a red suite, exit 5, an override invoked exactly once with no probe, an
empty override, and the PATH candidate in both directions.
AGENTS.md makes immutability mandatory and the helper built `env` by assigning
into it. Rather than reassigning a `let` through spreads, the two stub paths are
now resolved before the object exists, so `env` is a single `const` built in one
expression with the conditional keys spread in. Nothing to mutate and nothing to
rebind.
Two holes in the resolver this branch added, both found in review.
A virtualenv path may contain spaces. `resolve_pytest` returned one string and
the caller expanded it unquoted, so `/home/me/my env/bin/python -m pytest` split
into `/home/me/my` and `env/bin/python`. The probe that accepted the candidate
was correctly quoted, so the hook reported the venv as usable and then failed to
run anything in it -- rejecting the push for a reason with nothing to do with
the code being pushed. It now builds an argv array and runs `"${PYTEST_CMD[@]}"`.
The resolver's contract is that every candidate is confirmed to be pytest, and
two of them were not. `ECC_PYTEST_CMD` was returned unchecked, so
`ECC_PYTEST_CMD=true` made the hook run `true -q`, exit 0 and report a Python
project verified by nothing. The PATH branch used `command -v pytest`, which
proves only that a file of that name exists. Both now go through `is_pytest`,
which runs `--version` and requires the output to name pytest -- `--version`
alone is not evidence, since `true --version` also exits 0.
A bad `ECC_PYTEST_CMD` fails the push rather than falling through to the next
candidate. An operator who set it asked for that command, and silently running
a different one hides the misconfiguration -- which is the same silent-gate
failure this branch exists to remove, one level along.
Three regression tests cover the three paths: a venv whose directory name
contains a space, an override that is not pytest, and an override that is.
Expand proven-passive heredoc recognition beyond bare `cat` so documentation
writes via `tee`, `/bin/cat`, and `command cat` no longer trip the destructive
command detector on body text, while still failing closed for shells and pipes.
The /ecc-doctor false-negative for users who installed @tintinweb/pi-subagents
(a working, separately-published subagents implementation) because matching
was a bare exact string match against COMPANION_PACKAGES.
Matching is now asymmetric:
- exact match first (stable reported name when both present)
- unscoped entry also satisfied by @scope/<bare-name> (via @ + /suffix check,
not plain endsWith, so my-pi-subagents does not qualify)
- scoped entry is exact-only (no silent substitution by another publisher's
rpiv-todo for @juicesharp/rpiv-todo)
- non-exact match emits "satisfied by: <actual>" so user sees the real impl
Verification (already performed, not re-run here):
- node tests/pi/pi-extension-adapter.test.js: 31/31 pass
- Mutation test: the three reverts (old .has loop; delete scoped guard;
plain endsWith) each fail the new test
- Real settings.json with npm:@tintinweb/pi-subagents produces correct
"installed pi-subagents" / "satisfied by: @tintinweb/pi-subagents"
- Full npm test: 4546/4636 pass, failures byte-identical to clean main
Refs: pi-subagents capability, ecc-doctor report
Count only positional reads from the cost-log descriptor and use actual bytes returned, so the bounded-read assertions remain deterministic under full-suite concurrency.
Skip POSIX permission-bit equality on Windows, where stat reports synthesized mode bits, while retaining the atomic publication and readback assertions on every platform.
