fix(security): harden public error boundaries

changelog.d/fixes/0000-public-error-boundary-hardening.md

@@ -0,0 +1 @@
+- **fix(security):** Sanitize provider and runtime failures before public API, SSE and MCP responses and before persistent request, proxy and usage logs, preventing credentials, stack traces and host filesystem paths from crossing those boundaries while preserving stable error codes and useful diagnostics.

docs/security/ERROR_SANITIZATION.md

@@ -1,14 +1,16 @@
 ---
 title: "Error Message Sanitization"
-version: 3.8.40
-lastUpdated: 2026-06-28
+version: 3.8.51
+lastUpdated: 2026-09-02
 ---
 
 # Error Message Sanitization
 
-> **Source of truth:** `open-sse/utils/error.ts` — `sanitizeErrorMessage`, `buildErrorBody`, `createErrorResult`
-> **Tests:** `tests/unit/error-message-sanitization.test.ts`
-> **Last updated:** 2026-06-28 — v3.8.40
+> **Source of truth:** `open-sse/utils/errorSanitization.ts`,
+> `open-sse/utils/errorPathRedaction.ts`, and the public builders in `open-sse/utils/error.ts`
+> **Tests:** `tests/unit/error-message-sanitization.test.ts`,
+> `tests/unit/error-public-boundaries-hardening.test.ts`
+> **Last updated:** 2026-09-02 — v3.8.51
 > **Audience:** Any engineer touching error responses (HTTP routes, SSE streams, executors, MCP handlers).
 > **Status:** **MANDATORY** for every code path that returns an error message to a client.
 
@@ -20,10 +22,18 @@ CodeQL rule `js/stack-trace-exposure` (CWE-209) flags any code path where an err
 - Library / framework versions inferred from stack frames → targeted exploit selection.
 - Sensitive runtime values that may be string-interpolated into errors (DB queries, config values).
 
-The `sanitizeErrorMessage` helper in `open-sse/utils/error.ts` strips both classes of leakage:
+The `sanitizeErrorMessage` helper exported by `open-sse/utils/error.ts` strips these classes of
+leakage:
 
-1. Multi-line stack traces — only the first line (the actual error message) is kept.
-2. Absolute paths (`/...*.{ts,js,tsx,jsx,mjs,cjs}[:line[:col]]` and `C:\...`) — replaced with `<path>`.
+1. Physical, serialized, and unambiguously inline JavaScript stack-frame tails.
+2. Absolute POSIX, Windows, UNC, and `file://` filesystem paths, while preserving safe HTTPS URLs
+   and explicitly marked API routes.
+3. Credential assignments, common provider token formats, private-key PEM blocks, and base64 data
+   URLs.
+
+The sanitizer caps input length and fails closed when a thrown value rejects string coercion.
+Recursive upstream JSON sanitization also drops unsafe credential/path keys, session aliases, and
+prototype-control keys before a response is serialized.
 
 ## The mandatory pattern
 
@@ -59,7 +69,10 @@ import {
 } from "@omniroute/open-sse/utils/error.ts";
 ```
 
-All of these route through `buildErrorBody` and therefore through `sanitizeErrorMessage`. **You never need to call `sanitizeErrorMessage` manually** when using these helpers.
+All of these apply the canonical public-error boundary. `errorResponse`, `writeStreamError`, and
+`createErrorResult` route through `buildErrorBody`; the three specialized retry/circuit helpers
+project and sanitize their public context directly. **You never need to call
+`sanitizeErrorMessage` manually** when using these helpers.
 
 ### 2. Custom error envelopes (rare)
 
@@ -81,17 +94,25 @@ This is the only sanctioned way to assemble a custom error body. See `open-sse/e
 
 ### 3. Logging vs. responding
 
-`sanitizeErrorMessage` should **only** wrap the value that crosses the network boundary. Internal logs (`pino`, `console`) should keep the full message, including stack, so operators can debug. Pattern:
+Trusted internal exceptions may keep their full message and stack so operators can debug. Values
+originating at provider, validation, browser-session, or credential-adjacent boundaries must be
+sanitized before they enter console output, audit metadata, or persistent call logs. Pattern:
 
 ```ts
 try {
   // ...
 } catch (err) {
-  log.error({ err }, "handler failed"); // full err with stack — internal log
+  log.error({ err }, "handler failed"); // trusted internal exception only
   return errorResponse(500, getErrorMessage(err)); // sanitized — sent to client
 }
 ```
 
+For provider-controlled failures, project the logged value too:
+
+```ts
+log.error({ message: sanitizeErrorMessage(err) || "Provider request failed" });
+```
+
 ### 4. Forbidden patterns
 
 ❌ **Never** put raw exception output in a Response body:
@@ -112,7 +133,9 @@ const safe = String(err).split("\n")[0];
 
 ❌ **Never** sanitize in the route and forget the SSE path. Anything that writes to a stream goes through `writeStreamError` (or its underlying `buildErrorBody`).
 
-❌ **Never** include `process.cwd()`, `__filename`, `__dirname`, env-derived paths in error messages — they bypass the path regex and reveal the deployment topology.
+❌ **Never** intentionally include `process.cwd()`, `__filename`, `__dirname`, or env-derived paths
+in error messages. The sanitizer covers absolute paths as defense in depth, but callers must not
+construct topology-bearing messages in the first place.
 
 ## Coverage in CI
 
@@ -129,7 +152,9 @@ When adding a new route or executor, copy the assertion pattern from this file.
 ## Related controls
 
 - `js/stack-trace-exposure` CodeQL alerts in `.github/security` should always be **either** fixed via these helpers **or** dismissed with a comment citing this doc.
-- The `pino` redaction config (`src/shared/utils/logRedaction.ts`) handles structured log redaction separately. This doc covers only the response-message surface.
+- The `pino` redaction config (`src/shared/utils/logRedaction.ts`) handles trusted structured logs
+  separately. This document covers public response messages and provider-controlled values that
+  cross persistent call/proxy-log boundaries.
 - Upstream-header denylist (`src/shared/constants/upstreamHeaders.ts`) covers header leakage — keep both files aligned when adding a new exfiltration concern.
 
 ## Upstream details passthrough
@@ -138,27 +163,39 @@ When adding a new route or executor, copy the assertion pattern from this file.
 parsed body from the upstream provider). When provided, it is sanitized by
 `sanitizeUpstreamDetails` before inclusion in the response as `upstream_details`.
 
-An optional fourth argument `classification` (`{ type?: string; code?: string }`)
-preserves an explicit error type/code instead of re-deriving both from the
-status-code table — used when the caller already classified the failure (e.g.
-HTTP 499 → `client_disconnected`).
+An optional fourth argument `classification`
+(`{ type?: string; code?: string; reason?: string }`) accepts an explicit public classification.
+Every field is projected onto the bounded public-identifier vocabulary. Unsafe, credential-shaped,
+control-character, or overlong values fall back to the status-derived type/code; an unsafe optional
+reason is omitted. Three-digit HTTP status identifiers (`100` through `599`) remain valid for
+provider contracts that expose the numeric upstream status as a machine-readable code. The same
+bounded range is accepted in the locally generated HTTP-status placeholder form; arbitrary provider
+numbers and names remain outside the vocabulary.
+
+Pass every explicit classification in that fourth argument. Never overwrite
+`body.error.code`, `body.error.type`, or `body.error.reason` after `buildErrorBody()` returns;
+post-builder mutation bypasses the public projection.
 
 Sanitization rules applied to `upstreamDetails`:
 
 1. String leaves: run through `sanitizeErrorMessage` (strips stacks + absolute paths).
-2. Key blocklist: keys matching `/stack|trace|path|file|cwd|dir|password|secret|token|key/i`
-   are removed.
+2. Unsafe path, credential, session-alias, and prototype-control keys are removed.
 3. Depth cap: nesting beyond 4 levels is replaced with the string `"[truncated]"`.
 4. Arrays are capped at 32 elements.
 
-Only the seven upstream-error `createErrorResult` call sites in `chatCore.ts` pass
-`upstreamErrorBody`. Internal OmniRoute errors (SSE parse failures, empty content,
-guardrail blocks) do not include `upstream_details`.
+Only call sites with a parsed provider error body should pass `upstreamDetails`. Internal OmniRoute
+errors (SSE parse failures, empty content, guardrail blocks) must not include it.
 
 Do NOT pass raw `err.stack`, `err.message`, or any string from a runtime exception to
 `upstreamDetails`. Those must still go through `errorResponse` / `buildErrorBody(code, msg)`
 without an upstream body.
 
+Selective upstream 4xx passthrough preserves the provider's safe JSON shape and wording required by
+client auto-recovery, but it is not byte-for-byte passthrough: the recursive sanitizer always runs
+before serialization. Cyclic, BigInt-bearing, or hostile `toJSON()` bodies fail closed and are not
+eligible for passthrough. OCR and moderation apply the same rule; non-JSON, blank, or mislabeled
+upstream bodies are converted to the canonical OmniRoute JSON error envelope.
+
 ## Known CodeQL limitation: custom sanitizers not recognized
 
 The CodeQL query [`js/stack-trace-exposure`](https://codeql.github.com/codeql-query-help/javascript/js-stack-trace-exposure/) uses a fixed allowlist of sanitizer patterns (e.g. inline `.split("\n")[0]`, `String#replace` with specific regex shapes, access to `.message` on `Error`). It does **not** recognize indirection through a custom helper like our `sanitizeErrorMessage()`.

open-sse/executors/claude-web.ts

@@ -216,9 +216,10 @@ function makeErrorResponse(
     extraHeaders?: Record<string, string>;
   }
 ): Response {
-  const body = buildErrorBody(status, message, options?.details);
-  if (options?.type) body.error.type = options.type;
-  if (options?.code) body.error.code = options.code;
+  const body = buildErrorBody(status, message, options?.details, {
+    type: options?.type,
+    code: options?.code,
+  });
   const headers: Record<string, string> = { "Content-Type": "application/json" };
   if (options?.extraHeaders) {
     for (const [key, value] of Object.entries(options.extraHeaders)) {

open-sse/executors/claude-web/stream.ts

@@ -453,9 +453,10 @@ function makeChunk(
 }
 
 function protocolErrorBody(): Record<string, unknown> {
-  const body = buildErrorBody(502, "Claude Web stream protocol error");
-  body.error.type = "upstream_protocol_error";
-  body.error.code = "claude_web_protocol_error";
+  const body = buildErrorBody(502, "Claude Web stream protocol error", undefined, {
+    type: "upstream_protocol_error",
+    code: "claude_web_protocol_error",
+  });
   return body as unknown as Record<string, unknown>;
 }
 

open-sse/executors/ninerouter.ts

@@ -72,8 +72,7 @@ export class NineRouterExecutor extends BaseExecutor {
    * Message goes through buildErrorBody to satisfy hard rule #12 (no raw err.message).
    */
   private buildServiceUnavailableResponse(message: string): Response {
-    const body = buildErrorBody(503, message);
-    body.error.code = "service_not_running";
+    const body = buildErrorBody(503, message, undefined, { code: "service_not_running" });
     return new Response(JSON.stringify(body), {
       status: 503,
       headers: {

open-sse/handlers/chatCore.ts

@@ -5,7 +5,8 @@ import {
 import { injectMemoryAndSkills } from "./chatCore/memorySkillsInjection.ts";
 import { resolveChatCoreRequestSetup } from "./chatCore/requestSetup.ts";
 import { normalizeOpenAICompatibleTools } from "./chatCore/openAICompatibleTools.ts";
-import { buildFailureUsageRecord } from "./chatCore/failureUsage.ts";
+import { buildFailureUsageRecord, projectFailureUsageErrorCode } from "./chatCore/failureUsage.ts";
+import { createTranslationFailureResult } from "./chatCore/translationFailure.ts";
 import { estimateFinalInputTokens } from "./chatCore/contextEstimation.ts";
 import {
   extractSystemRoleMessages,
@@ -2513,35 +2514,11 @@ export async function handleChatCore({
         : HTTP_STATUS.SERVER_ERROR;
     const message = error?.message || "Invalid request";
     const errorType = typeof error?.errorType === "string" ? error.errorType : null;
-
-    log?.warn?.("TRANSLATE", `Request translation failed: ${message}`);
-
-    if (errorType) {
-      trackPendingRequest(model, provider, connectionId, false);
-      return {
-        success: false,
-        status: statusCode,
-        error: message,
-        response: new Response(
-          JSON.stringify({
-            error: {
-              message,
-              type: errorType,
-              code: errorType,
-            },
-          }),
-          {
-            status: statusCode,
-            headers: {
-              "Content-Type": "application/json",
-            },
-          }
-        ),
-      };
-    }
+    const result = createTranslationFailureResult(statusCode, message, errorType);
+    log?.warn?.("TRANSLATE", `Request translation failed: ${result.error}`);
 
     trackPendingRequest(model, provider, connectionId, false);
-    return createErrorResult(statusCode, message);
+    return result;
   }
 
   // The latest OmniGlyph release has protocol-native OpenAI transforms. Run
@@ -3924,10 +3901,14 @@ export async function handleChatCore({
       streamController.handleError(error);
       return createErrorResult(499, "Request aborted");
     }
-    persistFailureUsage(
-      failureStatus,
-      upstreamErrorCode || (error instanceof Error && error.name ? error.name : "upstream_error")
-    );
+    const persistentErrorCode = projectFailureUsageErrorCode({
+      statusCode: failureStatus,
+      message: failureMessage,
+      errorCode:
+        upstreamErrorCode || (error instanceof Error && error.name ? error.name : "upstream_error"),
+      errorType: upstreamErrorType,
+    });
+    persistFailureUsage(failureStatus, persistentErrorCode);
     console.log(`${COLORS.red}[ERROR] ${failureMessage}${COLORS.reset}`);
     if (stream && upstreamErrorCode) {
       const result = createStreamingErrorResult(
@@ -4253,6 +4234,9 @@ export async function handleChatCore({
         `${decision.kind} (model remaining: ${decision.snapshot.modelRemaining ?? "unknown"}, total remaining: ${decision.snapshot.totalRemaining ?? "unknown"})`
       );
     }
+    // Classifiers and recovery paths above consume the raw provider wording.
+    // Project a separate value only at persistent connection-state boundaries.
+    const persistentMessage = sanitizeErrorMessage(message) || "Provider request failed";
     const errorConnectionId = getCurrentConnectionId();
     if (errorConnectionId && errorType) {
       try {
@@ -4264,7 +4248,7 @@ export async function handleChatCore({
               {
                 testStatus: "banned",
                 isActive: false,
-                lastError: message,
+                lastError: persistentMessage,
                 lastErrorType: errorType,
                 errorCode: String(statusCode),
               },
@@ -4295,7 +4279,7 @@ export async function handleChatCore({
           ) {
             await updateProviderConnection(errorConnectionId, {
               lastErrorType: errorType,
-              lastError: message,
+              lastError: persistentMessage,
               errorCode: statusCode,
             });
             console.warn(
@@ -4308,7 +4292,7 @@ export async function handleChatCore({
               {
                 testStatus: "deactivated",
                 isActive: false,
-                lastError: message,
+                lastError: persistentMessage,
                 lastErrorType: errorType,
                 errorCode: String(statusCode),
               },
@@ -4332,7 +4316,7 @@ export async function handleChatCore({
                 errorConnectionId,
                 {
                   testStatus: "credits_exhausted",
-                  lastError: message,
+                  lastError: persistentMessage,
                   lastErrorType: errorType,
                   errorCode: String(statusCode),
                 },
@@ -4418,7 +4402,7 @@ export async function handleChatCore({
                   rateLimitedUntil: kimiRateLimitResetAt,
                   backoffLevel: 0,
                   lastErrorType: PROVIDER_ERROR_TYPES.RATE_LIMITED,
-                  lastError: message,
+                  lastError: persistentMessage,
                   errorCode: statusCode,
                 });
                 console.warn(
@@ -4447,7 +4431,7 @@ export async function handleChatCore({
                   errorConnectionId,
                   {
                     testStatus: "credits_exhausted",
-                    lastError: message,
+                    lastError: persistentMessage,
                     lastErrorType: errorType,
                     errorCode: String(statusCode),
                   },
@@ -4463,14 +4447,14 @@ export async function handleChatCore({
           // Normal 401 (token/session auth issue): keep account active for refresh/re-auth.
           await updateProviderConnection(errorConnectionId, {
             lastErrorType: errorType,
-            lastError: message,
+            lastError: persistentMessage,
             errorCode: statusCode,
           });
         } else if (errorType === PROVIDER_ERROR_TYPES.OAUTH_INVALID_TOKEN) {
           // OAuth 401 with invalid credentials - token refresh can recover
           await updateProviderConnection(errorConnectionId, {
             lastErrorType: errorType,
-            lastError: message,
+            lastError: persistentMessage,
             errorCode: statusCode,
           });
           console.warn(
@@ -4480,7 +4464,7 @@ export async function handleChatCore({
           // Cloud Code 403 with stale project: not a ban, keep account active.
           await updateProviderConnection(errorConnectionId, {
             lastErrorType: errorType,
-            lastError: message,
+            lastError: persistentMessage,
             errorCode: statusCode,
           });
           console.warn(
@@ -4496,7 +4480,7 @@ export async function handleChatCore({
           const geoCooldownMs = COOLDOWN_MS.geoBlocked ?? 24 * 60 * 60 * 1000;
           await updateProviderConnection(errorConnectionId, {
             lastErrorType: errorType,
-            lastError: message,
+            lastError: persistentMessage,
             errorCode: statusCode,
           });
           // T-PROBE: the 24h exclusion is a routing mutation — a probe must
@@ -4521,7 +4505,7 @@ export async function handleChatCore({
           const byopCooldownMs = COOLDOWN_MS.gcpProjectRequired ?? 24 * 60 * 60 * 1000;
           await updateProviderConnection(errorConnectionId, {
             lastErrorType: errorType,
-            lastError: message,
+            lastError: persistentMessage,
             errorCode: statusCode,
           });
           try {
@@ -5305,9 +5289,12 @@ export async function handleChatCore({
       }).catch(() => {});
       const malformed = describeMalformedNonStream(translatedResponse, malformedTranslatedReason);
       const malformedMessage = `[${provider}/${model}] ${malformed.message}`;
-      const malformedClientBody = buildErrorBody(HTTP_STATUS.BAD_GATEWAY, malformedMessage);
-      malformedClientBody.error.code = malformed.code;
-      malformedClientBody.error.type = malformed.type;
+      const malformedClientBody = buildErrorBody(
+        HTTP_STATUS.BAD_GATEWAY,
+        malformedMessage,
+        undefined,
+        { code: malformed.code, type: malformed.type }
+      );
       persistAttemptLogs({
         status: HTTP_STATUS.BAD_GATEWAY,
         tokens: usage,