From f2869ef76ba973ab3226877db0e36e70593131d0 Mon Sep 17 00:00:00 2001 From: Agnik47 <140933190+Agnik47@users.noreply.github.com> Date: Tue, 11 Aug 2026 22:40:07 +0530 Subject: [PATCH] fix: handle unhandled EPIPE crash in web fetch proxy, and Cloudflare 200 false positive Two independent bugs in web fetch, both hit on a clean install: 1. safe-proxy's CONNECT tunnel handler had no 'error' listener on the client-facing socket at all -- only on the upstream leg. Once either side of an established tunnel resets or half-closes, the missing listener on 'client' turns the resulting error into an unhandled 'error' event, which crashes the whole process. The plain HTTP path had the same gap on request/response. Added persistent, symmetric error handlers on both legs of both paths, wired to destroy the peer instead of leaking the error. Also, proxy.close() only called server.close(), which merely stops accepting new connections -- it does not close a live CONNECT tunnel, so close() would hang indefinitely with one in flight. Now tracks every socket/request it opens and destroys them before closing the server. 2. isChallengeResponse treated bare CDN vendor names ('cloudflare', 'akamai') found anywhere in headers as decisive block evidence on a 200, even though 'server: cloudflare' is present on every response that CDN fronts, blocked or not. Split markers into strong challenge-specific signals (cf-chl, cf-mitigated, datadome, perimeterx, captcha, 'just a moment', ...), which stay decisive at any status including 200, from bare vendor names, which now only corroborate an already-suspicious non-200 status. Both regressions were verified against the original code before the fix (temporarily reverted, confirmed each new test fails) to make sure the new tests actually catch what's described in the issue, not just pass by construction. Fixes #283 --- src/fetch/classify.test.ts | 14 +++++ src/fetch/classify.ts | 15 ++++- src/fetch/safe-proxy.test.ts | 106 ++++++++++++++++++++++++++++++++++- src/fetch/safe-proxy.ts | 44 +++++++++++++-- 4 files changed, 171 insertions(+), 8 deletions(-) diff --git a/src/fetch/classify.test.ts b/src/fetch/classify.test.ts index 7d4f42f7..91bbcfc7 100644 --- a/src/fetch/classify.test.ts +++ b/src/fetch/classify.test.ts @@ -6,5 +6,19 @@ describe('fetch classification', () => { expect(isChallengeResponse(403, { server: 'cloudflare' }, 'Just a moment...')).toBe(true); expect(isChallengeResponse(403, {}, 'forbidden')).toBe(false); }); + it('does not flag an ordinary 200 just because it is served through a CDN (issue #283)', () => { + expect(isChallengeResponse(200, { server: 'cloudflare', 'cf-cache-status': 'HIT', 'content-type': 'text/html' }, 'Example Domain')).toBe(false); + expect(isChallengeResponse(200, { server: 'akamaighost' }, 'ordinary page')).toBe(false); + }); + it('still flags a CDN vendor name as a challenge signal once the status itself looks blocked', () => { + expect(isChallengeResponse(503, { server: 'cloudflare' }, '')).toBe(true); + expect(isChallengeResponse(429, { server: 'akamaighost' }, '')).toBe(true); + }); + it('flags a 200 challenge interstitial via a body marker, without needing a CDN header', () => { + expect(isChallengeResponse(200, {}, 'Just a moment...')).toBe(true); + }); + it('flags a 200 challenge via a specific challenge header even without body markers', () => { + expect(isChallengeResponse(200, { server: 'cloudflare', 'cf-mitigated': 'challenge' }, 'interstitial')).toBe(true); + }); it('recognizes script-heavy app shells', () => expect(isJavaScriptShell('
')).toBe(true)); }); diff --git a/src/fetch/classify.ts b/src/fetch/classify.ts index 4051c13c..1cf5d3fc 100644 --- a/src/fetch/classify.ts +++ b/src/fetch/classify.ts @@ -1,8 +1,19 @@ -const challengeMarkers = /cloudflare|cf-chl|datadome|perimeterx|px-captcha|akamai|captcha|just a moment|verify you are human/i; +// Strings that don't legitimately appear except on an actual challenge/block page — +// decisive on their own, at any status including a plain 200 (e.g. a Cloudflare +// managed challenge can render its interstitial with a 200). +const strongChallengeMarkers = /cf-chl|cf-mitigated|datadome|perimeterx|px-captcha|captcha|just a moment|verify you are human/i; +// Bare CDN/vendor names are not evidence of blocking by themselves — `server: cloudflare` +// (or akamai) shows up on every response those networks front, challenged or not, so +// treating it as decisive on a 200 flags most of the ordinary CDN-fronted web. Only +// corroborate an already-suspicious non-200 status with it. +const cdnVendorMarkers = /cloudflare|akamai/i; export function isChallengeResponse(status: number, headers: Record, body: string): boolean { const evidence = `${Object.entries(headers).map(([key, value]) => `${key}:${value}`).join('\n')}\n${body.slice(0, 20_000)}`; - return challengeMarkers.test(evidence) && (status === 403 || status === 429 || status === 503 || status === 200); + const blockedStatus = status === 403 || status === 429 || status === 503; + if (strongChallengeMarkers.test(evidence)) return blockedStatus || status === 200; + if (cdnVendorMarkers.test(evidence)) return blockedStatus; + return false; } export function isJavaScriptShell(body: string): boolean { diff --git a/src/fetch/safe-proxy.test.ts b/src/fetch/safe-proxy.test.ts index 3d58353e..315703b4 100644 --- a/src/fetch/safe-proxy.test.ts +++ b/src/fetch/safe-proxy.test.ts @@ -1,5 +1,13 @@ import { describe, expect, it } from 'vitest'; -import { isSafeAddress } from './safe-proxy.js'; +import * as net from 'node:net'; +import { spawn } from 'node:child_process'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import * as path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import { createSafeProxy, isSafeAddress } from './safe-proxy.js'; + +const moduleDir = path.dirname(fileURLToPath(import.meta.url)); describe('isSafeAddress', () => { it.each(['127.0.0.1', '10.0.0.1', '172.16.0.1', '192.168.1.1', '169.254.169.254', '0.0.0.0', '::1', '::', 'fe80::1', '::ffff:127.0.0.1'])('rejects private address %s', address => { @@ -7,3 +15,99 @@ describe('isSafeAddress', () => { }); it('allows public IPv4 addresses', () => expect(isSafeAddress('93.184.216.34')).toBe(true)); }); + +describe('createSafeProxy CONNECT tunnel', () => { + async function startFakeUpstream(): Promise<{ port: number; socket: Promise; close(): Promise }> { + let resolveSocket: (socket: net.Socket) => void; + const socket = new Promise((resolve) => { resolveSocket = resolve; }); + const server = net.createServer((s) => { s.resume(); resolveSocket(s); }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as net.AddressInfo; + return { port, socket, close: () => new Promise((resolve) => server.close(() => resolve())) }; + } + + function runNodeScript(scriptPath: string): Promise<{ status: number | null; stdout: string; stderr: string }> { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, ['--import', 'tsx', scriptPath], { stdio: ['ignore', 'pipe', 'pipe'] }); + const stdout: Buffer[] = []; const stderr: Buffer[] = []; + child.stdout.on('data', (chunk) => stdout.push(Buffer.from(chunk))); + child.stderr.on('data', (chunk) => stderr.push(Buffer.from(chunk))); + child.once('error', reject); + child.once('close', (status) => resolve({ status, stdout: Buffer.concat(stdout).toString('utf8'), stderr: Buffer.concat(stderr).toString('utf8') })); + }); + } + + async function openTunnel(proxyUrl: string, targetPort: number): Promise { + const url = new URL(proxyUrl); + const client = net.connect({ host: url.hostname, port: Number(url.port) }); + await new Promise((resolve, reject) => { + client.once('connect', () => client.write(`CONNECT 127.0.0.1:${targetPort} HTTP/1.1\r\nHost: 127.0.0.1:${targetPort}\r\n\r\n`)); + client.once('data', (chunk) => { + expect(chunk.toString()).toContain('200 Connection Established'); + resolve(); + }); + client.once('error', reject); + }); + return client; + } + + // A crashed process kills the whole vitest worker, not just this test, so the + // repro has to run isolated in a real child process to observe pass/fail cleanly. + it('does not crash the process when the client leg resets the connection', async () => { + const safeProxyUrl = pathToFileURL(path.join(moduleDir, 'safe-proxy.ts')).href; + const dir = await mkdtemp(path.join(tmpdir(), 'webcmd-safe-proxy-crash-')); + const scriptPath = path.join(dir, 'repro.mjs'); + await writeFile(scriptPath, [ + "import * as net from 'node:net';", + `import { createSafeProxy } from ${JSON.stringify(safeProxyUrl)};`, + '', + "const fakeUpstream = net.createServer((s) => s.resume());", + "await new Promise((resolve) => fakeUpstream.listen(0, '127.0.0.1', resolve));", + 'const upstreamPort = fakeUpstream.address().port;', + '', + 'const proxy = await createSafeProxy({ allowPrivate: true });', + 'const proxyUrl = new URL(proxy.url);', + 'const client = net.connect({ host: proxyUrl.hostname, port: Number(proxyUrl.port) });', + 'await new Promise((resolve, reject) => {', + " client.once('connect', () => client.write(`CONNECT 127.0.0.1:${upstreamPort} HTTP/1.1\\r\\nHost: 127.0.0.1:${upstreamPort}\\r\\n\\r\\n`));", + " client.once('data', () => resolve());", + " client.once('error', reject);", + '});', + '', + '// A hard RST (not a graceful end()) is what an aborted/timed-out real client', + "// produces, and reliably surfaces as an 'error' event on the proxy's peer socket", + '// -- the exact unhandled error reported in issue #283.', + 'client.resetAndDestroy();', + 'await new Promise((resolve) => setTimeout(resolve, 200));', + '', + 'await proxy.close();', + "await new Promise((resolve) => fakeUpstream.close(() => resolve()));", + 'process.exit(0);', + '', + ].join('\n')); + + const result = await runNodeScript(scriptPath); + await rm(dir, { recursive: true, force: true }); + + expect(result.stderr).not.toContain('ECONNRESET'); + expect(result.status).toBe(0); + }, 15_000); + + it('close() destroys dangling tunnel sockets instead of hanging or leaking them', async () => { + const fakeUpstream = await startFakeUpstream(); + const proxy = await createSafeProxy({ allowPrivate: true }); + + try { + const client = await openTunnel(proxy.url, fakeUpstream.port); + await fakeUpstream.socket; // tunnel is fully live on both legs + + const clientClosed = new Promise((resolve) => client.once('close', () => resolve())); + // server.close() alone waits for every existing connection to end on its own -- + // an established CONNECT tunnel never does that by itself, so pre-fix this hangs. + await proxy.close(); + await clientClosed; + } finally { + await fakeUpstream.close(); + } + }); +}); diff --git a/src/fetch/safe-proxy.ts b/src/fetch/safe-proxy.ts index 302f4710..67f9300b 100644 --- a/src/fetch/safe-proxy.ts +++ b/src/fetch/safe-proxy.ts @@ -35,33 +35,67 @@ async function resolve(host: string, lookup: typeof dnsLookup, allowPrivate: boo return addresses[0]!.address; } +/** + * Sockets that outlive their handler close over the tunnel's lifetime (CONNECT + * tunnels become raw bidirectional pipes; plain requests may be keep-alive), so + * `server.close()` alone — which only stops accepting new connections — leaves + * them dangling. Tracked here so `close()` can force them shut instead of racing + * whatever tears them down next. + */ +type Handle = { destroy(error?: Error): void; once(event: 'close', listener: () => void): unknown }; + export async function createSafeProxy(options: SafeProxyOptions = {}): Promise { const lookup = options.lookup ?? dnsLookup; const allowPrivate = options.allowPrivate === true; + const openHandles = new Set(); + const track = (handle: Handle): void => { + openHandles.add(handle); + handle.once('close', () => openHandles.delete(handle)); + }; + const server = http.createServer(async (request, response) => { + track(request.socket); + let upstream: http.ClientRequest | undefined; + // Registered before any async work: a half-closed peer can error at any point in + // the request lifetime, and an unhandled 'error' event crashes the whole process. + request.on('error', () => { upstream?.destroy(); response.destroy(); }); + response.on('error', () => { upstream?.destroy(); request.destroy(); }); try { const target = new URL(request.url ?? ''); const address = await resolve(target.hostname, lookup, allowPrivate); - const upstream = http.request({ host: address, port: Number(target.port) || 80, method: request.method, path: `${target.pathname}${target.search}`, headers: { ...request.headers, host: target.host } }, upstreamResponse => { + upstream = http.request({ host: address, port: Number(target.port) || 80, method: request.method, path: `${target.pathname}${target.search}`, headers: { ...request.headers, host: target.host } }, upstreamResponse => { response.writeHead(upstreamResponse.statusCode ?? 502, upstreamResponse.headers); upstreamResponse.pipe(response); }); + track(upstream); upstream.on('error', error => response.destroy(error)); request.pipe(upstream); } catch (error) { response.writeHead(403).end(error instanceof Error ? error.message : 'Unsafe fetch destination'); } }); server.on('connect', async (request, client, head) => { + track(client); + let upstream: net.Socket | undefined; + // Same reasoning as above: once the tunnel is piping both ways, either side can + // reset first, and only one direction (upstream's own 'error') was covered before. + client.on('error', () => upstream?.destroy()); try { const [host, portText] = (request.url ?? '').replace(/^\[/, '').replace(']', '').split(':'); if (!host) throw new Error('Invalid CONNECT target'); const address = await resolve(host, lookup, allowPrivate); - const upstream = net.connect({ host: address, port: Number(portText) || 443 }); - upstream.once('connect', () => { client.write('HTTP/1.1 200 Connection Established\r\n\r\n'); if (head.length) upstream.write(head); upstream.pipe(client); client.pipe(upstream); }); - upstream.once('error', error => client.destroy(error)); + upstream = net.connect({ host: address, port: Number(portText) || 443 }); + track(upstream); + upstream.on('error', error => client.destroy(error)); + upstream.once('connect', () => { client.write('HTTP/1.1 200 Connection Established\r\n\r\n'); if (head.length) upstream!.write(head); upstream!.pipe(client); client.pipe(upstream!); }); } catch (error) { client.end(`HTTP/1.1 403 Forbidden\r\n\r\n${error instanceof Error ? error.message : ''}`); } }); await new Promise((resolveListen, reject) => { server.once('error', reject); server.listen(0, '127.0.0.1', () => resolveListen()); }); const address = server.address(); if (!address || typeof address === 'string') throw new Error('Safe proxy did not bind'); - return { url: `http://127.0.0.1:${address.port}`, close: () => new Promise((resolveClose, reject) => server.close(error => error ? reject(error) : resolveClose())) }; + return { + url: `http://127.0.0.1:${address.port}`, + close: () => new Promise((resolveClose, reject) => { + for (const handle of openHandles) handle.destroy(); + server.close(error => error ? reject(error) : resolveClose()); + }), + }; }