diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index 807c7401fc..2e0c8de3a4 100644 --- a/docs/core-data-structures/bash.md +++ b/docs/core-data-structures/bash.md @@ -17,6 +17,23 @@ interface BashExecRequest { timeoutMs?: number | undefined /** Abort signal — implementations kill the command when it fires. */ 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). + */ + 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. + */ + env?: Record | undefined /** * Opaque OWNER token for a background task — the consumer's isolation key * (the tool layer passes the owning agent's `session.header.id`). The @@ -36,6 +53,23 @@ interface BashExecSpec { timeoutMs: number /** Abort signal — implementations kill the command when it fires. */ signal?: AbortSignal | undefined + /** + * Bytes to write to the command's stdin (then close it), carried through + * 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). + */ + stdin?: string | undefined + /** + * Extra environment entries, carried through verbatim from + * {@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. + */ + env?: Record | undefined /** * Opaque owner token, REQUIRED-but-nullable (mirrors `workdir`/`timeoutMs` * being required on the resolved spec): {@link BashExecutor.resolve} carries @@ -50,6 +84,8 @@ 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. + 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. ## Foreground runs: `BashRunResult` diff --git a/docs/rfc/README.md b/docs/rfc/README.md index e27e83842e..9b1996e89b 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -121,6 +121,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Branded IDs everywhere they belong](implemented/architecture/2026-06-20-branded-ids.md) | 2026-06-20 | | [Extract example apps into packages](implemented/architecture/2026-06-20-extract-example-app-packages.md) | 2026-06-20 | | [Event-domain semantics — session is the fact log, agent is the live surface](implemented/architecture/2026-06-30-event-domain-semantics.md) | 2026-06-30 | +| [stdin + extra env on the bash seam — a trusted-plugin surface](implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md) | 2026-06-30 | ### Process 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 new file mode 100644 index 0000000000..ce1c8df0be --- /dev/null +++ b/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md @@ -0,0 +1,33 @@ +# RFC: stdin + extra env on the bash seam — a trusted-plugin surface + +Status: implemented (accepted 2026-06-30) + + + +## Context + +The hooks subsystem (stack PR-A…PR-F) 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 the bridges do not re-implement subprocess plumbing — but the seam had no way to write stdin or set extra env. + +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". + +## Decision + +Add `stdin?: string` and `env?: Record` 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: + +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. + +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. + +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`. + +## 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. + +## Consequences + +The hooks bridges (PR-F) build a `BashExecRequest` with the hook's JSON payload as `stdin` and its `CLAUDE_*`/`PLUGIN_ROOT` vars as `env`, and run 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. diff --git a/packages/bash/bash-local/README.md b/packages/bash/bash-local/README.md index 016f57d2a9..2ae905b628 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** — `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. +- **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). - **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-local/src/index.ts b/packages/bash/bash-local/src/index.ts index 05f1ed75dd..53c369a24c 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -116,6 +116,10 @@ export class LocalBashExecutor extends BashExecutor { workdir: request.workdir ?? this.config.cwd ?? process.cwd(), timeoutMs, ...request.signal ? { signal: request.signal } : {}, + // Carry the trusted-plugin stdin/env through verbatim — optional, no + // config default (absent means none). env merges AFTER the scrub in run.ts. + ...request.stdin !== undefined ? { stdin: request.stdin } : {}, + ...request.env !== undefined ? { env: request.env } : {}, // Carry the owner through verbatim (required-but-nullable on the spec): // the executor never interprets it — the consumer's access policy does. owner: request.owner, @@ -129,6 +133,8 @@ export class LocalBashExecutor extends BashExecutor { timeoutMs: spec.timeoutMs, maxOutputBytes: this.config.maxOutputBytes, signal: spec.signal, + stdin: spec.stdin, + env: spec.env, }, this.internals).done return { ...outcome, timeoutMs: spec.timeoutMs } } @@ -145,6 +151,8 @@ export class LocalBashExecutor extends BashExecutor { timeoutMs: 0, maxOutputBytes: this.config.maxOutputBytes, signal: spec.signal, + stdin: spec.stdin, + env: spec.env, }, this.internals) const id = BashTaskId(`bash-${this.nextTaskId++}`) diff --git a/packages/bash/bash-local/src/run.ts b/packages/bash/bash-local/src/run.ts index 8a8d2065d2..faba210656 100644 --- a/packages/bash/bash-local/src/run.ts +++ b/packages/bash/bash-local/src/run.ts @@ -42,13 +42,24 @@ export const ENV_OVERRIDES = { */ export const SENSITIVE_ENV_PATTERN = /KEY|SECRET|TOKEN/i -/** process.env minus credential-shaped vars, plus the model-friendly overrides. */ -export function childEnv(): NodeJS.ProcessEnv { +/** + * `process.env` minus credential-shaped vars, plus the model-friendly + * overrides, plus any caller-supplied `extra` entries. + * + * Layering matters: the scrub drops `process.env` credentials, then + * `ENV_OVERRIDES` forces the model-friendly terminal vars, then `extra` is + * merged LAST so a TRUSTED-PLUGIN entry wins even when its name matches the + * scrub pattern (the scrub guards against leaking the HARNESS's ambient + * credentials into model-driven commands; an in-process plugin that explicitly + * sets a var has taken responsibility for it). `extra` is NEVER model-supplied + * — `dsh-tool-bash` does not forward model input here (see its module doc). + */ +export function childEnv(extra?: Record): NodeJS.ProcessEnv { const env: NodeJS.ProcessEnv = {} for (const [key, value] of Object.entries(process.env)) { if (!SENSITIVE_ENV_PATTERN.test(key)) env[key] = value } - return { ...env, ...ENV_OVERRIDES } + return { ...env, ...ENV_OVERRIDES, ...extra } } /** What to run and under which limits (resolved — no defaults in here). */ @@ -61,6 +72,18 @@ export interface SpawnSpec { maxOutputBytes: number /** Abort signal — kills the process group when fired. */ signal?: AbortSignal | undefined + /** + * Bytes to write to the child's stdin, then close it. Absent (or empty) + * leaves stdin closed/empty. A TRUSTED-PLUGIN surface (see {@link SpawnSpec}'s + * consumer `dsh-bash`); never carries model input. + */ + stdin?: string | undefined + /** + * Extra environment entries, merged onto the scrubbed env AFTER the + * credential scrub and the model-friendly overrides (so an explicit entry + * wins). A TRUSTED-PLUGIN surface; never carries model input. + */ + env?: Record | undefined } /** Raw outcome of one closed process (before result shaping). */ @@ -272,13 +295,24 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB throw new Error(`aborted before spawn: ${String(spec.signal.reason ?? 'aborted')}`) } + // stdin is ALWAYS a pipe (kept literal so the typed spawn overload guarantees + // non-null stdout/stderr) and is closed immediately: with bytes when a + // trusted plugin supplied stdin, empty otherwise. A closed empty pipe gives a + // reading child EOF exactly as `/dev/null` would, so the no-stdin path (every + // model-driven call) is unchanged. const child = spawn('bash', ['-c', spec.command], { cwd: spec.cwd, - env: childEnv(), - stdio: ['ignore', 'pipe', 'pipe'], + env: childEnv(spec.env), + stdio: ['pipe', 'pipe', 'pipe'], detached: true, }) + // A child that exits without reading stdin makes the write error EPIPE — + // swallow it (the command's outcome rides on its exit code/output, not the + // stdin write) so it never crashes the host or rejects `done`. + child.stdin.on('error', () => { /* EPIPE: child closed stdin early; outcome rides on exit. */ }) + child.stdin.end(spec.stdin ?? '') + const stdout = new OutputCollector(spec.maxOutputBytes, 'stdout', spillDir) const stderr = new OutputCollector(spec.maxOutputBytes, 'stderr', spillDir) child.stdout.on('data', (chunk: Buffer) => { stdout.push(chunk) }) diff --git a/packages/bash/bash-local/tests/executor.spec.ts b/packages/bash/bash-local/tests/executor.spec.ts index 2a27f29731..03f602a2f1 100644 --- a/packages/bash/bash-local/tests/executor.spec.ts +++ b/packages/bash/bash-local/tests/executor.spec.ts @@ -106,6 +106,23 @@ describe('LocalBashExecutor.run', () => { const { bash } = await setup() await expect(bash.run(bash.resolve({ command: 'true', workdir: '/nonexistent-dsh' }))).rejects.toThrow(/ENOENT/) }) + + it('resolve() carries stdin/env onto the spec, and run() threads them to the command', async () => { + const { bash } = await setup() + const spec = bash.resolve({ command: 'cat; echo "[$DSH_SEAM_VAR]"', stdin: 'piped\n', env: { DSH_SEAM_VAR: 'env-ok' } }) + // resolve() keeps the trusted-plugin fields verbatim (optional, no default). + expect(spec.stdin).toBe('piped\n') + expect(spec.env).toEqual({ DSH_SEAM_VAR: 'env-ok' }) + const result = await bash.run(spec) + expect(result.stdout.text).toBe('piped\n[env-ok]\n') + }) + + it('resolve() omits stdin/env when the request supplies neither', async () => { + const { bash } = await setup() + const spec = bash.resolve({ command: 'true' }) + expect('stdin' in spec).toBe(false) + expect('env' in spec).toBe(false) + }) }) describe('LocalBashExecutor background tasks', () => { @@ -131,6 +148,19 @@ describe('LocalBashExecutor background tasks', () => { await Promise.all([first.done, second.done]) }) + it('threads stdin and extra env into a background task', async () => { + const { bash } = await setup() + const task = bash.start(bash.resolve({ + command: 'cat; echo "[$DSH_BG_VAR]"', + stdin: 'bg-stdin\n', + env: { DSH_BG_VAR: 'bg-env' }, + })) + const read = await readUntil(bash, task.id, '[bg-env]') + expect(read.delta).toContain('bg-stdin') + await task.done + expect(task.exitCode).toBe(0) + }) + it('readOutput returns increments without re-delivery', async () => { const { bash } = await setup() const task = bash.start(bash.resolve({ command: 'echo first; sleep 1; echo second' })) diff --git a/packages/bash/bash-local/tests/run.spec.ts b/packages/bash/bash-local/tests/run.spec.ts index b770c4a6c1..3859f2ac39 100644 --- a/packages/bash/bash-local/tests/run.spec.ts +++ b/packages/bash/bash-local/tests/run.spec.ts @@ -158,6 +158,49 @@ describe('runBash', () => { }) }) +describe('stdin and extra env (trusted-plugin surface)', () => { + it('writes stdin to the command and closes it', async () => { + const result = await runBash(spec('cat', { stdin: 'hello from stdin\n' })).done + expect(result.exitCode).toBe(0) + expect(result.stdout.text).toBe('hello from stdin\n') + }) + + it('a command that reads stdin sees EOF when none is supplied', async () => { + // No stdin → the always-piped-but-empty stdin closes immediately, so `cat` + // reads EOF and exits 0 with no output (it does NOT block). + const result = await runBash(spec('cat')).done + expect(result.exitCode).toBe(0) + expect(result.stdout.text).toBe('') + }) + + it('merges extra env entries onto the scrubbed environment', async () => { + const result = await runBash(spec('echo "$DSH_EXTRA_ONE/$DSH_EXTRA_TWO"', { + env: { DSH_EXTRA_ONE: 'alpha', DSH_EXTRA_TWO: 'beta' }, + })).done + expect(result.stdout.text).toBe('alpha/beta\n') + }) + + it('an explicit extra env entry overrides the model-friendly override and the scrub', async () => { + // TERM is a model-friendly OVERRIDE (dumb); an explicit extra entry wins. + // DSH_OVERRIDE_KEY matches the credential scrub pattern, yet an explicit + // entry is still honored — the scrub only drops AMBIENT process.env creds. + const result = await runBash(spec('echo "$TERM/$DSH_OVERRIDE_KEY"', { + env: { TERM: 'xterm-256color', DSH_OVERRIDE_KEY: 'explicit-wins' }, + })).done + expect(result.stdout.text).toBe('xterm-256color/explicit-wins\n') + }) + + it('does not crash or reject when the child ignores a large stdin (EPIPE)', async () => { + // The child exits immediately without reading; closing our end of a stdin + // pipe still holding ~1MiB triggers EPIPE on the write. The handler must + // swallow it: `done` resolves normally with the child's real exit. + const big = 'x'.repeat(1024 * 1024) + const result = await runBash(spec('exit 7', { stdin: big })).done + expect(result.exitCode).toBe(7) + expect(result.aborted).toBe(false) + }) +}) + describe('output truncation and spill', () => { it('keeps the tail and spills the full stream to disk', async () => { // 200 numbered lines of ~10 bytes; cap at 500 bytes keeps a late tail. diff --git a/packages/bash/bash/README.md b/packages/bash/bash/README.md index 6123565e7a..cec9e5834a 100644 --- a/packages/bash/bash/README.md +++ b/packages/bash/bash/README.md @@ -28,4 +28,6 @@ Implementations subclass `BashExecutor`, implement the abstract methods, and cal ## Vocabulary -`BashExecRequest` (command, workdir?, timeoutMs?, signal?, owner?) resolves to `BashExecSpec` (command, workdir, timeoutMs, signal?, 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. +`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). diff --git a/packages/bash/bash/src/types.ts b/packages/bash/bash/src/types.ts index d9ab9f9b4d..f5be9f11fe 100644 --- a/packages/bash/bash/src/types.ts +++ b/packages/bash/bash/src/types.ts @@ -45,6 +45,23 @@ export interface BashExecRequest { timeoutMs?: number | undefined /** Abort signal — implementations kill the command when it fires. */ 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). + */ + 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. + */ + env?: Record | undefined /** * Opaque OWNER token for a background task — the consumer's isolation key * (the tool layer passes the owning agent's `session.header.id`). The @@ -70,6 +87,23 @@ export interface BashExecSpec { timeoutMs: number /** Abort signal — implementations kill the command when it fires. */ signal?: AbortSignal | undefined + /** + * Bytes to write to the command's stdin (then close it), carried through + * 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). + */ + stdin?: string | undefined + /** + * Extra environment entries, carried through verbatim from + * {@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. + */ + env?: Record | undefined /** * Opaque owner token, REQUIRED-but-nullable (mirrors `workdir`/`timeoutMs` * being required on the resolved spec): {@link BashExecutor.resolve} carries diff --git a/packages/bash/tool-bash/README.md b/packages/bash/tool-bash/README.md index 656a22cdb1..f7a15894f9 100644 --- a/packages/bash/tool-bash/README.md +++ b/packages/bash/tool-bash/README.md @@ -40,6 +40,10 @@ 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 `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). + ## Permissions `TODO(permissions)`: commands run with the executor's full authority. The permission/sandbox seam is the `tools/execute` waterfall (veto or ask) plus sandboxing `BashExecutor` implementations — see docs/architecture.md. `@cordisjs/plugin-capability` (a named-permission service with a session `test()`) is a candidate building block for that work. diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index 9de079fd33..0845163193 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -863,3 +863,99 @@ describe('tool-owned UI presentation (presentCall / presentResult)', () => { expect(ctx.tools.get('bash')?.presentCall?.({ command: 'ls' })).toBeUndefined() }) }) + +describe('trusted-plugin boundary: the model-facing bash tool never sets env/stdin', () => { + /** + * 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. + */ + class RecordingBashExecutor extends BashExecutor { + readonly requests: BashExecRequest[] = [] + resolve(request: BashExecRequest): BashExecSpec { + this.requests.push(request) + return { + command: request.command, + workdir: request.workdir ?? process.cwd(), + timeoutMs: request.timeoutMs ?? 0, + ...request.signal ? { signal: request.signal } : {}, + ...request.stdin !== undefined ? { stdin: request.stdin } : {}, + ...request.env !== undefined ? { env: request.env } : {}, + owner: request.owner, + } + } + run(): Promise { + return Promise.resolve({ + exitCode: 0, signal: null, timedOut: false, aborted: false, timeoutMs: 0, + stdout: { text: 'ok', truncated: false }, stderr: { text: '', truncated: false }, + }) + } + start(): BashTask { throw new Error('unused') } + get(): BashTask | undefined { return undefined } + ownerOf(): OwnerToken | undefined { return undefined } + list(): BashTask[] { return [] } + readOutput(): BashTaskRead { throw new Error('unused') } + kill(): boolean { return false } + } + + async function setupRecording() { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(AgentRegistry) + await ctx.plugin(RecordingBashExecutor) + await ctx.plugin(ToolBash) + return { ctx, bash: ctx.bash as RecordingBashExecutor } + } + + it('does not forward env/stdin even when the model smuggles 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. + await ctx.tools.execute({ + callId: CallId('boundary-1'), + name: 'bash', + arguments: { + command: 'echo hi', + description: 'echo', + env: { SNEAKY_API_KEY: 'leak' }, + stdin: 'malicious payload', + }, + }) + expect(bash.requests).toHaveLength(1) + const request = bash.requests[0]! + expect(request.command).toBe('echo hi') + expect('env' in request).toBe(false) + expect('stdin' in request).toBe(false) + }) + + it('a background bash call likewise carries no env/stdin', async () => { + const { ctx, bash } = await setupRecording() + // start() throws in this recorder, but resolve() runs first and records the + // request — which is all this boundary assertion needs. + await ctx.tools.execute({ + callId: CallId('boundary-2'), + name: 'bash', + arguments: { + command: 'sleep 1', + description: 'sleep', + run_in_background: true, + env: { TOKEN: 'leak' }, + stdin: 'x', + }, + }) + expect(bash.requests).toHaveLength(1) + const request = bash.requests[0]! + expect('env' in request).toBe(false) + expect('stdin' in request).toBe(false) + // The owner token IS set on a background call (the isolation fence) — proving + // the recorder sees the real request the consumer built, so the absent + // env/stdin above is a real negative, not a recorder that drops everything. + expect('owner' in request).toBe(true) + }) +})