diff --git a/packages/adapter-utils/src/execution-target-stdin-race.test.ts b/packages/adapter-utils/src/execution-target-stdin-race.test.ts index 26fb6a0ff9..225adc00cc 100644 --- a/packages/adapter-utils/src/execution-target-stdin-race.test.ts +++ b/packages/adapter-utils/src/execution-target-stdin-race.test.ts @@ -1,10 +1,10 @@ import { execFile as execFileCallback, spawn } from "node:child_process"; -import { mkdir, mkdtemp, readFile, readdir, rename, rm, writeFile } from "node:fs/promises"; +import { chmod, mkdir, mkdtemp, readFile, readdir, rename, rm, writeFile } from "node:fs/promises"; import net from "node:net"; import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterAll, afterEach, describe, expect, it } from "vitest"; import { getProcessSessionRemoteSource, @@ -480,11 +480,11 @@ describe("stdin file race (parent PAP-4037)", () => { await waitFor(() => finalizeStarted.includes("000000000001.json"), 8_000); // Stop the bridge while file 1's write is still pending. `stop()` awaits - // the chained `stdinEnd` write, so both finalizes are complete when it - // returns, in send order. + // the chained `stdinEnd` write, then chains a `shutdown` write after it, + // so all three finalizes are complete when it returns, in send order. await bridge!.stop(); stopped = true; - expect(finalizeOrder).toEqual(["000000000001.json", "000000000002.json"]); + expect(finalizeOrder).toEqual(["000000000001.json", "000000000002.json", "000000000003.json"]); } finally { peer?.destroy(); if (!stopped) await bridge?.stop(); @@ -621,3 +621,1197 @@ describe("stdin file race (parent PAP-4037)", () => { expect(observedComplete).toBeGreaterThan(0); }); }); + +// Coverage for deterministic wrapper shutdown (parent PAP-5307): a bridge +// stop must leave no remote wrapper process and no direct child process +// alive, the host must send no operating-system signal, and the host must +// store no process identifier. These tests drive the real emitted wrapper as +// a node process and, where noted, the real bridge through a local runner. +describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () => { + const cleanupDirs: string[] = []; + + afterEach(async () => { + while (cleanupDirs.length > 0) { + const dir = cleanupDirs.pop(); + if (dir) await rm(dir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); + } + + async function waitFor(check: () => boolean | Promise, timeoutMs = 8_000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (await check()) return; + await delay(20); + } + throw new Error("Timed out waiting for condition."); + } + + function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } + } + + // A read-only, test-only liveness probe over the real OS process table. It + // never signals anything; it only greps `ps` output to tell the test + // whether a specific test-authored script (identified by its own temp file + // path) is still running. Production host code never does this — it never + // matches or signals a process by name or command line. + async function findLivePidsByArgvSubstring(substring: string): Promise { + try { + const { stdout } = await execFile("ps", ["-eo", "pid=,args="]); + const pids: number[] = []; + for (const line of stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + const match = /^(\d+)\s+(.*)$/.exec(trimmed); + if (match && match[2].includes(substring)) { + const pid = Number.parseInt(match[1], 10); + if (Number.isFinite(pid)) pids.push(pid); + } + } + return pids; + } catch { + return []; + } + } + + type WrapperFrame = { + seq?: number; + type?: string; + stream?: string; + data?: string; + code?: number | null; + signal?: string | null; + message?: string; + }; + + // A test-only preload module for the wrapper's node process (PAP-5338). + // This sandbox's filesystems all report a real, working birthtime, so a + // test cannot reach the two known "no usable creation time" fallbacks by + // using a real filesystem alone. This preload patches fs.promises.lstat + // inside the wrapper's own process, for one directory the test names + // through an env var, so the wrapper observes the exact Stats shape each + // fallback produces. It never runs unless a test opts in, and it never + // touches this test file's own process. + // Kept outside cleanupDirs (which afterEach drains after every single test): + // this preload file is created once and reused by every test in this + // describe block, so an early test's cleanup must not delete it out from + // under a later test. + let fakeBirthtimePreloadDir: string | null = null; + afterAll(async () => { + if (fakeBirthtimePreloadDir) await rm(fakeBirthtimePreloadDir, { recursive: true, force: true }).catch(() => undefined); + }); + let fakeBirthtimePreloadPath: Promise | null = null; + async function getFakeBirthtimePreloadPath(): Promise { + if (!fakeBirthtimePreloadPath) { + fakeBirthtimePreloadPath = (async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), "paperclip-birthtime-preload-")); + fakeBirthtimePreloadDir = dir; + const preloadPath = path.join(dir, "fake-birthtime-preload.cjs"); + await writeFile( + preloadPath, + [ + `const fs = require("fs");`, + `const path = require("path");`, + `const target = process.env.PAPERCLIP_TEST_FAKE_BIRTHTIME_TARGET;`, + `const mode = process.env.PAPERCLIP_TEST_FAKE_BIRTHTIME_MODE;`, + `const sessionDir = process.env.PAPERCLIP_PROCESS_SESSION_DIR;`, + `if (target && mode && sessionDir) {`, + ` const resolvedTarget = path.resolve(target === "stdinDir" ? path.join(sessionDir, "stdin") : sessionDir);`, + ` const originalLstat = fs.promises.lstat.bind(fs.promises);`, + ` fs.promises.lstat = async (candidatePath, opts) => {`, + ` const stats = await originalLstat(candidatePath, opts);`, + ` if (path.resolve(String(candidatePath)) === resolvedTarget) {`, + ` const fakeValue = mode === "zero" ? 0 : stats.ctimeMs;`, + ` Object.defineProperty(stats, "birthtimeMs", { value: fakeValue, configurable: true });`, + ` }`, + ` return stats;`, + ` };`, + `}`, + ].join("\n"), + "utf8", + ); + return preloadPath; + })(); + } + return fakeBirthtimePreloadPath; + } + + // Run the real emitted wrapper (either variant) as a node process, with no + // sandbox and no bridge in front of it. The test owns the wrapper's node + // ChildProcess handle directly, so it can observe the wrapper's own exit + // without storing or signaling any process identifier itself. + async function startWrapperProcess(options?: { + outputToStdout?: boolean; + command?: string; + args?: string[]; + maxRetries?: number; + terminateGraceMs?: number; + // A dedicated parent for this wrapper's session directory, instead of the + // shared OS temp directory. A test that must chmod sessionDir's own + // parent (to force EACCES on sessionDir itself) needs a parent it owns, + // never the shared OS temp directory every other process on the host + // also uses. + parentDir?: string; + // Makes the wrapper's own process observe an unusable birthtimeMs on one + // control directory, through the preload above. See PAP-5338 AC-1: a + // real "no usable creation time" filesystem is not reachable in this + // sandbox, so the test simulates the exact Stats shape instead. + fakeBirthtime?: { target: "sessionDir" | "stdinDir"; mode: "zero" | "followCtime" }; + }) { + const sessionDir = await mkdtemp(path.join(options?.parentDir ?? os.tmpdir(), "paperclip-wrapper-lifecycle-")); + cleanupDirs.push(sessionDir); + const stdinDir = path.join(sessionDir, "stdin"); + const eventsDir = path.join(sessionDir, "events"); + await mkdir(stdinDir, { recursive: true }); + if (options?.outputToStdout !== true) await mkdir(eventsDir, { recursive: true }); + + const wrapperPath = path.join(sessionDir, "wrapper.mjs"); + await writeFile(wrapperPath, getProcessSessionRemoteSource({ outputToStdout: options?.outputToStdout === true }), "utf8"); + + const config = { command: options?.command ?? "cat", args: options?.args ?? [], cwd: sessionDir, env: {} }; + const commandPayload = Buffer.from(JSON.stringify(config), "utf8").toString("base64"); + + const env: Record = { + ...process.env, + PAPERCLIP_PROCESS_SESSION_DIR: sessionDir, + PAPERCLIP_PROCESS_SESSION_COMMAND_B64: commandPayload, + }; + if (options?.maxRetries != null) env.PAPERCLIP_PROCESS_SESSION_STDIN_MAX_RETRIES = String(options.maxRetries); + if (options?.terminateGraceMs != null) env.PAPERCLIP_PROCESS_SESSION_TERMINATE_GRACE_MS = String(options.terminateGraceMs); + + const execArgv: string[] = []; + if (options?.fakeBirthtime) { + env.PAPERCLIP_TEST_FAKE_BIRTHTIME_TARGET = options.fakeBirthtime.target; + env.PAPERCLIP_TEST_FAKE_BIRTHTIME_MODE = options.fakeBirthtime.mode; + execArgv.push("--require", await getFakeBirthtimePreloadPath()); + } + + const child = spawn(process.execPath, [...execArgv, wrapperPath], { + cwd: sessionDir, + env, + stdio: ["ignore", "pipe", "pipe"], + }); + + const frames: WrapperFrame[] = []; + let stdoutBuffer = ""; + child.stdout.on("data", (chunk: Buffer) => { + stdoutBuffer += chunk.toString("utf8"); + const lines = stdoutBuffer.split("\n"); + stdoutBuffer = lines.pop() || ""; + for (const line of lines) { + if (!line.trim()) continue; + try { + frames.push(JSON.parse(line) as WrapperFrame); + } catch { + // A partial line at a chunk boundary; ignore. + } + } + }); + let stderrText = ""; + child.stderr.on("data", (chunk: Buffer) => { + stderrText += chunk.toString("utf8"); + }); + + let exitCode: number | null = null; + let exitSignal: NodeJS.Signals | null = null; + const exited = new Promise((resolve) => { + child.on("close", (code, signal) => { + exitCode = code; + exitSignal = signal; + resolve(); + }); + }); + + async function readEventFiles(): Promise { + const names = (await readdir(eventsDir).catch(() => [])).filter((name) => name.endsWith(".json")).sort(); + const out: WrapperFrame[] = []; + for (const name of names) { + const body = await readFile(path.join(eventsDir, name), "utf8").catch(() => ""); + if (!body.trim()) continue; + try { + out.push(JSON.parse(body) as WrapperFrame); + } catch { + // Not fully written yet; the test polls again. + } + } + return out; + } + + return { + sessionDir, + stdinDir, + eventsDir, + wrapperPath, + frames, + stderrText: () => stderrText, + exited, + exitInfo: () => ({ code: exitCode, signal: exitSignal }), + readEventFiles, + }; + } + + // A runner that runs each bridge shell script as a real child process + // (matches the harness in the stdin-race describe block above), so a test + // drives the whole legacy-poll bridge for real: the socket handler, the + // command-managed `writeTextFile`/`remove` scripts, the nohup wrapper + // launch, and the output poll. + function createLocalSandboxRunner(onExecute?: (script: string) => Promise) { + let counter = 0; + return { + execute: async (input: { + command: string; + args?: string[]; + cwd?: string; + env?: Record; + stdin?: string; + timeoutMs?: number; + onLog?: (stream: "stdout" | "stderr", chunk: string) => Promise; + }): Promise => { + counter += 1; + const script = input.args?.[1] ?? ""; + if (onExecute) await onExecute(script); + const command = + input.command === "bash" ? "/bin/bash" : input.command === "sh" ? "/bin/sh" : input.command; + return runChildProcess(`wrapper-lifecycle-run-${counter}`, command, input.args ?? [], { + cwd: input.cwd ?? process.cwd(), + env: input.env ?? {}, + stdin: input.stdin, + timeoutSec: Math.max(1, Math.ceil((input.timeoutMs ?? 30_000) / 1000)), + graceSec: 5, + onLog: input.onLog ?? (async () => {}), + }); + }, + }; + } + + function trackedChildSource(pidFile: string): string { + return [ + `import { writeFileSync } from "node:fs";`, + `writeFileSync(${JSON.stringify(pidFile)}, String(process.pid));`, + `process.stdin.resume();`, + ].join("\n"); + } + + // T2's child ignores SIGTERM, so `terminate()` must escalate to SIGKILL + // after its grace period. Ignoring end-of-file on stdin alone would not + // prove that: a plain `cat`-like child already dies from the default + // SIGTERM disposition. + function stubbornChildSource(pidFile: string): string { + return [ + `import { writeFileSync } from "node:fs";`, + `writeFileSync(${JSON.stringify(pidFile)}, String(process.pid));`, + `process.stdin.resume();`, + `process.on("SIGTERM", () => {});`, + `setInterval(() => {}, 1000);`, + ].join("\n"); + } + + async function startTrackedBridgeSession(input: { + rootDir: string; + runId: string; + childSource: string; + runner: ReturnType; + }) { + const pidFile = path.join(input.rootDir, `${input.runId}.pid`); + const childPath = path.join(input.rootDir, `${input.runId}-child.mjs`); + await writeFile(childPath, input.childSource, "utf8"); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: input.rootDir, + timeoutMs: 30_000, + runner: input.runner, + }; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: input.runId, + target, + runtimeRootDir: path.posix.join(input.rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: input.rootDir, + env: {}, + timeoutSec: 5, + onLog: async () => {}, + }); + expect(bridge).not.toBeNull(); + await waitFor(async () => (await readFile(pidFile, "utf8").catch(() => "")).trim().length > 0, 8_000); + const pid = Number.parseInt((await readFile(pidFile, "utf8")).trim(), 10); + return { bridge: bridge!, pid }; + } + + it("T1 exits within a bounded time when its session directory disappears with no message ever sent", async () => { + const wrapper = await startWrapperProcess({ outputToStdout: false, terminateGraceMs: 200 }); + // Let the poll loop run a few cycles before the directory disappears. + await delay(150); + await rm(wrapper.sessionDir, { recursive: true, force: true }); + await Promise.race([ + wrapper.exited, + delay(4_000).then(() => { + throw new Error("The wrapper did not exit after its session directory disappeared."); + }), + ]); + }); + + it("T2 leaves neither the wrapper nor a stubborn child alive after stop()", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-stubborn-child-")); + cleanupDirs.push(rootDir); + const runner = createLocalSandboxRunner(); + const session = await startTrackedBridgeSession({ + rootDir, + runId: "stubborn", + childSource: stubbornChildSource(path.join(rootDir, "stubborn.pid")), + runner, + }); + // The emitted wrapper script's own path is unique to this test (it lives + // under this test's fresh temp root), so a `ps` grep on it identifies + // only this test's wrapper process, not a sibling test's. + const wrapperScriptSubstring = path.posix.join(rootDir, ".paperclip-runtime", "acpx", "process-sessions"); + try { + expect(isPidAlive(session.pid)).toBe(true); + await waitFor(async () => (await findLivePidsByArgvSubstring(wrapperScriptSubstring)).length > 0, 4_000); + await session.bridge.stop(); + // The child ignores SIGTERM, so `terminate()` needs its own grace + // period (default 3s) before it escalates to SIGKILL. + await waitFor(() => !isPidAlive(session.pid), 8_000); + expect(isPidAlive(session.pid)).toBe(false); + await waitFor(async () => (await findLivePidsByArgvSubstring(wrapperScriptSubstring)).length === 0, 4_000); + } finally { + await session.bridge.stop().catch(() => undefined); + } + }, 15_000); + + it("T3 exits on its own when the child exits first and no stdinEnd is ever sent", async () => { + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: ["-e", "process.exit(0)"], + }); + await Promise.race([ + wrapper.exited, + delay(4_000).then(() => { + throw new Error("The wrapper did not exit on its own after its child exited."); + }), + ]); + const events = await wrapper.readEventFiles(); + expect(events.some((event) => event.type === "exit")).toBe(true); + }); + + it("T4 stopping one session leaves a sibling session's wrapper and child alive", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-session-isolation-")); + cleanupDirs.push(rootDir); + const runner = createLocalSandboxRunner(); + const sessionA = await startTrackedBridgeSession({ + rootDir, + runId: "session-a", + childSource: trackedChildSource(path.join(rootDir, "session-a.pid")), + runner, + }); + const sessionB = await startTrackedBridgeSession({ + rootDir, + runId: "session-b", + childSource: trackedChildSource(path.join(rootDir, "session-b.pid")), + runner, + }); + try { + expect(isPidAlive(sessionA.pid)).toBe(true); + expect(isPidAlive(sessionB.pid)).toBe(true); + + await sessionA.bridge.stop(); + await waitFor(() => !isPidAlive(sessionA.pid), 8_000); + + expect(isPidAlive(sessionA.pid)).toBe(false); + // Session B never received a stdinEnd or a shutdown message. + expect(isPidAlive(sessionB.pid)).toBe(true); + } finally { + await sessionA.bridge.stop().catch(() => undefined); + await sessionB.bridge.stop().catch(() => undefined); + } + }, 15_000); + + it("T5 still writes both control messages, finishes fast, and warns never after a forged exit event the live poll reads early", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-forged-exit-")); + cleanupDirs.push(rootDir); + const childPath = path.join(rootDir, "quiet-child.mjs"); + await writeFile(childPath, "process.stdin.resume();\n", "utf8"); + + const scripts: string[] = []; + const runner = createLocalSandboxRunner(async (script) => { + scripts.push(script); + }); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + let warnedCount = 0; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-forged-exit", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async (stream, chunk) => { + if (stream === "stderr" && chunk.includes("did not acknowledge shutdown")) warnedCount += 1; + }, + }); + expect(bridge).not.toBeNull(); + + const mkdirScript = scripts.find((script) => script.startsWith("mkdir -p")); + const dirsMatch = /mkdir -p '([^']+)' '([^']+)'/.exec(mkdirScript ?? ""); + expect(dirsMatch).not.toBeNull(); + const eventsDir = dirsMatch![2]; + + // Forge an exit event from outside the wrapper, before any real shutdown. + await mkdir(eventsDir, { recursive: true }); + await writeFile(path.join(eventsDir, "999999999999.json"), `${JSON.stringify({ type: "exit", code: 0 })}\n`, "utf8"); + + // Give the live 100 ms host poll time to read the forged file well + // before stop() runs. This closes the gap T5 used to leave open: a + // forged event `stop()` observes only through its own bounded reader + // (not through the live poll, which sets `stopping` on its own and + // stops re-arming) must not shorten the wait either. + await delay(600); + + scripts.length = 0; + const start = Date.now(); + await bridge!.stop(); + const elapsedMs = Date.now() - start; + + const finalizeWrites = scripts.filter((script) => script.includes("base64 -d") && script.includes(".paperclip-upload.decoded")); + // stdinEnd, then shutdown: both control messages still land. + expect(finalizeWrites.length).toBeGreaterThanOrEqual(2); + const removeScript = scripts.find((script) => script.trim().startsWith("rm -rf")); + expect(removeScript).toBeDefined(); + // The child never exits on its own, so the wrapper's own genuine + // shutdownAck -- not the forged exit event -- is the only thing that can + // finish this fast with no warning. T14 (below) proves the forged event + // alone gives no such shortcut when no genuine acknowledgement ever + // follows it. + expect(elapsedMs).toBeLessThan(1_000); + expect(warnedCount).toBe(0); + }, 10_000); + + it("T14 a forged exit event alone does not shorten the wait when the wrapper never truly runs", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-forged-only-")); + cleanupDirs.push(rootDir); + const childPath = path.join(rootDir, "quiet-child.mjs"); + await writeFile(childPath, "process.stdin.resume();\n", "utf8"); + + const scripts: string[] = []; + let counter = 0; + // Run every script for real except the wrapper launch itself, so the + // wrapper never starts. The forged file below is then the only event + // that will ever exist under the session's events directory. + const runner = { + execute: async (input: { + command: string; + args?: string[]; + cwd?: string; + env?: Record; + stdin?: string; + timeoutMs?: number; + onLog?: (stream: "stdout" | "stderr", chunk: string) => Promise; + }): Promise => { + const script = input.args?.[1] ?? ""; + scripts.push(script); + if (script.includes("nohup node")) { + return { exitCode: 0, signal: null, timedOut: false, stdout: "", stderr: "", pid: null, startedAt: null }; + } + counter += 1; + const command = + input.command === "bash" ? "/bin/bash" : input.command === "sh" ? "/bin/sh" : input.command; + return runChildProcess(`forged-only-run-${counter}`, command, input.args ?? [], { + cwd: input.cwd ?? process.cwd(), + env: input.env ?? {}, + stdin: input.stdin, + timeoutSec: Math.max(1, Math.ceil((input.timeoutMs ?? 30_000) / 1000)), + graceSec: 5, + onLog: input.onLog ?? (async () => {}), + }); + }, + }; + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + + let warnedCount = 0; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-forged-only", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async (stream, chunk) => { + if (stream === "stderr" && chunk.includes("did not acknowledge shutdown")) warnedCount += 1; + }, + }); + expect(bridge).not.toBeNull(); + + const mkdirScript = scripts.find((script) => script.startsWith("mkdir -p")); + const dirsMatch = /mkdir -p '([^']+)' '([^']+)'/.exec(mkdirScript ?? ""); + expect(dirsMatch).not.toBeNull(); + const eventsDir = dirsMatch![2]; + + // Forge a terminal event from outside the wrapper. The wrapper never + // started, so this is the only event that will ever exist on disk. + await mkdir(eventsDir, { recursive: true }); + await writeFile(path.join(eventsDir, "999999999999.json"), `${JSON.stringify({ type: "exit", code: 0 })}\n`, "utf8"); + + // Give the live 100 ms host poll time to read the forged file well + // before stop() runs, so this test cannot pass by accident: `stop()` + // never itself observes this event through its own first-line + // `stopping` flag. + await delay(600); + + const start = Date.now(); + await bridge!.stop(); + const elapsedMs = Date.now() - start; + + // An exit event under `sessionDir` is untrusted telemetry. With no + // genuine wrapper ever running to write a real shutdownAck, the wait + // still runs its full budget and still warns, exactly as it would with + // no forged event at all (compare T13). + expect(elapsedMs).toBeGreaterThanOrEqual(2_900); + expect(warnedCount).toBe(1); + const removeScript = scripts.find((script) => script.trim().startsWith("rm -rf")); + expect(removeScript).toBeDefined(); + }, 10_000); + + it("T6 issues no operating-system signal from the host during stop()", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-no-signal-")); + cleanupDirs.push(rootDir); + const childPath = path.join(rootDir, "quiet-child.mjs"); + await writeFile(childPath, "process.stdin.resume();\n", "utf8"); + + const scripts: string[] = []; + const runner = createLocalSandboxRunner(async (script) => { + scripts.push(script); + }); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-no-signal", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async () => {}, + }); + expect(bridge).not.toBeNull(); + + scripts.length = 0; + await bridge!.stop(); + + // stop() only ever writes files and removes a directory. None of the + // scripts it runs names a signal or a kill command. + const signalLike = scripts.filter((script) => /\bkill\b|SIGTERM|SIGKILL/i.test(script)); + expect(signalLike).toEqual([]); + }); + + it("T8 running terminate() a second time, after the child already exited, is a safe no-op", async () => { + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: ["-e", "process.exit(0)"], + }); + await Promise.race([ + wrapper.exited, + delay(4_000).then(() => { + throw new Error("The wrapper did not exit after its child exited on its own."); + }), + ]); + // The wrapper's own child-close handler already ran terminate() once (the + // child was already gone, so its child.kill() call no-opped). The + // wrapper did not throw and did not hang. + expect(wrapper.stderrText()).toBe(""); + expect(wrapper.exitInfo().code).toBe(0); + const events = await wrapper.readEventFiles(); + expect(events.filter((event) => event.type === "exit").length).toBe(1); + // No stray signal-triggered event (e.g. a second exit from a SIGKILL) + // ever landed. + expect(events.filter((event) => event.type === "error").length).toBe(0); + }); + + it("T9 exits with an error event when sessionDir is a symbolic link", async () => { + const targetDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-symlink-target-")); + cleanupDirs.push(targetDir); + const linkDir = `${targetDir}-link`; + const { symlink } = await import("node:fs/promises"); + await symlink(targetDir, linkDir, "dir"); + cleanupDirs.push(linkDir); + + const wrapperPath = path.join(targetDir, "wrapper.mjs"); + await writeFile(wrapperPath, getProcessSessionRemoteSource({ outputToStdout: true }), "utf8"); + const config = { command: "cat", args: [] as string[], cwd: targetDir, env: {} }; + const commandPayload = Buffer.from(JSON.stringify(config), "utf8").toString("base64"); + + const child = spawn(process.execPath, [wrapperPath], { + cwd: targetDir, + env: { + ...process.env, + PAPERCLIP_PROCESS_SESSION_DIR: linkDir, + PAPERCLIP_PROCESS_SESSION_COMMAND_B64: commandPayload, + }, + stdio: ["ignore", "pipe", "pipe"], + }); + const frames: WrapperFrame[] = []; + let stdoutBuffer = ""; + child.stdout.on("data", (chunk: Buffer) => { + stdoutBuffer += chunk.toString("utf8"); + const lines = stdoutBuffer.split("\n"); + stdoutBuffer = lines.pop() || ""; + for (const line of lines) { + if (!line.trim()) continue; + frames.push(JSON.parse(line) as WrapperFrame); + } + }); + const exited = new Promise((resolve) => child.on("close", () => resolve())); + + await Promise.race([ + exited, + delay(4_000).then(() => { + throw new Error("The wrapper did not exit after sessionDir was a symbolic link."); + }), + ]); + expect(frames.some((frame) => frame.type === "error" && typeof frame.message === "string" && frame.message.includes("symbolic link"))).toBe( + true, + ); + }); + + it("T10 the emitted wrapper strips its own session env vars from the child", async () => { + const wrapper = await startWrapperProcess({ + outputToStdout: true, + command: process.execPath, + args: [ + "-e", + "process.stdout.write(JSON.stringify(Object.keys(process.env).filter((k) => k.startsWith('PAPERCLIP_PROCESS_SESSION'))));process.exit(0)", + ], + }); + await waitFor(() => wrapper.frames.some((frame) => frame.type === "exit"), 4_000); + const text = wrapper.frames + .filter((frame) => frame.type === "data" && frame.stream === "stdout" && typeof frame.data === "string") + .map((frame) => Buffer.from(frame.data as string, "base64").toString("utf8")) + .join(""); + const leakedKeys = JSON.parse(text || "[]") as string[]; + expect(leakedKeys).toEqual([]); + }); + + it("T11 each wrapper source has exactly one spawn call site and every kill call is child.kill()", () => { + for (const outputToStdout of [true, false]) { + const src = getProcessSessionRemoteSource({ outputToStdout }); + // Strip `//` line comments first, so prose that happens to mention + // "spawn(" or "kill(" (e.g. explaining `ChildProcess#kill()`) is never + // mistaken for a call site. This checks the code, not the comments. + const code = src + .split("\n") + .map((line) => line.replace(/\/\/.*$/, "")) + .join("\n"); + const spawnCallSites = code.match(/\bspawn\(/g) ?? []; + expect(spawnCallSites.length).toBe(1); + const killCallSites = [...code.matchAll(/[A-Za-z0-9_.$]*kill\(/g)].map((match) => match[0]); + expect(killCallSites.length).toBeGreaterThan(0); + for (const site of killCallSites) { + expect(site).toBe("child.kill("); + } + + // Regression coverage for PAP-5336: the shared tail must carry the + // session-identity latch, not the old counter it replaced. A counter + // that a successful `readdir` reset to zero let an attacker who + // recreated a deleted control directory keep the wrapper alive + // forever. Both wrapper variants append the same shared tail, so this + // check runs once per variant and fails if a future edit lands the + // latch in only one of them. + expect(src).not.toContain("missingSessionDirStreak"); + const identityLatchDeclarations = code.match(/\blet identityLost = false;/g) ?? []; + expect(identityLatchDeclarations.length).toBe(1); + const identityCaptureCallSites = code.match(/\bcaptureSessionIdentity\(\)/g) ?? []; + expect(identityCaptureCallSites.length).toBeGreaterThan(0); + const identityVerifyCallSites = code.match(/\bverifySessionIdentity\(\)/g) ?? []; + expect(identityVerifyCallSites.length).toBeGreaterThan(0); + // The capture must run before the first poll cycle: its call site must + // precede the `pollStdin()` call site in the emitted source. + expect(code.indexOf("await captureSessionIdentity();")).toBeGreaterThan(0); + expect(code.indexOf("await captureSessionIdentity();")).toBeLessThan(code.indexOf("void pollStdin()")); + } + }); + + // Regression coverage for PAP-5323: the host used to burn the full + // shutdown budget and log a false warning on every normal run, because + // the file-poll loop stopped re-arming right after it delivered the + // `exit` event and so never read the `shutdownAck` file that followed it. + it("T12 stop() finishes well inside the shutdown budget and logs no warning after a normal child exit", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-normal-exit-")); + cleanupDirs.push(rootDir); + const childPath = path.join(rootDir, "quick-exit-child.mjs"); + await writeFile(childPath, "process.exit(0);\n", "utf8"); + + const runner = createLocalSandboxRunner(); + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + + let warnedCount = 0; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-normal-exit", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async (stream, chunk) => { + if (stream === "stderr" && chunk.includes("did not acknowledge shutdown")) warnedCount += 1; + }, + }); + expect(bridge).not.toBeNull(); + + // Let the child exit and the wrapper write its own `exit` event before + // stop() runs, so this matches a normal run-completion teardown. + await delay(500); + + const start = Date.now(); + await bridge!.stop(); + const elapsedMs = Date.now() - start; + + expect(elapsedMs).toBeLessThan(1_000); + expect(warnedCount).toBe(0); + }, 10_000); + + // Regression coverage for PAP-5323: a genuinely stuck wrapper, one that + // never writes any event, must still warn after the budget and still + // remove `sessionDir`. The fix must not turn the bounded wait into an + // unconditional skip. + it("T13 warns and still removes sessionDir when the wrapper never acknowledges and never exits", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-never-acks-")); + cleanupDirs.push(rootDir); + const childPath = path.join(rootDir, "quiet-child.mjs"); + await writeFile(childPath, "process.stdin.resume();\n", "utf8"); + + const scripts: string[] = []; + let counter = 0; + // Run every script for real except the wrapper launch itself, so the + // wrapper never starts and the events directory stays empty on every + // poll. `stop()` can then never observe a real `shutdownAck` or a real + // terminal `exit`/`error` event. + const runner = { + execute: async (input: { + command: string; + args?: string[]; + cwd?: string; + env?: Record; + stdin?: string; + timeoutMs?: number; + onLog?: (stream: "stdout" | "stderr", chunk: string) => Promise; + }): Promise => { + const script = input.args?.[1] ?? ""; + scripts.push(script); + if (script.includes("nohup node")) { + return { exitCode: 0, signal: null, timedOut: false, stdout: "", stderr: "", pid: null, startedAt: null }; + } + counter += 1; + const command = + input.command === "bash" ? "/bin/bash" : input.command === "sh" ? "/bin/sh" : input.command; + return runChildProcess(`never-acks-run-${counter}`, command, input.args ?? [], { + cwd: input.cwd ?? process.cwd(), + env: input.env ?? {}, + stdin: input.stdin, + timeoutSec: Math.max(1, Math.ceil((input.timeoutMs ?? 30_000) / 1000)), + graceSec: 5, + onLog: input.onLog ?? (async () => {}), + }); + }, + }; + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + + let warnedCount = 0; + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-never-acks", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async (stream, chunk) => { + if (stream === "stderr" && chunk.includes("did not acknowledge shutdown")) warnedCount += 1; + }, + }); + expect(bridge).not.toBeNull(); + + const start = Date.now(); + await bridge!.stop(); + const elapsedMs = Date.now() - start; + + // The full shutdown budget elapsed, because nothing ever proved the + // wrapper stopped. + expect(elapsedMs).toBeGreaterThanOrEqual(2_900); + expect(warnedCount).toBe(1); + const removeScript = scripts.find((script) => script.trim().startsWith("rm -rf")); + expect(removeScript).toBeDefined(); + }, 10_000); + + // Regression coverage for PAP-5336: the finding this test reproduces is a + // sandbox control-plane integrity failure, not a timing quirk. During + // `stop()`, a sandbox peer with access to the session directory can (1) + // delete the real `shutdown` control file before the wrapper ever reads + // it, (2) forge a `shutdownAck` event so the host's wait ends early, and + // (3) recreate `sessionDir/stdin` right after the host removes + // `sessionDir`. On the parent commit, a successful `readdir` on the + // recreated directory reset the wrapper's only terminal counter to zero, + // so the wrapper (and its child) polled forever. + // + // The fix replaces the counter with an identity captured at startup: the + // device number, the inode number, and the inode's own creation time. All + // three matter for this test to be a real regression check, not a check + // that passes by luck. Recreating a directory at the same path right after + // removal, with nothing else on the filesystem in between, can reissue the + // exact same device and inode numbers on common filesystems (this test's + // recreate step does exactly that): a device/inode-only identity would + // then wrongly read as unchanged. The creation time does not have this + // gap, because it is set fresh on every inode allocation even when the + // allocator reissues an old inode number. + it("T15 latches on a lost session identity: a recreated control directory cannot keep the wrapper or its child alive", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-lost-identity-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t15-child.pid"); + const childPath = path.join(rootDir, "t15-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + let sessionDir = ""; + let stdinDir = ""; + let eventsDir = ""; + let shutdownFileDeleted = false; + let shutdownAckForged = false; + let stdinDirRecreated = false; + const scripts: string[] = []; + let counter = 0; + + const syntheticSuccess: RunProcessResult = { + exitCode: 0, + signal: null, + timedOut: false, + stdout: "", + stderr: "", + pid: null, + startedAt: null, + }; + + // Run every script for real on the local filesystem (matching T2/T4/T5's + // harness), so a real wrapper process and a real child process come up, + // with two exceptions that make the attack deterministic instead of a + // race against the wrapper's own 50 ms poll: + // + // 1. With no stdin data ever sent, the host's shutdown control message + // always targets stdin file 000000000002.json (file 1 is stdinEnd). + // Never let that write's script pipeline actually run: this is + // equivalent to an attacker who deletes the file before the wrapper + // ever reads it, but with no window in which the wrapper could win a + // race and read it first. + // 2. Perform the sessionDir removal and the sessionDir/stdin, + // sessionDir/events recreation as direct filesystem calls in this + // same async step, instead of spawning a real `rm -rf` subprocess. + // That removes an external process's scheduling latency from the + // window the wrapper's next poll cycle has to observe the recreated + // directory. + const runner = { + execute: async (input: { + command: string; + args?: string[]; + cwd?: string; + env?: Record; + stdin?: string; + timeoutMs?: number; + onLog?: (stream: "stdout" | "stderr", chunk: string) => Promise; + }): Promise => { + counter += 1; + const script = input.args?.[1] ?? ""; + scripts.push(script); + + const shutdownFilePath = stdinDir ? path.posix.join(stdinDir, "000000000002.json") : null; + if (shutdownFilePath && script.includes(shutdownFilePath)) { + if (!shutdownFileDeleted) { + shutdownFileDeleted = true; + await writeFile( + path.join(eventsDir, "999999999999.json"), + `${JSON.stringify({ type: "shutdownAck" })}\n`, + "utf8", + ).catch(() => undefined); + shutdownAckForged = true; + } + return syntheticSuccess; + } + + // Match the removal of sessionDir itself, not the host's own + // per-file event cleanup (which also runs `rm -rf` on a path that + // has sessionDir as a substring). + if (!stdinDirRecreated && sessionDir && script.trim() === `rm -rf '${sessionDir}'`) { + stdinDirRecreated = true; + await rm(sessionDir, { recursive: true, force: true }).catch(() => undefined); + await mkdir(stdinDir, { recursive: true }).catch(() => undefined); + await mkdir(eventsDir, { recursive: true }).catch(() => undefined); + return syntheticSuccess; + } + + const command = + input.command === "bash" ? "/bin/bash" : input.command === "sh" ? "/bin/sh" : input.command; + return runChildProcess(`lost-identity-run-${counter}`, command, input.args ?? [], { + cwd: input.cwd ?? process.cwd(), + env: input.env ?? {}, + stdin: input.stdin, + timeoutSec: Math.max(1, Math.ceil((input.timeoutMs ?? 30_000) / 1000)), + graceSec: 5, + onLog: input.onLog ?? (async () => {}), + }); + }, + }; + + const target: AdapterSandboxExecutionTarget = { + kind: "remote", + transport: "sandbox", + providerKey: "local-test", + remoteCwd: rootDir, + timeoutMs: 30_000, + runner, + }; + + const bridge = await startAdapterExecutionTargetProcessSessionBridge({ + runId: "run-lost-identity", + target, + runtimeRootDir: path.posix.join(rootDir, ".paperclip-runtime", "acpx"), + adapterKey: "acpx", + command: process.execPath, + args: [childPath], + cwd: rootDir, + env: {}, + timeoutSec: 5, + onLog: async () => {}, + }); + expect(bridge).not.toBeNull(); + + const mkdirScript = scripts.find((script) => script.startsWith("mkdir -p")); + const dirsMatch = /mkdir -p '([^']+)' '([^']+)'/.exec(mkdirScript ?? ""); + expect(dirsMatch).not.toBeNull(); + stdinDir = dirsMatch![1]; + eventsDir = dirsMatch![2]; + sessionDir = path.posix.dirname(stdinDir); + + await waitFor(async () => (await readFile(pidFile, "utf8").catch(() => "")).trim().length > 0, 8_000); + const pid = Number.parseInt((await readFile(pidFile, "utf8")).trim(), 10); + expect(isPidAlive(pid)).toBe(true); + const wrapperScriptSubstring = path.posix.join(rootDir, ".paperclip-runtime", "acpx", "process-sessions"); + await waitFor(async () => (await findLivePidsByArgvSubstring(wrapperScriptSubstring)).length > 0, 4_000); + + await bridge!.stop(); + + // Both the attacker's forged shutdownAck and its directory recreation + // ran; this is the full chain the finding describes, not a partial one. + expect(shutdownFileDeleted).toBe(true); + expect(shutdownAckForged).toBe(true); + expect(stdinDirRecreated).toBe(true); + + // The wrapper process and its child process both exit within a bounded + // time, even though the wrapper never read a real shutdown message and + // the host's wait ended early on a forged hint. Only the wrapper's own + // identity latch can explain this: it observes the recreated directory + // carries a different identity than the one captured at startup. + await waitFor(() => !isPidAlive(pid), 8_000); + expect(isPidAlive(pid)).toBe(false); + await waitFor(async () => (await findLivePidsByArgvSubstring(wrapperScriptSubstring)).length === 0, 8_000); + }, 15_000); + + // ---- PAP-5338: fail closed on an unusable creation time, and on every + // lstat error during verification ------------------------------------- + + async function waitForTrackedChildPid(pidFile: string): Promise { + await waitFor(async () => (await readFile(pidFile, "utf8").catch(() => "")).trim().length > 0, 8_000); + return Number.parseInt((await readFile(pidFile, "utf8")).trim(), 10); + } + + async function expectWrapperAndTrackedChildToDie( + wrapper: { exited: Promise }, + pid: number, + ): Promise { + await waitFor(() => !isPidAlive(pid), 8_000); + expect(isPidAlive(pid)).toBe(false); + await Promise.race([ + wrapper.exited, + delay(8_000).then(() => { + throw new Error("The wrapper process did not exit."); + }), + ]); + } + + // A capture failure latches and calls terminate() before the poll loop + // ever starts, often within a few milliseconds of the child's own spawn() + // call returning. A freshly spawned Node.js child needs real wall-clock + // time just to boot before it can run its own code, so it can lose the + // race to write a pid file before terminate()'s SIGTERM reaches it. This + // is the correct, intended shape of a fail-fast capture: the child never + // gets a chance to become a live orphan. So these two tests prove death + // through the OS process table by the child's own script path (the same + // technique T15 above uses for the wrapper itself), which needs no + // cooperation from code inside the child. + async function expectNoLiveProcessByArgvSubstring(substring: string): Promise { + await waitFor(async () => (await findLivePidsByArgvSubstring(substring)).length === 0, 8_000); + expect(await findLivePidsByArgvSubstring(substring)).toEqual([]); + } + + it("T16 fails closed at capture when the reported creation time is zero, so no orphan wrapper or child ever starts polling", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-birthtime-zero-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t16-child.pid"); + const childPath = path.join(rootDir, "t16-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + fakeBirthtime: { target: "sessionDir", mode: "zero" }, + }); + + await Promise.race([ + wrapper.exited, + delay(8_000).then(() => { + throw new Error("The wrapper process did not exit."); + }), + ]); + expect(wrapper.stderrText()).toMatch(/not usable/); + await expectNoLiveProcessByArgvSubstring(childPath); + }, 15_000); + + it("T17 fails closed at capture when the reported creation time follows the change time, so a change-time copy never passes as a real creation time", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-birthtime-followctime-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t17-child.pid"); + const childPath = path.join(rootDir, "t17-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + fakeBirthtime: { target: "stdinDir", mode: "followCtime" }, + }); + + await Promise.race([ + wrapper.exited, + delay(8_000).then(() => { + throw new Error("The wrapper process did not exit."); + }), + ]); + await expectNoLiveProcessByArgvSubstring(childPath); + expect(wrapper.stderrText()).toMatch(/changed after a probe write/); + }, 15_000); + + it("T18 latches on an EACCES lstat failure on sessionDir during verification, not only on a removed directory", async () => { + // sessionDir lives inside a parent this test owns, never the shared OS + // temp directory: the test denies traversal on that parent, and doing + // that to the shared OS temp directory would break every other process + // on the host that also uses it. + const parentDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-eacces-sessiondir-")); + cleanupDirs.push(parentDir); + const pidFile = path.join(parentDir, "t18-child.pid"); + const childPath = path.join(parentDir, "t18-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + parentDir, + terminateGraceMs: 200, + }); + + const pid = await waitForTrackedChildPid(pidFile); + // Let capture succeed and the poll loop run a clean cycle first, so the + // termination below proves the verify-time latch, not the capture-time + // one. + await delay(150); + await chmod(parentDir, 0o000); + try { + await expectWrapperAndTrackedChildToDie(wrapper, pid); + } finally { + // Restore permission so the shared cleanup can remove this directory. + await chmod(parentDir, 0o700).catch(() => undefined); + } + expect(wrapper.stderrText()).toMatch(/Latching on a lost process session identity/); + }, 15_000); + + it("T19 latches on an EACCES lstat failure on stdinDir during verification, even though sessionDir itself still stats cleanly", async () => { + const wrapperOptions = { outputToStdout: false as const, terminateGraceMs: 200 }; + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-eacces-stdindir-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t19-child.pid"); + const childPath = path.join(rootDir, "t19-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ ...wrapperOptions, command: process.execPath, args: [childPath] }); + + const pid = await waitForTrackedChildPid(pidFile); + await delay(150); + // Deny traversal into sessionDir itself: lstat(stdinDir) fails EACCES + // while lstat(sessionDir) still succeeds, since a directory's own mode + // never gates lstat of the directory itself, only lookups inside it. + await chmod(wrapper.sessionDir, 0o000); + try { + await expectWrapperAndTrackedChildToDie(wrapper, pid); + } finally { + await chmod(wrapper.sessionDir, 0o700).catch(() => undefined); + } + expect(wrapper.stderrText()).toMatch(/Latching on a lost process session identity/); + }, 15_000); +}); diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 3f028b3cb6..801af1ab55 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -1596,6 +1596,12 @@ const PROCESS_SESSION_REMOTE_SCRIPT = "paperclip-process-session-remote.mjs"; // hash-skip gate thrashing when a run switches output mode. const PROCESS_SESSION_REMOTE_STREAM_SCRIPT = "paperclip-process-session-remote-stream.mjs"; const PROCESS_SESSION_AUTH_TIMEOUT_MS = 5_000; +// The bounded budget `stop()` waits for the wrapper's `shutdownAck` event +// before it removes `sessionDir` unconditionally. The wrapper writes the +// acknowledgement right after it arms its own kill timer, well before its +// child actually exits, so this budget only needs to cover message delivery, +// not the child's full shutdown. +const DEFAULT_PROCESS_SESSION_SHUTDOWN_WAIT_MS = 3_000; function jsonLine(value: unknown): string { return `${JSON.stringify(value)}\n`; @@ -1792,10 +1798,11 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { args: shellCommandArgs( [ `mkdir -p ${shellQuote(stdinDir)} ${shellQuote(eventsDir)}`, + // I3: no numeric process identifier anywhere. Background the + // wrapper and let it go; do not capture `$!`. `PAPERCLIP_PROCESS_SESSION_DIR=${shellQuote(sessionDir)} ` + `PAPERCLIP_PROCESS_SESSION_COMMAND_B64=${shellQuote(commandPayload)} ` + `nohup node ${shellQuote(remoteScriptPath)} >/dev/null 2>&1 < /dev/null &`, - "printf '%s\\n' \"$!\"", ].join("\n"), ), cwd: target.remoteCwd, @@ -1841,6 +1848,19 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { }> = []; const token = createSandboxCallbackBridgeToken(18); const proxyDir = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-process-session-proxy-")); + // `stop()` waits on this promise, bounded, for the wrapper's `shutdownAck` + // event. `deliverRemoteEvent` resolves it below and never forwards the + // event further: it is a host-internal control ack, not part of the ACP + // output stream. An event under `sessionDir` is untrusted telemetry: an + // `exit` or `error` event is never treated as proof of shutdown, because + // any process running under the sandbox can write one. Only `shutdownAck` + // counts, and `stop()` also gives itself a dedicated reader for it below, + // so a late `shutdownAck` still lands even after the long-lived poll has + // stopped re-arming. + let signalShutdownAcknowledged: () => void = () => {}; + const shutdownAcknowledged = new Promise((resolve) => { + signalShutdownAcknowledged = resolve; + }); const writeRemoteEventToSocket = (event: (typeof pendingRemoteEvents)[number]) => { if (!socket) return false; @@ -1856,6 +1876,10 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { }; const deliverRemoteEvent = (event: (typeof pendingRemoteEvents)[number]) => { + if (event.type === "shutdownAck") { + signalShutdownAcknowledged(); + return; + } if (socket) { writeRemoteEventToSocket(event); return; @@ -1966,6 +1990,10 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { const poll = async () => { if (stopping) return; try { + // Read every file this tick fetched before this loop decides whether to + // keep polling. A `shutdownAck` can land in the same batch right after + // an `exit` event; deliver it too, so this tick never drops an + // already-fetched (and already-removed-from-disk) event. const events = await readRemoteJsonFiles({ client, dir: eventsDir }); for (const event of events) { const parsed = JSON.parse(event.body) as { @@ -1977,7 +2005,6 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { message?: string; }; deliverRemoteEvent(parsed); - if (parsed.type === "exit" || parsed.type === "error") return; } } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -2130,6 +2157,43 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { schedulePoll(); } + // `stop()` cannot rely on the long-lived poll above to observe a late + // `shutdownAck`: that poll stops re-arming as soon as it forwards a + // terminal `exit`/`error` event, and `stop()` itself sets `stopping` on + // its own first line. A normal completion's `shutdownAck` file, written a + // moment after `exit`, can then land on disk after nobody reads the events + // directory any more. Give `stop()` its own bounded reader that looks only + // for `shutdownAck` and ignores every other event type, so the wait below + // shortens on the wrapper's own proof of shutdown -- never on an `exit` or + // `error` event, which any process running under `sessionDir` can forge. + let stopReadingForShutdownAck = false; + const readShutdownAckUntil = (deadlineEpochMs: number) => { + if (stopReadingForShutdownAck) return; + void (async () => { + try { + const events = await readRemoteJsonFiles({ client, dir: eventsDir }); + if (stopReadingForShutdownAck) return; + for (const event of events) { + try { + const parsed = JSON.parse(event.body) as { type?: string }; + if (parsed.type === "shutdownAck") { + signalShutdownAcknowledged(); + return; + } + } catch { + // Not readable JSON yet. It is not a `shutdownAck`; ignore it. + } + } + } catch { + // Best-effort: a read failure here is not proof of anything. + } + if (!stopReadingForShutdownAck && Date.now() < deadlineEpochMs) { + const timer = setTimeout(() => readShutdownAckUntil(deadlineEpochMs), 100); + timer.unref?.(); + } + })(); + }; + return { agentCommand, stop: async () => { @@ -2155,6 +2219,53 @@ export async function startAdapterExecutionTargetProcessSessionBridge(input: { ); stdinWriteChain = stdinEndWrite.then(() => undefined, () => undefined); await stdinEndWrite.catch(() => undefined); + // The `shutdown` control message tells the wrapper to terminate itself + // and its own child (I3: no operating-system signal and no process + // identifier cross this boundary — only a file-queue message does). + // Chain it onto the same per-session write order as `stdinEnd`, so its + // file never lands before the earlier one. + const shutdownPath = path.posix.join( + stdinDir, + `${String(stdinSeq + 2).padStart(12, "0")}.json`, + ); + const shutdownWrite = stdinWriteChain.then(() => + client.writeTextFile(shutdownPath, jsonLine({ type: "shutdown" })), + ); + stdinWriteChain = shutdownWrite.then(() => undefined, () => undefined); + await shutdownWrite.catch(() => undefined); + // Wait a bounded budget for a hint that the wrapper stopped: only the + // `shutdownAck` event counts; an `exit` or `error` event is untrusted + // telemetry from inside the sandbox and never shortens this wait or + // suppresses the warning below. `shutdownAck` itself is ALSO an + // untrusted hint, not proof: any process that shares the sandbox can + // write the same event under this session's event directory. It can + // only shorten this wait and suppress the warning below; it never + // gates, shortens, or replaces the unconditional removal further down. + // What actually makes the wrapper's own termination deterministic is + // the wrapper-side session-identity latch, not this event. + let acknowledgedInTime = false; + readShutdownAckUntil(Date.now() + DEFAULT_PROCESS_SESSION_SHUTDOWN_WAIT_MS); + await Promise.race([ + shutdownAcknowledged.then(() => { + acknowledgedInTime = true; + }), + new Promise((resolve) => { + const budgetTimer = setTimeout(resolve, DEFAULT_PROCESS_SESSION_SHUTDOWN_WAIT_MS); + budgetTimer.unref?.(); + }), + ]); + stopReadingForShutdownAck = true; + if (!acknowledgedInTime) { + await onLog( + "stderr", + `[paperclip] ACP process session wrapper did not acknowledge shutdown within ${DEFAULT_PROCESS_SESSION_SHUTDOWN_WAIT_MS}ms; removing the session directory anyway.\n`, + ).catch(() => undefined); + } + // Unconditional: this removal runs whether or not the wrapper + // acknowledged, and whether or not any event (real or forged) arrived + // under `sessionDir`. `stop()` runs during run teardown and must stay + // non-fatal, so every step above is best-effort and this step never + // throws. await client.remove(sessionDir).catch(() => undefined); await fs.rm(proxyDir, { recursive: true, force: true }).catch(() => undefined); }, @@ -2244,11 +2355,274 @@ const stdinParseRetries = new Map(); let stdinExpectedSeq = 1; let stdinGapRetries = 0; +// The bounded grace period between the SIGTERM and the SIGKILL a terminate() +// call sends. A test can override it through the environment, so a stubborn +// child does not force a slow test. +const terminateGraceMs = (() => { + const raw = Number.parseInt(process.env.PAPERCLIP_PROCESS_SESSION_TERMINATE_GRACE_MS || "", 10); + return Number.isFinite(raw) && raw > 0 ? raw : 3000; +})(); + +// I2: terminate() is the only function in this wrapper that calls +// child.kill(). No child event handler and no sibling callback calls it. +// terminate() is idempotent: a second call, or a first call after the child +// already exited on its own, does nothing beyond what already ran. +async function terminate() { + if (terminated) return; + terminated = true; + shuttingDown = true; + stdinClosed = true; + child.stdin.end(); + // A \`false\` return means the child's process handle is already gone (the + // child exited before this call ran). ChildProcess#kill() is handle-scoped: + // once Node clears the handle at reap, the method call above sends no + // signal and never falls back to a stored process identifier (I3). Treat + // \`false\` as a no-op and do not retry through a numeric identifier. + const sentTerm = child.kill("SIGTERM"); + if (sentTerm) { + killTimer = setTimeout(() => { + // Escalate on the same handle only (I2): the grace period expired, so + // send SIGKILL through the same child handle, never a numeric + // identifier and never a process-group signal. + child.kill("SIGKILL"); + }, terminateGraceMs); + killTimer.unref?.(); + } + // This event is an untrusted latency hint, not proof. Any process that can + // reach this session's event directory can write the same event type. It + // can only shorten the host's shutdown wait and suppress the host's + // timeout warning; it is never evidence that this wrapper's lifecycle + // completed, and the host's cleanup never depends on it. The identity + // latch below is what makes this wrapper's own termination deterministic. + await writeEvent({ type: "shutdownAck" }); +} + +// A sandbox peer can delete sessionDir and stdinDir, then recreate a +// directory at the same pathname. A pathname does not prove identity: any +// process that shares the sandbox can write it. So this wrapper captures the +// OS-level identity of both paths once at startup, before the first poll +// cycle, and checks it on every later cycle. +// +// The identity is the device number, the inode number, AND the inode's own +// creation time. The device/inode pair alone is not enough: a filesystem can +// reissue the exact inode number a just-removed directory held to the very +// next directory created at the same path, with no attacker action needed +// beyond the recreate the finding already describes. The creation time does +// not have this gap: it is set fresh on every inode allocation, even when the +// allocator reissues an old inode number, so a recreated directory always +// carries a different creation time. The creation time alone is not enough +// either, on a filesystem or kernel too old to report it, so this wrapper +// keeps the device/inode pair as a second signal rather than relying on +// either alone. Ordinary use of stdinDir (the host writing and this wrapper +// deleting individual stdin files) changes that directory's OWN change time, +// but never its creation time, so the creation time is safe to latch on +// without producing a false positive on every stdin message. +// +// A filesystem or kernel that cannot report a real creation time does not +// always report a value of zero. Node fails in one of two ways, and both are +// grounded, not assumed: on Linux, when the statx() call finds no creation +// time support, the kernel leaves the field unset and Node reports 0. On a +// platform whose stat() call has no creation-time field at all, Node copies +// the change time into the creation time instead. A 0 value fails open (any +// recreated directory then matches on birthtimeMs alone), and a change-time +// copy fails closed but far too often (it would move on every stdin file +// this wrapper deletes). captureSessionIdentity() below proves the value is +// usable before it trusts it, and fails closed on both known fallbacks. +let sessionDirIdentity = null; +let stdinDirIdentity = null; +// The latch. Once set, it never clears. This replaces a counter that a +// successful read reset to zero: an attacker who recreated the directory +// before the counter reached its threshold kept the wrapper polling forever. +// A latch has no threshold to race and no reset path. +let identityLost = false; + +async function statPathIdentity(candidatePath) { + const stats = await fs.lstat(candidatePath); + if (stats.isSymbolicLink()) { + const error = new Error("Refusing a symbolic link on a process session control path."); + error.code = "EPAPERCLIP_SYMLINK"; + throw error; + } + if (!stats.isDirectory()) { + const error = new Error("A process session control path is not a directory."); + error.code = "ENOTDIR"; + throw error; + } + return { dev: stats.dev, ino: stats.ino, birthtimeMs: stats.birthtimeMs }; +} + +function sameIdentity(left, right) { + return left.dev === right.dev && left.ino === right.ino && left.birthtimeMs === right.birthtimeMs; +} + +async function latchAndTerminate() { + if (identityLost) return; + identityLost = true; + await terminate(); +} + +function isUsableBirthtimeMs(value) { + return typeof value === "number" && Number.isFinite(value) && value !== 0; +} + +let probeSeq = 0; + +// A probe file name that pollStdin() can never read as a stdin message: it +// does not end in ".json", so the ".json" filter in pollStdin() skips it if +// a poll cycle ever lists the directory during the probe's short window. +function nextProbeFileName() { + probeSeq += 1; + return ".paperclip-birthtime-probe-" + process.pid + "-" + probeSeq; +} + +// Proves a directory's reported birthtimeMs is a real creation time, not a +// change-time copy. Creating and removing a file inside a directory changes +// that directory's OWN change time but never its true creation time, so a +// birthtimeMs that moves across the probe is a change-time copy. Returns +// false both on a detected copy and on a probe that cannot run at all (for +// example a permission error): either way the caller must not trust the +// value. +async function birthtimeSurvivesProbe(dirPath) { + let before; + try { + before = (await fs.lstat(dirPath)).birthtimeMs; + } catch { + return false; + } + const probePath = path.posix.join(dirPath, nextProbeFileName()); + try { + await fs.writeFile(probePath, ""); + } catch { + return false; + } finally { + await fs.rm(probePath, { force: true }).catch(() => undefined); + } + let after; + try { + after = (await fs.lstat(dirPath)).birthtimeMs; + } catch { + return false; + } + return before === after; +} + +async function refuseUnusableCreationTime(label, dirPath, reason) { + process.stderr.write( + "Refusing to trust the process session control path " + label + " (" + dirPath + "): " + reason + + ". This filesystem or kernel gives no usable creation time. Terminating.\\n", + ); + await latchAndTerminate(); +} + +// Runs once, before the first poll cycle, and before this wrapper captures +// the identities it later checks on every cycle. A failed capture fails +// closed: the wrapper has no verified identity to check on later cycles, so +// it terminates now instead of polling a control path it never verified. +// +// This wrapper cannot assume stats.birthtimeMs is a real creation time. Node +// reports it in one of two unusable shapes on a filesystem or kernel that +// cannot supply one: 0 (the Linux statx() path when the filesystem reports +// no STATX_BTIME), or a copy of the change time (the generic POSIX stat() +// path on a platform with no birthtime field). A 0 value fails open, so this +// wrapper rejects it outright. A change-time copy fails closed but far too +// aggressively (it would move on every stdin file this wrapper deletes), so +// this wrapper proves the value is not a copy with a probe before it trusts +// it, run once here, before either directory's identity is captured. +async function captureSessionIdentity() { + try { + if (!(await birthtimeSurvivesProbe(sessionDir))) { + await refuseUnusableCreationTime("sessionDir", sessionDir, "its reported creation time changed after a probe write"); + return; + } + if (!(await birthtimeSurvivesProbe(stdinDir))) { + await refuseUnusableCreationTime("stdinDir", stdinDir, "its reported creation time changed after a probe write"); + return; + } + const session = await statPathIdentity(sessionDir); + const stdin = await statPathIdentity(stdinDir); + if (!isUsableBirthtimeMs(session.birthtimeMs)) { + await refuseUnusableCreationTime("sessionDir", sessionDir, "its reported creation time (" + session.birthtimeMs + ") is not usable"); + return; + } + if (!isUsableBirthtimeMs(stdin.birthtimeMs)) { + await refuseUnusableCreationTime("stdinDir", stdinDir, "its reported creation time (" + stdin.birthtimeMs + ") is not usable"); + return; + } + sessionDirIdentity = session; + stdinDirIdentity = stdin; + } catch (error) { + process.stderr.write( + "Failed to capture the process session identity: " + + (error instanceof Error ? error.message : String(error)) + ". Terminating.\\n", + ); + await latchAndTerminate(); + } +} + +// Runs on every poll cycle, before the wrapper reads stdinDir. Terminate and +// latch on any proof the control path is no longer the one this wrapper +// captured at startup (a missing path, a path that is no longer a directory, +// a symbolic link, or a directory whose identity changed), AND on every +// other lstat failure. A permission error is not transient here: a sandbox +// peer can deny search permission on the control directory without removing +// it, and treating that as transient would leave the wrapper and its child +// alive forever. The error code below only picks the stderr message, so an +// operator can still tell a removed directory from a permission error; it +// never decides whether to latch. +// +// Contrast readStdinDirNames() right below, whose catch block stays narrow +// on purpose: readdir() opens a directory descriptor, so it can fail with a +// genuinely transient error under descriptor exhaustion, and latching there +// would kill live sessions under load. lstat() opens no descriptor, and this +// function already runs before every call to readStdinDirNames(), so a +// permission error latches here before readdir() is ever reached. +async function verifySessionIdentity() { + if (identityLost) return false; + try { + const session = await statPathIdentity(sessionDir); + const stdin = await statPathIdentity(stdinDir); + if (!sameIdentity(session, sessionDirIdentity) || !sameIdentity(stdin, stdinDirIdentity)) { + await latchAndTerminate(); + return false; + } + return true; + } catch (error) { + const code = error && typeof error === "object" ? error.code : undefined; + const reason = + code === "ENOENT" + ? "the control path no longer exists" + : code === "ENOTDIR" + ? "the control path is no longer a directory" + : code === "EPAPERCLIP_SYMLINK" + ? "the control path is now a symbolic link" + : "lstat failed" + (code ? " with " + code : ""); + process.stderr.write("Latching on a lost process session identity: " + reason + ". Terminating.\\n"); + await latchAndTerminate(); + return false; + } +} + +// This catch block stays narrow on purpose: see the comment above +// verifySessionIdentity() for why a permission error here is treated as +// transient while the same error latches there. +async function readStdinDirNames() { + if (!(await verifySessionIdentity())) return []; + try { + return await fs.readdir(stdinDir); + } catch (error) { + const code = error && typeof error === "object" ? error.code : undefined; + if (code === "ENOENT" || code === "ENOTDIR") { + await latchAndTerminate(); + } + return []; + } +} + async function pollStdin() { - while (!stdinClosed) { - const entries = (await fs.readdir(stdinDir).catch(() => [])).filter((name) => name.endsWith(".json")).sort(); + while (!shuttingDown) { + const entries = (await readStdinDirNames()).filter((name) => name.endsWith(".json")).sort(); for (const name of entries) { - if (stdinClosed) break; + if (shuttingDown) break; const entrySeq = Number.parseInt(name, 10); // Hold the send order when an earlier file has not appeared. Do not consume // this later file: wait for the missing file on a later cycle, bounded by @@ -2271,7 +2645,12 @@ async function pollStdin() { const file = path.posix.join(stdinDir, name); let message; try { - const raw = await fs.readFile(file, "utf8"); + // Hardening (I3): open with O_NOFOLLOW where the platform defines it, + // so a control-path symbolic link swapped in after the directory + // check fails the read instead of following it. + const readFlag = + typeof fsConstants.O_NOFOLLOW === "number" ? fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW : "r"; + const raw = await fs.readFile(file, { encoding: "utf8", flag: readFlag }); // An empty read means the content is not on disk yet. Treat it the same // as a parse failure: keep the file and retry on a later cycle. if (!raw) throw new Error("stdin file is empty"); @@ -2317,12 +2696,17 @@ async function pollStdin() { stdinClosed = true; child.stdin.end(); break; + } else if (message.type === "shutdown") { + await terminate(); + break; } } - if (!stdinClosed) await new Promise((resolve) => setTimeout(resolve, 50)); + if (!shuttingDown) await new Promise((resolve) => setTimeout(resolve, 50)); } } +await captureSessionIdentity(); + void pollStdin().catch((error) => void writeEvent({ type: "error", message: error instanceof Error ? error.message : String(error) })); `; @@ -2335,7 +2719,7 @@ void pollStdin().catch((error) => void writeEvent({ type: "error", message: erro // command settles and the session shell (the subshell wrap around it) survives. function getProcessSessionRemoteStreamSource(): string { return `import { spawn } from "node:child_process"; -import { promises as fs } from "node:fs"; +import { promises as fs, constants as fsConstants } from "node:fs"; import path from "node:path"; const sessionDir = process.env.PAPERCLIP_PROCESS_SESSION_DIR; @@ -2345,6 +2729,9 @@ if (!sessionDir || !commandPayload) throw new Error("Missing process session bri const stdinDir = path.posix.join(sessionDir, "stdin"); let seq = 0; let stdinClosed = false; +let shuttingDown = false; +let terminated = false; +let killTimer = null; const config = JSON.parse(Buffer.from(commandPayload, "base64").toString("utf8")); await fs.mkdir(stdinDir, { recursive: true }); @@ -2356,9 +2743,36 @@ function writeEvent(event) { process.stdout.write(JSON.stringify({ seq, ...event }) + "\\n"); } +// Hardening (I3): refuse a symbolic link on a control path before this +// wrapper reads or writes through it. A symbolic link here could let another +// sandbox process redirect the wrapper's file I/O outside the session tree. +async function isSymbolicLink(candidatePath) { + try { + const stats = await fs.lstat(candidatePath); + return stats.isSymbolicLink(); + } catch { + return false; + } +} + +if ((await isSymbolicLink(sessionDir)) || (await isSymbolicLink(stdinDir))) { + await writeEvent({ type: "error", message: "Refusing a symbolic link on a process session control path." }); + process.exitCode = 1; + process.exit(1); +} + +// Hardening (I3, not containment): the wrapper's own launch env carries the +// session dir and the command payload. Scrub both keys before they reach the +// spawned child, so the child never inherits a path to its own control files. +const childEnv = { ...process.env, ...(config.env || {}) }; +delete childEnv.PAPERCLIP_PROCESS_SESSION_DIR; +delete childEnv.PAPERCLIP_PROCESS_SESSION_COMMAND_B64; + +// I1: exactly one child process per emitted wrapper. Do not add a second +// tracked child handle. const child = spawn(config.command, Array.isArray(config.args) ? config.args : [], { cwd: config.cwd || process.cwd(), - env: { ...process.env, ...(config.env || {}) }, + env: childEnv, stdio: ["pipe", "pipe", "pipe"], }); @@ -2366,12 +2780,21 @@ child.stdout.on("data", (chunk) => writeEvent({ type: "data", stream: "stdout", child.stderr.on("data", (chunk) => writeEvent({ type: "data", stream: "stderr", data: Buffer.from(chunk).toString("base64") })); child.on("error", (error) => writeEvent({ type: "error", message: error.message })); // "close" (not "exit") so stdout/stderr fully drain before the exit frame. -// Stop the stdin poll and set the exit code, then let the event loop drain: a -// natural exit flushes the stdout pipe, so the exit frame always lands. +// Queue the exit frame first, then run terminate(), so the exit frame always +// lands even when the child closes on its own, with no stdinEnd and no +// shutdown message ever received. writeEvent() only queues an asynchronous +// write. terminate()'s own synchronous work (ending the child's stdin and +// sending SIGTERM) already runs in this same handler by the time the exit +// frame becomes readable on disk. terminate() is idempotent and its +// child.kill() call here is always a no-op (I2): the child's process handle +// is already gone by the time "close" fires. An error frame carries no such +// guarantee: child.on("error", ...) below does not call terminate(), and +// neither does the poll loop's own error writes, so those fire while the +// wrapper and its child are still fully alive. child.on("close", (code, signal) => { writeEvent({ type: "exit", code, signal }); - stdinClosed = true; process.exitCode = typeof code === "number" ? code : 1; + void terminate(); }); ${PROCESS_SESSION_STDIN_POLL_TAIL}`; @@ -2379,7 +2802,7 @@ ${PROCESS_SESSION_STDIN_POLL_TAIL}`; function getProcessSessionRemoteEventFileSource(): string { return `import { spawn } from "node:child_process"; -import { promises as fs } from "node:fs"; +import { promises as fs, constants as fsConstants } from "node:fs"; import path from "node:path"; const sessionDir = process.env.PAPERCLIP_PROCESS_SESSION_DIR; @@ -2390,6 +2813,9 @@ const stdinDir = path.posix.join(sessionDir, "stdin"); const eventsDir = path.posix.join(sessionDir, "events"); let seq = 0; let stdinClosed = false; +let shuttingDown = false; +let terminated = false; +let killTimer = null; const config = JSON.parse(Buffer.from(commandPayload, "base64").toString("utf8")); await fs.mkdir(stdinDir, { recursive: true }); @@ -2408,9 +2834,36 @@ function writeEvent(event) { return write; } +// Hardening (I3): refuse a symbolic link on a control path before this +// wrapper reads or writes through it. A symbolic link here could let another +// sandbox process redirect the wrapper's file I/O outside the session tree. +async function isSymbolicLink(candidatePath) { + try { + const stats = await fs.lstat(candidatePath); + return stats.isSymbolicLink(); + } catch { + return false; + } +} + +if ((await isSymbolicLink(sessionDir)) || (await isSymbolicLink(stdinDir))) { + await writeEvent({ type: "error", message: "Refusing a symbolic link on a process session control path." }); + process.exitCode = 1; + process.exit(1); +} + +// Hardening (I3, not containment): the wrapper's own launch env carries the +// session dir and the command payload. Scrub both keys before they reach the +// spawned child, so the child never inherits a path to its own control files. +const childEnv = { ...process.env, ...(config.env || {}) }; +delete childEnv.PAPERCLIP_PROCESS_SESSION_DIR; +delete childEnv.PAPERCLIP_PROCESS_SESSION_COMMAND_B64; + +// I1: exactly one child process per emitted wrapper. Do not add a second +// tracked child handle. const child = spawn(config.command, Array.isArray(config.args) ? config.args : [], { cwd: config.cwd || process.cwd(), - env: { ...process.env, ...(config.env || {}) }, + env: childEnv, stdio: ["pipe", "pipe", "pipe"], }); @@ -2419,7 +2872,22 @@ child.stderr.on("data", (chunk) => void writeEvent({ type: "data", stream: "stde child.on("error", (error) => void writeEvent({ type: "error", message: error.message })); // "close" (not "exit") so stdout/stderr fully drain before the exit event; // the write chain then guarantees the exit file lands after every data file. -child.on("close", (code, signal) => void writeEvent({ type: "exit", code, signal })); +// Queue the exit event first, then run terminate(), so the poll loop ends +// even when the child closes on its own, with no stdinEnd and no shutdown +// message ever received. writeEvent() only queues an asynchronous write. +// terminate()'s own synchronous work (ending the child's stdin and sending +// SIGTERM) already runs in this same handler by the time the exit file +// becomes readable on disk. terminate() is idempotent and its child.kill() +// call here is always a no-op (I2): the child's process handle is already +// gone by the time "close" fires. An error event carries no such guarantee: +// child.on("error", ...) below does not call terminate(), and neither does +// the poll loop's own error writes, so those fire while the wrapper and its +// child are still fully alive. +child.on("close", (code, signal) => { + void writeEvent({ type: "exit", code, signal }); + process.exitCode = typeof code === "number" ? code : 1; + void terminate(); +}); ${PROCESS_SESSION_STDIN_POLL_TAIL}`; }