fix(ses): handle properties already override protected

packages/ses/src/enable-property-overrides.js

@@ -11,7 +11,6 @@ import {
   defineProperty,
   getOwnPropertyDescriptor,
   getOwnPropertyDescriptors,
-  getOwnPropertyNames,
   isObject,
   objectHasOwnProperty,
   ownKeys,
@@ -148,23 +147,20 @@ 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.
         // eslint-disable-next-line no-continue
         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];
 

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

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.
    */

packages/ses/src/tame-faux-data-properties.js

@@ -0,0 +1,131 @@
+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}
+ */
+export const tameFauxDataProperty = (obj, prop, expectedValue) => {
+  if (obj === undefined) {
+    return false;
+  }
+  const desc = getOwnPropertyDescriptor(obj, prop);
+  if (!desc || 'value' in desc) {
+    return false;
+  }
+  const { get, set } = desc;
+  if (typeof get !== 'function') {
+    return false;
+  }
+  if (typeof set !== 'function') {
+    return false;
+  }
+  const observedValue = get();
+  if (observedValue !== expectedValue) {
+    return false;
+  }
+  if (apply(get, obj, []) !== expectedValue) {
+    return false;
+  }
+  const testValue = 'Seems to be a setter';
+  const subject1 = { __proto__: null };
+  apply(set, subject1, [testValue]);
+  if (subject1[prop] !== testValue) {
+    return false;
+  }
+  const subject2 = { __proto__: obj };
+  apply(set, subject2, [testValue]);
+  if (subject2[prop] !== testValue) {
+    return false;
+  }
+  if (!throws(() => apply(set, obj, [testValue]))) {
+    return false;
+  }
+  if ('originalValue' in get) {
+    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 an accessor, 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.
+
+  defineProperty(obj, prop, {
+    value: expectedValue,
+    writable: true,
+    enumerable: desc.enumerable,
+    configurable: true,
+  });
+
+  return true;
+};
+
+/**
+ * For each, see it the property is an accessor property that
+ * seems to emulate a data property with this expectedValue,
+ * and seems to be an accessor whose purpose is to protect against
+ * the override mistake, i.e., enable these properties to be overridden
+ * by assignment. If all these expected conditions are met, then
+ * *temporarily* turn it into the data property it emulated.
+ *
+ * Coordinate with `enablements.js` so the appropriate ones are
+ * turned back to accessor that protect against override mistake,
+ * but accessors we know.
+ *
+ * @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',
+  );
+};

packages/ses/test/test-tame-faux-data-properties.js

@@ -0,0 +1,112 @@
+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);
+  t.is(tfdp({}, 'foo', 'bar'), false);
+  t.is(tfdp({ foo: 'bar' }, 'foo', 'bar'), false);
+
+  t.is(
+    tfdp(
+      {
+        get foo() {
+          return 'bar';
+        },
+      },
+      'foo',
+      'bar',
+    ),
+    false,
+  );
+
+  t.is(
+    tfdp({
+      get foo() {
+        return 'bar';
+      },
+      set foo(_newValue) {
+        throw TypeError('always throws');
+      },
+    }),
+    false,
+  );
+
+  t.is(
+    tfdp({
+      get foo() {
+        return 'bar';
+      },
+      set foo(_newValue) {
+        // never throws
+      },
+    }),
+    false,
+  );
+
+  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);
+
+  const subject2 = {
+    get foo() {
+      return 'bar';
+    },
+    set foo(newValue) {
+      defineProperty(this, 'foo', { value: newValue });
+    },
+  };
+  t.is(tfdp(subject2, 'foo', 'bar'), false);
+
+  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);
+
+  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);
+
+  const desc4 = getOwnPropertyDescriptor(subject4, 'foo');
+  t.deepEqual(desc4, {
+    get: desc4.get,
+    set: desc4.set,
+    enumerable: true,
+    configurable: true,
+  });
+  t.is(tfdp(subject4, 'foo', 'zip'), true);
+  t.deepEqual(getOwnPropertyDescriptor(subject4, 'foo'), {
+    value: 'zip',
+    writable: true,
+    enumerable: true,
+    configurable: true,
+  });
+});