From 251e7ab8cb46a1f4904a7fea0e71ba3f19d33434 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Wed, 12 Aug 2026 11:41:40 +0200 Subject: [PATCH 1/2] fix(protect): egress redirect re-screening, redaction case-sensitivity, IPv6-ULA over-match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three output-filtering / egress correctness fixes surfaced by a review of the response + egress phases: - egress: the guarded fetch only screened the initial URL and let native `redirect: follow` follow a 3xx to an internal address — an SSRF-via-open-redirect bypass (302 -> 169.254.169.254 reached the metadata endpoint). Follow redirects ourselves with `redirect: manual`, screening every hop; buffer the body once so 307/308 can replay it, rewrite 303/POST -> GET, and strip Authorization/Cookie on cross-origin hops. Fail open on un-normalizable input. - response redaction: `contains`/`stripos` detection is case-insensitive but the mask was case-sensitive, so a `contains: "SECRET"` rule detected `secret` yet masked nothing — the leak was served while telemetry reported a redaction. Mask case-insensitively. - isInternalHost: the `fc`/`fd` IPv6-ULA prefix check fired on any hostname starting with those letters (e.g. fcm.googleapis.com), blocking legitimate egress in block mode. Require a colon (actual IPv6) before applying it. 600 tests pass (+5 new in tests/protect/correctness-fixes.test.ts); typecheck clean. Co-Authored-By: Claude Opus 4.8 --- src/protect/egress.js | 66 +++++++++++++++--- src/protect/engine/engine.js | 6 +- src/protect/runtime.js | 8 ++- tests/protect/correctness-fixes.test.ts | 89 +++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 tests/protect/correctness-fixes.test.ts diff --git a/src/protect/egress.js b/src/protect/egress.js index 942a2ba..62eb006 100644 --- a/src/protect/egress.js +++ b/src/protect/egress.js @@ -62,20 +62,70 @@ export async function installEgressGuard({ shouldBlock, onBlock, dnsScreen = tru // 1. global fetch — synchronous install, so it's active the instant this returns (no startup race). const originalFetch = globalThis.fetch; if (typeof originalFetch === 'function' && !originalFetch.__patchstackGuarded) { - const guarded = async (input, init) => { - const url = typeof input === 'string' ? input : (input && input.url) || String(input); + const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + const MAX_REDIRECTS = 20; + + // Screen one outbound URL: hostname/allowlist/literal-IP check, then a DNS-resolution check for + // real hostnames. Throws if the destination is disallowed. + const screenUrl = async (u, method) => { let host = null; try { - host = new URL(url).hostname; + host = new URL(u).hostname; } catch { host = null; } - const method = (init && init.method) || (input && input.method) || 'GET'; - // hostname / allowlist / literal-IP check, then a DNS-resolution check for real hostnames. - if (block(url, host, method) || (await resolvesToDisallowed(url, host, method))) { - throw new Error(`Patchstack blocked an outbound request to a disallowed address: ${host ?? url}`); + if (block(u, host, method) || (await resolvesToDisallowed(u, host, method))) { + throw new Error(`Patchstack blocked an outbound request to a disallowed address: ${host ?? u}`); + } + }; + + const guarded = async (input, init) => { + let cur; + try { + cur = new Request(input, { ...(init || {}), redirect: 'manual' }); + } catch { + return originalFetch(input, init); // odd input we can't normalize — fail open, don't break the caller + } + const callerRedirect = (init && init.redirect) || (input && input.redirect) || 'follow'; + + let url = cur.url; + let method = cur.method; + await screenUrl(url, method); + + // Caller manages redirects itself (manual/error) → screen once, hand back the raw response. + if (callerRedirect !== 'follow') return originalFetch(input, init); + + // Otherwise follow redirects ourselves so EVERY hop is screened. Native `follow` re-resolves + // internally and would let a 3xx to an internal address slip past the initial check — SSRF via + // an open redirect. Buffer the body once (a stream can't be re-read) so 307/308 can replay it. + const headers = new Headers(cur.headers); + const signal = cur.signal; + let body = method === 'GET' || method === 'HEAD' ? undefined : await cur.clone().arrayBuffer(); + + for (let hop = 0; ; hop++) { + const resp = await originalFetch( + hop === 0 ? cur : new Request(url, { method, headers, body, redirect: 'manual', signal }), + ); + const location = REDIRECT_STATUSES.has(resp.status) ? resp.headers.get('location') : null; + if (!location) return resp; + if (hop >= MAX_REDIRECTS) throw new Error('Patchstack blocked an outbound request: too many redirects'); + + const next = new URL(location, url).href; + // Fetch redirect semantics: 303, and 301/302 on a POST, become a bodyless GET. + if (resp.status === 303 || ((resp.status === 301 || resp.status === 302) && method === 'POST')) { + method = 'GET'; + body = undefined; + headers.delete('content-type'); + headers.delete('content-length'); + } + // Drop credentials on a cross-origin hop, mirroring the browser. + if (new URL(next).origin !== new URL(url).origin) { + headers.delete('authorization'); + headers.delete('cookie'); + } + await screenUrl(next, method); + url = next; } - return originalFetch(input, init); }; guarded.__patchstackGuarded = true; globalThis.fetch = guarded; diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index 3b17091..141d9df 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -117,7 +117,11 @@ function isInternalHost(hostname) { if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local')) return true; if (host === 'metadata.google.internal') return true; if (host === '::1' || host === '::') return true; - if (host.startsWith('fe80:') || host.startsWith('fc') || host.startsWith('fd')) return true; + if (host.startsWith('fe80:')) return true; + // IPv6 unique-local (fc00::/7) — only when it is actually IPv6 (contains a colon), so ordinary + // hostnames that merely start with fc/fd (e.g. fcm.googleapis.com, fd-cdn.example.net) are not + // misclassified as internal and blocked. + if (host.includes(':') && (host.startsWith('fc') || host.startsWith('fd'))) return true; // Dotted IPv4 (incl. dotted IPv4-mapped `::ffff:127.0.0.1`, which ends in dotted form). const v4 = host.match(/(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); diff --git a/src/protect/runtime.js b/src/protect/runtime.js index 3240098..202eb59 100644 --- a/src/protect/runtime.js +++ b/src/protect/runtime.js @@ -673,7 +673,13 @@ function applyRedactors(body, redactors, mask, transform) { let out = body; for (const r of redactors) { if (r.re) out = out.replace(r.re, transform ? (m) => transform(m) : mask); - else if (r.literal) out = out.split(r.literal).join(transform ? transform(r.literal) : mask); + else if (r.literal) { + // Detection (matchValue for contains/stripos) is case-insensitive, so mask case-insensitively + // too — otherwise a `contains: "SECRET"` redactor detects `secret` but masks nothing, serving + // the leak while reporting a redaction. Escape the literal so it matches literally, not as regex. + const re = new RegExp(r.literal.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'gi'); + out = out.replace(re, (m) => (transform ? transform(m) : mask)); + } } return out; } diff --git a/tests/protect/correctness-fixes.test.ts b/tests/protect/correctness-fixes.test.ts new file mode 100644 index 0000000..6c2cced --- /dev/null +++ b/tests/protect/correctness-fixes.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from 'vitest'; +import { createProtection } from '../../src/protect/runtime.js'; +import { _testExports } from '../../src/protect/engine/engine.js'; + +const { isInternalHost } = _testExports as { isInternalHost: (h: string) => boolean }; + +// Regression tests for three output-filtering / egress correctness fixes. + +describe('isInternalHost — IPv6 ULA (fc/fd) must not over-match hostnames', () => { + it('classifies real IPv6 unique-local addresses as internal', () => { + expect(isInternalHost('fc00::1')).toBe(true); + expect(isInternalHost('fd12:3456:789a:1::1')).toBe(true); + expect(isInternalHost('fe80::1')).toBe(true); + }); + + it('does NOT classify ordinary hostnames that merely start with fc/fd as internal', () => { + expect(isInternalHost('fcm.googleapis.com')).toBe(false); // Firebase Cloud Messaging + expect(isInternalHost('fd-cdn.example.net')).toBe(false); + expect(isInternalHost('fastly.example.com')).toBe(false); + }); +}); + +describe('response redaction — contains/stripos must mask case-insensitively', () => { + it('masks a lowercase leak matched by an upper-case contains rule', async () => { + const rule = { + phase: 'response', + category: 'x', + action: 'redact', + rule_v2: [{ parameter: 'response.body', match: { type: 'contains', value: 'SECRET' } }], + }; + const p: any = await createProtection({ + rules: { firewall: [], whitelists: [], whitelist_keys: {} }, + responseRules: [rule], + mode: 'block', + }); + const out = await p.screenResponse( + new Response('leaked secret value', { status: 200, headers: { 'content-type': 'text/plain' } }), + ); + const text = await out.text(); + // Detection is case-insensitive; before the fix the mask was case-sensitive, so the lowercase + // "secret" was reported redacted but served in the clear. + expect(/secret/i.test(text)).toBe(false); + expect(text).not.toBe('leaked secret value'); + }); +}); + +describe('egress fetch — redirects are re-screened', () => { + it('blocks a 3xx whose Location points at an internal host', async () => { + const origFetch = globalThis.fetch; + globalThis.fetch = (async (input: any) => { + const url = typeof input === 'string' ? input : input.url; + if (url.includes('169.254.169.254')) return new Response('metadata'); // must never be reached + return new Response(null, { + status: 302, + headers: { location: 'http://169.254.169.254/latest/meta-data/' }, + }); + }) as any; + const p: any = await createProtection({ egress: true, mode: 'block', allowHosts: [] }); + try { + let blocked = false; + try { + await globalThis.fetch('http://93.184.216.34/'); // allowed external IP → 302 → internal + } catch (e) { + blocked = /Patchstack blocked/.test(String(e)); + } + expect(blocked).toBe(true); + } finally { + p.uninstallEgress?.(); + globalThis.fetch = origFetch; + } + }); + + it('follows a redirect to an allowed host and returns the final response', async () => { + const origFetch = globalThis.fetch; + globalThis.fetch = (async (input: any) => { + const url = typeof input === 'string' ? input : input.url; + if (url.includes('/final')) return new Response('final-body'); + return new Response(null, { status: 302, headers: { location: 'http://93.184.216.34/final' } }); + }) as any; + const p: any = await createProtection({ egress: true, mode: 'block', allowHosts: [] }); + try { + const res = await globalThis.fetch('http://93.184.216.34/start'); + expect(await res.text()).toBe('final-body'); + } finally { + p.uninstallEgress?.(); + globalThis.fetch = origFetch; + } + }); +}); From 157650993e74540f9bee9283627d88863647ab09 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Wed, 12 Aug 2026 13:01:04 +0200 Subject: [PATCH 2/2] test(protect): deeper coverage for the egress-redirect / redaction fixes Add edge cases for the correctness fixes: redirect method/body semantics (307 preserves method+body, 303 rewrites to a bodyless GET), cross-origin Authorization stripping, the max-redirects cap, caller redirect:'manual' passthrough (not followed), uppercase/bracketed IPv6-ULA classification, and case-insensitive redaction of a secret in a response header as well as the body. 13 tests in the file; full suite green. Co-Authored-By: Claude Opus 4.8 --- tests/protect/correctness-fixes.test.ts | 118 ++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/tests/protect/correctness-fixes.test.ts b/tests/protect/correctness-fixes.test.ts index 6c2cced..d01b2c8 100644 --- a/tests/protect/correctness-fixes.test.ts +++ b/tests/protect/correctness-fixes.test.ts @@ -87,3 +87,121 @@ describe('egress fetch — redirects are re-screened', () => { } }); }); + +describe('egress fetch — redirect semantics', () => { + const withStub = async (stub: (req: Request) => Response | Promise, fn: () => Promise) => { + const origFetch = globalThis.fetch; + globalThis.fetch = (async (input: any, init?: any) => stub(new Request(input, init))) as any; + const p: any = await createProtection({ egress: true, mode: 'block', allowHosts: [] }); + try { + await fn(); + } finally { + p.uninstallEgress?.(); + globalThis.fetch = origFetch; + } + }; + + it('preserves method and body across a 307 redirect', async () => { + const seen: Array<{ url: string; method: string; body: string | null }> = []; + await withStub( + async (req) => { + const body = req.method === 'GET' || req.method === 'HEAD' ? null : await req.clone().text().catch(() => null); + seen.push({ url: req.url, method: req.method, body }); + if (req.url.includes('/final')) return new Response('ok'); + return new Response(null, { status: 307, headers: { location: 'http://93.184.216.34/final' } }); + }, + async () => { + await globalThis.fetch('http://93.184.216.34/start', { method: 'POST', body: 'payload' }); + const final = seen.find((s) => s.url.includes('/final'))!; + expect(final.method).toBe('POST'); + expect(final.body).toBe('payload'); + }, + ); + }); + + it('rewrites a 303 redirect to a bodyless GET', async () => { + const seen: Array<{ url: string; method: string; body: string | null }> = []; + await withStub( + async (req) => { + const body = req.method === 'GET' || req.method === 'HEAD' ? null : await req.clone().text().catch(() => null); + seen.push({ url: req.url, method: req.method, body }); + if (req.url.includes('/final')) return new Response('ok'); + return new Response(null, { status: 303, headers: { location: 'http://93.184.216.34/final' } }); + }, + async () => { + await globalThis.fetch('http://93.184.216.34/start', { method: 'POST', body: 'payload' }); + const final = seen.find((s) => s.url.includes('/final'))!; + expect(final.method).toBe('GET'); + expect(final.body).toBeNull(); + }, + ); + }); + + it('strips Authorization on a cross-origin redirect hop', async () => { + const seen: Array<{ url: string; auth: string | null }> = []; + await withStub( + async (req) => { + seen.push({ url: req.url, auth: req.headers.get('authorization') }); + if (req.url.includes('216.35')) return new Response('ok'); // cross-origin target + return new Response(null, { status: 302, headers: { location: 'http://93.184.216.35/x' } }); + }, + async () => { + await globalThis.fetch('http://93.184.216.34/start', { headers: { authorization: 'Bearer t0ken' } }); + expect(seen.find((s) => s.url.includes('216.34'))!.auth).toBe('Bearer t0ken'); // kept on initial + expect(seen.find((s) => s.url.includes('216.35'))!.auth).toBeNull(); // stripped cross-origin + }, + ); + }); + + it('throws after too many redirects', async () => { + await withStub( + async () => new Response(null, { status: 302, headers: { location: 'http://93.184.216.34/loop' } }), + async () => { + await expect(globalThis.fetch('http://93.184.216.34/start')).rejects.toThrow(/too many redirects/); + }, + ); + }); + + it('does not follow redirects when the caller sets redirect: manual', async () => { + await withStub( + async () => new Response(null, { status: 302, headers: { location: 'http://169.254.169.254/' } }), + async () => { + const res = await globalThis.fetch('http://93.184.216.34/start', { redirect: 'manual' }); + expect(res.status).toBe(302); // handed back raw; internal Location NOT followed/screened + }, + ); + }); +}); + +describe('isInternalHost + redaction — extra edge cases', () => { + it('classifies uppercase / bracketed IPv6 ULA as internal', () => { + expect(isInternalHost('FC00::1')).toBe(true); + expect(isInternalHost('[fd00::1]')).toBe(true); + }); + + it('does not misclassify fe/fd hostnames', () => { + expect(isInternalHost('fedex.example.com')).toBe(false); + expect(isInternalHost('fd00shop.example.com')).toBe(false); + }); + + it('masks a case-insensitively matched secret in the body AND a response header', async () => { + const rule = { + phase: 'response', + category: 'x', + action: 'redact', + rule_v2: [{ parameter: 'response.body', match: { type: 'contains', value: 'TOPSECRET' } }], + }; + const p: any = await createProtection({ + rules: { firewall: [], whitelists: [], whitelist_keys: {} }, + responseRules: [rule], + mode: 'block', + }); + const resp = new Response('topsecret in body', { + status: 200, + headers: { 'content-type': 'text/plain', 'x-leak': 'topsecret in header' }, + }); + const out = await p.screenResponse(resp); + expect((await out.text()).includes('topsecret')).toBe(false); + expect(out.headers.get('x-leak')?.includes('topsecret')).toBe(false); + }); +});