Skip to content

Commit 845f5e0

Browse files
committed
fix(dev-middleware): don't let one bad socket kill the dev server
InspectorProxy never attached an 'error' listener to the WebSocket connections it accepts. In Node an 'error' event with no listener throws, so a socket-level failure on a single connection took down the whole dev server rather than just that connection. Both #createDeviceConnectionWSServer and #createDebuggerConnectionWSServer listened for 'message' and 'close' only. They now also listen for 'error', attached synchronously before the first await — these handlers are async, so an error arriving while suspended would otherwise still be unhandled. Reported in #57793, where a normally-connected iOS simulator tripped the maxFragments cap in the vendored ws and exited Metro with 'RangeError: Too many message fragments' (close code 1008). That cap is deliberate hardening and is untouched here; what is fixed is that tripping it was fatal to the process instead of to the connection.
1 parent 63e9c15 commit 845f5e0

2 files changed

Lines changed: 132 additions & 0 deletions

File tree

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @flow strict-local
8+
* @format
9+
*/
10+
11+
import {fetchJson} from './FetchUtils';
12+
import {createDeviceMock} from './InspectorDeviceUtils';
13+
import {withAbortSignalForEachTest} from './ResourceUtils';
14+
import {withServerForEachTest} from './ServerUtils';
15+
import until from 'wait-for-expect';
16+
import WS from 'ws';
17+
18+
// WebSocket is unreliable when using fake timers.
19+
jest.useRealTimers();
20+
21+
jest.setTimeout(10000);
22+
23+
/**
24+
* A socket-level failure on one connection must not be fatal to the proxy.
25+
*
26+
* Without an 'error' listener on the accepted socket, Node throws on the
27+
* unhandled 'error' event and the whole dev server exits — so a single
28+
* misbehaving peer can end everyone's session. Reported as #57793, where a
29+
* normally-connected iOS simulator tripped the `maxFragments` cap in the
30+
* vendored `ws` and took Metro down with it.
31+
*/
32+
describe('inspector proxy socket errors', () => {
33+
const serverRef = withServerForEachTest({
34+
logger: undefined,
35+
secure: false,
36+
});
37+
const autoCleanup = withAbortSignalForEachTest();
38+
39+
afterEach(() => {
40+
jest.clearAllMocks();
41+
});
42+
43+
test.each([
44+
['device', '/inspector/device?device=badDevice&name=foo&app=bar'],
45+
['debugger', '/inspector/debug?device=badDevice&page=1'],
46+
])(
47+
'a protocol error on a %s connection does not take down the proxy',
48+
async (_role, path) => {
49+
// A well-behaved device, so there is a live session to lose.
50+
const device = await createDeviceMock(
51+
`${serverRef.serverBaseWsUrl}/inspector/device?device=device1&name=foo&app=bar`,
52+
autoCleanup.signal,
53+
);
54+
try {
55+
device.getPages.mockImplementation(() => [
56+
{
57+
app: 'bar-app',
58+
id: 'page1',
59+
title: 'bar-title',
60+
vm: 'bar-vm',
61+
},
62+
]);
63+
await until(async () =>
64+
expect((await fetchJson(`${serverRef.serverBaseUrl}/json`)).length)
65+
.toBeGreaterThan(0),
66+
);
67+
68+
// Now break one connection at the protocol level: a single message in
69+
// more fragments than the vendored `ws` permits, which fails the
70+
// receiver with RangeError and close code 1008.
71+
const bad = new WS(`${serverRef.serverBaseWsUrl}${path}`);
72+
bad.on('error', () => {});
73+
await new Promise((resolve, reject) => {
74+
bad.on('open', resolve);
75+
bad.on('close', resolve);
76+
bad.on('error', reject);
77+
}).catch(() => {});
78+
79+
if (bad.readyState === WS.OPEN) {
80+
const sender = bad._sender;
81+
sender.send(Buffer.from('x'), {fin: false, opcode: 1, mask: true}, () => {});
82+
for (let i = 0; i < 17000; i++) {
83+
sender.send(Buffer.from('x'), {fin: false, opcode: 0, mask: true}, () => {});
84+
}
85+
sender.send(Buffer.from('x'), {fin: true, opcode: 0, mask: true}, () => {});
86+
}
87+
88+
// The proxy is still serving, and the healthy device is still listed.
89+
await until(async () => {
90+
const pages = await fetchJson(`${serverRef.serverBaseUrl}/json`);
91+
expect(pages.length).toBeGreaterThan(0);
92+
});
93+
} finally {
94+
device.close();
95+
}
96+
},
97+
);
98+
});

packages/dev-middleware/src/inspector-proxy/InspectorProxy.js

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,23 @@ export default class InspectorProxy implements InspectorProxyQueries {
334334
});
335335
// $FlowFixMe[value-as-type]
336336
wss.on('connection', async (socket: WS, req) => {
337+
// Attached before the first await: this handler is async, so an error
338+
// arriving while it is suspended would otherwise have no listener. In
339+
// Node an 'error' event with no listener throws, which takes down the
340+
// whole dev server over a single bad connection.
341+
socket.on('error', error => {
342+
this.#logger?.error(
343+
'Error on device connection, closing it: %s',
344+
error?.message ?? String(error),
345+
);
346+
// terminate() rather than close(): close() waits for a closing
347+
// handshake, and a socket that failed mid-frame may never complete
348+
// one, which would leak the connection instead.
349+
try {
350+
socket.terminate();
351+
} catch {}
352+
});
353+
337354
const wssTimestamp = Date.now();
338355

339356
const fallbackDeviceId = String(this.#deviceCounter++);
@@ -523,6 +540,23 @@ export default class InspectorProxy implements InspectorProxyQueries {
523540

524541
// $FlowFixMe[value-as-type]
525542
wss.on('connection', async (socket: WS, req) => {
543+
// Attached before the first await: this handler is async, so an error
544+
// arriving while it is suspended would otherwise have no listener. In
545+
// Node an 'error' event with no listener throws, which takes down the
546+
// whole dev server over a single bad connection.
547+
socket.on('error', error => {
548+
this.#logger?.error(
549+
'Error on debugger connection, closing it: %s',
550+
error?.message ?? String(error),
551+
);
552+
// terminate() rather than close(): close() waits for a closing
553+
// handshake, and a socket that failed mid-frame may never complete
554+
// one, which would leak the connection instead.
555+
try {
556+
socket.terminate();
557+
} catch {}
558+
});
559+
526560
const wssTimestamp = Date.now();
527561

528562
const query = tryParseQueryParams(req.url);

0 commit comments

Comments
 (0)