Robust websocket server for use by a game running on node

Question

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((new Date()) + " Server is listening on port " + THIS.port);
	});
	
	// build websocket server, which is attached to the HTTP server
	this.wsServer = new websocket.Server({httpServer: this.server});
	
	// connect the server callbacks to this object
	this.__connectCallbacks();
}

/**
 * @method Server#getClients
 * @desc get the clients on this server
 * @returns {Object} a hash of the clients
 */
Server.prototype.getClients = function(){
	return this.clients;
};

/**
 * @method Server#getClient
 * @desc Get the client with the given ID, or null if none exists
 * @param {String} clientId
 * @returns {Client|null} client if one is found, null otherwise
 */
Server.prototype.getClient = function(clientId) {
	
	// return null if no client was found
	if (!this.clients.hasOwnProperty(clientId)) {
		return null;
	}
	
	// if it was found, we can return the client
	return this.clients[clientId];
};

/**
 * @method Server#addMessageHandler
 * @desc Add a message handler - this object will be called when messages come through that the server doesnt handle internally
 * @param {Object} handler the handler object 
 */
Server.prototype.addMessageHandler = function(handler){
	
	// guarantee that its not already attached
	for (var i = 0; i < this.messageHandlers.length; i++) {
		if (this.messageHandlers[i] == handler) {
			return;
		}
	}

	this.messageHandlers.push(handler);
};

/**
 * @method Server#serialize
 * @desc take this object and turn it into a string for transportation using lzw compression
 * @param  {Object} object the object to be serialized, be sure it doesn't contain cycles
 * @return {String} a serialized string
 */
Server.prototype.serialize = function(object) {

	var json = JSON.stringify(object);

	var lz = lz_string.compressToUTF16(json);

	return lz;
};

/**
 * @method Server#sendMessages
 * @desc Send a message
 * @param {Client|String} client who to send it to - either their ID or the actual client
 * @param {String} type message type
 * @param {Object} params object of all the parameters
 */
Server.prototype.sendMessage = function(client, type, params) {

	if( typeof(client) == typeof (" ")) {
		client = this.getClient(client);
	}
	
	var message = {
		type : type,
		params : params
	};

	// encode as a string, utf format
	client.connection.sendUTF(this.serialize(message));
};

/**
 * @method Server#broadcastMessage
 * @desc Broadcast the given message to all clients
 * @param {String} type the type of message
 * @param {Object} params the parameters of the message
 */
Server.prototype.broadcastMessage = function(type, params) {

	// send a message to each client
	for (var send_k in this.clients) {
		if (this.clients.hasOwnProperty(send_k)) {
			this.sendMessage(this.clients[send_k], type, params);
		}
	}
};

// Hook up all the callbacks that the server will require
Server.prototype.__connectCallbacks = function() {
	var THIS = this;
	this.wsServer.on('request', function(request) {
		return THIS.addNewClient(request);
	});
};

/**
 * @method Server#handleAuthentication
 * @desc handle an authentication message on the given client
 * @param client the client
 * @param params the parameters of auth message
 */
Server.prototype.handleAuthentication = function(client, params) {

	// if they are already authenticated, ignore this
	if (client.authenticated) {
		return;
	}

	// the server then has to check the database to ensure the client was registered
	client.authenticated = true;

	// give a new nick
	client.nickname = params.requested_name + Math.round(Math.random() * 255);

	// now that they've authenticated, make the client permanent
	this.clients[client.clientId] = client;

	// send back the "accepted" string along with their new nickname, which may be different than what they wanted
	console.log("Connection accepted for client " + client.clientId);

	// send the client's details, include times to allow the client to synchronise with the server
	var connectionAcceptedParams = {
		clientId  : client.clientId,
		nickname  : client.nickname,
		lastTime    : client.lastTime,
		currentTime : client.currentTime
	};

	// send
	this.sendMessage(client, 'CONNECTION_ACCEPTED', connectionAcceptedParams);
	
	// now delegate to message handlers
	for ( var i = 0; i < this.messageHandlers.length; i++) {
		try {
			this.messageHandlers[i].handleClientAuthentication(client);
		} catch(err) {
			console.error(err);
			console.error(err.stack);
		}
	}

};

/**
 * @method Server#handleNetworkMessage
 *  @desc handle messages (other than authentication, which is seperate) on the given client
 *  @param client the client the message came from
 *  @param messageType the type of message
 *  @param params the paramaters of the message
 */
Server.prototype.handleNetworkMessage = function(client, messageType, params) {

	// until they are authenticated, ignore other types of message - silently fail
	if (!client.authenticated) {
		return;
	}

	// update the client's "last seen" time
	client.lastSeenTime = Date.now();

	// response to ping includes the given send time, so the client can judge latency
	if (messageType === "PING") {
		this.sendMessage(client, "ACK", {sendTime : params.sendTime});
	}

	// now delegate to message handlers
	for ( var i = 0; i < this.messageHandlers.length; i++) {
		try {
			this.messageHandlers[i].handleNetworkMessage(client, messageType, params);
		} catch(err) {
			console.error(err);
			console.error(err.stack);
		}
	}

};

/**
 * @method Server#onMessage
 * @desc called when Recieved a message from a client
 * @param client the client it came from
 * @param message the contents of the message
 */
Server.prototype.onMessage = function(client, message) {

	// as far as the client is concerned, silently fail if the server had an error
	try {

		// accept only utf8
		// silently fail from the client's perspecive
		if (message.type !== 'utf8') { 
			return;
		}

		// safely parse the json - this library restricts certain things which may allow code injections
		message = jsonSafeParse(message.utf8Data);

		// make message types case insensitive
		var messageType   = message.type.toUpperCase();
		var messageParams = message.params;	

		// specific type of message which the server will always handle itself, without delegating the handshaking
		if (messageType === "CONNECTION_REQUEST") {
			this.handleAuthentication(client, messageParams);

		// for all other messages, process normally
		} else {
			// now route the message through this function
			this.handleNetworkMessage(client, messageType, messageParams);
		}

	} catch(err) {
		console.error(err);
		console.error(err.stack);
		return;
	}

};

/**
 * @method Server#closeConnection
 * @desc CLose the connection to the given client
 * @param client the client to disconnect
 */
Server.prototype.closeConnection = function(client) {

    if (client.authenticated) {
    	console.log("Disconnecting client " + client.clientId);
    	
		// now delegate to message handlers - all messages except authentication may be passed down to clients
		for ( var i = 0; i < this.messageHandlers.length; i++) {
			try {
				this.messageHandlers[i].handleClientDisconnect(client);

			} catch(err) {
				console.error(err);
				console.error(err.stack);
			}
		}
		
		this.broadcastMessage("CLIENT_DISCONNECT", {clientId : client.clientId});
		
        // remove user from the list of connected clients
        delete this.clients[client.clientId];
    }
};

/**
 * Called when a new client connects 
 * @param request the data from the remote websocket
 */
Server.prototype.addNewClient = function(request) {

	var THIS = this;

	// get a new client ID
	var time = new Date().getTime();
	var clientId = (Math.random() * Math.pow(2, 32)) + "_" + time % 1000;

	// accept connection
    var connection = request.accept(null, request.origin); 

    // build the initial client object
	var client = {
		clientId      : clientId,
		nickname      : "anonymous",
		authenticated : false,
		connection    : connection,
		lastTime      : Date.now(),
		currentTime   : Date.now(),
		
		lastSeenTime : Date.now()
	};

	// when a message is received, delegate to this function
	connection.on('message', function(message) {
		return THIS.onMessage(client, message);
	});

	// when connection is closed, delegate to this function
	connection.on('close', function() {
		return THIS.closeConnection(client);
	});

};

// export public stuff
exports.Server = Server;

Answer 1

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:

1. Is your webSocket library safe from malformed packets?
2. Is your webSocket library safe from DOS attacks with giant messages?
3. Do you need rate limiting per connection to be safe from DOS attacks from a single connection?
4. 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?
5. What happens if a single client who passes authentication, connects a zillion times?
6. 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.