From 56a533ee400fc3ded7312c067f5d6def3b251869 Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Tue, 17 Mar 2026 14:04:00 -0400 Subject: [PATCH 1/2] fix(ssrf): honor empty URL allowlists as deny-all Preserve explicit empty URL allowlists in gateway input config so `urlAllowlist: []` fails closed instead of behaving like an unset policy. Add regression coverage for the low-level SSRF guard and the OpenAI/OpenResponses HTTP entrypoints to lock in the deny-all semantics for explicit empty allowlists. --- src/gateway/input-allowlist.test.ts | 11 +++-- src/gateway/input-allowlist.ts | 12 +++-- src/gateway/openai-http.test.ts | 66 ++++++++++++++++++++++++++ src/gateway/openresponses-http.test.ts | 51 ++++++++++++++++++++ src/infra/net/fetch-guard.ssrf.test.ts | 12 +++++ src/infra/net/ssrf.pinning.test.ts | 27 +++++++++++ src/infra/net/ssrf.ts | 15 ++++-- 7 files changed, 181 insertions(+), 13 deletions(-) diff --git a/src/gateway/input-allowlist.test.ts b/src/gateway/input-allowlist.test.ts index 169e8ac03e2..5381107b4d9 100644 --- a/src/gateway/input-allowlist.test.ts +++ b/src/gateway/input-allowlist.test.ts @@ -2,13 +2,16 @@ import { describe, expect, it } from "vitest"; import { normalizeInputHostnameAllowlist } from "./input-allowlist.js"; describe("normalizeInputHostnameAllowlist", () => { - it("treats missing and empty allowlists as unset", () => { + it("treats a missing allowlist as unset", () => { expect(normalizeInputHostnameAllowlist(undefined)).toBeUndefined(); - expect(normalizeInputHostnameAllowlist([])).toBeUndefined(); }); - it("drops whitespace-only entries and treats the result as unset", () => { - expect(normalizeInputHostnameAllowlist(["", " "])).toBeUndefined(); + it("preserves an explicit empty allowlist as deny-all", () => { + expect(normalizeInputHostnameAllowlist([])).toEqual([]); + }); + + it("fails closed when configured entries trim down to nothing", () => { + expect(normalizeInputHostnameAllowlist(["", " "])).toEqual([]); }); it("preserves trimmed hostname patterns", () => { diff --git a/src/gateway/input-allowlist.ts b/src/gateway/input-allowlist.ts index 61ad9d06cc4..a47d23c3453 100644 --- a/src/gateway/input-allowlist.ts +++ b/src/gateway/input-allowlist.ts @@ -2,15 +2,17 @@ * Normalize optional gateway URL-input hostname allowlists. * * Semantics are intentionally: - * - missing / empty / whitespace-only list => no hostname allowlist restriction - * - deny-all URL fetching => use the corresponding `allowUrl: false` switch + * - missing list => no hostname allowlist restriction + * - explicit empty / fully trimmed-empty list => deny all URL fetches */ export function normalizeInputHostnameAllowlist( values: string[] | undefined, ): string[] | undefined { - if (!values || values.length === 0) { + if (values === undefined) { return undefined; } - const normalized = values.map((value) => value.trim()).filter((value) => value.length > 0); - return normalized.length > 0 ? normalized : undefined; + if (values.length === 0) { + return []; + } + return values.map((value) => value.trim()).filter((value) => value.length > 0); } diff --git a/src/gateway/openai-http.test.ts b/src/gateway/openai-http.test.ts index 82130807a1b..6b5ea18ee2f 100644 --- a/src/gateway/openai-http.test.ts +++ b/src/gateway/openai-http.test.ts @@ -1,3 +1,5 @@ +import fs from "node:fs/promises"; +import path from "node:path"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { HISTORY_CONTEXT_MARKER } from "../auto-reply/reply/history.js"; import { CURRENT_MESSAGE_MARKER } from "../auto-reply/reply/mentions.js"; @@ -45,6 +47,15 @@ async function startServer(port: number, opts?: { openAiChatCompletionsEnabled?: }); } +async function writeGatewayConfig(config: Record) { + const configPath = process.env.OPENCLAW_CONFIG_PATH; + if (!configPath) { + throw new Error("OPENCLAW_CONFIG_PATH is required for gateway config tests"); + } + await fs.mkdir(path.dirname(configPath), { recursive: true }); + await fs.writeFile(configPath, JSON.stringify(config, null, 2), "utf-8"); +} + async function postChatCompletions(port: number, body: unknown, headers?: Record) { const res = await fetch(`http://127.0.0.1:${port}/v1/chat/completions`, { method: "POST", @@ -636,6 +647,61 @@ describe("OpenAI-compatible HTTP API (e2e)", () => { ); }); + it("treats an explicit empty image URL allowlist as deny-all", async () => { + const configPath = process.env.OPENCLAW_CONFIG_PATH; + if (!configPath) { + throw new Error("OPENCLAW_CONFIG_PATH is required for gateway config tests"); + } + const previousConfig = await fs.readFile(configPath, "utf-8").catch(() => undefined); + const denyAllConfig = { + gateway: { + http: { + endpoints: { + chatCompletions: { + enabled: true, + images: { + allowUrl: true, + urlAllowlist: [], + }, + }, + }, + }, + }, + }; + + await writeGatewayConfig(denyAllConfig); + + const port = await getFreePort(); + const server = await startServer(port); + try { + agentCommand.mockClear(); + const res = await postChatCompletions(port, { + model: "openclaw", + messages: [ + { + role: "user", + content: [ + { type: "text", text: "look at this" }, + { type: "image_url", image_url: { url: "https://example.com/image.png" } }, + ], + }, + ], + }); + expect(res.status).toBe(400); + const json = (await res.json()) as { error?: { type?: string; message?: string } }; + expect(json.error?.type).toBe("invalid_request_error"); + expect(json.error?.message ?? "").toMatch(/invalid image_url content|allowlist|blocked/i); + expect(agentCommand).not.toHaveBeenCalled(); + } finally { + await server.close({ reason: "openai empty allowlist hardening test done" }); + if (previousConfig === undefined) { + await fs.rm(configPath, { force: true }); + } else { + await fs.writeFile(configPath, previousConfig, "utf-8"); + } + } + }); + it("streams SSE chunks when stream=true", async () => { const port = enabledPort; try { diff --git a/src/gateway/openresponses-http.test.ts b/src/gateway/openresponses-http.test.ts index 3f6cb43917d..1dab33a3f57 100644 --- a/src/gateway/openresponses-http.test.ts +++ b/src/gateway/openresponses-http.test.ts @@ -636,6 +636,57 @@ describe("OpenResponses HTTP API (e2e)", () => { await allowlistServer.close({ reason: "responses allowlist hardening test done" }); } + const denyAllConfig = { + gateway: { + http: { + endpoints: { + responses: { + enabled: true, + maxUrlParts: 1, + files: { + allowUrl: true, + urlAllowlist: [], + }, + images: { + allowUrl: true, + urlAllowlist: [], + }, + }, + }, + }, + }, + }; + await writeGatewayConfig(denyAllConfig); + + const denyAllPort = await getFreePort(); + const denyAllServer = await startServer(denyAllPort, { openResponsesEnabled: true }); + try { + agentCommand.mockClear(); + + const fileBlocked = await postResponses(denyAllPort, { + model: "openclaw", + input: buildUrlInputMessage({ + kind: "input_file", + text: "fetch this", + url: "https://cdn.example.com/file.txt", + }), + }); + await expectInvalidRequest(fileBlocked, /invalid request|allowlist|blocked/i); + + const imageBlocked = await postResponses(denyAllPort, { + model: "openclaw", + input: buildUrlInputMessage({ + kind: "input_image", + text: "fetch this", + url: "https://images.example.com/image.png", + }), + }); + await expectInvalidRequest(imageBlocked, /invalid request|allowlist|blocked/i); + expect(agentCommand).not.toHaveBeenCalled(); + } finally { + await denyAllServer.close({ reason: "responses empty allowlist hardening test done" }); + } + const capConfig = buildResponsesUrlPolicyConfig(0); await writeGatewayConfig(capConfig); diff --git a/src/infra/net/fetch-guard.ssrf.test.ts b/src/infra/net/fetch-guard.ssrf.test.ts index f90df5271f1..082907beffd 100644 --- a/src/infra/net/fetch-guard.ssrf.test.ts +++ b/src/infra/net/fetch-guard.ssrf.test.ts @@ -144,6 +144,18 @@ describe("fetchWithSsrFGuard hardening", () => { expect(fetchImpl).toHaveBeenCalledTimes(1); }); + it("blocks all fetches when hostnameAllowlist is an explicit empty array", async () => { + const fetchImpl = vi.fn(); + await expect( + fetchWithSsrFGuard({ + url: "https://example.com/file.txt", + fetchImpl, + policy: { hostnameAllowlist: [] }, + }), + ).rejects.toThrow(/allowlist/i); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + it("enforces hostname allowlist policies", async () => { const fetchImpl = vi.fn(); await expect( diff --git a/src/infra/net/ssrf.pinning.test.ts b/src/infra/net/ssrf.pinning.test.ts index a8847c26642..df92d38b1b6 100644 --- a/src/infra/net/ssrf.pinning.test.ts +++ b/src/infra/net/ssrf.pinning.test.ts @@ -108,6 +108,33 @@ describe("ssrf pinning", () => { ).toThrow("Pinned lookup requires at least one address for example.com"); }); + it("blocks all hostnames when hostnameAllowlist is an explicit empty array", async () => { + const lookup = vi.fn(async () => [ + { address: "93.184.216.34", family: 4 }, + ]) as unknown as LookupFn; + + await expect( + resolvePinnedHostnameWithPolicy("example.com", { + lookupFn: lookup, + policy: { hostnameAllowlist: [] }, + }), + ).rejects.toThrow(/allowlist/i); + expect(lookup).not.toHaveBeenCalled(); + }); + + it("allows all hostnames when hostnameAllowlist is undefined (not configured)", async () => { + const lookup = vi.fn(async () => [ + { address: "93.184.216.34", family: 4 }, + ]) as unknown as LookupFn; + + await expect( + resolvePinnedHostnameWithPolicy("example.com", { + lookupFn: lookup, + policy: {}, + }), + ).resolves.toMatchObject({ hostname: "example.com" }); + }); + it("enforces hostname allowlist when configured", async () => { const lookup = vi.fn(async () => [ { address: "93.184.216.34", family: 4 }, diff --git a/src/infra/net/ssrf.ts b/src/infra/net/ssrf.ts index fd633fcb20d..83001d2a2da 100644 --- a/src/infra/net/ssrf.ts +++ b/src/infra/net/ssrf.ts @@ -50,8 +50,12 @@ function normalizeHostnameSet(values?: string[]): Set { return new Set(values.map((value) => normalizeHostname(value)).filter(Boolean)); } -function normalizeHostnameAllowlist(values?: string[]): string[] { - if (!values || values.length === 0) { +// Returns `null` when not configured (allow all), `[]` when explicit empty (deny all). +function normalizeHostnameAllowlist(values?: string[]): string[] | null { + if (values === undefined || values === null) { + return null; + } + if (values.length === 0) { return []; } return Array.from( @@ -91,10 +95,13 @@ function isHostnameAllowedByPattern(hostname: string, pattern: string): boolean return hostname === pattern; } -function matchesHostnameAllowlist(hostname: string, allowlist: string[]): boolean { - if (allowlist.length === 0) { +function matchesHostnameAllowlist(hostname: string, allowlist: string[] | null): boolean { + if (allowlist === null) { return true; } + if (allowlist.length === 0) { + return false; + } return allowlist.some((pattern) => isHostnameAllowedByPattern(hostname, pattern)); } From 3c9e3190476a6f090b7b94d0a68b9367ccb9ffac Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Thu, 19 Mar 2026 08:41:30 -0400 Subject: [PATCH 2/2] fix(ssrf): document bare-wildcard deny-all behavior and restore config in test --- src/gateway/openresponses-http.test.ts | 12 ++++++++++++ src/infra/net/ssrf.pinning.test.ts | 21 +++++++++++++++++++++ src/infra/net/ssrf.ts | 4 ++++ 3 files changed, 37 insertions(+) diff --git a/src/gateway/openresponses-http.test.ts b/src/gateway/openresponses-http.test.ts index 1dab33a3f57..50a0149b3fa 100644 --- a/src/gateway/openresponses-http.test.ts +++ b/src/gateway/openresponses-http.test.ts @@ -636,6 +636,11 @@ describe("OpenResponses HTTP API (e2e)", () => { await allowlistServer.close({ reason: "responses allowlist hardening test done" }); } + const configPath = process.env.OPENCLAW_CONFIG_PATH; + const previousConfig = configPath + ? await fs.readFile(configPath, "utf-8").catch(() => undefined) + : undefined; + const denyAllConfig = { gateway: { http: { @@ -685,6 +690,13 @@ describe("OpenResponses HTTP API (e2e)", () => { expect(agentCommand).not.toHaveBeenCalled(); } finally { await denyAllServer.close({ reason: "responses empty allowlist hardening test done" }); + if (configPath) { + if (previousConfig === undefined) { + await fs.rm(configPath, { force: true }); + } else { + await fs.writeFile(configPath, previousConfig, "utf-8"); + } + } } const capConfig = buildResponsesUrlPolicyConfig(0); diff --git a/src/infra/net/ssrf.pinning.test.ts b/src/infra/net/ssrf.pinning.test.ts index df92d38b1b6..0162384be09 100644 --- a/src/infra/net/ssrf.pinning.test.ts +++ b/src/infra/net/ssrf.pinning.test.ts @@ -122,6 +122,27 @@ describe("ssrf pinning", () => { expect(lookup).not.toHaveBeenCalled(); }); + it("blocks all hostnames when hostnameAllowlist contains only bare wildcards", async () => { + const lookup = vi.fn(async () => [ + { address: "93.184.216.34", family: 4 }, + ]) as unknown as LookupFn; + + await expect( + resolvePinnedHostnameWithPolicy("example.com", { + lookupFn: lookup, + policy: { hostnameAllowlist: ["*"] }, + }), + ).rejects.toThrow(/allowlist/i); + expect(lookup).not.toHaveBeenCalled(); + + await expect( + resolvePinnedHostnameWithPolicy("example.com", { + lookupFn: lookup, + policy: { hostnameAllowlist: ["*."] }, + }), + ).rejects.toThrow(/allowlist/i); + }); + it("allows all hostnames when hostnameAllowlist is undefined (not configured)", async () => { const lookup = vi.fn(async () => [ { address: "93.184.216.34", family: 4 }, diff --git a/src/infra/net/ssrf.ts b/src/infra/net/ssrf.ts index 83001d2a2da..dd044356a4f 100644 --- a/src/infra/net/ssrf.ts +++ b/src/infra/net/ssrf.ts @@ -51,6 +51,7 @@ function normalizeHostnameSet(values?: string[]): Set { } // Returns `null` when not configured (allow all), `[]` when explicit empty (deny all). +// Non-empty input where every entry is invalid (e.g. ["*"]) also returns [] (fail closed). function normalizeHostnameAllowlist(values?: string[]): string[] | null { if (values === undefined || values === null) { return null; @@ -58,6 +59,9 @@ function normalizeHostnameAllowlist(values?: string[]): string[] | null { if (values.length === 0) { return []; } + // Bare "*" and "*." are not valid hostname patterns (they would only match a literal + // "*" hostname in isHostnameAllowedByPattern). Strip them so operators who mistakenly + // configure ["*"] get a clear deny-all rather than a silent pass-through. return Array.from( new Set( values