Worker dies when you pass an error object to next() inside MIDDLEWARE_HANDSHAKE_WS

[maxwellhaydn]

According to the documentation for MIDDLEWARE_HANDSHAKE_WS, you can pass an error object to next() to block a connection before the underlying WebSocket is created. However, doing so causes the SocketCluster worker to crash:

$ node server
   [Busy] Launching SocketCluster
   !! The sc-hot-reboot plugin is watching for code changes in the /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp directory
   >> Broker PID: 15257
   >> WorkerCluster PID: 15258
   >> Worker PID: 15259
   [Active] SocketCluster started
            Version: 14.3.2
            Environment: dev
            WebSocket engine: ws
            Port: 8000
            Master PID: 15256
            Worker count: 1
            Broker count: 1

1542668941065 - Origin: Worker (PID 15259)
   [Warning] Error: This is an error object, not a string
    at Object.<anonymous> (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/worker.js:20:14)
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:228:33
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1105:9
    at replenish (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:982:17)
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:986:9
    at _asyncMap (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1103:5)
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1189:16
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1016:16
    at Object.<anonymous> (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:227:20)
    at Object.<anonymous> (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:89:12)
1542668941065 - Origin: Worker (PID 15259)
   [Error] TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be one of type string, Buffer, or ArrayBuffer. Received type object
    at Function.byteLength (buffer.js:514:11)
    at abortHandshake (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/ws/lib/websocket-server.js:342:33)
    at options.verifyClient (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/ws/lib/websocket-server.js:208:33)
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/socketcluster-server/scserver.js:661:13
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1110:9
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:460:16
    at iterateeCallback (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:962:17)
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:944:16
    at /Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/node_modules/async/dist/async.js:1107:13
    at Object.<anonymous> (/Users/maxwellcarey/projects/socketcluster-middleware-handshake-ws-obj-not-string/myApp/worker.js:20:9)
1542668941071 - Worker 0 exited - Exit code: 1
   >> Worker PID: 15262
1542668941343 - Worker 0 was respawned

Here's the worker.js that generated the above output:

var SCWorker = require('socketcluster/scworker');
var express = require('express');
var serveStatic = require('serve-static');
var path = require('path');

class Worker extends SCWorker {
  run() {
    console.log('   >> Worker PID:', process.pid);
    var environment = this.options.environment;

    var httpServer = this.httpServer;
    var scServer = this.scServer;

    var app = express();
    app.use(serveStatic(path.resolve(__dirname, 'public')));

    httpServer.on('request', app);

    scServer.addMiddleware(scServer.MIDDLEWARE_HANDSHAKE_WS, function(req, next) {
        next(new Error('This is an error object, not a string'));
    });
  }
}

new Worker();

---

This happens because the WebSocket backend's verifyClient callback expects the third argument to be a string, but SocketCluster is passing the error object. See the docs for ws verifyClient:

if verifyClient is provided with two arguments then those are:

• info {Object} Same as above.
• cb {Function} A callback that must be called by the user upon inspection of the info fields. Arguments in this callback are:
  • result {Boolean} Whether or not to accept the handshake.
  • code {Number} When result is false this field determines the HTTP error status code to be sent to the client.
  • name {String} When result is false this field determines the HTTP reason phrase.
  • headers {Object} When result is false this field determines additional HTTP headers to be sent to the client. For example, { 'Retry-After': 120 }.

It can be fixed in socketcluster-server as follows (although it would be better to check whether err is actually an object with a message property):

--- scserver.js	2018-10-11 01:11:20.000000000 -0500
+++ scserver.js.new	2018-11-19 17:27:44.000000000 -0600
@@ -658,7 +658,7 @@ SCServer.prototype.verifyHandshake = fun
             } else if (self.middlewareEmitWarnings) {
               self.emit('warning', err);
             }
-            cb(false, 401, err);
+            cb(false, 401, err.message);
           } else {
             cb(true);
           }

[jondubois]

@maxwellhaydn thanks for reporting this. For some reason I cannot reproduce the issue (maybe a settings difference). Also the docker image doesn't seem to be affected.

That said, the third argument passed to the cb should in fact be a string; so I agree with your suggestion. We need to account for the possibility that the developer might pass back a string to the next function; so maybe it should be:

var errorReason = typeof err === 'string' ? err : err.message;
cb(false, 401, errorReason);

Also, we need to do the same with the other cb(...) below.

[maxwellhaydn]

Ah yes, I like your solution of accepting a string or an Error object better.

[jondubois]

@maxwellhaydn Ok, I published the patch in [email protected].

[maxwellhaydn]

@jondubois Great! Thanks for the quick turnaround!