From 57f1b4e636b84ceeef4f048ce26d2c2348479727 Mon Sep 17 00:00:00 2001 From: Khafra Date: Sat, 7 Sep 2024 17:34:50 -0400 Subject: [PATCH] handle websocket closed correctly (#3565) Co-authored-by: eXhumer --- lib/web/websocket/connection.js | 22 ++++++++++++++-------- lib/web/websocket/receiver.js | 25 ++++++++++++------------- lib/web/websocket/util.js | 5 +++-- lib/web/websocket/websocket.js | 20 +++++++++++++------- test/websocket/issue-3546.js | 24 ++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 30 deletions(-) create mode 100644 test/websocket/issue-3546.js diff --git a/lib/web/websocket/connection.js b/lib/web/websocket/connection.js index 0356d51d9c7..2d6675e076c 100644 --- a/lib/web/websocket/connection.js +++ b/lib/web/websocket/connection.js @@ -1,6 +1,6 @@ 'use strict' -const { uid } = require('./constants') +const { uid, states } = require('./constants') const { failWebsocketConnection, parseExtensions } = require('./util') const { channels } = require('../../core/diagnostics') const { makeRequest } = require('../fetch/request') @@ -94,10 +94,16 @@ function establishWebSocketConnection (url, protocols, client, handler, options) useParallelQueue: true, dispatcher: options.dispatcher, processResponse (response) { + if (response.type === 'error') { + // If the WebSocket connection could not be established, it is also said + // that _The WebSocket Connection is Closed_, but not _cleanly_. + handler.readyState = states.CLOSED + } + // 1. If response is a network error or its status is not 101, // fail the WebSocket connection. if (response.type === 'error' || response.status !== 101) { - failWebsocketConnection(handler, 'Received network error or non-101 status code.') + failWebsocketConnection(handler, 1002, 'Received network error or non-101 status code.') return } @@ -106,7 +112,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, 'Server did not respond with sent protocols.') + failWebsocketConnection(handler, 1002, 'Server did not respond with sent protocols.') return } @@ -121,7 +127,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options) // insensitive match for the value "websocket", the client MUST // _Fail the WebSocket Connection_. if (response.headersList.get('Upgrade')?.toLowerCase() !== 'websocket') { - failWebsocketConnection(handler, 'Server did not set Upgrade header to "websocket".') + failWebsocketConnection(handler, 1002, 'Server did not set Upgrade header to "websocket".') return } @@ -130,7 +136,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options) // ASCII case-insensitive match for the value "Upgrade", the client // MUST _Fail the WebSocket Connection_. if (response.headersList.get('Connection')?.toLowerCase() !== 'upgrade') { - failWebsocketConnection(handler, 'Server did not set Connection header to "upgrade".') + failWebsocketConnection(handler, 1002, 'Server did not set Connection header to "upgrade".') return } @@ -144,7 +150,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options) const secWSAccept = response.headersList.get('Sec-WebSocket-Accept') const digest = crypto.createHash('sha1').update(keyValue + uid).digest('base64') if (secWSAccept !== digest) { - failWebsocketConnection(handler, 'Incorrect hash received in Sec-WebSocket-Accept header.') + failWebsocketConnection(handler, 1002, 'Incorrect hash received in Sec-WebSocket-Accept header.') return } @@ -162,7 +168,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options) extensions = parseExtensions(secExtension) if (!extensions.has('permessage-deflate')) { - failWebsocketConnection(handler, 'Sec-WebSocket-Extensions header does not match.') + failWebsocketConnection(handler, 1002, 'Sec-WebSocket-Extensions header does not match.') return } } @@ -183,7 +189,7 @@ function establishWebSocketConnection (url, protocols, client, handler, options) // the selected subprotocol values in its response for the connection to // be established. if (!requestProtocols.includes(secProtocol)) { - failWebsocketConnection(handler, 'Protocol was not set in the opening handshake.') + failWebsocketConnection(handler, 1002, 'Protocol was not set in the opening handshake.') return } } diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js index 04236ecc90b..5364de0784a 100644 --- a/lib/web/websocket/receiver.js +++ b/lib/web/websocket/receiver.js @@ -88,12 +88,12 @@ class ByteParser extends Writable { const rsv3 = buffer[0] & 0x10 if (!isValidOpcode(opcode)) { - failWebsocketConnection(this.#handler, 'Invalid opcode received') + failWebsocketConnection(this.#handler, 1002, 'Invalid opcode received') return callback() } if (masked) { - failWebsocketConnection(this.#handler, 'Frame cannot be masked') + failWebsocketConnection(this.#handler, 1002, 'Frame cannot be masked') return callback() } @@ -107,43 +107,43 @@ class ByteParser extends Writable { // WebSocket connection where a PMCE is in use, this bit indicates // whether a message is compressed or not. if (rsv1 !== 0 && !this.#extensions.has('permessage-deflate')) { - failWebsocketConnection(this.#handler, 'Expected RSV1 to be clear.') + failWebsocketConnection(this.#handler, 1002, 'Expected RSV1 to be clear.') return } if (rsv2 !== 0 || rsv3 !== 0) { - failWebsocketConnection(this.#handler, 'RSV1, RSV2, RSV3 must be clear') + failWebsocketConnection(this.#handler, 1002, 'RSV1, RSV2, RSV3 must be clear') return } if (fragmented && !isTextBinaryFrame(opcode)) { // Only text and binary frames can be fragmented - failWebsocketConnection(this.#handler, 'Invalid frame type was fragmented.') + failWebsocketConnection(this.#handler, 1002, 'Invalid frame type was fragmented.') return } // If we are already parsing a text/binary frame and do not receive either // a continuation frame or close frame, fail the connection. if (isTextBinaryFrame(opcode) && this.#fragments.length > 0) { - failWebsocketConnection(this.#handler, 'Expected continuation frame') + failWebsocketConnection(this.#handler, 1002, 'Expected continuation frame') return } if (this.#info.fragmented && fragmented) { // A fragmented frame can't be fragmented itself - failWebsocketConnection(this.#handler, 'Fragmented frame exceeded 125 bytes.') + failWebsocketConnection(this.#handler, 1002, 'Fragmented frame exceeded 125 bytes.') return } // "All control frames MUST have a payload length of 125 bytes or less // and MUST NOT be fragmented." if ((payloadLength > 125 || fragmented) && isControlFrame(opcode)) { - failWebsocketConnection(this.#handler, 'Control frame either too large or fragmented') + failWebsocketConnection(this.#handler, 1002, 'Control frame either too large or fragmented') return } if (isContinuationFrame(opcode) && this.#fragments.length === 0 && !this.#info.compressed) { - failWebsocketConnection(this.#handler, 'Unexpected continuation frame') + failWebsocketConnection(this.#handler, 1002, 'Unexpected continuation frame') return } @@ -189,7 +189,7 @@ class ByteParser extends Writable { // https://source.chromium.org/chromium/chromium/src/+/main:v8/src/common/globals.h;drc=1946212ac0100668f14eb9e2843bdd846e510a1e;bpv=1;bpt=1;l=1275 // https://source.chromium.org/chromium/chromium/src/+/main:v8/src/objects/js-array-buffer.h;l=34;drc=1946212ac0100668f14eb9e2843bdd846e510a1e if (upper > 2 ** 31 - 1) { - failWebsocketConnection(this.#handler, 'Received payload length > 2^31 bytes.') + failWebsocketConnection(this.#handler, 1009, 'Received payload length > 2^31 bytes.') return } @@ -341,7 +341,7 @@ class ByteParser extends Writable { if (opcode === opcodes.CLOSE) { if (payloadLength === 1) { - failWebsocketConnection(this.#handler, 'Received close frame with a 1-byte body.') + failWebsocketConnection(this.#handler, 1002, 'Received close frame with a 1-byte body.') return false } @@ -350,8 +350,7 @@ class ByteParser extends Writable { if (this.#info.closeInfo.error) { const { code, reason } = this.#info.closeInfo - closeWebSocketConnection(this.#handler, code, reason, reason.length) - failWebsocketConnection(this.#handler, reason) + failWebsocketConnection(this.#handler, code, reason) return false } diff --git a/lib/web/websocket/util.js b/lib/web/websocket/util.js index 9d3890c1eb0..a24fc622b4d 100644 --- a/lib/web/websocket/util.js +++ b/lib/web/websocket/util.js @@ -150,10 +150,11 @@ function isValidStatusCode (code) { /** * @param {import('./websocket').Handler} handler + * @param {number} code * @param {string|undefined} reason */ -function failWebsocketConnection (handler, reason) { - handler.onFail(reason) +function failWebsocketConnection (handler, code, reason) { + handler.onFail(code, reason) } /** diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js index 2b50c753df2..7e465f56e99 100644 --- a/lib/web/websocket/websocket.js +++ b/lib/web/websocket/websocket.js @@ -28,7 +28,7 @@ const { channels } = require('../../core/diagnostics') /** * @typedef {object} Handler * @property {(response: any, extensions?: string[]) => void} onConnectionEstablished - * @property {(reason: any) => void} onFail + * @property {(code: number, reason: any) => void} onFail * @property {(opcode: number, data: Buffer) => void} onMessage * @property {(code: number, reason: any, reasonByteLength: number) => void} onClose * @property {(error: Error) => void} onParserError @@ -62,7 +62,7 @@ class WebSocket extends EventTarget { /** @type {Handler} */ #handler = { onConnectionEstablished: (response, extensions) => this.#onConnectionEstablished(response, extensions), - onFail: (reason) => this.#onFail(reason), + onFail: (code, reason) => this.#onFail(code, reason), onMessage: (opcode, data) => this.#onMessage(opcode, data), onClose: (code, reason, reasonByteLength) => this.#onClose(code, reason, reasonByteLength), onParserError: (err) => this.#onParserError(err), @@ -515,15 +515,21 @@ class WebSocket extends EventTarget { fireEvent('open', this) } - #onFail (reason) { + #onFail (code, reason) { + // If _The WebSocket Connection is Established_ prior to the point where + // the endpoint is required to _Fail the WebSocket Connection_, the + // endpoint SHOULD send a Close frame with an appropriate status code + // (Section 7.4) before proceeding to _Close the WebSocket Connection_. + if (isEstablished(this.#handler.readyState)) { + this.#onClose(code, reason, reason?.length) + } + this.#controller.abort() if (this.#handler.socket && !this.#handler.socket.destroyed) { this.#handler.socket.destroy() } - this.#handler.readyState = states.CLOSED - if (reason) { // TODO: process.nextTick fireEvent('error', this, (type, init) => new ErrorEvent(type, init), { @@ -548,7 +554,7 @@ class WebSocket extends EventTarget { try { dataForEvent = utf8Decode(data) } catch { - failWebsocketConnection(this.#handler, 'Received invalid UTF-8 in text frame.') + failWebsocketConnection(this.#handler, 1007, 'Received invalid UTF-8 in text frame.') return } } else if (type === opcodes.BINARY) { @@ -582,7 +588,7 @@ class WebSocket extends EventTarget { // If the WebSocket connection is not yet established // Fail the WebSocket connection and set this's ready state // to CLOSING (2). - failWebsocketConnection(this.#handler, 'Connection was closed before it was established.') + failWebsocketConnection(this.#handler, 1002, 'Connection was closed before it was established.') this.#handler.readyState = states.CLOSING } else if (this.#handler.closeState === sentCloseFrameState.NOT_SENT) { // If the WebSocket closing handshake has not yet been started diff --git a/test/websocket/issue-3546.js b/test/websocket/issue-3546.js new file mode 100644 index 00000000000..5d328902538 --- /dev/null +++ b/test/websocket/issue-3546.js @@ -0,0 +1,24 @@ +'use strict' + +const { test } = require('node:test') +const { WebSocket } = require('../..') +const { tspl } = require('@matteo.collina/tspl') + +test('first error than close event is fired on failed connection', async (t) => { + const { completed, strictEqual } = tspl(t, { plan: 2 }) + const ws = new WebSocket('ws://localhost:1') + + let orderOfEvents = 0 + + ws.addEventListener('error', () => { + strictEqual(orderOfEvents++, 0) + strictEqual(ws.readyState, WebSocket.CLOSED) + }) + + ws.addEventListener('close', () => { + strictEqual(orderOfEvents++, 1) + strictEqual(ws.readyState, WebSocket.CLOSED) + }) + + await completed +})