From 5f54fcbb24b287b8aa4b431496f0fbac07855947 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 04:58:38 +0200 Subject: [PATCH] harden: end a site's unremembered connection when its address or wallet is removed (closes #245) A site connected without "Remember" lives only in the background's in-memory connectedSites map. Removing an address or deleting a wallet dropped the remembered permissions but never told the background; the entry went only as a side effect of the accountsChanged broadcast, which empties the whole map when the active address changes. dropSitePermissions(), shared by both removal paths, now sends AUTISTMASK_ADDRESSES_REMOVED with the removed addresses, and the background deletes their entries. Only the extension's own pages may send it. Model: opus-5-5 --- README.md | 8 +++ TODO.md | 11 +++ src/background/index.js | 15 ++++ src/shared/walletDelete.js | 5 +- tests/backgroundApproval.test.js | 96 ++++++++++++++++++++++++++ tests/deleteWalletLostPassword.test.js | 9 ++- 6 files changed, 141 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index d514eb4..c7cf35c 100644 --- a/README.md +++ b/README.md @@ -1646,6 +1646,10 @@ view would leave a wallet one click from deletion. - Either way, the active address moves only if it belonged to the deleted wallet, and `AUTISTMASK_ACTIVE_CHANGED` is broadcast when it does (`src/shared/walletDelete.js`) + - Either way, every address the wallet held loses its site permissions of + both kinds: the remembered ones in storage, and the connections approved + without "Remember", which only the background holds, in memory, and drops + on `AUTISTMASK_ADDRESSES_REMOVED` - "Confirm Delete" (wrong password) → "That password is incorrect. Please try again." on the error line, nothing deleted - "I have lost my password" → **DeleteWalletLostPassword** @@ -1742,6 +1746,10 @@ view would leave a wallet one click from deletion. so a connected site stops being told about an address the user removed (`src/shared/walletDelete.js`). A selection in any other wallet is left alone; one in this wallet follows the splice. +- The address loses its site permissions of both kinds, whether or not it was + the active one: the remembered ones in storage, and any connection approved + without "Remember", which only the background holds, in memory, and drops on + `AUTISTMASK_ADDRESSES_REMOVED`. - The wallet's derivation counter (`nextIndex`) is not rewound, so "+" derives a fresh address rather than handing back the one just removed. diff --git a/TODO.md b/TODO.md index da95389..f06c9c2 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,17 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-04: Removing an address or deleting a wallet ends every site + connection approved without "Remember" for the addresses removed + ([#245](https://git.eeqj.de/sneak/AutistMask/issues/245)). Such a connection + lives only in the background's in-memory `connectedSites` map. Both removal + paths dropped the remembered `allowedSites`/`deniedSites` entries, but nothing + told the background, so its entry was cleared only as a side effect: every + change of active address empties the whole map, and removing the active + address changes it. `dropSitePermissions()` in `src/shared/walletDelete.js`, + shared by both paths, now also sends `AUTISTMASK_ADDRESSES_REMOVED` with the + removed addresses, and the background deletes their `connectedSites` entries; + only the extension's own pages may send it. - 2026-10-03: The typed-data signing screen warns for a token permission, and names the primary type ethers signs ([#400](https://git.eeqj.de/sneak/AutistMask/issues/400)). A Permit or Permit2 diff --git a/src/background/index.js b/src/background/index.js index 6668a4c..8087341 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -1305,6 +1305,7 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { "AUTISTMASK_GET_APPROVAL", "AUTISTMASK_TX_RESPONSE", "AUTISTMASK_SIGN_RESPONSE", + "AUTISTMASK_ADDRESSES_REMOVED", ]; if (POPUP_ONLY_TYPES.includes(msg.type) && !isExtensionSender(sender)) { sendResponse({ error: "Unauthorized sender" }); @@ -1647,6 +1648,20 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { return false; } + // The popup removed these addresses, so no site stays connected to them. + // A connectedSites key is origin + ":" + address, and an origin can carry + // a port, so the address is what follows the last colon. + if (msg.type === "AUTISTMASK_ADDRESSES_REMOVED") { + const removed = Array.isArray(msg.addresses) ? msg.addresses : []; + for (const key of Object.keys(connectedSites)) { + const address = key.slice(key.lastIndexOf(":") + 1); + if (removed.some((a) => sameAddress(a, address))) { + delete connectedSites[key]; + } + } + return false; + } + if (msg.type === "AUTISTMASK_REMOVE_SITE") { // Popup already saved state; nothing else needed return false; diff --git a/src/shared/walletDelete.js b/src/shared/walletDelete.js index 1be1983..7911005 100644 --- a/src/shared/walletDelete.js +++ b/src/shared/walletDelete.js @@ -12,12 +12,15 @@ function sameAddress(a, b) { return String(a).toLowerCase() === String(b).toLowerCase(); } -// Forget every site permission held against the given addresses. +// Forget every site permission held against the given addresses: the +// remembered ones in `state`, and the connections approved without +// "Remember", which only the background holds, in memory. function dropSitePermissions(state, addresses) { for (const addr of addresses) { delete state.allowedSites[addr]; delete state.deniedSites[addr]; } + notify({ type: "AUTISTMASK_ADDRESSES_REMOVED", addresses }); } // Remove wallet `walletIdx` from `state` and repair the derived state. diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index e94f44e..18c7375 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -25,6 +25,10 @@ const { Network, Wallet } = require("ethers"); // what the user is actually shown. const { describeSigningFailure } = require("../src/shared/approvalVerify"); const { makeStorageStub } = require("./support/storageStub"); +const { + removeAddressFromState, + removeWalletFromState, +} = require("../src/shared/walletDelete"); const SIGNER_KEY = "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d"; @@ -2096,3 +2100,95 @@ describe("a site connection decided as the popup closes", () => { }); }); }); + +// A site connected without "Remember" is held only in the background's memory, +// keyed to the address it was connected to. Removing that address, or the +// wallet holding it, must end the connection as part of the removal itself. +// The accountsChanged broadcast the views send afterwards also clears it, but +// only when the active address moved, and nothing waits for it to arrive. +// +// Each test asks the background while storage still names the removed address +// as active, because the popup has not saved yet, so the answer turns on the +// connection alone. +describe("removing an address ends a site's connection to it", () => { + // FRESH_ORIGIN connected to the active address without "Remember", in a + // wallet holding a second address so that one can be removed at all. The + // popup's messages reach the background as they would from the popup. + async function connectedBackground() { + const bg = loadBackground({ actionPopup: true }); + const stored = bg.storage.read("autistmask"); + stored.wallets[0].addresses.push({ + address: other.address, + balance: "0", + tokenBalances: [], + }); + bg.storage.write("autistmask", stored); + + const pending = bg.requestSite(); + await settle(); + bg.connectApproval(pending.id()).decide(true, false); + await settle(); + expect(pending.result()).toEqual({ result: [signer.address] }); + + global.chrome.runtime.sendMessage = (msg) => { + bg.send(msg, bg.fromPopup); + }; + return bg; + } + + // What FRESH_ORIGIN is told when it asks which account it may use. + async function siteAccounts(bg) { + const { sendResponse } = bg.send( + { type: "AUTISTMASK_RPC", method: "eth_accounts", params: [] }, + { origin: FRESH_ORIGIN }, + ); + await settle(); + return sendResponse.mock.calls[0][0]; + } + + test("removing the connected address ends the connection", async () => { + const bg = await connectedBackground(); + expect(await siteAccounts(bg)).toEqual({ result: [signer.address] }); + + const popupState = bg.storage.read("autistmask"); + expect(removeAddressFromState(popupState, 0, 0).removed).toBe(true); + + expect(await siteAccounts(bg)).toEqual({ result: [] }); + }); + + test("deleting the wallet holding the connected address ends the connection", async () => { + const bg = await connectedBackground(); + expect(await siteAccounts(bg)).toEqual({ result: [signer.address] }); + + const popupState = bg.storage.read("autistmask"); + removeWalletFromState(popupState, 0); + + expect(await siteAccounts(bg)).toEqual({ result: [] }); + }); + + test("removing a different address leaves the connection alone", async () => { + const bg = await connectedBackground(); + + const popupState = bg.storage.read("autistmask"); + expect(removeAddressFromState(popupState, 0, 1).removed).toBe(true); + + expect(await siteAccounts(bg)).toEqual({ result: [signer.address] }); + }); + + test("a page cannot end the connection", async () => { + const bg = await connectedBackground(); + + const spoof = bg.send( + { + type: "AUTISTMASK_ADDRESSES_REMOVED", + addresses: [signer.address], + }, + { url: FRESH_ORIGIN + "/index.html" }, + ); + + expect(spoof.sendResponse).toHaveBeenCalledWith({ + error: "Unauthorized sender", + }); + expect(await siteAccounts(bg)).toEqual({ result: [signer.address] }); + }); +}); diff --git a/tests/deleteWalletLostPassword.test.js b/tests/deleteWalletLostPassword.test.js index 6f2915a..7c3edf0 100644 --- a/tests/deleteWalletLostPassword.test.js +++ b/tests/deleteWalletLostPassword.test.js @@ -407,7 +407,9 @@ describe("deleting without the password", () => { expect(saved.activeAddress).toBe(A0); expect(saved.selectedWallet).toBe(0); expect(saved.selectedAddress).toBe(0); - expect(sent).toEqual([]); + expect(sent).toEqual([ + { type: "AUTISTMASK_ADDRESSES_REMOVED", addresses: [B0] }, + ]); // Settings is stubbed, so this is where the route hands over, not // where it renders. expect(mockSettingsShow).toHaveBeenCalled(); @@ -426,7 +428,10 @@ describe("deleting without the password", () => { "Wallet 3", ]); expect(saved.activeAddress).toBe(B0); - expect(sent).toEqual([{ type: "AUTISTMASK_ACTIVE_CHANGED" }]); + expect(sent).toEqual([ + { type: "AUTISTMASK_ADDRESSES_REMOVED", addresses: [A0, A1] }, + { type: "AUTISTMASK_ACTIVE_CHANGED" }, + ]); }); test("deleting the last wallet lands on Welcome with nothing left", async () => {