lib: enable global WebCrypto by default

aduh95 · aduh95 · commit 6de2673a9f94 · 2022-09-17T15:54:33.000+02:00
Enables `--experimental-global-webcrypto` by default, and ensures that the classic `node:crypto` core module is still available in `--eval` or `--print` contexts. PR-URL: #42083 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -322,6 +322,7 @@ module.exports = {
     CompressionStream: 'readable',
     CountQueuingStrategy: 'readable',
     CustomEvent: 'readable',
+    crypto: 'readable',
     Crypto: 'readable',
     CryptoKey: 'readable',
     DecompressionStream: 'readable',
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -351,16 +351,6 @@ added:
 
 Expose the [CustomEvent Web API][] on the global scope.
 
-### `--experimental-global-webcrypto`
-
-<!-- YAML
-added:
-  - v17.6.0
-  - v16.15.0
--->
-
-Expose the [Web Crypto API][] on the global scope.
-
 ### `--experimental-import-meta-resolve`
 
 <!-- YAML
@@ -413,6 +403,14 @@ added: v18.0.0
 
 Disable experimental support for the [Fetch API][].
 
+### `--no-experimental-global-webcrypto`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+Disable exposition of [Web Crypto API][] on the global scope.
+
 ### `--no-experimental-repl-await`
 
 <!-- YAML
@@ -1839,7 +1837,6 @@ Node.js options that are allowed are:
 * `--enable-source-maps`
 * `--experimental-abortcontroller`
 * `--experimental-global-customevent`
-* `--experimental-global-webcrypto`
 * `--experimental-import-meta-resolve`
 * `--experimental-json-modules`
 * `--experimental-loader`
@@ -1872,6 +1869,7 @@ Node.js options that are allowed are:
 * `--no-addons`
 * `--no-deprecation`
 * `--no-experimental-fetch`
+* `--no-experimental-global-webcrypto`
 * `--no-experimental-repl-await`
 * `--no-extra-info-on-fatal-exception`
 * `--no-force-async-hooks-checks`
diff --git a/doc/api/globals.md b/doc/api/globals.md
@@ -345,10 +345,14 @@ A browser-compatible implementation of [`CountQueuingStrategy`][].
 added:
   - v17.6.0
   - v16.15.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42083
+    description: No longer behind `--experimental-global-webcrypto` CLI flag.
 -->
 
-> Stability: 1 - Experimental. Enable this API with the
-> [`--experimental-global-webcrypto`][] CLI flag.
+> Stability: 1 - Experimental. Disable this API with the
+> [`--no-experimental-global-webcrypto`][] CLI flag.
 
 A browser-compatible implementation of {Crypto}. This global is available
 only if the Node.js binary was compiled with including support for the
@@ -360,10 +364,14 @@ only if the Node.js binary was compiled with including support for the
 added:
   - v17.6.0
   - v16.15.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42083
+    description: No longer behind `--experimental-global-webcrypto` CLI flag.
 -->
 
-> Stability: 1 - Experimental. Enable this API with the
-> [`--experimental-global-webcrypto`][] CLI flag.
+> Stability: 1 - Experimental. Disable this API with the
+> [`--no-experimental-global-webcrypto`][] CLI flag.
 
 A browser-compatible implementation of the [Web Crypto API][].
 
@@ -373,10 +381,14 @@ A browser-compatible implementation of the [Web Crypto API][].
 added:
   - v17.6.0
   - v16.15.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42083
+    description: No longer behind `--experimental-global-webcrypto` CLI flag.
 -->
 
-> Stability: 1 - Experimental. Enable this API with the
-> [`--experimental-global-webcrypto`][] CLI flag.
+> Stability: 1 - Experimental. Disable this API with the
+> [`--no-experimental-global-webcrypto`][] CLI flag.
 
 A browser-compatible implementation of {CryptoKey}. This global is available
 only if the Node.js binary was compiled with including support for the
@@ -725,10 +737,14 @@ The WHATWG [`structuredClone`][] method.
 added:
   - v17.6.0
   - v16.15.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42083
+    description: No longer behind `--experimental-global-webcrypto` CLI flag.
 -->
 
-> Stability: 1 - Experimental. Enable this API with the
-> [`--experimental-global-webcrypto`][] CLI flag.
+> Stability: 1 - Experimental. Disable this API with the
+> [`--no-experimental-global-webcrypto`][] CLI flag.
 
 A browser-compatible implementation of {SubtleCrypto}. This global is available
 only if the Node.js binary was compiled with including support for the
@@ -870,8 +886,8 @@ A browser-compatible implementation of [`WritableStreamDefaultWriter`][].
 
 [Web Crypto API]: webcrypto.md
 [`--experimental-global-customevent`]: cli.md#--experimental-global-customevent
-[`--experimental-global-webcrypto`]: cli.md#--experimental-global-webcrypto
 [`--no-experimental-fetch`]: cli.md#--no-experimental-fetch
+[`--no-experimental-global-webcrypto`]: cli.md#--no-experimental-global-webcrypto
 [`AbortController`]: https://developer.mozilla.org/en-US/docs/Web/API/AbortController
 [`ByteLengthQueuingStrategy`]: webstreams.md#class-bytelengthqueuingstrategy
 [`CompressionStream`]: webstreams.md#class-compressionstream
diff --git a/doc/node.1 b/doc/node.1
@@ -165,6 +165,9 @@ Use this flag to enable ShadowRealm support.
 .It Fl -no-experimental-fetch
 Disable experimental support for the Fetch API.
 .
+.It Fl -no-experimental-global-webcrypto
+Disable exposition of the Web Crypto API on the global scope.
+.
 .It Fl -no-experimental-repl-await
 Disable top-level await keyword support in REPL.
 .
diff --git a/lib/internal/main/eval_string.js b/lib/internal/main/eval_string.js
@@ -4,6 +4,8 @@
 // `--interactive`.
 
 const {
+  ObjectDefineProperty,
+  RegExpPrototypeExec,
   globalThis,
 } = primordials;
 
@@ -25,9 +27,31 @@ const print = getOptionValue('--print');
 const loadESM = getOptionValue('--import').length > 0;
 if (getOptionValue('--input-type') === 'module')
   evalModule(source, print);
-else
+else {
+  // For backward compatibility, we want the identifier crypto to be the
+  // `node:crypto` module rather than WebCrypto.
+  const isUsingCryptoIdentifier =
+                             getOptionValue('--experimental-global-webcrypto') &&
+                             RegExpPrototypeExec(/\bcrypto\b/, source) !== null;
+  const shouldDefineCrypto = isUsingCryptoIdentifier && internalBinding('config').hasOpenSSL;
+
+  if (isUsingCryptoIdentifier && !shouldDefineCrypto) {
+    // This is taken from `addBuiltinLibsToObject`.
+    const object = globalThis;
+    const name = 'crypto';
+    const setReal = (val) => {
+      // Deleting the property before re-assigning it disables the
+      // getter/setter mechanism.
+      delete object[name];
+      object[name] = val;
+    };
+    ObjectDefineProperty(object, name, { __proto__: null, set: setReal });
+  }
   evalScript('[eval]',
-             source,
+             shouldDefineCrypto ? (
+               print ? `let crypto=require("node:crypto");{${source}}` : `(crypto=>{{${source}}})(require('node:crypto'))`
+             ) : source,
              getOptionValue('--inspect-brk'),
              print,
              loadESM);
+}
diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js
@@ -25,6 +25,7 @@ const {
 
 const {
   ERR_MANIFEST_ASSERT_INTEGRITY,
+  ERR_NO_CRYPTO,
 } = require('internal/errors').codes;
 const assert = require('internal/assert');
 
@@ -247,23 +248,29 @@ function setupFetch() {
 //               removed.
 function setupWebCrypto() {
   if (process.config.variables.node_no_browser_globals ||
-      !getOptionValue('--experimental-global-webcrypto')) {
+      getOptionValue('--no-experimental-global-webcrypto')) {
     return;
   }
 
-  let webcrypto;
-  ObjectDefineProperty(globalThis, 'crypto',
-                       { __proto__: null, ...ObjectGetOwnPropertyDescriptor({
-                         get crypto() {
-                           webcrypto ??= require('internal/crypto/webcrypto');
-                           return webcrypto.crypto;
-                         }
-                       }, 'crypto') });
   if (internalBinding('config').hasOpenSSL) {
-    webcrypto ??= require('internal/crypto/webcrypto');
+    const webcrypto = require('internal/crypto/webcrypto');
+    ObjectDefineProperty(globalThis, 'crypto',
+                         { __proto__: null, ...ObjectGetOwnPropertyDescriptor({
+                           get crypto() {
+                             return webcrypto.crypto;
+                           }
+                         }, 'crypto') });
     exposeInterface(globalThis, 'Crypto', webcrypto.Crypto);
     exposeInterface(globalThis, 'CryptoKey', webcrypto.CryptoKey);
     exposeInterface(globalThis, 'SubtleCrypto', webcrypto.SubtleCrypto);
+  } else {
+    ObjectDefineProperty(globalThis, 'crypto',
+                         { __proto__: null, ...ObjectGetOwnPropertyDescriptor({
+                           get crypto() {
+                             throw new ERR_NO_CRYPTO();
+                           }
+                         }, 'crypto') });
+
   }
 }
 
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -370,7 +370,8 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
   AddOption("--experimental-global-webcrypto",
             "expose experimental Web Crypto API on the global scope",
             &EnvironmentOptions::experimental_global_web_crypto,
-            kAllowedInEnvironment);
+            kAllowedInEnvironment,
+            true);
   AddOption("--experimental-json-modules", "", NoOp{}, kAllowedInEnvironment);
   AddOption("--experimental-loader",
             "use the specified module as a custom loader",
diff --git a/src/node_options.h b/src/node_options.h
@@ -110,7 +110,7 @@ class EnvironmentOptions : public Options {
   bool enable_source_maps = false;
   bool experimental_fetch = true;
   bool experimental_global_customevent = false;
-  bool experimental_global_web_crypto = false;
+  bool experimental_global_web_crypto = true;
   bool experimental_https_modules = false;
   std::string experimental_specifier_resolution;
   bool experimental_wasm_modules = false;
diff --git a/test/common/index.js b/test/common/index.js
@@ -357,7 +357,9 @@ if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {
     const leaked = [];
 
     for (const val in global) {
-      if (!knownGlobals.includes(global[val])) {
+      // globalThis.crypto is a getter that throws if Node.js was compiled
+      // without OpenSSL.
+      if (val !== 'crypto' && !knownGlobals.includes(global[val])) {
         leaked.push(val);
       }
     }
diff --git a/test/known_issues/test-cli-print-var-crypto.js b/test/known_issues/test-cli-print-var-crypto.js
@@ -0,0 +1,21 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+if (!common.hasCrypto) {
+  assert.fail('When Node.js is compiled without OpenSSL, overriding the global ' +
+              'crypto is allowed on string eval');
+}
+
+const child = require('child_process');
+const nodejs = `"${process.execPath}"`;
+
+// Trying to define a variable named `crypto` using `var` triggers an exception.
+
+child.exec(
+  `${nodejs} ` +
+      '-p "var crypto = {randomBytes:1};typeof crypto.randomBytes"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /^number/);
+  }));
diff --git a/test/parallel/test-assert-checktag.js b/test/parallel/test-assert-checktag.js
@@ -1,5 +1,10 @@
 'use strict';
-require('../common');
+const common = require('../common');
+
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+
 const assert = require('assert');
 
 // Disable colored output to prevent color codes from breaking assertion
diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js
@@ -206,6 +206,17 @@ if (common.hasIntl) {
   expectedModules.add('NativeModule url');
 }
 
+if (common.hasCrypto) {
+  expectedModules.add('Internal Binding crypto')
+    .add('NativeModule internal/crypto/hash')
+    .add('NativeModule internal/crypto/hashnames')
+    .add('NativeModule internal/crypto/keys')
+    .add('NativeModule internal/crypto/random')
+    .add('NativeModule internal/crypto/util')
+    .add('NativeModule internal/crypto/webcrypto')
+    .add('NativeModule internal/streams/lazy_transform');
+}
+
 if (process.features.inspector) {
   expectedModules.add('Internal Binding inspector');
   expectedModules.add('NativeModule internal/inspector_async_hook');
diff --git a/test/parallel/test-cli-eval.js b/test/parallel/test-cli-eval.js
@@ -288,3 +288,69 @@ child.exec(
   common.mustSucceed((stdout) => {
     assert.strictEqual(stdout, '.mjs file\n');
   }));
+
+if (common.hasCrypto) {
+  // Assert that calls to crypto utils work without require.
+  child.exec(
+    `${nodejs} ` +
+      '-e "console.log(crypto.randomBytes(16).toString(\'hex\'))"',
+    common.mustSucceed((stdout) => {
+      assert.match(stdout, /[0-9a-f]{32}/i);
+    }));
+  child.exec(
+    `${nodejs} ` +
+      '-p "crypto.randomBytes(16).toString(\'hex\')"',
+    common.mustSucceed((stdout) => {
+      assert.match(stdout, /[0-9a-f]{32}/i);
+    }));
+}
+// Assert that overriding crypto works.
+child.exec(
+  `${nodejs} ` +
+      '-p "crypto=Symbol(\'test\')"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /Symbol\(test\)/i);
+  }));
+child.exec(
+  `${nodejs} ` +
+    '-e "crypto = {};console.log(\'randomBytes\', typeof crypto.randomBytes)"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /randomBytes\sundefined/);
+  }));
+// Assert that overriding crypto with a local variable works.
+child.exec(
+  `${nodejs} ` +
+    '-e "const crypto = {};console.log(\'randomBytes\', typeof crypto.randomBytes)"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /randomBytes\sundefined/);
+  }));
+child.exec(
+  `${nodejs} ` +
+    '-e "let crypto = {};console.log(\'randomBytes\', typeof crypto.randomBytes)"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /randomBytes\sundefined/);
+  }));
+child.exec(
+  `${nodejs} ` +
+    '-e "var crypto = {};console.log(\'randomBytes\', typeof crypto.randomBytes)"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /randomBytes\sundefined/);
+  }));
+child.exec(
+  `${nodejs} ` +
+    '-p "const crypto = {randomBytes:1};typeof crypto.randomBytes"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /^number/);
+  }));
+child.exec(
+  `${nodejs} ` +
+    '-p "let crypto = {randomBytes:1};typeof crypto.randomBytes"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /^number/);
+  }));
+child.exec(
+  `${nodejs} --no-experimental-global-webcrypto ` +
+    '-p "var crypto = {randomBytes:1};typeof crypto.randomBytes"',
+  common.mustSucceed((stdout) => {
+    assert.match(stdout, /^number/);
+  }));
diff --git a/test/parallel/test-global-webcrypto-classes.js b/test/parallel/test-global-webcrypto-classes.js
@@ -1,4 +1,4 @@
-// Flags: --experimental-global-webcrypto --expose-internals
+// Flags: --expose-internals
 'use strict';
 
 const common = require('../common');
diff --git a/test/parallel/test-global-webcrypto-disbled.js b/test/parallel/test-global-webcrypto-disbled.js
diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js

Original file line number	Diff line number	Diff line change
`@@ -165,6 +165,9 @@ Use this flag to enable ShadowRealm support.`
`165`	`165`	`.It Fl -no-experimental-fetch`
`166`	`166`	`Disable experimental support for the Fetch API.`
`167`	`167`	`.`
	`168`	`+.It Fl -no-experimental-global-webcrypto`
	`169`	`+Disable exposition of the Web Crypto API on the global scope.`
	`170`	`+.`
`168`	`171`	`.It Fl -no-experimental-repl-await`
`169`	`172`	`Disable top-level await keyword support in REPL.`
`170`	`173`	`.`
Original file line number	Diff line number	Diff line change
`@@ -357,7 +357,9 @@ if (process.env.NODE_TEST_KNOWN_GLOBALS !== '0') {`
`357`	`357`	`const leaked = [];`
`358`	`358`
`359`	`359`	`for (const val in global) {`
`360`		`- if (!knownGlobals.includes(global[val])) {`
	`360`	`+ // globalThis.crypto is a getter that throws if Node.js was compiled`
	`361`	`+ // without OpenSSL.`
	`362`	`+ if (val !== 'crypto' && !knownGlobals.includes(global[val])) {`
`361`	`363`	`leaked.push(val);`
`362`	`364`	`}`
`363`	`365`	`}`