diff --git a/lib/web/websocket/connection.js b/lib/web/websocket/connection.js index 897f122cd10..79601177198 100644 --- a/lib/web/websocket/connection.js +++ b/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 diff --git a/test/fixtures/websocket-h2-rejected-handshake.js b/test/fixtures/websocket-h2-rejected-handshake.js new file mode 100644 index 00000000000..d12a69a7031 --- /dev/null +++ b/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 +}) diff --git a/test/websocket/h2-rejected-handshake.js b/test/websocket/h2-rejected-handshake.js new file mode 100644 index 00000000000..2beb518b354 --- /dev/null +++ b/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}`) + } + }) +}