fix: passStyleOf full input validation (#1250)

packages/marshal/src/helpers/passStyle-helpers.js

@@ -9,6 +9,7 @@ const { details: X, quote: q } = assert;
 const {
   getOwnPropertyDescriptor,
   hasOwnProperty: objectHasOwnProperty,
+  isFrozen,
 } = Object;
 const { apply } = Reflect;
 const { isArray } = Array;
@@ -111,14 +112,14 @@ harden(getTag);
 export const checkTagRecord = (tagRecord, passStyle, check = x => x) => {
   return (
     check(
-      typeof tagRecord === 'object',
+      typeof tagRecord === 'object' && tagRecord !== null,
       X`A non-object cannot be a tagRecord: ${tagRecord}`,
     ) &&
+    check(isFrozen(tagRecord), X`A tagRecord must be frozen: ${tagRecord}`) &&
     check(
       !isArray(tagRecord),
       X`An array cannot be a tagRecords: ${tagRecord}`,
     ) &&
-    check(tagRecord !== null, X`null cannot be a tagRecord`) &&
     checkNormalProperty(tagRecord, PASS_STYLE, 'symbol', false, check) &&
     check(
       tagRecord[PASS_STYLE] === passStyle,

packages/marshal/src/helpers/safe-promise.js

@@ -0,0 +1,61 @@
+// @ts-check
+
+/// <reference types="ses"/>
+
+import { isPromise } from '@endo/promise-kit';
+import { assertChecker } from './passStyle-helpers.js';
+
+/** @typedef {import('../types.js').Checker} Checker */
+
+const { details: X, quote: q } = assert;
+const { isFrozen, getPrototypeOf } = Object;
+const { ownKeys } = Reflect;
+
+/**
+ * Under Hardened JS a promise is "safe" if its `then` method can be called
+ * synchronously without giving the promise an opportunity for a
+ * reentrancy attack during that call.
+ *
+ * https://github.com/Agoric/agoric-sdk/issues/9
+ * raises the issue of testing that a specimen is a safe promise
+ * such that the test also does not give the specimen a
+ * reentrancy opportunity. That is well beyond the ambition here.
+ * TODO Though if we figure out a nice solution, it might be good to
+ * use it here as well.
+ *
+ * @param {unknown} pr The value to examine
+ * @param {Checker} [check]
+ * @returns {pr is Promise} Whether it is a safe promise
+ */
+const checkSafePromise = (pr, check = x => x) => {
+  let keys;
+  return (
+    check(isFrozen(pr), X`${pr} - Must be frozen`) &&
+    check(isPromise(pr), X`${pr} - Must be a promise`) &&
+    check(
+      getPrototypeOf(pr) === Promise.prototype,
+      X`${pr} - Must inherit from Promise.prototype: ${q(getPrototypeOf(pr))}`,
+    ) &&
+    check(
+      // Suppressing prettier for the following line because it wants to
+      // remove the "extra" parens around `pr`. However, these parens are
+      // required for the TypeScript case syntax. We know this case is safe
+      // because we only get here if `ifPromise(pr)` already passed.
+      // eslint-disable-next-line prettier/prettier
+      (keys = ownKeys(/** @type {Promise} pr */(pr))).length === 0,
+      X`{pr} - Must not have any own properties: ${q(keys)}`,
+    )
+  );
+};
+harden(checkSafePromise);
+
+/**
+ * Determine if the argument is a Promise.
+ *
+ * @param {unknown} pr The value to examine
+ * @returns {pr is Promise} Whether it is a promise
+ */
+export const isSafePromise = pr => checkSafePromise(pr);
+harden(isSafePromise);
+
+export const assertSafePromise = pr => checkSafePromise(pr, assertChecker);

packages/marshal/src/passStyleOf.js

@@ -12,6 +12,7 @@ import { RemotableHelper } from './helpers/remotable.js';
 import { ErrorHelper } from './helpers/error.js';
 
 import { assertPassableSymbol } from './helpers/symbol.js';
+import { assertSafePromise } from './helpers/safe-promise.js';
 
 /** @typedef {import('./helpers/internal-types.js').PassStyleHelper} PassStyleHelper */
 /** @typedef {import('./types.js').Passable} Passable */
@@ -149,6 +150,7 @@ const makePassStyleOf = passStyleHelpers => {
             X`Cannot pass non-frozen objects like ${inner}. Use harden()`,
           );
           if (isPromise(inner)) {
+            assertSafePromise(inner);
             return 'promise';
           }
           assert(

packages/marshal/test/test-passStyleOf.js

@@ -0,0 +1,135 @@
+import { test } from './prepare-test-env-ava.js';
+import { passStyleOf } from '../src/passStyleOf.js';
+import { Far } from '../src/make-far.js';
+import { makeTagged } from '../src/makeTagged.js';
+import { PASS_STYLE } from '../src/helpers/passStyle-helpers.js';
+
+test('passStyleOf basic success cases', t => {
+  // Test in same order as `passStyleOf` for easier maintenance.
+  // Remotables tested separately below.
+  t.is(passStyleOf(undefined), 'undefined');
+  t.is(passStyleOf('foo'), 'string');
+  t.is(passStyleOf(true), 'boolean');
+  t.is(passStyleOf(33), 'number');
+  t.is(passStyleOf(33n), 'bigint');
+  t.is(passStyleOf(Symbol.for('foo')), 'symbol');
+  t.is(passStyleOf(Symbol.iterator), 'symbol');
+  t.is(passStyleOf(null), 'null');
+  t.is(passStyleOf(harden(Promise.resolve(null))), 'promise');
+  t.is(passStyleOf(harden([3, 4])), 'copyArray');
+  t.is(passStyleOf(harden({ foo: 3 })), 'copyRecord');
+  t.is(passStyleOf(harden({ then: 'non-function then ok' })), 'copyRecord');
+  t.is(passStyleOf(makeTagged('unknown', undefined)), 'tagged');
+  t.is(passStyleOf(harden(Error('ok'))), 'error');
+});
+
+test('some passStyleOf rejections', t => {
+  t.throws(() => passStyleOf(Symbol('unique')), {
+    message: /Only registered symbols or well-known symbols are passable: "\[Symbol\(unique\)\]"/,
+  });
+  t.throws(() => passStyleOf({}), {
+    message: /Cannot pass non-frozen objects like {}. Use harden\(\)/,
+  });
+
+  const prbad1 = Promise.resolve();
+  Object.setPrototypeOf(prbad1, { __proto__: Promise.prototype });
+  harden(prbad1);
+  t.throws(() => passStyleOf(prbad1), {
+    message: /"\[Promise\]" - Must inherit from Promise.prototype: "\[Promise\]"/,
+  });
+
+  const prbad2 = Promise.resolve();
+  prbad2.extra = 'unexpected own property';
+  harden(prbad2);
+  t.throws(() => passStyleOf(prbad2), {
+    message: /{pr} - Must not have any own properties: \["extra"\]/,
+  });
+
+  const prbad3 = Promise.resolve();
+  Object.defineProperty(prbad3, 'then', { value: () => 'bad then' });
+  harden(prbad3);
+  t.throws(() => passStyleOf(prbad3), {
+    message: /{pr} - Must not have any own properties: \["then"\]/,
+  });
+
+  const thenable1 = harden({ then: () => 'thenable' });
+  t.throws(() => passStyleOf(thenable1), {
+    message: /Cannot pass non-promise thenables/,
+  });
+
+  const thenable2 = Far('remote thenable', { then: () => 'thenable' });
+  t.throws(() => passStyleOf(thenable2), {
+    message: /Cannot pass non-promise thenables/,
+  });
+});
+
+test('passStyleOf testing remotables', t => {
+  t.is(passStyleOf(Far('foo', {})), 'remotable');
+  t.is(passStyleOf(Far('foo', () => 'far function')), 'remotable');
+
+  const tagRecord1 = Object.create(Object.prototype, {
+    [PASS_STYLE]: { value: 'remotable' },
+    [Symbol.toStringTag]: { value: 'Alleged: manually constructed' },
+  });
+  const farObj1 = harden({
+    __proto__: tagRecord1,
+  });
+  t.is(passStyleOf(farObj1), 'remotable');
+
+  const tagRecord2 = Object.create(Object.prototype, {
+    [PASS_STYLE]: { value: 'remotable' },
+    [Symbol.toStringTag]: { value: 'Alleged: tagRecord not hardened' },
+  });
+  const farObj2 = Object.freeze({
+    __proto__: tagRecord2,
+  });
+  t.throws(() => passStyleOf(farObj2), {
+    message: /A tagRecord must be frozen: "\[Alleged: tagRecord not hardened\]"/,
+  });
+
+  const tagRecord3 = Object.freeze(
+    Object.create(Object.prototype, {
+      [PASS_STYLE]: { value: 'remotable' },
+      [Symbol.toStringTag]: { value: 'Alleged: both manually frozen' },
+    }),
+  );
+  const farObj3 = Object.freeze({
+    __proto__: tagRecord3,
+  });
+  t.is(passStyleOf(farObj3), 'remotable');
+
+  const tagRecord4 = Object.create(Object.prototype, {
+    [PASS_STYLE]: { value: 'remotable' },
+    [Symbol.toStringTag]: { value: 'Remotable' },
+  });
+  const farObj4 = harden({
+    __proto__: tagRecord4,
+  });
+  t.is(passStyleOf(farObj4), 'remotable');
+
+  const tagRecord5 = Object.create(Object.prototype, {
+    [PASS_STYLE]: { value: 'remotable' },
+    [Symbol.toStringTag]: { value: 'Not alleging' },
+  });
+  const farObj5 = harden({
+    __proto__: tagRecord5,
+  });
+  t.throws(() => passStyleOf(farObj5), {
+    message: /For now, iface "Not alleging" must be "Remotable" or begin with "Alleged: "; unimplemented/,
+  });
+
+  // We need this to succeed to enable far classes
+  const tagRecord6 = Object.create(Object.prototype, {
+    [PASS_STYLE]: { value: 'remotable' },
+    [Symbol.toStringTag]: { value: 'Alleged: manually constructed' },
+  });
+  const farObjProto6 = harden({
+    __proto__: tagRecord6,
+  });
+  const farObj6 = harden({
+    __proto__: farObjProto6,
+  });
+  t.throws(() => passStyleOf(farObj6), {
+    message: /"\[Symbol\(passStyle\)\]" property expected: "\[Alleged: manually constructed\]"/,
+  });
+});