mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-22 06:42:19 +03:00
Compare commits
3 Commits
fix/13380-
...
fix/12569-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
b9390ccb19 | ||
|
|
f8ea49bb78 | ||
|
|
b6cc3e57c6 |
1
changelog.d/fixes/12569-webhook-dns-rebinding-ssrf.md
Normal file
1
changelog.d/fixes/12569-webhook-dns-rebinding-ssrf.md
Normal file
@@ -0,0 +1 @@
|
||||
- fix(api): close DNS-rebinding SSRF gap in webhook outbound-URL guard (#12569)
|
||||
@@ -1354,11 +1354,6 @@
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/app/api/webhooks/[id]/test/route.ts": {
|
||||
"@typescript-eslint/no-unused-vars": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/app/login/page.tsx": {
|
||||
"@typescript-eslint/no-unused-vars": {
|
||||
"count": 3
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
{
|
||||
"_rebaseline_2026_09_11_mergebatch_v3851_diego": "/merge-batch 2026-09-11 (v3.8.51), owner batch. open-sse/handlers/chatCore.ts 6144->6146 (+2): #13278 requires a Responses-shaped body before the native OpenAI-compatible passthrough (+1) and #13276 stops the reactive-compaction log from claiming a compaction when compression is disabled (+2/-1). Both are guard conditions at existing call sites, no new branching structure. open-sse/utils/stream.ts is deliberately NOT rebaselined: already 3115 > 3098 on the pure tip with zero contribution from this batch (base-red #12732, owned by /sweep-reds). Covered by 256 assertions across the batch's test files (246 node:test + 10 vitest).",
|
||||
"_rebaseline_2026_09_11_mergebatch_v3851_houminxi": "/merge-batch 2026-09-11 (v3.8.51), batch by HouMinXi. Final combined values, set on the first PR merged so every intermediate state is covered. open-sse/handlers/chatCore.ts 6036->6144: #13069 routes the non-streaming leg through the same provider-failure classification, model lockout and credential-refresh path the streaming leg already used (+443/-340 = +103 net; it extracts applyProviderFailureClassification and wires both legs to it, which is what #13043 reported missing), plus #13050 stamping that the client asked for SSE before the web_search fallback flips stream off (+6) and #13038 threading the dispatched target index (+3). src/sse/services/auth.ts 3488->3542: #13017 adds the explicit-pin one-shot probe for a recoverable inactive row with its 60s storm gate (+42 net) and #13061 makes a grok-cli 402 a connection-wide shared-wallet signal instead of a per-model billing miss (+12 net). src/sse/handlers/chat.ts 2458->2462: #13038 (+5). open-sse/services/combo/executeTargetAttempt.ts 1205->1212: #13006 feeds the 402 it already classified into the quota cache instead of dropping it (+7). open-sse/services/accountFallback.ts 2468->2469: #13060 adds the Cline re-auth phrase to OAUTH_INVALID_TOKEN_SIGNALS (+1). open-sse/utils/stream.ts is deliberately NOT rebaselined: already 3115 > 3098 on the pure tip with zero contribution from this batch (base-red #12732, owned by /sweep-reds). The file also carried \"open-sse/handlers/chatCore.ts\" twice (6026 and 6036); JSON keeps the last, so the first was dead weight any writer could have picked instead. Collapsed to one entry at the live value. Covered by 531 focused assertions across the batch's 46 test files.",
|
||||
"_rebaseline_2026_09_11_12358_chat_pipeline_custom_node": "PR #12358 own test growth: tests/integration/chat-pipeline.test.ts 1648->1736 (+88). One new integration case, \"#11884 chat pipeline sends a custom node's edited Chat API type upstream\": it seeds a custom OpenAI-compatible node with an edited Chat/Responses API type, stubs fetch, drives handleChatCore and asserts the upstream request carries the live connection setting rather than the format baked into the node id at creation. Irreducible at this layer — the point of the test is the full route-to-upstream path, which is what #11884 regressed. Nothing else in the file changed. Covered by the case itself plus tests/unit/chat-helpers.test.ts (28/28).",
|
||||
"_rebaseline_2026_09_10_12975_rotation_correlation_id": "PR #12975 own growth: open-sse/executors/base.ts 1751->1753 (+2) and open-sse/handlers/chatCore.ts 6021->6024 (+3). The opencode rotation lines carry the request correlationId: one optional ExecuteInput field and one correlationId argument at each of the three executor.execute call sites in handleChatCore. Irreducible plumbing at existing call sites; the rotation logic itself lives in open-sse/executors/opencode.ts and the new leaf predicates (under cap). Covered by tests/unit/opencode-transient-rotation.test.ts and tests/unit/chat-correlation-id-exhaustion.test.ts.",
|
||||
@@ -431,7 +432,7 @@
|
||||
"open-sse/executors/codex.ts": 1505,
|
||||
"open-sse/executors/cursor.ts": 1759,
|
||||
"open-sse/executors/muse-spark-web.ts": 1405,
|
||||
"open-sse/handlers/chatCore.ts": 6144,
|
||||
"open-sse/handlers/chatCore.ts": 6146,
|
||||
"open-sse/handlers/imageGeneration.ts": 3259,
|
||||
"open-sse/handlers/search.ts": 1789,
|
||||
"open-sse/mcp-server/schemas/tools.ts": 1621,
|
||||
|
||||
@@ -12,8 +12,7 @@ import { buildTelegramUrl, buildTelegramPayload } from "@/lib/webhooks/integrati
|
||||
import { buildDiscordPayload } from "@/lib/webhooks/integrations/discord";
|
||||
import { requireManagementAuth } from "@/lib/api/requireManagementAuth";
|
||||
import { insertDelivery } from "@/lib/db/webhookDeliveries";
|
||||
import { isPrivateHost, OutboundUrlGuardError } from "@/shared/network/outboundUrlGuard";
|
||||
import { parseAndValidateWebhookUrl } from "@/shared/network/outboundUrlGuardPolicy";
|
||||
import { fetchWebhookUrl } from "@/shared/network/webhookFetch";
|
||||
import crypto from "crypto";
|
||||
|
||||
const MAX_RESPONSE_BODY = 2048;
|
||||
@@ -31,35 +30,43 @@ async function testFetch(
|
||||
}> {
|
||||
const start = Date.now();
|
||||
try {
|
||||
const parsed = parseAndValidateWebhookUrl(url);
|
||||
// For private (opted-in) targets, return connectivity diagnostics only — never the
|
||||
// upstream response body, so this endpoint can't be used to exfiltrate content from
|
||||
// internal services reachable from the server. (#3269 hardening)
|
||||
const redactBody = isPrivateHost(parsed.hostname);
|
||||
const controller = new AbortController();
|
||||
const timeoutId = setTimeout(() => controller.abort(), 10_000);
|
||||
const res = await fetch(url, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
"User-Agent": "OmniRoute-Webhook/1.0",
|
||||
...headers,
|
||||
},
|
||||
body: JSON.stringify(body),
|
||||
signal: controller.signal,
|
||||
});
|
||||
clearTimeout(timeoutId);
|
||||
let response: Response;
|
||||
let redactBody: boolean;
|
||||
try {
|
||||
({ response, redactBody } = await fetchWebhookUrl(
|
||||
url,
|
||||
{
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
"User-Agent": "OmniRoute-Webhook/1.0",
|
||||
...headers,
|
||||
},
|
||||
body: JSON.stringify(body),
|
||||
},
|
||||
{ signal: controller.signal }
|
||||
));
|
||||
} finally {
|
||||
clearTimeout(timeoutId);
|
||||
}
|
||||
const latencyMs = Date.now() - start;
|
||||
// For private (opted-in) targets, return connectivity diagnostics only — never the
|
||||
// upstream response body, so this endpoint can't be used to exfiltrate content from
|
||||
// internal services reachable from the server. (#3269 hardening) The verdict is derived
|
||||
// from the DNS-resolved address, not the raw hostname string, so a public-looking hostname
|
||||
// rebound to a private IP is redacted too.
|
||||
let rawBody = "";
|
||||
try {
|
||||
rawBody = await res.text();
|
||||
rawBody = await response.text();
|
||||
if (rawBody.length > MAX_RESPONSE_BODY) rawBody = rawBody.slice(0, MAX_RESPONSE_BODY) + "…";
|
||||
} catch {
|
||||
rawBody = "";
|
||||
}
|
||||
return {
|
||||
success: res.ok,
|
||||
status: res.status,
|
||||
success: response.ok,
|
||||
status: response.status,
|
||||
latencyMs,
|
||||
responseBody: redactBody ? "<redacted: private target>" : rawBody,
|
||||
};
|
||||
|
||||
@@ -6,7 +6,8 @@
|
||||
|
||||
import crypto from "crypto";
|
||||
import { encrypt, decrypt } from "./db/encryption";
|
||||
import { parseAndValidateWebhookUrl } from "@/shared/network/outboundUrlGuardPolicy";
|
||||
import { OutboundUrlGuardError } from "@/shared/network/outboundUrlGuard";
|
||||
import { fetchWebhookUrl, type WebhookFetchOptions } from "@/shared/network/webhookFetch";
|
||||
import type { WebhookEvent } from "./webhooks/eventDescriptions";
|
||||
|
||||
export type { WebhookEvent };
|
||||
@@ -17,6 +18,10 @@ export interface WebhookPayload {
|
||||
data: Record<string, any>;
|
||||
}
|
||||
|
||||
/** DNS-resolve/fetch overrides — production callers never pass these; tests inject a fake
|
||||
* resolver and/or fetch to avoid real network access (#12569). */
|
||||
export type WebhookDeliveryOptions = Pick<WebhookFetchOptions, "lookup" | "fetchImpl">;
|
||||
|
||||
function signPayload(payload: string, secret: string): string {
|
||||
return `sha256=${crypto.createHmac("sha256", secret).update(payload).digest("hex")}`;
|
||||
}
|
||||
@@ -38,21 +43,24 @@ export function decryptMetadata(encrypted: string | null): Record<string, string
|
||||
|
||||
async function deliverRaw(
|
||||
url: string,
|
||||
body: Record<string, unknown>
|
||||
body: Record<string, unknown>,
|
||||
options?: WebhookDeliveryOptions
|
||||
): Promise<{ success: boolean; status: number; latencyMs: number; error?: string }> {
|
||||
const start = Date.now();
|
||||
try {
|
||||
parseAndValidateWebhookUrl(url);
|
||||
const controller = new AbortController();
|
||||
const timeoutId = setTimeout(() => controller.abort(), 10_000);
|
||||
try {
|
||||
const res = await fetch(url, {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json", "User-Agent": "OmniRoute-Webhook/1.0" },
|
||||
body: JSON.stringify(body),
|
||||
signal: controller.signal,
|
||||
});
|
||||
return { success: res.ok, status: res.status, latencyMs: Date.now() - start };
|
||||
const { response } = await fetchWebhookUrl(
|
||||
url,
|
||||
{
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json", "User-Agent": "OmniRoute-Webhook/1.0" },
|
||||
body: JSON.stringify(body),
|
||||
},
|
||||
{ ...options, signal: controller.signal }
|
||||
);
|
||||
return { success: response.ok, status: response.status, latencyMs: Date.now() - start };
|
||||
} finally {
|
||||
// Always clear the abort timer — on a non-timeout fetch error the previous code skipped
|
||||
// clearTimeout, leaving a dangling 10s timer (and AbortController) per failed call.
|
||||
@@ -72,13 +80,9 @@ export async function deliverWebhook(
|
||||
url: string,
|
||||
payload: WebhookPayload,
|
||||
secret?: string | null,
|
||||
maxRetries = 3
|
||||
maxRetries = 3,
|
||||
options?: WebhookDeliveryOptions
|
||||
): Promise<{ success: boolean; status: number; error?: string }> {
|
||||
try {
|
||||
parseAndValidateWebhookUrl(url);
|
||||
} catch (error: any) {
|
||||
return { success: false, status: 0, error: error.message || "Blocked outbound URL" };
|
||||
}
|
||||
const body = JSON.stringify(payload);
|
||||
const headers: Record<string, string> = {
|
||||
"Content-Type": "application/json",
|
||||
@@ -96,29 +100,31 @@ export async function deliverWebhook(
|
||||
const controller = new AbortController();
|
||||
const timeoutId = setTimeout(() => controller.abort(), 10_000);
|
||||
|
||||
let res: Response;
|
||||
let response: Response;
|
||||
try {
|
||||
res = await fetch(url, {
|
||||
method: "POST",
|
||||
headers,
|
||||
body,
|
||||
signal: controller.signal,
|
||||
});
|
||||
({ response } = await fetchWebhookUrl(
|
||||
url,
|
||||
{ method: "POST", headers, body },
|
||||
{ ...options, signal: controller.signal }
|
||||
));
|
||||
} finally {
|
||||
// Clear the abort timer on every path — a non-timeout fetch error previously skipped
|
||||
// clearTimeout, leaking a dangling 10s timer + AbortController per failed attempt.
|
||||
clearTimeout(timeoutId);
|
||||
}
|
||||
|
||||
if (res.ok || res.status < 500) {
|
||||
return { success: res.ok, status: res.status };
|
||||
if (response.ok || response.status < 500) {
|
||||
return { success: response.ok, status: response.status };
|
||||
}
|
||||
|
||||
if (attempt < maxRetries) {
|
||||
await new Promise((r) => setTimeout(r, Math.pow(2, attempt) * 1000));
|
||||
}
|
||||
} catch (error: any) {
|
||||
if (attempt === maxRetries) {
|
||||
// A blocked outbound URL (private/metadata resolved address, or a redirect hop that
|
||||
// resolved to one) is never transient — fail closed immediately instead of burning
|
||||
// retries/backoff on something that will keep resolving the same way.
|
||||
if (attempt === maxRetries || error instanceof OutboundUrlGuardError) {
|
||||
return { success: false, status: 0, error: error.message || "Network error" };
|
||||
}
|
||||
await new Promise((r) => setTimeout(r, Math.pow(2, attempt) * 1000));
|
||||
|
||||
94
src/shared/network/dnsPinnedFetch.ts
Normal file
94
src/shared/network/dnsPinnedFetch.ts
Normal file
@@ -0,0 +1,94 @@
|
||||
import { isIP } from "node:net";
|
||||
import dns from "node:dns";
|
||||
import { Agent, fetch as undiciFetch } from "undici";
|
||||
|
||||
/**
|
||||
* Shared DNS-resolve-then-pin primitives (#12569). Originally written only for
|
||||
* `remoteImageFetch.ts` (GHSA-cmhj-wh2f-9cgx); extracted here so the webhook outbound-URL
|
||||
* guard (`webhookFetch.ts`) can reuse the exact same connection-pinning mechanism instead of
|
||||
* duplicating it. `remoteImageFetch.ts` re-exports `createPinnedFetch` from here for backward
|
||||
* compatibility with its existing import path.
|
||||
*/
|
||||
|
||||
export interface DnsLookupResult {
|
||||
address: string;
|
||||
family: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* Minimal DNS lookup contract — matches the shape returned by
|
||||
* `node:dns/promises`.lookup(host, { all: true }). Exposed as an option so
|
||||
* tests can inject a fake resolver without touching real DNS.
|
||||
*/
|
||||
export type DnsLookup = (hostname: string) => Promise<DnsLookupResult[]>;
|
||||
|
||||
export const defaultDnsLookup: DnsLookup = (hostname) =>
|
||||
dns.promises.lookup(hostname, { all: true });
|
||||
|
||||
/** Strip literal IPv6 brackets: "[::1]" -> "::1". */
|
||||
export function bareHostname(hostname: string): string {
|
||||
return hostname.startsWith("[") && hostname.endsWith("]") ? hostname.slice(1, -1) : hostname;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve every DNS answer for a hostname, short-circuiting for an IP literal (which needs no
|
||||
* lookup — it already IS the connect-time address). Fails closed: a lookup error or an empty
|
||||
* answer set throws rather than being treated as "no restriction applies".
|
||||
*/
|
||||
export async function resolveHostnameAddresses(
|
||||
hostname: string,
|
||||
lookup: DnsLookup = defaultDnsLookup
|
||||
): Promise<DnsLookupResult[]> {
|
||||
const bare = bareHostname(hostname);
|
||||
if (!bare) return [];
|
||||
const literalFamily = isIP(bare);
|
||||
if (literalFamily) return [{ address: bare, family: literalFamily }];
|
||||
const resolved = await lookup(bare);
|
||||
if (!resolved.length) {
|
||||
throw new Error(`Host "${bare}" could not be resolved`);
|
||||
}
|
||||
return resolved;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a `fetch` bound to a single already-DNS-validated address, ignoring
|
||||
* whatever the hostname resolves to at connect time. Exported for direct
|
||||
* testing: this is the mechanism that closes the DNS-rebinding TOCTOU gap
|
||||
* (GHSA-cmhj-wh2f-9cgx) — a second, real DNS lookup at connect time could
|
||||
* otherwise return a different (possibly private) address than the one
|
||||
* validated up-front.
|
||||
*/
|
||||
export function createPinnedFetch(address: string, family: number): typeof fetch {
|
||||
const dispatcher = new Agent({
|
||||
connect: {
|
||||
// Node's `net.connect`/`tls.connect` invoke a custom `lookup` in one of
|
||||
// two incompatible shapes depending on `options.all`: modern Node
|
||||
// (autoSelectFamily / Happy Eyeballs, on by default since Node 18)
|
||||
// calls `lookup(hostname, { all: true, ... }, callback)` and requires
|
||||
// `callback(err, addresses[])` — an array of `{ address, family }`.
|
||||
// Only when `all` is falsy does it accept the single-address form
|
||||
// `callback(err, address, family)`. Handling only the single-address
|
||||
// form here (as an earlier draft did) throws `ERR_INVALID_IP_ADDRESS`
|
||||
// for every real request once autoSelectFamily kicks in, silently
|
||||
// breaking every pinned fetch — verified by
|
||||
// `tests/unit/remote-image-fetch-pin-dns-connection.test.ts`.
|
||||
lookup: (_hostname, options, callback) => {
|
||||
if (options && typeof options === "object" && "all" in options && options.all) {
|
||||
callback(null, [{ address, family }]);
|
||||
return;
|
||||
}
|
||||
callback(null, address, family);
|
||||
},
|
||||
},
|
||||
});
|
||||
return (async (input, init) => {
|
||||
try {
|
||||
return (await undiciFetch(input as string | URL, {
|
||||
...(init as Parameters<typeof undiciFetch>[1]),
|
||||
dispatcher,
|
||||
})) as unknown as Response;
|
||||
} finally {
|
||||
await dispatcher.close();
|
||||
}
|
||||
}) as typeof fetch;
|
||||
}
|
||||
@@ -1,6 +1,5 @@
|
||||
import { isIP } from "node:net";
|
||||
import dns from "node:dns";
|
||||
import { Agent, fetch as undiciFetch } from "undici";
|
||||
import {
|
||||
type OutboundUrlGuardMode,
|
||||
isPrivateHost,
|
||||
@@ -9,6 +8,13 @@ import {
|
||||
parseOutboundUrl,
|
||||
} from "@/shared/network/outboundUrlGuard";
|
||||
import { getProviderOutboundGuard } from "@/shared/network/outboundUrlGuardPolicy";
|
||||
// #12569: `createPinnedFetch` now lives in the shared `dnsPinnedFetch.ts` module so the
|
||||
// webhook outbound-URL guard can reuse the exact same connection-pinning mechanism instead of
|
||||
// duplicating it. Re-exported here for backward compatibility with existing importers of
|
||||
// `@/shared/network/remoteImageFetch`.
|
||||
import { createPinnedFetch } from "@/shared/network/dnsPinnedFetch";
|
||||
|
||||
export { createPinnedFetch };
|
||||
|
||||
const DEFAULT_MAX_REMOTE_IMAGE_BYTES = 20 * 1024 * 1024;
|
||||
const DEFAULT_MAX_REDIRECTS = 3;
|
||||
@@ -95,48 +101,6 @@ async function assertHostnameResolvesPublic(
|
||||
}
|
||||
return resolved;
|
||||
}
|
||||
/**
|
||||
* Build a `fetch` bound to a single already-DNS-validated address, ignoring
|
||||
* whatever the hostname resolves to at connect time. Exported for direct
|
||||
* testing: this is the mechanism that closes the DNS-rebinding TOCTOU gap
|
||||
* (GHSA-cmhj-wh2f-9cgx) — a second, real DNS lookup at connect time could
|
||||
* otherwise return a different (possibly private) address than the one
|
||||
* `assertHostnameResolvesPublic` validated.
|
||||
*/
|
||||
export function createPinnedFetch(address: string, family: number): typeof fetch {
|
||||
const dispatcher = new Agent({
|
||||
connect: {
|
||||
// Node's `net.connect`/`tls.connect` invoke a custom `lookup` in one of
|
||||
// two incompatible shapes depending on `options.all`: modern Node
|
||||
// (autoSelectFamily / Happy Eyeballs, on by default since Node 18)
|
||||
// calls `lookup(hostname, { all: true, ... }, callback)` and requires
|
||||
// `callback(err, addresses[])` — an array of `{ address, family }`.
|
||||
// Only when `all` is falsy does it accept the single-address form
|
||||
// `callback(err, address, family)`. Handling only the single-address
|
||||
// form here (as an earlier draft did) throws `ERR_INVALID_IP_ADDRESS`
|
||||
// for every real request once autoSelectFamily kicks in, silently
|
||||
// breaking every pinned fetch — verified by
|
||||
// `tests/unit/remote-image-fetch-pin-dns-connection.test.ts`.
|
||||
lookup: (_hostname, options, callback) => {
|
||||
if (options && typeof options === "object" && "all" in options && options.all) {
|
||||
callback(null, [{ address, family }]);
|
||||
return;
|
||||
}
|
||||
callback(null, address, family);
|
||||
},
|
||||
},
|
||||
});
|
||||
return (async (input, init) => {
|
||||
try {
|
||||
return (await undiciFetch(input as string | URL, {
|
||||
...(init as Parameters<typeof undiciFetch>[1]),
|
||||
dispatcher,
|
||||
})) as unknown as Response;
|
||||
} finally {
|
||||
await dispatcher.close();
|
||||
}
|
||||
}) as typeof fetch;
|
||||
}
|
||||
function combineSignals(signal: AbortSignal | undefined, timeoutMs: number) {
|
||||
const timeoutSignal = AbortSignal.timeout(timeoutMs);
|
||||
if (!signal) return timeoutSignal;
|
||||
|
||||
167
src/shared/network/webhookFetch.ts
Normal file
167
src/shared/network/webhookFetch.ts
Normal file
@@ -0,0 +1,167 @@
|
||||
import {
|
||||
createPinnedFetch,
|
||||
defaultDnsLookup,
|
||||
resolveHostnameAddresses,
|
||||
type DnsLookup,
|
||||
type DnsLookupResult,
|
||||
} from "@/shared/network/dnsPinnedFetch";
|
||||
import {
|
||||
isCloudMetadataHost,
|
||||
isPrivateHost,
|
||||
OutboundUrlGuardError,
|
||||
parseOutboundUrl,
|
||||
PROVIDER_URL_BLOCKED_MESSAGE,
|
||||
} from "@/shared/network/outboundUrlGuard";
|
||||
import { arePrivateProviderUrlsAllowed } from "@/shared/network/outboundUrlGuardPolicy";
|
||||
|
||||
/**
|
||||
* #12569 — DNS-resolve-then-pin fetch for webhook outbound calls (custom webhook delivery +
|
||||
* the webhook test-diagnostics endpoint). `parseAndValidateWebhookUrl` in
|
||||
* `outboundUrlGuardPolicy.ts` only classifies the literal hostname STRING, so a hostname an
|
||||
* attacker controls (DNS pointed at 169.254.169.254 / an RFC1918 address) passed that guard
|
||||
* and reached the real `fetch()` unmodified. This module resolves DNS up front, rejects any
|
||||
* resolved answer that is cloud-metadata (always) or private (unless the private-provider-URL
|
||||
* opt-in is on), pins the connection to a validated address, and revalidates every redirect
|
||||
* hop the same way — a public host answering 302 to an internal address no longer escapes
|
||||
* the guard.
|
||||
*/
|
||||
|
||||
const DEFAULT_MAX_REDIRECTS = 3;
|
||||
|
||||
export interface WebhookFetchOptions {
|
||||
/** DNS resolver override. Tests inject a fake resolver to avoid real network lookups. */
|
||||
lookup?: DnsLookup;
|
||||
/** Fetch override. Takes priority over connection pinning — the mockable escape hatch used
|
||||
* by existing tests that stub `globalThis.fetch`. */
|
||||
fetchImpl?: typeof fetch;
|
||||
/** Pin the connection to the validated DNS answer. Default true — this is the mechanism
|
||||
* that closes the DNS-rebinding TOCTOU gap. */
|
||||
pinDns?: boolean;
|
||||
maxRedirects?: number;
|
||||
signal?: AbortSignal;
|
||||
}
|
||||
|
||||
export interface WebhookFetchResult {
|
||||
response: Response;
|
||||
finalUrl: string;
|
||||
/** True when a resolved hop is a private address explicitly allowed via opt-in — the
|
||||
* caller must not surface the upstream response body for such a target (#3269). */
|
||||
redactBody: boolean;
|
||||
}
|
||||
|
||||
/** Reject a resolved address set that includes a metadata or (non-opted-in) private IP. */
|
||||
function assertAddressesAllowed(addresses: DnsLookupResult[], url: URL): boolean {
|
||||
const allowPrivate = arePrivateProviderUrlsAllowed();
|
||||
let sawPrivate = false;
|
||||
for (const { address } of addresses) {
|
||||
if (isCloudMetadataHost(address)) {
|
||||
throw new OutboundUrlGuardError(PROVIDER_URL_BLOCKED_MESSAGE, {
|
||||
code: "OUTBOUND_URL_GUARD_BLOCKED",
|
||||
url: url.toString(),
|
||||
hostname: address,
|
||||
});
|
||||
}
|
||||
if (isPrivateHost(address)) {
|
||||
if (!allowPrivate) {
|
||||
throw new OutboundUrlGuardError(PROVIDER_URL_BLOCKED_MESSAGE, {
|
||||
code: "OUTBOUND_URL_GUARD_BLOCKED",
|
||||
url: url.toString(),
|
||||
hostname: address,
|
||||
});
|
||||
}
|
||||
sawPrivate = true;
|
||||
}
|
||||
}
|
||||
return sawPrivate;
|
||||
}
|
||||
|
||||
async function resolveHop(
|
||||
currentUrl: string | URL,
|
||||
lookup: DnsLookup
|
||||
): Promise<{ url: URL; addresses: DnsLookupResult[]; redactBody: boolean }> {
|
||||
const url = parseOutboundUrl(currentUrl);
|
||||
let addresses: DnsLookupResult[];
|
||||
try {
|
||||
addresses = await resolveHostnameAddresses(url.hostname, lookup);
|
||||
} catch {
|
||||
throw new OutboundUrlGuardError("Webhook host could not be resolved (blocked)", {
|
||||
code: "OUTBOUND_URL_GUARD_BLOCKED",
|
||||
url: url.toString(),
|
||||
hostname: url.hostname || null,
|
||||
});
|
||||
}
|
||||
const redactBody = assertAddressesAllowed(addresses, url);
|
||||
return { url, addresses, redactBody };
|
||||
}
|
||||
|
||||
function pickFetchImpl(
|
||||
fetchImpl: typeof fetch | undefined,
|
||||
pinDns: boolean,
|
||||
addresses: DnsLookupResult[]
|
||||
): typeof fetch {
|
||||
if (fetchImpl) return fetchImpl;
|
||||
if (pinDns && addresses.length) return createPinnedFetch(addresses[0].address, addresses[0].family);
|
||||
return fetch;
|
||||
}
|
||||
|
||||
function nextRedirectUrl(
|
||||
response: Response,
|
||||
currentUrl: URL,
|
||||
redirectCount: number,
|
||||
maxRedirects: number
|
||||
): URL {
|
||||
const location = response.headers.get("location");
|
||||
if (!location) {
|
||||
throw new OutboundUrlGuardError("Webhook redirect missing Location header", {
|
||||
code: "OUTBOUND_URL_INVALID",
|
||||
url: currentUrl.toString(),
|
||||
});
|
||||
}
|
||||
if (redirectCount >= maxRedirects) {
|
||||
throw new OutboundUrlGuardError(`Webhook exceeded ${maxRedirects} redirect limit`, {
|
||||
code: "OUTBOUND_URL_GUARD_BLOCKED",
|
||||
url: currentUrl.toString(),
|
||||
});
|
||||
}
|
||||
return new URL(location, currentUrl);
|
||||
}
|
||||
|
||||
/**
|
||||
* DNS-resolve-then-pin POST/GET for a webhook URL, following redirects manually and
|
||||
* revalidating DNS at every hop. Throws `OutboundUrlGuardError` when the target (or a
|
||||
* redirect target) resolves to a blocked address.
|
||||
*/
|
||||
export async function fetchWebhookUrl(
|
||||
input: string,
|
||||
init: RequestInit,
|
||||
options: WebhookFetchOptions = {}
|
||||
): Promise<WebhookFetchResult> {
|
||||
const lookup = options.lookup ?? defaultDnsLookup;
|
||||
const maxRedirects = options.maxRedirects ?? DEFAULT_MAX_REDIRECTS;
|
||||
const pinDns = options.pinDns !== false;
|
||||
let currentUrl: string | URL = input;
|
||||
let redactBody = false;
|
||||
|
||||
for (let redirectCount = 0; redirectCount <= maxRedirects; redirectCount++) {
|
||||
const hop = await resolveHop(currentUrl, lookup);
|
||||
redactBody = redactBody || hop.redactBody;
|
||||
const fetchImpl = pickFetchImpl(options.fetchImpl, pinDns, hop.addresses);
|
||||
const response = await fetchImpl(hop.url.toString(), {
|
||||
...init,
|
||||
redirect: "manual",
|
||||
signal: options.signal,
|
||||
});
|
||||
|
||||
if (response.status >= 300 && response.status < 400) {
|
||||
currentUrl = nextRedirectUrl(response, hop.url, redirectCount, maxRedirects);
|
||||
continue;
|
||||
}
|
||||
|
||||
return { response, finalUrl: hop.url.toString(), redactBody };
|
||||
}
|
||||
|
||||
throw new OutboundUrlGuardError(`Webhook exceeded ${maxRedirects} redirect limit`, {
|
||||
code: "OUTBOUND_URL_GUARD_BLOCKED",
|
||||
url: String(input),
|
||||
});
|
||||
}
|
||||
@@ -8,10 +8,15 @@ const { deliverWebhook } = await import("../../src/lib/webhookDispatcher.ts");
|
||||
// called clearTimeout on the success path, so a non-timeout fetch rejection
|
||||
// (ECONNREFUSED, DNS failure, etc.) skipped clearTimeout, leaking a live 10s timer
|
||||
// + AbortController per failed delivery. The fix clears the timer in a `finally`.
|
||||
//
|
||||
// #12569: deliverWebhook now DNS-resolves and pins the connection before dispatch, so a
|
||||
// `globalThis.fetch` stub alone no longer intercepts the outbound call (the pinned fetch talks
|
||||
// to undici directly). Inject a fake `lookup` (no real DNS) and `fetchImpl` (the documented
|
||||
// escape hatch — see `WebhookDeliveryOptions`) instead, so this test stays deterministic and
|
||||
// network-free while still exercising the exact "fetch rejects" path it targets.
|
||||
test("deliverWebhook clears the abort timer even when fetch rejects", async () => {
|
||||
const realSetTimeout = globalThis.setTimeout;
|
||||
const realClearTimeout = globalThis.clearTimeout;
|
||||
const realFetch = globalThis.fetch;
|
||||
|
||||
const abortTimerIds = new Set<unknown>();
|
||||
const clearedIds = new Set<unknown>();
|
||||
@@ -26,17 +31,20 @@ test("deliverWebhook clears the abort timer even when fetch rejects", async () =
|
||||
clearedIds.add(id);
|
||||
return realClearTimeout(id);
|
||||
}) as typeof clearTimeout;
|
||||
// Non-timeout network failure — the exact path that previously skipped clearTimeout.
|
||||
globalThis.fetch = (async () => {
|
||||
throw new Error("ECONNREFUSED");
|
||||
}) as typeof fetch;
|
||||
|
||||
try {
|
||||
const res = await deliverWebhook(
|
||||
"https://example.com/webhook",
|
||||
{ event: "test.event" as any, timestamp: new Date().toISOString(), data: {} },
|
||||
null,
|
||||
0 // maxRetries=0 → single attempt, no exponential-backoff timers
|
||||
0, // maxRetries=0 → single attempt, no exponential-backoff timers
|
||||
{
|
||||
lookup: async () => [{ address: "203.0.113.5", family: 4 }],
|
||||
// Non-timeout network failure — the exact path that previously skipped clearTimeout.
|
||||
fetchImpl: async () => {
|
||||
throw new Error("ECONNREFUSED");
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
assert.equal(res.success, false, "delivery should fail when fetch rejects");
|
||||
@@ -52,6 +60,5 @@ test("deliverWebhook clears the abort timer even when fetch rejects", async () =
|
||||
} finally {
|
||||
globalThis.setTimeout = realSetTimeout;
|
||||
globalThis.clearTimeout = realClearTimeout;
|
||||
globalThis.fetch = realFetch;
|
||||
}
|
||||
});
|
||||
|
||||
135
tests/unit/webhook-dns-rebinding-ssrf-12569.test.ts
Normal file
135
tests/unit/webhook-dns-rebinding-ssrf-12569.test.ts
Normal file
@@ -0,0 +1,135 @@
|
||||
/**
|
||||
* Regression for issue #12569: the webhook outbound-URL guard
|
||||
* (`parseAndValidateWebhookUrl`, `isPrivateHost`, `isCloudMetadataHost`) classified only the
|
||||
* literal hostname STRING in the configured webhook URL. It never resolved DNS before
|
||||
* deciding a target was public, so a domain an attacker controls (DNS A record pointed at
|
||||
* 169.254.169.254 / an RFC1918 address) passed the guard, and the real `fetch()` that
|
||||
* followed resolved DNS itself and reached the internal target (DNS rebinding).
|
||||
*
|
||||
* Fixed by `fetchWebhookUrl` (`src/shared/network/webhookFetch.ts`), which resolves DNS
|
||||
* up-front, rejects any resolved answer that is cloud-metadata/private, and pins the
|
||||
* connection to the validated address (so a *second*, real DNS lookup at connect time cannot
|
||||
* rebind to a different address either).
|
||||
*
|
||||
* Run with:
|
||||
* node --import tsx/esm --test tests/unit/webhook-dns-rebinding-ssrf-12569.test.ts
|
||||
*/
|
||||
|
||||
import { describe, it, mock, after } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import dns from "node:dns";
|
||||
|
||||
import { deliverWebhook } from "../../src/lib/webhookDispatcher.ts";
|
||||
|
||||
const REBOUND_HOSTNAME = "evil.example.com";
|
||||
const IMDS_ADDRESS = "169.254.169.254";
|
||||
|
||||
const originalLookup = dns.promises.lookup;
|
||||
mock.method(
|
||||
dns.promises,
|
||||
"lookup",
|
||||
async (hostname: string): Promise<dns.LookupAddress[]> => {
|
||||
if (hostname === REBOUND_HOSTNAME) {
|
||||
return [{ address: IMDS_ADDRESS, family: 4 }];
|
||||
}
|
||||
return originalLookup(hostname, { all: true });
|
||||
}
|
||||
);
|
||||
|
||||
after(() => {
|
||||
mock.restoreAll();
|
||||
});
|
||||
|
||||
describe("#12569 — webhook outbound guard is hostname-string-only (DNS rebinding)", () => {
|
||||
it("does NOT let a hostname that resolves to the cloud-metadata IP reach fetch()", async () => {
|
||||
const fetchCalls: string[] = [];
|
||||
const originalFetch = globalThis.fetch;
|
||||
// @ts-expect-error - stubbing global fetch for the probe
|
||||
globalThis.fetch = async (input: string) => {
|
||||
fetchCalls.push(String(input));
|
||||
return new Response("ok", { status: 200 });
|
||||
};
|
||||
|
||||
try {
|
||||
const res = await deliverWebhook(
|
||||
`http://${REBOUND_HOSTNAME}/hook`,
|
||||
{ event: "test.ping", timestamp: new Date().toISOString(), data: {} },
|
||||
"secret"
|
||||
);
|
||||
|
||||
assert.equal(
|
||||
fetchCalls.length,
|
||||
0,
|
||||
`guard should have blocked dispatch to a hostname resolving to ${IMDS_ADDRESS}, ` +
|
||||
`but fetch() was called with: ${JSON.stringify(fetchCalls)}`
|
||||
);
|
||||
assert.equal(res.success, false);
|
||||
} finally {
|
||||
globalThis.fetch = originalFetch;
|
||||
}
|
||||
});
|
||||
|
||||
it("blocks a hostname that resolves to an RFC1918 address, without retrying", async () => {
|
||||
const start = Date.now();
|
||||
const res = await deliverWebhook(
|
||||
"http://rebind-to-lan.example.com/hook",
|
||||
{ event: "test.ping", timestamp: new Date().toISOString(), data: {} },
|
||||
null,
|
||||
3,
|
||||
{ lookup: async () => [{ address: "10.1.2.3", family: 4 }] }
|
||||
);
|
||||
const elapsedMs = Date.now() - start;
|
||||
|
||||
assert.equal(res.success, false);
|
||||
assert.ok(
|
||||
typeof res.error === "string" && /private|blocked|local/i.test(res.error),
|
||||
`expected guard error, got: ${res.error}`
|
||||
);
|
||||
// A guard-blocked verdict must fail fast — no exponential-backoff retries (1s+2s+4s) for
|
||||
// something that will keep resolving the same way.
|
||||
assert.ok(elapsedMs < 900, `blocked delivery must not retry with backoff (took ${elapsedMs}ms)`);
|
||||
});
|
||||
|
||||
it("blocks when any of several resolved addresses is private (multi-A trick)", async () => {
|
||||
const fetchCalls: string[] = [];
|
||||
const res = await deliverWebhook(
|
||||
"http://multi-answer.example.com/hook",
|
||||
{ event: "test.ping", timestamp: new Date().toISOString(), data: {} },
|
||||
null,
|
||||
0,
|
||||
{
|
||||
lookup: async () => [
|
||||
{ address: "203.0.113.5", family: 4 },
|
||||
{ address: "169.254.169.254", family: 4 },
|
||||
],
|
||||
fetchImpl: async (input: string | URL) => {
|
||||
fetchCalls.push(String(input));
|
||||
return new Response("ok", { status: 200 });
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
assert.equal(res.success, false);
|
||||
assert.equal(fetchCalls.length, 0, "fetch must never fire when any resolved IP is blocked");
|
||||
});
|
||||
|
||||
it("allows a hostname that resolves only to public addresses", async () => {
|
||||
const fetchCalls: string[] = [];
|
||||
const res = await deliverWebhook(
|
||||
"http://public-looking.example.com/hook",
|
||||
{ event: "test.ping", timestamp: new Date().toISOString(), data: {} },
|
||||
null,
|
||||
0,
|
||||
{
|
||||
lookup: async () => [{ address: "203.0.113.5", family: 4 }],
|
||||
fetchImpl: async (input: string | URL) => {
|
||||
fetchCalls.push(String(input));
|
||||
return new Response("ok", { status: 200 });
|
||||
},
|
||||
}
|
||||
);
|
||||
|
||||
assert.equal(res.success, true);
|
||||
assert.equal(fetchCalls.length, 1);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user