fix(compartment-mapper): disallow dynamic requires of arbitrary, unkn…

…own paths

This changes the logic so that this particular class of dynamic requires is disallowed.

If the specifier is not accessible per policy, the usual policy-enforcement error message applies.  If the specifier is not accessible because it's nowhere in the exit module, the error will mention that it can't load an unknown module.

Also fixes:

- dynamic requires of exit modules (e.g., Node.js builtins)
- dynamic requires made from the entry compartment descriptor
- some peer review comments

packages/compartment-mapper/src/import-hook.js

@@ -7,27 +7,33 @@
  */
 
 // @ts-check
-
-/** @import {ImportHook} from 'ses' */
-/** @import {ImportNowHook} from 'ses' */
-/** @import {StaticModuleType} from 'ses' */
-/** @import {RedirectStaticModuleInterface} from 'ses' */
-/** @import {CreateStaticModuleTypeOperators} from './types.js' */
-/** @import {CreateStaticModuleTypeOptions} from './types.js' */
-/** @import {CreateStaticModuleTypeYieldables} from './types.js' */
-/** @import {ParseResult} from './types.js' */
-/** @import {MakeImportNowHookMakerOptions} from './types.js' */
-/** @import {ModuleDescriptor} from './types.js' */
-/** @import {ReadFn} from './types.js' */
-/** @import {SyncReadPowers} from './types.js' */
-/** @import {ReadPowers} from './types.js' */
-/** @import {HashFn} from './types.js' */
-/** @import {Sources} from './types.js' */
-/** @import {CompartmentDescriptor} from './types.js' */
-/** @import {ImportHookMaker} from './types.js' */
-/** @import {ExitModuleImportHook} from './types.js' */
-/** @import {SourceMapHook} from './types.js' */
-/** @import {ImportNowHookMaker} from './types.js' */
+/**
+ * @import {
+ *   ImportHook,
+ *   ImportNowHook,
+ *   RedirectStaticModuleInterface,
+ *   StaticModuleType
+ * } from 'ses'
+ * @import {
+ *   CompartmentDescriptor,
+ *   CreateStaticModuleTypeOperators,
+ *   CreateStaticModuleTypeOptions,
+ *   CreateStaticModuleTypeYieldables,
+ *   ExitModuleImportHook,
+ *   FindRedirectParams,
+ *   HashFn,
+ *   ImportHookMaker,
+ *   ImportNowHookMaker,
+ *   MakeImportNowHookMakerOptions,
+ *   ModuleDescriptor,
+ *   ParseResult,
+ *   ReadFn,
+ *   ReadPowers,
+ *   SourceMapHook,
+ *   Sources,
+ *   SyncReadPowers
+ * } from './types.js'
+ */
 
 import { asyncTrampoline, syncTrampoline } from '@endo/trampoline';
 import { resolve } from './node-module-specifier.js';
@@ -85,6 +91,63 @@ const nodejsConventionSearchSuffixes = [
   '/index.node',
 ];
 
+/**
+ * Given a module specifier which is an absolute path, attempt to match it with
+ * an existing compartment; return a {@link RedirectStaticModuleInterface} if found.
+ *
+ * @throws If we determine `absoluteModuleSpecifier` is unknown
+ * @param {FindRedirectParams} params Parameters
+ * @returns {RedirectStaticModuleInterface|undefined} A redirect or nothing
+ */
+const findRedirect = ({
+  compartmentDescriptor,
+  compartmentDescriptors,
+  compartments,
+  absoluteModuleSpecifier,
+  packageLocation,
+}) => {
+  let specifierIsArbitrary = true;
+  for (const [someDescriptorName, someDescriptor] of entries(
+    compartmentDescriptors,
+  )) {
+    if (someDescriptorName !== ATTENUATORS_COMPARTMENT) {
+      const moduleSpecifierUrl = `${new URL(absoluteModuleSpecifier, packageLocation)}`;
+      if (!compartmentDescriptor.location.startsWith(moduleSpecifierUrl)) {
+        if (moduleSpecifierUrl.startsWith(someDescriptor.location)) {
+          // compartmentDescriptor is a dependency of someDescriptor; we
+          // can use that compartment
+          if (someDescriptor.name in compartmentDescriptor.modules) {
+            return {
+              specifier: absoluteModuleSpecifier,
+              compartment: compartments[someDescriptorName],
+            };
+          }
+          // compartmentDescriptor is a dependent of someSomeDescriptor;
+          // this is more dubious but a common pattern
+          if (compartmentDescriptor.name in someDescriptor.modules) {
+            return {
+              specifier: absoluteModuleSpecifier,
+              compartment: compartments[someDescriptorName],
+            };
+          }
+          if (compartmentDescriptor === someDescriptor) {
+            // this compartmentDescriptor wants to dynamically load its own module
+            // using an absolute path
+            specifierIsArbitrary = false;
+          }
+        }
+      }
+    }
+  }
+
+  if (specifierIsArbitrary) {
+    throw new Error(
+      `Could not import unknown module: ${q(absoluteModuleSpecifier)}`,
+    );
+  }
+  return undefined;
+};
+
 /**
  * @param {object} params
  * @param {Record<string, any>=} params.modules
@@ -542,18 +605,11 @@ export function makeImportNowHookMaker(
   const makeImportNowHook = ({
     packageLocation,
     packageName: _packageName,
-    entryCompartmentDescriptor,
     parse,
     compartments,
   }) => {
     const compartmentDescriptor = compartmentDescriptors[packageLocation] || {};
 
-    // this is necessary, because there's nothing that prevents someone from calling this function with the entry compartment.
-    assert(
-      compartmentDescriptor !== entryCompartmentDescriptor,
-      'Cannot create an ImportNowHook for the entry compartment',
-    );
-
     // this is not strictly necessary, since the types should handle it.
     assert(
       parse[Symbol.toStringTag] !== 'AsyncFunction',
@@ -594,50 +650,15 @@ export function makeImportNowHookMaker(
     /** @type {ImportNowHook} */
     const importNowHook = moduleSpecifier => {
       if (isAbsolute(moduleSpecifier)) {
-        let shouldCallExitModuleImportNowHook = true;
-        for (const [someDescriptorName, someDescriptor] of entries(
+        const record = findRedirect({
+          compartmentDescriptor,
           compartmentDescriptors,
-        )) {
-          if (someDescriptorName !== ATTENUATORS_COMPARTMENT) {
-            const moduleSpecifierUrl = resolveLocation(
-              moduleSpecifier,
-              packageLocation,
-            );
-            if (
-              !compartmentDescriptor.location.startsWith(moduleSpecifierUrl)
-            ) {
-              if (moduleSpecifierUrl.startsWith(someDescriptor.location)) {
-                if (compartmentDescriptor.name in someDescriptor.modules) {
-                  return {
-                    specifier: moduleSpecifier,
-                    compartment: compartments[someDescriptorName],
-                  };
-                } else if (compartmentDescriptor === someDescriptor) {
-                  shouldCallExitModuleImportNowHook = false;
-                }
-              }
-            }
-          }
-        }
-        if (shouldCallExitModuleImportNowHook) {
-          if (exitModuleImportNowHook) {
-            // this hook is responsible for ensuring that the moduleSpecifier actually refers to an exit module
-            const record = exitModuleImportNowHook(
-              moduleSpecifier,
-              packageLocation,
-            );
-
-            if (!record) {
-              throw new Error(`Could not import module: ${q(moduleSpecifier)}`);
-            }
-
-            return record;
-          }
-          throw new Error(
-            `Could not import module: ${q(
-              moduleSpecifier,
-            )}; try providing an importNowHook`,
-          );
+          compartments,
+          absoluteModuleSpecifier: moduleSpecifier,
+          packageLocation,
+        });
+        if (record) {
+          return record;
         }
       }
 

packages/compartment-mapper/src/link.js

@@ -8,22 +8,27 @@
 
 // @ts-check
 
-/** @import {ImportNowHook} from 'ses' */
-/** @import {ModuleMapHook} from 'ses' */
-/** @import {ImportNowHookMaker, ModuleTransform, ParseFnAsync, SyncModuleTransform} from './types.js' */
-/** @import {LinkResult} from './types.js' */
-/** @import {ParseFn} from './types.js' */
-/** @import {ParserForLanguage} from './types.js' */
-/** @import {SyncLinkOptions} from './types.js' */
-/** @import {SyncModuleTransforms} from './types.js' */
-/** @import {ParserImplementation} from './types.js' */
-/** @import {ShouldDeferError} from './types.js' */
-/** @import {ModuleTransforms} from './types.js' */
-/** @import {LanguageForExtension} from './types.js' */
-/** @import {ModuleDescriptor} from './types.js' */
-/** @import {CompartmentDescriptor} from './types.js' */
-/** @import {CompartmentMapDescriptor} from './types.js' */
-/** @import {LinkOptions} from './types.js' */
+/** @import {ImportNowHook, ModuleMapHook} from 'ses' */
+/**
+ * @import {
+ *   CompartmentDescriptor,
+ *   CompartmentMapDescriptor,
+ *   ImportNowHookMaker,
+ *   ImportNowHookMakerParams,
+ *   LanguageForExtension,
+ *   LinkOptions,
+ *   LinkResult,
+ *   ModuleDescriptor,
+ *   ModuleTransforms,
+ *   ParseFn,
+ *   ParseFnAsync,
+ *   ParserForLanguage,
+ *   ParserImplementation,
+ *   ShouldDeferError,
+ *   SyncLinkOptions,
+ *   SyncModuleTransforms
+ * } from './types.js'
+ */
 /** @import {ERef} from '@endo/eventual-send' */
 
 import { mapParsers } from './map-parser.js';
@@ -297,23 +302,26 @@ export const link = (
     Compartment = defaultCompartment,
   } = options;
 
-  const isSync = isSyncOptions(options);
-  const syncModuleTransforms = isSync
-    ? options.syncModuleTransforms
-    : undefined;
-  const moduleTransforms = isSync
-    ? undefined
-    : /** @type {ModuleTransforms|undefined} */ ({
-        ...options.syncModuleTransforms,
-        ...options.moduleTransforms,
-      });
-
-  const makeImportNowHook = isSync ? options.makeImportNowHook : undefined;
-  ``;
+  /** @type {SyncModuleTransforms|undefined} */
+  let syncModuleTransforms;
+  /** @type {ModuleTransforms|undefined} */
+  let moduleTransforms;
+  /** @type {ImportNowHookMaker|undefined} */
+  let makeImportNowHook;
+
+  if (isSyncOptions(options)) {
+    syncModuleTransforms = options.syncModuleTransforms;
+    makeImportNowHook = options.makeImportNowHook;
+  } else {
+    // we can fold syncModuleTransforms into moduleTransforms because
+    // async supports sync, but not vice-versa
+    moduleTransforms = /** @type {ModuleTransforms|undefined} */ ({
+      ...options.syncModuleTransforms,
+      ...options.moduleTransforms,
+    });
+  }
 
   const { compartment: entryCompartmentName } = entry;
-  const entryCompartmentDescriptor =
-    compartmentDescriptors[entryCompartmentName];
 
   /** @type {Record<string, Compartment>} */
   const compartments = create(null);
@@ -408,24 +416,21 @@ export const link = (
     /** @type {ImportNowHook | undefined} */
     let importNowHook;
 
-    if (isSync) {
-      if (entryCompartmentDescriptor !== compartmentDescriptor) {
-        const syncParse = /** @type {ParseFn} */ (parse);
-        importNowHook = /** @type {ImportNowHookMaker} */ (makeImportNowHook)({
-          entryCompartmentDescriptor,
-          packageLocation: location,
-          packageName: name,
-          parse: syncParse,
-          compartments,
-        });
-      } else {
-        // should never happen
-        importNowHook = () => {
-          throw new Error(
-            'importNowHook should not be called by entry compartment',
-          );
-        };
-      }
+    if (makeImportNowHook) {
+      // Note: using the LHS of this statement as the value for `params.parse`
+      // below causes a `no-object-shorthand` error. afaict there is no such
+      // configuration for this rule which would allow the below type assertion
+      // to be used.
+      const syncParse = /** @type {ParseFn} */ (parse);
+
+      /** @type {ImportNowHookMakerParams} */
+      const params = {
+        packageLocation: location,
+        packageName: name,
+        parse: syncParse,
+        compartments,
+      };
+      importNowHook = makeImportNowHook(params);
     } else {
       importNowHook = () => {
         throw new Error('Provided read powers do not support dynamic requires');

packages/compartment-mapper/src/policy.js

@@ -3,14 +3,15 @@
 
 // @ts-check
 
-/** @import {Policy} from './types.js' */
+/** @import {FindRedirectParams, Policy} from './types.js' */
 /** @import {PackagePolicy} from './types.js' */
 /** @import {AttenuationDefinition} from './types.js' */
 /** @import {PackageNamingKit} from './types.js' */
 /** @import {DeferredAttenuatorsProvider} from './types.js' */
 /** @import {CompartmentDescriptor} from './types.js' */
 /** @import {Attenuator} from './types.js' */
 /** @import {SomePolicy} from './types.js' */
+/** @import {RedirectStaticModuleInterface} from 'ses' */
 
 import {
   getAttenuatorFromDefinition,

packages/compartment-mapper/src/types.js

@@ -299,7 +299,6 @@ export {};
 
 /**
  * @typedef {object} ImportNowHookMakerParams
- * @property {CompartmentDescriptor} entryCompartmentDescriptor
  * @property {string} packageLocation
  * @property {string} packageName
  * @property {ParseFn} parse
@@ -885,10 +884,9 @@ export {};
  * @property {CompartmentSources} packageSources Sources
  * @property {ReadPowers|ReadFn} readPowers Powers
  * @property {SourceMapHook} [sourceMapHook] Source map hook
- * @property {(compartmentName: string) => Set<string>}
- * strictlyRequiredForCompartment Function returning a set of module names
- * (scoped to the compartment) whose parser is not using heuristics to determine
- * imports.
+ * @property {(compartmentName: string) => Set<string>} strictlyRequiredForCompartment Function
+ *   returning a set of module names (scoped to the compartment) whose parser is not using
+ *   heuristics to determine imports.
  */
 
 /**
@@ -932,3 +930,14 @@ export {};
  * ReturnType<CreateStaticModuleTypeOperators['parse']>}
  * CreateStaticModuleTypeYieldables
  */
+
+/**
+ * Parameters for `findRedirect()`.
+ *
+ * @typedef FindRedirectParams
+ * @property {CompartmentDescriptor} compartmentDescriptor
+ * @property {Record<string, CompartmentDescriptor>} compartmentDescriptors
+ * @property {Record<string, Compartment>} compartments
+ * @property {string} absoluteModuleSpecifier A module specifier which is an absolute path
+ * @property {string} packageLocation
+ */