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

[dimitrikochnev]

+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?

[cjhowe-us]

@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.