Skip to content

refactor(wallet-cli): Wrap daemon password/SRP in opaque Password/Srp types - #8863

Merged
sirtimid merged 14 commits into
mainfrom
sirtimid/wallet-cli-branded-password-srp
Jul 14, 2026
Merged

refactor(wallet-cli): Wrap daemon password/SRP in opaque Password/Srp types#8863
sirtimid merged 14 commits into
mainfrom
sirtimid/wallet-cli-branded-password-srp

Conversation

@sirtimid

@sirtimid sirtimid commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • Introduce `Password` and `Srp` opaque class wrappers in `packages/wallet-cli/src/daemon/secrets.ts`. Both redact themselves under `util.inspect`, `JSON.stringify`, `toString`, and template-literal interpolation; the raw string is reachable only via `unwrap()`.
  • Thread the new types through `DaemonSpawnConfig`, `daemon start` flag parsing, `MM_WALLET_PASSWORD` / `MM_WALLET_SRP` env reads in `daemon-entry`, and `createWallet`. `unwrap()` is called only at trust boundaries.
  • `Srp.from` validates word count (12/15/18/21/24), every word against the BIP-39 English wordlist, and the BIP-39 checksum via `validateMnemonic`, so typos fail fast at the CLI boundary. Input whitespace is normalized (trimmed and collapsed) so copy-pasted phrases with accidental spaces are accepted.
  • `Password.from` matches the `@metamask/keyring-controller` convention: non-empty only.
  • `@metamask/wallet`'s `importSecretRecoveryPhrase` signature stays on `string` — branding is scoped to wallet-cli.

Closes #8778.

Test plan

  • `yarn workspace @metamask/wallet-cli run test` — 100% coverage on all touched files
  • `secrets.test.ts` covers validation (word count, wordlist, checksum), whitespace normalization, redaction, and unwrap round-trip
  • All test fixtures updated to use the standard BIP-39 test vector (`abandon` × 11 + `about`) instead of the fake non-checksummed phrase

🤖 Generated with Claude Code


Note

Medium Risk
Touches wallet password and SRP handling at daemon startup and spawn; changes are defensive (redaction, earlier env scrub, stricter SRP validation) but incorrect validation could block legitimate mnemonics or affect startup paths.

Overview
Introduces opaque Password and Srp wrappers in secrets.ts so secrets show as [redacted] in logs, JSON.stringify, and util.inspect, with raw values exposed only via unwrap() at trust boundaries (child-process env, importSecretRecoveryPhrase).

Those types replace plain strings through daemon start, ensureDaemon, daemon-entry, and createWallet. Srp.from validates BIP-39 word count, English wordlist, and checksum (via @metamask/scure-bip39); Password.from requires a non-empty string.

daemon-entry now deletes MM_WALLET_PASSWORD / MM_WALLET_SRP from the environment before wrapping/validation so a bad mnemonic does not leave raw secrets in the long-lived daemon’s env. Tests and fixtures switch to the standard abandon×11 + about mnemonic.

Reviewed by Cursor Bugbot for commit ef98685. Bugbot is set up for automated code reviews on this repo. Configure here.

@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-branded-password-srp branch from dcd72a7 to 2eb8cdb Compare July 1, 2026 12:56
@sirtimid
sirtimid changed the base branch from rekm/wallet-cli to sirtimid/wallet-cli-daemon-e2e July 1, 2026 12:56
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-daemon-e2e branch 2 times, most recently from 9b0489c to 56d3cf9 Compare July 1, 2026 14:02
Base automatically changed from sirtimid/wallet-cli-daemon-e2e to main July 1, 2026 14:11
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-branded-password-srp branch from 2eb8cdb to 93c963e Compare July 1, 2026 17:14
@sirtimid
sirtimid marked this pull request as ready for review July 1, 2026 17:50
@sirtimid
sirtimid requested review from a team as code owners July 1, 2026 17:50
@sirtimid
sirtimid temporarily deployed to default-branch July 1, 2026 17:50 — with GitHub Actions Inactive
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-branded-password-srp branch from 1fabfeb to 3e28be0 Compare July 13, 2026 12:37
Comment thread packages/wallet-cli/src/daemon/daemon-entry.ts Outdated
sirtimid and others added 12 commits July 13, 2026 16:49
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
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: ['<rootDir>/tests']` instead of
  `**/*.daemon-e2e.test.ts`
- jest.config.js: ignore `<rootDir>/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) <noreply@anthropic.com>
… 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) <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ge on startup failure

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sirtimid
sirtimid force-pushed the sirtimid/wallet-cli-branded-password-srp branch from 015a9d1 to 3f0012f Compare July 13, 2026 14:49
rekmarks
rekmarks previously approved these changes Jul 14, 2026

@rekmarks rekmarks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one nit

Comment on lines +107 to +117
const normalized = words.join(' ');
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',
);
}
}
if (!validateMnemonic(normalized, wordlist)) {
throw new Error('Secret recovery phrase has an invalid checksum');
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
const normalized = words.join(' ');
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',
);
}
}
if (!validateMnemonic(normalized, wordlist)) {
throw new Error('Secret recovery phrase has an invalid checksum');
}
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',
);
}
}
const normalized = words.join(' ');
if (!validateMnemonic(normalized, wordlist)) {
throw new Error('Secret recovery phrase has an invalid checksum');
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ef98685

… mnemonic

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sirtimid
sirtimid enabled auto-merge July 14, 2026 16:48
@sirtimid
sirtimid requested a review from rekmarks July 14, 2026 16:48
@sirtimid
sirtimid added this pull request to the merge queue Jul 14, 2026
Merged via the queue into main with commit 05f013a Jul 14, 2026
421 checks passed
@sirtimid
sirtimid deleted the sirtimid/wallet-cli-branded-password-srp branch July 14, 2026 16:55
sirtimid added a commit that referenced this pull request Jul 14, 2026
Reconcile the auto-unlock (optional `--password`) work on this branch with
main's opaque `Password`/`Srp` daemon-secret types (#8863) and dispatch-time
RPC param validation (#8846):

- `DaemonSpawnConfig`/`CreateWalletConfig` password is now `password?: Password`
  (optional) with `srp: Srp`.
- daemon-entry wraps the env password only when present
  (`passwordRaw ? Password.from(passwordRaw) : undefined`); empty/absent stays
  "start locked".
- daemon-spawn unwraps `Password`/`Srp` at the child-env boundary while keeping
  the strip-inherited-password logic for the no-password case.
- wallet-factory drops the now-redundant `blankToUndefined` (opaque `Password`
  is validated non-empty at construction) and unwraps at the keyring boundary.
- start command builds `password` as `Password | undefined` so `--password`
  stays optional.
- Tests updated to opaque types; dropped the empty-string-password cases that
  can no longer reach `createWallet`. Old `wallet-factory-integration.test.ts`
  removed in favour of this branch's renamed `wallet-factory.integration.test.ts`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wallet-cli: Branded Password / Srp opaque types

2 participants