diff --git a/open-sse/handlers/search.ts b/open-sse/handlers/search.ts index b1397f3c5d..afc3035872 100644 --- a/open-sse/handlers/search.ts +++ b/open-sse/handlers/search.ts @@ -302,6 +302,15 @@ function parseDomainFilter(domainFilter?: string[]): { return { includes, excludes }; } +/** Read one string setting from a SINGLE source, so callers can distinguish trust. */ +function readProviderSettingString( + source: Record | undefined, + key: string +): string | undefined { + const value = source?.[key]; + return typeof value === "string" && value.trim().length > 0 ? value.trim() : undefined; +} + function getProviderSettingString( params: Pick, key: string @@ -323,21 +332,56 @@ 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(/\/+$/, ""); + // The two override sources are NOT equally trusted, and treating them as one + // was GHSA-3f8g-pfh9-j687. + // + // `providerSpecificData` is the stored provider connection (see the + // `credentials?.providerSpecificData` wiring below) — operator config. It is + // how an operator points at a self-hosted searxng, so loopback/LAN keeps + // working under the block-metadata policy: cloud-metadata endpoints (IMDS + // credential theft) stay rejected (GHSA-j7j4-g9qc-q69c). + const operatorOverride = readProviderSettingString(params.providerSpecificData, "baseUrl"); + if (operatorOverride) { + parseAndValidateNonMetadataUrl(operatorOverride); + return operatorOverride.replace(/\/+$/, ""); } + + // `providerOptions` is `body.provider_options` — straight off the request, so + // tenant input. It is refused outright rather than validated: + // + // - For a keyed provider the builder attaches the OPERATOR's API 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 honoring a caller-chosen host hands the operator's + // third-party key to that host. A block-metadata check does nothing about + // it — the attacker just uses their own public host. + // - For any provider the response body is parsed and returned to the caller, + // so a caller-chosen target is SSRF with readback. + // + // No provider has a legitimate need for a per-request caller-chosen fetch + // target; the self-hosted story is served by the operator field above. If one + // ever does, it needs an explicit per-provider opt-in plus a host allow-list, + // not a validated free-form URL. + const tenantOverride = readProviderSettingString(params.providerOptions, "baseUrl"); + if (tenantOverride) { + throw new SearchBaseUrlOverrideError(config.id); + } + return config.baseUrl.replace(/\/+$/, ""); } +/** 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}". ` + + `The base URL is operator configuration; set it on the provider connection instead.` + ); + this.name = "SearchBaseUrlOverrideError"; + } +} + 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/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..9359e80988 --- /dev/null +++ b/tests/unit/search-baseurl-client-override-3f8g.test.ts @@ -0,0 +1,130 @@ +/** + * 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("refuses a tenant baseUrl for keyless providers too (SSRF readback)", () => { + for (const id of ["searxng-search", "context7", "duckduckgo-free"]) { + const config = SEARCH_PROVIDERS[id]; + if (!config) continue; + for (const target of [ + "https://attacker.example", + "http://127.0.0.1:9999", + "http://10.0.0.5", + ]) { + assert.throws( + () => resolveSearchBaseUrl(config, { ...base, providerOptions: { baseUrl: target } }), + `${id} honored tenant baseUrl ${target}` + ); + } + } + }); + + 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" );