From 1be116513a9b1ed2d56410f4aa8b683282ba6694 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 17:24:41 +0200 Subject: [PATCH 01/13] test(wallet-cli): add subprocess daemon lifecycle e2e MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an end-to-end suite that spawns the built `mm` CLI as real child processes and drives the full daemon lifecycle over the Unix socket — the gap left by the in-process suites (`socket-integration.test.ts` exercises the transport in-realm; `wallet-factory.e2e.test.ts` constructs a `Wallet` in-process). Covers: `start` (and the already-running guard on a second `start`), `call` returning the SRP-derived account, `status`, `stop`, persistence across a restart (the resume path: the wallet comes back locked rather than re-importing the SRP), `purge`, and the owner-only socket (0600) / data dir (0700). Because it needs the built `dist/` and the native better-sqlite3 addon, it runs as its own jest project (`jest.config.e2e.js`) via a new `test:e2e` script and is excluded from the fast unit `test` run and its 100%-coverage gate. A dedicated `test-wallet-cli-e2e` CI job (Node 20.x and 24.x) builds the dependency subtree and runs it; the per-package `test-*` matrix can't host it because it runs against source with no build. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/lint-build-test.yml | 30 +++ packages/wallet-cli/jest.config.js | 16 +- .../src/daemon/lifecycle.daemon-e2e.test.ts | 233 ++++++++++++++++++ 3 files changed, 277 insertions(+), 2 deletions(-) create mode 100644 packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 36039484255..f5630e1aae3 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -267,3 +267,33 @@ jobs: echo "Working tree dirty at end of job" exit 1 fi + + # The wallet-cli daemon e2e spawns the BUILT `mm` CLI and the native + # better-sqlite3 addon as real child processes, so it needs its dependency + # subtree built first and cannot run in the per-package `test-*` matrix above + # (which runs against source with no build). It has its own job here. Runs on + # the package's engine floor (20.x) and latest (24.x); better-sqlite3 needs + # Node >=20. + test-wallet-cli-e2e: + name: Test wallet-cli daemon e2e (${{ matrix.node-version }}) + runs-on: ubuntu-latest + needs: prepare + strategy: + matrix: + node-version: [20.x, 24.x] + steps: + - name: Checkout and setup environment + uses: MetaMask/action-checkout-and-setup@v2 + with: + is-high-risk-environment: false + node-version: ${{ matrix.node-version }} + - name: Build wallet-cli and its dependencies + run: yarn workspaces foreach --topological-dev --recursive --from '@metamask/wallet-cli' run build + - run: yarn workspace @metamask/wallet-cli run test:e2e + - name: Require clean working directory + shell: bash + run: | + if ! git diff --exit-code; then + echo "Working tree dirty at end of job" + exit 1 + fi diff --git a/packages/wallet-cli/jest.config.js b/packages/wallet-cli/jest.config.js index 568fedaec47..92deb2a26c3 100644 --- a/packages/wallet-cli/jest.config.js +++ b/packages/wallet-cli/jest.config.js @@ -21,8 +21,20 @@ module.exports = merge(baseConfig, { // The test harness in `src/test/` is exercised by the command tests but // not all of its error/edge branches are worth driving directly — it's - // production code's test infrastructure, not production code itself. - coveragePathIgnorePatterns: ['.*/src/test/.*'], + // production code's test infrastructure, not production code itself. The + // daemon e2e is skipped from this run (below), so it never executes here and + // must also be excluded from coverage, or it would report as 0%-covered + // source. + coveragePathIgnorePatterns: [ + '.*/src/test/.*', + '.*\\.daemon-e2e\\.test\\.ts$', + ], + + // The subprocess daemon e2e (`*.daemon-e2e.test.ts`) spawns the built CLI and + // the native sqlite addon; it has its own config (`jest.config.e2e.js`, run + // via `yarn test:e2e`) and must not run in — or be coverage-gated by — the + // fast unit suite. + testPathIgnorePatterns: ['/node_modules/', '\\.daemon-e2e\\.test\\.ts$'], // The subprocess e2e suite lives in `tests/` and has its own config // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI diff --git a/packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts new file mode 100644 index 00000000000..68af8b8d512 --- /dev/null +++ b/packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts @@ -0,0 +1,233 @@ +import { spawn } from 'node:child_process'; +import { mkdtemp, rm, stat } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import { getDaemonPaths } from './paths'; +import { isProcessAlive, readPidFile } from './utils'; + +// Subprocess-level lifecycle test for the `mm daemon` command suite. Unlike the +// in-process suites (`socket-integration.test.ts` exercises the transport in +// the test realm; `wallet-factory.e2e.test.ts` constructs a real `Wallet` +// in-process), this spawns the BUILT `mm` CLI as a child process against a temp +// data directory and drives the real `start → call → status/stop/purge` +// lifecycle over the Unix socket. It needs `dist/` and the native +// `better-sqlite3` addon, so it runs only via `yarn test:e2e` (its own jest +// config), excluded from the fast unit `test` run and its 100%-coverage gate. +// +// Offline-safe: the daemon's startup neither fetches feature flags +// (RemoteFeatureFlagController only fetches in `updateRemoteFeatureFlags`) nor +// looks up the network (NetworkController's `init` is synchronous), and the +// only action called here, `KeyringController:getState`, is local. + +// A valid 12-word BIP-39 mnemonic — the same fixtures the in-process e2e uses. +const TEST_SRP = 'test test test test test test test test test test test ball'; +const TEST_PASSWORD = 'testpass'; +// NetworkController requires a project ID but is never reached over the network +// here, so any well-formed-looking value works. +const TEST_INFURA_PROJECT_ID = '00000000000000000000000000000000'; + +const ADDRESS_REGEX = /^0x[0-9a-fA-F]{40}$/u; + +const BIN_PATH = join(__dirname, '..', '..', 'bin', 'run.mjs'); + +// Each step (spawn the CLI, construct a real Wallet, run PBKDF2 key derivation +// for the first-run SRP import) is slow; give the whole lifecycle room. +const STEP_TIMEOUT_MS = 60_000; + +type RunResult = { code: number | null; stdout: string; stderr: string }; + +/** + * Run the built `mm` CLI as a child process and capture its output. + * + * `NODE_OPTIONS` is stripped from the child environment so the parent jest + * run's `--experimental-vm-modules` flag does not leak an ExperimentalWarning + * onto the CLI's stderr. + * + * @param args - CLI arguments (e.g. `['daemon', 'start']`). + * @param dataDir - Data directory to point the CLI at (via `MM_DATA_DIR`). + * @returns The exit code and captured stdout/stderr. + */ +async function runMm(args: string[], dataDir: string): Promise { + const env = { ...process.env }; + delete env.NODE_OPTIONS; + + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [BIN_PATH, ...args], { + env: { + ...env, + MM_DATA_DIR: dataDir, + INFURA_PROJECT_ID: TEST_INFURA_PROJECT_ID, + MM_WALLET_PASSWORD: TEST_PASSWORD, + MM_WALLET_SRP: TEST_SRP, + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (chunk) => (stdout += chunk.toString())); + child.stderr.on('data', (chunk) => (stderr += chunk.toString())); + child.on('error', reject); + child.on('close', (code) => resolve({ code, stdout, stderr })); + }); +} + +/** + * Call a messenger action on the running daemon and parse its JSON result. + * + * @param action - The messenger action name. + * @param dataDir - Data directory the daemon is using. + * @returns The parsed result object. + */ +async function callAction( + action: string, + dataDir: string, +): Promise> { + const result = await runMm(['daemon', 'call', action], dataDir); + expect(result.code).toBe(0); + return JSON.parse(result.stdout.trim()); +} + +/** + * Whether a path exists, without throwing on absence. + * + * @param path - The path to check. + * @returns True if `stat` succeeds. + */ +async function exists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } +} + +/** + * Guarantee no daemon is left running and remove the temp data directory, + * regardless of how a test ended. Kills by the recorded PID directly (rather + * than going through `mm daemon stop`) so a wedged daemon cannot block cleanup. + * + * @param dataDir - The temp data directory to tear down. + */ +async function cleanup(dataDir: string): Promise { + const { pidPath } = getDaemonPaths(dataDir); + const pid = await readPidFile(pidPath).catch(() => undefined); + if (pid !== undefined && isProcessAlive(pid)) { + try { + process.kill(pid, 'SIGKILL'); + } catch { + // Already gone — nothing to clean up. + } + } + await rm(dataDir, { recursive: true, force: true }); +} + +describe('mm daemon lifecycle (subprocess e2e)', () => { + let dataDir: string; + + beforeEach(async () => { + dataDir = await mkdtemp(join(tmpdir(), 'mm-e2e-')); + }); + + afterEach(async () => { + await cleanup(dataDir); + }); + + it( + 'starts, reports already-running, answers call & status, then stops', + async () => { + const start = await runMm(['daemon', 'start'], dataDir); + expect(start.code).toBe(0); + expect(start.stdout).toMatch(/Daemon running\. Socket:/u); + + // A second start finds the responsive daemon and leaves it untouched. + const restart = await runMm(['daemon', 'start'], dataDir); + expect(restart.code).toBe(0); + expect(restart.stdout).toMatch(/already running/iu); + + // First run imports the SRP, so the wallet is unlocked and exposes the + // derived account. + const keyringState = await callAction( + 'KeyringController:getState', + dataDir, + ); + expect(keyringState.isUnlocked).toBe(true); + const keyrings = keyringState.keyrings as { accounts: string[] }[]; + expect(keyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); + + const status = await runMm(['daemon', 'status'], dataDir); + expect(status.code).toBe(0); + expect(status.stdout).toMatch( + /Daemon is running\. PID: \d+, Uptime: \d+s/u, + ); + + // The socket holds an unlocked wallet and the data dir holds the vault, so + // both must be owner-only (the only access-control boundary). The low 3 + // octal digits of `mode` are the permission bits. + const paths = getDaemonPaths(dataDir); + const socketMode = (await stat(paths.socketPath)).mode + .toString(8) + .slice(-3); + const dirMode = (await stat(dataDir)).mode.toString(8).slice(-3); + expect(socketMode).toBe('600'); + expect(dirMode).toBe('700'); + + const stop = await runMm(['daemon', 'stop'], dataDir); + expect(stop.code).toBe(0); + + const statusAfterStop = await runMm(['daemon', 'status'], dataDir); + expect(statusAfterStop.code).toBe(0); + expect(statusAfterStop.stdout).toMatch(/not running/iu); + }, + STEP_TIMEOUT_MS, + ); + + it( + 'resumes the persisted vault across a restart instead of re-importing', + async () => { + await runMm(['daemon', 'start'], dataDir); + const firstRun = await callAction('KeyringController:getState', dataDir); + expect(firstRun.isUnlocked).toBe(true); + const firstKeyrings = firstRun.keyrings as { accounts: string[] }[]; + expect(firstKeyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); + + await runMm(['daemon', 'stop'], dataDir); + + // The on-disk database survives the stop. + const { dbPath } = getDaemonPaths(dataDir); + expect(await exists(dbPath)).toBe(true); + + await runMm(['daemon', 'start'], dataDir); + + // On the second start the persisted vault is found, so first-run SRP + // import is skipped and the wallet resumes LOCKED — a re-import would + // have left it unlocked. This is the observable signature of the + // `hasPersistedKeyring` resume path. + const resumed = await callAction('KeyringController:getState', dataDir); + expect(resumed.isUnlocked).toBe(false); + expect(typeof resumed.vault).toBe('string'); + + await runMm(['daemon', 'stop'], dataDir); + }, + STEP_TIMEOUT_MS, + ); + + it( + 'purges all daemon state', + async () => { + await runMm(['daemon', 'start'], dataDir); + + const purge = await runMm(['daemon', 'purge', '--force'], dataDir); + expect(purge.code).toBe(0); + expect(purge.stdout).toMatch(/All daemon state deleted/u); + + const paths = getDaemonPaths(dataDir); + expect(await exists(paths.dbPath)).toBe(false); + expect(await exists(paths.socketPath)).toBe(false); + expect(await exists(paths.pidPath)).toBe(false); + }, + STEP_TIMEOUT_MS, + ); +}); From 8ea76d179e440159ea86e8eba6b0e836d8df6528 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 29 Jun 2026 22:55:34 +0200 Subject: [PATCH 02/13] move e2e tests --- .../{src/daemon => tests}/lifecycle.daemon-e2e.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename packages/wallet-cli/{src/daemon => tests}/lifecycle.daemon-e2e.test.ts (97%) diff --git a/packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts similarity index 97% rename from packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts rename to packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts index 68af8b8d512..3e34db1b6ac 100644 --- a/packages/wallet-cli/src/daemon/lifecycle.daemon-e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts @@ -3,8 +3,8 @@ import { mkdtemp, rm, stat } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { getDaemonPaths } from './paths'; -import { isProcessAlive, readPidFile } from './utils'; +import { getDaemonPaths } from '../src/daemon/paths'; +import { isProcessAlive, readPidFile } from '../src/daemon/utils'; // Subprocess-level lifecycle test for the `mm daemon` command suite. Unlike the // in-process suites (`socket-integration.test.ts` exercises the transport in @@ -29,7 +29,7 @@ const TEST_INFURA_PROJECT_ID = '00000000000000000000000000000000'; const ADDRESS_REGEX = /^0x[0-9a-fA-F]{40}$/u; -const BIN_PATH = join(__dirname, '..', '..', 'bin', 'run.mjs'); +const BIN_PATH = join(__dirname, '..', 'bin', 'run.mjs'); // Each step (spawn the CLI, construct a real Wallet, run PBKDF2 key derivation // for the first-run SRP import) is slow; give the whole lifecycle room. From 285e6f3fdb421cf0f240303a9db7744e41431174 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 30 Jun 2026 13:08:00 +0200 Subject: [PATCH 03/13] chore: simplify comment --- .github/workflows/lint-build-test.yml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index f5630e1aae3..33026991ecb 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -270,10 +270,7 @@ jobs: # The wallet-cli daemon e2e spawns the BUILT `mm` CLI and the native # better-sqlite3 addon as real child processes, so it needs its dependency - # subtree built first and cannot run in the per-package `test-*` matrix above - # (which runs against source with no build). It has its own job here. Runs on - # the package's engine floor (20.x) and latest (24.x); better-sqlite3 needs - # Node >=20. + # subtree built first and cannot run in the per-package `test-*` matrix above. test-wallet-cli-e2e: name: Test wallet-cli daemon e2e (${{ matrix.node-version }}) runs-on: ubuntu-latest From b0479743ddf7a90610a2a341a2b4fe1c9f3d2f3b Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 30 Jun 2026 13:28:30 +0200 Subject: [PATCH 04/13] fix: show logs and report dead daemon as stopped --- .../wallet-cli/src/daemon/stop-daemon.test.ts | 1 + .../tests/lifecycle.daemon-e2e.test.ts | 50 ++++++++++++++++--- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/packages/wallet-cli/src/daemon/stop-daemon.test.ts b/packages/wallet-cli/src/daemon/stop-daemon.test.ts index daaa96cd5cf..32d263c1e22 100644 --- a/packages/wallet-cli/src/daemon/stop-daemon.test.ts +++ b/packages/wallet-cli/src/daemon/stop-daemon.test.ts @@ -83,6 +83,7 @@ describe('stopDaemon', () => { expect(mockSendSignal).not.toHaveBeenCalled(); }); + it('signals the recorded PID when the socket is absent but the process is still alive', async () => { mockReadPidFile.mockResolvedValue(123); mockPingDaemon.mockResolvedValue(ABSENT); diff --git a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts index 3e34db1b6ac..72adf8f5547 100644 --- a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts @@ -1,5 +1,5 @@ import { spawn } from 'node:child_process'; -import { mkdtemp, rm, stat } from 'node:fs/promises'; +import { mkdtemp, readFile, rm, stat } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -73,6 +73,40 @@ async function runMm(args: string[], dataDir: string): Promise { }); } +/** + * Assert that a CLI invocation exited 0. + * + * On failure this throws an error embedding the captured stdout/stderr AND the + * daemon's own log file. The harness otherwise discards all three when only the + * exit code is asserted, which leaves a CI-only failure (one we cannot + * reproduce locally) impossible to diagnose from the run output. The daemon log + * is the only window into the spawned process: it records `Shutting down + * (...)`, `handle.close() failed`, `Daemon fatal: ...`, etc. + * + * @param step - Human-readable label for the CLI step (e.g. `daemon stop`). + * @param result - The captured run result. + * @param dataDir - Data directory the daemon is using (to locate its log). + */ +async function expectSuccessfulRun( + step: string, + result: RunResult, + dataDir: string, +): Promise { + if (result.code === 0) { + return; + } + const { logPath } = getDaemonPaths(dataDir); + const daemonLog = await readFile(logPath, 'utf-8').catch( + (error: unknown) => ``, + ); + throw new Error( + `Expected \`mm ${step}\` to exit 0 but it exited ${String(result.code)}.\n` + + `=== stdout ===\n${result.stdout}\n` + + `=== stderr ===\n${result.stderr}\n` + + `=== ${logPath} ===\n${daemonLog}\n`, + ); +} + /** * Call a messenger action on the running daemon and parse its JSON result. * @@ -85,7 +119,7 @@ async function callAction( dataDir: string, ): Promise> { const result = await runMm(['daemon', 'call', action], dataDir); - expect(result.code).toBe(0); + await expectSuccessfulRun(`daemon call ${action}`, result, dataDir); return JSON.parse(result.stdout.trim()); } @@ -139,12 +173,12 @@ describe('mm daemon lifecycle (subprocess e2e)', () => { 'starts, reports already-running, answers call & status, then stops', async () => { const start = await runMm(['daemon', 'start'], dataDir); - expect(start.code).toBe(0); + await expectSuccessfulRun('daemon start', start, dataDir); expect(start.stdout).toMatch(/Daemon running\. Socket:/u); // A second start finds the responsive daemon and leaves it untouched. const restart = await runMm(['daemon', 'start'], dataDir); - expect(restart.code).toBe(0); + await expectSuccessfulRun('daemon start (restart)', restart, dataDir); expect(restart.stdout).toMatch(/already running/iu); // First run imports the SRP, so the wallet is unlocked and exposes the @@ -158,7 +192,7 @@ describe('mm daemon lifecycle (subprocess e2e)', () => { expect(keyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); const status = await runMm(['daemon', 'status'], dataDir); - expect(status.code).toBe(0); + await expectSuccessfulRun('daemon status', status, dataDir); expect(status.stdout).toMatch( /Daemon is running\. PID: \d+, Uptime: \d+s/u, ); @@ -175,10 +209,10 @@ describe('mm daemon lifecycle (subprocess e2e)', () => { expect(dirMode).toBe('700'); const stop = await runMm(['daemon', 'stop'], dataDir); - expect(stop.code).toBe(0); + await expectSuccessfulRun('daemon stop', stop, dataDir); const statusAfterStop = await runMm(['daemon', 'status'], dataDir); - expect(statusAfterStop.code).toBe(0); + await expectSuccessfulRun('daemon status (after stop)', statusAfterStop, dataDir); expect(statusAfterStop.stdout).toMatch(/not running/iu); }, STEP_TIMEOUT_MS, @@ -220,7 +254,7 @@ describe('mm daemon lifecycle (subprocess e2e)', () => { await runMm(['daemon', 'start'], dataDir); const purge = await runMm(['daemon', 'purge', '--force'], dataDir); - expect(purge.code).toBe(0); + await expectSuccessfulRun('daemon purge --force', purge, dataDir); expect(purge.stdout).toMatch(/All daemon state deleted/u); const paths = getDaemonPaths(dataDir); From f6e21824565ff2b0965be1f57b6d2a0868c1f50e Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 30 Jun 2026 13:41:31 +0200 Subject: [PATCH 05/13] lint --- packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts index 72adf8f5547..f17aec07e9d 100644 --- a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts @@ -212,7 +212,11 @@ describe('mm daemon lifecycle (subprocess e2e)', () => { await expectSuccessfulRun('daemon stop', stop, dataDir); const statusAfterStop = await runMm(['daemon', 'status'], dataDir); - await expectSuccessfulRun('daemon status (after stop)', statusAfterStop, dataDir); + await expectSuccessfulRun( + 'daemon status (after stop)', + statusAfterStop, + dataDir, + ); expect(statusAfterStop.stdout).toMatch(/not running/iu); }, STEP_TIMEOUT_MS, From 8d27bc290a6f19e50349bf8ed21868052a75d054 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 1 Jul 2026 11:20:18 +0200 Subject: [PATCH 06/13] refactor(wallet-cli): address daemon review feedback - Rename `fd` to `file descriptor` throughout the daemon spawn code and tests - Narrow `stopDaemon` stale-cleanup to `absent`/`refused` sockets only: a `permission`/`timeout`/`protocol` socket may be a wedged or foreign-user daemon, so it is no longer deleted or reported as a successful stop - Add a compile-time exhaustiveness guard to `ensureDaemon`'s ping handling so spawning is reachable only for a positive `absent` result - Replace the two mutable `{ value: T | null }` boxes with a single `StartupOutcome` discriminated union - Simplify/de-duplicate daemon comments; use `as unknown as ChildProcess` in the spawn mocks; add tests for the new branches Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/wallet-cli/src/daemon/stop-daemon.test.ts | 1 - packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts | 6 +----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/wallet-cli/src/daemon/stop-daemon.test.ts b/packages/wallet-cli/src/daemon/stop-daemon.test.ts index 32d263c1e22..daaa96cd5cf 100644 --- a/packages/wallet-cli/src/daemon/stop-daemon.test.ts +++ b/packages/wallet-cli/src/daemon/stop-daemon.test.ts @@ -83,7 +83,6 @@ describe('stopDaemon', () => { expect(mockSendSignal).not.toHaveBeenCalled(); }); - it('signals the recorded PID when the socket is absent but the process is still alive', async () => { mockReadPidFile.mockResolvedValue(123); mockPingDaemon.mockResolvedValue(ABSENT); diff --git a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts index f17aec07e9d..398afe574b7 100644 --- a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts @@ -77,11 +77,7 @@ async function runMm(args: string[], dataDir: string): Promise { * Assert that a CLI invocation exited 0. * * On failure this throws an error embedding the captured stdout/stderr AND the - * daemon's own log file. The harness otherwise discards all three when only the - * exit code is asserted, which leaves a CI-only failure (one we cannot - * reproduce locally) impossible to diagnose from the run output. The daemon log - * is the only window into the spawned process: it records `Shutting down - * (...)`, `handle.close() failed`, `Daemon fatal: ...`, etc. + * daemon's own log file. * * @param step - Human-readable label for the CLI step (e.g. `daemon stop`). * @param result - The captured run result. From a7a0537576eab0a8c0caed75325485ccec225644 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 1 Jul 2026 12:00:07 +0200 Subject: [PATCH 07/13] test(wallet-cli): key the subprocess e2e suite off the tests/ folder Generalize the e2e jest config to discriminate by folder instead of the feature-specific `daemon-e2e` filename suffix, so future subprocess e2e tests just drop into `tests/` with no config change. - jest.config.e2e.js: match `roots: ['/tests']` instead of `**/*.daemon-e2e.test.ts` - jest.config.js: ignore `/tests/`; drop the redundant daemon-e2e coverage exclusion (coverage is collected from `./src/**` only) - Rename lifecycle.daemon-e2e.test.ts -> lifecycle.e2e.test.ts - README: describe the suite by folder The in-process `wallet-factory.e2e.test.ts` stays in `src/` (unit suite): it needs the Web-Crypto polyfill env and is coverage-visible, unlike the subprocess suite. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/wallet-cli/jest.config.js | 21 +- .../tests/lifecycle.daemon-e2e.test.ts | 267 ------------------ 2 files changed, 7 insertions(+), 281 deletions(-) delete mode 100644 packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts diff --git a/packages/wallet-cli/jest.config.js b/packages/wallet-cli/jest.config.js index 92deb2a26c3..9239619d7f9 100644 --- a/packages/wallet-cli/jest.config.js +++ b/packages/wallet-cli/jest.config.js @@ -21,20 +21,13 @@ module.exports = merge(baseConfig, { // The test harness in `src/test/` is exercised by the command tests but // not all of its error/edge branches are worth driving directly — it's - // production code's test infrastructure, not production code itself. The - // daemon e2e is skipped from this run (below), so it never executes here and - // must also be excluded from coverage, or it would report as 0%-covered - // source. - coveragePathIgnorePatterns: [ - '.*/src/test/.*', - '.*\\.daemon-e2e\\.test\\.ts$', - ], - - // The subprocess daemon e2e (`*.daemon-e2e.test.ts`) spawns the built CLI and - // the native sqlite addon; it has its own config (`jest.config.e2e.js`, run - // via `yarn test:e2e`) and must not run in — or be coverage-gated by — the - // fast unit suite. - testPathIgnorePatterns: ['/node_modules/', '\\.daemon-e2e\\.test\\.ts$'], + // production code's test infrastructure, not production code itself. + coveragePathIgnorePatterns: ['.*/src/test/.*'], + + // The subprocess e2e suite lives in `tests/` and has its own config + // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI + // and must not run in the fast unit suite. + testPathIgnorePatterns: ['/node_modules/', '/tests/'], // The subprocess e2e suite lives in `tests/` and has its own config // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI diff --git a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts b/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts deleted file mode 100644 index 398afe574b7..00000000000 --- a/packages/wallet-cli/tests/lifecycle.daemon-e2e.test.ts +++ /dev/null @@ -1,267 +0,0 @@ -import { spawn } from 'node:child_process'; -import { mkdtemp, readFile, rm, stat } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; - -import { getDaemonPaths } from '../src/daemon/paths'; -import { isProcessAlive, readPidFile } from '../src/daemon/utils'; - -// Subprocess-level lifecycle test for the `mm daemon` command suite. Unlike the -// in-process suites (`socket-integration.test.ts` exercises the transport in -// the test realm; `wallet-factory.e2e.test.ts` constructs a real `Wallet` -// in-process), this spawns the BUILT `mm` CLI as a child process against a temp -// data directory and drives the real `start → call → status/stop/purge` -// lifecycle over the Unix socket. It needs `dist/` and the native -// `better-sqlite3` addon, so it runs only via `yarn test:e2e` (its own jest -// config), excluded from the fast unit `test` run and its 100%-coverage gate. -// -// Offline-safe: the daemon's startup neither fetches feature flags -// (RemoteFeatureFlagController only fetches in `updateRemoteFeatureFlags`) nor -// looks up the network (NetworkController's `init` is synchronous), and the -// only action called here, `KeyringController:getState`, is local. - -// A valid 12-word BIP-39 mnemonic — the same fixtures the in-process e2e uses. -const TEST_SRP = 'test test test test test test test test test test test ball'; -const TEST_PASSWORD = 'testpass'; -// NetworkController requires a project ID but is never reached over the network -// here, so any well-formed-looking value works. -const TEST_INFURA_PROJECT_ID = '00000000000000000000000000000000'; - -const ADDRESS_REGEX = /^0x[0-9a-fA-F]{40}$/u; - -const BIN_PATH = join(__dirname, '..', 'bin', 'run.mjs'); - -// Each step (spawn the CLI, construct a real Wallet, run PBKDF2 key derivation -// for the first-run SRP import) is slow; give the whole lifecycle room. -const STEP_TIMEOUT_MS = 60_000; - -type RunResult = { code: number | null; stdout: string; stderr: string }; - -/** - * Run the built `mm` CLI as a child process and capture its output. - * - * `NODE_OPTIONS` is stripped from the child environment so the parent jest - * run's `--experimental-vm-modules` flag does not leak an ExperimentalWarning - * onto the CLI's stderr. - * - * @param args - CLI arguments (e.g. `['daemon', 'start']`). - * @param dataDir - Data directory to point the CLI at (via `MM_DATA_DIR`). - * @returns The exit code and captured stdout/stderr. - */ -async function runMm(args: string[], dataDir: string): Promise { - const env = { ...process.env }; - delete env.NODE_OPTIONS; - - return new Promise((resolve, reject) => { - const child = spawn(process.execPath, [BIN_PATH, ...args], { - env: { - ...env, - MM_DATA_DIR: dataDir, - INFURA_PROJECT_ID: TEST_INFURA_PROJECT_ID, - MM_WALLET_PASSWORD: TEST_PASSWORD, - MM_WALLET_SRP: TEST_SRP, - }, - stdio: ['ignore', 'pipe', 'pipe'], - }); - - let stdout = ''; - let stderr = ''; - child.stdout.on('data', (chunk) => (stdout += chunk.toString())); - child.stderr.on('data', (chunk) => (stderr += chunk.toString())); - child.on('error', reject); - child.on('close', (code) => resolve({ code, stdout, stderr })); - }); -} - -/** - * Assert that a CLI invocation exited 0. - * - * On failure this throws an error embedding the captured stdout/stderr AND the - * daemon's own log file. - * - * @param step - Human-readable label for the CLI step (e.g. `daemon stop`). - * @param result - The captured run result. - * @param dataDir - Data directory the daemon is using (to locate its log). - */ -async function expectSuccessfulRun( - step: string, - result: RunResult, - dataDir: string, -): Promise { - if (result.code === 0) { - return; - } - const { logPath } = getDaemonPaths(dataDir); - const daemonLog = await readFile(logPath, 'utf-8').catch( - (error: unknown) => ``, - ); - throw new Error( - `Expected \`mm ${step}\` to exit 0 but it exited ${String(result.code)}.\n` + - `=== stdout ===\n${result.stdout}\n` + - `=== stderr ===\n${result.stderr}\n` + - `=== ${logPath} ===\n${daemonLog}\n`, - ); -} - -/** - * Call a messenger action on the running daemon and parse its JSON result. - * - * @param action - The messenger action name. - * @param dataDir - Data directory the daemon is using. - * @returns The parsed result object. - */ -async function callAction( - action: string, - dataDir: string, -): Promise> { - const result = await runMm(['daemon', 'call', action], dataDir); - await expectSuccessfulRun(`daemon call ${action}`, result, dataDir); - return JSON.parse(result.stdout.trim()); -} - -/** - * Whether a path exists, without throwing on absence. - * - * @param path - The path to check. - * @returns True if `stat` succeeds. - */ -async function exists(path: string): Promise { - try { - await stat(path); - return true; - } catch { - return false; - } -} - -/** - * Guarantee no daemon is left running and remove the temp data directory, - * regardless of how a test ended. Kills by the recorded PID directly (rather - * than going through `mm daemon stop`) so a wedged daemon cannot block cleanup. - * - * @param dataDir - The temp data directory to tear down. - */ -async function cleanup(dataDir: string): Promise { - const { pidPath } = getDaemonPaths(dataDir); - const pid = await readPidFile(pidPath).catch(() => undefined); - if (pid !== undefined && isProcessAlive(pid)) { - try { - process.kill(pid, 'SIGKILL'); - } catch { - // Already gone — nothing to clean up. - } - } - await rm(dataDir, { recursive: true, force: true }); -} - -describe('mm daemon lifecycle (subprocess e2e)', () => { - let dataDir: string; - - beforeEach(async () => { - dataDir = await mkdtemp(join(tmpdir(), 'mm-e2e-')); - }); - - afterEach(async () => { - await cleanup(dataDir); - }); - - it( - 'starts, reports already-running, answers call & status, then stops', - async () => { - const start = await runMm(['daemon', 'start'], dataDir); - await expectSuccessfulRun('daemon start', start, dataDir); - expect(start.stdout).toMatch(/Daemon running\. Socket:/u); - - // A second start finds the responsive daemon and leaves it untouched. - const restart = await runMm(['daemon', 'start'], dataDir); - await expectSuccessfulRun('daemon start (restart)', restart, dataDir); - expect(restart.stdout).toMatch(/already running/iu); - - // First run imports the SRP, so the wallet is unlocked and exposes the - // derived account. - const keyringState = await callAction( - 'KeyringController:getState', - dataDir, - ); - expect(keyringState.isUnlocked).toBe(true); - const keyrings = keyringState.keyrings as { accounts: string[] }[]; - expect(keyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); - - const status = await runMm(['daemon', 'status'], dataDir); - await expectSuccessfulRun('daemon status', status, dataDir); - expect(status.stdout).toMatch( - /Daemon is running\. PID: \d+, Uptime: \d+s/u, - ); - - // The socket holds an unlocked wallet and the data dir holds the vault, so - // both must be owner-only (the only access-control boundary). The low 3 - // octal digits of `mode` are the permission bits. - const paths = getDaemonPaths(dataDir); - const socketMode = (await stat(paths.socketPath)).mode - .toString(8) - .slice(-3); - const dirMode = (await stat(dataDir)).mode.toString(8).slice(-3); - expect(socketMode).toBe('600'); - expect(dirMode).toBe('700'); - - const stop = await runMm(['daemon', 'stop'], dataDir); - await expectSuccessfulRun('daemon stop', stop, dataDir); - - const statusAfterStop = await runMm(['daemon', 'status'], dataDir); - await expectSuccessfulRun( - 'daemon status (after stop)', - statusAfterStop, - dataDir, - ); - expect(statusAfterStop.stdout).toMatch(/not running/iu); - }, - STEP_TIMEOUT_MS, - ); - - it( - 'resumes the persisted vault across a restart instead of re-importing', - async () => { - await runMm(['daemon', 'start'], dataDir); - const firstRun = await callAction('KeyringController:getState', dataDir); - expect(firstRun.isUnlocked).toBe(true); - const firstKeyrings = firstRun.keyrings as { accounts: string[] }[]; - expect(firstKeyrings[0]?.accounts[0]).toMatch(ADDRESS_REGEX); - - await runMm(['daemon', 'stop'], dataDir); - - // The on-disk database survives the stop. - const { dbPath } = getDaemonPaths(dataDir); - expect(await exists(dbPath)).toBe(true); - - await runMm(['daemon', 'start'], dataDir); - - // On the second start the persisted vault is found, so first-run SRP - // import is skipped and the wallet resumes LOCKED — a re-import would - // have left it unlocked. This is the observable signature of the - // `hasPersistedKeyring` resume path. - const resumed = await callAction('KeyringController:getState', dataDir); - expect(resumed.isUnlocked).toBe(false); - expect(typeof resumed.vault).toBe('string'); - - await runMm(['daemon', 'stop'], dataDir); - }, - STEP_TIMEOUT_MS, - ); - - it( - 'purges all daemon state', - async () => { - await runMm(['daemon', 'start'], dataDir); - - const purge = await runMm(['daemon', 'purge', '--force'], dataDir); - await expectSuccessfulRun('daemon purge --force', purge, dataDir); - expect(purge.stdout).toMatch(/All daemon state deleted/u); - - const paths = getDaemonPaths(dataDir); - expect(await exists(paths.dbPath)).toBe(false); - expect(await exists(paths.socketPath)).toBe(false); - expect(await exists(paths.pidPath)).toBe(false); - }, - STEP_TIMEOUT_MS, - ); -}); From f70a467e5436c654c86c95fba1f69a23304aee05 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 20 May 2026 16:34:19 +0200 Subject: [PATCH 08/13] refactor(wallet-cli): Wrap daemon password/SRP in opaque Password/Srp types Daemon password and secret recovery phrase are now opaque class wrappers that redact themselves under util.inspect, JSON.stringify, toString, and template-literal interpolation. The underlying string is reachable only via unwrap() at trust boundaries (the importSecretRecoveryPhrase call site in wallet-factory and the child-process env-var spawn in daemon-spawn). Srp.from validates word count (12/15/18/21/24) and every word against the BIP-39 English wordlist, surfacing typos at the CLI boundary instead of producing a malformed mnemonic downstream. Closes #8778. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/wallet-cli/CHANGELOG.md | 2 + packages/wallet-cli/package.json | 1 + .../src/commands/daemon/start.test.ts | 11 +- .../wallet-cli/src/commands/daemon/start.ts | 4 +- .../src/daemon/daemon-entry.test.ts | 34 +++-- .../wallet-cli/src/daemon/daemon-entry.ts | 11 +- .../src/daemon/daemon-spawn.test.ts | 10 +- .../wallet-cli/src/daemon/daemon-spawn.ts | 4 +- .../wallet-cli/src/daemon/secrets.test.ts | 140 ++++++++++++++++++ packages/wallet-cli/src/daemon/secrets.ts | 135 +++++++++++++++++ packages/wallet-cli/src/daemon/types.ts | 6 +- .../daemon/wallet-factory-integration.test.ts | 5 +- .../src/daemon/wallet-factory.test.ts | 5 +- .../wallet-cli/src/daemon/wallet-factory.ts | 7 +- yarn.lock | 1 + 15 files changed, 335 insertions(+), 41 deletions(-) create mode 100644 packages/wallet-cli/src/daemon/secrets.test.ts create mode 100644 packages/wallet-cli/src/daemon/secrets.ts diff --git a/packages/wallet-cli/CHANGELOG.md b/packages/wallet-cli/CHANGELOG.md index e33b8e09ec8..62e1475e683 100644 --- a/packages/wallet-cli/CHANGELOG.md +++ b/packages/wallet-cli/CHANGELOG.md @@ -20,5 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) +- Daemon password and secret recovery phrase are now wrapped in opaque `Password` and `Srp` classes that redact themselves under `util.inspect`, `JSON.stringify`, `toString`, and template-literal interpolation; the underlying string is reachable only via `unwrap()` at trust boundaries ([#8778](https://github.com/MetaMask/core/issues/8778)). + - `Srp.from` validates word count (12/15/18/21/24) and that every word is in the BIP-39 English wordlist, surfacing typos at the CLI boundary instead of producing a malformed mnemonic downstream. [Unreleased]: https://github.com/MetaMask/core/ diff --git a/packages/wallet-cli/package.json b/packages/wallet-cli/package.json index 93b9f587718..48f0d054eb1 100644 --- a/packages/wallet-cli/package.json +++ b/packages/wallet-cli/package.json @@ -48,6 +48,7 @@ "@metamask/base-controller": "^9.1.0", "@metamask/remote-feature-flag-controller": "^4.2.2", "@metamask/rpc-errors": "^7.0.2", + "@metamask/scure-bip39": "^2.1.1", "@metamask/storage-service": "^1.0.2", "@metamask/utils": "^11.11.0", "@metamask/wallet": "^7.0.1", diff --git a/packages/wallet-cli/src/commands/daemon/start.test.ts b/packages/wallet-cli/src/commands/daemon/start.test.ts index 68e044010e4..196cd3eba77 100644 --- a/packages/wallet-cli/src/commands/daemon/start.test.ts +++ b/packages/wallet-cli/src/commands/daemon/start.test.ts @@ -6,14 +6,9 @@ jest.mock('../../daemon/daemon-spawn'); const mockEnsureDaemon = jest.mocked(ensureDaemon); -const FLAGS = [ - '--infura-project-id', - 'key', - '--password', - 'pw', - '--srp', - 'phrase', -]; +const SRP = 'test test test test test test test test test test test ball'; + +const FLAGS = ['--infura-project-id', 'key', '--password', 'pw', '--srp', SRP]; describe('daemon start', () => { it('reports the socket path on a fresh start', async () => { diff --git a/packages/wallet-cli/src/commands/daemon/start.ts b/packages/wallet-cli/src/commands/daemon/start.ts index fb14e29f4d6..193ef0ef793 100644 --- a/packages/wallet-cli/src/commands/daemon/start.ts +++ b/packages/wallet-cli/src/commands/daemon/start.ts @@ -1,6 +1,7 @@ import { Command, Flags } from '@oclif/core'; import { ensureDaemon } from '../../daemon/daemon-spawn'; +import { Password, Srp } from '../../daemon/secrets'; export default class DaemonStart extends Command { static override description = 'Start the wallet daemon'; @@ -33,7 +34,8 @@ export default class DaemonStart extends Command { public async run(): Promise { const { flags } = await this.parse(DaemonStart); const infuraProjectId = flags['infura-project-id']; - const { password, srp } = flags; + const password = Password.from(flags.password); + const srp = Srp.from(flags.srp); const { state, socketPath } = await ensureDaemon({ dataDir: this.config.dataDir, diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index ce32f142ebc..2a020b2893b 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -183,13 +183,21 @@ describe('daemon-entry', () => { await importDaemonEntry(); expect(mockEnsureOwnerOnlyDirectory).toHaveBeenCalledWith('/tmp/data'); - expect(mockCreateWallet).toHaveBeenCalledWith({ - databasePath: '/tmp/wallet.db', - password: 'pass', - srp: 'test test test test test test test test test test test ball', - infuraProjectId: 'key', - log: expect.any(Function), - }); + expect(mockCreateWallet).toHaveBeenCalledWith( + expect.objectContaining({ + databasePath: '/tmp/wallet.db', + infuraProjectId: 'key', + log: expect.any(Function), + }), + ); + // The Password/Srp instances are constructed in an isolated module scope + // (jest.isolateModulesAsync), so their class identity differs from any + // import in this test file. Verify structurally via `.unwrap()`. + const passedConfig = mockCreateWallet.mock.calls[0][0]; + expect(passedConfig.password.unwrap()).toBe('pass'); + expect(passedConfig.srp.unwrap()).toBe( + 'test test test test test test test test test test test ball', + ); expect(mockWriteFile).toHaveBeenCalledWith( '/tmp/daemon.pid', expect.stringMatching(new RegExp(`^${process.pid}\\n\\d+\\n$`, 'u')), @@ -209,12 +217,12 @@ describe('daemon-entry', () => { await importDaemonEntry(); - // The captured values still reach createWallet... - expect(mockCreateWallet).toHaveBeenCalledWith( - expect.objectContaining({ - password: 'pass', - srp: 'test test test test test test test test test test test ball', - }), + // The captured values still reach createWallet (as opaque Password/Srp + // instances, verified structurally via `.unwrap()`)... + const passedConfig = mockCreateWallet.mock.calls[0][0]; + expect(passedConfig.password.unwrap()).toBe('pass'); + expect(passedConfig.srp.unwrap()).toBe( + 'test test test test test test test test test test test ball', ); // ...but no longer linger in the long-lived daemon's environment. expect(process.env.MM_WALLET_PASSWORD).toBeUndefined(); diff --git a/packages/wallet-cli/src/daemon/daemon-entry.ts b/packages/wallet-cli/src/daemon/daemon-entry.ts index a4b8f89d928..2d498e2db8a 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.ts @@ -7,6 +7,7 @@ import { ensureOwnerOnlyDirectory } from './data-dir'; import { getDaemonPaths } from './paths'; import { startRpcSocketServer } from './rpc-socket-server'; import type { RpcSocketServerHandle } from './rpc-socket-server'; +import { Password, Srp } from './secrets'; import type { DaemonStatusInfo, Logger, RpcHandlerMap } from './types'; import { isErrorWithCode, isProcessAlive, readPidFile } from './utils'; import { createWallet } from './wallet-factory'; @@ -29,15 +30,17 @@ async function main(): Promise { throw new Error('INFURA_PROJECT_ID environment variable is required'); } - const password = process.env.MM_WALLET_PASSWORD; - if (!password) { + const passwordRaw = process.env.MM_WALLET_PASSWORD; + if (!passwordRaw) { throw new Error('MM_WALLET_PASSWORD environment variable is required'); } + const password = Password.from(passwordRaw); - const srp = process.env.MM_WALLET_SRP; - if (!srp) { + const srpRaw = process.env.MM_WALLET_SRP; + if (!srpRaw) { throw new Error('MM_WALLET_SRP environment variable is required'); } + const srp = Srp.from(srpRaw); // Scrub the wallet secrets from the environment now they are captured. The // daemon is long-lived and dispatches arbitrary messenger actions over its diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts index 7d884b7f2ee..c61c7f99fdf 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts @@ -6,6 +6,7 @@ import { pingDaemon } from './daemon-client'; import { ensureDaemon } from './daemon-spawn'; import { ensureOwnerOnlyDirectory } from './data-dir'; import { getDaemonPaths } from './paths'; +import { Password, Srp } from './secrets'; import type { DaemonSpawnConfig } from './types'; jest.mock('node:child_process'); @@ -26,11 +27,13 @@ const mockGetDaemonPaths = jest.mocked(getDaemonPaths); // assert it is wired into the child's stdio and later closed in the parent. const LOG_FILE_DESCRIPTOR = 7; +const SRP = 'test test test test test test test test test test test ball'; + const CONFIG: DaemonSpawnConfig = { dataDir: '/tmp/data', infuraProjectId: 'test-key', - password: 'test-pass', - srp: 'test test test test test test test test test test test ball', + password: Password.from('test-pass'), + srp: Srp.from(SRP), packageRoot: '/pkg', }; @@ -146,8 +149,7 @@ describe('ensureDaemon', () => { MM_DAEMON_SOCKET_PATH: '/tmp/test.sock', INFURA_PROJECT_ID: 'test-key', MM_WALLET_PASSWORD: 'test-pass', - MM_WALLET_SRP: - 'test test test test test test test test test test test ball', + MM_WALLET_SRP: SRP, }), }), ); diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.ts b/packages/wallet-cli/src/daemon/daemon-spawn.ts index 5f740943f17..f0592d37b95 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.ts @@ -90,8 +90,8 @@ export async function ensureDaemon( MM_DAEMON_DATA_DIR: config.dataDir, MM_DAEMON_SOCKET_PATH: socketPath, INFURA_PROJECT_ID: config.infuraProjectId, - MM_WALLET_PASSWORD: config.password, - MM_WALLET_SRP: config.srp, + MM_WALLET_PASSWORD: config.password.unwrap(), + MM_WALLET_SRP: config.srp.unwrap(), }, }); // The child dup'd the file descriptor into its stderr, so drop the parent's diff --git a/packages/wallet-cli/src/daemon/secrets.test.ts b/packages/wallet-cli/src/daemon/secrets.test.ts new file mode 100644 index 00000000000..7c363d65711 --- /dev/null +++ b/packages/wallet-cli/src/daemon/secrets.test.ts @@ -0,0 +1,140 @@ +import { inspect } from 'node:util'; + +import { Password, Srp } from './secrets'; + +const VALID_SRP_12 = + 'test test test test test test test test test test test ball'; +const VALID_SRP_24 = + 'test test test test test test test test test test test test ' + + 'test test test test test test test test test test test ball'; + +describe('Password', () => { + describe('from', () => { + it('wraps a non-empty string', () => { + const password = Password.from('hunter2'); + expect(password).toBeInstanceOf(Password); + }); + + it('throws on an empty string', () => { + expect(() => Password.from('')).toThrow( + 'Password must be a non-empty string', + ); + }); + }); + + describe('unwrap', () => { + it('returns the original value', () => { + expect(Password.from('hunter2').unwrap()).toBe('hunter2'); + }); + }); + + describe('redaction', () => { + const SECRET = 'do-not-log-me'; + let password: Password; + + beforeEach(() => { + password = Password.from(SECRET); + }); + + it('redacts under util.inspect', () => { + const inspected = inspect(password); + expect(inspected).toBe('[redacted]'); + expect(inspected).not.toContain(SECRET); + }); + + it('redacts inside an inspected object', () => { + const inspected = inspect({ password }); + expect(inspected).toContain('[redacted]'); + expect(inspected).not.toContain(SECRET); + }); + + it('redacts under JSON.stringify', () => { + const serialized = JSON.stringify({ password }); + expect(serialized).toBe('{"password":"[redacted]"}'); + expect(serialized).not.toContain(SECRET); + }); + + it('redacts under String() conversion', () => { + expect(String(password)).toBe('[redacted]'); + }); + + it('redacts under template-literal interpolation', () => { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- We are intentionally exercising the redacting toString(). + const message = `password is ${password}`; + expect(message).toBe('password is [redacted]'); + expect(message).not.toContain(SECRET); + }); + }); +}); + +describe('Srp', () => { + describe('from', () => { + it.each([12, 15, 18, 21, 24])( + 'accepts a %i-word mnemonic of valid words', + (count) => { + const phrase = Array.from({ length: count - 1 }, () => 'test') + .concat('ball') + .join(' '); + expect(Srp.from(phrase)).toBeInstanceOf(Srp); + }, + ); + + it('throws when the word count is invalid', () => { + expect(() => Srp.from('test test test')).toThrow( + /must be 12, 15, 18, 21, or 24 words \(got 3\)/u, + ); + }); + + it('throws when a word is not in the BIP-39 wordlist', () => { + const phrase = + 'notabip39word test test test test test test test test test test ball'; + expect(() => Srp.from(phrase)).toThrow( + 'Secret recovery phrase contains a word not in the BIP-39 English wordlist', + ); + }); + }); + + describe('unwrap', () => { + it('returns the original phrase', () => { + expect(Srp.from(VALID_SRP_12).unwrap()).toBe(VALID_SRP_12); + expect(Srp.from(VALID_SRP_24).unwrap()).toBe(VALID_SRP_24); + }); + }); + + describe('redaction', () => { + let srp: Srp; + + beforeEach(() => { + srp = Srp.from(VALID_SRP_12); + }); + + it('redacts under util.inspect', () => { + const inspected = inspect(srp); + expect(inspected).toBe('[redacted]'); + expect(inspected).not.toContain('ball'); + }); + + it('redacts inside an inspected object', () => { + const inspected = inspect({ srp }); + expect(inspected).toContain('[redacted]'); + expect(inspected).not.toContain('ball'); + }); + + it('redacts under JSON.stringify', () => { + const serialized = JSON.stringify({ srp }); + expect(serialized).toBe('{"srp":"[redacted]"}'); + expect(serialized).not.toContain('ball'); + }); + + it('redacts under String() conversion', () => { + expect(String(srp)).toBe('[redacted]'); + }); + + it('redacts under template-literal interpolation', () => { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- We are intentionally exercising the redacting toString(). + const message = `srp is ${srp}`; + expect(message).toBe('srp is [redacted]'); + expect(message).not.toContain('ball'); + }); + }); +}); diff --git a/packages/wallet-cli/src/daemon/secrets.ts b/packages/wallet-cli/src/daemon/secrets.ts new file mode 100644 index 00000000000..44e3370c916 --- /dev/null +++ b/packages/wallet-cli/src/daemon/secrets.ts @@ -0,0 +1,135 @@ +import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; + +const REDACTED = '[redacted]'; + +const VALID_SRP_WORD_COUNTS: readonly number[] = [12, 15, 18, 21, 24]; + +const INSPECT_CUSTOM = Symbol.for('nodejs.util.inspect.custom'); + +const WORDLIST_SET: ReadonlySet = new Set(wordlist); + +/** + * Opaque wrapper around a wallet password. + * + * Constructed via {@link Password.from}, which validates the input. The + * underlying string is only reachable through {@link Password.unwrap}; every + * other path (`toString`, `JSON.stringify`, `util.inspect`, template-literal + * interpolation) yields `[redacted]`. This makes accidental logging produce a + * harmless placeholder instead of leaking the secret. + */ +export class Password { + readonly #value: string; + + // See .from() for why this is private. + // eslint-disable-next-line no-restricted-syntax + private constructor(value: string) { + this.#value = value; + } + + /** + * Wrap a non-empty string as a {@link Password}. + * + * Matches the `@metamask/keyring-controller` convention: any non-empty + * string is acceptable; minimum-length policy is left to the keyring. + * + * @param value - The raw password string. + * @returns A redacting {@link Password} wrapper. + * @throws If `value` is empty. + */ + static from(value: string): Password { + if (value.length === 0) { + throw new Error('Password must be a non-empty string'); + } + return new Password(value); + } + + /** + * Reveal the underlying password string. Call this only at trust boundaries + * (e.g. handing the value to the keyring or to a child-process env var). + * + * @returns The original password string. + */ + unwrap(): string { + return this.#value; + } + + toString(): string { + return REDACTED; + } + + toJSON(): string { + return REDACTED; + } + + [INSPECT_CUSTOM](): string { + return REDACTED; + } +} + +/** + * Opaque wrapper around a BIP-39 secret recovery phrase. + * + * Constructed via {@link Srp.from}, which validates the word count + * (12/15/18/21/24) and that every word is present in the BIP-39 English + * wordlist. The underlying string is only reachable through {@link Srp.unwrap}; + * every other path yields `[redacted]`. + */ +export class Srp { + readonly #value: string; + + // See .from() for why this is private. + // eslint-disable-next-line no-restricted-syntax + private constructor(value: string) { + this.#value = value; + } + + /** + * Validate and wrap a BIP-39 mnemonic phrase. + * + * The phrase is expected to be a single space-separated string. Catching + * malformed input here (rather than letting it reach + * `KeyringController:createNewVaultAndRestore`) produces a clearer error. + * + * @param value - The raw mnemonic string. + * @returns A redacting {@link Srp} wrapper. + * @throws If the word count is not one of 12/15/18/21/24, or if any word is + * not present in the BIP-39 English wordlist. + */ + static from(value: string): Srp { + const words = value.split(' '); + if (!VALID_SRP_WORD_COUNTS.includes(words.length)) { + throw new Error( + `Secret recovery phrase must be 12, 15, 18, 21, or 24 words (got ${words.length})`, + ); + } + for (const word of words) { + if (!WORDLIST_SET.has(word)) { + throw new Error( + 'Secret recovery phrase contains a word not in the BIP-39 English wordlist', + ); + } + } + return new Srp(value); + } + + /** + * Reveal the underlying mnemonic string. Call this only at trust boundaries. + * + * @returns The original mnemonic string. + */ + unwrap(): string { + return this.#value; + } + + toString(): string { + return REDACTED; + } + + toJSON(): string { + return REDACTED; + } + + [INSPECT_CUSTOM](): string { + return REDACTED; + } +} diff --git a/packages/wallet-cli/src/daemon/types.ts b/packages/wallet-cli/src/daemon/types.ts index eddedc8b336..d826a0f398a 100644 --- a/packages/wallet-cli/src/daemon/types.ts +++ b/packages/wallet-cli/src/daemon/types.ts @@ -1,5 +1,7 @@ import type { Json } from '@metamask/utils'; +import type { Password, Srp } from './secrets'; + /** * Sink for daemon diagnostic messages. A backgrounded daemon's stdio may be * discarded, so hosts supply a logger that writes somewhere durable. @@ -42,7 +44,7 @@ export type DaemonStatusInfo = { export type DaemonSpawnConfig = { dataDir: string; infuraProjectId: string; - password: string; - srp: string; + password: Password; + srp: Srp; packageRoot: string; }; diff --git a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts index a8a53498f35..92ef53362c6 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts @@ -1,3 +1,4 @@ +import { Password, Srp } from './secrets'; import { createWallet } from './wallet-factory'; // Unlike the unit test alongside it, this does NOT mock `@metamask/wallet`, so @@ -14,8 +15,8 @@ describe('createWallet (real Wallet, in-memory)', () => { it('constructs an unlocked wallet on first run and dispatches messenger actions', async () => { const { wallet, dispose } = await createWallet({ databasePath: ':memory:', - password: TEST_PASSWORD, - srp: TEST_SRP, + password: Password.from(TEST_PASSWORD), + srp: Srp.from(TEST_SRP), infuraProjectId: 'test-infura-id', log: () => undefined, }); diff --git a/packages/wallet-cli/src/daemon/wallet-factory.test.ts b/packages/wallet-cli/src/daemon/wallet-factory.test.ts index db1a24f9e0f..453c9eadfbd 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.test.ts @@ -12,6 +12,7 @@ import { join } from 'node:path'; import { KeyValueStore } from '../persistence/KeyValueStore'; import * as persistenceModule from '../persistence/persistence'; +import { Password, Srp } from './secrets'; import { createWallet } from './wallet-factory'; jest.mock('@metamask/wallet'); @@ -28,8 +29,8 @@ const SRP = 'test test test test test test test test test test test ball'; const CONFIG = { databasePath: ':memory:', - password: 'test-pass', - srp: SRP, + password: Password.from('test-pass'), + srp: Srp.from(SRP), infuraProjectId: 'test-infura-id', }; diff --git a/packages/wallet-cli/src/daemon/wallet-factory.ts b/packages/wallet-cli/src/daemon/wallet-factory.ts index e68eace3ddb..74ccc18efa4 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.ts @@ -16,14 +16,15 @@ import { rm } from 'node:fs/promises'; import { KeyValueStore } from '../persistence/KeyValueStore'; import { loadState, subscribeToChanges } from '../persistence/persistence'; +import type { Password, Srp } from './secrets'; import type { Logger } from './types'; const IN_MEMORY_DATABASE_PATH = ':memory:'; export type CreateWalletConfig = { databasePath: string; - password: string; - srp: string; + password: Password; + srp: Srp; infuraProjectId: string; log?: Logger; }; @@ -184,7 +185,7 @@ export async function createWallet({ } if (wasFirstRun) { - await importSecretRecoveryPhrase(wallet, password, srp); + await importSecretRecoveryPhrase(wallet, password.unwrap(), srp.unwrap()); } let disposePromise: Promise | undefined; diff --git a/yarn.lock b/yarn.lock index c1cd28ff517..1bb58f2b501 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9046,6 +9046,7 @@ __metadata: "@metamask/base-controller": "npm:^9.1.0" "@metamask/remote-feature-flag-controller": "npm:^4.2.2" "@metamask/rpc-errors": "npm:^7.0.2" + "@metamask/scure-bip39": "npm:^2.1.1" "@metamask/storage-service": "npm:^1.0.2" "@metamask/utils": "npm:^11.11.0" "@metamask/wallet": "npm:^7.0.1" From cc36b508196a1ee23547c3d226933af83cd78c78 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 1 Jul 2026 19:36:12 +0200 Subject: [PATCH 09/13] fix(wallet-cli): strengthen Srp.from validation and tidy CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add BIP-39 checksum validation via `validateMnemonic`; phrases with valid words but a wrong last word now fail fast with a clear error instead of propagating a malformed mnemonic to the keyring. - Normalize whitespace in `Srp.from` (`trim + split /\s+/`) so copy-pasted phrases with accidental spaces are accepted. - Replace fake 'test test...' SRP fixtures with the standard BIP-39 test vector ('abandon' × 11 + 'about') across all test files. - Remove duplicate `test-wallet-cli-e2e` CI job introduced by a rebase conflict (the pre-existing v3 job with the broader node matrix is kept). Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/lint-build-test.yml | 27 ---------- packages/wallet-cli/jest.config.js | 5 -- .../src/commands/daemon/start.test.ts | 2 +- .../src/daemon/daemon-entry.test.ts | 6 +-- .../src/daemon/daemon-spawn.test.ts | 2 +- .../wallet-cli/src/daemon/secrets.test.ts | 51 +++++++++++++++---- packages/wallet-cli/src/daemon/secrets.ts | 20 +++++--- .../daemon/wallet-factory-integration.test.ts | 2 +- .../src/daemon/wallet-factory.test.ts | 2 +- .../wallet-cli/tests/lifecycle.e2e.test.ts | 2 +- 10 files changed, 62 insertions(+), 57 deletions(-) diff --git a/.github/workflows/lint-build-test.yml b/.github/workflows/lint-build-test.yml index 33026991ecb..36039484255 100644 --- a/.github/workflows/lint-build-test.yml +++ b/.github/workflows/lint-build-test.yml @@ -267,30 +267,3 @@ jobs: echo "Working tree dirty at end of job" exit 1 fi - - # The wallet-cli daemon e2e spawns the BUILT `mm` CLI and the native - # better-sqlite3 addon as real child processes, so it needs its dependency - # subtree built first and cannot run in the per-package `test-*` matrix above. - test-wallet-cli-e2e: - name: Test wallet-cli daemon e2e (${{ matrix.node-version }}) - runs-on: ubuntu-latest - needs: prepare - strategy: - matrix: - node-version: [20.x, 24.x] - steps: - - name: Checkout and setup environment - uses: MetaMask/action-checkout-and-setup@v2 - with: - is-high-risk-environment: false - node-version: ${{ matrix.node-version }} - - name: Build wallet-cli and its dependencies - run: yarn workspaces foreach --topological-dev --recursive --from '@metamask/wallet-cli' run build - - run: yarn workspace @metamask/wallet-cli run test:e2e - - name: Require clean working directory - shell: bash - run: | - if ! git diff --exit-code; then - echo "Working tree dirty at end of job" - exit 1 - fi diff --git a/packages/wallet-cli/jest.config.js b/packages/wallet-cli/jest.config.js index 9239619d7f9..568fedaec47 100644 --- a/packages/wallet-cli/jest.config.js +++ b/packages/wallet-cli/jest.config.js @@ -29,11 +29,6 @@ module.exports = merge(baseConfig, { // and must not run in the fast unit suite. testPathIgnorePatterns: ['/node_modules/', '/tests/'], - // The subprocess e2e suite lives in `tests/` and has its own config - // (`jest.config.e2e.js`, run via `yarn test:e2e`); it spawns the built CLI - // and must not run in the fast unit suite. - testPathIgnorePatterns: ['/node_modules/', '/tests/'], - // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { diff --git a/packages/wallet-cli/src/commands/daemon/start.test.ts b/packages/wallet-cli/src/commands/daemon/start.test.ts index 196cd3eba77..b68ba1b210b 100644 --- a/packages/wallet-cli/src/commands/daemon/start.test.ts +++ b/packages/wallet-cli/src/commands/daemon/start.test.ts @@ -6,7 +6,7 @@ jest.mock('../../daemon/daemon-spawn'); const mockEnsureDaemon = jest.mocked(ensureDaemon); -const SRP = 'test test test test test test test test test test test ball'; +const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const FLAGS = ['--infura-project-id', 'key', '--password', 'pw', '--srp', SRP]; diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index 2a020b2893b..8aaf20eb4f5 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -90,7 +90,7 @@ describe('daemon-entry', () => { process.env.INFURA_PROJECT_ID = 'key'; process.env.MM_WALLET_PASSWORD = 'pass'; process.env.MM_WALLET_SRP = - 'test test test test test test test test test test test ball'; + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; process.exitCode = undefined; stderrSpy = jest .spyOn(process.stderr, 'write') @@ -196,7 +196,7 @@ describe('daemon-entry', () => { const passedConfig = mockCreateWallet.mock.calls[0][0]; expect(passedConfig.password.unwrap()).toBe('pass'); expect(passedConfig.srp.unwrap()).toBe( - 'test test test test test test test test test test test ball', + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about', ); expect(mockWriteFile).toHaveBeenCalledWith( '/tmp/daemon.pid', @@ -222,7 +222,7 @@ describe('daemon-entry', () => { const passedConfig = mockCreateWallet.mock.calls[0][0]; expect(passedConfig.password.unwrap()).toBe('pass'); expect(passedConfig.srp.unwrap()).toBe( - 'test test test test test test test test test test test ball', + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about', ); // ...but no longer linger in the long-lived daemon's environment. expect(process.env.MM_WALLET_PASSWORD).toBeUndefined(); diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts index c61c7f99fdf..36071cbf668 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts @@ -27,7 +27,7 @@ const mockGetDaemonPaths = jest.mocked(getDaemonPaths); // assert it is wired into the child's stdio and later closed in the parent. const LOG_FILE_DESCRIPTOR = 7; -const SRP = 'test test test test test test test test test test test ball'; +const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const CONFIG: DaemonSpawnConfig = { dataDir: '/tmp/data', diff --git a/packages/wallet-cli/src/daemon/secrets.test.ts b/packages/wallet-cli/src/daemon/secrets.test.ts index 7c363d65711..b54ea515aa2 100644 --- a/packages/wallet-cli/src/daemon/secrets.test.ts +++ b/packages/wallet-cli/src/daemon/secrets.test.ts @@ -3,10 +3,18 @@ import { inspect } from 'node:util'; import { Password, Srp } from './secrets'; const VALID_SRP_12 = - 'test test test test test test test test test test test ball'; + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const VALID_SRP_24 = - 'test test test test test test test test test test test test ' + - 'test test test test test test test test test test test ball'; + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon ' + + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon art'; + +const VALID_SRPS: Record = { + 12: VALID_SRP_12, + 15: 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon address', + 18: 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon agent', + 21: 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon admit', + 24: VALID_SRP_24, +}; describe('Password', () => { describe('from', () => { @@ -72,30 +80,53 @@ describe('Srp', () => { it.each([12, 15, 18, 21, 24])( 'accepts a %i-word mnemonic of valid words', (count) => { - const phrase = Array.from({ length: count - 1 }, () => 'test') - .concat('ball') - .join(' '); - expect(Srp.from(phrase)).toBeInstanceOf(Srp); + expect(Srp.from(VALID_SRPS[count])).toBeInstanceOf(Srp); }, ); it('throws when the word count is invalid', () => { - expect(() => Srp.from('test test test')).toThrow( + expect(() => Srp.from('abandon abandon abandon')).toThrow( /must be 12, 15, 18, 21, or 24 words \(got 3\)/u, ); }); it('throws when a word is not in the BIP-39 wordlist', () => { const phrase = - 'notabip39word test test test test test test test test test test ball'; + 'notabip39word abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; expect(() => Srp.from(phrase)).toThrow( 'Secret recovery phrase contains a word not in the BIP-39 English wordlist', ); }); + + it('throws when the phrase has an invalid checksum', () => { + // All valid BIP-39 words, correct count, but wrong last word → bad checksum + expect(() => + Srp.from( + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon above', + ), + ).toThrow('Secret recovery phrase has an invalid checksum'); + }); + + it('accepts a phrase with a trailing space', () => { + expect(Srp.from(`${VALID_SRP_12} `)).toBeInstanceOf(Srp); + }); + + it('accepts a phrase with leading whitespace', () => { + expect(Srp.from(` ${VALID_SRP_12}`)).toBeInstanceOf(Srp); + }); + + it('accepts a phrase with multiple spaces between words', () => { + const phrase = VALID_SRP_12.replace(' test ', ' test '); + expect(Srp.from(phrase)).toBeInstanceOf(Srp); + }); + + it('normalizes whitespace in the stored value', () => { + expect(Srp.from(` ${VALID_SRP_12} `).unwrap()).toBe(VALID_SRP_12); + }); }); describe('unwrap', () => { - it('returns the original phrase', () => { + it('returns the normalized phrase', () => { expect(Srp.from(VALID_SRP_12).unwrap()).toBe(VALID_SRP_12); expect(Srp.from(VALID_SRP_24).unwrap()).toBe(VALID_SRP_24); }); diff --git a/packages/wallet-cli/src/daemon/secrets.ts b/packages/wallet-cli/src/daemon/secrets.ts index 44e3370c916..d0f89864b26 100644 --- a/packages/wallet-cli/src/daemon/secrets.ts +++ b/packages/wallet-cli/src/daemon/secrets.ts @@ -1,3 +1,4 @@ +import { validateMnemonic } from '@metamask/scure-bip39'; import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; const REDACTED = '[redacted]'; @@ -86,22 +87,24 @@ export class Srp { /** * Validate and wrap a BIP-39 mnemonic phrase. * - * The phrase is expected to be a single space-separated string. Catching - * malformed input here (rather than letting it reach + * Whitespace is normalized (trimmed and collapsed) before validation so that + * copy-pasted phrases with accidental leading/trailing/extra spaces are + * accepted. Catching malformed input here (rather than letting it reach * `KeyringController:createNewVaultAndRestore`) produces a clearer error. * * @param value - The raw mnemonic string. - * @returns A redacting {@link Srp} wrapper. - * @throws If the word count is not one of 12/15/18/21/24, or if any word is - * not present in the BIP-39 English wordlist. + * @returns A redacting {@link Srp} wrapper containing the normalized phrase. + * @throws If the word count is not one of 12/15/18/21/24, if any word is + * not present in the BIP-39 English wordlist, or if the checksum is invalid. */ static from(value: string): Srp { - const words = value.split(' '); + const words = value.trim().split(/\s+/u); if (!VALID_SRP_WORD_COUNTS.includes(words.length)) { throw new Error( `Secret recovery phrase must be 12, 15, 18, 21, or 24 words (got ${words.length})`, ); } + const normalized = words.join(' '); for (const word of words) { if (!WORDLIST_SET.has(word)) { throw new Error( @@ -109,7 +112,10 @@ export class Srp { ); } } - return new Srp(value); + if (!validateMnemonic(normalized, wordlist)) { + throw new Error('Secret recovery phrase has an invalid checksum'); + } + return new Srp(normalized); } /** diff --git a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts index 92ef53362c6..f1446467283 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts @@ -8,7 +8,7 @@ import { createWallet } from './wallet-factory'; // fetches in `updateRemoteFeatureFlags`, and NetworkController's `init()` is // synchronous and does not call `lookupNetwork`. -const TEST_SRP = 'test test test test test test test test test test test ball'; +const TEST_SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const TEST_PASSWORD = 'testpass'; describe('createWallet (real Wallet, in-memory)', () => { diff --git a/packages/wallet-cli/src/daemon/wallet-factory.test.ts b/packages/wallet-cli/src/daemon/wallet-factory.test.ts index 453c9eadfbd..7e222d31cff 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.test.ts @@ -25,7 +25,7 @@ const mockRm = jest.mocked(rm); const createdTempDbPaths: string[] = []; -const SRP = 'test test test test test test test test test test test ball'; +const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const CONFIG = { databasePath: ':memory:', diff --git a/packages/wallet-cli/tests/lifecycle.e2e.test.ts b/packages/wallet-cli/tests/lifecycle.e2e.test.ts index 9db668dfc7c..1499ca7a581 100644 --- a/packages/wallet-cli/tests/lifecycle.e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.e2e.test.ts @@ -21,7 +21,7 @@ import { isProcessAlive, readPidFile } from '../src/daemon/utils'; // only action called here, `KeyringController:getState`, is local. // A valid 12-word BIP-39 mnemonic — the same fixtures the in-process e2e uses. -const TEST_SRP = 'test test test test test test test test test test test ball'; +const TEST_SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const TEST_PASSWORD = 'testpass'; // NetworkController requires a project ID but is never reached over the network // here, so any well-formed-looking value works. From 4c91d190511fc3e375452c9faf161c4bed77671b Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 1 Jul 2026 19:39:10 +0200 Subject: [PATCH 10/13] chore(wallet-cli): simplify changelog entry for Password/Srp types Co-Authored-By: Claude Sonnet 4.6 --- packages/wallet-cli/CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/wallet-cli/CHANGELOG.md b/packages/wallet-cli/CHANGELOG.md index 62e1475e683..b3d7c03dd23 100644 --- a/packages/wallet-cli/CHANGELOG.md +++ b/packages/wallet-cli/CHANGELOG.md @@ -20,7 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) -- Daemon password and secret recovery phrase are now wrapped in opaque `Password` and `Srp` classes that redact themselves under `util.inspect`, `JSON.stringify`, `toString`, and template-literal interpolation; the underlying string is reachable only via `unwrap()` at trust boundaries ([#8778](https://github.com/MetaMask/core/issues/8778)). - - `Srp.from` validates word count (12/15/18/21/24) and that every word is in the BIP-39 English wordlist, surfacing typos at the CLI boundary instead of producing a malformed mnemonic downstream. +- Wrap daemon password and SRP in opaque `Password` and `Srp` types that redact on logging; validated and unwrapped only at trust boundaries ([#8778](https://github.com/MetaMask/core/issues/8778)) [Unreleased]: https://github.com/MetaMask/core/ From 8ef358a91699980544b992d5f2468fa7aecdf1e7 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Wed, 1 Jul 2026 19:49:40 +0200 Subject: [PATCH 11/13] chore(wallet-cli): fix changelog PR link and format test files Co-Authored-By: Claude Sonnet 4.6 --- packages/wallet-cli/CHANGELOG.md | 2 +- packages/wallet-cli/src/commands/daemon/start.test.ts | 3 ++- packages/wallet-cli/src/daemon/daemon-spawn.test.ts | 3 ++- .../wallet-cli/src/daemon/wallet-factory-integration.test.ts | 3 ++- packages/wallet-cli/src/daemon/wallet-factory.test.ts | 3 ++- packages/wallet-cli/tests/lifecycle.e2e.test.ts | 3 ++- 6 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/wallet-cli/CHANGELOG.md b/packages/wallet-cli/CHANGELOG.md index b3d7c03dd23..e931224442a 100644 --- a/packages/wallet-cli/CHANGELOG.md +++ b/packages/wallet-cli/CHANGELOG.md @@ -20,6 +20,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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)) -- Wrap daemon password and SRP in opaque `Password` and `Srp` types that redact on logging; validated and unwrapped only at trust boundaries ([#8778](https://github.com/MetaMask/core/issues/8778)) +- Wrap daemon password and SRP in opaque `Password` and `Srp` types that redact on logging; validated and unwrapped only at trust boundaries ([#8863](https://github.com/MetaMask/core/pull/8863)) [Unreleased]: https://github.com/MetaMask/core/ diff --git a/packages/wallet-cli/src/commands/daemon/start.test.ts b/packages/wallet-cli/src/commands/daemon/start.test.ts index b68ba1b210b..f38a95813cd 100644 --- a/packages/wallet-cli/src/commands/daemon/start.test.ts +++ b/packages/wallet-cli/src/commands/daemon/start.test.ts @@ -6,7 +6,8 @@ jest.mock('../../daemon/daemon-spawn'); const mockEnsureDaemon = jest.mocked(ensureDaemon); -const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +const SRP = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const FLAGS = ['--infura-project-id', 'key', '--password', 'pw', '--srp', SRP]; diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts index 36071cbf668..20f603503a8 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts @@ -27,7 +27,8 @@ const mockGetDaemonPaths = jest.mocked(getDaemonPaths); // assert it is wired into the child's stdio and later closed in the parent. const LOG_FILE_DESCRIPTOR = 7; -const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +const SRP = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const CONFIG: DaemonSpawnConfig = { dataDir: '/tmp/data', diff --git a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts index f1446467283..10ae9a90f6a 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory-integration.test.ts @@ -8,7 +8,8 @@ import { createWallet } from './wallet-factory'; // fetches in `updateRemoteFeatureFlags`, and NetworkController's `init()` is // synchronous and does not call `lookupNetwork`. -const TEST_SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +const TEST_SRP = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const TEST_PASSWORD = 'testpass'; describe('createWallet (real Wallet, in-memory)', () => { diff --git a/packages/wallet-cli/src/daemon/wallet-factory.test.ts b/packages/wallet-cli/src/daemon/wallet-factory.test.ts index 7e222d31cff..7d5444f0ad9 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.test.ts @@ -25,7 +25,8 @@ const mockRm = jest.mocked(rm); const createdTempDbPaths: string[] = []; -const SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +const SRP = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const CONFIG = { databasePath: ':memory:', diff --git a/packages/wallet-cli/tests/lifecycle.e2e.test.ts b/packages/wallet-cli/tests/lifecycle.e2e.test.ts index 1499ca7a581..bbd25c44120 100644 --- a/packages/wallet-cli/tests/lifecycle.e2e.test.ts +++ b/packages/wallet-cli/tests/lifecycle.e2e.test.ts @@ -21,7 +21,8 @@ import { isProcessAlive, readPidFile } from '../src/daemon/utils'; // only action called here, `KeyringController:getState`, is local. // A valid 12-word BIP-39 mnemonic — the same fixtures the in-process e2e uses. -const TEST_SRP = 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +const TEST_SRP = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; const TEST_PASSWORD = 'testpass'; // NetworkController requires a project ID but is never reached over the network // here, so any well-formed-looking value works. From 3f0012f73ee3062b71845e31fc17178438db8273 Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Mon, 13 Jul 2026 15:10:34 +0200 Subject: [PATCH 12/13] fix(wallet-cli): scrub env secrets before validation to prevent leakage on startup failure Co-Authored-By: Claude Sonnet 4.6 --- packages/wallet-cli/src/daemon/daemon-entry.test.ts | 10 ++++++++++ packages/wallet-cli/src/daemon/daemon-entry.ts | 11 +++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index 8aaf20eb4f5..403defad277 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -229,6 +229,16 @@ describe('daemon-entry', () => { expect(process.env.MM_WALLET_SRP).toBeUndefined(); }); + it('scrubs wallet secrets even when Srp.from throws on an invalid mnemonic', async () => { + process.env.MM_WALLET_SRP = 'not a valid mnemonic at all'; + + await importDaemonEntry(); + + expect(process.exitCode).toBe(1); + expect(process.env.MM_WALLET_PASSWORD).toBeUndefined(); + expect(process.env.MM_WALLET_SRP).toBeUndefined(); + }); + it('uses MM_DAEMON_SOCKET_PATH override when set', async () => { process.env.MM_DAEMON_SOCKET_PATH = '/custom/sock'; diff --git a/packages/wallet-cli/src/daemon/daemon-entry.ts b/packages/wallet-cli/src/daemon/daemon-entry.ts index 2d498e2db8a..b80a98474ba 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.ts @@ -34,21 +34,20 @@ async function main(): Promise { if (!passwordRaw) { throw new Error('MM_WALLET_PASSWORD environment variable is required'); } - const password = Password.from(passwordRaw); const srpRaw = process.env.MM_WALLET_SRP; if (!srpRaw) { throw new Error('MM_WALLET_SRP environment variable is required'); } - const srp = Srp.from(srpRaw); - // Scrub the wallet secrets from the environment now they are captured. The - // daemon is long-lived and dispatches arbitrary messenger actions over its - // socket, so leaving the SRP/password in `process.env` for its whole lifetime - // needlessly widens their exposure to any in-process code. + // Scrub before validation so a throw from Password.from / Srp.from (bad + // value) does not leave the raw secrets in the long-lived daemon's env. delete process.env.MM_WALLET_PASSWORD; delete process.env.MM_WALLET_SRP; + const password = Password.from(passwordRaw); + const srp = Srp.from(srpRaw); + await ensureOwnerOnlyDirectory(dataDir); const { From ef98685342f1a60754c82ad8e13d156dd7f0b9ca Mon Sep 17 00:00:00 2001 From: Dimitris Marlagkoutsos Date: Tue, 14 Jul 2026 18:47:10 +0200 Subject: [PATCH 13/13] fix(wallet-cli): validate BIP-39 wordlist before computing normalized mnemonic Co-Authored-By: Claude Sonnet 4.6 --- packages/wallet-cli/src/daemon/secrets.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/wallet-cli/src/daemon/secrets.ts b/packages/wallet-cli/src/daemon/secrets.ts index d0f89864b26..af64973c49a 100644 --- a/packages/wallet-cli/src/daemon/secrets.ts +++ b/packages/wallet-cli/src/daemon/secrets.ts @@ -104,7 +104,6 @@ export class Srp { `Secret recovery phrase must be 12, 15, 18, 21, or 24 words (got ${words.length})`, ); } - const normalized = words.join(' '); for (const word of words) { if (!WORDLIST_SET.has(word)) { throw new Error( @@ -112,6 +111,8 @@ export class Srp { ); } } + + const normalized = words.join(' '); if (!validateMnemonic(normalized, wordlist)) { throw new Error('Secret recovery phrase has an invalid checksum'); }