crypto: fix ucs2/ucs-2/utf16le/utf-16le encoding check #8301
Conversation
nodejs-github-bot
added
the
crypto
| @@ -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. | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also, space after // please.
|
If you change the This generally looks good to me (LGTM), thanks for doing the work here! /cc @nodejs/crypto |
|
Minor nit: first line of the commit message is a tad too long (>50) |
|
|
||
| var decipher = crypto.createDecipher('aes192', key); | ||
| var txt = decipher.update(ciph, 'base64', 'utf16le'); | ||
| txt += decipher.final('utf16le'); //should not throw |
There was a problem hiding this comment.
Same comment about spacing after //, here and below.
|
LGTM with the nits addressed. |
|
Do I have to amend anything? Or this is done during the merge process? |
|
@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 |
|
|
||
| var decipher = crypto.createDecipher('aes192', key); | ||
| var txt = decipher.update(ciph, 'base64', 'utf16le'); | ||
| txt += decipher.final('utf16le'); // Should not throw |
There was a problem hiding this comment.
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
atstoyanov
force-pushed
the
cryptofix
branch
from
039003a
to
1ecd092
Compare
|
Landed in a6f7b13, thanks! |
|
Depends on #7207 |
|
I’ll do a backport tomorrow then, this should probably land in v6.x at some point (would just be adding the |
|
I’m removing the |
|
should this be backported to v4.x? |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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