refactor(wallet-cli): enforce RPC param validation at dispatch time via superstruct

eslint-suppressions.json

@@ -2240,6 +2240,11 @@
       "count": 2
     }
   },
+  "packages/transaction-pay-controller/src/strategy/bridge/bridge-submit.ts": {
+    "no-restricted-syntax": {
+      "count": 1
+    }
+  },
   "packages/transaction-pay-controller/src/strategy/relay/hyperliquid-withdraw.ts": {
     "no-restricted-syntax": {
       "count": 1

packages/wallet-cli/CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- The daemon RPC server now validates `params` against each handler's superstruct before dispatch, returning a `-32602 invalidParams` error on mismatch instead of passing raw params to the handler ([#8846](https://github.com/MetaMask/core/pull/8846))
 - Report daemon socket connection errors consistently across `mm daemon call` and `mm daemon list` ([#9339](https://github.com/MetaMask/core/pull/9339))
 - Bump `@metamask/wallet` from `^3.0.0` to `^7.0.1` ([#9218](https://github.com/MetaMask/core/pull/9218), [#9263](https://github.com/MetaMask/core/pull/9263), [#9349](https://github.com/MetaMask/core/pull/9349), [#9396](https://github.com/MetaMask/core/pull/9396), [#9470](https://github.com/MetaMask/core/pull/9470))
 

packages/wallet-cli/package.json

@@ -49,6 +49,7 @@
     "@metamask/remote-feature-flag-controller": "^4.2.2",
     "@metamask/rpc-errors": "^7.0.2",
     "@metamask/storage-service": "^1.0.2",
+    "@metamask/superstruct": "^3.1.0",
     "@metamask/utils": "^11.11.0",
     "@metamask/wallet": "^7.0.1",
     "@oclif/core": "^4.10.5",

packages/wallet-cli/src/daemon/daemon-entry.test.ts

@@ -1,3 +1,4 @@
+import { validate } from '@metamask/superstruct';
 import { appendFile, readFile, rm, writeFile } from 'node:fs/promises';
 
 import { pingDaemon } from './daemon-client';
@@ -489,7 +490,7 @@ describe('daemon-entry', () => {
 
     const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
     const { handlers } = callArgs;
-    const status = (await handlers.getStatus(null)) as {
+    const status = (await handlers.getStatus.run(null)) as {
       pid: number;
       uptime: number;
     };
@@ -512,14 +513,36 @@ describe('daemon-entry', () => {
     await importDaemonEntry();
 
     const { handlers } = mockStartRpcSocketServer.mock.calls[0][0];
-    const actions = await handlers.listActions(null);
+    const actions = await handlers.listActions.run(null);
 
     expect(actions).toStrictEqual([
       'NetworkController:getState',
       'KeyringController:getState',
     ]);
   });
 
+  it('getStatus paramsStruct rejects non-null params', async () => {
+    mockCreateWallet.mockResolvedValue(createMockWallet());
+    mockStartRpcSocketServer.mockResolvedValue(createMockHandle());
+
+    await importDaemonEntry();
+
+    const { handlers } = mockStartRpcSocketServer.mock.calls[0][0];
+    const [error] = validate(['unexpected'], handlers.getStatus.paramsStruct);
+    expect(error).toBeDefined();
+  });
+
+  it('listActions paramsStruct rejects non-null params', async () => {
+    mockCreateWallet.mockResolvedValue(createMockWallet());
+    mockStartRpcSocketServer.mockResolvedValue(createMockHandle());
+
+    await importDaemonEntry();
+
+    const { handlers } = mockStartRpcSocketServer.mock.calls[0][0];
+    const [error] = validate(['unexpected'], handlers.listActions.paramsStruct);
+    expect(error).toBeDefined();
+  });
+
   it('logs to file via makeLogger', async () => {
     mockCreateWallet.mockResolvedValue(createMockWallet());
     mockStartRpcSocketServer.mockResolvedValue(createMockHandle());
@@ -625,6 +648,28 @@ describe('daemon-entry', () => {
     );
   });
 
+  it('logs dispose error during shutdown without aborting cleanup', async () => {
+    const result = createMockWallet();
+    (result.dispose as jest.Mock).mockRejectedValue(
+      new Error('dispose failed'),
+    );
+    mockCreateWallet.mockResolvedValue(result);
+    const handle = createMockHandle();
+    mockStartRpcSocketServer.mockResolvedValue(handle);
+
+    await importDaemonEntry();
+
+    const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
+    const onShutdown = callArgs.onShutdown as () => Promise<void>;
+    await onShutdown();
+
+    expect(handle.close).toHaveBeenCalled();
+    expect(mockAppendFile).toHaveBeenCalledWith(
+      '/tmp/daemon.log',
+      expect.stringContaining('dispose() failed during shutdown'),
+    );
+  });
+
   it('handles rm rejection during shutdown cleanup gracefully', async () => {
     const result = createMockWallet();
     mockCreateWallet.mockResolvedValue(result);
@@ -714,13 +759,18 @@ describe('daemon-entry', () => {
 
   describe('call handler', () => {
     /**
-     * Import the daemon entry and extract the `call` handler from the
-     * handlers map, along with the mock wallet for assertions.
+     * Import the daemon entry and extract the `call` handler definition from
+     * the handlers map, along with the mock wallet for assertions.
      *
-     * @returns The call handler function and mock wallet result.
+     * @returns The call handler definition and mock wallet result.
      */
     async function setupCallHandler(): Promise<{
-      callHandler: (params: unknown) => Promise<unknown>;
+      callHandler: {
+        paramsStruct: import('@metamask/superstruct').Struct<
+          [string, ...unknown[]]
+        >;
+        run: (params: [string, ...unknown[]]) => Promise<unknown>;
+      };
       result: MockCreateWalletResult;
     }> {
       const result = createMockWallet();
@@ -730,28 +780,33 @@ describe('daemon-entry', () => {
       await importDaemonEntry();
 
       const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
-      const callHandler = callArgs.handlers.call as (
-        params: unknown,
-      ) => Promise<unknown>;
+      const callHandler = callArgs.handlers.call as unknown as {
+        paramsStruct: import('@metamask/superstruct').Struct<
+          [string, ...unknown[]]
+        >;
+        run: (params: [string, ...unknown[]]) => Promise<unknown>;
+      };
       return { callHandler, result };
     }
 
-    it('registers a call handler', async () => {
+    it('registers a call handler definition', async () => {
       mockCreateWallet.mockResolvedValue(createMockWallet());
       mockStartRpcSocketServer.mockResolvedValue(createMockHandle());
 
       await importDaemonEntry();
 
       const callArgs = mockStartRpcSocketServer.mock.calls[0][0];
-      expect(typeof callArgs.handlers.call).toBe('function');
+      const callDefinition = callArgs.handlers.call;
+      expect(callDefinition).toHaveProperty('paramsStruct');
+      expect(typeof callDefinition.run).toBe('function');
     });
 
     it('forwards action and args to messenger.call', async () => {
       const { callHandler, result } = await setupCallHandler();
       const mockCall = result.wallet.messenger.call as jest.Mock;
       mockCall.mockReturnValue({ accounts: [] });
 
-      const callResult = await callHandler([
+      const callResult = await callHandler.run([
         'Controller:action',
         'arg1',
         'arg2',
@@ -770,7 +825,7 @@ describe('daemon-entry', () => {
       const mockCall = result.wallet.messenger.call as jest.Mock;
       mockCall.mockReturnValue('ok');
 
-      await callHandler(['Controller:action']);
+      await callHandler.run(['Controller:action']);
 
       expect(mockCall).toHaveBeenCalledWith('Controller:action');
     });
@@ -780,7 +835,7 @@ describe('daemon-entry', () => {
       const mockCall = result.wallet.messenger.call as jest.Mock;
       mockCall.mockResolvedValue({ async: true });
 
-      const callResult = await callHandler(['Controller:asyncAction']);
+      const callResult = await callHandler.run(['Controller:asyncAction']);
 
       expect(callResult).toStrictEqual({ async: true });
     });
@@ -792,33 +847,29 @@ describe('daemon-entry', () => {
         throw new Error('A handler for Unknown:action has not been registered');
       });
 
-      await expect(callHandler(['Unknown:action'])).rejects.toThrow(
+      await expect(callHandler.run(['Unknown:action'])).rejects.toThrow(
         'A handler for Unknown:action has not been registered',
       );
     });
 
-    it('throws when params is null', async () => {
-      const { callHandler } = await setupCallHandler();
-
-      await expect(callHandler(null)).rejects.toThrow(
-        'Expected params to be an array with an action name',
-      );
-    });
-
-    it('throws when params is an empty array', async () => {
+    it.each([
+      ['null', null],
+      ['empty array', []],
+      ['non-string first element', [42]],
+      ['non-array', { foo: 'bar' }],
+    ])('paramsStruct rejects invalid params (%s)', async (_label, value) => {
       const { callHandler } = await setupCallHandler();
-
-      await expect(callHandler([])).rejects.toThrow(
-        'Expected params to be an array with an action name',
-      );
+      const [error] = validate(value, callHandler.paramsStruct);
+      expect(error).toBeDefined();
     });
 
-    it('throws when action name is not a string', async () => {
+    it('paramsStruct accepts a non-empty array starting with a string', async () => {
       const { callHandler } = await setupCallHandler();
-
-      await expect(callHandler([42])).rejects.toThrow(
-        'Expected params to be an array with an action name',
+      const [error] = validate(
+        ['Controller:action', 1, 'two'],
+        callHandler.paramsStruct,
       );
+      expect(error).toBeUndefined();
     });
   });
 });

packages/wallet-cli/src/daemon/daemon-entry.ts

@@ -1,3 +1,4 @@
+import { define, literal } from '@metamask/superstruct';
 import type { Json } from '@metamask/utils';
 import type { Wallet } from '@metamask/wallet';
 import { appendFile, readFile, rm, writeFile } from 'node:fs/promises';
@@ -7,10 +8,37 @@ import { ensureOwnerOnlyDirectory } from './data-dir';
 import { getDaemonPaths } from './paths';
 import { startRpcSocketServer } from './rpc-socket-server';
 import type { RpcSocketServerHandle } from './rpc-socket-server';
-import type { DaemonStatusInfo, Logger, RpcHandlerMap } from './types';
+import { defineHandler } from './types';
+import type {
+  DaemonStatusInfo,
+  Logger,
+  RpcDispatcher,
+  RpcHandlerMap,
+} from './types';
 import { isErrorWithCode, isProcessAlive, readPidFile } from './utils';
 import { createWallet } from './wallet-factory';
 
+/**
+ * Params struct for the `call` RPC method. `params` must be a non-empty array
+ * whose first element is the messenger action name; remaining elements are
+ * positional action arguments forwarded as-is to `messenger.call`.
+ */
+const callParamsStruct = define<[string, ...unknown[]]>(
+  'CallParams',
+  (value) => {
+    if (!Array.isArray(value)) {
+      return 'Expected an array';
+    }
+    if (value.length === 0) {
+      return 'Expected a non-empty array';
+    }
+    if (typeof value[0] !== 'string') {
+      return 'Expected the first element to be a string action name';
+    }
+    return true;
+  },
+);
+
 const startTime = Date.now();
 
 main().catch((error: unknown) => {
@@ -98,39 +126,37 @@ async function main(): Promise<void> {
     }));
 
     const constructedWallet = wallet;
+    // Arbitrary messenger dispatch is intentional: the CLI exposes the full
+    // messenger surface over a Unix socket inside the per-user oclif data
+    // directory. The dataDir is chmodded to 0o700 above and the socket to
+    // 0o600 by the RPC server on bind, so only the owning user can open them,
+    // but there is no in-process auth check beyond that filesystem-permission
+    // barrier. The messenger is strongly typed by action name; we narrow it
+    // once here to the RpcDispatcher shape the `call` handler needs.
+    const dispatch = constructedWallet.messenger.call.bind(
+      constructedWallet.messenger,
+    ) as unknown as RpcDispatcher;
+
     const handlers: RpcHandlerMap = {
-      getStatus: async (): Promise<DaemonStatusInfo> => ({
-        pid: process.pid,
-        uptime: Math.floor((Date.now() - startTime) / 1000),
+      getStatus: defineHandler(
+        literal(null),
+        async (): Promise<DaemonStatusInfo> => ({
+          pid: process.pid,
+          uptime: Math.floor((Date.now() - startTime) / 1000),
+        }),
+      ),
+      call: defineHandler(callParamsStruct, async (params) => {
+        const [action, ...args] = params;
+        return await dispatch(action, ...(args as Json[]));
       }),
       // Exposes the callable surface for discovery: it grows silently as
       // controllers are wired, so consumers need a way to see it without a
       // hand-kept catalog that would rot.
-      listActions: async (): Promise<Json> =>
-        constructedWallet.messenger.getRegisteredActionTypes(),
-      // Arbitrary messenger dispatch is intentional: the CLI exposes the full
-      // messenger surface over a Unix socket inside the per-user oclif data
-      // directory. The dataDir is chmodded to 0o700 above and the socket to
-      // 0o600 by the RPC server on bind, so only the owning user can open
-      // them, but there is no in-process auth check beyond that
-      // filesystem-permission barrier.
-      call: async (params) => {
-        if (!Array.isArray(params) || typeof params[0] !== 'string') {
-          throw new Error('Expected params to be an array with an action name');
-        }
-        const [action, ...args] = params as [string, ...Json[]];
-        // The messenger's `call` is typed to a literal action-name union; the
-        // daemon dispatches arbitrary action names from RPC. Cast to a
-        // string-keyed `call` (which preserves arity) rather than to `any`, so
-        // the only untyped value is the `unknown` result narrowed below.
-        type ArbitraryDispatch = {
-          call: (actionName: string, ...callArgs: Json[]) => unknown;
-        };
-        const result = (
-          constructedWallet.messenger as unknown as ArbitraryDispatch
-        ).call(action, ...args);
-        return (result instanceof Promise ? await result : result) as Json;
-      },
+      listActions: defineHandler(
+        literal(null),
+        async (): Promise<Json> =>
+          constructedWallet.messenger.getRegisteredActionTypes(),
+      ),
     };
 
     // `startRpcSocketServer` restricts the socket to the owner (chmod 0o600)
@@ -182,7 +208,11 @@ async function main(): Promise<void> {
         } catch (closeError) {
           log(`handle.close() failed: ${String(closeError)}`);
         }
-        await activeDispose();
+        try {
+          await activeDispose();
+        } catch (disposeError) {
+          log(`dispose() failed during shutdown: ${String(disposeError)}`);
+        }
         await Promise.all([
           removeOwnedPidFile(pidPath, pidFileContents).catch(
             (rmError: unknown) => {