From 4277ecbb2e7b80cda02c86641a14ae31d0d1a5ba Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Wed, 26 Aug 2026 15:12:49 -0700 Subject: [PATCH] fix(adapter-utils): terminate the remote process-session wrapper deterministically on bridge stop (#12244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Adapter utilities run remote process-session wrappers for sandbox work. > - A wrapper can outlive its host run when the host removes its session directory during shutdown. > - A failed directory read can look like an empty queue, so the wrapper can poll forever. > - This pull request adds an explicit shutdown acknowledgement and fail-closed identity checks. > - The benefit is deterministic wrapper cleanup without killing an unrelated session. ## Linked Issues or Issue Description Refs: #11916 **What happened?** Remote process-session wrappers could remain after a host run ended. The host could remove the session directory before the wrapper read the shutdown marker. The wrapper then treated directory errors as an empty queue and continued to poll. **Expected behavior** The host must receive an explicit shutdown acknowledgement before it treats the wrapper as stopped. The wrapper must stop when its session identity becomes invalid or untrusted. **Steps to reproduce** 1. Start a remote process-session wrapper. 2. Stop the bridge while the wrapper polls its session directory. 3. Remove the session directory during the poll. 4. Observe that the wrapper must terminate with its child. **Paperclip version or commit** `7cfbd1ecbe4a40261ba51fed07f624524352ada2` **Deployment mode** Built from source with the adapter-utils test suite. ## What Changed - Add a shutdown control file and wait for a bounded `shutdownAck` before session cleanup. - Require `shutdownAck` as proof of host-side shutdown. - Capture and verify session and stdin directory identity before each poll. - Terminate and latch the wrapper on missing, changed, linked, non-directory, or untrusted paths. - Reject unusable creation times and treat all identity-check `lstat` errors as terminal. - Add focused regression coverage for shutdown races and identity failures. ## Verification - `npx vitest run packages/adapter-utils/src/execution-target-stdin-race.test.ts` passes. - The full execution-target set passes: 175 tests across three files. - The `packages/adapter-utils` typecheck passes with `tsc --noEmit`. - CI will run on this pull request. - Greptile will review the pull request. ## Risks - A platform with unreliable directory creation times can stop a wrapper earlier than before. This fail-closed result prevents an orphan. - A transient identity-check error now stops the wrapper. This favors cleanup over continued polling when the session identity cannot be trusted. - Session cleanup remains unconditional after the bounded acknowledgement wait. ## Model Used OpenAI Codex — GPT-5. Context window size is not exposed in this run. The model used tool calls and code execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../src/execution-target-stdin-race.test.ts | 1204 ++++++++++++++++- .../adapter-utils/src/execution-target.ts | 498 ++++++- 2 files changed, 1682 insertions(+), 20 deletions(-) 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}`; }