fix: give a wallet whose password is lost a way out, and say the password cannot be reset (closes #312)
A user who forgot their password but held their recovery phrase was permanently locked out: deletion was password-gated and re-importing the phrase was refused as a duplicate. Their only escape was destroying extension storage through browser internals, taking every other wallet with it. DeleteWallet gains an "I have lost my password" route that destroys the stored secret after the wallet's name is typed back. No password gate was added: requiring one to discard a secret protects nothing, since an attacker who wants destruction can uninstall the extension, and the only person it stops is the legitimate user who lost it. The screen is excluded from RESTORABLE_VIEWS and registers an onViewLeave cleanup. Deletion was chosen over re-import because a key wallet is duplicate-checked by address rather than xpub, so an xpub-only relaxation would leave that user still wedged; because re-import makes the user retype their recovery phrase into a live popup merely to change a password; and because it reaches no end state that delete-then-import plus scanForAddresses() does not. The attacker argument did not decide it — re-import clears the "no worse than the phrase alone" bar. All three AddWallet password hints now state the password cannot be recovered or reset and name that mode's only backup, the xprv mode correctly claiming no recovery phrase. deleteAddress.js no longer tells the user that deleting a wallet asks for a password, which this change made false. The typed confirmation collapses internal whitespace on both sides: a wallet renamed with two spaces displays with one, so the string a user could see and type could never match, making the confirmation untypable on the one screen whose purpose is un-wedging a stuck user. Measured, not reasoned, after review found the first reserve twice too large and pushing the Import button below the fold: #btn-add-wallet-confirm bottom 628.13 -> 580.13 at 360x600, scrollHeight 636 -> 600, hint box 48px identical across all three tabs and on re-entry. make check 40 suites / 828 tests, test-e2e 55/55, test-e2e-firefox 8/8.
This commit was merged in pull request #334.
This commit is contained in:
@@ -14,8 +14,29 @@ const {
|
||||
} = require("../../shared/walletDelete");
|
||||
|
||||
let deleteWalletIndex = null;
|
||||
let lostPasswordIndex = null;
|
||||
let ctx = null;
|
||||
|
||||
// The name shown for a wallet, and on the lost-password screen the string
|
||||
// the user has to type back. One function so the two cannot disagree: a
|
||||
// confirmation that asks for a name other than the one on screen is
|
||||
// unusable.
|
||||
function displayName(walletIdx) {
|
||||
const wallet = state.wallets[walletIdx];
|
||||
return (wallet && wallet.name) || "Wallet " + (walletIdx + 1);
|
||||
}
|
||||
|
||||
// What the typed confirmation and the wallet name are compared as. HTML
|
||||
// collapses runs of whitespace when it renders the name, so a wallet named
|
||||
// "My Wallet" with two spaces DISPLAYS as "My Wallet": the user cannot
|
||||
// see the second space and cannot type a string that matches the stored
|
||||
// name. Comparing collapsed on both sides is what keeps the confirmation
|
||||
// satisfiable, on the one screen whose whole purpose is unwedging a user
|
||||
// who is already stuck. Case and surrounding space go the same way.
|
||||
function confirmKey(name) {
|
||||
return name.trim().replace(/\s+/g, " ").toLowerCase();
|
||||
}
|
||||
|
||||
// Drop the password from the DOM and the wallet selection from the
|
||||
// closure. Registered as the view-leave handler as well as run on entry,
|
||||
// so the typed password does not sit in the hidden view after the user
|
||||
@@ -27,19 +48,89 @@ function clear() {
|
||||
$("delete-wallet-flash").style.visibility = "hidden";
|
||||
}
|
||||
|
||||
// The lost-password screen holds no secret — a wallet name is not one —
|
||||
// but it is wiped on leave for the neighbouring reason: a typed
|
||||
// confirmation left standing in a hidden view is one click away from
|
||||
// destroying a wallet the user has since navigated off. The button is
|
||||
// re-enabled here too, so a screen left mid-delete is usable on re-entry.
|
||||
function clearLostPassword() {
|
||||
lostPasswordIndex = null;
|
||||
$("delete-wallet-lost-name-input").value = "";
|
||||
$("delete-wallet-lost-flash").textContent = "";
|
||||
$("delete-wallet-lost-flash").style.visibility = "hidden";
|
||||
const btn = $("btn-delete-wallet-lost-confirm");
|
||||
btn.disabled = false;
|
||||
btn.classList.remove("text-muted");
|
||||
}
|
||||
|
||||
function show(walletIdx) {
|
||||
clear();
|
||||
deleteWalletIndex = walletIdx;
|
||||
const wallet = state.wallets[walletIdx];
|
||||
$("delete-wallet-name").textContent =
|
||||
wallet.name || "Wallet " + (walletIdx + 1);
|
||||
$("delete-wallet-name").textContent = displayName(walletIdx);
|
||||
showView("delete-wallet-confirm");
|
||||
}
|
||||
|
||||
// The two delete screens are siblings, not parent and child: nothing is
|
||||
// pushed on the way here, and Back goes to show() rather than goBack().
|
||||
// Both then have the same Back target — Settings, the screen that pushed
|
||||
// delete-wallet-confirm — and re-entering through show() hands the confirm
|
||||
// screen its wallet selection back, which a bare goBack() onto a view
|
||||
// whose leave hook has already nulled that selection would not.
|
||||
function showLostPassword() {
|
||||
const walletIdx = deleteWalletIndex;
|
||||
if (walletIdx === null) {
|
||||
goBack();
|
||||
return;
|
||||
}
|
||||
const name = displayName(walletIdx);
|
||||
clearLostPassword();
|
||||
$("delete-wallet-lost-name").textContent = name;
|
||||
$("delete-wallet-lost-name-echo").textContent = name;
|
||||
// showView() runs the leave hook of delete-wallet-confirm, which nulls
|
||||
// deleteWalletIndex, so this screen's own selection is recorded after
|
||||
// it and not before.
|
||||
showView("delete-wallet-lost-password");
|
||||
lostPasswordIndex = walletIdx;
|
||||
}
|
||||
|
||||
// Remove the wallet and put the user somewhere sensible. Shared by both
|
||||
// routes onto this screen, so the selection repair, the site-permission
|
||||
// cleanup and the accountsChanged broadcast cannot drift apart between
|
||||
// them.
|
||||
async function finishDelete(walletIdx) {
|
||||
const { activeAddressChanged } = removeWalletFromState(state, walletIdx);
|
||||
|
||||
deleteWalletIndex = null;
|
||||
lostPasswordIndex = null;
|
||||
|
||||
if (!state.hasWallet) {
|
||||
clearViewStack();
|
||||
await saveState();
|
||||
// Save before broadcasting: the background reads the active
|
||||
// address back out of storage to build accountsChanged.
|
||||
if (activeAddressChanged) broadcastActiveChanged();
|
||||
showView("welcome");
|
||||
return;
|
||||
}
|
||||
|
||||
await saveState();
|
||||
if (activeAddressChanged) broadcastActiveChanged();
|
||||
// Reset stack to [main] so Settings back goes home.
|
||||
// Use require() lazily to avoid circular dependency
|
||||
// (settings.js requires deleteWallet.js).
|
||||
clearViewStack();
|
||||
state.viewStack.push("main");
|
||||
ctx.renderWalletList();
|
||||
const settings = require("./settings");
|
||||
settings.show();
|
||||
showFlash("Wallet deleted.");
|
||||
}
|
||||
|
||||
function init(_ctx) {
|
||||
ctx = _ctx;
|
||||
|
||||
onViewLeave("delete-wallet-confirm", clear);
|
||||
onViewLeave("delete-wallet-lost-password", clearLostPassword);
|
||||
|
||||
// No wipe here: goBack() routes through showView(), which runs the
|
||||
// leave hook.
|
||||
@@ -47,6 +138,60 @@ function init(_ctx) {
|
||||
goBack();
|
||||
});
|
||||
|
||||
// The escape hatch, and deliberately not gated on anything a user who
|
||||
// has lost the password cannot produce. 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 — and before
|
||||
// this route existed that owner could neither delete the wallet nor
|
||||
// import its recovery phrase again, because AddWallet refuses the xpub
|
||||
// as a duplicate while the wallet is still stored.
|
||||
$("btn-delete-wallet-lost-password").addEventListener("click", () => {
|
||||
showLostPassword();
|
||||
});
|
||||
|
||||
$("btn-delete-wallet-lost-back").addEventListener("click", () => {
|
||||
const walletIdx = lostPasswordIndex;
|
||||
if (walletIdx === null) {
|
||||
goBack();
|
||||
return;
|
||||
}
|
||||
show(walletIdx);
|
||||
});
|
||||
|
||||
$("btn-delete-wallet-lost-confirm").addEventListener("click", async () => {
|
||||
if (lostPasswordIndex === null) {
|
||||
$("delete-wallet-lost-flash").textContent =
|
||||
"No wallet selected for deletion.";
|
||||
$("delete-wallet-lost-flash").style.visibility = "visible";
|
||||
return;
|
||||
}
|
||||
|
||||
// Case, surrounding spaces and repeated inner spaces are not part
|
||||
// of the confirmation; see confirmKey(). This asks whether the
|
||||
// user knows which wallet they are on; it is not a secret, and
|
||||
// refusing "wallet 2" for "Wallet 2" would only teach the user to
|
||||
// distrust the control.
|
||||
const typed = $("delete-wallet-lost-name-input").value;
|
||||
const expected = displayName(lostPasswordIndex);
|
||||
if (confirmKey(typed) !== confirmKey(expected)) {
|
||||
$("delete-wallet-lost-flash").textContent =
|
||||
"That is not the name of this wallet. Type " +
|
||||
expected +
|
||||
" to confirm.";
|
||||
$("delete-wallet-lost-flash").style.visibility = "visible";
|
||||
return;
|
||||
}
|
||||
|
||||
const btn = $("btn-delete-wallet-lost-confirm");
|
||||
btn.disabled = true;
|
||||
btn.classList.add("text-muted");
|
||||
|
||||
// finishDelete() navigates, and the leave hook re-enables the
|
||||
// button and wipes the typed name on the way out.
|
||||
await finishDelete(lostPasswordIndex);
|
||||
});
|
||||
|
||||
$("btn-delete-wallet-confirm").addEventListener("click", async () => {
|
||||
const pw = $("delete-wallet-password").value;
|
||||
if (!pw) {
|
||||
@@ -82,34 +227,7 @@ function init(_ctx) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Remove the wallet and repair selection, permissions and hasWallet
|
||||
const { activeAddressChanged } = removeWalletFromState(
|
||||
state,
|
||||
walletIdx,
|
||||
);
|
||||
|
||||
deleteWalletIndex = null;
|
||||
|
||||
if (!state.hasWallet) {
|
||||
clearViewStack();
|
||||
await saveState();
|
||||
// Save before broadcasting: the background reads the active
|
||||
// address back out of storage to build accountsChanged.
|
||||
if (activeAddressChanged) broadcastActiveChanged();
|
||||
showView("welcome");
|
||||
} else {
|
||||
await saveState();
|
||||
if (activeAddressChanged) broadcastActiveChanged();
|
||||
// Reset stack to [main] so Settings back goes home.
|
||||
// Use require() lazily to avoid circular dependency
|
||||
// (settings.js requires deleteWallet.js).
|
||||
clearViewStack();
|
||||
state.viewStack.push("main");
|
||||
ctx.renderWalletList();
|
||||
const settings = require("./settings");
|
||||
settings.show();
|
||||
showFlash("Wallet deleted.");
|
||||
}
|
||||
await finishDelete(walletIdx);
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user