Merge pull request #1785 from endojs/markm-label-promise-instances

feat(pass-style): Safe promises can override @@toStringTag with a string

packages/marshal/test/test-encode-slot-in-promise.js

@@ -0,0 +1,54 @@
+import { test } from './prepare-test-env-ava.js';
+
+// eslint-disable-next-line import/order
+import { passStyleOf } from '@endo/pass-style';
+import { makeMarshal } from '../src/marshal.js';
+
+const { getOwnPropertyDescriptor, defineProperty } = Object;
+
+const { toStringTag } = Symbol;
+
+test('use safe promise loophole', t => {
+  const convertSlotToVal = (slot, _iface) => {
+    const p = Promise.resolve(`${slot} placeholder`);
+    defineProperty(p, toStringTag, {
+      value: `Promise ${slot}`,
+    });
+    harden(p);
+    t.is(passStyleOf(p), 'promise');
+    return p;
+  };
+
+  const PromiseNameRE = /^Promise (.*)$/;
+
+  const convertValToSlot = p => {
+    t.is(passStyleOf(p), 'promise');
+    const desc = getOwnPropertyDescriptor(p, toStringTag);
+    assert(desc !== undefined);
+    const name = desc.value;
+    t.is(typeof name, 'string');
+    const match = PromiseNameRE.exec(name);
+    assert(Array.isArray(match));
+    return match[1];
+  };
+  const { toCapData, fromCapData } = makeMarshal(
+    convertValToSlot,
+    convertSlotToVal,
+    {
+      serializeBodyFormat: 'smallcaps',
+    },
+  );
+
+  const markedPromise = convertSlotToVal('I am kref 3');
+
+  const passable1 = harden([{ markedPromise }]);
+  const capData1 = toCapData(passable1);
+  t.deepEqual(capData1, {
+    body: '#[{"markedPromise":"&0"}]',
+    slots: ['I am kref 3'],
+  });
+  const passable2 = fromCapData(capData1);
+  t.deepEqual(passable1, passable2);
+  const capData2 = toCapData(passable2);
+  t.deepEqual(capData1, capData2);
+});

packages/pass-style/src/safe-promise.js

@@ -6,8 +6,9 @@ import { assertChecker, hasOwnPropertyOf } from './passStyle-helpers.js';
 /** @typedef {import('./types.js').Checker} Checker */
 
 const { details: X, quote: q } = assert;
-const { isFrozen, getPrototypeOf } = Object;
+const { isFrozen, getPrototypeOf, getOwnPropertyDescriptor } = Object;
 const { ownKeys } = Reflect;
+const { toStringTag } = Symbol;
 
 /**
  * @param {Promise} pr The value to examine
@@ -22,6 +23,15 @@ const checkPromiseOwnKeys = (pr, check) => {
     return true;
   }
 
+  /**
+   * This excludes those symbol-named own properties that are also found on
+   * `Promise.prototype`, so that overrides of these properties can be
+   * explicitly tolerated if they pass the `checkSafeOwnKey` check below.
+   * In particular, we wish to tolerate
+   *   * An overriding `toStringTag` non-enumerable data property
+   *     with a string value.
+   *   * Those own properties that might be added by Node's async_hooks.
+   */
   const unknownKeys = keys.filter(
     key => typeof key !== 'symbol' || !hasOwnPropertyOf(Promise.prototype, key),
   );
@@ -33,12 +43,17 @@ const checkPromiseOwnKeys = (pr, check) => {
   }
 
   /**
+   * Explicitly tolerate a `toStringTag` symbol-named non-enumerable
+   * data property whose value is a string. Otherwise, tolerate those
+   * symbol-named properties that might be added by NodeJS's async_hooks,
+   * if they obey the expected safety properties.
+   *
    * At the time of this writing, Node's async_hooks contains the
-   * following code, which we can also safely tolerate
+   * following code, which we can safely tolerate
    *
    * ```js
    * function destroyTracking(promise, parent) {
-   * trackPromise(promise, parent);
+   *   trackPromise(promise, parent);
    *   const asyncId = promise[async_id_symbol];
    *   const destroyed = { destroyed: false };
    *   promise[destroyedSymbol] = destroyed;
@@ -48,7 +63,27 @@ const checkPromiseOwnKeys = (pr, check) => {
    *
    * @param {string|symbol} key
    */
-  const checkSafeAsyncHooksKey = key => {
+  const checkSafeOwnKey = key => {
+    if (key === toStringTag) {
+      // TODO should we also enforce anything on the contents of the string,
+      // such as that it must start with `'Promise'`?
+      const tagDesc = getOwnPropertyDescriptor(pr, toStringTag);
+      assert(tagDesc !== undefined);
+      return (
+        (hasOwnPropertyOf(tagDesc, 'value') ||
+          reject(
+            X`Own @@toStringTag must be a data property, not an accessor: ${q(
+              tagDesc,
+            )}`,
+          )) &&
+        (typeof tagDesc.value === 'string' ||
+          reject(
+            X`Own @@toStringTag value must be a string: ${q(tagDesc.value)}`,
+          )) &&
+        (!tagDesc.enumerable ||
+          reject(X`Own @@toStringTag must not be enumerable: ${q(tagDesc)}`))
+      );
+    }
     const val = pr[key];
     if (val === undefined || typeof val === 'number') {
       return true;
@@ -79,7 +114,7 @@ const checkPromiseOwnKeys = (pr, check) => {
     );
   };
 
-  return keys.every(checkSafeAsyncHooksKey);
+  return keys.every(checkSafeOwnKey);
 };
 
 /**

packages/pass-style/test/test-safe-promise.js

@@ -0,0 +1,66 @@
+import { test } from './prepare-test-env-ava.js';
+
+import { passStyleOf } from '../src/passStyleOf.js';
+
+const { defineProperty } = Object;
+
+const { toStringTag } = Symbol;
+
+test('safe promise loophole', t => {
+  {
+    const p1 = Promise.resolve('p1');
+    t.is(passStyleOf(harden(p1)), 'promise');
+    t.is(p1[toStringTag], 'Promise');
+    t.is(`${p1}`, '[object Promise]');
+  }
+
+  {
+    const p2 = Promise.resolve('p2');
+    p2.silly = 'silly own property';
+    t.throws(() => passStyleOf(harden(p2)), {
+      message: '"[Promise]" - Must not have any own properties: ["silly"]',
+    });
+    t.is(p2[toStringTag], 'Promise');
+    t.is(`${p2}`, '[object Promise]');
+  }
+
+  {
+    const p3 = Promise.resolve('p3');
+    t.throws(
+      () => {
+        p3[toStringTag] = 3;
+      },
+      {
+        // Override mistake
+        message:
+          "Cannot assign to read only property 'Symbol(Symbol.toStringTag)' of object '[object Promise]'",
+      },
+    );
+    defineProperty(p3, toStringTag, {
+      value: 3,
+    });
+    t.throws(() => passStyleOf(harden(p3)), {
+      message: 'Own @@toStringTag value must be a string: 3',
+    });
+  }
+
+  {
+    const p4 = Promise.resolve('p4');
+    defineProperty(p4, toStringTag, {
+      value: 'Promise p4',
+      enumerable: true,
+    });
+    t.throws(() => passStyleOf(harden(p4)), {
+      message:
+        'Own @@toStringTag must not be enumerable: {"configurable":false,"enumerable":true,"value":"Promise p4","writable":false}',
+    });
+
+    const p5 = Promise.resolve('p5');
+    defineProperty(p5, toStringTag, {
+      value: 'Promise p5',
+    });
+    t.is(passStyleOf(harden(p5)), 'promise');
+    t.is(p5[toStringTag], 'Promise p5');
+    t.is(`${p5}`, '[object Promise p5]');
+  }
+});