From 51d5c9aa7479c29ada6aaab998140e67a5a025fb Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 8 Nov 2024 06:00:20 -0800 Subject: [PATCH] Disable top-level await in require with a compat flag --- src/workerd/api/node/module.c++ | 26 +++--- .../node/tests/module-create-require-test.js | 9 +- .../tests/module-create-require-test.wd-test | 6 +- .../api/tests/new-module-registry-test.js | 2 +- src/workerd/io/compatibility-date.capnp | 8 ++ src/workerd/io/worker.c++ | 3 + src/workerd/jsg/jsg.c++ | 4 + src/workerd/jsg/jsg.h | 1 + src/workerd/jsg/modules.c++ | 84 +++++++++++-------- src/workerd/jsg/modules.h | 9 +- src/workerd/jsg/setup.h | 9 ++ 11 files changed, 112 insertions(+), 49 deletions(-) diff --git a/src/workerd/api/node/module.c++ b/src/workerd/api/node/module.c++ index 16c4fbfd697..6f2482d9599 100644 --- a/src/workerd/api/node/module.c++ +++ b/src/workerd/api/node/module.c++ @@ -3,6 +3,7 @@ // https://opensource.org/licenses/Apache-2.0 #include "module.h" +#include #include namespace workerd::api::node { @@ -84,19 +85,22 @@ jsg::JsValue ModuleUtil::createRequire(jsg::Lock& js, kj::String path) { auto module = info.module.getHandle(js); - jsg::instantiateModule(js, module); - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - if (prom->State() == v8::Promise::PromiseState::kPending) { - js.runMicrotasks(); - } - JSG_REQUIRE(prom->State() != v8::Promise::PromiseState::kPending, Error, - "Module evaluation did not complete synchronously."); - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); + auto features = FeatureFlags::get(js); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (features.getNoTopLevelAwaitInRequire()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + if (isEsm) { // If the import is an esm module, we will return the namespace object. jsg::JsObject obj(module->GetModuleNamespace().As()); diff --git a/src/workerd/api/node/tests/module-create-require-test.js b/src/workerd/api/node/tests/module-create-require-test.js index d2579495150..724a48465c0 100644 --- a/src/workerd/api/node/tests/module-create-require-test.js +++ b/src/workerd/api/node/tests/module-create-require-test.js @@ -24,7 +24,14 @@ export const doTheTest = { strictEqual(assert, required); throws(() => require('invalid'), { - message: 'Module evaluation did not complete synchronously.', + message: 'Top-level await in module is not permitted at this time.', + }); + // Trying to require the module again should throw the same error. + throws(() => require('invalid'), { + message: 'Top-level await in module is not permitted at this time.', + }); + throws(() => require('invalid2'), { + message: 'Top-level await in module is not permitted at this time.', }); throws(() => require('does not exist')); diff --git a/src/workerd/api/node/tests/module-create-require-test.wd-test b/src/workerd/api/node/tests/module-create-require-test.wd-test index cc97fcb5074..c550ff58d5e 100644 --- a/src/workerd/api/node/tests/module-create-require-test.wd-test +++ b/src/workerd/api/node/tests/module-create-require-test.wd-test @@ -10,10 +10,12 @@ const unitTests :Workerd.Config = ( (name = "bar", esModule = "export default 2; export const __cjsUnwrapDefault = true;"), (name = "baz", commonJsModule = "module.exports = 3;"), (name = "worker/qux", text = "4"), - (name = "invalid", esModule = "const p = new Promise(() => {}); await p;"), + (name = "invalid", esModule = "await new Promise(() => {});"), + (name = "invalid2", commonJsModule = "require('invalid3');"), + (name = "invalid3", esModule = "await new Promise(() => {});"), ], compatibilityDate = "2024-08-01", - compatibilityFlags = ["nodejs_compat_v2"] + compatibilityFlags = ["disable_top_level_await_in_require", "nodejs_compat_v2"] ) ), ], diff --git a/src/workerd/api/tests/new-module-registry-test.js b/src/workerd/api/tests/new-module-registry-test.js index d271a41b6e4..6013b7885fb 100644 --- a/src/workerd/api/tests/new-module-registry-test.js +++ b/src/workerd/api/tests/new-module-registry-test.js @@ -8,7 +8,7 @@ import { import { foo, default as def } from 'foo'; import { default as fs } from 'node:fs'; import { Buffer } from 'buffer'; -const { foo: foo2, default: def2 } = await import('bar'); +import { foo as foo2, default as def2 } from 'bar'; // Verify that import.meta.url is correct here. strictEqual(import.meta.url, 'file:///worker'); diff --git a/src/workerd/io/compatibility-date.capnp b/src/workerd/io/compatibility-date.capnp index b0fa6b93736..8fc235973a1 100644 --- a/src/workerd/io/compatibility-date.capnp +++ b/src/workerd/io/compatibility-date.capnp @@ -658,4 +658,12 @@ struct CompatibilityFlags @0x8f8c1b68151b6cef { # # This is a compat flag so that we can opt in our test workers into it before rolling out to # everyone. + + noTopLevelAwaitInRequire @67 :Bool + $compatEnableFlag("disable_top_level_await_in_require") + $compatDisableFlag("enable_top_level_await_in_require") + $compatEnableDate("2024-12-02"); + # When enabled, use of top-level await syntax in require() calls will be disallowed. + # The ecosystem and runtimes are moving to a state where top level await in modules + # is being strongly discouraged. } diff --git a/src/workerd/io/worker.c++ b/src/workerd/io/worker.c++ index c56b49438bb..467abe1d0bc 100644 --- a/src/workerd/io/worker.c++ +++ b/src/workerd/io/worker.c++ @@ -999,6 +999,9 @@ Worker::Isolate::Isolate(kj::Own apiParam, if (features.getNodeJsCompatV2()) { lock->setNodeJsCompatEnabled(); } + if (!features.getNoTopLevelAwaitInRequire()) { + lock->enableTopLevelAwait(); + } if (impl->inspector != kj::none || ::kj::_::Debug::shouldLog(::kj::LogSeverity::INFO)) { lock->setLoggerCallback([this](jsg::Lock& js, kj::StringPtr message) { diff --git a/src/workerd/jsg/jsg.c++ b/src/workerd/jsg/jsg.c++ index 05e346f2af0..e1cfe5916fd 100644 --- a/src/workerd/jsg/jsg.c++ +++ b/src/workerd/jsg/jsg.c++ @@ -195,6 +195,10 @@ void Lock::setNodeJsCompatEnabled() { IsolateBase::from(v8Isolate).setNodeJsCompatEnabled({}, true); } +void Lock::enableTopLevelAwait() { + IsolateBase::from(v8Isolate).enableTopLevelAwait(); +} + void Lock::setToStringTag() { IsolateBase::from(v8Isolate).enableSetToStringTag(); } diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 3190cdfd966..0b73927475b 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -2516,6 +2516,7 @@ class Lock { void setNodeJsCompatEnabled(); void setToStringTag(); + void enableTopLevelAwait(); using Logger = void(Lock&, kj::StringPtr); void setLoggerCallback(kj::Function&& logger); diff --git a/src/workerd/jsg/modules.c++ b/src/workerd/jsg/modules.c++ index 4107313bb88..bc5b1a53eb8 100644 --- a/src/workerd/jsg/modules.c++ +++ b/src/workerd/jsg/modules.c++ @@ -272,24 +272,24 @@ v8::Local CommonJsModuleContext::require(jsg::Lock& js, kj::String sp // Adding imported from suffix here not necessary like it is for resolveCallback, since we have a // js stack that will include the parent module's name and location of the failed require(). - JSG_REQUIRE_NONNULL( - info.maybeSynthetic, TypeError, "Cannot use require() to import an ES Module."); - auto module = info.module.getHandle(js); - check(module->InstantiateModule(js.v8Context(), &resolveCallback)); - auto handle = check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - - // This assert should always pass since evaluateSyntheticModuleCallback() for CommonJS - // modules (below) always returns an already-resolved promise. - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); - - if (module->GetStatus() == v8::Module::kErrored) { - throwTunneledException(js.v8Isolate, module->GetException()); + auto& isolateBase = IsolateBase::from(js.v8Isolate); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (!isolateBase.isTopLevelAwaitEnabled()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + // Originally, This returned an object like `{default: module.exports}` when we really // intended to return the module exports raw. We should be extracting `default` here. // Unfortunately, there is a user depending on the wrong behavior in production, so we @@ -322,15 +322,22 @@ NonModuleScript NonModuleScript::compile(kj::StringPtr code, jsg::Lock& js, kj:: return NonModuleScript(js, check(v8::ScriptCompiler::CompileUnboundScript(isolate, &source))); } -void instantiateModule(jsg::Lock& js, v8::Local& module) { +void instantiateModule( + jsg::Lock& js, v8::Local& module, InstantiateModuleOptions options) { KJ_ASSERT(!module.IsEmpty()); auto isolate = js.v8Isolate; auto context = js.v8Context(); auto status = module->GetStatus(); - // Nothing to do if the module is already instantiated, evaluated, or errored. - if (status == v8::Module::Status::kInstantiated || status == v8::Module::Status::kEvaluated || - status == v8::Module::Status::kErrored) + + // If the previous instantiation failed, throw the exception. + if (status == v8::Module::Status::kErrored) { + isolate->ThrowException(module->GetException()); + throw jsg::JsExceptionThrown(); + } + + // Nothing to do if the module is already instantiated, evaluated. + if (status == v8::Module::Status::kInstantiated || status == v8::Module::Status::kEvaluated) return; JSG_REQUIRE(status == v8::Module::Status::kUninstantiated, Error, @@ -341,13 +348,22 @@ void instantiateModule(jsg::Lock& js, v8::Local& module) { jsg::check(module->InstantiateModule(context, &resolveCallback)); auto prom = jsg::check(module->Evaluate(context)).As(); - js.runMicrotasks(); + + if (module->IsGraphAsync() && prom->State() == v8::Promise::kPending) { + // Pump the microtasks if there's an unsettled top-level await in the module. + // Because we do not support i/o in this scope, this *should* resolve in a + // single drain of the microtask queue (tho it's possible that it'll take + // multiple tasks). When the runMicrotasks() is complete, the promise should + // be settled. + JSG_REQUIRE(options != InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT, Error, + "Top-level await in module is not permitted at this time."); + js.runMicrotasks(); + } switch (prom->State()) { case v8::Promise::kPending: // Let's make sure nobody is depending on pending modules that do not resolve first. - KJ_LOG(WARNING, "Async module was not immediately resolved."); - break; + JSG_FAIL_REQUIRE(Error, "Top-level await in module is unsettled."); case v8::Promise::kRejected: // Since we don't actually support I/O when instantiating a worker, we don't return the // promise from module->Evaluate, which means we lose any errors that happen during @@ -594,20 +610,22 @@ v8::Local NodeJsModuleContext::require(jsg::Lock& js, kj::String spec auto module = info.module.getHandle(js); - jsg::instantiateModule(js, module); - - auto handle = jsg::check(module->Evaluate(js.v8Context())); - KJ_ASSERT(handle->IsPromise()); - auto prom = handle.As(); - - // This assert should always pass since evaluateSyntheticModuleCallback() for CommonJS - // modules (below) always returns an already-resolved promise. - KJ_ASSERT(prom->State() != v8::Promise::PromiseState::kPending); - - if (module->GetStatus() == v8::Module::kErrored) { - jsg::throwTunneledException(js.v8Isolate, module->GetException()); + auto& isolateBase = IsolateBase::from(js.v8Isolate); + jsg::InstantiateModuleOptions options = jsg::InstantiateModuleOptions::DEFAULT; + if (!isolateBase.isTopLevelAwaitEnabled()) { + options = jsg::InstantiateModuleOptions::NO_TOP_LEVEL_AWAIT; + + // If the module was already evaluated, let's check if it is async. + // If it is, we will throw an error. This case can happen if a previous + // attempt to require the module failed because the module was async. + if (module->GetStatus() == v8::Module::kEvaluated) { + JSG_REQUIRE(!module->IsGraphAsync(), Error, + "Top-level await in module is not permitted at this time."); + } } + jsg::instantiateModule(js, module, options); + return js.v8Get(module->GetModuleNamespace().As(), "default"_kj); } diff --git a/src/workerd/jsg/modules.h b/src/workerd/jsg/modules.h index 0b68aca2726..b418582b8ef 100644 --- a/src/workerd/jsg/modules.h +++ b/src/workerd/jsg/modules.h @@ -191,7 +191,14 @@ class NonModuleScript { v8::Global unboundScript; }; -void instantiateModule(jsg::Lock& js, v8::Local& module); +enum class InstantiateModuleOptions { + DEFAULT, + NO_TOP_LEVEL_AWAIT, +}; + +void instantiateModule(jsg::Lock& js, + v8::Local& module, + InstantiateModuleOptions options = InstantiateModuleOptions::DEFAULT); enum class ModuleInfoCompileOption { // The BUNDLE options tells the compile operation to treat the content as coming diff --git a/src/workerd/jsg/setup.h b/src/workerd/jsg/setup.h index c3ad32cb5d4..31d50888227 100644 --- a/src/workerd/jsg/setup.h +++ b/src/workerd/jsg/setup.h @@ -151,6 +151,14 @@ class IsolateBase { setToStringTag = true; } + inline void enableTopLevelAwait() { + allowTopLevelAwait = true; + } + + inline bool isTopLevelAwaitEnabled() const { + return allowTopLevelAwait; + } + // The logger will be optionally set by the isolate setup logic if there is anywhere // for the log to go (for instance, if debug logging is enabled or the inspector is // being used). @@ -238,6 +246,7 @@ class IsolateBase { bool asyncContextTrackingEnabled = false; bool nodeJsCompatEnabled = false; bool setToStringTag = false; + bool allowTopLevelAwait = false; kj::Maybe> maybeLogger; kj::Maybe> maybeErrorReporter;