fix(store): reject empty-object keys which might not retain identity

When #2018 changes the interpretation of `harden({})` to become pass-by-copy,
any code which was previously using that to make a "handle" will break,
because the things they send will be treated as pass-by-copy. To catch cases
where this is happening, we forbid such keys from being used in
store/weakstore. Clients should use `Far('interfacename')` to create handles,
and these will be accepted by store/weakstore as keys.

packages/store/package.json

@@ -33,7 +33,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",

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);

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);
     },

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);
     },

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:/ });
 });