fix(ses): makeError defaults to making passable errors

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);

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

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

@@ -30,17 +30,27 @@ 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));
 
   // toPassableError hardens, and then checks whether the hardened argument
   // 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));
 });

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;
 };

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?)
  */
 
 /**