Repository: endojs/endo
Pull request #2393 (2024-08-01 00:27:12)

fix(import-bundle): Adapt inescapable compartment mapper constructor

## Description

Recent changes to align the Compartment constructor in SES with its XS counterpart changed the argument pattern and this broke the interface of all extant wrappers around the Compartment constructor, of which we only know of one. This change fixes that wrapper and restores continuity of compatibility.

### Security Considerations

None.

### Scaling Considerations

None.

### Documentation Considerations

None.

### Testing Considerations

Inescapable global properties should cover this failure. It’s not yet clear why it did not reveal the failure earlier.

### Compatibility Considerations

Restores compatibility. Reveals that the Compartment change broke more than we anticipated, but not enough to roll back.

### Upgrade Considerations

None.


Patch:
diff --git a/packages/import-bundle/src/compartment-wrapper.js b/packages/import-bundle/src/compartment-wrapper.js
index bd420142f..506241432 100644
--- a/packages/import-bundle/src/compartment-wrapper.js
+++ b/packages/import-bundle/src/compartment-wrapper.js
@@ -1,3 +1,54 @@
+/** @import {ModuleDescriptor} from 'ses' */
+
+// In order to facilitate migration from the deprecated signature
+// of the compartment constructor,
+//   new Compartent(globals?, modules?, options?)
+// to the new signature:
+//   new Compartment(options?)
+// where globals and modules are expressed in the options bag instead of
+// positional arguments, this function detects the temporary sigil __options__
+// on the first argument and coerces compartments arguments into a single
+// compartments object.
+const compartmentOptions = (...args) => {
+  if (args.length === 0) {
+    return {};
+  }
+  if (
+    args.length === 1 &&
+    typeof args[0] === 'object' &&
+    args[0] !== null &&
+    '__options__' in args[0]
+  ) {
+    const { __options__, ...options } = args[0];
+    assert(
+      __options__ === true,
+      `Compartment constructor only supports true __options__ sigil, got ${__options__}`,
+    );
+    return options;
+  } else {
+    const [
+      globals = /** @type {Map<string, any>} */ ({}),
+      modules = /** @type {Map<string, ModuleDescriptor>} */ ({}),
+      options = {},
+    ] = args;
+    assert.equal(
+      options.modules,
+      undefined,
+      `Compartment constructor must receive either a module map argument or modules option, not both`,
+    );
+    assert.equal(
+      options.globals,
+      undefined,
+      `Compartment constructor must receive either globals argument or option, not both`,
+    );
+    return {
+      ...options,
+      globals,
+      modules,
+    };
+  }
+};
+
 export function wrapInescapableCompartment(
   OldCompartment,
   inescapableTransforms,
@@ -6,16 +57,14 @@ export function wrapInescapableCompartment(
   // This is the new Compartment constructor. We name it `Compartment` so
   // that it's .name property is correct, but we hold it in 'NewCompartment'
   // so that lint doesn't think we're shadowing the original.
-  const NewCompartment = function Compartment(
-    endowments,
-    modules,
-    oldOptions = {},
-  ) {
-    const { transforms: oldTransforms = [], ...otherOptions } = oldOptions;
+  const NewCompartment = function Compartment(...args) {
+    const { transforms: oldTransforms = [], ...otherOptions } =
+      compartmentOptions(...args);
     const newTransforms = [...oldTransforms, ...inescapableTransforms];
     const newOptions = {
       ...otherOptions,
       transforms: newTransforms,
+      __options__: true,
     };
 
     // The real Compartment is defined as a class, so 'new Compartment()'
@@ -29,11 +78,7 @@ export function wrapInescapableCompartment(
 
     // It, or a subclass, was called as a constructor
 
-    const c = Reflect.construct(
-      OldCompartment,
-      [endowments, modules, newOptions],
-      new.target,
-    );
+    const c = Reflect.construct(OldCompartment, [newOptions], new.target);
     // The confinement applies to all compartments too. This relies upon the
     // child's normal Compartment behaving the same way as the parent's,
     // which will cease to be the case soon (their module tables are
diff --git a/packages/ses/src/error/assert.js b/packages/ses/src/error/assert.js
index 29f6f8d76..06d764da2 100644
--- a/packages/ses/src/error/assert.js
+++ b/packages/ses/src/error/assert.js
@@ -579,4 +579,5 @@ export {
   note as annotateError,
   redactedDetails as X,
   quote as q,
+  bare as b,
 };
diff --git a/packages/ses/src/module-load.js b/packages/ses/src/module-load.js
index dff6c35bc..acd1a65e3 100644
--- a/packages/ses/src/module-load.js
+++ b/packages/ses/src/module-load.js
@@ -21,7 +21,7 @@ import {
   weakmapGet,
   weakmapHas,
 } from './commons.js';
-import { makeError, annotateError, q, X } from './error/assert.js';
+import { makeError, annotateError, q, b, X } from './error/assert.js';
 
 const noop = () => {};
 
@@ -154,7 +154,7 @@ function* loadWithoutErrorAnnotation(
         'importNowHook',
       );
       throw makeError(
-        X`${moduleHookName} needed to load module ${q(
+        X`${b(moduleHookName)} needed to load module ${q(
           moduleSpecifier,
         )} in compartment ${q(compartment.name)}`,
       );


Test patch:
diff --git a/packages/import-bundle/test/export-inescapable-global.js b/packages/import-bundle/test/export-inescapable-global.js
new file mode 100644
index 000000000..afcf00c66
--- /dev/null
+++ b/packages/import-bundle/test/export-inescapable-global.js
@@ -0,0 +1,1 @@
+export default new Compartment().evaluate('inescapableGlobalValue');
diff --git a/packages/import-bundle/test/import-bundle.test.js b/packages/import-bundle/test/import-bundle.test.js
index 7b37bcdb4..447ccdd47 100644
--- a/packages/import-bundle/test/import-bundle.test.js
+++ b/packages/import-bundle/test/import-bundle.test.js
@@ -123,3 +123,45 @@ test('inescapable transforms', async t => {
   });
   t.is(ns.f4('is ok'), 'substitution is ok', `iT ns.f4 ok`);
 });
+
+test('inescapable global properties, get export format', async t => {
+  const bundle = await bundleSource(
+    url.fileURLToPath(new URL('export-inescapable-global.js', import.meta.url)),
+    'getExport',
+  );
+
+  const ns = await importBundle(bundle, {
+    inescapableGlobalProperties: {
+      inescapableGlobalValue: 42,
+    },
+  });
+  t.is(ns.default, 42);
+});
+
+test('inescapable global properties, nested evaluate format', async t => {
+  const bundle = await bundleSource(
+    url.fileURLToPath(new URL('export-inescapable-global.js', import.meta.url)),
+    'nestedEvaluate',
+  );
+
+  const ns = await importBundle(bundle, {
+    inescapableGlobalProperties: {
+      inescapableGlobalValue: 42,
+    },
+  });
+  t.is(ns.default, 42);
+});
+
+test('inescapable global properties, zip base64 format', async t => {
+  const bundle = await bundleSource(
+    url.fileURLToPath(new URL('export-inescapable-global.js', import.meta.url)),
+    'endoZipBase64',
+  );
+
+  const ns = await importBundle(bundle, {
+    inescapableGlobalProperties: {
+      inescapableGlobalValue: 42,
+    },
+  });
+  t.is(ns.default, 42);
+});