From 7bcce04d7d7c9e51e5d763e8244d59d9b4a36c6a Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Thu, 2 Jan 2025 15:06:25 -0800 Subject: [PATCH] fixup! reveiwer suggestions --- packages/no-trapping-shim/README.md | 1 - packages/no-trapping-shim/index.js | 1 - .../CHANGELOG.md | 0 .../LICENSE | 0 .../NEWS.md | 0 packages/non-trapping-shim/README.md | 12 +++ .../SECURITY.md | 0 packages/non-trapping-shim/index.js | 1 + .../package.json | 6 +- .../shim.js | 2 +- .../src/non-trapping-pony.js} | 99 ++++++++++--------- .../test/non-trapping-pony.test.js} | 8 +- .../test/non-trapping-shim.test.js} | 6 +- .../tsconfig.build.json | 0 .../tsconfig.json | 0 packages/ses/docs/preparing-for-stabilize.md | 2 +- yarn.lock | 4 +- 17 files changed, 82 insertions(+), 60 deletions(-) delete mode 100644 packages/no-trapping-shim/README.md delete mode 100644 packages/no-trapping-shim/index.js rename packages/{no-trapping-shim => non-trapping-shim}/CHANGELOG.md (100%) rename packages/{no-trapping-shim => non-trapping-shim}/LICENSE (100%) rename packages/{no-trapping-shim => non-trapping-shim}/NEWS.md (100%) create mode 100644 packages/non-trapping-shim/README.md rename packages/{no-trapping-shim => non-trapping-shim}/SECURITY.md (100%) create mode 100644 packages/non-trapping-shim/index.js rename packages/{no-trapping-shim => non-trapping-shim}/package.json (90%) rename packages/{no-trapping-shim => non-trapping-shim}/shim.js (72%) rename packages/{no-trapping-shim/src/no-trapping-pony.js => non-trapping-shim/src/non-trapping-pony.js} (74%) rename packages/{no-trapping-shim/test/no-trapping-pony.test.js => non-trapping-shim/test/non-trapping-pony.test.js} (76%) rename packages/{no-trapping-shim/test/no-trapping-shim.test.js => non-trapping-shim/test/non-trapping-shim.test.js} (84%) rename packages/{no-trapping-shim => non-trapping-shim}/tsconfig.build.json (100%) rename packages/{no-trapping-shim => non-trapping-shim}/tsconfig.json (100%) diff --git a/packages/no-trapping-shim/README.md b/packages/no-trapping-shim/README.md deleted file mode 100644 index 8b13789179..0000000000 --- a/packages/no-trapping-shim/README.md +++ /dev/null @@ -1 +0,0 @@ - diff --git a/packages/no-trapping-shim/index.js b/packages/no-trapping-shim/index.js deleted file mode 100644 index 42a68f007b..0000000000 --- a/packages/no-trapping-shim/index.js +++ /dev/null @@ -1 +0,0 @@ -export * from './src/no-trapping-pony.js'; diff --git a/packages/no-trapping-shim/CHANGELOG.md b/packages/non-trapping-shim/CHANGELOG.md similarity index 100% rename from packages/no-trapping-shim/CHANGELOG.md rename to packages/non-trapping-shim/CHANGELOG.md diff --git a/packages/no-trapping-shim/LICENSE b/packages/non-trapping-shim/LICENSE similarity index 100% rename from packages/no-trapping-shim/LICENSE rename to packages/non-trapping-shim/LICENSE diff --git a/packages/no-trapping-shim/NEWS.md b/packages/non-trapping-shim/NEWS.md similarity index 100% rename from packages/no-trapping-shim/NEWS.md rename to packages/non-trapping-shim/NEWS.md diff --git a/packages/non-trapping-shim/README.md b/packages/non-trapping-shim/README.md new file mode 100644 index 0000000000..1207ff7a0b --- /dev/null +++ b/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. diff --git a/packages/no-trapping-shim/SECURITY.md b/packages/non-trapping-shim/SECURITY.md similarity index 100% rename from packages/no-trapping-shim/SECURITY.md rename to packages/non-trapping-shim/SECURITY.md diff --git a/packages/non-trapping-shim/index.js b/packages/non-trapping-shim/index.js new file mode 100644 index 0000000000..e61784152d --- /dev/null +++ b/packages/non-trapping-shim/index.js @@ -0,0 +1 @@ +export * from './src/non-trapping-pony.js'; diff --git a/packages/no-trapping-shim/package.json b/packages/non-trapping-shim/package.json similarity index 90% rename from packages/no-trapping-shim/package.json rename to packages/non-trapping-shim/package.json index 42c20f99de..0e22fb507b 100644 --- a/packages/no-trapping-shim/package.json +++ b/packages/non-trapping-shim/package.json @@ -1,8 +1,8 @@ { - "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", @@ -10,7 +10,7 @@ "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" diff --git a/packages/no-trapping-shim/shim.js b/packages/non-trapping-shim/shim.js similarity index 72% rename from packages/no-trapping-shim/shim.js rename to packages/non-trapping-shim/shim.js index b20931f038..d6f4837849 100644 --- a/packages/no-trapping-shim/shim.js +++ b/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; diff --git a/packages/no-trapping-shim/src/no-trapping-pony.js b/packages/non-trapping-shim/src/non-trapping-pony.js similarity index 74% rename from packages/no-trapping-shim/src/no-trapping-pony.js rename to packages/non-trapping-shim/src/non-trapping-pony.js index 5b3d356225..2c2fe392a2 100644 --- a/packages/no-trapping-shim/src/no-trapping-pony.js +++ b/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,7 +236,7 @@ const metaHandler = freeze({ * * @param {ProxyHandler} originalHandler * @returns {ProxyHandler & { - * isNoTrapping: (target: any) => boolean, + * isNonTrapping: (target: any) => boolean, * suppressTrapping: (target: any) => boolean, * originalHandler: ProxyHandler * }} @@ -233,19 +244,11 @@ const metaHandler = freeze({ 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,10 +259,18 @@ 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); @@ -267,7 +278,7 @@ ProxyPlus.revocable = (target, handler) => { return { proxy, revoke() { - if (isNoTrappingInternal(target, true)) { + if (isNonTrappingInternal(target, true)) { throw TypeError('Cannot revoke non-trapping proxy'); } revoke(); diff --git a/packages/no-trapping-shim/test/no-trapping-pony.test.js b/packages/non-trapping-shim/test/non-trapping-pony.test.js similarity index 76% rename from packages/no-trapping-shim/test/no-trapping-pony.test.js rename to packages/non-trapping-shim/test/non-trapping-pony.test.js index 15a2c385ff..7fae41ac0a 100644 --- a/packages/no-trapping-shim/test/no-trapping-pony.test.js +++ b/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); }); diff --git a/packages/no-trapping-shim/test/no-trapping-shim.test.js b/packages/non-trapping-shim/test/non-trapping-shim.test.js similarity index 84% rename from packages/no-trapping-shim/test/no-trapping-shim.test.js rename to packages/non-trapping-shim/test/non-trapping-shim.test.js index b6ac48c25d..9279928d29 100644 --- a/packages/no-trapping-shim/test/no-trapping-shim.test.js +++ b/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); }); diff --git a/packages/no-trapping-shim/tsconfig.build.json b/packages/non-trapping-shim/tsconfig.build.json similarity index 100% rename from packages/no-trapping-shim/tsconfig.build.json rename to packages/non-trapping-shim/tsconfig.build.json diff --git a/packages/no-trapping-shim/tsconfig.json b/packages/non-trapping-shim/tsconfig.json similarity index 100% rename from packages/no-trapping-shim/tsconfig.json rename to packages/non-trapping-shim/tsconfig.json diff --git a/packages/ses/docs/preparing-for-stabilize.md b/packages/ses/docs/preparing-for-stabilize.md index 592917abdd..63a432b46f 100644 --- a/packages/ses/docs/preparing-for-stabilize.md +++ b/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. diff --git a/yarn.lock b/yarn.lock index 9a92741957..98e3a54cd0 100644 --- a/yarn.lock +++ b/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"