socketio / socketio/socket.io

Errors thrown by middlewares do not get captured or reported

Open
#5,263 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

to triage
Dominant language
TypeScript
Stars
63.2k
Forks
10.3k
Avg merge
11d 20h
Merged PRs (30d)
2

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:
https://github.com/socketio/socket.io/blob/7427109658591e7ce677a183a664d1f5327f37ea/packages/engine.io/lib/server.ts#L772

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at packages/engine.io/lib/server.ts around line 772 and trace how middleware-thrown errors are handled and how the HTTP response is attempted. Determine how errors should be surfaced, including the proposed logger or console.error fallback, and verify that unexpected middleware errors are reported without producing the secondary headers error.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.