Skip to content

Commit

Permalink
upgrade ws package to 7.5.1
Browse files Browse the repository at this point in the history
Summary:
## Context

Closes #674.

There is a security vulnerability with the current version of ws, that requires it to be upgraded to 5.2.3. I upgraded it to 7.5.1 while I was it.

## In this diff

A few of the API changes that needed to be fixed:
- `upgradeReq` was removed from the `WebSocket` object, the fix being to take the URL from the `request` param instead.

- `onError` now correctly passes an `ErrorEvent` instead of an `Error` object

- [can't attach more than one websocket per http server](websockets/ws#885 (comment)): this is the big one. On metro we use multiple websocket servers to do different things: HMR, JS debugger and generally talking back and forth with the dev server. The trick is to basically have an on `upgrade` handler that does the manual routing between the various servers. This is a breaking change for anybody who was using the Metro api to then attach a web socket server.

**The new recommended way to attach a websocket server if need be** is to pass a `websocketEndpoints` argument to the Metro runtime options in `Metro.runServer` that looks like `{ [path: string] : <an instance of WebSocketServer created with the "noServer: true" option> }`.

Reviewed By: motiz88

Differential Revision: D30012392

fbshipit-source-id: e69503f1a4da2ee417e7dcc9d42680e4dc0415d8
  • Loading branch information
Jimmy Lai authored and facebook-github-bot committed Aug 4, 2021
1 parent 46bef54 commit 38a200e
Show file tree
Hide file tree
Showing 8 changed files with 120 additions and 80 deletions.
2 changes: 1 addition & 1 deletion packages/metro-inspector-proxy/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"dependencies": {
"connect": "^3.6.5",
"debug": "^2.2.0",
"ws": "^1.1.5",
"ws": "^7.5.1",
"yargs": "^15.3.1"
}
}
34 changes: 20 additions & 14 deletions packages/metro-inspector-proxy/src/InspectorProxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,21 @@ class InspectorProxy {
}

// Adds websocket listeners to the provided HTTP/HTTPS server.
addWebSocketListener(server: HttpServer | HttpsServer) {
createWebSocketListeners(
server: HttpServer | HttpsServer,
): {
[path: string]: typeof WS.Server,
} {
const {port} = server.address();
if (server.address().family === 'IPv6') {
this._serverAddressWithPort = `[::1]:${port}`;
} else {
this._serverAddressWithPort = `localhost:${port}`;
}
this._addDeviceConnectionHandler(server);
this._addDebuggerConnectionHandler(server);
return {
[WS_DEVICE_URL]: this._createDeviceConnectionWSServer(),
[WS_DEBUGGER_URL]: this._createDebuggerConnectionWSServer(),
};
}

// Converts page information received from device into PageDescription object
Expand Down Expand Up @@ -151,16 +157,15 @@ class InspectorProxy {
// HTTP GET params.
// For each new websocket connection we parse device and app names and create
// new instance of Device class.
_addDeviceConnectionHandler(server: HttpServer | HttpsServer) {
_createDeviceConnectionWSServer() {
const wss = new WS.Server({
server,
path: WS_DEVICE_URL,
noServer: true,
perMessageDeflate: true,
});
// $FlowFixMe[value-as-type]
wss.on('connection', async (socket: WS) => {
wss.on('connection', async (socket: WS, req) => {
try {
const query = url.parse(socket.upgradeReq.url || '', true).query || {};
const query = url.parse(req.url || '', true).query || {};
const deviceName = query.name || 'Unknown';
const appName = query.app || 'Unknown';
const deviceId = this._deviceCounter++;
Expand All @@ -180,23 +185,23 @@ class InspectorProxy {
socket.close(INTERNAL_ERROR_CODE, e);
}
});
return wss;
}

// Adds websocket handler for debugger connections.
// Returns websocket handler for debugger connections.
// Debugger connects to webSocketDebuggerUrl that we return as part of page description
// in /json response.
// When debugger connects we try to parse device and page IDs from the query and pass
// websocket object to corresponding Device instance.
_addDebuggerConnectionHandler(server: HttpServer | HttpsServer) {
_createDebuggerConnectionWSServer() {
const wss = new WS.Server({
server,
path: WS_DEBUGGER_URL,
noServer: true,
perMessageDeflate: false,
});
// $FlowFixMe[value-as-type]
wss.on('connection', async (socket: WS) => {
wss.on('connection', async (socket: WS, req) => {
try {
const query = url.parse(socket.upgradeReq.url || '', true).query || {};
const query = url.parse(req.url || '', true).query || {};
const deviceId = query.device;
const pageId = query.page;

Expand All @@ -215,6 +220,7 @@ class InspectorProxy {
socket.close(INTERNAL_ERROR_CODE, e);
}
});
return wss;
}
}

Expand Down
20 changes: 19 additions & 1 deletion packages/metro-inspector-proxy/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

const InspectorProxy = require('./InspectorProxy');

const {parse} = require('url');
// Runs new HTTP Server and attaches Inspector Proxy to it.
// Requires are inlined here because we don't want to import them
// when someone needs only InspectorProxy instance (without starting
Expand All @@ -24,7 +25,24 @@ function runInspectorProxy(port: number, projectRoot: string) {

const httpServer = require('http').createServer(app);
httpServer.listen(port, '127.0.0.1', () => {
inspectorProxy.addWebSocketListener(httpServer);
const websocketEndpoints = inspectorProxy.createWebSocketListeners(
httpServer,
);
httpServer.on('upgrade', (request, socket, head) => {
const {pathname} = parse(request.url);
if (pathname != null && websocketEndpoints[pathname]) {
websocketEndpoints[pathname].handleUpgrade(
request,
socket,
head,
ws => {
websocketEndpoints[pathname].emit('connection', ws, request);
},
);
} else {
socket.destroy();
}
});
});
}

Expand Down
2 changes: 1 addition & 1 deletion packages/metro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"strip-ansi": "^6.0.0",
"temp": "0.8.3",
"throat": "^5.0.0",
"ws": "^1.1.5",
"ws": "^7.5.1",
"yargs": "^15.3.1"
},
"devDependencies": {
Expand Down
22 changes: 11 additions & 11 deletions packages/metro/src/HmrServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,16 @@ class HmrServer<TClient: Client> {
this._clientGroups = new Map();
}

async onClientConnect(
onClientConnect: (
requestUrl: string,
sendFn: (data: string) => void,
): Promise<Client> {
) => Promise<Client> = async (requestUrl, sendFn) => {
return {
sendFn,
revisionIds: [],
optedIntoHMR: false,
};
}
};

async _registerEntryPoint(
client: Client,
Expand Down Expand Up @@ -185,11 +185,11 @@ class HmrServer<TClient: Client> {
send([sendFn], {type: 'bundle-registered'});
}

async onClientMessage(
onClientMessage: (
client: TClient,
message: string,
sendFn: (data: string) => void,
): Promise<void> {
) => Promise<void> = async (client, message, sendFn) => {
let data: HmrClientMessage;
try {
data = JSON.parse(message);
Expand Down Expand Up @@ -224,17 +224,17 @@ class HmrServer<TClient: Client> {
}
}
return Promise.resolve();
}
};

onClientError(client: TClient, e: Error): void {
onClientError: (client: TClient, e: ErrorEvent) => void = (client, e) => {
this._config.reporter.update({
type: 'hmr_client_error',
error: e,
error: e.error,
});
this.onClientDisconnect(client);
}
};

onClientDisconnect(client: TClient): void {
onClientDisconnect: (client: TClient) => void = client => {
client.revisionIds.forEach(revisionId => {
const group = this._clientGroups.get(revisionId);
if (group != null) {
Expand All @@ -246,7 +246,7 @@ class HmrServer<TClient: Client> {
}
}
});
}
};

async _handleFileChange(
group: ClientGroup,
Expand Down
68 changes: 53 additions & 15 deletions packages/metro/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,20 @@ const IncrementalBundler = require('./IncrementalBundler');
const MetroHmrServer = require('./HmrServer');
const MetroServer = require('./Server');

const attachWebsocketServer = require('./lib/attachWebsocketServer');
const chalk = require('chalk');
const createWebsocketServer = require('./lib/createWebsocketServer');
const fs = require('fs');
const http = require('http');
const https = require('https');
const makeBuildCommand = require('./commands/build');
const makeDependenciesCommand = require('./commands/dependencies');
const makeServeCommand = require('./commands/serve');
const outputBundle = require('./shared/output/bundle');
const ws = require('ws');

const {loadConfig, mergeConfig, getDefaultConfig} = require('metro-config');
const {InspectorProxy} = require('metro-inspector-proxy');
const {parse} = require('url');

import type {Graph} from './DeltaBundler';
import type {ServerOptions} from './Server';
Expand Down Expand Up @@ -57,6 +59,9 @@ export type RunServerOptions = {|
secure?: boolean, // deprecated
secureCert?: string, // deprecated
secureKey?: string, // deprecated
websocketEndpoints?: {
[path: string]: typeof ws.Server,
},
|};

type BuildGraphOptions = {|
Expand Down Expand Up @@ -148,16 +153,23 @@ exports.createConnectMiddleware = async function(

return {
attachHmrServer(httpServer: HttpServer | HttpsServer): void {
attachWebsocketServer({
httpServer,
path: '/hot',
// $FlowFixMe[method-unbinding]
const wss = createWebsocketServer({
websocketServer: new MetroHmrServer(
metroServer.getBundler(),
metroServer.getCreateModuleId(),
config,
),
});
httpServer.on('upgrade', (request, socket, head) => {
const {pathname} = parse(request.url);
if (pathname === '/hot') {
wss.handleUpgrade(request, socket, head, ws => {
wss.emit('connection', ws, request);
});
} else {
socket.destroy();
}
});
},
metroServer,
middleware: enhancedMiddleware,
Expand All @@ -178,6 +190,7 @@ exports.runServer = async (
secure, //deprecated
secureCert, // deprecated
secureKey, // deprecated
websocketEndpoints = {},
}: RunServerOptions,
): Promise<HttpServer | HttpsServer> => {
if (secure != null || secureCert != null || secureKey != null) {
Expand All @@ -194,13 +207,12 @@ exports.runServer = async (

const serverApp = connect();

const {
attachHmrServer,
middleware,
end,
} = await exports.createConnectMiddleware(config, {
hasReducedPerformance,
});
const {middleware, end, metroServer} = await exports.createConnectMiddleware(
config,
{
hasReducedPerformance,
},
);

serverApp.use(middleware);

Expand Down Expand Up @@ -244,10 +256,36 @@ exports.runServer = async (
onReady(httpServer);
}

attachHmrServer(httpServer);
if (inspectorProxy) {
inspectorProxy.addWebSocketListener(httpServer);
Object.assign(websocketEndpoints, {
...(inspectorProxy
? {...inspectorProxy.createWebSocketListeners(httpServer)}
: {}),
'/hot': createWebsocketServer({
websocketServer: new MetroHmrServer(
metroServer.getBundler(),
metroServer.getCreateModuleId(),
config,
),
}),
});

httpServer.on('upgrade', (request, socket, head) => {
const {pathname} = parse(request.url);
if (pathname != null && websocketEndpoints[pathname]) {
websocketEndpoints[pathname].handleUpgrade(
request,
socket,
head,
ws => {
websocketEndpoints[pathname].emit('connection', ws, request);
},
);
} else {
socket.destroy();
}
});

if (inspectorProxy) {
// TODO(hypuk): Refactor inspectorProxy.processRequest into separate request handlers
// so that we could provide routes (/json/list and /json/version) here.
// Currently this causes Metro to give warning about T31407894.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,14 @@
* @format
*/

'use strict';

import type {Server as HttpServer} from 'http';
import type {Server as HttpsServer} from 'https';

import ws from 'ws';
type WebsocketServiceInterface<T> = interface {
+onClientConnect: (
url: string,
sendFn: (data: string) => void,
) => Promise<?T>,
+onClientDisconnect?: (client: T) => mixed,
+onClientError?: (client: T, e: Error) => mixed,
+onClientError?: (client: T, e: ErrorEvent) => mixed,
+onClientMessage?: (
client: T,
message: string,
Expand All @@ -28,14 +24,12 @@ type WebsocketServiceInterface<T> = interface {
};

type HMROptions<TClient> = {
httpServer: HttpServer | HttpsServer,
websocketServer: WebsocketServiceInterface<TClient>,
path: string,
...
};

/**
* Attach a websocket server to an already existing HTTP[S] server, and forward
* Returns a WebSocketServer to be attached to an existing HTTP instance. It forwards
* the received events on the given "websocketServer" parameter. It must be an
* object with the following fields:
*
Expand All @@ -45,20 +39,16 @@ type HMROptions<TClient> = {
* - onClientDisconnect
*/

module.exports = function attachWebsocketServer<TClient: Object>({
httpServer,
module.exports = function createWebsocketServer<TClient: Object>({
websocketServer,
path,
}: HMROptions<TClient>): void {
const WebSocketServer = require('ws').Server;
const wss = new WebSocketServer({
server: httpServer,
path,
}: HMROptions<TClient>): typeof ws.Server {
const wss = new ws.Server({
noServer: true,
});

wss.on('connection', async ws => {
wss.on('connection', async (ws, req) => {
let connected = true;
const url = ws.upgradeReq.url;
const url = req.url;

const sendFn = (...args) => {
if (connected) {
Expand Down Expand Up @@ -88,4 +78,5 @@ module.exports = function attachWebsocketServer<TClient: Object>({
websocketServer.onClientMessage(client, message, sendFn);
});
});
return wss;
};
Loading

0 comments on commit 38a200e

Please sign in to comment.