feat: password-gated recovery phrase display for HD wallets (closes #161) #215
Reference in New Issue
Block a user
Delete Branch "feat/issue-161-show-recovery-phrase"
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?
Closes #161.
Entry point
The wallet row in Settings, next to the rename and delete actions that are
already per-wallet: a
[recovery phrase]button rendered only on HD walletrows, opening a new
show-phrasescreen. The AddressDetail "more" menu wasthe alternative and is wrong — the phrase belongs to the wallet, not to one
address, and AddressDetail already owns the per-address private key export.
Structure, password gate and warning treatment mirror
src/popup/views/addressDetail.js:297-374(ExportPrivKey).How each security requirement is discharged
walletHasRecoveryPhrase()insrc/shared/wallet.jsis anallowlist on
type === "hd", sokeyandxprvare both excluded, and sois any type added later. It gates the button in Settings and is re-checked
in
show()and in the reveal handler, so reaching the screen by anotherroute still cannot produce a phrase.
show()renders only the wallet nameand the password prompt.
decryptWithPasswordis the only thing thatproduces the phrase, and its result is written to
#show-phrase-valueonlyon success. A wrong password sets a full-sentence error, leaves the value
node empty and keeps the result section hidden.
cleanup with
showView()viaonViewLeave()insrc/popup/views/helpers.js. A clear wired only to "Back" would leakthrough the Settings gear, which navigates away without touching that
button.
reveal()captures a generation counter that everyclear()bumps, andwrites nothing if it has moved (or if the current view is no longer
show-phrase). See the rework section below.state, so it cannotbe persisted.
RESTORABLE_VIEWSmoves fromsrc/popup/index.jstosrc/popup/restorableViews.js, unchanged, withshow-phraseabsent — thepopup entry point cannot be required outside a browser, so the exclusion
was untestable where it lived.
showPhrase.jsdoes not importsrc/shared/log.jsatall, and the failed-decrypt path reports a fixed sentence rather than the
caught error. Two unit tests pin both.
Rework after review
Against the findings in
the review.
Blocking: the phrase was written into the DOM after the screen had been
left.
reveal()awaiteddecryptWithPassword()and then wrote#show-phrase-valuewith no check that the view was still current, so adecrypt in flight when the screen was left landed the phrase after
clear()had run, with nothing scheduled to wipe again.Fixed in
src/popup/views/showPhrase.js: a module-levelrevealGenerationcounter, bumped by every
clear(), is captured before the await;isCurrentReveal(generation)requires that counter to be unmoved, a walletto still be selected and
state.currentViewto still beshow-phrase. Thesuccess path and the failed-decrypt path both bail on it before touching the
DOM. A counter rather than a bare
walletIndex === nullcheck so thatleaving and re-entering for a different wallet during one decrypt also
discards the stale result.
Every other await in the view, audited.
decryptWithPassword()is theonly one —
grep -n "await\|\.then\|setTimeout\|Promise\|async"oversrc/popup/views/showPhrase.jsreturns that call and its enclosingasync function reveal(), nothing else.show(),clear(),fail()andinit()are fully synchronous. The copy handler callsnavigator.clipboard.writeText()without awaiting it, but it reads thephrase out of the DOM synchronously first and writes nothing back, so it has
no post-await write to guard. No module outside this view writes any
show-phrasenode: the only other references to that name are the entry inVIEWS, the Settings button that opens the screen, and the comment inrestorableViews.js.The probe, before and after. New e2e test 12 forces the interleaving the
reviewer used — both clicks dispatched inside one page task, since
crypto_pwhashis synchronous and a human cannot interleave them oncelibsodium's wasm is warm. It waits for the Reveal button to be re-enabled
(the same continuation that would have written the phrase) rather than
guessing at a duration, and prints its measurement on every run.
Same test, same commit, guard reverted:
With the guard in place:
Minor:
pushCurrentView()could orphan a stack entry. Fixed. The pushmoves out of the Settings click handler and into
showPhrase.show(), afterthe type gate and immediately before
showView(), so nothing is pushed onthe paths where
show()returns without navigating.Minor:
show-phrasereaching the persistedstate.viewStack. Notchanged here. The stack is restored verbatim by
src/shared/state.js, sothis is the general "a non-restorable view can be a Back target" behaviour
rather than anything specific to this screen; fixing it means filtering the
restored stack, which changes navigation for
export-privkeyand everyother non-restorable view. As the review says, no secret is exposed — the
screen comes back empty with
walletIndex === null. Left for its own issuerather than widened into this one.
Pre-existing
export-privkeyequivalent. Out of scope here; tracked as#221.
Corrected claim. An earlier version of this description said the README
RESTORABLE_VIEWSreference was a stale reference introduced by the ScreenMap rewrite. That attribution was wrong: the reference to
src/popup/index.jswas correct atb9bc226, and it is this PR that makesit stale by moving the file. The README text is updated for that reason, not
as a fix to earlier work.
Test evidence
make check(unit, jest) covers the type gate, theRESTORABLE_VIEWSexclusion and the absence of a logger path. The DOM behaviour is driven
against the real popup in a real Chrome by
make test-e2e, which is wherethis repo tests views — nine new e2e tests, all green on the rebased branch:
The e2e tests assert against the wallet's real phrase, captured from the
creation flow:
createWallet()now returns it. "Some twelve words" wouldpass against the wrong wallet, and "nothing at all" would pass against a
screen that showed what it was meant to hide.
Each property was demonstrated red before it was satisfied, by breaking it
on the finished branch and re-running:
type !== "key":2 failed, 151 passed—an xprv wallet does not,an unknown or missing wallet type does not.show-phraseadded toRESTORABLE_VIEWS:1 failed, 152 passed—the recovery phrase screen is not restorable,Expected: false / Received: true.log.errorf()added to the failed-decrypt path:2 failed, 151 passed— both logger tests.onViewLeave()replaced by aclear on the "Back" button only:
10/12—not ok 6 ... the key wallet was offered the recovery phrase actionandnot ok 11 ... phrase still in the DOM after leaving via the settings gear. Test 10 stayed green, which is the point: Back alone is notenough.
not ok 12, quoted above.No pre-fix demonstration exists for the wrong-password test, because before
this change there was no screen to enter a password into.
make checkandmake test-e2eBoth run on this head, rebased onto
nextat86cdea5:The repo's
check / check (push)status is "Waiting to run" on this head, asit is repo-wide; CI green is therefore unverified and the results above are
from local runs.
Docs
README Screen Map gains a ShowRecoveryPhrase entry in the format the map was
just rebuilt into, plus the two Settings lines that point at it, and the
Secret handling paragraph states the in-flight rule. The Navigation
paragraph's
RESTORABLE_VIEWSreference now points atsrc/popup/restorableViews.js, the file this PR moves it to. The End-to-EndTests section names the mid-decrypt case. README TODO checkbox ticked;
TODO.mdCompleted Steps gains one line.A user who created a wallet in AutistMask and did not write the phrase down had no way to retrieve it. Adds a "Show recovery phrase" action on the wallet row in Settings, next to the per-wallet actions that already live there, mirroring the per-address private key export in structure, password gate and warning treatment. The screen displays the secret that owns every address in the wallet, so: - Only HD wallets are offered it. walletHasRecoveryPhrase() is an allowlist on type "hd", so the key and xprv types — which have no phrase at all — are excluded, as is any type added later. - Nothing is decrypted and nothing enters the page until decryptWithPassword accepts the password. A wrong password produces a full-sentence error and leaves the value node empty. - Leaving the screen wipes it by any route, not just "Back": views that hold a secret register a cleanup with showView() via onViewLeave(), which also covers the settings gear. - The phrase is never assigned to state, so it cannot be persisted, and the view is not in RESTORABLE_VIEWS — reopening the popup lands on Home. That set moves to src/popup/restorableViews.js so the exclusion can be asserted directly; the popup entry point cannot be required outside a browser. - The phrase cannot reach the logger: the view does not import src/shared/log.js, and the failed-decrypt path reports a fixed sentence rather than the caught error. Tests: unit coverage for the type gate, the RESTORABLE_VIEWS exclusion and the absence of any logger path; the DOM behaviour is driven against the real popup in the e2e suite, which is where this repo tests views.FAIL —
needs-rework.src/popup/views/showPhrase.js:83-93— the phrase is written into the DOM after the screen has been left, and nothing wipes it afterwards.reveal()awaitsdecryptWithPassword()and then writes$("show-phrase-value").textContent = phraseand un-hides#show-phrase-resultwith no check that the view is still current. If the user leaves during the decrypt,clear()(theonViewLeavehook) has already run, so the write lands after the wipe and no further wipe is scheduled: the phrase sits in#show-phrase-valueinside the hidden#view-show-phrasefor the rest of the popup's life — through Settings, Home, Send — until the user re-enters and re-leaves this screen, or the popup closes. Demonstrated on this head commit in the pinned e2e container, leaving via the settings gear while the decrypt was in flight:# PROBE len=81 equalsPhrase=true resultHidden=false viewHidden=true— the wallet's real phrase, verbatim, five seconds after the user left the screen.Reachability, stated plainly rather than overclaimed: libsodium's
crypto_pwhashis synchronous, so once the wasm is warm the only suspension point is a microtask and a human click cannot interleave; the probe forces the interleave by dispatching both clicks in one task. The human-reachable window is a still-pendingsodium.readyon the first vault use of that page load. The defect is that a screen whose entire contract is "leaving wipes it" performs its one secret-writing operation with no liveness check at all.Acceptable: after the await, bail before touching the DOM if the screen has been left — e.g. return without writing when
walletIndex === null(clear()nulls it) orstate.currentView !== VIEW. Tests 10 and 11 pass today only because they leave after the reveal has completed; the guard needs a test that leaves during it.Minor:
src/popup/views/settings.js:124-127:pushCurrentView()runs beforeshowPhrase.show(idx), which can return without navigating (non-HD, missing wallet), orphaning an entry onstate.viewStack. Not reachable through the UI today because the button renders for HD wallets only; push only when the view is actually shown.show-phraseonto the persistedstate.viewStack. After the popup is reopened onto Settings, "Back" lands on the phrase screen withwalletIndex === nulland "Reveal" answers "No wallet is selected." No secret is exposed; it is a dead end.export-privkeyregisters noonViewLeave, so leaving it by the settings gear leaves the private key in#export-privkey-valuefor the life of the popup. The new hook makes that a two-line fix — worth its own issue.Checked and clean: type gate (allowlist on
"hd", re-checked inshow()andreveal(), no other caller); nothing in the markup before unlock; no logger import, no logger call, no phrase in any error message;RESTORABLE_VIEWSmoved verbatim (same ten entries, same order,show-phraseandexport-privkeyabsent); phrase never assigned tostate, sosaveState()cannot persist it; full-sentence wrong-password error revealing nothing;onViewLeave()fires only for the view that registers it; single commit onnext, title ends(closes #161); no Claude/Anthropic references or attribution trailers;make check159/159 andmake test-e2e12/12 green here on2957601.The repo
check / check (push)status on2957601is still "Waiting to run", so CI green is unverified — that is separate from the rework above.2957601fcdtoad34fa8699ad34fa8699toc951028837PASS at
c951028: six added interleavings all clean,make check183/183 + prettier clean,make test-e2e13/13 (20/20 with my probes), clean merge ontonextatf455b0awithTODO.mdretaining every landed entry, single commit authoredclawbotending(closes #161), no attribution trailers or vendor references.Probes run, each demonstrated red with the guard reverted and green with it, so shipped test 12 is not vacuous (
len=73 equalsPhrase=truereverted vslen=0intact): leave and re-enter the SAME wallet mid-decrypt; leave and re-enter a DIFFERENT wallet (reverted, wallet 1's phrase landed verbatim on wallet 3's screen); two decrypts genuinely in flight at once (button force-enabled — the first reveal does disable it, so the UI cannot produce this unaided); a failed decrypt racing a re-entry (reverted, the stale failure stomped the live screen); popup closed mid-decrypt then reopened. In the re-entry casesviewHidden=falseandwalletIndexwere both restored before the continuation ran, sorevealGenerationwas the only condition rejecting the write — the stated reason for a counter over a bare null check holds.clear()is the sole mutation of the counter and is reached by every entry (show()) and every leave (onViewLeaveviashowView()); no path leaves it unbumped. Await audit re-done independently: oneawait, oneasync, no.then/setTimeout/Promise/queueMicrotaskin the view, all threeaddEventListenercalls ininit()and none insidereveal(); the only deferred helper reachable from this view isflashCopyFeedback(), which touches classes only.Anomalies that pass anyway:
showPhrase.show()pushes the nav stack itself, unlike every other view (pushed by the caller). Deliberate and commented;settings.js:128is the only caller, and no route into Settings gains a double- or missing-push.titleattribute only; the handler is correct by inspection but nothing clicks it.show-phraseontoviewStackin-session as well as across a reopen, so Back from Settings lands on the empty screen. Same class as the tracked restored-stack item and identical forexport-privkey; not counted here.Disclosures: tracker CI ignored on instruction (runner misattributes unrelated jobs);
script/lintis hostprettier --checkby this repo's own design rather than containerized, and was run throughmake check.c951028837to2c1f724545