Repository: endojs/endo
Pull request #2266 (2024-05-05 03:31:32)

feat(pass-style,exo): exo boundary only throws throwables
closes: #XXXX
refs: https://github.com/endojs/endo/pull/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.


erights: agoric-sdk compat testing underway at https://github.com/Agoric/agoric-sdk/pull/9201 , with 

pound-endo-branch: markm-to-throwable-2
and
force:integration label

erights: @kriskowal I hopefully marked this as next-release because it would be really nice for that to happen, and I also hope this is an easy enough review to not impose much delay. But if not, feel free not to include this in the next release.
erights: CI at https://github.com/Agoric/agoric-sdk/pull/9201 failed due to the presence of 
`patches/@endo+exo+1.4.0.patch` , which seems will need to disappear in the next sync anyway.
So I just added to it a removal of that patch file to https://github.com/Agoric/agoric-sdk/pull/9201 and am trying again.
erights: CI is still failing a lot, but none of the failures seem to have anything to do with this PR.
erights: Restaged https://github.com/Agoric/agoric-sdk/pull/9201 on https://github.com/Agoric/agoric-sdk/pull/8774 and trying again. I see that https://github.com/Agoric/agoric-sdk/pull/8774 is not green either. But if https://github.com/Agoric/agoric-sdk/pull/9201 only had the same failures as https://github.com/Agoric/agoric-sdk/pull/8774 , I'll count that as a success.
erights: Reviewers, because it is possible in theory that some old code was depending on an exo throwing something that would now be classified as a non-throwable, this PR is in theory a compat break. So once again, please let me know whether you think this PR should be marked as a `feat(pass-style,exo)!:`. IMO if https://github.com/Agoric/agoric-sdk/pull/9201 comes out as clean as https://github.com/Agoric/agoric-sdk/pull/8774 , I hope the answer will be no, we do not need the `!`.

Speaking of which, https://github.com/Agoric/agoric-sdk/pull/9201 CI now has novel failures such as

```
Error: src/storage-test-utils.js(2,1): error TS9006: Declaration emit for this file requires using private name 'PASS_STYLE' from module '"/home/runner/work/agoric-sdk/agoric-sdk/node_modules/@endo/pass-style/src/types"'. An explicit type annotation may unblock declaration emit.
```
at https://github.com/Agoric/agoric-sdk/actions/runs/8956132010/job/24597592447?pr=9201#step:6:2640

These still seem unrelated to this PR, so I don't understand why they are different from https://github.com/Agoric/agoric-sdk/pull/8774 . OTOH, it does seem to have something to do with the @endo/pass-style module, so perhaps it is related to this PR in a way I don't understand.
kriskowal: You are down to a couple ts-expect-errors that can be removed in integration. You can do me a favor by adding a commit to `endo-integration-master` on Agoric SDK so it gets picked up by CI and eventually by me for the sync. https://github.com/Agoric/agoric-sdk/pull/9201#issuecomment-2096909380

Patch:
diff --git a/packages/exo/NEWS.md b/packages/exo/NEWS.md
index 344e39b30..ced390368 100644
--- a/packages/exo/NEWS.md
+++ b/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.
diff --git a/packages/exo/src/exo-tools.js b/packages/exo/src/exo-tools.js
index 6c0eda20b..d228f1b7f 100644
--- a/packages/exo/src/exo-tools.js
+++ b/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;
diff --git a/packages/pass-style/NEWS.md b/packages/pass-style/NEWS.md
index b2a572be1..d0435aad9 100644
--- a/packages/pass-style/NEWS.md
+++ b/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.
diff --git a/packages/pass-style/index.js b/packages/pass-style/index.js
index 32ee73514..fea9d7b19 100644
--- a/packages/pass-style/index.js
+++ b/packages/pass-style/index.js
@@ -29,6 +29,7 @@ export {
   isPassable,
   assertPassable,
   toPassableError,
+  toThrowable,
 } from './src/passStyleOf.js';
 
 export { makeTagged } from './src/makeTagged.js';
diff --git a/packages/pass-style/src/passStyleOf.js b/packages/pass-style/src/passStyleOf.js
index 12d856356..7c4dd78b2 100644
--- a/packages/pass-style/src/passStyleOf.js
+++ b/packages/pass-style/src/passStyleOf.js
@@ -14,6 +14,7 @@ import {
   checkRecursivelyPassableErrorPropertyDesc,
   checkRecursivelyPassableError,
   getErrorConstructor,
+  isErrorLike,
 } from './error.js';
 import { RemotableHelper } from './remotable.js';
 
@@ -22,7 +23,7 @@ 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' */
@@ -30,7 +31,7 @@ import { assertPassableString } from './string.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);


Test patch:
diff --git a/packages/exo/test/test-exo-only-throwables.js b/packages/exo/test/test-exo-only-throwables.js
new file mode 100644
index 000000000..95fea0017
--- /dev/null
+++ b/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);
+});
diff --git a/packages/pass-style/test/test-errors.js b/packages/pass-style/test/test-errors.js
index 8ef7f607b..2c0fd0fcc 100644
--- a/packages/pass-style/test/test-errors.js
+++ b/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);
 });