export-privkey registered no view-leave cleanup. Leaving it by any route other than its own Back button — the settings gear, for instance — left the decrypted private key in #export-privkey-value inside the hidden view for the rest of the popup's life.
The post-await hole: present
Confirmed present, and fixed. The old confirm handler in src/popup/views/addressDetail.js awaited decryptWithPassword, then derived the key and wrote it into the DOM with no check that the screen was still live. A decrypt still running when the screen was left resolved after the (previously nonexistent) wipe and wrote the key into the hidden view, with nothing scheduled to wipe it again — so the leave hook alone would not have closed the bug.
The fix is the revealGeneration counter the recovery phrase screen uses: clear() bumps it, reveal() captures it before the await, and isCurrentReveal() requires the generation to be unmoved, an address to still be selected, and the view to still be current. The guard sits in front of the key derivation, so an abandoned decrypt does not even derive the key. The failure path is guarded the same way, so a wrong-password error cannot be written onto a screen the user has left.
Shape of the change
The screen moves out of addressDetail.js into src/popup/views/exportPrivkey.js, modelled on src/popup/views/showPhrase.js: onViewLeave("export-privkey", clear), a clear() that wipes the value node, the password input and the closure state, and no import of src/shared/log.js. addressDetail.js now only opens the screen and calls its init(). The Back button's hand-rolled wipe is gone — goBack() routes through showView(), which runs the hook, so every route is covered by one code path rather than one route by a per-button wipe.
Two incidental changes came with the move: the password input is now cleared as soon as it has been spent, and the wrong-password message became the full sentence "That password is not correct. Please try again." (repo language rule; the previous "Wrong password." fragment is documented in README.md, which is updated).
Audit of every other secret-bearing screen
Covered — each held secret material in the DOM, cleared on entry only, and so kept it in its hidden view after the screen navigated on:
AddWallet (add-wallet) — the strongest of the five: #wallet-mnemonic holds a freshly generated recovery phrase after the die button, plus a pasted phrase, private key or extended private key, plus the password. Now wiped on leave, including on the successful import that navigates to Home.
ConfirmTx (confirm-tx) — #confirm-tx-password survived the navigation to the wait screen after a send.
DeleteWallet (delete-wallet-confirm) — #delete-wallet-password survived the navigation to Settings or Welcome after the deletion.
ApproveTx (approve-tx) and ApproveSign (approve-sign) — the approval window navigates on to the wait screen after signing; the password stayed behind for the life of that window.
None of the five writes a secret after an await — the values are user input, written synchronously — so none needs a generation guard. Ruled out: ShowRecoveryPhrase (show-phrase) already has both the hook and the guard, from #161; no other view reads or writes secret material.
RESTORABLE_VIEWS
export-privkey is still absent from src/popup/restorableViews.js, and tests/exportPrivkey.test.js now asserts that rather than leaving it observed. That file is otherwise untouched.
Tests
tests/exportPrivkey.test.js (13 tests) drives the module against a small DOM stub — the module is deliberately shaped so it needs nothing a browser would have to provide.
The load-bearing test leaves the screen mid-decrypt (the settings gear) and asserts the key never lands in #export-privkey-value, and that getSignerForAddress was never called at all. It is genuinely load-bearing: with the liveness guard removed from reveal(), make check fails with 2 failing tests, both in this file — that one, plus the harder variant that leaves mid-decrypt and then re-enters the screen, which the generation counter catches and a current-view check alone would not. A companion test asserts an uninterrupted reveal still puts the key on screen, so a guard that rejected every write could not pass the suite.
Plus: Back, the settings gear and any other navigation each clear a revealed key; nothing is written before the password is accepted; a wrong password reveals nothing; the module imports no logger.
Verification
make check green on the rebased branch: 15 suites, 374 tests passed, prettier clean.
script/cibuild (docker build, runs make check and make build in the container) exit 0, with both layers executing rather than CACHED: #11 [7/8] RUN make check → 374 passed, prettier clean; #12 RUN make build → 4 bundles verified autistmask-build-debug=off.
make fmt run over the changed markdown.
Closes https://git.eeqj.de/sneak/AutistMask/issues/221.
## The bug
`export-privkey` registered no view-leave cleanup. Leaving it by any route other than its own Back button — the settings gear, for instance — left the decrypted private key in `#export-privkey-value` inside the hidden view for the rest of the popup's life.
## The post-await hole: present
Confirmed present, and fixed. The old confirm handler in `src/popup/views/addressDetail.js` awaited `decryptWithPassword`, then derived the key and wrote it into the DOM with no check that the screen was still live. A decrypt still running when the screen was left resolved after the (previously nonexistent) wipe and wrote the key into the hidden view, with nothing scheduled to wipe it again — so the leave hook alone would not have closed the bug.
The fix is the `revealGeneration` counter the recovery phrase screen uses: `clear()` bumps it, `reveal()` captures it before the await, and `isCurrentReveal()` requires the generation to be unmoved, an address to still be selected, and the view to still be current. The guard sits *in front of* the key derivation, so an abandoned decrypt does not even derive the key. The failure path is guarded the same way, so a wrong-password error cannot be written onto a screen the user has left.
## Shape of the change
The screen moves out of `addressDetail.js` into `src/popup/views/exportPrivkey.js`, modelled on `src/popup/views/showPhrase.js`: `onViewLeave("export-privkey", clear)`, a `clear()` that wipes the value node, the password input and the closure state, and no import of `src/shared/log.js`. `addressDetail.js` now only opens the screen and calls its `init()`. The Back button's hand-rolled wipe is gone — `goBack()` routes through `showView()`, which runs the hook, so every route is covered by one code path rather than one route by a per-button wipe.
Two incidental changes came with the move: the password input is now cleared as soon as it has been spent, and the wrong-password message became the full sentence "That password is not correct. Please try again." (repo language rule; the previous "Wrong password." fragment is documented in `README.md`, which is updated).
## Audit of every other secret-bearing screen
Covered — each held secret material in the DOM, cleared on entry only, and so kept it in its hidden view after the screen navigated on:
- **AddWallet** (`add-wallet`) — the strongest of the five: `#wallet-mnemonic` holds a freshly *generated* recovery phrase after the die button, plus a pasted phrase, private key or extended private key, plus the password. Now wiped on leave, including on the successful import that navigates to Home.
- **ConfirmTx** (`confirm-tx`) — `#confirm-tx-password` survived the navigation to the wait screen after a send.
- **DeleteWallet** (`delete-wallet-confirm`) — `#delete-wallet-password` survived the navigation to Settings or Welcome after the deletion.
- **ApproveTx** (`approve-tx`) and **ApproveSign** (`approve-sign`) — the approval window navigates on to the wait screen after signing; the password stayed behind for the life of that window.
None of the five writes a secret after an `await` — the values are user input, written synchronously — so none needs a generation guard. Ruled out: **ShowRecoveryPhrase** (`show-phrase`) already has both the hook and the guard, from https://git.eeqj.de/sneak/AutistMask/issues/161; no other view reads or writes secret material.
## `RESTORABLE_VIEWS`
`export-privkey` is still absent from `src/popup/restorableViews.js`, and `tests/exportPrivkey.test.js` now asserts that rather than leaving it observed. That file is otherwise untouched.
## Tests
`tests/exportPrivkey.test.js` (13 tests) drives the module against a small DOM stub — the module is deliberately shaped so it needs nothing a browser would have to provide.
The load-bearing test leaves the screen mid-decrypt (the settings gear) and asserts the key never lands in `#export-privkey-value`, and that `getSignerForAddress` was never called at all. It is genuinely load-bearing: with the liveness guard removed from `reveal()`, `make check` fails with 2 failing tests, both in this file — that one, plus the harder variant that leaves mid-decrypt and then *re-enters* the screen, which the generation counter catches and a current-view check alone would not. A companion test asserts an uninterrupted reveal still puts the key on screen, so a guard that rejected every write could not pass the suite.
Plus: Back, the settings gear and any other navigation each clear a revealed key; nothing is written before the password is accepted; a wrong password reveals nothing; the module imports no logger.
## Verification
- `make check` green on the rebased branch: 15 suites, 374 tests passed, prettier clean.
- `script/cibuild` (docker build, runs `make check` and `make build` in the container) exit 0, with both layers executing rather than `CACHED`: `#11 [7/8] RUN make check` → 374 passed, prettier clean; `#12 RUN make build` → 4 bundles verified `autistmask-build-debug=off`.
- `make fmt` run over the changed markdown.
The export screen registered no view-leave cleanup, so leaving it by any
route other than its own Back button — the settings gear, for instance —
left the decrypted private key in #export-privkey-value inside the hidden
view for the rest of the popup's life. The reveal path had the same
post-await hole the recovery phrase screen had: the write landed after
the wipe, with nothing scheduled to wipe it again.
The screen moves out of addressDetail.js into its own module shaped like
showPhrase.js: onViewLeave() cleanup that wipes the value node, the
password input and the closure state, and a revealGeneration liveness
guard captured before the decrypt. The guard sits in front of the key
derivation, so a decrypt that resolves after the screen was left does not
even derive the key.
The audit for the same bug class covered every other screen holding
secret material in the DOM. AddWallet (a generated or pasted recovery
phrase, an imported private key or extended private key, and the
password) and the password inputs on ConfirmTx, DeleteWallet, ApproveTx
and ApproveSign were all cleared on entry only, so each survived in its
hidden view after the screen navigated on. All five now register the same
cleanup. None of them writes a secret after an await, so none needs a
generation guard.
The load-bearing test leaves the screen mid-decrypt and asserts the key
never lands in the DOM, and that it still does not land once the user has
returned to the screen — which the generation counter catches and a
current-view check alone would not.
The change itself passes review. It does not apply to current next.
Finding 1 — head does not rebase onto current origin/next (TODO.md)
origin/next has moved from ba35282 (this branch's base) to ce4a0d7, picking up #230 and #182.
Both that unit and this one insert a bullet at the top of the # Completed Steps list in TODO.md:47, so the replay conflicts.
Reproduction:
git fetch origin next
git checkout 93a16bf
git rebase origin/next
# Auto-merging README.md
# CONFLICT (content): Merge conflict in TODO.md
README.md auto-merges; TODO.md is the only conflicted path.
Acceptable: rebase fix/issue-221-privkey-dom-wipe onto ce4a0d7, keeping BOTH bullets — the #230 entry that landed on next and this branch's #221 entry — still a single commit, then re-run make check.
Note the PR's mergeable: true flag is stale: it was computed against ba35282 when the PR was opened at 10:27.
Disclosures
make check and script/cibuild were re-run here and the author's figures reproduce exactly: containerized make check layer EXECUTED (18.9s, not CACHED) — 15 suites, 374 tests passed, prettier clean; make build executed, 4 bundles.
make test-e2e run: exit 0, 13/13. The e2e suite has no export-privkey coverage — all of its secret-screen cases are show-phrase (#161). Popup init is exercised, which is where a ReferenceError from the five touched views would surface, and the DoD does not ask for an e2e case, so this is noted, not held against the PR.
Mutation tests reproduce the author's claim and go beyond it: dropping the reveal() liveness guard fails exactly 2 tests, both in tests/exportPrivkey.test.js; dropping only the generation === revealGeneration term from isCurrentReveal() fails exactly the re-entry test, so the counter is load-bearing over a plain current-view check; dropping onViewLeave(VIEW, clear) fails 3. Mutations were made in a private clone and reverted; nothing was committed or pushed.
An independent residue sweep (scratch test, since deleted) checked every stub DOM node's textContent/value/innerHTML and every chrome.storage.local write for the key after Back, the settings gear, a jump to main, re-entry, copy-then-leave, and two overlapping reveals where the stale one resolves last: no residue on any route, and the decrypted wallet secret never reaches the DOM or storage. The sweep asserts the key IS present before each leave, so it is not vacuous.
Pre-existing and untouched, so out of scope here, but they violate RULES.md:120 (error messages are full sentences): src/popup/views/confirmTx.js:425 and src/popup/views/deleteWallet.js:76 still say "Wrong password." — the fragment this PR replaced on the export screen.
## FAIL — `needs-rebase`
The change itself passes review. It does not apply to current `next`.
### Finding 1 — head does not rebase onto current `origin/next` (`TODO.md`)
`origin/next` has moved from `ba35282` (this branch's base) to `ce4a0d7`, picking up
https://git.eeqj.de/sneak/AutistMask/issues/230 and https://git.eeqj.de/sneak/AutistMask/issues/182.
Both that unit and this one insert a bullet at the top of the `# Completed Steps` list in `TODO.md:47`, so the replay conflicts.
Reproduction:
```
git fetch origin next
git checkout 93a16bf
git rebase origin/next
# Auto-merging README.md
# CONFLICT (content): Merge conflict in TODO.md
```
`README.md` auto-merges; `TODO.md` is the only conflicted path.
Acceptable: rebase `fix/issue-221-privkey-dom-wipe` onto `ce4a0d7`, keeping BOTH bullets — the
https://git.eeqj.de/sneak/AutistMask/issues/230 entry that landed on `next` and this branch's
https://git.eeqj.de/sneak/AutistMask/issues/221 entry — still a single commit, then re-run `make check`.
Note the PR's `mergeable: true` flag is stale: it was computed against `ba35282` when the PR was opened at 10:27.
### Disclosures
- `make check` and `script/cibuild` were re-run here and the author's figures reproduce exactly: containerized `make check` layer EXECUTED (18.9s, not `CACHED`) — 15 suites, 374 tests passed, prettier clean; `make build` executed, 4 bundles.
- `make test-e2e` run: exit 0, 13/13. The e2e suite has no `export-privkey` coverage — all of its secret-screen cases are `show-phrase` (https://git.eeqj.de/sneak/AutistMask/issues/161). Popup init is exercised, which is where a `ReferenceError` from the five touched views would surface, and the DoD does not ask for an e2e case, so this is noted, not held against the PR.
- Mutation tests reproduce the author's claim and go beyond it: dropping the `reveal()` liveness guard fails exactly 2 tests, both in `tests/exportPrivkey.test.js`; dropping only the `generation === revealGeneration` term from `isCurrentReveal()` fails exactly the re-entry test, so the counter is load-bearing over a plain current-view check; dropping `onViewLeave(VIEW, clear)` fails 3. Mutations were made in a private clone and reverted; nothing was committed or pushed.
- An independent residue sweep (scratch test, since deleted) checked every stub DOM node's `textContent`/`value`/`innerHTML` and every `chrome.storage.local` write for the key after Back, the settings gear, a jump to `main`, re-entry, copy-then-leave, and two overlapping reveals where the stale one resolves last: no residue on any route, and the decrypted wallet secret never reaches the DOM or storage. The sweep asserts the key IS present before each leave, so it is not vacuous.
- Pre-existing and untouched, so out of scope here, but they violate `RULES.md:120` (error messages are full sentences): `src/popup/views/confirmTx.js:425` and `src/popup/views/deleteWallet.js:76` still say "Wrong password." — the fragment this PR replaced on the export screen.
- Tracker CI status ignored per https://git.eeqj.de/sneak/AutistMask/issues/220.
PASS (narrow conflict-resolution verification of 374b4bb): #247's defect gate in src/popup/views/addressDetail.js:317-321 returns before pushCurrentView(), any view change and any password UI — driven with a depth-7 xprv fixture, no key derived and no password screen reached, and deleting the gate makes that driver fail; no dead import remains referenced; src/popup/views/approval.js keeps both sides (walletDefect, gateOnWalletDefect() at the tail of showTxApproval/showSignApproval plus the two in-handler checks, and onViewLeave + clearTxPassword/clearSignPassword registered in init()); mutations reproduce exactly 2 and exactly 3 failures; make check 20 suites / 429 tests and make test-e2e 14/14, both executed; fast-forward onto current origin/next, TODO.md retains all landed entries, single clawbot-authored commit. Disclosures: currentAddress is imported unused in src/popup/views/addressDetail.js:15 — pre-existing on next, not introduced here, and lint is prettier-only so nothing flags it; the ad-hoc gate driver was a scratch file outside the branch, run directly rather than through make (all suite/e2e counts above came from the make entrypoints on a byte-identical restored tree).
PASS (narrow conflict-resolution verification of `374b4bb`): #247's defect gate in `src/popup/views/addressDetail.js:317-321` returns before `pushCurrentView()`, any view change and any password UI — driven with a depth-7 `xprv` fixture, no key derived and no password screen reached, and deleting the gate makes that driver fail; no dead import remains referenced; `src/popup/views/approval.js` keeps both sides (`walletDefect`, `gateOnWalletDefect()` at the tail of `showTxApproval`/`showSignApproval` plus the two in-handler checks, and `onViewLeave` + `clearTxPassword`/`clearSignPassword` registered in `init()`); mutations reproduce exactly 2 and exactly 3 failures; `make check` 20 suites / 429 tests and `make test-e2e` 14/14, both executed; fast-forward onto current `origin/next`, `TODO.md` retains all landed entries, single `clawbot`-authored commit. Disclosures: `currentAddress` is imported unused in `src/popup/views/addressDetail.js:15` — pre-existing on `next`, not introduced here, and lint is prettier-only so nothing flags it; the ad-hoc gate driver was a scratch file outside the branch, run directly rather than through `make` (all suite/e2e counts above came from the `make` entrypoints on a byte-identical restored tree).
clawbot
merged commit 23712b53cb into next2026-08-12 10:54:37 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #221.
The bug
export-privkeyregistered no view-leave cleanup. Leaving it by any route other than its own Back button — the settings gear, for instance — left the decrypted private key in#export-privkey-valueinside the hidden view for the rest of the popup's life.The post-await hole: present
Confirmed present, and fixed. The old confirm handler in
src/popup/views/addressDetail.jsawaiteddecryptWithPassword, then derived the key and wrote it into the DOM with no check that the screen was still live. A decrypt still running when the screen was left resolved after the (previously nonexistent) wipe and wrote the key into the hidden view, with nothing scheduled to wipe it again — so the leave hook alone would not have closed the bug.The fix is the
revealGenerationcounter the recovery phrase screen uses:clear()bumps it,reveal()captures it before the await, andisCurrentReveal()requires the generation to be unmoved, an address to still be selected, and the view to still be current. The guard sits in front of the key derivation, so an abandoned decrypt does not even derive the key. The failure path is guarded the same way, so a wrong-password error cannot be written onto a screen the user has left.Shape of the change
The screen moves out of
addressDetail.jsintosrc/popup/views/exportPrivkey.js, modelled onsrc/popup/views/showPhrase.js:onViewLeave("export-privkey", clear), aclear()that wipes the value node, the password input and the closure state, and no import ofsrc/shared/log.js.addressDetail.jsnow only opens the screen and calls itsinit(). The Back button's hand-rolled wipe is gone —goBack()routes throughshowView(), which runs the hook, so every route is covered by one code path rather than one route by a per-button wipe.Two incidental changes came with the move: the password input is now cleared as soon as it has been spent, and the wrong-password message became the full sentence "That password is not correct. Please try again." (repo language rule; the previous "Wrong password." fragment is documented in
README.md, which is updated).Audit of every other secret-bearing screen
Covered — each held secret material in the DOM, cleared on entry only, and so kept it in its hidden view after the screen navigated on:
add-wallet) — the strongest of the five:#wallet-mnemonicholds a freshly generated recovery phrase after the die button, plus a pasted phrase, private key or extended private key, plus the password. Now wiped on leave, including on the successful import that navigates to Home.confirm-tx) —#confirm-tx-passwordsurvived the navigation to the wait screen after a send.delete-wallet-confirm) —#delete-wallet-passwordsurvived the navigation to Settings or Welcome after the deletion.approve-tx) and ApproveSign (approve-sign) — the approval window navigates on to the wait screen after signing; the password stayed behind for the life of that window.None of the five writes a secret after an
await— the values are user input, written synchronously — so none needs a generation guard. Ruled out: ShowRecoveryPhrase (show-phrase) already has both the hook and the guard, from #161; no other view reads or writes secret material.RESTORABLE_VIEWSexport-privkeyis still absent fromsrc/popup/restorableViews.js, andtests/exportPrivkey.test.jsnow asserts that rather than leaving it observed. That file is otherwise untouched.Tests
tests/exportPrivkey.test.js(13 tests) drives the module against a small DOM stub — the module is deliberately shaped so it needs nothing a browser would have to provide.The load-bearing test leaves the screen mid-decrypt (the settings gear) and asserts the key never lands in
#export-privkey-value, and thatgetSignerForAddresswas never called at all. It is genuinely load-bearing: with the liveness guard removed fromreveal(),make checkfails with 2 failing tests, both in this file — that one, plus the harder variant that leaves mid-decrypt and then re-enters the screen, which the generation counter catches and a current-view check alone would not. A companion test asserts an uninterrupted reveal still puts the key on screen, so a guard that rejected every write could not pass the suite.Plus: Back, the settings gear and any other navigation each clear a revealed key; nothing is written before the password is accepted; a wrong password reveals nothing; the module imports no logger.
Verification
make checkgreen on the rebased branch: 15 suites, 374 tests passed, prettier clean.script/cibuild(docker build, runsmake checkandmake buildin the container) exit 0, with both layers executing rather thanCACHED:#11 [7/8] RUN make check→ 374 passed, prettier clean;#12 RUN make build→ 4 bundles verifiedautistmask-build-debug=off.make fmtrun over the changed markdown.FAIL —
needs-rebaseThe change itself passes review. It does not apply to current
next.Finding 1 — head does not rebase onto current
origin/next(TODO.md)origin/nexthas moved fromba35282(this branch's base) toce4a0d7, picking up#230 and #182.
Both that unit and this one insert a bullet at the top of the
# Completed Stepslist inTODO.md:47, so the replay conflicts.Reproduction:
README.mdauto-merges;TODO.mdis the only conflicted path.Acceptable: rebase
fix/issue-221-privkey-dom-wipeontoce4a0d7, keeping BOTH bullets — the#230 entry that landed on
nextand this branch's#221 entry — still a single commit, then re-run
make check.Note the PR's
mergeable: trueflag is stale: it was computed againstba35282when the PR was opened at 10:27.Disclosures
make checkandscript/cibuildwere re-run here and the author's figures reproduce exactly: containerizedmake checklayer EXECUTED (18.9s, notCACHED) — 15 suites, 374 tests passed, prettier clean;make buildexecuted, 4 bundles.make test-e2erun: exit 0, 13/13. The e2e suite has noexport-privkeycoverage — all of its secret-screen cases areshow-phrase(#161). Popup init is exercised, which is where aReferenceErrorfrom the five touched views would surface, and the DoD does not ask for an e2e case, so this is noted, not held against the PR.reveal()liveness guard fails exactly 2 tests, both intests/exportPrivkey.test.js; dropping only thegeneration === revealGenerationterm fromisCurrentReveal()fails exactly the re-entry test, so the counter is load-bearing over a plain current-view check; droppingonViewLeave(VIEW, clear)fails 3. Mutations were made in a private clone and reverted; nothing was committed or pushed.textContent/value/innerHTMLand everychrome.storage.localwrite for the key after Back, the settings gear, a jump tomain, re-entry, copy-then-leave, and two overlapping reveals where the stale one resolves last: no residue on any route, and the decrypted wallet secret never reaches the DOM or storage. The sweep asserts the key IS present before each leave, so it is not vacuous.RULES.md:120(error messages are full sentences):src/popup/views/confirmTx.js:425andsrc/popup/views/deleteWallet.js:76still say "Wrong password." — the fragment this PR replaced on the export screen.93a16bfca8to374b4bbce6PASS (narrow conflict-resolution verification of
374b4bb): #247's defect gate insrc/popup/views/addressDetail.js:317-321returns beforepushCurrentView(), any view change and any password UI — driven with a depth-7xprvfixture, no key derived and no password screen reached, and deleting the gate makes that driver fail; no dead import remains referenced;src/popup/views/approval.jskeeps both sides (walletDefect,gateOnWalletDefect()at the tail ofshowTxApproval/showSignApprovalplus the two in-handler checks, andonViewLeave+clearTxPassword/clearSignPasswordregistered ininit()); mutations reproduce exactly 2 and exactly 3 failures;make check20 suites / 429 tests andmake test-e2e14/14, both executed; fast-forward onto currentorigin/next,TODO.mdretains all landed entries, singleclawbot-authored commit. Disclosures:currentAddressis imported unused insrc/popup/views/addressDetail.js:15— pre-existing onnext, not introduced here, and lint is prettier-only so nothing flags it; the ad-hoc gate driver was a scratch file outside the branch, run directly rather than throughmake(all suite/e2e counts above came from themakeentrypoints on a byte-identical restored tree).