diff --git a/packages/marshal/src/marshal.js b/packages/marshal/src/marshal.js index 942b9674a83..359ef6df554 100644 --- a/packages/marshal/src/marshal.js +++ b/packages/marshal/src/marshal.js @@ -11,6 +11,7 @@ import { getInterfaceOf, getErrorConstructor, assertCanBeRemotable, + assertIface, } from './passStyleOf'; import './types'; @@ -119,18 +120,23 @@ function pureCopy(val, already = new WeakMap()) { harden(pureCopy); export { pureCopy }; -const makeRemotableProto = (oldProto, allegedName) => { +/** + * @param {Object|null} oldProto + * @param {InterfaceSpec} iface + * @returns {Object} + */ +const makeRemotableProto = (oldProto, iface) => { assert( oldProto === objectPrototype || oldProto === null, X`For now, remotables cannot inherit from anything unusual`, ); // Assign the arrow function to a variable to set its .name. - const toString = () => `[${allegedName}]`; + const toString = () => `[${iface}]`; return harden( create(oldProto, { [PASS_STYLE]: { value: 'remotable' }, toString: { value: toString }, - [Symbol.toStringTag]: { value: allegedName }, + [Symbol.toStringTag]: { value: iface }, }), ); }; @@ -629,24 +635,12 @@ export function makeMarshal( * @param {object} [remotable={}] The object used as the remotable * @returns {object} remotable, modified for debuggability */ -const Remotable = (iface = 'Remotable', props = undefined, remotable = {}) => { - // TODO unimplemented - assert.typeof( - iface, - 'string', - X`Interface ${iface} must be a string; unimplemented`, - ); - // TODO unimplemented - assert( - iface === 'Remotable' || iface.startsWith('Alleged: '), - X`For now, iface ${q( - iface, - )} must be "Remotable" or begin with "Alleged: "; unimplemented`, - ); +function Remotable(iface = 'Remotable', props = undefined, remotable = {}) { + assertIface(iface); iface = pureCopy(harden(iface)); + assert(iface); // TODO: When iface is richer than just string, we need to get the allegedName // in a different way. - const allegedName = iface; assert(props === undefined, X`Remotable props not yet implemented ${props}`); // Fail fast: check that the unmodified object is able to become a Remotable. @@ -661,10 +655,7 @@ const Remotable = (iface = 'Remotable', props = undefined, remotable = {}) => { ); // Ensure that the remotable isn't already frozen. assert(!isFrozen(remotable), X`Remotable ${remotable} is already frozen`); - const remotableProto = makeRemotableProto( - getPrototypeOf(remotable), - allegedName, - ); + const remotableProto = makeRemotableProto(getPrototypeOf(remotable), iface); // Take a static copy of the enumerable own properties as data properties. // const propDescs = getOwnPropertyDescriptors({ ...props }); @@ -685,7 +676,7 @@ const Remotable = (iface = 'Remotable', props = undefined, remotable = {}) => { // We're committed, so keep the interface for future reference. assert(iface !== undefined); // To make TypeScript happy return remotable; -}; +} harden(Remotable); export { Remotable }; diff --git a/packages/marshal/src/passStyleOf.js b/packages/marshal/src/passStyleOf.js index 079786d1281..681130e447e 100644 --- a/packages/marshal/src/passStyleOf.js +++ b/packages/marshal/src/passStyleOf.js @@ -8,6 +8,15 @@ import { isPromise } from '@agoric/promise-kit'; import './types'; +// Setting this flag to true is what allows objects with `null` or +// `Object.prototype` prototypes to be treated as remotable. Setting to `false` +// means that only objects declared with `Remotable(...)`, including `Far(...)` +// can be used as remotables. +// +// TODO: once the policy changes to force remotables to be explicit, remove this +// flag entirely and fix code that uses it (as if it were always `false`). +const ALLOW_IMPLICIT_REMOTABLES = true; + const { getPrototypeOf, getOwnPropertyDescriptors, @@ -19,24 +28,6 @@ const { ownKeys } = Reflect; export const PASS_STYLE = Symbol.for('passStyle'); -/** @type {MarshalGetInterfaceOf} */ -export function getInterfaceOf(val) { - if (typeof val !== 'object' || val === null) { - return undefined; - } - if (val[PASS_STYLE] !== 'remotable') { - return undefined; - } - assert(isFrozen(val), X`Remotable ${val} must be frozen`, TypeError); - const iface = val[Symbol.toStringTag]; - assert.typeof( - iface, - 'string', - X`Remotable interface currently can only be a string`, - ); - return iface; -} - const errorConstructors = new Map([ ['Error', Error], ['EvalError', EvalError], @@ -190,95 +181,213 @@ function isPassByCopyRecord(val) { return true; } +// Below we have a series of predicate functions and their (curried) assertion +// functions. The semantics of the assertion function is just to assert that +// the corresponding predicate function would have returned true. But it +// reproduces the internal tests so failures can give a better error message. + /** - * Throw if val is not the correct shape for the prototype of a Remotable. - * - * TODO: It would be nice to typedef this shape and then declare that this - * function asserts it, but we can't declare a type with PASS_STYLE from JSDoc. + * @callback Checker + * @param {boolean} cond + * @param {Parameters[1]} details + * @returns {boolean} + */ + +/** @type {Checker} */ +const assertChecker = (cond, details) => { + assert(cond, details); + return true; +}; + +/** + * @param {InterfaceSpec} iface + * @param {Checker} check + */ +const checkIface = (iface, check = x => x) => { + return ( + // TODO other possible ifaces, once we have third party veracity + check( + typeof iface === 'string', + X`Interface ${iface} must be a string; unimplemented`, + ) && + check( + iface === 'Remotable' || iface.startsWith('Alleged: '), + X`For now, iface ${q( + iface, + )} must be "Remotable" or begin with "Alleged: "; unimplemented`, + ) + ); +}; + +/** + * @param {InterfaceSpec} iface + */ +const assertIface = iface => checkIface(iface, assertChecker); +harden(assertIface); +export { assertIface }; + +/** + * TODO: It would be nice to typedef this shape, but we can't declare a type + * with PASS_STYLE from JSDoc. * - * @param {{ [PASS_STYLE]: string, [Symbol.toStringTag]: string, toString: () => - * void}} val the value to verify + * @param {{ + * [PASS_STYLE]: string, + * [Symbol.toStringTag]: string, + * toString: () => void }} val the value to verify + * @param {Checker} [check] + * @returns {boolean} */ -const assertRemotableProto = val => { - assert.typeof(val, 'object', X`cannot serialize non-objects like ${val}`); - assert(!Array.isArray(val), X`Arrays cannot be pass-by-remote`); - assert(val !== null, X`null cannot be pass-by-remote`); +const checkRemotableProto = (val, check = x => x) => { + if ( + !( + check( + typeof val === 'object', + X`cannot serialize non-objects like ${val}`, + ) && + check(!Array.isArray(val), X`Arrays cannot be pass-by-remote`) && + check(val !== null, X`null cannot be pass-by-remote`) + ) + ) { + return false; + } const protoProto = getPrototypeOf(val); - assert( - protoProto === objectPrototype || protoProto === null, - X`The Remotable Proto marker cannot inherit from anything unusual`, - ); - assert(isFrozen(val), X`The Remotable proto must be frozen`); + if ( + !( + check( + protoProto === objectPrototype || protoProto === null, + X`The Remotable Proto marker cannot inherit from anything unusual`, + ) && check(isFrozen(val), X`The Remotable proto must be frozen`) + ) + ) { + return false; + } + const { - [PASS_STYLE]: { value: passStyleValue }, - toString: { value: toStringValue }, + [PASS_STYLE]: passStyleDesc, + toString: toStringDesc, // @ts-ignore https://github.com/microsoft/TypeScript/issues/1863 - [Symbol.toStringTag]: { value: toStringTagValue }, + [Symbol.toStringTag]: ifaceDesc, ...rest } = getOwnPropertyDescriptors(val); - assert( - ownKeys(rest).length === 0, - X`Unexpect properties on Remotable Proto ${ownKeys(rest)}`, - ); - assert( - passStyleValue === 'remotable', - X`Expected 'remotable', not ${q(passStyleValue)}`, + + return ( + check( + ownKeys(rest).length === 0, + X`Unexpected properties on Remotable Proto ${ownKeys(rest)}`, + ) && + check(!!passStyleDesc, X`Remotable must have a [PASS_STYLE]`) && + check( + passStyleDesc.value === 'remotable', + X`Expected 'remotable', not ${q(passStyleDesc.value)}`, + ) && + check( + typeof toStringDesc.value === 'function', + X`toString must be a function`, + ) && + checkIface(ifaceDesc && ifaceDesc.value, check) ); - assert.typeof(toStringValue, 'function', X`toString must be a function`); - assert.typeof(toStringTagValue, 'string', X`@@toStringTag must be a string`); }; /** - * Ensure that val could become a legitimate remotable. This is used - * internally both in the construction of a new remotable and - * mustPassByRemote. + * Ensure that val could become a legitimate remotable. This is used internally + * both in the construction of a new remotable and checkRemotable. * * @param {*} val The remotable candidate to check + * @param {Checker} [check] + * @returns {boolean} */ -export function assertCanBeRemotable(val) { - // throws exception if cannot - assert.typeof(val, 'object', X`cannot serialize non-objects like ${val}`); - assert(!Array.isArray(val), X`Arrays cannot be pass-by-remote`); - assert(val !== null, X`null cannot be pass-by-remote`); +function checkCanBeRemotable(val, check = x => x) { + if ( + !( + check( + typeof val === 'object', + X`cannot serialize non-objects like ${val}`, + ) && + check(!Array.isArray(val), X`Arrays cannot be pass-by-remote`) && + check(val !== null, X`null cannot be pass-by-remote`) + ) + ) { + return false; + } const descs = getOwnPropertyDescriptors(val); const keys = ownKeys(descs); // enumerable-and-not, string-or-Symbol - keys.forEach(key => { - assert( + return keys.every( + key => // Typecast needed due to https://github.com/microsoft/TypeScript/issues/1863 - !('get' in descs[/** @type {string} */ (key)]), - X`cannot serialize objects with getters like ${q(String(key))} in ${val}`, - ); - assert.typeof( - // @ts-ignore https://github.com/microsoft/TypeScript/issues/1863 - val[key], - 'function', - X`cannot serialize objects with non-methods like ${q( - String(key), - )} in ${val}`, - ); - assert( - key !== PASS_STYLE, - X`A pass-by-remote cannot shadow ${q(PASS_STYLE)}`, - ); - }); + check( + !('get' in descs[/** @type {string} */ (key)]), + X`cannot serialize objects with getters like ${q( + String(key), + )} in ${val}`, + ) && + check( + typeof val[key] === 'function', + X`cannot serialize objects with non-methods like ${q( + String(key), + )} in ${val}`, + ) && + check( + key !== PASS_STYLE, + X`A pass-by-remote cannot shadow ${q(PASS_STYLE)}`, + ), + ); } +const canBeRemotable = val => checkCanBeRemotable(val); +harden(canBeRemotable); +export { canBeRemotable }; + +const assertCanBeRemotable = val => { + checkCanBeRemotable(val, assertChecker); +}; +harden(assertCanBeRemotable); +export { assertCanBeRemotable }; + /** * @param {Remotable} val + * @param {Checker} [check] + * @returns {boolean} */ -function assertRemotable(val) { - assert(isFrozen(val), X`cannot serialize non-frozen objects like ${val}`); - - assertCanBeRemotable(val); - +function checkRemotable(val, check = x => x) { + const not = (cond, details) => !check(cond, details); + if (not(isFrozen(val), X`cannot serialize non-frozen objects like ${val}`)) { + return false; + } + if (!checkCanBeRemotable(val, check)) { + return false; + } const p = getPrototypeOf(val); - if (p !== null && p !== objectPrototype) { - assertRemotableProto(p); + + if (ALLOW_IMPLICIT_REMOTABLES && (p === null || p === objectPrototype)) { + return true; } + return checkRemotableProto(p, check); } +/** + * @param {Remotable} val + */ +const assertRemotable = val => { + checkRemotable(val, assertChecker); +}; + +/** @type {MarshalGetInterfaceOf} */ +const getInterfaceOf = val => { + if ( + typeof val !== 'object' || + val === null || + val[PASS_STYLE] !== 'remotable' || + !checkRemotable(val) + ) { + return undefined; + } + return val[Symbol.toStringTag]; +}; +harden(getInterfaceOf); +export { getInterfaceOf }; + /** * objects can only be passed in one of two/three forms: * 1: pass-by-remote: all properties (own and inherited) are methods, diff --git a/packages/marshal/test/test-marshal.js b/packages/marshal/test/test-marshal.js index 6a0e2a9ccef..9ddbb8b26b9 100644 --- a/packages/marshal/test/test-marshal.js +++ b/packages/marshal/test/test-marshal.js @@ -302,6 +302,87 @@ test('Remotable/getInterfaceOf', t => { }); }); +const GOOD_PASS_STYLE = Symbol.for('passStyle'); +const BAD_PASS_STYLE = Symbol('passStyle'); + +const goodRemotableProto = harden({ + [GOOD_PASS_STYLE]: 'remotable', + toString: Object, // Any function will do + [Symbol.toStringTag]: 'Alleged: Good remotable proto', +}); + +const badRemotableProto1 = harden({ + [BAD_PASS_STYLE]: 'remotable', + toString: Object, // Any function will do + [Symbol.toStringTag]: 'Alleged: Good remotable proto', +}); +const badRemotableProto2 = harden({ + [GOOD_PASS_STYLE]: 'string', + toString: Object, // Any function will do + [Symbol.toStringTag]: 'Alleged: Good remotable proto', +}); +const badRemotableProto3 = harden({ + [GOOD_PASS_STYLE]: 'remotable', + toString: {}, // Any function will do + [Symbol.toStringTag]: 'Alleged: Good remotable proto', +}); +const badRemotableProto4 = harden({ + [GOOD_PASS_STYLE]: 'remotable', + toString: Object, // Any function will do + [Symbol.toStringTag]: 'Bad remotable proto', +}); + +const sub = sup => harden({ __proto__: sup }); + +test('getInterfaceOf validation', t => { + t.is(getInterfaceOf(goodRemotableProto), undefined); + t.is(getInterfaceOf(badRemotableProto1), undefined); + t.is(getInterfaceOf(badRemotableProto2), undefined); + t.is(getInterfaceOf(badRemotableProto3), undefined); + t.is(getInterfaceOf(badRemotableProto4), undefined); + + t.is( + getInterfaceOf(sub(goodRemotableProto)), + 'Alleged: Good remotable proto', + ); + t.is(getInterfaceOf(sub(badRemotableProto1)), undefined); + t.is(getInterfaceOf(sub(badRemotableProto2)), undefined); + t.is(getInterfaceOf(sub(badRemotableProto3)), undefined); + t.is(getInterfaceOf(sub(badRemotableProto4)), undefined); +}); + +const NON_METHOD = { + message: /cannot serialize objects with non-methods like .* in .*/, +}; +const TO_STRING_NONFUNC = { + message: /toString must be a function/, +}; +const IFACE_ALLEGED = { + message: /For now, iface "Bad remotable proto" must be "Remotable" or begin with "Alleged: "; unimplemented/, +}; +const UNEXPECTED_PROPS = { + message: /Unexpected properties on Remotable Proto .*/, +}; +const EXPECTED_PRESENCE = { + message: /Expected 'remotable', not "string"/, +}; + +// Parallels the getInterfaceOf validation cases, explaining why +// each failure failed. +test('passStyleOf validation of remotables', t => { + t.throws(() => passStyleOf(goodRemotableProto), NON_METHOD); + t.throws(() => passStyleOf(badRemotableProto1), NON_METHOD); + t.throws(() => passStyleOf(badRemotableProto2), NON_METHOD); + t.throws(() => passStyleOf(badRemotableProto3), NON_METHOD); + t.throws(() => passStyleOf(badRemotableProto4), NON_METHOD); + + t.is(passStyleOf(sub(goodRemotableProto)), 'remotable'); + t.throws(() => passStyleOf(sub(badRemotableProto1)), UNEXPECTED_PROPS); + t.throws(() => passStyleOf(sub(badRemotableProto2)), EXPECTED_PRESENCE); + t.throws(() => passStyleOf(sub(badRemotableProto3)), TO_STRING_NONFUNC); + t.throws(() => passStyleOf(sub(badRemotableProto4)), IFACE_ALLEGED); +}); + test('records', t => { function convertValToSlot(_val) { return 'slot';