fix: eth_sign's warning and the README say what it signs (closes #409)

eth_sign is signed as an EIP-191 personal message over its data, which is
what the Ethereum JSON-RPC specification defines it to return. Its warning
on the signature prompt said the site was asking to sign a raw hash that
could drain funds, and the README said the same; neither was true. The
warning now says the site used the older eth_sign method and that the data
will be signed as a personal message, and the README's SignApproval section
says so too. Tests pin that the background reads eth_sign address first and
data second, and that the sign screen answers it with the personal message
signature over the data.

Model: opus-5-5

README.md

@@ -2128,7 +2128,12 @@ view would leave a wallet one click from deletion.
       blocklist)
     - Site origin (bold, scheme and port included) + "wants you to sign a
       message"
-    - Danger warning box (shown for `eth_sign`, which signs a raw hash)
+    - Warning box, shown for `eth_sign`: "This site used the older eth_sign
+      method. The data will be signed as a personal message." An `eth_sign`
+      request names the address first and the data second. Its data is signed
+      exactly as a `personal_sign` message is, as an EIP-191 personal message,
+      which is what the Ethereum JSON-RPC specification defines `eth_sign` to
+      return.
     - Type: "Personal message" or "Typed data (EIP-712)"
     - From: color dot + full address + etherscan link
     - Message: for `personal_sign` and `eth_sign`, the text the message's bytes

src/background/index.js

@@ -988,9 +988,8 @@ async function handleRpc(method, params, origin) {
 
         if (method === "eth_sign") {
             signParams.dangerWarning =
-                "\u26a0\ufe0f DANGER: This site is requesting to sign a raw hash. " +
-                "This can be used to sign transactions that drain your funds. " +
-                "Only proceed if you fully understand what you are signing.";
+                "This site used the older eth_sign method. " +
+                "The data will be signed as a personal message.";
         }
 
         if (findPendingApproval(origin, "sign")) {

src/popup/views/approval.js

@@ -725,7 +725,7 @@ function showSignApproval(details) {
             "This message is plain text, not hex, so it cannot be signed.";
     }
 
-    // Display danger warning for eth_sign (raw hash signing)
+    // The warning the background attaches to an eth_sign request.
     const warningEl = $("approve-sign-danger-warning");
     if (warningEl) {
         if (sp.dangerWarning) {

src/popup/views/transactionDetail.js

@@ -183,7 +183,7 @@ function render() {
         if (el) el.classList.add("hidden");
     }
 
-    loadFullTxDetails(tx.hash, tx.chainId);
+    loadFullTxDetails(tx.hash, tx.to, tx.chainId);
 
     const isoStr = isoDate(tx.timestamp);
     $("tx-detail-time").innerHTML =
@@ -291,7 +291,7 @@ function populateOnChainDetails(txData, chainId) {
     }
 }
 
-async function loadFullTxDetails(txHash, chainId) {
+async function loadFullTxDetails(txHash, toAddress, chainId) {
     const section = $("tx-detail-calldata-section");
     const actionEl = $("tx-detail-calldata-action");
     const detailsEl = $("tx-detail-calldata-details");
@@ -313,10 +313,7 @@ async function loadFullTxDetails(txHash, chainId) {
         const inputData = txData.raw_input || txData.input || null;
         if (!inputData || inputData === "0x") return;
 
-        // Decoded against the address the transaction was sent to, as the
-        // explorer reports it, not the history entry's `to`: for a token
-        // transfer that is the transfer's recipient, not the token contract.
-        const decoded = decodeCalldata(inputData, txData.to?.hash || "");
+        const decoded = decodeCalldata(inputData, toAddress || "");
         if (decoded) {
             // Render decoded calldata matching approval view style
             actionEl.textContent = decoded.name;

src/shared/walletDefects.js

@@ -36,11 +36,13 @@ const DEFECTS = {
                 "derivation path beneath that key instead of from a master " +
                 "key, so the addresses listed here are not the ones that key " +
                 "produces under the standard path.",
-            "Signing and sending are disabled for this wallet. The addresses " +
-                "do descend from the extended private key you imported, so " +
-                "anything they hold is still reachable by software that " +
-                "repeats the same non-standard derivation. Check them in a " +
-                "block explorer before deciding what to do.",
+            "Signing, sending and private key export are disabled for this " +
+                "wallet. Anything these addresses hold can be reached outside " +
+                "this wallet with the extended private key you imported, if " +
+                "you kept a copy of it, or with the master extended private " +
+                "key or recovery phrase it came from, in software that " +
+                "repeats the same non-standard derivation. Check the " +
+                "addresses in a block explorer before deciding what to do.",
             "To see the addresses this key produces under the standard path, " +
                 "import the master extended private key, or the recovery " +
                 "phrase it came from, as a new wallet. Nothing here has been " +

tests/backgroundApproval.test.js

@@ -1606,6 +1606,55 @@ describe("what the approval is verified against", () => {
     });
 });
 
+// eth_sign names the address first and the data second, the reverse of
+// personal_sign, and is answered as the JSON-RPC specification defines it:
+// with an EIP-191 personal message signature over the data, the same
+// signature personal_sign returns
+// (https://git.eeqj.de/sneak/AutistMask/issues/409). The signature below is
+// made by the test; that the sign screen makes the same one is tested in
+// tests/ethSign.test.js.
+describe("eth_sign", () => {
+    test("is read address first, and returns a personal message signature over the data", async () => {
+        const bg = loadBackground();
+        const page = bg.send(
+            {
+                type: "AUTISTMASK_RPC",
+                method: "eth_sign",
+                params: [signer.address, MESSAGE],
+            },
+            { origin: ORIGIN },
+        );
+        await settle();
+        const id = approvalIdIn(bg.created.map((c) => c.url));
+
+        const details = bg.send(
+            { type: "AUTISTMASK_GET_APPROVAL", id },
+            { url: bg.fromPopup.url },
+        );
+        const { signParams } = details.sendResponse.mock.calls[0][0];
+        expect(signParams).toMatchObject({
+            method: "eth_sign",
+            message: MESSAGE,
+            from: signer.address,
+        });
+        expect(signParams.dangerWarning).toBe(
+            "This site used the older eth_sign method. " +
+                "The data will be signed as a personal message.",
+        );
+
+        const signature = await signer.signMessage(
+            Buffer.from(MESSAGE.slice(2), "hex"),
+        );
+        bg.send(
+            { type: "AUTISTMASK_SIGN_RESPONSE", id, approved: true, signature },
+            { url: bg.fromPopup.url },
+        );
+        await settle();
+
+        expect(page.sendResponse).toHaveBeenCalledWith({ result: signature });
+    });
+});
+
 // The interlock must not cost the retry the approval exists to allow.
 describe("the interlock releases a failed attempt", () => {
     test("a retryable failure before the broadcast leaves the approval usable", async () => {

tests/ethSign.test.js

@@ -0,0 +1,102 @@
+// The sign screen signs an eth_sign request the way it signs personal_sign: as
+// an EIP-191 personal message over the bytes of the data
+// (https://git.eeqj.de/sneak/AutistMask/issues/409). Booted through the real
+// popup entry point as the approval window. The background's half, which reads
+// the address first and the data second and checks the signature, is tested in
+// tests/backgroundApproval.test.js.
+
+// Mocked so that no password has to be hashed: the stored secret decrypts to
+// the test key below.
+jest.mock("../src/shared/vault", () => ({
+    decryptWithPassword: jest.fn(),
+    encryptWithPassword: jest.fn(),
+}));
+
+const { Wallet, getBytes } = require("ethers");
+const {
+    bootPopup,
+    cleanupPopup,
+    unversionedValidProfile,
+} = require("./support/popupBoot");
+
+// A published test key, never used for real funds.
+const SIGNER_KEY =
+    "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d";
+const signer = new Wallet(SIGNER_KEY);
+// "Hello AutistMask", as the hex a page sends.
+const MESSAGE = "0x48656c6c6f204175746973744d61736b";
+const ORIGIN = "https://dapp.example";
+
+afterEach(cleanupPopup);
+
+test("the sign screen answers eth_sign with a personal message signature over the data", async () => {
+    const sent = [];
+    const page = await bootPopup(
+        unversionedValidProfile({
+            wallets: [
+                {
+                    type: "key",
+                    name: "Wallet 1",
+                    encryptedSecret: "encrypted-key",
+                    addresses: [
+                        {
+                            address: signer.address,
+                            balance: "0.0000",
+                            tokenBalances: [],
+                        },
+                    ],
+                },
+            ],
+            activeAddress: signer.address,
+            allowedSites: { [signer.address]: [ORIGIN] },
+        }),
+        {
+            search: "?approval=approval-1",
+            runtime: {
+                connect: () => ({ postMessage: () => {} }),
+                // Answered as the background answers: with the eth_sign
+                // approval this window was opened for, and by handing back
+                // the signature the popup sends.
+                sendMessage: (msg, reply) => {
+                    sent.push(msg);
+                    if (msg.type === "AUTISTMASK_GET_APPROVAL") {
+                        reply({
+                            type: "sign",
+                            origin: ORIGIN,
+                            isPhishingDomain: false,
+                            approvedFrom: signer.address,
+                            signParams: {
+                                method: "eth_sign",
+                                message: MESSAGE,
+                                from: signer.address,
+                            },
+                        });
+                    } else if (msg.type === "AUTISTMASK_SIGN_RESPONSE") {
+                        reply({ signature: msg.signature });
+                    } else {
+                        reply(undefined);
+                    }
+                },
+            },
+        },
+    );
+    globalThis.window.close = jest.fn();
+    const { decryptWithPassword } = require("../src/shared/vault");
+    decryptWithPassword.mockResolvedValue(SIGNER_KEY);
+
+    expect(page.visibleViews()).toEqual(["approve-sign"]);
+    page.node("approve-sign-password").value = "any password";
+    await page.click("btn-approve-sign");
+
+    expect(
+        sent.filter((msg) => msg.type === "AUTISTMASK_SIGN_RESPONSE"),
+    ).toEqual([
+        {
+            type: "AUTISTMASK_SIGN_RESPONSE",
+            id: "approval-1",
+            approved: true,
+            signature: await signer.signMessage(getBytes(MESSAGE)),
+        },
+    ]);
+    expect(globalThis.window.close).toHaveBeenCalled();
+});

tests/tokenTransferDetail.test.js

@@ -1,176 +0,0 @@
-// The decoded details on the transaction detail screen of a token transfer
-// (https://git.eeqj.de/sneak/AutistMask/issues/515).
-//
-// A history row for a token transfer holds the transfer's recipient as its
-// `to`, not the contract the transaction was sent to. The screen used to decode
-// the transaction's data against that `to`, so its Token line named the
-// recipient, and the amount read "decimals unknown" for a token the wallet
-// tracks. It decodes against the address the explorer reports the transaction
-// was sent to.
-//
-// The row is the one the real history parser makes from the explorer's answer,
-// opened through transactionDetail.show(), which is what a click on the row
-// calls. Driven against a minimal DOM stub in the shape
-// tests/contractCreation.test.js uses.
-
-const { Interface } = require("ethers");
-
-jest.mock("../src/shared/log", () => ({
-    log: {
-        debugf: () => {},
-        infof: () => {},
-        warnf: () => {},
-        errorf: () => {},
-    },
-    debugFetch: async (url) => ({
-        ok: true,
-        json: async () => mockExplorerAnswer(url),
-    }),
-    setRuntimeDebug: () => {},
-    isDebug: () => false,
-}));
-
-globalThis.chrome = {
-    storage: { local: { get: async () => ({}), set: async () => {} } },
-};
-
-const { state } = require("../src/shared/state");
-const { fetchRecentTransactions } = require("../src/shared/transactions");
-const transactionDetail = require("../src/popup/views/transactionDetail");
-
-const EXPLORER = "https://explorer.example/api/v2";
-const HOLDER = "0x0000000000000000000000000000000000000a11";
-const RECIPIENT = "0x711e691962E5c054b78071C789A0C8CE657377aD";
-// A token the wallet tracks and the bundled token list does not name.
-const TOKEN = "0x74de5d4FCbf63E00296fd95d33236B9794016631";
-const TX_HASH =
-    "0x85215772ed26ea8b39c2b3b18779030487efbe0b5fd7e882592b2f62b837be84";
-
-// HOLDER sends 1234.5 of the 6-decimal token to RECIPIENT.
-const TRANSFER_DATA = new Interface([
-    "function transfer(address to, uint256 amount)",
-]).encodeFunctionData("transfer", [RECIPIENT, 1234500000n]);
-
-// The explorer's answers: the transfer in HOLDER's token transfers, and the
-// transaction itself, sent to the token contract.
-function mockExplorerAnswer(url) {
-    if (
-        url ===
-        EXPLORER + "/addresses/" + HOLDER + "/token-transfers?type=ERC-20"
-    ) {
-        return {
-            items: [
-                {
-                    transaction_hash: TX_HASH,
-                    block_number: 100,
-                    timestamp: "2026-10-01T00:00:00Z",
-                    from: { hash: HOLDER },
-                    to: { hash: RECIPIENT },
-                    total: { value: "1234500000", decimals: "6" },
-                    token: {
-                        address_hash: TOKEN,
-                        symbol: "TDOL",
-                        holders_count: "5000",
-                    },
-                },
-            ],
-        };
-    }
-    if (url === EXPLORER + "/transactions/" + TX_HASH) {
-        return { to: { hash: TOKEN }, raw_input: TRANSFER_DATA };
-    }
-    return { items: [] };
-}
-
-function makeElement(id) {
-    const classes = new Set();
-    const el = {
-        id,
-        textContent: "",
-        value: "",
-        innerHTML: "",
-        style: {},
-        dataset: {},
-        classList: {
-            add: (...names) => names.forEach((n) => classes.add(n)),
-            remove: (...names) => names.forEach((n) => classes.delete(n)),
-            contains: (n) => classes.has(n),
-            toggle: () => false,
-        },
-        addEventListener: () => {},
-        querySelectorAll: () => [],
-        appendChild: () => {},
-    };
-    // Views reach for .parentElement to hide whole sections.
-    Object.defineProperty(el, "parentElement", {
-        get: () => node(id + "-parent"),
-    });
-    return el;
-}
-
-function makeDocument() {
-    const els = new Map();
-    return {
-        getElementById(id) {
-            // The debug banner is created on demand by helpers.js; absent
-            // is the state a non-debug, non-testnet popup is in.
-            if (id === "debug-banner") return null;
-            if (!els.has(id)) els.set(id, makeElement(id));
-            return els.get(id);
-        },
-        createElement: () => makeElement("created"),
-        body: { prepend: () => {} },
-    };
-}
-
-function node(id) {
-    return globalThis.document.getElementById(id);
-}
-
-// One line of the decoded details: its label and what is drawn under it.
-function decodedLine(label) {
-    return node("tx-detail-calldata-details")
-        .innerHTML.split('<div class="mb-2">')
-        .find((line) =>
-            line.startsWith(`<div class="text-muted">${label}</div>`),
-        );
-}
-
-beforeEach(() => {
-    globalThis.document = makeDocument();
-    globalThis.window = { location: { search: "" } };
-    state.blockscoutUrl = EXPLORER;
-    state.wallets = [
-        {
-            name: "Main",
-            type: "key",
-            addresses: [{ address: HOLDER, balance: "0.0000" }],
-        },
-    ];
-    state.trackedTokens = [{ address: TOKEN, symbol: "TDOL", decimals: 6 }];
-    state.viewData = {};
-    state.viewStack = [];
-    state.currentView = null;
-});
-
-test("a token transfer's decoded details name the token contract and the amount at its scale", async () => {
-    const [row] = await fetchRecentTransactions(HOLDER, EXPLORER, 1);
-    expect(row.to).toBe(RECIPIENT);
-    expect(row.contractAddress).toBe(TOKEN.toLowerCase());
-
-    transactionDetail.show(row);
-    // The decoded details are drawn once the explorer has answered.
-    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
-
-    expect(node("tx-detail-calldata-action").textContent).toBe(
-        "Token Transfer",
-    );
-    const token = decodedLine("Token");
-    expect(token).toContain('<div class="font-bold">TDOL</div>');
-    expect(token).toContain(TOKEN);
-    expect(token).not.toContain(RECIPIENT);
-    expect(decodedLine("Recipient")).toContain(RECIPIENT);
-    expect(decodedLine("Amount")).toContain(
-        '<div class="font-bold">1234.5000 TDOL</div>',
-    );
-});

tests/walletDefects.test.js

@@ -153,6 +153,18 @@ describe("the explanatory copy", () => {
         expect(all).not.toMatch(/\blost\b|\bstolen\b|\bgone\b/i);
     });
 
+    // The wallet holds the imported key but will not export it, so the copy
+    // must name every key the user may hold that reaches the addresses
+    // outside it.
+    test("it names the keys that reach the addresses outside this wallet", () => {
+        const body = defect.paragraphs.join(" ");
+
+        expect(body).toMatch(/private key export are disabled/);
+        expect(body).toMatch(
+            /with the extended private key you imported, if you kept a copy of it, or with the master extended private key or recovery phrase it came from, in software that repeats the same non-standard derivation/,
+        );
+    });
+
     test("it says the wallet is not deleted and what the user can do", () => {
         const body = defect.paragraphs.join(" ");