diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 31087ae031..5a9364dc52 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -35,6 +35,10 @@ proposal is successful, the changes it released will be moved from this file to #### Security +- The Encode ICRC-1 Account utility now reads a large decimal subaccount ID as + a decimal number. Before, it read it as hexadecimal and returned a different + account. + #### Not Published ### Operations diff --git a/frontend/src/lib/components/alfred/BuildIcrcAccountUtil.svelte b/frontend/src/lib/components/alfred/BuildIcrcAccountUtil.svelte index bbf4fafd7f..a3619e4a2a 100644 --- a/frontend/src/lib/components/alfred/BuildIcrcAccountUtil.svelte +++ b/frontend/src/lib/components/alfred/BuildIcrcAccountUtil.svelte @@ -28,6 +28,18 @@ return sub; }; + // The largest 32-byte value, 2**256 - 1, has 78 decimal digits. + const MAX_DECIMAL_SUBACCOUNT_DIGITS = 78; + + // A decimal ID is the big-endian value of the 32-byte subaccount. + const parseDecimalSubAccount = (decimal: string): SubAccount => { + const digits = decimal.replace(/^0+/, "") || "0"; + if (digits.length > MAX_DECIMAL_SUBACCOUNT_DIGITS) { + throw new Error($i18n.alfred.build_icrc_account_subaccount_error); + } + return parseHexSubAccount(BigInt(digits).toString(16)); + }; + const parseSubAccount = (input: string): SubAccount => { const trimmed = input.trim(); @@ -42,11 +54,7 @@ const isDecimalNumber = /^\d+$/.test(trimmed); if (isDecimalNumber) { - const num = Number(trimmed); - if (num > Number.MAX_SAFE_INTEGER) { - return parseHexSubAccount(trimmed); - } - return SubAccount.fromID(num); + return parseDecimalSubAccount(trimmed); } throw new Error($i18n.alfred.build_icrc_account_subaccount_error); diff --git a/frontend/src/tests/e2e/alfred-icrc-account.spec.ts b/frontend/src/tests/e2e/alfred-icrc-account.spec.ts new file mode 100644 index 0000000000..6f84cc9a00 --- /dev/null +++ b/frontend/src/tests/e2e/alfred-icrc-account.spec.ts @@ -0,0 +1,122 @@ +import { AppPo } from "$tests/page-objects/App.page-object"; +import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object"; +import { signInWithNewUser, step } from "$tests/utils/e2e.test-utils"; +import { expect, test, type Page } from "@playwright/test"; + +// A canister principal. It keeps the expected account text short and fixed. +const OWNER = "rrkah-fqaaa-aaaaa-aaaaq-cai"; + +// The largest u64, and a common account ID. It is above +// Number.MAX_SAFE_INTEGER, so it is the value the old code read as hexadecimal. +const U64_MAX = "18446744073709551615"; + +// The account for the decimal value of U64_MAX. +const U64_MAX_AS_DECIMAL = `${OWNER}-pxt75ay.ffffffffffffffff`; + +// The account the old code produced. It read the same digits as hexadecimal. +const U64_MAX_AS_HEX = `${OWNER}-rtjdh2q.18446744073709551615`; + +// 2**256 - 1 is the largest subaccount. It has 78 decimal digits. +const MAX_SUBACCOUNT = (2n ** 256n - 1n).toString(); +const MAX_SUBACCOUNT_ACCOUNT = `${OWNER}-37tqaua.${"f".repeat(64)}`; + +// One digit more than the largest subaccount holds. +const TOO_LARGE_SUBACCOUNT = "1" + "0".repeat(78); + +// A paste that is far longer than any real ID. The parser must reject it +// without a long BigInt conversion. +const VERY_LONG_INPUT = "1".repeat(200000); + +// The panel drops a key that arrives within this delay of the one before it. +const KEY_DEBOUNCE_MS = 100; + +const SUBACCOUNT_ERROR = + "Invalid subaccount. Use a number, a 0x-prefixed hex, or a 64-char hex string."; + +const openEncodeUtil = async (page: Page) => { + const palette = page.getByTestId("alfred-component"); + + await page.getByTestId("open-alfred").click(); + await expect(palette).toBeVisible(); + await palette.getByTestId("alfred-input").fill("Encode ICRC-1 Account"); + await palette.getByTestId("alfred-result-button").first().click(); + await expect(palette.getByTestId("alfred-util-principal")).toBeVisible(); +}; + +// The parse function is pure, and the component spec at +// `src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts` covers every +// value. This spec covers what that one cannot: the util is reachable through +// the command palette, and a very long paste does not freeze the tab. +// +// `test.fixme` because no local replica runs on the review machine. +// `dfx start --pocketic` fails: dfx 0.32.0 ships pocket-ic-server 13.0.0, and +// that server rejects the saved snsdemo snapshot state. Remove `.fixme` when +// the replica starts again. +test.fixme( + "Encode ICRC-1 Account reads a decimal subaccount ID as decimal", + async ({ page, browser }) => { + const appPo = new AppPo(PlaywrightPageObjectElement.fromPage(page)); + const palette = page.getByTestId("alfred-component"); + const output = palette.getByTestId("alfred-util-hex-output"); + const error = palette.locator(".error-message"); + const principalInput = palette.getByTestId("alfred-util-principal"); + const subaccountInput = palette.getByTestId("alfred-util-subaccount"); + + await page.goto("/"); + await expect(page).toHaveTitle("Portfolio | Network Nervous System"); + + await step("Sign in"); + await signInWithNewUser({ page, context: browser.contexts()[0] }); + + await step("Open the Encode ICRC-1 Account util from the palette"); + await openEncodeUtil(page); + + await step("A small decimal ID is unchanged"); + await principalInput.fill(OWNER); + await subaccountInput.fill("12345"); + await expect(output).toHaveText(`${OWNER}-xhbeadi.3039`); + + await step("A decimal ID above MAX_SAFE_INTEGER reads as decimal"); + await subaccountInput.fill(U64_MAX); + await expect(output).toHaveText(U64_MAX_AS_DECIMAL); + + await step("That ID is never read as hexadecimal"); + // This is the whole finding. The old code produced U64_MAX_AS_HEX here, and + // a deposit to it landed on another subaccount. + await expect(output).not.toHaveText(U64_MAX_AS_HEX); + + await step("Leading zeros do not change the account"); + await subaccountInput.fill(`${"0".repeat(40)}12345`); + await expect(output).toHaveText(`${OWNER}-xhbeadi.3039`); + + await step("The largest subaccount, 2**256 - 1, is accepted"); + await subaccountInput.fill(MAX_SUBACCOUNT); + await expect(output).toHaveText(MAX_SUBACCOUNT_ACCOUNT); + + await step("A decimal ID above 32 bytes shows the error"); + await subaccountInput.fill(TOO_LARGE_SUBACCOUNT); + await expect(error).toHaveText(SUBACCOUNT_ERROR); + await expect(output).toBeHidden(); + + await step("A 0x-prefixed hex ID still works"); + await subaccountInput.fill("0xffffffffffffffff"); + await expect(output).toHaveText(U64_MAX_AS_DECIMAL); + + await step("A very long paste shows the error and does not freeze the tab"); + await subaccountInput.fill(VERY_LONG_INPUT); + await expect(error).toHaveText(SUBACCOUNT_ERROR); + + await step("The util still answers after that paste"); + await subaccountInput.fill(U64_MAX); + await expect(output).toHaveText(U64_MAX_AS_DECIMAL); + + await step("Escape closes the util and then the palette"); + await page.keyboard.press("Escape"); + await expect(palette.getByTestId("alfred-input")).toBeVisible(); + await page.waitForTimeout(KEY_DEBOUNCE_MS); + await page.keyboard.press("Escape"); + await expect(palette).toBeHidden(); + + await appPo.waitForNotBusy(); + } +); diff --git a/frontend/src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts b/frontend/src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts index 7e3a0c5692..a9bc52b53a 100644 --- a/frontend/src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts +++ b/frontend/src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts @@ -101,18 +101,51 @@ describe("BuildIcrcAccountUtil", () => { ); }); - it("should treat all-digit string exceeding MAX_SAFE_INTEGER as hex", async () => { + it("should encode a decimal subaccount ID above MAX_SAFE_INTEGER as decimal", async () => { const { container } = render(BuildIcrcAccountUtil); + // 99999999999999999 is 0x016345785d89ffff. const bigDigitString = "99999999999999999"; + expect(Number(bigDigitString)).toBeGreaterThan(Number.MAX_SAFE_INTEGER); + + await setInputValues(container, { + principal: testPrincipal, + subaccount: bigDigitString, + }); + + const output = getOutput(container); + expect(output).not.toBeNull(); + expect(output?.textContent).toBe( + expectedIcrcAccount({ + principal: testPrincipal, + subaccountBytes: hexStringToUint8Array( + "016345785d89ffff".padStart(64, "0") + ), + }) + ); + }); + + it("should not read a decimal subaccount ID as hex", async () => { + const { container } = render(BuildIcrcAccountUtil); + + const bigDigitString = "18446744073709551615"; await setInputValues(container, { principal: testPrincipal, subaccount: bigDigitString, }); + const uint64MaxBytes = new Uint8Array(32); + uint64MaxBytes.fill(0xff, 24); + const output = getOutput(container); expect(output).not.toBeNull(); expect(output?.textContent).toBe( + expectedIcrcAccount({ + principal: testPrincipal, + subaccountBytes: uint64MaxBytes, + }) + ); + expect(output?.textContent).not.toBe( expectedIcrcAccount({ principal: testPrincipal, subaccountBytes: hexStringToUint8Array( @@ -122,6 +155,48 @@ describe("BuildIcrcAccountUtil", () => { ); }); + it("should show error for a decimal subaccount ID above 32 bytes", async () => { + const { container } = render(BuildIcrcAccountUtil); + + await setInputValues(container, { + principal: testPrincipal, + subaccount: "9".repeat(78), + }); + + expect(getOutput(container)).toBeNull(); + expect(getError(container)).not.toBeNull(); + }); + + it("should show error for a very long digit string", async () => { + const { container } = render(BuildIcrcAccountUtil); + + await setInputValues(container, { + principal: testPrincipal, + subaccount: "1".repeat(100000), + }); + + expect(getOutput(container)).toBeNull(); + expect(getError(container)).not.toBeNull(); + }); + + it("should ignore leading zeros in a decimal subaccount ID", async () => { + const { container } = render(BuildIcrcAccountUtil); + + await setInputValues(container, { + principal: testPrincipal, + subaccount: "0".repeat(100) + "12345", + }); + + const output = getOutput(container); + expect(output).not.toBeNull(); + expect(output?.textContent).toBe( + expectedIcrcAccount({ + principal: testPrincipal, + subaccountBytes: SubAccount.fromID(12345).toUint8Array(), + }) + ); + }); + it("should encode with a 32-byte hex subaccount", async () => { const { container } = render(BuildIcrcAccountUtil);