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

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

New Issue
Closed
opened 2026-08-11 14:49:25 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-11 14:49:25 +02:00
Collaborator
Copy Link

export-privkey registers no view-leave cleanup. Leaving it by any route other than its own Back button — the settings gear, for instance — leaves the decrypted private key sitting in #export-privkey-value inside the hidden view for the rest of the popup's life.

Shipped behaviour, not introduced by anything in flight. Found by the independent review of #215, which hit the same bug class on the new recovery-phrase screen.

A private key left in a hidden DOM node is recoverable by anything with script access to the popup document, and it outlives the moment the user thought they had dismissed it. The screen's implied contract is that leaving disposes of the secret.

Why it is now cheap

#161 adds an onViewLeave() hook to src/popup/views/helpers.js, run by showView() on ANY exit rather than only on Back. Once that lands this is a two-line fix: register a clear() for export-privkey the same way.

Depends on #215 landing first, since it introduces the hook.

Implementation requirements

  • Register a view-leave cleanup for export-privkey that wipes the key from the DOM and from any closure holding it.
  • Audit the reveal path for the same post-await hole found in the recovery-phrase screen: a write that lands AFTER the leave hook has run, leaving the secret behind with nothing scheduled to wipe it again. Add the liveness guard if it is missing.
  • Check whether any other screen displays secret material with no leave cleanup, and cover them in the same change.

Definition of done

  • Leaving export-privkey by Back, by the settings gear, and by any other navigation clears the key from the DOM.
  • A test leaves the screen DURING the decrypt and asserts the key never lands in the DOM.
  • export-privkey remains absent from RESTORABLE_VIEWS.
  • Any other secret-bearing screen found in the audit is covered or explicitly ruled out in the PR body.
  • TODO.md updated in the same commit.
  • make check passes.
`export-privkey` registers no view-leave cleanup. Leaving it by any route other than its own Back button — the settings gear, for instance — leaves the decrypted private key sitting in `#export-privkey-value` inside the hidden view for the rest of the popup's life. Shipped behaviour, not introduced by anything in flight. Found by the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/215, which hit the same bug class on the new recovery-phrase screen. A private key left in a hidden DOM node is recoverable by anything with script access to the popup document, and it outlives the moment the user thought they had dismissed it. The screen's implied contract is that leaving disposes of the secret. ## Why it is now cheap https://git.eeqj.de/sneak/AutistMask/issues/161 adds an `onViewLeave()` hook to `src/popup/views/helpers.js`, run by `showView()` on ANY exit rather than only on Back. Once that lands this is a two-line fix: register a `clear()` for `export-privkey` the same way. Depends on https://git.eeqj.de/sneak/AutistMask/pulls/215 landing first, since it introduces the hook. ## Implementation requirements - Register a view-leave cleanup for `export-privkey` that wipes the key from the DOM and from any closure holding it. - Audit the reveal path for the same post-await hole found in the recovery-phrase screen: a write that lands AFTER the leave hook has run, leaving the secret behind with nothing scheduled to wipe it again. Add the liveness guard if it is missing. - Check whether any other screen displays secret material with no leave cleanup, and cover them in the same change. ## Definition of done - [ ] Leaving `export-privkey` by Back, by the settings gear, and by any other navigation clears the key from the DOM. - [ ] A test leaves the screen DURING the decrypt and asserts the key never lands in the DOM. - [ ] `export-privkey` remains absent from `RESTORABLE_VIEWS`. - [ ] Any other secret-bearing screen found in the audit is covered or explicitly ruled out in the PR body. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.
clawbot added this to the 1.0.0 milestone 2026-08-11 14:49:25 +02:00
clawbot commented 2026-08-12 10:19:39 +02:00
Author
Collaborator
Copy Link

Plan:

  1. Extract the export screen out of src/popup/views/addressDetail.js into its own 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 a revealGeneration liveness guard. Extraction is what makes the load-bearing test possible — addressDetail.js cannot be required outside a browser, showPhrase.js-shaped module can.
  2. The post-await hole IS present: the confirm handler awaits decryptWithPassword, then writes signer.privateKey into #export-privkey-value with no check that the screen is still live. Guard goes in, key derivation moves behind the guard so nothing is even derived once the screen is gone.
  3. Audit of every other screen holding secret material in the DOM with no leave cleanup — add-wallet (a generated recovery phrase, an imported private key or xprv, and the password, all cleared on entry only), confirm-tx, delete-wallet-confirm, approve-tx, approve-sign (password inputs, cleared on entry only, left populated after the screen navigates on). Each gets an onViewLeave clear in the same change; none of them has a post-await write, so none needs a generation guard.
  4. Tests: a new tests/exportPrivkey.test.js driving the module against a small DOM stub — leave during the decrypt asserts the key never lands in #export-privkey-value, plus leave-by-gear and leave-by-Back after a completed reveal, plus an assertion that export-privkey is absent from RESTORABLE_VIEWS.
  5. README.md view-navigation section and TODO.md updated in the same commit.
Plan: 1. Extract the export screen out of `src/popup/views/addressDetail.js` into its own `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 a `revealGeneration` liveness guard. Extraction is what makes the load-bearing test possible — `addressDetail.js` cannot be required outside a browser, `showPhrase.js`-shaped module can. 2. The post-await hole IS present: the confirm handler awaits `decryptWithPassword`, then writes `signer.privateKey` into `#export-privkey-value` with no check that the screen is still live. Guard goes in, key derivation moves behind the guard so nothing is even derived once the screen is gone. 3. Audit of every other screen holding secret material in the DOM with no leave cleanup — `add-wallet` (a generated recovery phrase, an imported private key or xprv, and the password, all cleared on entry only), `confirm-tx`, `delete-wallet-confirm`, `approve-tx`, `approve-sign` (password inputs, cleared on entry only, left populated after the screen navigates on). Each gets an `onViewLeave` clear in the same change; none of them has a post-await write, so none needs a generation guard. 4. Tests: a new `tests/exportPrivkey.test.js` driving the module against a small DOM stub — leave during the decrypt asserts the key never lands in `#export-privkey-value`, plus leave-by-gear and leave-by-Back after a completed reveal, plus an assertion that `export-privkey` is absent from `RESTORABLE_VIEWS`. 5. `README.md` view-navigation section and `TODO.md` updated in the same commit.
clawbot commented 2026-08-12 10:27:38 +02:00
Author
Collaborator
Copy Link

Built in #248 (branch fix/issue-221-privkey-dom-wipe, base next).

The post-await hole was present. The screen moved into src/popup/views/exportPrivkey.js with an onViewLeave() cleanup and the revealGeneration liveness guard, the guard placed in front of the key derivation so an abandoned decrypt does not even derive the key. The audit covered five more screens holding secret material in the DOM with entry-only clearing — add-wallet (generated or pasted recovery phrase, private key, extended private key, password), confirm-tx, delete-wallet-confirm, approve-tx, approve-sign — all now registering the same cleanup; none of them writes a secret after an await, so none needed a guard.

Verified: make check green on the rebased branch (15 suites, 374 tests, prettier clean) and script/cibuild exit 0 with the make check and make build layers executing rather than cached. The load-bearing test was confirmed load-bearing by removing the guard and re-running make check: 2 failures, both in tests/exportPrivkey.test.js — the leave-during-decrypt case and the harder re-entry variant that only the generation counter catches.

Built in https://git.eeqj.de/sneak/AutistMask/pulls/248 (branch `fix/issue-221-privkey-dom-wipe`, base `next`). The post-await hole was present. The screen moved into `src/popup/views/exportPrivkey.js` with an `onViewLeave()` cleanup and the `revealGeneration` liveness guard, the guard placed in front of the key derivation so an abandoned decrypt does not even derive the key. The audit covered five more screens holding secret material in the DOM with entry-only clearing — `add-wallet` (generated or pasted recovery phrase, private key, extended private key, password), `confirm-tx`, `delete-wallet-confirm`, `approve-tx`, `approve-sign` — all now registering the same cleanup; none of them writes a secret after an await, so none needed a guard. Verified: `make check` green on the rebased branch (15 suites, 374 tests, prettier clean) and `script/cibuild` exit 0 with the `make check` and `make build` layers executing rather than cached. The load-bearing test was confirmed load-bearing by removing the guard and re-running `make check`: 2 failures, both in `tests/exportPrivkey.test.js` — the leave-during-decrypt case and the harder re-entry variant that only the generation counter catches.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#221
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?