Robust websocket server for use by a game running on node

This code is a server for websocket connections, it handles the low level stuff and delegates incoming messages to handler objects. I wanted performance to win in any trade off against maintenance costs or readability, but security should be as good as possible without becoming unfit for purpose.

It "works" and I'm using it for a multiplayer shooter which is near finished, but I wanted to get a second opinion on its quality, as it is a core element of the network infrastructure of the game.

I wanted it to be robust enough that bad input from a client won't allow the server to be crashed, and that bad code within handler objects is also unable to crash the server (its OK if those objects crash but this code should run indefinitely).

It has to handle errors internally so that one bad actor doesn't bring down the server for everybody else - a delay of even a few milliseconds while the server recovers from bad input would reduce the user experience on the client. If a client sends bad input it should experience a silent failure.

It also needs to compress its output to reduce traffic, so I chose a library to handle this, and it shouldn't (in theory) be possible to inject code via JSON as I've used the node library "json-safe-parse"



`"use strict";

var websocket = {
Server : require("websocket").server
};

var http = require("http");
var jsonSafeParse = require("json-safe-parse");
var lz_string = require("lz-string");

/**
* @class server.Server
* @desc Game server, handles client connections and disconnections and delegates events to any message handlers it is given
* @param {Number} port the port to run from
*/
function Server(port) {

var THIS = this;

this.port = port;

// list of currently connected clients (users)
this.clients = {};

// array of objects which will respond to messages from clients
this.messageHandlers = [];

// build HTTP server
this.server = http.createServer();
this.server.listen(this.port, function() {
console.log((n

A bunch of small things:

In .addNewClient(), you can change this:

var time = new Date().getTime();

to this:

var time = Date.now();

Then, later in that same function you can use that value rather than calling Date.now() three more times.

In .sendMessage(), you can change this:

if( typeof(client) == typeof (" "))

to this:

if( typeof(client) === "string")

You should avoid assigning to named argument variables like you are doing in .sendMessage() because this prevents some JS optimizations.

When coining your clientID, you don't need to multiply at all (you said you cared about performance). You can just leave the random number in decimal form. You're just trying to have a random string so it's no big deal if you have a decimal point in it. If you really don't want a decimal, then you could just remove the decimal with a string replace rather than multiply. Also, why time % 1000? Why not just time (it's more unique without the %)?

So, you can change this:

var clientId = (Math.random() * Math.pow(2, 32)) + "_" + time % 1000;

to this:

var clientId = Math.random() + "_" + time;

Your this.clients object would be simpler if it was a Map object because you can avoid all the hasOwnProperty() stuff and just use Map methods like .has(), .delete(), etc... It also has built in .forEach() iterator instead of your manual iteration.

On your .closeConnection() method, you are only removing the client from the this.clients data structure if client.authentication is already true. I know it's supposed to be the case that those two operations are innately tied together, but why not remove it from this.clients no matter what? You don't want any chance of a memory leak here and it's not like some random attacker can send an unauthenticated message using your client object. The client object is uniquely associated with the socket.

Change == to ===.

In .addMessageHandler(), why don't you use .indexOf() to see if a handler is already in the array rather than do your own from scratch iteration?

Bigger things to check on:

• Is your webSocket library safe from malformed packets?

• Is your webSocket library safe from DOS attacks with giant messages?

• Do you need rate limiting per connection to be safe from DOS attacks from a single connection?

• Do you know what happens if a client connection just silently disappears without an orderly TCP shut-down. Will your server eventually close the socket and remove the client object? Or do you need to check for inactive connections/clients and get rid of them?

• What happens if a single client who passes authentication, connects a zillion times?

• I don't understand your authentication step. The code you show doesn't actually do any authentication so the client gets into the this.clients map without passing anything and then you call some message handlers for authentication, but they don't have any return value to actually indicate failure.

StackExchange Code Review Q#137994, answer score: 8