diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index 2e0c8de3a4..273ba5ebe8 100644 --- a/docs/core-data-structures/bash.md +++ b/docs/core-data-structures/bash.md @@ -19,19 +19,20 @@ interface BashExecRequest { signal?: AbortSignal | undefined /** * Bytes to write to the command's stdin, then close it. Absent leaves stdin - * closed/empty (the default for model-driven tool calls). A TRUSTED-PLUGIN - * surface: the model-facing bash tool does NOT thread model-supplied input - * here — it is set by in-process plugins (e.g. the hooks bridges, which write - * a hook command's JSON payload to its stdin). + * closed/empty (the default for model-driven tool calls). Set by in-process + * plugins (e.g. the hooks bridges, which write a hook command's JSON payload + * to its stdin); the model-facing bash tool does not expose it as a parameter + * (a model that needs stdin uses shell syntax like a heredoc or a pipe). */ stdin?: string | undefined /** * Extra environment entries for the command, merged AFTER the * implementation's credential scrub (so an explicit entry here is honored even - * when its name matches the scrub pattern — the caller takes responsibility). - * Like {@link stdin}, a TRUSTED-PLUGIN surface: the model-facing bash tool - * never forwards model-supplied env; in-process plugins (the hooks bridges) - * set hook env vars (`CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, …) here. + * when its name matches the scrub pattern — the caller named a value it holds, + * not the harness's ambient secret). Set by in-process plugins (the hooks + * bridges set `CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, …); the model-facing + * bash tool does not expose it as a parameter (a model that needs an env var + * uses shell syntax like `FOO=bar cmd`). */ env?: Record | undefined /** @@ -58,8 +59,7 @@ interface BashExecSpec { * verbatim from {@link BashExecRequest.stdin}. OPTIONAL on the resolved spec * (unlike `owner`): it has no config default, so a missing one means "no * stdin" — the safe, ordinary case — not a silent footgun, so it stays a - * plain optional rather than required-but-nullable. A TRUSTED-PLUGIN surface - * (see the request field). + * plain optional rather than required-but-nullable (see the request field). */ stdin?: string | undefined /** @@ -67,7 +67,7 @@ interface BashExecSpec { * {@link BashExecRequest.env} and merged by the implementation AFTER its * credential scrub (an explicit entry wins even when its name matches the * scrub pattern). OPTIONAL on the spec for the same reason as `stdin` — no - * config default, absent means "no extra env". A TRUSTED-PLUGIN surface. + * config default, absent means "no extra env". */ env?: Record | undefined /** @@ -84,7 +84,7 @@ interface BashExecSpec { The `owner` token is the isolation key: the executor stores it but never interprets it (access policy is the consumer's job), so a background task started by one agent isn't readable cross-session. A required-but-nullable field makes a forgotten owner a visible `undefined` rather than a silently-unowned task. -`stdin` and `env` are a **trusted-plugin surface**: an in-process plugin (the hooks bridges, native plugins) sets them to feed a hook command its JSON payload on stdin and its `CLAUDE_PROJECT_DIR`/`CLAUDE_PLUGIN_ROOT` env. The model-facing `dsh-tool-bash` tool deliberately NEVER forwards model input into either field — its request is built from `command`/`workdir`/`timeoutMs`/`signal`/`owner` only — so a model cannot smuggle an env var or stdin payload past the credential scrub (a guard test asserts this). `env` is merged AFTER the scrub so a trusted caller can set even a credential-shaped var; the scrub's job is to stop the harness's OWN ambient credentials leaking into model-driven commands, not to constrain a trusted plugin. +`stdin` and `env` are set by in-process plugins (the hooks bridges, native plugins) to feed a hook command its JSON payload on stdin and its `CLAUDE_PROJECT_DIR`/`CLAUDE_PLUGIN_ROOT` env. The model-facing `dsh-tool-bash` tool does not expose them as parameters — its request is built from `command`/`workdir`/`timeoutMs`/`signal`/`owner` only — because a model already has equivalent power through shell syntax (`FOO=bar cmd`, a heredoc), so duplicating them as tool params would be redundant. This is NOT a security boundary: the credential scrub in `dsh-bash-local` is what stops the harness's ambient secrets reaching a spawned command, and it works regardless of these fields (a model cannot read a value the scrub removed, and tool-call args are static JSON, never shell-evaluated). A guard test asserts the tool doesn't forward model `env`/`stdin` — to catch a future `...args` spread, not to defend a trust wall. `env` is merged AFTER the scrub so an explicit caller entry (a value it already holds) wins even on a credential-shaped name. See [the bash-stdin-env RFC](../rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). Both ids the seam handles are [branded](core.md) (zero-cost `string` brands, the same machinery as `SessionId`/`AgentId`): `BashTaskId` (a tracked background task, generated `bash-N` by the local executor) and `OwnerToken` (the opaque isolation key). `OwnerToken` is deliberately a DISTINCT brand from `SessionId`, not an alias: the bash seam is a capability seam that must not know what an owner token *means*, so it never imports `dsh-session`'s vocabulary — the `dsh-tool-bash` consumer is the single boundary that casts the owning agent's `SessionId` into an `OwnerToken`. Branding both stops a raw `string` (or a `BashTaskId` where an `OwnerToken` is expected, or vice versa) from slipping through the type checker on the model-facing `task_id` path. diff --git a/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md b/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md index 224ff6c24a..aafa24cb1d 100644 --- a/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md +++ b/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md @@ -1,4 +1,4 @@ -# RFC: stdin + extra env on the bash seam — a trusted-plugin surface +# RFC: stdin + extra env on the bash seam Status: implemented (accepted 2026-06-30) @@ -6,9 +6,9 @@ Status: implemented (accepted 2026-06-30) ## Context -The hooks subsystem runs external hook commands the way Claude Code and Codex do: a hook is a shell command that receives its event payload as **JSON on stdin** and reads context from a handful of **environment variables** (`CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, `PLUGIN_ROOT`, …). The harness already has a perfectly good command runner behind the `ctx.bash` capability seam ([dsh-bash](../../../../packages/bash/bash) → [dsh-bash-local](../../../../packages/bash/bash-local)), with process-group kills, output truncation/spill, and a credential scrub. Reusing it for hook execution means a hook bridge does not re-implement subprocess plumbing — but the seam had no way to write stdin or set extra env. +The hooks subsystem runs external hook commands the way Claude Code and Codex do: a hook is a shell command that receives its event payload as **JSON on stdin** and reads context from a handful of **environment variables** (`CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, `PLUGIN_ROOT`, …). The harness already has a perfectly good command runner behind the `ctx.bash` capability seam ([dsh-bash](../../../../packages/bash/bash) → [dsh-bash-local](../../../../packages/bash/bash-local)), with process-group kills, output truncation/spill, and a credential scrub. Reusing it for hook execution means a hook bridge does not re-implement subprocess plumbing — but the seam had no way to write stdin or set extra env. This RFC adds those two inputs. -The friction is that those two inputs are **dangerous in exactly the way the seam was built to prevent**. [dsh-bash-local](../../../../packages/bash/bash-local)'s `childEnv()` deliberately scrubs `*KEY*`/`*SECRET*`/`*TOKEN*` from the child environment so the harness's own `DEEPSEEK_API_KEY` cannot leak into model-driven command output (see [AGENTS.md](../../../../AGENTS.md) § Defensive patterns, "Never hand untrusted/model output the ambient environment or predictable paths"). An arbitrary-env / arbitrary-stdin capability is the opposite of that guarantee. So the question this RFC answers is not "can we add stdin/env" — it is "who is allowed to use them, and how is that boundary enforced". +**These fields are NOT a new security boundary.** It is tempting to frame arbitrary-stdin / arbitrary-env as "dangerous, so gate who may use them" — but that framing is wrong, because a model driving the `bash` tool **already** has equivalent power through ordinary shell syntax: `FOO=bar cmd` sets an env var, a heredoc or `printf … | cmd` feeds arbitrary stdin. Adding `env`/`stdin` as seam fields grants the model no capability it lacks. In particular they cannot exfiltrate the harness's ambient credentials: the real control for that is the **credential scrub** in [dsh-bash-local](../../../../packages/bash/bash-local)'s `childEnv()`, which strips `*KEY*`/`*SECRET*`/`*TOKEN*` from `process.env` before the child sees it (see [AGENTS.md](../../../../AGENTS.md) § Defensive patterns, "Never hand untrusted/model output the ambient environment or predictable paths"). The scrub works regardless of these fields — a model cannot read a value that is not in the environment, and tool-call arguments are static JSON, never shell-evaluated, so a model cannot write `env: {LEAK: $DEEPSEEK_API_KEY}` and have it expand. So the security question is already answered by the scrub; this RFC is only about giving trusted in-process callers a clean way to pass a JSON payload + `CLAUDE_*` vars without routing them through model-visible shell text. ## Decision @@ -16,18 +16,18 @@ Add `stdin?: string` and `env?: Record` to **both** `BashExecReq Three deliberate choices: -1. **`stdin`/`env` are a TRUSTED-PLUGIN surface, enforced at the consumer, not the seam.** The seam itself imposes no access policy (consistent with how `owner` works — the executor stores but never interprets it). The enforcement lives in the model-facing consumer [dsh-tool-bash](../../../../packages/bash/tool-bash): its `bash` tool builds its `BashExecRequest` from `command`/`workdir`/`timeoutMs`/`signal`/`owner` **only**, and never reads model arguments into `stdin`/`env`. A model that smuggles `env`/`stdin` keys into the tool-call arguments gets them ignored. A regression guard (`tool-bash` "trusted-plugin boundary" tests) drives the real tool with adversarial args and asserts the recorded request carries neither field — and is proven to go red if the consumer ever forwards them. Only in-process plugins (the hooks bridges, native plugins) that construct a `BashExecRequest` directly can set them. +1. **The model-facing `bash` tool simply does NOT expose `stdin`/`env` as parameters** — not as a security wall, but because bash syntax already covers the model's needs, so duplicating them as tool params would be redundant surface. [dsh-tool-bash](../../../../packages/bash/tool-bash)'s `bash` tool builds its `BashExecRequest` from `command`/`workdir`/`timeoutMs`/`signal`/`owner` only; a model that includes `env`/`stdin` keys in its tool-call arguments simply has them ignored. A regression guard (`tool-bash` "does not forward env/stdin" tests) drives the real tool with those extra args and asserts the recorded request carries neither field — its purpose is to catch a future refactor that blindly spreads `...args` into the request and silently starts forwarding model input into the post-scrub `env` merge, NOT to defend a trust boundary. In-process plugins (the hooks bridges, native plugins) that construct a `BashExecRequest` directly set the fields; the seam imposes no access policy (consistent with how `owner` works — the executor stores but never interprets it). -2. **`env` merges AFTER the credential scrub, so a trusted caller's explicit entry always wins** — even a credential-shaped name. This is correct precisely because the scrub's job is narrow: stop the harness's *ambient* `process.env` credentials from leaking into *model-driven* commands. A trusted plugin that explicitly sets a var has taken responsibility for it; the scrub is not a constraint on trusted callers. `childEnv(extra?)` layers `scrub(process.env)` → `ENV_OVERRIDES` (the model-friendly `TERM=dumb` etc.) → `extra`, last-wins. +2. **`env` merges AFTER the credential scrub, so an explicit caller entry always wins** — even a credential-shaped name. This is correct because the scrub's job is narrow: stop the harness's *ambient* `process.env` credentials from leaking into a spawned command. A caller that explicitly sets a var has named a value it already holds (not the ambient secret), so the scrub is not a constraint on it. `childEnv(extra?)` layers `scrub(process.env)` → `ENV_OVERRIDES` (the model-friendly `TERM=dumb` etc.) → `extra`, last-wins. 3. **`stdin`/`env` are required-absent-OK (plain optional) on the resolved spec, NOT required-but-nullable like `owner`.** `owner` is required-but-nullable because a *silently* missing owner yields an unowned, cross-session-readable task — a security footgun that a visible `undefined` guards against. `stdin`/`env` have no such hazard: a missing one means "no stdin / no extra env", which is the safe, ordinary case (every model-driven call). So they stay plain optionals, matching `signal`. -`dsh-bash-local` now ALWAYS spawns stdin as a `'pipe'` and closes it immediately — with the supplied bytes when a trusted plugin set `stdin`, empty otherwise. A closed empty pipe gives a reading child EOF exactly as the previous `'ignore'` (`/dev/null`) did, so the no-stdin path is behavior-equivalent; keeping the `stdio` tuple a literal `['pipe','pipe','pipe']` also preserves the typed `spawn` overload that guarantees non-null `stdout`/`stderr`. A child that exits without reading makes the stdin write fail EPIPE; that error is swallowed (the command's outcome rides on its exit code/output, not the write) so it never crashes the host or rejects `done`. +`dsh-bash-local` now ALWAYS spawns stdin as a `'pipe'` and closes it immediately — with the supplied bytes when a caller set `stdin`, empty otherwise. A closed empty pipe gives a reading child EOF exactly as the previous `'ignore'` (`/dev/null`) did, so the no-stdin path is behavior-equivalent; keeping the `stdio` tuple a literal `['pipe','pipe','pipe']` also preserves the typed `spawn` overload that guarantees non-null `stdout`/`stderr`. A child that exits without reading makes the stdin write fail EPIPE; that error is swallowed (the command's outcome rides on its exit code/output, not the write) so it never crashes the host or rejects `done`. ## Scope: configurable scrub pattern is NOT included -An earlier sketch of this work also proposed making `SENSITIVE_ENV_PATTERN` configurable. Validating against the code, that is **speculative and already subsumed**: `run.ts` documents a configurable whitelist as future work, and the new explicit `env` field — merged after the scrub — already gives a trusted plugin full control, including over credential-shaped vars. There is no current caller that needs to *broaden* the ambient scrub (the hazard runs the other way). Adding a config knob now would be a feature with no consumer, against [AGENTS.md](../../../../AGENTS.md) § "Don't add features beyond what the task requires". If a real workflow ever needs to forward a specific ambient credential, the explicit `env` field is the supported path; a configurable scrub can be reconsidered then. +An earlier sketch of this work also proposed making `SENSITIVE_ENV_PATTERN` configurable. Validating against the code, that is **speculative and already subsumed**: `run.ts` documents a configurable whitelist as future work, and the new explicit `env` field — merged after the scrub — already gives a caller full control, including over credential-shaped vars. There is no current caller that needs to *broaden* the ambient scrub (the hazard runs the other way). Adding a config knob now would be a feature with no consumer, against [AGENTS.md](../../../../AGENTS.md) § "Don't add features beyond what the task requires". If a real workflow ever needs to forward a specific ambient credential, the explicit `env` field is the supported path; a configurable scrub can be reconsidered then. ## Consequences -A hook bridge builds a `BashExecRequest` with the hook's JSON payload as `stdin` and its `CLAUDE_*`/`PLUGIN_ROOT` vars as `env`, and runs it through the same `ctx.bash` everything else uses — no bespoke subprocess code, and the full process-group-kill / truncation / spill machinery for free. The model-facing attack surface is unchanged: the consumer's request-building is the single boundary, guarded by a test that fails if it regresses. The vocabulary addition is documented in [docs/core-data-structures/bash.md](../../../core-data-structures/bash.md) (the `type-equiv` request/spec blocks) and the three bash-package READMEs; the trusted-plugin rule mirrors the existing scrub/predictable-path discipline in [AGENTS.md](../../../../AGENTS.md) § Defensive patterns. +A hook bridge builds a `BashExecRequest` with the hook's JSON payload as `stdin` and its `CLAUDE_*`/`PLUGIN_ROOT` vars as `env`, and runs it through the same `ctx.bash` everything else uses — no bespoke subprocess code, and the full process-group-kill / truncation / spill machinery for free. The model-facing attack surface is unchanged (the credential scrub, not these fields, is what bounds it), and the `bash` tool's request-building stays the single place that decides which fields a model call carries — guarded by a test that fails if a refactor starts forwarding model input. The vocabulary addition is documented in [docs/core-data-structures/bash.md](../../../core-data-structures/bash.md) (the `type-equiv` request/spec blocks) and the three bash-package READMEs. diff --git a/packages/bash/bash-local/README.md b/packages/bash/bash-local/README.md index 2ae905b628..7cb6f809c5 100644 --- a/packages/bash/bash-local/README.md +++ b/packages/bash/bash-local/README.md @@ -21,7 +21,7 @@ Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; - **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `XXX(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them. - **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools. - **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported when available. If the final spill close reports a delayed writeback failure, the executor still returns the tail but withholds the path rather than advertising a possibly incomplete file. -- **Model-friendly env + credential scrub** — `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`), then `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. A spec's **trusted-plugin** `env` is merged LAST (after the scrub), so an in-process plugin's explicit entry wins even on a credential-shaped name — the scrub guards the harness's *ambient* credentials from *model-driven* commands, not a trusted caller. The spec's `stdin` (also trusted-plugin) is written to the child and closed; with none supplied, stdin is an immediately-closed empty pipe (EOF, as before). See [the trusted-plugin RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). +- **Model-friendly env + credential scrub** — `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`), then `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. This scrub is the security control that keeps the harness's *ambient* credentials out of a spawned command. A spec's `env` is merged LAST (after the scrub), so a caller's explicit entry — a value it already holds — wins even on a credential-shaped name. The spec's `stdin` is written to the child and closed; with none supplied, stdin is an immediately-closed empty pipe (EOF, as before). Both `env`/`stdin` are set by in-process plugins (the hooks bridges); the model-facing tool doesn't expose them. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). - **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. The spec's opaque `owner` token is stored on the tracked task and returned by `ownerOf(id)` — the executor never interprets it (the consumer's access policy does), and because it lives with the task here it survives a `tool-bash` HMR reload. ## Sandboxing diff --git a/packages/bash/bash/README.md b/packages/bash/bash/README.md index cec9e5834a..39318ae371 100644 --- a/packages/bash/bash/README.md +++ b/packages/bash/bash/README.md @@ -30,4 +30,4 @@ Implementations subclass `BashExecutor`, implement the abstract methods, and cal `BashExecRequest` (command, workdir?, timeoutMs?, signal?, stdin?, env?, owner?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?, stdin?, env?, owner) before execution; `owner` is optional on the request and **required-but-nullable** (`OwnerToken | undefined`) on the resolved spec, so a forgotten owner is a visible `undefined` rather than a silently-absent property. The task id (`BashTaskId`) and the `owner` token (`OwnerToken`) are [branded](../../util/brand) — `OwnerToken` is a DISTINCT brand from `SessionId` (the seam never imports `dsh-session`; the `dsh-tool-bash` consumer is the single boundary that casts its `SessionId` into one). `run()` returns `BashRunResult` (exitCode, signal, timedOut, aborted, timeoutMs, stdout/stderr as `CollectedOutput`) and `start()`/`readOutput()` use `BashTask`/`BashTaskRead` for the background side. See `src/types.ts` for the full contracts. -`stdin` and `env` are a **trusted-plugin surface**: an in-process plugin (the hooks bridges, native plugins) sets them to feed a hook command its JSON payload on stdin and its `CLAUDE_PROJECT_DIR`/`CLAUDE_PLUGIN_ROOT` env. The model-facing `dsh-tool-bash` tool deliberately never forwards model input into either — so a model cannot smuggle an env var or stdin payload past the implementation's credential scrub. They are plain optionals on the resolved spec (unlike `owner`'s required-but-nullable): a missing one means "none", the safe default, not a security footgun. See [the trusted-plugin RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). +`stdin` and `env` are set by in-process plugins (the hooks bridges, native plugins) to feed a hook command its JSON payload on stdin and its `CLAUDE_PROJECT_DIR`/`CLAUDE_PLUGIN_ROOT` env. The model-facing `dsh-tool-bash` tool does not expose them as parameters — a model already has equivalent power through shell syntax (`FOO=bar cmd`, a heredoc), so they would be redundant tool params. This is not a security boundary: the implementation's credential scrub (not these fields) is what keeps the harness's ambient secrets out of a spawned command. They are plain optionals on the resolved spec (unlike `owner`'s required-but-nullable): a missing one means "none", the safe default. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). diff --git a/packages/bash/bash/src/types.ts b/packages/bash/bash/src/types.ts index f5be9f11fe..9acd5c7cb7 100644 --- a/packages/bash/bash/src/types.ts +++ b/packages/bash/bash/src/types.ts @@ -47,19 +47,20 @@ export interface BashExecRequest { signal?: AbortSignal | undefined /** * Bytes to write to the command's stdin, then close it. Absent leaves stdin - * closed/empty (the default for model-driven tool calls). A TRUSTED-PLUGIN - * surface: the model-facing bash tool does NOT thread model-supplied input - * here — it is set by in-process plugins (e.g. the hooks bridges, which write - * a hook command's JSON payload to its stdin). + * closed/empty (the default for model-driven tool calls). Set by in-process + * plugins (e.g. the hooks bridges, which write a hook command's JSON payload + * to its stdin); the model-facing bash tool does not expose it as a parameter + * (a model that needs stdin uses shell syntax like a heredoc or a pipe). */ stdin?: string | undefined /** * Extra environment entries for the command, merged AFTER the * implementation's credential scrub (so an explicit entry here is honored even - * when its name matches the scrub pattern — the caller takes responsibility). - * Like {@link stdin}, a TRUSTED-PLUGIN surface: the model-facing bash tool - * never forwards model-supplied env; in-process plugins (the hooks bridges) - * set hook env vars (`CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, …) here. + * when its name matches the scrub pattern — the caller named a value it holds, + * not the harness's ambient secret). Set by in-process plugins (the hooks + * bridges set `CLAUDE_PROJECT_DIR`, `CLAUDE_PLUGIN_ROOT`, …); the model-facing + * bash tool does not expose it as a parameter (a model that needs an env var + * uses shell syntax like `FOO=bar cmd`). */ env?: Record | undefined /** @@ -92,8 +93,7 @@ export interface BashExecSpec { * verbatim from {@link BashExecRequest.stdin}. OPTIONAL on the resolved spec * (unlike `owner`): it has no config default, so a missing one means "no * stdin" — the safe, ordinary case — not a silent footgun, so it stays a - * plain optional rather than required-but-nullable. A TRUSTED-PLUGIN surface - * (see the request field). + * plain optional rather than required-but-nullable (see the request field). */ stdin?: string | undefined /** @@ -101,7 +101,7 @@ export interface BashExecSpec { * {@link BashExecRequest.env} and merged by the implementation AFTER its * credential scrub (an explicit entry wins even when its name matches the * scrub pattern). OPTIONAL on the spec for the same reason as `stdin` — no - * config default, absent means "no extra env". A TRUSTED-PLUGIN surface. + * config default, absent means "no extra env". */ env?: Record | undefined /** diff --git a/packages/bash/tool-bash/README.md b/packages/bash/tool-bash/README.md index f7a15894f9..b81e49d027 100644 --- a/packages/bash/tool-bash/README.md +++ b/packages/bash/tool-bash/README.md @@ -40,9 +40,9 @@ These tools own how their calls render in a UI (an editor's tool-call card) via When a background task finishes, a short notice is injected into the owning agent's session (`agent.inject()`, source `{kind: 'plugin', plugin: 'tool-bash'}`). The owning agent is found by its session token: the listener reads `ctx.bash.ownerOf(task.id)` and scans `ctx.get('agents')?.list()` for an agent whose `session.header.id` matches (read via `ctx.get` — `onTaskDone` runs on the bash fiber, a foreign fiber, so the `ctx.agents` proxy would throw). If no live agent carries that token — e.g. the owning session disconnected and its agent was disposed while the task ran on — the notice is dropped cleanly. Injection is **durable context for the next model request, not a wake-up** — an idle agent stays idle until something sends a message. That's why the tool descriptions tell the model to poll with `bash_output`. -## Trusted-plugin boundary: env / stdin are never model-driven +## The tool builds its request from named args only -The `BashExecRequest` seam carries optional `stdin` and `env` (a **trusted-plugin surface** used by the hooks bridges to feed a hook command its JSON payload and `CLAUDE_*` env). This tool deliberately **never** threads model input into either: its request is built from `command`/`workdir`/`timeoutMs`/`signal`/`owner` only, so a model that includes `env` or `stdin` keys in its tool arguments has them ignored — it cannot smuggle an environment variable or stdin payload past `dsh-bash-local`'s credential scrub. A regression guard (the "trusted-plugin boundary" tests) drives the real tool with adversarial args and asserts the resulting request carries neither field. See [the trusted-plugin RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). +The `BashExecRequest` seam carries optional `stdin` and `env`, used by the hooks bridges to feed a hook command its JSON payload and `CLAUDE_*` env. This tool does **not** expose them as parameters: its request is built from `command`/`workdir`/`timeoutMs`/`signal`/`owner` only, so a model that includes `env` or `stdin` keys in its tool arguments has them ignored. This is not a trust boundary — a model already has equivalent power through shell syntax (`FOO=bar cmd`, a heredoc), and the real defense against leaking the harness's ambient secrets is `dsh-bash-local`'s credential scrub, which works regardless. A regression guard drives the real tool with those extra args and asserts the resulting request carries neither field — its job is to catch a future refactor that blindly spreads `...args` into the request (which would silently forward model input into the post-scrub `env` merge), not to defend a wall. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). ## Permissions diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index 0845163193..79e8ea183d 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -864,14 +864,18 @@ describe('tool-owned UI presentation (presentCall / presentResult)', () => { }) }) -describe('trusted-plugin boundary: the model-facing bash tool never sets env/stdin', () => { +describe('the model-facing bash tool builds its request from named args only (no {...args} forward)', () => { /** * Records every {@link BashExecRequest} the consumer hands to `resolve()`, so a - * test can assert what the model-facing tool DID and DID NOT forward. `stdin` - * and `env` are a TRUSTED-PLUGIN surface (in-process plugins only); the `bash` - * tool must never thread model-supplied input into them, even when the model - * smuggles extra keys into the tool arguments. Foreground `run()` returns a - * canned result; `start()` is unused here. + * test can assert what the model-facing tool DID and DID NOT forward. The `bash` + * tool does not expose `stdin`/`env` as parameters (bash syntax already gives a + * model that power), so it must build its request from named args only and + * never spread unknown tool-call keys into it. This guard's job is to catch a + * future refactor that blindly forwards `...args` — which would silently thread + * model input into the post-scrub `env` merge — NOT to defend a trust boundary + * (the credential scrub in dsh-bash-local is the security control; see the + * bash-stdin-env RFC). Foreground `run()` returns a canned result; `start()` is + * unused here. */ class RecordingBashExecutor extends BashExecutor { readonly requests: BashExecRequest[] = [] @@ -911,12 +915,14 @@ describe('trusted-plugin boundary: the model-facing bash tool never sets env/std return { ctx, bash: ctx.bash as RecordingBashExecutor } } - it('does not forward env/stdin even when the model smuggles them as extra arguments', async () => { + it('does not forward env/stdin even when the model includes them as extra arguments', async () => { const { ctx, bash } = await setupRecording() - // Adversarial args: the model includes `env` and `stdin` keys (and a - // credential-shaped value) hoping they reach the executor. The bash tool's - // schema ignores unknown keys, and execute() builds the request from only - // command/workdir/timeoutMs/signal — so the recorded request carries NEITHER. + // Extra args: the model includes `env` and `stdin` keys hoping they reach the + // executor. The bash tool's schema ignores unknown keys, and execute() builds + // the request from only command/workdir/timeoutMs/signal — so the recorded + // request carries NEITHER. (Not a security wall — the model could set an env + // var or feed stdin via shell syntax anyway; this just keeps the request + // shape honest so a future `...args` spread can't silently forward input.) await ctx.tools.execute({ callId: CallId('boundary-1'), name: 'bash',