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
@@ -2143,12 +2148,7 @@ view would leave a wallet one click from deletion.
       Bytes that are not UTF-8 are shown as "This message is not text." For
       typed data, formatted domain/type/message fields (EIP-712). The primary
       type shown is the one ethers signs, derived from the typed data's `types`,
-      not the type the site states. The message shows only the fields that type
+      not the type the site states.
-      declares, at every level of nesting and in the type's order: ethers signs
-      those and drops any other key. Typed data carrying other keys is not
-      refused; each such key is listed after the message under "Not part of the
-      signature", by its place in the message (`details.note`,
-      `details[1].note`) and its value.
     - Token permission warning, at the top of the message (typed data whose
       primary type is `Permit`, as in EIP-2612, or one of Permit2's signature
       types): "⚠️ TOKEN PERMISSION: Signing this lets the spender below take the

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/views/approval.js

@@ -626,38 +626,10 @@ function permitWarningHtml(types, primaryType, domain, message) {
     return html;
 }
 
-// What ethers signs of a message value of type `type`: a struct keeps only
-// the fields its type declares, in the type's order, and a list is read item
-// by item. ethers drops every other key, so each one is added to `notSigned`
-// with its place in the message and its value, for the screen to list apart.
-function signedValue(types, type, value, path, notSigned) {
-    const itemType = type.replace(/\[\d*\]$/, "");
-    if (itemType !== type) {
-        if (!Array.isArray(value)) return value;
-        return value.map((item, i) =>
-            signedValue(types, itemType, item, `${path}[${i}]`, notSigned),
-        );
-    }
-    const fields = types[type];
-    if (!fields || !value || typeof value !== "object") return value;
-    const at = (name) => (path ? `${path}.${name}` : name);
-    for (const [key, val] of Object.entries(value)) {
-        if (!fields.some((f) => f.name === key)) notSigned.push([at(key), val]);
-    }
-    return Object.fromEntries(
-        fields.map((f) => [
-            f.name,
-            signedValue(types, f.type, value[f.name], at(f.name), notSigned),
-        ]),
-    );
-}
-
 // The typed data as the screen shows it. The primary type shown is the one
 // ethers signs, never the page's word for it; typedDataRefusal() keeps the two
-// from differing on anything that can be signed. The Message section shows
+// from differing on anything that can be signed. Only typed data that cannot
-// only what is signed, and any other key the page sent is listed after it as
+// be read at all is shown as raw text, and typedDataRefusal() refuses it.
-// not part of the signature. Only typed data that cannot be read at all is
-// shown as raw text, and typedDataRefusal() refuses it.
 function formatTypedDataHtml(jsonStr) {
     let data;
     let primaryType;
@@ -691,30 +663,12 @@ function formatTypedDataHtml(jsonStr) {
     html += `<div class="mb-2"><div class="text-muted">Primary type</div>`;
     html += `<div class="font-bold">${escapeHtml(primaryType)}</div></div>`;
 
-    const lineHtml = (key, val) =>
-        `<div><span class="text-muted">${escapeHtml(key)}:</span> <span class="break-all">${escapeHtml(display(val))}</span></div>`;
-
     if (data.message) {
-        const notSigned = [];
-        const message = signedValue(
-            data.types,
-            primaryType,
-            data.message,
-            "",
-            notSigned,
-        );
         html += `<div class="mb-2"><div class="text-muted">Message</div>`;
-        for (const [key, val] of Object.entries(message)) {
+        for (const [key, val] of Object.entries(data.message)) {
-            html += lineHtml(key, val);
+            html += `<div><span class="text-muted">${escapeHtml(key)}:</span> <span class="break-all">${escapeHtml(display(val))}</span></div>`;
         }
         html += `</div>`;
-        if (notSigned.length > 0) {
-            html += `<div class="mb-2"><div class="text-muted">Not part of the signature</div>`;
-            for (const [key, val] of notSigned) {
-                html += lineHtml(key, val);
-            }
-            html += `</div>`;
-        }
     }
 
     return html;
@@ -771,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) {

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

@@ -233,26 +233,6 @@ function warningOf(data) {
     return html.slice(0, html.indexOf(">Domain<"));
 }
 
-const NOT_SIGNED = ">Not part of the signature<";
-
-// The Message section alone, without the keys listed after it as not part of
-// the signature.
-function messageOf(data) {
-    const html = formatTypedDataHtml(JSON.stringify(data));
-    const end = html.indexOf(NOT_SIGNED);
-    return html.slice(
-        html.indexOf(">Message<"),
-        end === -1 ? html.length : end,
-    );
-}
-
-// The keys listed as not part of the signature, or "" when there are none.
-function notSignedOf(data) {
-    const html = formatTypedDataHtml(JSON.stringify(data));
-    const start = html.indexOf(NOT_SIGNED);
-    return start === -1 ? "" : html.slice(start);
-}
-
 function request(data) {
     return { method: "eth_signTypedData_v4", typedData: JSON.stringify(data) };
 }
@@ -332,8 +312,7 @@ describe("the warning reads only the fields the signed type declares", () => {
     });
 
     // A Permit whose type names its spender `operator`; the page adds a
-    // `spender` key the type does not declare. The Message section names
+    // `spender` key the type does not declare.
-    // only the operator, and the added key is listed apart as not signed.
     test("an added key is not named as the spender", () => {
         const data = {
             ...PERMIT,
@@ -352,10 +331,6 @@ describe("the warning reads only the fields the signed type declares", () => {
         const warning = warningOf(data);
         expect(warning).toContain(WARNING);
         expect(warning).not.toContain(DECOY);
-        expect(messageOf(data)).toContain(SPENDER);
-        expect(messageOf(data)).not.toContain(DECOY);
-        expect(notSignedOf(data)).toContain("spender:");
-        expect(notSignedOf(data)).toContain(DECOY);
     });
 
     // The permit for a Uniswap v3 position NFT: a `Permit` with no amount
@@ -396,47 +371,6 @@ describe("the warning reads only the fields the signed type declares", () => {
     });
 });
 
-// ethers signs only the declared fields at every level of nesting, so an
-// added key inside a struct is not signed either. Such typed data is not
-// refused; the added key is listed apart, with where it sits in the message.
-describe("the Message section shows only the fields that are signed", () => {
-    const NOTE = "This signature only signs you in";
-
-    test("an added key inside a nested struct is listed as not signed", () => {
-        const data = {
-            ...PERMIT_SINGLE,
-            message: {
-                ...PERMIT_SINGLE.message,
-                details: { ...PERMIT_SINGLE.message.details, note: NOTE },
-            },
-        };
-        expect(messageOf(data)).toContain("expiration");
-        expect(messageOf(data)).not.toContain(NOTE);
-        expect(notSignedOf(data)).toContain("details.note:");
-        expect(notSignedOf(data)).toContain(NOTE);
-        expect(typedDataRefusal(request(data))).toBeNull();
-    });
-
-    test("an added key inside a list item is listed with its index", () => {
-        const [first, second] = PERMIT_BATCH.message.details;
-        const data = {
-            ...PERMIT_BATCH,
-            message: {
-                ...PERMIT_BATCH.message,
-                details: [first, { ...second, note: NOTE }],
-            },
-        };
-        expect(messageOf(data)).not.toContain(NOTE);
-        expect(notSignedOf(data)).toContain("details[1].note:");
-        expect(notSignedOf(data)).toContain(NOTE);
-    });
-
-    test("nothing is listed apart when every key is declared", () => {
-        expect(notSignedOf(PERMIT_SINGLE)).toBe("");
-        expect(notSignedOf(LOGIN)).toBe("");
-    });
-});
-
 describe("the primary type shown is the one ethers signs", () => {
     // A drain stated as a sign-in: the page says Login, the types say
     // PermitSingle, and ethers would sign PermitSingle.