Restrict the size of the permissions metadata store (#8596)

* refactor add metadata functionality

* create pending site metadata cache

* remove metadata for domains w/o permissions if cache exceeds max size

app/scripts/controllers/permissions/enums.js

@@ -7,6 +7,8 @@ export const LOG_STORE_KEY = 'permissionsLog'
 
 export const METADATA_STORE_KEY = 'domainMetadata'
 
+export const METADATA_CACHE_MAX_SIZE = 100
+
 export const CAVEAT_NAMES = {
   exposedAccounts: 'exposedAccounts',
 }

app/scripts/controllers/permissions/index.js

@@ -14,6 +14,7 @@ import {
   SAFE_METHODS, // methods that do not require any permissions to use
   WALLET_PREFIX,
   METADATA_STORE_KEY,
+  METADATA_CACHE_MAX_SIZE,
   LOG_STORE_KEY,
   HISTORY_STORE_KEY,
   CAVEAT_NAMES,
@@ -35,8 +36,8 @@ export class PermissionsController {
     restoredPermissions = {},
     restoredState = {}) {
 
+    // additional top-level store key set in _initializeMetadataStore
     this.store = new ObservableStore({
-      [METADATA_STORE_KEY]: restoredState[METADATA_STORE_KEY] || {},
       [LOG_STORE_KEY]: restoredState[LOG_STORE_KEY] || [],
       [HISTORY_STORE_KEY]: restoredState[HISTORY_STORE_KEY] || {},
     })
@@ -61,6 +62,8 @@ export class PermissionsController {
     this._lastSelectedAddress = preferences.getState().selectedAddress
     this.preferences = preferences
 
+    this._initializeMetadataStore(restoredState)
+
     preferences.subscribe(async ({ selectedAddress }) => {
       if (selectedAddress && selectedAddress !== this._lastSelectedAddress) {
         this._lastSelectedAddress = selectedAddress
@@ -75,13 +78,10 @@ export class PermissionsController {
       throw new Error('Must provide non-empty string origin.')
     }
 
-    if (extensionId) {
+    const metadataState = this.store.getState()[METADATA_STORE_KEY]
-      this.store.updateState({
+
-        [METADATA_STORE_KEY]: {
+    if (extensionId && metadataState[origin]?.extensionId !== extensionId) {
-          ...this.store.getState()[METADATA_STORE_KEY],
+      this.addDomainMetadata(origin, { extensionId })
-          [origin]: { extensionId },
-        },
-      })
     }
 
     const engine = new JsonRpcEngine()
@@ -89,8 +89,7 @@ export class PermissionsController {
     engine.push(this.permissionsLog.createMiddleware())
 
     engine.push(createMethodMiddleware({
-      store: this.store,
+      addDomainMetadata: this.addDomainMetadata.bind(this),
-      storeKey: METADATA_STORE_KEY,
       getAccounts: this.getAccounts.bind(this, origin),
       getUnlockPromise: () => this._getUnlockPromise(true),
       hasPermission: this.hasPermission.bind(this, origin),
@@ -376,10 +375,7 @@ export class PermissionsController {
       .filter((acc) => acc !== account)
 
     if (newPermittedAccounts.length === 0) {
-
+      this.removePermissionsFor({ [origin]: [ 'eth_accounts' ] })
-      this.permissions.removePermissionsFor(
-        origin, [{ parentCapability: 'eth_accounts' }]
-      )
     } else {
 
       this.permissions.updateCaveatFor(
@@ -510,6 +506,102 @@ export class PermissionsController {
     })
   }
 
+  /**
+   * Removes all known domains and their related permissions.
+   */
+  clearPermissions () {
+    this.permissions.clearDomains()
+    this.notifyAllDomains({
+      method: NOTIFICATION_NAMES.accountsChanged,
+      result: [],
+    })
+  }
+
+  /**
+   * Stores domain metadata for the given origin (domain).
+   * Deletes metadata for domains without permissions in a FIFO manner, once
+   * more than 100 distinct origins have been added since boot.
+   * Metadata is never deleted for domains with permissions, to prevent a
+   * degraded user experience, since metadata cannot yet be requested on demand.
+   *
+   * @param {string} origin - The origin whose domain metadata to store.
+   * @param {Object} metadata - The domain's metadata that will be stored.
+   */
+  addDomainMetadata (origin, metadata) {
+
+    const oldMetadataState = this.store.getState()[METADATA_STORE_KEY]
+    const newMetadataState = { ...oldMetadataState }
+
+    // delete pending metadata origin from queue, and delete its metadata if
+    // it doesn't have any permissions
+    if (this._pendingSiteMetadata.size >= METADATA_CACHE_MAX_SIZE) {
+      const permissionsDomains = this.permissions.getDomains()
+
+      const oldOrigin = this._pendingSiteMetadata.values().next().value
+      this._pendingSiteMetadata.delete(oldOrigin)
+      if (!permissionsDomains[oldOrigin]) {
+        delete newMetadataState[oldOrigin]
+      }
+    }
+
+    // add new metadata to store after popping
+    newMetadataState[origin] = {
+      ...oldMetadataState[origin],
+      ...metadata,
+      lastUpdated: Date.now(),
+    }
+    this._pendingSiteMetadata.add(origin)
+    this._setDomainMetadata(newMetadataState)
+  }
+
+  /**
+   * Removes all domains without permissions from the restored metadata state,
+   * and rehydrates the metadata store.
+   *
+   * Requires PermissionsController._initializePermissions to have been called first.
+   *
+   * @param {Object} restoredState - The restored permissions controller state.
+   */
+  _initializeMetadataStore (restoredState) {
+
+    const metadataState = restoredState[METADATA_STORE_KEY] || {}
+    const newMetadataState = this._trimDomainMetadata(metadataState)
+
+    this._pendingSiteMetadata = new Set()
+    this._setDomainMetadata(newMetadataState)
+  }
+
+  /**
+   * Trims the given metadataState object by removing metadata for all origins
+   * without permissions.
+   * Returns a new object; does not mutate the argument.
+   *
+   * @param {Object} metadataState - The metadata store state object to trim.
+   * @returns {Object} The new metadata state object.
+   */
+  _trimDomainMetadata (metadataState) {
+
+    const newMetadataState = { ...metadataState }
+    const origins = Object.keys(metadataState)
+    const permissionsDomains = this.permissions.getDomains()
+
+    origins.forEach((origin) => {
+      if (!permissionsDomains[origin]) {
+        delete newMetadataState[origin]
+      }
+    })
+
+    return newMetadataState
+  }
+
+  /**
+   * Replaces the existing domain metadata with the passed-in object.
+   * @param {Object} newMetadataState - The new metadata to set.
+   */
+  _setDomainMetadata (newMetadataState) {
+    this.store.updateState({ [METADATA_STORE_KEY]: newMetadataState })
+  }
+
   /**
    * Get current set of permitted accounts for the given origin
    *
@@ -577,17 +669,6 @@ export class PermissionsController {
     })
   }
 
-  /**
-   * Removes all known domains and their related permissions.
-   */
-  clearPermissions () {
-    this.permissions.clearDomains()
-    this.notifyAllDomains({
-      method: NOTIFICATION_NAMES.accountsChanged,
-      result: [],
-    })
-  }
-
   /**
    * Adds a pending approval.
    * @param {string} id - The id of the pending approval.

app/scripts/controllers/permissions/methodMiddleware.js

@@ -5,12 +5,11 @@ import { ethErrors } from 'eth-json-rpc-errors'
  * Create middleware for handling certain methods and preprocessing permissions requests.
  */
 export default function createMethodMiddleware ({
+  addDomainMetadata,
   getAccounts,
   getUnlockPromise,
   hasPermission,
   requestAccountsPermission,
-  store,
-  storeKey,
 }) {
 
   let isProcessingRequestAccounts = false
@@ -73,30 +72,12 @@ export default function createMethodMiddleware ({
         return
 
       // custom method for getting metadata from the requesting domain,
-      // sent automatically by the inpage provider
+      // sent automatically by the inpage provider when it's initialized
       case 'wallet_sendDomainMetadata':
 
-        const storeState = store.getState()[storeKey]
+        if (typeof req.domainMetadata?.name === 'string') {
-        const extensionId = storeState[req.origin]
+          addDomainMetadata(req.origin, req.domainMetadata)
-          ? storeState[req.origin].extensionId
-          : undefined
-
-        if (
-          req.domainMetadata &&
-          typeof req.domainMetadata.name === 'string'
-        ) {
-
-          store.updateState({
-            [storeKey]: {
-              ...storeState,
-              [req.origin]: {
-                extensionId,
-                ...req.domainMetadata,
-              },
-            },
-          })
         }
-
         res.result = true
         return
 

test/unit/app/controllers/permissions/permissions-controller-test.js

@@ -5,6 +5,7 @@ import sinon from 'sinon'
 
 import {
   METADATA_STORE_KEY,
+  METADATA_CACHE_MAX_SIZE,
   WALLET_PREFIX,
 } from '../../../../../app/scripts/controllers/permissions/enums'
 
@@ -1190,10 +1191,15 @@ describe('permissions controller', function () {
   // see permissions-middleware-test for testing the middleware itself
   describe('createMiddleware', function () {
 
-    let permController
+    let permController, clock
 
     beforeEach(function () {
       permController = initPermController()
+      clock = sinon.useFakeTimers(1)
+    })
+
+    afterEach(function () {
+      clock.restore()
     })
 
     it('should throw on bad origin', function () {
@@ -1266,7 +1272,7 @@ describe('permissions controller', function () {
       const metadataStore = permController.store.getState()[METADATA_STORE_KEY]
 
       assert.deepEqual(
-        metadataStore[ORIGINS.a], { extensionId },
+        metadataStore[ORIGINS.a], { extensionId, lastUpdated: 1 },
         'metadata should be stored'
       )
     })
@@ -1318,6 +1324,180 @@ describe('permissions controller', function () {
     })
   })
 
+  describe('addDomainMetadata', function () {
+
+    let permController, clock
+
+    function getMockMetadata (size) {
+      const dummyData = {}
+      for (let i = 0; i < size; i++) {
+        const key = i.toString()
+        dummyData[key] = {}
+      }
+      return dummyData
+    }
+
+    beforeEach(function () {
+      permController = initPermController()
+      permController._setDomainMetadata = sinon.fake()
+      clock = sinon.useFakeTimers(1)
+    })
+
+    afterEach(function () {
+      clock.restore()
+    })
+
+    it('calls setter function with expected new state when adding domain', function () {
+
+      permController.store.getState = sinon.fake.returns({
+        [METADATA_STORE_KEY]: {
+          [ORIGINS.a]: {
+            foo: 'bar',
+          },
+        },
+      })
+
+      permController.addDomainMetadata(ORIGINS.b, { foo: 'bar' })
+
+      assert.ok(
+        permController.store.getState.called,
+        'should have called store.getState'
+      )
+      assert.equal(
+        permController._setDomainMetadata.getCalls().length, 1,
+        'should have called _setDomainMetadata once'
+      )
+      assert.deepEqual(
+        permController._setDomainMetadata.lastCall.args,
+        [{
+          [ORIGINS.a]: {
+            foo: 'bar',
+          },
+          [ORIGINS.b]: {
+            foo: 'bar',
+            lastUpdated: 1,
+          },
+        }]
+      )
+    })
+
+    it('calls setter function with expected new states when updating existing domain', function () {
+
+      permController.store.getState = sinon.fake.returns({
+        [METADATA_STORE_KEY]: {
+          [ORIGINS.a]: {
+            foo: 'bar',
+          },
+          [ORIGINS.b]: {
+            bar: 'baz',
+          },
+        },
+      })
+
+      permController.addDomainMetadata(ORIGINS.b, { foo: 'bar' })
+
+      assert.ok(
+        permController.store.getState.called,
+        'should have called store.getState'
+      )
+      assert.equal(
+        permController._setDomainMetadata.getCalls().length, 1,
+        'should have called _setDomainMetadata once'
+      )
+      assert.deepEqual(
+        permController._setDomainMetadata.lastCall.args,
+        [{
+          [ORIGINS.a]: {
+            foo: 'bar',
+          },
+          [ORIGINS.b]: {
+            foo: 'bar',
+            bar: 'baz',
+            lastUpdated: 1,
+          },
+        }]
+      )
+    })
+
+    it('pops metadata on add when too many origins are pending', function () {
+
+      sinon.spy(permController._pendingSiteMetadata, 'delete')
+
+      const mockMetadata = getMockMetadata(METADATA_CACHE_MAX_SIZE)
+      const expectedDeletedOrigin = Object.keys(mockMetadata)[0]
+
+      permController.store.getState = sinon.fake.returns({
+        [METADATA_STORE_KEY]: { ...mockMetadata },
+      })
+
+      // populate permController._pendingSiteMetadata, as though these origins
+      // were actually added
+      Object.keys(mockMetadata).forEach((origin) => {
+        permController._pendingSiteMetadata.add(origin)
+      })
+
+      permController.addDomainMetadata(ORIGINS.a, { foo: 'bar' })
+
+      assert.ok(
+        permController.store.getState.called,
+        'should have called store.getState'
+      )
+
+      const expectedMetadata = {
+        ...mockMetadata,
+        [ORIGINS.a]: {
+          foo: 'bar',
+          lastUpdated: 1,
+        },
+      }
+      delete expectedMetadata[expectedDeletedOrigin]
+
+      assert.ok(
+        permController._pendingSiteMetadata.delete.calledOnceWithExactly(expectedDeletedOrigin),
+        'should have called _pendingSiteMetadata.delete once'
+      )
+      assert.equal(
+        permController._setDomainMetadata.getCalls().length, 1,
+        'should have called _setDomainMetadata once'
+      )
+      assert.deepEqual(
+        permController._setDomainMetadata.lastCall.args,
+        [expectedMetadata],
+      )
+    })
+  })
+
+  describe('_trimDomainMetadata', function () {
+
+    const permController = initPermController()
+
+    it('trims domain metadata for domains without permissions', function () {
+
+      const metadataArg = {
+        [ORIGINS.a]: {},
+        [ORIGINS.b]: {},
+      }
+
+      permController.permissions.getDomains = sinon.fake.returns({
+        [ORIGINS.a]: {},
+      })
+
+      const metadataResult = permController._trimDomainMetadata(metadataArg)
+
+      assert.equal(
+        permController.permissions.getDomains.getCalls().length, 1,
+        'should have called permissions.getDomains once'
+      )
+      assert.deepEqual(
+        metadataResult,
+        {
+          [ORIGINS.a]: {},
+        },
+        'should have produced expected state'
+      )
+    })
+  })
+
   describe('miscellanea and edge cases', function () {
 
     let permController

test/unit/app/controllers/permissions/permissions-middleware-test.js

@@ -1,4 +1,5 @@
 import { strict as assert } from 'assert'
+import { useFakeTimers } from 'sinon'
 
 import {
   METADATA_STORE_KEY,
@@ -690,10 +691,15 @@ describe('permissions middleware', function () {
 
   describe('wallet_sendDomainMetadata', function () {
 
-    let permController
+    let permController, clock
 
     beforeEach(function () {
       permController = initPermController()
+      clock = useFakeTimers(1)
+    })
+
+    afterEach(function () {
+      clock.restore()
     })
 
     it('records domain metadata', async function () {
@@ -716,7 +722,7 @@ describe('permissions middleware', function () {
 
       assert.deepEqual(
         metadataStore,
-        { [ORIGINS.c]: { name, extensionId: undefined } },
+        { [ORIGINS.c]: { name, lastUpdated: 1 } },
         'metadata should have been added to store'
       )
     })
@@ -743,7 +749,7 @@ describe('permissions middleware', function () {
 
       assert.deepEqual(
         metadataStore,
-        { [ORIGINS.c]: { name, extensionId } },
+        { [ORIGINS.c]: { name, extensionId, lastUpdated: 1 } },
         'metadata should have been added to store'
       )
     })