From ed8e42789e97246dcee7c7c8ac24cec79cb7f56c Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Sat, 4 May 2024 20:30:55 -0700 Subject: [PATCH] feat(pass-style,exo): exo boundary only throws throwables --- packages/exo/NEWS.md | 4 + packages/exo/src/exo-tools.js | 37 ++++---- packages/exo/test/test-exo-only-throwables.js | 51 +++++++++++ packages/pass-style/NEWS.md | 4 + packages/pass-style/index.js | 1 + packages/pass-style/src/passStyleOf.js | 89 +++++++++++++++++-- packages/pass-style/test/test-errors.js | 65 ++++++++++++-- 7 files changed, 224 insertions(+), 27 deletions(-) create mode 100644 packages/exo/test/test-exo-only-throwables.js diff --git a/packages/exo/NEWS.md b/packages/exo/NEWS.md index 344e39b304..ced3903685 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 6c0eda20bf..d228f1b7f6 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/exo/test/test-exo-only-throwables.js b/packages/exo/test/test-exo-only-throwables.js new file mode 100644 index 0000000000..95fea0017a --- /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/NEWS.md b/packages/pass-style/NEWS.md index b2a572be12..d0435aad91 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 32ee735144..fea9d7b19c 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 12d8563561..7c4dd78b2e 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} 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} + */ +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} */ (specimen); +}; +harden(toThrowable); diff --git a/packages/pass-style/test/test-errors.js b/packages/pass-style/test/test-errors.js index 8ef7f607bc..2c0fd0fcce 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); });