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 <boneskull@boneskull.com>

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)
 

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;
   };

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,