diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 2f5f7a2595e..ee2609b25f4 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/CHANGELOG-Nns-Dapp-unreleased.md @@ -38,6 +38,10 @@ proposal is successful, the changes it released will be moved from this file to #### Security +- A proposal payload string that reads `__UNDEFINED__` now shows as that + string. Before, it showed as `undefined`. A quote in front of it could also + garble the rest of the payload view. + #### Not Published ### Operations diff --git a/frontend/src/lib/utils/utils.ts b/frontend/src/lib/utils/utils.ts index 1839adc9fb5..dc21e96688f 100644 --- a/frontend/src/lib/utils/utils.ts +++ b/frontend/src/lib/utils/utils.ts @@ -9,8 +9,79 @@ export const isPrincipal = (value: unknown): value is Principal => typeof value === "object" && (value as Principal)?._isPrincipal === true; /** - * Transform bigint to string to avoid serialization error. - * devMode transforms 123n -> "BigInt(123)" + * Maps a value to the value that is serialized in its place. + * Returns the value itself when no mapping applies. + */ +const replaceJsonValue = ( + value: unknown, + options?: { + devMode?: boolean; + } +): unknown => { + switch (typeof value) { + case "function": + return "f () { ... }"; + case "symbol": + return value.toString(); + case "object": { + // Represent Principals as strings rather than as byte arrays when serializing to JSON strings + if (isPrincipal(value)) { + const asText = value.toString(); + // To not stringify NOT Principal instance that contains _isPrincipal field + return asText === "[object Object]" ? value : asText; + } + + // For proposal rendering, historically we display {principal: "1234"}, but in stringified JSON, principals are now encoded as {"__principal__": "1234"}. + if (nonNullish(value) && JSON_KEY_PRINCIPAL in value) { + return value[JSON_KEY_PRINCIPAL]; + } + + // optimistic hash stringifying + if (Array.isArray(value) && isHash(value)) { + return bytesToHexString(value); + } + + if (value instanceof Promise) { + return "Promise(...)"; + } + + if (value instanceof ArrayBuffer) { + return new Uint8Array(value).toString(); + } + + break; + } + case "bigint": { + if (options?.devMode !== undefined && options.devMode) { + return `BigInt('${value.toString()}')`; + } + return value.toString(); + } + } + return value; +}; + +// The `indentation` that `JSON.stringify` accepts, clamped the same way. +// `JSON.stringify` treats NaN as 0. `Math.max`/`Math.min` already clamp +// `Infinity` to 10 and `-Infinity` to 0, so only NaN needs a guard here. +const jsonGap = (indentation: number): string => + " ".repeat( + Number.isNaN(indentation) + ? 0 + : Math.min(10, Math.max(0, Math.floor(indentation))) + ); + +/** + * Serializes a value the way `JSON.stringify` does, with two differences: + * - `undefined` is written as the bare word `undefined` instead of being dropped. + * - the values that `replaceJsonValue` maps are written in their mapped form. + * + * The word `undefined` is written only for a value that is `undefined`. A string + * of any content is written by `JSON.stringify`, so no payload string can read + * as `undefined`. + * + * A bigint becomes a string, to avoid a serialization error. With `devMode`, + * 123n becomes "BigInt('123')". */ export const stringifyJson = ( value: unknown, @@ -19,58 +90,90 @@ export const stringifyJson = ( devMode?: boolean; } ): string => { - const __UNDEFINED__ = "__UNDEFINED__"; - const result = JSON.stringify( - value, - (_, value) => { - switch (typeof value) { - case "function": - return "f () { ... }"; - case "symbol": - return value.toString(); - case "object": { - // Represent Principals as strings rather than as byte arrays when serializing to JSON strings - if (isPrincipal(value)) { - const asText = value.toString(); - // To not stringify NOT Principal instance that contains _isPrincipal field - return asText === "[object Object]" ? value : asText; - } - - // For proposal rendering, historically we display {principal: "1234"}, but in stringified JSON, principals are now encoded as {"__principal__": "1234"}. - if (nonNullish(value) && JSON_KEY_PRINCIPAL in value) { - return value[JSON_KEY_PRINCIPAL]; - } - - // optimistic hash stringifying - if (Array.isArray(value) && isHash(value)) { - return bytesToHexString(value); - } - - if (value instanceof Promise) { - return "Promise(...)"; - } - - if (value instanceof ArrayBuffer) { - return new Uint8Array(value).toString(); - } - - break; - } - case "bigint": { - if (options?.devMode !== undefined && options.devMode) { - return `BigInt('${value.toString()}')`; - } - return value.toString(); - } + const gap = jsonGap(options?.indentation ?? 0); + // `JSON.stringify` writes a space after the key colon only when it indents. + const colonSpace = gap === "" ? "" : " "; + // The chain of objects that are currently open, to detect a circular structure. + const ancestors: object[] = []; + + const wrap = ( + entries: string[], + open: string, + close: string, + level: number + ): string => { + if (entries.length === 0) { + return `${open}${close}`; + } + if (gap === "") { + return `${open}${entries.join(",")}${close}`; + } + const indent = gap.repeat(level + 1); + return `${open}\n${indent}${entries.join( + `,\n${indent}` + )}\n${gap.repeat(level)}${close}`; + }; + + const serialize = (raw: unknown, key: string, level: number): string => { + // `JSON.stringify` calls `toJSON` before the replacer. `Principal` has one. + const value = replaceJsonValue( + typeof raw === "object" && + raw !== null && + typeof (raw as { toJSON?: unknown }).toJSON === "function" + ? (raw as { toJSON: (key: string) => unknown }).toJSON(key) + : raw, + options + ); + + if (value === undefined) { + return "undefined"; + } + if ( + typeof value !== "object" || + value === null || + value instanceof String || + value instanceof Number || + value instanceof Boolean + ) { + return JSON.stringify(value); + } + + if (ancestors.includes(value)) { + throw new TypeError("Converting circular structure to JSON"); + } + ancestors.push(value); + try { + if (Array.isArray(value)) { + // `Array.from` visits every index. `map` skips a hole and keeps it a hole. + return wrap( + Array.from(value, (item, index) => + serialize(item, `${index}`, level + 1) + ), + "[", + "]", + level + ); } - return value === undefined ? __UNDEFINED__ : value; - }, - options?.indentation ?? 0 - ); + const record = value as Record; + return wrap( + Object.keys(record).map( + (name) => + `${JSON.stringify(name)}:${colonSpace}${serialize( + record[name], + name, + level + 1 + )}` + ), + "{", + "}", + level + ); + } finally { + ancestors.pop(); + } + }; - return ( - result?.replace(new RegExp(`"${__UNDEFINED__}"`, "g"), "undefined") ?? "" - ); + return serialize(value, "", 0); }; /** diff --git a/frontend/src/tests/e2e/proposal-payload-undefined-token.spec.ts b/frontend/src/tests/e2e/proposal-payload-undefined-token.spec.ts new file mode 100644 index 00000000000..c523bf21001 --- /dev/null +++ b/frontend/src/tests/e2e/proposal-payload-undefined-token.spec.ts @@ -0,0 +1,148 @@ +import { AppPo } from "$tests/page-objects/App.page-object"; +import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object"; +import { createDummyProposal } from "$tests/utils/e2e.nns-proposals.test-utils"; +import { + disableCssAnimations, + signInWithNewUser, + step, +} from "$tests/utils/e2e.test-utils"; +import { ProposalStatus, Topic } from "@icp-sdk/canisters/nns"; +import { expect, test } from "@playwright/test"; + +// Whoever submits a proposal writes the payload of the proposal. The payload +// carries strings that the proposer chooses. `stringifyJson` used to serialize +// an absent value as the token below and then replace that token in the text, +// so a payload string equal to the token showed as the bare word `undefined`. +// +// This test submits a real proposal whose payload holds that string, then reads +// the three surfaces that show a payload: the tree view, the raw view and the +// copy button. On each surface the string must show as the plain quoted string +// it is. +// +// The unit tests in frontend/src/tests/lib/utils/utils.spec.ts cover the other +// shapes of the same attack: a key named after the token, and a value that +// carries a quote in front of the token. +const TOKEN = "__UNDEFINED__"; + +// The app loads this script to create the dummy proposals of a testnet. The +// test serves its own version, so that one proposal carries the payload above. +// See frontend/src/lib/api/dev.api.ts and +// frontend/static/assets/libs/dummy-proposals.utils.js. +const DUMMY_PROPOSALS_SCRIPT_PATH = "**/assets/libs/dummy-proposals.utils.js"; + +const dummyProposalsModule = ` +export const makeDummyProposals = async ({ neuronId, canister }) => { + await canister.makeProposal({ + neuronId, + title: "Test proposal title - a token in the payload", + url: "https://forum.dfinity.org/t/announcing-juno-build-on-the-ic-using-frontend-code-only", + summary: "A proposal whose payload carries the token.", + action: { + Motion: { + motionText: ${JSON.stringify(TOKEN)}, + }, + }, + }); +}; +`; + +test("Test a proposal payload string that reads the undefined token", async ({ + page, + context, +}) => { + step("Serve a dummy proposal whose payload carries the token"); + await page.route(DUMMY_PROPOSALS_SCRIPT_PATH, (route) => + route.fulfill({ + status: 200, + contentType: "application/javascript", + body: dummyProposalsModule, + }) + ); + + await context.grantPermissions(["clipboard-read", "clipboard-write"]); + + await page.goto("/"); + await disableCssAnimations(page); + await expect(page).toHaveTitle("Portfolio | Network Nervous System"); + + await signInWithNewUser({ page, context }); + + const pageElement = PlaywrightPageObjectElement.fromPage(page); + const appPo = new AppPo(pageElement); + + step("Get some ICP"); + await appPo.getIcpTokens(21); + + step("Stake a neuron for voting"); + await appPo.goToStaking(); + await appPo + .getStakingPo() + .stakeFirstNnsNeuron({ amount: 10, dissolveDelayDays: "max" }); + + step("Create the proposal"); + const proposerNeuronId = await createDummyProposal(appPo); + + step("Open the Internet Computer proposals"); + await appPo.goToProposals(); + await appPo.openUniverses(); + await appPo.getSelectUniverseListPo().clickOnInternetComputer(); + const nnsProposalListPo = appPo.getProposalsPo().getNnsProposalListPo(); + await nnsProposalListPo.waitForContentLoaded(); + + await appPo + .getProposalsPo() + .getNnsProposalFiltersPo() + .getActionableProposalsSegmentPo() + .clickAllProposals(); + + step("Filter the open Governance proposals"); + await appPo + .getProposalsPo() + .getNnsProposalFiltersPo() + .selectTopicFilter([Topic.Governance]); + await nnsProposalListPo.waitForContentLoaded(); + await appPo + .getProposalsPo() + .getNnsProposalFiltersPo() + .selectStatusFilter([ProposalStatus.Open]); + await nnsProposalListPo.waitForContentLoaded(); + + step("Open the proposal"); + const proposalCard = + await nnsProposalListPo.getFirstProposalCardPoForProposer(proposerNeuronId); + await proposalCard.click(); + const nnsProposalPo = appPo.getProposalDetailPo().getNnsProposalPo(); + await nnsProposalPo.waitForContentLoaded(); + + const payloadPo = nnsProposalPo.getProposalProposerActionsEntryPo(); + await payloadPo.waitFor(); + const jsonPreviewPo = payloadPo.getJsonPreviewPo(); + const togglePo = payloadPo.getJsonRepresentationModeTogglePo(); + + step("Read the tree view"); + await togglePo.setEnabled(false); + await jsonPreviewPo.getTreeJson().waitFor(); + const treeText = await jsonPreviewPo.getTreeText(); + + // The tree view shows the plain quoted string. Before the fix it showed the + // bare word `undefined`, so a set field read as an unset one. + expect(treeText).toContain(`"${TOKEN}"`); + expect(treeText).not.toContain("undefined"); + + step("Read the raw view"); + await togglePo.setEnabled(true); + await jsonPreviewPo.getRawJson().waitFor(); + const rawText = await jsonPreviewPo.getRawText(); + + // The raw view is valid JSON. Before the fix it read `"motion_text": + // undefined`, which is neither valid JSON nor the value the proposer set. + expect(rawText).toContain(`"${TOKEN}"`); + expect(rawText).not.toContain("undefined"); + const parsed = JSON.parse(rawText) as Record; + expect(Object.values(parsed)).toContain(TOKEN); + + step("Read the copy text"); + await payloadPo.getCopyButtonPo().click(); + const copyText = await page.evaluate(() => navigator.clipboard.readText()); + expect(copyText).toBe(rawText); +}); diff --git a/frontend/src/tests/lib/components/common/JsonPreview.spec.ts b/frontend/src/tests/lib/components/common/JsonPreview.spec.ts index 51e932190d1..c108ca817c9 100644 --- a/frontend/src/tests/lib/components/common/JsonPreview.spec.ts +++ b/frontend/src/tests/lib/components/common/JsonPreview.spec.ts @@ -58,6 +58,18 @@ describe("JsonPreview", () => { expect(await po.getRawText()).toBe(`{\n "test": undefined\n}`); }); + it("should render a string that reads __UNDEFINED__ in raw view", async () => { + jsonRepresentationStore.setMode("raw"); + const po = renderComponent({ test: "__UNDEFINED__" }); + expect(await po.getRawText()).toBe(`{\n "test": "__UNDEFINED__"\n}`); + }); + + it("should render a string that reads __UNDEFINED__ in tree view", async () => { + jsonRepresentationStore.setMode("tree"); + const po = renderComponent({ test: "__UNDEFINED__" }); + expect(await po.getTreeText()).toBe('test "__UNDEFINED__"'); + }); + it("should not render expand button when there is no children", async () => { jsonRepresentationStore.setMode("tree"); const po = renderComponent({ hello: "world" }); diff --git a/frontend/src/tests/lib/components/proposal-detail/ProposalProposerActionsEntry.spec.ts b/frontend/src/tests/lib/components/proposal-detail/ProposalProposerActionsEntry.spec.ts index 3c7ac6b9df7..d82367b46bf 100644 --- a/frontend/src/tests/lib/components/proposal-detail/ProposalProposerActionsEntry.spec.ts +++ b/frontend/src/tests/lib/components/proposal-detail/ProposalProposerActionsEntry.spec.ts @@ -39,6 +39,24 @@ describe("ProposalProposerActionsEntry", () => { }); }); + it("should copy a string that reads __UNDEFINED__ as that string", async () => { + Object.assign(window.navigator, { + clipboard: { + writeText: vi.fn().mockImplementation(() => Promise.resolve()), + }, + }); + const po = renderComponent({ + actionKey: "testKey", + actionData: { test: "__UNDEFINED__" }, + }); + + await po.getCopyButtonPo().click(); + + expect(window.navigator.clipboard.writeText).toHaveBeenCalledWith( + `{\n "test": "__UNDEFINED__"\n}` + ); + }); + it("should render preview mode toggle", async () => { const po = renderComponent({ actionKey: "actionKey", diff --git a/frontend/src/tests/lib/utils/utils.spec.ts b/frontend/src/tests/lib/utils/utils.spec.ts index 420b6231911..804491ff9b1 100644 --- a/frontend/src/tests/lib/utils/utils.spec.ts +++ b/frontend/src/tests/lib/utils/utils.spec.ts @@ -55,6 +55,14 @@ describe("utils", () => { ); }); + it("should treat a non-finite indentation as 0, like JSON.stringify", () => { + for (const indentation of [NaN, Infinity, -Infinity]) { + expect(stringifyJson(SAMPLE, { indentation })).toBe( + JSON.stringify(SAMPLE, null, indentation) + ); + } + }); + it("should convert bigints to function call in devMode", () => { expect( stringifyJson( @@ -75,6 +83,92 @@ describe("utils", () => { `{"_isPrincipal":true}` ); }); + + it("should render a string that reads __UNDEFINED__ as that string", () => { + expect(stringifyJson({ a: "__UNDEFINED__" })).toBe( + `{"a":"__UNDEFINED__"}` + ); + }); + + it("should not let a string break the rest of the payload", () => { + const value = { a: 'x"__UNDEFINED__', b: 1 }; + expect(stringifyJson(value)).toBe(JSON.stringify(value)); + }); + + it("should render a key named __UNDEFINED__ as that key", () => { + expect(stringifyJson({ __UNDEFINED__: 1 })).toBe(`{"__UNDEFINED__":1}`); + }); + + it("should tell undefined apart from the string __UNDEFINED__", () => { + expect(stringifyJson({ a: undefined, b: "__UNDEFINED__" })).toBe( + `{"a":undefined,"b":"__UNDEFINED__"}` + ); + expect(stringifyJson({ a: undefined })).not.toBe( + stringifyJson({ a: "__UNDEFINED__" }) + ); + }); + + it("should tell undefined apart from the string __UNDEFINED__ at the root", () => { + expect(stringifyJson("__UNDEFINED__")).toBe(`"__UNDEFINED__"`); + expect(stringifyJson(undefined)).toBe("undefined"); + }); + + it("should lay values out the way JSON.stringify does", () => { + const value = { + object: { nested: { deep: 1 } }, + array: [1, [2, 3], { a: "b" }], + emptyObject: {}, + emptyArray: [], + nullValue: null, + number: -1.5, + negativeZero: -0, + text: 'a "quoted" \\ value\n', + notANumber: NaN, + boxedString: new String('a "boxed" value'), + boxedNumber: new Number(3), + boxedBoolean: new Boolean(false), + }; + expect(stringifyJson(value)).toBe(JSON.stringify(value)); + expect(stringifyJson(value, { indentation: 2 })).toBe( + JSON.stringify(value, null, 2) + ); + }); + + it("should indent nested undefined values", () => { + expect( + stringifyJson({ a: [undefined, { b: undefined }] }, { indentation: 2 }) + ).toBe( + `{\n "a": [\n undefined,\n {\n "b": undefined\n }\n ]\n}` + ); + }); + + it("should render an array hole as undefined", () => { + // A sparse array. eslint forbids the `[1, , 3]` literal. + const sparse: unknown[] = [1]; + sparse[2] = 3; + + expect(sparse.length).toBe(3); + expect(1 in sparse).toBe(false); + + expect(stringifyJson(sparse)).toBe("[1,undefined,3]"); + expect(stringifyJson(sparse, { indentation: 2 })).toBe( + `[\n 1,\n undefined,\n 3\n]` + ); + }); + + it("should call toJSON the way JSON.stringify does", () => { + const value = { date: new Date(0), nested: { date: new Date(1000) } }; + expect(stringifyJson(value)).toBe(JSON.stringify(value)); + expect(stringifyJson(value)).toBe( + `{"date":"1970-01-01T00:00:00.000Z","nested":{"date":"1970-01-01T00:00:01.000Z"}}` + ); + }); + + it("should throw on a circular structure", () => { + const value: Record = { a: 1 }; + value.self = value; + expect(() => stringifyJson(value)).toThrow(TypeError); + }); }); describe("uniqueObjects", () => { diff --git a/frontend/src/tests/page-objects/ProposalProposerActionsEntry.page-object.ts b/frontend/src/tests/page-objects/ProposalProposerActionsEntry.page-object.ts index d4cfa3e97d1..265219153e5 100644 --- a/frontend/src/tests/page-objects/ProposalProposerActionsEntry.page-object.ts +++ b/frontend/src/tests/page-objects/ProposalProposerActionsEntry.page-object.ts @@ -1,3 +1,4 @@ +import { ButtonPo } from "$tests/page-objects/Button.page-object"; import { JsonPreviewPo } from "$tests/page-objects/JsonPreview.page-object"; import { JsonRepresentationModeTogglePo } from "$tests/page-objects/JsonRepresentationModeToggle.page-object"; import { BasePageObject } from "$tests/page-objects/base.page-object"; @@ -23,4 +24,8 @@ export class ProposalProposerActionsEntryPo extends BasePageObject { getJsonPreviewPo(): JsonPreviewPo { return JsonPreviewPo.under(this.root); } + + getCopyButtonPo(): ButtonPo { + return ButtonPo.under({ element: this.root, testId: "copy-component" }); + } }