Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customize response headers with Express middleware #668

Closed
thernstig opened this issue Jan 24, 2023 · 6 comments
Closed

Customize response headers with Express middleware #668

thernstig opened this issue Jan 24, 2023 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@thernstig
Copy link

Is your feature request related to a problem? Please describe.
A common Express middleware needs an interface like (req, res, next). A common example would be helmet() which adds securtiy headers in responses.

But the new headers introduced in https://socket.io/blog/socket-io-4-1-0/#add-a-way-to-customize-the-response-headers does not get the res in the callback:

io.engine.on("initial_headers", (headers, req) => {
  headers["test"] = "123";
  headers["set-cookie"] = "mycookie=456";
});

io.engine.on("headers", (headers, req) => {
  headers["test"] = "789";
});

This means we cannot use common Express middleware to add headers properly.

Describe the solution you'd like
I would have wanted this:

io.engine.on("initial_headers", (headers, req, res) => {
  helmet()(req, res);
});

io.engine.on("headers", (headers, req) => {
  helmet()(req, res);
});

Describe alternatives you've considered
Our current solution is this which is a hack we want to replace with the new solution of io.engine.on("initial_headers" and io.engine.on("headers"

/**
 * Attach the socket.io server to an HTTP(S) server.
 *
 * @param {http.Server|https.Server} server - Server to attach to
 */
function attachSocketIoToServer(server) {
  const socketIoPath = `socket.io`;
  io.attach(server, {
    serveClient: false,
    path: socketIoPath,
  });

  // Workarounds to add secure headers to socket.io responses. Different
  // workarounds are needed for polling and websocket transports.
  //
  // 1. Polling transport: When attaching to a server, socket.io removes all
  //    other listeners of the 'request' event and adds itself. If the path
  //    matches the socket.io path it handles the request, otherwise it calls
  //    the other listeners itself.
  //
  //    By doing the same thing, we can use Helmet to add the headers to the
  //    response before socket.io gets involved.
  if (server.listeners('request').length !== 1) {
    throw new Error('Unexpected number of "request" listeners on server');
  }
  const socketIoListener = server.listeners('request')[0];
  server.removeAllListeners('request');
  server.on('request', (req, res) => {
    // @ts-ignore -- Fault in Node.js types, see https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/54920
    if (req.url.startsWith(socketIoPath)) {
      helmet(getHelmetOptions())(req, res, () => {
        socketIoListener(req, res);
      });
    } else {
      socketIoListener(req, res);
    }
  });
  // 2. Websocket transport: The underlying 'ws' package emits a 'headers' event
  //    when the headers are ready, which is meant to be used for inspecting or
  //    modifying the headers before they are sent.
  //
  //    We hook into this after the server has begun listening, since engine.io
  //    recreates the ws object at that point.
  server.on('listening', () => {
    io.eio.ws.on('headers', (headers) => {
      headers.push(
        'something something'
      );
    });
  });
}
@thernstig thernstig added the enhancement New feature or request label Jan 24, 2023
@darrachequesne
Copy link
Member

Hi! As a temporary workaround, res can be found in the req object:

io.engine.on("headers", (headers, req) => {
  if (req.res) { // only for HTTP requests (not during WebSocket upgrade)
    helmet()(req, req.res);
  }
});

The only problem with the suggestion there is that the WebSocket upgrade does not expose a ServerResponse object, so we would need to create a dummy ServerResponse object and forward any call to the setHeader() method.

@thernstig
Copy link
Author

thernstig commented Jan 25, 2023

In socketio/socket.io#4609 you suggested:

io.engine.use(yourMiddleware);

What we are using now are two ways to add middlewares, using the same Express middlewares we have.

For something like helmet() to add headers to the HTTP response during the WebSocket HTTP upgrade request/response, we use the above.

For e.g. authentication middlware we use something like:

const authMiddleware = (req, res, next) => {
  passport.authenticate('session')(req, res, next);
};

io.of(namespace).use((socket, next) =>
  authMiddleware(socket.request, {}, next)
);

That works fine. But was mostly for the first part of using helmet() we needed something else.

But maybe you are onto something of adding a better way to add generic middleware that works in both scenarios?

darrachequesne added a commit that referenced this issue Feb 6, 2023
This commit implements middlewares at the Engine.IO level, because
Socket.IO middlewares are meant for namespace authorization and are not
executed during a classic HTTP request/response cycle.

A workaround was possible by using the allowRequest option and the
"headers" event, but this feels way cleaner and works with upgrade
requests too.

Syntax:

```js
engine.use((req, res, next) => {
  // do something

  next();
});

// with express-session
import session from "express-session";

engine.use(session({
  secret: "keyboard cat",
  resave: false,
  saveUninitialized: true,
  cookie: { secure: true }
});

// with helmet
import helmet from "helmet";

engine.use(helmet());
```

Related:

- #668
- #651
- socketio/socket.io#4609
- socketio/socket.io#3933
- a lot of other issues asking for compatibility with express-session
@darrachequesne
Copy link
Member

Here we go! Improved support for Express middlewares was added in 24786e7, included in engine.io@6.4.0 and socket.io@4.6.0:

io.engine.use(yourMiddleware);

Added in the documentation there: https://socket.io/docs/v4/middlewares/#compatibility-with-express-middleware

Please reopen if needed.

@darrachequesne darrachequesne added this to the 6.4.0 milestone Feb 8, 2023
@thernstig
Copy link
Author

thernstig commented Feb 8, 2023

@darrachequesne maybe I am a bit tired now.

The example in https://socket.io/docs/v4/middlewares/#compatibility-with-express-middleware shows to use this with:

io.engine.use(session({}));

For the passport-session and session() function does it not make more sense to put this on a io.of(namespace).use() call so it is called on every new sockect connection event, instead of just the initial HTTP request?

Would then not a better option be to show the usage of https://github.com/helmetjs/helmet as:

import session from "express-session";

io.engine.use(helmet());

Maybe I am missing how the session() call works and that it is not needed per socket connection attempt but rather per HTTP request (even if there is just one done by socket.io for the HTTP upgrade request/response cycle)?

@darrachequesne
Copy link
Member

For the passport-session and session() function does it not make more sense to put this on a io.of(namespace).use() call so it is called on every new sockect connection event, instead of just the initial HTTP request?

Even if you use a Socket.IO middleware (io.of(namespace).use()), socket.request refers to the initial HTTP request, so the result will be the same.

That being said, the example could indeed be improved because we only need to run it once I think:

import session from "express-session";

const sessionMiddleware = session({
  secret: "keyboard cat",
  resave: false,
  saveUninitialized: true,
  cookie: { secure: true }
});

io.engine.use((req, res, next) => {
  if (req._query.sid === undefined) {
    // first HTTP request
    sessionMiddleware(req, res, next);
  } else {
    // subsequent HTTP requests
    next();
  }
});

@thernstig
Copy link
Author

@darrachequesne I see.

But then I am not sure the idea of showing how to use sessions with Express middleware as done in https://socket.io/docs/v4/middlewares/#compatibility-with-express-middleware is great. As it could make user use that pattern for authentication when it might not be ideal. It is of course great for the initial HTTP request.

Hence why I think showing usage with helmet() is better as that is explicitly for HTTP responses.

The rationale I have is that one would want to make the session authentication done for every new socket.io socket. Which is ideally done with io.on("new_namespace", ...). Or even on even better on a "connection" event https://socket.io/docs/v4/server-instance/#connection i.e. that on a new_namespace event one sets up the ``"connection"` event there. So all namespaces are and new sockets to all of them are authenticated.

I might be entirely incorrect here in my reasoning. My goal is of course to make this as usability friendly as possible for everyone to have authentication logic with socket.io that works the best security-wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants