From 62b42efa0aacd183f2d591e2213aaa1e8848ad4e Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 4 Apr 2024 17:52:27 -0700 Subject: [PATCH] fix(ses): makeError defaults to making passable errors --- packages/marshal/src/marshal.js | 1 + packages/pass-style/src/passStyleOf.js | 1 + packages/pass-style/test/test-errors.js | 18 ++++-- packages/ses/src/error/assert.js | 77 ++++++++++++++++++++++++- packages/ses/src/error/types.js | 17 +++++- 5 files changed, 108 insertions(+), 6 deletions(-) diff --git a/packages/marshal/src/marshal.js b/packages/marshal/src/marshal.js index 281c9590fd..ed9065657b 100644 --- a/packages/marshal/src/marshal.js +++ b/packages/marshal/src/marshal.js @@ -296,6 +296,7 @@ export const makeMarshal = ( : `Remote${errConstructor.name}(${dErrorId})`; const options = { errorName, + sanitize: false, }; if (cause) { options.cause = decodeRecur(cause); diff --git a/packages/pass-style/src/passStyleOf.js b/packages/pass-style/src/passStyleOf.js index d96528314a..1b6f497ba2 100644 --- a/packages/pass-style/src/passStyleOf.js +++ b/packages/pass-style/src/passStyleOf.js @@ -298,6 +298,7 @@ export const toPassableError = err => { cause, errors, }); + // Still needed, because `makeError` only does a shallow freeze. harden(newError); // Even the cleaned up error copy, if sent to the console, should // cause hidden diagnostic information of the original error diff --git a/packages/pass-style/test/test-errors.js b/packages/pass-style/test/test-errors.js index b1e0e44a70..8ef7f607bc 100644 --- a/packages/pass-style/test/test-errors.js +++ b/packages/pass-style/test/test-errors.js @@ -30,9 +30,14 @@ test('style of extended errors', t => { }); test('toPassableError rejects unfrozen errors', t => { - const e = makeError('test error'); + const e = makeError('test error', undefined, { + sanitize: false, + }); // I include this test because I was recently surprised that the errors - // make by `makeError` are not frozen, and therefore not passable. + // made by `makeError` are not frozen, and therefore not passable. + // Since then, we changed `makeError` to make reasonable effort + // to return a passable error by default. But also added the + // `sanitize: false` option to suppress that. t.false(Object.isFrozen(e)); t.false(isPassable(e)); @@ -40,7 +45,12 @@ test('toPassableError rejects unfrozen errors', t => { // is a passable error. const e2 = toPassableError(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.true(Object.isFrozen(e)); - t.true(isPassable(e)); }); diff --git a/packages/ses/src/error/assert.js b/packages/ses/src/error/assert.js index bacf5124c9..cb918a57c5 100644 --- a/packages/ses/src/error/assert.js +++ b/packages/ses/src/error/assert.js @@ -35,6 +35,11 @@ import { weakmapHas, weakmapSet, AggregateError, + getOwnPropertyDescriptors, + ownKeys, + create, + objectPrototype, + objectHasOwnProperty, } from '../commons.js'; import { an, bestEffortStringify } from './stringify-utils.js'; import './types.js'; @@ -254,13 +259,80 @@ const tagError = (err, optErrorName = err.name) => { return errorTag; }; +/** + * Make reasonable best efforts to make a `Passable` error. + * - `sanitizeError` will remove any "extraneous" own properties already added + * by the host, + * such as `fileName`,`lineNumber` on FireFox or `line` on Safari. + * - If any such "extraneous" properties were removed, `sanitizeError` will + * annotate + * the error with them, so they still appear on the causal console + * log output for diagnostic purposes, but not be otherwise visible. + * - `sanitizeError` will ensure that any expected properties already + * added by the host are data + * properties, converting accessor properties to data properties as needed, + * such as `stack` on v8 (Chrome, Brave, Edge?) + * - `sanitizeError` will freeze the error, preventing any correct engine from + * adding or + * altering any of the error's own properties `sanitizeError` is done. + * + * However, `sanitizeError` will not, for example, `harden` + * (i.e., deeply freeze) + * or ensure that the `cause` or `errors` property satisfy the `Passable` + * constraints. The purpose of `sanitizeError` is only to protect against + * mischief the host may have already added to the error as created, + * not to ensure that the error is actually Passable. For that, + * see `toPassableError` in `@endo/pass-style`. + * + * @param {Error} error + */ +const sanitizeError = error => { + const descs = getOwnPropertyDescriptors(error); + const { + name: _nameDesc, + message: _messageDesc, + errors: _errorsDesc = undefined, + cause: _causeDesc = undefined, + stack: _stackDesc = undefined, + ...restDescs + } = descs; + + const restNames = ownKeys(restDescs); + if (restNames.length >= 1) { + for (const name of restNames) { + delete error[name]; + } + const droppedNote = create(objectPrototype, restDescs); + // eslint-disable-next-line no-use-before-define + note( + error, + redactedDetails`originally with properties ${quote(droppedNote)}`, + ); + } + for (const name of ownKeys(error)) { + // @ts-expect-error TS still confused by symbols as property names + const desc = descs[name]; + if (desc && objectHasOwnProperty(desc, 'get')) { + defineProperty(error, name, { + value: error[name], // invoke the getter to convert to data property + }); + } + } + freeze(error); +}; + /** * @type {AssertMakeError} */ const makeError = ( optDetails = redactedDetails`Assert failed`, errConstructor = globalThis.Error, - { errorName = undefined, cause = undefined, errors = undefined } = {}, + { + errorName = undefined, + cause = undefined, + errors = undefined, + sanitize = true, + } = {}, ) => { if (typeof optDetails === 'string') { // If it is a string, use it as the literal part of the template so @@ -299,6 +371,9 @@ const makeError = ( if (errorName !== undefined) { tagError(error, errorName); } + if (sanitize) { + sanitizeError(error); + } // The next line is a particularly fruitful place to put a breakpoint. return error; }; diff --git a/packages/ses/src/error/types.js b/packages/ses/src/error/types.js index d3ecdfaa52..0631c08a1e 100644 --- a/packages/ses/src/error/types.js +++ b/packages/ses/src/error/types.js @@ -27,9 +27,24 @@ /** * @typedef {object} AssertMakeErrorOptions * @property {string} [errorName] + * Does not affect the error.name property. That remains determined by + * the constructor. Rather, the `errorName` determines how this error is + * identified in the causal console log's output. * @property {Error} [cause] + * Discloses the error that caused this one, typically from a lower + * layer of abstraction. This is represented by a public `cause` data property + * on the error, not a hidden annotation. * @property {Error[]} [errors] - * Normally only used when the ErrorConstuctor is `AggregateError` + * Normally only used when the ErrorConstuctor is `AggregateError`, to + * represent the set of prior errors aggregated together in this error, + * typically by `Promise.any`. But `makeError` allows it on any error. + * This is represented by a public `errors` data property on the error, + * not a hidden annotation. + * @property {boolean} [sanitize] + * Defaults to true. If true, `makeError` will apply `sanitizeError` + * to the error before returning it. See the comments on `sanitizeError`. + * (TODO what is the proper jsdoc manner to link to another function's + * doc-comment?) */ /**