fix(compartment-mapper): Fix mismatch between specifier and resource …

…identifier in attenuators policy AND allow skipping powerless packages (merge #1838)

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

@@ -95,6 +95,9 @@ const makeArchiveImportHookMaker = (
           // module is a "builtin" module and the policy needs to be enforced.
           enforceModulePolicy(moduleSpecifier, compartmentDescriptor, {
             exit: true,
+            errorHint: `Blocked in loading. ${q(
+              moduleSpecifier,
+            )} was not in the archive and an attempt was made to load it as a builtin`,
           });
           const record = await exitModuleImportHook(moduleSpecifier);
           if (record) {

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

@@ -220,6 +220,9 @@ export const makeImportHookMaker = (
             // 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.

packages/compartment-mapper/src/link.js

@@ -19,7 +19,6 @@ import { parseExtension } from './extension.js';
 import {
   enforceModulePolicy,
   ATTENUATORS_COMPARTMENT,
-  diagnoseMissingCompartmentError,
   attenuateGlobals,
   makeDeferredAttenuatorsProvider,
 } from './policy.js';
@@ -232,10 +231,14 @@ const makeModuleMapHook = (
         return undefined; // fall through to import hook
       }
       if (foreignModuleSpecifier !== undefined) {
+        // archive goes through foreignModuleSpecifier for local modules too
         if (!moduleSpecifier.startsWith('./')) {
-          // archive goes through foreignModuleSpecifier for local modules too
+          // This code path seems to only be reached on subsequent imports of the same specifier in the same compartment.
+          // The check should be redundant and is only left here out of abundance of caution.
           enforceModulePolicy(moduleSpecifier, compartmentDescriptor, {
             exit: false,
+            errorHint:
+              'This check should not be reachable. If you see this error, please file an issue.',
           });
         }
 
@@ -244,12 +247,7 @@ const makeModuleMapHook = (
           throw Error(
             `Cannot import from missing compartment ${q(
               foreignCompartmentName,
-            )}${diagnoseMissingCompartmentError({
-              moduleSpecifier,
-              compartmentDescriptor,
-              foreignModuleSpecifier,
-              foreignCompartmentName,
-            })}`,
+            )}}`,
           );
         }
         return foreignCompartment.module(foreignModuleSpecifier);
@@ -279,20 +277,17 @@ const makeModuleMapHook = (
           throw Error(
             `Cannot import from missing compartment ${q(
               foreignCompartmentName,
-            )}${diagnoseMissingCompartmentError({
-              moduleSpecifier,
-              compartmentDescriptor,
-              foreignModuleSpecifier,
-              foreignCompartmentName,
-            })}`,
+            )}`,
           );
         }
 
-        // Despite all non-exit modules not allowed by policy being dropped
-        // while building the graph, this check is necessary because module
-        // is written back to the compartment map below.
         enforceModulePolicy(scopePrefix, compartmentDescriptor, {
           exit: false,
+          errorHint: `Blocked in linking. ${q(
+            moduleSpecifier,
+          )} is part of the compartment map and resolves to ${q(
+            foreignCompartmentName,
+          )}.`,
         });
         // The following line is weird.
         // Information is flowing backward.

packages/compartment-mapper/src/node-modules.js

@@ -566,7 +566,7 @@ const graphPackages = async (
  * @param {Graph} graph
  * @param {Set<string>} tags - build tags about the target environment
  * for selecting relevant exports, e.g., "browser" or "node".
- * @param {object} policy
+ * @param {object|undefined} policy
  * @returns {CompartmentMapDescriptor}
  */
 const translateGraph = (
@@ -611,11 +611,6 @@ const translateGraph = (
       },
       policy,
     );
-    // do not include compartments for packages not covered by policy
-    if (policy && !packagePolicy) {
-      // eslint-disable-next-line no-continue
-      continue;
-    }
 
     /**
      * @param {string} dependencyName

packages/compartment-mapper/src/policy-format.js

@@ -122,7 +122,7 @@ export const assertPackagePolicy = (allegedPackagePolicy, path, url) => {
   const packagePolicy = Object(allegedPackagePolicy);
   assert(
     allegedPackagePolicy === packagePolicy && !isArray(allegedPackagePolicy),
-    `${path} must be an object, got ${allegedPackagePolicy}${inUrl}`,
+    `${path} must be an object, got ${q(allegedPackagePolicy)}${inUrl}`,
   );
   const {
     packages,

packages/compartment-mapper/src/policy.js

@@ -17,7 +17,7 @@ import {
   isAllowingEverything,
 } from './policy-format.js';
 
-const { entries, values, assign, keys, freeze } = Object;
+const { create, entries, values, assign, keys, freeze } = Object;
 const q = JSON.stringify;
 
 /**
@@ -113,29 +113,6 @@ export const dependencyAllowedByPolicy = (namingKit, packagePolicy) => {
   return !!policyLookupHelper(packagePolicy, 'packages', canonicalName);
 };
 
-const validateDependencies = (policy, canonicalName) => {
-  const packages = policy.resources[canonicalName].packages;
-  if (!packages || isAllowingEverything(packages)) {
-    return;
-  }
-
-  const packageNames = keys(packages);
-  const attenuators = detectAttenuators(policy);
-  // Join attenuators with packageNames into a Set to deduplicate and check if all are listed in policy.resources
-  const allSpecifiers = new Set([...packageNames, ...attenuators]);
-  for (const specifier of allSpecifiers) {
-    if (!(specifier in policy.resources)) {
-      throw Error(
-        `Package ${q(specifier)} is allowed for ${q(
-          canonicalName,
-        )} to import but its policy is not defined. Please add a policy for ${q(
-          specifier,
-        )}`,
-      );
-    }
-  }
-};
-
 /**
  * Returns the policy applicable to the canonicalName of the package
  *
@@ -154,20 +131,14 @@ export const getPolicyForPackage = (namingKit, policy) => {
   if (canonicalName === ATTENUATORS_COMPARTMENT) {
     return {
       defaultAttenuator: policy.defaultAttenuator,
-      packages: detectAttenuators(policy).reduce((packages, specifier) => {
-        packages[specifier] = true;
-        return packages;
-      }, {}),
+      packages: 'any',
     };
   }
-  if (policy.resources && policy.resources[canonicalName]) {
-    validateDependencies(policy, canonicalName);
+  if (policy.resources && policy.resources[canonicalName] !== undefined) {
     return policy.resources[canonicalName];
   } else {
-    console.warn(
-      `No policy for '${canonicalName}', omitting from compartment map.`,
-    );
-    return undefined;
+    // Allow skipping policy entries for packages with no powers.
+    return create(null);
   }
 };
 
@@ -357,25 +328,37 @@ export const attenuateGlobals = (
   freezeGlobalThisUnlessOptedOut();
 };
 
+const diagnoseModulePolicy = errorHint => {
+  if (!errorHint) {
+    return '';
+  }
+  return ` (info: ${errorHint})`;
+};
 /**
  * Throws if importing of the specifier is not allowed by the policy
  *
  * @param {string} specifier
- * @param {object} compartmentDescriptor
+ * @param {import('./types.js').CompartmentDescriptor} compartmentDescriptor
  * @param {object} [info]
  */
-export const enforceModulePolicy = (specifier, compartmentDescriptor, info) => {
-  const { policy, modules } = compartmentDescriptor;
+export const enforceModulePolicy = (
+  specifier,
+  compartmentDescriptor,
+  info = {},
+) => {
+  const { policy, modules, label } = compartmentDescriptor;
   if (!policy) {
     return;
   }
 
   if (!info.exit) {
     if (!modules[specifier]) {
       throw Error(
-        `Importing ${q(specifier)} was not allowed by policy packages:${q(
+        `Importing ${q(specifier)} in ${q(
+          label,
+        )} was not allowed by packages policy ${q(
           policy.packages,
-        )}`,
+        )}${diagnoseModulePolicy(info.errorHint)}`,
       );
     }
     return;
@@ -385,7 +368,7 @@ export const enforceModulePolicy = (specifier, compartmentDescriptor, info) => {
     throw Error(
       `Importing ${q(specifier)} was not allowed by policy builtins:${q(
         policy.builtins,
-      )}`,
+      )}${diagnoseModulePolicy(info.errorHint)}`,
     );
   }
 };
@@ -459,61 +442,3 @@ export const attenuateModuleHook = async (
     originalModuleRecord,
   });
 };
-
-const padDiagnosis = text => ` (${text})`;
-/**
- * Provide dignostic information for a missing compartment error
- *
- * @param {object}  args
- * @param {string}  args.moduleSpecifier
- * @param {object}  args.compartmentDescriptor
- * @param {string}  args.foreignModuleSpecifier
- * @param {string}  args.foreignCompartmentName
- * @returns {string}
- */
-export const diagnoseMissingCompartmentError = ({
-  moduleSpecifier,
-  compartmentDescriptor,
-  foreignModuleSpecifier,
-  foreignCompartmentName,
-}) => {
-  const { policy, name, scopes } = compartmentDescriptor;
-
-  if (policy) {
-    if (!policy.packages) {
-      return padDiagnosis(
-        `There were no allowed packages specified in policy for ${q(name)}`,
-      );
-    }
-    if (name === ATTENUATORS_COMPARTMENT) {
-      return padDiagnosis(
-        `Attenuator ${q(
-          moduleSpecifier,
-        )} was imported but there is no policy resources entry defined for it.`,
-      );
-    }
-
-    const scopeNames = entries(scopes)
-      .filter(([_name, scope]) => scope.compartment === foreignCompartmentName)
-      .map(([scopeName]) => scopeName);
-    if (scopeNames.length === 1 && scopeNames[0] === moduleSpecifier) {
-      return padDiagnosis(
-        `Package ${q(
-          moduleSpecifier,
-        )} is missing. Are you sure there is an entry in policy resources specified for it?`,
-      );
-    } else {
-      return padDiagnosis(
-        `Package ${q(moduleSpecifier)} resolves to ${q(
-          foreignModuleSpecifier,
-        )} in ${q(
-          foreignCompartmentName,
-        )} which seems disallowed by policy. There is likely an override defined that causes another package to be imported as ${q(
-          moduleSpecifier,
-        )}.`,
-      );
-    }
-  }
-  // Omit diagnostics when parent package had no policy - it means there was no policy.
-  return '';
-};