fix: an already-imported non-master xprv wallet will throw at signing time with no explanation #234
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
#210 makes extended-key import reject a non-master (depth > 0) key, because the serialized format does not carry the path and guessing the remaining components silently produces the wrong wallet. Correct for new imports.
It does not address wallets already in stored state. A
type: "xprv"wallet holding a depth-3 key — only possible via the old broken path, so its addresses were never the user's to begin with — now throws at signing time instead of silently signing for the wrong tree. Throwing is the better of the two, but it happens at the worst possible moment, with no explanation, on the screen where the user is trying to move money.There is no migration and no user-facing account of what happened.
Raised by the implementer of #232 and deliberately left out of its scope.
Implementation requirements
type: "xprv"wallet whose stored secret is not a master key, at unlock or wallet-list render time — not at signing time.Definition of done
TODO.mdupdated in the same commit.make checkpasses.Plan.
Detection without the password. The stored
xpubon anxprvwallet is the neutered node four levels below the imported key (both the current import path and the pre-fix one derive four levels:m/44'/60'/0'/0absolute,44'/60'/0'/0relative). A master import therefore stores a depth-4 xpub; a depth-dimport stores depthd + 4. Soxpub.depth !== 4is an exact read on "the imported key was not a master key", available at wallet-list render time with no decryption. Newsrc/shared/walletDefects.jsholds that check and the copy.Surfacing. The wallet list renders a named notice block under the wallet's name, and the "+" add-address button is suppressed for such a wallet. Send (both the main-view button and the address-detail one), the dapp approve-transaction and approve-sign screens, and Export Private Key are gated before any decryption, so the user sees the explanation rather than a decrypt-then-throw.
getSignerForAddresskeeps throwing as the backstop.Copy. The addresses do descend from the key the user imported, at a non-standard path, so I will not say the funds are gone and I will not say they are safe. The notice states that the derivation path was applied beneath a non-master key, that these are not the addresses that key produces under the standard path, that anything held at them is still reachable by software repeating the same non-standard derivation, and that the wallet is not deleted.
Nothing is deleted or rewritten. Tests: a state fixture with a depth-3 xprv wallet and a normal depth-0 one, asserted against the rendered wallet-list markup and against the signing paths.
Built in #247.
New
src/shared/walletDefects.jsdetects the wallet from the depth of its storedxpub— a master import stores depth 4, a depth-dimport storesd + 4— so no password is needed and the check runs at wallet-list render time. The list renders the explanation under the wallet's name and withholds the "+" button; send from all three screens, dapp transaction approval, dapp message signing and Export Private Key all refuse before asking for a password;getSignerForAddressstays as the backstop with a message written for signing rather than for import. The wallet record is not modified, and a test asserts that.Copy is in the PR body. It says the addresses do descend from the imported key and that this build cannot sign for them, and it neither claims the funds are safe nor implies a loss.
Verified: 22 new tests in
tests/walletDefects.test.js, demonstrated failing first — with the module stubbed to returnnullandwalletListHtmlabsent,make testgave 13 failed / 370 passed. After the fixmake checkis green on the rebased branch (15 suites, 383 tests,prettier --checkclean twice), andmake buildemits 4 bundles verifiedautistmask-build-debug=off.