From d4b52270717d8db207e0fac694a3964bf6d66916 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Sun, 12 Jul 2026 17:03:11 +0800 Subject: [PATCH] fix(bash): validate managed env namespace --- ...7-10-agent-session-identity-and-log-location.md | 2 +- packages/bash/bash-local/README.md | 2 +- packages/bash/bash-local/src/run.ts | 14 ++++++++++---- packages/bash/bash-local/tests/run.spec.ts | 7 +++++++ packages/bash/bash/src/types.ts | 5 +++-- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.md b/docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.md index 9127d70207..118009dedb 100644 --- a/docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.md +++ b/docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.md @@ -38,7 +38,7 @@ The registry rebuilds a trusted overlay for every foreground and background bash Session persistence remains the fact owner: JSONL does not depend on tool-bash or register shell variables itself, and hooks continue to consume `locate()` directly. Tool-bash is the translation layer from the persistence fact into a shell convention. Other plugins that need shell-visible facts depend on the registry and register their own keys; they do not modify `process.env`. -The bash seam exports `DSH_ENV_PREFIX` as the single namespace source and derives `DshEnvironmentKey` from its `typeof`. Tool-bash derives built-in names and model guidance from that constant, while executors use it for filtering and ordinary-env rejection. The seam carries the managed overlay separately as `BashExecRequest.dshEnv` / `BashExecSpec.dshEnv`. Ordinary `env` remains the general in-process plugin surface used by hooks, but cannot contain managed keys; the local executor rejects that wrong channel, removes every inherited ambient managed key, applies its ordinary scrub/terminal environment/explicit `env`, and finally merges the trusted `dshEnv` snapshot. This guarantees that a missing value means absent now rather than inherited from an outer or previous harness. The model-facing tool still ignores model-supplied `env`/`stdin` arguments. +The bash seam exports `DSH_ENV_PREFIX` as the single namespace source and derives `DshEnvironmentKey` from its `typeof`. Tool-bash derives built-in names and model guidance from that constant, while executors use it for filtering and channel validation. The seam carries the managed overlay separately as `BashExecRequest.dshEnv` / `BashExecSpec.dshEnv`. Ordinary `env` remains the general in-process plugin surface used by hooks, but cannot contain managed keys; symmetrically, `dshEnv` cannot contain ordinary keys. The local executor rejects either wrong channel before spawn, removes every inherited ambient managed key, applies its ordinary scrub/terminal environment/explicit `env`, and finally merges the trusted `dshEnv` snapshot. This guarantees that a missing value means absent now rather than inherited from an outer or previous harness. The model-facing tool still ignores model-supplied `env`/`stdin` arguments. The bash tool description teaches only the durable convention: current harness environment facts are available through managed `$DSH_*` variables and may be inspected when needed. It does not enumerate persistence-specific keys or add a permanent system-prompt section. Tool schemas are already logged in request headers and tool output is logged as `tool/result`, so no new session event is required. diff --git a/packages/bash/bash-local/README.md b/packages/bash/bash-local/README.md index 9a00dc4a9b..2efc76d77d 100644 --- a/packages/bash/bash-local/README.md +++ b/packages/bash/bash-local/README.md @@ -22,7 +22,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 the `graceMs` grace (default 3s — 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/namespace scrub** — start with `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`) and every `DSH_*`, then apply `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat`. Ordinary spec `env` is merged next and may restore caller-held credential-shaped values, but is rejected if it tries to set reserved `DSH_*`; the trusted spec `dshEnv` snapshot is merged last. This keeps ambient secrets out and prevents a nested/previous harness identity from surviving when the current registry omits it. The spec's `stdin`, when supplied, is written to the child and closed; with none supplied, fd 0 is `/dev/null`. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md) and [the session environment RFC](../../../docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.md). +- **Model-friendly env + credential/namespace scrub** — start with `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`) and every `DSH_*`, then apply `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat`. Ordinary spec `env` is merged next and may restore caller-held credential-shaped values, but is rejected if it tries to set reserved `DSH_*`; the managed spec `dshEnv` snapshot is rejected if it contains ordinary names and otherwise merges last. This keeps ambient secrets out, catches wrong-channel plugin configuration before spawn, and prevents a nested/previous harness identity from surviving when the current registry omits it. The spec's `stdin`, when supplied, is written to the child and closed; with none supplied, fd 0 is `/dev/null`. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md) and [the session environment RFC](../../../docs/rfc/implemented/feature/2026-07-10-agent-session-identity-and-log-location.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/run.ts b/packages/bash/bash-local/src/run.ts index 9cd9ac2553..6b1f2064c0 100644 --- a/packages/bash/bash-local/src/run.ts +++ b/packages/bash/bash-local/src/run.ts @@ -58,10 +58,11 @@ export const SENSITIVE_ENV_PATTERN = /KEY|SECRET|TOKEN/i * `ENV_OVERRIDES` then forces model-friendly terminal values, ordinary `extra` * follows, and `dshEnv` merges last. Ordinary `extra` may restore a * credential-shaped name whose value the caller already holds, but cannot set - * the managed namespace. `dsh-tool-bash` builds both channels from trusted - * named fields and never forwards model-provided environment objects. + * the managed namespace; `dshEnv` rejects ordinary names symmetrically. + * `dsh-tool-bash` builds both channels from trusted named fields and never + * forwards model-provided environment objects. * @param extra - ordinary caller-supplied entries; `DSH_*` names are rejected. - * @param dshEnv - trusted managed `DSH_*` entries for the current execution. + * @param dshEnv - managed entries; names outside `DSH_*` are rejected. * @returns the environment to hand to `spawn` for the child process. */ export function childEnv( @@ -77,6 +78,11 @@ export function childEnv( throw new Error(`ordinary bash env cannot set reserved variable "${key}"; use dshEnv`) } } + for (const key of Object.keys(dshEnv ?? {})) { + if (!key.startsWith(DSH_ENV_PREFIX)) { + throw new Error(`managed bash env cannot set ordinary variable "${key}"; use env`) + } + } return { ...env, ...ENV_OVERRIDES, ...extra, ...dshEnv } } @@ -107,7 +113,7 @@ export interface SpawnSpec { * terminal overrides. `DSH_*` names are rejected and belong in `dshEnv`. */ env?: Record | undefined - /** Harness-owned `DSH_*` entries merged after ambient `DSH_*` removal. */ + /** Harness-owned entries; non-`DSH_*` names are rejected before spawn. */ dshEnv?: DshEnvironment | undefined } diff --git a/packages/bash/bash-local/tests/run.spec.ts b/packages/bash/bash-local/tests/run.spec.ts index 48f00a861b..1726e30743 100644 --- a/packages/bash/bash-local/tests/run.spec.ts +++ b/packages/bash/bash-local/tests/run.spec.ts @@ -4,6 +4,7 @@ import { dirname, join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { killGroup, OutputCollector, runBash } from '@deepseek-ai/dsh-bash-local' import type { RunningBash } from '@deepseek-ai/dsh-bash-local' +import type { DshEnvironment } from '@deepseek-ai/dsh-bash' const { failNextClose } = vi.hoisted(() => ({ failNextClose: { value: false } })) vi.mock('node:fs', async (importOriginal) => { @@ -392,6 +393,12 @@ describe('review fixes: env scrubbing and spill hardening', () => { .toThrow(/DSH_WRONG_CHANNEL.*dshEnv/) }) + it('rejects ordinary variables on the managed env channel', () => { + const invalid = { PATH: '/wrong-channel' } as unknown as DshEnvironment + expect(() => runBash(spec('true', { dshEnv: invalid }))) + .toThrow(/managed bash env.*PATH.*use env/) + }) + it('creates spill files with owner-only permissions and random names', async () => { const result = await runBash( spec('for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', { maxOutputBytes: 500 }), diff --git a/packages/bash/bash/src/types.ts b/packages/bash/bash/src/types.ts index 479b968cfc..a3a6bba116 100644 --- a/packages/bash/bash/src/types.ts +++ b/packages/bash/bash/src/types.ts @@ -125,7 +125,8 @@ export interface BashExecRequest { /** * Harness-owned `DSH_*` variables for this execution. Executors discard * ambient `DSH_*` entries before merging this snapshot, so an unavailable - * current fact cannot inherit a stale value from the harness process. + * current fact cannot inherit a stale value from the harness process, and + * reject non-`DSH_*` names supplied through this managed channel. */ dshEnv?: DshEnvironment | undefined /** @@ -182,7 +183,7 @@ export interface BashExecSpec { * ordinary extra environment. */ env?: Record | undefined - /** Trusted `DSH_*` snapshot carried through from {@link BashExecRequest.dshEnv}. */ + /** Managed `DSH_*` snapshot; implementations reject ordinary names. */ dshEnv?: DshEnvironment | undefined /** * Opaque owner token, REQUIRED-but-nullable (mirrors `workdir`/`timeoutMs`