diff --git a/docs/security/ERROR_SANITIZATION.md b/docs/security/ERROR_SANITIZATION.md index 461db1a8d8..1b15304c8e 100644 --- a/docs/security/ERROR_SANITIZATION.md +++ b/docs/security/ERROR_SANITIZATION.md @@ -132,6 +132,27 @@ When adding a new route or executor, copy the assertion pattern from this file. - The `pino` redaction config (`src/lib/log/redaction.ts` — if present) handles structured log redaction separately. This doc covers only the response-message surface. - Upstream-header denylist (`src/shared/constants/upstreamHeaders.ts`) covers header leakage — keep both files aligned when adding a new exfiltration concern. +## Upstream details passthrough + +`buildErrorBody` accepts an optional third argument `upstreamDetails` (raw +parsed body from the upstream provider). When provided, it is sanitized by +`sanitizeUpstreamDetails` before inclusion in the response as `upstream_details`. + +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. +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`. + +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. + ## 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()`. diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index a1a49f2b33..7a668b4d0b 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -4124,7 +4124,8 @@ export async function handleChatCore({ errMsg, retryAfterMs, upstreamErrorCode, - upstreamErrorType + upstreamErrorType, + upstreamErrorBody ); } } diff --git a/tests/unit/error-message-sanitization.test.ts b/tests/unit/error-message-sanitization.test.ts index e855d0dde1..2f87c19158 100644 --- a/tests/unit/error-message-sanitization.test.ts +++ b/tests/unit/error-message-sanitization.test.ts @@ -240,6 +240,104 @@ test("buildErrorBody never exposes stack traces in its message", async () => { assert.ok(!body.error.message.includes("at /opt")); }); +// ── sanitizeUpstreamDetails ────────────────────────────────────────────────── + +test("sanitizeUpstreamDetails — basic pass-through for safe fields", async () => { + const { sanitizeUpstreamDetails } = await import("../../open-sse/utils/error.ts"); + const input = { error: { message: "context_length_exceeded", type: "invalid_request_error" } }; + const out = sanitizeUpstreamDetails(input) as any; + assert.equal(out.error.message, "context_length_exceeded"); + assert.equal(out.error.type, "invalid_request_error"); +}); + +test("sanitizeUpstreamDetails — sanitizes string values (absolute path)", async () => { + const { sanitizeUpstreamDetails } = await import("../../open-sse/utils/error.ts"); + const input = { error: { message: "bad input at /srv/app/src/lib/db.ts:42" } }; + const out = sanitizeUpstreamDetails(input) as any; + assert.ok(!out.error.message.includes("/srv/app/src/lib/db.ts"), "absolute path must be stripped"); + assert.ok(out.error.message.includes(""), "path placeholder must be present"); +}); + +test("sanitizeUpstreamDetails — removes blocked keys (stack, apiKey)", async () => { + const { sanitizeUpstreamDetails } = await import("../../open-sse/utils/error.ts"); + const input = { error: { message: "oops" }, stack: "Error\n at foo.ts:1", apiKey: "sk-secret" }; + const out = sanitizeUpstreamDetails(input) as any; + assert.ok(!("stack" in out), "stack key must be removed"); + assert.ok(!("apiKey" in out), "apiKey key must be removed"); + assert.equal(out.error.message, "oops"); +}); + +test("sanitizeUpstreamDetails — depth cap replaces nested value at depth > 4", async () => { + const { sanitizeUpstreamDetails } = await import("../../open-sse/utils/error.ts"); + // Build depth-6 nesting: a.b.c.d.e.f = "leaf" + const input = { a: { b: { c: { d: { e: { f: "leaf" } } } } } }; + const out = sanitizeUpstreamDetails(input) as any; + // depth 0:a, 1:b, 2:c, 3:d, 4:e → e is at depth 4, f would be depth 5 → truncated + assert.equal(out.a.b.c.d.e, "[truncated]"); +}); + +// ── buildErrorBody with upstreamDetails ────────────────────────────────────── + +test("buildErrorBody — without upstream details omits upstream_details field", async () => { + const { buildErrorBody } = await import("../../open-sse/utils/error.ts"); + const body = buildErrorBody(400, "bad request"); + assert.ok(!("upstream_details" in body), "upstream_details must be absent when not provided"); +}); + +test("buildErrorBody — with safe upstream details embeds upstream_details", async () => { + const { buildErrorBody } = await import("../../open-sse/utils/error.ts"); + const body = buildErrorBody(400, "bad request", { error: { message: "context_length_exceeded" } }); + assert.ok("upstream_details" in body, "upstream_details must be present"); + assert.equal((body.upstream_details as any).error.message, "context_length_exceeded"); +}); + +test("buildErrorBody — upstream details with stack key are stripped", async () => { + const { buildErrorBody } = await import("../../open-sse/utils/error.ts"); + const body = buildErrorBody(500, "err", { stack: "Error\n at foo.ts:1", code: "internal" }); + assert.ok("upstream_details" in body, "upstream_details must be present"); + assert.ok(!("stack" in (body.upstream_details as any)), "stack must be stripped from upstream_details"); + assert.equal((body.upstream_details as any).code, "internal"); +}); + +// ── createErrorResult with upstreamDetails ─────────────────────────────────── + +test("createErrorResult — response body includes upstream_details when provided", async () => { + const { createErrorResult } = await import("../../open-sse/utils/error.ts"); + const result = createErrorResult( + 400, + "context too long", + null, + "context_length_exceeded", + "invalid_request_error", + { error: { message: "context_length_exceeded" } } + ); + const body = (await result.response.clone().json()) as any; + assert.ok("upstream_details" in body, "upstream_details must be in response body"); + assert.equal(body.upstream_details.error.message, "context_length_exceeded"); +}); + +test("createErrorResult — response body excludes upstream_details when not provided", async () => { + const { createErrorResult } = await import("../../open-sse/utils/error.ts"); + const result = createErrorResult(400, "bad request", null, "bad_request"); + const body = (await result.response.clone().json()) as any; + assert.ok(!("upstream_details" in body), "upstream_details must be absent when not provided"); +}); + +test("regression: upstream_details never contains stack trace text", async () => { + const { createErrorResult } = await import("../../open-sse/utils/error.ts"); + const upstream = { error: { message: "err" }, stack: "Error\n at /abs/path.ts:1:2" }; + const result = createErrorResult(500, "upstream err", null, undefined, undefined, upstream); + const body = (await result.response.clone().json()) as any; + const serialized = JSON.stringify(body); + assert.ok( + !serialized.includes("at /abs/path.ts"), + "stack trace path must not appear in response body" + ); + assert.ok(!("stack" in (body.upstream_details || {})), "stack key must not be present"); +}); + +// ── existing tests continue ────────────────────────────────────────────────── + test("GET /token-health response never leaks stack frames or absolute paths", async () => { const tokenHealthRoute = await import("../../src/app/api/token-health/route.ts"); const res = await tokenHealthRoute.GET();