Commit6088927
fix(security): webhook SSRF guard was bypassable by one redirect
fix(security): webhook SSRF guard was bypassable by one redirect Two independent holes in the guard that webhook delivery relies on. 1. Redirects. `fetch()` defaults to `redirect: "follow"`, and assertPublicUrl only ever saw the URL the user registered. A genuinely public endpoint could answer 302 -> http://169.254.169.254/latest/meta-data/ (or any RFC1918 host) and fetch would obediently follow it, POSTing the signed payload at internal infrastructure and returning nothing to the attacker but reaching it all the same. One redirect defeated the whole guard. 2. The DNS layer was written but never wired. resolvesToPrivate() — which catches a public-looking hostname pointing at a private address — was fully implemented and unit-tested, yet had ZERO production callers; only src/__tests__/ssrf-guard.test.ts imported it. `evil.example.com A 127.0.0.1` sailed through the synchronous literal check. Deliveries now go through deliverFollowingRedirects(), which sets redirect: "manual" and re-runs BOTH checks on every hop, bounded at 3. Redirects are followed rather than refused because http->https upgrades are legitimate and common for webhook endpoints; refusing outright would break working integrations. Relative Locations resolve against the current hop. The DNS check honours the same SSRF_ALLOW_PRIVATE / test-env escape hatch assertPublicUrl uses. The first version of this patch did not, and the suite caught it: it blocked 127.0.0.1 in the test environment, where suites legitimately point webhooks at a local Bun.serve(). Residual risk, deliberately not solved: DNS rebinding. Resolve and connect are separate operations, so a hostname can return a public address to our lookup and a private one to the socket. Closing that needs connect-time IP pinning, which Bun's fetch does not expose. This moves the bar from "trivially bypassable with a redirect" to "requires an active rebinding attack". Verification is structural plus the existing ssrf-guard suite. A functional test would have to bind a redirecting localhost server, but the guard default-allows private addresses under the test env, so the harness would need to disable the behaviour under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed+167−36088927482f3b1fbb7f382784dffd908dfef027d
2 changed files+167−3
Addedsrc/__tests__/webhook-ssrf-redirect.test.ts+94−0View fileUnifiedSplit
@@ -0,0 +1,94 @@
1/**
2 * Webhook delivery SSRF: redirect hops and the DNS layer.
3 *
4 * Two independent bypasses of the guard that webhook delivery relies on:
5 *
6 * 1. `fetch()` defaults to `redirect: "follow"`. assertPublicUrl only ever saw
7 * the URL the user registered, so a genuinely public endpoint could answer
8 * 302 → http://169.254.169.254/latest/meta-data/ and fetch would follow it.
9 * One redirect defeated the entire guard.
10 *
11 * 2. resolvesToPrivate() — the DNS layer that catches a public-looking
12 * hostname pointing at a private address — existed, was fully unit-tested,
13 * and had ZERO production callers. Only src/__tests__/ssrf-guard.test.ts
14 * imported it. `evil.example.com A 127.0.0.1` passed the literal check.
15 *
16 * These assertions are structural. A functional test would need to bind a
17 * redirecting server on localhost, but the guard deliberately default-allows
18 * private addresses under the test env, so the harness would have to disable
19 * the very behaviour under test. The guard's own address-classification logic
20 * is already covered functionally by ssrf-guard.test.ts.
21 */
22
23import { describe, expect, it } from "bun:test";
24import { readFileSync } from "fs";
25
26const SRC = readFileSync("src/lib/webhook-delivery.ts", "utf8");
27const fn = SRC.slice(
28 SRC.indexOf("async function deliverFollowingRedirects"),
29 SRC.indexOf("// ---", SRC.indexOf("async function deliverFollowingRedirects"))
30);
31
32describe("redirects cannot escape the guard", () => {
33 it("never lets fetch follow redirects itself", () => {
34 expect(fn).toContain('redirect: "manual"');
35 });
36
37 it("no bare fetch(row.url) with default redirect handling remains", () => {
38 // The original call site. If this reappears the guard is bypassable again.
39 expect(SRC).not.toMatch(/await fetch\(row\.url, \{\s*\n\s*method: "POST"/);
40 });
41
42 it("re-validates the URL on every hop, not just the first", () => {
43 // assertPublicUrl must be inside the loop.
44 const loopStart = fn.indexOf("for (let hop");
45 const guardCall = fn.indexOf("assertPublicUrl(url)");
46 expect(loopStart).toBeGreaterThan(-1);
47 expect(guardCall).toBeGreaterThan(loopStart);
48 });
49
50 it("bounds the hop count so a redirect cycle cannot spin", () => {
51 expect(SRC).toContain("MAX_REDIRECTS");
52 expect(fn).toContain("hop <= MAX_REDIRECTS");
53 expect(fn).toContain("more than ${MAX_REDIRECTS} redirects");
54 });
55
56 it("resolves a relative Location against the current hop", () => {
57 // `Location: /latest/meta-data/` on a redirect must not be treated as
58 // an absolute URL or silently dropped.
59 expect(fn).toContain("new URL(location, url)");
60 });
61
62 it("treats a 3xx with no Location as a terminal response", () => {
63 expect(fn).toContain("if (!location) return { status: res.status }");
64 });
65});
66
67describe("the DNS layer is actually wired in", () => {
68 it("webhook delivery calls resolvesToPrivate", () => {
69 // It had zero production callers before this.
70 expect(SRC).toContain("resolvesToPrivate");
71 expect(fn).toContain("await resolvesToPrivate(guard.url.hostname)");
72 });
73
74 it("checks DNS before issuing the request", () => {
75 const dns = fn.indexOf("resolvesToPrivate");
76 const req = fn.indexOf("await fetch(url");
77 expect(dns).toBeLessThan(req);
78 });
79
80 it("honours the same allow-private escape hatch as assertPublicUrl", () => {
81 // Without this the DNS layer blocks 127.0.0.1 under SSRF_ALLOW_PRIVATE=1
82 // and in the test env, breaking suites that point webhooks at a local
83 // Bun.serve() — which is exactly what it did on the first attempt.
84 expect(fn).toContain("ssrfPrivateAllowed()");
85 expect(fn).toContain("enforcing &&");
86 });
87});
88
89describe("blocked deliveries are reported as blocked", () => {
90 it("surfaces an SSRF block distinctly from an HTTP error", () => {
91 expect(SRC).toContain("blockedReason");
92 expect(SRC).toContain("(SSRF protection)");
93 });
94});
Modifiedsrc/lib/webhook-delivery.ts+73−3View fileUnifiedSplit
@@ -67,6 +67,75 @@ const DELIVERY_TIMEOUT_MS = 10_000;
6767/** Cap on stored last_error string. */
6868const ERROR_CAP = 2_000;
6969
70/** How many redirect hops a webhook delivery may follow. */
71const MAX_REDIRECTS = 3;
72
73/**
74 * POST to a user-supplied webhook URL, re-running the SSRF guard on every
75 * redirect hop.
76 *
77 * Two holes this closes:
78 *
79 * 1. `fetch()` defaults to `redirect: "follow"`. assertPublicUrl only ever saw
80 * the URL the user registered, so a perfectly public endpoint could answer
81 * 302 → http://169.254.169.254/latest/meta-data/ (or any RFC1918 host) and
82 * fetch would obediently follow it. The guard was bypassed entirely by one
83 * redirect.
84 *
85 * 2. assertPublicUrl is a synchronous literal check. resolvesToPrivate() — the
86 * DNS layer that catches a public-looking hostname pointing at a private
87 * address — existed, was fully unit-tested, and had ZERO production
88 * callers: only the test file imported it. `evil.example.com A 127.0.0.1`
89 * sailed through.
90 *
91 * Redirects are followed rather than refused because http→https upgrades are
92 * a legitimate and common webhook endpoint behaviour; refusing them outright
93 * would break working integrations.
94 *
95 * Residual risk, deliberately not solved here: DNS rebinding. The resolve and
96 * the connect are separate operations, so a hostname can return a public
97 * address to our lookup and a private one to the socket. Eliminating that
98 * needs connect-time pinning (resolve, validate, then dial the literal IP with
99 * a Host header), which Bun's fetch does not expose. This raises the bar from
100 * "trivially bypassable with a redirect" to "requires an active rebinding
101 * attack".
102 */
103async function deliverFollowingRedirects(
104 startUrl: string,
105 init: { method: string; headers: Record<string, string>; body: string }
106): Promise<{ status: number; blockedReason?: string }> {
107 const { assertPublicUrl, resolvesToPrivate, ssrfPrivateAllowed } =
108 await import("./ssrf-guard");
109
110 // Honour the same escape hatch assertPublicUrl uses. Without this the DNS
111 // layer would still block 127.0.0.1 under SSRF_ALLOW_PRIVATE=1 and in the
112 // test env, where suites legitimately point webhooks at a local Bun.serve().
113 const enforcing = !ssrfPrivateAllowed();
114
115 let url = startUrl;
116 for (let hop = 0; hop <= MAX_REDIRECTS; hop++) {
117 const guard = assertPublicUrl(url);
118 if (!guard.ok) return { status: 0, blockedReason: guard.reason };
119 if (enforcing && (await resolvesToPrivate(guard.url.hostname))) {
120 return { status: 0, blockedReason: "hostname resolves to a private address" };
121 }
122
123 const res = await fetch(url, {
124 ...init,
125 redirect: "manual",
126 signal: AbortSignal.timeout(DELIVERY_TIMEOUT_MS),
127 });
128
129 if (res.status < 300 || res.status >= 400) return { status: res.status };
130
131 const location = res.headers.get("location");
132 if (!location) return { status: res.status };
133 // Resolve relative Locations against the current hop.
134 url = new URL(location, url).toString();
135 }
136 return { status: 0, blockedReason: `more than ${MAX_REDIRECTS} redirects` };
137}
138
70139// ---------------------------------------------------------------------------
71140// Signing
72141// ---------------------------------------------------------------------------
@@ -268,16 +337,17 @@ export async function attemptDelivery(
268337 };
269338 if (d.signature) headers["X-Gluecron-Signature"] = d.signature;
270339
271 const res = await fetch(row.url, {
340 const res = await deliverFollowingRedirects(row.url, {
272341 method: "POST",
273342 headers,
274343 body: d.payload,
275 signal: AbortSignal.timeout(DELIVERY_TIMEOUT_MS),
276344 });
277345 statusCode = res.status;
278346 success = res.status >= 200 && res.status < 300;
279347 if (!success) {
280 errorMessage = `HTTP ${res.status}`;
348 errorMessage = res.blockedReason
349 ? `blocked: ${res.blockedReason} (SSRF protection)`
350 : `HTTP ${res.status}`;
281351 }
282352 } catch (err) {
283353 errorMessage =
284354