Files

Tianyi Cui bb9ae2ba99 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.

2026-07-02 04:08:24 +08:00

6.9 KiB

Raw Blame History

RFC: stdin + extra env on the bash seam

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 → dsh-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.

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's childEnv(), which strips *KEY*/*SECRET*/*TOKEN* from process.env before the child sees it (see 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

Add stdin?: string and env?: Record<string, string> to both BashExecRequest (the model-/plugin-facing request) and BashExecSpec (the resolved spec run/start act on), and thread them through dsh-bash-local: resolve() carries them verbatim, run()/start() pass them to runBash, which writes the bytes to the child's stdin and merges the extra env.

Three deliberate choices:

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'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).
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.
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 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 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 § "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 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 (the type-equiv request/spec blocks) and the three bash-package READMEs.

6.9 KiB Raw Blame History