From bd65c6d89ee2862144835613b807f2ee12bc92e1 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:52:59 -0300 Subject: [PATCH] fix(search): refuse a tenant-supplied provider_options.baseUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-3f8g-pfh9-j687: a caller with search scope could set `provider_options.baseUrl` and redirect the SaaS search builders, so the OPERATOR's third-party API key went to a caller-chosen host — `key=`/`api_key=` in the query for google-pse/searchapi, `X-API-Key`/`Authorization` for you.com/linkup/nimble/ollama — and the parsed response came back, making the same primitive an SSRF with readback. My own GHSA-j7j4-g9qc-q69c fix is the one this slipped past. That fix added `parseAndValidateNonMetadataUrl` to this exact resolver, which blocks IMDS and nothing else: against key exfiltration it does nothing at all, because the attacker just names their own public host. I treated a credential-destination problem as an SSRF problem. The j7j4 regression test missed it for a sharper reason worth recording: its fixture was `searxng-search`, the one keyless provider where redirecting the base URL leaks no credential. The test picked the benign case and passed. The two override sources are not equally trusted, and reading them through one `getProviderSettingString()` call erased the distinction: - `providerSpecificData` comes from the stored provider connection (`credentials?.providerSpecificData`) — operator config. This is how an operator points at a self-hosted searxng, so it keeps working on loopback/LAN under block-metadata, and IMDS stays rejected. - `providerOptions` is `body.provider_options` — caller input. Now refused outright rather than validated. No provider needs a per-request caller-chosen fetch target; the self-hosted story is served by the operator field. If one ever does, it needs a per-provider opt-in plus a host allow-list, not a validated free-form URL. Operator config now also wins over a caller value instead of being shadowed by it. Behaviour change, deliberate: a caller that today sends `provider_options.baseUrl` gets an explicit refusal instead of a silently redirected request. Silently ignoring it would leave the caller believing the override applied. tests/unit/search-baseurl-client-override-3f8g.test.ts — 23 tests, red before the fix. It iterates EVERY keyed provider in the registry instead of one fixture, and asserts the registry still has keyed providers so the suite cannot degrade to zero coverage. search-baseurl-ssrf-guard.test.ts had one assertion encoding the wrong contract (caller-supplied loopback allowed). Moved to the operator source with the reasoning inline — the j7j4 metadata cases are untouched. Reported by @skeletonsec. Closes GHSA-3f8g-pfh9-j687 --- open-sse/handlers/search.ts | 66 +++++++-- ...earch-baseurl-client-override-3f8g.test.ts | 130 ++++++++++++++++++ tests/unit/search-baseurl-ssrf-guard.test.ts | 14 +- 3 files changed, 196 insertions(+), 14 deletions(-) create mode 100644 tests/unit/search-baseurl-client-override-3f8g.test.ts 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" );