diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 9ab5775244..a5e8441a5f 100644 --- 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 index 67579fb984..80497952f5 100644 --- 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 index 64494285f8..8a2c44f54a 100644 --- 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; @@ -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, + ) { + 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 index 701307f8f8..dd5099adc8 100644 --- 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 index 86fc4c2815..8d91cfe2cf 100644 --- 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 index 2a907eeb4b..f601546128 100644 --- 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 index 72ad9dfb1f..6a39e802f7 100644 --- 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', ]); /**