Unblock wallet_requestSnaps and add dynamic permission support (#1421)

* Unblock eth_requestAccounts * Add revokeDynamicSnapPermissions * Disallow certain confirmation screens * Block personal_sign * Add tests * Update coverage * Update coverage again * Add more blocked methods * Add test for clearState function
MetaMask · Jun 29, 2023 · 2fce138 · 2fce138
1 parent 53c4ebe
commit 2fce138
Show file tree

Hide file tree

Showing 7 changed files with 162 additions and 84 deletions.
diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json
@@ -1,6 +1,6 @@
 {
-  "branches": 88.52,
-  "functions": 94.46,
-  "lines": 96.16,
-  "statements": 95.75
+  "branches": 88.55,
+  "functions": 95.97,
+  "lines": 96.74,
+  "statements": 96.39
 }
diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts
@@ -4887,6 +4887,40 @@ describe('SnapController', () => {
     });
   });
 
+  describe('clearState', () => {
+    it('clears the state and terminates running snaps', async () => {
+      const rootMessenger = getControllerMessenger();
+      const messenger = getSnapControllerMessenger(rootMessenger);
+      const snapController = getSnapController(
+        getSnapControllerOptions({
+          messenger,
+          state: {
+            snaps: getPersistedSnapsState(),
+          },
+        }),
+      );
+
+      const callActionSpy = jest.spyOn(messenger, 'call');
+
+      expect(snapController.has(MOCK_SNAP_ID)).toBe(true);
+
+      await snapController.clearState();
+
+      expect(snapController.has(MOCK_SNAP_ID)).toBe(false);
+
+      expect(callActionSpy).toHaveBeenCalledWith(
+        'ExecutionService:terminateAllSnaps',
+      );
+
+      expect(callActionSpy).toHaveBeenCalledWith(
+        'PermissionController:revokeAllPermissions',
+        MOCK_SNAP_ID,
+      );
+
+      snapController.destroy();
+    });
+  });
+
   describe('SnapController actions', () => {
     describe('SnapController:get', () => {
       it('gets a snap', () => {
@@ -5377,4 +5411,47 @@ describe('SnapController', () => {
       snapController.destroy();
     });
   });
+
+  describe('SnapController:revokeDynamicPermissions', () => {
+    it('calls PermissionController:revokePermissions', () => {
+      const messenger = getSnapControllerMessenger();
+      const snapController = getSnapController(
+        getSnapControllerOptions({
+          messenger,
+        }),
+      );
+
+      const callActionSpy = jest.spyOn(messenger, 'call');
+
+      messenger.call('SnapController:revokeDynamicPermissions', MOCK_SNAP_ID, [
+        'eth_accounts',
+      ]);
+
+      expect(callActionSpy).toHaveBeenCalledWith(
+        'PermissionController:revokePermissions',
+        { [MOCK_SNAP_ID]: ['eth_accounts'] },
+      );
+
+      snapController.destroy();
+    });
+
+    it('throws if input permission is not a dynamic permission', () => {
+      const messenger = getSnapControllerMessenger();
+      const snapController = getSnapController(
+        getSnapControllerOptions({
+          messenger,
+        }),
+      );
+
+      expect(() =>
+        messenger.call(
+          'SnapController:revokeDynamicPermissions',
+          MOCK_SNAP_ID,
+          ['snap_notify'],
+        ),
+      ).toThrow('Non-dynamic permissions cannot be revoked');
+
+      snapController.destroy();
+    });
+  });
 });
diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts
@@ -356,6 +356,11 @@ export type DisconnectOrigin = {
   handler: SnapController['removeSnapFromSubject'];
 };
 
+export type RevokeDynamicPermissions = {
+  type: `${typeof controllerName}:revokeDynamicPermissions`;
+  handler: SnapController['revokeDynamicSnapPermissions'];
+};
+
 export type SnapControllerActions =
   | ClearSnapState
   | GetSnap
@@ -374,7 +379,8 @@ export type SnapControllerActions =
   | IncrementActiveReferences
   | DecrementActiveReferences
   | GetRegistryMetadata
-  | DisconnectOrigin;
+  | DisconnectOrigin
+  | RevokeDynamicPermissions;
 
 // Controller Messenger Events
 
@@ -510,6 +516,11 @@ type SnapControllerArgs = {
    */
   closeAllConnections: CloseAllConnectionsFunction;
 
+  /**
+   * A list of permissions that are allowed to be dynamic, meaning they can be revoked from the snap whenever.
+   */
+  dynamicPermissions: string[];
+
   /**
    * The names of endowment permissions whose values are the names of JavaScript
    * APIs that will be added to the snap execution environment at runtime.
@@ -632,6 +643,8 @@ export class SnapController extends BaseController<
 > {
   #closeAllConnections: CloseAllConnectionsFunction;
 
+  #dynamicPermissions: string[];
+
   #environmentEndowmentPermissions: string[];
 
   #excludedPermissions: Record<string, string>;
@@ -666,6 +679,7 @@ export class SnapController extends BaseController<
     closeAllConnections,
     messenger,
     state,
+    dynamicPermissions = ['eth_accounts'],
     environmentEndowmentPermissions = [],
     excludedPermissions = {},
     idleTimeCheckInterval = inMilliseconds(5, Duration.Second),
@@ -736,6 +750,7 @@ export class SnapController extends BaseController<
     });
 
     this.#closeAllConnections = closeAllConnections;
+    this.#dynamicPermissions = dynamicPermissions;
     this.#environmentEndowmentPermissions = environmentEndowmentPermissions;
     this.#excludedPermissions = excludedPermissions;
     this.#featureFlags = featureFlags;
@@ -941,6 +956,11 @@ export class SnapController extends BaseController<
       `${controllerName}:disconnectOrigin`,
       (...args) => this.removeSnapFromSubject(...args),
     );
+
+    this.messagingSystem.registerActionHandler(
+      `${controllerName}:revokeDynamicPermissions`,
+      (...args) => this.revokeDynamicSnapPermissions(...args),
+    );
   }
 
   #pollForLastRequestStatus() {
@@ -1404,7 +1424,7 @@ export class SnapController extends BaseController<
     });
 
     await this.messagingSystem.call('ExecutionService:terminateAllSnaps');
-    snapIds.forEach((snapId) => this.revokeAllSnapPermissions(snapId));
+    snapIds.forEach((snapId) => this.#revokeAllSnapPermissions(snapId));
 
     this.update((state: any) => {
       state.snaps = {};
@@ -1441,7 +1461,7 @@ export class SnapController extends BaseController<
         // it. This ensures that the snap will not be restarted or otherwise
         // affect the host environment while we are deleting it.
         await this.disableSnap(snapId);
-        this.revokeAllSnapPermissions(snapId);
+        this.#revokeAllSnapPermissions(snapId);
 
         this.#removeSnapFromSubjects(snapId);
 
@@ -1503,6 +1523,28 @@ export class SnapController extends BaseController<
     }
   }
 
+  /**
+   * Checks if a list of permissions are dynamic and allowed to be revoked, if they are they will all be revoked.
+   *
+   * @param snapId - The snap ID.
+   * @param permissionNames - The names of the permissions.
+   * @throws If non-dynamic permissions are passed.
+   */
+  revokeDynamicSnapPermissions(
+    snapId: string,
+    permissionNames: NonEmptyArray<string>,
+  ) {
+    assert(
+      permissionNames.every((permissionName) =>
+        this.#dynamicPermissions.includes(permissionName),
+      ),
+      'Non-dynamic permissions cannot be revoked',
+    );
+    this.messagingSystem.call('PermissionController:revokePermissions', {
+      [snapId]: permissionNames,
+    });
+  }
+
   /**
    * Removes a snap's permission (caveat) from all subjects.
    *
@@ -1522,7 +1564,7 @@ export class SnapController extends BaseController<
    *
    * @param snapId - The snap ID.
    */
-  private revokeAllSnapPermissions(snapId: string) {
+  #revokeAllSnapPermissions(snapId: string) {
     if (
       this.messagingSystem.call('PermissionController:hasPermissions', snapId)
     ) {
@@ -1723,7 +1765,7 @@ export class SnapController extends BaseController<
 
     // Existing snaps that should be re-installed should not maintain their existing permissions
     if (existingSnap && location.shouldAlwaysReload) {
-      this.revokeAllSnapPermissions(snapId);
+      this.#revokeAllSnapPermissions(snapId);
     }
 
     try {

diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts
@@ -255,6 +255,10 @@ export const getControllerMessenger = (registry = new MockSnapsRegistry()) => {
 
   messenger.registerActionHandler('ExecutionService:executeSnap', asyncNoOp);
   messenger.registerActionHandler('ExecutionService:terminateSnap', asyncNoOp);
+  messenger.registerActionHandler(
+    'ExecutionService:terminateAllSnaps',
+    asyncNoOp,
+  );
 
   messenger.registerActionHandler(
     'SnapsRegistry:get',
@@ -332,6 +336,7 @@ export const getSnapControllerMessenger = (
       'SnapsRegistry:get',
       'SnapsRegistry:getMetadata',
       'SnapController:disconnectOrigin',
+      'SnapController:revokeDynamicPermissions',
     ],
   });
 

diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts
@@ -325,46 +325,6 @@ describe('BaseSnapExecutor', () => {
     });
   });
 
-  it("doesn't allow eth_requestAccounts in the Ethereum provider", async () => {
-    const CODE = `
-      module.exports.onRpcRequest = () => ethereum.request({ method: 'eth_requestAccounts', params: [] });
-    `;
-
-    const executor = new TestSnapExecutor();
-    await executor.executeSnap(1, MOCK_SNAP_ID, CODE, ['ethereum']);
-
-    expect(await executor.readCommand()).toStrictEqual({
-      jsonrpc: '2.0',
-      id: 1,
-      result: 'OK',
-    });
-
-    await executor.writeCommand({
-      jsonrpc: '2.0',
-      id: 2,
-      method: 'snapRpc',
-      params: [
-        MOCK_SNAP_ID,
-        HandlerType.OnRpcRequest,
-        MOCK_ORIGIN,
-        { jsonrpc: '2.0', method: '', params: [] },
-      ],
-    });
-
-    expect(await executor.readCommand()).toStrictEqual({
-      jsonrpc: '2.0',
-      error: {
-        code: -32601,
-        message: 'The method does not exist / is not available.',
-        data: {
-          method: 'eth_requestAccounts',
-        },
-        stack: expect.any(String),
-      },
-      id: 2,
-    });
-  });
-
   it("doesn't allow wallet_requestSnaps in the Ethereum provider", async () => {
     const CODE = `
       module.exports.onRpcRequest = () => ethereum.request({ method: 'wallet_requestSnaps', params: [] });

diff --git a/packages/snaps-execution-environments/src/common/utils.test.ts b/packages/snaps-execution-environments/src/common/utils.test.ts
@@ -1,4 +1,5 @@
 import {
+  BLOCKED_RPC_METHODS,
   assertEthereumOutboundRequest,
   assertSnapOutboundRequest,
   constructError,
@@ -41,26 +42,16 @@ describe('assertSnapOutboundRequest', () => {
     );
   });
 
-  it('disallows eth_requestAccounts', () => {
-    expect(() =>
-      assertSnapOutboundRequest({ method: 'eth_requestAccounts' }),
-    ).toThrow(
-      'The global Snap API only allows RPC methods starting with `wallet_*` and `snap_*`.',
+  it.each(
+    BLOCKED_RPC_METHODS.filter(
+      (method) => method.startsWith('wallet_') || method.startsWith('snap_'),
+    ),
+  )('disallows %s', (method) => {
+    expect(() => assertSnapOutboundRequest({ method })).toThrow(
+      'The method does not exist / is not available.',
     );
   });
 
-  it('disallows wallet_requestSnaps', () => {
-    expect(() =>
-      assertSnapOutboundRequest({ method: 'wallet_requestSnaps' }),
-    ).toThrow('The method does not exist / is not available.');
-  });
-
-  it('disallows wallet_requestPermissions', () => {
-    expect(() =>
-      assertSnapOutboundRequest({ method: 'wallet_requestPermissions' }),
-    ).toThrow('The method does not exist / is not available.');
-  });
-
   it('throws for invalid JSON values', () => {
     expect(() =>
       assertSnapOutboundRequest({
@@ -89,22 +80,10 @@ describe('assertEthereumOutboundRequest', () => {
     ).toThrow('The method does not exist / is not available.');
   });
 
-  it('disallows wallet_requestPermissions', () => {
-    expect(() =>
-      assertEthereumOutboundRequest({ method: 'wallet_requestPermissions' }),
-    ).toThrow('The method does not exist / is not available.');
-  });
-
-  it('disallows eth_requestAccounts', () => {
-    expect(() =>
-      assertEthereumOutboundRequest({ method: 'eth_requestAccounts' }),
-    ).toThrow('The method does not exist / is not available.');
-  });
-
-  it('disallows wallet_requestSnaps', () => {
-    expect(() =>
-      assertEthereumOutboundRequest({ method: 'wallet_requestSnaps' }),
-    ).toThrow('The method does not exist / is not available.');
+  it.each(BLOCKED_RPC_METHODS)('disallows %s', (method) => {
+    expect(() => assertEthereumOutboundRequest({ method })).toThrow(
+      'The method does not exist / is not available.',
+    );
   });
 
   it('throws for invalid JSON values', () => {

diff --git a/packages/snaps-execution-environments/src/common/utils.ts b/packages/snaps-execution-environments/src/common/utils.ts
@@ -102,10 +102,25 @@ export function proxyStreamProvider(
 }
 
 // We're blocking these RPC methods for v1, will revisit later.
-const BLOCKED_RPC_METHODS = Object.freeze([
-  'eth_requestAccounts',
+export const BLOCKED_RPC_METHODS = Object.freeze([
   'wallet_requestSnaps',
   'wallet_requestPermissions',
+  // We disallow all of these confirmations for now, since the screens are not ready for Snaps.
+  'eth_sendRawTransaction',
+  'eth_sendTransaction',
+  'personal_sign',
+  'eth_sign',
+  'eth_signTypedData',
+  'eth_signTypedData_v1',
+  'eth_signTypedData_v3',
+  'eth_signTypedData_v4',
+  'eth_decrypt',
+  'eth_getEncryptionPublicKey',
+  'wallet_addEthereumChain',
+  'wallet_switchEthereumChain',
+  'wallet_watchAsset',
+  'wallet_registerOnboarding',
+  'wallet_scanQRCode',
 ]);
 
 /**