fix: a site allowed without Remember stays connected when the background worker idles (closes #526)

A connection allowed without "Remember my choice for this site" lived in a module-level object in the background, so it ended the first time Chrome stopped the idle worker. Each such connection is now one item in the browser's session storage, which outlives the worker and is emptied when the browser closes, read and written through new wrappers in src/shared/browserApi.js. Removal in Settings, address switch, address removal and wallet removal still end it; Settings now waits for the removal before redrawing.

manifest/firefox.json now declares a minimum of Firefox 115.0, the first with session storage.

Judgement call: address and wallet removal still notify the background without waiting.

Model: opus-5-5

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

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" }),

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<Object>} 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<void>}
+ * @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<void>}
+ * @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,

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];