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: hide native handles from JS modules #22747

Closed
wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 7, 2018

Another attempt to reduce the undocumented API surface. I am defensively marking this as semver-major, but as far as I can tell, no popular packages are using these properties. It would be kind of pointless anyway since there is no advantage in using _handle here. (This is not true for other parts of our API, but it seems to be valid for the affected crypto APIs.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Sep 7, 2018
@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2018
@jasnell
Copy link
Member

jasnell commented Sep 7, 2018

It's likely safer to turn _handle into a deprecated getter/alias for [kHandle], at least through one major release cycle.

@tniessen
Copy link
Member Author

tniessen commented Sep 7, 2018

Definitely safer, yes, but I wonder whether people are using it at all. (I can't think of a single legitimate use case.)

@@ -73,11 +75,11 @@ function getUIntOption(options, key) {
function createCipherBase(cipher, credential, options, decipher, iv) {
const authTagLength = getUIntOption(options, 'authTagLength');

this._handle = new CipherBase(decipher);
this[kHandle] = new CipherBase(decipher);
if (iv === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Since the purpose of this is to hide the actual implementation details, I would prefer to use a non-enumerable symbol. Otherwise it will still show up in the repl and when inspecting the value (including console calls).

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the idea to do this after reviewing #22684. If we want to use non-enumerable symbols, maybe we should add that to the guidelines?

@Trott
Copy link
Member

Trott commented Sep 7, 2018

@nodejs/security-wg

@tniessen tniessen mentioned this pull request Sep 7, 2018
3 tasks
@jasnell
Copy link
Member

jasnell commented Sep 7, 2018

Definitely safer, yes, but I wonder whether people are using it at all. (I can't think of a single legitimate use case.)

They likely aren't, but that doesn't make it any less the right thing to do... at least just for a single major cycle. Node.js 11 would not be a LTS branch so the deprecated aliases wouldn't be around for long.

@lirantal
Copy link
Member

lirantal commented Sep 8, 2018

Does it make sense to add a test that will assert on this new expectation that _handle isn't accessible as an enumerable on the returned object?

@tniessen
Copy link
Member Author

tniessen commented Sep 9, 2018

I changed the PR to deprecate the _handle property. PTAL.

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. labels Sep 11, 2018
@tniessen
Copy link
Member Author

tniessen commented Sep 11, 2018

@tniessen
Copy link
Member Author

CI was stopped, resuming a previous build again... https://ci.nodejs.org/job/node-test-pull-request/17140/

tniessen added a commit to tniessen/node that referenced this pull request Sep 12, 2018
PR-URL: nodejs#22747
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@tniessen
Copy link
Member Author

Landed in 0ade10d.

@tniessen tniessen closed this Sep 12, 2018
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
@vsemozhetbyt
Copy link
Contributor

DEP00XX seems forgotten to replace)

tniessen added a commit to tniessen/node that referenced this pull request Sep 12, 2018
tniessen added a commit that referenced this pull request Sep 12, 2018
PR-URL: #22827
Refs: #22747
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit to targos/node that referenced this pull request Nov 9, 2018
Creating deprecated accessors each time an object is created is very
time consuming.

Refs: nodejs#22747
Fixes: nodejs#24266
targos added a commit that referenced this pull request Nov 11, 2018
Creating deprecated accessors each time an object is created is very
time consuming.

Refs: #22747
Fixes: #24266

PR-URL: #24269
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Creating deprecated accessors each time an object is created is very
time consuming.

Refs: #22747
Fixes: #24266

PR-URL: #24269
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Creating deprecated accessors each time an object is created is very
time consuming.

Refs: nodejs#22747
Fixes: nodejs#24266

PR-URL: nodejs#24269
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.