fix(frontend): stop a payload string masquerading as undefined in raw view

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

frontend/src/lib/utils/utils.ts

@@ -9,8 +9,73 @@ 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.
+const jsonGap = (indentation: number): string =>
+  " ".repeat(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 +84,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<string, unknown>;
+      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);
 };
 
 /**

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<string, unknown>;
+  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);
+});

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