feat(pass-style,exo): exo boundary only throws throwables (#2266)

closes: #XXXX
refs: #2223 

## Description

Modest step towards #2223 , 
- bringing all the intended safety: the exo boundary only throws
throwables, and that `callWhen` async exo methods only reject with
throwable reasons.
- but with less flexibility: the definition of passable errors and
throwables is narrower than it need be.

### Security Considerations

This is one of three necessary steps towards having the exo boundaries
become the new intravat defensive security boundary, rather than
treating every object boundary as a potential defensive security
boundary. The rationale for this step is that the throw-control-path is
too hard for reviewers to pay full attention to, so we wish to prevent
caps from leaking over the exo boundary via the throw path. (E prevented
caps from escaping via throws in general, but we're not in a position to
be able to enforce that in general within Hardened JS.)

The other two steps are
- Converting uses of `Far` for making far objects into making exos.
- Addressing the leakage of promise settlements across exo boundaries
due to the inability to synchronously enforce such a constraint on a
promise passed through the boundary.

### Scaling Considerations

Given that the exceptional pathways (throws and rejects) are only used
for low frequency exceptional conditions, or at least exceptional
conditions whose speed we don't care about, making this slow path a tiny
bit slower should be inconsequential. Indeed, I expect the overall
impact to be unmeasurable. (But we won't know until we measure!)

### Documentation Considerations

This restriction on what can be throws across the exo boundary (or for
exo async `callWhen` methods, what can be used as a rejection reason)
needs to be documented. But it should not affect any normal code, and so
documents an edge case.

### Testing Considerations

tests included

### Compatibility Considerations

If there are currently any throws or rejects that violate these
constraints (likely) where other code depends on these violations
(unlikely), then we have a compat problem.

### Upgrade Considerations

None.

- ~[ ] Includes `*BREAKING*:` in the commit message with migration
instructions for any breaking change.~
- [x] Updates `NEWS.md` for user-facing changes.

packages/exo/NEWS.md

@@ -1,5 +1,9 @@
 User-visible changes in `@endo/exo`:
 
+# Next release
+
+- A call to an exo will only throw a throwable, i.e., a Passable without capabilities, i.e., without Remotables or Promises. It will consist only of copy data and Passable errors. Passable errors themselves cannot contain capabilities, and so are throwable. An async exo `callWhen` method will likewise only reject with a throwable reason. Both contraints help security reviews, since experience shows it is too hard for reviewers to be adequately vigilant about capabilities communicated over the implicit exceptional control flow pathways.
+
 # v0.2.6 (2023-09-11)
 
 - Adds support for symbol-keyed methods in interface guards, e.g.

packages/exo/src/exo-tools.js

@@ -1,5 +1,5 @@
 import { getMethodNames } from '@endo/eventual-send/utils.js';
-import { hasOwnPropertyOf } from '@endo/pass-style';
+import { hasOwnPropertyOf, toThrowable } from '@endo/pass-style';
 import { E, Far } from '@endo/far';
 import {
   mustMatch,
@@ -164,14 +164,18 @@ const defendSyncMethod = (
   const { syncMethod } = {
     // Note purposeful use of `this` and concise method syntax
     syncMethod(...syncArgs) {
-      const context = getContext(this);
-      // Only harden args and return value if not dealing with a raw value guard.
-      const realArgs = defendSyncArgs(syncArgs, matchConfig, label);
-      const result = apply(behaviorMethod, context, realArgs);
-      if (!isRawReturn) {
-        mustMatch(harden(result), returnGuard, `${label}: result`);
+      try {
+        const context = getContext(this);
+        // Only harden args and return value if not dealing with a raw value guard.
+        const realArgs = defendSyncArgs(syncArgs, matchConfig, label);
+        const result = apply(behaviorMethod, context, realArgs);
+        if (!isRawReturn) {
+          mustMatch(harden(result), returnGuard, `${label}: result`);
+        }
+        return result;
+      } catch (thrownThing) {
+        throw toThrowable(thrownThing);
       }
-      return result;
     },
   };
   return syncMethod;
@@ -251,13 +255,16 @@ const defendAsyncMethod = (
           return apply(behaviorMethod, context, realArgs);
         },
       );
-      if (isRawReturn) {
-        return resultP;
-      }
-      return E.when(resultP, result => {
-        mustMatch(harden(result), returnGuard, `${label}: result`);
-        return result;
-      });
+      return E.when(resultP, fulfillment => {
+        if (!isRawReturn) {
+          mustMatch(harden(fulfillment), returnGuard, `${label}: result`);
+        }
+        return fulfillment;
+      }).catch(reason =>
+        // Done is a chained `.catch` rather than an onRejected clause of the
+        // `E.when` above in case the `mustMatch` throws.
+        Promise.reject(toThrowable(reason)),
+      );
     },
   };
   return asyncMethod;

packages/exo/test/test-exo-only-throwables.js

@@ -0,0 +1,51 @@
+import test from '@endo/ses-ava/prepare-endo.js';
+
+import { makeError } from '@endo/errors';
+import { Far, isPassable } from '@endo/pass-style';
+import { M } from '@endo/patterns';
+import { makeExo } from '../src/exo-makers.js';
+
+const { defineProperty } = Object;
+
+const thrower = makeExo(
+  'Thrower',
+  M.interface('Thrower', {
+    throw: M.call(M.raw()).returns(M.any()),
+    reject: M.callWhen(M.raw()).returns(M.any()),
+  }),
+  {
+    throw(val) {
+      throw val;
+    },
+    reject(val) {
+      throw val;
+    },
+  },
+);
+
+test('exo only throwables', async t => {
+  const e = makeError('test error', undefined, {
+    sanitize: false,
+  });
+
+  // Remotables cannot be in passable errors or throwables
+  defineProperty(e, 'foo', { value: Far('Foo', {}) });
+
+  let caught;
+  try {
+    thrower.throw(e);
+  } catch (thrown) {
+    caught = thrown;
+  }
+  t.false(isPassable(e));
+  t.true(isPassable(caught));
+  t.log('throw caught', caught);
+
+  try {
+    await thrower.reject(e);
+  } catch (thrown) {
+    caught = thrown;
+  }
+  t.true(isPassable(caught));
+  t.log('reject caught', caught);
+});

packages/pass-style/NEWS.md

@@ -1,5 +1,9 @@
 User-visible changes in `@endo/pass-style`:
 
+# Next release
+
+- Adds `toThrowable` as a generalization of `toPassableError` that also admits copy data containing passable errors, but still without passable caps, i.e, without remotables or promises. This is in support of the exo boundary throwing only throwables, to ease security review.
+
 # v1.3.0 (2024-03-19)
 
 - Exports `isWellFormedString` and `assertWellFormedString`. Unfortunately the [standard `String.prototype.isWellFormed`](https://tc39.es/proposal-is-usv-string/) first coerces its input to string, leading it to claim that some non-strings are well-formed strings. By contrast, `isWellFormedString` and `assertWellFormedString` will not judge any non-strings to be well-formed strings.

packages/pass-style/index.js

@@ -29,6 +29,7 @@ export {
   isPassable,
   assertPassable,
   toPassableError,
+  toThrowable,
 } from './src/passStyleOf.js';
 
 export { makeTagged } from './src/makeTagged.js';

packages/pass-style/src/passStyleOf.js

@@ -14,6 +14,7 @@ import {
   checkRecursivelyPassableErrorPropertyDesc,
   checkRecursivelyPassableError,
   getErrorConstructor,
+  isErrorLike,
 } from './error.js';
 import { RemotableHelper } from './remotable.js';
 
@@ -22,15 +23,15 @@ import { assertSafePromise } from './safe-promise.js';
 import { assertPassableString } from './string.js';
 
 /** @import {PassStyleHelper} from './internal-types.js' */
-/** @import {Passable} from './types.js' */
+/** @import {CopyArray, CopyRecord, CopyTagged, Passable} from './types.js' */
 /** @import {PassStyle} from './types.js' */
 /** @import {PassStyleOf} from './types.js' */
 /** @import {PrimitiveStyle} from './types.js' */
 
 /** @typedef {Exclude<PassStyle, PrimitiveStyle | "promise">} HelperPassStyle */
 
 const { ownKeys } = Reflect;
-const { isFrozen, getOwnPropertyDescriptors } = Object;
+const { isFrozen, getOwnPropertyDescriptors, values } = Object;
 
 /**
  * @param {PassStyleHelper[]} passStyleHelpers
@@ -262,10 +263,14 @@ const isPassableErrorPropertyDesc = (name, desc) =>
   checkRecursivelyPassableErrorPropertyDesc(name, desc, passStyleOf);
 
 /**
- * Return a passable error that propagates the diagnostic info of the
- * original, and is linked to the original as a note.
- * `toPassableError` hardens the argument before checking if it is already
- * a passable error. If it is, then `toPassableError` returns the argument.
+ * After hardening, if `err` is a passable error, return it.
+ *
+ * Otherwise, return a new passable error that propagates the diagnostic
+ * info of the original, and is linked to the original as a note.
+ *
+ * TODO Adopt a more flexible notion of passable error, in which
+ * a passable error can contain other own data properties with
+ * throwable values.
  *
  * @param {Error} err
  * @returns {Error}
@@ -304,3 +309,75 @@ export const toPassableError = err => {
   return newError;
 };
 harden(toPassableError);
+
+/**
+ * After hardening, if `specimen` is throwable, return it.
+ * A specimen is throwable iff it is Passable and contains no PassableCaps,
+ * i.e., no Remotables or Promises.
+ * IOW, if it contains only copy-data and passable errors.
+ *
+ * Otherwise, if `specimen` is *almost* throwable, for example, it is
+ * an error that can be made throwable by `toPassableError`, then
+ * return `specimen` converted to a throwable.
+ *
+ * Otherwise, throw a diagnostic indicating a failure to coerce.
+ *
+ * This is in support of the exo boundary throwing only throwables, to ease
+ * security review.
+ *
+ * TODO Adopt a more flexitble notion of throwable, in which
+ * data containers containing non-passable errors can themselves be coerced
+ * to throwable by coercing to a similar containers containing
+ * the results of coercing those errors to passable errors.
+ *
+ * @param {unknown} specimen
+ * @returns {Passable<never, Error>}
+ */
+export const toThrowable = specimen => {
+  harden(specimen);
+  if (isErrorLike(specimen)) {
+    return toPassableError(/** @type {Error} */ (specimen));
+  }
+  // Note that this step will fail if `specimen` would be a passable container
+  // except that it contains non-passable errors that could be converted.
+  // This will need to be fixed to do the TODO above.
+  const passStyle = passStyleOf(specimen);
+  if (isObject(specimen)) {
+    switch (passStyle) {
+      case 'copyArray': {
+        const elements = /** @type {CopyArray} */ (specimen);
+        for (const element of elements) {
+          element === toThrowable(element) ||
+            Fail`nested toThrowable coercion not yet supported ${element}`;
+        }
+        break;
+      }
+      case 'copyRecord': {
+        const rec = /** @type {CopyRecord} */ (specimen);
+        for (const val of values(rec)) {
+          val === toThrowable(val) ||
+            Fail`nested toThrowable coercion not yet supported ${val}`;
+        }
+        break;
+      }
+      case 'tagged': {
+        const tg = /** @type {CopyTagged} */ (specimen);
+        const { payload } = tg;
+        payload === toThrowable(payload) ||
+          Fail`nested toThrowable coercion not yet supported ${payload}`;
+        break;
+      }
+      case 'error': {
+        const er = /** @type {Error} */ (specimen);
+        er === toThrowable(er) ||
+          Fail`nested toThrowable coercion not yet supported ${er}`;
+        break;
+      }
+      default: {
+        throw Fail`A ${q(passStyle)} is not throwable: ${specimen}`;
+      }
+    }
+  }
+  return /** @type {Passable<never,never>} */ (specimen);
+};
+harden(toThrowable);

packages/pass-style/test/test-errors.js

@@ -6,7 +6,12 @@ import {
   passStyleOf,
   isPassable,
   toPassableError,
+  toThrowable,
 } from '../src/passStyleOf.js';
+import { Far } from '../src/make-far.js';
+import { makeTagged } from '../src/makeTagged.js';
+
+const { defineProperty } = Object;
 
 test('style of extended errors', t => {
   const e1 = Error('e1');
@@ -29,10 +34,14 @@ test('style of extended errors', t => {
   }
 });
 
-test('toPassableError rejects unfrozen errors', t => {
+test('toPassableError, toThrowable', t => {
   const e = makeError('test error', undefined, {
     sanitize: false,
   });
+
+  // Remotables cannot be in passable errors or throwables
+  defineProperty(e, 'foo', { value: Far('Foo', {}) });
+
   // I include this test because I was recently surprised that the errors
   // made by `makeError` are not frozen, and therefore not passable.
   // Since then, we changed `makeError` to make reasonable effort
@@ -45,12 +54,56 @@ test('toPassableError rejects unfrozen errors', t => {
   // is a passable error.
   const e2 = toPassableError(e);
 
+  t.true(Object.isFrozen(e));
+  t.false(isPassable(e));
+
   t.true(Object.isFrozen(e2));
   t.true(isPassable(e2));
 
-  // May not be true on all platforms, depending on what "extraneous"
-  // properties the host added to the error before `makeError` returned it.
-  // If this fails, please let us know. See the doccomment on the
-  // `sanitizeError` function is the ses-shim's `assert.js`.
-  t.is(e, e2);
+  t.not(e, e2);
+  t.log('passable', e2);
+
+  t.is(e2, toThrowable(e2));
+  t.deepEqual(toThrowable(e), e2);
+
+  const notYetCoercable = harden([e]);
+  // Note: eventually `toThrowable(notYetCoercable)` should return
+  // a throwable singleton copyArray containing a toThrowable(e), i.e.,
+  // an error like e2.
+  t.throws(() => toThrowable(notYetCoercable), {
+    message: 'Passable Error has extra unpassed property "foo"',
+  });
+
+  const throwable = harden([e2, { e2 }, makeTagged('e2', e2)]);
+  t.is(throwable, toThrowable(throwable));
+});
+
+/**
+ * Copied from
+ * https://github.com/Agoric/agoric-sdk/blob/286302a192b9eb2e222faa08479f496645bb7b9a/packages/internal/src/upgrade-api.js#L25-L39
+ * to verify that an UpgradeDisconnection object is throwable, as we need it
+ * to be.
+ *
+ * Makes an Error-like object for use as the rejection reason of promises
+ * abandoned by upgrade.
+ *
+ * @param {string} upgradeMessage
+ * @param {number} toIncarnationNumber
+ * @returns {UpgradeDisconnection}
+ */
+const makeUpgradeDisconnection = (upgradeMessage, toIncarnationNumber) =>
+  harden({
+    name: 'vatUpgraded',
+    upgradeMessage,
+    incarnationNumber: toIncarnationNumber,
+  });
+
+/**
+ * Copied from
+ * https://github.com/Agoric/agoric-sdk/blob/286302a192b9eb2e222faa08479f496645bb7b9a/packages/internal/test/test-upgrade-api.js#L9
+ */
+const disconnection = makeUpgradeDisconnection('vat upgraded', 2);
+
+test('UpgradeDisconnection is throwable', t => {
+  t.is(toThrowable(disconnection), disconnection);
 });