crypto: fix default encoding of LazyTransform by lmoe · Pull Request #8611 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

crypto

Description of change

Change the default encoding from latin1 to utf8
and extend the test-crypto tests.

Superfluous line removal

Please address this.

Looking good so far, thanks for doing this!

/cc @indutny @mscdex ?

Can you use assert.strictEqual instead of assert.equal here, and maybe wrap the end listener in common.mustCall()?

There are no umlauts in ASCII, but the hash below is valid for latin-1 (aka ISO-8859-1), so I’d recommend naming everything as latin1 here:

$ echo -n öäü | iconv -f utf8 -t iso-8859-1 | sha256sum 
cd37bccd5786e2e76d9b18c871e919e6eb11cc12d868f5ae41c40ccff8e44830  -

I have trouble figuring out your message. What I would do is renaming the ascii part to latin1 but I'm not sure if that's your point. You have marked the Utf8 part and the comment of the ascii format which kind of makes it hard to understand.

@lmoe yeah, I think you got what I was trying to say. That you see the Utf8 part too is just github’s UI, usually the PR comments just apply to single lines (so, yeah, I’m talking about the part which is currently labelled ascii)

Also, I’ve tentatively labelled this as semver-major. It aligns the behaviour with our docs, so whether this is actually semver-major is up for debate.

fix default encoding of LazyTransform

I'm not clear on what this fixes? Is there an issue? If not could we please get a description in the commit message? :D

I'm not clear on what this fixes? Is there an issue? If not could we please get a description in the commit message? :D

Not really, I just noticed this while going through or code base. There’s #5522 and #5500, which changed the default encoding for strings that are passed to crypto functions to utf8, but only did so for the non-streaming API… that makes .update() and .write() behave weirdly incongruently. I assumed that to be an oversight, and if I understood @indutny correctly today, he agrees.

A bit on that would be cool for the commit message, yes.

LGTM

The this._options.defaultEncoding is not used yet :(

It seems to be working for me though.
I will look into this near the weekend.

LGTM with green CI.

CI: https://ci.nodejs.org/job/node-test-commit/5379/

@nodejs/crypto ... any objections on this?

One style nit, otherwise LGTM

LGTM, if CI is green.

@indutny @addaleax
What should I do about this merge conflict? Just to know it in the future. It seems it occurred a month later or so. Should I resolve this conflict on my own, or will your merge tools handle it on their own when you make a release?

@lmoe Please resolve and re-push, it distributes the maintenance work, and if there are any complexities, we may ask you to do so anyhow.

bump @lmoe – Could you rebase this against master to resolve the conflict?

Ping @lmoe again? Otherwise I’ll pick this up in the next couple of days.

Change the default encoding from latin1 to utf8 and extend the test-crypto tests.

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.

@addaleax Sorry, I totally forgot about the mail. I hope the branch is corrected now.

Thanks, still looks good! Going to land this soon.

CI: https://ci.nodejs.org/job/node-test-commit/7947/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/580/

Edit: The electron-prebuilt failure is real and related, I’m reaching out and fixing it.
Edit²: malept/sumchecker#5 …

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

Fresh CI:

CI: https://ci.nodejs.org/job/node-test-commit/8359/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/637/
Edit: New CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/638/ (should be a bit better since the electron-prebuilt failure should be fixed now)

I would like to land this unless any new surprises show up. The electron-prebuilt failure is just waiting on a dependency update, and we should definitely have this in Node 8.

(why? because:)

'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.

CITGM failures seem to be known/unrelated. 🎉

Landed in 443691a, thanks for the PR and for bearing with our process!

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 <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

zvictor reviewed Sep 17, 2016

View reviewed changes

zvictor mentioned this pull request Sep 17, 2016

crypto: fix default encoding of LazyTransform [2] #8624

Closed

3 tasks

addaleax requested changes Sep 17, 2016

View reviewed changes

addaleax added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 17, 2016

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

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

yorkie reviewed Sep 22, 2016

View reviewed changes

jasnell reviewed Sep 28, 2016

View reviewed changes

indutny suggested changes Oct 8, 2016

View reviewed changes

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

addaleax added this to the 8.0.0 milestone Feb 3, 2017

Lukas Möller added 2 commits February 16, 2017 10:00

crypto: fix default encoding of LazyTransform

74b56cd

Change the default encoding from latin1 to utf8 and extend the test-crypto tests.

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

addaleax mentioned this pull request Feb 16, 2017

Explicitly set default encoding in createHash() malept/sumchecker#5

Merged

jasnell approved these changes Mar 10, 2017

View reviewed changes

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 11, 2017

addaleax closed this Mar 11, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

tniessen mentioned this pull request Aug 13, 2023

crypto: do not overwrite _writableState.defaultEncoding #49140

Merged

crypto: fix default encoding of LazyTransform by lmoe · Pull Request #8611 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: fix default encoding of LazyTransform - #8611

crypto: fix default encoding of LazyTransform#8611lmoe wants to merge 2 commits intonodejs:masternodejs/node:masterfrom lmoe:fix-issue-7712lmoe/node:fix-issue-7712Copy head branch name to clipboard

Conversation

lmoe commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

zvictor Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmoe Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 17, 2016

Uh oh!

Fishrock123 commented Sep 17, 2016

Uh oh!

addaleax commented Sep 17, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

yorkie Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

lmoe Sep 28, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 30, 2016

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

lmoe commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Jan 9, 2017

Uh oh!

addaleax commented Feb 3, 2017

Uh oh!

addaleax commented Feb 15, 2017

Uh oh!

lmoe commented Feb 16, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

crypto: fix default encoding of LazyTransform#8611
lmoe wants to merge 2 commits into
nodejs:masterfrom
lmoe:fix-issue-7712

lmoe commented Sep 17, 2016 •
edited
Loading

addaleax Sep 17, 2016 •
edited
Loading

lmoe commented Jan 5, 2017 •
edited
Loading

addaleax commented Feb 16, 2017 •
edited
Loading

addaleax commented Mar 10, 2017 •
edited
Loading