fix(compartment-mapper): Defer all importHook errors when importing a… (

#2610) … dependency if they're intended to be deferred ## Description When importHook is used to load dependencies, they might be coming from `imports` that were identified by a parser that uses heuristics to find them. (e.g. cjs parser implementation uses a lexer to find all `require` calls even if they're not _the_ require function but a local require function) In such cases we want to defer loading errors to runtime, so the error doesn't happen until actual `require` is _actually_ called to get it. That was true for loading known dependency, but exit modules (via exitModuleImportHook) were not covered. ### Security Considerations no changes security-wise ### Scaling Considerations The way this is currently implemented - one big try-catch instead of returning the deferred error without throwing and catching, might be less performant. Should we consider trading aesthetics for a tiny performance gain here? Note this only makes a difference for cases where non-dynamic `require` is called on a specifier that causes an error. ### Documentation Considerations It's a fix of unintended behavior. No changes to what needs to be documented. ### Testing Considerations Fix was added test-first ### Compatibility Considerations Compatibility is improved. Packages can now do: ```js try { require('my-dev-dependency') } catch(){} ``` to silently attempt to load something that is only useful in their own development process. (typescript does that) ### Upgrade Considerations While it's a fix, not a breaking change, it does change one specific behavior - an error resulting from calling exitModuleImportHook (the `importHook` passed to compartment-mapper, not the one in Compartment options) is now thrown at runtime, not during loading, which could be noteworthy for the Archive use-case. [(insert mandatory xkcd about breaking changes)](https://xkcd.com/1172/) --------- Co-authored-by: Christopher Hiller <[email protected]>
endojs · Nov 5, 2024 · 28999e8 · 28999e8
1 parent a0f9253
commit 28999e8
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 67 deletions.
diff --git a/packages/compartment-mapper/NEWS.md b/packages/compartment-mapper/NEWS.md
@@ -4,6 +4,11 @@ User-visible changes to `@endo/compartment-mapper`:
 
 - Omits unused module descriptors from `compartment-map.json` in archived
   applications, potentially reducing file sizes.
+- Fixes an issue where errors thrown from exit module hooks (`importHook`) would
+  be thrown at parse-time when the parser uses heuristic import analysis
+  _instead of_ at runtime. Such errors will now be thrown at runtime, as
+  originally intended. To those who expected the previous behavior: if you
+  exist, please exercise caution when upgrading.
 
 # v1.3.0 (2024-10-10)
 

diff --git a/packages/compartment-mapper/src/import-hook.js b/packages/compartment-mapper/src/import-hook.js
@@ -504,89 +504,88 @@ export const makeImportHookMaker = (
       // for lint rule
       await null;
 
-      // per-module:
-
-      // In Node.js, an absolute specifier always indicates a built-in or
-      // third-party dependency.
-      // The `moduleMapHook` captures all third-party dependencies, unless
-      // we allow importing any exit.
-      if (moduleSpecifier !== '.' && !moduleSpecifier.startsWith('./')) {
-        if (exitModuleImportHook) {
-          const record = await exitModuleImportHook(moduleSpecifier);
-          if (record) {
-            // It'd be nice to check the policy before importing it, but we can only throw a policy error if the
-            // hook returns something. Otherwise, we need to fall back to the 'cannot find' error below.
-            enforceModulePolicy(moduleSpecifier, compartmentDescriptor, {
-              exit: true,
-              errorHint: `Blocked in loading. ${q(
+      // All importHook errors must be deferred if coming from loading dependencies
+      // identified by a parser that discovers imports heuristically.
+      try {
+        // per-module:
+
+        // In Node.js, an absolute specifier always indicates a built-in or
+        // third-party dependency.
+        // The `moduleMapHook` captures all third-party dependencies, unless
+        // we allow importing any exit.
+        if (moduleSpecifier !== '.' && !moduleSpecifier.startsWith('./')) {
+          if (exitModuleImportHook) {
+            const record = await exitModuleImportHook(moduleSpecifier);
+            if (record) {
+              // It'd be nice to check the policy before importing it, but we can only throw a policy error if the
+              // hook returns something. Otherwise, we need to fall back to the 'cannot find' error below.
+              enforceModulePolicy(moduleSpecifier, compartmentDescriptor, {
+                exit: true,
+                errorHint: `Blocked in loading. ${q(
+                  moduleSpecifier,
+                )} was not in the compartment map and an attempt was made to load it as a builtin`,
+              });
+              if (archiveOnly) {
+                // Return a place-holder.
+                // Archived compartments are not executed.
+                return freeze({ imports: [], exports: [], execute() {} });
+              }
+              // note it's not being marked as exit in sources
+              // it could get marked and the second pass, when the archive is being executed, would have the data
+              // to enforce which exits can be dynamically imported
+              const attenuatedRecord = await attenuateModuleHook(
                 moduleSpecifier,
-              )} was not in the compartment map and an attempt was made to load it as a builtin`,
-            });
-            if (archiveOnly) {
-              // Return a place-holder.
-              // Archived compartments are not executed.
-              return freeze({ imports: [], exports: [], execute() {} });
+                record,
+                compartmentDescriptor.policy,
+                attenuators,
+              );
+              return attenuatedRecord;
             }
-            // note it's not being marked as exit in sources
-            // it could get marked and the second pass, when the archive is being executed, would have the data
-            // to enforce which exits can be dynamically imported
-            const attenuatedRecord = await attenuateModuleHook(
-              moduleSpecifier,
-              record,
-              compartmentDescriptor.policy,
-              attenuators,
-            );
-            return attenuatedRecord;
           }
-        }
-        return deferError(
-          moduleSpecifier,
-          Error(
+          throw Error(
             `Cannot find external module ${q(
               moduleSpecifier,
             )} in package ${packageLocation}`,
-          ),
-        );
-      }
+          );
+        }
 
-      const { maybeRead } = unpackReadPowers(readPowers);
+        const { maybeRead } = unpackReadPowers(readPowers);
 
-      const candidates = nominateCandidates(moduleSpecifier, searchSuffixes);
+        const candidates = nominateCandidates(moduleSpecifier, searchSuffixes);
 
-      const record = await asyncTrampoline(
-        chooseModuleDescriptor,
-        {
-          candidates,
-          compartmentDescriptor,
-          compartmentDescriptors,
-          compartments,
-          computeSha512,
-          moduleDescriptors,
-          moduleSpecifier,
-          packageLocation,
-          packageSources,
-          readPowers,
-          sourceMapHook,
-          strictlyRequiredForCompartment,
-        },
-        { maybeRead, parse, shouldDeferError },
-      );
+        const record = await asyncTrampoline(
+          chooseModuleDescriptor,
+          {
+            candidates,
+            compartmentDescriptor,
+            compartmentDescriptors,
+            compartments,
+            computeSha512,
+            moduleDescriptors,
+            moduleSpecifier,
+            packageLocation,
+            packageSources,
+            readPowers,
+            sourceMapHook,
+            strictlyRequiredForCompartment,
+          },
+          { maybeRead, parse, shouldDeferError },
+        );
 
-      if (record) {
-        return record;
-      }
+        if (record) {
+          return record;
+        }
 
-      return deferError(
-        moduleSpecifier,
-        // TODO offer breadcrumbs in the error message, or how to construct breadcrumbs with another tool.
-        Error(
+        throw Error(
           `Cannot find file for internal module ${q(
             moduleSpecifier,
           )} (with candidates ${candidates
             .map(x => q(x))
             .join(', ')}) in package ${packageLocation}`,
-        ),
-      );
+        );
+      } catch (error) {
+        return deferError(moduleSpecifier, error);
+      }
     };
     return importHook;
   };

diff --git a/packages/compartment-mapper/test/cjs-compat.test.js b/packages/compartment-mapper/test/cjs-compat.test.js
@@ -15,6 +15,8 @@ const fixtureDirname = new URL(
   import.meta.url,
 ).toString();
 
+const q = JSON.stringify;
+
 const assertFixture = (t, { namespace, testCategoryHint }) => {
   const { assertions, results } = namespace;
 
@@ -66,6 +68,22 @@ scaffold(
   fixtureAssertionCount,
 );
 
+// Exit module errors are also deferred
+scaffold(
+  'fixtures-cjs-compat-exit-module',
+  test,
+  fixture,
+  assertFixture,
+  fixtureAssertionCount,
+  {
+    additionalOptions: {
+      importHook: async specifier => {
+        throw Error(`${q(specifier)} is NOT an exit module.`);
+      },
+    },
+  },
+);
+
 scaffold(
   'fixtures-cjs-compat-__dirname',
   test,