From bd6d4713b02ff646c581872cd9ffe753acff0d73 Mon Sep 17 00:00:00 2001 From: Asger Hautop Drewsen Date: Wed, 19 Apr 2023 22:25:16 +0200 Subject: [PATCH 1/7] fix(typings): make clientsCount public (#675) Related: https://github.com/socketio/engine.io/issues/672 --- lib/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/server.ts b/lib/server.ts index ac493bbc..8a21f3f5 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -144,7 +144,7 @@ export abstract class BaseServer extends EventEmitter { public opts: ServerOptions; protected clients: any; - private clientsCount: number; + public clientsCount: number; protected middlewares: Middleware[] = []; /** From 911d0e35757ea9ee93d1807c401c734661615e96 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Mon, 1 May 2023 07:24:27 +0200 Subject: [PATCH 2/7] refactor: return HTTP 400 upon invalid request overlap In both cases, the error comes from the client as it should not send multiple concurrent requests, so a HTTP 4xx code is mandated. Related: https://github.com/socketio/engine.io/issues/650 --- lib/transports/polling.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/transports/polling.ts b/lib/transports/polling.ts index 69a968eb..70be3411 100644 --- a/lib/transports/polling.ts +++ b/lib/transports/polling.ts @@ -75,8 +75,7 @@ export class Polling extends Transport { debug("request overlap"); // assert: this.res, '.req and .res should be (un)set together' this.onError("overlap from client"); - // TODO for the next major release: use an HTTP 400 status code (https://github.com/socketio/engine.io/issues/650) - res.writeHead(500); + res.writeHead(400); res.end(); return; } @@ -117,8 +116,7 @@ export class Polling extends Transport { if (this.dataReq) { // assert: this.dataRes, '.dataReq and .dataRes should be (un)set together' this.onError("data request overlap from client"); - // TODO for the next major release: use an HTTP 400 status code (https://github.com/socketio/engine.io/issues/650) - res.writeHead(500); + res.writeHead(400); res.end(); return; } From 93957828be1252c83275b56f0c7c0bd145a0ceb9 Mon Sep 17 00:00:00 2001 From: Ciel <9755720+cieldeville@users.noreply.github.com> Date: Tue, 2 May 2023 00:00:47 +0200 Subject: [PATCH 3/7] fix: include error handling for Express middlewares (#674) Following https://github.com/socketio/engine.io/commit/24786e77c5403b1c4b5a2bc84e2af06f9187f74a. Reference: https://expressjs.com/en/guide/error-handling.html --- lib/server.ts | 67 ++++++++++------- lib/userver.ts | 170 ++++++++++++++++++++++++-------------------- test/middlewares.js | 44 ++++++++++++ 3 files changed, 177 insertions(+), 104 deletions(-) diff --git a/lib/server.ts b/lib/server.ts index 8a21f3f5..1651a2ed 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -137,7 +137,7 @@ export interface ServerOptions { type Middleware = ( req: IncomingMessage, res: ServerResponse, - next: () => void + next: (err?: any) => void ) => void; export abstract class BaseServer extends EventEmitter { @@ -335,7 +335,7 @@ export abstract class BaseServer extends EventEmitter { protected _applyMiddlewares( req: IncomingMessage, res: ServerResponse, - callback: () => void + callback: (err?: any) => void ) { if (this.middlewares.length === 0) { debug("no middleware to apply, skipping"); @@ -344,7 +344,11 @@ export abstract class BaseServer extends EventEmitter { const apply = (i) => { debug("applying middleware n°%d", i + 1); - this.middlewares[i](req, res, () => { + this.middlewares[i](req, res, (err?: any) => { + if (err) { + return callback(err); + } + if (i + 1 < this.middlewares.length) { apply(i + 1); } else { @@ -655,8 +659,12 @@ export class Server extends BaseServer { } }; - this._applyMiddlewares(req, res, () => { - this.verify(req, false, callback); + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, false, callback); + } }); } @@ -673,32 +681,37 @@ export class Server extends BaseServer { this.prepare(req); const res = new WebSocketResponse(req, socket); + const callback = (errorCode, errorContext) => { + if (errorCode) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + abortUpgrade(socket, errorCode, errorContext); + return; + } - this._applyMiddlewares(req, res as unknown as ServerResponse, () => { - this.verify(req, true, (errorCode, errorContext) => { - if (errorCode) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - abortUpgrade(socket, errorCode, errorContext); - return; - } - - const head = Buffer.from(upgradeHead); - upgradeHead = null; + const head = Buffer.from(upgradeHead); + upgradeHead = null; - // some middlewares (like express-session) wait for the writeHead() call to flush their headers - // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 - res.writeHead(); + // some middlewares (like express-session) wait for the writeHead() call to flush their headers + // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 + res.writeHead(); - // delegate to ws - this.ws.handleUpgrade(req, socket, head, (websocket) => { - this.onWebSocket(req, socket, websocket); - }); + // delegate to ws + this.ws.handleUpgrade(req, socket, head, (websocket) => { + this.onWebSocket(req, socket, websocket); }); + }; + + this._applyMiddlewares(req, res as unknown as ServerResponse, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, true, callback); + } }); } diff --git a/lib/userver.ts b/lib/userver.ts index 29729bd1..6f4872f2 100644 --- a/lib/userver.ts +++ b/lib/userver.ts @@ -92,7 +92,11 @@ export class uServer extends BaseServer { }); } - override _applyMiddlewares(req: any, res: any, callback: () => void): void { + override _applyMiddlewares( + req: any, + res: any, + callback: (err?: any) => void + ): void { if (this.middlewares.length === 0) { return callback(); } @@ -100,12 +104,12 @@ export class uServer extends BaseServer { // needed to buffer headers until the status is computed req.res = new ResponseWrapper(res); - super._applyMiddlewares(req, req.res, () => { + super._applyMiddlewares(req, req.res, (err) => { // some middlewares (like express-session) wait for the writeHead() call to flush their headers // see https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/index.js#L220-L244 req.res.writeHead(); - callback(); + callback(err); }); } @@ -118,28 +122,34 @@ export class uServer extends BaseServer { req.res = res; - this._applyMiddlewares(req, res, () => { - this.verify(req, false, (errorCode, errorContext) => { - if (errorCode !== undefined) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - this.abortRequest(req.res, errorCode, errorContext); - return; - } + const callback = (errorCode, errorContext) => { + if (errorCode !== undefined) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + this.abortRequest(req.res, errorCode, errorContext); + return; + } + + if (req._query.sid) { + debug("setting new request for existing client"); + this.clients[req._query.sid].transport.onRequest(req); + } else { + const closeConnection = (errorCode, errorContext) => + this.abortRequest(res, errorCode, errorContext); + this.handshake(req._query.transport, req, closeConnection); + } + }; - if (req._query.sid) { - debug("setting new request for existing client"); - this.clients[req._query.sid].transport.onRequest(req); - } else { - const closeConnection = (errorCode, errorContext) => - this.abortRequest(res, errorCode, errorContext); - this.handshake(req._query.transport, req, closeConnection); - } - }); + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, false, callback); + } }); } @@ -154,63 +164,69 @@ export class uServer extends BaseServer { req.res = res; - this._applyMiddlewares(req, res, () => { - this.verify(req, true, async (errorCode, errorContext) => { - if (errorCode) { - this.emit("connection_error", { - req, - code: errorCode, - message: Server.errorMessages[errorCode], - context: errorContext, - }); - this.abortRequest(res, errorCode, errorContext); + const callback = async (errorCode, errorContext) => { + if (errorCode) { + this.emit("connection_error", { + req, + code: errorCode, + message: Server.errorMessages[errorCode], + context: errorContext, + }); + this.abortRequest(res, errorCode, errorContext); + return; + } + + const id = req._query.sid; + let transport; + + if (id) { + const client = this.clients[id]; + if (!client) { + debug("upgrade attempt for closed client"); + res.close(); + } else if (client.upgrading) { + debug("transport has already been trying to upgrade"); + res.close(); + } else if (client.upgraded) { + debug("transport had already been upgraded"); + res.close(); + } else { + debug("upgrading existing transport"); + transport = this.createTransport(req._query.transport, req); + client.maybeUpgrade(transport); + } + } else { + transport = await this.handshake( + req._query.transport, + req, + (errorCode, errorContext) => + this.abortRequest(res, errorCode, errorContext) + ); + if (!transport) { return; } + } - const id = req._query.sid; - let transport; - - if (id) { - const client = this.clients[id]; - if (!client) { - debug("upgrade attempt for closed client"); - res.close(); - } else if (client.upgrading) { - debug("transport has already been trying to upgrade"); - res.close(); - } else if (client.upgraded) { - debug("transport had already been upgraded"); - res.close(); - } else { - debug("upgrading existing transport"); - transport = this.createTransport(req._query.transport, req); - client.maybeUpgrade(transport); - } - } else { - transport = await this.handshake( - req._query.transport, - req, - (errorCode, errorContext) => - this.abortRequest(res, errorCode, errorContext) - ); - if (!transport) { - return; - } - } + // calling writeStatus() triggers the flushing of any header added in a middleware + req.res.writeStatus("101 Switching Protocols"); - // calling writeStatus() triggers the flushing of any header added in a middleware - req.res.writeStatus("101 Switching Protocols"); - - res.upgrade( - { - transport, - }, - req.getHeader("sec-websocket-key"), - req.getHeader("sec-websocket-protocol"), - req.getHeader("sec-websocket-extensions"), - context - ); - }); + res.upgrade( + { + transport, + }, + req.getHeader("sec-websocket-key"), + req.getHeader("sec-websocket-protocol"), + req.getHeader("sec-websocket-extensions"), + context + ); + }; + + this._applyMiddlewares(req, res, (err) => { + if (err) { + callback(Server.errors.BAD_REQUEST, { name: "MIDDLEWARE_FAILURE" }); + } else { + this.verify(req, true, callback); + } }); } diff --git a/test/middlewares.js b/test/middlewares.js index a81f1354..4045d5d0 100644 --- a/test/middlewares.js +++ b/test/middlewares.js @@ -247,4 +247,48 @@ describe("middlewares", () => { }); }); }); + + it("should fail on errors (polling)", (done) => { + const engine = listen((port) => { + engine.use((req, res, next) => { + next(new Error("will always fail")); + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ EIO: 4, transport: "polling" }) + .end((err, res) => { + expect(err).to.be.an(Error); + expect(res.status).to.eql(400); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + + it("should fail on errors (websocket)", (done) => { + const engine = listen((port) => { + engine.use((req, res, next) => { + next(new Error("will always fail")); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + const socket = new WebSocket( + `ws://localhost:${port}/engine.io/?EIO=4&transport=websocket` + ); + + socket.addEventListener("error", () => { + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + }); }); From 8b2216290330b174c9e67be32765bec0c74769f9 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 2 May 2023 00:26:44 +0200 Subject: [PATCH 4/7] fix(uws): prevent crash when using with middlewares The class used to accumulate the response headers did not expose the exact same API as its wrapped type, which could lead to the following error in some rare cases: > TypeError: Cannot read properties of undefined (reading 'end') > at Polling.onDataRequest (build/transports-uws/polling.js:109:53) > at Polling.onRequest (build/transports-uws/polling.js:47:18) > at callback (build/userver.js:94:56) > at uServer.verify (build/server.js:152:9) Related: https://github.com/socketio/socket.io/issues/4643 --- lib/userver.ts | 1 + test/middlewares.js | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/userver.ts b/lib/userver.ts index 6f4872f2..5616f78f 100644 --- a/lib/userver.ts +++ b/lib/userver.ts @@ -294,6 +294,7 @@ class ResponseWrapper { this.res.writeStatus(status); this.statusWritten = true; this.writeBufferedHeaders(); + return this; } public writeHeader(key: string, value: string) { diff --git a/test/middlewares.js b/test/middlewares.js index 4045d5d0..586df621 100644 --- a/test/middlewares.js +++ b/test/middlewares.js @@ -4,6 +4,7 @@ const request = require("superagent"); const { WebSocket } = require("ws"); const helmet = require("helmet"); const session = require("express-session"); +const { ClientSocket } = require("./common"); describe("middlewares", () => { it("should apply middleware (polling)", (done) => { @@ -291,4 +292,30 @@ describe("middlewares", () => { }); }); }); + + it("should not be receiving data when getting a message longer than maxHttpBufferSize when polling (with a middleware)", (done) => { + const opts = { + allowUpgrades: false, + transports: ["polling"], + maxHttpBufferSize: 5, + }; + const engine = listen(opts, (port) => { + engine.use((req, res, next) => { + next(); + }); + + const socket = new ClientSocket(`ws://localhost:${port}`); + engine.on("connection", (conn) => { + conn.on("message", () => { + done(new Error("Test invalidation (message is longer than allowed)")); + }); + }); + socket.on("open", () => { + socket.send("aasdasdakjhasdkjhasdkjhasdkjhasdkjhasdkjhasdkjha"); + }); + socket.on("close", (reason) => { + done(); + }); + }); + }); }); From 014195118535669af0ad3bde38a76601dafa4d81 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 2 May 2023 00:33:33 +0200 Subject: [PATCH 5/7] refactor(types): ensure compatibility with Express middlewares In order to prevent issues like: > error TS2345: Argument of type 'RequestHandler>' is not assignable to parameter of type 'Middleware'. > Types of parameters 'req' and 'req' are incompatible. > Type 'IncomingMessage' is missing the following properties from type 'Request>': get, header, accepts, acceptsCharsets, and 29 more. > > io.engine.use(sessionMiddleware); ~~~~~~~~~~~~~~~~~ Related: https://github.com/socketio/socket.io/issues/4644 We could also have use the RequestHandler type from the @types/express-serve-static-core package, but that would add 5 new dependencies. See also: https://github.com/socketio/engine.io/issues/673 --- lib/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/server.ts b/lib/server.ts index 1651a2ed..160b2d88 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -320,7 +320,7 @@ export abstract class BaseServer extends EventEmitter { * * @param fn */ - public use(fn: Middleware) { + public use(fn: any) { this.middlewares.push(fn); } From fc480b4f305e16fe5972cf337d055e598372dc44 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 2 May 2023 01:02:42 +0200 Subject: [PATCH 6/7] fix: prevent crash when provided with an invalid query param A specially crafted request could lead to the following exception: > TypeError: Cannot read properties of undefined (reading 'handlesUpgrades') > at Server.onWebSocket (build/server.js:515:67) This bug was introduced in [1], released in version 5.1.0 and included in version 4.1.0 of the `socket.io` parent package. Older versions are not impacted. [1]: https://github.com/socketio/engine.io/commit/7096e98a02295a62c8ea2aa56461d4875887092d --- lib/server.ts | 2 +- lib/userver.ts | 2 +- test/server.js | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/lib/server.ts b/lib/server.ts index 160b2d88..a4a4dcf8 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -682,7 +682,7 @@ export class Server extends BaseServer { const res = new WebSocketResponse(req, socket); const callback = (errorCode, errorContext) => { - if (errorCode) { + if (errorCode !== undefined) { this.emit("connection_error", { req, code: errorCode, diff --git a/lib/userver.ts b/lib/userver.ts index 5616f78f..951d3b96 100644 --- a/lib/userver.ts +++ b/lib/userver.ts @@ -165,7 +165,7 @@ export class uServer extends BaseServer { req.res = res; const callback = async (errorCode, errorContext) => { - if (errorCode) { + if (errorCode !== undefined) { this.emit("connection_error", { req, code: errorCode, diff --git a/test/server.js b/test/server.js index 19dd3fbe..bfda7cd9 100644 --- a/test/server.js +++ b/test/server.js @@ -11,6 +11,7 @@ const { ClientSocket, listen, createPartialDone } = require("./common"); const expect = require("expect.js"); const request = require("superagent"); const cookieMod = require("cookie"); +const { WebSocket } = require("ws"); /** * Tests. @@ -197,6 +198,51 @@ describe("server", () => { }); }); }); + + it("should disallow `__proto__` as transport (polling)", (done) => { + const partialDone = createPartialDone(done, 2); + + engine = listen((port) => { + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.be(0); + expect(err.message).to.be("Transport unknown"); + expect(err.context.transport).to.be("__proto__"); + partialDone(); + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ transport: "__proto__", EIO: 4 }) + .end((err, res) => { + expect(err).to.be.an(Error); + expect(res.status).to.be(400); + expect(res.body.code).to.be(0); + expect(res.body.message).to.be("Transport unknown"); + partialDone(); + }); + }); + }); + + it("should disallow `__proto__` as transport (websocket)", (done) => { + const partialDone = createPartialDone(done, 2); + + engine = listen((port) => { + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.be(0); + expect(err.message).to.be("Transport unknown"); + expect(err.context.transport).to.be("__proto__"); + partialDone(); + }); + + const socket = new WebSocket( + `ws://localhost:${port}/engine.io/?EIO=4&transport=__proto__` + ); + + socket.onerror = partialDone; + }); + }); }); describe("handshake", () => { From 95e215387c589025dde3982865bf8c862d049469 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 2 May 2023 01:27:20 +0200 Subject: [PATCH 7/7] chore(release): 6.4.2 Diff: https://github.com/socketio/engine.io/compare/6.4.1...6.4.2 --- CHANGELOG.md | 34 ++++++++++++++++++++++++++++++++++ package.json | 2 +- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45c9c8da..60ebdb9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## 2023 +- [6.4.2](#642-2023-05-02) (May 2023) - [6.4.1](#641-2023-02-20) (Feb 2023) - [6.4.0](#640-2023-02-06) (Feb 2023) - [6.3.1](#631-2023-01-12) (Jan 2023) @@ -46,6 +47,39 @@ # Release notes +## [6.4.2](https://github.com/socketio/engine.io/compare/6.4.1...6.4.2) (2023-05-02) + +:warning: This release contains an important security fix :warning: + +A malicious client could send a specially crafted HTTP request, triggering an uncaught exception and killing the Node.js process: + +``` +TypeError: Cannot read properties of undefined (reading 'handlesUpgrades') + at Server.onWebSocket (build/server.js:515:67) +``` + +Please upgrade as soon as possible. + + +### Bug Fixes + +* include error handling for Express middlewares ([#674](https://github.com/socketio/engine.io/issues/674)) ([9395782](https://github.com/socketio/engine.io/commit/93957828be1252c83275b56f0c7c0bd145a0ceb9)) +* prevent crash when provided with an invalid query param ([fc480b4](https://github.com/socketio/engine.io/commit/fc480b4f305e16fe5972cf337d055e598372dc44)) +* **typings:** make clientsCount public ([#675](https://github.com/socketio/engine.io/issues/675)) ([bd6d471](https://github.com/socketio/engine.io/commit/bd6d4713b02ff646c581872cd9ffe753acff0d73)) +* **uws:** prevent crash when using with middlewares ([8b22162](https://github.com/socketio/engine.io/commit/8b2216290330b174c9e67be32765bec0c74769f9)) + + +### Credits + +Huge thanks to [@tyilo](https://github.com/tyilo) and [@cieldeville](https://github.com/cieldeville) for helping! + + +### Dependencies + +- [`ws@~8.11.0`](https://github.com/websockets/ws/releases/tag/8.11.0) (no change) + + + ## [6.4.1](https://github.com/socketio/engine.io/compare/6.4.0...6.4.1) (2023-02-20) This release contains [6e78489](https://github.com/socketio/engine.io/commit/6e78489486f0d7570861fd6002a364d1ab87da4a), which exports the `BaseServer` class in order to restore the compatibility with the `nodenext` module resolution strategy of TypeScript. diff --git a/package.json b/package.json index a368fda6..0a8e2459 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "engine.io", - "version": "6.4.1", + "version": "6.4.2", "description": "The realtime engine behind Socket.IO. Provides the foundation of a bidirectional connection between client and server", "type": "commonjs", "main": "./build/engine.io.js",