Fix a few issues with passing non-serializable JSON (#1974)

`AbstractExecutionService` will now properly guard against passing
invalid JSON-RPC requests and `BaseSnapExecutor` will try to respond
with an error if it receives an invalid JSON-RPC request that is deemed
valid enough to respond.

packages/snaps-controllers/coverage.json

@@ -1,6 +1,6 @@
 {
-  "branches": 90.26,
+  "branches": 90.47,
   "functions": 96.32,
-  "lines": 97.22,
-  "statements": 96.9
+  "lines": 97.28,
+  "statements": 96.96
 }

packages/snaps-controllers/src/services/AbstractExecutionService.test.ts

@@ -1,4 +1,5 @@
 import { HandlerType } from '@metamask/snaps-utils';
+import { MOCK_SNAP_ID } from '@metamask/snaps-utils/test-utils';
 
 import { createService } from '../test-utils';
 import type { ExecutionServiceArgs } from './AbstractExecutionService';
@@ -101,4 +102,29 @@ describe('AbstractExecutionService', () => {
       `Snap execution service returned no RPC handler for running snap "${snapId}".`,
     );
   });
+
+  it('throws an error if RPC request is non JSON serializable', async () => {
+    const { service } = createService(MockExecutionService);
+    await service.executeSnap({
+      snapId: MOCK_SNAP_ID,
+      sourceCode: `
+        console.log('foo');
+      `,
+      endowments: ['console'],
+    });
+
+    await expect(
+      service.handleRpcRequest(MOCK_SNAP_ID, {
+        origin: 'foo.com',
+        handler: HandlerType.OnRpcRequest,
+        request: {
+          id: 6,
+          method: 'bar',
+          params: undefined,
+        },
+      }),
+    ).rejects.toThrow(
+      'Invalid JSON-RPC request: At path: params -- Expected the value to satisfy a union of `record | array`, but received: [object Object].',
+    );
+  });
 });

packages/snaps-controllers/src/services/AbstractExecutionService.ts

@@ -10,7 +10,12 @@ import type {
   JsonRpcRequest,
   PendingJsonRpcResponse,
 } from '@metamask/utils';
-import { Duration, isJsonRpcNotification, isObject } from '@metamask/utils';
+import {
+  Duration,
+  assertIsJsonRpcRequest,
+  isJsonRpcNotification,
+  isObject,
+} from '@metamask/utils';
 import { createStreamMiddleware } from 'json-rpc-middleware-stream';
 import { nanoid } from 'nanoid';
 import { pipeline } from 'readable-stream';
@@ -362,9 +367,7 @@ export abstract class AbstractExecutionService<WorkerType>
     jobId: string,
     message: JsonRpcRequest,
   ): Promise<Json | undefined> {
-    if (typeof message !== 'object') {
-      throw new Error('Must send object.');
-    }
+    assertIsJsonRpcRequest(message);
 
     const job = this.jobs.get(jobId);
     if (!job) {

packages/snaps-execution-environments/coverage.json

@@ -1,6 +1,6 @@
 {
-  "branches": 79.57,
+  "branches": 80.82,
   "functions": 89.72,
-  "lines": 90.6,
-  "statements": 90.2
+  "lines": 90.79,
+  "statements": 90.39
 }

packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts

@@ -1921,6 +1921,51 @@ describe('BaseSnapExecutor', () => {
     });
   });
 
+  it('throws when receiving an invalid RPC request', async () => {
+    const executor = new TestSnapExecutor();
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 2,
+      method: 'snapRpc',
+      params: [
+        MOCK_SNAP_ID,
+        HandlerType.OnRpcRequest,
+        MOCK_ORIGIN,
+        // @ts-expect-error Invalid JSON
+        { jsonrpc: '2.0', method: 'foo', params: undefined },
+      ],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      id: 2,
+      error: {
+        code: -32603,
+        message: 'JSON-RPC requests must be JSON serializable objects.',
+        stack: expect.any(String),
+      },
+    });
+  });
+
+  it('logs when receiving an invalid RPC request that cannot be responded to', async () => {
+    const executor = new TestSnapExecutor();
+
+    const consoleSpy = spy(console, 'log');
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      // @ts-expect-error Invalid JSON
+      id: undefined,
+      method: 'snapRpc',
+      params: [MOCK_SNAP_ID, HandlerType.OnRpcRequest, MOCK_ORIGIN, {}],
+    });
+
+    expect(consoleSpy.calls[0]?.args[0]).toStrictEqual(
+      'Command stream received a non-JSON-RPC request, and was unable to respond.',
+    );
+  });
+
   it('contains the self-referential global scopes', async () => {
     const CODE = `
       module.exports.onRpcRequest = () => globalThis !== undefined &&

packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts

@@ -17,6 +17,7 @@ import {
   SNAP_EXPORTS,
   WrappedSnapError,
   unwrapError,
+  logInfo,
 } from '@metamask/snaps-utils';
 import type {
   JsonRpcNotification,
@@ -31,9 +32,10 @@ import {
   isJsonRpcRequest,
   hasProperty,
   getSafeJson,
+  JsonRpcIdStruct,
 } from '@metamask/utils';
 import type { Duplex } from 'readable-stream';
-import { validate } from 'superstruct';
+import { validate, is } from 'superstruct';
 
 import { log } from '../logging';
 import type { CommandMethodsMapping } from './commands';
@@ -213,10 +215,27 @@ export class BaseSnapExecutor {
 
   private async onCommandRequest(message: JsonRpcRequest) {
     if (!isJsonRpcRequest(message)) {
-      throw rpcErrors.invalidRequest({
-        message: 'Command stream received a non-JSON-RPC request.',
-        data: message,
-      });
+      if (
+        hasProperty(message, 'id') &&
+        is((message as Pick<JsonRpcRequest, 'id'>).id, JsonRpcIdStruct)
+      ) {
+        // Instead of throwing, we directly respond with an error.
+        // We can only do this if the message ID is still valid.
+        await this.#write({
+          error: serializeError(
+            rpcErrors.internal(
+              'JSON-RPC requests must be JSON serializable objects.',
+            ),
+          ),
+          id: (message as Pick<JsonRpcRequest, 'id'>).id,
+          jsonrpc: '2.0',
+        });
+      } else {
+        logInfo(
+          'Command stream received a non-JSON-RPC request, and was unable to respond.',
+        );
+      }
+      return;
     }
 
     const { id, method, params } = message;