Exec stdin might lost data if pass in Readable.from(string)

[hax]

javascript/src/web-socket-handler.ts

Line 58 in b4fd82f

const buff = Buffer.alloc(data.length + 1);

Current implementation assume stdin is always a binary stream, but it does not really check the type. So if pass Readable.from(string) as stdin, most time it works, unless the string contains non-ASCII characters, in such case, data is string and data.length is number of UTF16 code units, which will smaller than the number of UTF8 code units or bytes.

I suggest the implementation should either deal with the string case, or just throw type error if not buffer.

[brendandburns]

Happy to take a PR with a unit test to repro this and either fix that you prefer.

[cjihrig]

Here is a small reproduction:

'use strict';
const { deepStrictEqual } = require('node:assert');
const { Readable } = require('node:stream');
const { test } = require('node:test');
const { WebSocketHandler } = require('./dist/web-socket-handler');

test('repro', () => {
  const { promise, resolve } = Promise.withResolvers();
  const stream = new Readable({ read() {} });
  const ws = {
    close() {},
    send(data) {
      deepStrictEqual(data, Buffer.from([0x0f, 0xe2, 0x98, 0x83]));
      resolve();
    }
  };

  stream.setEncoding('utf8');
  stream.push('☃');
  WebSocketHandler.handleStandardInput(ws, stream, 0x0f);
  return promise;
});

I'll work on a fix.

[cjihrig]

I think this can be closed since #1900 was merged. I'm a bit surprised that it didn't auto-close.

EDIT: Maybe it didn't auto-close since the PR targeted the release-1.x branch.