fix(ses): fix #2598 with cauterizeProperty reuse (#2624)

Closes: #2598 
Refs: #2563
#2334
#1221

## Description

#1221 was supposed to make ses tolerate undeletable `func.prototype`
properties that should be absent, so long as they could be set to
`undefined` instead, making them harmless. This tolerance came with a
warning to flag the remaining non-conformance.

However #2598 explains why #1221 sometimes fails to do this. #1221 did
come with a test, but it fell into the case where #1221 works, which is
a non-toplevel function.

#2563 (and #2334 ?) fell into the trap explained by #2598 and untested
by #1221, which is an undeletable `func.prototype` on a top-level
instrinsic. As a result, #2563 currently contains a workaround for #2598
which this PR would make unnecessary.

This PR fixes the problem by factoring out the `func.prototype`-tolerant
property deletion into a separate `cauterizeProperty` function which it
calls from both places. This PR also adds the test that was missing from
#1221 , having first checked that the test detects #2598 when run
without the rest of this PR.

If this PR gets merged before #2563, then #2563's workaround for #2598
can first be removed before it is merged.

- [ ] TODO should pass a genuine reporter in to all calls to
`cauterizeProperty`. @kriskowal , please advise how intrinsics.js should
arrange to do so.

### Security Considerations

Allowing a `func.prototype` property that really shouldn't be there
seems safe, so long as it is safely set to `undefined` first, which this
PR does, and then checks that it has done so.

### Scaling Considerations

none

### Documentation Considerations

generally, this would be one less thing to worry about, and thus one
less thing that needs to be documented for most users.

### Testing Considerations

Adds the test that was missing from #1221 that let #2598 go unnoticed
until #2563

### Compatibility Considerations

Should be none.

### Upgrade Considerations

Should be none.

packages/ses/src/cauterize-property.js

@@ -0,0 +1,69 @@
+import { objectHasOwnProperty } from './commons.js';
+
+/**
+ * @import {Reporter} from './reporting-types.js'
+ */
+
+/**
+ * Delete `obj[prop]` or at least make it harmless.
+ *
+ * If the property was not expected, then emit a reporter-dependent warning
+ * to bring attention to this case, so someone can determine what to do with it.
+ *
+ * If the property to be deleted is a function's `.prototype` property, this
+ * will normally be because the function was supposed to be a
+ * - builtin method or non-constructor function
+ * - arrow function
+ * - concise method
+ *
+ * all of whom are not supposed to have a `.prototype` property. Nevertheless,
+ * on some platforms (like older versions of Hermes), or as a result of
+ * some shim-based mods to the primordials (like core-js?), some of these
+ * functions may accidentally be more like `function` functions with
+ * an undeletable `.prototype` property. In these cases, if we can
+ * set the value of that bogus `.prototype` property to `undefined`,
+ * we do so, issuing a warning, rather than failing to initialize ses.
+ *
+ * @param {object} obj
+ * @param {PropertyKey} prop
+ * @param {boolean} known If deletion is expected, don't warn
+ * @param {string} subPath Used for warning messages
+ * @param {Reporter} reporter Where to issue warning or error.
+ * @returns {void}
+ */
+export const cauterizeProperty = (
+  obj,
+  prop,
+  known,
+  subPath,
+  { warn, error },
+) => {
+  // Either the object lacks a permit or the object doesn't match the
+  // permit.
+  // If the permit is specifically false, not merely undefined,
+  // this is a property we expect to see because we know it exists in
+  // some environments and we have expressly decided to exclude it.
+  // Any other disallowed property is one we have not audited and we log
+  // that we are removing it so we know to look into it, as happens when
+  // the language evolves new features to existing intrinsics.
+  if (!known) {
+    warn(`Removing ${subPath}`);
+  }
+  try {
+    delete obj[prop];
+  } catch (err) {
+    if (objectHasOwnProperty(obj, prop)) {
+      if (typeof obj === 'function' && prop === 'prototype') {
+        obj.prototype = undefined;
+        if (obj.prototype === undefined) {
+          warn(`Tolerating undeletable ${subPath} === undefined`);
+          return;
+        }
+      }
+      error(`failed to delete ${subPath}`, err);
+    } else {
+      error(`deleting ${subPath} threw`, err);
+    }
+    throw err;
+  }
+};

packages/ses/src/compartment-shim.js

@@ -2,12 +2,18 @@ import { globalThis } from './commons.js';
 import { makeCompartmentConstructor } from './compartment.js';
 import { tameFunctionToString } from './tame-function-tostring.js';
 import { getGlobalIntrinsics } from './intrinsics.js';
+import { chooseReporter } from './reporting.js';
 
 const markVirtualizedNativeFunction = tameFunctionToString();
 
+const muteReporter = chooseReporter('none');
+
 // @ts-ignore Compartment is definitely on globalThis.
 globalThis.Compartment = makeCompartmentConstructor(
   makeCompartmentConstructor,
-  getGlobalIntrinsics(globalThis),
+  // Any reporting that would need to be done should have already been done
+  // during `lockdown()`.
+  // See https://github.com/endojs/endo/pull/2624#discussion_r1840979770
+  getGlobalIntrinsics(globalThis, muteReporter),
   markVirtualizedNativeFunction,
 );

packages/ses/src/intrinsics.js

@@ -1,3 +1,4 @@
+import { cauterizeProperty } from './cauterize-property.js';
 import {
   TypeError,
   WeakSet,
@@ -23,6 +24,10 @@ import {
   permitted,
 } from './permits.js';
 
+/**
+ * @import {Reporter} from './reporting-types.js'
+ */
+
 const isFunction = obj => typeof obj === 'function';
 
 // Like defineProperty, but throws if it would modify an existing property.
@@ -71,7 +76,10 @@ function sampleGlobals(globalObject, newPropertyNames) {
   return newIntrinsics;
 }
 
-export const makeIntrinsicsCollector = () => {
+/**
+ * @param {Reporter} reporter
+ */
+export const makeIntrinsicsCollector = reporter => {
   /** @type {Record<any, any>} */
   const intrinsics = create(null);
   let pseudoNatives;
@@ -100,7 +108,15 @@ export const makeIntrinsicsCollector = () => {
       }
       const namePrototype = permit.prototype;
       if (!namePrototype) {
-        throw TypeError(`${name}.prototype property not permitted`);
+        cauterizeProperty(
+          intrinsic,
+          'prototype',
+          false,
+          `${name}.prototype`,
+          reporter,
+        );
+        // eslint-disable-next-line no-continue
+        continue;
       }
       if (
         typeof namePrototype !== 'string' ||
@@ -164,9 +180,11 @@ export const makeIntrinsicsCollector = () => {
  * *original* unsafe (feral, untamed) bindings of these global variables.
  *
  * @param {object} globalObject
+ * @param {Reporter} reporter
  */
-export const getGlobalIntrinsics = globalObject => {
-  const { addIntrinsics, finalIntrinsics } = makeIntrinsicsCollector();
+export const getGlobalIntrinsics = (globalObject, reporter) => {
+  // TODO pass a proper reporter to `makeIntrinsicsCollector`
+  const { addIntrinsics, finalIntrinsics } = makeIntrinsicsCollector(reporter);
 
   addIntrinsics(sampleGlobals(globalObject, sharedGlobalPropertyNames));
 

packages/ses/src/lockdown.js

@@ -281,7 +281,7 @@ export const repairIntrinsics = (options = {}) => {
   const markVirtualizedNativeFunction = tameFunctionToString();
 
   const { addIntrinsics, completePrototypes, finalIntrinsics } =
-    makeIntrinsicsCollector();
+    makeIntrinsicsCollector(reporter);
 
   // @ts-expect-error __hardenTaming__ could be any string
   const tamedHarden = tameHarden(safeHarden, __hardenTaming__);

packages/ses/src/permits-intrinsics.js

@@ -61,6 +61,7 @@ import {
   ownKeys,
   symbolKeyFor,
 } from './commons.js';
+import { cauterizeProperty } from './cauterize-property.js';
 
 /**
  * @import {Reporter} from './reporting-types.js'
@@ -77,7 +78,7 @@ import {
 export default function removeUnpermittedIntrinsics(
   intrinsics,
   markVirtualizedNativeFunction,
-  { warn, error },
+  reporter,
 ) {
   // These primitives are allowed for permits.
   const primitives = ['undefined', 'boolean', 'number', 'string', 'symbol'];
@@ -279,35 +280,7 @@ export default function removeUnpermittedIntrinsics(
       const subPermit = getSubPermit(obj, permit, propString);
 
       if (!subPermit || !isAllowedProperty(subPath, obj, prop, subPermit)) {
-        // Either the object lacks a permit or the object doesn't match the
-        // permit.
-        // If the permit is specifically false, not merely undefined,
-        // this is a property we expect to see because we know it exists in
-        // some environments and we have expressly decided to exclude it.
-        // Any other disallowed property is one we have not audited and we log
-        // that we are removing it so we know to look into it, as happens when
-        // the language evolves new features to existing intrinsics.
-        if (subPermit !== false) {
-          warn(`Removing ${subPath}`);
-        }
-        try {
-          delete obj[prop];
-        } catch (err) {
-          if (prop in obj) {
-            if (typeof obj === 'function' && prop === 'prototype') {
-              obj.prototype = undefined;
-              if (obj.prototype === undefined) {
-                warn(`Tolerating undeletable ${subPath} === undefined`);
-                // eslint-disable-next-line no-continue
-                continue;
-              }
-            }
-            error(`failed to delete ${subPath}`, err);
-          } else {
-            error(`deleting ${subPath} threw`, err);
-          }
-          throw err;
-        }
+        cauterizeProperty(obj, prop, subPermit === false, subPath, reporter);
       }
     }
   }

packages/ses/test/tolerate-empty-prototype-toplevel.test.js

@@ -0,0 +1,24 @@
+/* global globalThis */
+import test from 'ava';
+import '../index.js';
+
+// See https://github.com/zloirock/core-js/issues/1092
+// See https://github.com/endojs/endo/issues/2598
+const originalEscape = globalThis.escape;
+globalThis.escape = function escape(...args) {
+  return Reflect.apply(originalEscape, this, args);
+};
+
+lockdown();
+
+test('tolerate empty escape.prototype', t => {
+  t.is(globalThis.escape, escape);
+  t.assert('prototype' in escape);
+  t.is(escape.prototype, undefined);
+  t.deepEqual(Object.getOwnPropertyDescriptor(escape, 'prototype'), {
+    value: undefined,
+    writable: !!harden.isFake,
+    enumerable: false,
+    configurable: false,
+  });
+});

packages/ses/test/tolerate-empty-prototype.test.js

@@ -2,6 +2,9 @@ import test from 'ava';
 import '../index.js';
 
 // See https://github.com/zloirock/core-js/issues/1092
+// Does not detect https://github.com/endojs/endo/issues/2598 because
+// `push` is not toplevel.
+// See tolerate-empty-prototype-toplevel.test.js
 const originalPush = Array.prototype.push;
 // eslint-disable-next-line no-extend-native
 Array.prototype.push = function push(...args) {