From c3b1f32c86c2c7b1b7402ef1df224b8fd7523b06 Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Sun, 31 May 2026 22:34:19 +1000 Subject: [PATCH] fix: error-output sanitization + process-lifecycle hardening (#111) (#118) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the 2026-05-31 audit (3×P2 + 1×P3), all in the error/process lifecycle layer: 1. sanitizeError() helper — streaming error paths sent raw claude error_message / stderr to the client, leaking home-dir / credential-file paths that the non-streaming path already redacted. Factored the path-strip regex into one helper and applied it at all 9 client-facing jsonResponse/sendSSE error emits; de-duped the 3 pre-existing inline .replace() sites. Operator-log calls (logEvent/trackError) and admin-gated endpoints left raw by design. 2. res.on("close") SIGKILL escalation — a client disconnect sent only SIGTERM; a SIGTERM-resistant child held its concurrency slot until the request timeout (narrow #37 on the hottest exit path). Now escalates to SIGKILL 5s after SIGTERM, cleared on proc exit. Per review: gated on the child still being alive (exitCode===null && signalCode===null) so the normal-success close no longer fires a spurious SIGTERM or leaks the 5s timer; killTimer.unref() so a genuine disconnect timer never delays graceful shutdown. 3. Per-key quota TOCTOU — documented as best-effort/eventually-consistent (inline comment + README note): concurrent requests at the boundary can overshoot by up to MAX_CONCURRENT and cache hits are uncounted. Chose documentation over an in-flight counter to avoid a decrement-on-all-paths liability (the #37 class) on a low-blast-radius internal family rate-limiter — not a payment boundary. 4. [P3] overallTimer cleared on semantic completion — the request timer was cleared only on proc exit, so a streamed response that res.end()'d before the child exited could record a spurious post-success timeout. New clearOverallTimer() (clears the timer ONLY, never touches the `cleaned` slot-accounting flag — no slot leak) is called in the streaming stop-success path; cleanup() on exit still clears it idempotently and decrements the slot. ALIGNMENT.md: error-shaping / process-lifecycle / rate-limit documentation forward no Anthropic operation, so a cli.js citation is N/A under Rule 2. No blacklisted tokens or port literals introduced; alignment.yml passes. Independent fresh-context reviewer (opus): APPROVE WITH MINOR (Iron Rule 10) — the two critical concerns (clearOverallTimer slot-leak class, SIGKILL double-kill) were verified clean; MINOR #1 (kill-timer leak on success path) folded in; MINOR #2 (pre-existing regex over-redaction of ratios/URLs) left as out-of-scope. Closes #111. Co-authored-by: dtzp555 Co-authored-by: Claude Opus 4.8 --- README.md | 2 ++ server.mjs | 53 +++++++++++++++++++++++++++++++++-------------- test-features.mjs | 31 +++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 66fb149..cf91fc2 100644 --- a/README.md +++ b/README.md @@ -486,6 +486,8 @@ When a key exceeds its quota, OCP returns HTTP 429 with a structured error: - Admin and anonymous users are never subject to quotas - PATCH is a partial update — omitted fields are left unchanged +> **Note:** quotas are best-effort. Under concurrent bursts a key can exceed its cap by up to the server's max-concurrency (default 8), and cache hits are not counted toward quota. They cap budgets for cooperative family use, not adversarial abuse. + ### Important Notes - All users share your Claude Pro/Max **rate limits** (5h session + 7d weekly) diff --git a/server.mjs b/server.mjs index d3a1702..da3fa7b 100644 --- a/server.mjs +++ b/server.mjs @@ -819,7 +819,12 @@ function spawnClaudeProcess(model, messages, conversationId, keyName) { } }, TIMEOUT); - return { proc, cliModel, conversationId, t0, cleanup, handleSessionFailure, markFirstByte }; + // Clear ONLY the request timer (not the slot accounting) when the response has + // semantically completed (result/[DONE]) but the child hasn't exited yet — prevents + // a spurious post-success timeout. cleanup() (on exit) still clears it idempotently. (issue #111) + function clearOverallTimer() { clearTimeout(overallTimer); } + + return { proc, cliModel, conversationId, t0, cleanup, clearOverallTimer, handleSessionFailure, markFirstByte }; } // ── Call claude CLI (non-streaming) ───────────────────────────────────── @@ -973,10 +978,10 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} try { ctx = spawnClaudeProcess(model, messages, conversationId, authInfo.keyName); } catch (err) { - return jsonResponse(res, 500, { error: { message: err.message, type: "proxy_error" } }); + return jsonResponse(res, 500, { error: { message: sanitizeError(err.message), type: "proxy_error" } }); } - const { proc, cliModel, conversationId: convId, t0, cleanup, handleSessionFailure, markFirstByte } = ctx; + const { proc, cliModel, conversationId: convId, t0, cleanup, clearOverallTimer, handleSessionFailure, markFirstByte } = ctx; let stderr = ""; let headersSent = false; let totalChars = 0; @@ -1047,6 +1052,7 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} res.write("data: [DONE]\n\n"); res.end(); } + clearOverallTimer(); } else if (parsed.error) { // is_error result — emit error stop; do NOT set resultEventSeen (that would @@ -1056,12 +1062,12 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} 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: errStr, type: "provider_error" } }); + jsonResponse(res, 500, { error: { message: sanitizeError(errStr), type: "provider_error" } }); } else if (!res.writableEnded && !res.destroyed) { // Headers already sent (eager ensureHeaders) — can't send a JSON 500. Surface the // failure as an SSE error frame so the client can distinguish an upstream error // from a legitimately empty completion, instead of a success-looking finish_reason:"stop". (issue #110) - sendSSE(res, { error: { message: errStr, type: "provider_error" } }, hb); + sendSSE(res, { error: { message: sanitizeError(errStr), type: "provider_error" } }, hb); res.write("data: [DONE]\n\n"); res.end(); } @@ -1091,12 +1097,12 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} // 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" } }); + jsonResponse(res, 500, { error: { message: sanitizeError(stderr.slice(0, 300) || `claude exit ${code}`), type: "proxy_error" } }); } else if (!res.writableEnded && !res.destroyed) { // Headers already sent — surface the failure as an SSE error frame instead of a // success-looking finish_reason:"stop", so the client can tell the upstream crashed // rather than returned empty. (issue #110 — sibling of the parsed.error branch above.) - sendSSE(res, { error: { message: stderr.slice(0, 300) || `claude exit ${code}`, type: "proxy_error" } }, hb); + sendSSE(res, { error: { message: sanitizeError(stderr.slice(0, 300) || `claude exit ${code}`), type: "proxy_error" } }, hb); res.write("data: [DONE]\n\n"); res.end(); } @@ -1133,7 +1139,7 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} trackError(err.message); handleSessionFailure(); if (!headersSent && !res.writableEnded && !res.destroyed) { - jsonResponse(res, 500, { error: { message: err.message, type: "proxy_error" } }); + jsonResponse(res, 500, { error: { message: sanitizeError(err.message), type: "proxy_error" } }); } else if (!res.writableEnded && !res.destroyed) { res.end(); } @@ -1142,12 +1148,27 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {} // If client disconnects, kill the process to free resources res.on("close", () => { hb.stop(); - if (!proc.killed) { + // Only escalate when the child is still alive. On the normal-success path res.end() + // also fires "close", but the child has usually already exited — skip the spurious + // SIGTERM and the 5s kill-timer entirely (a post-exit proc.once("exit") never fires, + // so the timer would otherwise leak a closure over proc for 5s per request). (issue #111) + if (!proc.killed && proc.exitCode === null && proc.signalCode === null) { try { proc.kill("SIGTERM"); } catch {} + // Mirror the overallTimer escalation (server.mjs ~818): a SIGTERM-resistant child would + // otherwise hold its concurrency slot until the request timeout — #37 on the disconnect path. (issue #111) + const killTimer = setTimeout(() => { try { proc.kill("SIGKILL"); } catch {} }, 5000); + killTimer.unref(); + proc.once("exit", () => clearTimeout(killTimer)); } }); } +// Strip absolute filesystem paths from an error message before sending it to a client. +// claude error_message / stderr routinely embed home-dir / credential-file paths. (issue #111) +function sanitizeError(msg) { + return String(msg || "Internal error").replace(/\/[\w/.\-]+/g, "[path]"); +} + // ── Response helpers ──────────────────────────────────────────────────── function jsonResponse(res, status, data) { if (res.headersSent || res.writableEnded || res.destroyed) return; @@ -1664,6 +1685,11 @@ async function handleChatCompletions(req, res) { return jsonResponse(res, 400, { error: { message: "'messages' must be a non-empty array", type: "invalid_request_error" } }); } + // NOTE: quota is best-effort / eventually-consistent. The gate reads the recorded count + // at entry and records only after the upstream completes, so concurrent requests at the + // boundary can overshoot the cap by up to MAX_CONCURRENT, and cache hits (served before + // recordUsage) are not counted. This is internal family rate-limiting, not a payment + // boundary — bounded overshoot is acceptable. (issue #111) // Quota check — only for identified per-key users (not anonymous/admin/local) if (req._authKeyId) { let exceeded; @@ -1730,8 +1756,7 @@ async function handleChatCompletions(req, res) { return; } catch (err) { if (res.headersSent || res.writableEnded || res.destroyed) { try { res.end(); } catch {} return; } - const safeMessage = (err.message || "Internal error").replace(/\/[\w/.\-]+/g, "[path]"); - return jsonResponse(res, 500, { error: { message: safeMessage, type: "proxy_error" } }); + return jsonResponse(res, 500, { error: { message: sanitizeError(err.message), type: "proxy_error" } }); } } // Default: real stream-json streaming, unchanged. @@ -1773,8 +1798,7 @@ async function handleChatCompletions(req, res) { try { res.end(); } catch {} return; } - const safeMessage = (err.message || "Internal error").replace(/\/[\w/.\-]+/g, "[path]"); - return jsonResponse(res, 500, { error: { message: safeMessage, type: "proxy_error" } }); + return jsonResponse(res, 500, { error: { message: sanitizeError(err.message), type: "proxy_error" } }); } } @@ -1792,8 +1816,7 @@ async function handleChatCompletions(req, res) { return; } // Sanitize error: strip internal file paths before sending to client - const safeMessage = (err.message || "Internal error").replace(/\/[\w/.\-]+/g, "[path]"); - jsonResponse(res, 500, { error: { message: safeMessage, type: "proxy_error" } }); + jsonResponse(res, 500, { error: { message: sanitizeError(err.message), type: "proxy_error" } }); } } diff --git a/test-features.mjs b/test-features.mjs index fa942f0..a2ce6f6 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -1623,6 +1623,37 @@ test("messages guard: [{role:'user',content:'hi'}] → valid", () => { assert.equal(isValidMessages([{ role: "user", content: "hi" }]), true); }); +// ── sanitizeError helper (issue #111) ──────────────────────────────────── +// Replicated verbatim from server.mjs (cannot import server.mjs). +// The SIGKILL-escalation and timer changes are process-lifecycle and are not +// unit-testable here (no live-server harness). +console.log("\nsanitizeError (issue #111):"); + +function sanitizeError(msg) { + return String(msg || "Internal error").replace(/\/[\w/.\-]+/g, "[path]"); +} + +test("sanitizeError: strips home-dir path from message", () => { + const result = sanitizeError("failed at /Users/foo/.claude/creds.json"); + assert.ok(result.includes("[path]"), `expected [path] in: ${result}`); + assert.ok(!result.includes("/Users/foo"), `expected /Users/foo stripped, got: ${result}`); +}); + +test("sanitizeError: null input returns 'Internal error'", () => { + assert.equal(sanitizeError(null), "Internal error"); +}); + +test("sanitizeError: message with no path passes through unchanged", () => { + assert.equal(sanitizeError("no path here"), "no path here"); +}); + +test("sanitizeError: multiple paths all stripped", () => { + const result = sanitizeError("err /a/b and /c/d"); + assert.ok(!result.includes("/a/b"), `expected /a/b stripped, got: ${result}`); + assert.ok(!result.includes("/c/d"), `expected /c/d stripped, got: ${result}`); + assert.ok(result.includes("[path]"), `expected [path] in: ${result}`); +}); + // ── Cleanup ── closeDb();