Pass verifyClient result to `connection` event

[pwnall]

I'm doing some expensive work in verifyClient and I'd like to reuse the result in the connection event handler.

I'm currently using the fact that the undocumented WebSocket property upgradeReq is the same request as info.req in verifyClient, and I'm modifying the request. This feels dirty though.

Will you please consider allowing any truthy verifyClient result, and passing it into the connection event handler?

If this seems reasonable, I'd be glad to prepare a pull request.

[luin]

+1

[danturu]

+1

[ChrisZieba]

This would be a nice addition. I'm using jwt's in my verifyClient code and it would be nice to cleanly save the decoded result fo use in the connection handler. Something like this:

  const wss = new WebSocketServer({
    server: server,
    verifyClient: function(info, done) {
      let query = url.parse(info.req.url, true).query;
      jwt.verify(query.token, config.jwt.secret, function(err, decoded) {
        if (err) return done(false, 403, 'Not valid token');

        // Saving the decoded JWT on the client would be nice 
        done(true);
      });
    }
  });

  wss.on('connection', ws => {
   // get decoded JWT here?
  });

[stellanhaglund]

@pwnall do you store the jwt as a cookie or do you use it as part of the url?

I'm implementing jwt into my code but I'm not quite sure how to deliver the token.

Any suggestions?

[stellanhaglund]

oh and +1 ;)

[pwnall]

@stefanocudini I'm not using jwt. Here is what I'm doing.

https://github.com/pwnall/w3gram-server/blob/70a3024527e72f184cfb4d0139de218f96690848/src/ws_connection.coffee#L44 https://github.com/pwnall/w3gram-server/blob/70a3024527e72f184cfb4d0139de218f96690848/src/ws_connection.coffee#L58 https://github.com/pwnall/w3gram-server/blob/70a3024527e72f184cfb4d0139de218f96690848/src/ws_connection.coffee#L22

I hope this helps.

[dweremeichik]

Perhaps #1099 makes this functionality a little more public? Out of curiosity, how are you handling the invalid token on the client side? Using the 1006 error?

[cjhowedev]

@ChrisZieba I ended up verifying the JWT in my connection handler as well to get the decoded data. Did you find a better solution?

[dmax1]

To avoid double JWT encoding, I used global object (pertainInfosThroughConnectionProcess) where I store info’s that I want to retrieve upon opening connection. To distinguish to point right connection as key name I use JWT token itself.

var pertainInfosThroughConnectionProcess = {};

const wss = new WebSocketServer({
  server: server,
  verifyClient: function(info, done) {
    let query = url.parse(info.req.url, true).query;
    jwt.verify(query.token, config.jwt.secret, function(err, decoded) {
      if (err) return done(false, 403, 'Not valid token');

      // Using jwt as key name and storing uid
      pertainInfosThroughConnectionProcess[jwt] = decoded.uid;
      // Using jwt as key name and storing numerous values in object
      pertainInfosThroughConnectionProcess[jwt] = {
        uid: decoded.uid,
        anotherKey: 'another value',
        oneMoreKey: 'one more value'
      };

      done(true);
    });
  }
});

wss.on('connection', ws => {
  // Now we can use uid from global obejct pertainInfosThroughConnectionProcess
  // Note: I used 'sec-websocket-protocol' to send JWT in header, so upon opening connection I can access it with ws.protocol
  var uid = pertainInfosThroughConnectionProcess[ws.protocol];
  // or if you saved numerous values in object
  var uid = pertainInfosThroughConnectionProcess[ws.protocol].uid;
  var anotherKey = pertainInfosThroughConnectionProcess[ws.protocol].anotherKey;
  var oneMoreKey = pertainInfosThroughConnectionProcess[ws.protocol].oneMoreKey;

  // After retrieving data, we can delete this key value as is no longer needed
  // Note: delete is costly operation on object and there is way to optimize it, however for this purpose is not too bad
  delete pertainInfosThroughConnectionProcess[ws.protocol];
});

[marcelijanowski]

Is there a better way to do it rather than setting up global var?

[iatsiuk]

@marcelijanowski yep:

verifyClient: function({ req }, done) {
  req.jwt = jwt.verify(
    // ...
  );

  done(true);
});

wss.on('connection', (ws, req) => {
  const jwt = req.jwt;
});

[zoltan-mihalyi]

I solved this problem using the request object and a WeakMap.

const userRequestMap = new WeakMap();

const server = new ws.Server({
    port,
    verifyClient: (info, done) => {
        const user = authenticateUser(info);
        userRequestMap.set(info.req, user);
        done(user !== null);
    },
});

server.on('connection', (connection, request) =>{
    const user = userRequestMap.get(request);
});

[yaroslav-ilin]

+1 on this.

I will most likely end up with mutating info.req approach, but it seems fragile and doesn't play well with TypeScript out of the box. It also requires the on('connection', ...) handler to process the second argument which I wouldn't need otherwise (the same concern as in #1099). Instead it would be nice to have some documented way to approach this.

Right now the async verifyClient can invoke the callback with up to 4 arguments in case when result is false, but the truthy case suddenly doesn't care about the other 3 arguments. I'd propose to utilize one of these 3 and have connection object extended with some new property (say, verificationResult), so that whatever the developer passes to the second argument of the callback appears on that new property.

For example,

const server = new ws.Server({
    verifyClient: (info, callback) => {
        const verificationResult = { userId: 123 };
        callback(true, verificationResult);
    },
});

server.on('connection', (connection) =>{
    const { userId } = connection.verificationResult;
});

I'd keep the sync implementation of verifyClient as it is now, because sync computations IMHO either fairly cheep to be repeated inside on('connection', ...) handler or may not be needed there at all. And for those rare cases when the precomputed values may actually be needed, the developer should be able to refactor the code to use the callback instead.

This doesn't seem like a breaking change. Any concerns?

[dderevjanik]

@nilfalse that's what i'm looking for !

Are there any plans to implement that feature ? It would definetly help...

[yaroslav-ilin]

I would be glad to investigate & contributing this feature. But until any indication from a maintainer, it doesn't make sense to even start implementing it.

[lpinca]

verifyClient is a technical debt imho and the only reason why WebSocketServer.prototype.handleUpgrade() is async. I would like to remove it completely. Instead of using it, use something like this

const http = require('http');
const WebSocket = require('ws');

const server = http.createServer();
const wss = new WebSocket.Server({ noServer: true });

wss.on('connection', function connection(ws, request, ...args) {
  // ...
});

server.on('upgrade', async function upgrade(request, socket, head) {
  // Do what you normally do in `verifyClient()` here and then use
  // `WebSocketServer.prototype.handleUpgrade()`.
  let args;

  try {
    args = await getDataAsync();
  } catch (e) {
    socket.destroy();
    return;
  }

  wss.handleUpgrade(request, socket, head, function done(ws) {
    wss.emit('connection', ws, request, ...args);
  });
});

server.listen(8080);

It gives the developer a lot of more freedom.

[lpinca]

Anyway, adding custom data to the request object as suggested in some comments above, is ok. It's an established pattern in Express and Koa, for example, to pass data between middleware.

[bfailing]

@lpinca getDataAsync is undefined. What is it doing exactly?

[yaroslav-ilin]

@bfailing this is the code that you would otherwise have in your verifyClient

[lpinca]

@bfailing it is undefined because it is an example. getDataAsync is an example function to get some data you may need.

[dderevjanik]

Guys, thank you very much for all answers :)

@lpinca your answer was especially very helpful... and yes, after implementing it that ways I can understand tech debt behind verifyClient

[sneels]

@lpinca this is probably a dumb question, but how do I get the server.on('upgrade', async function upgrade(request, socket, head) to trigger?

I have my server generate a token to authenticate the connection by doing a post first, then the websocket client sends this token along in the initial connection, but I can't seem to trigger the update part

[netizen-ais]

@lpinca this is probably a dumb question, but how do I get the server.on('upgrade', async function upgrade(request, socket, head) to trigger?

I have my server generate a token to authenticate the connection by doing a post first, then the websocket client sends this token along in the initial connection, but I can't seem to trigger the update part

The websocket connection is a simple HTTP GET request with an "Upgade: Websocket" header. Look at the sample from @lpinca, the "http server" he creates receives the websocket client connections

[mgreenw]

@lpinca I'm worried that the proposed client auth solution enforces noServer: true and adds boilerplate to the auth process. I currently use an externally defined server and want to continue using it with all of the server options I have configured.

I think something like @nilfalse / @adrianhopebailie's solution would be more clear and reduce boilerplate that everyone wanting to do client auth will eventually need to write.

What is your main concern with something like #1612? Do you think there is an in-between solution that allows sync connections to be made while also allowing a dev to pass custom args into the 'connect' event from verifyClient in an async manner if desired?

One idea is to allow verifyClient to return either a value or a Promise. If verifyClient throws or the resulting Promise throws, then the client is not verified. If it resolves, then that value is passed into connect. This solution obviously ignores the code, name, and headers HTTP response if the verifications fails. I just want to spur additional discussion.

Contrived Async Example

async authenticateUser(info) {
  ...
}

const server = new ws.Server({
    verifyClient: (info) => {
       return authenticateUser(info);
    }
});

Contrived Sync Example

const users = {
  one: 'userOneId',
  two: 'userTwoId',
};

const server = new ws.Server({
    verifyClient: (info) => {
       return users[info.req.headers.user];
    }
});

[lpinca]

A good alternative is the one proposed by @zoltan-mihalyi here https://github.com/websockets/ws/issues/377#issuecomment-420169849. Adding stuff to the WebSocket or http.IncomingMessage object should be user decision and not the default (return value of verifyClient).

[mgreenw]

@lpinca Thanks for the reply! I agree that the solution proposed by @zoltan-mihalyi is good and fits most use cases. If that is a "supported" solution, then verifyClient should not be deprecated (#1613) as it is the only way to achieve that behavior.

[lpinca]

It is not currently deprecated, its use is discouraged. The difference is small but verifyClient is heavily used so it is not going away anytime soon.

[mgreenw]

@lpinca Gotcha, that makes complete sense. Thanks for the clarification!

[kcwitt]

@lpinca how to return error (401, 404, etc.) if the websocket connection is not authorized or the resource is not found, etc.?

Per the ws unit tests, the verifyClient function can be used as follows (for example to return a 404 error). verifyClient: (info, cb) => process.nextTick(cb, false, 404)

How to do this in your example above since there is no response object?

[lpinca]

In the same way it is done implicitly by verifyClient. See https://github.com/websockets/ws/blob/fa991731cca990f40ecedb120918d14d08129673/lib/websocket-server.js#L384-L406

This is why in my opinion with the usage suggested above the developer has more freedom. You can literally write whatever you want to the socket.

[kcwitt]

@lpinca Thanks!

By the way, do you know why socket.removeListener is called explicitly? I assumed socket would be available for garbage collection after the call to socket.destroy(), including its reference to socketOnError.

[lpinca]

It is not strictly needed but it does not harm.

[crussell52]

@lpinca

To make the preferred usage easier, how do you feel about making abortHandshake() part of the public API?

This way those who want to implement custom handshake preprocessing are not forced to re-implement abort logic. Instead, it would look something like this:

server.on('upgrade', async function upgrade(request, socket, head) {
  // Do what you normally do in `verifyClient()` here and then use
  // either `WebSocketServer.prototype.handleUpgrade()` OR 
  // `WebSocketServer.prototype.abortHandshake()
  let args;

  try {
    args = await getDataAsync();
  } catch (e) {
    wss.abortHandshake(socket, 400, "Invalid Request", {"x-custom-header": "foo"});
    return;
  }

  wss.handleUpgrade(request, socket, head, function done(ws) {
    wss.emit('connection', ws, request, ...args);
  });
});

[lpinca]

@crussell52 I prefer developers to write their own abortHandshake() helper if they have to. It's trivial to do.

[crussell52]

I prefer developers to write their own abortHandshake() helper if they have to. It's trivial to do.

I think "if they have to" is they key phrase. The current recommendation requires developers to do so even if they want the default abort behavior.

I guess it could be argued that they only "need to" if handshakes need to be aborted as part of the custom upgrade handler... But this is being recommended as a replacement for verifyClient() and aborting the handshake seems like a really common aspect of verifying the client 😬. By extension, re-implementing the abortHandshake() behavior becomes very common for those trying to follow the documented recommendations.

Allowing the ability to invoke the default behavior does not prevent developers from implementing their own, but it does reduce the cases that require developers to implement their own.

I realize that I don't know the nuances of this library, so Is there a particular risk to moving the behavior to the public API that I'm missing?

(edited for formatting)

[lpinca]

The current recommendation requires developers to do so even if they want the default abort behavior.

No, they can just call socket.destroy() if they don't want to write a full HTTP response back to the client. abortHandshake() can be replaced with a single socket.write() call in many cases.

socket.write('HTTP/1.1 400 Bad Request\r\n\r\n');
socket.destroy();

You can write JSON in the body, etc.

[crussell52]

It is still forced reimplementation of existing, proven, tested, and versioned behavior. It seems unfortunate that developers do not have access to invoke the built-in abort behavior. :(

I don't have any more feedback that would further the conversation and I appreciate the energy you put in as a maintainer.

I've already made a near-copy of the default behavior for this project and it is the only WS project I have on my plate right now. For future reference are you open to a pull request that gives developers access to the default abort behavior?

[lpinca]

It was not designed to be a public API. It's too limiting and hard to use and I prefer to not change it to fit every possible use case. It should be trivial and only handle cases where the request is invalid, like wrong HTTP method, wrong sec-websocket-key header, etc. Specifically

https://github.com/websockets/ws/blob/08c6c8ba70404818f7f4bc23eb5fd0bf9c94c039/lib/websocket-server.js#L208

https://github.com/websockets/ws/blob/08c6c8ba70404818f7f4bc23eb5fd0bf9c94c039/lib/websocket-server.js#L226

Everything else (custom status code, custom status message, custom headers) was added due to verifyClient and it would be great if it could go away with it one day.

http.ServerResponse would give the developer a more user friendly and familiar interface.

const { ServerResponse } = require('http');

const res = new ServerResponse({ httpVersionMajor: 1, httpVersionMinor: 1 });

res.assignSocket(socket);
res.shouldKeepAlive = false;

res.on('finish', function() {
  res.detachSocket(socket); // Not strictly needed.
  socket.destroySoon();
});

res.writeHead(/* ... */);
res.end(/* ... */);

[fungiboletus]

I understand that verifyClient is deprecated for good reasons, but I also think it would be good to keep a similar high level API. The recommended alternative works but I wouldn't dare to do more than copy pasting your code because things can go horribly wrong there.

I compared both versions:

Using verifyClient

const WebSocket = require('ws');

function getDataAsync() {
  return new Promise((resolve) => setTimeout(resolve.bind(null, 'Hello world'), 500));
}

const wss = new WebSocket.Server({
  port: 8080,
  verifyClient: async (info, done) => {
    let data;
    try {
      data = await getDataAsync();
    } catch (error) {
      done(false, 500, 'internal server error');
      return;
    }

    info.req.data = data;
    done(true);
  },
});

wss.on('connection', (ws, request) => {
  ws.send(request.data);
});

Using the recommended method

const http = require('http');
const WebSocket = require('ws');

function getDataAsync() {
  return new Promise((resolve) => setTimeout(resolve.bind(null, 'Hello world'), 500));
}

const server = http.createServer();
const wss = new WebSocket.Server({ noServer: true });

wss.on('connection', (ws, request, data) => {
  ws.send(data);
});

server.on('upgrade', async (request, socket, head) => {
  let data;

  try {
    data = await getDataAsync();
  } catch (error) {
    const res = new http.ServerResponse({ httpVersionMajor: 1, httpVersionMinor: 1 });

    res.assignSocket(socket);
    res.shouldKeepAlive = false;

    res.on('finish', () => {
      res.detachSocket(socket); // Not strictly needed.
      socket.destroySoon();
    });

    res.writeHead(500, 'internal server error');
    res.end();
    return;
  }

  wss.handleUpgrade(request, socket, head, (ws) => {
    wss.emit('connection', ws, request, data);
  });
});

server.listen(8080);

[lpinca]

@fungiboletus see https://github.com/websockets/ws/issues/377#issuecomment-537176176.

In your example you are only using res.writeHead() without specifying any headers so you could do something like this:

const http = require('http');
const WebSocket = require('ws');

function getDataAsync() {
  return new Promise((resolve) => setTimeout(resolve.bind(null, 'Hello world'), 500));
}

const server = http.createServer();
const wss = new WebSocket.Server({ noServer: true });

wss.on('connection', (ws, request, data) => {
  ws.send(data);
});

server.on('upgrade', async (request, socket, head) => {
  let data;

  try {
    data = await getDataAsync();
  } catch (error) {
    socket.write(`HTTP/1.1 500 ${http.STATUS_CODES[500]}\r\n\r\n`);
    socket.destroy();
    return;
  }

  wss.handleUpgrade(request, socket, head, (ws) => {
    wss.emit('connection', ws, request, data);
  });
});

server.listen(8080);

which is simpler than the verifyClient version.

[ghost]

Is there a way to use the recommended approach (verifying the client in the 'upgrade' event handler) when the WebSocket.Server is instantiated with an http server?

[lpinca]

@jplymale-jt no, because in that case the 'upgrade' listener is added internally.

[eliecer2000]

I know how to use verifyClient in the backend, what I don't know is how I send the header from the client, I can't find any example to help me.

Someone knows where I find an example code for the client to send headers for verification

[taiho007008]

I know how to use verifyClient in the backend, what I don't know is how I send the header from the client, I can't find any example to help me.

Someone knows where I find an example code for the client to send headers for verification

I also having this problem

[LongTengDao]

@lpinca

verifyClient is a technical debt imho and the only reason why WebSocketServer.prototype.handleUpgrade() is async. I would like to remove it completely.

Now that verifyClient is the only reason, could async/sync behaviour depends on there is or not a handleUpgrade callback (which means no verifyClient or verifyClient is sync)? It seems that remove verifyClient option is long time to wait...

const wss = new ( require('ws').Server )({
  noServer: true,
  verifyClient: null,// or sync
});

require('http').createServer().on('upgrade', async (request, socket, head) => {

  // Do what you normally do in `verifyClient()` here and then use `handleUpgrade`.
  try { await getDataAsync(); } catch (e) { return void socket.destroy(); }

  const ws = wss.handleUpgrade(request, socket, head);

  // Directly do things here,
  // which avoided to emit and listen exec on eventEmitter (for cpu),
  // and avoided to create one closure cb function (for memory).

}).listen(8080);;

Or, more simply, add the second argument (request) for the callback:

const wss = new ( require('ws').Server )({ noServer: true });

function onConnection (ws, request, head) { }

require('http').createServer().on('upgrade', (request, socket, head) => {
  wss.handleUpgrade(request, socket, head, onConnection);
}).listen(8080);;

How do you like it?

[emanavas]

this configuration its working for me, I have a ws client on microcontroller esp32. We most to know the standard ws header dont have Basic authorization so I add it in my ws client library and I recive the request of connection, if its valid acept or refuse. (some old version of browsers you cant use wss://user:pass@hostname:port/path to pass credentials)

const wss = new WebSocket.Server({
    url,
    server: httpsServer,
    perMessageDeflate: false,
    verifyClient: (info, callback) => {
        try {
            var authHeader = info.req.headers.authorization
            console.log(authHeader)
        } catch (error) {
            callback(false,203 ,"lack of authorization")
            return
        }
       var auth = new Buffer(authHeader.split(' ')[1], 'base64').toString().split(':')
       var username = auth[0]
       var password = auth[1]
       console.log("user="+username+" pass="+ password)

       if(username == 'edemone' && password =='secret'){
            callback(true,200,"Come in, We have job to do..")
       }else{
            callback(false,401,"authorization refused")
       }
    },
});

another option its acept the connection wait a second msg on upgrade request and ask fot credentials to continue the connection.

[ckvv]

I know how to use verifyClient in the backend, what I don't know is how I send the header from the client, I can't find any example to help me.

Someone knows where I find an example code for the client to send headers for verification

you can try in this way

node

  server.on('upgrade', async (request, socket) => {
    const protocol = request.headers['sec-websocket-protocol'];

    const user = await jwtVerify(protocol, config.cookie.key);

    if (!user || !user.id) {
      socket.write('HTTP/1.1 401 Unauthorized\r\n\r\n');
      socket.destroy();
      return;
    }
    request.user = user;
  });

in brower

const socket = new WebSocket('ws://localhost:9998',['protocol']);

[mateuscb]

I had this same question as @jplymale-jt

Is there a way to use the recommended approach (verifying the client in the 'upgrade' event handler) when the WebSocket.Server is instantiated with an http server?

The answer from @lpinca was the examples in this thread wouldn't work.

@jplymale-jt no, because in that case the 'upgrade' listener is added internally.

Is there some other way to prevent an upgrade if it is instantiated with an http server? I'm trying to prevent upgrade if the origin does not match to prevent CSRF ~(CORS).~

[lpinca]

Is there some other way to prevent an upgrade if it is instantiated with an http server? I'm trying to prevent upgrade if the origin does not match (CORS).

https://github.com/websockets/ws/blob/master/doc/ws.md#servershouldhandlerequest.

[fungiboletus]

I'm trying to prevent upgrade if the origin does not match (CORS).

Hi. I just want to be a bit pedantic and mention that WebSockets do not have CORS. Any website can open WebSocket connections towards other websites. And with CORS it's not up to the server to refuses a connection but to the client. Preventing a WebSocket upgrade if the origin does not match is fine though, it's just not CORS.