fix(compartment-mapper): correct error interpretations, negative poli…

…cy enforcement test

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

@@ -95,7 +95,9 @@ const makeArchiveImportHookMaker = (
           // module is a "builtin" module and the policy needs to be enforced.
           enforceModulePolicy(moduleSpecifier, compartmentDescriptor, {
             exit: true,
-            errorHint: 'The module was not in the archive and an attempt was made to load it as a builtin',
+            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,7 +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: 'The module was not in the compartment map and an attempt was made to load it as a builtin',
+              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

@@ -231,11 +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: `TODO`, // TODO: add a testcase for this and provide a helpful hint
+            errorHint:
+              'This check should not be reachable. If you see this error, please file an issue.',
           });
         }
 
@@ -280,11 +283,9 @@ const makeModuleMapHook = (
 
         enforceModulePolicy(scopePrefix, compartmentDescriptor, {
           exit: false,
-          errorHint: ` There is an alias defined that causes another package to be imported as ${q(
+          errorHint: `Blocked in linking. ${q(
             moduleSpecifier,
-          )}. Package ${q(moduleSpecifier)} resolves to ${q(
-            foreignModuleSpecifier,
-          )} in ${q(
+          )} is part of the compartment map and resolves to ${q(
             foreignCompartmentName,
           )}.`,
         });

packages/compartment-mapper/src/policy.js

@@ -328,31 +328,35 @@ export const attenuateGlobals = (
   freezeGlobalThisUnlessOptedOut();
 };
 
-
-const diagnoseModulePolicy = (errorHint) => {
-  if(!errorHint) {
+const diagnoseModulePolicy = errorHint => {
+  if (!errorHint) {
     return '';
   }
-  return ` (${errorHint})`;
-}
-
+  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)}`,
       );

packages/compartment-mapper/test/snapshots/test-policy.js.md

@@ -4,41 +4,77 @@ The actual snapshot is saved in `test-policy.js.snap`.
 
 Generated by [AVA](https://avajs.dev).
 
-## policy - attack - browser alias / loadLocation
+## policy - attack - browser alias - with alias hint / loadLocation
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
 
-## policy - attack - browser alias / importLocation
+## policy - attack - browser alias - with alias hint / importLocation
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
 
-## policy - attack - browser alias / makeArchive / parseArchive
+## policy - attack - browser alias - with alias hint / makeArchive / parseArchive
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
 
-## policy - attack - browser alias / makeArchive / parseArchive with a prefix
+## policy - attack - browser alias - with alias hint / makeArchive / parseArchive with a prefix
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
 
-## policy - attack - browser alias / writeArchive / loadArchive
+## policy - attack - browser alias - with alias hint / writeArchive / loadArchive
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
 
-## policy - attack - browser alias / writeArchive / importArchive
+## policy - attack - browser alias - with alias hint / writeArchive / importArchive
 
 > Snapshot 1
 
-    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" was not allowed by policy packages:{"dan":true} ( There is an alias defined that causes another package to be imported as "dan". Package "dan" resolves to "." in "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+    'Failed to load module "./attack.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (1 underlying failures: Importing "dan" in "eve-v1.0.0" was not allowed by packages policy {"dan":true} (info: Blocked in linking. "dan" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/hackity/".)'
+
+## policy - disallowed package with error hint / loadLocation
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
+
+## policy - disallowed package with error hint / importLocation
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
+
+## policy - disallowed package with error hint / makeArchive / parseArchive
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
+
+## policy - disallowed package with error hint / makeArchive / parseArchive with a prefix
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
+
+## policy - disallowed package with error hint / writeArchive / loadArchive
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
+
+## policy - disallowed package with error hint / writeArchive / importArchive
+
+> Snapshot 1
+
+    'Failed to load module "./index.js" in package "file://.../compartment-mapper/test/fixtures-policy/node_modules/app/" (2 underlying failures: Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".), Importing "carol" in "alice-v1.0.0" was not allowed by packages policy {"alice>carol":false} (info: Blocked in linking. "carol" is part of the compartment map and resolves to "file://.../compartment-mapper/test/fixtures-policy/node_modules/alice/node_modules/carol/".)'
 
 ## policy - attenuator error aggregation / loadLocation
 

packages/compartment-mapper/test/test-policy.js

@@ -113,11 +113,14 @@ const anyExpectations = {
 const powerlessCarolExpectations = {
   namespace: {
     ...defaultExpectations.namespace,
-    carol: { bluePill: 'undefined', redPill: 'undefined', purplePill: 'undefined' },
+    carol: {
+      bluePill: 'undefined',
+      redPill: 'undefined',
+      purplePill: 'undefined',
+    },
   },
 };
 
-
 const makeResultAssertions =
   expectations =>
   async (t, { namespace }) => {
@@ -167,15 +170,16 @@ scaffold(
 );
 
 scaffold(
-  'policy - attack - browser alias',
+  'policy - attack - browser alias - with alias hint',
   test,
   fixtureAttack,
   assertTestAlwaysThrows,
   2, // expected number of assertions
   {
     shouldFailBeforeArchiveOperations: true,
     onError: (t, { error }) => {
-      t.regex(error.message, /dan.*alias.*hackity/);
+      t.regex(error.message, /dan.*resolves.*hackity/);
+      // see the snapshot for the error hint in the message
       t.snapshot(sanitizePaths(error.message));
     },
     addGlobals: globals,
@@ -236,14 +240,18 @@ const recursiveEdit = editor => originalPolicy => {
 };
 const mutationEdit = editor => originalPolicy => {
   const policyToAlter = JSON.parse(JSON.stringify(originalPolicy));
-  editor(policyToAlter)
+  editor(policyToAlter);
   return policyToAlter;
-}
+};
 
-const skipCarol = mutationEdit((policyToAlter) => {
+const skipCarol = mutationEdit(policyToAlter => {
   policyToAlter.resources['alice>carol'] = undefined;
 });
 
+const disallowCarol = mutationEdit(policyToAlter => {
+  policyToAlter.resources.alice.packages['alice>carol'] = false;
+});
+
 const addAttenuatorForAllGlobals = recursiveEdit((key, obj) => {
   if (key === 'globals') {
     obj[key] = {
@@ -280,7 +288,6 @@ const nestedAttenuator = recursiveEdit((key, obj) => {
   }
 });
 
-
 scaffold(
   'policy - allow skipping policy entries for powerless compartments',
   test,
@@ -293,6 +300,23 @@ scaffold(
   },
 );
 
+scaffold(
+  'policy - disallowed package with error hint',
+  test,
+  fixture,
+  assertTestAlwaysThrows,
+  2, // expected number of assertions
+  {
+    shouldFailBeforeArchiveOperations: true,
+    onError: (t, { error }) => {
+      t.regex(error.message, /Importing.*carol.*in.*alice.*not allowed/i);
+      t.snapshot(sanitizePaths(error.message));
+    },
+    addGlobals: globals,
+    policy: disallowCarol(policy),
+  },
+);
+
 scaffold(
   'policy - globals attenuator',
   test,