fix: use string instead of symbol for getter property

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

@@ -1,15 +1,16 @@
 // Adapted from SES/Caja
 // Copyright (C) 2011 Google Inc.
 
+// @ts-check
+
 import {
   defineProperty,
   getOwnPropertyNames,
   getOwnPropertyDescriptor,
   getOwnPropertyDescriptors,
+  objectHasOwnProperty,
 } from './commons.js';
 
-// https://github.com/google/caja/blob/master/src/com/google/caja/ses/startSES.js
-// https://github.com/google/caja/blob/master/src/com/google/caja/ses/repairES5.js
 import enablements from './enablements.js';
 
 const { ownKeys } = Reflect;
@@ -19,35 +20,56 @@ function isObject(obj) {
 }
 
 /**
- * This symbol names a property that we're adding to getters that
- *    * identifies that the getter alleges that it was created to suppress the
- *      override mistake.
- *    * provides the original data property value as its value.
- *
- * This is intended for expert use and avoidance of accidental collision with
- * normal code. But we are not trying to hide the symbol beyond that. Thus, we
- * make it a registered symbol that can be reconstructed anywhere using the code
- * `Symbol.for('originalValue')`. We no longer export it, so that's the most
- * convenient way to obtain the symbol.
- */
-const originalValueSymbol = Symbol.for('originalValue');
-
-/**
- * For a special set of properties (defined in the enablement plan), it ensures
- * that the effect of freezing does not suppress the ability to override
- * these properties on derived objects by simple assignment.
+ * For a special set of properties defined in the `enablement` whitelist,
+ * `enablePropertyOverrides` ensures that the effect of freezing does not
+ * suppress the ability to override these properties on derived objects by
+ * simple assignment.
  *
  * Because of lack of sufficient foresight at the time, ES5 unfortunately
  * specified that a simple assignment to a non-existent property must fail if
- * it would override a non-writable data property of the same name. (In
- * retrospect, this was a mistake, but it is now too late and we must live
- * with the consequences.) As a result, simply freezing an object to make it
- * tamper proof has the unfortunate side effect of breaking previously correct
- * code that is considered to have followed JS best practices, if this
- * previous code used assignment to override.
+ * it would override a non-writable data property of the same name. In
+ * retrospect, this was a mistake, the so-called "override mistake". But it is
+ * now too late and we must live with the consequences.
+ *
+ * As a result, simply freezing an object to make it tamper proof has the
+ * unfortunate side effect of breaking previously correct code that is
+ * considered to have followed JS best practices, if this previous code used
+ * assignment to override.
+ *
+ * For the enabled properties, `enablePropertyOverrides` effectively shims what
+ * the assignment behavior would have been in the absence of the override
+ * mistake. However, the shim produces an imperfect emulation. It shims the
+ * behavior by turning these data properties into accessor properties, where
+ * the accessor's getter and setter provide the desired behavior. For
+ * non-reflective operations, the illusion is perfect. However, reflective
+ * operations like `getOwnPropertyDescriptor` see the descriptor of an accessor
+ * property rather than the descriptor of a data property. At the time of this
+ * writing, this is the best we know how to do.
+ *
+ * To the getter of the accessor we add a property named
+ * `'originalDataPropertyValue'` whose value is, as it says, the value that the
+ * data property had before being converted to an accessor property. We add
+ * this extra property to the getter for two reason:
+ *
+ * The harden algorithm walks the own properties reflectively, i.e., with
+ * `getOwnPropertyDescriptor` semantics, rather than `[[Get]]` semantics. When
+ * it sees an accessor property, it does not invoke the getter. Rather, it
+ * proceeds to walk both the getter and setter as part of its transitive
+ * traversal. Without this extra property, `enablePropertyOverrides` would have
+ * hidden the original data property value from `harden`, which would be bad.
+ * Instead, by exposing that value in an own data property on the getter,
+ * `harden` finds and walks it anyway.
+ *
+ * We enable a form of cooperative emulation, giving reflective code an
+ * opportunity to cooperate in upholding the illusion. When such cooperative
+ * reflective code sees an accessor property, where the accessor's getter
+ * has an `originalDataPropertyValue` property, it knows that the getter is
+ * alleging that it is the result of the `enablePropertyOverrides` conversion
+ * pattern, so it can decide to cooperatively "pretend" that it sees a data
+ * property with that value.
+ *
+ * @param {Record<string, any>} intrinsics
  */
-
-// TODO exmplain parameters
 export default function enablePropertyOverrides(intrinsics) {
   function enable(path, obj, prop, desc) {
     if ('value' in desc && desc.configurable) {
@@ -56,21 +78,15 @@ export default function enablePropertyOverrides(intrinsics) {
       function getter() {
         return value;
       }
-      // The presence of this symbol on an accessor's getter alleges that
-      // this accessor is a converted data property whose original value
-      // is the value of that property. By exposing it, the harden algorithm
-      // on encountering the getter will harden the getter function and also
-      // this value. Thereby preserving the same transitivity that harden would
-      // have had on the original data-property graph.
-      getter[originalValueSymbol] = value;
+      getter.originalDataPropertyValue = value;
 
       function setter(newValue) {
         if (obj === this) {
           throw new TypeError(
             `Cannot assign to read only property '${prop}' of '${path}'`,
           );
         }
-        if (hasOwnProperty.call(this, prop)) {
+        if (objectHasOwnProperty(this, prop)) {
           this[prop] = newValue;
         } else {
           defineProperty(this, prop, {

packages/ses/test/frozen-primordials.test.js

@@ -2,20 +2,15 @@ import test from 'tape';
 import '../lockdown.js';
 import { getOwnPropertyDescriptor } from '../src/commons.js';
 
-const originalValueSymbol = Symbol.for('originalValue');
-
 test('check if override-protected primordials are frozen', t => {
   lockdown();
 
-  // After removing the detachedProperties mechanism and before adding
-  // the originalValueSymbol mechanism, this test failed.
+  // After removing the detachedProperties mechanism and without
+  // the originalDataPropertyValue mechanism, this test failed.
   t.ok(Object.isFrozen(Object.prototype.toString));
 
-  // Just checking that reconstruction produces the *same* symbol
-  t.equals(originalValueSymbol, Symbol.for('originalValue'));
-
   const desc = getOwnPropertyDescriptor(Object.prototype, 'toString');
-  t.equals(desc.get[originalValueSymbol], Object.prototype.toString);
+  t.equals(desc.get.originalDataPropertyValue, Object.prototype.toString);
 
   t.end();
 });