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.

Sign up for GitHub

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

Jump to bottom

crypto: hide native handles from JS modules #22747

Closed
wants to merge 2 commits into from
Closed

crypto: hide native handles from JS modules #22747

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
Copy link
Collaborator

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/822/pipeline

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

BridgeAR
BridgeAR reviewed Sep 7, 2018
View reviewed changes
lib/internal/crypto/cipher.js Outdated
@@ -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
Closed
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 •
edited

CI: https://ci.nodejs.org/job/node-test-pull-request/17114/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17118/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17128/

@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
@tniessen
crypto: hide native handles from JS modules
0ade10d 
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
crypto: assign missing deprecation code
3459213 
Refs: nodejs#22747
@tniessen tniessen mentioned this pull request Sep 12, 2018
Closed
3 tasks
tniessen added a commit that referenced this pull request Sep 12, 2018
@tniessen
crypto: assign missing deprecation code
360465d 
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
@targos
crypto: put legacy _handle accessors on prototypes
e5006d3 
Creating deprecated accessors each time an object is created is very
time consuming.

Refs: nodejs#22747
Fixes: nodejs#24266
@targos targos mentioned this pull request Nov 9, 2018
Closed
2 tasks
targos added a commit that referenced this pull request Nov 11, 2018
@targos
crypto: put legacy _handle accessors on prototypes
e83d7e8 
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
@targos @BridgeAR
crypto: put legacy _handle accessors on prototypes
2b48c71 
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
@targos @kiyomizumia
crypto: put legacy _handle accessors on prototypes
1d3d327 
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
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@vdeturckheim vdeturckheim vdeturckheim approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet