From 20961106b55ecf263341be334f6bc0facf07c422 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 17 Oct 2026 18:48:32 +0200 Subject: [PATCH 1/2] fix(agent-guard): see through env/command; refuse a quoted ~ and the eval wrapper's other spellings Closes the gaps tracked in #1679, plus one found while running evals. - Every guard now looks through the prefix wrappers that run their argument: `env` (with -i, -u NAME, -S "...", and NAME=value assignments, which still count as inline overrides) and `command` (-p). `env git commit ...` used to reach no guard at all. `command -v git` is a lookup and is left alone. - vetted-ops-spelling refuses a quoted or escaped `~` in the --project/--from value: the shell does not expand it, so uv gets a directory named `~` and the exclusion does not match. - A sandbox exclusion matches a command line by its start, so the right spelling behind `env` / `command` still runs sandboxed: refused, with the command to run instead. - `magpie-run-evals.sh` launched by any path but the literal `~/.claude/scripts/magpie-run-evals.sh` (an expanded home, $HOME, a quoted `~`, a repository copy, a wrapper) is refused: it would run inside the sandbox and every eval case would error ungraded. Override: MAGPIE_ALLOW_SCRIPT_PATH=1. AGENTS.md's golden rule and the agent-guard README cover both. Generated-by: Claude Opus 5 --- AGENTS.md | 2 + .../agent-guard/src/agent_guard/__init__.py | 148 ++++++++++++++++-- .../setup_preflight/isolated_fingerprint.py | 2 +- tools/agent-guard/README.md | 7 +- tools/agent-guard/src/agent_guard/__init__.py | 148 ++++++++++++++++-- tools/agent-guard/tests/test_guards.py | 98 ++++++++++++ tools/agent-guard/tests/test_skill_guards.py | 18 +++ 7 files changed, 399 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 922cc2fc5..abc2f9572 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -502,6 +502,8 @@ The sandbox exclusions and permission rules name only the `~` spelling, so an ex A checkout path is worse: it runs code the agent can edit, which must never leave the sandbox. The agent-guard [`vetted-ops-spelling`](tools/agent-guard/README.md#guards) guard enforces this rule and names the corrected command. Codex's own absolute-path rewrite is the one exception. +The same holds for the eval wrapper, `~/.claude/scripts/magpie-run-evals.sh`, and for any sandbox-excluded command: +write it bare, with the literal `~`, as the whole command line (not behind `env` or `command`), or it runs inside the sandbox and fails. **Tool credentials live under `$HOME`, never in the project tree.** Any persistent token, API key, OAuth refresh token, or session cookie a diff --git a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py index 49442e446..631938509 100644 --- a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py +++ b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py @@ -129,6 +129,9 @@ DEFAULT_ATTRIBUTION = "generated-by" +_ASSIGNMENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) + + class Segment: """One simple command from a (possibly compound) shell line. @@ -137,24 +140,85 @@ class Segment: substring scans that survive heredocs, e.g. the Co-Authored-By trailer). """ - def __init__(self, tokens: list[str], raw: str) -> None: + def __init__(self, tokens: list[str], raw: str, command: str = "") -> None: self.env: dict[str, str] = {} - i = 0 - while i < len(tokens) and re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*=.*", tokens[i]): - name, _, value = tokens[i].partition("=") - self.env[name] = value - i += 1 - argv = tokens[i:] + #: True when an `env` / `command` wrapper was looked through. Sandbox + #: exclusions match a command line by its start, so a wrapped excluded + #: command runs sandboxed even when the command itself is spelled right. + self.wrapped = False + argv = self._unwrap(self._take_assignments(tokens)) + #: The command head as written (`~/.claude/scripts/x.sh`, `/usr/bin/git`), + #: for the rules that care how a command is spelled, not just its name. + self.head = argv[0] if argv else "" # Normalise the command head to its basename so a path-qualified # invocation (`/usr/bin/git`, `./git`) is guarded identically to the # bare name. Guard rules only inspect argv[0] as a command *name*; the # real execution path is untouched (exec_main hands the original argv - # to os.execvp). Prefix wrappers like `env git` / `command git` keep - # argv[0] == "env" and remain a separate, pre-existing gap. + # to os.execvp). if argv: argv[0] = os.path.basename(argv[0]) self.argv: list[str] = argv self.raw = raw + #: The whole command line as written, before tokenising: the one place + #: quoting is still visible (a quoted `~` is not expanded by the shell). + self.command = command + + def _take_assignments(self, tokens: list[str]) -> list[str]: + """Strip leading ``NAME=value`` assignments into ``env``.""" + i = 0 + while i < len(tokens) and _ASSIGNMENT_RE.fullmatch(tokens[i]): + name, _, value = tokens[i].partition("=") + self.env[name] = value + i += 1 + return tokens[i:] + + def _unwrap(self, argv: list[str]) -> list[str]: + """Look through prefix wrappers that run their argument as the command: + ``env [-i] [-u NAME] [NAME=value…] cmd …`` (its assignments join + ``env``, so an inline override still counts) and ``command [-p] cmd …``. + Without this, ``env git commit …`` reached no guard at all.""" + for _ in range(8): # nested wrappers (`env command git …`), bounded + if not argv: + return argv + head = os.path.basename(argv[0]) + if head in ("env", "command") and not (head == "command" and argv[1:2] in (["-v"], ["-V"])): + self.wrapped = True + if head == "env": + i = 1 + while i < len(argv): + tok = argv[i] + if tok in ("-u", "--unset", "-C", "--chdir"): + i += 2 + elif tok in ("-S", "--split-string") and i + 1 < len(argv): + # `env -S "git commit …"`: the string is the command. + argv = [*shlex.split(argv[i + 1]), *argv[i + 2 :]] + i = 0 + break + elif tok.startswith("--split-string=") or (tok.startswith("-S") and len(tok) > 2): + packed = tok.split("=", 1)[1] if tok.startswith("--") else tok[2:] + argv = [*shlex.split(packed), *argv[i + 1 :]] + i = 0 + break + elif tok == "--": + i += 1 + break + elif tok.startswith("-"): + i += 1 + else: + break + else: + return [] + argv = self._take_assignments(argv[i:]) + elif head == "command": + rest = argv[1:] + if rest[:1] and rest[0] in ("-v", "-V"): + return argv # a lookup, not an execution + while rest[:1] and rest[0] in ("-p", "--"): + rest = rest[1:] + argv = rest + else: + return argv + return argv def override(self, *names: str) -> bool: """True if any of ``names`` (or the global off switch) is set truthy, @@ -184,12 +248,12 @@ def split_segments(command: str) -> list[Segment]: for tok in tokens: if tok in SHELL_OPERATORS: if current: - segments.append(Segment(current, shlex.join(current))) + segments.append(Segment(current, shlex.join(current), command)) current = [] else: current.append(tok) if current: - segments.append(Segment(current, shlex.join(current))) + segments.append(Segment(current, shlex.join(current), command)) return segments @@ -1090,6 +1154,42 @@ def _toml_commands(node: object) -> list[str]: return found +#: User-scope scripts the sandbox exclusion names by their literal `~` path +#: (docs/setup/secure-agent-setup.md): basename -> the one spelling that matches. +EXCLUDED_SCRIPTS = { + "magpie-run-evals.sh": "~/.claude/scripts/magpie-run-evals.sh", +} + + +def guard_excluded_script_spelling(seg: Segment) -> str | None: + """Refuse a sandbox-excluded user-scope script launched by any other spelling. + + The exclusion matches only ``~/.claude/scripts/``: an expanded home + path (``/Users//…``), ``$HOME/…``, a quoted ``~`` or a repository copy + runs *inside* the sandbox, where the script cannot reach what it was + excluded for (the eval wrapper: the model API), so every case errors + without being graded. + """ + stable = EXCLUDED_SCRIPTS.get(seg.argv[0]) + if stable is None: + return None + quoted = re.search(r"""(?:^|[\s;&|(])(?:["']|\\)~/\.claude/scripts/""", seg.command) + if seg.head == stable and not quoted and not seg.wrapped: + return None + if seg.override("MAGPIE_ALLOW_SCRIPT_PATH"): + return None + shown = f"{'env/command … ' if seg.wrapped else ''}{seg.head}" + return ( + f"agent-guard[vetted-ops-spelling]: run `{stable} …` exactly so, as the whole command " + f"with a bare `~`, not `{shown}`. The sandbox exclusion names only that spelling; any " + "other runs the script inside the sandbox, where it fails. Override: MAGPIE_ALLOW_SCRIPT_PATH=1." + ) + + +#: `--project "~/…"`, `--from='~/…'`, `--project \~/…`: a `~` the shell leaves alone. +_QUOTED_TILDE_RE = re.compile(r"""(?:--project|--from)(?:=|\s+)(?:["']|\\)~""") + + def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str | None = None) -> str | None: launch = _vetted_launch(seg.argv) if launch is None: @@ -1098,6 +1198,27 @@ def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str tool = VETTED_ENTRY_POINTS[entry] stable = VETTED_STABLE_BASE + tool base = base.rstrip("/") + if base.startswith("~") and _QUOTED_TILDE_RE.search(seg.command): + # Tokenising drops the quotes, but the shell does not expand a quoted + # or escaped `~`: uv would get a directory literally named `~`, and the + # exclusion (which names the bare spelling) would not match either. + if seg.override("MAGPIE_ALLOW_VETTED_PATH"): + return None + return ( + f"agent-guard[vetted-ops-spelling]: the `~` in `{option} {base}` is quoted or escaped, " + "so the shell will not expand it and the sandbox exclusion will not match. " + f"Write it bare: `{seg.argv[0]} {'run ' if seg.argv[0] == 'uv' else ''}{option} {stable} {entry} …`." + ) + if base == stable and seg.wrapped: + # Right spelling, but behind `env` / `command`: the exclusion matches a + # command line by its start, so this would run inside the sandbox. + if seg.override("MAGPIE_ALLOW_VETTED_PATH"): + return None + return ( + f"agent-guard[vetted-ops-spelling]: run `{seg.argv[0]} {'run ' if seg.argv[0] == 'uv' else ''}" + f"{option} {stable} {entry} …` as the whole command, not behind `env` / `command`: " + "the sandbox exclusion matches only a command line that starts with it." + ) if base == stable: return None if harness == "codex" and entry == "vetted-op-read" and base in _codex_stable_roots(): @@ -1509,6 +1630,11 @@ def dispatch(command: str, cwd: str | None = None, harness: str | None = None) - if reason: return reason continue + if seg.argv and seg.argv[0] in EXCLUDED_SCRIPTS: + reason = guard_excluded_script_spelling(seg) + if reason: + return reason + continue if not seg.argv or seg.argv[0] not in GUARDED_HEADS: continue # fast path — non-guarded command family # Bundled guards (trusted, in-process; each self-filters its command). diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py index 6673e4868..158b7bc5d 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/isolated_fingerprint.py @@ -21,4 +21,4 @@ for installs that do not carry the framework source (see `isolated.py`). """ -FRAMEWORK_FINGERPRINT = "sha256:730903b8803cfd72" +FRAMEWORK_FINGERPRINT = "sha256:37a588576a8077bd" diff --git a/tools/agent-guard/README.md b/tools/agent-guard/README.md index 66ed2e221..c85bac539 100644 --- a/tools/agent-guard/README.md +++ b/tools/agent-guard/README.md @@ -118,7 +118,7 @@ dispatcher spelling, on for every project): |---|---|---| | `commit-trailer` | `git commit` whose message contains `Co-Authored-By:`, unless the repository's [commit-attribution convention](../../docs/setup/commit-attribution.md) is `co-authored-by` | AGENTS.md: agents use the project's attribution trailer (default `Generated-by:`), not co-authorship | | `empty-rebase` | `git push --force[-with-lease]` of a branch with 0 commits over its base | an empty push to a PR head auto-closes it + revokes write | -| `vetted-ops-spelling` | `uv run --project` / `uvx --from` (options on either side of `run`) launching `vetted-op`, `vetted-op-read`, `vetted-op-tracker` or `adversarial-review` from anything but the literal `~/.claude/magpie/` — an expanded home path, `$HOME/…`, or a checkout path, relative (`tools/vetted-ops`) or absolute inside the working tree | AGENTS.md golden rule: the sandbox exclusions and permission rules name only the `~` spelling. Under Codex, a `vetted-op-read` launched from the exact root its own [rewrite hook](../vetted-ops/hooks/codex-expand-stable-path.py) is registered with (`--stable-path` in the Codex configuration, else the account's home) is let through | +| `vetted-ops-spelling` | `uv run --project` / `uvx --from` (options on either side of `run`) launching `vetted-op`, `vetted-op-read`, `vetted-op-tracker` or `adversarial-review` from anything but the literal `~/.claude/magpie/` — an expanded home path, `$HOME/…`, or a checkout path, relative (`tools/vetted-ops`) or absolute inside the working tree; a quoted or escaped `~`; the right spelling behind `env` / `command` (the exclusion matches a line by its start); and `~/.claude/scripts/magpie-run-evals.sh` launched by any other path | AGENTS.md golden rule: the sandbox exclusions and permission rules name only the `~` spelling. Under Codex, a `vetted-op-read` launched from the exact root its own [rewrite hook](../vetted-ops/hooks/codex-expand-stable-path.py) is registered with (`--stable-path` in the Codex configuration, else the account's home) is let through | **Skill-owned** (each lives in its skill's `guards/` dir, discovered the same way — see [Contributing guards](#contributing-guards)): @@ -132,6 +132,11 @@ way — see [Contributing guards](#contributing-guards)): A denied command is **not** posted/run; the model is shown the reason and the deterministic fix (e.g. "use a backtick `` `login` `` instead of `@login`"). +Every guard sees through the prefix wrappers that run their argument as the +command: `env` (with `-i`, `-u NAME`, `-S "…"` and `NAME=value` assignments, +which count as inline overrides) and `command` (`-p`). `env git commit …` is +checked as `git commit …`; `command -v git` is a lookup and is left alone. + ## Per-command overrides Each guard is overridable by a **visible inline env assignment** so a maintainer diff --git a/tools/agent-guard/src/agent_guard/__init__.py b/tools/agent-guard/src/agent_guard/__init__.py index 49442e446..631938509 100644 --- a/tools/agent-guard/src/agent_guard/__init__.py +++ b/tools/agent-guard/src/agent_guard/__init__.py @@ -129,6 +129,9 @@ DEFAULT_ATTRIBUTION = "generated-by" +_ASSIGNMENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) + + class Segment: """One simple command from a (possibly compound) shell line. @@ -137,24 +140,85 @@ class Segment: substring scans that survive heredocs, e.g. the Co-Authored-By trailer). """ - def __init__(self, tokens: list[str], raw: str) -> None: + def __init__(self, tokens: list[str], raw: str, command: str = "") -> None: self.env: dict[str, str] = {} - i = 0 - while i < len(tokens) and re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*=.*", tokens[i]): - name, _, value = tokens[i].partition("=") - self.env[name] = value - i += 1 - argv = tokens[i:] + #: True when an `env` / `command` wrapper was looked through. Sandbox + #: exclusions match a command line by its start, so a wrapped excluded + #: command runs sandboxed even when the command itself is spelled right. + self.wrapped = False + argv = self._unwrap(self._take_assignments(tokens)) + #: The command head as written (`~/.claude/scripts/x.sh`, `/usr/bin/git`), + #: for the rules that care how a command is spelled, not just its name. + self.head = argv[0] if argv else "" # Normalise the command head to its basename so a path-qualified # invocation (`/usr/bin/git`, `./git`) is guarded identically to the # bare name. Guard rules only inspect argv[0] as a command *name*; the # real execution path is untouched (exec_main hands the original argv - # to os.execvp). Prefix wrappers like `env git` / `command git` keep - # argv[0] == "env" and remain a separate, pre-existing gap. + # to os.execvp). if argv: argv[0] = os.path.basename(argv[0]) self.argv: list[str] = argv self.raw = raw + #: The whole command line as written, before tokenising: the one place + #: quoting is still visible (a quoted `~` is not expanded by the shell). + self.command = command + + def _take_assignments(self, tokens: list[str]) -> list[str]: + """Strip leading ``NAME=value`` assignments into ``env``.""" + i = 0 + while i < len(tokens) and _ASSIGNMENT_RE.fullmatch(tokens[i]): + name, _, value = tokens[i].partition("=") + self.env[name] = value + i += 1 + return tokens[i:] + + def _unwrap(self, argv: list[str]) -> list[str]: + """Look through prefix wrappers that run their argument as the command: + ``env [-i] [-u NAME] [NAME=value…] cmd …`` (its assignments join + ``env``, so an inline override still counts) and ``command [-p] cmd …``. + Without this, ``env git commit …`` reached no guard at all.""" + for _ in range(8): # nested wrappers (`env command git …`), bounded + if not argv: + return argv + head = os.path.basename(argv[0]) + if head in ("env", "command") and not (head == "command" and argv[1:2] in (["-v"], ["-V"])): + self.wrapped = True + if head == "env": + i = 1 + while i < len(argv): + tok = argv[i] + if tok in ("-u", "--unset", "-C", "--chdir"): + i += 2 + elif tok in ("-S", "--split-string") and i + 1 < len(argv): + # `env -S "git commit …"`: the string is the command. + argv = [*shlex.split(argv[i + 1]), *argv[i + 2 :]] + i = 0 + break + elif tok.startswith("--split-string=") or (tok.startswith("-S") and len(tok) > 2): + packed = tok.split("=", 1)[1] if tok.startswith("--") else tok[2:] + argv = [*shlex.split(packed), *argv[i + 1 :]] + i = 0 + break + elif tok == "--": + i += 1 + break + elif tok.startswith("-"): + i += 1 + else: + break + else: + return [] + argv = self._take_assignments(argv[i:]) + elif head == "command": + rest = argv[1:] + if rest[:1] and rest[0] in ("-v", "-V"): + return argv # a lookup, not an execution + while rest[:1] and rest[0] in ("-p", "--"): + rest = rest[1:] + argv = rest + else: + return argv + return argv def override(self, *names: str) -> bool: """True if any of ``names`` (or the global off switch) is set truthy, @@ -184,12 +248,12 @@ def split_segments(command: str) -> list[Segment]: for tok in tokens: if tok in SHELL_OPERATORS: if current: - segments.append(Segment(current, shlex.join(current))) + segments.append(Segment(current, shlex.join(current), command)) current = [] else: current.append(tok) if current: - segments.append(Segment(current, shlex.join(current))) + segments.append(Segment(current, shlex.join(current), command)) return segments @@ -1090,6 +1154,42 @@ def _toml_commands(node: object) -> list[str]: return found +#: User-scope scripts the sandbox exclusion names by their literal `~` path +#: (docs/setup/secure-agent-setup.md): basename -> the one spelling that matches. +EXCLUDED_SCRIPTS = { + "magpie-run-evals.sh": "~/.claude/scripts/magpie-run-evals.sh", +} + + +def guard_excluded_script_spelling(seg: Segment) -> str | None: + """Refuse a sandbox-excluded user-scope script launched by any other spelling. + + The exclusion matches only ``~/.claude/scripts/``: an expanded home + path (``/Users//…``), ``$HOME/…``, a quoted ``~`` or a repository copy + runs *inside* the sandbox, where the script cannot reach what it was + excluded for (the eval wrapper: the model API), so every case errors + without being graded. + """ + stable = EXCLUDED_SCRIPTS.get(seg.argv[0]) + if stable is None: + return None + quoted = re.search(r"""(?:^|[\s;&|(])(?:["']|\\)~/\.claude/scripts/""", seg.command) + if seg.head == stable and not quoted and not seg.wrapped: + return None + if seg.override("MAGPIE_ALLOW_SCRIPT_PATH"): + return None + shown = f"{'env/command … ' if seg.wrapped else ''}{seg.head}" + return ( + f"agent-guard[vetted-ops-spelling]: run `{stable} …` exactly so, as the whole command " + f"with a bare `~`, not `{shown}`. The sandbox exclusion names only that spelling; any " + "other runs the script inside the sandbox, where it fails. Override: MAGPIE_ALLOW_SCRIPT_PATH=1." + ) + + +#: `--project "~/…"`, `--from='~/…'`, `--project \~/…`: a `~` the shell leaves alone. +_QUOTED_TILDE_RE = re.compile(r"""(?:--project|--from)(?:=|\s+)(?:["']|\\)~""") + + def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str | None = None) -> str | None: launch = _vetted_launch(seg.argv) if launch is None: @@ -1098,6 +1198,27 @@ def guard_vetted_ops_spelling(seg: Segment, cwd: str | None = None, harness: str tool = VETTED_ENTRY_POINTS[entry] stable = VETTED_STABLE_BASE + tool base = base.rstrip("/") + if base.startswith("~") and _QUOTED_TILDE_RE.search(seg.command): + # Tokenising drops the quotes, but the shell does not expand a quoted + # or escaped `~`: uv would get a directory literally named `~`, and the + # exclusion (which names the bare spelling) would not match either. + if seg.override("MAGPIE_ALLOW_VETTED_PATH"): + return None + return ( + f"agent-guard[vetted-ops-spelling]: the `~` in `{option} {base}` is quoted or escaped, " + "so the shell will not expand it and the sandbox exclusion will not match. " + f"Write it bare: `{seg.argv[0]} {'run ' if seg.argv[0] == 'uv' else ''}{option} {stable} {entry} …`." + ) + if base == stable and seg.wrapped: + # Right spelling, but behind `env` / `command`: the exclusion matches a + # command line by its start, so this would run inside the sandbox. + if seg.override("MAGPIE_ALLOW_VETTED_PATH"): + return None + return ( + f"agent-guard[vetted-ops-spelling]: run `{seg.argv[0]} {'run ' if seg.argv[0] == 'uv' else ''}" + f"{option} {stable} {entry} …` as the whole command, not behind `env` / `command`: " + "the sandbox exclusion matches only a command line that starts with it." + ) if base == stable: return None if harness == "codex" and entry == "vetted-op-read" and base in _codex_stable_roots(): @@ -1509,6 +1630,11 @@ def dispatch(command: str, cwd: str | None = None, harness: str | None = None) - if reason: return reason continue + if seg.argv and seg.argv[0] in EXCLUDED_SCRIPTS: + reason = guard_excluded_script_spelling(seg) + if reason: + return reason + continue if not seg.argv or seg.argv[0] not in GUARDED_HEADS: continue # fast path — non-guarded command family # Bundled guards (trusted, in-process; each self-filters its command). diff --git a/tools/agent-guard/tests/test_guards.py b/tools/agent-guard/tests/test_guards.py index 36f766e60..738830a65 100644 --- a/tools/agent-guard/tests/test_guards.py +++ b/tools/agent-guard/tests/test_guards.py @@ -838,3 +838,101 @@ def test_vetted_ops_spelling_refuses_an_absolute_checkout_path(monkeypatch, tmp_ # A harness's own plugin directory outside the working tree is left alone. outside = "uvx --from /opt/harness/plugins/magpie/tools/adversarial-review adversarial-review detect" assert agent_guard.dispatch(outside, cwd=str(repo)) is None + + +# --------------------------------------------------------------------------- # +# Prefix wrappers (#1679): `env …` / `command …` run their argument, so every +# guard sees through them. +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize( + "prefix", + ["env ", "env -i ", "env -u HOME ", "env A=1 B=2 ", "env -- ", "command ", "command -p ", "env command "], +) +def test_prefix_wrappers_reach_the_bundled_guards(prefix): + reason = agent_guard.dispatch(f'{prefix}git commit -m "x\n\nCo-Authored-By: a"') + assert reason and "Co-Authored-By" in reason + reason = agent_guard.dispatch(f"{prefix}uv run --project tools/vetted-ops vetted-op-read pr-view 1") + assert reason and "agent-guard[vetted-ops-spelling]" in reason + + +def test_env_split_string_reaches_the_guards(): + reason = agent_guard.dispatch("""env -S 'git commit -m "x Co-Authored-By: a"'""") + assert reason and "Co-Authored-By" in reason + + +def test_command_lookup_is_not_an_execution(): + assert agent_guard.dispatch("command -v git") is None + + +def test_an_env_assignment_still_overrides(): + assert ( + agent_guard.dispatch( + "env MAGPIE_ALLOW_VETTED_PATH=1 uv run --project tools/vetted-ops vetted-op-read x" + ) + is None + ) + + +@pytest.mark.parametrize( + "base", + ['"~/.claude/magpie/vetted-ops"', "'~/.claude/magpie/vetted-ops'", "\\~/.claude/magpie/vetted-ops"], +) +def test_vetted_ops_spelling_refuses_a_quoted_tilde(base): + reason = agent_guard.dispatch(f"uv run --project {base} vetted-op-read pr-view 1") + assert reason and "quoted or escaped" in reason + reason = agent_guard.dispatch(f"uvx --from={base} vetted-op-read pr-view 1") + assert reason and "quoted or escaped" in reason + + +def test_a_bare_tilde_is_still_allowed(): + assert ( + agent_guard.dispatch("uv run --project ~/.claude/magpie/vetted-ops vetted-op-read pr-view 1") is None + ) + + +# --------------------------------------------------------------------------- # +# Sandbox-excluded user-scope scripts: only the literal `~` spelling matches. +# --------------------------------------------------------------------------- # + +EVALS = "tools/skill-evals/evals/setup" + + +def test_the_eval_wrapper_spelled_with_tilde_is_allowed(): + assert agent_guard.dispatch(f"~/.claude/scripts/magpie-run-evals.sh {EVALS}") is None + + +@pytest.mark.parametrize( + "head", + [ + "/Users/alice/.claude/scripts/magpie-run-evals.sh", + "/home/alice/.claude/scripts/magpie-run-evals.sh", + "$HOME/.claude/scripts/magpie-run-evals.sh", + '"~/.claude/scripts/magpie-run-evals.sh"', + "\\~/.claude/scripts/magpie-run-evals.sh", + "tools/skill-evals/magpie-run-evals.sh", + "./magpie-run-evals.sh", + # Right spelling behind a wrapper: the exclusion matches the line's start. + "env ~/.claude/scripts/magpie-run-evals.sh", + "command ~/.claude/scripts/magpie-run-evals.sh", + ], +) +def test_the_eval_wrapper_spelled_otherwise_is_refused(head): + reason = agent_guard.dispatch(f"{head} {EVALS}") + assert reason and "~/.claude/scripts/magpie-run-evals.sh" in reason + + +@pytest.mark.parametrize("prefix", ["env ", "command ", "env -i "]) +def test_a_wrapped_vetted_launch_is_refused_even_with_the_tilde(prefix): + reason = agent_guard.dispatch(f"{prefix}uv run --project ~/.claude/magpie/vetted-ops vetted-op-read x") + assert reason and "whole command" in reason + + +def test_the_eval_wrapper_override(): + assert ( + agent_guard.dispatch( + f"MAGPIE_ALLOW_SCRIPT_PATH=1 /Users/a/.claude/scripts/magpie-run-evals.sh {EVALS}" + ) + is None + ) diff --git a/tools/agent-guard/tests/test_skill_guards.py b/tools/agent-guard/tests/test_skill_guards.py index 5eaf23bf3..889513400 100644 --- a/tools/agent-guard/tests/test_skill_guards.py +++ b/tools/agent-guard/tests/test_skill_guards.py @@ -544,3 +544,21 @@ def test_security_language_in_comment_allowed(): def test_security_override(): cmd = 'MAGPIE_ALLOW_SECURITY_LANG=1 gh pr create --title "Fix CVE-2026-1234" --body "x"' assert dispatch(cmd) is None + + +@pytest.mark.parametrize( + "prefix", ["env ", "env -i ", "env FOO=1 ", "command ", "env command ", "env -S 'gh"] +) +def test_prefix_wrappers_reach_the_mention_guard(monkeypatch, prefix): + monkeypatch.setattr(agent_guard, "_run", fake_run(gh_stub(author="alice"))) + if prefix.endswith("'gh"): + command = f"""{prefix} pr comment 5 --body @bob'""" + else: + command = f'{prefix}gh pr comment 5 --body "@bob please review"' + reason = dispatch(command) + assert reason and "bob" in reason and "mention" in reason + + +def test_an_override_on_env_still_counts(monkeypatch): + monkeypatch.setattr(agent_guard, "_run", fake_run(gh_stub(author="alice"))) + assert dispatch('env MAGPIE_ALLOW_MENTIONS=1 gh pr comment 5 --body "@bob ping"') is None From 6acbc281d8283cb6054d6dc3dcedd8a115e51035 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sat, 17 Oct 2026 19:50:09 +0200 Subject: [PATCH 2/2] fix(agent-guard): unwrap env/command completely; accept only the bare spelling Closes the bypasses a security review found in the previous commit: - Unwrapping stopped after eight nested wrappers and left `env` as the head, so nine wrappers reached no guard. It now runs until no wrapper is left; a line deeper than a generous limit, an `env` option that is not documented, a `command` flag other than -p, or a -S string that does not tokenise marks the segment unparsed, and dispatch refuses it. - `env` options are parsed as GNU / BSD env documents them: clustered short flags (`-iu NAME`, `-uNAME`, `-iCdir`, `-iS'...'`), long forms with `=` or a separate value, and the signal options. - A `~` spelling is accepted only when the raw line carries it bare and space-separated: a quote after the `~` (`~"/..."`) or the `--project=~/...` form leaves the `~` unexpanded as surely as a leading quote does. Generated-by: Claude Opus 5 --- .../agent-guard/src/agent_guard/__init__.py | 162 +++++++++++++----- .../setup_preflight/isolated_fingerprint.py | 2 +- tools/agent-guard/src/agent_guard/__init__.py | 162 +++++++++++++----- tools/agent-guard/tests/test_guards.py | 53 +++++- 4 files changed, 294 insertions(+), 85 deletions(-) diff --git a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py index 631938509..fd9255962 100644 --- a/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py +++ b/plugins/magpie-agent-guard/tools/agent-guard/src/agent_guard/__init__.py @@ -131,6 +131,17 @@ _ASSIGNMENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*=.*", re.DOTALL) +# `env` options (GNU coreutils and BSD): flags without a value, long options +# whose value is optional (`--default-signal[=SIG]`), and — handled in +# `_env_inner` — `-u`/`--unset`, `-C`/`--chdir`, `-S`/`--split-string`, which +# take one. Any other option leaves the command unparsed, and refused. +_ENV_SHORT_FLAGS = frozenset("i0v") +_ENV_LONG_FLAGS = frozenset({"ignore-environment", "null", "debug", "list-signal-handling"}) +_ENV_LONG_OPTIONAL = frozenset({"block-signal", "default-signal", "ignore-signal"}) +# Wrappers unwrapped per segment before the command counts as unparsed. Each +# step consumes input, so only a crafted line ever reaches this. +_UNWRAP_LIMIT = 64 + class Segment: """One simple command from a (possibly compound) shell line. @@ -146,6 +157,8 @@ def __init__(self, tokens: list[str], raw: str, command: str = "") -> None: #: exclusions match a command line by its start, so a wrapped excluded #: command runs sandboxed even when the command itself is spelled right. self.wrapped = False + #: True when a wrapper could not be parsed; `dispatch` refuses the line. + self.unparsed = False argv = self._unwrap(self._take_assignments(tokens)) #: The command head as written (`~/.claude/scripts/x.sh`, `/usr/bin/git`), #: for the rules that care how a command is spelled, not just its name. @@ -174,52 +187,101 @@ def _take_assignments(self, tokens: list[str]) -> list[str]: def _unwrap(self, argv: list[str]) -> list[str]: """Look through prefix wrappers that run their argument as the command: - ``env [-i] [-u NAME] [NAME=value…] cmd …`` (its assignments join - ``env``, so an inline override still counts) and ``command [-p] cmd …``. - Without this, ``env git commit …`` reached no guard at all.""" - for _ in range(8): # nested wrappers (`env command git …`), bounded - if not argv: + ``env [options] [NAME=value…] cmd …`` (its assignments join ``env``, so + an inline override still counts) and ``command [-p] cmd …``. + + Unwrapping runs until no wrapper is left — every step consumes input, so + it ends — and anything it cannot parse sets ``unparsed``, which + ``dispatch`` refuses: a wrapper is never a way past the guards. + """ + steps = 0 + while argv: + steps += 1 + if steps > _UNWRAP_LIMIT: + self.unparsed = True return argv head = os.path.basename(argv[0]) - if head in ("env", "command") and not (head == "command" and argv[1:2] in (["-v"], ["-V"])): - self.wrapped = True if head == "env": - i = 1 - while i < len(argv): - tok = argv[i] - if tok in ("-u", "--unset", "-C", "--chdir"): - i += 2 - elif tok in ("-S", "--split-string") and i + 1 < len(argv): - # `env -S "git commit …"`: the string is the command. - argv = [*shlex.split(argv[i + 1]), *argv[i + 2 :]] - i = 0 - break - elif tok.startswith("--split-string=") or (tok.startswith("-S") and len(tok) > 2): - packed = tok.split("=", 1)[1] if tok.startswith("--") else tok[2:] - argv = [*shlex.split(packed), *argv[i + 1 :]] - i = 0 - break - elif tok == "--": - i += 1 - break - elif tok.startswith("-"): - i += 1 - else: - break - else: - return [] - argv = self._take_assignments(argv[i:]) + inner = self._env_inner(argv[1:]) + if inner is None: + self.unparsed = True + return argv + self.wrapped = True + argv = inner elif head == "command": rest = argv[1:] - if rest[:1] and rest[0] in ("-v", "-V"): - return argv # a lookup, not an execution - while rest[:1] and rest[0] in ("-p", "--"): - rest = rest[1:] + while rest and rest[0].startswith("-") and rest[0] != "-": + flag = rest.pop(0) + if flag == "--": + break + letters = flag[1:] + if any(ch in "vV" for ch in letters): + return argv # `command -v x`: a lookup, not an execution + if any(ch != "p" for ch in letters): + self.unparsed = True + return argv + self.wrapped = True argv = rest else: return argv return argv + def _env_inner(self, args: list[str]) -> list[str] | None: + """The command ``env`` runs, after its options and assignments; None + when an option is not one GNU / BSD ``env`` documents, or a ``-S`` + string does not tokenise.""" + try: + return self._env_inner_parse(list(args)) + except ValueError: + return None + + def _env_inner_parse(self, args: list[str]) -> list[str] | None: + while args: + tok = args[0] + if tok == "--": + args.pop(0) + break + if tok.startswith("--"): + name, eq, value = tok[2:].partition("=") + if name in _ENV_LONG_FLAGS or name in _ENV_LONG_OPTIONAL: + args.pop(0) + continue + if name in ("unset", "chdir"): + args.pop(0) + if not eq: + if not args: + return None + args.pop(0) + continue + if name == "split-string": + args.pop(0) + if not eq: + if not args: + return None + value = args.pop(0) + args = [*shlex.split(value), *args] + continue + return None + if tok.startswith("-") and tok != "-": + args.pop(0) + letters = tok[1:] + for pos, ch in enumerate(letters): + if ch in _ENV_SHORT_FLAGS: + continue + if ch in "uCS": + attached = letters[pos + 1 :] + if not attached: + if not args: + return None + attached = args.pop(0) + if ch == "S": + args = [*shlex.split(attached), *args] + break + return None + continue + break + return self._take_assignments(args) + def override(self, *names: str) -> bool: """True if any of ``names`` (or the global off switch) is set truthy, either as an inline env assignment on this segment or in the hook's own @@ -1173,8 +1235,11 @@ def guard_excluded_script_spelling(seg: Segment) -> str | None: stable = EXCLUDED_SCRIPTS.get(seg.argv[0]) if stable is None: return None - quoted = re.search(r"""(?:^|[\s;&|(])(?:["']|\\)~/\.claude/scripts/""", seg.command) - if seg.head == stable and not quoted and not seg.wrapped: + # Accept only the bare spelling as written: tokenising drops quotes, and a + # quote anywhere in a `~` prefix (`"~/…"`, `~"/…"`, `\\~/…`) stops the shell + # from expanding it, so the line itself must carry `~/.claude/scripts/`. + bare = re.search(rf"(?:^|[\s;&|(]){re.escape(stable)}(?=\s|$)", seg.command) + if seg.head == stable and bare and not seg.wrapped: return None if seg.override("MAGPIE_ALLOW_SCRIPT_PATH"): return None @@ -1186,8 +1251,17 @@ def guard_excluded_script_spelling(seg: Segment) -> str | None: ) -#: `--project "~/…"`, `--from='~/…'`, `--project \~/…`: a `~` the shell leaves alone. -_QUOTED_TILDE_RE = re.compile(r"""(?:--project|--from)(?:=|\s+)(?:["']|\\)~""") +def _bare_base(command: str, option: str, base: str) -> bool: + """True when ``command`` carries ``