fix: Settings refuses an endpoint URL whose network changed before it was stored (closes #663)

The RPC and Blockscout "Save" buttons in Settings take the network that
Settings was last drawn for. A follow of a switch saved elsewhere sets
`state` first. Settings is drawn again only once the follow's write
returns.

The URL is set in `state` inside the save, holding the lock. The save is
refused if the network that Settings showed is no longer active, on the
page or in the stored record. The flash message then says that the
network changed, and "Saved." is shown only for a stored URL.

Before, a popup that followed a switch saved elsewhere could replace the
URL before the write and still show "Saved.". It could also store a URL
for a network that Settings did not show.

Model: opus-5-5

README.md

@@ -2281,6 +2281,8 @@ view would leave a wallet one click from deletion.
         - Endpoint URL input + "Save" button. "Save" asks the URL for `/stats`
           and saves the URL only if that request succeeds. The request carries
           no user data and is made whether or not "Use Blockscout" is ticked.
+    - If the network changes before either "Save" stores its URL, the URL is not
+      saved and a flash message says so
     - Etherscan:
         - "Check recipients on Etherscan" checkbox, unticked until the user
           ticks it

src/popup/views/settings.js

@@ -44,6 +44,12 @@ let ctx;
 let versionClickCount = 0;
 let versionClickTimer = null;
 
+// The network that Settings was last drawn for. A follow of a switch saved
+// elsewhere sets `state` to the new network first. Settings is drawn again only
+// once the follow's write returns. So both "Save" buttons take this network,
+// not the one in `state`.
+let shownNetwork = null;
+
 // One row per site origin, however many addresses it appears under, each with
 // an [x] that hands it to onRemove.
 function renderSiteList(containerId, origins, onRemove) {
@@ -199,6 +205,7 @@ function renderWalletListSettings() {
 }
 
 function show() {
+    shownNetwork = currentNetwork();
     $("settings-rpc").value = state.rpcUrl;
     $("settings-blockscout").value = state.blockscoutUrl;
     $("settings-network").value = state.networkId;
@@ -306,6 +313,7 @@ function showNewSettings() {
         state.checkRecipientsOnEtherscan;
     if (state.currentView === "settings") {
         markViewRendered("settings");
+        shownNetwork = currentNetwork();
         $("settings-network").value = state.networkId;
         $("settings-rpc").value = state.rpcUrl;
         $("settings-blockscout").value = state.blockscoutUrl;
@@ -319,6 +327,28 @@ function showNewSettings() {
     updateDebugBanner(state.currentView);
 }
 
+// Saves `url` as `field` ("rpcUrl" or "blockscoutUrl") of the network
+// `networkId`, the one that Settings showed when the user tapped "Save". The
+// URL is set in `state` inside the save, holding the lock. So the page cannot
+// follow a switch saved elsewhere (followStoredSettings() in
+// src/popup/index.js) between setting the URL and writing it. If the active
+// network is no longer `networkId`, on this page or in the stored record,
+// nothing is saved. The flash message then says that the network changed.
+async function saveEndpoint(field, url, networkId) {
+    let refused = false;
+    await saveState((stored) => {
+        refused =
+            state.networkId !== networkId || stored.networkId !== networkId;
+        if (refused) return false;
+        state[field] = url;
+    });
+    if (refused) {
+        showFlash("The URL was not saved because the network changed.");
+    } else {
+        showFlash("Saved.");
+    }
+}
+
 function init(_ctx) {
     ctx = _ctx;
     deleteWallet.init(ctx);
@@ -330,6 +360,8 @@ function init(_ctx) {
             showFlash("Please enter an RPC URL.");
             return;
         }
+        // The URL is for the network that Settings shows when "Save" is tapped.
+        const net = shownNetwork;
         showFlash("Testing endpoint...");
         try {
             const resp = await debugFetch(url, {
@@ -348,7 +380,6 @@ function init(_ctx) {
                 showFlash("Endpoint returned an error.");
                 return;
             }
-            const net = currentNetwork();
             if (json.result !== net.chainId) {
                 showFlash("Wrong network: expected " + net.name + ".");
                 return;
@@ -364,9 +395,7 @@ function init(_ctx) {
             showFlash("Could not reach endpoint.");
             return;
         }
-        state.rpcUrl = url;
-        await saveState();
-        showFlash("Saved.");
+        await saveEndpoint("rpcUrl", url, net.id);
     });
 
     $("btn-save-blockscout").addEventListener("click", async () => {
@@ -375,6 +404,7 @@ function init(_ctx) {
             showFlash("Please enter a Blockscout API URL.");
             return;
         }
+        const net = shownNetwork;
         showFlash("Testing endpoint...");
         try {
             const resp = await debugFetch(url + "/stats");
@@ -389,9 +419,7 @@ function init(_ctx) {
             showFlash("Could not reach endpoint.");
             return;
         }
-        state.blockscoutUrl = url;
-        await saveState();
-        showFlash("Saved.");
+        await saveEndpoint("blockscoutUrl", url, net.id);
     });
 
     // Each change is made inside the save, holding the lock. So

tests/popupFollowsNetworkSwitch.test.js

@@ -253,6 +253,271 @@ test("a switch back in Settings after a follow whose write failed is stored", as
     expect(told.at(-1).chainId).toBe(NETWORKS.mainnet.chainId);
 });
 
+// The user saves an endpoint URL in Settings on Ethereum Mainnet. Before the
+// URL is stored, the background saves a switch to Sepolia. Another write holds
+// the lock, so the popup's follow of that switch and the URL's save both wait
+// for the lock. Either may take it first. The URL is stored for neither
+// network. The flash message says that the network changed.
+test.each([
+    { button: "btn-save-rpc", field: "settings-rpc", first: "follow" },
+    { button: "btn-save-rpc", field: "settings-rpc", first: "save" },
+    {
+        button: "btn-save-blockscout",
+        field: "settings-blockscout",
+        first: "follow",
+    },
+    {
+        button: "btn-save-blockscout",
+        field: "settings-blockscout",
+        first: "save",
+    },
+])(
+    "$button saves nothing when the network changes before the URL is stored ($first first)",
+    async ({ button, field, first }) => {
+        const page = await bootOnAddress();
+        await page.click("btn-settings");
+        // This answer passes both endpoint checks. The RPC check reads
+        // Ethereum Mainnet's chain id from it. The Blockscout check reads `ok`.
+        globalThis.fetch.mockResolvedValue({
+            ok: true,
+            json: async () => ({ result: NETWORKS.mainnet.chainId }),
+        });
+        const url = "https://own-node.example.invalid";
+
+        let releaseLock;
+        const lockHeld = navigator.locks.request(
+            "autistmask",
+            () =>
+                new Promise((resolve) => {
+                    releaseLock = resolve;
+                }),
+        );
+        // The click handler runs until its save waits for the lock.
+        const tapSave = async () => {
+            page.node(field).value = url;
+            const saving = page.node(button).listeners.click.map((fn) => fn());
+            await page.settle();
+            return saving;
+        };
+        let saving;
+        if (first === "save") saving = await tapSave();
+        await backgroundSwitch(page, "sepolia");
+        if (first === "follow") saving = await tapSave();
+        releaseLock();
+        await lockHeld;
+        await Promise.all(saving);
+        await page.settle();
+
+        const { state } = require("../src/shared/state");
+        expect(state.networkId).toBe("sepolia");
+        expect(JSON.stringify(page.storage.read("autistmask"))).not.toContain(
+            url,
+        );
+        expect(page.text("flash-msg")).toBe(
+            "The URL was not saved because the network changed.",
+        );
+    },
+);
+
+// The popup's follow of the background's switch to Sepolia has set `state` to
+// Sepolia. The follow's write has not returned yet, so Settings still shows
+// Ethereum Mainnet. The user taps "Save" there. The URL is for Ethereum
+// Mainnet, so the save stores the URL for neither network.
+test.each([
+    { button: "btn-save-rpc", field: "settings-rpc" },
+    { button: "btn-save-blockscout", field: "settings-blockscout" },
+])(
+    "$button saves nothing when tapped while the follow's write waits",
+    async ({ button, field }) => {
+        const page = await bootOnAddress();
+        await page.click("btn-settings");
+        // This answer passes both endpoint checks on Ethereum Mainnet.
+        globalThis.fetch.mockResolvedValue({
+            ok: true,
+            json: async () => ({ result: NETWORKS.mainnet.chainId }),
+        });
+        const url = "https://own-node.example.invalid";
+
+        const { set } = page.storage.local;
+        const write = set.getMockImplementation();
+        let releaseWrite;
+        const writeHeld = new Promise((resolve) => {
+            releaseWrite = resolve;
+        });
+        // The background's write lands; the next, the follow's, waits.
+        set.mockImplementationOnce(write).mockImplementationOnce(
+            async (items) => {
+                await writeHeld;
+                await write(items);
+            },
+        );
+        await backgroundSwitch(page, "sepolia");
+        const { state } = require("../src/shared/state");
+        expect(state.networkId).toBe("sepolia");
+        expect(page.value("settings-network")).toBe("mainnet");
+
+        page.node(field).value = url;
+        const saving = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        releaseWrite();
+        await Promise.all(saving);
+        await page.settle();
+
+        expect(JSON.stringify(page.storage.read("autistmask"))).not.toContain(
+            url,
+        );
+        expect(page.text("flash-msg")).toBe(
+            "The URL was not saved because the network changed.",
+        );
+    },
+);
+
+// The user taps the RPC "Save" on Ethereum Mainnet. While the check waits for
+// the node's answer, the popup follows the background's switch to Sepolia. The
+// node answers with Sepolia's chain id. So the check refuses the node for
+// Ethereum Mainnet, and nothing is stored.
+test("the RPC check compares the answer with the network at the tap", async () => {
+    const page = await bootOnAddress();
+    await page.click("btn-settings");
+    let answer;
+    globalThis.fetch.mockReturnValue(
+        new Promise((resolve) => {
+            answer = resolve;
+        }),
+    );
+    const url = "https://sepolia-node.example.invalid";
+
+    page.node("settings-rpc").value = url;
+    const saving = page.node("btn-save-rpc").listeners.click.map((fn) => fn());
+    await page.settle();
+    await backgroundSwitch(page, "sepolia");
+    const { state } = require("../src/shared/state");
+    expect(state.networkId).toBe("sepolia");
+    answer({ json: async () => ({ result: NETWORKS.sepolia.chainId }) });
+    await Promise.all(saving);
+    await page.settle();
+
+    expect(JSON.stringify(page.storage.read("autistmask"))).not.toContain(url);
+    expect(page.text("flash-msg")).toBe(
+        "Wrong network: expected Ethereum Mainnet.",
+    );
+});
+
+// The user taps the Blockscout "Save" on Ethereum Mainnet. While the check
+// waits for Blockscout's answer, the popup follows the background's switch to
+// Sepolia. The URL is for Ethereum Mainnet, so nothing is stored.
+test("the Blockscout save is for the network at the tap", async () => {
+    const page = await bootOnAddress();
+    await page.click("btn-settings");
+    let answer;
+    globalThis.fetch.mockReturnValue(
+        new Promise((resolve) => {
+            answer = resolve;
+        }),
+    );
+    const url = "https://own-explorer.example.invalid/api/v2";
+
+    page.node("settings-blockscout").value = url;
+    const saving = page
+        .node("btn-save-blockscout")
+        .listeners.click.map((fn) => fn());
+    await page.settle();
+    await backgroundSwitch(page, "sepolia");
+    expect(page.value("settings-network")).toBe("sepolia");
+    answer({ ok: true });
+    await Promise.all(saving);
+    await page.settle();
+
+    expect(JSON.stringify(page.storage.read("autistmask"))).not.toContain(url);
+    expect(page.text("flash-msg")).toBe(
+        "The URL was not saved because the network changed.",
+    );
+});
+
+// Settings is open when the network changes to Sepolia. The popup follows the
+// background's switch, or the user picks Sepolia in the network selector. Each
+// "Save" after the switch stores its URL for Sepolia.
+test.each([
+    ["a follow", (page) => backgroundSwitch(page, "sepolia")],
+    [
+        "the network selector",
+        async (page) => {
+            const select = page.node("settings-network");
+            select.value = "sepolia";
+            for (const fn of select.listeners.change) await fn();
+            await page.settle();
+        },
+    ],
+])(
+    "after a switch to Sepolia by %s, Settings saves each URL for Sepolia",
+    async (_how, switchToSepolia) => {
+        const page = await bootOnAddress();
+        await page.click("btn-settings");
+        await switchToSepolia(page);
+        // This answer passes both endpoint checks on Sepolia. The RPC check
+        // reads Sepolia's chain id from it. The Blockscout check reads `ok`.
+        globalThis.fetch.mockResolvedValue({
+            ok: true,
+            json: async () => ({ result: NETWORKS.sepolia.chainId }),
+        });
+        const rpcUrl = "https://own-node.example.invalid";
+        const blockscoutUrl = "https://own-explorer.example.invalid/api/v2";
+
+        page.node("settings-rpc").value = rpcUrl;
+        await page.click("btn-save-rpc");
+        expect(page.text("flash-msg")).toBe("Saved.");
+        page.node("settings-blockscout").value = blockscoutUrl;
+        await page.click("btn-save-blockscout");
+        expect(page.text("flash-msg")).toBe("Saved.");
+
+        const stored = page.storage.read("autistmask");
+        expect(stored.networkId).toBe("sepolia");
+        expect(stored.rpcUrl).toBe(rpcUrl);
+        expect(stored.blockscoutUrl).toBe(blockscoutUrl);
+    },
+);
+
+// The user picks Sepolia in Settings, but the switch's write fails. The page is
+// then on Sepolia, while Settings was last drawn for Ethereum Mainnet. A "Save"
+// after the failed switch stores nothing for either network.
+test.each([
+    { button: "btn-save-rpc", field: "settings-rpc" },
+    { button: "btn-save-blockscout", field: "settings-blockscout" },
+])(
+    "$button saves nothing after a switch in Settings whose write failed",
+    async ({ button, field }) => {
+        const page = await bootOnAddress();
+        await page.click("btn-settings");
+        page.storage.local.set.mockRejectedValueOnce(
+            new Error("quota exceeded"),
+        );
+        const select = page.node("settings-network");
+        select.value = "sepolia";
+        for (const fn of select.listeners.change) {
+            await expect(fn()).rejects.toThrow("quota exceeded");
+        }
+        await page.settle();
+        const { state } = require("../src/shared/state");
+        expect(state.networkId).toBe("sepolia");
+        // This answer passes both endpoint checks on Ethereum Mainnet.
+        globalThis.fetch.mockResolvedValue({
+            ok: true,
+            json: async () => ({ result: NETWORKS.mainnet.chainId }),
+        });
+        const url = "https://own-node.example.invalid";
+
+        page.node(field).value = url;
+        await page.click(button);
+
+        expect(JSON.stringify(page.storage.read("autistmask"))).not.toContain(
+            url,
+        );
+        expect(page.text("flash-msg")).toBe(
+            "The URL was not saved because the network changed.",
+        );
+    },
+);
+
 // Such as one a newer build wrote, which this build cannot read. Nothing reads
 // or writes the stored record for it: the popup's next save finds it, as
 // before.