Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/wallet-cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- 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))
- 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/
1 change: 1 addition & 0 deletions packages/wallet-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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/superstruct": "^3.1.0",
"@metamask/utils": "^11.11.0",
Expand Down
12 changes: 4 additions & 8 deletions packages/wallet-cli/src/commands/daemon/start.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,10 @@ jest.mock('../../daemon/daemon-spawn');

const mockEnsureDaemon = jest.mocked(ensureDaemon);

const FLAGS = [
'--infura-project-id',
'key',
'--password',
'pw',
'--srp',
'phrase',
];
const SRP =
'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about';

const FLAGS = ['--infura-project-id', 'key', '--password', 'pw', '--srp', SRP];

describe('daemon start', () => {
it('reports the socket path on a fresh start', async () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/wallet-cli/src/commands/daemon/start.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -33,7 +34,8 @@ export default class DaemonStart extends Command {
public async run(): Promise<void> {
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,
Expand Down
46 changes: 32 additions & 14 deletions packages/wallet-cli/src/daemon/daemon-entry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,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')
Expand Down Expand Up @@ -184,13 +184,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(
'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about',
);
expect(mockWriteFile).toHaveBeenCalledWith(
'/tmp/daemon.pid',
expect.stringMatching(new RegExp(`^${process.pid}\\n\\d+\\n$`, 'u')),
Expand All @@ -210,18 +218,28 @@ 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(
'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();
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';

Expand Down
18 changes: 10 additions & 8 deletions packages/wallet-cli/src/daemon/daemon-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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 { defineHandler } from './types';
import type {
DaemonStatusInfo,
Expand Down Expand Up @@ -57,23 +58,24 @@ async function main(): Promise<void> {
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 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');
}

// 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 {
Expand Down
11 changes: 7 additions & 4 deletions packages/wallet-cli/src/daemon/daemon-spawn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -26,11 +27,14 @@ 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 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',
};

Expand Down Expand Up @@ -146,8 +150,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,
}),
}),
);
Expand Down
4 changes: 2 additions & 2 deletions packages/wallet-cli/src/daemon/daemon-spawn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
171 changes: 171 additions & 0 deletions packages/wallet-cli/src/daemon/secrets.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
import { inspect } from 'node:util';

import { Password, Srp } from './secrets';

const VALID_SRP_12 =
'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about';
const VALID_SRP_24 =
'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<number, string> = {
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', () => {
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) => {
expect(Srp.from(VALID_SRPS[count])).toBeInstanceOf(Srp);
},
);

it('throws when the word count is invalid', () => {
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 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 normalized 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');
});
});
});
Loading