BREAKING: Add changes to support blocking snaps by shasum (MetaMask#767)

* Add changes to support blocking snaps by shasum Improve unit test Add some missing parts of the shasum implementation Add improvements to unit tests * Update SnapInfo type * Review refactoring
chynacho · Sep 16, 2022 · 3a77db7 · 3a77db7
1 parent 664e7f7
commit 3a77db7
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 14 deletions.
diff --git a/packages/controllers/src/snaps/SnapController.test.ts b/packages/controllers/src/snaps/SnapController.test.ts
@@ -3519,6 +3519,8 @@ describe('SnapController', () => {
     it('returns whether a version of a snap is blocked', async () => {
       const checkBlockListSpy = jest.fn();
       const snapId = 'npm:example';
+      const version = '1.0.0';
+      const shasum = 'source-shasum';
 
       const snapController = getSnapController(
         getSnapControllerOptions({
@@ -3529,12 +3531,31 @@ describe('SnapController', () => {
       checkBlockListSpy.mockResolvedValueOnce({
         [snapId]: { blocked: false },
       });
-      expect(await snapController.isBlocked(snapId, '1.0.0')).toBe(false);
+
+      expect(
+        await snapController.isBlocked(snapId, {
+          version,
+          shasum,
+        }),
+      ).toBe(false);
 
       checkBlockListSpy.mockResolvedValueOnce({
         [snapId]: { blocked: true },
       });
-      expect(await snapController.isBlocked(snapId, '1.0.0')).toBe(true);
+
+      expect(
+        await snapController.isBlocked(snapId, {
+          version,
+          shasum,
+        }),
+      ).toBe(true);
+
+      expect(checkBlockListSpy).toHaveBeenCalledWith({
+        [snapId]: {
+          version,
+          shasum,
+        },
+      });
     });
   });
 
@@ -3576,6 +3597,18 @@ describe('SnapController', () => {
       });
       await snapController.updateBlockedSnaps();
 
+      // Ensure that CheckSnapBlockListArg is correct
+      expect(checkBlockListSpy).toHaveBeenCalledWith({
+        [mockSnapA.id]: {
+          version: mockSnapA.manifest.version,
+          shasum: mockSnapA.manifest.source.shasum,
+        },
+        [mockSnapB.id]: {
+          version: mockSnapB.manifest.version,
+          shasum: mockSnapB.manifest.source.shasum,
+        },
+      });
+
       // A is blocked and disabled
       expect(snapController.get(mockSnapA.id)?.blocked).toBe(true);
       expect(snapController.get(mockSnapA.id)?.enabled).toBe(false);

diff --git a/packages/controllers/src/snaps/SnapController.ts b/packages/controllers/src/snaps/SnapController.ts
@@ -410,7 +410,8 @@ type FeatureFlags = {
 };
 
 type SemVerVersion = string;
-export type CheckSnapBlockListArg = Record<SnapId, SemVerVersion>;
+type SnapInfo = { version: SemVerVersion; shasum: string };
+export type CheckSnapBlockListArg = Record<SnapId, SnapInfo>;
 
 export type CheckSnapBlockListResult = Record<
   SnapId,
@@ -828,9 +829,12 @@ export class SnapController extends BaseController<
    */
   async updateBlockedSnaps(): Promise<void> {
     const blockedSnaps = await this._checkSnapBlockList(
-      Object.values(this.state.snaps).reduce<Record<SnapId, SemVerVersion>>(
+      Object.values(this.state.snaps).reduce<CheckSnapBlockListArg>(
         (blockListArg, snap) => {
-          blockListArg[snap.id] = snap.version;
+          blockListArg[snap.id] = {
+            version: snap.version,
+            shasum: snap.manifest.source.shasum,
+          };
           return blockListArg;
         },
         {},
@@ -910,14 +914,16 @@ export class SnapController extends BaseController<
    * Checks the block list to determine whether a version of a snap is blocked.
    *
    * @param snapId - The snap id to check.
-   * @param version - The version of the snap to check.
+   * @param snapInfo - Snap information containing version and shasum.
    * @returns Whether the version of the snap is blocked or not.
    */
   async isBlocked(
     snapId: ValidatedSnapId,
-    version: SemVerVersion,
+    snapInfo: SnapInfo,
   ): Promise<boolean> {
-    const result = await this._checkSnapBlockList({ [snapId]: version });
+    const result = await this._checkSnapBlockList({
+      [snapId]: snapInfo,
+    });
     return result[snapId].blocked;
   }
 
@@ -926,15 +932,15 @@ export class SnapController extends BaseController<
    * if {@link SnapController._checkSnapBlockList} is undefined.
    *
    * @param snapId - The id of the snap to check.
-   * @param version - The version to check.
+   * @param snapInfo - Snap information containing version and shasum.
    */
   private async _assertIsUnblocked(
     snapId: ValidatedSnapId,
-    version: SemVerVersion,
+    snapInfo: SnapInfo,
   ) {
-    if (await this.isBlocked(snapId, version)) {
+    if (await this.isBlocked(snapId, snapInfo)) {
       throw new Error(
-        `Cannot install version "${version}" of snap "${snapId}": the version is blocked.`,
+        `Cannot install version "${snapInfo.version}" of snap "${snapId}": the version is blocked.`,
       );
     }
   }
@@ -1582,7 +1588,10 @@ export class SnapController extends BaseController<
       return null;
     }
 
-    await this._assertIsUnblocked(snapId, newVersion);
+    await this._assertIsUnblocked(snapId, {
+      version: newVersion,
+      shasum: newSnap.manifest.source.shasum,
+    });
 
     const processedPermissions = this.processSnapPermissions(
       newSnap.manifest.initialPermissions,
@@ -1694,7 +1703,10 @@ export class SnapController extends BaseController<
         }
 
         const fetchedSnap = await this._fetchSnap(snapId, args.versionRange);
-        await this._assertIsUnblocked(snapId, fetchedSnap.manifest.version);
+        await this._assertIsUnblocked(snapId, {
+          version: fetchedSnap.manifest.version,
+          shasum: fetchedSnap.manifest.source.shasum,
+        });
 
         return this._set({
           ...args,