Emit abort handshake event

[emanuele-dedonatis]

Is there an existing issue for this?

• I've searched for any related issues and avoided creating a duplicate issue.

Description

No response

ws version

8.6.0

Node.js Version

16

System

No response

Expected result

During the handleUpgrade it would be nice an event to catch the handshake fail

Actual result

I'm not able to catch upgrade errors, like in the attached screenshot where the client is sending an invalid sec-websocket-key

Attachments

[lpinca]

What would you do with this? It's basically only spam as it means the client is not following the spec.

[emanuele-dedonatis]

Our ws clients are deployed on remote devices and old firmwares are affected by that sec-websocket-key issue.

Until now we were using ws 6.0.0 which didn't validate that header (it's before commit [fix] Abort the handshake if the Sec-WebSocket-Key header is invalid)

Updating to ws 8.6.0, this handshake was failing but we were unable to understand the cause. That's why that event would be useful in our case. If it could be spam for some user, it could be ignored.

[lpinca]

The server already informs the client that something is wrong with the request by replying with a 400 status code. I'm not comfortable to create and emit an error (even if hidden behind an option) every time a bad request is received (e.g. when the request method is not GET, if the value of Upgrade header is not websocket, if there is no Sec-WebSocket-Key header, etc.).

[lpinca]

Something like this

diff --git a/lib/websocket-server.js b/lib/websocket-server.js
index d0a2978..6939e2f 100644
--- a/lib/websocket-server.js
+++ b/lib/websocket-server.js
@@ -236,15 +236,59 @@ class WebSocketServer extends EventEmitter {
         : false;
     const version = +req.headers['sec-websocket-version'];
 
-    if (
-      req.method !== 'GET' ||
-      req.headers.upgrade.toLowerCase() !== 'websocket' ||
-      !key ||
-      !keyRegex.test(key) ||
-      (version !== 8 && version !== 13) ||
-      !this.shouldHandle(req)
-    ) {
-      return abortHandshake(socket, 400);
+    if (req.method !== 'GET') {
+      abortHandshake(socket, 400, 'Invalid HTTP method');
+      return;
+    }
+
+    if (req.headers.upgrade.toLowerCase() !== 'websocket') {
+      abortHandshake(
+        socket,
+        400,
+        'The Updgrade header field value is not valid'
+      );
+      return;
+    }
+
+    if (!key) {
+      abortHandshake(
+        socket,
+        400,
+        'The Sec-WebSocket-Key header field is missing'
+      );
+      return;
+    }
+
+    if (!keyRegex.test(key)) {
+      abortHandshake(
+        socket,
+        400,
+        'The Sec-WebSocket-Key header field value is not valid'
+      );
+      return;
+    }
+
+    if (req.headers['sec-websocket-version'] === undefined) {
+      abortHandshake(
+        socket,
+        400,
+        'The Sec-WebSocket-Version header field is missing'
+      );
+      return;
+    }
+
+    if (version !== 8 && version !== 13) {
+      abortHandshake(
+        socket,
+        400,
+        'The Sec-WebSocket-Version header field value is not valid'
+      );
+      return;
+    }
+
+    if (!this.shouldHandle(req)) {
+      abortHandshake(socket, 400);
+      return;
     }
 
     const secWebSocketProtocol = req.headers['sec-websocket-protocol'];
@@ -254,7 +298,11 @@ class WebSocketServer extends EventEmitter {
       try {
         protocols = subprotocol.parse(secWebSocketProtocol);
       } catch (err) {
-        return abortHandshake(socket, 400);
+        abortHandshake(
+          socket,
+          400,
+          'The Sec-WebSocket-Protocol header field value is not valid'
+        );
       }
     }
 
@@ -279,7 +327,13 @@ class WebSocketServer extends EventEmitter {
           extensions[PerMessageDeflate.extensionName] = perMessageDeflate;
         }
       } catch (err) {
-        return abortHandshake(socket, 400);
+        abortHandshake(
+          socket,
+          400,
+          'The Sec-WebSocket-Extensions header field value is not valid or ' +
+            'acceptable'
+        );
+        return;
       }
     }

might make more sense but I'm still not very happy with it.