From 579294902f85fba6b171e2199a44ab7522258f46 Mon Sep 17 00:00:00 2001
From: "Mark S. Miller" <erights@users.noreply.github.com>
Date: Tue, 16 Jan 2024 18:35:40 -0800
Subject: [PATCH] fix(ses): handle properties that are already override
 protected (#1969)

---
 packages/ses/NEWS.md                          |  18 +-
 packages/ses/src/enable-property-overrides.js |  10 +-
 packages/ses/src/enablements.js               |  13 ++
 packages/ses/src/lockdown.js                  |   3 +
 packages/ses/src/tame-faux-data-properties.js | 214 ++++++++++++++++++
 .../test/test-tame-faux-data-properties.js    | 135 +++++++++++
 6 files changed, 385 insertions(+), 8 deletions(-)
 create mode 100644 packages/ses/src/tame-faux-data-properties.js
 create mode 100644 packages/ses/test/test-tame-faux-data-properties.js

diff --git a/packages/ses/NEWS.md b/packages/ses/NEWS.md
index 18ec344653..9dd539ffaa 100644
--- a/packages/ses/NEWS.md
+++ b/packages/ses/NEWS.md
@@ -1,4 +1,20 @@
 User-visible changes in SES:
+# next
+
+- The [iterators-helpers](https://github.com/tc39/proposal-iterator-helpers)
+  proposal includes two accessor properties whose purpose is to emulate
+  a data property, but without the override mistake problem. The ses-shim
+  creates many such properties, but was unprepared for them to already be
+  present in the JS platform it starts with. A Chrome Canary and Node 22
+  both implement the iterators-helper proposal, triggering this bug, preventing
+  the ses-shim from initializing. The ses-shim
+  [now copes safely](https://github.com/endojs/endo/pull/1969) with an
+  enumerated set of such properties, starting with these two properties from
+  the iterators-helpers proposal.
+- The ses-shim now permits the new methods from the
+  [set-methods](https://github.com/tc39/proposal-set-methods) proposal,
+  enabling these methods to be used on platforms where they are implemented,
+  which are currently a Chrome Canary and a Node 22.
 
 # v0.18.8 (2023-09-11)
 
@@ -27,7 +43,7 @@ User-visible changes in SES:
 
 # v0.18.6 (2023-08-07)
 
-- Censors the pattern `{...import(specifier)`}.
+- Censors the pattern `{...import(specifier)}`.
   We previously censored `import(specifier)` and expressly allowed
   `object.import(specifier)`.
   The relaxation for the latter form in version 0.13.0 inadvertently allowed
diff --git a/packages/ses/src/enable-property-overrides.js b/packages/ses/src/enable-property-overrides.js
index b4b18a41ce..06d6563186 100644
--- a/packages/ses/src/enable-property-overrides.js
+++ b/packages/ses/src/enable-property-overrides.js
@@ -11,7 +11,6 @@ import {
   defineProperty,
   getOwnPropertyDescriptor,
   getOwnPropertyDescriptors,
-  getOwnPropertyNames,
   isObject,
   objectHasOwnProperty,
   ownKeys,
@@ -156,12 +155,11 @@ export default function enablePropertyOverrides(
     }
     // TypeScript does not allow symbols to be used as indexes because it
     // cannot recokon types of symbolized properties.
-    // @ts-ignore
     arrayForEach(ownKeys(descs), prop => enable(path, obj, prop, descs[prop]));
   }
 
   function enableProperties(path, obj, plan) {
-    for (const prop of getOwnPropertyNames(plan)) {
+    for (const prop of ownKeys(plan)) {
       const desc = getOwnPropertyDescriptor(obj, prop);
       if (!desc || desc.get || desc.set) {
         // No not a value property, nothing to do.
@@ -169,10 +167,8 @@ export default function enablePropertyOverrides(
         continue;
       }
 
-      // Plan has no symbol keys and we use getOwnPropertyNames()
-      // so `prop` cannot only be a string, not a symbol. We coerce it in place
-      // with `String(..)` anyway just as good hygiene, since these paths are just
-      // for diagnostic purposes.
+      // In case `prop` is a symbol, we first coerce it with `String`,
+      // purely for diagnostic purposes.
       const subPath = `${path}.${String(prop)}`;
       const subPlan = plan[prop];
 
diff --git a/packages/ses/src/enablements.js b/packages/ses/src/enablements.js
index e45c45e1a1..222418121a 100644
--- a/packages/ses/src/enablements.js
+++ b/packages/ses/src/enablements.js
@@ -1,3 +1,5 @@
+import { toStringTagSymbol } from './commons.js';
+
 /**
  * @file Exports {@code enablements}, a recursively defined
  * JSON record defining the optimum set of intrinsics properties
@@ -74,6 +76,13 @@ export const minEnablements = {
   '%ErrorPrototype%': {
     name: true, // set by "precond", "ava", "node-fetch"
   },
+  '%IteratorPrototype%': {
+    toString: true,
+    // https://github.com/tc39/proposal-iterator-helpers
+    constructor: true,
+    // https://github.com/tc39/proposal-iterator-helpers
+    [toStringTagSymbol]: true,
+  },
 };
 
 /**
@@ -152,6 +161,10 @@ export const moderateEnablements = {
 
   '%IteratorPrototype%': {
     toString: true,
+    // https://github.com/tc39/proposal-iterator-helpers
+    constructor: true,
+    // https://github.com/tc39/proposal-iterator-helpers
+    [toStringTagSymbol]: true,
   },
 };
 
diff --git a/packages/ses/src/lockdown.js b/packages/ses/src/lockdown.js
index 01babf187e..56f339664a 100644
--- a/packages/ses/src/lockdown.js
+++ b/packages/ses/src/lockdown.js
@@ -54,6 +54,7 @@ import { getAnonymousIntrinsics } from './get-anonymous-intrinsics.js';
 import { makeCompartmentConstructor } from './compartment.js';
 import { tameHarden } from './tame-harden.js';
 import { tameSymbolConstructor } from './tame-symbol-constructor.js';
+import { tameFauxDataProperties } from './tame-faux-data-properties.js';
 
 /** @typedef {import('../types.js').LockdownOptions} LockdownOptions */
 
@@ -335,6 +336,8 @@ export const repairIntrinsics = (options = {}) => {
   // Replace *Locale* methods with their non-locale equivalents
   tameLocaleMethods(intrinsics, localeTaming);
 
+  tameFauxDataProperties(intrinsics);
+
   /**
    * 2. WHITELIST to standardize the environment.
    */
diff --git a/packages/ses/src/tame-faux-data-properties.js b/packages/ses/src/tame-faux-data-properties.js
new file mode 100644
index 0000000000..58576768ef
--- /dev/null
+++ b/packages/ses/src/tame-faux-data-properties.js
@@ -0,0 +1,214 @@
+import {
+  getOwnPropertyDescriptor,
+  apply,
+  defineProperty,
+  toStringTagSymbol,
+} from './commons.js';
+
+const throws = thunk => {
+  try {
+    thunk();
+  } catch (er) {
+    return true;
+  }
+  return false;
+};
+
+/**
+ * Exported for convenience of unit testing. Harmless, but not expected
+ * to be useful by itself.
+ *
+ * @param {any} obj
+ * @param {string|symbol} prop
+ * @param {any} expectedValue
+ * @returns {boolean}
+ * Returns whether `tameFauxDataProperty` turned the property in question
+ * from an apparent faux data property into the actual data property it
+ * seemed to emulate.
+ * If it returns `false`, then we hope no effects happened. However, the whole
+ * point of the validation tests here are to sniff out if an accessor
+ * property seems to be a faux data property. To do we, it invokes the
+ * getter and setter functions that might possibly have side effects.
+ * `tameFauxDataProperty` is not in a position to tell.
+ */
+export const tameFauxDataProperty = (obj, prop, expectedValue) => {
+  if (obj === undefined) {
+    // The object does not exist in this version of the platform
+    return false;
+  }
+  const desc = getOwnPropertyDescriptor(obj, prop);
+  if (!desc || 'value' in desc) {
+    // The property either doesn't exist, or is already an actual data property.
+    return false;
+  }
+  const { get, set } = desc;
+  if (typeof get !== 'function') {
+    // A faux data property has both a getter and a setter
+    return false;
+  }
+  if (typeof set !== 'function') {
+    return false;
+  }
+  if (get() !== expectedValue) {
+    // The getter called by itself should produce the expectedValue
+    return false;
+  }
+  if (apply(get, obj, []) !== expectedValue) {
+    // The getter called with `this === obj` should also return the
+    // expectedValue.
+    return false;
+  }
+  const testValue = 'Seems to be a setter';
+  const subject1 = { __proto__: null };
+  apply(set, subject1, [testValue]);
+  if (subject1[prop] !== testValue) {
+    // The setter called with an unrelated object as `this` should
+    // set the property on the object.
+    return false;
+  }
+  const subject2 = { __proto__: obj };
+  apply(set, subject2, [testValue]);
+  if (subject2[prop] !== testValue) {
+    // The setter called on an object that inherits from `obj` should
+    // override the property from `obj` as if by assignment.
+    return false;
+  }
+  if (!throws(() => apply(set, obj, [expectedValue]))) {
+    // The setter called with `this === obj` should throw without having
+    // caused any effect.
+    // This is the test that has the greatest danger of leaving behind some
+    // persistent side effect. The most obvious one is to emulate a
+    // successful assignment to the property. That's why this test
+    // uses `expectedValue`, so that case is likely not to actually
+    // change anything.
+    return false;
+  }
+  if ('originalValue' in get) {
+    // The ses-shim uniquely, as far as we know, puts an `originalValue`
+    // property on the getter, so that reflect property tranversal algorithms,
+    // like `harden`, will traverse into the enulated value without
+    // calling the getter. That does not happen until `permits-intrinsics.js`
+    // which is much later. So if we see one this early, we should
+    // not assume we understand what's going on.
+    //
+    return false;
+  }
+
+  // We assume that this code runs before any untrusted code runs, so
+  // we do not need to worry about the above conditions passing because of
+  // malicious intent. In fact, it runs even before vetted shims are supposed
+  // to run, between repair and hardening. Given that, after all these tests
+  // pass, we have adequately validated that the property in question is
+  // an accessor function whose purpose is suppressing the override mistake,
+  // i.e., enabling a non-writable property to be overridden by assignment.
+  // In that case, here we *temporarily* turn it into the data property
+  // it seems to emulate, but writable so that it does not trigger the
+  // override mistake while in this temporary state.
+
+  // For those properties that are also listed in `enablements.js`,
+  // that phase will re-enable override for these properties, but
+  // via accessor functions that ses controls, so we know what they are
+  // doing. In addition, the getter functions installed by
+  // `enable-property-overrides.js` have an `originalValue` field
+  // enabling meta-traversal code like harden to visit the original value
+  // without calling the getter.
+
+  if (desc.configurable === false) {
+    // Even though it seems to be a faux data property, we're unable to fix it.
+    return false;
+  }
+
+  // Many of the `return false;` cases above plausibly should be turned into
+  // errors, or an least generate warnings. However, for those, the checks
+  // following this phase are likely to signal an error anyway.
+
+  // At this point, we have passed all our sniff checks for validating that
+  // it seems to be a faux data property with the expected value. Turn
+  // it into the actual data property it emulates, but writable so there is
+  // not yet an override mistake problem.
+
+  defineProperty(obj, prop, {
+    value: expectedValue,
+    writable: true,
+    enumerable: desc.enumerable,
+    configurable: true,
+  });
+
+  return true;
+};
+
+/**
+ * In JavaScript, the so-called "override mistake" is the inability to
+ * override an inherited non-writable data property by assignment. A common
+ * workaround is to instead define an accessor property that acts like
+ * a non-writable data property, except that it allows an object that
+ * inherits this property to override it by assignment. Let's call
+ * an access property that acts this way a "faux data property". In this
+ * ses-shim, `enable-property-overrides.js` makes the properties listed in
+ * `enablements.js` into faux data properties.
+ *
+ * But the ses-shim is not the only one to use this trick. Starting with the
+ * [iterators-helpers proposal](https://github.com/tc39/proposal-iterator-helpers),
+ * some properties are defined as (what we call) faux data properties.
+ * Some of these are new properties (`Interator.prototype.constructor`) and
+ * some are old data properties converted to accessor properties
+ * (`Iterator.prototype[String.toStringTag]`). So the ses shim needs to be
+ * prepared for some enumerated set of properties to already be faux data
+ * properties in the platform prior to the ses-shim initialization.
+ *
+ * For these possible faux data properties, it is important that
+ * `permits.js` describe these as if they are a data property, so that
+ * it can further constrain the apparent value (the value
+ * that allegedly would be returned by the getter) according to its own permits.
+ *
+ * However, at the time of this writing, the precise behavior specified
+ * by the iterators-helpers proposal for these faux data properties is
+ * novel. We should not be too confident that all further such platform
+ * additions do what we would now expect. So, for each of these possible
+ * faux data properties, we do some sniffing to see if it behaves as we
+ * currently expect a faux data property to act. If not, then
+ * `tameFauxDataProperties` tries not to modify it, leaving it to later
+ * checks, especially `permits-intrinsics.js`, to error when it sees an
+ * accessor is expected.
+ *
+ * If one of these enumerated accessor properties does seem to be
+ * a faithful faux data property, then `tameFauxDataProperties` itself
+ * *tempoarily* turns it into the actual data property that it seems to emulate.
+ * This data property starts with writable, so that in this state it will
+ * not trigger the override mistake, i.e., assignment to an object inheriting
+ * this property is allowed to succeed at overriding this property.
+ *
+ * For those properties that should be a faux data property rather than an
+ * actual one, such as those from the iterators-helpers proposal,
+ * they should be listed as such in `enablements.js`, so
+ * `enable-property-overrides.js` will turn it back into a faux data property.
+ * But one controlled by the ses-shim, whose behavior we understand.
+ *
+ * `tameFauxDataProperties`, which turns these into actual data properties,
+ * happens during the `repairIntrinsics` phase
+ * of `lockdown`, before even vetted shim are supposed to run.
+ * `enable-property-overrides.js` runs after vetted shims, turning the
+ * appropriate ones back into faux data properties. Thus vetted shims
+ * can observe the possibly non-conforming state where these are temporarily
+ * actual data properties, rather than faux data properties.
+ *
+ * Coordinate the property enumeration here
+ * with `enablements.js`, so the appropriate properties are
+ * turned back to faux data properties.
+ *
+ * @param {Record<any,any>} intrinsics
+ */
+export const tameFauxDataProperties = intrinsics => {
+  // https://github.com/tc39/proposal-iterator-helpers
+  tameFauxDataProperty(
+    intrinsics['%IteratorPrototype%'],
+    'constructor',
+    intrinsics.Iterator,
+  );
+  // https://github.com/tc39/proposal-iterator-helpers
+  tameFauxDataProperty(
+    intrinsics['%IteratorPrototype%'],
+    toStringTagSymbol,
+    'Iterator',
+  );
+};
diff --git a/packages/ses/test/test-tame-faux-data-properties.js b/packages/ses/test/test-tame-faux-data-properties.js
new file mode 100644
index 0000000000..f60261c80e
--- /dev/null
+++ b/packages/ses/test/test-tame-faux-data-properties.js
@@ -0,0 +1,135 @@
+import test from 'ava';
+import '../index.js';
+
+import { tameFauxDataProperty as tfdp } from '../src/tame-faux-data-properties.js';
+
+const { freeze, defineProperty, getOwnPropertyDescriptor } = Object;
+
+test('unit test tameFauxDataProperty', t => {
+  t.is(tfdp(undefined, 'foo', 'bar'), false, 'the object does not exist');
+  t.is(tfdp({}, 'foo', 'bar'), false, 'the property does not exist');
+  t.is(tfdp({ foo: 'bar' }, 'foo', 'bar'), false, 'an actual data property');
+
+  t.is(
+    tfdp(
+      {
+        get foo() {
+          return 'bar';
+        },
+      },
+      'foo',
+      'bar',
+    ),
+    false,
+    'a getter without a setter',
+  );
+
+  t.is(
+    tfdp({
+      get foo() {
+        return 'bar';
+      },
+      set foo(_newValue) {
+        throw TypeError('always throws');
+      },
+    }),
+    false,
+    'setter should not always throw',
+  );
+
+  t.is(
+    tfdp({
+      get foo() {
+        return 'bar';
+      },
+      set foo(_newValue) {
+        // never throws
+      },
+    }),
+    false,
+    'setter should throw when "this === obj"',
+  );
+
+  const subject1 = {
+    get foo() {
+      return 'bar';
+    },
+    set foo(_newValue) {
+      if (this === subject1) {
+        throw TypeError('throws only when it should');
+      }
+    },
+  };
+  t.is(tfdp(subject1, 'foo', 'bar'), false, 'does not assign when it should');
+
+  const subject2 = {
+    get foo() {
+      return 'bar';
+    },
+    set foo(newValue) {
+      defineProperty(this, 'foo', { value: newValue });
+    },
+  };
+  t.is(
+    tfdp(subject2, 'foo', 'bar'),
+    false,
+    'setter must fail when "this === obj"',
+  );
+
+  const subject3 = {
+    get foo() {
+      return 'bar';
+    },
+    set foo(newValue) {
+      if (this === subject3) {
+        throw TypeError('throws only when it should');
+      }
+      defineProperty(this, 'foo', { value: newValue });
+    },
+  };
+  t.is(
+    tfdp(freeze(subject3), 'foo', 'bar'),
+    false,
+    'genuine faux data property, but non-configurable so we cannot change it anyway',
+  );
+
+  const subject4 = {
+    get foo() {
+      return 'zip';
+    },
+    set foo(newValue) {
+      if (this === subject4) {
+        throw TypeError('throws only when it should');
+      }
+      defineProperty(this, 'foo', { value: newValue });
+    },
+  };
+  t.is(
+    tfdp(subject4, 'foo', 'bar'),
+    false,
+    'genuine faux data property, but not the expected value',
+  );
+
+  const desc4 = getOwnPropertyDescriptor(subject4, 'foo');
+  t.deepEqual(
+    desc4,
+    {
+      get: desc4.get,
+      set: desc4.set,
+      enumerable: true,
+      configurable: true,
+    },
+    'what the faux data property looks like',
+  );
+  t.is(tfdp(subject4, 'foo', 'zip'), true, 'changed into actual data prop');
+  t.deepEqual(
+    getOwnPropertyDescriptor(subject4, 'foo'),
+    {
+      value: 'zip',
+      writable: true,
+      enumerable: true,
+      configurable: true,
+    },
+    'what the resulting actual data property looks like',
+  );
+});