fix: error-output sanitization + process-lifecycle hardening (#111) (#118)

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 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

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)

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: sanitizeError(err.message), type: "proxy_error" } });
-        return jsonResponse(res, 500, { error: { message: safeMessage, 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: sanitizeError(err.message), type: "proxy_error" } });
-      return jsonResponse(res, 500, { error: { message: safeMessage, 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: sanitizeError(err.message), type: "proxy_error" } });
-    jsonResponse(res, 500, { error: { message: safeMessage, type: "proxy_error" } });
   }
 }
 

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();