Skip to content

Commit d673bca

Browse files
committed
fix(daemon): contain browser tunnel teardown errors
1 parent 3b186fa commit d673bca

3 files changed

Lines changed: 179 additions & 9 deletions

File tree

apps/daemon/src/browser-network-proxy.ts

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ async function proxyConnect(
128128
policy: BrowserNetworkPolicy,
129129
tunnels: Set<Duplex>,
130130
): Promise<void> {
131+
const stopPendingClientError = guardPendingClient(client);
131132
try {
132133
const authority = request.url ?? '';
133134
const target = await resolveBrowserNetworkTarget(`https://${authority}`, policy);
@@ -136,14 +137,20 @@ async function proxyConnect(
136137
host: target.address,
137138
port: target.url.port ? Number(target.url.port) : 443,
138139
});
139-
trackTunnel(client, upstream, tunnels);
140+
let connected = false;
141+
trackBrowserNetworkTunnel(client, upstream, tunnels, (error) => {
142+
if (connected) return false;
143+
writeProxyFailure(client, 502, proxyErrorMessage(error));
144+
return true;
145+
});
146+
stopPendingClientError();
140147
upstream.once('connect', () => {
148+
connected = true;
141149
client.write('HTTP/1.1 200 Connection Established\r\n\r\n');
142150
if (head.length > 0) upstream.write(head);
143151
client.pipe(upstream);
144152
upstream.pipe(client);
145153
});
146-
upstream.once('error', (error) => writeProxyFailure(client, 502, proxyErrorMessage(error)));
147154
} catch (error) {
148155
writeProxyFailure(client, 403, `browser proxy blocked tunnel: ${proxyErrorMessage(error)}`);
149156
}
@@ -156,15 +163,23 @@ async function proxyUpgrade(
156163
policy: BrowserNetworkPolicy,
157164
tunnels: Set<Duplex>,
158165
): Promise<void> {
166+
const stopPendingClientError = guardPendingClient(client);
159167
try {
160168
const target = await resolveBrowserNetworkTarget(requestUrl(request).href, policy);
161169
const upstream = net.connect({
162170
family: target.family,
163171
host: target.address,
164172
port: target.url.port ? Number(target.url.port) : 80,
165173
});
166-
trackTunnel(client, upstream, tunnels);
174+
let connected = false;
175+
trackBrowserNetworkTunnel(client, upstream, tunnels, (error) => {
176+
if (connected) return false;
177+
writeProxyFailure(client, 502, proxyErrorMessage(error));
178+
return true;
179+
});
180+
stopPendingClientError();
167181
upstream.once('connect', () => {
182+
connected = true;
168183
const headers = upstreamHeaders(request, target.url.host);
169184
const serialized = Object.entries(headers)
170185
.filter(([, value]) => value != null)
@@ -178,19 +193,67 @@ async function proxyUpgrade(
178193
client.pipe(upstream);
179194
upstream.pipe(client);
180195
});
181-
upstream.once('error', (error) => writeProxyFailure(client, 502, proxyErrorMessage(error)));
182196
} catch (error) {
183197
writeProxyFailure(client, 403, `browser proxy blocked upgrade: ${proxyErrorMessage(error)}`);
184198
}
185199
}
186200

187-
function trackTunnel(client: Duplex, upstream: Duplex, tunnels: Set<Duplex>): void {
201+
function guardPendingClient(client: Duplex): () => void {
202+
const onError = () => client.destroy();
203+
const stop = () => client.off('error', onError);
204+
client.on('error', onError);
205+
client.once('close', stop);
206+
return stop;
207+
}
208+
209+
/**
210+
* A CONNECT/upgrade tunnel is one lifecycle even though Node exposes two
211+
* sockets. `pipe()` deliberately does not forward source errors, so both
212+
* endpoints need their own guard and either endpoint closing must tear down
213+
* its peer. Otherwise a normal Chrome TLS shutdown can surface EPIPE as an
214+
* uncaught daemon exception or leave a half-open upstream behind.
215+
*/
216+
export function trackBrowserNetworkTunnel(
217+
client: Duplex,
218+
upstream: Duplex,
219+
tunnels: Set<Duplex>,
220+
handleUpstreamConnectError?: (error: Error) => boolean,
221+
): void {
188222
tunnels.add(client);
189223
tunnels.add(upstream);
190-
const forget = () => {
224+
225+
let clientClosed = false;
226+
let upstreamClosed = false;
227+
const destroyBoth = () => {
228+
if (!client.destroyed) client.destroy();
229+
if (!upstream.destroyed) upstream.destroy();
230+
};
231+
const forgetWhenClosed = () => {
232+
if (!clientClosed || !upstreamClosed) return;
191233
tunnels.delete(client);
192234
tunnels.delete(upstream);
235+
client.off('error', onClientError);
236+
upstream.off('error', onUpstreamError);
237+
};
238+
const onClientError = () => destroyBoth();
239+
const onUpstreamError = (error: Error) => {
240+
const responseOwnsClient = handleUpstreamConnectError?.(error) ?? false;
241+
if (!upstream.destroyed) upstream.destroy();
242+
if (!responseOwnsClient && !client.destroyed) client.destroy();
193243
};
194-
client.once('close', forget);
195-
upstream.once('close', forget);
244+
const onClientClose = () => {
245+
clientClosed = true;
246+
if (!upstream.destroyed) upstream.destroy();
247+
forgetWhenClosed();
248+
};
249+
const onUpstreamClose = () => {
250+
upstreamClosed = true;
251+
if (!client.destroyed) client.destroy();
252+
forgetWhenClosed();
253+
};
254+
255+
client.on('error', onClientError);
256+
upstream.on('error', onUpstreamError);
257+
client.once('close', onClientClose);
258+
upstream.once('close', onUpstreamClose);
196259
}

apps/daemon/tests/browser-session-security.test.ts

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
11
import type { ChildProcess } from 'node:child_process';
22
import { EventEmitter } from 'node:events';
33
import { createServer, request } from 'node:http';
4+
import { createConnection, createServer as createTcpServer, type Socket } from 'node:net';
5+
import { PassThrough, type Duplex } from 'node:stream';
46

57
import { describe, expect, it, vi } from 'vitest';
68

79
import { WEB_CLONE_CDP_METHODS } from '../src/browser-cdp.js';
8-
import { createBrowserNetworkProxy } from '../src/browser-network-proxy.js';
10+
import {
11+
createBrowserNetworkProxy,
12+
trackBrowserNetworkTunnel,
13+
} from '../src/browser-network-proxy.js';
914
import { assertBrowserNetworkUrl, type BrowserDnsLookup } from '../src/browser-network-policy.js';
1015
import { removeBrowserProfile, terminateBrowserProcess } from '../src/browser-sessions.js';
1116

@@ -57,6 +62,59 @@ describe('Website Clone browser broker security boundary', () => {
5762
}
5863
});
5964

65+
it.each(['client', 'upstream'] as const)(
66+
'contains a normal HTTPS tunnel EPIPE from the %s endpoint and tears down its peer',
67+
async (source) => {
68+
const client = new PassThrough();
69+
const upstream = new PassThrough();
70+
const tunnels = new Set<Duplex>();
71+
const clientClosed = new Promise<void>((resolve) => client.once('close', () => resolve()));
72+
const upstreamClosed = new Promise<void>((resolve) => upstream.once('close', () => resolve()));
73+
74+
trackBrowserNetworkTunnel(client, upstream, tunnels);
75+
const socketError = Object.assign(new Error('normal HTTPS tunnel teardown'), { code: 'EPIPE' });
76+
expect(() => (source === 'client' ? client : upstream).emit('error', socketError)).not.toThrow();
77+
78+
await Promise.all([clientClosed, upstreamClosed]);
79+
expect(client.destroyed).toBe(true);
80+
expect(upstream.destroyed).toBe(true);
81+
expect(tunnels.size).toBe(0);
82+
},
83+
);
84+
85+
it('closes an HTTPS CONNECT peer and keeps the proxy available for the next browser tunnel', async () => {
86+
const targetSockets = new Set<Socket>();
87+
const target = createTcpServer((socket) => {
88+
targetSockets.add(socket);
89+
socket.on('error', () => undefined);
90+
socket.once('close', () => targetSockets.delete(socket));
91+
});
92+
await new Promise<void>((resolve) => target.listen(0, '127.0.0.1', resolve));
93+
const targetAddress = target.address();
94+
if (!targetAddress || typeof targetAddress === 'string') throw new Error('target did not bind');
95+
const proxy = await createBrowserNetworkProxy({ allowPrivateNetwork: true });
96+
97+
try {
98+
const first = await connectTunnel(proxy.port, targetAddress.port);
99+
first.on('error', () => undefined);
100+
await waitFor(() => targetSockets.size === 1);
101+
first.destroy();
102+
await waitFor(() => targetSockets.size === 0);
103+
104+
// A daemon-fatal uncaughtException would prevent this follow-up tunnel,
105+
// which represents the next browser session using the same broker.
106+
const second = await connectTunnel(proxy.port, targetAddress.port);
107+
second.on('error', () => undefined);
108+
expect(second.destroyed).toBe(false);
109+
second.destroy();
110+
await waitFor(() => targetSockets.size === 0);
111+
} finally {
112+
await proxy.close();
113+
for (const socket of targetSockets) socket.destroy();
114+
await new Promise<void>((resolve) => target.close(() => resolve()));
115+
}
116+
});
117+
60118
it('exposes only the CDP methods required by the staged recon adapter', () => {
61119
expect(WEB_CLONE_CDP_METHODS).toEqual(new Set([
62120
'Emulation.setDeviceMetricsOverride',
@@ -96,6 +154,39 @@ async function requestThroughProxy(proxyPort: number, url: string): Promise<{ bo
96154
});
97155
}
98156

157+
async function connectTunnel(proxyPort: number, targetPort: number): Promise<Socket> {
158+
return new Promise((resolve, reject) => {
159+
const socket = createConnection({ host: '127.0.0.1', port: proxyPort });
160+
let response = '';
161+
const onError = (error: Error) => reject(error);
162+
const onData = (chunk: Buffer) => {
163+
response += chunk.toString('latin1');
164+
if (!response.includes('\r\n\r\n')) return;
165+
socket.off('data', onData);
166+
socket.off('error', onError);
167+
if (!response.startsWith('HTTP/1.1 200')) {
168+
reject(new Error(`CONNECT failed: ${response}`));
169+
socket.destroy();
170+
return;
171+
}
172+
resolve(socket);
173+
};
174+
socket.once('error', onError);
175+
socket.on('data', onData);
176+
socket.once('connect', () => {
177+
socket.write(`CONNECT 127.0.0.1:${targetPort} HTTP/1.1\r\nHost: 127.0.0.1:${targetPort}\r\n\r\n`);
178+
});
179+
});
180+
}
181+
182+
async function waitFor(condition: () => boolean, timeoutMs = 1_000): Promise<void> {
183+
const startedAt = Date.now();
184+
while (!condition()) {
185+
if (Date.now() - startedAt >= timeoutMs) throw new Error('condition timed out');
186+
await new Promise((resolve) => setTimeout(resolve, 10));
187+
}
188+
}
189+
99190
describe('Website Clone browser process cleanup', () => {
100191
it('waits for a SIGTERM-resistant browser to exit after SIGKILL before deleting its profile', async () => {
101192
const child = new EventEmitter() as EventEmitter & {

e2e/specs/web-clone/main.spec.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,24 @@ describe('Website Clone main path', () => {
9898
);
9999
expect(closed.closed).toBe(true);
100100

101+
// Paired with the CONNECT teardown regression in the daemon suite, pin
102+
// that the daemon-wide broker can serve the next Website Clone session.
103+
const recreated = await requestJson<BrowserSessionResponse>(
104+
webUrl,
105+
`/api/projects/${encodeURIComponent(project.project.id)}/browser-sessions`,
106+
{ body: {} },
107+
);
108+
expect(recreated.browserSession.id).not.toBe(created.browserSession.id);
109+
const reclosed = await requestJson<{ closed: boolean }>(
110+
webUrl,
111+
`/api/projects/${encodeURIComponent(project.project.id)}/browser-sessions/${encodeURIComponent(recreated.browserSession.id)}`,
112+
{ method: 'DELETE' },
113+
);
114+
expect(reclosed.closed).toBe(true);
115+
101116
await suite.report.json('summary.json', {
102117
browserBroker: created.browserSession,
118+
browserBrokerRecreated: recreated.browserSession,
103119
electronRequired: false,
104120
playwrightInstalledInProject: false,
105121
projectId: project.project.id,

0 commit comments

Comments
 (0)