GeezerVison

Copy link

Copy Markdown

zvictor reviewed Sep 17, 2016

View reviewed changes

addaleax requested changes Sep 17, 2016

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lmoe force-pushed the fix-issue-7712 branch from 34c2b15 to 0669e2c Compare September 19, 2016 07:41

addaleax approved these changes Sep 20, 2016

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

jasnell reviewed Sep 28, 2016

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny suggested changes Oct 8, 2016

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

indutny approved these changes Oct 18, 2016

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lmoe force-pushed the fix-issue-7712 branch from 7ad8f75 to fe9de5d Compare February 16, 2017 09:09

addaleax approved these changes Feb 16, 2017

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Up until now, the default encoding for this is `binary`, but
this is scheduled to change in Node 8. Since this module is
currently always passing string input to the `Hash` object,
it needs to account for that.

A better fix would likely involve dropping all strings handling
and treating binary data using Buffer objects, but I kept
this change minimal to avoid any breakage.

Refs: nodejs/node#8611

Up until now, the default encoding for this is `binary`, but
this is scheduled to change in Node 8. Since this module is
currently always passing string input to the `Hash` object,
it needs to account for that.

A better fix would likely involve dropping all strings handling
and treating binary data using Buffer objects, but I kept
this change minimal to avoid any breakage.

Refs: nodejs/node#8611

'use strict';
const { createHash } = require('crypto');

const hash1 = createHash('sha256');
hash1.on('data', out => console.log('hash1 => ', out.toString('hex')));
hash1.end('💩');

const hash2 = createHash('sha256');
hash2.on('data', out => console.log('hash2 => ', out.toString('hex')));
hash2.end('⨽⪩');
// Surprise! Different input, same hash.

jasnell approved these changes Mar 10, 2017

View reviewed changes

PullRequest #5522 and #5500 described the change
of the default encoding into UTF8 in crypto functions.

This however was only changed for the non-streaming API.
The streaming API still used binary as the default encoding.

This commit will change the default streaming API encoding to UTF8
to make both APIs behave the same.

It will also add tests to validate the behavior.

Refs: #5522
Refs: #5500
PR-URL: #8611
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

PullRequest nodejs#5522 and nodejs#5500 described the change
of the default encoding into UTF8 in crypto functions.

This however was only changed for the non-streaming API.
The streaming API still used binary as the default encoding.

This commit will change the default streaming API encoding to UTF8
to make both APIs behave the same.

It will also add tests to validate the behavior.

Refs: nodejs#5522
Refs: nodejs#5500
PR-URL: nodejs#8611
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

This only affects the writable side of LazyTransform and should not
change the behavior of any LazyTransform streams (Cipher, Decipher,
Cipheriv, Decipheriv, Hash, Hmac).

If the user does not set defaultEncoding when creating a transform
stream, WritableState uses 'utf8' by default. Only LazyTransform
overwrites this with 'buffer' for strict backward compatibility. This
was necessary when crypto.DEFAULT_ENCODING still existed. Now that
DEFAULT_ENCODING has been removed, defaultEncoding is always 'buffer'.
The writable side of LazyTransform appears to treat 'utf8' and 'buffer'
in exactly the same way. Therefore, there seems to be no need to
overwrite _writableState.defaultEncoding at this point.

Nevertheless, because Node.js has failed to hide implementation details
such as _writableState from the ecosystem, we may want to consider this
a breaking change.

Refs: nodejs#47182
Refs: nodejs#8611

This only affects the writable side of LazyTransform and should not
change the behavior of any LazyTransform streams (Cipher, Decipher,
Cipheriv, Decipheriv, Hash, Hmac).

If the user does not set defaultEncoding when creating a transform
stream, WritableState uses 'utf8' by default. Only LazyTransform
overwrites this with 'buffer' for strict backward compatibility. This
was necessary when crypto.DEFAULT_ENCODING still existed. Now that
DEFAULT_ENCODING has been removed, defaultEncoding is always 'buffer'.
The writable side of LazyTransform appears to treat 'utf8' and 'buffer'
in exactly the same way. Therefore, there seems to be no need to
overwrite _writableState.defaultEncoding at this point.

Nevertheless, because Node.js has failed to hide implementation details
such as _writableState from the ecosystem, we may want to consider this
a breaking change.

Refs: #47182
Refs: #8611
PR-URL: #49140
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>