diff --git a/packages/marshal/test/test-encode-slot-in-promise.js b/packages/marshal/test/test-encode-slot-in-promise.js new file mode 100644 index 0000000000..baf5ee6e13 --- /dev/null +++ b/packages/marshal/test/test-encode-slot-in-promise.js @@ -0,0 +1,54 @@ +import { test } from './prepare-test-env-ava.js'; + +// eslint-disable-next-line import/order +import { passStyleOf } from '@endo/pass-style'; +import { makeMarshal } from '../src/marshal.js'; + +const { getOwnPropertyDescriptor, defineProperty } = Object; + +const { toStringTag } = Symbol; + +test('use safe promise loophole', t => { + const convertSlotToVal = (slot, _iface) => { + const p = Promise.resolve(`${slot} placeholder`); + defineProperty(p, toStringTag, { + value: `Promise ${slot}`, + }); + harden(p); + t.is(passStyleOf(p), 'promise'); + return p; + }; + + const PromiseNameRE = /^Promise (.*)$/; + + const convertValToSlot = p => { + t.is(passStyleOf(p), 'promise'); + const desc = getOwnPropertyDescriptor(p, toStringTag); + assert(desc !== undefined); + const name = desc.value; + t.is(typeof name, 'string'); + const match = PromiseNameRE.exec(name); + assert(Array.isArray(match)); + return match[1]; + }; + const { toCapData, fromCapData } = makeMarshal( + convertValToSlot, + convertSlotToVal, + { + serializeBodyFormat: 'smallcaps', + }, + ); + + const markedPromise = convertSlotToVal('I am kref 3'); + + const passable1 = harden([{ markedPromise }]); + const capData1 = toCapData(passable1); + t.deepEqual(capData1, { + body: '#[{"markedPromise":"&0"}]', + slots: ['I am kref 3'], + }); + const passable2 = fromCapData(capData1); + t.deepEqual(passable1, passable2); + const capData2 = toCapData(passable2); + t.deepEqual(capData1, capData2); +}); diff --git a/packages/pass-style/src/safe-promise.js b/packages/pass-style/src/safe-promise.js index 73d0760c22..30de135151 100644 --- a/packages/pass-style/src/safe-promise.js +++ b/packages/pass-style/src/safe-promise.js @@ -6,8 +6,9 @@ import { assertChecker, hasOwnPropertyOf } from './passStyle-helpers.js'; /** @typedef {import('./types.js').Checker} Checker */ const { details: X, quote: q } = assert; -const { isFrozen, getPrototypeOf } = Object; +const { isFrozen, getPrototypeOf, getOwnPropertyDescriptor } = Object; const { ownKeys } = Reflect; +const { toStringTag } = Symbol; /** * @param {Promise} pr The value to examine @@ -22,6 +23,15 @@ const checkPromiseOwnKeys = (pr, check) => { return true; } + /** + * This excludes those symbol-named own properties that are also found on + * `Promise.prototype`, so that overrides of these properties can be + * explicitly tolerated if they pass the `checkSafeOwnKey` check below. + * In particular, we wish to tolerate + * * An overriding `toStringTag` non-enumerable data property + * with a string value. + * * Those own properties that might be added by Node's async_hooks. + */ const unknownKeys = keys.filter( key => typeof key !== 'symbol' || !hasOwnPropertyOf(Promise.prototype, key), ); @@ -33,12 +43,17 @@ const checkPromiseOwnKeys = (pr, check) => { } /** + * Explicitly tolerate a `toStringTag` symbol-named non-enumerable + * data property whose value is a string. Otherwise, tolerate those + * symbol-named properties that might be added by NodeJS's async_hooks, + * if they obey the expected safety properties. + * * At the time of this writing, Node's async_hooks contains the - * following code, which we can also safely tolerate + * following code, which we can safely tolerate * * ```js * function destroyTracking(promise, parent) { - * trackPromise(promise, parent); + * trackPromise(promise, parent); * const asyncId = promise[async_id_symbol]; * const destroyed = { destroyed: false }; * promise[destroyedSymbol] = destroyed; @@ -48,7 +63,27 @@ const checkPromiseOwnKeys = (pr, check) => { * * @param {string|symbol} key */ - const checkSafeAsyncHooksKey = key => { + const checkSafeOwnKey = key => { + if (key === toStringTag) { + // TODO should we also enforce anything on the contents of the string, + // such as that it must start with `'Promise'`? + const tagDesc = getOwnPropertyDescriptor(pr, toStringTag); + assert(tagDesc !== undefined); + return ( + (hasOwnPropertyOf(tagDesc, 'value') || + reject( + X`Own @@toStringTag must be a data property, not an accessor: ${q( + tagDesc, + )}`, + )) && + (typeof tagDesc.value === 'string' || + reject( + X`Own @@toStringTag value must be a string: ${q(tagDesc.value)}`, + )) && + (!tagDesc.enumerable || + reject(X`Own @@toStringTag must not be enumerable: ${q(tagDesc)}`)) + ); + } const val = pr[key]; if (val === undefined || typeof val === 'number') { return true; @@ -79,7 +114,7 @@ const checkPromiseOwnKeys = (pr, check) => { ); }; - return keys.every(checkSafeAsyncHooksKey); + return keys.every(checkSafeOwnKey); }; /** diff --git a/packages/pass-style/test/test-safe-promise.js b/packages/pass-style/test/test-safe-promise.js new file mode 100644 index 0000000000..0a7f7f95f5 --- /dev/null +++ b/packages/pass-style/test/test-safe-promise.js @@ -0,0 +1,66 @@ +import { test } from './prepare-test-env-ava.js'; + +import { passStyleOf } from '../src/passStyleOf.js'; + +const { defineProperty } = Object; + +const { toStringTag } = Symbol; + +test('safe promise loophole', t => { + { + const p1 = Promise.resolve('p1'); + t.is(passStyleOf(harden(p1)), 'promise'); + t.is(p1[toStringTag], 'Promise'); + t.is(`${p1}`, '[object Promise]'); + } + + { + const p2 = Promise.resolve('p2'); + p2.silly = 'silly own property'; + t.throws(() => passStyleOf(harden(p2)), { + message: '"[Promise]" - Must not have any own properties: ["silly"]', + }); + t.is(p2[toStringTag], 'Promise'); + t.is(`${p2}`, '[object Promise]'); + } + + { + const p3 = Promise.resolve('p3'); + t.throws( + () => { + p3[toStringTag] = 3; + }, + { + // Override mistake + message: + "Cannot assign to read only property 'Symbol(Symbol.toStringTag)' of object '[object Promise]'", + }, + ); + defineProperty(p3, toStringTag, { + value: 3, + }); + t.throws(() => passStyleOf(harden(p3)), { + message: 'Own @@toStringTag value must be a string: 3', + }); + } + + { + const p4 = Promise.resolve('p4'); + defineProperty(p4, toStringTag, { + value: 'Promise p4', + enumerable: true, + }); + t.throws(() => passStyleOf(harden(p4)), { + message: + 'Own @@toStringTag must not be enumerable: {"configurable":false,"enumerable":true,"value":"Promise p4","writable":false}', + }); + + const p5 = Promise.resolve('p5'); + defineProperty(p5, toStringTag, { + value: 'Promise p5', + }); + t.is(passStyleOf(harden(p5)), 'promise'); + t.is(p5[toStringTag], 'Promise p5'); + t.is(`${p5}`, '[object Promise p5]'); + } +});