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

crypto: fix ucs2/ucs-2/utf16le/utf-16le encoding check #8301

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@atstojanov
Contributor

atstojanov commented Aug 27, 2016

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

Normalize the encoding in getDecoder() before using it. Fixes an
AssertionError: "Cannot change encoding" when encoding is "ucs2", "ucs-2"
or "utf-16le"

Fixes: #8236

@addaleax

View changes

Show outdated Hide outdated test/parallel/test-crypto-cipher-decipher.js
@@ -113,3 +113,27 @@ testCipher2(Buffer.from('0123456789abcdef'));
c.update('update', 'utf-8');
c.final('utf8'); // Should not throw.
}
//#8236 regression tests, 'ucs2', 'usc-2', 'utf-16le', 'utf16le' are identical.

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

nit: Using the full github URL for issue references is cool because that’s more accessible (even clickable in many cases) and helps distinguish from the https://github.com/nodejs/node-v0.x-archive issue tracker

@addaleax

addaleax Aug 27, 2016

Member

nit: Using the full github URL for issue references is cool because that’s more accessible (even clickable in many cases) and helps distinguish from the https://github.com/nodejs/node-v0.x-archive issue tracker

This comment has been minimized.

@mscdex

mscdex Aug 27, 2016

Contributor

also: s/usc-2/ucs-2/

@mscdex

mscdex Aug 27, 2016

Contributor

also: s/usc-2/ucs-2/

This comment has been minimized.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Also, space after // please.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Also, space after // please.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 27, 2016

Member

If you change the https://github.com/nodejs/node/issues/8236 in your commit message into Fixes: https://github.com/nodejs/node/issues/8236, the issue will automatically be closed once this is landed, which is kind of cool.

This generally looks good to me (LGTM), thanks for doing the work here!

/cc @nodejs/crypto

Member

addaleax commented Aug 27, 2016

If you change the https://github.com/nodejs/node/issues/8236 in your commit message into Fixes: https://github.com/nodejs/node/issues/8236, the issue will automatically be closed once this is landed, which is kind of cool.

This generally looks good to me (LGTM), thanks for doing the work here!

/cc @nodejs/crypto

@addaleax

This comment has been minimized.

Show comment
Hide comment
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Aug 27, 2016

Contributor

Minor nit: first line of the commit message is a tad too long (>50)

Contributor

mscdex commented Aug 27, 2016

Minor nit: first line of the commit message is a tad too long (>50)

@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-crypto-cipher-decipher.js
var decipher = crypto.createDecipher('aes192', key);
var txt = decipher.update(ciph, 'base64', 'utf16le');
txt += decipher.final('utf16le'); //should not throw

This comment has been minimized.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Same comment about spacing after //, here and below.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Same comment about spacing after //, here and below.

@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-crypto-cipher-decipher.js
var decipher = crypto.createDecipher('aes192', key);
var txt = decipher.update(ciph, 'base64', 'utf16le');
txt += decipher.final('utf16le'); //should not throw
assert.equal(txt, plaintext, 'decrypted result in ucs2');

This comment has been minimized.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Please use strictEqual() for your assertions.

@cjihrig

cjihrig Aug 28, 2016

Contributor

Please use strictEqual() for your assertions.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2016

Member

LGTM with the nits addressed.

Member

jasnell commented Aug 29, 2016

LGTM with the nits addressed.

@atstojanov

This comment has been minimized.

Show comment
Hide comment
@atstojanov

atstojanov Aug 29, 2016

Contributor

Do I have to amend anything? Or this is done during the merge process?

Contributor

atstojanov commented Aug 29, 2016

Do I have to amend anything? Or this is done during the merge process?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 29, 2016

Member

@atstojanov It would be great it you could amend your PR with the suggestions here – doing so allows everyone who wishes to take a look at PRs to make sure that they are in a state in which they can be merged, and allows CI runs with the changes in the way that they are expected to end up in master.

Member

addaleax commented Aug 29, 2016

@atstojanov It would be great it you could amend your PR with the suggestions here – doing so allows everyone who wishes to take a look at PRs to make sure that they are in a state in which they can be merged, and allows CI runs with the changes in the way that they are expected to end up in master.

@jasnell

View changes

Show outdated Hide outdated test/parallel/test-crypto-cipher-decipher.js
var decipher = crypto.createDecipher('aes192', key);
var txt = decipher.update(ciph, 'base64', 'utf16le');
txt += decipher.final('utf16le'); // Should not throw

This comment has been minimized.

@jasnell

jasnell Aug 30, 2016

Member

Oh, quick nit... Should wrap these in an assert.doesNotThrow() ... e.g.

assert.doesNotThrow(() => txt += decipher.final('utf16le'));
@jasnell

jasnell Aug 30, 2016

Member

Oh, quick nit... Should wrap these in an assert.doesNotThrow() ... e.g.

assert.doesNotThrow(() => txt += decipher.final('utf16le'));
crypto: fix getDecoder() encoding check
Normalize the encoding in getDecoder() before using it. Fixes an
AssertionError: "Cannot change encoding" when encoding is "ucs2",
"ucs-2" or "utf-16le"

Fixes: #8236
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 4, 2016

Member

Landed in a6f7b13, thanks!

Member

addaleax commented Sep 4, 2016

Landed in a6f7b13, thanks!

@addaleax addaleax closed this Sep 4, 2016

addaleax added a commit that referenced this pull request Sep 4, 2016

crypto: fix getDecoder() encoding check
Normalize the encoding in getDecoder() before using it. Fixes an
AssertionError: "Cannot change encoding" when encoding is "ucs2",
"ucs-2" or "utf-16le"

Fixes: #8236
PR-URL: #8301
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 8, 2016

Member

Depends on #7207

Member

Fishrock123 commented Sep 8, 2016

Depends on #7207

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 8, 2016

Member

I’ll do a backport tomorrow then, this should probably land in v6.x at some point (would just be adding the internal/util require again)

Member

addaleax commented Sep 8, 2016

I’ll do a backport tomorrow then, this should probably land in v6.x at some point (would just be adding the internal/util require again)

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 9, 2016

Member

I’m removing the dont-land-on-v6.x label here since this will land cleanly after the backport in #8463

Member

addaleax commented Sep 9, 2016

I’m removing the dont-land-on-v6.x label here since this will land cleanly after the backport in #8463

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

crypto: fix getDecoder() encoding check
Normalize the encoding in getDecoder() before using it. Fixes an
AssertionError: "Cannot change encoding" when encoding is "ucs2",
"ucs-2" or "utf-16le"

Fixes: #8236
PR-URL: #8301
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

should this be backported to v4.x?

Member

MylesBorins commented Sep 30, 2016

should this be backported to v4.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment