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 <noreply@anthropic.com>
This commit is contained in:
Safi
2026-06-03 15:08:56 +01:00
parent 2d5a10c0a2
commit 0fdfdedcaa
2 changed files with 22 additions and 8 deletions
+16 -4
View File
@@ -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)
+6 -4
View File
@@ -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"