fix(imported-tokens): validate the entered canisters anonymously

CHANGELOG-Nns-Dapp-unreleased.md

@@ -38,6 +38,10 @@ proposal is successful, the changes it released will be moved from this file to
 
 #### Security
 
+- The import token validation no longer sends the user principal to the entered
+  canisters. The ledger and index canister IDs come from the form or from the
+  URL, so the two validation calls now use the anonymous identity.
+
 #### Not Published
 
 ### Operations

frontend/src/lib/services/icrc-accounts.services.ts

@@ -7,8 +7,8 @@ import { FORCE_CALL_STRATEGY } from "$lib/constants/mockable.constants";
 import { failedExistentImportedTokenLedgerIdsStore } from "$lib/derived/imported-tokens.derived";
 import { snsTokensByLedgerCanisterIdStore } from "$lib/derived/sns/sns-tokens.derived";
 import {
+  getAnonymousIdentity,
   getAuthenticatedIdentity,
-  getCurrentIdentity,
 } from "$lib/services/auth.services";
 import {
   queryAndUpdate,
@@ -54,7 +54,9 @@ export const getIcrcTokenMetaData = async ({
   ledgerCanisterId: Principal;
 }): Promise<IcrcTokenMetadata> => {
   return queryIcrcToken({
-    identity: getCurrentIdentity(),
+    // The ledger canister ID is unverified user input, so the call must not
+    // carry the user principal. The token metadata is public data.
+    identity: getAnonymousIdentity(),
     canisterId: ledgerCanisterId,
     certified: false,
   });

frontend/src/lib/services/icrc-index.services.ts

@@ -1,5 +1,5 @@
 import { getLedgerId as getLedgerIdApi } from "$lib/api/icrc-index.api";
-import { getAuthenticatedIdentity } from "$lib/services/auth.services";
+import { getAnonymousIdentity } from "$lib/services/auth.services";
 import { toastsError } from "$lib/stores/toasts.store";
 import type { Principal } from "@icp-sdk/core/principal";
 
@@ -10,9 +10,11 @@ const getLedgerId = async ({
   indexCanisterId: Principal;
   certified: boolean;
 }): Promise<Principal> => {
-  const identity = await getAuthenticatedIdentity();
   const ledgerId = await getLedgerIdApi({
-    identity,
+    // The index canister ID is unverified user input, so the call must not
+    // carry the user principal. The ledger ID of an index canister is public
+    // data.
+    identity: getAnonymousIdentity(),
     indexCanisterId,
     certified,
   });

frontend/src/tests/e2e/import-token-anonymous-validation.spec.ts

@@ -0,0 +1,150 @@
+import { AppPo } from "$tests/page-objects/App.page-object";
+import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object";
+import {
+  closeHighlight,
+  dfxCanisterId,
+  disableCssAnimations,
+  signInWithNewUser,
+  step,
+} from "$tests/utils/e2e.test-utils";
+import { expect, test, type Request } from "@playwright/test";
+
+const TEST_TOKEN_NAME = "ckRED";
+
+// The IC request envelope is CBOR. These two markers read the envelope without
+// a CBOR parser.
+//
+// ANONYMOUS_SENDER is the field `sender` set to the anonymous principal:
+// text(6) "sender" (0x66 + "sender") followed by bytes(1) 0x04.
+const ANONYMOUS_SENDER = "6673656e6465724104";
+// agent-js adds `sender_pubkey` and `sender_sig` only when an identity signs
+// the envelope. An anonymous envelope carries neither field.
+const SIGNED_MARKER = Buffer.from("sender_pubkey", "utf8").toString("hex");
+
+type IcRequest = {
+  url: string;
+  bodyHex: string;
+};
+
+const isSigned = (request: IcRequest): boolean =>
+  request.bodyHex.includes(SIGNED_MARKER);
+
+// This test covers finding 22: an import-token deep link must not send the user
+// principal to the canisters that the URL names. The user chose neither
+// canister ID, and the modal calls both of them with no click.
+test("Import token deep link validates the URL canisters anonymously", async ({
+  page,
+  context,
+}) => {
+  const ledgerCanisterId = await dfxCanisterId("ckred_ledger");
+  const indexCanisterId = await dfxCanisterId("ckred_index");
+  const nnsDappCanisterId = await dfxCanisterId("nns-dapp");
+
+  await page.goto("/tokens");
+  await disableCssAnimations(page);
+  await signInWithNewUser({ page, context });
+
+  const requests: IcRequest[] = [];
+  const recordRequest = (request: Request) => {
+    const url = request.url();
+    if (!url.includes("/api/v")) return;
+    requests.push({
+      url,
+      bodyHex: (request.postDataBuffer() ?? Buffer.alloc(0)).toString("hex"),
+    });
+  };
+  const requestsTo = (canisterId: string): IcRequest[] =>
+    requests.filter((request) =>
+      request.url.includes(`/canister/${canisterId}/`)
+    );
+
+  step("Open the import token deep link");
+
+  page.on("request", recordRequest);
+
+  await page.goto(
+    `/tokens/?import-ledger-id=${ledgerCanisterId}&import-index-id=${indexCanisterId}`
+  );
+  await disableCssAnimations(page);
+  await closeHighlight(page);
+
+  const pageElement = PlaywrightPageObjectElement.fromPage(page);
+  const appPo = new AppPo(pageElement);
+  const importTokenModalPo = appPo
+    .getTokensPo()
+    .getTokensPagePo()
+    .getImportTokenModalPo();
+  const reviewPo = importTokenModalPo.getImportTokenReviewPo();
+
+  step("The review step opens without a click");
+
+  await importTokenModalPo.waitFor();
+  await reviewPo.waitFor();
+
+  expect(await reviewPo.getTokenName()).toBe(TEST_TOKEN_NAME);
+  expect(await reviewPo.getLedgerCanisterIdPo().getCanisterIdText()).toBe(
+    ledgerCanisterId
+  );
+  expect(await reviewPo.getIndexCanisterIdPo().getCanisterIdText()).toBe(
+    indexCanisterId
+  );
+
+  // Everything above happened without a click. Stop recording here, because
+  // the calls after the confirmation are allowed to carry the user identity.
+  page.off("request", recordRequest);
+
+  step("The zero click calls reach both URL canisters");
+
+  const ledgerRequests = requestsTo(ledgerCanisterId);
+  const indexRequests = requestsTo(indexCanisterId);
+  expect(ledgerRequests.length).toBeGreaterThan(0);
+  expect(indexRequests.length).toBeGreaterThan(0);
+
+  step("The session is signed in, and a signed envelope is detectable");
+
+  // Control for the two assertions below. The dapp loads the imported tokens
+  // from its own backend canister in the same window, with the user identity.
+  // If this call carried no signature, the checks below would pass for the
+  // wrong reason.
+  const nnsDappRequests = requestsTo(nnsDappCanisterId);
+  expect(nnsDappRequests.length).toBeGreaterThan(0);
+  expect(nnsDappRequests.filter(isSigned).length).toBeGreaterThan(0);
+
+  step("No zero click call to the URL canisters carries the user identity");
+
+  const urlCanisterRequests = [...ledgerRequests, ...indexRequests];
+  expect(urlCanisterRequests.filter(isSigned).map(({ url }) => url)).toEqual(
+    []
+  );
+  expect(
+    urlCanisterRequests
+      .filter(({ bodyHex }) => !bodyHex.includes(ANONYMOUS_SENDER))
+      .map(({ url }) => url)
+  ).toEqual([]);
+
+  step("Confirm still imports the token with the user identity");
+
+  await reviewPo.getConfirmButtonPo().click();
+
+  const walletPo = appPo.getWalletPo().getIcrcWalletPo();
+  await walletPo.waitFor();
+  expect(
+    await walletPo.getWalletPageHeaderPo().getUniverseSummaryPo().getTitle()
+  ).toEqual(TEST_TOKEN_NAME);
+
+  step("The imported token is present in the tokens table");
+
+  await appPo.goBack();
+  await appPo
+    .getTokensPo()
+    .getTokensPagePo()
+    .getImportedTokensTable()
+    .waitFor();
+  expect(
+    await appPo
+      .getTokensPo()
+      .getTokensPagePo()
+      .getImportedTokensTable()
+      .getTokenNames()
+  ).toContain(TEST_TOKEN_NAME);
+});

frontend/src/tests/lib/components/accounts/ImportTokenModal.spec.ts

@@ -20,6 +20,7 @@ import { setSnsProjects } from "$tests/utils/sns.test-utils";
 import { render } from "$tests/utils/svelte.test-utils";
 import { runResolvedPromises } from "$tests/utils/timers.test-utils";
 import { busyStore, toastsStore } from "@dfinity/gix-components";
+import { AnonymousIdentity } from "@icp-sdk/core/agent";
 import { tick } from "svelte";
 import { get } from "svelte/store";
 import type { MockInstance } from "vitest";
@@ -554,6 +555,74 @@ describe("ImportTokenModal", () => {
       });
     });
 
+    it("validates the URL canisters with the anonymous identity", async () => {
+      const getLedgerIdSpy = vi
+        .spyOn(icrcIndexApi, "getLedgerId")
+        .mockResolvedValue(ledgerCanisterId);
+      vi.spyOn(importedTokensApi, "getImportedTokens").mockResolvedValue({
+        imported_tokens: [],
+      });
+      const setImportedTokensSpy = vi
+        .spyOn(importedTokensApi, "setImportedTokens")
+        .mockResolvedValue();
+
+      const po = renderComponent();
+      const reviewPo = po.getImportTokenReviewPo();
+
+      await runResolvedPromises();
+
+      // The modal validates as soon as the imported tokens are loaded, without
+      // a click. The canister IDs come from the URL, so both validation calls
+      // must stay anonymous.
+      importedTokensStore.set({
+        importedTokens: [],
+        certified: true,
+      });
+      await runResolvedPromises();
+
+      expect(await reviewPo.isPresent()).toEqual(true);
+
+      expect(queryIcrcTokenSpy).toBeCalledTimes(1);
+      expect(queryIcrcTokenSpy).toBeCalledWith(
+        expect.objectContaining({
+          identity: new AnonymousIdentity(),
+          canisterId: ledgerCanisterId,
+        })
+      );
+      expect(getLedgerIdSpy).toBeCalledTimes(1);
+      expect(getLedgerIdSpy).toBeCalledWith(
+        expect.objectContaining({
+          identity: new AnonymousIdentity(),
+          indexCanisterId,
+        })
+      );
+
+      const validationIdentities = [
+        ...queryIcrcTokenSpy.mock.calls,
+        ...getLedgerIdSpy.mock.calls,
+      ].map(([{ identity }]) => identity.getPrincipal().toText());
+      expect(validationIdentities).toEqual([
+        new AnonymousIdentity().getPrincipal().toText(),
+        new AnonymousIdentity().getPrincipal().toText(),
+      ]);
+      expect(validationIdentities).not.toContain(
+        mockIdentity.getPrincipal().toText()
+      );
+
+      // The import itself still runs with the user identity.
+      expect(setImportedTokensSpy).toBeCalledTimes(0);
+
+      await reviewPo.getConfirmButtonPo().click();
+      await runResolvedPromises();
+
+      expect(setImportedTokensSpy).toBeCalledTimes(1);
+      expect(setImportedTokensSpy).toBeCalledWith(
+        expect.objectContaining({
+          identity: mockIdentity,
+        })
+      );
+    });
+
     it("removes the URL parameters on cancel click", async () => {
       vi.spyOn(console, "error").mockReturnValue();
       queryIcrcTokenSpy = vi

frontend/src/tests/lib/pages/IcrcWallet.spec.ts

@@ -36,6 +36,7 @@ import { JestPageObjectElement } from "$tests/page-objects/jest.page-object";
 import { blockAllCallsTo } from "$tests/utils/module.test-utils";
 import { runResolvedPromises } from "$tests/utils/timers.test-utils";
 import { busyStore, toastsStore } from "@dfinity/gix-components";
+import { AnonymousIdentity } from "@icp-sdk/core/agent";
 import { render } from "@testing-library/svelte";
 import { get } from "svelte/store";
 
@@ -804,9 +805,11 @@ describe("IcrcWallet", () => {
         ]);
         expect(get(busyStore)).toEqual([]);
         expect(spyOnGetLedgerId).toBeCalledTimes(1);
+        // The index canister ID is unverified user input, so the call must not
+        // carry the user principal.
         expect(spyOnGetLedgerId).toBeCalledWith({
           certified: true,
-          identity: mockIdentity,
+          identity: new AnonymousIdentity(),
           indexCanisterId,
         });
         expect(spyOnSetImportedTokens).toBeCalledTimes(0);

frontend/src/tests/lib/services/icrc-accounts.services.spec.ts

@@ -36,6 +36,7 @@ import { toastsStore } from "@dfinity/gix-components";
 import { encodeIcrcAccount } from "@icp-sdk/canisters/ledger/icrc";
 import {
   AgentError,
+  AnonymousIdentity,
   ErrorKindEnum,
   ReplicaRejectCode,
   requestIdOf,
@@ -821,13 +822,28 @@ describe("icrc-accounts-services", () => {
       });
       expect(ledgerApi.queryIcrcToken).toHaveBeenCalledTimes(1);
       expect(ledgerApi.queryIcrcToken).toHaveBeenCalledWith({
-        identity: mockIdentity,
+        identity: new AnonymousIdentity(),
         certified: false,
         canisterId: ledgerCanisterId,
       });
       expect(result).toEqual(mockToken);
     });
 
+    it("does not send the user principal to the ledger canister", async () => {
+      // The ledger canister ID is unverified user input, so the call must not
+      // carry the user principal.
+      await getIcrcTokenMetaData({
+        ledgerCanisterId,
+      });
+
+      expect(ledgerApi.queryIcrcToken).toHaveBeenCalledTimes(1);
+      const { identity } = vi.mocked(ledgerApi.queryIcrcToken).mock.calls[0][0];
+      expect(identity.getPrincipal().isAnonymous()).toEqual(true);
+      expect(identity.getPrincipal().toText()).not.toEqual(
+        mockIdentity.getPrincipal().toText()
+      );
+    });
+
     it("throws an error", async () => {
       const testError = new Error("test");
       vi.spyOn(ledgerApi, "queryIcrcToken").mockRejectedValue(testError);
@@ -842,7 +858,7 @@ describe("icrc-accounts-services", () => {
       await expect(call).rejects.toThrow(testError);
       expect(ledgerApi.queryIcrcToken).toHaveBeenCalledTimes(1);
       expect(ledgerApi.queryIcrcToken).toHaveBeenCalledWith({
-        identity: mockIdentity,
+        identity: new AnonymousIdentity(),
         certified: false,
         canisterId: ledgerCanisterId,
       });

frontend/src/tests/lib/services/icrc-index.services.spec.ts

@@ -3,6 +3,7 @@ import { matchLedgerIndexPair } from "$lib/services/icrc-index.services";
 import { mockIdentity, resetIdentity } from "$tests/mocks/auth.store.mock";
 import { principal } from "$tests/mocks/sns-projects.mock";
 import { toastsStore } from "@dfinity/gix-components";
+import { AnonymousIdentity } from "@icp-sdk/core/agent";
 import { get } from "svelte/store";
 
 describe("icrc-index.services", () => {
@@ -30,9 +31,16 @@ describe("icrc-index.services", () => {
       expect(spyOnGetLedgerId).toBeCalledTimes(1);
       expect(spyOnGetLedgerId).toBeCalledWith({
         certified: true,
-        identity: mockIdentity,
+        identity: new AnonymousIdentity(),
         indexCanisterId,
       });
+      // The index canister ID is unverified user input, so the call must not
+      // carry the user principal.
+      const { identity } = spyOnGetLedgerId.mock.calls[0][0];
+      expect(identity.getPrincipal().isAnonymous()).toEqual(true);
+      expect(identity.getPrincipal().toText()).not.toEqual(
+        mockIdentity.getPrincipal().toText()
+      );
     });
 
     it("should return false when the ledger canister IDs don't match", async () => {