Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Attach error codes to all receiver errors #1901

Merged
merged 5 commits into from Jun 15, 2021
Merged

Attach error codes to all receiver errors #1901

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ff56ba6
Attach error codes to all receiver errors
pimterry Jun 14, 2021
12e9995
Add docs for error codes
pimterry Jun 14, 2021
c818726
Change 'invalid data payload' error code to 'unsupported message'
pimterry Jun 15, 2021
11954cc
Add an separate WS_ERR_UNSUPPORTED_DATA_PAYLOAD_LENGTH error code
pimterry Jun 15, 2021
2acd3cb
Update error docs from review suggestions
pimterry Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 56 additions & 1 deletion doc/ws.md
View file
Open in desktop
Expand Up @@ -44,6 +44,17 @@
- [websocket.terminate()](#websocketterminate)
- [websocket.url](#websocketurl)
- [WebSocket.createWebSocketStream(websocket[, options])](#websocketcreatewebsocketstreamwebsocket-options)
- [WS Error Codes](#ws-error-codes)
- [WS_ERR_INVALID_DATA_PAYLOAD_LENGTH](#wserrinvaliddatapayloadlength)
- [WS_ERR_INVALID_CONTROL_PAYLOAD_LENGTH](#wserrinvalidcontrolpayloadlength)
pimterry marked this conversation as resolved.
Show resolved Hide resolved
- [WS_ERR_INVALID_UTF8](#wserrinvalidutf8)
- [WS_ERR_INVALID_OPCODE](#wserrinvalidopcode)
- [WS_ERR_INVALID_CLOSE_CODE](#wserrinvalidclosecode)
- [WS_ERR_UNEXPECTED_RSV_1](#wserrunexpectedrsv1)
- [WS_ERR_UNEXPECTED_RSV_2_3](#wserrunexpectedrsv23)
- [WS_ERR_EXPECTED_FIN](#wserrexpectedfin)
- [WS_ERR_EXPECTED_MASK](#wserrexpectedmask)
- [WS_ERR_UNEXPECTED_MASK](#wserrunexpectedmask)

## Class: WebSocket.Server

Expand Down Expand Up @@ -298,7 +309,7 @@ human-readable string explaining why the connection has been closed.

- `error` {Error}

Emitted when an error occurs.
Emitted when an error occurs. Errors may have a `.code` property, matching one of the string values defined below under [WS Error Codes](#ws-error-codes).

### Event: 'message'

Expand Down Expand Up @@ -493,6 +504,50 @@ The URL of the WebSocket server. Server clients don't have this attribute.
Returns a `Duplex` stream that allows to use the Node.js streams API on top of a
given `WebSocket`.

## WS Error Codes

Errors emitted by the websocket may have a `.code` property, describing the specific type of error that has occurred:

### WS_ERR_INVALID_DATA_PAYLOAD_LENGTH

A data frame with an unexpectedly long payload was received.

### WS_ERR_INVALID_CONTROL_PAYLOAD_LENGTH

A control frame with an unexpectedly long payload was received.

### WS_ERR_INVALID_UTF8

A text frame was received containing invalid UTF-8 data.
pimterry marked this conversation as resolved.
Show resolved Hide resolved

### WS_ERR_INVALID_OPCODE

A WebSocket frame was received with an invalid opcode.

### WS_ERR_INVALID_CLOSE_CODE

A WebSocket close frame was received with an invalid close code.

### WS_ERR_UNEXPECTED_RSV_1

A WebSocket frame was received with the RSV 1 bit set unexpectedly.
pimterry marked this conversation as resolved.
Show resolved Hide resolved

### WS_ERR_UNEXPECTED_RSV_2_3

A WebSocket frame was received with the RSV 2 or 3 bits set unexpectedly.
pimterry marked this conversation as resolved.
Show resolved Hide resolved

### WS_ERR_EXPECTED_FIN

A WebSocket frame was received with the FIN bit not set when it was expected.

### WS_ERR_EXPECTED_MASK

An unmasked WebSocket frame was received by a WebSocket server.

### WS_ERR_UNEXPECTED_MASK

A masked WebSocket frame was received by a WebSocket client.

[concurrency-limit]: https://github.com/websockets/ws/issues/1202
[duplex-options]:
https://nodejs.org/api/stream.html#stream_new_stream_duplex_options
Expand Down
1 change: 1 addition & 0 deletions lib/permessage-deflate.js
View file
Open in desktop
Expand Up @@ -495,6 +495,7 @@ function inflateOnData(chunk) {
}

this[kError] = new RangeError('Max payload size exceeded');
this[kError].code = 'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH';
this[kError][kStatusCode] = 1009;
this.removeListener('data', inflateOnData);
this.reset();
Expand Down
138 changes: 119 additions & 19 deletions lib/receiver.js
View file
Open in desktop
Expand Up @@ -168,14 +168,26 @@ class Receiver extends Writable {

if ((buf[0] & 0x30) !== 0x00) {
this._loop = false;
return error(RangeError, 'RSV2 and RSV3 must be clear', true, 1002);
return error(
RangeError,
'RSV2 and RSV3 must be clear',
true,
1002,
'WS_ERR_UNEXPECTED_RSV_2_3'
);
}

const compressed = (buf[0] & 0x40) === 0x40;

if (compressed && !this._extensions[PerMessageDeflate.extensionName]) {
this._loop = false;
return error(RangeError, 'RSV1 must be clear', true, 1002);
return error(
RangeError,
'RSV1 must be clear',
true,
1002,
'WS_ERR_UNEXPECTED_RSV_1'
);
}

this._fin = (buf[0] & 0x80) === 0x80;
Expand All @@ -185,31 +197,61 @@ class Receiver extends Writable {
if (this._opcode === 0x00) {
if (compressed) {
this._loop = false;
return error(RangeError, 'RSV1 must be clear', true, 1002);
return error(
RangeError,
'RSV1 must be clear',
true,
1002,
'WS_ERR_UNEXPECTED_RSV_1'
);
}

if (!this._fragmented) {
this._loop = false;
return error(RangeError, 'invalid opcode 0', true, 1002);
return error(
RangeError,
'invalid opcode 0',
true,
1002,
'WS_ERR_INVALID_OPCODE'
);
}

this._opcode = this._fragmented;
} else if (this._opcode === 0x01 || this._opcode === 0x02) {
if (this._fragmented) {
this._loop = false;
return error(RangeError, `invalid opcode ${this._opcode}`, true, 1002);
return error(
RangeError,
`invalid opcode ${this._opcode}`,
true,
1002,
'WS_ERR_INVALID_OPCODE'
);
}

this._compressed = compressed;
} else if (this._opcode > 0x07 && this._opcode < 0x0b) {
if (!this._fin) {
this._loop = false;
return error(RangeError, 'FIN must be set', true, 1002);
return error(
RangeError,
'FIN must be set',
true,
1002,
'WS_ERR_EXPECTED_FIN'
);
}

if (compressed) {
this._loop = false;
return error(RangeError, 'RSV1 must be clear', true, 1002);
return error(
RangeError,
'RSV1 must be clear',
true,
1002,
'WS_ERR_UNEXPECTED_RSV_1'
);
}

if (this._payloadLength > 0x7d) {
Expand All @@ -218,12 +260,19 @@ class Receiver extends Writable {
RangeError,
`invalid payload length ${this._payloadLength}`,
true,
1002
1002,
'WS_ERR_INVALID_CONTROL_PAYLOAD_LENGTH'
);
}
} else {
this._loop = false;
return error(RangeError, `invalid opcode ${this._opcode}`, true, 1002);
return error(
RangeError,
`invalid opcode ${this._opcode}`,
true,
1002,
'WS_ERR_INVALID_OPCODE'
);
}

if (!this._fin && !this._fragmented) this._fragmented = this._opcode;
Expand All @@ -232,11 +281,23 @@ class Receiver extends Writable {
if (this._isServer) {
if (!this._masked) {
this._loop = false;
return error(RangeError, 'MASK must be set', true, 1002);
return error(
RangeError,
'MASK must be set',
true,
1002,
'WS_ERR_EXPECTED_MASK'
);
}
} else if (this._masked) {
this._loop = false;
return error(RangeError, 'MASK must be clear', true, 1002);
return error(
RangeError,
'MASK must be clear',
true,
1002,
'WS_ERR_UNEXPECTED_MASK'
);
}

if (this._payloadLength === 126) this._state = GET_PAYLOAD_LENGTH_16;
Expand Down Expand Up @@ -285,7 +346,8 @@ class Receiver extends Writable {
RangeError,
'Unsupported WebSocket frame: payload length > 2^53 - 1',
false,
1009
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "invalid" is a bit misleading here. It's not invalid is not supported by ws.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WS_ERR_UNSUPPORTED_DATA_PAYLOAD_LENGTH?

);
}

Expand All @@ -304,7 +366,13 @@ class Receiver extends Writable {
this._totalPayloadLength += this._payloadLength;
if (this._totalPayloadLength > this._maxPayload && this._maxPayload > 0) {
this._loop = false;
return error(RangeError, 'Max payload size exceeded', false, 1009);
return error(
RangeError,
'Max payload size exceeded',
false,
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

@lpinca lpinca Jun 14, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about WS_ERR_MESSAGE_TOO_BIG or something like that? The payload length is not invalid per se. It might be the length of a continuation frame that added to the previous ones makes the message length too big.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed the WS_ERR_INVALID_DATA_PAYLOAD_LENGTH code to WS_ERR_UNSUPPORTED_MESSAGE_LENGTH. That's a bit more consistent with the other codes (MESSAGE_TOO_BIG isn't a noun), but it's explicitly a message rather than a single frame, and it's just not supported in this connection rather than completely invalid. Does that work for you?

Copy link
Member

@lpinca lpinca Jun 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WS_ERR_UNSUPPORTED_MESSAGE_LENGTH would work for #1901 (comment) where the payload length is greater than 2^53 - 1. However in this case it is not unsupported. The error in this case is that the message length exceeds the maximum allowed message length as defined by the maxPayload option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However in this case it is not unsupported. The error in this case is that the message length exceeds the maximum allowed message length as defined by the maxPayload option.

That's what I mean by 'unsupported'. Messages that go beyond it aren't "invalid" - imo invalid means against the specification, i.e. this will never work on any conformant websocket connection.

With 'unsupported' I'm trying to say that it may be valid, and it might work elsewhere, but it won't work on this connection right now (e.g. due to the configured maxPayload option). That matches standard use elsewhere - e.g. close code 1003 ("unsupported data" - valid data in a format that this server can't process) or HTTP 415 ("unsupported media type" - a valid request with a media type that this server refuses to handle).

I think that fits both this case (unsupported due to ws maxPayload config) and the other case (unsupported due to JS limitations).

I see what you mean about data frame size vs total message size though. How about we split the two cases into:

  • WS_ERR_UNSUPPORTED_MESSAGE_LENGTH - overall message is too long (> maxPayload)
  • WS_ERR_UNSUPPORTED_DATA_PAYLOAD_LENGTH - a single data frame is too long (> 2^53-1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we split the two cases into

Works for me and that is what I prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, done 👍

);
}
}

Expand Down Expand Up @@ -384,7 +452,13 @@ class Receiver extends Writable {
this._messageLength += buf.length;
if (this._messageLength > this._maxPayload && this._maxPayload > 0) {
return cb(
error(RangeError, 'Max payload size exceeded', false, 1009)
error(
RangeError,
'Max payload size exceeded',
false,
1009,
'WS_ERR_INVALID_DATA_PAYLOAD_LENGTH'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

)
);
}

Expand Down Expand Up @@ -431,7 +505,13 @@ class Receiver extends Writable {

if (!isValidUTF8(buf)) {
this._loop = false;
return error(Error, 'invalid UTF-8 sequence', true, 1007);
return error(
Error,
'invalid UTF-8 sequence',
true,
1007,
'WS_ERR_INVALID_UTF8'
);
}

this.emit('message', buf.toString());
Expand All @@ -456,18 +536,36 @@ class Receiver extends Writable {
this.emit('conclude', 1005, '');
this.end();
} else if (data.length === 1) {
return error(RangeError, 'invalid payload length 1', true, 1002);
return error(
RangeError,
'invalid payload length 1',
true,
1002,
'WS_ERR_INVALID_CONTROL_PAYLOAD_LENGTH'
);
} else {
const code = data.readUInt16BE(0);

if (!isValidStatusCode(code)) {
return error(RangeError, `invalid status code ${code}`, true, 1002);
return error(
RangeError,
`invalid status code ${code}`,
true,
1002,
'WS_ERR_INVALID_CLOSE_CODE'
);
}

const buf = data.slice(2);

if (!isValidUTF8(buf)) {
return error(Error, 'invalid UTF-8 sequence', true, 1007);
return error(
Error,
'invalid UTF-8 sequence',
true,
1007,
'WS_ERR_INVALID_UTF8'
);
}

this.emit('conclude', code, buf.toString());
Expand All @@ -493,15 +591,17 @@ module.exports = Receiver;
* @param {Boolean} prefix Specifies whether or not to add a default prefix to
* `message`
* @param {Number} statusCode The status code
* @param {String} errorCode The exposed error code
* @return {(Error|RangeError)} The error
* @private
*/
function error(ErrorCtor, message, prefix, statusCode) {
function error(ErrorCtor, message, prefix, statusCode, errorCode) {
const err = new ErrorCtor(
prefix ? `Invalid WebSocket frame: ${message}` : message
);

Error.captureStackTrace(err, error);
err.code = errorCode;
err[kStatusCode] = statusCode;
return err;
}
1 change: 1 addition & 0 deletions test/create-websocket-stream.test.js
View file
Open in desktop
Expand Up @@ -210,6 +210,7 @@ describe('createWebSocketStream', () => {

duplex.on('error', (err) => {
assert.ok(err instanceof RangeError);
assert.strictEqual(err.code, 'WS_ERR_INVALID_OPCODE');
assert.strictEqual(
err.message,
'Invalid WebSocket frame: invalid opcode 5'
Expand Down