feat(ses): review suggestions

packages/ses/src/lockdown-shim.js

@@ -264,10 +264,6 @@ export const repairIntrinsics = (options = {}) => {
 
   tameDomains(domainTaming);
 
-  const SharedSymbol = tameSymbolConstructor();
-  // Must happen before `makeIntrinsicsCollector()`
-  globalThis.Symbol = SharedSymbol;
-
   const { addIntrinsics, completePrototypes, finalIntrinsics } =
     makeIntrinsicsCollector();
 
@@ -280,6 +276,7 @@ export const repairIntrinsics = (options = {}) => {
   addIntrinsics(tameErrorConstructor(errorTaming, stackFiltering));
   addIntrinsics(tameMathObject(mathTaming));
   addIntrinsics(tameRegExpConstructor(regExpTaming));
+  addIntrinsics(tameSymbolConstructor());
 
   addIntrinsics(getAnonymousIntrinsics());
 

packages/ses/src/tame-symbol-constructor.js

@@ -8,27 +8,30 @@ import {
 } from './commons.js';
 
 /**
- * This taming replaces the original `Symbol` constructor with one that seems
- * identical, except that all its properties are "temporarily" configurable.
- * This assumes two succeeding phases of processing: A whitelisting phase, that
+ * This taming provides a tamed alternative to the original `Symbol` constructor
+ * that starts off identical, except that all its properties are "temporarily"
+ * configurable. The original `Symbol` constructor remains unmodified on
+ * the start compartment's global. The tamed alternative is used as the shared
+ * `Symbol` constructor on constructed compartments.
+ *
+ * Starting these properties as configurable assumes two succeeding phases of
+ * processing: A whitelisting phase, that
  * removes all properties not on the whitelist (which requires them to be
  * configurable) and a global hardening step that freezes all primordials,
- * returning these properties to their non-configurable status.
+ * returning these properties to their expected non-configurable status.
  *
- * However, the ses shim is constructed to enable vetter shims to run between
- * repair and global hardening. Such vetter shims will see the replacement
- * `Symbol` constructor with any "extra" properties that the whitelisting will
- * remove, and with the well-known-symbol properties being configurable, in
- * violation of the JavaScript spec.
+ * The ses shim is constructed to eventually enable vetted shims to run between
+ * repair and global hardening. However, such vetted shims would normally
+ * run in the start compartment, which continues to use the original unmodified
+ * `Symbol`, so they should not normally be affected by the remporary
+ * configurability of these properties.
  *
  * Note that the spec refers to the global `Symbol` function as the
  * ["Symbol Constructor"](https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-symbol-constructor)
  * even though it has a call behavior (can be called as a function) and does not
  * not have a construct behavior (cannot be called with `new`). Accordingly,
  * to tame it, we must replace it with a function without a construct
  * behavior.
- *
- * @returns {SymbolConstructor}
  */
 export const tameSymbolConstructor = () => {
   const OriginalSymbol = Symbol;
@@ -58,5 +61,5 @@ export const tameSymbolConstructor = () => {
   );
   defineProperties(SharedSymbol, descs);
 
-  return /** @type {SymbolConstructor} */ (SharedSymbol);
+  return { '%SharedSymbol%': SharedSymbol };
 };

packages/ses/src/whitelist-intrinsics.js

@@ -47,6 +47,7 @@ import { whitelist, FunctionInstance, isAccessorPermit } from './whitelist.js';
 import {
   Map,
   String,
+  Symbol,
   TypeError,
   arrayFilter,
   arrayIncludes,
@@ -78,15 +79,14 @@ export default function whitelistIntrinsics(
 
   // These symbols are allowed as well-known symbols
   const wellKnownSymbolNames = new Map(
-    intrinsics.Symbol
+    Symbol
       ? arrayMap(
           arrayFilter(
-            entries(whitelist.Symbol),
+            entries(whitelist['%SharedSymbol%']),
             ([name, permit]) =>
-              permit === 'symbol' &&
-              typeof intrinsics.Symbol[name] === 'symbol',
+              permit === 'symbol' && typeof Symbol[name] === 'symbol',
           ),
-          ([name]) => [intrinsics.Symbol[name], `@@${name}`],
+          ([name]) => [Symbol[name], `@@${name}`],
         )
       : [],
   );

packages/ses/src/whitelist.js

@@ -67,7 +67,6 @@ export const universalPropertyNames = {
   ReferenceError: 'ReferenceError',
   Set: 'Set',
   String: 'String',
-  Symbol: 'Symbol',
   SyntaxError: 'SyntaxError',
   TypeError: 'TypeError',
   Uint8Array: 'Uint8Array',
@@ -109,6 +108,11 @@ export const initialGlobalPropertyNames = {
   Error: '%InitialError%',
   RegExp: '%InitialRegExp%',
 
+  // Omit `Symbol`, because we want the original to appear on the
+  // start compartment without passing through the whitelist mechanism, since
+  // we want to preserve all its properties, even if we never heard of them.
+  // Symbol: '%InitialSymbol%',
+
   // *** Other Properties of the Global Object
 
   Math: '%InitialMath%',
@@ -137,6 +141,7 @@ export const sharedGlobalPropertyNames = {
   Date: '%SharedDate%',
   Error: '%SharedError%',
   RegExp: '%SharedRegExp%',
+  Symbol: '%SharedSymbol%',
 
   // *** Other Properties of the Global Object
 
@@ -492,7 +497,7 @@ export const whitelist = {
     valueOf: fn,
   },
 
-  Symbol: {
+  '%SharedSymbol%': {
     // Properties of the Symbol Constructor
     '[[Proto]]': '%FunctionPrototype%',
     asyncDispose: 'symbol',
@@ -517,7 +522,7 @@ export const whitelist = {
 
   '%SymbolPrototype%': {
     // Properties of the Symbol Prototype Object
-    constructor: 'Symbol',
+    constructor: '%SharedSymbol%',
     description: getter,
     toString: fn,
     valueOf: fn,

packages/ses/test/test-get-global-intrinsics.js

@@ -50,7 +50,7 @@ test.skip('getGlobalIntrinsics', () => {
     'Set',
     // 'SharedArrayBuffer'  // removed on Jan 5, 2018
     'String',
-    'Symbol',
+    // 'Symbol', original left in place, but omitted from whitelist
     'SyntaxError',
     'TypeError',
     'Uint8Array',

packages/ses/test/test-tame-symbol-constructor.js

@@ -1,7 +1,12 @@
 import test from 'ava';
 import '../index.js';
 
-import { defineProperty, getOwnPropertyDescriptor } from '../src/commons.js';
+import {
+  defineProperty,
+  getOwnPropertyDescriptor as gopd,
+  isExtensible,
+  isFrozen,
+} from '../src/commons.js';
 
 defineProperty(Symbol, 'dummy', {
   value: Symbol.for('faux well known symbol'),
@@ -10,13 +15,25 @@ defineProperty(Symbol, 'dummy', {
   configurable: false,
 });
 
-// Since Symbol.dummy is not even mentioned on the whitelist, this test should
-// also print
-// "Removing intrinsics.Symbol.dummy"
-// on the console.
+// Since %SharedSymbol%.dummy is not even mentioned on the whitelist,
+// this test should also print on the console:
+// > Removing intrinsics.%SharedSymbol%.dummy
 lockdown();
 
 test('Symbol cleaned by whitelist', t => {
-  t.false('dummy' in Symbol);
-  t.false(getOwnPropertyDescriptor(Symbol, 'iterator').configurable);
+  t.true('dummy' in Symbol);
+  t.false(gopd(Symbol, 'iterator').configurable);
+  t.true(isExtensible(Symbol));
+  t.false(isFrozen(Symbol));
+  t.not(Symbol.constructor, Symbol);
+
+  const c = new Compartment();
+  const SharedSymbol = c.globalThis.Symbol;
+  t.is(Symbol.prototype, SharedSymbol.prototype);
+
+  t.false('dummy' in SharedSymbol);
+  t.false(gopd(SharedSymbol, 'iterator').configurable);
+  t.false(isExtensible(SharedSymbol));
+  t.true(isFrozen(SharedSymbol));
+  t.is(SharedSymbol.prototype.constructor, SharedSymbol);
 });

packages/ses/test/test-whitelist-intrinsics.js

@@ -7,7 +7,11 @@ if (!eval.toString().includes('native code')) {
   throw TypeError('Module "esm" enabled: aborting');
 }
 
-test('whitelistIntrinsics - Well-known symbols', t => {
+// This test as stated no longer makes sense, now that `whitelistIntrinsics`
+// decodes symbol strings using the real `globalThis.Symbol` rather than
+// `intrinsics.Symbol`. But I left this in as a `test.skip` in case anyone
+// wants to rescure some of the purpose of this test.
+test.skip('whitelistIntrinsics - Well-known symbols', t => {
   const SymbolIterator = Symbol('Symbol.iterator');
   const RogueSymbolIterator = Symbol('Symbol.iterator');
   const ArrayProto = { [RogueSymbolIterator]() {} };