Send valid JSON-RPC notifications from executors (#639)

`BaseSnapExecutor.notify()` was sending invalid [JSON-RPC notifications](https://www.jsonrpc.org/specification#notification) to the execution service over the command stream. This PR ensures that only valid notifications are sent.

packages/controllers/jest.config.js

@@ -8,10 +8,10 @@ module.exports = {
   coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
   coverageThreshold: {
     global: {
-      branches: 72.56,
+      branches: 72.14,
       functions: 86.41,
-      lines: 85.02,
-      statements: 85.1,
+      lines: 85.06,
+      statements: 85.15,
     },
   },
   globals: {

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

@@ -0,0 +1,112 @@
+import { ControllerMessenger } from '@metamask/controllers';
+import {
+  ErrorMessageEvent,
+  ExecutionServiceMessenger,
+} from './ExecutionService';
+import { NodeThreadExecutionService } from './node';
+
+class MockExecutionService extends NodeThreadExecutionService {
+  constructor(messenger: ExecutionServiceMessenger) {
+    super({
+      messenger,
+      setupSnapProvider: () => undefined,
+    });
+  }
+
+  getJobs() {
+    return this.jobs;
+  }
+}
+
+describe('AbstractExecutionService', () => {
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  it('logs error for unrecognized notifications', async () => {
+    const consoleErrorSpy = jest.spyOn(console, 'error');
+
+    const controllerMessenger = new ControllerMessenger<
+      never,
+      ErrorMessageEvent
+    >();
+    const service = new MockExecutionService(
+      controllerMessenger.getRestricted<
+        'ExecutionService',
+        never,
+        ErrorMessageEvent['type']
+      >({
+        name: 'ExecutionService',
+      }),
+    );
+
+    await service.executeSnap({
+      snapId: 'TestSnap',
+      sourceCode: `
+        console.log('foo');
+      `,
+      endowments: ['console'],
+    });
+
+    const { streams } = service.getJobs().values().next().value;
+    streams.command.emit('data', {
+      jsonrpc: '2.0',
+      method: 'foo',
+    });
+
+    await service.terminateAllSnaps();
+
+    expect(consoleErrorSpy).toHaveBeenCalledTimes(1);
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      new Error(`Received unexpected command stream notification "foo".`),
+    );
+  });
+
+  it('logs error for malformed UnhandledError notification', async () => {
+    const consoleErrorSpy = jest.spyOn(console, 'error');
+
+    const controllerMessenger = new ControllerMessenger<
+      never,
+      ErrorMessageEvent
+    >();
+    const service = new MockExecutionService(
+      controllerMessenger.getRestricted<
+        'ExecutionService',
+        never,
+        ErrorMessageEvent['type']
+      >({
+        name: 'ExecutionService',
+      }),
+    );
+
+    await service.executeSnap({
+      snapId: 'TestSnap',
+      sourceCode: `
+        console.log('foo');
+      `,
+      endowments: ['console'],
+    });
+
+    const { streams } = service.getJobs().values().next().value;
+    streams.command.emit('data', {
+      jsonrpc: '2.0',
+      method: 'UnhandledError',
+      params: 2,
+    });
+
+    streams.command.emit('data', {
+      jsonrpc: '2.0',
+      method: 'UnhandledError',
+      params: { error: null },
+    });
+
+    await service.terminateAllSnaps();
+
+    const expectedError = new Error(
+      `Received malformed "UnhandledError" command stream notification.`,
+    );
+    expect(consoleErrorSpy).toHaveBeenCalledTimes(2);
+    expect(consoleErrorSpy).toHaveBeenNthCalledWith(1, expectedError);
+    expect(consoleErrorSpy).toHaveBeenNthCalledWith(2, expectedError);
+  });
+});

packages/controllers/src/services/AbstractExecutionService.ts

@@ -1,8 +1,13 @@
 import { Duplex } from 'stream';
 import ObjectMultiplex from '@metamask/object-multiplex';
-import { SnapExecutionData } from '@metamask/snap-types';
+import { ErrorJSON, SnapExecutionData } from '@metamask/snap-types';
 import { SNAP_STREAM_NAMES } from '@metamask/execution-environments';
-import { Duration } from '@metamask/utils';
+import {
+  Duration,
+  isJsonRpcRequest,
+  isObject,
+  JsonRpcNotification,
+} from '@metamask/utils';
 import {
   JsonRpcEngine,
   // TODO: Replace with @metamask/utils version after bumping json-rpc-engine
@@ -23,7 +28,7 @@ const controllerName = 'ExecutionService';
 
 export type SetupSnapProvider = (snapId: string, stream: Duplex) => void;
 
-type ExecutionServiceArgs = {
+export type ExecutionServiceArgs = {
   setupSnapProvider: SetupSnapProvider;
   messenger: ExecutionServiceMessenger;
   terminationTimeout?: number;
@@ -217,23 +222,40 @@ export abstract class AbstractExecutionService<WorkerType>
 
     // Handle out-of-band errors, i.e. errors thrown from the snap outside of the req/res cycle.
     // Also keep track of outbound request/responses
-    const notificationHandler = (data: any) => {
-      if (data.id !== null && data.id !== undefined) {
+    const notificationHandler = (
+      message: JsonRpcRequest<unknown> | JsonRpcNotification<unknown>,
+    ) => {
+      if (isJsonRpcRequest(message)) {
         return;
       }
+
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
       const snapId = this.jobToSnapMap.get(jobId)!;
-      if (data.result?.type === 'OutboundRequest') {
+      if (message.method === 'OutboundRequest') {
         this._messenger.publish('ExecutionService:outboundRequest', snapId);
-      } else if (data.result?.type === 'OutboundResponse') {
+      } else if (message.method === 'OutboundResponse') {
         this._messenger.publish('ExecutionService:outboundResponse', snapId);
-      } else if (data.error) {
-        this._messenger.publish(
-          'ExecutionService:unhandledError',
-          snapId,
-          data.error,
+      } else if (message.method === 'UnhandledError') {
+        if (isObject(message.params) && message.params.error) {
+          this._messenger.publish(
+            'ExecutionService:unhandledError',
+            snapId,
+            message.params.error as ErrorJSON,
+          );
+          commandStream.removeListener('data', notificationHandler);
+        } else {
+          console.error(
+            new Error(
+              `Received malformed "${message.method}" command stream notification.`,
+            ),
+          );
+        }
+      } else {
+        console.error(
+          new Error(
+            `Received unexpected command stream notification "${message.method}".`,
+          ),
         );
-        commandStream.removeListener('data', notificationHandler);
       }
     };
     commandStream.on('data', notificationHandler);

packages/controllers/src/services/iframe/IframeExecutionService.ts

@@ -5,27 +5,24 @@ import {
 import {
   Job,
   AbstractExecutionService,
-  SetupSnapProvider,
+  ExecutionServiceArgs,
 } from '../AbstractExecutionService';
-import { ExecutionServiceMessenger } from '../ExecutionService';
 
 type IframeExecutionEnvironmentServiceArgs = {
-  setupSnapProvider: SetupSnapProvider;
   iframeUrl: URL;
-  messenger: ExecutionServiceMessenger;
-};
+} & ExecutionServiceArgs;
 
 export class IframeExecutionService extends AbstractExecutionService<Window> {
   public iframeUrl: URL;
 
   constructor({
-    setupSnapProvider,
     iframeUrl,
     messenger,
+    setupSnapProvider,
   }: IframeExecutionEnvironmentServiceArgs) {
     super({
-      setupSnapProvider,
       messenger,
+      setupSnapProvider,
     });
     this.iframeUrl = iframeUrl;
   }

packages/execution-environments/jest.config.js

@@ -6,7 +6,7 @@ module.exports = {
   coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'],
   coverageThreshold: {
     global: {
-      branches: 84.62,
+      branches: 85,
       functions: 92.38,
       lines: 86.18,
       statements: 86.38,

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

@@ -465,7 +465,7 @@ describe('BaseSnapExecutor', () => {
 
     expect(await executor.readCommand()).toStrictEqual({
       jsonrpc: '2.0',
-      result: { type: 'OutboundRequest' },
+      method: 'OutboundRequest',
     });
 
     const providerRequest = await executor.readRpc();
@@ -512,7 +512,7 @@ describe('BaseSnapExecutor', () => {
 
     expect(await executor.readCommand()).toStrictEqual({
       jsonrpc: '2.0',
-      result: { type: 'OutboundResponse' },
+      method: 'OutboundResponse',
     });
 
     expect(await executor.readCommand()).toStrictEqual({
@@ -570,13 +570,16 @@ describe('BaseSnapExecutor', () => {
 
     expect(await executor.readCommand()).toStrictEqual({
       jsonrpc: '2.0',
-      error: {
-        code: -32603,
-        data: {
-          stack: testError.stack,
-          snapName: 'local:foo',
+      method: 'UnhandledError',
+      params: {
+        error: {
+          code: -32603,
+          data: {
+            stack: testError.stack,
+            snapName: 'local:foo',
+          },
+          message: testError.message,
         },
-        message: testError.message,
       },
     });
   });
@@ -629,13 +632,16 @@ describe('BaseSnapExecutor', () => {
 
     expect(await executor.readCommand()).toStrictEqual({
       jsonrpc: '2.0',
-      error: {
-        code: -32603,
-        data: {
-          stack: testError.stack,
-          snapName: 'local:foo',
+      method: 'UnhandledError',
+      params: {
+        error: {
+          code: -32603,
+          data: {
+            stack: testError.stack,
+            snapName: 'local:foo',
+          },
+          message: testError.message,
         },
-        message: testError.message,
       },
     });
   });
@@ -688,13 +694,16 @@ describe('BaseSnapExecutor', () => {
 
     expect(await executor.readCommand()).toStrictEqual({
       jsonrpc: '2.0',
-      error: {
-        code: -32603,
-        data: {
-          stack: testError.stack,
-          snapName: 'local:foo',
+      method: 'UnhandledError',
+      params: {
+        error: {
+          code: -32603,
+          data: {
+            stack: testError.stack,
+            snapName: 'local:foo',
+          },
+          message: testError.message,
         },
-        message: testError.message,
       },
     });
   });

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

@@ -4,6 +4,7 @@ import { Duplex } from 'stream';
 import { MetaMaskInpageProvider } from '@metamask/providers';
 import { SnapProvider } from '@metamask/snap-types';
 import { errorCodes, ethErrors, serializeError } from 'eth-rpc-errors';
+import { JsonRpcNotification } from '@metamask/utils';
 import EEOpenRPCDocument from '../openrpc.json';
 import {
   Endowments,
@@ -68,6 +69,7 @@ export class BaseSnapExecutor {
             `No onRpcRequest handler exported for snap "${target}`,
           );
         }
+
         // We're capturing the handler in case someone modifies the data object before the call
         const handler = data.exports.onRpcRequest;
         return this.executeInSnapContext(target, () =>
@@ -85,11 +87,14 @@ export class BaseSnapExecutor {
       shouldIncludeStack: false,
     });
     this.notify({
-      error: {
-        ...serializedError,
-        data: {
-          ...data,
-          stack: constructedError?.stack,
+      method: 'UnhandledError',
+      params: {
+        error: {
+          ...serializedError,
+          data: {
+            ...data,
+            stack: constructedError?.stack,
+          },
         },
       },
     });
@@ -144,7 +149,12 @@ export class BaseSnapExecutor {
     }
   }
 
-  protected notify(requestObject: Record<string, unknown>) {
+  protected notify(
+    requestObject: Omit<
+      JsonRpcNotification<Record<string, unknown> | unknown[]>,
+      'jsonrpc'
+    >,
+  ) {
     this.commandStream.write({
       ...requestObject,
       jsonrpc: '2.0',
@@ -276,11 +286,11 @@ export class BaseSnapExecutor {
     const originalRequest = provider.request;
 
     provider.request = async (args) => {
-      this.notify({ result: { type: 'OutboundRequest' } });
+      this.notify({ method: 'OutboundRequest' });
       try {
         return await originalRequest(args);
       } finally {
-        this.notify({ result: { type: 'OutboundResponse' } });
+        this.notify({ method: 'OutboundResponse' });
       }
     };