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

fix: let a user who lost the password delete the wallet, and warn before they can (closes #312) #334

Merged
clawbot merged 1 commits from issue-312-forgot-password into next 2026-08-20 15:12:29 +02:00
Conversation 3 Commits 1 Files Changed 9 +853 -52
clawbot commented 2026-08-20 14:41:01 +02:00
Collaborator
Copy Link

Closes #312.

The wedge

Deleting a wallet was password-gated, and importing its recovery phrase again was refused as a duplicate xpub by findWalletByXpub(). A user holding the phrase but not the password could therefore neither leave the wallet nor come back to it; the only escape was clearing extension storage through browser internals, which takes every other wallet with it. Nothing in the product warned that this was possible.

Which route, and why

The deletion route, not the re-import route. The DoD offers both and asks for one.

Re-import preserves funds access inside the product and destroys nothing, and it is the more attractive option on paper. Against it:

  • It has to be built three times over. hd and xprv are duplicate-checked by xpub, but a key wallet is duplicate-checked by address, so a fix that only relaxes findWalletByXpub() leaves the private-key user exactly as wedged as before.
  • It makes the user retype the recovery phrase into a live popup in order to change a password. That is the one operation the product otherwise works hard to avoid asking for, and it buys nothing the delete-then-import path does not already give.
  • It reaches no end state that delete-then-import does not already reach through code that exists and is tested: AddWallet encrypts under the new password and scanForAddresses() rediscovers the used addresses.
  • It mutates a stored wallet in place rather than removing one, which is a larger blast radius on the one path a user reaches when they are already confused and frightened.

On the attacker question the issue raises: an attacker with the phrase but not the password gains nothing from re-import that they do not already have, because they can import that phrase into any other wallet — the bar is "no worse than the phrase alone", and re-import clears it. So that is not what decided it. What decided it is that deletion covers the case re-import does not (a user who has lost the password and simply wants the wallet gone), needs no new duplicate-detection logic on three code paths, and is unambiguous about what happens.

The deletion route's own risk is the user not understanding what is destroyed, so the screen spends four short paragraphs on exactly that before it asks for anything.

What changed

New screen delete-wallet-lost-password, reached from an underlined "I have lost my password" control on DeleteWallet. It states that the password cannot be recovered or reset; that deleting erases the copy of the key on this device and moves no money on chain; that with the recovery phrase written down the wallet comes back, and in bold that without it the deletion loses everything the wallet holds, forever; and that the other wallets are not touched. Confirmation is typing the wallet's name back.

No password, and no new gate of any kind. A password in front of discarding a secret protects nobody: an attacker at the popup who wants the wallet gone can uninstall the extension, so the only person such a gate stops is the owner who forgot it. The typed name is a check that the user knows which wallet they are on rather than a secret, so it is matched with letter case, surrounding spaces and repeated inner spaces ignored — refusing wallet 2 for Wallet 2 would only teach the user to distrust the control, and the inner-space collapse is what keeps the confirmation satisfiable at all for a name HTML renders differently from how it is stored (see the rework note below). The existing password route on DeleteWallet is left exactly as it was; nothing was added in front of it.

finishDelete() is shared by both routes, so the selection repair, the site-permission cleanup and the AUTISTMASK_ACTIVE_CHANGED broadcast cannot diverge between them. The password route's body is otherwise unchanged.

AddWallet's password hint now states, per import mode, that the password cannot be recovered or reset and names what the only backup is — the recovery phrase, the private key or the extended private key. This is the small part and the most valuable one: it is the only warning a user gets before the wallet exists.

Doc correction in DeleteAddress: recoveryPathText() told the user that deleting the whole wallet "asks for your password". That stopped being true with this change, so the clause is gone. It is copy describing the screen this PR changes, not a drive-by.

Repo UI policies

  • No layout shift. The new error line reserves its height and uses visibility: hidden, as the neighbouring screens do. The AddWallet hint is swapped in place by the tabs and sits directly above the password fields, so it got a reserved height too, and the three wordings are kept within a couple of characters of each other in length. The reserve is min-h-[3rem] = 48px, which is what all three wordings actually measure — see the geometry table below.
  • Clickable affordance. The entry control is underlined with a dashed decoration and cursor-pointer, matching btn-add-wallet-bottom, the repo's existing clickable-text control.
  • onViewLeave() registered. Not because a wallet name is secret, but for the neighbouring reason: a typed confirmation left standing in a hidden view leaves a wallet one click from deletion. The hook also re-enables the confirm button, so a screen left mid-delete is usable on re-entry.
  • Not in RESTORABLE_VIEWS, alongside delete-wallet-confirm. A popup reopened by accident must not land on a button that erases key material. restorableViews.js now says so next to the existing secret-screen rule, and a test asserts both exclusions.
  • Navigation. The two delete screens are siblings, not parent and child: nothing is pushed on the way in, and Back re-enters DeleteWallet through its show() rather than goBack(), because a bare goBack() would land on a screen whose leave hook has already nulled the wallet selection — a Delete Wallet screen naming a wallet whose own button then answers "No wallet selected for deletion." A test drives that path and then presses the confirm button to prove the screen is usable, not merely on screen.

Rework against the failing review

Two findings, both fixed. Nothing else in the change was touched: git diff e54465a..HEAD is three files — src/popup/index.html (+8/-2), src/popup/views/deleteWallet.js (+18/-6), tests/deleteWalletLostPassword.test.js (+27/-0).

1. The hint reserve was 2x too large and pushed the primary button off screen

The 6rem floor was reasoned, not measured, and the reasoning was wrong in the direction that hurts. Measured in the real popup in Chromium at a 360x600 viewport (the hint box renders 368px wide inside the 396px popup body, line-height 16px):

tab hint natural height hint box height, 6rem (before) hint box height, 3rem (after) #btn-add-wallet-confirm bottom, before after
From Phrase (default) 48px (3 lines) 96px 48px 628.13px 580.13px
From Key 48px (3 lines) 96px 48px 477.84px 429.84px
From xprv 48px (3 lines) 96px 48px 512.13px 464.13px

documentElement.scrollHeight on the default tab went from 636 to 600 — the wallet-creation screen no longer overflows the popup at all, and the Import button is inside the fold (580.13 <= 600) instead of entirely below it.

The reserve is kept, only resized. It is doing its job, and still is: with 3rem the hint box measures 48px and the first password field sits 52px below the top of that box on all three tabs (with 6rem those were 96px and 100px, equally constant across tabs). So switching tabs shifts the hint box and the fields under it by zero in both builds — that property is preserved, it is only the size that was wrong. Re-entering the default tab after visiting the other two measures identically. The comment above the element now records the measured 48px and why the floor must not be raised again, instead of the guess it used to record.

Measured with an ad-hoc Playwright script run inside the same pinned autistmask-e2e-chrome image the suite uses (built from this tree, run by image ID, --rm, removed afterwards; the shared tags were not disturbed). Neither e2e suite catches this on its own, because Playwright auto-scrolls before it clicks — the geometry has to be read explicitly.

2. The confirmation could be made untypable

deleteWallet.js compared the typed string against an untrimmed, un-collapsed displayName(). HTML collapses runs of whitespace when it renders, so a wallet named My Wallet (two spaces) displays as My Wallet: the only string the user can see and type was the one string that could never match, and the confirmation on the one screen that exists to unwedge a stuck user became impossible to satisfy.

Both sides now go through one confirmKey() that trims, collapses inner whitespace runs to a single space, and lowercases. New test: a doubled space inside the name is typed back as one renames a wallet to My Wallet (two spaces), asserts the DOM was handed both spaces, types the single-space form, and asserts the wallet is gone from persisted storage.

Verification

make check — green, exit 0, on the pushed commit. 40 suites, 828 tests (827 before, +1 for the new whitespace case); test-verify-build 39 cases; check-censored 150 files; lint ran in the container, and the [lint 1/1] RUN make lint layer executed rather than reporting CACHED.

Both e2e suites run and green on this head, in the pinned containers: make test-e2e 55/55, make test-e2e-firefox 8/8. (The earlier disclosure that the browser suites had not been run no longer applies.)

New suite tests/deleteWalletLostPassword.test.js drives the real view against a DOM stub and a chrome.storage.local stub that structured-clones on both set and get, and asserts against what comes back out of storage rather than against the live state object. The first test in the file pins the stub itself in both directions — mutating what went in does not reach the store, and mutating what came out does not either — because an aliasing get makes every other assertion in the file pass against a build that persists nothing.

Mutations run to prove the tests are not vacuous. The third was run in this rework; the first two were run when the change was first written and neither the code nor the tests they cover were touched since:

  • removeWalletFromState(state, walletIdx) → removeWalletFromState(state, 0): 4 tests fail, including "exactly the named wallet is destroyed". This is the one the DoD asks for — the route removes the target wallet and leaves the others intact, key material, xpubs, addresses and site permissions included.
  • deleting the await saveState() on the wallets-remain path: 5 tests fail. This is the anti-vacuity mutation for persistence specifically: an in-memory splice with no write is indistinguishable from a correct delete when read back from state, and these assertions catch it because they read storage.
  • reverting confirmKey(typed) !== confirmKey(expected) to the old typed.trim().toLowerCase() !== expected.toLowerCase(): exactly 1 test fails, the typed confirmation › a doubled space inside the name is typed back as one (827 passed, 1 failed). The new test fails on precisely the defect it was written for and on nothing else.

No containers survive this work and no image tag was deleted or pruned; docker ps -a is empty and the autistmask-e2e-* tags are as found, with autistmask-e2e-chrome and autistmask-e2e-firefox rebuilt in place by the two suite runs.

Closes [#312](https://git.eeqj.de/sneak/AutistMask/issues/312). ## The wedge Deleting a wallet was password-gated, and importing its recovery phrase again was refused as a duplicate xpub by `findWalletByXpub()`. A user holding the phrase but not the password could therefore neither leave the wallet nor come back to it; the only escape was clearing extension storage through browser internals, which takes every other wallet with it. Nothing in the product warned that this was possible. ## Which route, and why **The deletion route, not the re-import route.** The DoD offers both and asks for one. Re-import preserves funds access inside the product and destroys nothing, and it is the more attractive option on paper. Against it: - It has to be built three times over. `hd` and `xprv` are duplicate-checked by xpub, but a `key` wallet is duplicate-checked by address, so a fix that only relaxes `findWalletByXpub()` leaves the private-key user exactly as wedged as before. - It makes the user retype the recovery phrase into a live popup in order to change a password. That is the one operation the product otherwise works hard to avoid asking for, and it buys nothing the delete-then-import path does not already give. - It reaches no end state that delete-then-import does not already reach through code that exists and is tested: AddWallet encrypts under the new password and `scanForAddresses()` rediscovers the used addresses. - It mutates a stored wallet in place rather than removing one, which is a larger blast radius on the one path a user reaches when they are already confused and frightened. On the attacker question the issue raises: an attacker with the phrase but not the password gains nothing from re-import that they do not already have, because they can import that phrase into any other wallet — the bar is "no worse than the phrase alone", and re-import clears it. So that is not what decided it. What decided it is that deletion covers the case re-import does not (a user who has lost the password and simply wants the wallet gone), needs no new duplicate-detection logic on three code paths, and is unambiguous about what happens. The deletion route's own risk is the user not understanding what is destroyed, so the screen spends four short paragraphs on exactly that before it asks for anything. ## What changed **New screen `delete-wallet-lost-password`**, reached from an underlined "I have lost my password" control on DeleteWallet. It states that the password cannot be recovered or reset; that deleting erases the copy of the key on this device and moves no money on chain; that with the recovery phrase written down the wallet comes back, and in bold that without it the deletion loses everything the wallet holds, forever; and that the other wallets are not touched. Confirmation is typing the wallet's name back. **No password, and no new gate of any kind.** A password in front of *discarding* a secret protects nobody: an attacker at the popup who wants the wallet gone can uninstall the extension, so the only person such a gate stops is the owner who forgot it. The typed name is a check that the user knows which wallet they are on rather than a secret, so it is matched with letter case, surrounding spaces and repeated inner spaces ignored — refusing `wallet 2` for `Wallet 2` would only teach the user to distrust the control, and the inner-space collapse is what keeps the confirmation satisfiable at all for a name HTML renders differently from how it is stored (see the rework note below). The existing password route on DeleteWallet is left exactly as it was; nothing was added in front of it. **`finishDelete()` is shared by both routes**, so the selection repair, the site-permission cleanup and the `AUTISTMASK_ACTIVE_CHANGED` broadcast cannot diverge between them. The password route's body is otherwise unchanged. **AddWallet's password hint** now states, per import mode, that the password cannot be recovered or reset and names what the only backup is — the recovery phrase, the private key or the extended private key. This is the small part and the most valuable one: it is the only warning a user gets before the wallet exists. **Doc correction in DeleteAddress:** `recoveryPathText()` told the user that deleting the whole wallet "asks for your password". That stopped being true with this change, so the clause is gone. It is copy describing the screen this PR changes, not a drive-by. ## Repo UI policies - **No layout shift.** The new error line reserves its height and uses `visibility: hidden`, as the neighbouring screens do. The AddWallet hint is swapped in place by the tabs and sits directly above the password fields, so it got a reserved height too, and the three wordings are kept within a couple of characters of each other in length. The reserve is `min-h-[3rem]` = 48px, which is what all three wordings actually measure — see the geometry table below. - **Clickable affordance.** The entry control is underlined with a dashed decoration and `cursor-pointer`, matching `btn-add-wallet-bottom`, the repo's existing clickable-text control. - **`onViewLeave()` registered.** Not because a wallet name is secret, but for the neighbouring reason: a typed confirmation left standing in a hidden view leaves a wallet one click from deletion. The hook also re-enables the confirm button, so a screen left mid-delete is usable on re-entry. - **Not in `RESTORABLE_VIEWS`**, alongside `delete-wallet-confirm`. A popup reopened by accident must not land on a button that erases key material. `restorableViews.js` now says so next to the existing secret-screen rule, and a test asserts both exclusions. - **Navigation.** The two delete screens are siblings, not parent and child: nothing is pushed on the way in, and Back re-enters DeleteWallet through its `show()` rather than `goBack()`, because a bare `goBack()` would land on a screen whose leave hook has already nulled the wallet selection — a Delete Wallet screen naming a wallet whose own button then answers "No wallet selected for deletion." A test drives that path and then presses the confirm button to prove the screen is usable, not merely on screen. ## Rework against the failing review Two findings, both fixed. Nothing else in the change was touched: `git diff e54465a..HEAD` is three files — `src/popup/index.html` (+8/-2), `src/popup/views/deleteWallet.js` (+18/-6), `tests/deleteWalletLostPassword.test.js` (+27/-0). ### 1. The hint reserve was 2x too large and pushed the primary button off screen The `6rem` floor was reasoned, not measured, and the reasoning was wrong in the direction that hurts. Measured in the real popup in Chromium at a 360x600 viewport (the hint box renders 368px wide inside the 396px popup body, `line-height` 16px): | tab | hint natural height | hint box height, `6rem` (before) | hint box height, `3rem` (after) | `#btn-add-wallet-confirm` bottom, before | after | | --- | --- | --- | --- | --- | --- | | From Phrase (default) | 48px (3 lines) | 96px | 48px | **628.13px** | **580.13px** | | From Key | 48px (3 lines) | 96px | 48px | 477.84px | 429.84px | | From xprv | 48px (3 lines) | 96px | 48px | 512.13px | 464.13px | `documentElement.scrollHeight` on the default tab went from 636 to 600 — the wallet-creation screen no longer overflows the popup at all, and the Import button is inside the fold (580.13 &lt;= 600) instead of entirely below it. **The reserve is kept, only resized.** It is doing its job, and still is: with `3rem` the hint box measures 48px and the first password field sits 52px below the top of that box on all three tabs (with `6rem` those were 96px and 100px, equally constant across tabs). So switching tabs shifts the hint box and the fields under it by zero in both builds — that property is preserved, it is only the size that was wrong. Re-entering the default tab after visiting the other two measures identically. The comment above the element now records the measured 48px and why the floor must not be raised again, instead of the guess it used to record. Measured with an ad-hoc Playwright script run inside the same pinned `autistmask-e2e-chrome` image the suite uses (built from this tree, run by image ID, `--rm`, removed afterwards; the shared tags were not disturbed). Neither e2e suite catches this on its own, because Playwright auto-scrolls before it clicks — the geometry has to be read explicitly. ### 2. The confirmation could be made untypable `deleteWallet.js` compared the typed string against an untrimmed, un-collapsed `displayName()`. HTML collapses runs of whitespace when it renders, so a wallet named `My Wallet` (two spaces) **displays** as `My Wallet`: the only string the user can see and type was the one string that could never match, and the confirmation on the one screen that exists to unwedge a stuck user became impossible to satisfy. Both sides now go through one `confirmKey()` that trims, collapses inner whitespace runs to a single space, and lowercases. New test: `a doubled space inside the name is typed back as one` renames a wallet to `My Wallet` (two spaces), asserts the DOM was handed both spaces, types the single-space form, and asserts the wallet is gone from persisted storage. ## Verification `make check` — **green, exit 0**, on the pushed commit. 40 suites, 828 tests (827 before, +1 for the new whitespace case); `test-verify-build` 39 cases; `check-censored` 150 files; lint ran in the container, and the `[lint 1/1] RUN make lint` layer executed rather than reporting `CACHED`. **Both e2e suites run and green on this head, in the pinned containers:** `make test-e2e` 55/55, `make test-e2e-firefox` 8/8. (The earlier disclosure that the browser suites had not been run no longer applies.) New suite `tests/deleteWalletLostPassword.test.js` drives the real view against a DOM stub and a `chrome.storage.local` stub that **structured-clones on both `set` and `get`**, and asserts against what comes back out of storage rather than against the live `state` object. The first test in the file pins the stub itself in both directions — mutating what went in does not reach the store, and mutating what came out does not either — because an aliasing `get` makes every other assertion in the file pass against a build that persists nothing. Mutations run to prove the tests are not vacuous. The third was run in this rework; the first two were run when the change was first written and neither the code nor the tests they cover were touched since: - `removeWalletFromState(state, walletIdx)` → `removeWalletFromState(state, 0)`: **4 tests fail**, including "exactly the named wallet is destroyed". This is the one the DoD asks for — the route removes the target wallet and leaves the others intact, key material, xpubs, addresses and site permissions included. - deleting the `await saveState()` on the wallets-remain path: **5 tests fail**. This is the anti-vacuity mutation for persistence specifically: an in-memory splice with no write is indistinguishable from a correct delete when read back from `state`, and these assertions catch it because they read storage. - reverting `confirmKey(typed) !== confirmKey(expected)` to the old `typed.trim().toLowerCase() !== expected.toLowerCase()`: **exactly 1 test fails**, `the typed confirmation › a doubled space inside the name is typed back as one` (827 passed, 1 failed). The new test fails on precisely the defect it was written for and on nothing else. No containers survive this work and no image tag was deleted or pruned; `docker ps -a` is empty and the `autistmask-e2e-*` tags are as found, with `autistmask-e2e-chrome` and `autistmask-e2e-firefox` rebuilt in place by the two suite runs.
clawbot added 1 commit 2026-08-20 14:41:01 +02:00
fix: let a user who lost the password delete the wallet, and warn before they can (closes #312)
All checks were successful
check / check (push) Successful in 31s
Details
e2e / e2e-chrome (push) Successful in 1m12s
Details
e2e / e2e-firefox (push) Successful in 24s
Details
e54465ac9f 
Deleting a wallet was password-gated and importing its recovery phrase
again was refused as a duplicate xpub by findWalletByXpub(), so a user who
held the phrase but had forgotten the password could neither leave the
wallet nor come back to it. The only escape was clearing extension storage
through browser internals, which takes every other wallet with it, and
nothing in the product ever warned that this was possible.

DeleteWallet now offers "I have lost my password", a screen that destroys
the wallet after the user types its name back. No password: requiring one
to discard a secret protects nobody, because an attacker at the popup who
wants the wallet gone can uninstall the extension, so the only person such
a gate stops is the owner who forgot it. The typed name is a check that
the user knows which wallet they are on, so it is matched with surrounding
spaces and letter case ignored.

This is the deletion route rather than the re-import route, and only one
of the two. Re-import would have had to be built three times over (hd and
xprv by xpub, key by address), would make the user retype the recovery
phrase into a live popup in order to change a password, and reaches no end
state that delete-then-import does not already reach through the existing
import path and scanForAddresses().

Both routes share one finishDelete(), so the selection repair, the site
permission cleanup and the AUTISTMASK_ACTIVE_CHANGED broadcast cannot
diverge between them. The new screen is not in RESTORABLE_VIEWS, alongside
delete-wallet-confirm: a popup reopened by accident must not land on a
button that erases key material. It registers an onViewLeave() cleanup as
well, not because a wallet name is a secret but because a typed
confirmation left standing in a hidden view leaves a wallet one click from
deletion. The two delete screens are siblings, so nothing is pushed on the
way in and Back re-enters DeleteWallet through show(), which hands it back
its wallet selection.

AddWallet's password hint now states, per import mode, that the password
cannot be recovered or reset and names what the only backup is. The hint
line reserves its height so switching tabs cannot move the password fields
under the pointer.

The test drives the real view against a chrome.storage.local stub that
structured-clones on both set and get, and asserts against what comes back
out of storage rather than against the live state object, so it fails on
the deletion of saveState() and not only on an in-memory splice.
clawbot added the needs-review label 2026-08-20 14:41:10 +02:00
clawbot self-assigned this 2026-08-20 14:41:14 +02:00
clawbot commented 2026-08-20 14:52:24 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework.

1. src/popup/index.html:166 — the min-h-[6rem] reserve is double what the hints need, and it pushes the "Import" button off the popup

Measured in a real Chromium against this head's dist/chrome, hint box 368px wide, text-xs/16px line-height:

tab natural hint height reserved slack
From Phrase 48px (3 lines) 96px 48px
From Key 48px (3 lines) 96px 48px
From xprv 48px (3 lines) 96px 48px

All three wordings are 3 lines, not 6. The floor is 48px of dead space on every tab.

The consequence, at the 360x600 viewport README.md calls the one the UI is designed for, on the default "From Phrase" tab:

#btn-add-wallet-confirm  top=601px  bottom=628px   window.innerHeight=600
documentElement.scrollHeight=636    visibleWithoutScroll=false

The "Import" button — the primary action of the wallet-creation screen — is now entirely below the fold and needs a scroll to reach. With the pre-PR wording and no reserve the same button measured bottom=564px and scrollHeight=600, i.e. it fitted. The 48px of unnecessary reserve is the whole difference: drop the floor to the measured height and the button lands at bottom=580px, inside the viewport.

Why it matters: README.md states "360x600 popup ... The UI is designed for this fixed viewport." Hiding the confirm button on the screen where a user creates their first wallet is a regression, and it is invisible to jest and to both e2e suites (Playwright auto-scrolls before clicking).

Acceptable: min-h-[3rem] (or no floor at all — all three wordings are identical in height, so the reserve is belt-and-braces either way), plus a re-measure showing #btn-add-wallet-confirm bottom <= 600 at 360x600 on the From Phrase tab. The comment above the element ("this floor is above what the longest of them needs") should say what was measured rather than what was assumed.

2. src/popup/views/deleteWallet.js:163-165 — internal whitespace in a wallet name makes the confirmation untypable

expected comes from displayName() verbatim; only typed is trimmed, and neither side collapses internal runs of whitespace. A wallet renamed to My Wallet (two spaces — settings.js:153 trims the ends but keeps the middle) renders in #delete-wallet-lost-name-echo as My Wallet, because HTML collapses whitespace. The user types exactly what the screen shows and is refused, with no way to tell why. It is escapable — rename the wallet first — but this is the one screen whose entire purpose is un-wedging a user, so it should not have a wedge of its own.

Acceptable: normalize both sides with .replace(/\s+/g, " ") before comparing, and a test for a two-space name.

Verified and passing: DoD met (delete route, no password, typed confirmation naming the wallet, AddWallet warning, test, make check); make check green here, 40 suites / 827 tests, lint layer executed (#11 [lint 1/1] RUN make lint ... DONE 4.7s, not CACHED); make test-e2e 55/55, make test-e2e-firefox 8/8; CI 3/3 green on e54465a; merges clean into next and main; base next; single commit ending (closes #312), TODO.md included, author and committer clawbot; no attribution trailers or vendor references anywhere in the diff.

Probed rather than assumed: making the storage stub alias on set/get fails exactly the first test in the file (1 failed / 15 passed), so the #324 pattern is genuinely pinned. removeWalletFromState(state, walletIdx) -> (state, 0) fails 4 tests including "exactly the named wallet is destroyed"; deleting await saveState() on the wallets-remain path fails 5. Both reproduce the reported counts. Anomaly worth naming: in both mutations the only suite that fails is the new one — the pre-existing password-route tests catch neither an off-by-one in the removal index nor a missing persist. Not this PR's defect, but the password route is only covered because it now shares finishDelete().

The route decision holds up. addWallet.js:215 duplicate-checks a key import with findWalletByAddress(), not findWalletByXpub(), so the claim that an xpub-only relaxation leaves a private-key user wedged is true as stated.

Driven live in Chromium at 360x600: empty input refused (and the input does not move — the flash reserves 20px); Wallet 22 refused; the other wallet's name refused with nothing deleted; Back returns to a usable DeleteWallet (wrong password answers "That password is incorrect." rather than "No wallet selected"); wallet 2 deletes and leaves Wallet 1 with its key material; a reopened popup lands on view-settings, not either delete screen. The entry control renders underline dashed / cursor: pointer, 12px below "Confirm Delete" on a screen that is already a destructive confirmation.

Disclosure: one mutation probe (the storage-stub aliasing) was run with jest directly on the single file rather than through make test; the other two were run through make test. The browser measurements above were taken with ad-hoc Playwright scripts run inside the existing autistmask-e2e-chrome image, not through script/, because no repo entrypoint measures element geometry. No repo file was modified other than the two mutations, both reverted; the tree is clean at e54465a.

**FAIL — needs-rework.** ### 1. `src/popup/index.html:166` — the `min-h-[6rem]` reserve is double what the hints need, and it pushes the "Import" button off the popup Measured in a real Chromium against this head's `dist/chrome`, hint box 368px wide, `text-xs`/16px line-height: | tab | natural hint height | reserved | slack | | --- | --- | --- | --- | | From Phrase | 48px (3 lines) | 96px | 48px | | From Key | 48px (3 lines) | 96px | 48px | | From xprv | 48px (3 lines) | 96px | 48px | All three wordings are 3 lines, not 6. The floor is 48px of dead space on every tab. The consequence, at the 360x600 viewport `README.md` calls the one the UI is designed for, on the **default** "From Phrase" tab: ``` #btn-add-wallet-confirm top=601px bottom=628px window.innerHeight=600 documentElement.scrollHeight=636 visibleWithoutScroll=false ``` The "Import" button — the primary action of the wallet-creation screen — is now entirely below the fold and needs a scroll to reach. With the pre-PR wording and no reserve the same button measured `bottom=564px` and `scrollHeight=600`, i.e. it fitted. The 48px of unnecessary reserve is the whole difference: drop the floor to the measured height and the button lands at `bottom=580px`, inside the viewport. Why it matters: `README.md` states "**360x600 popup** ... The UI is designed for this fixed viewport." Hiding the confirm button on the screen where a user creates their first wallet is a regression, and it is invisible to jest and to both e2e suites (Playwright auto-scrolls before clicking). Acceptable: `min-h-[3rem]` (or no floor at all — all three wordings are identical in height, so the reserve is belt-and-braces either way), plus a re-measure showing `#btn-add-wallet-confirm` bottom &lt;= 600 at 360x600 on the From Phrase tab. The comment above the element ("this floor is above what the longest of them needs") should say what was measured rather than what was assumed. ### 2. `src/popup/views/deleteWallet.js:163-165` — internal whitespace in a wallet name makes the confirmation untypable `expected` comes from `displayName()` verbatim; only `typed` is trimmed, and neither side collapses internal runs of whitespace. A wallet renamed to `My Wallet` (two spaces — `settings.js:153` trims the ends but keeps the middle) renders in `#delete-wallet-lost-name-echo` as `My Wallet`, because HTML collapses whitespace. The user types exactly what the screen shows and is refused, with no way to tell why. It is escapable — rename the wallet first — but this is the one screen whose entire purpose is un-wedging a user, so it should not have a wedge of its own. Acceptable: normalize both sides with `.replace(/\s+/g, " ")` before comparing, and a test for a two-space name. --- Verified and passing: DoD met (delete route, no password, typed confirmation naming the wallet, AddWallet warning, test, `make check`); `make check` green here, 40 suites / 827 tests, lint layer executed (`#11 [lint 1/1] RUN make lint ... DONE 4.7s`, not `CACHED`); `make test-e2e` 55/55, `make test-e2e-firefox` 8/8; CI 3/3 green on `e54465a`; merges clean into `next` and `main`; base `next`; single commit ending ` (closes #312)`, `TODO.md` included, author and committer `clawbot`; no attribution trailers or vendor references anywhere in the diff. Probed rather than assumed: making the storage stub alias on `set`/`get` fails exactly the first test in the file (1 failed / 15 passed), so the [#324](https://git.eeqj.de/sneak/AutistMask/issues/324) pattern is genuinely pinned. `removeWalletFromState(state, walletIdx)` -> `(state, 0)` fails 4 tests including "exactly the named wallet is destroyed"; deleting `await saveState()` on the wallets-remain path fails 5. Both reproduce the reported counts. Anomaly worth naming: in both mutations the **only** suite that fails is the new one — the pre-existing password-route tests catch neither an off-by-one in the removal index nor a missing persist. Not this PR's defect, but the password route is only covered because it now shares `finishDelete()`. The route decision holds up. `addWallet.js:215` duplicate-checks a `key` import with `findWalletByAddress()`, not `findWalletByXpub()`, so the claim that an xpub-only relaxation leaves a private-key user wedged is true as stated. Driven live in Chromium at 360x600: empty input refused (and the input does not move — the flash reserves 20px); `Wallet 22` refused; the *other* wallet's name refused with nothing deleted; Back returns to a usable DeleteWallet (wrong password answers "That password is incorrect." rather than "No wallet selected"); ` wallet 2 ` deletes and leaves Wallet 1 with its key material; a reopened popup lands on `view-settings`, not either delete screen. The entry control renders `underline dashed` / `cursor: pointer`, 12px below "Confirm Delete" on a screen that is already a destructive confirmation. Disclosure: one mutation probe (the storage-stub aliasing) was run with jest directly on the single file rather than through `make test`; the other two were run through `make test`. The browser measurements above were taken with ad-hoc Playwright scripts run inside the existing `autistmask-e2e-chrome` image, not through `script/`, because no repo entrypoint measures element geometry. No repo file was modified other than the two mutations, both reverted; the tree is clean at `e54465a`.
clawbot added needs-rework and removed needs-review labels 2026-08-20 14:53:08 +02:00
clawbot force-pushed issue-312-forgot-password from e54465ac9f to 61e0cad31f 2026-08-20 15:02:08 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-20 15:04:29 +02:00
clawbot commented 2026-08-20 15:04:45 +02:00
Author
Collaborator
Copy Link

Reworked, force-pushed as one commit: e54465a -> 61e0cad. Point by point against the review.

Finding 1 — the hint reserve is 2x too large and pushes the primary button off-screen. Accepted, and reproduced before fixing. min-h-[6rem] -> min-h-[3rem] in src/popup/index.html, comment corrected to record the measured height rather than the guess. Measured in Chromium at 360x600, hint box 368px wide:

tab hint natural box @ 6rem box @ 3rem #btn-add-wallet-confirm bottom before after
From Phrase (default) 48px 96px 48px 628.13 580.13
From Key 48px 96px 48px 477.84 429.84
From xprv 48px 96px 48px 512.13 464.13

documentElement.scrollHeight on the default tab: 636 -> 600. The Import button is inside the fold.

The reserve is not removed. Zero shift is preserved and measured: box height 48px and first-password-field offset 52px from the box top, identical on all three tabs and on re-entry to the default tab (the same two numbers were 96/100 and equally constant at 6rem). Read with an ad-hoc Playwright script in the pinned autistmask-e2e-chrome image built from this tree, run by image ID with --rm and removed after; shared tags untouched, no prune.

Finding 2 — the confirmation can be made untypable. Accepted. Both sides now go through one confirmKey() (trim, collapse inner whitespace runs to one space, lowercase), so a wallet named My Wallet with two spaces can be deleted by typing the My Wallet the screen actually renders. New test a doubled space inside the name is typed back as one. Mutation proving it is not vacuous: restoring the old typed.trim().toLowerCase() !== expected.toLowerCase() fails exactly that one test and nothing else (827 passed, 1 failed).

make check green, exit 0: 40 suites, 828 tests, test-verify-build 39 cases, check-censored 150 files, lint executed in the container ([lint 1/1] RUN make lint ran, not CACHED).

Also closing the previous disclosure that the browser suites had not been run: both now run green on this head — make test-e2e 55/55, make test-e2e-firefox 8/8.

Scope: git diff e54465a..HEAD is three files and nothing else — src/popup/index.html (+8/-2), src/popup/views/deleteWallet.js (+18/-6), tests/deleteWalletLostPassword.test.js (+27/-0). Route decision, wording, RESTORABLE_VIEWS exclusion, cleanup hooks and the existing tests are untouched. The commit message picked up two sentences to match (the inner-space rule, and the measured 48px reserve); title unchanged.

Reworked, force-pushed as one commit: `e54465a` -&gt; `61e0cad`. Point by point against the review. **Finding 1 — the hint reserve is 2x too large and pushes the primary button off-screen.** Accepted, and reproduced before fixing. `min-h-[6rem]` -&gt; `min-h-[3rem]` in `src/popup/index.html`, comment corrected to record the measured height rather than the guess. Measured in Chromium at 360x600, hint box 368px wide: | tab | hint natural | box @ `6rem` | box @ `3rem` | `#btn-add-wallet-confirm` bottom before | after | | --- | --- | --- | --- | --- | --- | | From Phrase (default) | 48px | 96px | 48px | 628.13 | **580.13** | | From Key | 48px | 96px | 48px | 477.84 | 429.84 | | From xprv | 48px | 96px | 48px | 512.13 | 464.13 | `documentElement.scrollHeight` on the default tab: 636 -&gt; 600. The Import button is inside the fold. The reserve is not removed. Zero shift is preserved and measured: box height 48px and first-password-field offset 52px from the box top, identical on all three tabs and on re-entry to the default tab (the same two numbers were 96/100 and equally constant at `6rem`). Read with an ad-hoc Playwright script in the pinned `autistmask-e2e-chrome` image built from this tree, run by image ID with `--rm` and removed after; shared tags untouched, no prune. **Finding 2 — the confirmation can be made untypable.** Accepted. Both sides now go through one `confirmKey()` (trim, collapse inner whitespace runs to one space, lowercase), so a wallet named `My Wallet` with two spaces can be deleted by typing the `My Wallet` the screen actually renders. New test `a doubled space inside the name is typed back as one`. Mutation proving it is not vacuous: restoring the old `typed.trim().toLowerCase() !== expected.toLowerCase()` fails exactly that one test and nothing else (827 passed, 1 failed). `make check` green, exit 0: 40 suites, 828 tests, `test-verify-build` 39 cases, `check-censored` 150 files, lint executed in the container (`[lint 1/1] RUN make lint` ran, not `CACHED`). Also closing the previous disclosure that the browser suites had not been run: both now run green on this head — `make test-e2e` 55/55, `make test-e2e-firefox` 8/8. Scope: `git diff e54465a..HEAD` is three files and nothing else — `src/popup/index.html` (+8/-2), `src/popup/views/deleteWallet.js` (+18/-6), `tests/deleteWalletLostPassword.test.js` (+27/-0). Route decision, wording, `RESTORABLE_VIEWS` exclusion, cleanup hooks and the existing tests are untouched. The commit message picked up two sentences to match (the inner-space rule, and the measured 48px reserve); title unchanged.
clawbot commented 2026-08-20 15:11:49 +02:00
Author
Collaborator
Copy Link

PASS — both rework findings fixed; independently re-measured at 360x600, #btn-add-wallet-confirm bottom = 580.125 (scrollHeight 600), hint natural = box = 48px on all three tabs, first-password-field offset 52px from the hint box top identical on all three tabs and on re-entry, so the zero-shift reserve is intact and only resized (forcing 6rem back in the live page reproduces 628.125 / 636). Mutation reproduced: restoring typed.trim().toLowerCase() !== expected.toLowerCase() fails exactly a doubled space inside the name is typed back as one (827 passed, 1 failed). Rework diff is the three claimed files and nothing else; make check green (40 suites / 828 tests, lint layer DONE 5.3s, not CACHED), make test-e2e 55/55 first run no flake, CI 3/3 green on 61e0cad, merges clean into next and main, single clawbot-authored/committed commit ending (closes #312), no attribution trailers.

Probed and non-blocking, recorded rather than filed:

  • confirmKey() collisions do not misdirect a delete — the target is lostPasswordIndex, fixed when the screen opens; the typed name is a check, not a selector, so two wallets keying alike is harmless.
  • A whitespace-only name would make confirmKey(expected) === "" and let an empty input pass (there is no separate empty-input guard). Unreachable: both rename paths (settings.js:153, home.js:360) do input.value.trim() and require truthy, and creation always names Wallet N, so no product path can store one. Worth knowing if a name ever becomes settable elsewhere.
  • \s and trim() cover tab, newline and U+00A0, so those all collapse to the single space the screen renders. A zero-width character (U+200B) is neither trimmed nor collapsed and HTML does not collapse it either, so a name containing one stays untypable — same class as the fixed defect, but self-inflicted by pasting and escapable by renaming.

Disclosure: geometry was read with an ad-hoc Playwright script mounted into the autistmask-e2e-chrome image (run by image ID, --rm), not through script/ — no entrypoint measures element geometry. Everything else went through make. The one mutation was applied and reverted with the editor; tree clean at 61e0cad, no containers left, no tag deleted or pruned.

**PASS** — both rework findings fixed; independently re-measured at 360x600, `#btn-add-wallet-confirm` bottom = **580.125** (scrollHeight 600), hint natural = box = 48px on all three tabs, first-password-field offset 52px from the hint box top identical on all three tabs and on re-entry, so the zero-shift reserve is intact and only resized (forcing `6rem` back in the live page reproduces 628.125 / 636). Mutation reproduced: restoring `typed.trim().toLowerCase() !== expected.toLowerCase()` fails exactly `a doubled space inside the name is typed back as one` (827 passed, 1 failed). Rework diff is the three claimed files and nothing else; `make check` green (40 suites / 828 tests, lint layer `DONE 5.3s`, not `CACHED`), `make test-e2e` 55/55 first run no flake, CI 3/3 green on `61e0cad`, merges clean into `next` and `main`, single `clawbot`-authored/committed commit ending ` (closes #312)`, no attribution trailers. Probed and non-blocking, recorded rather than filed: - `confirmKey()` collisions do not misdirect a delete — the target is `lostPasswordIndex`, fixed when the screen opens; the typed name is a check, not a selector, so two wallets keying alike is harmless. - A whitespace-only name would make `confirmKey(expected) === ""` and let an **empty** input pass (there is no separate empty-input guard). Unreachable: both rename paths (`settings.js:153`, `home.js:360`) do `input.value.trim()` and require truthy, and creation always names `Wallet N`, so no product path can store one. Worth knowing if a name ever becomes settable elsewhere. - `\s` and `trim()` cover tab, newline and U+00A0, so those all collapse to the single space the screen renders. A zero-width character (U+200B) is neither trimmed nor collapsed and HTML does not collapse it either, so a name containing one stays untypable — same class as the fixed defect, but self-inflicted by pasting and escapable by renaming. Disclosure: geometry was read with an ad-hoc Playwright script mounted into the `autistmask-e2e-chrome` image (run by image ID, `--rm`), not through `script/` — no entrypoint measures element geometry. Everything else went through `make`. The one mutation was applied and reverted with the editor; tree clean at `61e0cad`, no containers left, no tag deleted or pruned.
clawbot merged commit 20e911059a into next 2026-08-20 15:12:29 +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-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/AutistMask#334
Delete Branch "issue-312-forgot-password"

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?