mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-28 09:17:02 +00:00
fix: pin interpreters whose path contains a space (#2166)
`graphify hook install` emitted `_PINNED=''` for some Windows uv-tool installs, so every interpreter probe failed, each commit printed "could not locate a Python with graphify installed" and the graph never rebuilt. Root cause is the install-time allowlist in `_pinned_python()`, not the uv layout: it accepted `[a-zA-Z0-9/_.@:\-]` but not a plain space, so any `sys.executable` under a profile whose name contains one -- `C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe`, or the equally common `C:\Program Files\Python312\python.exe` -- was rejected wholesale and nothing was recorded. A space-free Windows uv path pins correctly, which is why this looks layout-specific. A space is safe to allow because every consumer already quotes the value: the hook scripts embed it as `_PINNED='<path>'` and dereference `"$_PINNED"`, so a space can neither split a word nor start a command. Adding it to the allowlist therefore fixes the pin without weakening the injection guard -- `$`, backtick, `;`, `'` and `"` are all still rejected. `_register_merge_driver` did interpolate the path unquoted into the `merge.graphify.driver` command, which git runs through a shell; that would split a spaced path into two words, so it is now double-quoted. Double quotes are safe here precisely because the allowlist keeps `$` and backticks out. Tests: spaced Windows/POSIX paths are pinned; metacharacter paths are still rejected (including `'` and `"`); the merge driver quotes a spaced interpreter; and the installed post-commit/post-checkout hooks carry the real path rather than `_PINNED=''`.
This commit is contained in:
committed by
safishamsi
parent
44241dd10c
commit
cfe15f9161
+16
-4
@@ -503,10 +503,17 @@ def _pinned_python() -> str:
|
||||
that is not a valid plain filesystem path character, preventing $(...),
|
||||
backtick, double-quote, semicolon, etc. from being injected into generated
|
||||
shell scripts or the merge-driver command line. The allowlist includes ':'
|
||||
and '\\' so Windows paths (C:\\...) are accepted. An empty return means
|
||||
callers must fall back to the `graphify` launcher on PATH — safe degradation.
|
||||
and '\\' so Windows paths (C:\\...) are accepted, and a plain space so
|
||||
Windows profile paths (C:\\Users\\First Last\\...) are too — a space cannot
|
||||
start a substitution or a new command, and every consumer quotes the value:
|
||||
the hook scripts embed it as '$_PINNED' (single-quoted, then referenced as
|
||||
"$_PINNED") and _register_merge_driver double-quotes it (#2166). Before that
|
||||
a space rejected the whole path, so hooks installed under any Windows user
|
||||
whose profile name contains a space silently pinned nothing. An empty return
|
||||
means callers must fall back to the `graphify` launcher on PATH — safe
|
||||
degradation.
|
||||
"""
|
||||
if re.search(r"[^a-zA-Z0-9/_.@:\\-]", sys.executable):
|
||||
if re.search(r"[^a-zA-Z0-9/_.@: \\-]", sys.executable):
|
||||
return ""
|
||||
return sys.executable
|
||||
|
||||
@@ -551,7 +558,12 @@ def _register_merge_driver(root: Path) -> str:
|
||||
import subprocess as _sp
|
||||
pinned = _pinned_python()
|
||||
if pinned:
|
||||
driver = f"{pinned} -m graphify merge-driver %O %A %B"
|
||||
# Double-quoted: the allowlist in _pinned_python() permits a space (Windows
|
||||
# profile paths), and git runs this driver string through a shell, so an
|
||||
# unquoted "C:\\Users\\First Last\\...\\python.exe" would split into two
|
||||
# words and the driver would never run (#2166). The same allowlist keeps
|
||||
# '$' and backticks out, so double quotes cannot introduce expansion.
|
||||
driver = f'"{pinned}" -m graphify merge-driver %O %A %B'
|
||||
else:
|
||||
driver = "graphify merge-driver %O %A %B"
|
||||
try:
|
||||
|
||||
@@ -676,3 +676,82 @@ def test_uninstall_removes_merge_driver_keeps_other_attrs(tmp_path):
|
||||
content = (repo / ".gitattributes").read_text(encoding="utf-8")
|
||||
assert "*.png binary" in content
|
||||
assert "merge=graphify" not in content
|
||||
|
||||
|
||||
@pytest.mark.parametrize("exe", [
|
||||
r"C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe",
|
||||
r"C:\Program Files\Python312\python.exe",
|
||||
"/home/first last/.local/share/uv/tools/graphifyy/bin/python",
|
||||
])
|
||||
def test_pinned_python_accepts_paths_containing_spaces(exe, monkeypatch):
|
||||
"""#2166: a space must not empty the pin.
|
||||
|
||||
The install-time allowlist had no space, so `sys.executable` under any Windows
|
||||
profile whose name contains one (`C:\\Users\\First Last\\...`, or the very common
|
||||
`C:\\Program Files\\...`) was rejected wholesale and the hook shipped `_PINNED=''`.
|
||||
Every interpreter probe then failed and each commit no-op'd with the "could not
|
||||
locate a Python" warning, so the graph never rebuilt.
|
||||
"""
|
||||
import sys as _sys
|
||||
|
||||
from graphify.hooks import _pinned_python
|
||||
|
||||
monkeypatch.setattr(_sys, "executable", exe)
|
||||
assert _pinned_python() == exe, "a path containing a space must still be pinned"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("exe", [
|
||||
r"C:\Users\evil\python.exe; rm -rf /",
|
||||
"/tmp/py`id`",
|
||||
"/tmp/py$(id)",
|
||||
"/tmp/py$IFS",
|
||||
r"C:\Users\ev'il\python.exe",
|
||||
'/tmp/py"quote',
|
||||
])
|
||||
def test_pinned_python_still_rejects_shell_metacharacters(exe, monkeypatch):
|
||||
"""Widening the allowlist for spaces (#2166) must not admit anything that can
|
||||
start a substitution, end the single-quoted assignment, or chain a command."""
|
||||
import sys as _sys
|
||||
|
||||
from graphify.hooks import _pinned_python
|
||||
|
||||
monkeypatch.setattr(_sys, "executable", exe)
|
||||
assert _pinned_python() == "", f"dangerous interpreter path accepted: {exe!r}"
|
||||
|
||||
|
||||
def test_merge_driver_quotes_interpreter_with_spaces(tmp_path, monkeypatch):
|
||||
"""#2166: git runs the merge driver through a shell, so a pinned path with a
|
||||
space has to be quoted or the driver splits into two words and never runs."""
|
||||
import subprocess
|
||||
import sys as _sys
|
||||
|
||||
from graphify.hooks import install
|
||||
|
||||
exe = r"C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe"
|
||||
repo = _make_git_repo(tmp_path)
|
||||
monkeypatch.setattr(_sys, "executable", exe)
|
||||
install(repo)
|
||||
|
||||
driver = subprocess.run(
|
||||
["git", "-C", str(repo), "config", "--get", "merge.graphify.driver"],
|
||||
capture_output=True, text=True, check=True,
|
||||
).stdout.strip()
|
||||
assert driver.startswith(f'"{exe}"'), f"interpreter not quoted in merge driver: {driver!r}"
|
||||
assert driver.endswith("-m graphify merge-driver %O %A %B")
|
||||
|
||||
|
||||
def test_install_pins_interpreter_path_with_spaces(tmp_path, monkeypatch):
|
||||
"""#2166 end to end: the emitted hooks must carry the real interpreter, not ''."""
|
||||
import sys as _sys
|
||||
|
||||
from graphify.hooks import install
|
||||
|
||||
exe = r"C:\Users\First Last\AppData\Roaming\uv\tools\graphifyy\Scripts\python.exe"
|
||||
repo = _make_git_repo(tmp_path)
|
||||
monkeypatch.setattr(_sys, "executable", exe)
|
||||
install(repo)
|
||||
|
||||
for name in ("post-commit", "post-checkout"):
|
||||
script = (repo / ".git" / "hooks" / name).read_text()
|
||||
assert f"_PINNED='{exe}'" in script, f"{name} did not pin the spaced interpreter"
|
||||
assert "_PINNED=''" not in script, f"{name} pinned an empty interpreter (#2166)"
|
||||
|
||||
Reference in New Issue
Block a user