Skip to content
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, string_bytes: treat `buffer` str as `utf8` #5522

Closed

Conversation

Projects
None yet
9 participants
@indutny
Copy link
Member

commented Mar 2, 2016

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.

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

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

See #5504

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

cc @nodejs/collaborators @nodejs/ctc

@indutny indutny referenced this pull request Mar 2, 2016

Closed

src: remove BINARY encoding #5504

4 of 4 tasks complete

@Trott Trott added the crypto label Mar 2, 2016

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

Is this semver-major?

@indutny indutny added the semver-major label Mar 2, 2016

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

It is.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

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?

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

LGTM

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

Landed in b010c87, thank you!

@indutny indutny closed this Mar 2, 2016

@indutny indutny deleted the indutny:feature/no-default-binary-in-crypto.js branch Mar 2, 2016

indutny added a commit that referenced this pull request Mar 2, 2016

crypto, string_bytes: treat `buffer` str as `utf8`
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>
@Fishrock123

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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...

@indutny

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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

@@ -811,7 +811,7 @@ or [buffers][`Buffer`]. The default value is `'buffer'`, which makes methods
default to [`Buffer`][] objects.

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.

This comment has been minimized.

Copy link
@mscdex

mscdex Mar 2, 2016

Contributor

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

This comment has been minimized.

Copy link
@indutny

indutny Mar 2, 2016

Author Member

Gosh, this is true.

This comment has been minimized.

Copy link
@indutny

indutny Mar 2, 2016

Author Member

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

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 2, 2016

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".)

@rvagg

This comment has been minimized.

Copy link
Member

commented Mar 3, 2016

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.

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

@nolanlawson nolanlawson referenced this pull request Apr 28, 2016

Closed

node6 is permafail #5102

@fanatid fanatid referenced this pull request Apr 29, 2016

Merged

utf8 as default encoding #4

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

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

(pouchdb#5102) - specify md5 data encoding
The default encoding was switched to utf8 in nodejs/node#5522

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

(pouchdb#5102) - specify md5 data encoding
The default encoding was switched to utf8 in nodejs/node#5522

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

(#5102) - Specify md5 data encoding
The default encoding was switched to utf8 in nodejs/node#5522
@tejasmanohar

This comment has been minimized.

Copy link

commented May 11, 2016

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Dec 14, 2016

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

@indutny

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2016

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

@sam-github

This comment has been minimized.

Copy link
Member

commented Dec 14, 2016

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.

@indutny

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2016

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.

@indutny

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2016

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

lmoe pushed a commit to lmoe/node that referenced this pull request Feb 16, 2017

Lukas Möller
crypto: fix default encoding of LazyTransform
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.

addaleax added a commit that referenced this pull request Mar 11, 2017

crypto: fix default encoding of LazyTransform
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>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

crypto: fix default encoding of LazyTransform
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.