fixup! reveiwer suggestions

packages/non-trapping-shim/README.md

@@ -0,0 +1,12 @@
+# Ponyfill and Shim for Non-trapping Integrity Trait
+
+Emulates support for the non-trapping integrity trait from the
+[Stabilize proposal](https://github.com/tc39/proposal-stabilize).
+
+A *shim* attempts to be as full fidelity as practical an emulation of the proposal itself. This is sometimes called a "polyfill". A *ponyfill* provides the functionality of the shim, but sacrifices fidelity of the API in order to avoid monkey patching the primordials. Confusingly, this is also sometimes called a "polyfill", which is why we avoid that ambiguous term.
+
+A shim typically "exports" its functionality by adding properties to primordial objects. A ponyfill typically exports its functionality by explicit module exports, to be explicitly imported by code wishing to use it.
+
+See https://github.com/endojs/endo/blob/master/packages/ses/docs/preparing-for-stabilize.md for guidance on how to prepare for the changes that will be introduced by this proposal.
+
+TODO: More explanation.

packages/non-trapping-shim/index.js

@@ -0,0 +1 @@
+export * from './src/non-trapping-pony.js';

packages/no-trapping-shim/package.json → packages/non-trapping-shim/package.json

@@ -1,16 +1,16 @@
 {
-  "name": "@endo/no-trapping-shim",
+  "name": "@endo/non-trapping-shim",
   "version": "0.1.0",
   "private": true,
-  "description": "shim and ponyfill for no-trapping integrity level",
+  "description": "shim and ponyfill for non-trapping integrity trait",
   "keywords": [],
   "author": "Endo contributors",
   "license": "Apache-2.0",
   "homepage": "https://github.com/endojs/endo/tree/master/packages/skel#readme",
   "repository": {
     "type": "git",
     "url": "git+https://github.com/endojs/endo.git",
-    "directory": "packages/no-trapping-shim"
+    "directory": "packages/non-trapping-shim"
   },
   "bugs": {
     "url": "https://github.com/endojs/endo/issues"

packages/no-trapping-shim/shim.js → packages/non-trapping-shim/shim.js

@@ -1,5 +1,5 @@
 /* global globalThis */
-import { ReflectPlus, ObjectPlus, ProxyPlus } from './src/no-trapping-pony.js';
+import { ReflectPlus, ObjectPlus, ProxyPlus } from './src/non-trapping-pony.js';
 
 globalThis.Reflect = ReflectPlus;
 

packages/no-trapping-shim/src/no-trapping-pony.js → packages/non-trapping-shim/src/non-trapping-pony.js

@@ -4,48 +4,48 @@ const OriginalProxy = Proxy;
 const { freeze, defineProperty, hasOwn } = OriginalObject;
 const { apply, construct, ownKeys } = OriginalReflect;
 
-const noTrappingSet = new WeakSet();
+const nonTrappingSet = new WeakSet();
 
 const proxyHandlerMap = new WeakMap();
 
 const isPrimitive = specimen => OriginalObject(specimen) !== specimen;
 
 /**
- * Corresponds to the internal function shared by `Object.isNoTrapping` and
- * `Reflect.isNoTrapping`.
+ * Corresponds to the internal function shared by `Object.isNonTrapping` and
+ * `Reflect.isNonTrapping`.
  *
  * @param {any} specimen
  * @param {boolean} shouldThrow
  * @returns {boolean}
  */
-const isNoTrappingInternal = (specimen, shouldThrow) => {
-  if (noTrappingSet.has(specimen)) {
+const isNonTrappingInternal = (specimen, shouldThrow) => {
+  if (nonTrappingSet.has(specimen)) {
     return true;
   }
   if (!proxyHandlerMap.has(specimen)) {
     return false;
   }
   const [target, handler] = proxyHandlerMap.get(specimen);
-  if (isNoTrappingInternal(target, shouldThrow)) {
-    noTrappingSet.add(specimen);
+  if (isNonTrappingInternal(target, shouldThrow)) {
+    nonTrappingSet.add(specimen);
     return true;
   }
-  const trap = handler.isNoTrapping;
+  const trap = handler.isNonTrapping;
   if (trap === undefined) {
     return false;
   }
   const result = apply(trap, handler, [target]);
-  const ofTarget = isNoTrappingInternal(target, shouldThrow);
+  const ofTarget = isNonTrappingInternal(target, shouldThrow);
   if (result !== ofTarget) {
     if (shouldThrow) {
       throw TypeError(
-        `'isNoTrapping' proxy trap does not reflect 'isNoTrapping' of proxy target (which is '${ofTarget}')`,
+        `'isNonTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
       );
     }
     return false;
   }
   if (result) {
-    noTrappingSet.add(specimen);
+    nonTrappingSet.add(specimen);
   }
   return result;
 };
@@ -59,49 +59,49 @@ const isNoTrappingInternal = (specimen, shouldThrow) => {
  * @returns {boolean}
  */
 const suppressTrappingInternal = (specimen, shouldThrow) => {
-  if (noTrappingSet.has(specimen)) {
+  if (nonTrappingSet.has(specimen)) {
     return true;
   }
   freeze(specimen);
   if (!proxyHandlerMap.has(specimen)) {
-    noTrappingSet.add(specimen);
+    nonTrappingSet.add(specimen);
     return true;
   }
   const [target, handler] = proxyHandlerMap.get(specimen);
-  if (isNoTrappingInternal(target, shouldThrow)) {
-    noTrappingSet.add(specimen);
+  if (isNonTrappingInternal(target, shouldThrow)) {
+    nonTrappingSet.add(specimen);
     return true;
   }
   const trap = handler.suppressTrapping;
   if (trap === undefined) {
     const result = suppressTrappingInternal(target, shouldThrow);
     if (result) {
-      noTrappingSet.add(specimen);
+      nonTrappingSet.add(specimen);
     }
     return result;
   }
   const result = apply(trap, handler, [target]);
-  const ofTarget = isNoTrappingInternal(target, shouldThrow);
+  const ofTarget = isNonTrappingInternal(target, shouldThrow);
   if (result !== ofTarget) {
     if (shouldThrow) {
       throw TypeError(
-        `'suppressTrapping' proxy trap does not reflect 'isNoTrapping' of proxy target (which is '${ofTarget}')`,
+        `'suppressTrapping' proxy trap does not reflect 'isNonTrapping' of proxy target (which is '${ofTarget}')`,
       );
     }
     return false;
   }
   if (result) {
-    noTrappingSet.add(specimen);
+    nonTrappingSet.add(specimen);
   }
   return result;
 };
 
 export const extraReflectMethods = freeze({
-  isNoTrapping(target) {
+  isNonTrapping(target) {
     if (isPrimitive(target)) {
-      throw TypeError('Reflect.isNoTrapping called on non-object');
+      throw TypeError('Reflect.isNonTrapping called on non-object');
     }
-    return isNoTrappingInternal(target, false);
+    return isNonTrappingInternal(target, false);
   },
   suppressTrapping(target) {
     if (isPrimitive(target)) {
@@ -112,11 +112,11 @@ export const extraReflectMethods = freeze({
 });
 
 export const extraObjectMethods = freeze({
-  isNoTrapping(target) {
+  isNonTrapping(target) {
     if (isPrimitive(target)) {
       return true;
     }
-    return isNoTrappingInternal(target, true);
+    return isNonTrappingInternal(target, true);
   },
   suppressTrapping(target) {
     if (isPrimitive(target)) {
@@ -125,7 +125,7 @@ export const extraObjectMethods = freeze({
     if (suppressTrappingInternal(target, true)) {
       return target;
     }
-    throw TypeError('preventExtensions trap returned falsy');
+    throw TypeError('suppressTrapping trap returned falsy');
   },
 });
 
@@ -167,6 +167,17 @@ ObjectPlus.prototype = OriginalObject.prototype;
 addExtras(ObjectPlus, OriginalObject, extraObjectMethods);
 export { ObjectPlus };
 
+/**
+ * A way to store the `originalHandler` on the `handlerPlus` without
+ * possible conflict with an future trap name.
+ *
+ * Normally, we'd use a WeakMap for this, so the property is also
+ * undiscoverable. But in this case, the `handlerPlus` objects are
+ * safely encapsulated within this module, so no one is in a position to
+ * discovery this property by inspection.
+ */
+const ORIGINAL_HANDLER = Symbol('OriginalHandler');
+
 const metaHandler = freeze({
   get(_, trapName, handlerPlus) {
     /**
@@ -182,7 +193,7 @@ const metaHandler = freeze({
      * @param {any[]} rest
      */
     const trapPlus = freeze((target, ...rest) => {
-      if (isNoTrappingInternal(target, true)) {
+      if (isNonTrappingInternal(target, true)) {
         defineProperty(handlerPlus, trapName, {
           value: undefined,
           writable: false,
@@ -198,7 +209,7 @@ const metaHandler = freeze({
             configurable: true,
           });
         }
-        const { originalHandler } = handlerPlus;
+        const { [ORIGINAL_HANDLER]: originalHandler } = handlerPlus;
         const trap = originalHandler[trapName];
         if (trap !== undefined) {
           // Note that whether `trap === undefined` can change dynamically,
@@ -225,27 +236,19 @@ const metaHandler = freeze({
  *
  * @param {ProxyHandler<any>} originalHandler
  * @returns {ProxyHandler<any> & {
- *   isNoTrapping: (target: any) => boolean,
+ *   isNonTrapping: (target: any) => boolean,
  *   suppressTrapping: (target: any) => boolean,
  *   originalHandler: ProxyHandler<any>
  * }}
  */
 const makeHandlerPlus = originalHandler => ({
   // @ts-expect-error TS does not know what this __proto__ is doing
   __proto__: new OriginalProxy({}, metaHandler),
-  // relies on there never being a trap named `originalHandler`.
-  originalHandler,
+  [ORIGINAL_HANDLER]: originalHandler,
 });
 
-/**
- * In the shim, `ProxyPlus` replaces the global `Proxy`.
- *
- * @type {ProxyConstructor}
- */
-// @ts-expect-error We reject non-new calls in the body
-const ProxyPlus = function Proxy(target, handler) {
-  // @ts-expect-error Yes, we mean to compare these.
-  if (new.target !== ProxyPlus) {
+const ProxyInternal = function Proxy(target, handler) {
+  if (new.target !== ProxyInternal) {
     if (new.target === undefined) {
       throw TypeError('Proxy constructor requires "new"');
     }
@@ -256,18 +259,26 @@ const ProxyPlus = function Proxy(target, handler) {
   proxyHandlerMap.set(proxy, [target, handler]);
   return proxy;
 };
-// The `OriginalProxy` is both constructible (i.e., responsive to `new`) and
-// lacks a `prototype` property. The closest we can come to this is to set
-// `ProxyPlus.prototype` to `undefined`
-ProxyPlus.prototype = undefined;
+
+/**
+ * In the shim, `ProxyPlus` replaces the global `Proxy`.
+ *
+ * We use `bind` as the only way for user code to produce a
+ * constructible function (i.e., one that responds to `new`) without a
+ * `.prototype` property.
+ *
+ * @type {ProxyConstructor}
+ */
+const ProxyPlus = ProxyInternal.bind(undefined);
+
 ProxyPlus.revocable = (target, handler) => {
   const handlerPlus = makeHandlerPlus(handler);
   const { proxy, revoke } = OriginalProxy.revocable(target, handlerPlus);
   proxyHandlerMap.set(proxy, [target, handler]);
   return {
     proxy,
     revoke() {
-      if (isNoTrappingInternal(target, true)) {
+      if (isNonTrappingInternal(target, true)) {
         throw TypeError('Cannot revoke non-trapping proxy');
       }
       revoke();

packages/no-trapping-shim/test/no-trapping-pony.test.js → packages/non-trapping-shim/test/non-trapping-pony.test.js

@@ -2,11 +2,11 @@
 // dependencies. We will need similar tests is higher level packages, in order
 // to test compat with ses and ses-ava.
 import test from 'ava';
-import { ReflectPlus, ProxyPlus } from '../src/no-trapping-pony.js';
+import { ReflectPlus, ProxyPlus } from '../src/non-trapping-pony.js';
 
 const { freeze, isFrozen } = Object;
 
-test('no-trapping-pony', async t => {
+test('non-trapping-pony', async t => {
   const specimen = { foo: 8 };
 
   const sillyHandler = freeze({
@@ -17,13 +17,13 @@ test('no-trapping-pony', async t => {
 
   const safeProxy = new ProxyPlus(specimen, sillyHandler);
 
-  t.false(ReflectPlus.isNoTrapping(specimen));
+  t.false(ReflectPlus.isNonTrapping(specimen));
   t.false(isFrozen(specimen));
   t.deepEqual(safeProxy.foo, [specimen, 'foo', safeProxy]);
 
   t.true(ReflectPlus.suppressTrapping(specimen));
 
-  t.true(ReflectPlus.isNoTrapping(specimen));
+  t.true(ReflectPlus.isNonTrapping(specimen));
   t.true(isFrozen(specimen));
   t.deepEqual(safeProxy.foo, 8);
 });

packages/no-trapping-shim/test/no-trapping-shim.test.js → packages/non-trapping-shim/test/non-trapping-shim.test.js

@@ -6,7 +6,7 @@ import '../shim.js';
 
 const { freeze, isFrozen } = Object;
 
-test('no-trapping-pony', async t => {
+test('non-trapping-pony', async t => {
   const specimen = { foo: 8 };
 
   const sillyHandler = freeze({
@@ -17,13 +17,13 @@ test('no-trapping-pony', async t => {
 
   const safeProxy = new Proxy(specimen, sillyHandler);
 
-  t.false(Reflect.isNoTrapping(specimen));
+  t.false(Reflect.isNonTrapping(specimen));
   t.false(isFrozen(specimen));
   t.deepEqual(safeProxy.foo, [specimen, 'foo', safeProxy]);
 
   t.true(Reflect.suppressTrapping(specimen));
 
-  t.true(Reflect.isNoTrapping(specimen));
+  t.true(Reflect.isNonTrapping(specimen));
   t.true(isFrozen(specimen));
   t.deepEqual(safeProxy.foo, 8);
 });

packages/ses/docs/preparing-for-stabilize.md

@@ -5,7 +5,7 @@ The [Stabilize proposal](https://github.com/tc39/proposal-stabilize) is currentl
 - ***overridable***: would mitigate the assignment-override mistake by enabling non-writable properties inherited from an object with this trait to be overridden by property assignment on an inheriting object.
 - ***non-trapping***: would mitigate proxy-based reentrancy hazards by having a proxy whose target carries this trait never trap to its handler, but rather just perform the default action directly on this non-trapping target.
 
-Draft PR [feat(no-trapping-shim): ponyfill and shim for the no-trapping integrity trait #2673](https://github.com/endojs/endo/pull/2673) is a ponyfill and shim for this non-trapping integrity trait. The names it introduces are placeholders, since the bikeshedding process for these names has not yet concluded.
+Draft PR [feat(non-trapping-shim): ponyfill and shim for the non-trapping integrity trait #2673](https://github.com/endojs/endo/pull/2673) is a ponyfill and shim for this non-trapping integrity trait. The names it introduces are placeholders, since the bikeshedding process for these names has not yet concluded.
 
 Draft PR [feat(ses,pass-style): use non-trapping integrity trait for safety #2675](https://github.com/endojs/endo/pull/2675) uses this support for the non-trapping integity trait to mitigate reentrancy attacks from hardened objects, expecially passable copy-data objects like copyLists, copyRecords, and taggeds. To do so, it makes two fundamental changes:
 - Where `harden` made the object at every step frozen, that PR changes `harden` to also make those objects non-trapping.

yarn.lock

@@ -702,9 +702,9 @@ __metadata:
   languageName: unknown
   linkType: soft
 
-"@endo/no-trapping-shim@workspace:packages/no-trapping-shim":
+"@endo/non-trapping-shim@workspace:packages/non-trapping-shim":
   version: 0.0.0-use.local
-  resolution: "@endo/no-trapping-shim@workspace:packages/no-trapping-shim"
+  resolution: "@endo/non-trapping-shim@workspace:packages/non-trapping-shim"
   dependencies:
     ava: "npm:^6.1.3"
     c8: "npm:^7.14.0"