Repository navigation
Errors thrown by middlewares do not get captured or reported #5263
Description
Describe the bug
If an unexpected error is thrown inside of a middleware, the error is completely ignored and not logged to any output stream or file. This makes errors impossible to debug without adding breakpoints into the dependency. This is obviously bad practice for a production system as rare errors will all get swept under the rug.
See here:
socket.io/packages/engine.io/lib/server.ts
Line 772 in 7427109
To Reproduce
Please fill the following code example:
Socket.IO server version: 4.8.x
Server
import { Server } from "socket.io";
const io = new Server(3000, {});
io.engine.use((req, res, next) => {
if (!req.session?.hasSocketAccess) {
return next(new Error("You do not have access"));
}
next();
});
io.on("connection", (socket) => {
console.log(`connect ${socket.id}`);
socket.on("disconnect", () => {
console.log(`disconnect ${socket.id}`);
});
});Socket.IO client version: 4.8.x
Client
import { io } from "socket.io-client";
const socket = io("ws://localhost:3000/", {});
socket.on("connect", () => {
console.log(`connect ${socket.id}`);
});
socket.on("disconnect", () => {
console.log("disconnect");
});Expected behavior
When the error is thrown in the middleware, it is able to be logged in some manner. Given that the current behavior tries to respond to the open HTTP connection and throws (yet another) error "Cannot set headers after they are sent to the client", I think socket.io needs to support being provided an error logger and if not provided, automatically log such errors to console.error.
Platform:
- Device: Irrelevant.
- OS: Irrelevant.
Additional context
I would have made a PR to resolve this, but I'm sure the current maintainers would have a preferred implementation for satisfying this need. The underlying "Cannot set headers after they are sent" error is probably an artifact of the implementation here, which might be resolved anyway by adding this logging support. I'm not totally sure what the intention is for that specific method, so I'll leave that up to the maintainers.
Activity
knackstedt commentedon Dec 17, 2024
Edit: Also affects this closure.
socket.io/packages/engine.io/lib/server.ts
Line 815 in 7427109
(GitHub is currently quite laggy so I can't modify the original post :/ )
cipherprofessor commentedon Sep 24, 2026
Confirmed this on current
main. Inpackages/engine.io/lib/server.ts,_applyMiddlewares:The middleware call itself isn't wrapped in try/catch. If a registered middleware (
engine.use(...), e.g.express-session,helmet, or a user's own auth middleware) throws synchronously instead of callingnext(err), the exception propagates straight out of thehttp.Server'request'/'upgrade'event listener — Node doesn't wrap those, so depending on the host setup it either crashes the process or gets silently swallowed somewhere above. Both call sites (handleRequestandhandleUpgrade) go through this same unguarded path, matching @knackstedt's follow-up above.Proposed fix: wrap the middleware invocation in try/catch and route a caught exception into
callback(err), the same path an explicitnext(err)already takes.Before I put together a PR — is routing it through the existing
callback(err)/connection_errorpath the behavior you'd want, or would you rather it surface differently (e.g. logged and re-thrown)? Happy to add a regression test alongside the existing ones inpackages/engine.io/test/middlewares.jseither way.fix(engine.io): catch synchronous middleware errors
fix(engine.io): invoke each middleware callback at most once