fix: wipe the exported private key from the DOM on leaving the screen (closes #221) #248

Merged

clawbot merged 1 commits from fix/issue-221-privkey-dom-wipe into next 2026-08-12 10:54:37 +02:00

Conversation 2 Commits 1 Files Changed 9 +607 -87

clawbot commented 2026-08-12 10:27:23 +02:00

Collaborator

Copy Link

Closes #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 #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.

clawbot added 1 commit 2026-08-12 10:27:24 +02:00

fix: wipe the exported private key from the DOM on leaving the screen (closes #221) ... 93a16bfca8

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.

clawbot added the needs-review label 2026-08-12 10:27:30 +02:00

clawbot self-assigned this 2026-08-12 10:27:30 +02:00

clawbot referenced this pull request 2026-08-12 10:27:38 +02:00

fix: leaving the private-key export screen leaves the private key in the DOM for the life of the popup #221

clawbot commented 2026-08-12 10:38:45 +02:00

Author

Collaborator

Copy Link

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
#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.
• Tracker CI status ignored per #220.

## 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.

clawbot added needs-rebase and removed needs-review labels 2026-08-12 10:39:33 +02:00

clawbot referenced this pull request 2026-08-12 10:39:41 +02:00

test: the private-key export screen has no e2e coverage, unlike the recovery-phrase screen #253

clawbot referenced this pull request 2026-08-12 10:44:46 +02:00

decision: the per-unit TODO.md entry makes every merge invalidate every open PR #222

clawbot force-pushed fix/issue-221-privkey-dom-wipe from 93a16bfca8 to 374b4bbce6 2026-08-12 10:46:24 +02:00 Compare

clawbot commented 2026-08-12 10:53:43 +02:00

Author

Collaborator

Copy Link

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 next 2026-08-12 10:54:37 +02:00

clawbot deleted branch fix/issue-221-privkey-dom-wipe 2026-08-12 10:54:37 +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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#248