Skip to content

Commit c146748

Browse files
committed
fix(agent-guard): fail closed on lefthook install behind global options and package runners
The lefthook hooks-path guard took the first bare word as the subcommand, so `lefthook --colors off install --force` read `off` and was allowed, and `npx lefthook install --force` never reached the guard at all. Any `install` word now makes the segment an install, and npx, pnpm, yarn, bun and npm exec are looked through when they launch lefthook. argv[0] is normalised once with the helper guarded_head uses, so git.exe or a path-qualified git also reaches the git:* contributed guards. Closes #1918
1 parent 183441a commit c146748

6 files changed

Lines changed: 139 additions & 35 deletions

File tree

‎plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py‎

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -325,13 +325,14 @@ def __init__(self, tokens: list[str], raw: str, command: str = "") -> None:
325325
#: The command head as written (`~/.claude/scripts/x.sh`, `/usr/bin/git`),
326326
#: for the rules that care how a command is spelled, not just its name.
327327
self.head = argv[0] if argv else ""
328-
# Normalise the command head to its basename so a path-qualified
329-
# invocation (`/usr/bin/git`, `./git`) is guarded identically to the
330-
# bare name. Guard rules only inspect argv[0] as a command *name*; the
331-
# real execution path is untouched (exec_main hands the original argv
332-
# to os.execvp).
328+
# Normalise the command head to its name so a path-qualified invocation
329+
# (`/usr/bin/git`, `./git`, `C:\Git\cmd\git.exe`) is guarded identically
330+
# to the bare name. Guard rules only inspect argv[0] as a command *name*;
331+
# the real execution path is untouched (exec_main hands the original
332+
# argv to os.execvp).
333333
if argv:
334-
argv[0] = os.path.basename(argv[0])
334+
argv[0] = command_name(argv[0])
335+
argv = _package_runner_target(argv) or argv
335336
self.argv: list[str] = argv
336337
self.raw = raw
337338
#: The whole command line as written, before tokenising: the one place
@@ -2598,6 +2599,43 @@ def run(self, args: list[str]) -> str | None:
25982599
return _run(args, cwd=self.cwd)
25992600

26002601

2602+
def command_name(head: str) -> str:
2603+
"""The name a command head runs: its basename on any platform, without ``.exe``.
2604+
2605+
:class:`Segment` stores ``argv[0]`` in this form, so the rules that key on
2606+
a literal ``argv[0] == "git"`` see the same name :func:`guarded_head` does.
2607+
"""
2608+
base = head.replace("\\", "/").rsplit("/", 1)[-1]
2609+
if base.lower().endswith(".exe"):
2610+
base = base[:-4]
2611+
return base
2612+
2613+
2614+
# Package runners that reach a package's binary without it being on PATH
2615+
# (`npx lefthook`, `pnpm exec lefthook`, `yarn dlx lefthook`, `bunx lefthook`).
2616+
_PACKAGE_RUNNERS = frozenset({"npx", "pnpx", "bunx", "npm", "pnpm", "yarn", "bun"})
2617+
_RUNNER_TARGETS = frozenset({"lefthook"})
2618+
2619+
2620+
def _package_runner_target(argv: list[str]) -> list[str] | None:
2621+
"""The guarded command a package runner launches, or None.
2622+
2623+
Fails closed: the first word naming a guarded package (``lefthook``,
2624+
``lefthook@1.11``, ``@evilmartians/lefthook``) is taken as the command, so
2625+
the runner's own options and subcommand never need parsing. Only package
2626+
managers whose CLI can run a package's binary are looked through.
2627+
"""
2628+
if not argv or argv[0] not in _PACKAGE_RUNNERS:
2629+
return None
2630+
for index, token in enumerate(argv[1:], start=1):
2631+
if token.startswith("-"):
2632+
continue
2633+
package = token.rsplit("/", 1)[-1].split("@", 1)[0]
2634+
if package in _RUNNER_TARGETS:
2635+
return [package, *argv[index + 1 :]]
2636+
return None
2637+
2638+
26012639
def guarded_head(argv: list[str]) -> str | None:
26022640
"""The guarded command family a segment runs, or None.
26032641
@@ -2607,13 +2645,8 @@ def guarded_head(argv: list[str]) -> str | None:
26072645
"""
26082646
if not argv:
26092647
return None
2610-
head = argv[0]
2611-
if head in GUARDED_HEADS:
2612-
return head
2613-
base = head.replace("\\", "/").rsplit("/", 1)[-1]
2614-
if base.lower().endswith(".exe"):
2615-
base = base[:-4]
2616-
return base if base in GUARDED_HEADS else None
2648+
name = command_name(argv[0])
2649+
return name if name in GUARDED_HEADS else None
26172650

26182651

26192652
def command_kinds(seg: Segment) -> set[str]:
@@ -2637,10 +2670,14 @@ def command_kinds(seg: Segment) -> set[str]:
26372670
sub = gh_subcommand(seg.argv)
26382671
kinds.add(f"{head}:{sub[0] if sub else seg.argv[1]}")
26392672
elif head == "lefthook":
2640-
# lefthook accepts global flags before the subcommand.
2673+
# lefthook accepts global flags before the subcommand, and one can take
2674+
# its value as a separate word (`--colors off install`), so the first
2675+
# bare word is not always the subcommand. Fail closed on `install`.
26412676
lefthook_sub = next((tok for tok in seg.argv[1:] if not tok.startswith("-")), None)
26422677
if lefthook_sub:
26432678
kinds.add(f"lefthook:{lefthook_sub}")
2679+
if "install" in seg.argv[1:]:
2680+
kinds.add("lefthook:install")
26442681
return kinds
26452682

26462683

‎plugins/magpie-setup/skills/isolated-setup-install/guards/lefthook_hooks_path.py‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,14 @@
3232

3333

3434
def _install_arguments(argv):
35-
"""The arguments after the ``install`` subcommand, or an empty list."""
36-
for index, token in enumerate(argv[1:], start=1):
37-
if token.startswith("-"):
38-
continue
39-
return argv[index + 1 :] if token == "install" else []
40-
return []
35+
"""Every argument of a ``lefthook install``, or an empty list.
36+
37+
A global option can take its value as a separate word (`lefthook --colors
38+
off install --force`), so the first bare word is not always the subcommand.
39+
Fail closed: any ``install`` word makes this an install, and every argument
40+
is checked for a destructive flag.
41+
"""
42+
return argv[1:] if "install" in argv[1:] else []
4143

4244

4345
def _destructive_flag(argument):

‎tools/agent-guard/README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ way — see [Contributing guards](#contributing-guards)):
136136
| `mention` | `pr-management-triage` | `gh pr comment` / `gh issue comment` that `@`-mentions anyone other than the PR/issue author; **any** `@`-mention in `gh pr edit --body[-file]` | denoise: author-directed feedback never pings maintainers; body edits stay silent. Exempt: the operator commenting on their **own** PR/issue (author == authenticated `gh` user), and the `MAGPIE_ALLOW_MENTIONS=1` override |
137137
| `mark-ready` | `pr-management-triage` | adding `ready for maintainer review` while the PR head SHA has GitHub Actions runs awaiting approval | Golden rule 1b |
138138
| `security-language` | `security-issue-fix` | a CVE id / security-fix language in a **public** `gh pr create`/`gh pr edit` title/body (not comments) | public-PR scrubbing |
139-
| `lefthook-hooks-path` | `setup-isolated-setup-install` | `lefthook install --force` / `-f` or `--reset-hooks-path` / `-r` while the global `core.hooksPath` is the framework's | the first writes over the dispatcher for every repository, the second unsets the whole-user setup; the shim hides the global path, so a plain `lefthook install` is all that is needed. Override: `MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1` |
139+
| `lefthook-hooks-path` | `setup-isolated-setup-install` | `lefthook install --force` / `-f` or `--reset-hooks-path` / `-r` while the global `core.hooksPath` is the framework's, also behind global options and package runners (`npx`, `pnpm exec`, `yarn exec` / `dlx`, `bunx`) | the first writes over the dispatcher for every repository, the second unsets the whole-user setup; the shim hides the global path, so a plain `lefthook install` is all that is needed. Override: `MAGPIE_ALLOW_LEFTHOOK_HOOKS_PATH=1` |
140140

141141
A denied command is **not** posted/run; the model is shown the reason and the
142142
deterministic fix (e.g. "use a backtick `` `login` `` instead of `@login`").

‎tools/agent-guard/src/agent_guard/__init__.py‎

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -325,13 +325,14 @@ def __init__(self, tokens: list[str], raw: str, command: str = "") -> None:
325325
#: The command head as written (`~/.claude/scripts/x.sh`, `/usr/bin/git`),
326326
#: for the rules that care how a command is spelled, not just its name.
327327
self.head = argv[0] if argv else ""
328-
# Normalise the command head to its basename so a path-qualified
329-
# invocation (`/usr/bin/git`, `./git`) is guarded identically to the
330-
# bare name. Guard rules only inspect argv[0] as a command *name*; the
331-
# real execution path is untouched (exec_main hands the original argv
332-
# to os.execvp).
328+
# Normalise the command head to its name so a path-qualified invocation
329+
# (`/usr/bin/git`, `./git`, `C:\Git\cmd\git.exe`) is guarded identically
330+
# to the bare name. Guard rules only inspect argv[0] as a command *name*;
331+
# the real execution path is untouched (exec_main hands the original
332+
# argv to os.execvp).
333333
if argv:
334-
argv[0] = os.path.basename(argv[0])
334+
argv[0] = command_name(argv[0])
335+
argv = _package_runner_target(argv) or argv
335336
self.argv: list[str] = argv
336337
self.raw = raw
337338
#: The whole command line as written, before tokenising: the one place
@@ -2598,6 +2599,43 @@ def run(self, args: list[str]) -> str | None:
25982599
return _run(args, cwd=self.cwd)
25992600

26002601

2602+
def command_name(head: str) -> str:
2603+
"""The name a command head runs: its basename on any platform, without ``.exe``.
2604+
2605+
:class:`Segment` stores ``argv[0]`` in this form, so the rules that key on
2606+
a literal ``argv[0] == "git"`` see the same name :func:`guarded_head` does.
2607+
"""
2608+
base = head.replace("\\", "/").rsplit("/", 1)[-1]
2609+
if base.lower().endswith(".exe"):
2610+
base = base[:-4]
2611+
return base
2612+
2613+
2614+
# Package runners that reach a package's binary without it being on PATH
2615+
# (`npx lefthook`, `pnpm exec lefthook`, `yarn dlx lefthook`, `bunx lefthook`).
2616+
_PACKAGE_RUNNERS = frozenset({"npx", "pnpx", "bunx", "npm", "pnpm", "yarn", "bun"})
2617+
_RUNNER_TARGETS = frozenset({"lefthook"})
2618+
2619+
2620+
def _package_runner_target(argv: list[str]) -> list[str] | None:
2621+
"""The guarded command a package runner launches, or None.
2622+
2623+
Fails closed: the first word naming a guarded package (``lefthook``,
2624+
``lefthook@1.11``, ``@evilmartians/lefthook``) is taken as the command, so
2625+
the runner's own options and subcommand never need parsing. Only package
2626+
managers whose CLI can run a package's binary are looked through.
2627+
"""
2628+
if not argv or argv[0] not in _PACKAGE_RUNNERS:
2629+
return None
2630+
for index, token in enumerate(argv[1:], start=1):
2631+
if token.startswith("-"):
2632+
continue
2633+
package = token.rsplit("/", 1)[-1].split("@", 1)[0]
2634+
if package in _RUNNER_TARGETS:
2635+
return [package, *argv[index + 1 :]]
2636+
return None
2637+
2638+
26012639
def guarded_head(argv: list[str]) -> str | None:
26022640
"""The guarded command family a segment runs, or None.
26032641
@@ -2607,13 +2645,8 @@ def guarded_head(argv: list[str]) -> str | None:
26072645
"""
26082646
if not argv:
26092647
return None
2610-
head = argv[0]
2611-
if head in GUARDED_HEADS:
2612-
return head
2613-
base = head.replace("\\", "/").rsplit("/", 1)[-1]
2614-
if base.lower().endswith(".exe"):
2615-
base = base[:-4]
2616-
return base if base in GUARDED_HEADS else None
2648+
name = command_name(argv[0])
2649+
return name if name in GUARDED_HEADS else None
26172650

26182651

26192652
def command_kinds(seg: Segment) -> set[str]:
@@ -2637,10 +2670,14 @@ def command_kinds(seg: Segment) -> set[str]:
26372670
sub = gh_subcommand(seg.argv)
26382671
kinds.add(f"{head}:{sub[0] if sub else seg.argv[1]}")
26392672
elif head == "lefthook":
2640-
# lefthook accepts global flags before the subcommand.
2673+
# lefthook accepts global flags before the subcommand, and one can take
2674+
# its value as a separate word (`--colors off install`), so the first
2675+
# bare word is not always the subcommand. Fail closed on `install`.
26412676
lefthook_sub = next((tok for tok in seg.argv[1:] if not tok.startswith("-")), None)
26422677
if lefthook_sub:
26432678
kinds.add(f"lefthook:{lefthook_sub}")
2679+
if "install" in seg.argv[1:]:
2680+
kinds.add("lefthook:install")
26442681
return kinds
26452682

26462683

‎tools/agent-guard/tests/test_guards.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,14 @@ def test_no_verify_guard_denied_past_a_global_flag():
433433
assert reason and "no-verify" in reason
434434

435435

436+
@pytest.mark.parametrize("head", ["/usr/bin/git", "/mingw64/bin/git.exe", "git.exe"])
437+
def test_no_verify_guard_denied_for_a_path_qualified_git(head):
438+
# guarded_head let these through to the guards, but the `git:commit`
439+
# TRIGGERS pre-filter keyed on a literal argv[0] == "git".
440+
reason = dispatch(f'{head} commit -m "x" --no-verify')
441+
assert reason and "no-verify" in reason
442+
443+
436444
# --------------------------------------------------------------------------- #
437445
# contributed-guard discovery
438446
# --------------------------------------------------------------------------- #

‎tools/agent-guard/tests/test_skill_guards.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,20 @@ def handler(args):
608608
("lefthook install -rf=false", "--reset-hooks-path"),
609609
("lefthook --verbose install --force", "--force"),
610610
("./node_modules/.bin/lefthook install --force", "--force"),
611+
("lefthook --colors off install --force", "--force"),
612+
("lefthook --colors off install -r", "--reset-hooks-path"),
613+
("lefthook install pre-commit --force", "--force"),
614+
("npx lefthook install --force", "--force"),
615+
("npx -y lefthook@1.11.0 install -f", "--force"),
616+
("npx @evilmartians/lefthook install --force", "--force"),
617+
("npm exec -- lefthook install --force", "--force"),
618+
("pnpm exec lefthook install --reset-hooks-path", "--reset-hooks-path"),
619+
("pnpm --filter app exec lefthook install -f", "--force"),
620+
("pnpm dlx lefthook install --force", "--force"),
621+
("yarn exec lefthook install -f", "--force"),
622+
("yarn dlx lefthook install --force", "--force"),
623+
("bunx lefthook install -r", "--reset-hooks-path"),
624+
("env CI=1 npx lefthook --colors off install --force", "--force"),
611625
],
612626
)
613627
def test_lefthook_destructive_install_denied(monkeypatch, hooks_path, command, flag):
@@ -623,6 +637,12 @@ def test_lefthook_destructive_install_denied(monkeypatch, hooks_path, command, f
623637
"lefthook install --verbose",
624638
"lefthook run pre-commit",
625639
"lefthook --version",
640+
"lefthook --colors off install",
641+
"npx lefthook install",
642+
"npx lefthook run pre-commit --force",
643+
"npm install --save-dev lefthook --force",
644+
"pnpm add -D lefthook",
645+
"npx prettier --write install",
626646
],
627647
)
628648
def test_lefthook_plain_invocations_allowed(monkeypatch, command):

0 commit comments

Comments
 (0)