diff --git a/packages/store/package.json b/packages/store/package.json index 6296a7f26d2..6013adc3efe 100644 --- a/packages/store/package.json +++ b/packages/store/package.json @@ -32,7 +32,8 @@ }, "homepage": "https://github.com/Agoric/agoric-sdk#readme", "dependencies": { - "@agoric/assert": "^0.2.2" + "@agoric/assert": "^0.2.2", + "@agoric/marshal": "^0.3.2" }, "devDependencies": { "@agoric/install-ses": "^0.5.2", diff --git a/packages/store/src/helper.js b/packages/store/src/helper.js new file mode 100644 index 00000000000..1c341d39ff8 --- /dev/null +++ b/packages/store/src/helper.js @@ -0,0 +1,20 @@ +// @ts-check + +import { getInterfaceOf } from '@agoric/marshal'; + +/** + * Helper function to reject keys which are empty objects but not marked as + * Remotable. This is intended to catch code which uses harden({}) (which + * will become pass-by-copy, see #2018) as a "handle" or "marker object" + * when they should have used Far(). + * + * @param { unknown } key + */ +export function isEmptyNonRemotableObject(key) { + return ( + typeof key === 'object' && + Reflect.ownKeys(key).length === 0 && + getInterfaceOf(key) === undefined + ); +} +harden(isEmptyNonRemotableObject); diff --git a/packages/store/src/store.js b/packages/store/src/store.js index 9e116304af2..1e59dba98e5 100644 --- a/packages/store/src/store.js +++ b/packages/store/src/store.js @@ -3,6 +3,7 @@ // @ts-check import { assert, details as X, q } from '@agoric/assert'; +import { isEmptyNonRemotableObject } from './helper'; /** * Distinguishes between adding a new key (init) and updating or @@ -21,21 +22,30 @@ export function makeStore(keyName = 'key') { assert(!store.has(key), X`${q(keyName)} already registered: ${key}`); const assertKeyExists = key => assert(store.has(key), X`${q(keyName)} not found: ${key}`); + const assertNotBadKey = key => + assert(!isEmptyNonRemotableObject(key), X`${q(keyName)} bad key: ${key}`); return harden({ - has: key => store.has(key), + has: key => { + assertNotBadKey(key); + return store.has(key); + }, init: (key, value) => { + assertNotBadKey(key); assertKeyDoesNotExist(key); store.set(key, value); }, get: key => { + assertNotBadKey(key); assertKeyExists(key); return store.get(key); }, set: (key, value) => { + assertNotBadKey(key); assertKeyExists(key); store.set(key, value); }, delete: key => { + assertNotBadKey(key); assertKeyExists(key); store.delete(key); }, diff --git a/packages/store/src/weak-store.js b/packages/store/src/weak-store.js index 35d202ede34..fc8b8ff103f 100644 --- a/packages/store/src/weak-store.js +++ b/packages/store/src/weak-store.js @@ -3,6 +3,7 @@ // @ts-check import { assert, details as X, q } from '@agoric/assert'; +import { isEmptyNonRemotableObject } from './helper'; import './types'; /** @@ -17,21 +18,30 @@ export function makeWeakStore(keyName = 'key') { assert(!wm.has(key), X`${q(keyName)} already registered: ${key}`); const assertKeyExists = key => assert(wm.has(key), X`${q(keyName)} not found: ${key}`); + const assertNotBadKey = key => + assert(!isEmptyNonRemotableObject(key), X`${q(keyName)} bad key: ${key}`); return harden({ - has: key => wm.has(key), + has: key => { + assertNotBadKey(key); + return wm.has(key); + }, init: (key, value) => { + assertNotBadKey(key); assertKeyDoesNotExist(key); wm.set(key, value); }, get: key => { + assertNotBadKey(key); assertKeyExists(key); return wm.get(key); }, set: (key, value) => { + assertNotBadKey(key); assertKeyExists(key); wm.set(key, value); }, delete: key => { + assertNotBadKey(key); assertKeyExists(key); wm.delete(key); }, diff --git a/packages/store/test/test-store.js b/packages/store/test/test-store.js index 4b8f4450365..f36ac4d2954 100644 --- a/packages/store/test/test-store.js +++ b/packages/store/test/test-store.js @@ -2,9 +2,17 @@ /* eslint-disable no-use-before-define */ import '@agoric/install-ses'; import test from 'ava'; +import { Far } from '@agoric/marshal'; import { makeStore, makeWeakStore } from '../src/index'; +import { isEmptyNonRemotableObject } from '../src/helper'; import '../src/types'; +test('empty object check', t => { + const f = isEmptyNonRemotableObject; + t.truthy(f(harden({}))); + t.falsy(f(Far())); +}); + function check(t, mode, objMaker) { // Check the full API, and make sure object identity isn't a problem by // creating two potentially-similar things for use as keys. @@ -78,8 +86,34 @@ test('store', t => { // makeStore check(t, 'strong', count => count); // simple numeric keys check(t, 'strong', count => `${count}`); // simple strings - check(t, 'strong', () => harden({})); + check(t, 'strong', () => Far('handle', {})); // makeWeakStore - check(t, 'weak', () => harden({})); + check(t, 'weak', () => Far('handle', {})); +}); + +test('reject unmarked empty objects', t => { + // Older client code used harden({}) to create a "handle" that served as an + // otherwise-empty key for a store/weakstore, but ticket #2018 changes + // marshal to treat unmarked empty objects as pass-by-copy, so they won't + // retain identity across messages, breaking old-style handles in + // surprising ways (key collisions). New client code should use Far() + // instead, which arrives here as an object with a non-empty + // getInterfaceOf(). To catch older clients that need to be updated, we + // reject the use of plain empty objects as keys. + + const k = harden({}); + const s = makeStore('store1'); + t.throws(() => s.init(k, 1), { message: /"store1" bad key:/ }); + t.throws(() => s.has(k), { message: /"store1" bad key:/ }); + t.throws(() => s.get(k), { message: /"store1" bad key:/ }); + t.throws(() => s.set(k, 1), { message: /"store1" bad key:/ }); + t.throws(() => s.delete(k), { message: /"store1" bad key:/ }); + + const w = makeWeakStore('store1'); + t.throws(() => w.init(k, 1), { message: /"store1" bad key:/ }); + t.throws(() => w.has(k), { message: /"store1" bad key:/ }); + t.throws(() => w.get(k), { message: /"store1" bad key:/ }); + t.throws(() => w.set(k, 1), { message: /"store1" bad key:/ }); + t.throws(() => w.delete(k), { message: /"store1" bad key:/ }); });