Skip to content

crypto: fix default encoding of LazyTransform #8611

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.

Sign up for GitHub

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

Closed
lmoe wants to merge 2 commits into from
Closed

crypto: fix default encoding of LazyTransform #8611

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74b56cd
crypto: fix default encoding of LazyTransform
Sep 17, 2016
fe9de5d
crypto: fix default encoding of LazyTransform
Sep 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
crypto: fix default encoding of LazyTransform 
Change the default encoding from latin1 to utf8
and extend the test-crypto tests.
  • Loading branch information
Lukas Möller committed Feb 16, 2017
commit 74b56cd775a517d3e404893643ce958940e3395b
7 changes: 6 additions & 1 deletion lib/internal/streams/lazy_transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

const stream = require('stream');
const util = require('util');
const crypto = require('crypto');

module.exports = LazyTransform;

Expand All @@ -22,7 +23,11 @@ util.inherits(LazyTransform, stream.Transform);
get: function() {
stream.Transform.call(this, this._options);
this._writableState.decodeStrings = false;
this._writableState.defaultEncoding = 'latin1';

if (!this._options || !this._options.defaultEncoding) {
this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
}
Copy link
Copy Markdown
Contributor

@yorkie yorkie Sep 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@lmoe lmoe Sep 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return this[prop];
},
set: function(val) {
Expand Down
39 changes: 39 additions & 0 deletions test/parallel/test-crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,42 @@ console.log(crypto.randomBytes(16));
assert.throws(function() {
tls.createSecureContext({ crl: 'not a CRL' });
}, /^Error: Failed to parse CRL$/);

/**
* Check if the stream function uses utf8 as a default encoding.
**/

function testEncoding(options, assertionHash) {
const hash = crypto.createHash('sha256', options);
let hashValue = '';

hash.on('data', (data) => {
hashValue += data.toString('hex');
});

hash.on('end', () => {
assert.equal(hashValue, assertionHash);
Copy link
Copy Markdown
Member

@addaleax addaleax Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

hash.write('öäü');
hash.end();
}

// Hash of "öäü" in utf8 format
const assertionHashUtf8 =
'4f53d15bee524f082380e6d7247cc541e7cb0d10c64efdcc935ceeb1e7ea345c';

// Hash of "öäü" in ascii format
Copy link
Copy Markdown
Member

@addaleax addaleax Sep 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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  -

Copy link
Copy Markdown
Author

@lmoe lmoe Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@addaleax addaleax Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

const assertionHashAscii =
'cd37bccd5786e2e76d9b18c871e919e6eb11cc12d868f5ae41c40ccff8e44830';

testEncoding(undefined, assertionHashUtf8);
testEncoding({}, assertionHashUtf8);

testEncoding({
defaultEncoding: 'utf8'
}, assertionHashUtf8);

testEncoding({
defaultEncoding: 'ascii'
}, assertionHashAscii);