" : codePointMark(c), + ); +} + // The type ethers will sign typed data as. ethers does not read the page's // `primaryType`: it takes the one struct in `types` that no other struct // refers to. Throws when the types name no such single struct, which ethers @@ -657,15 +686,33 @@ function showSignApproval(details) { ? "Typed data (EIP-712)" : "Personal message"; + // A personal message is signed as the bytes its hex encodes, so the hex + // is shown as well as any text it decodes to, and that text is laid out + // left to right in the order of its bytes. Signing reads the bytes from + // the hex, so a message that is not hex cannot be signed: it is shown as + // the text it is, and refused. + let refusal = null; + $("approve-sign-hex-section").classList.add("hidden"); + $("approve-sign-message").classList.toggle("am-byte-order", !isTyped); if (isTyped) { $("approve-sign-message").innerHTML = formatTypedDataHtml(sp.typedData); - } else { + refusal = typedDataRefusal(sp); + } else if (isHexMessage(sp.message)) { const decoded = decodeHexMessage(sp.message); if (decoded !== null) { - $("approve-sign-message").textContent = decoded; + $("approve-sign-message").innerHTML = + markInvisibleCharacters(decoded); } else { - $("approve-sign-message").textContent = sp.message; + $("approve-sign-message").textContent = "This message is not text."; } + $("approve-sign-hex").textContent = sp.message; + $("approve-sign-hex-section").classList.remove("hidden"); + } else { + $("approve-sign-message").innerHTML = markInvisibleCharacters( + sp.message, + ); + refusal = + "This message is plain text, not hex, so it cannot be signed."; } // Display danger warning for eth_sign (raw hash signing) @@ -687,7 +734,6 @@ function showSignApproval(details) { showView("approve-sign"); attachCopyHandlers("view-approve-sign"); - const refusal = typedDataRefusal(sp); if (refusal) { showError("approve-sign-error", refusal); $("btn-approve-sign").disabled = true; diff --git a/src/shared/symbolSpoof.js b/src/shared/symbolSpoof.js index af908fe..5caea3d 100644 --- a/src/shared/symbolSpoof.js +++ b/src/shared/symbolSpoof.js @@ -34,6 +34,11 @@ function normalizeAddress(addr) { return (addr || "").toLowerCase(); } +// The characters that paint nothing; normalizeSymbol below says which they +// are. The signature screen marks them in a personal message +// (src/popup/views/approval.js). +const INVISIBLE_CHARACTERS = /[\p{Cf}\p{Default_Ignorable_Code_Point}\x7F]/gu; + // Fold a symbol onto what a user actually sees, and no further: // // NFKC collapses compatibility variants that render as the ASCII @@ -82,7 +87,7 @@ function normalizeAddress(addr) { function normalizeSymbol(symbol) { return String(symbol || "") .normalize("NFKC") - .replace(/[\p{Cf}\p{Default_Ignorable_Code_Point}\x7F]/gu, "") + .replace(INVISIBLE_CHARACTERS, "") .trim() .toUpperCase(); } @@ -104,5 +109,6 @@ function isSpoofedSymbol(symbol, contractAddress) { } module.exports = { + INVISIBLE_CHARACTERS, isSpoofedSymbol, }; diff --git a/tests/e2e/run.js b/tests/e2e/run.js index e547926..4990bc6 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -3232,6 +3232,64 @@ test("personal_sign rejected returns a rejection to the page (#183)", async (env ); }); +// A right-to-left character must not move the characters around it: U+05C3 +// between "5" and "00" would otherwise put "500" on screen before it. A +// paragraph separator (U+2029) before it, left in the text, would end the +// byte-order layout and bring that back +// (https://git.eeqj.de/sneak/AutistMask/issues/403). +test("a personal message is laid out in the order of its bytes (#403)", async (env) => { + const rightToLeft = String.fromCodePoint(0x05c3); + const text = + "Sign in" + + String.fromCodePoint(0x2029) + + "Pay 5" + + rightToLeft + + "00 ETH"; + await startRequest(env.dapp, "sign-bidi", "personal_sign", [ + hexlify(toUtf8Bytes(text)), + env.expectedAddress, + ]); + const popup = await waitForApprovalWindow(env.ctx); + await visible(popup, "#view-approve-sign"); + + // The text on screen, marks included, and the left edge of each of its + // characters, in byte order. A character the browser's fonts draw with + // no width shares its neighbour's edge. + const shown = await popup.evaluate(() => { + const message = document.getElementById("approve-sign-message"); + const walker = document.createTreeWalker(message, NodeFilter.SHOW_TEXT); + const range = document.createRange(); + let text = ""; + const lefts = []; + for (let node = walker.nextNode(); node; node = walker.nextNode()) { + for (let i = 0; i < node.length; i++) { + range.setStart(node, i); + range.setEnd(node, i + 1); + lefts.push(range.getBoundingClientRect().left); + } + text += node.data; + } + return { text, lefts }; + }); + await clickAndClose(popup, "#btn-reject-sign"); + await assertUserRejection( + env.dapp, + "sign-bidi", + "the byte-order personal_sign rejection", + ); + + assert( + shown.text === "Sign inU+2029Pay 5" + rightToLeft + "00 ETH", + "the paragraph separator is not shown as a mark: " + + JSON.stringify(shown.text), + ); + assert( + shown.lefts.every((left, i) => i === 0 || left >= shown.lefts[i - 1]), + "the personal message is not laid out in byte order: " + + JSON.stringify(shown.lefts), + ); +}); + test("eth_signTypedData_v4 signs, and the signature recovers (#183)", async (env) => { await startRequest(env.dapp, "typed", "eth_signTypedData_v4", [ env.expectedAddress, diff --git a/tests/personalSignDisplay.test.js b/tests/personalSignDisplay.test.js new file mode 100644 index 0000000..7402f74 --- /dev/null +++ b/tests/personalSignDisplay.test.js @@ -0,0 +1,229 @@ +// The signature prompt shows a personal message as the bytes that are signed +// (https://git.eeqj.de/sneak/AutistMask/issues/403): the raw data in hex, the +// text it decodes to with control characters, line and paragraph separators +// and characters that paint nothing marked rather than obeyed, markup shown as +// text, laid out in byte order, and a message that is not hex as plain text +// that cannot be signed. +// +// Driven against a minimal DOM stub in the shape +// tests/approvalOrigin.test.js uses. That the layout keeps right-to-left +// characters in byte order needs a real browser: tests/e2e/run.js checks it. + +globalThis.chrome = { + storage: { local: { get: async () => ({}), set: async () => {} } }, +}; + +const { hexlify, toUtf8Bytes } = require("ethers"); +const { state } = require("../src/shared/state"); +const approval = require("../src/popup/views/approval"); + +const FROM = "0x0000000000000000000000000000000000000a11"; + +// Built from their code points so that this file holds none of them. +const RIGHT_TO_LEFT_OVERRIDE = String.fromCodePoint(0x202e); +const POP_DIRECTIONAL_FORMATTING = String.fromCodePoint(0x202c); +const ZERO_WIDTH_SPACE = String.fromCodePoint(0x200b); +const VARIATION_SELECTOR_1 = String.fromCodePoint(0xfe00); +const VARIATION_SELECTOR_17 = String.fromCodePoint(0xe0100); +const HANGUL_FILLER = String.fromCodePoint(0x3164); +const LINE_SEPARATOR = String.fromCodePoint(0x2028); +const PARAGRAPH_SEPARATOR = String.fromCodePoint(0x2029); + +function makeElement(id) { + const classes = new Set(); + return { + id, + textContent: "", + value: "", + innerHTML: "", + disabled: false, + 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: (n, force) => { + const on = force === undefined ? !classes.has(n) : force; + if (on) classes.add(n); + else classes.delete(n); + return on; + }, + }, + addEventListener: () => {}, + querySelectorAll: () => [], + appendChild: () => {}, + }; +} + +function makeDocument() { + const els = new Map(); + return { + getElementById(id) { + 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); +} + +// Open the signature prompt for a personal_sign of `message`, the way the +// popup does: it asks the background for the approval and show() draws it. +async function openPersonalSign(message) { + globalThis.document = makeDocument(); + globalThis.window = { location: { search: "" } }; + globalThis.chrome.runtime = { + connect: () => ({ postMessage: () => {} }), + sendMessage: (msg, reply) => { + if (!reply) return; + if (msg.type !== "AUTISTMASK_GET_APPROVAL") return reply(null); + reply({ + type: "sign", + origin: "https://dapp.example", + isPhishingDomain: false, + approvedFrom: FROM, + signParams: { method: "personal_sign", message, from: FROM }, + }); + }, + }; + approval.init({}); + await approval.show(1); +} + +// The message box's markup as the text a reader sees: tags dropped. +function shownMessage() { + return node("approve-sign-message").innerHTML.replace(/<[^>]*>/g, ""); +} + +beforeEach(() => { + state.wallets = []; + state.activeAddress = FROM; + state.viewData = {}; + state.viewStack = []; + state.currentView = null; +}); + +test("a right-to-left override is marked, so the text reads in byte order", async () => { + // Obeyed, the override shows "0001" as "1000". + const text = + "Pay " + + RIGHT_TO_LEFT_OVERRIDE + + "0001" + + POP_DIRECTIONAL_FORMATTING + + " ETH"; + await openPersonalSign(hexlify(toUtf8Bytes(text))); + const html = node("approve-sign-message").innerHTML; + expect(html).not.toContain(RIGHT_TO_LEFT_OVERRIDE); + expect(html).not.toContain(POP_DIRECTIONAL_FORMATTING); + expect(shownMessage()).toBe("Pay U+202E0001U+202C ETH"); +}); + +test("a zero-width character is marked", async () => { + await openPersonalSign( + hexlify(toUtf8Bytes("pay" + ZERO_WIDTH_SPACE + "pal.com")), + ); + expect(node("approve-sign-message").innerHTML).not.toContain( + ZERO_WIDTH_SPACE, + ); + expect(shownMessage()).toBe("payU+200Bpal.com"); +}); + +test("variation selectors and a Hangul filler are marked", async () => { + // Each paints nothing, so a page could hide bytes after "Sign in". + await openPersonalSign( + hexlify( + toUtf8Bytes( + "Sign in" + + VARIATION_SELECTOR_1 + + VARIATION_SELECTOR_17 + + HANGUL_FILLER, + ), + ), + ); + expect(shownMessage()).toBe("Sign inU+FE00U+E0100U+3164"); +}); + +test("the message is laid out in byte order", async () => { + await openPersonalSign(hexlify(toUtf8Bytes("Hello"))); + expect( + node("approve-sign-message").classList.contains("am-byte-order"), + ).toBe(true); +}); + +test("a control character other than a line feed is marked", async () => { + await openPersonalSign(hexlify(toUtf8Bytes("a\u0000b\tc"))); + expect(shownMessage()).toBe("aU+0000bU+0009c"); +}); + +test("line and paragraph separators are marked", async () => { + // Left in the text, a paragraph separator would end the byte-order + // layout for everything after it. + await openPersonalSign( + hexlify(toUtf8Bytes("a" + LINE_SEPARATOR + "b" + PARAGRAPH_SEPARATOR)), + ); + const html = node("approve-sign-message").innerHTML; + expect(html).not.toContain(LINE_SEPARATOR); + expect(html).not.toContain(PARAGRAPH_SEPARATOR); + expect(shownMessage()).toBe("aU+2028bU+2029"); +}); + +test("a line feed is shown as a line break", async () => { + await openPersonalSign(hexlify(toUtf8Bytes("Sign in\nNonce: 7"))); + expect(node("approve-sign-message").innerHTML).toBe("Sign in
Nonce: 7"); +}); + +// The message box is written as HTML, so a site's markup has to arrive there +// escaped, as the text it is. +const MARKUP = "x