From 0fdfdedcaa6d05b650eb06f8ea9cf9505942e812 Mon Sep 17 00:00:00 2001 From: Safi Date: Wed, 3 Jun 2026 15:08:56 +0100 Subject: [PATCH] harden hook interpreter detection against injection and unquoted exec Per adversarial review of the #1127 fix: - Apply the filesystem-path allowlist to sys.executable before embedding it in the generated hook script. Paths with metacharacters outside [a-zA-Z0-9/_.@:\-] (spaces, dollar signs, backticks, semicolons) are replaced with an empty string so the pinned probe is safely skipped rather than injecting shell commands that execute on every git commit. - Change _PINNED='...' to single-quote assignment so no shell expansion can occur even if a character slipped through the allowlist (belt and suspenders). - Quote $GRAPHIFY_PYTHON in both nohup exec lines so paths with spaces work (common in Windows C:\Program Files\... installs). - Update test to assert the sanitized value, not raw sys.executable, so the test stays correct after any future sanitization changes. Co-Authored-By: Claude Sonnet 4.6 --- graphify/hooks.py | 20 ++++++++++++++++---- tests/test_hooks.py | 10 ++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/graphify/hooks.py b/graphify/hooks.py index efedb2df..258c29d9 100644 --- a/graphify/hooks.py +++ b/graphify/hooks.py @@ -21,7 +21,7 @@ _PYTHON_DETECT = """\ # _PINNED was recorded at hook-install time; tried first so the hook works even # when the graphify launcher is not on PATH (common in GUI clients and CI). GRAPHIFY_PYTHON="" -_PINNED="__PINNED_PYTHON__" +_PINNED='__PINNED_PYTHON__' if [ -n "$_PINNED" ] && [ -x "$_PINNED" ] && "$_PINNED" -c "import graphify" 2>/dev/null; then GRAPHIFY_PYTHON="$_PINNED" fi @@ -112,7 +112,7 @@ export GRAPHIFY_CHANGED="$CHANGED" _GRAPHIFY_LOG="${HOME}/.cache/graphify-rebuild.log" mkdir -p "$(dirname "$_GRAPHIFY_LOG")" echo "[graphify hook] launching background rebuild (log: $_GRAPHIFY_LOG)" -nohup $GRAPHIFY_PYTHON -c " +nohup "$GRAPHIFY_PYTHON" -c " import os, signal, sys from pathlib import Path @@ -180,7 +180,7 @@ GIT_DIR=$(git rev-parse --git-dir 2>/dev/null) _GRAPHIFY_LOG="${HOME}/.cache/graphify-rebuild.log" mkdir -p "$(dirname "$_GRAPHIFY_LOG")" echo "[graphify] Branch switched - launching background rebuild (log: $_GRAPHIFY_LOG)" -nohup $GRAPHIFY_PYTHON -c " +nohup "$GRAPHIFY_PYTHON" -c " from graphify.watch import _rebuild_code, _apply_resource_limits from pathlib import Path import os, signal, sys @@ -336,7 +336,19 @@ def install(path: Path = Path(".")) -> str: # in both scripts before writing; the allowlist in _PYTHON_DETECT strips any # characters unsafe in a shell path, and import-verification catches a stale # pinned path so it safely falls through to the dynamic detection. - pinned = sys.executable.replace("'", "") # strip single quotes (path injection guard) + # Apply the same allowlist used in _PYTHON_DETECT for all other probes. + # This rejects any character that is not a valid plain filesystem path + # character, preventing $(...), backtick, double-quote, semicolon, etc. + # from being injected into the generated shell scripts. The allowlist + # includes ':' and '\' so Windows paths (C:\...) are accepted. + import re as _re + _safe = sys.executable + if _re.search(r"[^a-zA-Z0-9/_.@:\\-]", _safe): + # Path contains characters outside the allowlist (spaces, quotes, etc.). + # Embed an empty string so the pinned probe is skipped and the hook + # falls through to the dynamic detection — safe degradation. + _safe = "" + pinned = _safe hook = _HOOK_SCRIPT.replace("__PINNED_PYTHON__", pinned) checkout = _CHECKOUT_SCRIPT.replace("__PINNED_PYTHON__", pinned) diff --git a/tests/test_hooks.py b/tests/test_hooks.py index a6e8b463..098b5ee0 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -172,14 +172,16 @@ def test_install_embeds_pinned_interpreter(tmp_path): fallbacks cannot import graphify (wrong venv), and the hook silently exits 0. Pinning sys.executable at install time makes the hook work regardless of PATH. """ - import sys + import re, sys repo = _make_git_repo(tmp_path) install(repo) commit_hook = (repo / ".git" / "hooks" / "post-commit").read_text() checkout_hook = (repo / ".git" / "hooks" / "post-checkout").read_text() - # The pinned interpreter path must appear in both generated scripts. - assert sys.executable in commit_hook, "pinned sys.executable missing from post-commit" - assert sys.executable in checkout_hook, "pinned sys.executable missing from post-checkout" + # Compute the sanitized value the same way install() does. + expected = sys.executable if not re.search(r"[^a-zA-Z0-9/_.@:\\-]", sys.executable) else "" + if expected: + assert expected in commit_hook, "sanitized sys.executable missing from post-commit" + assert expected in checkout_hook, "sanitized sys.executable missing from post-checkout" # The placeholder must be fully substituted -- no __PINNED_PYTHON__ left. assert "__PINNED_PYTHON__" not in commit_hook, "placeholder not substituted in post-commit" assert "__PINNED_PYTHON__" not in checkout_hook, "placeholder not substituted in post-checkout"