From fe39e11448e9b4009edf3c30a39bc1d6c2e36b94 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 5 Nov 2025 14:17:19 +0100 Subject: [PATCH 1/6] Keep dynamic permissions on update --- .../src/snaps/SnapController.test.tsx | 225 ++++++++++++++++++ .../src/snaps/SnapController.ts | 72 +++++- .../snaps-controllers/src/snaps/constants.ts | 12 + 3 files changed, 306 insertions(+), 3 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index 057dc7e7a4..041f2c04ae 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -58,6 +58,7 @@ import { DEFAULT_SOURCE_PATH, DEFAULT_ICON_PATH, TEST_SECRET_RECOVERY_PHRASE_SEED_BYTES, + MOCK_INITIAL_PERMISSIONS, } from '@metamask/snaps-utils/test-utils'; import type { SemVerRange, SemVerVersion, Json } from '@metamask/utils'; import { @@ -8427,6 +8428,230 @@ describe('SnapController', () => { controller.destroy(); }); + it('updates a Snap with a dynamic permission, and keeps the dynamic permission if one of the dependent permissions is used', async () => { + const { manifest } = await getMockSnapFilesWithUpdatedChecksum({ + manifest: getSnapManifest({ + version: '1.1.0' as SemVerVersion, + initialPermissions: { + ...MOCK_INITIAL_PERMISSIONS, + 'endowment:ethereum-provider': {}, + }, + }), + }); + + const detectSnapLocation = jest + .fn() + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: manifest.result, + }), + ); + + const rootMessenger = getControllerMessenger(); + const messenger = getSnapControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => ({ + ...MOCK_SNAP_PERMISSIONS, + 'endowment:ethereum-provider': { + caveats: null, + date: 1664187844588, + id: 'izn0WGUO8cvq_jqvLQuQP', + invoker: MOCK_SNAP_ID, + parentCapability: 'endowment:ethereum-provider', + }, + 'endowment:caip25': { + caveats: null, + date: 1664187844588, + id: 'izn0WGUO8cvq_jqvLQuQP', + invoker: MOCK_SNAP_ID, + parentCapability: 'endowment:caip25', + }, + }), + ); + + const controller = getSnapController( + getSnapControllerOptions({ + messenger, + detectSnapLocation, + }), + ); + const callActionSpy = jest.spyOn(messenger, 'call'); + const onSnapUpdated = jest.fn(); + + await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} }); + await controller.stopSnap(MOCK_SNAP_ID); + + messenger.subscribe('SnapController:snapUpdated', onSnapUpdated); + + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); + + expect(callActionSpy).toHaveBeenCalledTimes(21); + expect(callActionSpy).toHaveBeenNthCalledWith( + 12, + 'ApprovalController:addRequest', + { + origin: MOCK_ORIGIN, + id: expect.any(String), + type: SNAP_APPROVAL_UPDATE, + requestData: { + metadata: { + id: expect.any(String), + dappOrigin: MOCK_ORIGIN, + origin: MOCK_SNAP_ID, + }, + snapId: MOCK_SNAP_ID, + }, + requestState: { + loading: true, + }, + }, + true, + ); + + expect(messenger.call).toHaveBeenNthCalledWith( + 15, + 'ApprovalController:updateRequestState', + expect.objectContaining({ + id: expect.any(String), + requestState: { + loading: false, + permissions: {}, + newVersion: '1.1.0', + newPermissions: {}, + approvedPermissions: { + ...MOCK_SNAP_PERMISSIONS, + 'endowment:ethereum-provider': expect.any(Object), + 'endowment:caip25': expect.any(Object), + }, + unusedPermissions: {}, + newConnections: {}, + unusedConnections: {}, + approvedConnections: {}, + }, + }), + ); + + controller.destroy(); + }); + + it('updates a Snap with a dynamic permission, and revokes the dynamic permission if none of the dependent permissions are used', async () => { + const { manifest } = await getMockSnapFilesWithUpdatedChecksum({ + manifest: getSnapManifest({ + version: '1.1.0' as SemVerVersion, + initialPermissions: { + ...MOCK_INITIAL_PERMISSIONS, + }, + }), + }); + + const detectSnapLocation = jest + .fn() + .mockImplementationOnce(() => new LoopbackLocation()) + .mockImplementationOnce( + () => + new LoopbackLocation({ + manifest: manifest.result, + }), + ); + + const rootMessenger = getControllerMessenger(); + const messenger = getSnapControllerMessenger(rootMessenger); + + rootMessenger.registerActionHandler( + 'PermissionController:getPermissions', + () => ({ + ...MOCK_SNAP_PERMISSIONS, + 'endowment:ethereum-provider': { + caveats: null, + date: 1664187844588, + id: 'izn0WGUO8cvq_jqvLQuQP', + invoker: MOCK_SNAP_ID, + parentCapability: 'endowment:ethereum-provider', + }, + 'endowment:caip25': { + caveats: null, + date: 1664187844588, + id: 'izn0WGUO8cvq_jqvLQuQP', + invoker: MOCK_SNAP_ID, + parentCapability: 'endowment:caip25', + }, + }), + ); + + const controller = getSnapController( + getSnapControllerOptions({ + messenger, + detectSnapLocation, + }), + ); + const callActionSpy = jest.spyOn(messenger, 'call'); + const onSnapUpdated = jest.fn(); + + await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} }); + await controller.stopSnap(MOCK_SNAP_ID); + + messenger.subscribe('SnapController:snapUpdated', onSnapUpdated); + + await controller.installSnaps(MOCK_ORIGIN, { + [MOCK_SNAP_ID]: { version: '1.1.0' }, + }); + + expect(callActionSpy).toHaveBeenCalledTimes(22); + expect(callActionSpy).toHaveBeenNthCalledWith( + 12, + 'ApprovalController:addRequest', + { + origin: MOCK_ORIGIN, + id: expect.any(String), + type: SNAP_APPROVAL_UPDATE, + requestData: { + metadata: { + id: expect.any(String), + dappOrigin: MOCK_ORIGIN, + origin: MOCK_SNAP_ID, + }, + snapId: MOCK_SNAP_ID, + }, + requestState: { + loading: true, + }, + }, + true, + ); + + expect(messenger.call).toHaveBeenNthCalledWith( + 15, + 'ApprovalController:updateRequestState', + expect.objectContaining({ + id: expect.any(String), + requestState: { + loading: false, + permissions: {}, + newVersion: '1.1.0', + newPermissions: {}, + approvedPermissions: { + ...MOCK_SNAP_PERMISSIONS, + }, + unusedPermissions: { + 'endowment:ethereum-provider': expect.any(Object), + 'endowment:caip25': expect.any(Object), + }, + newConnections: {}, + unusedConnections: {}, + approvedConnections: {}, + }, + }), + ); + + controller.destroy(); + }); + it('can update crashed snap', async () => { const { manifest } = await getMockSnapFilesWithUpdatedChecksum({ manifest: getSnapManifest({ diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 0a547002f3..4bb9745705 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -144,6 +144,7 @@ import { gt, gte } from 'semver'; import { ALLOWED_PERMISSIONS, CLIENT_ONLY_HANDLERS, + DYNAMIC_PERMISSION_DEPENDENCIES, LEGACY_ENCRYPTION_KEY_DERIVATION_OPTIONS, METAMASK_ORIGIN, STATE_DEBOUNCE_TIMEOUT, @@ -4272,6 +4273,68 @@ export class SnapController extends BaseController< }); } + /** + * Calculate changes to dynamic permissions based on the desired permissions + * set and their dependencies. + * + * @param approvedPermissions - The permissions that are already approved. + * @param unusedPermissions - The permissions that are no longer used. + * @param newPermissions - The new permissions that are being requested. + * @returns The updated desired permissions set including dynamic permissions. + */ + #calculateDynamicPermissionsChange( + approvedPermissions: SubjectPermissions< + ValidPermission> + >, + unusedPermissions: SubjectPermissions< + ValidPermission> + >, + newPermissions: Record>, + ) { + const groupedPermissions = new Set([ + ...Object.keys(newPermissions), + ...Object.keys(approvedPermissions), + ]); + + return Object.entries(DYNAMIC_PERMISSION_DEPENDENCIES).reduce<{ + unusedPermissions: SubjectPermissions< + ValidPermission> + >; + approvedPermissions: SubjectPermissions< + ValidPermission> + >; + newPermissions: Record>; + }>( + (accumulator, [permission, dependencies]) => { + // If the Snap has a dynamic permission, it should always be in the + // unused permissions at this point, since it can't be requested + // directly in the manifest. + if (!accumulator.unusedPermissions[permission]) { + return accumulator; + } + + const hasDependency = dependencies.some((dependency) => + groupedPermissions.has(dependency), + ); + + // If a dependency exists, move the assumed unused dynamic permission + // back to approved permissions. + if (hasDependency) { + accumulator.approvedPermissions[permission] = + accumulator.unusedPermissions[permission]; + delete accumulator.unusedPermissions[permission]; + } + + return accumulator; + }, + { + approvedPermissions, + unusedPermissions, + newPermissions, + }, + ); + } + #calculatePermissionsChange( snapId: SnapId, desiredPermissionsSet: Record< @@ -4294,8 +4357,7 @@ export class SnapController extends BaseController< desiredPermissionsSet, oldPermissions, ); - // TODO(ritave): The assumption that these are unused only holds so long as we do not - // permit dynamic permission requests. + const unusedPermissions = permissionsDiff( oldPermissions, desiredPermissionsSet, @@ -4308,7 +4370,11 @@ export class SnapController extends BaseController< unusedPermissions, ); - return { newPermissions, unusedPermissions, approvedPermissions }; + return this.#calculateDynamicPermissionsChange( + approvedPermissions, + unusedPermissions, + newPermissions, + ); } #isSubjectConnectedToSnap(snapId: SnapId, origin: string) { diff --git a/packages/snaps-controllers/src/snaps/constants.ts b/packages/snaps-controllers/src/snaps/constants.ts index 412b7811c3..65ceade265 100644 --- a/packages/snaps-controllers/src/snaps/constants.ts +++ b/packages/snaps-controllers/src/snaps/constants.ts @@ -46,3 +46,15 @@ export const CLIENT_ONLY_HANDLERS = Object.freeze([ HandlerType.OnAssetsMarketData, HandlerType.OnWebSocketEvent, ]); + +/** + * A mapping of dynamic permission to their required dependencies, i.e., if the + * dynamic permission is requested, at least one of its dependencies should + * also be requested in order to use the dynamic permission. + * + * This is primarily used to grant or revoke the permission if its dependencies + * are granted or revoked. + */ +export const DYNAMIC_PERMISSION_DEPENDENCIES = Object.freeze({ + 'endowment:caip25': ['endowment:ethereum-provider'], +}); From f535cdb680620ad24fa401d94e2c43067d4d102d Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 5 Nov 2025 16:30:21 +0100 Subject: [PATCH 2/6] Simplify logic --- .../src/snaps/SnapController.ts | 85 +++++++------------ .../snaps-controllers/src/snaps/constants.ts | 4 +- 2 files changed, 34 insertions(+), 55 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 4bb9745705..7263958650 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -4274,65 +4274,42 @@ export class SnapController extends BaseController< } /** - * Calculate changes to dynamic permissions based on the desired permissions - * set and their dependencies. + * Get the desired permissions including dynamic permissions. That is, if a + * dynamic permission was previously granted and at least one of its + * dependencies is still desired, it will be included in the desired + * permissions. * - * @param approvedPermissions - The permissions that are already approved. - * @param unusedPermissions - The permissions that are no longer used. - * @param newPermissions - The new permissions that are being requested. - * @returns The updated desired permissions set including dynamic permissions. + * @param oldPermissions - The old permissions. + * @param desiredPermissions - The desired permissions. + * @returns The desired permissions including dynamic permissions. */ - #calculateDynamicPermissionsChange( - approvedPermissions: SubjectPermissions< - ValidPermission> - >, - unusedPermissions: SubjectPermissions< + #getDesiredPermissions( + oldPermissions: SubjectPermissions< ValidPermission> >, - newPermissions: Record>, + desiredPermissions: Record>, ) { - const groupedPermissions = new Set([ - ...Object.keys(newPermissions), - ...Object.keys(approvedPermissions), - ]); - - return Object.entries(DYNAMIC_PERMISSION_DEPENDENCIES).reduce<{ - unusedPermissions: SubjectPermissions< - ValidPermission> - >; - approvedPermissions: SubjectPermissions< - ValidPermission> - >; - newPermissions: Record>; - }>( - (accumulator, [permission, dependencies]) => { - // If the Snap has a dynamic permission, it should always be in the - // unused permissions at this point, since it can't be requested - // directly in the manifest. - if (!accumulator.unusedPermissions[permission]) { - return accumulator; - } + return Object.keys(oldPermissions).reduce< + Record> + >((accumulator, permissionName) => { + if ( + this.#dynamicPermissions.includes(permissionName) && + hasProperty(DYNAMIC_PERMISSION_DEPENDENCIES, permissionName) + ) { + const dependencies = + DYNAMIC_PERMISSION_DEPENDENCIES[permissionName] ?? []; const hasDependency = dependencies.some((dependency) => - groupedPermissions.has(dependency), + hasProperty(desiredPermissions, dependency), ); - // If a dependency exists, move the assumed unused dynamic permission - // back to approved permissions. if (hasDependency) { - accumulator.approvedPermissions[permission] = - accumulator.unusedPermissions[permission]; - delete accumulator.unusedPermissions[permission]; + accumulator[permissionName] = oldPermissions[permissionName]; } + } - return accumulator; - }, - { - approvedPermissions, - unusedPermissions, - newPermissions, - }, - ); + return accumulator; + }, desiredPermissions); } #calculatePermissionsChange( @@ -4352,15 +4329,19 @@ export class SnapController extends BaseController< } { const oldPermissions = this.messenger.call('PermissionController:getPermissions', snapId) ?? {}; + const desiredPermissionsSetWithDynamic = this.#getDesiredPermissions( + oldPermissions, + desiredPermissionsSet, + ); const newPermissions = permissionsDiff( - desiredPermissionsSet, + desiredPermissionsSetWithDynamic, oldPermissions, ); const unusedPermissions = permissionsDiff( oldPermissions, - desiredPermissionsSet, + desiredPermissionsSetWithDynamic, ); // It's a Set Intersection of oldPermissions and desiredPermissionsSet @@ -4370,11 +4351,7 @@ export class SnapController extends BaseController< unusedPermissions, ); - return this.#calculateDynamicPermissionsChange( - approvedPermissions, - unusedPermissions, - newPermissions, - ); + return { newPermissions, unusedPermissions, approvedPermissions }; } #isSubjectConnectedToSnap(snapId: SnapId, origin: string) { diff --git a/packages/snaps-controllers/src/snaps/constants.ts b/packages/snaps-controllers/src/snaps/constants.ts index 65ceade265..da10e2678a 100644 --- a/packages/snaps-controllers/src/snaps/constants.ts +++ b/packages/snaps-controllers/src/snaps/constants.ts @@ -55,6 +55,8 @@ export const CLIENT_ONLY_HANDLERS = Object.freeze([ * This is primarily used to grant or revoke the permission if its dependencies * are granted or revoked. */ -export const DYNAMIC_PERMISSION_DEPENDENCIES = Object.freeze({ +export const DYNAMIC_PERMISSION_DEPENDENCIES: Readonly< + Record +> = Object.freeze({ 'endowment:caip25': ['endowment:ethereum-provider'], }); From 073aade746d9c68797125ef0ea30878207f94f5b Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 5 Nov 2025 16:46:21 +0100 Subject: [PATCH 3/6] Update coverage --- packages/snaps-controllers/coverage.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 61ba9a304f..89feea7f15 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 94.87, + "branches": 94.77, "functions": 98.03, "lines": 98.63, "statements": 98.43 From 24de86d76d8c2501b310e09203a47395be4fbd44 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Thu, 6 Nov 2025 10:55:11 +0100 Subject: [PATCH 4/6] Add assertions for `PermissionController:revokePermissions` calls --- .../src/snaps/SnapController.test.tsx | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index 041f2c04ae..47b93595fd 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/packages/snaps-controllers/src/snaps/SnapController.test.tsx @@ -8537,6 +8537,11 @@ describe('SnapController', () => { }), ); + expect(messenger.call).not.toHaveBeenCalledWith( + 'PermissionController:revokePermissions', + expect.anything(), + ); + controller.destroy(); }); @@ -8649,6 +8654,14 @@ describe('SnapController', () => { }), ); + expect(messenger.call).toHaveBeenNthCalledWith( + 18, + 'PermissionController:revokePermissions', + { + [MOCK_SNAP_ID]: ['endowment:ethereum-provider', 'endowment:caip25'], + }, + ); + controller.destroy(); }); From a8608131f938acafc557333a0674b3da7243360c Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Thu, 6 Nov 2025 11:03:30 +0100 Subject: [PATCH 5/6] Keep permissions without dependencies --- .../src/snaps/SnapController.ts | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index 7263958650..f70d213c41 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -4292,18 +4292,19 @@ export class SnapController extends BaseController< return Object.keys(oldPermissions).reduce< Record> >((accumulator, permissionName) => { - if ( - this.#dynamicPermissions.includes(permissionName) && - hasProperty(DYNAMIC_PERMISSION_DEPENDENCIES, permissionName) - ) { - const dependencies = - DYNAMIC_PERMISSION_DEPENDENCIES[permissionName] ?? []; - - const hasDependency = dependencies.some((dependency) => - hasProperty(desiredPermissions, dependency), + if (this.#dynamicPermissions.includes(permissionName)) { + const hasDependencies = hasProperty( + DYNAMIC_PERMISSION_DEPENDENCIES, + permissionName, ); - if (hasDependency) { + const hasDependency = DYNAMIC_PERMISSION_DEPENDENCIES[ + permissionName + ]?.some((dependency) => hasProperty(desiredPermissions, dependency)); + + // If the permission doesn't have dependencies, or if at least one of + // its dependencies is desired, include it in the desired permissions. + if (!hasDependencies || hasDependency) { accumulator[permissionName] = oldPermissions[permissionName]; } } From 0ee3f2a2326923cc03cb843ccb473a2c9c6ef85f Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Thu, 6 Nov 2025 11:06:28 +0100 Subject: [PATCH 6/6] Add TODO back --- packages/snaps-controllers/src/snaps/SnapController.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts index f70d213c41..317332a404 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.ts +++ b/packages/snaps-controllers/src/snaps/SnapController.ts @@ -4340,6 +4340,8 @@ export class SnapController extends BaseController< oldPermissions, ); + // TODO: The assumption that these are unused only holds so long as we do + // not permit dynamic permission requests. const unusedPermissions = permissionsDiff( oldPermissions, desiredPermissionsSetWithDynamic,