Node websockets

Question

I have this code I use it for two things

1. connect to lxd instances and emits any operation events it recieves over socket.io to the client
2. Bridge between client and lxd for terminals

Its pretty poorly written so any pointers would be great (but it works fine)

// This originated from https://gist.github.com/CalebEverett/bed94582b437ffe88f650819d772b682
// and was modified to suite our needs
const fs = require('fs'),
    WebSocket = require('ws'),
    express = require('express'),
    https = require('https'),
    mysql = require('mysql'),
    expressWs = require('express-ws'),
    path = require('path'),
    cors = require('cors');

const envImportResult = require('dotenv').config({
    path: "/var/www/LxdMosaic/.env"
});

if (envImportResult.error) {
    throw envImportResult.error
}

// Https certificate and key file location for secure websockets + https server
var privateKey = fs.readFileSync(process.env.CERT_PRIVATE_KEY, 'utf8'),
    certificate = fs.readFileSync(process.env.CERT_PATH, 'utf8');
    certDir = "/var/www/LxdMosaic/src/sensitiveData/certs/",
    lxdConsoles = [],
    credentials = {
        key: privateKey,
        cert: certificate
    },
    app = express();

app.use(cors());
var bodyParser = require('body-parser')
app.use( bodyParser.json() );       // to support JSON-encoded bodies
app.use(bodyParser.urlencoded({     // to support URL-encoded bodies
  extended: true
}));

var httpsServer = https.createServer(credentials, app);
var io = require('socket.io')(httpsServer);

var operationSocket = io.of("/operations")
// expressWs(app, httpsServer);

var con = mysql.createConnection({
    host: process.env.DB_HOST,
    user: process.env.DB_USER,
    password: process.env.DB_PASS,
    database: process.env.DB_NAME
});

var hostDetails = {};

function createExecOptions(host, container) {
    return {
        method: 'POST',
        host: hostDetails[host].hostWithOutProtoOrPort,
        port: hostDetails[host].port,
        path: '/1.0/containers/' + container + '/exec',
        cert: fs.readFileSync(hostDetails[host].cert),
        key: fs.readFileSync(hostDetails[host].key),
        rejectUnauthorized: false
    }
}

const lxdExecBody = JSON.stringify({
    "command": ["bash"],
    "environment": {
        "HOME": "/root",
        "TERM": "xterm",
        "USER": "root"
    },
    "wait-for-websocket": true,
    "interactive": true,
})


con.connect(function(err) {
    if (err) {
        throw err;
    }
});

function createWebSockets() {
    con.query("SELECT * FROM Hosts", function(err, result, fields) {
        if (err) {
            throw err;
        }
        for (i = 0; i < result.length; i++) {
            let lxdClientCert = certDir + result[i].Host_Cert_Only_File
            let lxdClientKey = certDir + result[i].Host_Key_File

            if(result[i].Host_Online == 0){
                continue;
            }

            // Connecting to the lxd server/s
            const wsoptions = {
                cert: fs.readFileSync(lxdClientCert),
                key: fs.readFileSync(lxdClientKey),
                rejectUnauthorized: false,
            }

            var portRegex = /:[0-9]+/;

            let stringUrl = result[i].Host_Url_And_Port;
            let urlURL = new URL(result[i].Host_Url_And_Port);

            let hostWithOutProto = stringUrl.replace("https://", "");
            let hostWithOutProtoOrPort = hostWithOutProto.replace(portRegex, "");

            hostDetails[result[i].Host_Url_And_Port] = {
                cert: lxdClientCert,
                key: lxdClientKey,
                hostWithOutProtoOrPort: hostWithOutProtoOrPort,
                port: urlURL.port
            };

            var ws = new WebSocket('wss://' + hostWithOutProto + '/1.0/events?type=operation', wsoptions);

            ws.on('message', function(data, flags) {
                var buf = Buffer.from(data)
                let message = JSON.parse(data.toString());
                message.host = hostWithOutProtoOrPort;
                operationSocket.emit('operationUpdate', message);
            });
        }
    });
}


httpsServer.listen(3000, function() {});


app.get('/hosts/reload/', function(req, res) {
    createWebSockets();
    res.send({
        success: "reloaded"
    });
});

app.post('/hosts/message/', function(req, res) {
    console.log(req.body.data);
    operationSocket.emit(req.body.type, req.body.data);
    res.send({
        success: "delivered"
    });
});

app.get('/', function(req, res) {
    res.sendFile(path.join(__dirname + '/index.html'));
});

var terminalsIo = io.of("/terminals");

terminalsIo.on("connect", function(socket) {

    let indentifier = socket.handshake.query.pid;

    if(lxdConsoles[indentifier] == undefined) {
        let host = socket.handshake.query.host;
        let container = socket.handshake.query.container;


        let execOptions = createExecOptions(host, container);

        const wsoptions = {
            cert: execOptions.cert,
            key: execOptions.key,
            rejectUnauthorized: false
        }

        const lxdReq = https.request(execOptions, res => {
            res.on('data', d => {

                const output = JSON.parse(d);

                if(output.hasOwnProperty("error") && output.error !== ""){
                    socket.emit("data", "Container Offline");
                    return false;
                }

                const lxdWs = new WebSocket('wss://' +
                    execOptions.host + ':' + execOptions.port + output.operation +
                    '/websocket?secret=' + output.metadata.metadata.fds['0'],
                    wsoptions
                );

                lxdWs.on('error', error => console.log(error));

                lxdWs.on('message', data => {
                    try {
                        const buf = Buffer.from(data);
                        data = buf.toString();
                        socket.emit("data", data);
                    } catch (ex) {
                        // The WebSocket is not open, ignore
                    }
                });
                lxdConsoles.push(lxdWs);
            });
        });
        lxdReq.write(lxdExecBody);
        lxdReq.end();
    }

    //NOTE When user inputs from browser
    socket.on('data', function(msg) {
        lxdConsoles[indentifier].send(msg, {
            binary: true
        }, () => {});
    });

    socket.on('close', function(indentifier) {
        setTimeout(() => {
            if(lxdConsoles[indentifier] == undefined){
                return
            }
             lxdConsoles[indentifier].send('exit  \r', { binary: true }, function(){
                lxdConsoles[indentifier].close();
                delete lxdConsoles[indentifier];
            });
         }, 100);
    });
});

app.post('/terminals', function(req, res) {
    // Create a indentifier for the console, this should allow multiple consolses
    // per user
    res.send(lxdConsoles.length.toString());
});


createWebSockets();

(This comes from an open source project I maintain here so if you do give an answer be aware it may be used there)

Answer 1

In node, throwing an error without catching it will kill the process. This seems to be the primary error handling mode (and based on this it seems you then rely on pm2 to restart the process). This means that if even one client manages to trigger an error in your code, every client will be disconnected. Fixing this would require rewriting a lot of the code to handle asynchronous errors correctly -- for example, make createWebSockets return a promise and reject that promise if the DB query errors, and then handle that error in /hosts/reload/ and pass the error along to the client.

---

createWebSockets() calls fs.readFileSync in a loop -- if there are a large number of hosts this could cause the whole server to freeze while the certs/keys are being reread for every host. You could consider both caching keys that have already been read (so you're not just rereading the same keys on every reload) and/or using the asynchronous fs.readFile API. (If you're familiar with promises you can use async/await and fs.promises.readFile to keep a similar code structure)

---

/hosts/reload/ sounds like it is intended to be run many times to reload the list of hosts, but createWebSockets() doesn't handle reloads any differently than the initial setup. This has a few issues:

• If a host is deleted from the DB, hostDetails will still include information about the host
• The events?type=operation websockets are setup, but then the handle to them is thrown away. So not only are errors and host deletions not handled, but a fresh websocket will also be created for hosts that already existed during the previous reload. This means there will be multiple websockets connected to a single host, with their listeners still forwarding messages. This is not only a memory leak, but it also means there will be duplicate operationUpdate messages being sent over operationSocket.
  • Similarly, long lived sockets like these should usually have error handling and retry logic.

---

if(lxdConsoles[indentifier] == undefined) {

and

res.send(lxdConsoles.length.toString());

It looks like lxdConsoles.length is used as a unique identifier (here) -- but this isn't guaranteed to work if two clients both try and open a console at the same time. It could happen that they both request POST /terminals, get the same id back, and then both try to pass that same id back to the websocket.

An easy fix would be to switch lxdConsoles to be an object instead of an array and have the POST /terminals handler just return a uuid instead of lxdConsoles.length. Though since the server doesn't authenticate the token at all, we may as well just have the client generate the uuid.

---

res.on('data', d => { is not the correct way to read response data from an HTTP request. The data event signifies that a single chunk of data has been received, not the full message. Technically this code needs to have a buffer, store up all of the data chunks, and then process the full buffer on the end event.

Roughly:

let body = '';
res.on('data', d => body += d.toString('utf-8'));
res.on('end', () => { /* do something with the complete "body" */ });

Or just use a simpler HTTP library, like request-promise

Based on the LXD Docs, the returned message is guaranteed to be small... but this code is still fragile (if LXD ever changes to return more data, or if the URL is changed to something that returns more data, this could break).

---

 try {
     const buf = Buffer.from(data);
     data = buf.toString();
     socket.emit("data", data);
 } catch (ex) {
     // The WebSocket is not open, ignore
 }

This is concerning: try/catch is being used to just throw away an error without even logging it (so users may have mysterious behavior with no potential to even see the cause in logs) and also the comment reads weirdly: there's two websockets in play here (the client socket and the server lxdWs so the comment should specify which and also describe why the error doesn't need to be handled)

---

socket.on('close', function(indentifier) {
    setTimeout(() => {
        // ...
     }, 100);
});

It's not clear what the setTimeout is doing here -- if it's needed for some specific timing thing, then a comment would be a good idea. Otherwise you can remove it.

---

Small style things:

• The code has inconsistent whitespace, quoting, var/const usage, and semicolons. Just use prettier and never worry about it again.
• indentifier is spelled wrong.