diff --git a/open-sse/config/searchRegistry.ts b/open-sse/config/searchRegistry.ts index e4d4546212..3e9105ad35 100644 --- a/open-sse/config/searchRegistry.ts +++ b/open-sse/config/searchRegistry.ts @@ -33,6 +33,17 @@ export interface SearchProviderConfig { */ fallbackOnly?: boolean; disabled?: boolean; + /** + * May a CALLER-supplied `provider_options.baseUrl` redirect this provider? + * + * Only ever true for a keyless, self-hosted provider. For anything with + * `authType: "apikey"` the builder attaches the OPERATOR's key to whatever + * host the base URL resolves to, so honoring a caller-chosen value hands that + * key to the caller's server (GHSA-3f8g-pfh9-j687). The invariant + * "never set alongside authType: apikey" is enforced by + * tests/unit/search-baseurl-client-override-3f8g.test.ts. + */ + allowClientBaseUrlOverride?: boolean; } export const SEARCH_PROVIDERS: Record = { @@ -232,6 +243,11 @@ export const SEARCH_PROVIDERS: Record = { timeoutMs: 10_000, cacheTTLMs: 3 * 60 * 1000, fallbackOnly: true, + // Keyless and self-hosted by definition: the caller names their own SearXNG + // instance and no operator credential travels with the request. This is a + // documented flow (tests/unit/search-route.test.ts). Still block-metadata + // guarded, so IMDS stays unreachable. + allowClientBaseUrlOverride: true, }, "ollama-search": { diff --git a/open-sse/handlers/search.ts b/open-sse/handlers/search.ts index b1397f3c5d..e68603e459 100644 --- a/open-sse/handlers/search.ts +++ b/open-sse/handlers/search.ts @@ -20,6 +20,9 @@ import { randomUUID } from "crypto"; * } */ +export { resolveSearchBaseUrl, SearchBaseUrlOverrideError } from "./search/baseUrl.ts"; +import { resolveSearchBaseUrl } from "./search/baseUrl.ts"; + import { getSearchProvider, isUnconfiguredLoopbackSearchProvider, @@ -36,7 +39,6 @@ import * as anysearchSearch from "./search/anysearchSearch.ts"; import { freeWebSearch } from "../services/freeWebSearch.ts"; import { saveCallLog } from "@/lib/usageDb"; import { safeOutboundFetch } from "@/shared/network/safeOutboundFetch"; -import { parseAndValidateNonMetadataUrl } from "@/shared/network/outboundUrlGuard"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { z } from "zod"; @@ -319,25 +321,6 @@ function getProviderSettingString( return undefined; } -export function resolveSearchBaseUrl( - config: SearchProviderConfig, - params: SearchRequestParams -): string { - const override = getProviderSettingString(params, "baseUrl"); - if (override) { - // GHSA-j7j4-g9qc-q69c: the override is client-controlled (provider_options / - // providerSpecificData) and flows into a plain fetch() sink — validate it - // before any builder uses it as the server-side fetch target. Mode is - // block-metadata (NOT public-only): the primary searxng use case is a - // self-hosted instance on loopback/LAN, so private hosts keep working, - // while cloud-metadata endpoints (IMDS credential theft) are rejected. - // The catalog's own config.baseUrl is operator config and stays untouched. - parseAndValidateNonMetadataUrl(override); - return override.replace(/\/+$/, ""); - } - return config.baseUrl.replace(/\/+$/, ""); -} - function toSearchPageNumber(offset: number | undefined, maxResults: number): number | undefined { if (typeof offset !== "number" || offset <= 0 || maxResults <= 0) return undefined; return Math.floor(offset / maxResults) + 1; diff --git a/open-sse/handlers/search/baseUrl.ts b/open-sse/handlers/search/baseUrl.ts new file mode 100644 index 0000000000..ba1e1fe6b0 --- /dev/null +++ b/open-sse/handlers/search/baseUrl.ts @@ -0,0 +1,66 @@ +/** + * Base-URL resolution for /v1/search — the trust decision, on its own. + * + * The two override sources are NOT equally trusted, and reading them through + * one call is what produced GHSA-3f8g-pfh9-j687: + * + * - `providerSpecificData` is the stored provider connection + * (`credentials?.providerSpecificData`) — OPERATOR config. Honored under + * block-metadata, so a self-hosted SearXNG on loopback/LAN keeps working + * while cloud metadata (IMDS credential theft) stays rejected (GHSA-j7j4). + * - `providerOptions` is `body.provider_options` — CALLER input. Honored only + * by a provider that is keyless AND opted in via + * `allowClientBaseUrlOverride`. A keyed builder attaches the OPERATOR's key + * to whatever host this resolves to (`key=`/`api_key=` in the query for + * google-pse/searchapi, `X-API-Key`/`Authorization` for + * you.com/linkup/nimble/ollama), so a caller-chosen host would collect it — + * and a block-metadata check does nothing about that, because the attacker + * simply names their own public host. + * + * Full coverage: tests/unit/search-baseurl-client-override-3f8g.test.ts. + */ + +import { parseAndValidateNonMetadataUrl } from "@/shared/network/outboundUrlGuard"; +import type { SearchProviderConfig } from "../../config/searchRegistry.ts"; + +interface BaseUrlParams { + providerOptions?: Record; + providerSpecificData?: Record; +} + +/** Read one string setting from a SINGLE source, so callers can distinguish trust. */ +function readSetting(source: Record | undefined, key: string): string | undefined { + const value = source?.[key]; + return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; +} + +/** Refusal for a caller-supplied `provider_options.baseUrl` (GHSA-3f8g-pfh9-j687). */ +export class SearchBaseUrlOverrideError extends Error { + readonly code = "SEARCH_BASE_URL_OVERRIDE_REFUSED"; + constructor(providerId: string) { + super( + `provider_options.baseUrl is not accepted for search provider "${providerId}". ` + + `Set the base URL on the provider connection instead.` + ); + this.name = "SearchBaseUrlOverrideError"; + } +} + +export function resolveSearchBaseUrl(config: SearchProviderConfig, params: BaseUrlParams): string { + const operatorOverride = readSetting(params.providerSpecificData, "baseUrl"); + if (operatorOverride) { + parseAndValidateNonMetadataUrl(operatorOverride); + return operatorOverride.replace(/\/+$/, ""); + } + + const callerOverride = readSetting(params.providerOptions, "baseUrl"); + if (callerOverride) { + if (!config.allowClientBaseUrlOverride || config.authType === "apikey") { + throw new SearchBaseUrlOverrideError(config.id); + } + parseAndValidateNonMetadataUrl(callerOverride); + return callerOverride.replace(/\/+$/, ""); + } + + return config.baseUrl.replace(/\/+$/, ""); +} diff --git a/open-sse/utils/error.ts b/open-sse/utils/error.ts index 4fafd05b1a..66563c618f 100644 --- a/open-sse/utils/error.ts +++ b/open-sse/utils/error.ts @@ -40,8 +40,36 @@ function looksLikeAbsolutePath(tok: string): boolean { return (SOURCE_EXT as readonly string[]).includes(ext); } +/** + * Raw credential shapes that carry no `key=` label to key off — the token IS the + * whole match, so the only way to redact them is to recognize the shape. + * + * GHSA-qv45-56jc-4wmj: `upstreamErrorPassthrough.ts` already recognized `sk-` + * and refused verbatim passthrough for bodies containing it, then handed those + * bodies to THIS sanitizer — which had no such pattern, so the key came back to + * the caller anyway. The passthrough file's comment claimed to "mirror the + * vocabulary of redactSensitiveErrorText"; the mirror had drifted. It now + * imports this array instead of keeping a second copy, so the two cannot drift + * again. + * + * Quantifiers are upper-bounded (AGENTS.md → PII learnings §1, ReDoS): these run + * over untrusted upstream error bodies. + */ +export const RAW_CREDENTIAL_PATTERNS: ReadonlyArray = [ + // OpenAI/Anthropic/Stripe-style secret keys: sk-…, sk-ant-…, sk_live_… + /\bsk[-_][A-Za-z0-9._-]{8,200}/g, + // Google API keys + /\bAIza[A-Za-z0-9_-]{20,200}/g, + // JWTs (three base64url segments) + /\beyJ[A-Za-z0-9_-]{8,400}\.[A-Za-z0-9_-]{8,800}\.[A-Za-z0-9_-]{8,800}/g, +]; + export function redactSensitiveErrorText(value: string): string { - return value + let out = value; + for (const pattern of RAW_CREDENTIAL_PATTERNS) { + out = out.replace(pattern, "[REDACTED_CREDENTIAL]"); + } + return out .replace(/data:[^,\s]+;base64,[A-Za-z0-9+/=_-]+/gi, "[REDACTED_DATA_URL]") .replace(/\b(Bearer|Basic)\s+[A-Za-z0-9._~+/=-]+/gi, "$1 [REDACTED]") .replace( diff --git a/open-sse/utils/upstreamErrorPassthrough.ts b/open-sse/utils/upstreamErrorPassthrough.ts index 21d0c6c964..b62fff2adf 100644 --- a/open-sse/utils/upstreamErrorPassthrough.ts +++ b/open-sse/utils/upstreamErrorPassthrough.ts @@ -1,3 +1,4 @@ +import { RAW_CREDENTIAL_PATTERNS } from "./error.ts"; /** * Selective upstream 4xx error passthrough (Claude Code auto-recover contract). * @@ -24,8 +25,23 @@ const INTERNAL_LEAK_RE = /\sat\s\/|node_modules|omniroute\//i; // caller fall back to the sanitized buildErrorBody path. Bodies without a // secret (the overwhelming majority, carrying capability/quota wording) still // relay verbatim. Mirrors the vocabulary of redactSensitiveErrorText in error.ts. -const CREDENTIAL_LEAK_RE = - /\b(?:Bearer|Basic)\s+[A-Za-z0-9._~+/=-]{8,}|\bsk-[A-Za-z0-9._-]{8,}|(?:api[_-]?key|access[_-]?token|refresh[_-]?token|authorization|cookie|secret)\\?["']?\s*[:=]\s*\\?["']?[^"'\\,\s}]{6,}/i; +const LABELLED_CREDENTIAL_RE = + /\b(?:Bearer|Basic)\s+[A-Za-z0-9._~+/=-]{8,}|(?:api[_-]?key|access[_-]?token|refresh[_-]?token|authorization|cookie|secret)\\?["']?\s*[:=]\s*\\?["']?[^"'\\,\s}]{6,}/i; + +/** + * The raw-token shapes (sk-…, AIza…, JWT) come from error.ts's + * RAW_CREDENTIAL_PATTERNS rather than a second local copy. The previous local + * copy carried `sk-` while the sanitizer this file falls back to did NOT, so a + * body recognized as leaky here was returned unredacted there + * (GHSA-qv45-56jc-4wmj). One source, no drift. + */ +function containsCredential(text: string): boolean { + if (LABELLED_CREDENTIAL_RE.test(text)) return true; + return RAW_CREDENTIAL_PATTERNS.some((pattern) => { + pattern.lastIndex = 0; // the shared patterns are /g — reset before .test() + return pattern.test(text); + }); +} export function shouldPassthroughUpstreamError(statusCode: number, upstreamBody: unknown): boolean { if (statusCode < PASSTHROUGH_MIN || statusCode > PASSTHROUGH_MAX) return false; @@ -34,7 +50,7 @@ export function shouldPassthroughUpstreamError(statusCode: number, upstreamBody: const text = JSON.stringify(upstreamBody); if (INTERNAL_LEAK_RE.test(text)) return false; // Refuse passthrough when the provider echoed a credential back to us. - if (CREDENTIAL_LEAK_RE.test(text)) return false; + if (containsCredential(text)) return false; return true; } diff --git a/open-sse/utils/upstreamResponseHeaders.ts b/open-sse/utils/upstreamResponseHeaders.ts index 834d354f18..9f6c16d1e6 100644 --- a/open-sse/utils/upstreamResponseHeaders.ts +++ b/open-sse/utils/upstreamResponseHeaders.ts @@ -45,3 +45,36 @@ export function filterUpstreamResponseHeaderEntries( } export const STRIP_UPSTREAM_HEADER_NAMES: ReadonlySet = STRIP_HEADER_NAMES; + +/** + * Response headers that must never be relayed back to a client. + * + * A relay sends its own credential upstream (the bifrost route sends + * `Authorization: Bearer ${BIFROST_API_KEY}` to the sidecar). If that upstream + * echoes the header back — or sets its own session cookie — copying the response + * headers wholesale hands it to whoever holds the relay token + * (GHSA-9m72-44hg-w32g). `set-cookie` matters as much as `authorization`: it is + * a session, and the browser would store it against OUR origin. + */ +const SENSITIVE_RESPONSE_HEADER_NAMES: ReadonlyArray = [ + "authorization", + "proxy-authorization", + "x-api-key", + "x-goog-api-key", + "api-key", + "cookie", + "set-cookie", +]; + +/** + * New Headers with the stale framing set AND any echoed credential/session + * header removed. Use this instead of `new Headers(upstream.headers)` on every + * path that relays an upstream response to a client. Does not mutate the input. + */ +export function stripSensitiveResponseHeaders(input: Headers): Headers { + return new Headers( + filterUpstreamResponseHeaderEntries(input.entries(), SENSITIVE_RESPONSE_HEADER_NAMES) + ); +} + +export { SENSITIVE_RESPONSE_HEADER_NAMES }; diff --git a/src/app/api/v1/relay/chat/completions/bifrost/route.ts b/src/app/api/v1/relay/chat/completions/bifrost/route.ts index 31b007959f..8674125c38 100644 --- a/src/app/api/v1/relay/chat/completions/bifrost/route.ts +++ b/src/app/api/v1/relay/chat/completions/bifrost/route.ts @@ -29,9 +29,14 @@ */ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; +import { stripSensitiveResponseHeaders } from "@omniroute/open-sse/utils/upstreamResponseHeaders"; import { createInjectionGuard } from "@/middleware/promptInjectionGuard"; import { getRelayTokenByHash, checkRateLimit, recordRelayUsage } from "@/lib/db/relayProxies"; -import { buildErrorBody } from "@omniroute/open-sse/utils/error"; +import { + buildErrorBody, + parseUpstreamError, + sanitizeErrorMessage, +} from "@omniroute/open-sse/utils/error"; import { getProviderPluginManifestHeader } from "@omniroute/open-sse/config/providerPluginManifestUrl.ts"; import { z } from "zod"; import { @@ -299,7 +304,11 @@ export async function POST(request: Request) { }); }; - const newHeaders = new Headers(upstream.headers); + // Never copy the upstream headers wholesale: the sidecar receives our + // `Authorization: Bearer ${BIFROST_API_KEY}` and anything it echoes back — + // that header, its own set-cookie — would reach the relay-token holder + // (GHSA-9m72-44hg-w32g). + const newHeaders = stripSensitiveResponseHeaders(upstream.headers); newHeaders.set("X-Routed-By", "bifrost"); newHeaders.set("X-Relay-Token", token.tokenPrefix + "..."); if (!wantsStream) { @@ -321,6 +330,28 @@ export async function POST(request: Request) { } clearTimeout(tid); + + // Normalize non-2xx through parseUpstreamError + buildErrorBody instead of + // relaying the sidecar body verbatim — parity with the TS sibling route and + // Hard Rule #12 (GHSA-9m72-44hg-w32g). + if (!upstream.ok) { + const parsed = await parseUpstreamError(upstream, null); + const errorBody = buildErrorBody( + parsed.statusCode, + sanitizeErrorMessage(parsed.message), + parsed.responseBody + ); + newHeaders.set("Content-Type", "application/json"); + if (parsed.retryAfterMs && parsed.retryAfterMs > 0) { + newHeaders.set("Retry-After", String(Math.ceil(parsed.retryAfterMs / 1000))); + } + recordUsage("error", parsed.statusCode); + return new Response(JSON.stringify(errorBody), { + status: parsed.statusCode, + headers: newHeaders, + }); + } + recordUsage(upstream.status < 500 ? "success" : "error", upstream.status); return new Response(upstream.body, { diff --git a/src/app/api/v1/relay/chat/completions/route.ts b/src/app/api/v1/relay/chat/completions/route.ts index fa40d4a009..4e43bb7a13 100644 --- a/src/app/api/v1/relay/chat/completions/route.ts +++ b/src/app/api/v1/relay/chat/completions/route.ts @@ -7,6 +7,7 @@ */ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; +import { stripSensitiveResponseHeaders } from "@omniroute/open-sse/utils/upstreamResponseHeaders"; import { handleChat } from "@/sse/handlers/chat"; import { withChatAdmission } from "@/shared/middleware/withChatAdmission"; import { createInjectionGuard } from "@/middleware/promptInjectionGuard"; @@ -109,7 +110,9 @@ async function forwardToBifrost( signal: ac.signal, }); - const headers = new Headers(upstream.headers); + // Same strip as the bifrost sibling: an echoed upstream credential or + // set-cookie must not reach the relay-token holder (GHSA-9m72-44hg-w32g). + const headers = stripSensitiveResponseHeaders(upstream.headers); headers.set("X-Routed-By", "bifrost"); headers.set("X-Routing-Backend", "bifrost"); headers.set("X-Relay-Token", token.tokenPrefix + "..."); diff --git a/stryker.conf.json b/stryker.conf.json index 96f421f624..27e2825662 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -244,6 +244,7 @@ "tests/unit/embeddings-auth.test.ts", "tests/unit/error-classification.test.ts", "tests/unit/error-message-sanitization.test.ts", + "tests/unit/error-sanitizer-sk-key-qv45.test.ts", "tests/unit/error-sensitive-redaction.test.ts", "tests/unit/execute-chat-resource-pressure-breaker.test.ts", "tests/unit/executor-antigravity.test.ts", diff --git a/tests/unit/bifrost-relay-response-leak-9m72.test.ts b/tests/unit/bifrost-relay-response-leak-9m72.test.ts new file mode 100644 index 0000000000..c5affaf655 --- /dev/null +++ b/tests/unit/bifrost-relay-response-leak-9m72.test.ts @@ -0,0 +1,103 @@ +/** + * GHSA-9m72-44hg-w32g — the standalone bifrost relay route copied ALL upstream + * response headers (`new Headers(upstream.headers)`) and returned non-2xx bodies + * verbatim, while its TypeScript sibling routed non-2xx through + * parseUpstreamError + buildErrorBody + stripStaleEncodingHeaders. + * + * The relay sends `Authorization: Bearer ${BIFROST_API_KEY}` to the sidecar, so + * anything the sidecar (or a further upstream) echoes back — that header, its own + * `set-cookie`, an `x-api-key` — reached the relay-token holder untouched. + * + * Both relay routes copy upstream headers, so the strip is a shared helper used + * by both rather than a fix in one and a second copy waiting to drift (the + * failure mode of GHSA-v7g9 and GHSA-qv45). + * + * Run with: + * node --import tsx/esm --test tests/unit/bifrost-relay-response-leak-9m72.test.ts + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +import { stripSensitiveResponseHeaders } from "../../open-sse/utils/upstreamResponseHeaders.ts"; + +const read = (rel: string) => + readFileSync(fileURLToPath(new URL(`../../${rel}`, import.meta.url)), "utf8"); + +const BIFROST_ROUTE = "src/app/api/v1/relay/chat/completions/bifrost/route.ts"; +const TS_ROUTE = "src/app/api/v1/relay/chat/completions/route.ts"; + +describe("stripSensitiveResponseHeaders", () => { + it("drops credentials and cookies the upstream echoed back", () => { + const upstream = new Headers([ + ["authorization", "Bearer sidecar-secret"], + ["x-api-key", "op-key"], + ["x-goog-api-key", "goog-key"], + ["api-key", "azure-key"], + ["cookie", "session=abc"], + ["set-cookie", "session=abc; HttpOnly"], + ["proxy-authorization", "Basic zzz"], + ["content-type", "application/json"], + ["x-request-id", "keep-me"], + ]); + const out = stripSensitiveResponseHeaders(upstream); + for (const gone of [ + "authorization", + "x-api-key", + "x-goog-api-key", + "api-key", + "cookie", + "set-cookie", + "proxy-authorization", + ]) { + assert.equal(out.get(gone), null, `${gone} survived`); + } + assert.equal(out.get("content-type"), "application/json"); + assert.equal(out.get("x-request-id"), "keep-me"); + }); + + it("also drops the stale framing headers", () => { + const out = stripSensitiveResponseHeaders( + new Headers([ + ["content-encoding", "gzip"], + ["content-length", "123"], + ["transfer-encoding", "chunked"], + ["x-keep", "yes"], + ]) + ); + assert.equal(out.get("content-encoding"), null); + assert.equal(out.get("content-length"), null); + assert.equal(out.get("transfer-encoding"), null); + assert.equal(out.get("x-keep"), "yes"); + }); + + it("does not mutate the input Headers", () => { + const input = new Headers([["authorization", "Bearer x"]]); + stripSensitiveResponseHeaders(input); + assert.equal(input.get("authorization"), "Bearer x"); + }); +}); + +describe("both relay routes use the shared strip (GHSA-9m72-44hg-w32g)", () => { + for (const route of [BIFROST_ROUTE, TS_ROUTE]) { + it(`${route} strips sensitive upstream response headers`, () => { + const src = read(route); + assert.ok( + src.includes("stripSensitiveResponseHeaders"), + `${route} relays upstream headers verbatim — a sidecar-echoed credential reaches the caller` + ); + assert.ok( + !/new Headers\(upstream\.headers\)/.test(src), + `${route} still copies upstream headers wholesale` + ); + }); + } + + it("the bifrost route normalizes non-2xx through the error sanitizer", () => { + const src = read(BIFROST_ROUTE); + assert.ok(src.includes("buildErrorBody"), "bifrost non-2xx must not be relayed verbatim"); + assert.ok(src.includes("sanitizeErrorMessage"), "bifrost non-2xx must be sanitized (HR#12)"); + }); +}); diff --git a/tests/unit/error-sanitizer-sk-key-qv45.test.ts b/tests/unit/error-sanitizer-sk-key-qv45.test.ts new file mode 100644 index 0000000000..b92d5ebbb1 --- /dev/null +++ b/tests/unit/error-sanitizer-sk-key-qv45.test.ts @@ -0,0 +1,93 @@ +/** + * GHSA-qv45-56jc-4wmj — two copies of one redaction rule, and only one got the + * `sk-` pattern. + * + * `upstreamErrorPassthrough.ts`'s CREDENTIAL_LEAK_RE matches `\bsk-[…]{8,}` and + * REFUSES verbatim passthrough when an upstream 4xx echoes a key — correctly + * treating it as a leak. The body then falls through to `buildErrorBody` → + * `sanitizeErrorMessage` → `redactSensitiveErrorText`, which had no `sk-` + * pattern at all. So the layer that recognized the credential handed it to a + * layer that did not, and OpenAI-style `Incorrect API key provided: sk-proj-…` + * bodies were returned to the caller verbatim. + * + * The passthrough file's own comment says it "mirrors the vocabulary of + * redactSensitiveErrorText" — the mirror had diverged. This suite pins both + * directions so it cannot diverge again. + * + * Run with: + * node --import tsx/esm --test tests/unit/error-sanitizer-sk-key-qv45.test.ts + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; + +import { redactSensitiveErrorText, sanitizeErrorMessage } from "../../open-sse/utils/error.ts"; + +const LEAKY_BODIES = [ + "Incorrect API key provided: sk-proj-AbCdEfGhIjKlMnOpQrStUv. You can find your API key at …", + "401 Unauthorized: sk-ant-api03-abcdefghijklmnopqrstuvwxyz-1234567890", + "invalid key sk_live_51H8xKzAbCdEfGhIjKlMn", + "Bad credentials for AIzaSyA1B2C3D4E5F6G7H8I9J0KaLbMcNdOeP", + "token rejected: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.dozjgNryP4J3jVmNHl0w5N_XgL0n3I9PlFUP0THsR8U", +]; + +describe("redactSensitiveErrorText — raw credential patterns (GHSA-qv45-56jc-4wmj)", () => { + for (const body of LEAKY_BODIES) { + it(`redacts the raw credential in: ${body.slice(0, 42)}…`, () => { + const out = redactSensitiveErrorText(body); + assert.ok(!/\bsk[-_][A-Za-z0-9._-]{8,}/.test(out), `sk- survived: ${out}`); + assert.ok(!/\bAIza[A-Za-z0-9_-]{20,}/.test(out), `Google key survived: ${out}`); + assert.ok(!/\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\./.test(out), `JWT survived: ${out}`); + assert.ok(out.includes("[REDACTED"), `nothing was redacted: ${out}`); + }); + } + + it("redacts through sanitizeErrorMessage, the path buildErrorBody actually uses", () => { + const out = sanitizeErrorMessage("Incorrect API key provided: sk-proj-AbCdEfGhIjKlMnOpQr."); + assert.ok(!out.includes("sk-proj-AbCdEfGhIjKlMnOpQr"), out); + }); + + it("keeps the pre-existing redactions working", () => { + assert.match(redactSensitiveErrorText("401: Bearer abc123def456"), /Bearer \[REDACTED\]/); + assert.match( + redactSensitiveErrorText('{"api_key":"secret-value","detail":"bad"}'), + /\[REDACTED\]/ + ); + assert.match( + redactSensitiveErrorText("data:image/png;base64,AAAABBBBCCCC"), + /\[REDACTED_DATA_URL\]/ + ); + }); + + it("does not maul ordinary error prose that merely contains 'sk'", () => { + for (const benign of [ + "Model gpt-5 is not available on this plan", + "risk score too high", + "task sk failed", // short, no credential shape + "Rate limit reached for requests", + ]) { + assert.equal(redactSensitiveErrorText(benign), benign); + } + }); +}); + +describe("the two redaction layers stay in step", () => { + it("every credential shape the passthrough layer refuses is also redacted here", async () => { + // If passthrough REFUSES a body as leaky, the fallback sanitizer is the only + // thing standing between that body and the caller. Anything the first layer + // calls a credential, the second must scrub. + const { shouldPassthroughUpstreamError } = + await import("../../open-sse/utils/upstreamErrorPassthrough.ts"); + for (const body of LEAKY_BODIES) { + const payload = { error: { message: body } }; + const relayedVerbatim = shouldPassthroughUpstreamError(401, payload); + if (relayedVerbatim) continue; // not classified as a leak — nothing to assert + const scrubbed = redactSensitiveErrorText(body); + assert.notEqual( + scrubbed, + body, + `passthrough refused this body as leaky but the sanitizer left it untouched: ${body}` + ); + } + }); +}); diff --git a/tests/unit/search-baseurl-client-override-3f8g.test.ts b/tests/unit/search-baseurl-client-override-3f8g.test.ts new file mode 100644 index 0000000000..17ada75dce --- /dev/null +++ b/tests/unit/search-baseurl-client-override-3f8g.test.ts @@ -0,0 +1,159 @@ +/** + * GHSA-3f8g-pfh9-j687 — a tenant-supplied `provider_options.baseUrl` redirected + * the SaaS search builders, so the OPERATOR's search-provider API key was sent + * to a tenant-chosen host (query string for google-pse/searchapi, header for + * you.com/linkup/nimble/ollama). + * + * This is the same override the GHSA-j7j4-g9qc-q69c fix hardened — and that fix + * only added a block-metadata check, which does nothing here: the attacker + * points at their own PUBLIC host and collects the key. The j7j4 regression test + * missed it because its fixture was `searxng-search`, the one keyless provider + * where redirecting the base URL leaks no credential. + * + * The two override sources have different trust: + * - `providerSpecificData` comes from the stored provider connection + * (`credentials?.providerSpecificData`, search.ts:1510) — OPERATOR config. + * This is how an operator points at their self-hosted searxng, so it keeps + * working on loopback/LAN under the block-metadata policy. + * - `providerOptions` comes straight off the request body + * (`body.provider_options`, v1/search/route.ts:366) — TENANT input. It is + * refused outright: no provider needs a per-request caller-chosen fetch + * target, and honoring one is credential exfiltration for keyed providers + * and SSRF-with-readback for every provider. + * + * Run with: + * node --import tsx/esm --test tests/unit/search-baseurl-client-override-3f8g.test.ts + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; + +import { resolveSearchBaseUrl } from "../../open-sse/handlers/search.ts"; +import type { SearchProviderConfig } from "../../open-sse/config/searchRegistry.ts"; +import { SEARCH_PROVIDERS } from "../../open-sse/config/searchRegistry.ts"; + +const base = { query: "test", searchType: "web", maxResults: 5 }; + +/** Every provider whose builder attaches an operator-held key. */ +const KEYED_PROVIDER_IDS = Object.values(SEARCH_PROVIDERS) + .filter((p) => p.authType === "apikey") + .map((p) => p.id); + +describe("resolveSearchBaseUrl — tenant-supplied baseUrl (GHSA-3f8g-pfh9-j687)", () => { + it("covers every keyed provider in the registry, not one hand-picked fixture", () => { + // The j7j4 test only exercised searxng. Assert the registry actually has + // keyed providers so this suite cannot silently degrade to zero coverage. + assert.ok( + KEYED_PROVIDER_IDS.length >= 5, + `expected keyed providers, got ${KEYED_PROVIDER_IDS.length}` + ); + }); + + for (const id of KEYED_PROVIDER_IDS) { + it(`refuses a tenant baseUrl for the keyed provider ${id}`, () => { + const config = SEARCH_PROVIDERS[id]; + assert.throws( + () => + resolveSearchBaseUrl(config, { + ...base, + providerOptions: { baseUrl: "https://attacker.example" }, + }), + `${id} honored a tenant baseUrl — the operator key would be sent there` + ); + }); + } + + it("the opt-in flag is NEVER set on a provider that carries an operator key", () => { + // The whole invariant in one assertion: if this ever pairs with authType + // "apikey", a caller can redirect the operator's key again. + for (const p of Object.values(SEARCH_PROVIDERS)) { + if (p.allowClientBaseUrlOverride) { + assert.equal(p.authType, "none", `${p.id} opts into a caller baseUrl while holding a key`); + } + } + }); + + it("refuses a caller baseUrl for keyless providers that did NOT opt in", () => { + for (const id of ["context7", "duckduckgo-free"]) { + const config = SEARCH_PROVIDERS[id]; + if (!config || config.allowClientBaseUrlOverride) continue; + assert.throws( + () => + resolveSearchBaseUrl(config, { + ...base, + providerOptions: { baseUrl: "https://attacker.example" }, + }), + `${id} honored a caller baseUrl without opting in` + ); + } + }); + + it("SearXNG keeps its documented self-hosted caller override, IMDS still blocked", () => { + // Opted in: keyless, so no operator credential travels with the request. + // Loopback/LAN is the point of a self-hosted instance (search-route.test.ts + // covers the route-level flow); cloud metadata stays rejected. + const config = SEARCH_PROVIDERS["searxng-search"]; + assert.equal(config.allowClientBaseUrlOverride, true); + assert.equal( + resolveSearchBaseUrl(config, { + ...base, + providerOptions: { baseUrl: "http://127.0.0.1:8888/search" }, + }), + "http://127.0.0.1:8888/search" + ); + assert.throws(() => + resolveSearchBaseUrl(config, { + ...base, + providerOptions: { baseUrl: "http://169.254.169.254/latest/meta-data/" }, + }) + ); + }); + + it("keeps the operator-configured override working, including loopback/LAN", () => { + const config = SEARCH_PROVIDERS["searxng-search"]; + assert.equal( + resolveSearchBaseUrl(config, { + ...base, + providerSpecificData: { baseUrl: "http://127.0.0.1:8888/search" }, + }), + "http://127.0.0.1:8888/search" + ); + assert.equal( + resolveSearchBaseUrl(config, { + ...base, + providerSpecificData: { baseUrl: "http://10.0.0.5:8888" }, + }), + "http://10.0.0.5:8888" + ); + }); + + it("still blocks cloud metadata from the operator source (j7j4 must not regress)", () => { + const config = SEARCH_PROVIDERS["searxng-search"]; + assert.throws(() => + resolveSearchBaseUrl(config, { + ...base, + providerSpecificData: { baseUrl: "http://169.254.169.254/latest/meta-data/" }, + }) + ); + }); + + it("the operator source wins over a tenant one instead of the tenant shadowing it", () => { + const config = SEARCH_PROVIDERS["searxng-search"]; + assert.equal( + resolveSearchBaseUrl(config, { + ...base, + providerOptions: { baseUrl: "https://attacker.example" }, + providerSpecificData: { baseUrl: "http://127.0.0.1:8888" }, + }), + "http://127.0.0.1:8888" + ); + }); + + it("falls back to the catalog baseUrl when neither source supplies one", () => { + const config: SearchProviderConfig = { + ...SEARCH_PROVIDERS["searxng-search"], + baseUrl: "http://localhost:8888/search", + }; + assert.equal(resolveSearchBaseUrl(config, base), "http://localhost:8888/search"); + }); +}); diff --git a/tests/unit/search-baseurl-ssrf-guard.test.ts b/tests/unit/search-baseurl-ssrf-guard.test.ts index f42335edd3..5e4e1a6217 100644 --- a/tests/unit/search-baseurl-ssrf-guard.test.ts +++ b/tests/unit/search-baseurl-ssrf-guard.test.ts @@ -58,18 +58,26 @@ describe("resolveSearchBaseUrl — SSRF guard on client-controlled baseUrl (GHSA }); } - it("still allows a self-hosted loopback/LAN override (block-metadata, not public-only)", () => { + // CORRECTED for GHSA-3f8g-pfh9-j687. This case originally asserted the + // loopback/LAN override via `providerOptions` — i.e. via TENANT input — which + // is the SSRF-with-readback half of that advisory. The self-hosted use case + // this test was written to protect is operator configuration, and it arrives + // on `providerSpecificData` (search.ts reads it from the stored connection's + // `credentials?.providerSpecificData`), so that is where it is asserted now. + // Tenant-supplied overrides are refused outright — see + // search-baseurl-client-override-3f8g.test.ts. + it("still allows a self-hosted loopback/LAN override from OPERATOR config", () => { assert.equal( resolveSearchBaseUrl(config, { ...base, - providerOptions: { baseUrl: "http://127.0.0.1:9999" }, + providerSpecificData: { baseUrl: "http://127.0.0.1:9999" }, }), "http://127.0.0.1:9999" ); assert.equal( resolveSearchBaseUrl(config, { ...base, - providerOptions: { baseUrl: "http://10.0.0.5:8080" }, + providerSpecificData: { baseUrl: "http://10.0.0.5:8080" }, }), "http://10.0.0.5:8080" );