diff --git a/.changeset/selectable-exec-shell.md b/.changeset/selectable-exec-shell.md new file mode 100644 index 00000000..7cedaf8d --- /dev/null +++ b/.changeset/selectable-exec-shell.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/computerd": minor +--- + +The exec runner now takes an optional shell naming the interpreter each command runs under, and computerd reads the same value from EXEC_SHELL. Both default to /bin/sh, so existing behavior is unchanged. On a Debian-family image /bin/sh is dash, where bash-only syntax such as the PIPESTATUS array is a parse error that aborts the command rather than a missing feature, and that array is how a caller recovers the real exit status of a pipeline whose output it filters. Repointing /bin/sh in the image was the only previous workaround, which changes echo semantics for every other script in that image and is unavailable when the image is prebuilt. diff --git a/packages/computerd/README.md b/packages/computerd/README.md index 64345557..afa24eaf 100644 --- a/packages/computerd/README.md +++ b/packages/computerd/README.md @@ -113,10 +113,13 @@ Additional environment variables: ```sh EXEC_LOG_MAX_BYTES=1048576 # cap the in-memory exec log buffer (bytes) +EXEC_SHELL=/usr/bin/bash # interpreter exec runs commands under (default /bin/sh) RPC_CLIENT_SECRET= # require Authorization: Bearer on every route but /health COMPUTER_VAR_NODE_ENV=production # forwarded into exec as NODE_ENV ``` +`EXEC_SHELL` must be an absolute path. It exists because `/bin/sh` is `dash` on a Debian-family image, where bash-only syntax is a parse error that aborts the command rather than a missing feature: `${PIPESTATUS[@]}`, arrays, `[[ ... ]]`, and process substitution all fail that way. `PIPESTATUS` is the usual way to recover the real exit status of a pipeline whose output is filtered — a command redacting a credential through `sed`, for instance — so a caller that needs it can select an interpreter that has it without repointing `/bin/sh` for every other script in the image. + `FUSE_MOUNT=auto` is the friendly default: if `/dev/fuse` (or macFUSE) is available `computerd` mounts a real FUSE filesystem, otherwise it transparently falls back to the userspace shim. Pin the value (`fuse` / `macfuse` / `shim` / `none`) when a test needs to assert a specific code path. ## `FUSE_MOUNT=shim` — userspace dev shim diff --git a/packages/computerd/src/cli/computerd.ts b/packages/computerd/src/cli/computerd.ts index 5036db4b..8afd4513 100644 --- a/packages/computerd/src/cli/computerd.ts +++ b/packages/computerd/src/cli/computerd.ts @@ -636,12 +636,23 @@ async function main(): Promise { } logMaxBytesOverride = parsed; } + // EXEC_SHELL picks the interpreter exec runs commands under, for images + // whose /bin/sh cannot be repointed. Default lives in the Runner. + const shellEnv = process.env.EXEC_SHELL; + let shellOverride: string | undefined; + if (shellEnv !== undefined && shellEnv !== "") { + if (!shellEnv.startsWith("/")) { + throw new Error(`EXEC_SHELL must be an absolute path; got ${JSON.stringify(shellEnv)}`); + } + shellOverride = shellEnv; + } const runner = new Runner({ db, // When we have a mount (real FUSE or the shim) point spawned // children at it so writes from exec flow through the VFS. ...(fuse !== undefined ? { cwd: mountPoint } : {}), ...(logMaxBytesOverride !== undefined ? { logMaxBytes: logMaxBytesOverride } : {}), + ...(shellOverride !== undefined ? { shell: shellOverride } : {}), }); // Heartbeat events are computerd-local and must not cross the RPC boundary. // Wrap the runner so every exec/get stream drops heartbeat events before diff --git a/packages/computerd/src/exec/runner.test.ts b/packages/computerd/src/exec/runner.test.ts index 09f417da..4bc6f0d9 100644 --- a/packages/computerd/src/exec/runner.test.ts +++ b/packages/computerd/src/exec/runner.test.ts @@ -1,3 +1,5 @@ +import { existsSync } from "node:fs"; + import { Database, initializeSchema, WorkspaceFilesystem } from "@cloudflare/dofs"; import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; import { expect, test } from "vitest"; @@ -517,3 +519,74 @@ test("a spawned command sees the allowlisted environment, not the daemon's", asy dispose(); } }); + +// The interpreter is a per-consumer choice, not a property of the image. +// +// A caller that redacts credentials through a pipe -- `git push "$URL" 2>&1 | +// sed -E 's#//[^@]*@#//***@#'` -- gets the pipeline's last exit status, so a +// failed push reads as success. PIPESTATUS is the usual recovery, and under +// dash it is a parse error that aborts the command rather than a missing +// feature, which is worse than the problem it was reached for. +const hasBash = existsSync("/usr/bin/bash"); + +test("defaults to /bin/sh when no shell is given", async () => { + const { runner, dispose } = fixture(); + try { + const handle = runner.exec("printf '%s' \"$0\""); + const events = await drain(handle.events); + const stdout = events + .filter((event) => event.name === "stdout") + .map((event) => decode(event.value as Uint8Array)) + .join(""); + expect(stdout).toBe("/bin/sh"); + } finally { + dispose(); + } +}); + +test.skipIf(!hasBash)("runs commands under an explicitly chosen shell", async () => { + const { runner, dispose } = fixture({ shell: "/usr/bin/bash" }); + try { + const handle = runner.exec("printf '%s' \"$0\""); + const events = await drain(handle.events); + const stdout = events + .filter((event) => event.name === "stdout") + .map((event) => decode(event.value as Uint8Array)) + .join(""); + expect(stdout).toBe("/usr/bin/bash"); + } finally { + dispose(); + } +}); + +test.skipIf(!hasBash)("a chosen shell resolves PIPESTATUS instead of aborting", async () => { + const { runner, dispose } = fixture({ shell: "/usr/bin/bash" }); + try { + // false | true leaves $? as true's 0 while the first pipeline stage's real + // failure survives in the PIPESTATUS array. The trailing marker proves the + // command was not aborted: under dash the expansion is fatal and "after" + // never prints. The expansion is assembled from parts so its braces are + // not linted as a JavaScript template placeholder. + const first = ['"$', "{PIPESTATUS[0]}", '"'].join(""); + const handle = runner.exec(`false | true; printf '[%s]' ${first}; printf "after"`); + const events = await drain(handle.events); + const stdout = events + .filter((event) => event.name === "stdout") + .map((event) => decode(event.value as Uint8Array)) + .join(""); + expect(stdout).toBe("[1]after"); + } finally { + dispose(); + } +}); + +test("rejects a shell that is not an absolute path", () => { + const storage = new SQLiteTestStorage(); + const db = new Database(storage); + initializeSchema(db, () => Date.now()); + try { + expect(() => new Runner({ db, shell: "bash" })).toThrow(/absolute path/); + } finally { + storage.close?.(); + } +}); diff --git a/packages/computerd/src/exec/runner.ts b/packages/computerd/src/exec/runner.ts index 21333eaa..6904e8d7 100644 --- a/packages/computerd/src/exec/runner.ts +++ b/packages/computerd/src/exec/runner.ts @@ -61,6 +61,7 @@ const DEFAULTS = { // After SIGTERM, give the child this long to exit on its own // before sending SIGKILL. killGraceMs: 5_000, + shell: "/bin/sh", } as const; export interface RunnerInit extends RunnerOptions { @@ -80,6 +81,7 @@ export class Runner { sweepIntervalMs: number; defaultTimeoutMs: number; heartbeatIntervalMs: number; + shell: string; now: () => number; }; private readonly records = new Map(); @@ -96,8 +98,14 @@ export class Runner { sweepIntervalMs: init.sweepIntervalMs ?? DEFAULTS.sweepIntervalMs, defaultTimeoutMs: init.defaultTimeoutMs ?? DEFAULTS.defaultTimeoutMs, heartbeatIntervalMs: init.heartbeatIntervalMs ?? 0, + shell: init.shell ?? DEFAULTS.shell, now: init.now ?? Date.now, }; + // Fail here rather than letting every exec() surface a bare ENOENT from + // spawn, which names the interpreter but not the misconfiguration. + if (!this.opts.shell.startsWith("/")) { + throw new Error(`shell must be an absolute path; got ${JSON.stringify(this.opts.shell)}`); + } initializeExecSchema(this.db); if (init.resetSchema !== false) clearExecState(this.db); } @@ -131,7 +139,7 @@ export class Runner { // status pipe. If cwd lives inside computerd's own FUSE mount, the // child's chdir issues a FUSE LOOKUP that computerd can't service // (its event loop is stuck in uv_spawn), and the whole - // process deadlocks. Have /bin/sh do the chdir instead: by + // process deadlocks. Have the shell do the chdir instead: by // the time the shell runs its `cd`, computerd's event loop is back // and can answer the FUSE callback normally. // @@ -143,7 +151,7 @@ export class Runner { // (`spawn("/bin/sh", ["-c", command])`) was already a shell- // owned exec, so this is no change in process shape. const wrapped = cwd !== undefined ? `cd ${shellQuote(cwd)} && ${command}` : command; - const child = spawn("/bin/sh", ["-c", wrapped], { + const child = spawn(this.opts.shell, ["-c", wrapped], { env, stdio: [options.stdin === undefined ? "ignore" : "pipe", "pipe", "pipe"], }); diff --git a/packages/computerd/src/exec/types.ts b/packages/computerd/src/exec/types.ts index 68fc5cbf..5e222392 100644 --- a/packages/computerd/src/exec/types.ts +++ b/packages/computerd/src/exec/types.ts @@ -62,6 +62,13 @@ export interface RunnerOptions { // Emit a heartbeat event every this many milliseconds while a child // process is alive. When unset or zero, no heartbeat events are emitted. heartbeatIntervalMs?: number; + // Absolute path to the interpreter each command runs under. Defaults to + // /bin/sh, which on a Debian-family image is dash: bash-only syntax a + // caller may reach for, notably ${PIPESTATUS[@]}, is a parse error there + // and aborts the command rather than degrading. Not inferred from SHELL, + // which names the caller's login shell and is deliberately absent from the + // env allowlist. + shell?: string; // Test seam: replaces Date.now() for retention math and log ts. now?: () => number; }