crypto: fix ucs2/ucs-2/utf16le/utf-16le encoding check by atstoyanov · Pull Request #8301 · nodejs/node

atstoyanov · 2016-08-27T16:46:50Z

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 · 2016-08-27T16:53:33Z

test/parallel/test-crypto-cipher-decipher.js

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

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

Also, space after // please.

addaleax · 2016-08-27T16:56:41Z

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 · 2016-08-27T17:53:17Z

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

mscdex · 2016-08-27T18:55:26Z

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

cjihrig · 2016-08-28T01:54:02Z

test/parallel/test-crypto-cipher-decipher.js

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

jasnell · 2016-08-29T16:49:45Z

LGTM with the nits addressed.

atstoyanov · 2016-08-29T17:20:45Z

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

addaleax · 2016-08-29T17:23:38Z

@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 · 2016-08-30T20:50:48Z

test/parallel/test-crypto-cipher-decipher.js

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

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

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

addaleax · 2016-09-02T21:59:31Z

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

addaleax · 2016-09-04T16:51:23Z

Landed in a6f7b13, thanks!

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 · 2016-09-08T23:19:17Z

Depends on #7207

addaleax · 2016-09-08T23:21:24Z

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 · 2016-09-09T18:47:53Z

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

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 · 2016-09-30T22:09:01Z

should this be backported to v4.x?

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 27, 2016

addaleax reviewed Aug 27, 2016
View reviewed changes

atstoyanov force-pushed the cryptofix branch from 5cab2e8 to b2f6b78 Compare August 27, 2016 17:29

cjihrig reviewed Aug 28, 2016
View reviewed changes

test/parallel/test-crypto-cipher-decipher.js Outdated

Copy link

Contributor

cjihrig Aug 28, 2016

Choose a reason for hiding this comment

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

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

atstoyanov force-pushed the cryptofix branch from b2f6b78 to 039003a Compare August 30, 2016 07:43

jasnell reviewed Aug 30, 2016
View reviewed changes

crypto: fix getDecoder() encoding check

1ecd092

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

atstoyanov force-pushed the cryptofix branch from 039003a to 1ecd092 Compare September 1, 2016 13:00

addaleax closed this Sep 4, 2016

Fishrock123 mentioned this pull request Sep 6, 2016

v6.6.0 pre-proposal #8428

Closed

Fishrock123 added the dont-land-on-v6.x label Sep 8, 2016

addaleax removed the dont-land-on-v6.x label Sep 9, 2016

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

Uh oh!

Conversation

atstoyanov commented Aug 27, 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!

addaleax Aug 27, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Aug 27, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 27, 2016

Uh oh!

addaleax commented Aug 27, 2016

Uh oh!

mscdex commented Aug 27, 2016

Uh oh!

cjihrig Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 29, 2016

Uh oh!

atstoyanov commented Aug 29, 2016

Uh oh!

addaleax commented Aug 29, 2016

Uh oh!

jasnell Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 2, 2016

Uh oh!

addaleax commented Sep 4, 2016

Uh oh!

Fishrock123 commented Sep 8, 2016

Uh oh!

addaleax commented Sep 8, 2016

Uh oh!

addaleax commented Sep 9, 2016

Uh oh!

MylesBorins commented Sep 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

atstoyanov commented Aug 27, 2016 •

edited

Loading