diff --git a/README.md b/README.md index a4f6f98..96ee75f 100644 --- a/README.md +++ b/README.md @@ -571,9 +571,12 @@ the worker stopping. It starts Chromium itself, speaks the DevTools protocol over `--remote-debugging-pipe`, and attaches to pages only. Before raising the approvals it requires the worker to stop on its own with nothing pending, so a run in which something else keeps the worker running fails. The site is -connected and the RPC endpoint set through the extension's own screens, against -the loopback dApp page and stub node the Firefox suite also uses -(`tests/e2e/dapp.js`). Unlike the suite above, it does not fail on page errors. +connected, without "Remember my choice for this site", and the RPC endpoint set +through the extension's own screens, against the loopback dApp page and stub +node the Firefox suite also uses (`tests/e2e/dapp.js`). Once the worker has +stopped, the run requires the site still to be told the address +([#526](https://git.eeqj.de/sneak/AutistMask/issues/526)). Unlike the suite +above, it does not fail on page errors. ### Firefox (`make test-e2e-firefox`) @@ -857,7 +860,11 @@ on the next event. Two consequences shape every recurring job in the background: - Module-level variables do not survive either. Anything that must be remembered across a restart goes in extension storage. `localStorage` does not exist in a service worker at all — the one remaining user of it, `src/shared/ens.js`, - runs only in the popup and is marked as such. + runs only in the popup and is marked as such. The sites allowed without + "Remember my choice for this site" are kept in the browser's session storage + (`storage.session`), which outlives the worker but not the browser, so such a + connection lasts until it is ended or the browser closes. Firefox has session + storage from version 115, the minimum `manifest/firefox.json` declares. Pending approvals are the exception: an approval waiting for the user, and the site's request waiting on it, live only in the worker's memory, and only the @@ -1936,7 +1943,7 @@ view would leave a wallet one click from deletion. under any address, with remove buttons - Connected Sites: the origins of the sites allowed without "Remember my choice" that are still connected, with remove buttons. Only the background - holds these, in memory, and Settings asks it for them with + holds these, in session storage, and Settings asks it for them with `AUTISTMASK_GET_CONNECTED_SITES` - Denied Sites: the origins remembered as denied, under any address, with remove buttons @@ -1955,9 +1962,10 @@ view would leave a wallet one click from deletion. its origin is dropped from Allowed Sites under every address, and `AUTISTMASK_REMOVE_SITE` has the background end every connection approved without "Remember" from that origin, under any address, and send - `accountsChanged` with an empty list to the open tabs of that origin. The - same host under another scheme or port is another site and is left alone. - Only the extension's own pages may send either message + `accountsChanged` with an empty list to the open tabs of that origin. + Settings waits for the background's answer before it draws the lists + again. The same host under another scheme or port is another site and is + left alone. Only the extension's own pages may send either message - `[x]` on a denied site → forgets the refusal, in place; it connects nothing and tells the background nothing - Ten clicks on the version → reveals the Debug well (no screen change) @@ -2018,8 +2026,8 @@ view would leave a wallet one click from deletion. (`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` + without "Remember", which only the background holds, in session storage, + 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** @@ -2132,8 +2140,8 @@ view would leave a wallet one click from deletion. 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`. + without "Remember", which only the background holds, in session storage, 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/manifest/firefox.json b/manifest/firefox.json index 489c1a6..74b6e6f 100644 --- a/manifest/firefox.json +++ b/manifest/firefox.json @@ -27,7 +27,8 @@ "web_accessible_resources": ["src/content/inpage.js"], "browser_specific_settings": { "gecko": { - "id": "autistmask@sneak.berlin" + "id": "autistmask@sneak.berlin", + "strict_min_version": "115.0" } } } diff --git a/src/background/index.js b/src/background/index.js index 16eea83..33c9f83 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -49,6 +49,9 @@ const { actionApi, getPlatformInfo, runtimeApi, + sessionStorageGet, + sessionStorageRemove, + sessionStorageSet, tabsQuery, tabsSendMessage, windowsApi, @@ -61,12 +64,31 @@ const runtime = runtimeApi(); const windowsNs = windowsApi(); const actionNs = actionApi(); -// Connected sites (in-memory, non-persisted): { "origin:address": true } +// Connections the user allowed without "Remember my choice for this site". +// Each is one item in the browser's session storage, key +// origin + ":" + address and value true, and session storage holds nothing +// else. Session storage outlives this worker being stopped when idle and +// started again, so such a connection lasts until it is ended below or the +// browser closes. One item per connection means that making one is a single +// write, and two never overwrite each other. // // A site is its full origin (scheme://host[:port]), here and in the // remembered allowedSites/deniedSites lists alike: a grant to // https://dapp.example says nothing about http://dapp.example or another port. -const connectedSites = {}; +// An origin can carry a port, so the address is what follows the key's last +// colon and the origin is what precedes it. +async function isConnected(origin, address) { + const key = origin + ":" + address; + const items = await sessionStorageGet(key); + return items[key] === true; +} + +// End every connection allowed without "Remember" whose key `ends` returns +// true for. +async function endConnections(ends) { + const keys = Object.keys(await sessionStorageGet(null)).filter(ends); + if (keys.length > 0) await sessionStorageRemove(keys); +} // Pending approval requests: { id: { origin, resolve } } const pendingApprovals = {}; @@ -764,10 +786,10 @@ async function handleConnectionRequest(origin) { }; } - // Check allowed list or in-memory connected + // Check allowed list or a connection allowed without "Remember" if ( allowed.includes(origin) || - connectedSites[origin + ":" + activeAddress] + (await isConnected(origin, activeAddress)) ) { return { result: [activeAddress] }; } @@ -804,7 +826,7 @@ async function handleConnectionRequest(origin) { if (decision.remember) { await rememberSiteChoice("allowedSites", activeAddress, origin); } else { - connectedSites[origin + ":" + activeAddress] = true; + await sessionStorageSet({ [origin + ":" + activeAddress]: true }); } return { result: [activeAddress] }; } else { @@ -858,7 +880,7 @@ async function handleRpc(method, params, origin) { const allowed = s.allowedSites[activeAddress] || []; if ( allowed.includes(origin) || - connectedSites[origin + ":" + activeAddress] + (await isConnected(origin, activeAddress)) ) { return { result: [activeAddress] }; } @@ -894,7 +916,7 @@ async function handleRpc(method, params, origin) { const allowed = s.allowedSites[activeAddress] || []; if ( !allowed.includes(origin) && - !connectedSites[origin + ":" + activeAddress] + !(await isConnected(origin, activeAddress)) ) { return { error: { code: 4100, message: "Unauthorized" } }; } @@ -987,10 +1009,10 @@ async function handleRpc(method, params, origin) { const s = await getState(); const activeAddress = activeAddressOf(s); const allowed = s.allowedSites[activeAddress] || []; - const isConnected = + const hasPermission = allowed.includes(origin) || - connectedSites[origin + ":" + activeAddress]; - if (!isConnected || !activeAddress) { + (await isConnected(origin, activeAddress)); + if (!hasPermission || !activeAddress) { return { result: [] }; } return { @@ -1017,7 +1039,7 @@ async function handleRpc(method, params, origin) { const allowed = s.allowedSites[activeAddress] || []; if ( !allowed.includes(origin) && - !connectedSites[origin + ":" + activeAddress] + !(await isConnected(origin, activeAddress)) ) { return { error: { code: 4100, message: "Unauthorized" } }; } @@ -1071,7 +1093,7 @@ async function handleRpc(method, params, origin) { const allowed = s.allowedSites[activeAddress] || []; if ( !allowed.includes(origin) && - !connectedSites[origin + ":" + activeAddress] + !(await isConnected(origin, activeAddress)) ) { return { error: { code: 4100, message: "Unauthorized" } }; } @@ -1142,7 +1164,7 @@ async function handleSendTransaction(params, origin) { const allowed = s.allowedSites[activeAddress] || []; if ( !allowed.includes(origin) && - !connectedSites[origin + ":" + activeAddress] + !(await isConnected(origin, activeAddress)) ) { return { error: { code: 4100, message: "Unauthorized" } }; } @@ -1254,10 +1276,6 @@ async function broadcastChainChanged(chainId) { // Broadcast accountsChanged to all tabs, respecting per-address permissions async function broadcastAccountsChanged() { - // Clear non-remembered approvals on address switch - for (const key of Object.keys(connectedSites)) { - delete connectedSites[key]; - } // Reject and close any pending approval popups so they don't hang. An // approval an attempt has already claimed is left alone entirely: it is // being signed and broadcast right now, and neither rejecting it to the @@ -1276,6 +1294,10 @@ async function broadcastAccountsChanged() { } } resetPopupUrl(); + // An address switch ends every connection allowed without "Remember". This + // waits on storage, so it comes after the pending approvals above, which + // are still rejected the moment the switch arrives. + await endConnections(() => true); const s = await getState(); const activeAddress = activeAddressOf(s); const allowed = activeAddress ? s.allowedSites[activeAddress] || [] : []; @@ -1290,7 +1312,7 @@ async function broadcastAccountsChanged() { const hasPermission = activeAddress && (allowed.includes(origin) || - connectedSites[origin + ":" + activeAddress]); + (await isConnected(origin, activeAddress))); // Same as chainChanged above: a tab without our content script // rejects, and that is expected rather than a fault. tabsSendMessage(tab.id, { @@ -1917,41 +1939,57 @@ runtime.onMessage.addListener((msg, sender, sendResponse) => { } // 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)) { + endConnections((key) => { const address = key.slice(key.lastIndexOf(":") + 1); - if (removed.some((a) => sameAddress(a, address))) { - delete connectedSites[key]; - } - } + return removed.some((a) => sameAddress(a, address)); + }).catch((err) => { + log.errorf( + "ending the removed addresses' connections failed:", + err, + ); + }); return false; } - // Settings lists the sites connected without "Remember", which only this - // worker knows. The origin is what precedes the key's last colon. + // Settings lists the sites connected without "Remember", which only + // session storage holds. if (msg.type === "AUTISTMASK_GET_CONNECTED_SITES") { - sendResponse( - Object.keys(connectedSites).map((key) => - key.slice(0, key.lastIndexOf(":")), - ), - ); - return false; + sessionStorageGet(null) + .then((items) => + Object.keys(items).map((key) => + key.slice(0, key.lastIndexOf(":")), + ), + ) + .catch((err) => { + log.errorf("listing the connected sites failed:", err); + return []; + }) + .then(sendResponse); + return true; } // Settings removed this site (msg.origin) and has already dropped its // remembered entries. Its connections approved without "Remember" end // here, under every address, and its open tabs are told it has no account. + // The answer goes back once they have ended, and Settings waits for it + // before it lists the connected sites again. if (msg.type === "AUTISTMASK_REMOVE_SITE") { - for (const key of Object.keys(connectedSites)) { - if (key.slice(0, key.lastIndexOf(":")) === msg.origin) { - delete connectedSites[key]; - } - } - broadcastSiteRemoved(msg.origin); - return false; + endConnections( + (key) => key.slice(0, key.lastIndexOf(":")) === msg.origin, + ) + .catch((err) => { + log.errorf( + "ending the removed site's connections failed:", + err, + ); + }) + .then(() => { + broadcastSiteRemoved(msg.origin); + sendResponse(null); + }); + return true; } // Settings switched the network and has saved it. Open tabs are told the diff --git a/src/popup/views/settings.js b/src/popup/views/settings.js index 07c3fbb..fd6186c 100644 --- a/src/popup/views/settings.js +++ b/src/popup/views/settings.js @@ -76,11 +76,13 @@ function forgetOrigin(siteMap, origin) { // Removing a site from Allowed Sites or Connected Sites disconnects it: it is // no longer allowed under any address, and the background ends its -// connections approved without "Remember" and tells its open tabs. +// connections approved without "Remember" and tells its open tabs. The +// background answers once those have ended, so the lists drawn after it no +// longer show the site. async function removeAllowedSite(origin) { forgetOrigin(state.allowedSites, origin); await saveState(); - notify({ type: "AUTISTMASK_REMOVE_SITE", origin }); + await sendMessage({ type: "AUTISTMASK_REMOVE_SITE", origin }); await renderSiteLists(); } @@ -230,8 +232,8 @@ async function renderSiteLists() { Object.values(state.deniedSites).flat(), removeDeniedSite, ); - // Sites allowed without "Remember" are held only by the background, in - // memory, so it is asked for them. + // Sites allowed without "Remember" are held only in session storage, which + // the background keeps, so it is asked for them. renderSiteList( "settings-connected-sites", await sendMessage({ type: "AUTISTMASK_GET_CONNECTED_SITES" }), diff --git a/src/shared/browserApi.js b/src/shared/browserApi.js index 48064c9..472d88b 100644 --- a/src/shared/browserApi.js +++ b/src/shared/browserApi.js @@ -174,9 +174,9 @@ function notify(message) { // default: resolving {} would make an existing wallet read back as no wallet, // and resolving a no-op write would discard the user's state with nothing // logged. A caller that wants to degrade takes storageLocal() directly. -function storageUnavailable(method) { +function storageUnavailable(area, method) { return Promise.reject( - new Error("extension storage.local is not available: " + method), + new Error("extension storage." + area + " is not available: " + method), ); } @@ -187,7 +187,7 @@ function storageUnavailable(method) { */ function storageGet(keys) { const storage = storageLocal(); - if (!storage) return storageUnavailable("get"); + if (!storage) return storageUnavailable("local", "get"); return Promise.resolve(storage.get(keys)); } @@ -198,7 +198,7 @@ function storageGet(keys) { */ function storageSet(items) { const storage = storageLocal(); - if (!storage) return storageUnavailable("set"); + if (!storage) return storageUnavailable("local", "set"); return Promise.resolve(storage.set(items)); } @@ -215,7 +215,55 @@ function storageSet(items) { */ function storageRemove(keys) { const storage = storageLocal(); - if (!storage) return storageUnavailable("remove"); + if (!storage) return storageUnavailable("local", "remove"); + return Promise.resolve(storage.remove(keys)); +} + +// `storage.session`, the browser's session storage: what is kept there lasts +// until the browser closes. It survives the background worker being stopped +// and started again, which module-level variables do not (README, Background +// scheduling). The background keeps the connections allowed without +// "Remember" there; see src/background/index.js. Firefox has it from version +// 115, the minimum manifest/firefox.json declares. +// +// The three below reject where it is absent, as the three above do where +// `storage.local` is: a write that silently did nothing would tell a site it +// is connected and then refuse its next request. +function storageSession() { + const storage = namespaceMember("storage"); + return (storage && storage.session) || null; +} + +/** + * @param {string|string[]|null} keys null for every item. + * @returns {Promise} the stored items. + * @throws rejects where `storage.session` is absent. + */ +function sessionStorageGet(keys) { + const storage = storageSession(); + if (!storage) return storageUnavailable("session", "get"); + return Promise.resolve(storage.get(keys)); +} + +/** + * @param {Object} items + * @returns {Promise} + * @throws rejects where `storage.session` is absent. + */ +function sessionStorageSet(items) { + const storage = storageSession(); + if (!storage) return storageUnavailable("session", "set"); + return Promise.resolve(storage.set(items)); +} + +/** + * @param {string|string[]} keys + * @returns {Promise} + * @throws rejects where `storage.session` is absent. + */ +function sessionStorageRemove(keys) { + const storage = storageSession(); + if (!storage) return storageUnavailable("session", "remove"); return Promise.resolve(storage.remove(keys)); } @@ -307,6 +355,9 @@ module.exports = { notify, runtimeApi, sendMessage, + sessionStorageGet, + sessionStorageRemove, + sessionStorageSet, storageGet, storageLocal, storageRemove, diff --git a/src/shared/walletDelete.js b/src/shared/walletDelete.js index 7911005..0734180 100644 --- a/src/shared/walletDelete.js +++ b/src/shared/walletDelete.js @@ -14,7 +14,7 @@ function sameAddress(a, b) { // 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. +// "Remember", which only the background holds, in session storage. function dropSitePermissions(state, addresses) { for (const addr of addresses) { delete state.allowedSites[addr]; diff --git a/tests/backgroundApproval.test.js b/tests/backgroundApproval.test.js index 6ebf26f..0ae287a 100644 --- a/tests/backgroundApproval.test.js +++ b/tests/backgroundApproval.test.js @@ -321,9 +321,10 @@ function loadBackground(options) { require("../src/background/index"); // Send a message the way the browser would, and hand back whatever the - // handler passed to sendResponse. - function send(msg, sender) { - const sendResponse = jest.fn(); + // handler passed to sendResponse. `onResponse`, when given, receives the + // answer when the handler sends it, as the sender's callback does. + function send(msg, sender, onResponse) { + const sendResponse = jest.fn(onResponse); const kept = messageListener(msg, sender || {}, sendResponse); return { sendResponse, kept }; } @@ -2615,9 +2616,9 @@ async function siteAccounts(bg, origin) { return sendResponse.mock.calls[0][0]; } -// 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. +// A site connected without "Remember" is held only in the background's session +// storage, 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. // @@ -2796,8 +2797,7 @@ describe("removing a site in Settings disconnects it", () => { const element = (id) => (lists[id] ||= fakeSiteList()); global.document = { getElementById: element }; global.chrome.runtime.sendMessage = (msg, callback) => { - const { sendResponse } = bg.send(msg, bg.fromPopup); - if (callback) callback(sendResponse.mock.calls[0]?.[0]); + bg.send(msg, bg.fromPopup, callback); }; await require("../src/shared/state").loadState(); await require("../src/popup/views/settings").renderSiteLists(); diff --git a/tests/connectedSitesWorkerRestart.test.js b/tests/connectedSitesWorkerRestart.test.js new file mode 100644 index 0000000..25dc1f8 --- /dev/null +++ b/tests/connectedSitesWorkerRestart.test.js @@ -0,0 +1,354 @@ +// A site allowed without "Remember my choice for this site" stays connected +// when the background worker is stopped and started again +// (https://git.eeqj.de/sneak/AutistMask/issues/526). +// +// Chrome stops the worker after about 30 seconds idle and evaluates the +// module afresh on the next event, so nothing held at module scope survives +// it. Such a connection used to be held exactly there, and so lasted until the +// first idle spell. Each test here makes the connection in one load of the +// background, loads the module afresh over the same storage, as the browser +// does, and asks the new one. The paths that end a connection are driven after +// that restart, so each is shown to end what the earlier worker stored. + +const { networkById } = require("../src/shared/networks"); +const { makeStorageStub } = require("./support/storageStub"); +const { + removeAddressFromState, + removeWalletFromState, +} = require("../src/shared/walletDelete"); + +const ADDRESS = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; +const OTHER_ADDRESS = "0x70997970C51812dc3A010C7d01b50e0d17dc79C8"; + +const SITE = "https://dapp.example"; +const OTHER_SITE = "https://other.example"; + +const EXT_URL = "chrome-extension://autistmask/"; +const FROM_POPUP = { url: EXT_URL + "src/popup/index.html" }; + +// One wallet with two addresses, so that one of them can be removed, and no +// remembered sites, so that every connection here is one made without +// "Remember". +function storedProfile() { + const mainnet = networkById("mainnet"); + return { + hasWallet: true, + wallets: [ + { + name: "Wallet 1", + type: "hd", + addresses: [ + { address: ADDRESS, balance: "0", tokenBalances: [] }, + { address: OTHER_ADDRESS, balance: "0", tokenBalances: [] }, + ], + }, + ], + activeAddress: ADDRESS, + networkId: "mainnet", + rpcUrl: mainnet.defaultRpcUrl, + blockscoutUrl: mainnet.defaultBlockscoutUrl, + allowedSites: {}, + deniedSites: {}, + trackedTokens: [], + }; +} + +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +afterEach(() => { + delete global.chrome; + jest.resetModules(); +}); + +// Load the background module afresh, as the browser does when it starts the +// worker, over `storage`. The storage stub stands in for the browser's own +// storage, local and session, so it outlives each load. +function startWorker(storage) { + jest.resetModules(); + jest.doMock("../src/shared/balances", () => ({ + getProvider: () => ({}), + refreshBalances: jest.fn(async () => {}), + })); + jest.doMock("../src/shared/phishingDomains", () => ({ + isPhishingDomain: () => false, + })); + jest.doMock("../src/shared/alarms", () => ({ + BALANCE_REFRESH_ALARM: "balance", + BALANCE_REFRESH_PERIOD_MINUTES: 1, + ensureRecurringAlarms: jest.fn(async () => {}), + registerAlarmHandlers: jest.fn(), + })); + + let messageListener = null; + let connectListener = null; + // Every URL the toolbar popup was set to open: a site's connection prompt + // is shown there, so this is where its approval id appears. + const toolbarPopups = []; + const windowsCreated = []; + const sentToTabs = []; + + global.chrome = { + storage, + runtime: { + getURL: (path) => EXT_URL + path, + onMessage: { + addListener: (fn) => { + messageListener = fn; + }, + }, + onConnect: { + addListener: (fn) => { + connectListener = fn; + }, + }, + getPlatformInfo: (cb) => cb({ os: "linux" }), + // What the popup sends reaches the worker from the extension's own + // page. + sendMessage: (msg, callback) => { + messageListener(msg, FROM_POPUP, callback || (() => {})); + }, + lastError: null, + }, + windows: { + getLastFocused: (cb) => cb(null), + create: (options, cb) => { + windowsCreated.push(options); + cb({ id: windowsCreated.length }); + }, + remove: (id, cb) => { + if (cb) cb(); + }, + onRemoved: { addListener: () => {} }, + }, + tabs: { + query: (q, cb) => cb([{ id: 1, url: SITE + "/app" }]), + sendMessage: (tabId, msg, cb) => { + sentToTabs.push(msg); + if (cb) cb(); + }, + }, + action: { + setPopup: (o) => { + toolbarPopups.push(o.popup); + }, + openPopup: () => Promise.resolve(), + }, + }; + + require("../src/background/index"); + + // A request from a site, answered once the worker has had time to answer. + async function rpc(method, origin, params) { + let response = null; + messageListener( + { type: "AUTISTMASK_RPC", method, params: params || [] }, + { origin }, + (r) => { + response = r; + }, + ); + await settle(); + return response; + } + + // A message from the popup, and the worker's answer to it. + async function fromPopup(msg) { + let response; + messageListener(msg, FROM_POPUP, (r) => { + response = r; + }); + await settle(); + return response; + } + + // The site asks to connect, and the user allows it in the prompt with + // "Remember my choice for this site" left unchecked. + async function connectWithoutRemember(origin) { + let response = null; + messageListener( + { type: "AUTISTMASK_RPC", method: "eth_requestAccounts" }, + { origin }, + (r) => { + response = r; + }, + ); + await settle(); + const id = new URL(toolbarPopups.at(-1), EXT_URL).searchParams.get( + "approval", + ); + const decisionListeners = []; + const port = { + name: "approval:" + id, + sender: { url: EXT_URL + "src/popup/index.html?approval=" + id }, + onMessage: { addListener: (fn) => decisionListeners.push(fn) }, + onDisconnect: { addListener: () => {} }, + }; + connectListener(port); + for (const fn of decisionListeners) { + fn( + { + type: "AUTISTMASK_APPROVAL_DECISION", + approved: true, + remember: false, + }, + port, + ); + } + await settle(); + return response; + } + + return { + rpc, + fromPopup, + connectWithoutRemember, + windowsCreated, + sentToTabs, + }; +} + +// A site connected without "Remember" in one worker, and the worker started +// again over the same storage, where the site is still connected. +async function connectedThenRestarted() { + const storage = makeStorageStub({ autistmask: storedProfile() }); + const first = startWorker(storage); + expect(await first.connectWithoutRemember(SITE)).toEqual({ + result: [ADDRESS], + }); + const worker = startWorker(storage); + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ + result: [ADDRESS], + }); + return { storage, worker }; +} + +describe("a site allowed without Remember, after the worker restarts", () => { + test("is still told the address, and is not asked again", async () => { + const { worker } = await connectedThenRestarted(); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ + result: [ADDRESS], + }); + expect(await worker.rpc("eth_requestAccounts", SITE)).toEqual({ + result: [ADDRESS], + }); + }); + + test("is still listed under Connected Sites in Settings", async () => { + const { worker } = await connectedThenRestarted(); + + expect( + await worker.fromPopup({ type: "AUTISTMASK_GET_CONNECTED_SITES" }), + ).toEqual([SITE]); + }); + + test("may still ask for a signature", async () => { + // Refused with 4100 before the fix. Allowed, it raises the signing + // prompt and waits on the user. + const { worker } = await connectedThenRestarted(); + + const response = await worker.rpc("personal_sign", SITE, [ + "0x48656c6c6f", + ADDRESS, + ]); + + expect(response).toBeNull(); + expect(worker.windowsCreated).toHaveLength(1); + }); + + test("is not connected after the browser restarts", async () => { + // The browser empties session storage when it closes; the wallet's + // own storage keeps nothing of the connection. + const { storage } = await connectedThenRestarted(); + const afterBrowserRestart = startWorker( + makeStorageStub({ autistmask: storage.read("autistmask") }), + ); + + expect(await afterBrowserRestart.rpc("eth_accounts", SITE)).toEqual({ + result: [], + }); + }); +}); + +describe("each way a connection allowed without Remember ends, after the worker restarts", () => { + test("removing the site in Settings", async () => { + const { worker } = await connectedThenRestarted(); + expect(await worker.connectWithoutRemember(OTHER_SITE)).toEqual({ + result: [ADDRESS], + }); + + await worker.fromPopup({ + type: "AUTISTMASK_REMOVE_SITE", + origin: SITE, + }); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ result: [] }); + expect(worker.sentToTabs).toEqual([ + { + type: "AUTISTMASK_EVENT", + eventName: "accountsChanged", + data: [], + }, + ]); + expect( + await worker.fromPopup({ type: "AUTISTMASK_GET_CONNECTED_SITES" }), + ).toEqual([OTHER_SITE]); + expect(await worker.rpc("eth_accounts", OTHER_SITE)).toEqual({ + result: [ADDRESS], + }); + }); + + test("switching to another address", async () => { + // Switched away and back again, so the answer turns on the connection + // having ended rather than on which address is active. + const { storage, worker } = await connectedThenRestarted(); + const switched = { ...storage.read("autistmask") }; + + switched.activeAddress = OTHER_ADDRESS; + storage.write("autistmask", switched); + await worker.fromPopup({ type: "AUTISTMASK_ACTIVE_CHANGED" }); + switched.activeAddress = ADDRESS; + storage.write("autistmask", switched); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ result: [] }); + expect( + await worker.fromPopup({ type: "AUTISTMASK_GET_CONNECTED_SITES" }), + ).toEqual([]); + }); + + // The two below ask while storage still names the removed address as + // active, because the popup has not saved yet, so the answer turns on the + // connection alone. + test("removing the connected address", async () => { + const { storage, worker } = await connectedThenRestarted(); + + const popupState = storage.read("autistmask"); + expect(removeAddressFromState(popupState, 0, 0).removed).toBe(true); + await settle(); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ result: [] }); + }); + + test("removing the wallet holding the connected address", async () => { + const { storage, worker } = await connectedThenRestarted(); + + removeWalletFromState(storage.read("autistmask"), 0); + await settle(); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ result: [] }); + }); + + test("removing a different address leaves it connected", async () => { + const { storage, worker } = await connectedThenRestarted(); + + const popupState = storage.read("autistmask"); + expect(removeAddressFromState(popupState, 0, 1).removed).toBe(true); + await settle(); + + expect(await worker.rpc("eth_accounts", SITE)).toEqual({ + result: [ADDRESS], + }); + }); +}); diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 52a8883..c5e221f 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -3861,10 +3861,8 @@ test("eth_requestAccounts approved returns the selected address (#183)", async ( // all; this measures that the whole address is visible (#517). await assertAddressesFit(popup, "the connection prompt"); - // Remembered, so the connection survives a background worker that MV3 - // terminates after 30 seconds idle. The in-memory connectedSites map - // does not, and the sign and transaction tests below all require the - // origin to still be authorized. + // Remembered, because the sign and transaction tests below all + // require the origin to still be authorized. await popup.check("#approve-remember"); await clickAndClose(popup, "#btn-approve"); diff --git a/tests/e2e/slowApproval.js b/tests/e2e/slowApproval.js index d78c0aa..0ddeeab 100644 --- a/tests/e2e/slowApproval.js +++ b/tests/e2e/slowApproval.js @@ -22,6 +22,11 @@ // Before the approvals are raised it watches the worker stop on its own, so a // run in which something else keeps the worker running cannot pass. // +// The site is connected without "Remember", and once the worker has stopped +// the program requires the site still to be told the address: such a +// connection used to be held in the worker's memory and ended with it +// (https://git.eeqj.de/sneak/AutistMask/issues/526). +// // The dApp page and the stub node are the loopback ones the Firefox suite also // uses (tests/e2e/dapp.js). The site is connected and the node chosen // through the extension's own screens, so nothing is written to its storage @@ -311,8 +316,8 @@ async function main() { ); await send("Target.closeTarget", { targetId: popup.targetId }); - // The site, connected through the real prompt with "Remember", so - // the connection is stored rather than held by the worker. + // The site, connected through the real prompt without "Remember", so + // the connection is one that has to outlive the worker stopping below. const dapp = await openPage(node.url); await until("the injected provider", () => dapp.evaluate("!!window.ethereum && !!window.__dapp"), @@ -329,7 +334,7 @@ async function main() { inPrompt(shows("view-approve-site")), ); await inPrompt( - "document.getElementById('approve-remember').checked = true", + "document.getElementById('approve-remember').checked = false", ); // The prompt closes itself on the click. await inPrompt(click("btn-approve")).catch(() => {}); @@ -365,6 +370,20 @@ async function main() { "s", ); + // The site's request starts the worker again, and the new worker still + // has the connection allowed without "Remember" + // (https://git.eeqj.de/sneak/AutistMask/issues/526). + const stillConnected = await dapp.evaluate( + "window.ethereum.request({ method: 'eth_accounts' })", + ); + check( + "a site allowed without Remember is still connected after the worker stopped", + Array.isArray(stillConnected) && + stillConnected.length === 1 && + getAddress(stillConnected[0]) === address, + "eth_accounts answered " + JSON.stringify(stillConnected), + ); + // A transaction approval and a signature approval, both open at once. await dapp.evaluate( "window.__dapp.start('tx', 'eth_sendTransaction', [" + diff --git a/tests/support/storageStub.js b/tests/support/storageStub.js index bce6354..5eb9004 100644 --- a/tests/support/storageStub.js +++ b/tests/support/storageStub.js @@ -1,4 +1,4 @@ -// A chrome.storage.local stub that behaves like the real one. +// A chrome.storage stub that behaves like the real one. // // The real extension storage API is a serialization boundary: `set` writes a // structured clone of what it is given, and `get` hands back a structured @@ -17,13 +17,8 @@ // So every test that drives real persistence uses this, and nothing rebuilds // a storage stub by hand. -// `initial` is the starting contents, keyed as storage is: { autistmask: {...} }. -// `onOp` runs before each operation, for a test that needs to advance a clock -// or count round trips. -function makeStorageStub(initial, onOp) { - const store = initial ? structuredClone(initial) : {}; - const tick = onOp || (() => {}); - +// One storage area: get, set and remove over its own store, cloning both ways. +function makeArea(store, tick) { const get = jest.fn(async (key) => { tick(); if (key === undefined || key === null) return structuredClone(store); @@ -49,15 +44,28 @@ function makeStorageStub(initial, onOp) { for (const k of Array.isArray(key) ? key : [key]) delete store[k]; }); + return { get, set, remove }; +} + +// `initial` is the starting contents of `storage.local`, keyed as storage is: +// { autistmask: {...} }. `storage.session` starts empty. +// `onOp` runs before each operation on either area, for a test that needs to +// advance a clock or count round trips. +function makeStorageStub(initial, onOp) { + const store = initial ? structuredClone(initial) : {}; + const tick = onOp || (() => {}); + const local = makeArea(store, tick); + return { // Drop this straight in as chrome.storage. - local: { get, set, remove }, - get, - set, - remove, - // What is stored, cloned on the way out: a test can neither observe a - // later write through an object it read nor reach into the store by - // mutating one. + local, + session: makeArea({}, tick), + get: local.get, + set: local.set, + remove: local.remove, + // What is stored in `storage.local`, cloned on the way out: a test can + // neither observe a later write through an object it read nor reach + // into the store by mutating one. read: (key) => key === undefined ? structuredClone(store)