mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
fix(checkpoints): isolate shadow git repo from user's global config (#11261)
Users with 'commit.gpgsign = true' in their global git config got a pinentry popup (or a failed commit) every time the agent took a background filesystem snapshot — every write_file, patch, or diff mid-session. With GPG_TTY unset, pinentry-qt/gtk would spawn a GUI window, constantly interrupting the session. The shadow repo is internal Hermes infrastructure. It must not inherit user-level git settings (signing, hooks, aliases, credential helpers, etc.) under any circumstance. Fix is layered: 1. _git_env() sets GIT_CONFIG_GLOBAL=os.devnull, GIT_CONFIG_SYSTEM=os.devnull, and GIT_CONFIG_NOSYSTEM=1. Shadow git commands no longer see ~/.gitconfig or /etc/gitconfig at all (uses os.devnull for Windows compat). 2. _init_shadow_repo() explicitly writes commit.gpgsign=false and tag.gpgSign=false into the shadow's own config, so the repo is correct even if inspected or run against directly without the env vars, and for older git versions (<2.32) that predate GIT_CONFIG_GLOBAL. 3. _take() passes --no-gpg-sign inline on the commit call. This covers existing shadow repos created before this fix — they will never re-run _init_shadow_repo (it is gated on HEAD not existing), so they would miss layer 2. Layer 1 still protects them, but the inline flag guarantees correctness at the commit call itself. Existing checkpoints, rollback, list, diff, and restore all continue to work — history is untouched. Users who had the bug stop getting pinentry popups; users who didn't see no observable change. Tests: 5 new regression tests in TestGpgAndGlobalConfigIsolation, including a full E2E repro with fake HOME, global gpgsign=true, and a deliberately broken GPG binary — checkpoint succeeds regardless.
This commit is contained in:
parent
d38b73fa57
commit
edefec4e68
2 changed files with 143 additions and 3 deletions
|
|
@ -587,3 +587,112 @@ class TestSecurity:
|
||||||
|
|
||||||
result = mgr.restore(str(work_dir), target_hash, file_path="subdir/test.txt")
|
result = mgr.restore(str(work_dir), target_hash, file_path="subdir/test.txt")
|
||||||
assert result["success"] is True
|
assert result["success"] is True
|
||||||
|
|
||||||
|
|
||||||
|
# =========================================================================
|
||||||
|
# GPG / global git config isolation
|
||||||
|
# =========================================================================
|
||||||
|
# Regression tests for the bug where users with ``commit.gpgsign = true``
|
||||||
|
# in their global git config got a pinentry popup (or a failed commit)
|
||||||
|
# every time the agent took a background snapshot.
|
||||||
|
|
||||||
|
import os as _os
|
||||||
|
|
||||||
|
|
||||||
|
class TestGpgAndGlobalConfigIsolation:
|
||||||
|
def test_git_env_isolates_global_and_system_config(self, tmp_path):
|
||||||
|
"""_git_env must null out GIT_CONFIG_GLOBAL / GIT_CONFIG_SYSTEM so the
|
||||||
|
shadow repo does not inherit user-level gpgsign, hooks, aliases, etc."""
|
||||||
|
env = _git_env(tmp_path / "shadow", str(tmp_path))
|
||||||
|
assert env["GIT_CONFIG_GLOBAL"] == _os.devnull
|
||||||
|
assert env["GIT_CONFIG_SYSTEM"] == _os.devnull
|
||||||
|
assert env["GIT_CONFIG_NOSYSTEM"] == "1"
|
||||||
|
|
||||||
|
def test_init_sets_commit_gpgsign_false(self, work_dir, checkpoint_base, monkeypatch):
|
||||||
|
monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
|
||||||
|
shadow = _shadow_repo_path(str(work_dir))
|
||||||
|
_init_shadow_repo(shadow, str(work_dir))
|
||||||
|
# Inspect the shadow's own config directly — the settings must be
|
||||||
|
# written into the repo, not just inherited via env vars.
|
||||||
|
result = subprocess.run(
|
||||||
|
["git", "config", "--file", str(shadow / "config"), "--get", "commit.gpgsign"],
|
||||||
|
capture_output=True, text=True,
|
||||||
|
)
|
||||||
|
assert result.stdout.strip() == "false"
|
||||||
|
|
||||||
|
def test_init_sets_tag_gpgsign_false(self, work_dir, checkpoint_base, monkeypatch):
|
||||||
|
monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
|
||||||
|
shadow = _shadow_repo_path(str(work_dir))
|
||||||
|
_init_shadow_repo(shadow, str(work_dir))
|
||||||
|
result = subprocess.run(
|
||||||
|
["git", "config", "--file", str(shadow / "config"), "--get", "tag.gpgSign"],
|
||||||
|
capture_output=True, text=True,
|
||||||
|
)
|
||||||
|
assert result.stdout.strip() == "false"
|
||||||
|
|
||||||
|
def test_checkpoint_works_with_global_gpgsign_and_broken_gpg(
|
||||||
|
self, work_dir, checkpoint_base, monkeypatch, tmp_path
|
||||||
|
):
|
||||||
|
"""The real bug scenario: user has global commit.gpgsign=true but GPG
|
||||||
|
is broken or pinentry is unavailable. Before the fix, every snapshot
|
||||||
|
either failed or spawned a pinentry window. After the fix, snapshots
|
||||||
|
succeed without ever invoking GPG."""
|
||||||
|
monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
|
||||||
|
|
||||||
|
# Fake HOME with global gpgsign=true and a deliberately broken GPG
|
||||||
|
# binary. If isolation fails, the commit will try to exec this
|
||||||
|
# nonexistent path and the checkpoint will fail.
|
||||||
|
fake_home = tmp_path / "fake_home"
|
||||||
|
fake_home.mkdir()
|
||||||
|
(fake_home / ".gitconfig").write_text(
|
||||||
|
"[user]\n email = real@user.com\n name = Real User\n"
|
||||||
|
"[commit]\n gpgsign = true\n"
|
||||||
|
"[tag]\n gpgSign = true\n"
|
||||||
|
"[gpg]\n program = /nonexistent/fake-gpg-binary\n"
|
||||||
|
)
|
||||||
|
monkeypatch.setenv("HOME", str(fake_home))
|
||||||
|
monkeypatch.delenv("GPG_TTY", raising=False)
|
||||||
|
monkeypatch.delenv("DISPLAY", raising=False) # block GUI pinentry
|
||||||
|
|
||||||
|
mgr = CheckpointManager(enabled=True)
|
||||||
|
assert mgr.ensure_checkpoint(str(work_dir), reason="with-global-gpgsign") is True
|
||||||
|
assert len(mgr.list_checkpoints(str(work_dir))) == 1
|
||||||
|
|
||||||
|
def test_checkpoint_works_on_prefix_shadow_without_local_gpgsign(
|
||||||
|
self, work_dir, checkpoint_base, monkeypatch, tmp_path
|
||||||
|
):
|
||||||
|
"""Users with shadow repos created before the fix will not have
|
||||||
|
commit.gpgsign=false in their shadow's own config. The inline
|
||||||
|
``--no-gpg-sign`` flag on the commit call must cover them."""
|
||||||
|
monkeypatch.setattr("tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base)
|
||||||
|
|
||||||
|
# Simulate a pre-fix shadow repo: init without commit.gpgsign=false
|
||||||
|
# in its own config. _init_shadow_repo now writes it, so we must
|
||||||
|
# manually remove it to mimic the pre-fix state.
|
||||||
|
shadow = _shadow_repo_path(str(work_dir))
|
||||||
|
_init_shadow_repo(shadow, str(work_dir))
|
||||||
|
subprocess.run(
|
||||||
|
["git", "config", "--file", str(shadow / "config"),
|
||||||
|
"--unset", "commit.gpgsign"],
|
||||||
|
capture_output=True, text=True, check=False,
|
||||||
|
)
|
||||||
|
subprocess.run(
|
||||||
|
["git", "config", "--file", str(shadow / "config"),
|
||||||
|
"--unset", "tag.gpgSign"],
|
||||||
|
capture_output=True, text=True, check=False,
|
||||||
|
)
|
||||||
|
|
||||||
|
# And simulate hostile global config
|
||||||
|
fake_home = tmp_path / "fake_home"
|
||||||
|
fake_home.mkdir()
|
||||||
|
(fake_home / ".gitconfig").write_text(
|
||||||
|
"[commit]\n gpgsign = true\n"
|
||||||
|
"[gpg]\n program = /nonexistent/fake-gpg-binary\n"
|
||||||
|
)
|
||||||
|
monkeypatch.setenv("HOME", str(fake_home))
|
||||||
|
monkeypatch.delenv("GPG_TTY", raising=False)
|
||||||
|
monkeypatch.delenv("DISPLAY", raising=False)
|
||||||
|
|
||||||
|
mgr = CheckpointManager(enabled=True)
|
||||||
|
assert mgr.ensure_checkpoint(str(work_dir), reason="prefix-shadow") is True
|
||||||
|
assert len(mgr.list_checkpoints(str(work_dir))) == 1
|
||||||
|
|
|
||||||
|
|
@ -126,7 +126,22 @@ def _shadow_repo_path(working_dir: str) -> Path:
|
||||||
|
|
||||||
|
|
||||||
def _git_env(shadow_repo: Path, working_dir: str) -> dict:
|
def _git_env(shadow_repo: Path, working_dir: str) -> dict:
|
||||||
"""Build env dict that redirects git to the shadow repo."""
|
"""Build env dict that redirects git to the shadow repo.
|
||||||
|
|
||||||
|
The shadow repo is internal Hermes infrastructure — it must NOT inherit
|
||||||
|
the user's global or system git config. User-level settings like
|
||||||
|
``commit.gpgsign = true``, signing hooks, or credential helpers would
|
||||||
|
either break background snapshots or, worse, spawn interactive prompts
|
||||||
|
(pinentry GUI windows) mid-session every time a file is written.
|
||||||
|
|
||||||
|
Isolation strategy:
|
||||||
|
* ``GIT_CONFIG_GLOBAL=<os.devnull>`` — ignore ``~/.gitconfig`` (git 2.32+).
|
||||||
|
* ``GIT_CONFIG_SYSTEM=<os.devnull>`` — ignore ``/etc/gitconfig`` (git 2.32+).
|
||||||
|
* ``GIT_CONFIG_NOSYSTEM=1`` — legacy belt-and-suspenders for older git.
|
||||||
|
|
||||||
|
The shadow repo still has its own per-repo config (user.email, user.name,
|
||||||
|
commit.gpgsign=false) set in ``_init_shadow_repo``.
|
||||||
|
"""
|
||||||
normalized_working_dir = _normalize_path(working_dir)
|
normalized_working_dir = _normalize_path(working_dir)
|
||||||
env = os.environ.copy()
|
env = os.environ.copy()
|
||||||
env["GIT_DIR"] = str(shadow_repo)
|
env["GIT_DIR"] = str(shadow_repo)
|
||||||
|
|
@ -134,6 +149,13 @@ def _git_env(shadow_repo: Path, working_dir: str) -> dict:
|
||||||
env.pop("GIT_INDEX_FILE", None)
|
env.pop("GIT_INDEX_FILE", None)
|
||||||
env.pop("GIT_NAMESPACE", None)
|
env.pop("GIT_NAMESPACE", None)
|
||||||
env.pop("GIT_ALTERNATE_OBJECT_DIRECTORIES", None)
|
env.pop("GIT_ALTERNATE_OBJECT_DIRECTORIES", None)
|
||||||
|
# Isolate the shadow repo from the user's global/system git config.
|
||||||
|
# Prevents commit.gpgsign, hooks, aliases, credential helpers, etc. from
|
||||||
|
# leaking into background snapshots. Uses os.devnull for cross-platform
|
||||||
|
# support (``/dev/null`` on POSIX, ``nul`` on Windows).
|
||||||
|
env["GIT_CONFIG_GLOBAL"] = os.devnull
|
||||||
|
env["GIT_CONFIG_SYSTEM"] = os.devnull
|
||||||
|
env["GIT_CONFIG_NOSYSTEM"] = "1"
|
||||||
return env
|
return env
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -211,6 +233,13 @@ def _init_shadow_repo(shadow_repo: Path, working_dir: str) -> Optional[str]:
|
||||||
|
|
||||||
_run_git(["config", "user.email", "hermes@local"], shadow_repo, working_dir)
|
_run_git(["config", "user.email", "hermes@local"], shadow_repo, working_dir)
|
||||||
_run_git(["config", "user.name", "Hermes Checkpoint"], shadow_repo, working_dir)
|
_run_git(["config", "user.name", "Hermes Checkpoint"], shadow_repo, working_dir)
|
||||||
|
# Explicitly disable commit/tag signing in the shadow repo. _git_env
|
||||||
|
# already isolates from the user's global config, but writing these into
|
||||||
|
# the shadow's own config is belt-and-suspenders — it guarantees the
|
||||||
|
# shadow repo is correct even if someone inspects or runs git against it
|
||||||
|
# directly (without the GIT_CONFIG_* env vars).
|
||||||
|
_run_git(["config", "commit.gpgsign", "false"], shadow_repo, working_dir)
|
||||||
|
_run_git(["config", "tag.gpgSign", "false"], shadow_repo, working_dir)
|
||||||
|
|
||||||
info_dir = shadow_repo / "info"
|
info_dir = shadow_repo / "info"
|
||||||
info_dir.mkdir(exist_ok=True)
|
info_dir.mkdir(exist_ok=True)
|
||||||
|
|
@ -552,9 +581,11 @@ class CheckpointManager:
|
||||||
logger.debug("Checkpoint skipped: no changes in %s", working_dir)
|
logger.debug("Checkpoint skipped: no changes in %s", working_dir)
|
||||||
return False
|
return False
|
||||||
|
|
||||||
# Commit
|
# Commit. ``--no-gpg-sign`` inline covers shadow repos created before
|
||||||
|
# the commit.gpgsign=false config was added to _init_shadow_repo — so
|
||||||
|
# users with existing checkpoints never hit a GPG pinentry popup.
|
||||||
ok, _, err = _run_git(
|
ok, _, err = _run_git(
|
||||||
["commit", "-m", reason, "--allow-empty-message"],
|
["commit", "-m", reason, "--allow-empty-message", "--no-gpg-sign"],
|
||||||
shadow, working_dir, timeout=_GIT_TIMEOUT * 2,
|
shadow, working_dir, timeout=_GIT_TIMEOUT * 2,
|
||||||
)
|
)
|
||||||
if not ok:
|
if not ok:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue