From 05a984df891d7750cea1127c18caa61dd435a9be Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Sun, 31 May 2026 13:17:36 +1000 Subject: [PATCH] fix: OCP code-audit P1+P2 hardening (crash bugs + multi-tenant gates) (#106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(daemon): P1-1 guard proc.stdin against EPIPE crash In spawnClaudeProcess, attach an error listener on proc.stdin BEFORE the write/end calls so an EPIPE (child closed stdin mid-write) is swallowed and logged rather than thrown as an unhandled exception. The existing proc.on("error") listener is on the ChildProcess object, NOT on the stdin Writable — it does not catch stdin write errors. Hardening per OCP code audit; entry-surface contract unchanged for single-user default path. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(tui): P1-3 remove tool_use from isTerminalLine; only turn_duration is terminal In interactive TUI mode, stop_reason=tool_use does NOT mean the turn is complete. Claude handles the tool call internally and continues generating — the transcript advances to another assistant entry. Treating tool_use as terminal truncated tool-using turns mid-flight. Only {type:"system", subtype:"turn_duration"} is the authoritative completion marker (claude CLI v2.1.157+ interactive session transcript). Updated two unit tests that previously asserted tool_use → true; they now assert false (the correct behaviour). The real-fixture terminal detection test is unaffected because the fixture uses turn_duration. Hardening per OCP code audit; TUI path behaviour fix. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(tui): P2-5 add -l (literal) flag to send-keys prompt paste A prompt that equals a tmux key token (e.g. "C-c", "Escape") would be interpreted as that key binding rather than typed as literal text. The -l flag forces literal character-by-character input. The separate Enter key event afterward deliberately omits -l so tmux sends a real carriage-return keypress to submit the prompt line. Authority: tmux send-keys(1) § -l flag. Hardening per OCP code audit. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(audit): String-coerce parsed.error + stale tool_use comment (review fast-follow) Folds the 2 minor findings from the independent review of the audit fixes: - String(parsed.error) before .slice/message in callClaude + callClaudeStreaming (defensive: claude could emit a non-string result/error_message). - correct the readTuiTranscript comment that still listed tool_use as terminal. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: dtzp555 Co-authored-by: Claude Opus 4.8 (1M context) --- lib/tui/session.mjs | 8 +- lib/tui/transcript.mjs | 16 ++-- server.mjs | 175 +++++++++++++++++++++++++++++++++-------- test-features.mjs | 8 +- 4 files changed, 166 insertions(+), 41 deletions(-) diff --git a/lib/tui/session.mjs b/lib/tui/session.mjs index e6e1537..3c0342d 100644 --- a/lib/tui/session.mjs +++ b/lib/tui/session.mjs @@ -223,9 +223,15 @@ export async function runTuiTurn({ // 2. Submit prompt body via `"$(cat file)"` — byte-safe for any content — // then settle, then send a SEPARATE Enter key event to submit the line. + // + // The `-l` (literal) flag is required on the paste send-keys call so that + // a prompt that happens to equal a tmux key token (e.g. "C-c", "Escape") + // is typed literally as text rather than being interpreted as a key binding. + // The SEPARATE Enter event below deliberately omits -l so that tmux sends a + // real keypress (carriage return) to submit the prompt line. spawnSync( "sh", - ["-c", `${shq(TMUX)} send-keys -t ${shq(tmuxName)} -- "$(cat ${shq(promptFile)})"`], + ["-c", `${shq(TMUX)} send-keys -t ${shq(tmuxName)} -l -- "$(cat ${shq(promptFile)})"`], { env, encoding: "utf8" }, ); await sleep(PASTE_SETTLE_MS); diff --git a/lib/tui/transcript.mjs b/lib/tui/transcript.mjs index 496d2a6..773ce07 100644 --- a/lib/tui/transcript.mjs +++ b/lib/tui/transcript.mjs @@ -52,12 +52,18 @@ export function parseTranscriptLines(text) { } // A line marks the assistant turn complete when it is the turn_duration system -// event, or an assistant message that stopped to hand off to a tool. +// event. That is the ONLY reliable terminal marker in interactive TUI mode. +// +// Why tool_use is NOT a terminal marker: +// In interactive claude, when the model decides to call a tool (stop_reason= +// "tool_use"), claude handles the tool call internally and then continues +// generating — the turn is NOT complete. The transcript advances to another +// assistant entry after the tool result. Only {type:"system", +// subtype:"turn_duration"} signals that claude has fully finished the turn. +// Treating tool_use as terminal would truncate tool-using turns mid-flight. export function isTerminalLine(obj) { if (!obj || typeof obj !== "object") return false; - if (obj.type === "system" && obj.subtype === "turn_duration") return true; - const sr = (obj.message && obj.message.stop_reason) || obj.stop_reason; - return sr === "tool_use"; + return obj.type === "system" && obj.subtype === "turn_duration"; } // Text of the LAST assistant turn: concatenate its text content blocks @@ -96,7 +102,7 @@ export function verifyEntrypoint(events) { return null; } -// Block until the session transcript is terminal (turn_duration / tool_use) or +// Block until the session transcript is terminal (turn_duration) or // the wall-clock cap elapses, polling the file (no fs.watch — robust over NFS / // editors). Returns the latest assistant text. On cap with text, returns the // partial text; on cap with no text at all, throws. diff --git a/server.mjs b/server.mjs index c327b1f..0f0aeb0 100644 --- a/server.mjs +++ b/server.mjs @@ -293,10 +293,15 @@ const TUI_CWD = process.env.OCP_TUI_CWD || `${process.env.HOME}/.ocp-tui/work` const TUI_HOME = process.env.OCP_TUI_HOME || process.env.HOME; const TUI_ENTRYPOINT = process.env.OCP_TUI_ENTRYPOINT || "cli"; // cli|auto|off — see ADR 0007 -// SECURITY fail-loud: TUI-mode is incompatible with multi-user auth. Under TUI a -// guest/anonymous prompt would run interactive claude with the OPERATOR's full -// filesystem access (home is NOT isolation). Refuse to boot until B-path isolation -// (tools-off + per-key ephemeral home + sandbox) lands. See ADR 0007. +// SECURITY fail-loud: TUI-mode is incompatible with any configuration that allows +// non-operator prompts to reach the interactive claude session. Three cases: +// 1. AUTH_MODE=multi — guest/anonymous keys can submit prompts. +// 2. BIND_ADDRESS=0.0.0.0 — server is LAN-exposed; any LAN peer can send prompts +// unless per-request trust is in place. Override with OCP_TUI_ALLOW_LAN=1 +// ONLY if you have a separate network-layer trust (firewall, VPN). +// 3. PROXY_ANONYMOUS_KEY set — anonymous callers can submit prompts without a key. +// In all three cases TUI runs interactive claude with the OPERATOR's full filesystem +// access — home is NOT isolation. Refuse to boot. See ADR 0007. if (TUI_MODE && AUTH_MODE === "multi") { console.error( "FATAL: CLAUDE_TUI_MODE=true is incompatible with CLAUDE_AUTH_MODE=multi.\n" + @@ -306,6 +311,25 @@ if (TUI_MODE && AUTH_MODE === "multi") { ); process.exit(1); } +if (TUI_MODE && BIND_ADDRESS === "0.0.0.0" && process.env.OCP_TUI_ALLOW_LAN !== "1") { + console.error( + "FATAL: CLAUDE_TUI_MODE=true with CLAUDE_BIND=0.0.0.0 is unsafe.\n" + + " TUI runs interactive claude with operator filesystem access; LAN-exposed without\n" + + " per-request isolation means any LAN peer could drive the operator's claude session.\n" + + " Either bind to 127.0.0.1 (default) or set OCP_TUI_ALLOW_LAN=1 if you have a\n" + + " separate network-layer trust (firewall/VPN). See docs/adr/0007-tui-interactive-mode.md." + ); + process.exit(1); +} +if (TUI_MODE && PROXY_ANONYMOUS_KEY) { + console.error( + "FATAL: CLAUDE_TUI_MODE=true with PROXY_ANONYMOUS_KEY set is unsafe.\n" + + " TUI runs interactive claude with operator filesystem access; anonymous callers\n" + + " could drive the operator's claude session without a named key.\n" + + " Remove PROXY_ANONYMOUS_KEY or disable TUI-mode. See docs/adr/0007-tui-interactive-mode.md." + ); + process.exit(1); +} if (PROXY_ANONYMOUS_KEY && AUTH_MODE !== "multi") { console.warn("WARNING: PROXY_ANONYMOUS_KEY is set but AUTH_MODE is not 'multi' — anonymous key will be ignored"); @@ -566,7 +590,27 @@ function buildCliArgs(cliModel, systemPrompt) { ]; // Permissions - if (SKIP_PERMISSIONS) { + // ADR 0007 B-path: in multi-tenant mode, suppress operator-FS tools so a guest + // prompt cannot drive Bash/Read/Write/Edit/etc. on the operator's filesystem. + // For AUTH_MODE !== "multi" (none/shared — single-operator/trusted), preserve + // existing behaviour unchanged. + if (AUTH_MODE === "multi") { + // Disallow the full operator-FS + web + agent surface. "--disallowedTools" may + // be repeated; claude accepts multiple occurrences (TUI path already uses it). + args.push( + "--disallowedTools", "Bash", + "--disallowedTools", "Read", + "--disallowedTools", "Write", + "--disallowedTools", "Edit", + "--disallowedTools", "Glob", + "--disallowedTools", "Grep", + "--disallowedTools", "WebFetch", + "--disallowedTools", "WebSearch", + "--disallowedTools", "Agent", + "--disallowedTools", "mcp__*", + ); + // Do NOT push --allowedTools in multi mode. + } else if (SKIP_PERMISSIONS) { args.push("--dangerously-skip-permissions"); } else if (ALLOWED_TOOLS.length > 0) { args.push("--allowedTools", ...ALLOWED_TOOLS); @@ -730,6 +774,11 @@ function spawnClaudeProcess(model, messages, conversationId, keyName) { } } + // Guard stdin writes against EPIPE (child may close stdin before we finish + // writing, e.g. early exit on bad model). The ChildProcess "error" event is on + // the spawned process, NOT on the stdin Writable — it does not catch this. + proc.stdin.on("error", (e) => logEvent("warn", "stdin_write_error", { error: e.message })); + // Write prompt to stdin immediately proc.stdin.write(prompt); proc.stdin.end(); @@ -794,7 +843,7 @@ function callClaude(model, messages, conversationId, keyName) { resultEventSeen = true; } else if (parsed.error) { // is_error result — treat as process error - reject(new Error(parsed.error)); + reject(new Error(String(parsed.error))); } } }); @@ -916,6 +965,10 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} let lineBuffer = ""; let isFirstDelta = true; let resultEventSeen = false; + // Separate flag for is_error result — must NOT be conflated with resultEventSeen. + // If errored===true the close handler must not cache the response or record success + // (mirrors callClaude which rejects and never caches on is_error). + let errored = false; function ensureHeaders() { if (res.writableEnded || res.destroyed) return false; @@ -977,12 +1030,14 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} } } else if (parsed.error) { - // is_error result — emit error stop - resultEventSeen = true; - logEvent("error", "claude_result_error", { model: cliModel, error: parsed.error.slice(0, 200) }); - trackError(parsed.error.slice(0, 200)); + // is_error result — emit error stop; do NOT set resultEventSeen (that would + // cause the close handler to record success + write cache). Set errored instead. + errored = true; + const errStr = String(parsed.error); + logEvent("error", "claude_result_error", { model: cliModel, error: errStr.slice(0, 200) }); + trackError(errStr.slice(0, 200)); if (!headersSent && !res.writableEnded && !res.destroyed) { - jsonResponse(res, 500, { error: { message: parsed.error, type: "provider_error" } }); + jsonResponse(res, 500, { error: { message: errStr, type: "provider_error" } }); } else if (!res.writableEnded && !res.destroyed) { sendSSE(res, { id, object: "chat.completion.chunk", created, model, @@ -1005,13 +1060,17 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} // Tolerate null exit code when result event was seen (sandbox-wrap noise, same // as OLP commit 2864275 — bwrap shell exits null after model completes). - if (code !== 0 && !resultEventSeen) { + // Also route to the error path when errored===true (is_error result received): + // never record success or write cache for an errored response. + if ((code !== 0 && !resultEventSeen) || errored) { recordModelError(cliModel, false); try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0), responseChars: 0, elapsedMs: elapsed, success: false }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } - logEvent("error", "claude_exit", { model: cliModel, code, signal: signal || "none", elapsed, stderr: stderr.slice(0, 300) }); + logEvent("error", "claude_exit", { model: cliModel, code, signal: signal || "none", elapsed, errored, stderr: stderr.slice(0, 300) }); trackError(stderr.slice(0, 300) || `claude exit ${code}`); handleSessionFailure(); + // If the error was already sent inline (parsed.error branch above), the + // response may be writableEnded — nothing more to send. if (!headersSent && !res.writableEnded && !res.destroyed) { jsonResponse(res, 500, { error: { message: stderr.slice(0, 300) || `claude exit ${code}`, type: "proxy_error" } }); } else if (!res.writableEnded && !res.destroyed) { @@ -1027,7 +1086,7 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} breakerRecordSuccess(cliModel); try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0), responseChars: totalChars, elapsedMs: elapsed, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } logEvent("info", "claude_ok", { model: cliModel, chars: totalChars, elapsed, session: convId ? convId.slice(0, 12) + "..." : "none" }); - // Cache write-back for streaming + // Cache write-back for streaming — only on true success (not errored) if (CACHE_TTL > 0 && authInfo.cacheHash) { try { setCachedResponse(authInfo.cacheHash, model, cachedContent); } catch (e) { logEvent("error", "cache_write_failed", { error: e.message }); } } @@ -1507,9 +1566,16 @@ async function handleSettings(req, res) { // PATCH let body = ""; - for await (const chunk of req) { - body += chunk; - if (body.length > 10000) return jsonResponse(res, 413, { error: "Body too large" }); + try { + for await (const chunk of req) { + body += chunk; + if (body.length > 10000) return jsonResponse(res, 413, { error: "Body too large" }); + } + } catch (e) { + if (!res.headersSent && !res.writableEnded) { + try { return jsonResponse(res, 400, { error: { message: "request aborted", type: "invalid_request_error" } }); } catch {} + } + return; } let updates; try { updates = JSON.parse(body); } catch { return jsonResponse(res, 400, { error: "Invalid JSON" }); } @@ -1546,11 +1612,18 @@ const VALID_MODELS = new Set(Object.keys(MODEL_MAP)); async function handleChatCompletions(req, res) { let body = ""; - for await (const chunk of req) { - body += chunk; - if (body.length > MAX_BODY_SIZE) { - return jsonResponse(res, 413, { error: { message: "Request body too large (max 5MB)", type: "invalid_request_error" } }); + try { + for await (const chunk of req) { + body += chunk; + if (body.length > MAX_BODY_SIZE) { + return jsonResponse(res, 413, { error: { message: "Request body too large (max 5MB)", type: "invalid_request_error" } }); + } } + } catch (e) { + if (!res.headersSent && !res.writableEnded) { + try { return jsonResponse(res, 400, { error: { message: "request aborted", type: "invalid_request_error" } }); } catch {} + } + return; } let parsed; @@ -1796,6 +1869,12 @@ const server = createServer(async (req, res) => { req._authKeyName = authKeyName; req._authKeyId = authKeyId; + // isAdmin computed here (early, before any admin-gated handler) so that + // DELETE /sessions, GET /logs, GET /usage, GET /status, PATCH /settings + // can all gate on it. Localhost and explicit admin key are always admin; + // in multi-tenant mode only the "admin" named key qualifies. + const isAdmin = AUTH_MODE !== "multi" || authKeyName === "admin" || isLocalhost; + // GET /v1/models if (req.url === "/v1/models" && req.method === "GET") { return jsonResponse(res, 200, { @@ -1857,15 +1936,17 @@ const server = createServer(async (req, res) => { }); } - // DELETE /sessions — clear all sessions + // DELETE /sessions — clear all sessions (mutating; admin only) if (req.url === "/sessions" && req.method === "DELETE") { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); const count = sessions.size; sessions.clear(); return jsonResponse(res, 200, { cleared: count }); } - // GET /sessions — list active sessions + // GET /sessions — list active sessions (operator data; admin only) if (req.url === "/sessions" && req.method === "GET") { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); const list = []; for (const [id, s] of sessions) { // id is "${keyName}|${conversationId}"; expose only the public-facing conversationId @@ -1875,34 +1956,45 @@ const server = createServer(async (req, res) => { return jsonResponse(res, 200, { sessions: list }); } - // GET /usage — fetch plan usage limits from Anthropic API + // GET /usage — fetches plan usage from Anthropic API with operator token; admin only if (req.url === "/usage" && req.method === "GET") { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); return handleUsage(req, res); } - // GET /logs — recent proxy log entries (errors and key events) + // GET /logs — recent proxy log entries (errors and key events); admin only if (req.url?.startsWith("/logs") && req.method === "GET") { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); return handleLogs(req, res); } - // GET /status — combined usage + health summary + // GET /status — combined usage + health summary; uses operator token; admin only if (req.url === "/status" && req.method === "GET") { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); return handleStatus(req, res); } - // GET /settings — view current tunable settings - // PATCH /settings — update settings at runtime (JSON body) + // GET /settings — view current tunable settings (admin only) + // PATCH /settings — update settings at runtime (JSON body; admin only, mutating) if (req.url === "/settings" && (req.method === "GET" || req.method === "PATCH")) { + if (!isAdmin) return jsonResponse(res, 403, { error: { message: "admin only", type: "auth_error" } }); return handleSettings(req, res); } // ── Key management API ── - const isAdmin = AUTH_MODE !== "multi" || authKeyName === "admin" || isLocalhost; + // (isAdmin is computed early in the request handler, before the admin-gated routes) if (req.url === "/api/keys" && req.method === "POST") { if (!isAdmin) return jsonResponse(res, 403, { error: "Admin access required" }); let body = ""; - for await (const chunk of req) body += chunk; + try { + for await (const chunk of req) { body += chunk; if (body.length > 10000) return jsonResponse(res, 413, { error: "Body too large" }); } + } catch (e) { + if (!res.headersSent && !res.writableEnded) { + try { return jsonResponse(res, 400, { error: { message: "request aborted", type: "invalid_request_error" } }); } catch {} + } + return; + } let parsed; try { parsed = JSON.parse(body); } catch { return jsonResponse(res, 400, { error: "Invalid JSON" }); } const name = parsed.name || `key-${Date.now()}`; @@ -1928,7 +2020,14 @@ const server = createServer(async (req, res) => { if (!isAdmin) return jsonResponse(res, 403, { error: "Admin access required" }); const idOrName = decodeURIComponent(req.url.split("/api/keys/")[1].replace("/quota", "")); let body = ""; - for await (const chunk of req) { body += chunk; if (body.length > 10000) return jsonResponse(res, 413, { error: "Body too large" }); } + try { + for await (const chunk of req) { body += chunk; if (body.length > 10000) return jsonResponse(res, 413, { error: "Body too large" }); } + } catch (e) { + if (!res.headersSent && !res.writableEnded) { + try { return jsonResponse(res, 400, { error: { message: "request aborted", type: "invalid_request_error" } }); } catch {} + } + return; + } let quotaBody; try { quotaBody = JSON.parse(body); } catch { return jsonResponse(res, 400, { error: "Invalid JSON" }); } // Validate quota values: must be positive integers or null @@ -2032,6 +2131,20 @@ const server = createServer(async (req, res) => { }); +// ── Process-level safety nets ──────────────────────────────────────────── +// Prevent unhandled async rejections and synchronous exceptions from crashing +// the daemon. Each registers once at module level so they are installed before +// the first request arrives. These are global no-ops on the happy path. +process.on("unhandledRejection", (e) => + logEvent("error", "unhandled_rejection", { error: e && e.message ? e.message : String(e) }) +); +process.on("uncaughtException", (e) => + logEvent("error", "uncaught_exception", { error: e && e.message ? e.message : String(e) }) +); +// Destroy the socket on low-level HTTP parse errors so broken connections +// don't accumulate as open file descriptors. +server.on("clientError", (err, socket) => { try { socket.destroy(); } catch {} }); + // ── Graceful shutdown ──────────────────────────────────────────────────── let shuttingDown = false; diff --git a/test-features.mjs b/test-features.mjs index a24e31e..0690aa5 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -1309,11 +1309,11 @@ test("parseTranscriptLines skips blank + malformed/partial lines", () => { test("isTerminalLine true on turn_duration", () => { assert.equal(isTerminalLine({ type: "system", subtype: "turn_duration" }), true); }); -test("isTerminalLine true on stop_reason tool_use (message-wrapped)", () => { - assert.equal(isTerminalLine({ type: "assistant", message: { stop_reason: "tool_use" } }), true); +test("isTerminalLine false on stop_reason tool_use (message-wrapped) — tool_use is mid-turn in TUI mode", () => { + assert.equal(isTerminalLine({ type: "assistant", message: { stop_reason: "tool_use" } }), false); }); -test("isTerminalLine true on stop_reason tool_use (flat)", () => { - assert.equal(isTerminalLine({ stop_reason: "tool_use" }), true); +test("isTerminalLine false on stop_reason tool_use (flat) — claude continues after tool, turn not done", () => { + assert.equal(isTerminalLine({ stop_reason: "tool_use" }), false); }); test("isTerminalLine false on ordinary assistant text line", () => { assert.equal(isTerminalLine({ type: "assistant", message: { content: [{ type: "text", text: "hi" }] } }), false);