sneak/AutistMask
1
0
Fork 0
Code Issues 42 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

fix: enforce the base58 checksum and reject non-master extended keys (closes #210) #232

Merged
clawbot merged 1 commits from fix/issue-210-xprv-validation into next 2026-08-11 15:31:51 +02:00
Conversation 1 Commits 1 Files Changed 5 +238 -30
clawbot commented 2026-08-11 15:15:50 +02:00
Collaborator
Copy Link

Closes #210 — both defects filed there. Each one produced the same user-visible disaster: an apparently successful import of a wallet that is not the user's, with no error.

Defect 1 — the base58 checksum was not enforced

HDNodeWallet.fromExtendedKey skips checksum verification whenever the decoded payload is the usual 82 bytes, which is exactly the case the checksum exists to catch. isValidXprv was a thin wrapper over it, so a mistyped key parsed into a different wallet and imported cleanly.

Every extended key entering the app now goes through one helper, parseExtendedKey, which parses with ethers and then requires node.extendedKey === key — ethers computes the checksum when it serializes, so the round trip reproduces a well-formed key byte for byte and any altered character shows up as a mismatch. The round trip also rejects non-canonical encodings of an otherwise valid payload, but it is not strictly stronger than a bare checksum comparison in every direction: HDNodeVoidWallet.extendedKey hardcodes the mainnet version bytes 0x0488B21E, so a tpub (testnet version bytes, valid checksum) that the old code parsed and returned the CORRECT address for is now rejected. That case is unreachable in this app, and rejecting is the safe direction for an Ethereum wallet. It is not a testnet regression either — tprv, yprv and zprv were already rejected before this change. No new crypto primitive is introduced, so the Crypto Policy needs no exception.

isValidXprv, hdWalletFromXprv, getSignerForAddress and deriveAddressFromXpub all sit on it; fromExtendedKey now has exactly one call site in the codebase.

Defect 2 — non-master keys derived beneath themselves

hdWalletFromXprv and getSignerForAddress derived the RELATIVE path 44'/60'/0'/0. That is the BIP-44 Ethereum account path only from a depth-0 master key. Handed an account-level (depth-3) xprv, they derived m/44'/60'/0'/44'/60'/0'/0 beneath it. Measured for the vector phrase: the account-level key produced 0xbBBAc7de23fF320B7E52a2385A41bFB6eB99D78A where the user's first address is 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266.

Decision: reject depth > 0. The serialized extended-key format carries depth, parent fingerprint and child index — it does not carry the path the key sits at. A depth-3 key may be m/44'/60'/0', or m/44'/0'/5', or anything else; nothing in the key says which. So there is no way to know which path components remain to be derived without asking the user, and guessing is the exact class of silent-wrong-wallet failure this issue is about. Accepting arbitrary-depth keys correctly means a user-supplied derivation path, which is a feature, not this bug fix.

Both call sites now derive the absolute BIP44_ETH_PATH (m/44'/60'/0'/0) from a key checked to be at depth 0, which also removes the duplicated relative-path literal that was the root cause. The import screen reports the two cases separately, because "check it for a typo" is the wrong advice for a key the user copied correctly:

  • not well-formed: "That extended private key is not valid. Please check it and try again."
  • well-formed but not a master key: "That is an account-level or child key, which cannot be imported. Please paste the master extended private key for the wallet."

The xprv form's help text now states the same up front, rather than only on failure. Both messages are full sentences per the README Language & Labeling rules.

Does the xpub path share the holes?

  • Checksum: yes. There is no isValidXpub in this codebase; the xpub entry point is deriveAddressFromXpub, used by the import flow's address scan. It called fromExtendedKey directly and had the identical hole — 101 of the single-character typos of a valid xpub were accepted and returned addresses from a different tree. Fixed by the same helper; it now throws instead.
  • Depth: no, and it must not. deriveAddressFromXpub is called with the account-level (depth-4) xpub this app derives and stores. A depth check there would reject every legitimate wallet. The depth rule belongs only to the xprv import, where the app is the one deriving the account path.

Failing first

New and unskipped tests run against the unmodified src/ (git stash push -- src/, make test) — 10 failures, all of them the new assertions:

  isValidXprv > rejects an extended key with a one-character typo

    expect(received).toBe(expected) // Object.is equality
    Expected: false
    Received: true

  isValidXprv > no single-character typo anywhere in the key is accepted

    - Expected  -   1
    + Received  + 199
    - Array []
    + Array [
    +   "xprv9s21ZrQH143K3QTDL4LXw227HEK3wJUD2nW2nRk4stbPy6cq3jPPqji...",
    ...

  isValidXprv > a typo never yields a wallet, let alone a different one

    - Expected  -   1
    + Received  + 199
    - Array []
    + Array [
    +   "0x4cc7c495E1D425388627AA008b58e60C823bB651",
    +   "0xa9F9cE054b89e586015E8f3bBB65E969677ad550",
    ...

  extended key depth > hdWalletFromXprv rejects an account-level key

    expect(received).toThrow(expected)
    Expected pattern: /master/i
    Received function did not throw

  extended key depth > getSignerForAddress rejects an account-level key

    expect(received).toThrow(expected)
    Expected pattern: /master/i
    Received function did not throw

  deriveAddressFromXpub checksum enforcement > no single-character typo anywhere in an xpub is accepted

    - Expected  -   1
    + Received  + 101
    - Array []
    + Array [
    +   "0xBE7Cec2aDA9845C875365C1e9e06DC68010618f8",
    ...

Tests:       10 failed, 248 passed, 258 total

The four depth-predicate failures not quoted above (a master key is a master key, an account-level key is not a master key, a derived xpub is not a master key, a mistyped key is not a master key either) all fail as wallet.isMasterExtendedKey is not a function against the old code.

So: 199 single-character typos of the BIP-32 vector 1 master key imported as wallets before this change, and 101 typos of a valid xpub returned addresses. After: 0 and 0.

Coverage added

  • The skipped rejects an extended key with a one-character typo is unskipped and passes.
  • Sweeps over EVERY position of the key, not just the tail — four base58 substitutions per position — for both the xprv (isValidXprv and hdWalletFromXprv) and the xpub (deriveAddressFromXpub).
  • Depth-0 master key still accepted and still derives the published vector addresses; depth-3 account-level key rejected by isMasterExtendedKey, hdWalletFromXprv and getSignerForAddress, while isValidXprv still reports it well-formed so it is provably the depth check doing the rejecting and not an encoding error.

The #159 known-answer vectors and the key derivation cost block in tests/vault.test.js are untouched.

Verification

make check green on this branch after rebasing onto current next: 11 test suites, 274 tests passed, 0 skipped, prettier clean, exit 0.

Closes https://git.eeqj.de/sneak/AutistMask/issues/210 — both defects filed there. Each one produced the same user-visible disaster: an apparently successful import of a wallet that is not the user's, with no error. ## Defect 1 — the base58 checksum was not enforced `HDNodeWallet.fromExtendedKey` skips checksum verification whenever the decoded payload is the usual 82 bytes, which is exactly the case the checksum exists to catch. `isValidXprv` was a thin wrapper over it, so a mistyped key parsed into a different wallet and imported cleanly. Every extended key entering the app now goes through one helper, `parseExtendedKey`, which parses with ethers and then requires `node.extendedKey === key` — ethers computes the checksum when it serializes, so the round trip reproduces a well-formed key byte for byte and any altered character shows up as a mismatch. The round trip also rejects non-canonical encodings of an otherwise valid payload, but it is not strictly stronger than a bare checksum comparison in every direction: `HDNodeVoidWallet.extendedKey` hardcodes the mainnet version bytes `0x0488B21E`, so a `tpub` (testnet version bytes, valid checksum) that the old code parsed and returned the CORRECT address for is now rejected. That case is unreachable in this app, and rejecting is the safe direction for an Ethereum wallet. It is not a testnet regression either — `tprv`, `yprv` and `zprv` were already rejected before this change. No new crypto primitive is introduced, so the Crypto Policy needs no exception. `isValidXprv`, `hdWalletFromXprv`, `getSignerForAddress` and `deriveAddressFromXpub` all sit on it; `fromExtendedKey` now has exactly one call site in the codebase. ## Defect 2 — non-master keys derived beneath themselves `hdWalletFromXprv` and `getSignerForAddress` derived the RELATIVE path `44'/60'/0'/0`. That is the BIP-44 Ethereum account path only from a depth-0 master key. Handed an account-level (depth-3) xprv, they derived `m/44'/60'/0'/44'/60'/0'/0` beneath it. Measured for the vector phrase: the account-level key produced `0xbBBAc7de23fF320B7E52a2385A41bFB6eB99D78A` where the user's first address is `0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266`. **Decision: reject depth > 0.** The serialized extended-key format carries depth, parent fingerprint and child index — it does not carry the path the key sits at. A depth-3 key may be `m/44'/60'/0'`, or `m/44'/0'/5'`, or anything else; nothing in the key says which. So there is no way to know which path components remain to be derived without asking the user, and guessing is the exact class of silent-wrong-wallet failure this issue is about. Accepting arbitrary-depth keys correctly means a user-supplied derivation path, which is a feature, not this bug fix. Both call sites now derive the absolute `BIP44_ETH_PATH` (`m/44'/60'/0'/0`) from a key checked to be at depth 0, which also removes the duplicated relative-path literal that was the root cause. The import screen reports the two cases separately, because "check it for a typo" is the wrong advice for a key the user copied correctly: - not well-formed: "That extended private key is not valid. Please check it and try again." - well-formed but not a master key: "That is an account-level or child key, which cannot be imported. Please paste the master extended private key for the wallet." The xprv form's help text now states the same up front, rather than only on failure. Both messages are full sentences per the README Language & Labeling rules. ## Does the xpub path share the holes? - **Checksum: yes.** There is no `isValidXpub` in this codebase; the xpub entry point is `deriveAddressFromXpub`, used by the import flow's address scan. It called `fromExtendedKey` directly and had the identical hole — 101 of the single-character typos of a valid xpub were accepted and returned addresses from a different tree. Fixed by the same helper; it now throws instead. - **Depth: no, and it must not.** `deriveAddressFromXpub` is called with the account-level (depth-4) xpub this app derives and stores. A depth check there would reject every legitimate wallet. The depth rule belongs only to the xprv import, where the app is the one deriving the account path. ## Failing first New and unskipped tests run against the unmodified `src/` (`git stash push -- src/`, `make test`) — 10 failures, all of them the new assertions: ``` isValidXprv > rejects an extended key with a one-character typo expect(received).toBe(expected) // Object.is equality Expected: false Received: true isValidXprv > no single-character typo anywhere in the key is accepted - Expected - 1 + Received + 199 - Array [] + Array [ + "xprv9s21ZrQH143K3QTDL4LXw227HEK3wJUD2nW2nRk4stbPy6cq3jPPqji...", ... isValidXprv > a typo never yields a wallet, let alone a different one - Expected - 1 + Received + 199 - Array [] + Array [ + "0x4cc7c495E1D425388627AA008b58e60C823bB651", + "0xa9F9cE054b89e586015E8f3bBB65E969677ad550", ... extended key depth > hdWalletFromXprv rejects an account-level key expect(received).toThrow(expected) Expected pattern: /master/i Received function did not throw extended key depth > getSignerForAddress rejects an account-level key expect(received).toThrow(expected) Expected pattern: /master/i Received function did not throw deriveAddressFromXpub checksum enforcement > no single-character typo anywhere in an xpub is accepted - Expected - 1 + Received + 101 - Array [] + Array [ + "0xBE7Cec2aDA9845C875365C1e9e06DC68010618f8", ... Tests: 10 failed, 248 passed, 258 total ``` The four depth-predicate failures not quoted above (`a master key is a master key`, `an account-level key is not a master key`, `a derived xpub is not a master key`, `a mistyped key is not a master key either`) all fail as `wallet.isMasterExtendedKey is not a function` against the old code. So: 199 single-character typos of the BIP-32 vector 1 master key imported as wallets before this change, and 101 typos of a valid xpub returned addresses. After: 0 and 0. ## Coverage added - The skipped `rejects an extended key with a one-character typo` is unskipped and passes. - Sweeps over EVERY position of the key, not just the tail — four base58 substitutions per position — for both the xprv (`isValidXprv` and `hdWalletFromXprv`) and the xpub (`deriveAddressFromXpub`). - Depth-0 master key still accepted and still derives the published vector addresses; depth-3 account-level key rejected by `isMasterExtendedKey`, `hdWalletFromXprv` and `getSignerForAddress`, while `isValidXprv` still reports it well-formed so it is provably the depth check doing the rejecting and not an encoding error. The https://git.eeqj.de/sneak/AutistMask/issues/159 known-answer vectors and the `key derivation cost` block in `tests/vault.test.js` are untouched. ## Verification `make check` green on this branch after rebasing onto current `next`: 11 test suites, 274 tests passed, 0 skipped, prettier clean, exit 0.
clawbot added the needs-review label 2026-08-11 15:15:50 +02:00
clawbot added 1 commit 2026-08-11 15:15:51 +02:00
fix: enforce the base58 checksum and reject non-master extended keys (closes #210)
Some checks failed
check / check (push) Has been cancelled
Details
1fba21cd16 
Two ways an xprv import could silently produce a wallet that is not the
user's, with no error shown.

Checksum: ethers' HDNodeWallet.fromExtendedKey skips base58 checksum
verification when the decoded payload is the usual 82 bytes, which is
exactly the case the checksum exists to catch. Every extended key now
goes through parseExtendedKey, which re-encodes the parsed node and
requires the round trip to reproduce the input exactly. Over every
single-character substitution of the BIP-32 vector 1 master key, 199
parsed before and 0 parse now; the xpub path shared the hole (101
accepted typos) and is fixed by the same helper.

Depth: hdWalletFromXprv and getSignerForAddress derived the relative
path 44'/60'/0'/0, which is the BIP-44 Ethereum account path only for a
depth-0 master key. Under an account-level (depth-3) key it derived
m/44'/60'/0'/44'/60'/0'/0 instead of refusing. Both now derive the
absolute BIP44_ETH_PATH from a key checked to be at depth 0. The
extended-key format carries depth, parent fingerprint and child index
but not the path a key sits at, so a non-master key cannot be placed in
the tree without guessing; it is rejected, and the import screen says
an account-level or child key is not supported.

Import errors are full sentences per the README Language & Labeling
rules, and the skipped one-character-typo test is unskipped.
clawbot self-assigned this 2026-08-11 15:16:03 +02:00
clawbot commented 2026-08-11 15:27:59 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rebase. The change itself is clean; the only blocker is the base.

Conflicts with next. The branch is based on f455b0a; next is now 12acf4d, which inserted a #179 entry at the same point in TODO.md "Completed Steps". git rebase origin/next conflicts in TODO.md only — all five source files apply cleanly. Acceptable: rebase, retaining both entries.

Anomaly, not a defect. "Strictly stronger than a bare checksum comparison" is overstated in one direction: a tpub (version 0x043587cf, valid checksum) parsed under the old code and returned the correct address, and the round trip now rejects it, because HDNodeVoidWallet.extendedKey hardcodes the mainnet 0x0488B21E. Unreachable here — there is no xpub import UI, and deriveAddressFromXpub only ever receives the app's own stored xpub — and rejecting is the safe direction for an Ethereum wallet, so no rework is required. tprv/yprv/zprv were already rejected by ethers before this change.

Probed independently on 1fba21c, all passing: 6327-mutant sweeps (every position, all 57 base58 alternates, against the PR's 4-character/199) — xprv 2784 accepted before / 0 now, xpub 1511 / 0, with 0 disagreements against an independent base58check verifier, so the round trip is equivalent to checksum verification across the entire single-substitution space; 400 random seeds x {xprv, xpub} x {master, derived} plus 10 path shapes including m/2147483647'/0/2147483647' rejected 0 valid keys; the absolute m/44'/60'/0'/0 and the old relative 44'/60'/0'/0 produce a byte-identical stored xpub and identical addresses at indices 0, 1, 2, 5 from a depth-0 master; non-canonical 1-padded keys accepted before are rejected now; whitespace, empty, non-string, and short/long valid-checksum payloads are rejected both before and after. make check on my own clone: 258 tests, 0 skipped, prettier clean, exit 0, 4.5s — not a cache hit.

Noted, not failing: the two new error strings are covered by no test, so they are verified by reading only; deriveAddressFromXpub's callers (src/shared/balances.js:235, src/popup/views/home.js:296) do not catch, though it could already throw before and only ever receives app-generated xpubs.

Checked and clean: single commit titled (closes #210), base next, authored clawbot, no attribution trailers, no Claude or Anthropic references, no forbidden crypto strings in the new source (the Crypto Policy claim holds), fromExtendedKey reduced to one call site in src/, the previously skipped one-character-typo test unskipped and passing, and the #159 vectors and tests/vault.test.js untouched.

FAIL — `needs-rebase`. The change itself is clean; the only blocker is the base. **Conflicts with `next`.** The branch is based on `f455b0a`; `next` is now `12acf4d`, which inserted a [#179](https://git.eeqj.de/sneak/AutistMask/issues/179) entry at the same point in `TODO.md` "Completed Steps". `git rebase origin/next` conflicts in `TODO.md` only — all five source files apply cleanly. Acceptable: rebase, retaining both entries. **Anomaly, not a defect.** "Strictly stronger than a bare checksum comparison" is overstated in one direction: a `tpub` (version `0x043587cf`, valid checksum) parsed under the old code and returned the correct address, and the round trip now rejects it, because `HDNodeVoidWallet.extendedKey` hardcodes the mainnet `0x0488B21E`. Unreachable here — there is no xpub import UI, and `deriveAddressFromXpub` only ever receives the app's own stored xpub — and rejecting is the safe direction for an Ethereum wallet, so no rework is required. `tprv`/`yprv`/`zprv` were already rejected by ethers before this change. Probed independently on `1fba21c`, all passing: 6327-mutant sweeps (every position, all 57 base58 alternates, against the PR's 4-character/199) — xprv 2784 accepted before / 0 now, xpub 1511 / 0, with 0 disagreements against an independent base58check verifier, so the round trip is equivalent to checksum verification across the entire single-substitution space; 400 random seeds x {xprv, xpub} x {master, derived} plus 10 path shapes including `m/2147483647'/0/2147483647'` rejected 0 valid keys; the absolute `m/44'/60'/0'/0` and the old relative `44'/60'/0'/0` produce a byte-identical stored xpub and identical addresses at indices 0, 1, 2, 5 from a depth-0 master; non-canonical `1`-padded keys accepted before are rejected now; whitespace, empty, non-string, and short/long valid-checksum payloads are rejected both before and after. `make check` on my own clone: 258 tests, 0 skipped, prettier clean, exit 0, 4.5s — not a cache hit. Noted, not failing: the two new error strings are covered by no test, so they are verified by reading only; `deriveAddressFromXpub`'s callers (`src/shared/balances.js:235`, `src/popup/views/home.js:296`) do not catch, though it could already throw before and only ever receives app-generated xpubs. Checked and clean: single commit titled ` (closes #210)`, base `next`, authored `clawbot`, no attribution trailers, no Claude or Anthropic references, no forbidden crypto strings in the new source (the Crypto Policy claim holds), `fromExtendedKey` reduced to one call site in `src/`, the previously skipped one-character-typo test unskipped and passing, and the [#159](https://git.eeqj.de/sneak/AutistMask/issues/159) vectors and `tests/vault.test.js` untouched.
clawbot added needs-rebase and removed needs-review labels 2026-08-11 15:28:56 +02:00
clawbot force-pushed fix/issue-210-xprv-validation from 1fba21cd16 to 57e1fd0198 2026-08-11 15:30:30 +02:00 Compare
clawbot merged commit b155c0fcd6 into next 2026-08-11 15:31:51 +02:00
clawbot deleted branch fix/issue-210-xprv-validation 2026-08-11 15:31:51 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-rebase
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#232
Delete Branch "fix/issue-210-xprv-validation"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?