fix(ses): regexp taming

Bad RegExp taming revealed by #237 Exclude similar but new test failures
endojs · Mar 23, 2020 · b010f8a · b010f8a
1 parent f0f42f2
commit b010f8a
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 11 deletions.
diff --git a/packages/ses/src/tame-global-reg-exp-object.js b/packages/ses/src/tame-global-reg-exp-object.js
@@ -1,8 +1,4 @@
-const {
-  defineProperties,
-  getOwnPropertyDescriptors,
-  getOwnPropertyDescriptor,
-} = Object;
+const { defineProperties, getOwnPropertyDescriptor } = Object;
 
 export default function tameGlobalRegExpObject() {
   // Tame the %RegExp% intrinsic.
@@ -22,13 +18,17 @@ export default function tameGlobalRegExpObject() {
   };
 
   // Whitelist static properties.
+  // See https://github.com/Agoric/SES-shim/issues/239
   const desc = getOwnPropertyDescriptor(unsafeRegExp, Symbol.species);
   defineProperties(tamedRegExp, Symbol.species, desc);
 
-  // Copy prototype properties.
-  const prototypeDescs = getOwnPropertyDescriptors(unsafeRegExp.prototype);
-  prototypeDescs.constructor.value = tamedRegExp;
-  defineProperties(tamedRegExp.prototype, prototypeDescs);
+  const RegExpPrototype = unsafeRegExp.prototype;
+  defineProperties(tamedRegExp, {
+    prototype: { value: RegExpPrototype },
+  });
+  defineProperties(RegExpPrototype, {
+    constructor: { value: tamedRegExp },
+  });
 
   // Done with RegExp constructor.
   globalThis.RegExp = tamedRegExp;

diff --git a/packages/ses/test/tame-global-reg-exp-object.test.js b/packages/ses/test/tame-global-reg-exp-object.test.js
@@ -4,9 +4,60 @@ import tameGlobalRegExpObject from '../src/tame-global-reg-exp-object.js';
 
 const { test } = tap;
 
-test('tameGlobalRegExpObject - constructor', t => {
-  t.plan(8);
+test('tameGlobalRegExpObject - unsafeRegExp denied', t => {
+  const unsafeRegExp = RegExp;
+  const restore = captureGlobals('RegExp');
+  tameGlobalRegExpObject();
 
+  t.ok(unsafeRegExp !== RegExp, 'constructor not replaced');
+  const regexp = /./;
+  t.ok(regexp.constructor === RegExp, 'tamed constructor not reached');
+  // Don't leak the unsafe constructor
+  // https://github.com/Agoric/SES-shim/issues/237
+  t.ok(regexp.constructor !== unsafeRegExp, 'unsafe constructor reachable!');
+
+  restore();
+  t.end();
+});
+
+test('tameGlobalRegExpObject - undeniable prototype', t => {
+  const restore = captureGlobals('RegExp');
+  tameGlobalRegExpObject();
+
+  // Don't try to deny the undeniable
+  // https://github.com/Agoric/SES-shim/issues/237
+  const regexp1 = new RegExp('.');
+  const regexp2 = RegExp('.');
+  const regexp3 = /./;
+  t.ok(
+    // eslint-disable-next-line no-proto
+    regexp1.__proto__ === regexp2.__proto__,
+    'new vs non-new instances differ',
+  );
+  t.ok(
+    // eslint-disable-next-line no-proto
+    regexp1.__proto__ === regexp3.__proto__,
+    'new vs literal instances differ',
+  );
+
+  t.ok(
+    regexp1 instanceof RegExp,
+    'new instance not instanceof tamed constructor',
+  );
+  t.ok(
+    regexp2 instanceof RegExp,
+    'non-new instance not instanceof tamed constructor',
+  );
+  t.ok(
+    regexp3 instanceof RegExp,
+    'literal instance not instanceof tamed constructor',
+  );
+
+  restore();
+  t.end();
+});
+
+test('tameGlobalRegExpObject - constructor', t => {
   const restore = captureGlobals('RegExp');
   tameGlobalRegExpObject();
 
@@ -20,6 +71,7 @@ test('tameGlobalRegExpObject - constructor', t => {
   const properties = Reflect.ownKeys(RegExp);
   t.deepEqual(
     properties.sort(),
+    // See https://github.com/Agoric/SES-shim/issues/239
     ['length', 'name', 'prototype'].sort(),
     'RegExp should not have static properties',
   );
@@ -35,4 +87,5 @@ test('tameGlobalRegExpObject - constructor', t => {
   t.equal(RegExp('foo').test('foobar'), true);
 
   restore();
+  t.end();
 });
diff --git a/packages/ses/test262/tame-global-regexp-object.js b/packages/ses/test262/tame-global-regexp-object.js
@@ -23,8 +23,11 @@ test262Runner({
     'test/built-ins/RegExp/prototype/Symbol.split/last-index-exceeds-str-size.js',
     'test/built-ins/RegExp/prototype/test/S15.10.6.3_A9.js',
     'test/built-ins/RegExp/prototype/unicode/this-val-regexp-prototype.js',
+    'test/built-ins/RegExp/S15.10.3.1_A1_T1.js',
     'test/built-ins/RegExp/S15.10.3.1_A1_T2.js',
     'test/built-ins/RegExp/S15.10.3.1_A1_T3.js',
+    'test/built-ins/RegExp/S15.10.3.1_A1_T4.js',
+    'test/built-ins/RegExp/S15.10.3.1_A1_T5.js',
     'test/built-ins/RegExp/S15.10.3.1_A2_T2.js',
     'test/built-ins/RegExp/S15.10.3.1_A3_T1.js',
     'test/built-ins/RegExp/S15.10.3.1_A3_T2.js',