Merge pull request #5142 from cloudflare/jasnell/start-enabling-nmr-internal

jasnell · web-flow · commit b3dd0fd82d65 · 2025-09-23T15:38:57.000-07:00
diff --git a/src/workerd/api/tests/new-module-registry-test.js b/src/workerd/api/tests/new-module-registry-test.js
@@ -139,6 +139,54 @@ await rejects(import('module-not-found'), {
   message: /Module not found: file:\/\/\/bundle\/module-not-found/,
 });
 
+await rejects(import('file:///outside'), {
+  message: /Module not found/,
+});
+
+await import('file:///bundle/outside');
+
+const abc123 = await import('abc123');
+strictEqual(abc123.default, 1);
+
+// Full URLs can be used as module specifiers. These would not
+// actually resolve to network requests.
+const mod = await import('https://example.com/mod');
+strictEqual(mod.default, 'example');
+
+// Whitespace and leading ./, ../, and multiple slashes are
+// stripped and ignored when setting up the module registry,
+// so they should resolve as expected here as being relative
+// to the bundle base URL.
+const mod2 = await import('   ./should/be/ok   ');
+strictEqual(mod2.default, 1);
+
+// UTF-8 percent-encoded of 部品 (Japanese for "component")
+const mod3 = await import('%E9%83%A8%E5%93%81');
+const mod4 = await import('部品'); // Get's converted into UTF-8 bytes in source
+const mod5 = await import('\u90e8\u54c1'); // Specifically UTF-16 code units in source
+const mod6 = await import('\xE9\x83\xA8\xE5\x93\x81');
+import { default as mod7 } from '部品';
+import { default as mod8 } from '\u90e8\u54c1';
+import { default as mod9 } from '\xE9\x83\xA8\xE5\x93\x81';
+import { default as mod10 } from '%E9%83%A8%E5%93%81';
+
+strictEqual(mod3.default, 1);
+strictEqual(mod4.default, 1);
+strictEqual(mod5.default, 1);
+strictEqual(mod6.default, 1);
+strictEqual(mod7, 1);
+strictEqual(mod8, 1);
+strictEqual(mod9, 1);
+strictEqual(mod10, 1);
+
+// The percent-encoded UTF-16 form of 部品 should not work.
+await rejects(import('%E8%90%C1%54'), {
+  message: /Module not found/,
+});
+await rejects(import('%90%E8%54%C1'), {
+  message: /Module not found/,
+});
+
 // Verify that a module is unable to perform IO operations at the top level, even if
 // the dynamic import is initiated within the scope of an active IoContext.
 export const noTopLevelIo = {
@@ -260,6 +308,17 @@ export const wasmDynamicSourcePhaseFailureTest = {
   },
 };
 
+export const complexModuleTest = {
+  async test() {
+    const { abc } = await import('complex');
+    strictEqual(abc.foo, 1);
+    strictEqual(abc.def.bar, 2);
+
+    const { default: abc2 } = await import('abc');
+    strictEqual(abc2, 'file:///bundle/abc');
+  },
+};
+
 // TODO(now): Tests
 // * [x] Include tests for all known module types
 //   * [x] ESM
diff --git a/src/workerd/api/tests/new-module-registry-test.wd-test b/src/workerd/api/tests/new-module-registry-test.wd-test
@@ -41,7 +41,33 @@ const unitTests :Workerd.Config = (
           (name = "json", json = "{ \"foo\": 1 }"),
           (name = "invalid-json", json = "1n"),
 
-          (name = "wasm", wasm = embed "test.wasm")
+          (name = "wasm", wasm = embed "test.wasm"),
+
+          (name = "complex", esModule = "import * as abc from '././././././complex/complex2'; export { abc };"),
+          (name = "complex/complex2", esModule = "import * as def from '/bundle/complex/complex3'; export { def }; export const foo = 1;"),
+          (name = "complex/complex3", esModule = "export const bar = 2;"),
+
+          # When the user bundle name begins with a slash, it is stripped before
+          # processing, ensuring that the module is still processed relative to
+          # the bundle base URL. Query strings and fragments are ignored.
+          (name = "/%61bc?abc#123", esModule = "export default import.meta.url"),
+
+          # A module name should not be able to escape the bundle base URL,
+          # and leading/trailing whitespace should be trimmed.
+          (name = "    ..//////outside    ", esModule = "export default 1;"),
+
+          # Should resolve to "abc123" after normalization.
+          (name = "/foo/../bar/../../../../////abc123", esModule = "export default 1;"),
+
+          # It should be possible to have a module whose name is a valid URL.
+          (name = "https://example.com/mod", esModule = "export default 'example';"),
+
+          # Percent-encoded characters in the path should be normalized, and absolute
+          # file URLs are accepted as long as they are under the bundle base URL.
+          (name = "file:///bundl%65/should/b%65/ok", esModule = "export default 1;"),
+
+          # Unicode characters in the path should be handled as UTF-8 and supported.
+          (name = "部品", esModule = "export default 1;"),
         ],
         compatibilityDate = "2025-05-01",
         compatibilityFlags = [
diff --git a/src/workerd/io/worker-modules.h b/src/workerd/io/worker-modules.h
@@ -104,7 +104,8 @@ static kj::Arc<jsg::modules::ModuleRegistry> newWorkerModuleRegistry(
     const CompatibilityFlags::Reader& featureFlags,
     const jsg::Url& bundleBase,
     auto setupForApi,
-    jsg::modules::ModuleRegistry::Builder::Options options) {
+    jsg::modules::ModuleRegistry::Builder::Options options =
+        jsg::modules::ModuleRegistry::Builder::Options::NONE) {
   jsg::modules::ModuleRegistry::Builder builder(resolveObserver, bundleBase, options);
 
   // This callback is used when a module is being loaded to arrange evaluating the
@@ -206,13 +207,14 @@ static kj::Arc<jsg::modules::ModuleRegistry> newWorkerModuleRegistry(
           break;
         }
         KJ_CASE_ONEOF(content, Worker::Script::PythonModule) {
-          KJ_REQUIRE(featureFlags.getPythonWorkers(),
-              "The python_workers compatibility flag is required to use Python.");
-          firstEsm = false;
-          hasPythonModules = true;
-          kj::StringPtr entry = PYTHON_ENTRYPOINT;
-          bundleBuilder.addEsmModule(def.name, entry);
-          break;
+          KJ_FAIL_ASSERT("Python modules are not currently supported with the new module registry");
+          // KJ_REQUIRE(featureFlags.getPythonWorkers(),
+          //     "The python_workers compatibility flag is required to use Python.");
+          // firstEsm = false;
+          // hasPythonModules = true;
+          // kj::StringPtr entry = PYTHON_ENTRYPOINT;
+          // bundleBuilder.addEsmModule(def.name, entry);
+          // break;
         }
         KJ_CASE_ONEOF(content, Worker::Script::PythonRequirement) {
           // Handled separately
diff --git a/src/workerd/jsg/modules-new.c++ b/src/workerd/jsg/modules-new.c++
@@ -25,6 +25,22 @@ kj::Maybe<const Module&> checkModule(const ResolveContext& context, const Module
   return module;
 };
 
+kj::String specifierToString(jsg::Lock& js, v8::Local<v8::String> spec) {
+  // Source files in workers end up being converted to UTF-8 bytes, so if the specifier
+  // string contains non-ASCII unicode characters, those will be directly encoded as UTF-8
+  // bytes, which unfortunately end up double-encoded if we try to read them using the
+  // regular js.toString() method. Doh! Fortunately they come through as one-byte strings,
+  // so we can detect that case and handle those correctly here.
+  if (spec->ContainsOnlyOneByte()) {
+    auto buf = kj::heapArray<char>(spec->Length() + 1);
+    spec->WriteOneByteV2(js.v8Isolate, 0, spec->Length(), buf.asBytes().begin(),
+        v8::String::WriteFlags::kNullTerminate);
+    KJ_ASSERT(buf[buf.size() - 1] == '\0');
+    return kj::String(kj::mv(buf));
+  }
+  return js.toString(spec);
+}
+
 // Ensure that the given module has been instantiated or errored.
 // If false is returned, then an exception should have been scheduled
 // on the isolate.
@@ -746,7 +762,7 @@ v8::MaybeLocal<v8::Promise> dynamicImportModuleCallback(v8::Local<v8::Context> c
   auto& registry = IsolateModuleRegistry::from(js.v8Isolate);
   try {
     return js.tryCatch([&]() -> v8::MaybeLocal<v8::Promise> {
-      auto spec = js.toString(specifier);
+      auto spec = specifierToString(js, specifier);
 
       // The proposed specification for import attributes strongly recommends that
       // embedders reject import attributes and types they do not understand/implement.
@@ -862,7 +878,7 @@ v8::MaybeLocal<std::conditional_t<IsSourcePhase, v8::Object, v8::Module>> resolv
   auto& registry = IsolateModuleRegistry::from(js.v8Isolate);
 
   return js.tryCatch([&]() -> v8::MaybeLocal<ReturnType> {
-    auto spec = kj::str(specifier);
+    auto spec = specifierToString(js, specifier);
 
     // The proposed specification for import attributes strongly recommends that
     // embedders reject import attributes and types they do not understand/implement.
@@ -1201,11 +1217,100 @@ ModuleBundle::BundleBuilder::BundleBuilder(const jsg::Url& bundleBase)
     : ModuleBundle::Builder(Type::BUNDLE),
       bundleBase(bundleBase) {}
 
+namespace {
+static constexpr auto BUNDLE_CLONE_OPTIONS = jsg::Url::EquivalenceOption::IGNORE_FRAGMENTS |
+    jsg::Url::EquivalenceOption::IGNORE_SEARCH | jsg::Url::EquivalenceOption::NORMALIZE_PATH;
+
+// Takes the user-provided module name and normalizes it to a form that can be
+// resolved relative to the bundle base. This involves pre-parsing the name as a URL
+// relative to a dummy base URL in order to normalize out dot and double-dot segments,
+// then stripping off any leading slashes so that the name is always relative and cannot
+// be interpreted as an absolute path.
+jsg::Url normalizeModuleName(kj::StringPtr name, const jsg::Url& base) {
+  // This first step normalizes out path segments like "." and "..", drops query
+  // strings and fragments, and normalizes percent-encoding in the path.
+  auto url = KJ_ASSERT_NONNULL(base.tryResolve(name)).clone(BUNDLE_CLONE_OPTIONS);
+
+  // If the protocol is not file:, then we don't need to do any more processing
+  // here. We will check the validity of the result as a module URL in the next
+  // step.
+  if (url.getProtocol() != "file:"_kj) {
+    return kj::mv(url);
+  }
+
+  auto urlPath = url.getPathname();
+  auto basePath = base.getPathname();
+
+  // The url path must not be identical to the base...
+  KJ_REQUIRE(urlPath != basePath, "Invalid empty module name");
+
+  // If the url path starts with the base path, then we're good!
+  if (urlPath.startsWith(basePath)) {
+    return kj::mv(url);
+  }
+
+  // Otherwise, let's make sure that the url path is processed as
+  // relative to the base path. We do this by stripping off any
+  // leading slashes from the front of the URL then re-resolve
+  // against the base. This should be an exceedingly rare edge
+  // case if the worker bundle is being constructed properly. It's
+  // meant only to handle cases where silliness like "///foo" is
+  // given as a module name.
+  while (urlPath.startsWith("/"_kj) && urlPath.size() > 0) {
+    urlPath = urlPath.slice(1);
+  }
+  KJ_REQUIRE(urlPath.size() > 0, "Invalid empty module name");
+
+  return KJ_ASSERT_NONNULL(base.tryResolve(urlPath));
+}
+
+bool isValidBundleModuleUrl(const jsg::Url& url, const jsg::Url& base) {
+  KJ_DASSERT(base.getProtocol() == "file:"_kj);
+  KJ_DASSERT(base.getPathname().endsWith("/"_kj));
+
+  // Let's forbid users from using cloudflare: and workerd: URLs in bundles so that
+  // we can protect those namespaces for our own future use. Specifically, these
+  // should only be used by the runtime to refer to built-in modules. We don't
+  // restrict other non-standard protocols like node:
+  KJ_REQUIRE(url.getProtocol() != "cloudflare:"_kj,
+      "The cloudflare: protocol is reserved and cannot be used in module bundles");
+  KJ_REQUIRE(url.getProtocol() != "workerd:"_kj,
+      "The workerd: protocol is reserved and cannot be used in module bundles");
+
+  if (url.getProtocol() != "file:"_kj) {
+    // Different protocols are always OK
+    return true;
+  }
+
+  // Module file: URLs must not have a host component.
+  // We already know the protocol is "file:" here because of the check above.
+  if (url.getHost() != ""_kj) {
+    return false;
+  }
+
+  // Check if url is subordinate to the base.
+  // This means url's path should start with base's path as a prefix
+  auto aPath = url.getPathname();
+  auto bPath = base.getPathname();
+
+  return aPath.startsWith(bPath);
+}
+
+// Converts the name given for a user-bundle module into a fully qualified module url.
+// This involves normalizing the name such that it is relative to the bundle base, removes
+// any query parameters or fragments, removes dot and double-dot path segments, normalizes
+// percent-encoding, and otherwise validates that the resulting URL is a valid URL.
+const jsg::Url processModuleName(kj::StringPtr name, const jsg::Url& base) {
+  auto url = normalizeModuleName(name, base);
+  KJ_REQUIRE(isValidBundleModuleUrl(url, base), "Invalid module name: ", name);
+  return url;
+}
+
+}  // namespace
+
 ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addSyntheticModule(
     kj::StringPtr name, EvaluateCallback callback, kj::Array<kj::String> namedExports) {
-  auto url = KJ_ASSERT_NONNULL(bundleBase.tryResolve(name));
-  // Make sure that percent-encoding in the path is normalized so we can match correctly.
-  url = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
+  const auto url = processModuleName(name, bundleBase);
   add(url,
       [url = url.clone(), callback = kj::mv(callback), namedExports = kj::mv(namedExports),
           type = type()](const ResolveContext& context) mutable
@@ -1219,9 +1324,7 @@ ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addSyntheticModule(
 
 ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addEsmModule(
     kj::StringPtr name, kj::ArrayPtr<const char> source, Module::Flags flags) {
-  auto url = KJ_ASSERT_NONNULL(bundleBase.tryResolve(name));
-  // Make sure that percent-encoding in the path is normalized so we can match correctly.
-  url = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
+  const auto url = processModuleName(name, bundleBase);
   add(url,
       [url = url.clone(), source, flags, type = type()](const ResolveContext& context) mutable
       -> kj::Maybe<kj::OneOf<kj::String, kj::Own<Module>>> {
@@ -1233,10 +1336,8 @@ ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addEsmModule(
 
 ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addWasmModule(
     kj::StringPtr name, kj::ArrayPtr<const kj::byte> data) {
+  const auto url = processModuleName(name, bundleBase);
   auto callback = jsg::modules::Module::newWasmModuleHandler(data);
-  auto url = KJ_ASSERT_NONNULL(bundleBase.tryResolve(name));
-  // Make sure that percent-encoding in the path is normalized so we can match correctly.
-  url = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
   add(url,
       [url = url.clone(), callback = kj::mv(callback), type = type()](
           const ResolveContext& context) mutable
@@ -1250,8 +1351,8 @@ ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::addWasmModule(
 
 ModuleBundle::BundleBuilder& ModuleBundle::BundleBuilder::alias(
     kj::StringPtr alias, kj::StringPtr name) {
-  auto aliasUrl = KJ_ASSERT_NONNULL(bundleBase.tryResolve(alias));
-  auto id = KJ_ASSERT_NONNULL(bundleBase.tryResolve(name));
+  const auto id = processModuleName(name, bundleBase);
+  const auto aliasUrl = processModuleName(alias, bundleBase);
   Builder::alias(aliasUrl, id);
   return *this;
 }
diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++
@@ -4255,8 +4255,19 @@ kj::Promise<kj::Own<Server::WorkerService>> Server::makeWorkerImpl(kj::StringPtr
   // config. So for now this just uses the defaults.
   auto workerFs = newWorkerFileSystem(kj::heap<FsMap>(), getBundleDirectory(def.source));
 
+  // TODO(soon): Either make python workers support the new module registry before
+  // NMR is defaulted on, or disable NMR by default when python workers are enabled.
+  // While NMR is experimental, we'll just throw an error if both are enabled.
+  if (def.featureFlags.getPythonWorkers()) {
+    KJ_REQUIRE(!def.featureFlags.getNewModuleRegistry(),
+        "Python workers do not currently support the new ModuleRegistry implementation. "
+        "Please disable the new ModuleRegistry feature flag to use Python workers.");
+  }
+
+  bool usingNewModuleRegistry = def.featureFlags.getNewModuleRegistry();
   kj::Maybe<kj::Arc<jsg::modules::ModuleRegistry>> newModuleRegistry;
-  if (def.featureFlags.getNewModuleRegistry()) {
+  // TODO(soon): Python workers do not currently support the new module registry.
+  if (usingNewModuleRegistry) {
     KJ_REQUIRE(experimental,
         "The new ModuleRegistry implementation is an experimental feature. "
         "You must run workerd with `--experimental` to use this feature.");
@@ -4297,8 +4308,8 @@ kj::Promise<kj::Own<Server::WorkerService>> Server::makeWorkerImpl(kj::StringPtr
     inspectorPolicy = Worker::Isolate::InspectorPolicy::ALLOW_FULLY_TRUSTED;
   }
   Worker::LoggingOptions isolateLoggingOptions = loggingOptions;
-  isolateLoggingOptions.consoleMode = def.source.variant.is<WorkerSource::ScriptSource>() &&
-          !def.featureFlags.getNewModuleRegistry()
+  isolateLoggingOptions.consoleMode =
+      def.source.variant.is<WorkerSource::ScriptSource>() && !usingNewModuleRegistry
       ? Worker::ConsoleMode::INSPECTOR_ONLY
       : loggingOptions.consoleMode;
   auto isolate = kj::atomicRefcounted<Worker::Isolate>(kj::mv(api), kj::mv(observer), name,
@@ -4310,7 +4321,7 @@ kj::Promise<kj::Own<Server::WorkerService>> Server::makeWorkerImpl(kj::StringPtr
     isolateRegistrar->registerIsolate(name, isolate.get());
   }
 
-  if (!def.featureFlags.getNewModuleRegistry()) {
+  if (!usingNewModuleRegistry) {
     KJ_IF_SOME(moduleFallback, def.moduleFallback) {
       KJ_REQUIRE(experimental,
           "The module fallback service is an experimental feature. "
diff --git a/src/workerd/server/workerd-api.c++ b/src/workerd/server/workerd-api.c++
