unnecessary copy with .toString() ?

[bitpup]

Not to sound dumb, but, coming from a Node perspective, isn't this causing an unnecessary copy to occur?

ws/lib/receiver.js

Line 437 in fc7e27d

this.emit('message', buf.toString());

perhaps it's required for browser API?

[lpinca]

We can't use the buffer directly because it's a text message and we need to convert it to a string. Is there another way?

[bitpup]

it may well be a text payload, leave it's interpretation to the user?

for now i have a revised method with the buffer going straight to JSON.parse

[lpinca]

Passing a buffer to JSON.parse() will convert it to a string implicitly.

leave it's interpretation to the user?

We would then need to emit an additional argument in the 'message' event to let the user know whether the message is a binary message or a text message. This is a breaking change. Currently a Buffer is used for binary messages and a string for text messages.

[bitpup]

Agreed, I have a solution for now.

I'm not too sure if I like the arbitration of payload that WS employs, I'm thinking perhaps, in this respect it does too much for the user, e.g. it's just a payload... I am probably wrong ;-)

Well done on your efforts so far

[lpinca]

It is consistent with the browser interface where a DOMString or a Blob/ArrayBuffer is used depending on the type of the message.

There are already some deviations and we are not forced to follow it but I'm not very comfortable to always use a Buffer/ArrayBuffer because it's a big breaking change and there might be a few edge cases in WebSocket.createWebSocketStream() that must be thought carefully.

I understand that is some cases, for example when passing data around, avoiding the string conversion might lead to better performances.

[bitpup]

for completeness, here's my hack

I've also dropped using isValidUTF8 as I expect/hope JSON.parse will implement a similar validity check

const
GET_INFO = 0,
Receiver = require('ws/lib/receiver'),
{concat} = require('ws/lib/buffer-util');

function _dataMessage() {
	if (this._fin) {
		if (this._opcode === 2) {
			Receiver.prototype.dataMessage.call(this);

		} else {
			const
			messageLength = this._messageLength,
			fragments = this._fragments;
	
			this._totalPayloadLength = 0;
			this._messageLength = 0;
			this._fragmented = 0;
			this._fragments = [];

			this.emit('message', concat(fragments, messageLength));
		}
	}

	this._state = GET_INFO;
}
.
.
.
ws.on('open', function() {
	this._receiver.dataMessage = _dataMessage.bind(this._receiver);
});

Maybe, for the edge cases, facilitating overriding would be the next nice to have for WS.

[lpinca]

A possible, non-breaking way to avoid converting to a string and give the user a Buffer for text messages is using a new option, similar to the maxPayload option. It might make sense in some cases but I'm not too keen to add it at this time.

If there is demand for this feature, I'm open to reconsider it.

[lpinca]

@bitpup I would not skip UTF-8 validation:

1. It is mandatory per WebSocket specification.
2. Skipping it might lead to security vulnerabilities. This depends on what you are doing ofc.

[bitpup]

I think I'm covered with regards to the security vulnerability, as it appears any none utf8 character will be read from the buffer as the Replacement character - \uFFFD, which in turn will cause JSON.parse to fail, all to spec perhaps?

https://nodejs.org/dist/latest-v14.x/docs/api/buffer.html#buffer_buffers_and_character_encodings

https://en.wikipedia.org/wiki/Specials_(Unicode_block)#Replacement_character

[lpinca]

Doing JSON.parse(buf) is the same of doing JSON.parse(but.toString()) and yes buf.toString() replaces invalid bytes with U+FFFD but JSON.parse('"\uFFFD"') does not fail.

[bitpup]

> JSON.parse(Buffer.from('\uFFFD'))
Uncaught SyntaxError: Unexpected token � in JSON at position 0

[lpinca]

You are missing double quotes.

[lpinca]

JSON.parse(Buffer.from('"\uFFFD"'));

:)

and here is another example of invalid UTF-8 that will be JSON.parse()d with no thrown error if validation is skipped

JSON.parse(Buffer.from([0x22, 0xf8, 0x22]));

[bitpup]

Buffer.from([0x22, 0xf8, 0x22]).toString() is valid JSON ;-p

(my head hurts)