crypto, string_bytes: treat `buffer` str as `utf8` by indutny · Pull Request #5522 · nodejs/node · GitHub

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

Do not treat crypto inputs as binary strings, convert them to Buffers
using new Buffer(..., 'utf8'), or using newly updated StringBytes
APIs.

Do not treat crypto inputs as `binary` strings, convert them to Buffers using `new Buffer(..., 'utf8')`, or using newly updated StringBytes APIs.

See #5504

cc @nodejs/collaborators @nodejs/ctc

CI: https://ci.nodejs.org/job/node-test-pull-request/1806/

Is this semver-major?

It is.

Can you remove BINARY from enum encoding in src/node.h as well?

Oh wait, the intent is to change the default, not remove it altogether? LGTM in that case.

Thank you, @bnoordhuis .

How should we handle this major breakage @nodejs/ctc ? There is hardly a way to deprecate it, because it is a change of default value. Should we just land it and write some blog posts on this change?

Yeah, I think that's likely best. Not sure there's a better way.

LGTM

Landed in b010c87, thank you!

Do not treat crypto inputs as `binary` strings, convert them to Buffers using `new Buffer(..., 'utf8')`, or using newly updated StringBytes APIs. PR-URL: #5522 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

@indutny Please don't land major changes so fast?

Yeah... for stuff like this perhaps give it a day or two to give more people a chance to review. I wouldn't anticipate any issues with this one but still...

Oh, sorry... What's our process on this?

For a major I think it's something like at least 48 hours.

This sentence doesn't make much sense anymore since legacy programs were not assuming utf8, but binary.

Gosh, this is true.

May I ask you to submit follow-up PR to fix this?

For reference (so there can be broad agreement or else we can talk about updating it to be more accurate) from the current version of the Collaborator Guide:

Before landing pull requests, sufficient time should be left for input from other Collaborators. Leave at least 48 hours during the week and 72 hours over weekends to account for international time differences and work schedules. Trivial changes (e.g. those which fix minor bugs or improve performance without affecting API or causing other wide-reaching impact) may be landed after a shorter delay.

It does not appear to be in the Collaborator Guide, but the doc for onboarding, or at least the one I bookmarked at my onboarding, says:

semver-major (breaking) changes must be reviewed in some form by the CTC

If that's still correct, it's probably worth mentioning in the Collaborator Guide. (We may also want to get at least a tiny bit more specific than "in some form".)

For anything but trivial or otherwise urgent changes we should be leaving at least 48 hours. This is kind of major and deserves the chance for more discussion to flush out any possible objections.

details: nodejs/node#5522

The default encoding was switched to utf8 in nodejs/node#5522

Just saw websockets/ws#716. Did someone write a blog post / example in docs on what Node users need to change (for example, with hash#update calls)?

This PR made sweeping backwards-incompatible changes to the crypto API, but did not include documentation. cf. #10265

@sam-github clearly this PR changes all occurrences of binary to utf8 in the docs. What kind of documentation do you mean?

So, that basically boils down to "existing behaviour was undocumented, so there were no docs to change", which is one way to look at it.

I'm not necessarily saying that this PR shouldn't have landed without you being forced to document the crypto APIs, but I am saying that users of APIs like pbkdf2() (see #10265) could never have figured out from the docs that there has been a change, which is a problem that needs flagging.

Which is why I commented here, so that future readers of this PR can know that the changes it made weren't documented.

I agree we could do better on this. However, careful reader of docs would notice:

https://github.com/nodejs/node/pull/5522/files#diff-efc9444c79e30e68a167275387a55aa6R813

Which says that default encoding is utf8.

So I can't say that it was completely undocumented, just wasn't in pbkdf documentation itself.

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.

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>

crypto, string_bytes: treat buffer str as utf8

6bd1a8e

Do not treat crypto inputs as `binary` strings, convert them to Buffers using `new Buffer(..., 'utf8')`, or using newly updated StringBytes APIs.

indutny mentioned this pull request Mar 2, 2016

src: remove BINARY encoding #5504

Closed

4 tasks

Trott added the crypto Issues and PRs related to the crypto subsystem. label Mar 2, 2016

indutny added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 2, 2016

indutny closed this Mar 2, 2016

indutny deleted the feature/no-default-binary-in-crypto.js branch March 2, 2016 18:25

mscdex reviewed Mar 2, 2016
View reviewed changes

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

nolanlawson mentioned this pull request Apr 28, 2016

node6 is permafail apache/pouchdb#5102

Closed

fanatid mentioned this pull request Apr 29, 2016

utf8 as default encoding browserify/hash-base#4

Merged

fanatid added a commit to browserify/hash-base that referenced this pull request Apr 29, 2016

utf8 as default encoding

5d95d75

details: nodejs/node#5522

ArtemGovorov mentioned this pull request Apr 29, 2016

Node 6.0 support for browser tests wallabyjs/public#568

Closed

omsmith added a commit to omsmith/pouchdb that referenced this pull request Apr 29, 2016

(apache#5102 ) - specify md5 data encoding

c3704f3

The default encoding was switched to utf8 in nodejs/node#5522

omsmith mentioned this pull request Apr 29, 2016

(#5102) - specify md5 data encoding apache/pouchdb#5107

Merged

ArtemGovorov mentioned this pull request Apr 29, 2016

hixie 76 is broken in node 6 websockets/ws#716

Closed

omsmith added a commit to omsmith/pouchdb that referenced this pull request Apr 29, 2016

(apache#5102 ) - specify md5 data encoding

48bda89

The default encoding was switched to utf8 in nodejs/node#5522

daleharvey pushed a commit to apache/pouchdb that referenced this pull request Apr 29, 2016

(#5102 ) - Specify md5 data encoding

3d7d497

The default encoding was switched to utf8 in nodejs/node#5522

ChALkeR mentioned this pull request May 1, 2016

Doesn't work under Node.js 6.0.0 brianc/node-postgres#1000

Closed

fanatid mentioned this pull request May 19, 2016

Bug in sha implementation (likely due to international characters) browserify/sha.js#37

Closed

tejasmanohar mentioned this pull request Jun 13, 2016

specify data encoding type 'binary' at hash#update websockets/ws#746

Merged

addaleax mentioned this pull request Jul 13, 2016

Crypto - sign.update, verifier.update default parameter for strings 'utf8' not really true? #7712

Closed

This was referenced Sep 15, 2016

Code and Learn at Node.js Interactive in Amsterdam nodejs/code-and-learn#56

Closed

crypto: fix default encoding of LazyTransform #8611

Closed

dschenkelman mentioned this pull request Jan 20, 2017

Fix for node 6.x. Be explicit about crypto update input encoding latin1 mcastany/passport-wsfed-saml2#1

Merged

gits2501 mentioned this pull request Sep 28, 2017

More node version for testing gits2501/Hmac_Sha1#4

Merged

               The `crypto.DEFAULT_ENCODING` mechanism is provided for backwards compatibility
-              with legacy programs that expect `'binary'` to be the default encoding.
+              with legacy programs that expect `'utf8'` to be the default encoding.

crypto, string_bytes: treat `buffer` str as `utf8` by indutny · Pull Request #5522 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto, string_bytes: treat buffer str as utf8 - #5522

crypto, string_bytes: treat buffer str as utf8#5522indutny wants to merge 1 commit intonodejs:masternodejs/node:masterfrom indutny:feature/no-default-binary-in-crypto.jsindutny/io.js:feature/no-default-binary-in-crypto.jsCopy head branch name to clipboard

Conversation

indutny commented Mar 2, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

Fishrock123 commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

bnoordhuis commented Mar 2, 2016

Uh oh!

bnoordhuis commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

Fishrock123 commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

indutny commented Mar 2, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

mscdex Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

indutny Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 2, 2016

Uh oh!

rvagg commented Mar 3, 2016

Uh oh!

tejasmanohar commented May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Dec 14, 2016

Uh oh!

indutny commented Dec 14, 2016

Uh oh!

sam-github commented Dec 14, 2016

Uh oh!

indutny commented Dec 14, 2016

Uh oh!

indutny commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

crypto, string_bytes: treat buffer str as utf8#5522
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:feature/no-default-binary-in-crypto.js

tejasmanohar commented May 11, 2016 •
edited
Loading