From dda33c2bf13a39b504b60859c00b0556e73060eb Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 23:08:44 +0200 Subject: [PATCH] vm: reject in importModuleDynamically without --experimental-vm-modules Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: https://github.com/nodejs/node/pull/50137 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- doc/api/errors.md | 6 ++++ doc/api/vm.md | 24 ++++++++++++---- lib/internal/errors.js | 3 ++ lib/internal/modules/esm/utils.js | 8 +++++- lib/internal/vm.js | 16 +++++++++++ src/env_properties.h | 3 +- ...vm-dynamic-import-callback-missing-flag.js | 28 +++++++++++++++++++ 7 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 test/parallel/test-vm-dynamic-import-callback-missing-flag.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 24b1d0afeede9a..8070fffc388b9f 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2981,6 +2981,12 @@ An attempt was made to use something that was already closed. While using the Performance Timing API (`perf_hooks`), no valid performance entry types are found. + + +### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG` + +A dynamic import callback was invoked without `--experimental-vm-modules`. + ### `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING` diff --git a/doc/api/vm.md b/doc/api/vm.md index e5c116d54af0bf..2043a39f527249 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -98,7 +98,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAssertions` {Object} The `"assert"` value passed to the @@ -760,6 +762,9 @@ changes: * `importModuleDynamically` {Function} Called during evaluation of this module when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. + If `--experimental-vm-modules` isn't set, this callback will be ignored + and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `module` {vm.Module} * `importAssertions` {Object} The `"assert"` value passed to the @@ -1018,7 +1023,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API, and should not be - considered stable. + considered stable. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `function` {Function} * `importAssertions` {Object} The `"assert"` value passed to the @@ -1242,7 +1249,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAssertions` {Object} The `"assert"` value passed to the @@ -1341,7 +1350,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAssertions` {Object} The `"assert"` value passed to the @@ -1421,7 +1432,9 @@ changes: when `import()` is called. If this option is not specified, calls to `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. This option is part of the experimental modules API. We do not recommend - using it in a production environment. + using it in a production environment. If `--experimental-vm-modules` isn't + set, this callback will be ignored and calls to `import()` will reject with + [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`][]. * `specifier` {string} specifier passed to `import()` * `script` {vm.Script} * `importAssertions` {Object} The `"assert"` value passed to the @@ -1585,6 +1598,7 @@ are not controllable through the timeout either. [Source Text Module Record]: https://tc39.es/ecma262/#sec-source-text-module-records [Synthetic Module Record]: https://heycam.github.io/webidl/#synthetic-module-records [V8 Embedder's Guide]: https://v8.dev/docs/embed#contexts +[`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`]: errors.md#err_vm_dynamic_import_callback_missing_flag [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`]: errors.md#err_vm_dynamic_import_callback_missing [`ERR_VM_MODULE_STATUS`]: errors.md#err_vm_module_status [`Error`]: errors.md#class-error diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 76ce7867043c7d..a72236063b47fd 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1821,6 +1821,9 @@ E('ERR_VALID_PERFORMANCE_ENTRY_TYPE', 'At least one valid performance entry type is required', Error); E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING', 'A dynamic import callback was not specified.', TypeError); +E('ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG', + 'A dynamic import callback was invoked without --experimental-vm-modules', + TypeError); E('ERR_VM_MODULE_ALREADY_LINKED', 'Module has already been linked', Error); E('ERR_VM_MODULE_CANNOT_CREATE_CACHED_DATA', 'Cached data cannot be created for a module which has been evaluated', Error); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index cbb583ede526b5..9d3e1aefbe3a31 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -14,9 +14,11 @@ const { } = internalBinding('util'); const { default_host_defined_options, + vm_dynamic_import_missing_flag, } = internalBinding('symbols'); const { + ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG, ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, ERR_INVALID_ARG_VALUE, } = require('internal/errors').codes; @@ -132,7 +134,8 @@ const moduleRegistries = new SafeWeakMap(); */ function registerModule(referrer, registry) { const idSymbol = referrer[host_defined_option_symbol]; - if (idSymbol === default_host_defined_options) { + if (idSymbol === default_host_defined_options || + idSymbol === vm_dynamic_import_missing_flag) { // The referrer is compiled without custom callbacks, so there is // no registry to hold on to. We'll throw // ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING when a callback is @@ -173,6 +176,9 @@ async function importModuleDynamicallyCallback(symbol, specifier, attributes) { return importModuleDynamically(specifier, callbackReferrer, attributes); } } + if (symbol === vm_dynamic_import_missing_flag) { + throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG(); + } throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING(); } diff --git a/lib/internal/vm.js b/lib/internal/vm.js index 4f7b8c652f3c26..b67eb177b35f07 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -15,6 +15,7 @@ const { } = ContextifyScript.prototype; const { default_host_defined_options, + vm_dynamic_import_missing_flag, } = internalBinding('symbols'); const { validateFunction, @@ -22,6 +23,11 @@ const { kValidateObjectAllowArray, } = require('internal/validators'); +const { + getOptionValue, +} = require('internal/options'); + + function isContext(object) { validateObject(object, 'object', kValidateObjectAllowArray); @@ -41,6 +47,16 @@ function getHostDefinedOptionId(importModuleDynamically, filename) { // compilation cache can be hit. return default_host_defined_options; } + // We should've thrown here immediately when we introduced + // --experimental-vm-modules and importModuleDynamically, but since + // users are already using this callback to throw a similar error, + // we also defer the error to the time when an actual import() is called + // to avoid breaking them. To ensure that the isolate compilation + // cache can still be hit, use a constant sentinel symbol here. + if (!getOptionValue('--experimental-vm-modules')) { + return vm_dynamic_import_missing_flag; + } + return Symbol(filename); } diff --git a/src/env_properties.h b/src/env_properties.h index 93fa8afdca39d3..7356846d6e7085 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -46,7 +46,8 @@ V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ V(resource_symbol, "resource_symbol") \ - V(trigger_async_id_symbol, "trigger_async_id_symbol") + V(trigger_async_id_symbol, "trigger_async_id_symbol") \ + V(vm_dynamic_import_missing_flag, "vm_dynamic_import_missing_flag") // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. diff --git a/test/parallel/test-vm-dynamic-import-callback-missing-flag.js b/test/parallel/test-vm-dynamic-import-callback-missing-flag.js new file mode 100644 index 00000000000000..4b0d09ca3674a7 --- /dev/null +++ b/test/parallel/test-vm-dynamic-import-callback-missing-flag.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const { Script, compileFunction } = require('vm'); +const assert = require('assert'); + +assert( + !process.execArgv.includes('--experimental-vm-modules'), + 'This test must be run without --experimental-vm-modules'); + +assert.rejects(async () => { + const script = new Script('import("fs")', { + importModuleDynamically: common.mustNotCall(), + }); + const imported = script.runInThisContext(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG' +}).then(common.mustCall()); + +assert.rejects(async () => { + const imported = compileFunction('return import("fs")', [], { + importModuleDynamically: common.mustNotCall(), + })(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG' +}).then(common.mustCall());