From fde26da22f03a18045807d833c8e03c4409fd877 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Fri, 6 Oct 2023 21:04:32 -0700 Subject: [PATCH 1/3] fix(exo): allow richer behaviorMethods --- packages/exo/package.json | 1 + packages/exo/src/exo-tools.js | 63 +++--- packages/exo/src/get-interface.js | 7 +- packages/exo/test/test-exo-class-js-class.js | 77 +++++++ packages/exo/test/test-exo-wobbly-point.js | 205 ++++++++++++++++++ packages/exo/test/test-heap-classes.js | 4 +- .../exo/test/test-non-enumerable-methods.js | 129 +++++++++++ packages/pass-style/package.json | 1 + .../test/test-far-class-instances.js | 98 +++++++++ .../pass-style/test/test-far-wobbly-point.js | 145 +++++++++++++ .../patterns/src/patterns/patternMatchers.js | 9 +- 11 files changed, 700 insertions(+), 39 deletions(-) create mode 100644 packages/exo/test/test-exo-class-js-class.js create mode 100644 packages/exo/test/test-exo-wobbly-point.js create mode 100644 packages/exo/test/test-non-enumerable-methods.js create mode 100644 packages/pass-style/test/test-far-class-instances.js create mode 100644 packages/pass-style/test/test-far-wobbly-point.js diff --git a/packages/exo/package.json b/packages/exo/package.json index a0c68bc9ff..6ab75bb960 100644 --- a/packages/exo/package.json +++ b/packages/exo/package.json @@ -33,6 +33,7 @@ }, "dependencies": { "@endo/env-options": "^0.1.4", + "@endo/eventual-send": "^0.17.6", "@endo/far": "^0.2.22", "@endo/pass-style": "^0.1.7", "@endo/patterns": "^0.2.6" diff --git a/packages/exo/src/exo-tools.js b/packages/exo/src/exo-tools.js index 71c4403dc6..4b2250da11 100644 --- a/packages/exo/src/exo-tools.js +++ b/packages/exo/src/exo-tools.js @@ -1,5 +1,5 @@ +import { getMethodNames } from '@endo/eventual-send/src/local.js'; import { E, Far } from '@endo/far'; -import { hasOwnPropertyOf } from '@endo/pass-style'; import { listDifference, objectMap, @@ -12,7 +12,6 @@ import { getInterfaceGuardPayload, getCopyMapEntries, } from '@endo/patterns'; - import { GET_INTERFACE_GUARD } from './get-interface.js'; /** @typedef {import('@endo/patterns').Method} Method */ @@ -377,21 +376,6 @@ const bindMethod = ( return method; }; -/** - * - * @template {Record} T - * @param {T} behaviorMethods - * @param {InterfaceGuard<{ [M in keyof T]: MethodGuard }>} interfaceGuard - * @returns {T & import('./get-interface.js').GetInterfaceGuard} - */ -const withGetInterfaceGuardMethod = (behaviorMethods, interfaceGuard) => - harden({ - [GET_INTERFACE_GUARD]() { - return interfaceGuard; - }, - ...behaviorMethods, - }); - /** * @template {Record} T * @param {string} tag @@ -408,13 +392,20 @@ export const defendPrototype = ( interfaceGuard = undefined, ) => { const prototype = {}; - if (hasOwnPropertyOf(behaviorMethods, 'constructor')) { - // By ignoring any method named "constructor", we can use a + const methodNames = getMethodNames(behaviorMethods).filter( + // By ignoring any method that seems to be a constructor, we can use a // class.prototype as a behaviorMethods. - const { constructor: _, ...methods } = behaviorMethods; - // @ts-expect-error TS misses that hasOwn check makes this safe - behaviorMethods = harden(methods); - } + key => { + if (key !== 'constructor') { + return true; + } + const constructor = behaviorMethods.constructor; + return !( + constructor.prototype && + constructor.prototype.constructor === constructor + ); + }, + ); /** @type {Record | undefined} */ let methodGuards; /** @type {import('@endo/patterns').DefaultGuardType} */ @@ -432,10 +423,9 @@ export const defendPrototype = ( ...(symbolMethodGuards && fromEntries(getCopyMapEntries(symbolMethodGuards))), }); + assert(methodGuards !== undefined); defaultGuards = dg; { - const methodNames = ownKeys(behaviorMethods); - assert(methodGuards); const methodGuardNames = ownKeys(methodGuards); const unimplemented = listDifference(methodGuardNames, methodNames); unimplemented.length === 0 || @@ -446,12 +436,9 @@ export const defendPrototype = ( Fail`methods ${q(unguarded)} not guarded by ${q(interfaceName)}`; } } - behaviorMethods = withGetInterfaceGuardMethod( - behaviorMethods, - interfaceGuard, - ); } - for (const prop of ownKeys(behaviorMethods)) { + + for (const prop of methodNames) { prototype[prop] = bindMethod( `In ${q(prop)} method of (${tag})`, contextProvider, @@ -463,6 +450,22 @@ export const defendPrototype = ( ); } + if (!hasOwnPropertyOf(prototype, GET_INTERFACE_GUARD)) { + const getInterfaceGuardMethod = { + [GET_INTERFACE_GUARD]() { + // Note: May be `undefined` + return interfaceGuard; + }, + }[GET_INTERFACE_GUARD]; + prototype[GET_INTERFACE_GUARD] = bindMethod( + `In ${q(GET_INTERFACE_GUARD)} method of (${tag})`, + contextProvider, + getInterfaceGuardMethod, + thisfulMethods, + undefined, + ); + } + return Far( tag, /** @type {T & import('./get-interface.js').GetInterfaceGuard} */ ( diff --git a/packages/exo/src/get-interface.js b/packages/exo/src/get-interface.js index 9fd4baf9d0..1d1d894d36 100644 --- a/packages/exo/src/get-interface.js +++ b/packages/exo/src/get-interface.js @@ -15,8 +15,9 @@ export const GET_INTERFACE_GUARD = Symbol.for('getInterfaceGuard'); /** * @template {Record} M * @typedef {{ - * [GET_INTERFACE_GUARD]: () => import('@endo/patterns').InterfaceGuard<{ - * [K in keyof M]: import('@endo/patterns').MethodGuard - * }> + * [GET_INTERFACE_GUARD]: () => + * import('@endo/patterns').InterfaceGuard<{ + * [K in keyof M]: import('@endo/patterns').MethodGuard + * }> | undefined * }} GetInterfaceGuard */ diff --git a/packages/exo/test/test-exo-class-js-class.js b/packages/exo/test/test-exo-class-js-class.js new file mode 100644 index 0000000000..8cc19eae56 --- /dev/null +++ b/packages/exo/test/test-exo-class-js-class.js @@ -0,0 +1,77 @@ +/* eslint-disable max-classes-per-file */ +/* eslint-disable class-methods-use-this */ +// eslint-disable-next-line import/order +import { test } from './prepare-test-env-ava.js'; + +import { passStyleOf } from '@endo/pass-style'; +import { M, getInterfaceGuardPayload } from '@endo/patterns'; +import { makeExo, defineExoClass } from '../src/exo-makers.js'; + +// Based on FarSubclass1 in test-far-class-instances.js +class DoublerBehaviorClass { + double(x) { + return x + x; + } +} + +const DoublerI = M.interface('Doubler', { + double: M.call(M.lte(10)).returns(M.number()), +}); + +const doubler = makeExo('doubler', DoublerI, DoublerBehaviorClass.prototype); + +test('exo doubler using js classes', t => { + t.is(passStyleOf(doubler), 'remotable'); + t.is(doubler.double(3), 6); + t.throws(() => doubler.double('x'), { + message: 'In "double" method of (doubler): arg 0: "x" - Must be <= 10', + }); + t.throws(() => doubler.double(), { + message: + 'In "double" method of (doubler): Expected at least 1 arguments: []', + }); + t.throws(() => doubler.double(12), { + message: 'In "double" method of (doubler): arg 0: 12 - Must be <= 10', + }); +}); + +// Based on FarSubclass2 in test-far-class-instances.js +class DoubleAdderBehaviorClass extends DoublerBehaviorClass { + doubleAddSelfCall(x) { + const { + state: { y }, + self, + } = this; + return self.double(x) + y; + } + + doubleAddSuperCall(x) { + const { + state: { y }, + } = this; + return super.double(x) + y; + } +} + +const DoubleAdderI = M.interface('DoubleAdder', { + ...getInterfaceGuardPayload(DoublerI).methodGuards, + doubleAddSelfCall: M.call(M.number()).returns(M.number()), + doubleAddSuperCall: M.call(M.number()).returns(M.number()), +}); + +const makeDoubleAdder = defineExoClass( + 'doubleAdderClass', + DoubleAdderI, + y => ({ y }), + DoubleAdderBehaviorClass.prototype, +); + +test('exo inheritance self vs super call', t => { + const da = makeDoubleAdder(5); + t.is(da.doubleAddSelfCall(3), 11); + t.throws(() => da.doubleAddSelfCall(12), { + message: + 'In "double" method of (doubleAdderClass): arg 0: 12 - Must be <= 10', + }); + t.is(da.doubleAddSuperCall(12), 29); +}); diff --git a/packages/exo/test/test-exo-wobbly-point.js b/packages/exo/test/test-exo-wobbly-point.js new file mode 100644 index 0000000000..84be64755b --- /dev/null +++ b/packages/exo/test/test-exo-wobbly-point.js @@ -0,0 +1,205 @@ +/** + * Based on the WobblyPoint inheritance examples in + * https://storage.googleapis.com/google-code-archive-downloads/v2/code.google.com/google-caja/caja-spec-2007-12-21.pdf + * and + * test-far-wobbly-point.js + */ + +/* eslint-disable class-methods-use-this */ +/* eslint-disable max-classes-per-file */ +/* eslint-disable-next-line import/order */ +import { test } from './prepare-test-env-ava.js'; + +// TODO enable import of getMethodNames without deep import +// eslint-disable-next-line import/order +import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { passStyleOf, Far } from '@endo/pass-style'; +import { M } from '@endo/patterns'; +import { defineExoClass } from '../src/exo-makers.js'; +import { GET_INTERFACE_GUARD } from '../src/get-interface.js'; + +const { Fail, quote: q } = assert; +const { apply } = Reflect; + +const ExoEmptyI = M.interface('ExoEmpty', {}); + +class ExoBaseClass { + constructor() { + Fail`Turn Exo JS classes into Exo classes with defineExoClassFromJSClass: ${q( + new.target.name, + )}`; + } + + static implements = ExoEmptyI; + + static init() { + return harden({}); + } +} + +const defineExoClassFromJSClass = klass => + defineExoClass(klass.name, klass.implements, klass.init, klass.prototype); +harden(defineExoClassFromJSClass); + +const ExoPointI = M.interface('ExoPoint', { + toString: M.call().returns(M.string()), + getX: M.call().returns(M.gte(0)), + getY: M.call().returns(M.number()), + setY: M.call(M.number()).returns(), +}); + +class ExoAbstractPoint extends ExoBaseClass { + static implements = ExoPointI; + + toString() { + const { self } = this; + return `<${self.getX()},${self.getY()}>`; + } +} + +test('cannot make abstract class concrete', t => { + t.throws(() => defineExoClassFromJSClass(ExoAbstractPoint), { + message: + 'methods ["getX","getY","setY"] not implemented by "ExoAbstractPoint"', + }); +}); + +class ExoPoint extends ExoAbstractPoint { + static init(x, y) { + // Heap exos currently use the returned record directly, so + // needs to not be frozen for `state.y` to be assignable. + // TODO not true for other zones. May make heap zone more like + // the others in treatment of `state`. + return { x, y }; + } + + getX() { + const { + state: { x }, + } = this; + return x; + } + + getY() { + const { + state: { y }, + } = this; + return y; + } + + setY(newY) { + const { state } = this; + state.y = newY; + } +} +harden(ExoPoint); + +const makeExoPoint = defineExoClassFromJSClass(ExoPoint); + +test('ExoPoint instances', t => { + const pt = makeExoPoint(3, 5); + t.is(passStyleOf(pt), 'remotable'); + t.false(pt instanceof ExoPoint); + t.deepEqual(getMethodNames(pt), [ + GET_INTERFACE_GUARD, + 'getX', + 'getY', + 'setY', + 'toString', + ]); + t.is(pt.getX(), 3); + t.is(pt.getY(), 5); + t.is(`${pt}`, '<3,5>'); + pt.setY(6); + t.is(`${pt}`, '<3,6>'); + + const otherPt = makeExoPoint(1, 2); + t.is(apply(pt.getX, otherPt, []), 1); + + const negPt = makeExoPoint(-3, 5); + t.throws(() => negPt.getX(), { + message: 'In "getX" method of (ExoPoint): result: -3 - Must be >= 0', + }); + // `self` calls are guarded + t.throws(() => `${negPt}`, { + message: 'In "getX" method of (ExoPoint): result: -3 - Must be >= 0', + }); +}); + +class ExoWobblyPoint extends ExoPoint { + static init(x, y, getWobble) { + return { ...super.init(x, y), getWobble }; + } + + getX() { + const { + state: { getWobble }, + } = this; + return super.getX() + getWobble(); + } +} +harden(ExoWobblyPoint); + +const makeExoWobblyPoint = defineExoClassFromJSClass(ExoWobblyPoint); + +test('FarWobblyPoint inheritance', t => { + let wobble = 0; + // For heap classes currently, there is no reason to make `getWobble` passable. + // But other zones insist on at least passability, and TODO we may eventually + // make the heap zone act like this as well. + const getWobble = Far('getW', () => (wobble += 1)); + const wpt = makeExoWobblyPoint(3, 5, getWobble); + t.false(wpt instanceof ExoWobblyPoint); + t.false(wpt instanceof ExoPoint); + t.is(passStyleOf(wpt), 'remotable'); + t.deepEqual(getMethodNames(wpt), [ + GET_INTERFACE_GUARD, + 'getX', + 'getY', + 'setY', + 'toString', + ]); + t.is(`${wpt}`, '<4,5>'); + t.is(`${wpt}`, '<5,5>'); + t.is(`${wpt}`, '<6,5>'); + wpt.setY(6); + t.is(`${wpt}`, '<7,6>'); + + const otherPt = makeExoPoint(1, 2); + t.false(otherPt instanceof ExoWobblyPoint); + t.throws(() => apply(wpt.getX, otherPt, []), { + message: + '"In \\"getX\\" method of (ExoWobblyPoint)" may only be applied to a valid instance: "[Alleged: ExoPoint]"', + }); + t.throws(() => apply(wpt.getY, otherPt, []), { + message: + '"In \\"getY\\" method of (ExoWobblyPoint)" may only be applied to a valid instance: "[Alleged: ExoPoint]"', + }); + + const otherWpt = makeExoWobblyPoint(3, 5, () => 1); + t.is(`${otherWpt}`, '<4,5>'); + t.is(apply(wpt.getX, otherWpt, []), 4); + + // This error behavior shows the absence of the security vulnerability + // explained at the corresponding example in `test-far-wobbly-point.js` + // for the `@endo/pass-style` / `Far` level of abstraction. At the exo level + // of abstraction, a raw-method subclass overriding an inherited superclass + // method denies that method to clients of instances of the subclass. + // At the same time, this overridden method remains available within + // the overriding subclass via unguarded `super` calls. + t.throws(() => apply(otherPt.getX, otherWpt, []), { + message: + '"In \\"getX\\" method of (ExoPoint)" may only be applied to a valid instance: "[Alleged: ExoWobblyPoint]"', + }); + + const negWpt1 = makeExoWobblyPoint(-3, 5, () => 4); + t.is(negWpt1.getX(), 1); + // `super` calls are direct, bypassing guards and sharing context + t.is(`${negWpt1}`, '<1,5>'); + + const negWpt2 = makeExoWobblyPoint(1, 5, () => -4); + t.throws(() => `${negWpt2}`, { + // `self` calls are guarded + message: 'In "getX" method of (ExoWobblyPoint): result: -3 - Must be >= 0', + }); +}); diff --git a/packages/exo/test/test-heap-classes.js b/packages/exo/test/test-heap-classes.js index 45cb29d194..1d2c72d95e 100644 --- a/packages/exo/test/test-heap-classes.js +++ b/packages/exo/test/test-heap-classes.js @@ -185,9 +185,7 @@ test('missing interface', t => { message: 'In "makeSayHello" method of (greeterMaker): result: "[Symbol(passStyle)]" property expected: "[Function ]"', }); - t.throws(() => greeterMaker[GET_INTERFACE_GUARD](), { - message: 'greeterMaker[GET_INTERFACE_GUARD] is not a function', - }); + t.is(greeterMaker[GET_INTERFACE_GUARD](), undefined); }); const SloppyGreeterI = M.interface('greeter', {}, { sloppy: true }); diff --git a/packages/exo/test/test-non-enumerable-methods.js b/packages/exo/test/test-non-enumerable-methods.js new file mode 100644 index 0000000000..7ed454b162 --- /dev/null +++ b/packages/exo/test/test-non-enumerable-methods.js @@ -0,0 +1,129 @@ +// eslint-disable-next-line import/order +import { test } from './prepare-test-env-ava.js'; + +// eslint-disable-next-line import/order +import { getInterfaceMethodKeys, M } from '@endo/patterns'; +import { defineExoClass } from '../src/exo-makers.js'; +import { GET_INTERFACE_GUARD } from '../src/get-interface.js'; + +const { getPrototypeOf, getOwnPropertyDescriptors, create, fromEntries } = + Object; + +const { ownKeys } = Reflect; + +/** + * Borrowed from https://github.com/endojs/endo/pull/1815 to avoid + * depending on it being merged. TODO If it is merged, then delete this + * copy and import `objectMetaMap` instead. + * + * Like `objectMap`, but at the reflective level of property descriptors + * rather than property values. + * + * Except for hardening, the edge case behavior is mostly the opposite of + * the `objectMap` edge cases. + * * No matter how mutable the original object, the returned object is + * hardened. + * * All own properties of the original are mapped, even if symbol-named + * or non-enumerable. + * * If any of the original properties were accessors, the descriptor + * containing the getter and setter are given to `metaMapFn`. + * * The own properties of the returned are according to the descriptors + * returned by `metaMapFn`. + * * The returned object will always be a plain object whose state is + * only these mapped own properties. It will inherit from the third + * argument if provided, defaulting to `Object.prototype` if omitted. + * + * Because a property descriptor is distinct from `undefined`, we bundle + * mapping and filtering together. When the `metaMapFn` returns `undefined`, + * that property is omitted from the result. + * + * @template {Record} O + * @param {O} original + * @param {( + * desc: TypedPropertyDescriptor, + * key: keyof O + * ) => (PropertyDescriptor | undefined)} metaMapFn + * @param {any} [proto] + * @returns {any} + */ +export const objectMetaMap = ( + original, + metaMapFn, + proto = Object.prototype, +) => { + const descs = getOwnPropertyDescriptors(original); + const keys = ownKeys(original); + + const descEntries = /** @type {[PropertyKey,PropertyDescriptor][]} */ ( + keys + .map(key => [key, metaMapFn(descs[key], key)]) + .filter(([_key, optDesc]) => optDesc !== undefined) + ); + return harden(create(proto, fromEntries(descEntries))); +}; +harden(objectMetaMap); + +const UpCounterI = M.interface('UpCounter', { + incr: M.call() + // TODO M.number() should not be needed to get a better error message + .optional(M.and(M.number(), M.gte(0))) + .returns(M.number()), +}); + +const denumerate = obj => + objectMetaMap( + obj, + desc => ({ ...desc, enumerable: false }), + getPrototypeOf(obj), + ); + +test('test defineExoClass', t => { + const makeUpCounter = defineExoClass( + 'UpCounter', + UpCounterI, + /** @param {number} x */ + (x = 0) => ({ x }), + denumerate({ + incr(y = 1) { + const { state } = this; + state.x += y; + return state.x; + }, + }), + ); + const upCounter = makeUpCounter(3); + t.is(upCounter.incr(5), 8); + t.is(upCounter.incr(1), 9); + t.throws(() => upCounter.incr(-3), { + message: 'In "incr" method of (UpCounter): arg 0?: -3 - Must be >= 0', + }); + // @ts-expect-error bad arg + t.throws(() => upCounter.incr('foo'), { + message: + 'In "incr" method of (UpCounter): arg 0?: string "foo" - Must be a number', + }); + t.deepEqual(upCounter[GET_INTERFACE_GUARD](), UpCounterI); + t.deepEqual(getInterfaceMethodKeys(UpCounterI), ['incr']); + + const symbolic = Symbol.for('symbolic'); + const FooI = M.interface('Foo', { + m: M.call().returns(), + [symbolic]: M.call(M.boolean()).returns(), + }); + t.deepEqual(getInterfaceMethodKeys(FooI), ['m', Symbol.for('symbolic')]); + const makeFoo = defineExoClass( + 'Foo', + FooI, + () => ({}), + denumerate({ + m() {}, + [symbolic]() {}, + }), + ); + const foo = makeFoo(); + t.deepEqual(foo[GET_INTERFACE_GUARD](), FooI); + t.throws(() => foo[symbolic]('invalid arg'), { + message: + 'In "[Symbol(symbolic)]" method of (Foo): arg 0: string "invalid arg" - Must be a boolean', + }); +}); diff --git a/packages/pass-style/package.json b/packages/pass-style/package.json index 60a39fe909..82fc4f8d9e 100644 --- a/packages/pass-style/package.json +++ b/packages/pass-style/package.json @@ -37,6 +37,7 @@ "@fast-check/ava": "^1.1.5" }, "devDependencies": { + "@endo/eventual-send": "^0.17.6", "@endo/init": "^0.5.60", "@endo/ses-ava": "^0.2.44", "ava": "^5.3.0", diff --git a/packages/pass-style/test/test-far-class-instances.js b/packages/pass-style/test/test-far-class-instances.js new file mode 100644 index 0000000000..7b98427ce4 --- /dev/null +++ b/packages/pass-style/test/test-far-class-instances.js @@ -0,0 +1,98 @@ +/* eslint-disable class-methods-use-this */ +/* eslint-disable max-classes-per-file */ +import { test } from './prepare-test-env-ava.js'; + +// TODO enable import of getMethodNames without deep import +// eslint-disable-next-line import/order +import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { passStyleOf } from '../src/passStyleOf.js'; +import { Far } from '../src/make-far.js'; + +/** + * Classes whose instances should be Far objects may find it convenient to + * inherit from this base class. Note that the constructor of this base class + * freezes the instance in an empty state, so all is interesting attributes + * can only depend on its identity and what it inherits from. + * This includes private fields, as those are keyed only on + * this object's identity. However, we discourage (but cannot prevent) such + * use of private fields, as they cannot easily be refactored into Exo state. + */ +export const FarBaseClass = class FarBaseClass { + constructor() { + harden(this); + } +}; + +Far('FarBaseClass', FarBaseClass.prototype); +harden(FarBaseClass); + +class FarSubclass1 extends FarBaseClass { + double(x) { + return x + x; + } +} + +class FarSubclass2 extends FarSubclass1 { + #y = 0; + + constructor(y) { + super(); + this.#y = y; + } + + doubleAdd(x) { + return this.double(x) + this.#y; + } +} + +test('far class instances', t => { + const fb = new FarBaseClass(); + t.is(passStyleOf(fb), 'remotable'); + t.deepEqual(getMethodNames(fb), ['constructor']); + + t.assert(new fb.constructor() instanceof FarBaseClass); + t.throws(() => fb.constructor(), { + // TODO message depends on JS engine, and so is a fragile golden test + message: "Class constructor FarBaseClass cannot be invoked without 'new'", + }); + + const fs1 = new FarSubclass1(); + t.is(passStyleOf(fs1), 'remotable'); + t.is(fs1.double(4), 8); + t.assert(new fs1.constructor() instanceof FarSubclass1); + t.deepEqual(getMethodNames(fs1), ['constructor', 'double']); + + const fs2 = new FarSubclass2(3); + t.is(passStyleOf(fs2), 'remotable'); + t.is(fs2.double(4), 8); + t.is(fs2.doubleAdd(4), 11); + t.deepEqual(getMethodNames(fs2), ['constructor', 'double', 'doubleAdd']); + + const yField = new WeakMap(); + class FarSubclass3 extends FarSubclass1 { + constructor(y) { + super(); + yField.set(this, y); + } + + doubleAdd(x) { + return this.double(x) + yField.get(this); + } + } + + const fs3 = new FarSubclass3(3); + t.is(passStyleOf(fs3), 'remotable'); + t.is(fs3.double(4), 8); + t.is(fs3.doubleAdd(4), 11); + t.deepEqual(getMethodNames(fs3), ['constructor', 'double', 'doubleAdd']); +}); + +test('far class instance hardened empty', t => { + class FarClass4 extends FarBaseClass { + z = 0; + } + t.throws(() => new FarClass4(), { + // TODO message depends on JS engine, and so is a fragile golden test + message: 'Cannot define property z, object is not extensible', + }); +}); diff --git a/packages/pass-style/test/test-far-wobbly-point.js b/packages/pass-style/test/test-far-wobbly-point.js new file mode 100644 index 0000000000..34905c093d --- /dev/null +++ b/packages/pass-style/test/test-far-wobbly-point.js @@ -0,0 +1,145 @@ +/** + * Based on the WobblyPoint inheritance examples in + * https://storage.googleapis.com/google-code-archive-downloads/v2/code.google.com/google-caja/caja-spec-2007-12-21.pdf + */ + +/* eslint-disable class-methods-use-this */ +/* eslint-disable max-classes-per-file */ +import { test } from './prepare-test-env-ava.js'; + +// TODO enable import of getMethodNames without deep import +// eslint-disable-next-line import/order +import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { passStyleOf } from '../src/passStyleOf.js'; +import { Far } from '../src/make-far.js'; + +const { apply } = Reflect; + +/** + * Classes whose instances should be Far objects may find it convenient to + * inherit from this base class. Note that the constructor of this base class + * freezes the instance in an empty state, so all is interesting attributes + * can only depend on its identity and what it inherits from. + * This includes private fields, as those are keyed only on + * this object's identity. However, we discourage (but cannot prevent) such + * use of private fields, as they cannot easily be refactored into Exo state. + */ +class FarBaseClass { + constructor() { + harden(this); + } +} +Far('FarBaseClass', FarBaseClass.prototype); +harden(FarBaseClass); + +class FarPoint extends FarBaseClass { + #x; + + #y; + + constructor(x, y) { + super(); + this.#x = x; + this.#y = y; + } + + toString() { + return `<${this.getX()},${this.getY()}>`; + } + + getX() { + return this.#x; + } + + getY() { + return this.#y; + } + + setY(newY) { + this.#y = newY; + } +} +harden(FarPoint); + +test('FarPoint instances', t => { + const pt = new FarPoint(3, 5); + t.is(passStyleOf(pt), 'remotable'); + t.assert(pt instanceof FarPoint); + t.deepEqual(getMethodNames(pt), [ + 'constructor', + 'getX', + 'getY', + 'setY', + 'toString', + ]); + t.is(pt.getX(), 3); + t.is(pt.getY(), 5); + t.is(`${pt}`, '<3,5>'); + pt.setY(6); + t.is(`${pt}`, '<3,6>'); + + const otherPt = new FarPoint(1, 2); + t.is(apply(pt.getX, otherPt, []), 1); +}); + +class FarWobblyPoint extends FarPoint { + #getWobble; + + constructor(x, y, getWobble) { + super(x, y); + this.#getWobble = getWobble; + } + + getX() { + return super.getX() + this.#getWobble(); + } +} +harden(FarWobblyPoint); + +test('FarWobblyPoint inheritance', t => { + let wobble = 0; + const getWobble = () => (wobble += 1); + const wpt = new FarWobblyPoint(3, 5, getWobble); + t.assert(wpt instanceof FarWobblyPoint); + t.assert(wpt instanceof FarPoint); + t.is(passStyleOf(wpt), 'remotable'); + t.deepEqual(getMethodNames(wpt), [ + 'constructor', + 'getX', + 'getY', + 'setY', + 'toString', + ]); + t.is(`${wpt}`, '<4,5>'); + t.is(`${wpt}`, '<5,5>'); + t.is(`${wpt}`, '<6,5>'); + wpt.setY(6); + t.is(`${wpt}`, '<7,6>'); + + const otherPt = new FarPoint(1, 2); + t.false(otherPt instanceof FarWobblyPoint); + t.throws(() => apply(wpt.getX, otherPt, []), { + // TODO great error message, but is a golden specific to v8 + message: + 'Cannot read private member #getWobble from an object whose class did not declare it', + }); + t.is(apply(wpt.getY, otherPt, []), 2); + + const otherWpt = new FarWobblyPoint(3, 5, () => 1); + t.is(`${otherWpt}`, '<4,5>'); + t.is(apply(wpt.getX, otherWpt, []), 4); + + // This test, though correct, demonstrates a sucurity weakness of + // this approach to JS class inheritance at this + // `@endo/pass-style` / `Far` level of abstraction. The weakness + // is that the overridden method from a superclass can nevertheless + // be directly applied to an instance of the subclass. The + // subclass override did not suppress the use of the overridden + // method as, effectively, part of the subclass' instance's public + // API. + // + // See the corresponding example at + // `test-exo-wobbly-point.js` to see the absence of this vulnerability + // at the Exo level of abstraction. + t.is(apply(otherPt.getX, otherWpt, []), 3); +}); diff --git a/packages/patterns/src/patterns/patternMatchers.js b/packages/patterns/src/patterns/patternMatchers.js index 71a5beb8a3..48978f072a 100644 --- a/packages/patterns/src/patterns/patternMatchers.js +++ b/packages/patterns/src/patterns/patternMatchers.js @@ -1664,7 +1664,8 @@ const makePatternKit = () => { ), split: (base, rest = undefined) => { if (passStyleOf(harden(base)) === 'copyArray') { - // @ts-expect-error We know it should be an array + // TODO at-ts-expect-error works locally but not from @endo/exo + // @ts-ignore We know it should be an array return M.splitArray(base, rest && [], rest); } else { return M.splitRecord(base, rest && {}, rest); @@ -1672,7 +1673,8 @@ const makePatternKit = () => { }, partial: (base, rest = undefined) => { if (passStyleOf(harden(base)) === 'copyArray') { - // @ts-expect-error We know it should be an array + // TODO at-ts-expect-error works locally but not from @endo/exo + // @ts-ignore We know it should be an array return M.splitArray([], base, rest); } else { return M.splitRecord({}, base, rest); @@ -1945,7 +1947,8 @@ export const getInterfaceMethodKeys = interfaceGuard => { const { methodGuards, symbolMethodGuards = emptyCopyMap } = getInterfaceGuardPayload(interfaceGuard); /** @type {(string | symbol)[]} */ - // @ts-expect-error inference is too weak to see this is ok + // TODO at-ts-expect-error works locally but not from @endo/exo + // @ts-ignore inference is too weak to see this is ok return harden([ ...Reflect.ownKeys(methodGuards), ...getCopyMapKeys(symbolMethodGuards), From b07981215a64766b2813f92f6d6c430d181b5512 Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Fri, 3 Nov 2023 18:38:30 -0700 Subject: [PATCH 2/3] feat(pass-style): Far GET_METHOD_NAMES meta method --- packages/eventual-send/utils.js | 1 + packages/exo/src/exo-tools.js | 4 +- packages/exo/src/get-interface.js | 7 ++- packages/exo/test/test-exo-wobbly-point.js | 16 ++++--- packages/exo/test/test-heap-classes.js | 2 +- packages/marshal/test/test-marshal-testing.js | 12 ++++-- packages/pass-style/index.js | 7 ++- packages/pass-style/package.json | 2 +- packages/pass-style/src/make-far.js | 43 +++++++++++++++++++ .../test/test-far-class-instances.js | 30 +++++++++---- .../pass-style/test/test-far-wobbly-point.js | 16 ++++--- packages/pass-style/test/test-passStyleOf.js | 3 +- 12 files changed, 113 insertions(+), 30 deletions(-) create mode 100644 packages/eventual-send/utils.js diff --git a/packages/eventual-send/utils.js b/packages/eventual-send/utils.js new file mode 100644 index 0000000000..4fee534df0 --- /dev/null +++ b/packages/eventual-send/utils.js @@ -0,0 +1 @@ +export { getMethodNames } from './src/local.js'; diff --git a/packages/exo/src/exo-tools.js b/packages/exo/src/exo-tools.js index 4b2250da11..41b83a04be 100644 --- a/packages/exo/src/exo-tools.js +++ b/packages/exo/src/exo-tools.js @@ -1,4 +1,5 @@ -import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { getMethodNames } from '@endo/eventual-send/utils.js'; +import { hasOwnPropertyOf } from '@endo/pass-style'; import { E, Far } from '@endo/far'; import { listDifference, @@ -423,7 +424,6 @@ export const defendPrototype = ( ...(symbolMethodGuards && fromEntries(getCopyMapEntries(symbolMethodGuards))), }); - assert(methodGuards !== undefined); defaultGuards = dg; { const methodGuardNames = ownKeys(methodGuards); diff --git a/packages/exo/src/get-interface.js b/packages/exo/src/get-interface.js index 1d1d894d36..1657cf3c53 100644 --- a/packages/exo/src/get-interface.js +++ b/packages/exo/src/get-interface.js @@ -4,13 +4,16 @@ * The name of the automatically added default meta-method for * obtaining an exo's interface, if it has one. * + * Intended to be similar to `GET_METHOD_NAMES` from `@endo/pass-style`. + * * TODO Name to be bikeshed. Perhaps even whether it is a - * string or symbol to be bikeshed. + * string or symbol to be bikeshed. See + * https://github.com/endojs/endo/pull/1809#discussion_r1388052454 * * TODO Beware that an exo's interface can change across an upgrade, * so remotes that cache it can become stale. */ -export const GET_INTERFACE_GUARD = Symbol.for('getInterfaceGuard'); +export const GET_INTERFACE_GUARD = '__getInterfaceGuard__'; /** * @template {Record} M diff --git a/packages/exo/test/test-exo-wobbly-point.js b/packages/exo/test/test-exo-wobbly-point.js index 84be64755b..59ecc6e380 100644 --- a/packages/exo/test/test-exo-wobbly-point.js +++ b/packages/exo/test/test-exo-wobbly-point.js @@ -10,10 +10,9 @@ /* eslint-disable-next-line import/order */ import { test } from './prepare-test-env-ava.js'; -// TODO enable import of getMethodNames without deep import // eslint-disable-next-line import/order -import { getMethodNames } from '@endo/eventual-send/src/local.js'; -import { passStyleOf, Far } from '@endo/pass-style'; +import { getMethodNames } from '@endo/eventual-send/utils.js'; +import { passStyleOf, Far, GET_METHOD_NAMES } from '@endo/pass-style'; import { M } from '@endo/patterns'; import { defineExoClass } from '../src/exo-makers.js'; import { GET_INTERFACE_GUARD } from '../src/get-interface.js'; @@ -96,12 +95,18 @@ harden(ExoPoint); const makeExoPoint = defineExoClassFromJSClass(ExoPoint); +const assertMethodNames = (t, obj, names) => { + t.deepEqual(getMethodNames(obj), names); + t.deepEqual(obj[GET_METHOD_NAMES](), names); +}; + test('ExoPoint instances', t => { const pt = makeExoPoint(3, 5); t.is(passStyleOf(pt), 'remotable'); t.false(pt instanceof ExoPoint); - t.deepEqual(getMethodNames(pt), [ + assertMethodNames(t, pt, [ GET_INTERFACE_GUARD, + GET_METHOD_NAMES, 'getX', 'getY', 'setY', @@ -152,8 +157,9 @@ test('FarWobblyPoint inheritance', t => { t.false(wpt instanceof ExoWobblyPoint); t.false(wpt instanceof ExoPoint); t.is(passStyleOf(wpt), 'remotable'); - t.deepEqual(getMethodNames(wpt), [ + assertMethodNames(t, wpt, [ GET_INTERFACE_GUARD, + GET_METHOD_NAMES, 'getX', 'getY', 'setY', diff --git a/packages/exo/test/test-heap-classes.js b/packages/exo/test/test-heap-classes.js index 1d2c72d95e..c6c3525cf7 100644 --- a/packages/exo/test/test-heap-classes.js +++ b/packages/exo/test/test-heap-classes.js @@ -345,7 +345,7 @@ test('naked function call', t => { t.throws(() => gigm(), { message: - 'thisful method "In \\"[Symbol(getInterfaceGuard)]\\" method of (greeter)" called without \'this\' object', + 'thisful method "In \\"__getInterfaceGuard__\\" method of (greeter)" called without \'this\' object', }); t.deepEqual(gigm.bind(greeter)(), GreeterI); }); diff --git a/packages/marshal/test/test-marshal-testing.js b/packages/marshal/test/test-marshal-testing.js index 241887b5d8..7721e61efb 100644 --- a/packages/marshal/test/test-marshal-testing.js +++ b/packages/marshal/test/test-marshal-testing.js @@ -1,14 +1,18 @@ import { test } from './prepare-test-env-ava.js'; // eslint-disable-next-line import/order -import { Far, passStyleOf, Remotable } from '@endo/pass-style'; +import { passStyleOf, Remotable } from '@endo/pass-style'; import { makeMarshal } from '../src/marshal.js'; const { create } = Object; -const alice = Far('alice'); -const bob1 = Far('bob'); -const bob2 = Far('bob'); +// Use the lower level `Remotable` rather than `Far` to make an empty +// far object, i.e., one without even the miranda meta methods like +// `GET_METHOD_NAMES`. Such an empty far object should be `t.deepEqual` +// to its remote presences. +const alice = Remotable('Alleged: alice'); +const bob1 = Remotable('Alleged: bob'); +const bob2 = Remotable('Alleged: bob'); const convertValToSlot = val => passStyleOf(val) === 'remotable' ? 'far' : val; diff --git a/packages/pass-style/index.js b/packages/pass-style/index.js index ef54b29c9f..da985131a2 100644 --- a/packages/pass-style/index.js +++ b/packages/pass-style/index.js @@ -24,7 +24,12 @@ export { export { passStyleOf, assertPassable } from './src/passStyleOf.js'; export { makeTagged } from './src/makeTagged.js'; -export { Remotable, Far, ToFarFunction } from './src/make-far.js'; +export { + Remotable, + Far, + ToFarFunction, + GET_METHOD_NAMES, +} from './src/make-far.js'; export { assertRecord, diff --git a/packages/pass-style/package.json b/packages/pass-style/package.json index 82fc4f8d9e..45bf1234db 100644 --- a/packages/pass-style/package.json +++ b/packages/pass-style/package.json @@ -33,11 +33,11 @@ "test": "ava" }, "dependencies": { + "@endo/eventual-send": "^0.17.6", "@endo/promise-kit": "^0.2.60", "@fast-check/ava": "^1.1.5" }, "devDependencies": { - "@endo/eventual-send": "^0.17.6", "@endo/init": "^0.5.60", "@endo/ses-ava": "^0.2.44", "ava": "^5.3.0", diff --git a/packages/pass-style/src/make-far.js b/packages/pass-style/src/make-far.js index d77ad60c66..0b7b0916b0 100644 --- a/packages/pass-style/src/make-far.js +++ b/packages/pass-style/src/make-far.js @@ -1,5 +1,6 @@ /// +import { getMethodNames } from '@endo/eventual-send/utils.js'; import { assertChecker, PASS_STYLE } from './passStyle-helpers.js'; import { assertIface, getInterfaceOf, RemotableHelper } from './remotable.js'; @@ -128,9 +129,41 @@ export const Remotable = ( }; harden(Remotable); +/** + * The name of the automatically added default meta-method for obtaining a + * list of all methods of an object declared with `Far`, or an object that + * inherits from an object declared with `Far`. + * + * Modeled on `GET_INTERFACE_GUARD` from `@endo/exo`. + * + * TODO Name to be bikeshed. Perhaps even whether it is a + * string or symbol to be bikeshed. See + * https://github.com/endojs/endo/pull/1809#discussion_r1388052454 + * + * HAZARD: Beware that an exo's interface can change across an upgrade, + * so remotes that cache it can become stale. + */ +export const GET_METHOD_NAMES = '__getMethodNames__'; + +/** + * Note that `getMethodNamesMethod` is a thisful method! It must be so that + * it works as expected with far-object inheritance. + * + * @returns {(string|symbol)[]} + */ +const getMethodNamesMethod = harden({ + [GET_METHOD_NAMES]() { + return getMethodNames(this); + }, +})[GET_METHOD_NAMES]; + /** * A concise convenience for the most common `Remotable` use. * + * For far objects (as opposed to far functions), also adds a miranda + * `GET_METHOD_NAMES` method that returns an array of all the method names, + * if there is not yet any method named `GET_METHOD_NAMES`. (Hence "miranda") + * * @template {{}} T * @param {string} farName This name will be prepended with `Alleged: ` * for now to form the `Remotable` `iface` argument. @@ -138,6 +171,16 @@ harden(Remotable); */ export const Far = (farName, remotable = undefined) => { const r = remotable === undefined ? /** @type {T} */ ({}) : remotable; + if (typeof r === 'object' && !(GET_METHOD_NAMES in r)) { + // This test excludes far functions, since we currently consider them + // to only have a call-behavior, with no callable methods. + // Beware: Mutates the input argument! But `Remotable` + // * requires the object to be mutable + // * does further mutations, + // * hardens the mutated object before returning it. + // so this mutation is not unprecedented. But it is surprising! + r[GET_METHOD_NAMES] = getMethodNamesMethod; + } return Remotable(`Alleged: ${farName}`, undefined, r); }; harden(Far); diff --git a/packages/pass-style/test/test-far-class-instances.js b/packages/pass-style/test/test-far-class-instances.js index 7b98427ce4..ec5af211a5 100644 --- a/packages/pass-style/test/test-far-class-instances.js +++ b/packages/pass-style/test/test-far-class-instances.js @@ -2,11 +2,10 @@ /* eslint-disable max-classes-per-file */ import { test } from './prepare-test-env-ava.js'; -// TODO enable import of getMethodNames without deep import // eslint-disable-next-line import/order -import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { getMethodNames } from '@endo/eventual-send/utils.js'; import { passStyleOf } from '../src/passStyleOf.js'; -import { Far } from '../src/make-far.js'; +import { Far, GET_METHOD_NAMES } from '../src/make-far.js'; /** * Classes whose instances should be Far objects may find it convenient to @@ -17,7 +16,7 @@ import { Far } from '../src/make-far.js'; * this object's identity. However, we discourage (but cannot prevent) such * use of private fields, as they cannot easily be refactored into Exo state. */ -export const FarBaseClass = class FarBaseClass { +const FarBaseClass = class FarBaseClass { constructor() { harden(this); } @@ -45,10 +44,15 @@ class FarSubclass2 extends FarSubclass1 { } } +const assertMethodNames = (t, obj, names) => { + t.deepEqual(getMethodNames(obj), names); + t.deepEqual(obj[GET_METHOD_NAMES](), names); +}; + test('far class instances', t => { const fb = new FarBaseClass(); t.is(passStyleOf(fb), 'remotable'); - t.deepEqual(getMethodNames(fb), ['constructor']); + assertMethodNames(t, fb, [GET_METHOD_NAMES, 'constructor']); t.assert(new fb.constructor() instanceof FarBaseClass); t.throws(() => fb.constructor(), { @@ -60,13 +64,18 @@ test('far class instances', t => { t.is(passStyleOf(fs1), 'remotable'); t.is(fs1.double(4), 8); t.assert(new fs1.constructor() instanceof FarSubclass1); - t.deepEqual(getMethodNames(fs1), ['constructor', 'double']); + assertMethodNames(t, fs1, [GET_METHOD_NAMES, 'constructor', 'double']); const fs2 = new FarSubclass2(3); t.is(passStyleOf(fs2), 'remotable'); t.is(fs2.double(4), 8); t.is(fs2.doubleAdd(4), 11); - t.deepEqual(getMethodNames(fs2), ['constructor', 'double', 'doubleAdd']); + assertMethodNames(t, fs2, [ + GET_METHOD_NAMES, + 'constructor', + 'double', + 'doubleAdd', + ]); const yField = new WeakMap(); class FarSubclass3 extends FarSubclass1 { @@ -84,7 +93,12 @@ test('far class instances', t => { t.is(passStyleOf(fs3), 'remotable'); t.is(fs3.double(4), 8); t.is(fs3.doubleAdd(4), 11); - t.deepEqual(getMethodNames(fs3), ['constructor', 'double', 'doubleAdd']); + assertMethodNames(t, fs3, [ + GET_METHOD_NAMES, + 'constructor', + 'double', + 'doubleAdd', + ]); }); test('far class instance hardened empty', t => { diff --git a/packages/pass-style/test/test-far-wobbly-point.js b/packages/pass-style/test/test-far-wobbly-point.js index 34905c093d..e46060d91c 100644 --- a/packages/pass-style/test/test-far-wobbly-point.js +++ b/packages/pass-style/test/test-far-wobbly-point.js @@ -7,11 +7,10 @@ /* eslint-disable max-classes-per-file */ import { test } from './prepare-test-env-ava.js'; -// TODO enable import of getMethodNames without deep import // eslint-disable-next-line import/order -import { getMethodNames } from '@endo/eventual-send/src/local.js'; +import { getMethodNames } from '@endo/eventual-send/utils.js'; import { passStyleOf } from '../src/passStyleOf.js'; -import { Far } from '../src/make-far.js'; +import { Far, GET_METHOD_NAMES } from '../src/make-far.js'; const { apply } = Reflect; @@ -61,11 +60,17 @@ class FarPoint extends FarBaseClass { } harden(FarPoint); +const assertMethodNames = (t, obj, names) => { + t.deepEqual(getMethodNames(obj), names); + t.deepEqual(obj[GET_METHOD_NAMES](), names); +}; + test('FarPoint instances', t => { const pt = new FarPoint(3, 5); t.is(passStyleOf(pt), 'remotable'); t.assert(pt instanceof FarPoint); - t.deepEqual(getMethodNames(pt), [ + assertMethodNames(t, pt, [ + GET_METHOD_NAMES, 'constructor', 'getX', 'getY', @@ -103,7 +108,8 @@ test('FarWobblyPoint inheritance', t => { t.assert(wpt instanceof FarWobblyPoint); t.assert(wpt instanceof FarPoint); t.is(passStyleOf(wpt), 'remotable'); - t.deepEqual(getMethodNames(wpt), [ + assertMethodNames(t, wpt, [ + GET_METHOD_NAMES, 'constructor', 'getX', 'getY', diff --git a/packages/pass-style/test/test-passStyleOf.js b/packages/pass-style/test/test-passStyleOf.js index 9acf2cf6a7..ea6610e5af 100644 --- a/packages/pass-style/test/test-passStyleOf.js +++ b/packages/pass-style/test/test-passStyleOf.js @@ -266,7 +266,8 @@ test('passStyleOf testing remotables', t => { class NonFarBaseClass9 {} class Subclass9 extends NonFarBaseClass9 {} t.throws(() => Far('FarType9', Subclass9.prototype), { - message: 'For now, remotables cannot inherit from anything unusual, in {}', + message: + 'For now, remotables cannot inherit from anything unusual, in {"__getMethodNames__":"[Function __getMethodNames__]"}', }); const unusualTagRecordProtoMessage = From 1f29d4ae95013881d0ec113c202d0fdf00647b4d Mon Sep 17 00:00:00 2001 From: "Mark S. Miller" Date: Mon, 6 Nov 2023 14:04:46 -0800 Subject: [PATCH 3/3] chore(eventual-send)!: restrict exports --- packages/eventual-send/package.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/eventual-send/package.json b/packages/eventual-send/package.json index 232e1a42da..388018c587 100644 --- a/packages/eventual-send/package.json +++ b/packages/eventual-send/package.json @@ -18,6 +18,12 @@ "lint:types": "tsc", "lint:eslint": "eslint '**/*.js'" }, + "exports": { + "./package.json": "./package.json", + ".": "./src/no-shim.js", + "./shim.js": "./shim.js", + "./utils.js": "./utils.js" + }, "repository": { "type": "git", "url": "git+https://github.com/endojs/endo.git"