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()); + }), + }; }