fix: account for caveats in permissions difference calculation (#2345)

The permission diff calculation was incorrect when considering caveats.

Because of this, changes to caveats were not always correctly applied
when updating a Snap.

One specific instance of this was updating a Snap with:
```
'endowment:rpc': { dapps: true },
```
to
```
'endowment:rpc': { allowedOrigins: ['https://metamask.io'] },
```

This problem was fixed by introducing `permissionsDiff` which follows
the implementation of `setDiff` except for the additional condition that
when the permission is included in both permission sets, it is
subtracted if the caveats differ.

packages/snaps-controllers/coverage.json

@@ -1,6 +1,6 @@
 {
-  "branches": 91.32,
-  "functions": 96.58,
-  "lines": 97.85,
-  "statements": 97.52
+  "branches": 91.47,
+  "functions": 96.61,
+  "lines": 97.86,
+  "statements": 97.53
 }

packages/snaps-controllers/package.json

@@ -68,6 +68,7 @@
     "@xstate/fsm": "^2.0.0",
     "browserify-zlib": "^0.2.0",
     "concat-stream": "^2.0.0",
+    "fast-deep-equal": "^3.1.3",
     "get-npm-tarball-url": "^2.0.3",
     "immer": "^9.0.6",
     "nanoid": "^3.1.31",

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

@@ -4313,7 +4313,7 @@ describe('SnapController', () => {
           manifest: getSnapManifest({
             version: '1.2.3',
             initialPermissions: {
-              'endowment:rpc': { dapps: true },
+              'endowment:rpc': { dapps: false, snaps: true },
               // eslint-disable-next-line @typescript-eslint/naming-convention
               snap_getEntropy: {},
             },
@@ -5015,6 +5015,81 @@ describe('SnapController', () => {
       snapController.destroy();
     });
 
+    it('overwrites caveats on update for already approved permissions', async () => {
+      const initialPermissions = {
+        [handlerEndowments.onRpcRequest as string]: {
+          allowedOrigins: ['https://metamask.io'],
+        },
+      };
+      const { manifest } = await getMockSnapFilesWithUpdatedChecksum({
+        manifest: getSnapManifest({
+          version: '1.1.0' as SemVerVersion,
+          initialPermissions,
+        }),
+      });
+
+      const detectSnapLocation = loopbackDetect({
+        manifest: manifest.result,
+      });
+
+      const rootMessenger = getControllerMessenger();
+      const messenger = getSnapControllerMessenger(rootMessenger);
+      const snapController = getSnapController(
+        getSnapControllerOptions({
+          messenger,
+          state: {
+            snaps: getPersistedSnapsState(),
+          },
+          detectSnapLocation,
+        }),
+      );
+
+      await snapController.updateSnap(
+        MOCK_ORIGIN,
+        MOCK_SNAP_ID,
+        detectSnapLocation(),
+      );
+
+      expect(messenger.call).toHaveBeenNthCalledWith(
+        7,
+        'PermissionController:revokePermissions',
+        {
+          [MOCK_SNAP_ID]: [SnapEndowments.Rpc, 'snap_dialog'],
+        },
+      );
+
+      expect(messenger.call).toHaveBeenNthCalledWith(
+        8,
+        'PermissionController:grantPermissions',
+        {
+          approvedPermissions: {
+            [handlerEndowments.onRpcRequest as string]: {
+              caveats: [
+                {
+                  type: SnapCaveatType.RpcOrigin,
+                  value: {
+                    allowedOrigins: ['https://metamask.io'],
+                  },
+                },
+              ],
+            },
+          },
+          subject: { origin: MOCK_SNAP_ID },
+          requestData: {
+            metadata: {
+              origin: MOCK_SNAP_ID,
+              dappOrigin: MOCK_ORIGIN,
+              id: expect.any(String),
+            },
+
+            snapId: MOCK_SNAP_ID,
+          },
+        },
+      );
+
+      snapController.destroy();
+    });
+
     it('returns an error on invalid snap id', async () => {
       const snapId = 'foo';
       const messenger = getSnapControllerMessenger();

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

@@ -120,7 +120,13 @@ import {
   type ExportableKeyEncryptor,
   type KeyDerivationOptions,
 } from '../types';
-import { fetchSnap, hasTimedOut, setDiff, withTimeout } from '../utils';
+import {
+  fetchSnap,
+  hasTimedOut,
+  permissionsDiff,
+  setDiff,
+  withTimeout,
+} from '../utils';
 import { ALLOWED_PERMISSIONS } from './constants';
 import type { SnapLocation } from './location';
 import { detectSnapLocation } from './location';
@@ -3403,14 +3409,23 @@ export class SnapController extends BaseController<
         snapId,
       ) ?? {};
 
-    const newPermissions = setDiff(desiredPermissionsSet, oldPermissions);
+    const newPermissions = permissionsDiff(
+      desiredPermissionsSet,
+      oldPermissions,
+    );
     // TODO(ritave): The assumption that these are unused only holds so long as we do not
     //               permit dynamic permission requests.
-    const unusedPermissions = setDiff(oldPermissions, desiredPermissionsSet);
+    const unusedPermissions = permissionsDiff(
+      oldPermissions,
+      desiredPermissionsSet,
+    );
 
     // It's a Set Intersection of oldPermissions and desiredPermissionsSet
     // oldPermissions ∖ (oldPermissions ∖ desiredPermissionsSet) ⟺ oldPermissions ∩ desiredPermissionsSet
-    const approvedPermissions = setDiff(oldPermissions, unusedPermissions);
+    const approvedPermissions = permissionsDiff(
+      oldPermissions,
+      unusedPermissions,
+    );
 
     return { newPermissions, unusedPermissions, approvedPermissions };
   }

packages/snaps-controllers/src/test-utils/controller.ts

@@ -173,6 +173,19 @@ export const MOCK_DAPPS_RPC_ORIGINS_PERMISSION: PermissionConstraint = {
   parentCapability: SnapEndowments.Rpc,
 };
 
+export const MOCK_ALLOWED_RPC_ORIGINS_PERMISSION: PermissionConstraint = {
+  caveats: [
+    {
+      type: SnapCaveatType.RpcOrigin,
+      value: { allowedOrigins: ['https://metamask.io'] },
+    },
+  ],
+  date: 1664187844588,
+  id: 'izn0WGUO8cvq_jqvLQuQP',
+  invoker: MOCK_SNAP_ID,
+  parentCapability: SnapEndowments.Rpc,
+};
+
 export const MOCK_SNAP_DIALOG_PERMISSION: PermissionConstraint = {
   caveats: null,
   date: 1664187844588,

packages/snaps-controllers/src/utils.test.ts

@@ -5,8 +5,16 @@ import {
 } from '@metamask/snaps-utils/test-utils';
 import { assert } from '@metamask/utils';
 
-import { LoopbackLocation } from './test-utils';
-import { getSnapFiles, setDiff } from './utils';
+import { SnapEndowments } from '../../snaps-rpc-methods/src/endowments';
+import {
+  LoopbackLocation,
+  MOCK_ALLOWED_RPC_ORIGINS_PERMISSION,
+  MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
+  MOCK_LIFECYCLE_HOOKS_PERMISSION,
+  MOCK_RPC_ORIGINS_PERMISSION,
+  MOCK_SNAP_DIALOG_PERMISSION,
+} from './test-utils';
+import { getSnapFiles, permissionsDiff, setDiff } from './utils';
 
 describe('setDiff', () => {
   it('does nothing on empty type {}-B', () => {
@@ -38,6 +46,93 @@ describe('setDiff', () => {
   });
 });
 
+describe('permissionsDiff', () => {
+  it('does nothing on empty type {}-B', () => {
+    expect(
+      permissionsDiff(
+        {},
+        { [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION },
+      ),
+    ).toStrictEqual({});
+  });
+
+  it('does nothing on empty type A-{}', () => {
+    expect(
+      permissionsDiff(
+        { [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION },
+        {},
+      ),
+    ).toStrictEqual({
+      [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
+    });
+  });
+
+  it('does a difference', () => {
+    expect(
+      permissionsDiff(
+        {
+          [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
+          [SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
+        },
+        { [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION },
+      ),
+    ).toStrictEqual({
+      [SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
+    });
+  });
+
+  it('additional B permissions have no effect in A-B', () => {
+    expect(
+      permissionsDiff(
+        {
+          [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
+          [SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
+        },
+        {
+          [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION,
+          // eslint-disable-next-line @typescript-eslint/naming-convention
+          snap_dialog: MOCK_SNAP_DIALOG_PERMISSION,
+        },
+      ),
+    ).toStrictEqual({
+      [SnapEndowments.LifecycleHooks]: MOCK_LIFECYCLE_HOOKS_PERMISSION,
+    });
+  });
+
+  it('considers caveats in diff', () => {
+    expect(
+      permissionsDiff(
+        {
+          [SnapEndowments.Rpc]: MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
+        },
+        {
+          [SnapEndowments.Rpc]: MOCK_ALLOWED_RPC_ORIGINS_PERMISSION,
+        },
+      ),
+    ).toStrictEqual({
+      [SnapEndowments.Rpc]: MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
+    });
+
+    expect(
+      permissionsDiff(
+        {
+          [SnapEndowments.Rpc]: MOCK_ALLOWED_RPC_ORIGINS_PERMISSION,
+        },
+        {
+          [SnapEndowments.Rpc]: MOCK_DAPPS_RPC_ORIGINS_PERMISSION,
+        },
+      ),
+    ).toStrictEqual({
+      [SnapEndowments.Rpc]: MOCK_ALLOWED_RPC_ORIGINS_PERMISSION,
+    });
+  });
+
+  it('works for the same permissions A-A', () => {
+    const object = { [SnapEndowments.Rpc]: MOCK_RPC_ORIGINS_PERMISSION };
+    expect(permissionsDiff(object, object)).toStrictEqual({});
+  });
+});
+
 describe('getSnapFiles', () => {
   it('returns an empty array if `files` is undefined', async () => {
     const location = new LoopbackLocation();

packages/snaps-controllers/src/utils.ts

@@ -1,10 +1,12 @@
+import type { PermissionConstraint } from '@metamask/permission-controller';
 import type { SnapId } from '@metamask/snaps-sdk';
 import { getErrorMessage } from '@metamask/snaps-sdk';
 import {
   encodeBase64,
   getValidatedLocalizationFiles,
   validateFetchedSnap,
 } from '@metamask/snaps-utils';
+import deepEqual from 'fast-deep-equal';
 
 import type { SnapLocation } from './snaps';
 import { Timer } from './snaps/Timer';
@@ -38,6 +40,38 @@ export function setDiff<
   ) as Diff<ObjectA, ObjectB>;
 }
 
+/**
+ * Calculate a difference between two permissions objects.
+ *
+ * Similar to `setDiff` except for one additional condition:
+ * Permissions in B should be removed from A if they exist in both and have differing caveats.
+ *
+ * @param permissionsA - An object containing one or more partial permissions.
+ * @param permissionsB - An object containing one or more partial permissions to be subtracted from A.
+ * @returns The permissions set A without properties from B.
+ */
+export function permissionsDiff<
+  PermissionsA extends Record<string, Pick<PermissionConstraint, 'caveats'>>,
+  PermissionsB extends Record<string, Pick<PermissionConstraint, 'caveats'>>,
+>(
+  permissionsA: PermissionsA,
+  permissionsB: PermissionsB,
+): Diff<PermissionsA, PermissionsB> {
+  return Object.entries(permissionsA).reduce<
+    Record<string, Pick<PermissionConstraint, 'caveats'>>
+  >((acc, [key, value]) => {
+    const isIncluded = key in permissionsB;
+    if (
+      !isIncluded ||
+      (isIncluded &&
+        !deepEqual(value.caveats ?? [], permissionsB[key].caveats ?? []))
+    ) {
+      acc[key] = value;
+    }
+    return acc;
+  }, {}) as Diff<PermissionsA, PermissionsB>;
+}
+
 /**
  * A Promise that delays its return for a given amount of milliseconds.
  *

yarn.lock

@@ -5537,6 +5537,7 @@ __metadata:
     eslint-plugin-prettier: ^4.2.1
     eslint-plugin-promise: ^6.1.1
     expect-webdriverio: ^4.4.1
+    fast-deep-equal: ^3.1.3
     get-npm-tarball-url: ^2.0.3
     immer: ^9.0.6
     istanbul-lib-coverage: ^3.2.0