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.

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

options.defaultEncoding not validated in stream.Writable constructor #46301

Closed
Alexthemediocre opened this issue Jan 21, 2023 · 3 comments · Fixed by #46322
Closed

options.defaultEncoding not validated in stream.Writable constructor #46301

Alexthemediocre opened this issue Jan 21, 2023 · 3 comments · Fixed by #46322
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@Alexthemediocre
Copy link

Version

v19.4.0

Platform

Microsoft Windows NT 10.0.22000.0 x64

Subsystem

node:stream

What steps will reproduce the bug?

Create a new stream.Writeable and supply an invalid encoding for defaultEncoding. It will accept the encoding just fine, which can lead to some strange side effects.

For a very quick test, just to show that it does not validate it:

new (require('node:stream').Writable)({defaultEncoding: 'some invalid encoding'});

For a more complete test that shows more of its effects:

const stream = require('node:stream');

console.log('stream 1');
const s1 = new stream.Writable({
   defaultEncoding: 'my invalid encoding',
   write(chunk, enc, cb) {
      console.log('dat', chunk);
      console.log('enc', enc);
      cb();
   }
});
// output:
//TypeError: Unknown encoding: my invalid encoding
//...
try {
   s1.write('test data');
} catch (err) {
   console.error(err);
}

/* ============================== */

console.log('\nstream 2');
const s2 = new stream.Writable({
   defaultEncoding: 'my invalid encoding',
   decodeStrings: false,
   write(chunk, enc, cb) {
      console.log('dat', chunk);
      console.log('enc', enc);
      cb();
   }
});
// output:
//dat test data
//enc my invalid encoding
s2.write('test data');

/* ============================== */

console.log('\nstream 3');
const s3 = new stream.Writable({
   defaultEncoding: {an: 'object'},
   write(chunk, enc, cb) {
      console.log('dat', chunk);
      console.log('enc', enc);
      cb();
   }
});
// output:
//dat <Buffer 74 65 73 74 20 64 61 74 61>
//enc buffer
s3.write('test data');

/* ============================== */

console.log('\nstream 4');
const s4 = new stream.Transform({
   defaultEncoding: 'my invalid encoding',
   transform(chunk, enc, cb) {
      console.log('dat', chunk);
      console.log('enc', enc);
      cb(chunk);
   }
});
// output:
//TypeError: Unknown encoding: my invalid encoding
//...
try {
   s4.write('test data');
} catch (err) {
   console.error(err);
}

/* ============================== */

console.log('\nnode:crypto test');
console.log('crypto.createCipheriv');

const crypto = require('node:crypto');
const encrypt = crypto.createCipheriv('aes-128-gcm', crypto.randomBytes(16), crypto.randomBytes(16), {defaultEncoding: 'an invalid encoding'});

encrypt.on('data', (chunk) => {
   console.log('data received', chunk);
});
encrypt.write('some more data');
encrypt.end();

How often does it reproduce? Is there a required condition?

As far as I can tell, it always reproduces.

What is the expected behavior?

I would expect the stream.Writeable constructor to validate the defaultEncoding option and throw an error if it is invalid.

It still can throw an error with a call to writeable.write if objectMode is false, decodeStrings is true, the chunk is a string, and no encoding is specified in the write call. Weirdly enough, it doesn't throw an error if the defaultEncoding is something other than a string, like a number or object, and converts the string provided in the write call to a buffer anyway.

const stream = require('node:stream');
// would expect an error
new stream.Writable({defaultEncoding: 'some invalid encoding'});

// would very much expect an error
new stream.Writable({defaultEncoding: 42}).write('a string');

const s = new stream.Writeable();
// throws an error, which is exactly what I would expect
s.setDefaultEncoding('some invalid encoding');

// also throws an error (expected)
s.write('a string', 42);

What do you see instead?

It does not validate invalid default encodings in the constructor, and only throws an error if the invalid default encoding is used (such as if a string is provided in a write call with no other encoding specified).

Additional information

Trying a similar thing with stream.Readable throws an error instead.

// throws an error (which is expected)
new (require('node:stream').Readable)({encoding: 'some invalid encoding'});
@VoltrexKeyva VoltrexKeyva added the stream Issues and PRs related to the stream subsystem. label Jan 22, 2023
@marco-ippolito
Copy link
Member

I'll open a PR to validate encodings

@marco-ippolito
Copy link
Member

marco-ippolito commented Jan 23, 2023

There is this comment in writable streams (but its also in readable).

// Crypto is kind of old and crusty. Historically, its default string
// encoding is 'binary' so we have to make this configurable.
// Everything else in the universe uses 'utf8', though.
this.defaultEncoding = (options && options.defaultEncoding) || 'utf8';

@mcollina @ronag can give more context on why defaultEncoding is not validated and whether to add some validation.
On the other end readable encoding will throw because of StringDecoder:

this.decoder = null;
this.encoding = null;
if (options && options.encoding) {
this.decoder = new StringDecoder(options.encoding);
this.encoding = options.encoding;
}
}

@ronag
Copy link
Member

ronag commented Jan 23, 2023

I don't think there is any specific reason why it's not validated. PR welcome.

nodejs-github-bot pushed a commit that referenced this issue Jan 30, 2023
PR-URL: #46322
Fixes: #46301
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants