fix: close rejected HTTP/2 WebSocket handshake streams

lib/web/websocket/connection.js

@@ -101,13 +101,13 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
         // The presence of a session property on the socket indicates HTTP2
         // HTTP1
         if (response.socket?.session == null) {
-          failWebsocketConnection(handler, 1002, 'Received network error or non-101 status code.', response.error)
+          failHandshake(handler, response, 1002, 'Received network error or non-101 status code.', response.error)
           return
         }
 
         // HTTP2
         if (response.status !== 200) {
-          failWebsocketConnection(handler, 1002, 'Received network error or non-200 status code.', response.error)
+          failHandshake(handler, response, 1002, 'Received network error or non-200 status code.', response.error)
           return
         }
       }
@@ -122,7 +122,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
       //    header list results in null, failure, or the empty byte
       //    sequence, then fail the WebSocket connection.
       if (protocols.length !== 0 && !response.headersList.get('Sec-WebSocket-Protocol')) {
-        failWebsocketConnection(handler, 1002, 'Server did not respond with sent protocols.')
+        failHandshake(handler, response, 1002, 'Server did not respond with sent protocols.')
         return
       }
 
@@ -138,7 +138,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
       //    _Fail the WebSocket Connection_.
       //    For H2, no upgrade header is expected.
       if (response.socket.session == null && response.headersList.get('Upgrade')?.toLowerCase() !== 'websocket') {
-        failWebsocketConnection(handler, 1002, 'Server did not set Upgrade header to "websocket".')
+        failHandshake(handler, response, 1002, 'Server did not set Upgrade header to "websocket".')
         return
       }
 
@@ -148,7 +148,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
       //    MUST _Fail the WebSocket Connection_.
       //    For H2, no connection header is expected.
       if (response.socket.session == null && response.headersList.get('Connection')?.toLowerCase() !== 'upgrade') {
-        failWebsocketConnection(handler, 1002, 'Server did not set Connection header to "upgrade".')
+        failHandshake(handler, response, 1002, 'Server did not set Connection header to "upgrade".')
         return
       }
 
@@ -165,7 +165,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
         const secWSAccept = response.headersList.get('Sec-WebSocket-Accept')
         const digest = crypto.hash('sha1', keyValue + uid, 'base64')
         if (secWSAccept !== digest) {
-          failWebsocketConnection(handler, 1002, 'Incorrect hash received in Sec-WebSocket-Accept header.')
+          failHandshake(handler, response, 1002, 'Incorrect hash received in Sec-WebSocket-Accept header.')
           return
         }
       }
@@ -184,7 +184,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
         extensions = parseExtensions(secExtension)
 
         if (!extensions.has('permessage-deflate')) {
-          failWebsocketConnection(handler, 1002, 'Sec-WebSocket-Extensions header does not match.')
+          failHandshake(handler, response, 1002, 'Sec-WebSocket-Extensions header does not match.')
           return
         }
       }
@@ -205,7 +205,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options)
         // the selected subprotocol values in its response for the connection to
         // be established.
         if (requestProtocols === null || !requestProtocols.includes(secProtocol)) {
-          failWebsocketConnection(handler, 1002, 'Protocol was not set in the opening handshake.')
+          failHandshake(handler, response, 1002, 'Protocol was not set in the opening handshake.')
           return
         }
       }
@@ -307,6 +307,16 @@ function closeWebSocketConnection (object, code, reason, validate = false) {
   }
 }
 
+function failHandshake (handler, response, code, reason, cause) {
+  // The H2 upgrade request has already completed and handed off its stream.
+  // Aborting the request cannot close that stream after handshake validation fails.
+  if (response.socket?.session != null && !response.socket.destroyed) {
+    response.socket.destroy()
+  }
+
+  failWebsocketConnection(handler, code, reason, cause)
+}
+
 /**
  * @param {import('./websocket').Handler} handler
  * @param {number} code

test/fixtures/websocket-h2-rejected-handshake.js

@@ -0,0 +1,44 @@
+'use strict'
+
+const assert = require('node:assert/strict')
+const { createSecureServer } = require('node:http2')
+const { once } = require('node:events')
+const { key, cert } = require('@metcoder95/https-pem')
+const { Agent, WebSocket } = require('../..')
+
+async function main () {
+  let session
+  let receivedConnect = false
+  const server = createSecureServer({ key, cert, settings: { enableConnectProtocol: true } })
+  server.on('session', (s) => { session = s })
+  server.on('stream', (stream) => {
+    receivedConnect = true
+    stream.on('error', () => {})
+    // The status is accepted for H2, but the protocol is missing or unexpected.
+    // Keep the stream open until after WebSocket has reported the failure.
+    stream.respond(process.argv[2] === 'wrong-protocol'
+      ? { ':status': 200, 'sec-websocket-protocol': 'echo' }
+      : { ':status': 200 })
+  })
+
+  server.listen(0)
+  await once(server, 'listening')
+
+  const dispatcher = new Agent({ allowH2: true, connect: { rejectUnauthorized: false } })
+  const ws = new WebSocket(`wss://localhost:${server.address().port}`, { dispatcher, protocols: ['chat'] })
+  ws.addEventListener('error', () => {})
+  ws.addEventListener('open', () => { throw new Error('unexpected WebSocket open') })
+  const [event] = await once(ws, 'close')
+  assert.strictEqual(receivedConnect, true)
+  assert.strictEqual(event.code, 1006)
+
+  // Closing the session must not surface an unhandled error on an orphaned stream.
+  session.destroy()
+  await new Promise((resolve) => server.close(resolve))
+  await dispatcher.close()
+}
+
+main().catch((err) => {
+  console.error(err)
+  process.exitCode = 1
+})

test/websocket/h2-rejected-handshake.js

@@ -0,0 +1,21 @@
+'use strict'
+
+const { test } = require('node:test')
+const { spawnSync } = require('node:child_process')
+const { join } = require('node:path')
+
+// An unhandled ClientHttp2Stream 'error' exits the process, so test in a child.
+for (const response of ['missing-protocol', 'wrong-protocol']) {
+  test(`a rejected H2 WebSocket handshake (${response}) does not crash when the session closes`, () => {
+    const result = spawnSync(process.execPath, [join(__dirname, '../fixtures/websocket-h2-rejected-handshake.js'), response], {
+      encoding: 'utf8',
+      timeout: 10000
+    })
+
+    if (result.error) throw result.error
+    const { status, signal, stderr } = result
+    if (signal !== null || status !== 0) {
+      throw new Error(`child exited with status ${status}, signal ${signal}: ${stderr}`)
+    }
+  })
+}