Added Permissions validation to SnapManifest (#910)

* Added Permissions validation to SnapManifest

* Added missing permissions to validation

* Changes afte PR review

* Remove JSON Params hack

* Changes after PR review

* Update packages/execution-environments/src/common/BaseSnapExecutor.ts

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

* Fix example shasum

* Changes after rebase

* Fix test

* Coverage

* PR review

* yarn.lock

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

packages/examples/examples/insights/package.json

@@ -27,7 +27,7 @@
   },
   "dependencies": {
     "@metamask/abi-utils": "^1.0.0",
-    "@metamask/utils": "^3.3.0"
+    "@metamask/utils": "^3.3.1"
   },
   "devDependencies": {
     "@lavamoat/allow-scripts": "^2.0.3",

packages/examples/examples/insights/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps-monorepo.git"
   },
   "source": {
-    "shasum": "rWNQVlpSAlUXVByhpVyXxvNRTwEfabK4vS2MU6ANNwc=",
+    "shasum": "ZGhrs3HV9UT/d8A77+jBM4JtjojE1RGqCEk1HFg4xIk=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

packages/multichain-provider/package.json

@@ -27,7 +27,8 @@
   "dependencies": {
     "@metamask/safe-event-emitter": "^2.0.0",
     "@metamask/snaps-types": "^0.23.0",
-    "@metamask/utils": "^3.3.0",
+    "@metamask/snaps-utils": "^0.23.0",
+    "@metamask/utils": "^3.3.1",
     "nanoid": "^3.1.31"
   },
   "devDependencies": {

packages/multichain-provider/src/MultiChainProvider.test.ts

@@ -235,6 +235,47 @@ describe('MultiChainProvider', () => {
       expect(result).toBe('foo');
     });
 
+    it('sends params properly', async () => {
+      const msg = (params?: any) => ({
+        method: 'wallet_multiChainRequestHack',
+        params: {
+          id: expect.any(String),
+          jsonrpc: '2.0',
+          method: 'caip_request',
+          params: {
+            chainId: 'eip155:1',
+            request: {
+              method: 'asd',
+              params,
+            },
+          },
+        },
+      });
+
+      const provider = await getProvider();
+      const request = ethereum.request as jest.MockedFunction<
+        typeof ethereum.request
+      >;
+
+      request.mockImplementation(async () => 'foo');
+
+      await provider.request({
+        chainId: 'eip155:1',
+        request: {
+          method: 'asd',
+        },
+      });
+
+      await provider.request({
+        chainId: 'eip155:1',
+        request: { method: 'asd', params: { prop: 'bar' } },
+      });
+
+      expect(request).toHaveBeenCalledTimes(3);
+      expect(request).toHaveBeenNthCalledWith(2, msg());
+      expect(request).toHaveBeenNthCalledWith(3, msg({ prop: 'bar' }));
+    });
+
     it('generates a random request ID', async () => {
       const provider = await getProvider();
 

packages/multichain-provider/src/MultiChainProvider.ts

@@ -1,5 +1,4 @@
 import SafeEventEmitter from '@metamask/safe-event-emitter';
-import { nanoid } from 'nanoid';
 import {
   assertIsConnectArguments,
   assertIsMetaMaskNotification,
@@ -12,8 +11,9 @@ import {
   RequestNamespace,
   Session,
 } from '@metamask/snaps-utils';
-import { JsonRpcRequest } from '@metamask/utils';
+import { JsonRpcRequest, Json } from '@metamask/utils';
 import type { SnapProvider } from '@metamask/snaps-types';
+import { nanoid } from 'nanoid';
 import { Provider } from './Provider';
 
 declare global {
@@ -126,11 +126,17 @@ export class MultiChainProvider extends SafeEventEmitter implements Provider {
 
     assertIsMultiChainRequest(args);
 
+    // We're doing it this way to avoid sentRequest.params = undefined.
+    const sentRequest: Json = { method: args.request.method };
+    if (args.request.params !== undefined) {
+      sentRequest.params = args.request.params;
+    }
+
     return this.#rpcRequest({
       method: 'caip_request',
       params: {
         chainId: args.chainId,
-        request: { method: args.request.method, params: args.request.params },
+        request: sentRequest,
       },
     });
   }
@@ -152,7 +158,7 @@ export class MultiChainProvider extends SafeEventEmitter implements Provider {
    */
   async #rpcRequest(
     payload: { method: string } & Partial<
-      JsonRpcRequest<unknown[] | Record<string, unknown>>
+      JsonRpcRequest<Record<string, Json> | Json[] | undefined>
     >,
   ) {
     return await this.#getProvider().request({

packages/rpc-methods/jest.config.js

@@ -5,10 +5,10 @@ module.exports = deepmerge(baseConfig, {
   coveragePathIgnorePatterns: ['./src/index.ts'],
   coverageThreshold: {
     global: {
-      branches: 85.82,
-      functions: 80,
-      lines: 60.24,
-      statements: 60.24,
+      branches: 82.85,
+      functions: 80.85,
+      lines: 59.77,
+      statements: 59.77,
     },
   },
   testTimeout: 2500,

packages/rpc-methods/package.json

@@ -30,7 +30,7 @@
     "@metamask/key-tree": "^6.0.0",
     "@metamask/snaps-utils": "^0.23.0",
     "@metamask/types": "^1.1.0",
-    "@metamask/utils": "^3.3.0",
+    "@metamask/utils": "^3.3.1",
     "eth-rpc-errors": "^4.0.2",
     "nanoid": "^3.1.31",
     "superstruct": "^0.16.7"

packages/rpc-methods/src/restricted/getBip32Entropy.test.ts

@@ -5,81 +5,11 @@ import {
   getBip32EntropyCaveatSpecifications,
   getBip32EntropyImplementation,
   validateCaveatPaths,
-  validatePath,
 } from './getBip32Entropy';
 
 const TEST_SECRET_RECOVERY_PHRASE =
   'test test test test test test test test test test test ball';
 
-describe('validatePath', () => {
-  it.each([true, false, null, undefined, 'foo', [], new (class {})()])(
-    'throws if the value is not a plain object',
-    (value) => {
-      expect(() => validatePath(value)).toThrow('Expected a plain object.');
-    },
-  );
-
-  it.each([{}, { path: [] }, { path: 'foo' }])(
-    'throws if the path is invalid or empty',
-    () => {
-      expect(() => validatePath({})).toThrow(
-        'Invalid "path" parameter. The path must be a non-empty BIP-32 derivation path array.',
-      );
-    },
-  );
-
-  it('throws if the path does not start with "m"', () => {
-    expect(() => validatePath({ path: ["44'", "60'"] })).toThrow(
-      'Invalid "path" parameter. The path must start with "m".',
-    );
-  });
-
-  it.each([
-    { path: ['m', 'foo'] },
-    { path: ['m', '0', 'bar'] },
-    { path: ['m', 0] },
-  ])('throws if the path is invalid', (value) => {
-    expect(() => validatePath(value)).toThrow(
-      'Invalid "path" parameter. The path must be a valid BIP-32 derivation path array.',
-    );
-  });
-
-  it.each([{ path: ['m'] }, { path: ['m', "44'"] }])(
-    'throws if the path has a length of less than three',
-    (value) => {
-      expect(() => validatePath(value)).toThrow(
-        'Invalid "path" parameter. Paths must have a length of at least three.',
-      );
-    },
-  );
-
-  it('throws if the curve is invalid', () => {
-    expect(() =>
-      validatePath({ path: ['m', "44'", "60'"], curve: 'foo' }),
-    ).toThrow(
-      'Invalid "curve" parameter. The curve must be "secp256k1" or "ed25519".',
-    );
-  });
-
-  it('throws if the curve is ed25519 and the path has an unhardened index', () => {
-    expect(() =>
-      validatePath({ path: ['m', "44'", "60'", '1'], curve: 'ed25519' }),
-    ).toThrow(
-      'Invalid "path" parameter. Ed25519 does not support unhardened paths.',
-    );
-  });
-
-  it('does not throw if the path is valid', () => {
-    expect(() =>
-      validatePath({ path: ['m', "44'", "60'"], curve: 'secp256k1' }),
-    ).not.toThrow();
-
-    expect(() =>
-      validatePath({ path: ['m', "44'", "60'"], curve: 'ed25519' }),
-    ).not.toThrow();
-  });
-});
-
 describe('validateCaveatPaths', () => {
   it.each([[], null, undefined, 'foo'])(
     'throws if the value is not an array or empty',
@@ -89,7 +19,9 @@ describe('validateCaveatPaths', () => {
           type: SnapCaveatType.PermittedDerivationPaths,
           value,
         }),
-      ).toThrow('Expected non-empty array of paths.');
+      ).toThrow(
+        /^Invalid BIP-32 entropy caveat: At path: value -- Expected an? array/u,
+      ); // Different error messages for different types
     },
   );
 
@@ -99,7 +31,7 @@ describe('validateCaveatPaths', () => {
         type: SnapCaveatType.PermittedDerivationPaths,
         value: [{ path: ['foo'], curve: 'secp256k1' }],
       }),
-    ).toThrow('Invalid "path" parameter. The path must start with "m".');
+    ).toThrow('At path: value.0.path -- Path must start with "m".');
   });
 });
 
@@ -245,7 +177,7 @@ describe('getBip32EntropyCaveatSpecifications', () => {
           // @ts-expect-error Missing other required properties.
         })({ params: { ...params, path: [] } }),
       ).rejects.toThrow(
-        'Invalid "path" parameter. The path must be a non-empty BIP-32 derivation path array.',
+        'At path: path -- Path must be a non-empty BIP-32 derivation path array',
       );
     });
 
@@ -275,7 +207,7 @@ describe('getBip32EntropyCaveatSpecifications', () => {
           type: SnapCaveatType.PermittedDerivationPaths,
           value: [{ path: ['foo'], curve: 'secp256k1' }],
         }),
-      ).toThrow('Invalid "path" parameter. The path must start with "m".');
+      ).toThrow('At path: value.0.path -- Path must start with "m".');
     });
   });
 });