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: use globalThis.crypto over require('crypto').webcrypto #45817

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

panva
Copy link
Member

@panva panva commented Dec 11, 2022

Updates tests and some remaining documentation to use globalThis.crypto instead of require('crypto').webcrypto.

@aduh95 it would be great if we could make this into a lint rule but unfortunately the existing options I tried to do this with fall short:

  • no-restricted-modules cannot restrict an exported property from the crypto module.
  • no-restricted-properties does not catch use of const { webcrypto } = require('crypto').

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@panva panva added dont-land-on-v16.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Dec 11, 2022
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Dec 11, 2022
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2022
@panva panva force-pushed the crypto-use-global-webcrypto branch from 9c778e5 to bdc2204 Compare December 11, 2022 13:24
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2022
@nodejs-github-bot
Copy link
Collaborator

aduh95
aduh95 previously requested changes Dec 11, 2022
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I disagree with the changes in the test, we want to keep the test as is to ensure there's no regressions. The doc-changes LGTM.

@panva
Copy link
Member Author

panva commented Dec 11, 2022

I disagree with the changes in the test, we want to keep the test as is to ensure there's no regressions. The doc-changes LGTM.

What test change do you disagree with?

@aduh95
Copy link
Contributor

aduh95 commented Dec 11, 2022

What test change do you disagree with?

Changing the tests to test globalThis.crypto instead of require('node:crypto').webcrypto.

@panva
Copy link
Member Author

panva commented Dec 11, 2022

What test change do you disagree with?

Changing the tests to test globalThis.crypto instead of require('node:crypto').webcrypto.

We have other existing tests that ensure the two are equal. So I don't see why these changes would be a cause for concern.

@aduh95
Copy link
Contributor

aduh95 commented Dec 11, 2022

We have other existing tests that ensure the two are equal.

That seems like another reason not to make this change. Unless we decide to deprecate require('node:crypto').webcrypto, there's no reason to remove it from our tests IMO.

@panva
Copy link
Member Author

panva commented Dec 11, 2022

I'm unforunately not following your argumentation @aduh95.

I don't see the need for having to have a deprecation on it to recognize the global is the likely way of accessing a Web API, it is the way we already have in most doc examples, and it should therefore be the one tested.

@aduh95
Copy link
Contributor

aduh95 commented Dec 11, 2022

I guess we would have to agree to disagree, and wait to see if someone else wants to chime in. If someone else thinks we should merge it, I'm ready to dismiss my review.

Count me as -.5 on this because:

  • We usually don't accept PRs that are doing "stylistic changes" without a lint rule, and arguably accessing the API one way or another is a question of personal preference and qualify as a "stylistic change" IMO.
  • Even if there were a lint rule added with this PR, I would still be against it (but wouldn't block it), IMO both ways are "valid", and there's little value in enforcing one over the other.

and it should therefore be the one tested.

I disagree, the fact that one API is used less often than another doesn't mean it deserves less test coverage (but anyway it's a bit off-topic, since both APIs are equivalent in this case).

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I agree with @aduh95 regarding the usage of require('node:crypto').webcrypto instead of globalThis.crypto internally.

I'm in favor of blocking this because, if we are going to use globalThis.crypto instead of require('node:crypto').webcrypto we need a lint rule to protect this change in the upcoming pull requests.

@panva
Copy link
Member Author

panva commented Dec 11, 2022

if we are going to use globalThis.crypto instead of require('node:crypto').webcrypto we need a lint rule to protect this change in the upcoming pull requests.

And I've requested assistance with such in the very description of this PR myself.

@anonrig
Copy link
Member

anonrig commented Dec 11, 2022

And I've requested assistance with such in the very description of this PR myself.

I totally missed that part, my bad!

@anonrig

This comment was marked as resolved.

@panva

This comment was marked as resolved.

@panva panva force-pushed the crypto-use-global-webcrypto branch from bdc2204 to 3e0b149 Compare December 11, 2022 19:54
@anonrig
Copy link
Member

anonrig commented Dec 11, 2022

I see. I found the corresponding bug report for this: eslint/eslint#16412

@panva
Copy link
Member Author

panva commented Dec 11, 2022

I've added a working, albeit rudimentary, lint rule.

@anonrig

This comment was marked as resolved.

.eslintrc.js Show resolved Hide resolved
@panva panva force-pushed the crypto-use-global-webcrypto branch from 3e0b149 to cac299a Compare December 11, 2022 20:02
@panva

This comment was marked as resolved.

@aduh95
Copy link
Contributor

aduh95 commented Dec 11, 2022

lib/.eslintrc.yaml

Not while it's defined using no-restricted-syntax. When no-restricted-properties is fixed and used, sure.

lib/ should never use the global (which is user-mutable) but instead require the internal module. There's already a lint rule for that:

node/lib/.eslintrc.yaml

Lines 150 to 155 in 587367d

- name: crypto
message: Use `const { crypto } = require('internal/crypto/webcrypto');` instead of the global.
- name: Crypto
message: Use `const { Crypto } = require('internal/crypto/webcrypto');` instead of the global.
- name: CryptoKey
message: Use `const { CryptoKey } = require('internal/crypto/webcrypto');` instead of the global.

@aduh95 aduh95 dismissed their stale review December 11, 2022 23:32

Still -1 on this, I don't think this lint rule is a great addition, but won't block it.

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@panva panva removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 13, 2022
@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2022
@nodejs-github-bot nodejs-github-bot merged commit 7ad069c into nodejs:main Dec 16, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7ad069c

@panva panva deleted the crypto-use-global-webcrypto branch December 16, 2022 19:59
targos pushed a commit that referenced this pull request Jan 1, 2023
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
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. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants