Add JSON-RPC validation for invokeSnap

packages/rpc-methods/jest.config.js

@@ -7,10 +7,10 @@ module.exports = {
   coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'],
   coverageThreshold: {
     global: {
-      branches: 41.76,
-      functions: 55.84,
-      lines: 42.78,
-      statements: 42.63,
+      branches: 43.52,
+      functions: 59.74,
+      lines: 48.27,
+      statements: 48.04,
     },
   },
   moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'],

packages/rpc-methods/package.json

@@ -32,6 +32,7 @@
     "@metamask/types": "^1.1.0",
     "@metamask/utils": "^3.1.0",
     "eth-rpc-errors": "^4.0.2",
+    "nanoid": "^3.1.31",
     "superstruct": "^0.16.5"
   },
   "devDependencies": {

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

@@ -0,0 +1,104 @@
+import { PermissionType } from '@metamask/controllers';
+import {
+  MOCK_SNAP_ID,
+  MOCK_ORIGIN,
+  getTruncatedSnap,
+} from '@metamask/snap-utils/test-utils';
+import { invokeSnapBuilder, getInvokeSnapImplementation } from './invokeSnap';
+
+describe('builder', () => {
+  it('has the expected shape', () => {
+    expect(invokeSnapBuilder).toMatchObject({
+      targetKey: 'wallet_snap_*',
+      specificationBuilder: expect.any(Function),
+      methodHooks: {
+        getSnap: true,
+        handleSnapRpcRequest: true,
+      },
+    });
+  });
+
+  it('builder outputs expected specification', () => {
+    expect(
+      invokeSnapBuilder.specificationBuilder({
+        methodHooks: {
+          getSnap: jest.fn(),
+          handleSnapRpcRequest: jest.fn(),
+        },
+      }),
+    ).toMatchObject({
+      permissionType: PermissionType.RestrictedMethod,
+      targetKey: 'wallet_snap_*',
+      allowedCaveats: null,
+      methodImplementation: expect.any(Function),
+    });
+  });
+});
+
+describe('implementation', () => {
+  const getMockHooks = () =>
+    ({
+      getSnap: jest.fn(),
+      handleSnapRpcRequest: jest.fn(),
+    } as any);
+  it('calls handleSnapRpcRequest', async () => {
+    const hooks = getMockHooks();
+    hooks.getSnap.mockImplementation(getTruncatedSnap);
+    const implementation = getInvokeSnapImplementation(hooks);
+    await implementation({
+      context: { origin: MOCK_ORIGIN },
+      method: `wallet_snap_${MOCK_SNAP_ID}`,
+      params: [{ method: 'foo', params: {} }],
+    });
+
+    expect(hooks.getSnap).toHaveBeenCalledTimes(1);
+    expect(hooks.getSnap).toHaveBeenCalledWith(MOCK_SNAP_ID);
+    expect(hooks.handleSnapRpcRequest).toHaveBeenCalledWith({
+      handler: 'onRpcRequest',
+      origin: MOCK_ORIGIN,
+      request: {
+        jsonrpc: '2.0',
+        id: expect.any(String),
+        method: 'foo',
+        params: {},
+      },
+      snapId: MOCK_SNAP_ID,
+    });
+  });
+
+  it('throws if snap is not installed', async () => {
+    const hooks = getMockHooks();
+    const implementation = getInvokeSnapImplementation(hooks);
+    await expect(
+      implementation({
+        context: { origin: MOCK_ORIGIN },
+        method: `wallet_snap_${MOCK_SNAP_ID}`,
+        params: [{ method: 'foo', params: {} }],
+      }),
+    ).rejects.toThrow(
+      `The snap "${MOCK_SNAP_ID}" is not installed. This is a bug, please report it.`,
+    );
+    expect(hooks.getSnap).toHaveBeenCalledTimes(1);
+    expect(hooks.getSnap).toHaveBeenCalledWith(MOCK_SNAP_ID);
+
+    expect(hooks.handleSnapRpcRequest).not.toHaveBeenCalled();
+  });
+
+  it('throws if request is not valid', async () => {
+    const hooks = getMockHooks();
+    hooks.getSnap.mockImplementation(getTruncatedSnap);
+    const implementation = getInvokeSnapImplementation(hooks);
+    await expect(
+      implementation({
+        context: { origin: MOCK_ORIGIN },
+        method: `wallet_snap_${MOCK_SNAP_ID}`,
+        params: [{}],
+      }),
+    ).rejects.toThrow(
+      'Must specify a valid JSON-RPC request object as single parameter.',
+    );
+
+    expect(hooks.getSnap).toHaveBeenCalledTimes(0);
+    expect(hooks.handleSnapRpcRequest).not.toHaveBeenCalled();
+  });
+});

packages/rpc-methods/src/restricted/invokeSnap.ts

@@ -4,7 +4,7 @@ import {
   ValidPermissionSpecification,
   PermissionType,
 } from '@metamask/controllers';
-import { isObject, Json, NonEmptyArray } from '@metamask/utils';
+import { isJsonRpcRequest, Json, NonEmptyArray } from '@metamask/utils';
 import { ethErrors } from 'eth-rpc-errors';
 import {
   Snap,
@@ -13,6 +13,7 @@ import {
   HandlerType,
   SnapRpcHookArgs,
 } from '@metamask/snap-utils';
+import { nanoid } from 'nanoid';
 
 const methodPrefix = SNAP_PREFIX;
 const targetKey = `${methodPrefix}*` as const;
@@ -86,23 +87,26 @@ export const invokeSnapBuilder = Object.freeze({
  * @returns The method implementation which returns the result of `handleSnapRpcRequest`.
  * @throws If the params are invalid.
  */
-function getInvokeSnapImplementation({
+export function getInvokeSnapImplementation({
   getSnap,
   handleSnapRpcRequest,
 }: InvokeSnapMethodHooks) {
   return async function invokeSnap(
     options: RestrictedMethodOptions<[Record<string, Json>]>,
   ): Promise<Json> {
     const { params = [], method, context } = options;
-    const request = params[0];
+    const rawRequest = params[0];
 
-    if (!isObject(request)) {
+    const request = { jsonrpc: '2.0', id: nanoid(), ...rawRequest };
+
+    if (!isJsonRpcRequest(request)) {
       throw ethErrors.rpc.invalidParams({
-        message: 'Must specify snap RPC request object as single parameter.',
+        message:
+          'Must specify a valid JSON-RPC request object as single parameter.',
       });
     }
 
-    const snapId = method.substr(SNAP_PREFIX.length);
+    const snapId = method.slice(SNAP_PREFIX.length);
 
     if (!getSnap(snapId)) {
       throw ethErrors.rpc.invalidRequest({

yarn.lock

@@ -3027,6 +3027,7 @@ __metadata:
     eth-rpc-errors: ^4.0.2
     jest: ^29.0.2
     jest-it-up: ^2.0.0
+    nanoid: ^3.1.31
     prettier: ^2.3.2
     prettier-plugin-packagejson: ^2.2.11
     rimraf: ^3.0.2