feat: Detect unused permissions in Snaps CLI (#3335)

This adds unused permission detection for handlers to the Snaps CLI. It
will detect two cases:

- The Snap uses a certain permission, but does not export a handler for
it.
- The Snap exports a handler, but does not request permission for it.

Unfortunately due to how it works, it did require some refactors to the
CLI:

- The eval step in the build commands was removed in favour of the
Webpack plugin. Having a separate step made it complicated to support
things like watch mode, since that's handled by Webpack.
- Eval must run in order to detect unused permissions, so it's now run
in the `manifest` command as well.

---------

Co-authored-by: Frederik Bolding <[email protected]>

Author: Mrtenz

README.md

@@ -45,6 +45,7 @@ linkStyle default opacity:0.5
   snaps_utils(["@metamask/snaps-utils"]);
   snaps_webpack_plugin(["@metamask/snaps-webpack-plugin"]);
   create_snap --> snaps_utils;
+  snaps_cli --> snaps_rpc_methods;
   snaps_cli --> snaps_sandbox;
   snaps_cli --> snaps_sdk;
   snaps_cli --> snaps_utils;
@@ -67,6 +68,7 @@ linkStyle default opacity:0.5
   snaps_simulation --> snaps_sdk;
   snaps_simulation --> snaps_utils;
   snaps_utils --> snaps_sdk;
+  snaps_webpack_plugin --> snaps_rpc_methods;
   snaps_webpack_plugin --> snaps_sdk;
   snaps_webpack_plugin --> snaps_utils;
 ```

eslint.config.mjs

@@ -14,6 +14,7 @@ const config = createConfig([
       '**/docs',
       '**/public',
       '.yarn',
+      '!packages/snaps-cli/src/commands/build',
     ],
   },
 

packages/snaps-cli/package.json

@@ -64,6 +64,7 @@
     "test:watch": "jest --watch"
   },
   "dependencies": {
+    "@metamask/snaps-rpc-methods": "workspace:^",
     "@metamask/snaps-sandbox": "workspace:^",
     "@metamask/snaps-sdk": "workspace:^",
     "@metamask/snaps-utils": "workspace:^",

packages/snaps-cli/src/__mocks__/ora.ts

@@ -9,7 +9,9 @@ class MockSpinner {
 
   fail = jest.fn();
 
-  stop = jest.fn();
+  stop = jest.fn().mockImplementation(() => {
+    this.isSpinning = false;
+  });
 
   clear = jest.fn();
 

packages/snaps-cli/src/builders.ts

@@ -19,6 +19,11 @@ const builders = {
     normalize: true,
   },
 
+  eval: {
+    describe: 'Evaluate the Snap bundle',
+    type: 'boolean',
+  },
+
   fix: {
     describe: 'Attempt to fix snap.manifest.json',
     type: 'boolean',

packages/snaps-cli/src/commands/build/build.e2e.test.ts

@@ -1,6 +1,3 @@
-import { promises as fs } from 'fs';
-import { join } from 'path';
-
 import type { TestRunner } from '../../test-utils';
 import { getCommandRunner } from '../../test-utils';
 
@@ -21,8 +18,9 @@ describe('mm-snap build', () => {
         expect.stringMatching(/Checking the input file\./u),
       );
       expect(runner.stdout).toContainEqual(
-        expect.stringMatching(/Building the snap bundle\./u),
+        expect.stringMatching(/Building the Snap bundle\./u),
       );
+
       expect(runner.stderr).toContainEqual(
         expect.stringMatching(
           /Compiled \d+ files? in \d+ms with \d+ warnings?\./u,
@@ -33,9 +31,6 @@ describe('mm-snap build', () => {
           'No icon found in the Snap manifest. It is recommended to include an icon for the Snap. See https://docs.metamask.io/snaps/how-to/design-a-snap/#guidelines-at-a-glance for more information.',
         ),
       );
-      expect(runner.stdout).toContainEqual(
-        expect.stringMatching(/Evaluating the snap bundle\./u),
-      );
       expect(runner.exitCode).toBe(0);
     },
   );

packages/snaps-cli/src/commands/build/build.test.ts

@@ -6,18 +6,16 @@ import { BundleAnalyzerPlugin } from 'webpack-bundle-analyzer';
 import { buildHandler } from './build';
 import { build } from './implementation';
 import { getMockConfig } from '../../test-utils';
-import { evaluate } from '../eval';
 
 jest.mock('fs');
-jest.mock('../eval');
 jest.mock('./implementation');
 
 jest.mock('webpack-bundle-analyzer', () => ({
   BundleAnalyzerPlugin: jest.fn(),
 }));
 
 describe('buildHandler', () => {
-  it('builds a snap', async () => {
+  it('builds a Snap', async () => {
     await fs.promises.writeFile('/input.js', DEFAULT_SNAP_BUNDLE);
 
     jest.spyOn(console, 'log').mockImplementation();
@@ -34,16 +32,12 @@ describe('buildHandler', () => {
     expect(process.exitCode).not.toBe(1);
     expect(build).toHaveBeenCalledWith(config, {
       analyze: false,
-      evaluate: false,
+      evaluate: true,
       spinner: expect.any(Object),
     });
-
-    expect(evaluate).toHaveBeenCalledWith(
-      expect.stringMatching(/.*output\.js.*/u),
-    );
   });
 
-  it('analyzes a snap bundle', async () => {
+  it('analyzes a Snap bundle', async () => {
     await fs.promises.writeFile('/input.js', DEFAULT_SNAP_BUNDLE);
 
     jest.spyOn(console, 'log').mockImplementation();
@@ -79,7 +73,7 @@ describe('buildHandler', () => {
     expect(process.exitCode).not.toBe(1);
     expect(build).toHaveBeenCalledWith(config, {
       analyze: true,
-      evaluate: false,
+      evaluate: true,
       spinner: expect.any(Object),
     });
 
@@ -106,8 +100,11 @@ describe('buildHandler', () => {
     await buildHandler(config);
 
     expect(process.exitCode).not.toBe(1);
-    expect(build).toHaveBeenCalled();
-    expect(evaluate).not.toHaveBeenCalled();
+    expect(build).toHaveBeenCalledWith(config, {
+      analyze: false,
+      evaluate: false,
+      spinner: expect.any(Object),
+    });
   });
 
   it('checks if the input file exists', async () => {
@@ -121,7 +118,7 @@ describe('buildHandler', () => {
     expect(process.exitCode).toBe(1);
     expect(log).toHaveBeenCalledWith(
       expect.stringMatching(
-        /Input file not found: ".+"\. Make sure that the "input" field in your snap config is correct\./u,
+        /Input file not found: ".+"\. Make sure that the "input" field in your Snap config is correct\./u,
       ),
     );
   });

packages/snaps-cli/src/commands/build/build.ts

@@ -1,76 +1,57 @@
 import { isFile } from '@metamask/snaps-utils/node';
 import { assert } from '@metamask/utils';
-import { resolve as pathResolve } from 'path';
 
 import { build } from './implementation';
 import { getBundleAnalyzerPort } from './utils';
 import type { ProcessedConfig } from '../../config';
 import { CommandError } from '../../errors';
 import type { Steps } from '../../utils';
-import { success, executeSteps, info } from '../../utils';
-import { evaluate } from '../eval';
+import { success, executeSteps } from '../../utils';
 
 export type BuildContext = {
   analyze: boolean;
   build: boolean;
   config: ProcessedConfig;
+  exports?: string[];
   port?: number;
 };
 
 export const steps: Steps<BuildContext> = [
   {
     name: 'Checking the input file.',
-    condition: ({ build }) => build,
+    condition: ({ build: enableBuild }) => enableBuild,
     task: async ({ config }) => {
       const { input } = config;
 
       if (!(await isFile(input))) {
         throw new CommandError(
-          `Input file not found: "${input}". Make sure that the "input" field in your snap config is correct.`,
+          `Input file not found: "${input}". Make sure that the "input" field in your Snap config is correct.`,
         );
       }
     },
   },
   {
-    name: 'Building the snap bundle.',
-    condition: ({ build }) => build,
-    task: async ({ analyze, build: enableBuild, config, spinner }) => {
-      // We don't evaluate the bundle here, because it's done in a separate
-      // step.
+    name: 'Building the Snap bundle.',
+    condition: ({ build: enableBuild }) => enableBuild,
+    task: async (context) => {
+      const { analyze, config, spinner } = context;
+
       const compiler = await build(config, {
         analyze,
-        evaluate: false,
+        evaluate: config.evaluate,
         spinner,
       });
 
       if (analyze) {
         return {
-          analyze,
-          build: enableBuild,
-          config,
-          spinner,
+          ...context,
           port: await getBundleAnalyzerPort(compiler),
         };
       }
 
       return undefined;
     },
   },
-  {
-    name: 'Evaluating the snap bundle.',
-    condition: ({ build, config }) => build && config.evaluate,
-    task: async ({ config, spinner }) => {
-      const path = pathResolve(
-        process.cwd(),
-        config.output.path,
-        config.output.filename,
-      );
-
-      await evaluate(path);
-
-      info(`Snap bundle evaluated successfully.`, spinner);
-    },
-  },
   {
     name: 'Running analyser.',
     condition: ({ analyze }) => analyze,

packages/snaps-cli/src/commands/build/implementation.test.ts

@@ -8,7 +8,7 @@ import {
 } from '@metamask/snaps-utils/test-utils';
 import type { SemVerVersion } from '@metamask/utils';
 import normalFs from 'fs';
-import { dirname, resolve } from 'path';
+import { dirname } from 'path';
 import type { Configuration } from 'webpack';
 
 import { build } from './implementation';

packages/snaps-cli/src/commands/build/implementation.ts

@@ -11,10 +11,7 @@ import { getCompiler } from '../../webpack';
  * @param options - The Webpack options.
  * @returns A promise that resolves when the bundle is built.
  */
-export async function build(
-  config: ProcessedConfig,
-  options?: WebpackOptions,
-) {
+export async function build(config: ProcessedConfig, options?: WebpackOptions) {
   const compiler = await getCompiler(config, options);
   return await new Promise<Compiler>((resolve, reject) => {
     compiler.run((runError) => {