docs(bash): reframe stdin/env — the scrub is the security control, not a trust boundary
Address review: the "trusted-plugin surface" framing overstated the security story. A model driving the `bash` tool already has equivalent power to set env vars and feed stdin through ordinary shell syntax (`FOO=bar cmd`, heredocs), so the `env`/`stdin` seam fields grant it no new capability — and they cannot exfiltrate the harness's ambient credentials, because the credential SCRUB in dsh-bash-local (which strips *KEY*/*SECRET*/*TOKEN* from process.env before the child sees it) is the actual control, and it works regardless of these fields (tool-call args are static JSON, never shell-evaluated). So drop the "dangerous / trusted-plugin boundary" language across the RFC, the three bash-package READMEs, the bash/src/types.ts JSDoc, and docs/bash.md (both the type-equiv blocks — kept 1:1 with source — and the prose). The reality that remains: the `bash` tool doesn't EXPOSE env/stdin as parameters because they'd be redundant with shell syntax; the fields exist for in-process plugins (the hooks bridges) to pass a JSON payload + CLAUDE_* vars cleanly. The guard test is kept but reframed: it catches a future `...args` spread that would silently forward model input into the post-scrub env merge, NOT a trust wall. No code or behavior change.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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<string, string> | 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<string, string> | undefined
|
||||
/**
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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',
|
||||
|
||||
Reference in New Issue
Block a user