From 75e29b7c44425cfa75d292c5d4940bbb67be630b Mon Sep 17 00:00:00 2001 From: Bennett Payoyo Date: Sun, 27 Sep 2026 10:42:09 +0800 Subject: [PATCH 1/2] fix(engine.io): catch synchronous middleware errors Synchronous throws (and rejected promises) from engine.use middlewares now reach the same callback(err) path as next(err), so they surface as MIDDLEWARE_FAILURE / connection_error instead of being lost. Fixes #5263 --- packages/engine.io/lib/server.ts | 28 +++++++++++++--------- packages/engine.io/test/middlewares.js | 32 +++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/packages/engine.io/lib/server.ts b/packages/engine.io/lib/server.ts index c7c5716132..ddf3dce800 100644 --- a/packages/engine.io/lib/server.ts +++ b/packages/engine.io/lib/server.ts @@ -397,17 +397,23 @@ export abstract class BaseServer extends EventEmitter { const apply = (i) => { debug("applying middleware n°%d", i + 1); - this.middlewares[i](req, res, (err?: any) => { - if (err) { - return callback(err); - } + try { + Promise.resolve( + this.middlewares[i](req, res, (err?: any) => { + if (err) { + return callback(err); + } - if (i + 1 < this.middlewares.length) { - apply(i + 1); - } else { - callback(); - } - }); + if (i + 1 < this.middlewares.length) { + apply(i + 1); + } else { + callback(); + } + }), + ).catch(callback); + } catch (e) { + callback(e); + } }; apply(0); @@ -1116,7 +1122,7 @@ const validHdrChars = [ 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 // ... 255 -] +]; function checkInvalidHeaderChar(val?: string) { val += ""; diff --git a/packages/engine.io/test/middlewares.js b/packages/engine.io/test/middlewares.js index 60e1cc6b22..b74b5b9eac 100644 --- a/packages/engine.io/test/middlewares.js +++ b/packages/engine.io/test/middlewares.js @@ -4,7 +4,7 @@ const request = require("superagent"); const { WebSocket } = require("ws"); const helmet = require("helmet"); const session = require("express-session"); -const { ClientSocket } = require("./common"); +const { ClientSocket, createPartialDone } = require("./common"); describe("middlewares", () => { it("should apply middleware (polling)", (done) => { @@ -249,6 +249,36 @@ describe("middlewares", () => { }); }); + it("should fail on synchronous middleware errors (polling)", (done) => { + const partialDone = createPartialDone(done, 2); + const engine = listen((port) => { + engine.use(() => { + throw new Error("will always fail"); + }); + + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.eql(3); + expect(err.message).to.eql("Bad request"); + expect(err.context.name).to.eql("MIDDLEWARE_FAILURE"); + partialDone(); + }); + + 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(); + } + partialDone(); + }); + }); + }); + it("should fail on errors (polling)", (done) => { const engine = listen((port) => { engine.use((req, res, next) => { From ac812ef1853cc40decc30ef0f56cce080bf4ee62 Mon Sep 17 00:00:00 2001 From: Bennett Payoyo Date: Wed, 30 Sep 2026 12:23:53 +0000 Subject: [PATCH 2/2] fix(engine.io): invoke each middleware callback at most once A middleware that calls next() and then throws or rejects was reported again by the new error handler, so the request callback ran twice. Settle each step on the first next(), throw, or rejection. Errors thrown by next()'s own continuation still propagate, and an error response is skipped when the middleware already sent headers. Fixes #5263 Co-authored-by: Bennett Payoyo --- packages/engine.io/lib/server.ts | 60 ++++-- packages/engine.io/test/middlewares.js | 279 ++++++++++++++++++++++++- 2 files changed, 324 insertions(+), 15 deletions(-) diff --git a/packages/engine.io/lib/server.ts b/packages/engine.io/lib/server.ts index ddf3dce800..aa44ce09bc 100644 --- a/packages/engine.io/lib/server.ts +++ b/packages/engine.io/lib/server.ts @@ -397,22 +397,44 @@ export abstract class BaseServer extends EventEmitter { const apply = (i) => { debug("applying middleware n°%d", i + 1); - try { - Promise.resolve( - this.middlewares[i](req, res, (err?: any) => { - if (err) { - return callback(err); - } - if (i + 1 < this.middlewares.length) { - apply(i + 1); - } else { - callback(); - } - }), - ).catch(callback); + // next(), a throw, or a rejection may each reach callback. Only the first + // one wins: a middleware that calls next() and then throws must not fire + // callback a second time. Errors thrown by next()'s own continuation are + // not middleware failures and are rethrown. + let settled = false; + let continuationError: any; + + const done = (err?: any) => { + if (settled) { + debug("middleware already settled, ignoring extra callback"); + return; + } + settled = true; + + try { + if (err) { + callback(err); + } else if (i + 1 < this.middlewares.length) { + apply(i + 1); + } else { + callback(); + } + } catch (e) { + continuationError = e; + throw e; + } + }; + + try { + Promise.resolve(this.middlewares[i](req, res, done)).catch((err) => { + done(err); + }); } catch (e) { - callback(e); + if (continuationError) { + throw continuationError; + } + done(e); } }; @@ -1048,6 +1070,16 @@ function abortRequest( ? errorContext.message : Server.errorMessages[errorCode]; + // A middleware may have already committed the response before failing. + // Writing again throws "Cannot write headers after they are sent". + if (res.headersSent) { + debug("response headers already sent, skipping error response"); + if (!res.writableEnded && !res.finished) { + res.end(); + } + return; + } + res.writeHead(statusCode, { "Content-Type": "application/json" }); res.end( JSON.stringify({ diff --git a/packages/engine.io/test/middlewares.js b/packages/engine.io/test/middlewares.js index b74b5b9eac..1acac81add 100644 --- a/packages/engine.io/test/middlewares.js +++ b/packages/engine.io/test/middlewares.js @@ -1,4 +1,5 @@ -const listen = require("./common").listen; +const { listen } = require("./common"); +const { Server } = require(".."); const expect = require("expect.js"); const request = require("superagent"); const { WebSocket } = require("ws"); @@ -279,6 +280,282 @@ describe("middlewares", () => { }); }); + it("should fail on rejected middleware promises (polling)", (done) => { + const partialDone = createPartialDone(done, 2); + const engine = listen((port) => { + engine.use(() => { + return Promise.reject(new Error("will always fail")); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + engine.on("connection_error", (err) => { + expect(err.req).to.be.ok(); + expect(err.code).to.eql(3); + expect(err.message).to.eql("Bad request"); + expect(err.context.name).to.eql("MIDDLEWARE_FAILURE"); + partialDone(); + }); + + 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(); + } + partialDone(); + }); + }); + }); + + it("should ignore a throw after next() and continue (polling)", (done) => { + let connectionErrors = 0; + let connections = 0; + const engine = listen((port) => { + engine.use((req, res, next) => { + next(); + throw new Error("after next"); + }); + + engine.use((req, res, next) => { + res.setHeader("foo", "bar"); + next(); + }); + + engine.on("connection_error", () => { + connectionErrors++; + }); + + engine.on("connection", () => { + connections++; + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ EIO: 4, transport: "polling" }) + .end((err, res) => { + expect(err).to.be(null); + expect(res.status).to.eql(200); + expect(res.headers["foo"]).to.eql("bar"); + expect(connectionErrors).to.eql(0); + expect(connections).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + + it("should ignore a throw after next(err) and fail once (polling)", (done) => { + let connectionErrors = 0; + const engine = listen((port) => { + engine.use((req, res, next) => { + next(new Error("will always fail")); + throw new Error("after next(err)"); + }); + + engine.use(() => { + done(new Error("next middleware should not run")); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + engine.on("connection_error", (err) => { + connectionErrors++; + expect(err.req).to.be.ok(); + expect(err.code).to.eql(3); + expect(err.message).to.eql("Bad request"); + expect(err.context.name).to.eql("MIDDLEWARE_FAILURE"); + }); + + 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); + expect(connectionErrors).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + + it("should ignore a rejection after next() and continue (polling)", (done) => { + let connectionErrors = 0; + let connections = 0; + const engine = listen((port) => { + engine.use((req, res, next) => { + next(); + return Promise.reject(new Error("after next")); + }); + + engine.on("connection_error", () => { + connectionErrors++; + }); + + engine.on("connection", () => { + connections++; + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ EIO: 4, transport: "polling" }) + .end((err, res) => { + expect(err).to.be(null); + expect(res.status).to.eql(200); + expect(connectionErrors).to.eql(0); + expect(connections).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + + it("should propagate errors thrown while handling next()", () => { + const engine = new Server({ transports: ["polling"] }); + let calls = 0; + + engine.use((_req, _res, next) => { + next(); + }); + + try { + expect(() => { + engine._applyMiddlewares({}, {}, () => { + calls++; + throw new Error("continuation failed"); + }); + }).to.throwException((err) => { + expect(err.message).to.eql("continuation failed"); + }); + expect(calls).to.eql(1); + } finally { + engine.close(); + } + }); + + it("should keep the middleware response when it throws after sending (polling)", (done) => { + let connectionErrors = 0; + const engine = listen((port) => { + engine.use((_req, res) => { + res.writeHead(503, { "Content-Type": "text/plain" }); + res.end("nope"); + throw new Error("after end"); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + engine.on("connection_error", (err) => { + connectionErrors++; + expect(err.context.name).to.eql("MIDDLEWARE_FAILURE"); + }); + + 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(503); + expect(res.text).to.eql("nope"); + expect(connectionErrors).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + + it("should keep the middleware response when it rejects after sending (polling)", (done) => { + let connectionErrors = 0; + const engine = listen((port) => { + engine.use((_req, res) => { + res.writeHead(503, { "Content-Type": "text/plain" }); + res.end("nope"); + return Promise.reject(new Error("after end")); + }); + + engine.on("connection", () => { + done(new Error("should not connect")); + }); + + engine.on("connection_error", (err) => { + connectionErrors++; + expect(err.context.name).to.eql("MIDDLEWARE_FAILURE"); + }); + + 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(503); + expect(res.text).to.eql("nope"); + expect(connectionErrors).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + + it("should invoke the callback once when next() is called twice (polling)", (done) => { + let runs = 0; + let connections = 0; + const engine = listen((port) => { + engine.use((req, res, next) => { + next(); + next(); + }); + + engine.use((req, res, next) => { + runs++; + next(); + }); + + engine.on("connection", () => { + connections++; + }); + + request + .get(`http://localhost:${port}/engine.io/`) + .query({ EIO: 4, transport: "polling" }) + .end((err, res) => { + expect(err).to.be(null); + expect(res.status).to.eql(200); + expect(runs).to.eql(1); + expect(connections).to.eql(1); + + if (engine.httpServer) { + engine.httpServer.close(); + } + done(); + }); + }); + }); + it("should fail on errors (polling)", (done) => { const engine = listen((port) => { engine.use((req, res, next) => {