Skip to content

Commit 0970e9e

Browse files
MrtenzMrtenz
authored
fix: Keep dynamic permissions on update (#3726)
This fixes a bug where dynamic permissions would be revoked when updating a Snap. For example, if a Snap dynamically requested accounts through the `endowment:caip25` permission, that permission would be deemed unused, since the Snap can't request it in the manifest. To solve it, I've added some logic that checks if dynamic permissions are present in the old permissions, checks if the desired permissions has at least one of the "dependencies" of the dynamic permission (e.g., `endowment:caip25` requires `endowment:ethereum-provider` to be useful), and adds it back to the desired permissions. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Ensures dynamic permissions persist on Snap update when dependencies remain requested, and are revoked when none are used. > > - **SnapController**: > - Add `#getDesiredPermissions` to retain dynamic permissions if any dependency in `DYNAMIC_PERMISSION_DEPENDENCIES` is still desired. > - Update `#calculatePermissionsChange` to use desired permissions including dynamic ones; adjust unused/new permissions logic. > - **Constants**: > - Introduce `DYNAMIC_PERMISSION_DEPENDENCIES` mapping (e.g., `endowment:caip25` → `endowment:ethereum-provider`). > - **Tests**: > - Add tests verifying dynamic permission retention/revocation during update scenarios. > - **Misc**: > - Minor coverage metric change in `coverage.json`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 0ee3f2a. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent dd4e37c commit 0970e9e

File tree

4 files changed

+303
-5
lines changed

4 files changed

+303
-5
lines changed

‎packages/snaps-controllers/coverage.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"branches": 94.87,
2+
"branches": 94.77,
33
"functions": 98.03,
44
"lines": 98.63,
55
"statements": 98.43

‎packages/snaps-controllers/src/snaps/SnapController.test.tsx‎

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import {
5858
DEFAULT_SOURCE_PATH,
5959
DEFAULT_ICON_PATH,
6060
TEST_SECRET_RECOVERY_PHRASE_SEED_BYTES,
61+
MOCK_INITIAL_PERMISSIONS,
6162
} from '@metamask/snaps-utils/test-utils';
6263
import type { SemVerRange, SemVerVersion, Json } from '@metamask/utils';
6364
import {
@@ -8427,6 +8428,243 @@ describe('SnapController', () => {
84278428
controller.destroy();
84288429
});
84298430

8431+
it('updates a Snap with a dynamic permission, and keeps the dynamic permission if one of the dependent permissions is used', async () => {
8432+
const { manifest } = await getMockSnapFilesWithUpdatedChecksum({
8433+
manifest: getSnapManifest({
8434+
version: '1.1.0' as SemVerVersion,
8435+
initialPermissions: {
8436+
...MOCK_INITIAL_PERMISSIONS,
8437+
'endowment:ethereum-provider': {},
8438+
},
8439+
}),
8440+
});
8441+
8442+
const detectSnapLocation = jest
8443+
.fn()
8444+
.mockImplementationOnce(() => new LoopbackLocation())
8445+
.mockImplementationOnce(
8446+
() =>
8447+
new LoopbackLocation({
8448+
manifest: manifest.result,
8449+
}),
8450+
);
8451+
8452+
const rootMessenger = getControllerMessenger();
8453+
const messenger = getSnapControllerMessenger(rootMessenger);
8454+
8455+
rootMessenger.registerActionHandler(
8456+
'PermissionController:getPermissions',
8457+
() => ({
8458+
...MOCK_SNAP_PERMISSIONS,
8459+
'endowment:ethereum-provider': {
8460+
caveats: null,
8461+
date: 1664187844588,
8462+
id: 'izn0WGUO8cvq_jqvLQuQP',
8463+
invoker: MOCK_SNAP_ID,
8464+
parentCapability: 'endowment:ethereum-provider',
8465+
},
8466+
'endowment:caip25': {
8467+
caveats: null,
8468+
date: 1664187844588,
8469+
id: 'izn0WGUO8cvq_jqvLQuQP',
8470+
invoker: MOCK_SNAP_ID,
8471+
parentCapability: 'endowment:caip25',
8472+
},
8473+
}),
8474+
);
8475+
8476+
const controller = getSnapController(
8477+
getSnapControllerOptions({
8478+
messenger,
8479+
detectSnapLocation,
8480+
}),
8481+
);
8482+
const callActionSpy = jest.spyOn(messenger, 'call');
8483+
const onSnapUpdated = jest.fn();
8484+
8485+
await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} });
8486+
await controller.stopSnap(MOCK_SNAP_ID);
8487+
8488+
messenger.subscribe('SnapController:snapUpdated', onSnapUpdated);
8489+
8490+
await controller.installSnaps(MOCK_ORIGIN, {
8491+
[MOCK_SNAP_ID]: { version: '1.1.0' },
8492+
});
8493+
8494+
expect(callActionSpy).toHaveBeenCalledTimes(21);
8495+
expect(callActionSpy).toHaveBeenNthCalledWith(
8496+
12,
8497+
'ApprovalController:addRequest',
8498+
{
8499+
origin: MOCK_ORIGIN,
8500+
id: expect.any(String),
8501+
type: SNAP_APPROVAL_UPDATE,
8502+
requestData: {
8503+
metadata: {
8504+
id: expect.any(String),
8505+
dappOrigin: MOCK_ORIGIN,
8506+
origin: MOCK_SNAP_ID,
8507+
},
8508+
snapId: MOCK_SNAP_ID,
8509+
},
8510+
requestState: {
8511+
loading: true,
8512+
},
8513+
},
8514+
true,
8515+
);
8516+
8517+
expect(messenger.call).toHaveBeenNthCalledWith(
8518+
15,
8519+
'ApprovalController:updateRequestState',
8520+
expect.objectContaining({
8521+
id: expect.any(String),
8522+
requestState: {
8523+
loading: false,
8524+
permissions: {},
8525+
newVersion: '1.1.0',
8526+
newPermissions: {},
8527+
approvedPermissions: {
8528+
...MOCK_SNAP_PERMISSIONS,
8529+
'endowment:ethereum-provider': expect.any(Object),
8530+
'endowment:caip25': expect.any(Object),
8531+
},
8532+
unusedPermissions: {},
8533+
newConnections: {},
8534+
unusedConnections: {},
8535+
approvedConnections: {},
8536+
},
8537+
}),
8538+
);
8539+
8540+
expect(messenger.call).not.toHaveBeenCalledWith(
8541+
'PermissionController:revokePermissions',
8542+
expect.anything(),
8543+
);
8544+
8545+
controller.destroy();
8546+
});
8547+
8548+
it('updates a Snap with a dynamic permission, and revokes the dynamic permission if none of the dependent permissions are used', async () => {
8549+
const { manifest } = await getMockSnapFilesWithUpdatedChecksum({
8550+
manifest: getSnapManifest({
8551+
version: '1.1.0' as SemVerVersion,
8552+
initialPermissions: {
8553+
...MOCK_INITIAL_PERMISSIONS,
8554+
},
8555+
}),
8556+
});
8557+
8558+
const detectSnapLocation = jest
8559+
.fn()
8560+
.mockImplementationOnce(() => new LoopbackLocation())
8561+
.mockImplementationOnce(
8562+
() =>
8563+
new LoopbackLocation({
8564+
manifest: manifest.result,
8565+
}),
8566+
);
8567+
8568+
const rootMessenger = getControllerMessenger();
8569+
const messenger = getSnapControllerMessenger(rootMessenger);
8570+
8571+
rootMessenger.registerActionHandler(
8572+
'PermissionController:getPermissions',
8573+
() => ({
8574+
...MOCK_SNAP_PERMISSIONS,
8575+
'endowment:ethereum-provider': {
8576+
caveats: null,
8577+
date: 1664187844588,
8578+
id: 'izn0WGUO8cvq_jqvLQuQP',
8579+
invoker: MOCK_SNAP_ID,
8580+
parentCapability: 'endowment:ethereum-provider',
8581+
},
8582+
'endowment:caip25': {
8583+
caveats: null,
8584+
date: 1664187844588,
8585+
id: 'izn0WGUO8cvq_jqvLQuQP',
8586+
invoker: MOCK_SNAP_ID,
8587+
parentCapability: 'endowment:caip25',
8588+
},
8589+
}),
8590+
);
8591+
8592+
const controller = getSnapController(
8593+
getSnapControllerOptions({
8594+
messenger,
8595+
detectSnapLocation,
8596+
}),
8597+
);
8598+
const callActionSpy = jest.spyOn(messenger, 'call');
8599+
const onSnapUpdated = jest.fn();
8600+
8601+
await controller.installSnaps(MOCK_ORIGIN, { [MOCK_SNAP_ID]: {} });
8602+
await controller.stopSnap(MOCK_SNAP_ID);
8603+
8604+
messenger.subscribe('SnapController:snapUpdated', onSnapUpdated);
8605+
8606+
await controller.installSnaps(MOCK_ORIGIN, {
8607+
[MOCK_SNAP_ID]: { version: '1.1.0' },
8608+
});
8609+
8610+
expect(callActionSpy).toHaveBeenCalledTimes(22);
8611+
expect(callActionSpy).toHaveBeenNthCalledWith(
8612+
12,
8613+
'ApprovalController:addRequest',
8614+
{
8615+
origin: MOCK_ORIGIN,
8616+
id: expect.any(String),
8617+
type: SNAP_APPROVAL_UPDATE,
8618+
requestData: {
8619+
metadata: {
8620+
id: expect.any(String),
8621+
dappOrigin: MOCK_ORIGIN,
8622+
origin: MOCK_SNAP_ID,
8623+
},
8624+
snapId: MOCK_SNAP_ID,
8625+
},
8626+
requestState: {
8627+
loading: true,
8628+
},
8629+
},
8630+
true,
8631+
);
8632+
8633+
expect(messenger.call).toHaveBeenNthCalledWith(
8634+
15,
8635+
'ApprovalController:updateRequestState',
8636+
expect.objectContaining({
8637+
id: expect.any(String),
8638+
requestState: {
8639+
loading: false,
8640+
permissions: {},
8641+
newVersion: '1.1.0',
8642+
newPermissions: {},
8643+
approvedPermissions: {
8644+
...MOCK_SNAP_PERMISSIONS,
8645+
},
8646+
unusedPermissions: {
8647+
'endowment:ethereum-provider': expect.any(Object),
8648+
'endowment:caip25': expect.any(Object),
8649+
},
8650+
newConnections: {},
8651+
unusedConnections: {},
8652+
approvedConnections: {},
8653+
},
8654+
}),
8655+
);
8656+
8657+
expect(messenger.call).toHaveBeenNthCalledWith(
8658+
18,
8659+
'PermissionController:revokePermissions',
8660+
{
8661+
[MOCK_SNAP_ID]: ['endowment:ethereum-provider', 'endowment:caip25'],
8662+
},
8663+
);
8664+
8665+
controller.destroy();
8666+
});
8667+
84308668
it('can update crashed snap', async () => {
84318669
const { manifest } = await getMockSnapFilesWithUpdatedChecksum({
84328670
manifest: getSnapManifest({

‎packages/snaps-controllers/src/snaps/SnapController.ts‎

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@ import { gt, gte } from 'semver';
144144
import {
145145
ALLOWED_PERMISSIONS,
146146
CLIENT_ONLY_HANDLERS,
147+
DYNAMIC_PERMISSION_DEPENDENCIES,
147148
LEGACY_ENCRYPTION_KEY_DERIVATION_OPTIONS,
148149
METAMASK_ORIGIN,
149150
STATE_DEBOUNCE_TIMEOUT,
@@ -4273,6 +4274,46 @@ export class SnapController extends BaseController<
42734274
});
42744275
}
42754276

4277+
/**
4278+
* Get the desired permissions including dynamic permissions. That is, if a
4279+
* dynamic permission was previously granted and at least one of its
4280+
* dependencies is still desired, it will be included in the desired
4281+
* permissions.
4282+
*
4283+
* @param oldPermissions - The old permissions.
4284+
* @param desiredPermissions - The desired permissions.
4285+
* @returns The desired permissions including dynamic permissions.
4286+
*/
4287+
#getDesiredPermissions(
4288+
oldPermissions: SubjectPermissions<
4289+
ValidPermission<string, Caveat<string, any>>
4290+
>,
4291+
desiredPermissions: Record<string, Pick<PermissionConstraint, 'caveats'>>,
4292+
) {
4293+
return Object.keys(oldPermissions).reduce<
4294+
Record<string, Pick<PermissionConstraint, 'caveats'>>
4295+
>((accumulator, permissionName) => {
4296+
if (this.#dynamicPermissions.includes(permissionName)) {
4297+
const hasDependencies = hasProperty(
4298+
DYNAMIC_PERMISSION_DEPENDENCIES,
4299+
permissionName,
4300+
);
4301+
4302+
const hasDependency = DYNAMIC_PERMISSION_DEPENDENCIES[
4303+
permissionName
4304+
]?.some((dependency) => hasProperty(desiredPermissions, dependency));
4305+
4306+
// If the permission doesn't have dependencies, or if at least one of
4307+
// its dependencies is desired, include it in the desired permissions.
4308+
if (!hasDependencies || hasDependency) {
4309+
accumulator[permissionName] = oldPermissions[permissionName];
4310+
}
4311+
}
4312+
4313+
return accumulator;
4314+
}, desiredPermissions);
4315+
}
4316+
42764317
#calculatePermissionsChange(
42774318
snapId: SnapId,
42784319
desiredPermissionsSet: Record<
@@ -4290,16 +4331,21 @@ export class SnapController extends BaseController<
42904331
} {
42914332
const oldPermissions =
42924333
this.messenger.call('PermissionController:getPermissions', snapId) ?? {};
4334+
const desiredPermissionsSetWithDynamic = this.#getDesiredPermissions(
4335+
oldPermissions,
4336+
desiredPermissionsSet,
4337+
);
42934338

42944339
const newPermissions = permissionsDiff(
4295-
desiredPermissionsSet,
4340+
desiredPermissionsSetWithDynamic,
42964341
oldPermissions,
42974342
);
4298-
// TODO(ritave): The assumption that these are unused only holds so long as we do not
4299-
// permit dynamic permission requests.
4343+
4344+
// TODO: The assumption that these are unused only holds so long as we do
4345+
// not permit dynamic permission requests.
43004346
const unusedPermissions = permissionsDiff(
43014347
oldPermissions,
4302-
desiredPermissionsSet,
4348+
desiredPermissionsSetWithDynamic,
43034349
);
43044350

43054351
// It's a Set Intersection of oldPermissions and desiredPermissionsSet

‎packages/snaps-controllers/src/snaps/constants.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,17 @@ export const CLIENT_ONLY_HANDLERS = Object.freeze([
4646
HandlerType.OnAssetsMarketData,
4747
HandlerType.OnWebSocketEvent,
4848
]);
49+
50+
/**
51+
* A mapping of dynamic permission to their required dependencies, i.e., if the
52+
* dynamic permission is requested, at least one of its dependencies should
53+
* also be requested in order to use the dynamic permission.
54+
*
55+
* This is primarily used to grant or revoke the permission if its dependencies
56+
* are granted or revoked.
57+
*/
58+
export const DYNAMIC_PERMISSION_DEPENDENCIES: Readonly<
59+
Record<string, string[]>
60+
> = Object.freeze({
61+
'endowment:caip25': ['endowment:ethereum-provider'],
62+
});

0 commit comments

Comments
 (0)