fix(hooks): stop probing a /bin/sh launcher wrapper as if it were python (#3027) - #3109
fix(hooks): stop probing a /bin/sh launcher wrapper as if it were python (#3027)#3109abhay-codes07 wants to merge 2 commits into
Conversation
…hon (Graphify-Labs#3027) pipx (via distlib's exec trick, used when the install path has spaces — macOS's "Application Support/pipx") generates a `graphify` launcher whose shebang is `#!/bin/sh` and whose second line execs the real interpreter. The hook parsed that shebang into GRAPHIFY_PYTHON and ran `/bin/sh -c "import ..."`: `import` is not a shell builtin, so it resolved on PATH to ImageMagick's screenshot tool and dumped its usage text on every commit (only stderr was silenced). The probe did fail and fall back to python3, so the rebuild ran — the bug was the noise. The shebang branch now takes the interpreter from the wrapper's exec line (exec' "/path/to/python" "$0" "$@") when the shebang names something that is not a python, and never probes an interpreter whose name is not python*/pypy*. Because such a path contains a space by construction, the injection allowlist folds spaces before matching — a space is not a metacharacter, $GRAPHIFY_PYTHON is always expanded quoted, and the allowlist pattern itself is unchanged.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Fixes the commit hook probing a /bin/sh launcher wrapper as if it were the Python interpreter (#3027): when pipx emits a shebang-wrapper (install paths with spaces, e.g. macOS's "Application Support/pipx"), the hook now reads the interpreter from the wrapper's exec line instead of running sh -c "import graphify", which resolved import on PATH to ImageMagick's screenshot tool and dumped usage text on every commit. Restricts probing to interpreters whose basename matches python*/pypy*, blanking anything else, and folds spaces before the path-safety allowlist check so quoted paths with spaces survive. Adds test_hook_sh_wrapper_launcher.py exercising the wrapper-exec, no-exec-line, non-importable, and plain-shebang paths against a stubbed PATH containing a fake import.
Worth a look
- Shebang detection now rejects valid Python interpreters not named python/pypy —
graphify/hooks.py:96· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 248 functions depend on the 50 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 122 callees - new:
install()— 33 callers, 7 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 248 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 98 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
python3.12, python@3.12, cpython and pypy3 all pass the probe gate; sh, bash and node still do not.
|
Follow-up commit on the review's point about interpreter names: the probe gate is now |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Fixes the git hook launcher probing a /bin/sh pipx wrapper as if it were Python, which resolved import graphify to ImageMagick's import tool and dumped usage text on every commit (#3027). The shebang detection in _PYTHON_DETECT now parses the wrapper's exec line for the real interpreter, refuses to probe anything whose basename doesn't look Python-like (*python*/*pypy*), and folds spaces before the path-allowlist check so space-containing venv paths survive. Adds test_hook_sh_wrapper_launcher.py covering the exec-line adoption, the missing-exec-line and non-importing-interpreter fall-throughs, and the plain-shebang path.
Worth a look
- Space-folding allowlist permits spaces in interpreter path, weakening injection guard —
graphify/hooks.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 248 functions depend on the 50 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 122 callees - new:
install()— 33 callers, 7 callees - new:
dispatch_install_cli()— 2 callers, 31 callees - new:
status()— 8 callers, 6 callees - new:
uninstall()— 9 callers, 5 callees - new:
uninstall_all()— 2 callers, 13 callees - new:
test_poisoned_manifest_is_healed()— 0 callers, 6 callees
Verification — 248 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 98 function(s) in the blast radius were not formally verified this run
· 7 more finding(s) on lines outside this diff (see the check run).
Closes #3027.
The problem
pipx generates the
graphifylauncher via distlib's "exec trick" when the install path contains a space — macOS's~/Library/Application Support/pipxis the common case. That launcher's shebang is#!/bin/sh, and its second line execs the real interpreter:The hook parsed the shebang into
GRAPHIFY_PYTHON=/bin/shand ran/bin/sh -c "import importlib.util, sys; ...".importis not a shell builtin, so it resolved onPATHto ImageMagick'simportscreenshot tool, which dumped its full usage text on every commit — only stderr was silenced. The probe did fail and the chain fell back topython3, so the graph still rebuilt; the bug was the noise, and the wasted opportunity to find the right interpreter.The change
In the shebang branch of
_PYTHON_DETECT:sedon the second line of the 256-byte head the script already reads);python*/pypy*— so a shell is never handed-c "import ..."again, whatever the wrapper looks like.The path recovered from a wrapper contains a space by construction, and the injection allowlist rejected spaces. A space is not a shell metacharacter here —
$GRAPHIFY_PYTHONis always expanded quoted — so the allowlist now matches against the path with spaces folded to_, leaving the proven pattern itself byte-for-byte unchanged (the#2126/#2641allowlist tests, which extract that exact pattern from the emitted text, still pass).Tests
tests/test_hook_sh_wrapper_launcher.py— 5 tests that run the real emitted_PYTHON_DETECTundershon a machine whosePATHholds a/bin/shwrapper, an ImageMagick-styleimportthat prints usage, and ambient pythons that cannot import graphify: the wrapper's interpreter (in a directory with a space) is adopted andimportis never run; a wrapper with no exec line falls through quietly; a wrapper whose interpreter fails the probe is not adopted; a plain python shebang still resolves; and the emitted script gates the probe on the interpreter's name. With the fix reverted, 3 of 5 fail.tests/test_hooks.pyis unchanged; the full suite matches thev8baseline.