Enable on headers handler to reject connection modifying headers param

[sdrsdr]

A common problem is authenticating and rejecting websocket clients. The headers handler of the server is in unique position to easily reject the client after expecting all the available information from the request. This minimal patch detects the attempt to close the connection and does so in graceful manner. See the added test for easy client rejection

[lpinca]

The 'headers' event was not designed to do this. See https://github.com/websockets/ws#client-authentication on how to properly authenticate and reject connections. Another option that still works is the verifyClient hook.

[sdrsdr]

What I don't like in the recommended way is that I as a user should be familiar enough with the library so he/she knows weather to call wss.handleUpgrade and what the exact callback it should be. Especially wss.emit('connection', ws, request, client); should NOT be in the scope of the user's usage of the library. In my eyes this is HIGHLY unportable and hakish way to use the library. What I currently do in my code is "abuse" the headers event and just req.destroy() when I have to, but this does not allow me to specify an error code for the HTTP protocol and recently this become important to me, hence the pull request.

[lpinca]

What I don't like in the recommended way is that I as a user should be familiar enough with the library so he/she knows weather to call wss.handleUpgrade and what the exact callback it should be. Especially wss.emit('connection', ws, request, client); should NOT be in the scope of the user's usage of the library. In my eyes this is HIGHLY unportable and hakish way to use the library.

This is documented and it is the recommended way to reject connections as it gives the user total freedom. Calling wss.emit('connection', ws, request, client); is optional. It is only sugar, you could stop at the server.handleUpgrade() callback.

Also, as written above, the verifyClient hook can also be used to do this if you don't want to use server.handleUpgrade().

Sorry but I'm not ok with this patch. Abusing the 'headers' event is not the proper way to reject connections.

[sdrsdr]

If I'm understanding the code correctly, I can't omit the callback of server.handleUpgrade() I MUST call it exactly as specified in the "documentation" or I'll break the usual flow of the events: server.on('connection') handlers will not be called at best, runtime errors will be thrown most probably.

I'm not huge fan of my patch also but it looks way better than the proposed way to handle client filtration. As verifyClient is strongly discouraged we're only left with code snippet from a documentation that looks so tightly coupled with inner working of the library that I will expect every other npm upgrade to break it silently.

Probably you should remove the stigma from verifyClient or device a clean method to handle rejection of connections.

Most simple way, probably, would be to provide a default callback for server.handleUpgrade() so the documentation can hide the inner workings of the library and and the users can be sure that all the workflow will unfold properly no-matter the version of the library used. If you don't like the approach with optional parameters you can introduce a server.handleUpgradePostAuth() with the same arguments as server.handleUpgrade() sans the cb param that internally calls server.handleUpgrade() with proper callback

P.S. I can propose a PR for this approach if you like it better?

[lpinca]

You can't omit the callback but you don't need server.on('connection'). When server.handleUpgrade() invokes the callback, you are done. Your WebSocket instance is there. Everything else is sugar.

import WebSocket from 'ws';
import { createServer } from 'http';

const server = createServer();
const wss = new WebSocketServer({ noServer: true });

server.on('upgrade', function upgrade(request, socket, head) {
  // This function is not defined on purpose. Implement it with your own logic.
  authenticate(request, (err, client) => {
    if (err || !client) {
      socket.write('HTTP/1.1 401 Unauthorized\r\n\r\n');
      socket.destroy();
      return;
    }

    wss.handleUpgrade(request, socket, head, function done(ws) {
      // You are done. `ws` is your `WebSocket` instance, you can do
      // whatever you want with it.
    });
  });
});

server.listen(8080);

As verifyClient is strongly discouraged we're only left with code snippet from a documentation that looks so tightly coupled with inner working of the library that I will expect every other npm upgrade to break it silently.

server.handleUpgrade() is a public api that has been there since forever. It will not break on every npm upgrade.

Probably you should remove the stigma from verifyClient or device a clean method to handle rejection of connections.

The verifyClient hook will not be removed anytime soon.

[sdrsdr]

Documenting a way to break the documented workflow of the library is nonsense. Imagine I've build a websocket server according to the documentation using on('connection') handler everything is fine then I decide to add client connection filtering following the same documentation and endup breaking my perfectly working code.

I'm pretty sure everybody will prefer to maintain only code related to his/her project and emitting the connection event is strictly library's business and nobody else.

A robust library should strive to keep a stable interface keeping the user code agnostic to library's inner workings. This will allow confident version upgrades for the users and freedom to expand the library with less worries that the user code could be broken from some needed change.

server.handleUpgrade() in current form is unusable as public API IMHO. If verifyClient is not discouraged please update the documentation. See Issue #337 ;)

The most important question here: If I create a PR introducing server.handleUpgradePostAuth() that will allow proper, according to me, way to handle client connection filtration, are you willing to consider accepting it?

[lpinca]

I'm pretty sure everybody will prefer to maintain only code related to his/her project and emitting the connection event is strictly library's business and nobody else.

When using server.handleUpgrade() directly there is no 'connection' event. It's up to the user to emit it (if they want to emit it).

server.handleUpgrade() in current form is unusable as public API IMHO.

I disagree. It is used by a lot projects.

The most important question here: If I create a PR introducing server.handleUpgradePostAuth() that will allow proper, according to me, way to handle client connection filtration, are you willing to consider accepting it?

If I understand correctly server.handleUpgradePostAuth() would be just a wrapper around server.handleUpgrade(), so no, the current API is ok in my opinion and is used by thousands of projects. We have received no complaints that I remember of on the suggested way of rejecting connections. Perhaps this is the first? If you don't like the API that's ok but I also don't like abusing the 'headers' event or introducing unnecessary methods.

Thank you anyway.

[sdrsdr]

Take a look here again #377 and evaluate how user frendly and easy to use is your library. Allowing small, backward compatible and consistent changes that will benefit your users will only help your project.

At least allowing passing no callback to server.handleUpgrade() and doing the proper thing that will emit connection event is something you should strongly consider.

[lpinca]

It seems that overall users are happy with the suggested solution. Some advantages over the verifyClient hook are:

1. Total control on the socket. Users can write whatever they want on it.
2. Ability to add custom arguments to the 'connection' event (if wanted) without hacks, which is the reason why Pass verifyClient result to connection event #377 was opened in the first place.

At least allowing passing no callback to server.handleUpgrade() and doing the proper thing that will emit connection event is something you should strongly consider.

As written in #377 (comment) server.handleUpgrade() takes a callback because the verifyClient hook can be asynchronous.

Again, you might not like the API and that's perfectly fine. We can't make everyone happy. Thank you for sharing your opinion.

[sdrsdr]

I Don't see how default callback will break anybody's code but mandating copy-paste a snippet from documentation to maintain default behaviour is over the red line for me

[lpinca]

There is no default behavior, just two different modes. If users use server.handleUpgrade(), then they should rely on its callback. If users do not use server.handleUpgrade(), then they should rely on the 'connection' event. The fact that server.handleUpgrade() is also used internally is an implementation detail.

[sdrsdr]

I personally find this https://en.wikipedia.org/wiki/Principle_of_least_astonishment very important. This leads to easy code refactoring and ease of use. If there is a event called when a connection is established it should be called when connection is established no matter how the connection is established. And this should happen no matter what my code does as long as the library does not crash. But this is just my expectations and all my code tries to be as predictable as possible but this is not my library and you're free to code it as you like