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

Pass verifyClient result to connection event #377

Open
pwnall opened this issue Oct 16, 2014 · 72 comments
Open

Pass verifyClient result to connection event #377

pwnall opened this issue Oct 16, 2014 · 72 comments

Comments

@pwnall
Copy link

pwnall commented Oct 16, 2014

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
Copy link

luin commented Feb 4, 2015

+1

1 similar comment
@danturu
Copy link

danturu commented Feb 8, 2016

+1

@ChrisZieba
Copy link

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
Copy link

@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
Copy link

oh and +1 ;)

@dweremeichik
Copy link

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
Copy link

cjhowedev commented Oct 18, 2017

@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
Copy link

dmax1 commented Feb 1, 2018

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
Copy link

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

@iatsiuk
Copy link

iatsiuk commented Aug 27, 2018

@marcelijanowski yep:

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

  done(true);
});

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

@zoltan-mihalyi
Copy link

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
Copy link

yaroslav-ilin commented Dec 26, 2018

+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
Copy link

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

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

@yaroslav-ilin
Copy link

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
Copy link
Member

lpinca commented Feb 10, 2019

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
Copy link
Member

lpinca commented Feb 10, 2019

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
Copy link

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

@yaroslav-ilin
Copy link

yaroslav-ilin commented Feb 12, 2019

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

@lpinca
Copy link
Member

lpinca commented Feb 12, 2019

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

@dderevjanik
Copy link

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
Copy link

sneels commented Jul 9, 2019

@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
Copy link

@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
Copy link

mgreenw commented Aug 7, 2019

@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
Copy link
Member

lpinca commented Aug 7, 2019

A good alternative is the one proposed by @zoltan-mihalyi here #377 (comment). Adding stuff to the WebSocket or http.IncomingMessage object should be user decision and not the default (return value of verifyClient).

@mgreenw
Copy link

mgreenw commented Aug 7, 2019

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

@mateuscb
Copy link

mateuscb commented Mar 26, 2021

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
Copy link
Member

lpinca commented Mar 26, 2021

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
Copy link

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.

@gladius
Copy link

gladius commented Dec 18, 2021

Taking cue from #377 (comment) I was able to pass decoded JWT token from verifyClient by setting the value into request.headers object and retrieving it in connection event's request object.

I am not sure if this is the right way to do it, but this works.

const { WebSocketServer } = require('ws');

const wss = new WebSocketServer({
  port: 8080, verifyClient: async ({ req }, cb) => {
    const { headers } = req;
    if (headers.token) {
      const decodedToken = await decodeToken(headers.token);
      req.headers.decodedToken = JSON.stringify(decodedToken);
      cb(true);
    } else {
      cb(false, 401, 'Unauthorized');
    }
  }
});


wss.on('connection', (ws, req)=> {
  console.log("on Connection decodedToken ==> ", JSON.parse(req.headers.decodedToken));
});

@sdrsdr
Copy link

sdrsdr commented Dec 18, 2021

Why not just attach the object to the request with som resonably named property? This is what JS is made for 😃

@Esqarrouth
Copy link
Contributor

From my socket connection, I'm trying to set a jwt and send it back as an http response, or somehow set the cookie of the client. Am I in the right thread? Is there a recommended method for this?

@sdrsdr
Copy link

sdrsdr commented Jan 31, 2022

From my socket connection, I'm trying to set a jwt and send it back as an http response, or somehow set the cookie of the client. Am I in the right thread? Is there a recommended method for this?

I think this is what headers event is for take a look at https://github.com/websockets/ws/blob/master/lib/websocket-server.js#L387 and line 352. But why go this way if you can just pass the JWT back to the client with a simple message

@Esqarrouth
Copy link
Contributor

From my socket connection, I'm trying to set a jwt and send it back as an http response, or somehow set the cookie of the client. Am I in the right thread? Is there a recommended method for this?

I think this is what headers event is for take a look at https://github.com/websockets/ws/blob/master/lib/websocket-server.js#L387 and line 352. But why go this way if you can just pass the JWT back to the client with a simple message

@sdrsdr Thank you, I will give it a try.
Meanwhile can you explain what you meant by passing JWT back to client with a simple message? What kind of message?

@sdrsdr
Copy link

sdrsdr commented Feb 1, 2022

You create websocket connection to pas messages between peers a browser an a server or two servers. The establishment of the connection folows fixed rules but after that you can send anything you like to your peer, including the JWT token, and the peer can handle it as it's most cinvinient

@rooton
Copy link

rooton commented Jul 2, 2022

Hi. Sorry, probably stupid question, but It is suggested to use

socket.write("HTTP/1.1 401 Unauthorized\r\n\r\n")
socket.destroy()

It is destroyed, but how to get HTTP/1.1 401 Unauthorized on client?

On client there is simple code.

const ws = new WebSocket("ws://127.0.0.1:8080/")

ws.onerror = (event) => {
  console.log("WebSocket onerror", event)
}

ws.onclose = (event) => {
  console.log("WebSocket onclose", event)
}

Dev console (Network/WS) has Request, but no Response tab

@trasherdk
Copy link

@rooton You can find a bunch of examples: Suggested handleUpgrade and connection flow

@farr64
Copy link

farr64 commented Jul 25, 2022

Hello @trasherdk:

Thanks for the examples.

Out of curiosity, I checked your repo https://github.com/trasherdk/ws and saw that you mention:

This branch is up to date with websockets/ws:master.

Could you please share the motivation for your branch?

Thanks.

@rooton
Copy link

rooton commented Nov 26, 2022

@rooton You can find a bunch of examples: Suggested handleUpgrade and connection flow

Thank you, but you are just copy/paste example. And my question is about socket.write. It is impossible to read message on client side to ensure thats an auth error.

@constantind
Copy link

my 5 cents: verifyClient is the only place to deploy connection rate limits, even tough it is not in the spec it is the ideal place to ban connections with 429 before authentication

@trasherdk
Copy link

@farr64 The reason for my clone/fork is test snippets

It's pretty much answers to other peoples issues, routed into folders on my mail-server. This way I don't have to look for answers in closed issues, but have a collection of stuff I find interesting.

The trasherdk-snippets branch was to avoid locked issue template, but opted for discussions later 😄

@trasherdk
Copy link

@constantind You are probably right about the rate-limit and verifyClient thingy. It makes sense to do that as early as possible.

@samal-rasmussen
Copy link

samal-rasmussen commented Aug 17, 2023

@trasherdk wrote:

@rooton You can find a bunch of examples: Suggested handleUpgrade and connection flow

I'm trying to figure out how to get an authentication failed close reason from a browser WebSocket client, just like @rooton did, and the linked examples don't answer this at all. The browser websocket doesn't produce a http response of any sort that you can inspect for status codes, so socket.write("HTTP/1.1 401 Unauthorized\r\n\r\n") and socket.destroy() will just kill the websocket without giving any close reason to the browser websocket.

I've moved the authentication handling inside the wss.handleUpgrade like this because the ws.close call can respond with a code and reason that the websocket.onclose handler on the browser websocket will actually get.

httpServer.on("upgrade", (request, socket, head) => {
  wss.handleUpgrade(request, socket, head, (ws) => {
    const authResult = authenticate();
    if (!authResult.ok) {
      ws.close(4000, "authentication failed");
    }
    wss.emit("connection", ws, request);
  });
});

I don't know if there are real costs/drawbacks to handling it here and not a level above in the httpServer.on("upgrade" handler. I would love it if anyone could share some wisdom on this.

@prests
Copy link

prests commented Aug 26, 2023

@samal-rasmussen did you ever determine if there were any cons associated with this? I'm also in the same boat and would love to know what's the best practice here!

@rotivleal
Copy link

rotivleal commented Apr 3, 2024

Does anyone know how do I get the server response (handshake response) in the client? I can only get 1006 (abnormal closure) in the close event. Is there really not a way to inspect the handshake response in this library?

EDIT: I had to use the unexpected-response event

@samal-rasmussen
Copy link

@samal-rasmussen did you ever determine if there were any cons associated with this? I'm also in the same boat and would love to know what's the best practice here!

Works fine so far 🤷‍♂️

@dlong500
Copy link

dlong500 commented May 2, 2024

@lpinca Can you comment on the approach @samal-rasmussen is using to actually return failure codes back to a client? I've struggled to see the point of writing an HTTP response code to the socket before destroying it in the http server upgrade handler because it doesn't appear that the client ever receives any response at all.

Yet such a pattern (writing a response code to the socket) seems to be used in all of the documentation involving authentication.

@lpinca
Copy link
Member

lpinca commented May 2, 2024

That is "ok" but in that case the authentication happens after the WebSocket connection is established. The 'open' event is emitted on the client. If you want to prevent the connection from being established you have to close it during the handshake.

@dlong500
Copy link

dlong500 commented May 2, 2024

Yes, that appears to be the trade-off. So are you confirming that there is no way to return an error code to the client (at least in a way that a browser can see it) without first establishing a websocket connection?

@lpinca
Copy link
Member

lpinca commented May 2, 2024

In the browser client, no. Other clients (like ws) might allow you to read the HTTP response.

@samal-rasmussen
Copy link

That is "ok" but in that case the authentication happens after the WebSocket connection is established. The 'open' event is emitted on the client. If you want to prevent the connection from being established you have to close it during the handshake.

Yes. This also means that you cannot assume that you are authenticated when you get the open event on the client. The client must wait for a message from the server than confirms the validation first.

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

No branches or pull requests