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
@@ -2663,7 +2668,8 @@ plaintext list of anyone's domain names.
 When a dApp on a blocklisted domain requests a wallet connection, transaction
 approval, or signature, the approval popup displays a prominent red warning
 banner alerting the user. The domain checker matches exact hostnames and all
-parent domains (subdomain matching).
+parent domains (subdomain matching), and checks a hostname written with a
+trailing dot (`name.`) as `name`.
 
 #### Transaction Decoding
 

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 site used the older eth_sign method. " +
-                "This can be used to sign transactions that drain your funds. " +
+                "The data will be signed as a personal message.";
-                "Only proceed if you fully understand what you are signing.";
         }
 
         if (findPendingApproval(origin, "sign")) {

src/popup/index.html

@@ -1716,7 +1716,10 @@
                     <div class="text-xs text-muted mb-1">
                         Address that will be shared:
                     </div>
-                    <div id="approve-address" class="text-xs mb-2"></div>
+                    <div
+                        id="approve-address"
+                        class="text-xs flex items-center mb-2"
+                    ></div>
                 </div>
                 <div class="mb-3">
                     <label

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/shared/phishingDomains.js

@@ -110,11 +110,14 @@ function hashListed(hash) {
  * Generate hostname variants for subdomain matching.
  * "sub.evil.com" yields ["sub.evil.com", "evil.com"].
  *
+ * A trailing dot is dropped: "evil.com." is the fully qualified spelling of
+ * "evil.com", and the browser loads the same site from either.
+ *
  * @param {string} hostname
  * @returns {string[]}
  */
 function hostnameVariants(hostname) {
-    const h = hostname.toLowerCase();
+    const h = hostname.toLowerCase().replace(/\.$/, "");
     const variants = [h];
     const parts = h.split(".");
     // Parent domains: a.b.c.d -> b.c.d, c.d

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 " +
+            "Signing, sending and private key export are disabled for this " +
-                "do descend from the extended private key you imported, so " +
+                "wallet. Anything these addresses hold can be reached outside " +
-                "anything they hold is still reachable by software that " +
+                "this wallet with the extended private key you imported, if " +
-                "repeats the same non-standard derivation. Check them in a " +
+                "you kept a copy of it, or with the master extended private " +
-                "block explorer before deciding what to do.",
+                "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/e2e/run.js

@@ -3857,9 +3857,6 @@ test("eth_requestAccounts approved returns the selected address (#183)", async (
             shown.toLowerCase().includes(env.expectedAddress.toLowerCase()),
             "the site prompt shows the wrong address: " + JSON.stringify(shown),
         );
-        // The text above is there even when the row is too narrow to show it
-        // 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

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/phishingDomains.test.js

@@ -89,6 +89,12 @@ describe("vendored blocklist", () => {
         expect(isPhishingDomain(LISTED[0].toUpperCase())).toBe(true);
     });
 
+    test("a hostname ending in a dot is the same host", () => {
+        expect(isPhishingDomain(LISTED[1] + ".")).toBe(true);
+        expect(isPhishingDomain("wallet." + LISTED[1] + ".")).toBe(true);
+        expect(isPhishingDomain(CLEAN[0] + ".")).toBe(false);
+    });
+
     test("returns false for an empty or missing hostname", () => {
         expect(isPhishingDomain("")).toBe(false);
         expect(isPhishingDomain(null)).toBe(false);
@@ -133,6 +139,13 @@ describe("hostnameVariants", () => {
     test("lowercases hostnames", () => {
         expect(hostnameVariants("Evil.COM")).toEqual(["evil.com"]);
     });
+
+    test("drops the trailing dot of a fully qualified hostname", () => {
+        expect(hostnameVariants("sub.evil.com.")).toEqual([
+            "sub.evil.com",
+            "evil.com",
+        ]);
+    });
 });
 
 describe("domain hashing", () => {

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(" ");