You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/09/09 21:53:09 UTC
(magpie) branch main updated: feat(vetted-ops): publish as a substrate plugin, with the privilege boundary on the entry point (#1188)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie.git
The following commit(s) were added to refs/heads/main by this push:
new a1d9e240 feat(vetted-ops): publish as a substrate plugin, with the privilege boundary on the entry point (#1188)
a1d9e240 is described below
commit a1d9e240013f88c929aa7a11e184394662257432
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Wed Sep 9 23:53:03 2026 +0200
feat(vetted-ops): publish as a substrate plugin, with the privilege boundary on the entry point (#1188)
* feat(vetted-ops): publish as a substrate plugin and wire its exclusion
The vetted-ops dispatcher trades a dozen wildcard `ask` rules for one
`allow`. That trade only holds while the agent holding the `allow`
cannot rewrite what it is permitted to do — but the tool shipped only
in the framework checkout, so adopters installing from the marketplace
had no copy of it at all, and anyone vendoring it into their own repo
put the operation catalogue inside the tree the agent edits.
Publish it as the `magpie-vetted-ops` substrate plugin, the pattern
`magpie-agent-guard` already established: the tool is reached through a
narrow symlink from the installed plugin root, which is neither a path
the agent edits nor inside any sandbox write root.
Substrate plugins previously all carried a hook, and the generator
required one. Publishing a tool from the plugin root is the general
reason to be a substrate plugin; wiring a hook is one instance of it.
Make `hooks` optional so a dispatcher can be published without
pretending to be a hook, and make the must-resolve diagnostic name the
right consequence for each shape.
Wire the exclusion through isolated setup — proposed with the `allow`
at install, drift-checked on update, and check 9 on verify. It covers
two surfaces, because they are two halves of one bound: an agent that
adds an op to the catalogue and an agent that adds itself to a caller
list in the policy arrive at the same place.
The docs state the asymmetry rather than rounding it off: the catalogue
is protected twice (deny rules, plus sitting outside every allowWrite
root), the policy only once. The policy lives in the sandbox-writable
project root, so a Bash-level write slips past an Edit/Write deny. That
is a real limit of the mechanism and adopters need it to judge the
trade, so it is documented, not implied.
* fix(vetted-ops): put the privilege boundary on the entry point, not --caller
The packaging commit wired an `allow` for the dispatcher on the strength of a
read-only *caller name*. That does not hold. `--caller` is an argv string
chosen by whoever runs the command, so an agent handed that `allow` reaches
every operation in the catalogue by naming a different caller — including
issue-close and the review submissions. It converts a confirmed `gh` write into
an unprompted one, which is worse than the wildcard `ask` rules it replaced.
The tool's own README already said per-caller scoping was not a boundary; the
recommendation was written as if it were.
Split the dispatcher instead. `vetted-op-read` refuses any operation with
writes=True before it consults the policy or looks at --caller, so no argv can
argue it into a mutation, and it is safe to allowlist outright. `vetted-op`
keeps its confirmation. A permission rule keys on the command, and argv cannot
change which binary is running — so the entry point can carry a boundary that a
parameter never could.
Repeated --caller is now an error. argparse keeps the last occurrence, so
`--caller read-only … --caller privileged` would match a rule written against
the read-only prefix while resolving to the privileged entry: a bypass of
exactly the mechanism this commit adds.
Body files were the other half. Validating a path and handing it to `gh` left
the checked file and the published file free to differ. read_body now requires
a workspace this user owns and that group and world cannot write, refuses
symlinks (O_NOFOLLOW, plus realpath for the parents), reads the bytes once, and
pipes them to `gh` on stdin. The bytes that were validated are the bytes that
get sent.
Docs say which dispatcher may be allowlisted and why the other may not, and
name per-caller scoping as least-privilege hygiene rather than isolation —
that misreading is what produced the unsafe rule in the first place.
* docs(rfc-ai-0002): record the vetted-ops layer and its harness portability
RFC-AI-0002 described four layers and stopped at `permissions.ask`. Layer 3's
real failure mode is not its rules but its volume: a wildcard that prompts for
`gh issue view` as loudly as for `gh issue close` produces a hundred prompts on
a thirty-tracker sweep, and the hundredth gets the attention the first deserved.
Prompt fatigue is the mechanism by which Layer 3 stops working, so the tool that
addresses it belongs in the RFC rather than only in its own README.
Record it as Layer 3a, and state where the boundary sits. `--caller` is an
argument, so it can never be one — on any runtime. What every harness can bind
is the command, which is why the dispatcher is split into a read entry point
that refuses writes structurally and a write entry point that keeps its
confirmation.
The harness table is the point of the section: the split is portable precisely
because it asks each runtime for a permission decision keyed on a command
string, which all of them have, and asks none of them to bind a decision to
which skill is calling, which none of them offer. Claude Code, OpenCode, Kiro
and Codex each express it in two rules, with the matching semantics that matter
noted per harness — last-match-wins on OpenCode, anchored regex on Kiro, and
Kiro's prompt-by-default making omission the safe state.
Two residual risks join the list, both previously unstated: a session is a
single principal, so argument-expressed scoping is advisory everywhere; and the
policy file sits in the project tree, which a sandboxed shell can write. The
second is survivable only because of the first fix — the read dispatcher ignores
policy when refusing writes — and saying so is the point. The open question is
whether per-skill scope can ever become a real boundary, which needs a runtime
primitive no shipping harness has.
---
.claude-plugin/marketplace.json | 6 +
docs/rfcs/RFC-AI-0002.md | 95 +++++++++++++
docs/setup/secure-agent-setup.md | 61 +++++++-
.../magpie-vetted-ops/.claude-plugin/plugin.json | 12 ++
plugins/magpie-vetted-ops/tools/vetted-ops | 1 +
skills/setup-isolated-setup-install/SKILL.md | 62 +++++++++
skills/setup-isolated-setup-update/SKILL.md | 27 ++++
skills/setup-isolated-setup-verify/SKILL.md | 40 +++++-
tools/dev/check-family-plugins.py | 36 ++++-
tools/vetted-ops/README.md | 104 +++++++++++---
tools/vetted-ops/pyproject.toml | 1 +
tools/vetted-ops/src/vetted_ops/__init__.py | 4 +-
tools/vetted-ops/src/vetted_ops/cli.py | 89 ++++++++++--
tools/vetted-ops/src/vetted_ops/ops.py | 70 ++++++++--
tools/vetted-ops/tests/test_vetted_ops.py | 155 ++++++++++++++++++++-
15 files changed, 704 insertions(+), 59 deletions(-)
diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json
index 769c6ed9..d80b4d72 100644
--- a/.claude-plugin/marketplace.json
+++ b/.claude-plugin/marketplace.json
@@ -18,6 +18,12 @@
"version": "0.2.0.dev202609090002",
"description": "Apache Magpie \u2014 deterministic pre-execution command guard: a PreToolUse hook that denies shell commands which would break a hard framework rule. Runs from the installed plugin, so no repository or worktree needs a local copy."
},
+ {
+ "name": "magpie-vetted-ops",
+ "source": "./plugins/magpie-vetted-ops",
+ "version": "0.2.0.dev202609090002",
+ "description": "Apache Magpie \u2014 vetted-ops: a dispatcher for fixed, policy-scoped forge operations, so a session needs one allowlist entry instead of a dozen wildcard `ask` rules. Runs from the installed plugin, which is what keeps the operation catalogue out of reach of the agent that calls it."
+ },
{
"name": "magpie-contributor-growth",
"source": "./plugins/magpie-contributor-growth",
diff --git a/docs/rfcs/RFC-AI-0002.md b/docs/rfcs/RFC-AI-0002.md
index b9326807..12c54599 100644
--- a/docs/rfcs/RFC-AI-0002.md
+++ b/docs/rfcs/RFC-AI-0002.md
@@ -98,6 +98,7 @@ The proposal in this RFC reduces the risk surface from *"anything reachable fro
| **1. Filesystem sandbox** | Claude Code's `sandbox.enabled: true` + bubblewrap (Linux) / Seatbelt (macOS) | Bash subprocess reads outside the project tree. |
| **2. Tool permissions** | Claude Code's `permissions.deny` for Read/Edit/Write/Bash | The agent's own tools cat-ing dotfiles or running `aws`/`curl`. |
| **3. Forced confirmation** | Claude Code's `permissions.ask` | Visible-to-others writes (`git push`, `gh pr create`, …) without an explicit yes. |
+| **3a. Bounded operations** | `vetted-ops` split dispatcher | Prompt fatigue at Layer 3 — read traffic stops competing with writes for the operator's attention. |
Layers 1, 2, and 3 are configured by the same project-scope `.claude/settings.json`. Layer 0 lives in the developer's shell. Two **visibility** mechanisms (a status-line indicator and a per-call bypass-warn hook) sit alongside the four layers; they do not enforce policy themselves but make the policy continuously legible.
@@ -213,6 +214,76 @@ The deny / allow split for `~/.config/gh/` and `~/.config/<project>/` is del
The `permissions.ask` block above intercepts every write-side action whose effect is **visible to others** — a `git push`, a `gh pr create`, a `gh issue comment`, a `gh release create`. Ask-rules do not block; they make the agent surface the exact command and require explicit human approval before running it. This closes the "agent ran a `git push` for me before I noticed" class of regressions.
+#### Layer 3a — Bounded operations (the `vetted-ops` dispatcher)
+
+Layer 3's weakness is not its rules, it is its **volume**. A wildcard like
+`Bash(gh *)` is doing real work — every unknown `gh` subcommand prompts — but it
+prompts for `gh issue view` as loudly as for `gh issue close`. On a sweep across
+thirty trackers that is a hundred prompts, and the hundredth gets the attention
+the first deserved. Prompt fatigue is not a usability complaint here; it is the
+mechanism by which Layer 3 stops working.
+
+[`tools/vetted-ops`](https://github.com/apache/magpie/blob/main/tools/vetted-ops)
+narrows the surface so read traffic can leave the prompt stream without the
+writes following it. Every operation is a closed shape — a name, typed
+parameters, and a builder returning an argv list executed without a shell — so a
+parameter can never become a command, a flag, or a different repository.
+
+**Where the boundary sits, and why it cannot sit anywhere else.** The dispatcher
+takes a `--caller` naming the invoking skill, and the policy declares which
+operations each caller may run. It is tempting to treat that as the boundary and
+allowlist the dispatcher on the strength of a read-only caller name. **That is
+wrong, on every harness.** `--caller` is an argument, and arguments are chosen by
+whoever runs the command; an agent that can pass one caller name can pass
+another. Per-caller scoping is least-privilege hygiene for a *cooperating* skill
+— worth having, since the common failure is an honest one — but it stops nothing
+that decides to name a different caller.
+
+What every harness *can* bind is the **command**. So the dispatcher ships as two
+console scripts over one catalogue:
+
+| Entry point | Can write? | Intended disposition |
+|---|---|---|
+| `vetted-op-read` | never — refused before the policy or `--caller` is consulted | allow, unattended |
+| `vetted-op` | yes, subject to policy | confirm, per call |
+
+The read entry point's refusal is structural: it does not consult the config, so
+no policy edit and no argument can reach a mutation through it. That is what
+makes allowlisting it defensible, and it is the whole reason the split exists.
+
+Two supporting details matter enough to state. A repeated `--caller` is an error
+rather than a last-wins convenience, because argument parsers conventionally keep
+the last occurrence — so `--caller read-only … --caller privileged` would satisfy
+a rule written against the read-only form while resolving to the privileged one.
+And bodies for write operations are read **once, by the dispatcher**, from a
+workspace the operator owns that group and world cannot write, then piped to the
+forge CLI on stdin: handing the CLI a path would leave the validated file and the
+published file free to differ.
+
+##### Applicability across harnesses
+
+The split is portable because it asks each harness for the one thing they all
+have — a permission decision keyed on the command string — and asks none of them
+for the thing none of them offer, which is binding a decision to *which skill is
+calling*. That asymmetry is why `--caller` cannot be the boundary on any runtime,
+not only on Claude Code.
+
+| Harness | Mechanism | Shape |
+|---|---|---|
+| **Claude Code** | `permissions.allow` / `ask` | `allow` the `vetted-op-read` prefix; leave `vetted-op` in `ask`. Rules are prefix-matched, and `vetted-op ` — with its trailing space — does not match `vetted-op-read`, so the two do not collide. |
+| **OpenCode** | `permission` policy ([docs](https://opencode.ai/docs/permissions/)) | The same split as a per-command decision map. Evaluation is **last-match-wins**, so the `vetted-op` confirm entry must not be shadowed by a broader `allow` appearing later. |
+| **Kiro** | `allowedCommands` / `deniedCommands` ([docs](https://kiro.dev/docs/cli/reference/built-in-tools#execute-shell-commands)) | Patterns are full-string-anchored regex and deny is evaluated first, so an anchored `vetted-op-read` pattern cannot leak to `vetted-op`. Kiro prompts by default, so the write dispatcher needs no rule at all — omitting it *is* the safe state. |
+| **Codex** | exec-policy rules | `allow` the read dispatcher; the write dispatcher takes `prompt`, alongside the existing rules forcing `prompt` on `gh` mutations. |
+| **Any other runtime** | `agent-guard --exec` | Harness-neutral command gating; the two program names are what the gate keys on. |
+
+Each is a permission-config change, not a code change: the dispatcher is
+harness-agnostic, and adopting it on a new runtime means writing two rules.
+
+The invariant a reviewer checks, on any harness, is short — **the write
+dispatcher must never appear in an unattended-allow position.** A read-only
+caller name sitting next to such a rule is not a mitigating factor; it is the
+misreading that produces the rule.
+
### Visibility — sandbox-bypass warning hook
Claude Code's Bash tool accepts a `dangerouslyDisableSandbox: true` flag that lets the model run a single command outside the sandbox — necessary for the (rare) cases where a legitimate task needs to read or write a path the sandbox denies. The runtime prompts the user before honouring the bypass, but in a long session the prompt is easy to skim past, especially when several appear in quick succession.
@@ -467,6 +538,22 @@ This setup substantially shrinks the credential-leakage surface, but some risks
- **Secrets in the project tree.** If a tracker issue body, a comment, or a committed file contains a secret, the agent's Read tool surfaces it to the context window. No layer above can prevent that once a Read happens. Mitigation: project-level policy that secrets never land in the tracker repo.
- **Domain fronting / CDN abuse via allow-listed hosts.** The `sandbox.network.allowedDomains` allowlist matches by SNI; an attacker who can publish content on `*.githubusercontent.com` could in principle exfiltrate via that channel. Mitigation: keep the allowlist as tight as actual usage and audit it whenever a new tool / SKILL is added.
+- **A single agent session is one principal.** Nothing in this setup separates
+ a read-only assessor subagent from the orchestrator that dispatched it: both
+ run under the same permission rules, so any scoping expressed in *arguments*
+ (the `vetted-ops` `--caller`, a skill name, a role string) is advisory. Layer
+ 3a works by giving the read path its own **command**, which the harness can
+ bind; it does not create per-skill principals, and no shipping runtime offers
+ a portable way to. Mitigation: keep unattended-allow rules to entry points
+ that cannot mutate, and treat per-caller policy as hygiene rather than
+ containment.
+- **The `vetted-ops` policy file lives in the project tree.** It is therefore
+ writable by a sandboxed shell, so an `Edit`/`Write` deny on it is only a
+ partial protection. This is survivable because the read dispatcher refuses
+ writes without consulting policy — the worst a policy rewrite buys is
+ redirecting a *read* — but the file's contents should be read as advisory
+ rather than enforced. Mitigation: for adopters who need more, keep the policy
+ outside every sandbox write root and pass `--config`.
- **MCP servers configured at user scope.** Claude Code does not isolate user-scope MCP servers from the project session — their tokens and tools come along. Mitigation: audit `~/.claude/.mcp.json` and `~/.claude.json` quarterly; remove any MCP server you don't actively use.
## Open questions
@@ -474,6 +561,14 @@ This setup substantially shrinks the credential-leakage surface, but some risks
- **Should this RFC apply ASF-wide, or only to projects handling pre-disclosure / embargoed content?** The threat model is general, but the cost of adoption is non-zero, and projects whose tracker repos contain only ordinary public source code may reasonably defer.
- **Should the framework provide a one-shot installer that abstracts the agent-guided skills behind a single command?** Trade-off: easier adoption vs. operator visibility into what is being changed.
- **macOS network egress.** A future enhancement could wrap macOS Bash subprocesses in a `sandbox-exec` profile that also restricts outbound `network*` operations the way the current profile restricts `file-read*`. Open follow-up.
+- **Can per-skill scope ever become a real boundary?** Layer 3a binds *what may
+ be run*, not *who is running it*, because the command string is the only thing
+ every harness can key on. A genuine per-skill principal needs the runtime to
+ bind a permission set to a subagent or a plugin — plugin manifests cannot
+ declare permissions today, and the nearest primitive (a subagent with a
+ restricted tool list) is Claude Code-specific. Worth revisiting whenever a
+ second runtime ships something equivalent; until then, adding caller names to
+ a policy should not be described as isolation.
- **Should the pinned-version manifest be a per-project artifact, or a foundation-wide canonical list?** The current per-project shape lets each project adopt at its own cadence; a foundation-wide list would centralise the cooldown discipline but add coordination overhead.
## Prior art and references
diff --git a/docs/setup/secure-agent-setup.md b/docs/setup/secure-agent-setup.md
index e7d21ff2..5ec7c284 100644
--- a/docs/setup/secure-agent-setup.md
+++ b/docs/setup/secure-agent-setup.md
@@ -541,7 +541,15 @@ below, annotated.
"Bash(gh workflow view *)", "Bash(gh workflow list *)",
"Bash(gh release view *)", "Bash(gh release list *)",
"Bash(gh label list *)", "Bash(gh cache list *)",
- "Bash(gh search *)", "Bash(gh browse *)", "Bash(gh auth status*)"
+ "Bash(gh search *)", "Bash(gh browse *)", "Bash(gh auth status*)",
+ // The vetted-ops READ dispatcher. Safe to `allow` rather than `ask`
+ // because it refuses every write operation before it consults policy or
+ // --caller, so no argv can talk it into a mutation. The write dispatcher
+ // (`vetted-op`) is deliberately NOT here — it is in `ask` below.
+ // Allowlisting it on the strength of a read-only *caller name* would
+ // grant the whole catalogue, because --caller is chosen by the caller.
+ // The version segment is globbed: the plugin cache is versioned per install.
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op-read *)"
],
"deny": [
"Read(~/.aws/**)", "Read(~/.ssh/**)", "Read(~/.netrc)",
@@ -554,10 +562,24 @@ below, annotated.
"Bash(aws *)", "Bash(gcloud *)", "Bash(az *)", "Bash(kubectl *)",
"Bash(docker login *)", "Bash(npm publish *)",
"Bash(pip install --upgrade *)", "Bash(uv self update *)",
- "Bash(gh auth token*)", "Bash(gh auth refresh*)" // gh runs unsandboxed (excludedCommands), so deny the two subcommands that would print/rotate the token
+ "Bash(gh auth token*)", "Bash(gh auth refresh*)", // gh runs unsandboxed (excludedCommands), so deny the two subcommands that would print/rotate the token
+ // The vetted-ops exclusion. The read dispatcher's `allow` rests on the
+ // catalogue's shape — which operations exist and which of them write —
+ // so the catalogue must not be editable by the agent that calls it.
+ // It gets two layers: these rules, and sitting outside every
+ // sandbox.filesystem.allowWrite root.
+ // The policy TOML gets one, because it lives inside the adopter repo and
+ // is therefore sandbox-writable. That is tolerable only because the read
+ // dispatcher ignores policy when refusing writes: editing the policy can
+ // widen which repo is READ, never turn a read into a write.
+ "Edit(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ "Write(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ "Edit(.apache-magpie-overrides/tools/vetted-ops/**)",
+ "Write(.apache-magpie-overrides/tools/vetted-ops/**)"
],
"ask": [
"Bash(git push *)", // including --force / --force-with-lease variants
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op *)", // the vetted-ops WRITE dispatcher: bounded in shape, but still a remote mutation, so it keeps a confirmation
"Bash(gh *)" // safe-by-default: EVERY gh command prompts unless it matches a more-specific read-only allow rule above. Guarantees every destructive / unknown gh subcommand (gh pr close, gh run delete, gh label delete, gh repo archive, gh variable set, gh project item-delete, …) is confirmed. `gh auth token`/`refresh` are denied above (deny > ask).
]
}
@@ -571,6 +593,29 @@ agent should never *see* it. `sandbox.filesystem.allowRead` permits
the bash subprocess to read the file; `permissions.deny[Read(...)]`
blocks the agent's Read tool from reading the same path.
+**What the vetted-ops exclusion does and does not cover.** The `deny`
+rules block the agent's own `Edit`/`Write` tools. The catalogue gets
+a second, independent layer for free: the plugin cache sits outside
+every `sandbox.filesystem.allowWrite` root, so a sandboxed `Bash`
+call cannot write it either. The **policy TOML has no such second
+layer** — it lives inside the adopter repo, which is sandbox-writable
+by design, so a Bash-level write (`sed -i`, a heredoc redirect) would
+slip past the `Edit`/`Write` deny.
+
+That asymmetry is survivable only because of where the privilege
+boundary sits. `vetted-op-read` refuses a write **before** it reads
+the policy, so rewriting the policy cannot convert a read into a
+write; the worst it buys is pointing a read at a different
+repository. Had the `allow` been written against `vetted-op` — the
+write dispatcher — with a read-only *caller name* as the supposed
+constraint, the policy file would have been load-bearing and its one
+layer would not have been enough. `--caller` is argv, and argv
+belongs to whoever runs the command.
+
+If even the read-widening matters for your threat model, keep the
+policy in a path the sandbox does not grant write to and point
+`--config` at it.
+
**OpenCode parity.** OpenCode has no per-command sandbox exclusion — its
isolation is the OS-level sandbox of the [clean-env wrapper](#the-clean-env-wrapper),
which already runs `gh` with the host keyring, so there is no `excludedCommands`
@@ -2122,6 +2167,18 @@ below and report ✓ done / ✗ missing / ⚠ partial, with the evidence
`[NO SANDBOX]`).
7. Run `cat ~/.aws/credentials`, `echo $AWS_ACCESS_KEY_ID`, and
`curl https://example.com` and confirm each is denied.
+8. The **vetted-ops split and exclusion**. Two things, and the
+ first matters more:
+ - Only `vetted-op-read` is in `permissions.allow`. If
+ `vetted-op` (the write dispatcher) appears in `allow`, that
+ is ✗ and worth stopping for: it grants every operation in
+ the catalogue, because the operation's caller name is chosen
+ by whoever runs the command.
+ - `permissions.deny` denies `Edit` and `Write` on both
+ `~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**`
+ (the catalogue) and
+ `.apache-magpie-overrides/tools/vetted-ops/**` (the policy).
+ If the repo has no vetted-ops policy at all, report n/a.
```
Re-run either form after every Claude Code upgrade — the sandbox
diff --git a/plugins/magpie-vetted-ops/.claude-plugin/plugin.json b/plugins/magpie-vetted-ops/.claude-plugin/plugin.json
new file mode 100644
index 00000000..e1c47993
--- /dev/null
+++ b/plugins/magpie-vetted-ops/.claude-plugin/plugin.json
@@ -0,0 +1,12 @@
+{
+ "name": "magpie-vetted-ops",
+ "description": "Apache Magpie \u2014 vetted-ops: a dispatcher for fixed, policy-scoped forge operations, so a session needs one allowlist entry instead of a dozen wildcard `ask` rules. Runs from the installed plugin, which is what keeps the operation catalogue out of reach of the agent that calls it.",
+ "version": "0.2.0.dev202609090002",
+ "author": {
+ "name": "Apache Magpie",
+ "url": "https://magpie.apache.org/"
+ },
+ "homepage": "https://magpie.apache.org/",
+ "repository": "https://github.com/apache/magpie",
+ "license": "Apache-2.0"
+}
diff --git a/plugins/magpie-vetted-ops/tools/vetted-ops b/plugins/magpie-vetted-ops/tools/vetted-ops
new file mode 120000
index 00000000..da51fe8a
--- /dev/null
+++ b/plugins/magpie-vetted-ops/tools/vetted-ops
@@ -0,0 +1 @@
+../../../tools/vetted-ops
\ No newline at end of file
diff --git a/skills/setup-isolated-setup-install/SKILL.md b/skills/setup-isolated-setup-install/SKILL.md
index 0fac1f7d..7fcf3c76 100644
--- a/skills/setup-isolated-setup-install/SKILL.md
+++ b/skills/setup-isolated-setup-install/SKILL.md
@@ -541,6 +541,68 @@ the same reason. The `post-checkout` git hook installed by
worktrees added via `git worktree add` after this install pass
inherit access automatically — no operator action needed.
+### Step V — The vetted-ops split and exclusion
+
+Only applies when the adopter routes forge operations through the
+`vetted-ops` dispatcher — i.e. the repo has
+`.apache-magpie-overrides/tools/vetted-ops/config.toml`, or the
+operator asks for the dispatcher to be wired now. If neither is
+true, skip this step and say so; do not create a policy the project
+has not asked for.
+
+There are two console scripts over one catalogue, and which of them
+gets the `allow` is the entire security question:
+
+| Entry point | Can write? | Permission |
+|---|---|---|
+| `vetted-op-read` | never — refused before policy or `--caller` is consulted | `allow` |
+| `vetted-op` | yes | `ask` |
+
+**Never propose an `allow` on `vetted-op`.** It is tempting, because
+the policy declares a read-only caller and the invocation names it.
+That reasoning is wrong: `--caller` is an argv string chosen by
+whoever runs the command, so an `allow` on the write dispatcher
+grants every operation in the catalogue no matter which caller the
+example names. The read dispatcher needs no such trust — it refuses
+writes structurally.
+
+```jsonc
+"permissions": {
+ "allow": [
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op-read *)"
+ ],
+ "ask": [
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op *)"
+ ],
+ "deny": [
+ // the operation catalogue — the read `allow` rests on its shape
+ "Edit(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ "Write(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ // the policy naming which caller may run which operation
+ "Edit(.apache-magpie-overrides/tools/vetted-ops/**)",
+ "Write(.apache-magpie-overrides/tools/vetted-ops/**)"
+ ]
+}
+```
+
+Tell the operator plainly what this buys and what it does not:
+
+- The catalogue is protected twice — by these rules, and by the
+ plugin cache sitting outside every `sandbox.filesystem.allowWrite`
+ root, so sandboxed Bash cannot write there either.
+- The policy TOML is protected **once**, and that is tolerable only
+ because the read dispatcher ignores policy when it refuses a
+ write. Editing the policy can widen which repo is *read*; it
+ cannot turn a read into a write.
+- Writes are not unattended. They keep the harness confirmation —
+ the dispatcher bounds their *shape*, not the operator's decision
+ to make them.
+
+Do not describe per-caller scoping as isolation. It is
+least-privilege hygiene for a cooperating skill, and it is worth
+having for that, but it stops nothing that chooses to name a
+different caller.
+
## After the install lands
Suggest two follow-up routines the user can wire later:
diff --git a/skills/setup-isolated-setup-update/SKILL.md b/skills/setup-isolated-setup-update/SKILL.md
index 686d8364..b87f397a 100644
--- a/skills/setup-isolated-setup-update/SKILL.md
+++ b/skills/setup-isolated-setup-update/SKILL.md
@@ -281,6 +281,33 @@ Walk each:
why). Report any newly-allowed call as a regression that
warrants attention.
+### The vetted-ops split and exclusion
+
+If the adopter uses the `vetted-ops` dispatcher (a
+`.apache-magpie-overrides/tools/vetted-ops/config.toml` exists, or
+`.claude/settings.json` carries a `vetted-op` rule), check two
+things.
+
+**First, and most important: has `vetted-op` drifted into `allow`?**
+Only `vetted-op-read` belongs there. The write dispatcher in an
+`allow` list grants the whole catalogue, because the caller name an
+invocation passes is chosen by whoever runs it. Report that as a
+must-fix, ahead of anything else in this section.
+
+**Second, `permissions.deny` still covers both surfaces**, each with
+`Edit` and `Write`:
+
+- `~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**`
+- `.apache-magpie-overrides/tools/vetted-ops/**`
+
+Report a missing rule as drift to repair, not a note. This is the
+check most likely to rot in practice: the plugin-cache path carries
+the plugin *name*, so a family rename or a move of the dispatcher to
+a different substrate plugin leaves a deny rule that still looks
+plausible but no longer matches anything. Resolve the glob against
+the installed tree and confirm it actually hits the catalogue,
+rather than eyeballing the string.
+
## After the report
If everything is in sync and verification still passes, say so
diff --git a/skills/setup-isolated-setup-verify/SKILL.md b/skills/setup-isolated-setup-verify/SKILL.md
index 5d413be7..e3d877eb 100644
--- a/skills/setup-isolated-setup-verify/SKILL.md
+++ b/skills/setup-isolated-setup-verify/SKILL.md
@@ -122,7 +122,7 @@ Drift severity:
path, the version string, the command output, the
`sandbox.enabled` value — never just "✓" or "✗" alone.
-## The 8 checks
+## The 9 checks
The canonical list lives in
[docs/setup/secure-agent-setup.md → Verification → Via a Claude Code prompt](../../docs/setup/secure-agent-setup.md#via-a-claude-code-prompt-1).
@@ -284,6 +284,44 @@ Walk each in order:
sub-check needed — the per-project mode is fully covered by
the static + live-probe checks above.
+9. **The vetted-ops split and exclusion.** Only meaningful when the
+ adopter routes forge operations through the `vetted-ops`
+ dispatcher; if the repo has no
+ `.apache-magpie-overrides/tools/vetted-ops/config.toml` and no
+ `vetted-op` rule, report **n/a** and move on.
+
+ **9a — which dispatcher is allowlisted.** This is the check that
+ matters. `permissions.allow` may contain `vetted-op-read` and
+ must **not** contain `vetted-op`. Finding the write dispatcher in
+ `allow` is ✗ and worth stopping the report to say so plainly: it
+ grants every operation in the catalogue, including `issue-close`
+ and every `pr-review-*`, with no confirmation. It looks safe
+ because the policy declares a read-only caller — but `--caller`
+ is an argv string chosen by whoever runs the command, so the
+ caller name in an example constrains nothing. Verify by
+ inspection, not by trusting a comment next to the rule.
+
+ `vetted-op` in `ask` (or absent) is correct.
+
+ **9b — the exclusion.** `permissions.deny` covers both surfaces,
+ each with `Edit` and `Write`:
+
+ - `~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**` —
+ the operation catalogue. The read dispatcher's `allow` rests on
+ its shape, so an editable catalogue dissolves that `allow`.
+ - `.apache-magpie-overrides/tools/vetted-ops/**` — the policy.
+
+ Any of the four missing is ✗.
+
+ Report two things as **notes**, not failures. The policy's
+ protection stops at the agent's editing tools — it sits in the
+ sandbox-writable project root, so a Bash-level write is not
+ covered; this is survivable only because the read dispatcher
+ refuses writes without consulting policy. And per-caller scoping
+ is least-privilege, not isolation: if the report describes it as
+ a boundary, correct that, because it is the misreading that
+ produces a `vetted-op` `allow` in the first place.
+
## After the report
If every check is ✓, say so explicitly and stop — no further
diff --git a/tools/dev/check-family-plugins.py b/tools/dev/check-family-plugins.py
index bcc24af7..402e86a8 100644
--- a/tools/dev/check-family-plugins.py
+++ b/tools/dev/check-family-plugins.py
@@ -151,6 +151,12 @@ DESC = {
# on every single Bash call. One dedicated owner runs it exactly once, whatever
# else is installed.
AGENT_GUARD_ENGINE = "tools/agent-guard/src/agent_guard/__init__.py"
+# The dispatcher's entry point. `vetted-ops` publishes no hook — it is invoked
+# from a skill's Bash call — but it is a substrate plugin for the *other* half of
+# the reason: the tool must live where the agent cannot rewrite it. An agent that
+# can edit `ops.py` has defeated the whole design, so the catalogue has to sit in
+# the installed plugin tree rather than in a consumer repository.
+VETTED_OPS_ENTRY = "tools/vetted-ops/src/vetted_ops/cli.py"
SUBSTRATE_PLUGINS: dict[str, dict] = {
"magpie-agent-guard": {
"description": (
@@ -177,6 +183,18 @@ SUBSTRATE_PLUGINS: dict[str, dict] = {
]
},
},
+ "magpie-vetted-ops": {
+ "description": (
+ "Apache Magpie \u2014 vetted-ops: a dispatcher for fixed, policy-scoped forge "
+ "operations, so a session needs one allowlist entry instead of a dozen wildcard "
+ "`ask` rules. Runs from the installed plugin, which is what keeps the operation "
+ "catalogue out of reach of the agent that calls it."
+ ),
+ "links": {"tools/vetted-ops": "vetted-ops"},
+ # The dispatcher is invoked directly by skills, so the entry point is what
+ # must resolve; there is no hook whose silence would hide a broken link.
+ "must_resolve": (VETTED_OPS_ENTRY,),
+ },
}
@@ -215,7 +233,13 @@ def substrate_manifest(name: str, shared: dict) -> dict:
"""The manifest `--fix` writes for a substrate plugin — the single source of
truth `check` compares the on-disk file against."""
spec = SUBSTRATE_PLUGINS[name]
- return {"name": name, "description": spec["description"], **shared, "hooks": spec["hooks"]}
+ manifest = {"name": name, "description": spec["description"], **shared}
+ # A substrate plugin exists to publish a tool from the installed plugin root.
+ # Wiring a hook is one reason to need that, not the only one, so a spec
+ # without `hooks` emits a manifest without the key rather than an empty one.
+ if "hooks" in spec:
+ manifest["hooks"] = spec["hooks"]
+ return manifest
def check_substrate(name: str, shared: dict) -> list[str]:
@@ -240,7 +264,7 @@ def check_substrate(name: str, shared: dict) -> list[str]:
errors.append(f"{path}: missing/empty 'description'")
if "skills" in data:
errors.append(f"{path}: substrate plugins ship a tool, not skills — drop 'skills'")
- if data.get("hooks") != spec["hooks"]:
+ if data.get("hooks") != spec.get("hooks"):
errors.append(
f"{path}: 'hooks' does not match the wiring the framework expects (regenerate with --fix)"
)
@@ -262,10 +286,12 @@ def check_substrate(name: str, shared: dict) -> list[str]:
# the file the command names is not reachable from the plugin root.
for rel in spec["must_resolve"]:
if not (pdir / rel).is_file():
- errors.append(
- f"{name}: {pdir / rel} does not resolve — the hook command names it, "
- f"so the guard would silently never run"
+ consequence = (
+ "the hook command names it, so the guard would silently never run"
+ if "hooks" in spec
+ else "the tool's entry point names it, so every call would fail to start"
)
+ errors.append(f"{name}: {pdir / rel} does not resolve — {consequence}")
return errors
diff --git a/tools/vetted-ops/README.md b/tools/vetted-ops/README.md
index 6ce7a805..74cd697e 100644
--- a/tools/vetted-ops/README.md
+++ b/tools/vetted-ops/README.md
@@ -9,7 +9,7 @@
- [Prerequisites](#prerequisites)
- [Why](#why)
- [What it actually guarantees](#what-it-actually-guarantees)
- - [What it does *not* guarantee](#what-it-does-not-guarantee)
+ - [The boundary is the entry point, not `--caller`](#the-boundary-is-the-entry-point-not---caller)
- [Configuration](#configuration)
- [CLI](#cli)
- [Wiring it into settings](#wiring-it-into-settings)
@@ -78,11 +78,20 @@ Being precise, because a security tool that overstates itself is worse than none
is read from policy. An operation cannot be pointed at another repository, and
parameters that reach an API *path* — repo paths and git refs — refuse `..`,
so none of them can walk out of the pinned repository either.
-- **Body content is free; body *location* is not.** Comment bodies are passed by
- file reference, so the text may contain anything — backticks, `$(…)`,
- newlines. What is constrained is which file may be read: it must resolve inside
- the configured workspace, so an operation cannot be talked into publishing
- `~/.ssh/id_rsa`.
+- **Body content is free; body *location* and *timing* are not.** Comment bodies
+ may contain anything — backticks, `$(…)`, newlines — because content is never
+ interpolated into a command. What is constrained is which file may be read and
+ when. The file must resolve inside the configured workspace, that workspace
+ must be owned by you and closed to group and world, and a symlink is refused
+ (`O_NOFOLLOW`, plus a realpath check for the parents). The dispatcher then
+ **reads the bytes itself and pipes them to `gh` on stdin**, so there is exactly
+ one open. Handing `gh` a path would leave a window between the check and the
+ send in which the file could be swapped; the bytes that were validated are the
+ bytes that get published.
+- **Writes cannot be reached through the read entry point.** `vetted-op-read`
+ refuses any operation with `writes=True` *before* it consults the policy or
+ looks at `--caller`, so the refusal cannot be argued out of it by argv. This
+ is the one property that makes an unattended `allow` defensible — see below.
- **GraphQL is named, not written.** `gh api graphql` normally takes a query as a
string — the widest surface `gh` offers. Here a caller names one of the
documents shipped in [`queries/`](src/vetted_ops/queries), the dispatcher
@@ -93,7 +102,7 @@ Being precise, because a security tool that overstates itself is worse than none
[`ops.py`](src/vetted_ops/ops.py) — a reviewed code change, not a runtime
decision.
-### What it does *not* guarantee
+### The boundary is the entry point, not `--caller`
**Per-caller scoping is least-privilege, not a security boundary.** The policy
declares which operations each caller may run, and `--caller` is required. But
@@ -101,17 +110,41 @@ within one agent session the agent is a single principal that chooses what to
pass as `--caller`. This protects against *the wrong skill reaching for the wrong
operation* — a real and common failure, especially under prompt injection, where
a hostile issue body talks a read-only triage pass into closing something. It
-does **not** stop a determined agent.
-
-A genuine boundary needs the runtime to bind scope to permissions. Today no
-portable mechanism exists: plugin manifests cannot declare permissions, and
-`permissionMode` is unsupported for plugin-shipped agents. The closest available
-primitive is a subagent with a restricted `tools` list, which is **Claude
-Code-specific** — the only other shipping runtime adapter is Codex
-(`experimental`). See
+does **not** stop a determined agent, and a permission rule must never be
+written as though it does. Concretely: allowlisting `vetted-op *` on the strength
+of a read-only caller name grants **every** operation in the catalogue, because
+the next invocation can name a different caller.
+
+What *can* be bound is the **entry point**, because a permission rule keys on the
+command and argv cannot change which binary is running. Hence two console
+scripts over one catalogue:
+
+| Entry point | Can write? | Intended permission |
+|---|---|---|
+| `vetted-op-read` | never — refused before policy is consulted | `allow` |
+| `vetted-op` | yes, subject to policy | `ask` (or unlisted) |
+
+That split is what lets the read path lose its prompts without the write path
+losing its gate, and it is why `--caller` repeated twice is an error rather than
+a last-wins convenience: argparse keeps the last occurrence, so repetition would
+let a command match a rule written against a read-only prefix while resolving to
+a privileged caller.
+
+For writes, the confirmation the harness puts in front of `vetted-op` is still
+doing the work. A finer boundary — per-skill scope bound by the runtime — needs
+something no portable mechanism offers yet: plugin manifests cannot declare
+permissions, and `permissionMode` is unsupported for plugin-shipped agents. The
+closest primitive is a subagent with a restricted `tools` list, which is **Claude
+Code-specific**. See
[`tools/spec-loop/specs/vetted-command-surface.md`](../spec-loop/specs/vetted-command-surface.md)
for that trajectory.
+**The policy file is only as protected as the filesystem makes it.** Rewriting
+the policy cannot reach a write through `vetted-op-read` — that refusal ignores
+the config entirely — but it can widen which repository the *reads* point at. If
+the policy lives somewhere the agent's shell can write, treat its contents as
+advisory rather than enforced.
+
**This is not a substitute for the sandbox.** It reduces prompt volume at Layer 3.
Layers 0–2 are unchanged and still carry the load.
@@ -197,18 +230,45 @@ underlying command failed.
Replace the wildcard `ask` rules with one `allow` entry, and keep an `ask` on
anything still invoked directly:
-```json
+```jsonc
"permissions": {
- "allow": [ "Bash(uv run --project <plugin>/tools/vetted-ops vetted-op *)" ]
+ // Only the READ dispatcher is allowlisted. Allowlisting `vetted-op` itself
+ // would grant every write in the catalogue, since --caller is argv.
+ "allow": [
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op-read *)"
+ ],
+ "ask": [
+ "Bash(uv run --project ~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/*/tools/vetted-ops vetted-op *)"
+ ],
+ "deny": [
+ "Edit(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ "Write(~/.claude/plugins/cache/apache-magpie/magpie-vetted-ops/**)",
+ "Edit(.apache-magpie-overrides/tools/vetted-ops/**)",
+ "Write(.apache-magpie-overrides/tools/vetted-ops/**)"
+ ]
}
```
The dispatcher must live where the agent cannot rewrite it — otherwise an agent
-that edits `ops.py` has defeated the whole design. Shipping it inside the
-installed plugin tree satisfies this: the plugin install is not a path the agent
-edits. If you vendor it into a repo the agent *does* edit, add a
-`permissions.deny` on `Write`/`Edit` for that path, and understand that the
-guarantee is then only as strong as that rule.
+that edits `ops.py` has defeated the whole design. That is why it ships as the
+`magpie-vetted-ops` **substrate plugin**: the installed plugin tree is not a path
+the agent edits, and it sits outside every `sandbox.filesystem.allowWrite` root,
+so sandboxed Bash cannot write there either.
+
+The `deny` rules above are the other half, and the `allow` is not defensible
+without them. Two surfaces define what the dispatcher may do — the operation
+catalogue and the **policy**, which names the caller→operation grants. An agent
+able to edit either one can grant itself the write access the wildcard `ask`
+rules were removed for; adding an op to the catalogue and adding itself to a
+caller list reach the same place.
+
+Be clear about the asymmetry: the catalogue is protected twice (deny rules plus
+the sandbox), the policy only once. The policy lives inside the adopter repo,
+which is sandbox-writable by design, so a Bash-level write (`sed -i`, a heredoc)
+is **not** covered by an `Edit`/`Write` deny. If that gap matters, keep the
+policy outside the writable root and point `--config` at it. Vendoring the
+dispatcher itself into a repo the agent edits reduces it to the same single
+layer.
## Tests
diff --git a/tools/vetted-ops/pyproject.toml b/tools/vetted-ops/pyproject.toml
index 0649ef44..e40c9cbc 100644
--- a/tools/vetted-ops/pyproject.toml
+++ b/tools/vetted-ops/pyproject.toml
@@ -30,6 +30,7 @@ dependencies = []
[project.scripts]
vetted-op = "vetted_ops:main"
+vetted-op-read = "vetted_ops:main_read"
[tool.hatch.build.targets.wheel]
packages = ["src/vetted_ops"]
diff --git a/tools/vetted-ops/src/vetted_ops/__init__.py b/tools/vetted-ops/src/vetted_ops/__init__.py
index c6e52601..b485daf9 100644
--- a/tools/vetted-ops/src/vetted_ops/__init__.py
+++ b/tools/vetted-ops/src/vetted_ops/__init__.py
@@ -17,6 +17,6 @@
# under the License.
"""Vetted, policy-scoped GitHub operations for agent sessions."""
-from .cli import main
+from .cli import main, main_read
-__all__ = ["main"]
+__all__ = ["main", "main_read"]
diff --git a/tools/vetted-ops/src/vetted_ops/cli.py b/tools/vetted-ops/src/vetted_ops/cli.py
index 302b8cf5..32cf5912 100644
--- a/tools/vetted-ops/src/vetted_ops/cli.py
+++ b/tools/vetted-ops/src/vetted_ops/cli.py
@@ -18,13 +18,28 @@
"""
The dispatcher.
-``vetted-op --caller <name> <operation> [param …]``
+``vetted-op --caller <name> <operation> [param …]`` (reads and writes)
+``vetted-op-read --caller <name> <operation> [param …]`` (reads only, by construction)
Everything the caller supplies is a *parameter*, never a fragment of a command.
The operation name selects a builder from the closed catalogue; the builder
returns an argv list; the argv list is executed without a shell. There is no
path by which a parameter becomes an argument to ``gh`` that the catalogue did
not put there.
+
+**Where the privilege boundary is, and where it is not.** ``--caller`` is an
+argv string chosen by whoever runs the command. If that is an agent, the agent
+picks its own caller name, so ``--caller`` scopes a *cooperating* skill to least
+privilege — it is not a boundary against a confused or hostile one, and must
+never be described as one.
+
+The boundary is the **entry point**, because the entry point is what a harness
+permission rule keys on and argv cannot change which binary is running.
+``vetted-op-read`` refuses every write operation in the catalogue before it
+looks at policy at all, so it is safe to allowlist outright. ``vetted-op`` can
+write and therefore has to keep whatever confirmation the harness puts in front
+of it. Splitting them is the whole point: it lets the read path lose its prompts
+without the write path losing its gate.
"""
from __future__ import annotations
@@ -32,6 +47,7 @@ from __future__ import annotations
import argparse
import subprocess
import sys
+from collections.abc import Sequence
from pathlib import Path
from . import ops as ops_mod
@@ -43,7 +59,9 @@ EXIT_POLICY = 3
EXIT_COMMAND = 4
-def _validate_params(op: ops_mod.Op, values: list[str], config: Config) -> dict[str, str]:
+def _validate_params(
+ op: ops_mod.Op, values: list[str], config: Config
+) -> tuple[dict[str, str], bytes | None]:
if len(values) != len(op.params):
raise ops_mod.ParamError(
f"operation {op.name!r} takes {len(op.params)} parameter(s) "
@@ -51,6 +69,7 @@ def _validate_params(op: ops_mod.Op, values: list[str], config: Config) -> dict[
)
resolved: dict[str, str] = {}
+ body: bytes | None = None
for name, raw in zip(op.params, values, strict=True):
# A parameter is never an option. Rejecting a leading dash removes the
# whole class of "smuggle a flag into gh" tricks before any validator
@@ -61,7 +80,16 @@ def _validate_params(op: ops_mod.Op, values: list[str], config: Config) -> dict[
if name in op.enums:
resolved[name] = ops_mod.enum(config.enum_values(op.enums[name]))(raw)
elif name in op.body_files:
- resolved[name] = ops_mod.body_file(raw, workspace=config.workspace)
+ # Read the content here and hand `gh` a "-" so it takes the body on
+ # stdin. The bytes that were validated are then, necessarily, the
+ # bytes that get published — there is no second open to race.
+ if body is not None:
+ raise ops_mod.ParamError(
+ f"operation {op.name!r} declares more than one body parameter; "
+ f"the dispatcher pipes exactly one body on stdin"
+ )
+ body = ops_mod.read_body(raw, workspace=config.workspace)
+ resolved[name] = "-"
elif name in {"number", "comment_id", "run_id"}:
resolved[name] = ops_mod.number(raw)
elif name in {"ref", "base", "head", "prefix"}:
@@ -76,7 +104,7 @@ def _validate_params(op: ops_mod.Op, values: list[str], config: Config) -> dict[
resolved[name] = ops_mod.node_id(raw)
else: # pragma: no cover - guarded by the catalogue test
raise ops_mod.ParamError(f"operation {op.name!r} declares unknown parameter {name!r}")
- return resolved
+ return resolved, body
def build_argv(op: ops_mod.Op, params: dict[str, str], config: Config) -> list[str]:
@@ -88,10 +116,33 @@ def build_argv(op: ops_mod.Op, params: dict[str, str], config: Config) -> list[s
return argv
-def main(argv: list[str] | None = None) -> int:
+def _reject_repeated_caller(argv: Sequence[str]) -> None:
+ """
+ Refuse a second ``--caller``.
+
+ argparse keeps the last occurrence, so ``--caller read-only … --caller
+ privileged`` resolves to the privileged one while still matching a
+ permission rule written against the read-only prefix. That turns a
+ prefix-matched allow rule into a bypass, so repetition is an error rather
+ than a last-wins convenience.
+ """
+ seen = sum(1 for a in argv if a == "--caller" or a.startswith("--caller="))
+ if seen > 1:
+ raise ops_mod.ParamError(
+ "--caller given more than once; it selects the policy entry and must be unambiguous"
+ )
+
+
+def main(argv: list[str] | None = None, *, read_only: bool = False) -> int:
+ raw = list(sys.argv[1:] if argv is None else argv)
+ prog = "vetted-op-read" if read_only else "vetted-op"
parser = argparse.ArgumentParser(
- prog="vetted-op",
- description="Run one fixed, policy-scoped GitHub operation. No pass-through arguments.",
+ prog=prog,
+ description=(
+ "Run one fixed, policy-scoped read operation. No pass-through arguments."
+ if read_only
+ else "Run one fixed, policy-scoped GitHub operation. No pass-through arguments."
+ ),
)
parser.add_argument("--caller", help="the skill or plugin invoking this operation")
parser.add_argument("--config", type=Path, default=None, help="path to the policy TOML")
@@ -102,6 +153,8 @@ def main(argv: list[str] | None = None) -> int:
if args.operation in (None, "list-ops"):
for name, op in sorted(ops_mod.OPS.items()):
+ if read_only and op.writes:
+ continue
kind = "write" if op.writes else "read "
print(f"{kind} {name:<22} {' '.join(op.params)}\n {op.summary}")
return EXIT_OK
@@ -121,14 +174,24 @@ def main(argv: list[str] | None = None) -> int:
return EXIT_USAGE
try:
+ _reject_repeated_caller(raw)
op = ops_mod.resolve(args.operation)
+ # Deliberately ahead of the policy lookup. This refusal does not depend
+ # on the config, on --caller, or on anything else the invoker chose: on
+ # this entry point a write operation does not exist. That is what makes
+ # the entry point allowlistable when `vetted-op` is not.
+ if read_only and op.writes:
+ raise ops_mod.ParamError(
+ f"{op.name!r} writes, and this is the read-only dispatcher; "
+ f"run it through 'vetted-op', which stays behind confirmation"
+ )
permitted = config.ops_for(args.caller)
if op.name not in permitted:
raise ops_mod.ParamError(
f"caller {args.caller!r} is not permitted to run {op.name!r}; "
f"permitted: {', '.join(sorted(permitted)) or '(none)'}"
)
- params = _validate_params(op, list(args.params), config)
+ params, body = _validate_params(op, list(args.params), config)
command = build_argv(op, params, config)
except (ops_mod.ParamError, ConfigError) as exc:
print(f"vetted-op: refused: {exc}", file=sys.stderr)
@@ -138,9 +201,15 @@ def main(argv: list[str] | None = None) -> int:
print(" ".join(command))
return EXIT_OK
- # No shell. The argv list is passed through verbatim.
- completed = subprocess.run(command, check=False)
+ # No shell. The argv list is passed through verbatim, and any body travels
+ # on stdin as bytes we already read — `gh` opens no file of ours.
+ completed = subprocess.run(command, check=False, input=body)
if completed.returncode != EXIT_OK:
print(f"vetted-op: {op.name} failed (gh exit {completed.returncode})", file=sys.stderr)
return EXIT_COMMAND
return EXIT_OK
+
+
+def main_read(argv: list[str] | None = None) -> int:
+ """The `vetted-op-read` console script: `main`, with writes made unreachable."""
+ return main(argv, read_only=True)
diff --git a/tools/vetted-ops/src/vetted_ops/ops.py b/tools/vetted-ops/src/vetted_ops/ops.py
index b3c54dac..a62b94b7 100644
--- a/tools/vetted-ops/src/vetted_ops/ops.py
+++ b/tools/vetted-ops/src/vetted_ops/ops.py
@@ -30,7 +30,9 @@ reviewed code change rather than a runtime decision.
from __future__ import annotations
+import os
import re
+import stat as stat_mod
from collections.abc import Callable, Sequence
from dataclasses import dataclass, field
from pathlib import Path
@@ -136,23 +138,65 @@ def query_name(value: str) -> str:
return str(path)
-def body_file(value: str, *, workspace: Path) -> str:
+def read_body(value: str, *, workspace: Path) -> bytes:
"""
- Validate a path holding body text.
-
- Content is passed to ``gh`` by *file reference*, never interpolated, so the
- body may contain anything at all — backticks, ``$(…)``, newlines, NUL-free
- binary. What is constrained is *which* file may be read: it must resolve
- inside the caller's declared workspace, so an operation cannot be talked into
- publishing ``~/.ssh/id_rsa`` or a credential file.
+ Validate and read body text, returning its **content**.
+
+ Content is never interpolated into a command, so a body may contain
+ anything at all — backticks, ``$(…)``, newlines. Two things are constrained:
+ *which* file may be read, and *when*.
+
+ The "when" is the part that is easy to get wrong. Returning a path for ``gh``
+ to open later leaves a window between validation and use: the file that was
+ checked and the file that gets published need not be the same one. So this
+ reads the content itself, from a descriptor it opened, and the caller pipes
+ those bytes to ``gh`` on stdin. There is exactly one open, and it is ours.
+
+ ``O_NOFOLLOW`` refuses a symlink as the final component, and the realpath
+ check refuses one anywhere above it, so no body can resolve out of the
+ workspace. The workspace itself must be owned by this user and closed to
+ group and world: a directory anyone can write is a directory anyone can
+ pre-seed, and these bodies become public comments.
"""
- path = Path(value).expanduser().resolve()
root = workspace.expanduser().resolve()
- if not path.is_file():
- raise ParamError(f"body file does not exist: {value!r}")
- if root not in path.parents and path != root:
+ try:
+ root_st = os.stat(root)
+ except OSError as exc:
+ raise ParamError(f"workspace {str(root)!r} is unusable: {exc}") from None
+ if not os.path.isdir(root):
+ raise ParamError(f"workspace {str(root)!r} is not a directory")
+ if root_st.st_uid != os.getuid():
+ raise ParamError(
+ f"workspace {str(root)!r} is owned by uid {root_st.st_uid}, not by you "
+ f"(uid {os.getuid()}) — refusing to publish a body from it"
+ )
+ if root_st.st_mode & 0o022:
+ raise ParamError(
+ f"workspace {str(root)!r} is group- or world-writable (mode "
+ f"{root_st.st_mode & 0o777:04o}) — anyone who can write it can choose "
+ f"what gets posted; chmod 700 it"
+ )
+
+ candidate = Path(value).expanduser()
+ resolved = Path(os.path.realpath(candidate))
+ if root not in resolved.parents:
raise ParamError(f"body file must live under the workspace {str(root)!r}: {value!r}")
- return str(path)
+
+ try:
+ fd = os.open(candidate, os.O_RDONLY | os.O_NOFOLLOW)
+ except FileNotFoundError:
+ raise ParamError(f"body file does not exist: {value!r}") from None
+ except OSError as exc:
+ raise ParamError(f"body file cannot be read ({exc.strerror}): {value!r}") from None
+ try:
+ st = os.fstat(fd)
+ if not stat_mod.S_ISREG(st.st_mode):
+ raise ParamError(f"body file is not a regular file: {value!r}")
+ with os.fdopen(fd, "rb", closefd=True) as handle:
+ return handle.read()
+ except ParamError:
+ os.close(fd)
+ raise
def enum(allowed: Sequence[str]) -> Callable[[str], str]:
diff --git a/tools/vetted-ops/tests/test_vetted_ops.py b/tools/vetted-ops/tests/test_vetted_ops.py
index 010e8c2c..b555d9fb 100644
--- a/tools/vetted-ops/tests/test_vetted_ops.py
+++ b/tools/vetted-ops/tests/test_vetted_ops.py
@@ -50,10 +50,22 @@ board_status_field_id = "PVTSSF_field"
"""
+@pytest.fixture()
+def policy_path(tmp_path: Path) -> Path:
+ """The policy on disk — for tests that drive `cli.main` end to end."""
+ workspace = tmp_path / "scratch"
+ workspace.mkdir()
+ workspace.chmod(0o700)
+ cfg_path = tmp_path / "config.toml"
+ cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
+ return cfg_path
+
+
@pytest.fixture()
def policy(tmp_path: Path) -> config.Config:
workspace = tmp_path / "scratch"
workspace.mkdir()
+ workspace.chmod(0o700)
cfg_path = tmp_path / "config.toml"
cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
return config.load(cfg_path)
@@ -153,7 +165,7 @@ def test_label_must_be_one_of_the_configured_values(policy: config.Config) -> No
def test_configured_label_is_accepted(policy: config.Config) -> None:
op = ops.resolve("issue-add-label")
- params = cli._validate_params(op, ["7", "cve allocated"], policy)
+ params, _body = cli._validate_params(op, ["7", "cve allocated"], policy)
argv = cli.build_argv(op, params, policy)
assert argv == [
"gh",
@@ -183,9 +195,11 @@ def test_body_file_content_may_contain_anything(policy: config.Config) -> None:
body = policy.workspace / "note.md"
body.write_text("`id` $(whoami) && rm -rf / ; drop table\n")
op = ops.resolve("issue-comment")
- params = cli._validate_params(op, ["7", str(body)], policy)
+ params, sent = cli._validate_params(op, ["7", str(body)], policy)
argv = cli.build_argv(op, params, policy)
- assert argv[-2:] == ["--body-file", str(body.resolve())]
+ # The body reaches `gh` on stdin, not as a path it opens for itself.
+ assert argv[-2:] == ["--body-file", "-"]
+ assert sent == b"`id` $(whoami) && rm -rf / ; drop table\n"
# --- per-caller scoping ------------------------------------------------------
@@ -196,6 +210,7 @@ def test_caller_may_not_run_an_operation_outside_its_manifest(
) -> None:
workspace = tmp_path / "scratch"
workspace.mkdir()
+ workspace.chmod(0o700)
cfg_path = tmp_path / "config.toml"
cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
@@ -207,6 +222,7 @@ def test_caller_may_not_run_an_operation_outside_its_manifest(
def test_unknown_caller_is_refused(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
workspace = tmp_path / "scratch"
workspace.mkdir()
+ workspace.chmod(0o700)
cfg_path = tmp_path / "config.toml"
cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
@@ -218,6 +234,7 @@ def test_unknown_caller_is_refused(tmp_path: Path, capsys: pytest.CaptureFixture
def test_permitted_caller_reaches_dry_run(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
workspace = tmp_path / "scratch"
workspace.mkdir()
+ workspace.chmod(0o700)
cfg_path = tmp_path / "config.toml"
cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
@@ -229,6 +246,7 @@ def test_permitted_caller_reaches_dry_run(tmp_path: Path, capsys: pytest.Capture
def test_caller_is_required(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
workspace = tmp_path / "scratch"
workspace.mkdir()
+ workspace.chmod(0o700)
cfg_path = tmp_path / "config.toml"
cfg_path.write_text(CONFIG_TOML.format(workspace=workspace))
@@ -242,7 +260,7 @@ def test_caller_is_required(tmp_path: Path, capsys: pytest.CaptureFixture[str])
def test_repo_cannot_be_influenced_by_a_parameter(policy: config.Config) -> None:
op = ops.resolve("issue-view")
- params = cli._validate_params(op, ["7"], policy)
+ params, _body = cli._validate_params(op, ["7"], policy)
argv = cli.build_argv(op, params, policy)
assert argv[argv.index("--repo") + 1] == "acme/tracker"
@@ -427,3 +445,132 @@ def test_no_operation_interpolates_a_traversing_ref(policy: config.Config) -> No
params[p] = sample[p]
for arg in op.build(policy.as_mapping(), **params):
assert "/../" not in arg and not arg.endswith("/.."), op.name
+
+
+# --------------------------------------------------------------------------
+# The privilege boundary is the entry point, not --caller
+# --------------------------------------------------------------------------
+
+
+def test_read_dispatcher_refuses_a_write_whatever_caller_is_named(
+ policy_path: Path, capsys: pytest.CaptureFixture[str]
+) -> None:
+ """
+ The finding this split exists to close.
+
+ `--caller` is chosen by whoever runs the command, so a caller name can never
+ be the thing that stops a write. On the read dispatcher the refusal has to
+ hold even when the invoker names the *most* privileged caller in the policy.
+ """
+ rc = cli.main(
+ [
+ "--caller",
+ "security-issue-sync",
+ "issue-add-label",
+ "7",
+ "cve allocated",
+ "--config",
+ str(policy_path),
+ "--dry-run",
+ ],
+ read_only=True,
+ )
+ assert rc == cli.EXIT_POLICY
+ assert "read-only dispatcher" in capsys.readouterr().err
+
+
+def test_read_dispatcher_refuses_writes_before_consulting_policy(
+ policy_path: Path, capsys: pytest.CaptureFixture[str]
+) -> None:
+ """A caller absent from the policy still gets the write refusal, not a
+ config error — proving the check does not depend on the config at all."""
+ rc = cli.main(
+ [
+ "--caller",
+ "no-such-caller",
+ "issue-close",
+ "7",
+ "completed",
+ "--config",
+ str(policy_path),
+ "--dry-run",
+ ],
+ read_only=True,
+ )
+ assert rc == cli.EXIT_POLICY
+ assert "read-only dispatcher" in capsys.readouterr().err
+
+
+def test_repeated_caller_is_refused(policy_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
+ """
+ argparse keeps the last `--caller`, so repetition would let a command match
+ a permission rule written against a read-only prefix while resolving to a
+ privileged caller.
+ """
+ rc = cli.main(
+ [
+ "--caller",
+ "security-issue-triage",
+ "--caller",
+ "security-issue-sync",
+ "issue-add-label",
+ "7",
+ "cve allocated",
+ "--config",
+ str(policy_path),
+ "--dry-run",
+ ],
+ )
+ assert rc == cli.EXIT_POLICY
+ assert "more than once" in capsys.readouterr().err
+
+
+def test_read_dispatcher_still_runs_reads(policy_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
+ rc = cli.main(
+ ["--caller", "security-issue-triage", "issue-view", "7", "--config", str(policy_path), "--dry-run"],
+ read_only=True,
+ )
+ assert rc == cli.EXIT_OK
+ assert "gh issue view 7" in capsys.readouterr().out
+
+
+# --------------------------------------------------------------------------
+# Body files: single open, owned workspace, no symlinks
+# --------------------------------------------------------------------------
+
+
+def test_symlinked_body_is_refused(policy: config.Config, tmp_path: Path) -> None:
+ """O_NOFOLLOW: a symlink sitting inside the workspace cannot smuggle out a
+ file from elsewhere, even though its own path passes the containment test."""
+ secret = tmp_path / "id_rsa"
+ secret.write_text("PRIVATE KEY")
+ link = policy.workspace / "innocent.md"
+ link.symlink_to(secret)
+ op = ops.resolve("issue-comment")
+ with pytest.raises(ops.ParamError):
+ cli._validate_params(op, ["7", str(link)], policy)
+
+
+def test_group_writable_workspace_is_refused(policy: config.Config) -> None:
+ """A workspace anyone can write is a workspace anyone can pre-seed, and
+ these bodies become public comments."""
+ body = policy.workspace / "note.md"
+ body.write_text("hello")
+ policy.workspace.chmod(0o770)
+ op = ops.resolve("issue-comment")
+ try:
+ with pytest.raises(ops.ParamError, match="group- or world-writable"):
+ cli._validate_params(op, ["7", str(body)], policy)
+ finally:
+ policy.workspace.chmod(0o700)
+
+
+def test_body_is_read_once_not_reopened(policy: config.Config) -> None:
+ """Content is captured at validation time, so a swap afterwards cannot
+ change what gets published."""
+ body = policy.workspace / "note.md"
+ body.write_text("approved text")
+ op = ops.resolve("issue-comment")
+ _params, captured = cli._validate_params(op, ["7", str(body)], policy)
+ body.write_text("substituted text")
+ assert captured == b"approved text"