crypto: clarify `require("crypto").getRandomValues` is Node.js specific by aduh95 · Pull Request #41782 · nodejs/node · GitHub

Review requested:

@nodejs/crypto

… specific

@jasnell could you help us here? It seems there is a confusion on what is the plan for exposing a crypto object on the global scope. Is the plan:

Exposing require('crypto') as crypto everywhere?
Exposing require('crypto').webcrypto as crypto everywhere, breaking change for the REPL?
Exposing require('crypto').webcrypto as crypto on files, keep exposing require('crypto') as crypto on the REPL?

If it's the latter, then I guess we should land this PR, as well as #41760, but this would need your input to move forward.

My preference is exposing require('crypto') as the global.

My preference is exposing require('crypto') as the global.

This is going to create all sort of compatibility problems (I've been myself affected by #41760), I'm -1 on this idea. Adding it to the TSC agenda so we can discuss this further.

Exposing require('crypto').webcrypto as crypto on files, keep exposing require('crypto') as crypto on the REPL?

I suggested this when talking to @jasnell earlier, except that I'd maybe add the aliases to crypto only in the REPL. (Not sure if it's a good idea after all.)

One possible approach we can take here is to modify Node.js' crypto to extend from Web Crypto crypto. Specifically, something like:

class NodeCrypto extends Crypto {
  // ... add the Node.js stuff...
};

module.exports = new NodeCrypto();

One possible approach we can take here is to modify Node.js' crypto to extend from Web Crypto crypto. Specifically, something like:
class NodeCrypto extends Crypto {
  // ... add the Node.js stuff...
};

module.exports = new NodeCrypto();

It wouldn't help folks doing import { getRandomValues } from 'node:crypto', right? In that case, that'd be fine with me, as long as we explicitly say that it's not a use-case we want to support.

Could you maybe explain why we wouldn't expose require('crypto').webcrypto as global.crypto? I'm uncomfortable at the idea of introducing another Node.js specific global, especially if it's competing with one already in browsers and Deno. Is it because REPL (and node --eval) already expose a global-like named crypto and you are concerned it would be too breaking to change that? Sorry if you already discussed that somewhere else, but I feel like I'm missing some context here.

Is it because REPL (and node --eval) already expose a global-like named crypto and you are concerned it would be too breaking to change that?

Yes, I think that was the motivation behind #41266 (see #41266 (comment)).

FYI, I've opened #41938 to suggest exposing the Web Crypto on the global scope instead of the Node.js one.

PR-URL: #41938 Refs: https://developer.mozilla.org/en-US/docs/Web/API/crypto_property Refs: #41782 Refs: https://w3c.github.io/webcrypto Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

FYI, I've opened #41938 to suggest exposing the Web Crypto on the global scope instead of the Node.js one.

@aduh95 Now that #41938 has landed, should this be closed or would you still like to remove require('crypto').getRandomValues?

would you still like to remove require('crypto').getRandomValues?

I don't think we get much value from having require('crypto').getRandomValues if it's already available (in a web-compatible way) through globalThis.crypto.getRandomValues. I still think we should remove it, or make it web compatible.

should this be closed

Whatever we decide to do we the alias, I think we should land this ASAP: all this PR is doing is documenting that the shortcut is not exactly the same as the web version, and it unblocks #41760.

Currently our docs for Web crypto are "broken", for example here we tell the user to use object destructuring to get a reference to getRandomValues:

node/doc/api/webcrypto.md

Lines 128 to 159 in b8de7aa

    
           ### Encryption and decryption 
        
           ```js 
        
           const { subtle, getRandomValues } = require('crypto').webcrypto; 
        
           async function aesEncrypt(plaintext) { 
        
             const ec = new TextEncoder(); 
        
             const key = await generateAesKey(); 
        
             const iv = getRandomValues(new Uint8Array(16)); 
        
             const ciphertext = await subtle.encrypt({ 
        
               name: 'AES-CBC', 
        
               iv, 
        
             }, key, ec.encode(plaintext)); 
        
             return { 
        
               key, 
        
               iv, 
        
               ciphertext 
        
             }; 
        
           } 
        
           async function aesDecrypt(ciphertext, key, iv) { 
        
             const dec = new TextDecoder(); 
        
             const plaintext = await subtle.decrypt({ 
        
               name: 'AES-CBC', 
        
               iv, 
        
             }, key, ciphertext); 
        
             return dec.decode(plaintext); 
        
           } 
        
           ```

If the user tries to use this code in a browser or Deno, it will throw an arguably quite surprising error (e.g. Chromium throws TypeError: Illegal invocation).

lgtm

@jasnell have you got any concerns in landing this as-is?

… specific

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

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

…Node.js specific

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

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

PR-URL: nodejs#41938 Refs: https://developer.mozilla.org/en-US/docs/Web/API/crypto_property Refs: nodejs#41782 Refs: https://w3c.github.io/webcrypto Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

PR-URL: #41938 Refs: https://developer.mozilla.org/en-US/docs/Web/API/crypto_property Refs: #41782 Refs: https://w3c.github.io/webcrypto Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Landed in 470c284

Refs: nodejs#41779 Refs: nodejs#41760 PR-URL: nodejs#41782 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@aduh95 this doesn't land cleanly on v16.x-staging. Can you make a backport for this? Thank you

PR-URL: nodejs#41938 Refs: https://developer.mozilla.org/en-US/docs/Web/API/crypto_property Refs: nodejs#41782 Refs: https://w3c.github.io/webcrypto Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

PR-URL: #41938 Refs: https://developer.mozilla.org/en-US/docs/Web/API/crypto_property Refs: #41782 Refs: https://w3c.github.io/webcrypto Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>

crypto: clarify require("crypto").getRandomValues is Node.js specific

f760ca2

Refs: nodejs#41779 Refs: nodejs#41760

aduh95 requested review from jasnell and targos January 31, 2022 10:43

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jan 31, 2022

targos reviewed Jan 31, 2022

View reviewed changes

Comment thread lib/crypto.js Outdated

fixup! crypto: clarify require("crypto").getRandomValues is Node.js…

e77161e

… specific

aduh95 added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 2, 2022

mhdawson mentioned this pull request Feb 7, 2022

Node.js Technical Steering Committee (TSC) Meeting 2022-02-10 nodejs/TSC#1162

Closed

aduh95 mentioned this pull request Feb 11, 2022

crypto: expose Web Crypto API on the global scope #41938

Merged

mhdawson mentioned this pull request Feb 14, 2022

Node.js Technical Steering Committee (TSC) Meeting 2022-02-16 nodejs/TSC#1166

Closed

mcollina approved these changes Feb 16, 2022

View reviewed changes

mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 16, 2022

tniessen reviewed Feb 16, 2022

View reviewed changes

Comment thread doc/api/crypto.md Outdated

fixup! crypto: clarify require("crypto").getRandomValues is Node.js…

a341523

… specific

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 19, 2022

aduh95 added the review wanted PRs that need reviews. label Feb 19, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2022

fixup! fixup! crypto: clarify require("crypto").getRandomValues is …

6e0f787

…Node.js specific

This was referenced Feb 20, 2022

CI Reliability 2022-02-20 nodejs/reliability#208

Open

CI Reliability 2022-02-21 nodejs/reliability#209

Open

github-actions Bot mentioned this pull request Feb 22, 2022

CI Reliability 2022-02-22 nodejs/reliability#210

Open

22 tasks

aduh95 merged commit 470c284 into nodejs:master Feb 22, 2022

aduh95 deleted the change-getrandomvalues-alias branch February 22, 2022 16:52

sxa pushed a commit to sxa/node that referenced this pull request Mar 7, 2022

crypto: clarify require("crypto").getRandomValues is Node.js specific

288f627

Refs: nodejs#41779 Refs: nodejs#41760 PR-URL: nodejs#41782 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

sxa mentioned this pull request Mar 8, 2022

v17.7.0 release proposal #42254

Merged

danielleadams added the backport-requested-v16.x label Apr 19, 2022

targos added dont-land-on-v16.x and removed backport-requested-v16.x labels Jul 31, 2022

brc-dd mentioned this pull request Oct 15, 2023

fix(node): use webcrypto export from node:crypto ai/nanoid#448

Merged

coderabbitai Bot mentioned this pull request Jul 17, 2026

webcrypto: raise Node v26.3.0 compat to 57% of the upstream suite (13→30 tests), match error semantics and KeyUsage ordering oven-sh/bun#34431

Merged

coderabbitai Bot mentioned this pull request Aug 14, 2026

Build private Owner Application zaingulel/RentCottage#62

Merged

crypto: clarify `require("crypto").getRandomValues` is Node.js specific by aduh95 · Pull Request #41782 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: clarify require("crypto").getRandomValues is Node.js specific - #41782

crypto: clarify require("crypto").getRandomValues is Node.js specific#41782aduh95 merged 4 commits intonodejs:masternodejs/node:masterfrom aduh95:change-getrandomvalues-aliasaduh95/node:change-getrandomvalues-aliasCopy head branch name to clipboard

Conversation

aduh95 commented Jan 31, 2022

Uh oh!

nodejs-github-bot commented Jan 31, 2022

Uh oh!

Uh oh!

aduh95 commented Feb 2, 2022

Uh oh!

jasnell commented Feb 2, 2022

Uh oh!

aduh95 commented Feb 2, 2022

Uh oh!

tniessen commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Feb 5, 2022

Uh oh!

aduh95 commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Feb 10, 2022

Uh oh!

aduh95 commented Feb 11, 2022

Uh oh!

tniessen commented Feb 16, 2022

Uh oh!

aduh95 commented Feb 16, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Feb 16, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

nodejs-github-bot commented Feb 19, 2022

Uh oh!

aduh95 commented Feb 22, 2022

Uh oh!

danielleadams commented Apr 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: clarify require("crypto").getRandomValues is Node.js specific#41782
aduh95 merged 4 commits into
nodejs:masterfrom
aduh95:change-getrandomvalues-alias

tniessen commented Feb 2, 2022 •
edited
Loading

aduh95 commented Feb 6, 2022 •
edited
Loading