Add workarounds to fudge & accept mildly invalid WebSocket protocols

Without this, WS will reject invalid subprotocols, for empty header
values, or some syntactically incorrect values. Some real clients send
these headers in ways that are accepted, so it's very useful to be able
to intercept them, and the intent is clear: an empty-string value should
be a null not-set protocol (and is generally treated as such).

In a perfect world, we'd forward the subprotocol exactly as-is without
this, and avoid taking an opinion entirely as long as we could do basic
handling, but WS has opinions and ties our hands on that.

Author: pimterry

src/rules/websockets/websocket-handlers.ts

@@ -346,10 +346,25 @@ export class PassThroughWebSocketHandler extends PassThroughWebSocketHandlerDefi
 
         // Subprotocols have to be handled explicitly. WS takes control of the headers itself,
         // and checks the response, so we need to parse the client headers and use them manually:
-        const subprotocols = findRawHeaders(rawHeaders, 'sec-websocket-protocol')
+        const originalSubprotocols = findRawHeaders(rawHeaders, 'sec-websocket-protocol')
             .flatMap(([_k, value]) => value.split(',').map(p => p.trim()));
 
-        const upstreamWebSocket = new WebSocket(wsUrl, subprotocols, {
+        // Drop empty subprotocols, to better handle mildly badly behaved clients
+        const filteredSubprotocols = originalSubprotocols.filter(p => !!p);
+
+        // If the subprotocols are invalid (there are some empty strings, or an entirely empty value) then
+        // WS will reject the upgrade. With this, we reset the header to the 'equivalent' valid version, to
+        // avoid unnecessarily rejecting clients who send mildly wrong headers (empty protocol values).
+        if (originalSubprotocols.length !== filteredSubprotocols.length) {
+            if (filteredSubprotocols.length) {
+                 // Note that req.headers is auto-lowercased by Node, so we can ignore case
+                req.headers['sec-websocket-protocol'] = filteredSubprotocols.join(',')
+            } else {
+                delete req.headers['sec-websocket-protocol'];
+            }
+        }
+
+        const upstreamWebSocket = new WebSocket(wsUrl, filteredSubprotocols, {
             maxPayload: 0,
             agent,
             lookup: getDnsLookupFunction(this.lookupOptions),

test/integration/websockets.spec.ts

@@ -1,5 +1,6 @@
 import * as net from 'net';
 import * as WebSocket from 'isomorphic-ws';
+import * as http from 'http';
 import * as https from 'https';
 import HttpProxyAgent = require('http-proxy-agent');
 import HttpsProxyAgent = require('https-proxy-agent');
@@ -188,7 +189,6 @@ nodeOnly(() => {
                 ]);
             });
 
-
             it("forwards the incoming requests' & resulting response's subprotocols", async () => {
                 mockServer.forAnyWebSocket().thenPassThrough();
 
@@ -225,6 +225,54 @@ nodeOnly(() => {
                 ]);
             });
 
+            it("ignores mildly invalid blank (empty string) subprotocol headers in incoming requests", async () => {
+                await mockServer.forAnyWebSocket().thenPassThrough();
+                const request = https.request(`https://localhost:${wsPort}`, {
+                    agent: new HttpProxyAgent(`http://localhost:${mockServer.port}`),
+                    headers: {
+                        'Connection': 'Upgrade',
+                        'Upgrade': 'websocket',
+                        'Sec-WebSocket-Version': 13,
+                        'Sec-WebSocket-Key': 'DxfWc2xtQqmWYmU/n8WUWg==',
+                        'Sec-WebSocket-Protocol': ' ' // Empty headers are invalid
+                    }
+                }).end();
+
+                const response = await new Promise<http.IncomingMessage>((resolve, reject) => {
+                    request.on('response', resolve);
+                    request.on('upgrade', resolve);
+                    request.on('error', reject);
+                });
+
+                expect(response.statusCode).to.equal(101);
+                expect(response.headers['sec-websocket-protocol']).to.equal(undefined);
+            });
+
+            it("handles mildly invalid non-empty subprotocol headers in incoming requests", async () => {
+                await mockServer.forAnyWebSocket().thenPassThrough();
+                const request = https.request(`https://localhost:${wsPort}`, {
+                    agent: new HttpProxyAgent(`http://localhost:${mockServer.port}`),
+                    headers: {
+                        'Connection': 'Upgrade',
+                        'Upgrade': 'websocket',
+                        'Sec-WebSocket-Version': 13,
+                        'Sec-WebSocket-Key': 'DxfWc2xtQqmWYmU/n8WUWg==',
+                        'Sec-WebSocket-Protocol': ' ', // Empty headers are invalid
+                        'sec-webSocket-protocol': 'a,,b', // Badly formatted other protocols
+                        'echo-ws-protocol-index': '0'
+                    }
+                }).end();
+
+                const response = await new Promise<http.IncomingMessage>((resolve, reject) => {
+                    request.on('response', resolve);
+                    request.on('upgrade', resolve);
+                    request.on('error', reject);
+                });
+
+                expect(response.statusCode).to.equal(101);
+                expect(response.headers['sec-websocket-protocol']).to.equal('a');
+            });
+
             it("can handle & proxy invalid client frames upstream", async () => {
                 mockServer.forAnyWebSocket().thenPassThrough();