fix(frontend): stop the transactions worker from paging forever

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 transactions sync now stops after a fixed number of pages, and it stops
+  when a page makes no progress. A hostile index canister can no longer make the
+  sync run without an end.
+
 #### Not Published
 
 ### Operations

frontend/src/lib/constants/constants.ts

@@ -3,6 +3,12 @@ export const MAX_ACTIONABLE_REQUEST_COUNT = 10;
 // Use a different limit for Icrc transactions
 // the Index canister needs to query the Icrc Ledger canister for each transaction - i.e. it needs an update call
 export const DEFAULT_INDEX_TRANSACTION_PAGE_LIMIT = 20;
+// The worker does not trust the Index canister to end the pagination.
+// It stops after this number of pages in one sync of one account.
+// Trade-off: an account with more than DEFAULT_INDEX_TRANSACTION_MAX_PAGES *
+// DEFAULT_INDEX_TRANSACTION_PAGE_LIMIT new transactions in one sync interval
+// shows a gap until the user reloads the page.
+export const DEFAULT_INDEX_TRANSACTION_MAX_PAGES = 10;
 
 export const DEFAULT_TOAST_DURATION_MILLIS = 4000;
 

frontend/src/lib/worker-services/icrc-transactions.worker-services.ts

@@ -1,5 +1,8 @@
 import type { GetTransactionsResponse } from "$lib/api/icrc-index.api";
-import { DEFAULT_INDEX_TRANSACTION_PAGE_LIMIT } from "$lib/constants/constants";
+import {
+  DEFAULT_INDEX_TRANSACTION_MAX_PAGES,
+  DEFAULT_INDEX_TRANSACTION_PAGE_LIMIT,
+} from "$lib/constants/constants";
 
 import type { IcrcAccountIdentifierText } from "$lib/types/icrc";
 import type {
@@ -13,7 +16,7 @@ import type {
   TimerWorkerUtilsJobData,
   TimerWorkerUtilsSyncParams,
 } from "$lib/worker-utils/timer.worker-utils";
-import { jsonReplacer, nonNullish } from "@dfinity/utils";
+import { isNullish, jsonReplacer, nonNullish } from "@dfinity/utils";
 import {
   decodeIcrcAccount,
   type IcrcIndexDid,
@@ -28,7 +31,7 @@ export type GetAccountsTransactionsResults = Omit<
 /**
  * Collect the ICRC transactions for a list of accounts.
  *
- * For each account provided as a parameter, the service ensures that no duplicate transactions are returned and handles fetching all transactions recursively, taking into account the pagination of the backend API calls.
+ * For each account provided as a parameter, the service ensures that no duplicate transactions are returned and fetches the pages of new transactions in a loop, taking into account the pagination of the backend API calls.
  *
  * @param object TimerWorkerUtilsJobData<PostMessageDataRequestTransactions> & { state: DictionaryWorkerState<TransactionsData>; }
  * @param object.identity
@@ -93,7 +96,7 @@ const getIcrcAccountTransactions = async ({
   host,
   state,
 }: GetAccountTransactionsParams): Promise<GetAccountsTransactionsResults> => {
-  const { mostRecentTxId, transactions, ...rest } = await getIcrcTransactions({
+  const { transactions: firstPage, ...rest } = await getIcrcTransactions({
     identity,
     indexCanisterId,
     accountIdentifier,
@@ -103,50 +106,66 @@ const getIcrcAccountTransactions = async ({
     state,
   });
 
-  // We compare IDs because we want to sort and find the oldest transaction ID to notice if we have fetched all new transactions or if there is a remaining gap.
-  //
-  // For example:
-  // New transactions [100, 99, 98]
-  // Most recent transaction ID 95
-  // Therefore, we still need to get between 95 and 98
-  //
-  // Note that  we do not perform a sort based on the timestamp but on the ID for simplicity reason as we do not really care here if two transactions have the same ID, we are just looking for the oldest ID.
-  const oldestTxId: IcrcIndexDid.BlockIndex | undefined = [
-    ...transactions,
-  ].sort(({ id: idA }, { id: idB }) => Number(idA - idB))[0]?.id;
-
-  // Did we fetch all new transactions or there were more transactions than the batch size (DEFAULT_ICRC_TRANSACTION_PAGE_LIMIT) since last time the worker fetched the transactions
-  const fetchMoreTransactions = (): boolean => {
+  // Collect the pages and flatten them once at the end.
+  const pages: IcrcIndexDid.TransactionWithId[][] = [firstPage];
+
+  let currentStart: bigint | undefined = start;
+  let currentPage: IcrcIndexDid.TransactionWithId[] = firstPage;
+
+  // The Index canister is not trusted, therefore the loop stops after
+  // DEFAULT_INDEX_TRANSACTION_MAX_PAGES pages whatever the canister answers.
+  while (pages.length < DEFAULT_INDEX_TRANSACTION_MAX_PAGES) {
+    // We compare IDs because we want to sort and find the oldest transaction ID to notice if we have fetched all new transactions or if there is a remaining gap.
+    //
+    // For example:
+    // New transactions [100, 99, 98]
+    // Most recent transaction ID 95
+    // Therefore, we still need to get between 95 and 98
+    //
+    // Note that  we do not perform a sort based on the timestamp but on the ID for simplicity reason as we do not really care here if two transactions have the same ID, we are just looking for the oldest ID.
+    const oldestTxId: IcrcIndexDid.BlockIndex | undefined = [
+      ...currentPage,
+    ].sort(({ id: idA }, { id: idB }) => Number(idA - idB))[0]?.id;
+
     const stateMostRecentTxId = state?.mostRecentTxId;
-    return (
-      nonNullish(stateMostRecentTxId) &&
-      nonNullish(oldestTxId) &&
-      oldestTxId > stateMostRecentTxId
-    );
-  };
+
+    // Did we fetch all new transactions or there were more transactions than the batch size (DEFAULT_ICRC_TRANSACTION_PAGE_LIMIT) since last time the worker fetched the transactions
+    if (
+      isNullish(stateMostRecentTxId) ||
+      isNullish(oldestTxId) ||
+      oldestTxId <= stateMostRecentTxId
+    ) {
+      break;
+    }
+
+    // The Index canister can answer a page that does not move the oldest ID
+    // down. Such a page makes no progress, so the loop stops with it.
+    if (nonNullish(currentStart) && oldestTxId >= currentStart) {
+      break;
+    }
+
+    // Two transactions can have the same Id - e.g. a transaction from/to same account.
+    // That is why we fetch the next batch of transactions starting from the same Id and not Id - 1n because otherwise there would be a chance that we might miss one.
+    // Note: when "start" is provided, getIcrcTransactions search from "start" and returns "start" included in the results.
+    currentStart = oldestTxId;
+
+    const { transactions } = await getIcrcTransactions({
+      identity,
+      indexCanisterId,
+      accountIdentifier,
+      start: currentStart,
+      fetchRootKey,
+      host,
+      state,
+    });
+
+    pages.push(transactions);
+    currentPage = transactions;
+  }
 
   return {
-    mostRecentTxId,
     ...rest,
-    transactions: [
-      ...transactions,
-      ...(fetchMoreTransactions() && nonNullish(oldestTxId)
-        ? (
-            await getIcrcAccountTransactions({
-              identity,
-              indexCanisterId,
-              accountIdentifier,
-              // Two transactions can have the same Id - e.g. a transaction from/to same account.
-              // That is why we fetch the next batch of transactions starting from the same Id and not Id - 1n because otherwise there would be a chance that we might miss one.
-              // Note: when "start" is provided, getIcrcTransactions search from "start" and returns "start" included in the results.
-              start: oldestTxId,
-              fetchRootKey,
-              host,
-              state,
-            })
-          ).transactions
-        : []),
-    ],
+    transactions: pages.flat(),
   };
 };
 

frontend/src/tests/e2e/transactions-worker-pagination.spec.ts

@@ -0,0 +1,157 @@
+import { SYNC_ACCOUNTS_TIMER_INTERVAL_MILLIS } from "$lib/constants/accounts.constants";
+import { DEFAULT_INDEX_TRANSACTION_MAX_PAGES } from "$lib/constants/constants";
+import { AppPo } from "$tests/page-objects/App.page-object";
+import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object";
+import {
+  dfxCanisterId,
+  disableCssAnimations,
+  signInWithNewUser,
+  step,
+} from "$tests/utils/e2e.test-utils";
+import { expect, test, type Request } from "@playwright/test";
+
+const TEST_TOKEN_NAME = "ckRED";
+
+// Time to let the wallet page finish its own load calls before the measured
+// window starts.
+const SETTLE_MILLIS = 5_000;
+
+// Two full timer intervals, plus room for the calls of the last tick to
+// complete. The window must hold more than one tick, otherwise the spec cannot
+// tell a bounded tick from a tick that never ends.
+const WINDOW_MILLIS = 2 * SYNC_ACCOUNTS_TIMER_INTERVAL_MILLIS + 10_000;
+
+// One sync sends its pages back to back, one network round trip each. Two syncs
+// are SYNC_ACCOUNTS_TIMER_INTERVAL_MILLIS apart. Any gap above this value
+// therefore starts a new sync.
+const BURST_GAP_MILLIS = 5_000;
+
+type IndexCall = {
+  at: number;
+};
+
+/**
+ * The transactions web worker pages the index canister. Before the fix it
+ * called itself again whenever the oldest transaction id of a page was above
+ * the most recent id it knew. Nothing required that id to go down, and nothing
+ * capped the number of pages, so an index canister that keeps answering ids
+ * above the known one makes one sync run without an end.
+ *
+ * This spec watches the HTTP traffic to the index canister of an imported
+ * token. It groups the get_account_transactions requests into syncs and checks
+ * that no sync sends more than DEFAULT_INDEX_TRANSACTION_MAX_PAGES requests.
+ *
+ * The local index canister is honest, so this spec bounds the honest path and
+ * proves the loop still polls and still feeds the page. The hostile index is
+ * pinned by the unit tests in
+ * src/tests/lib/worker-services/icrc-transactions.worker-services.spec.ts,
+ * which fail on `main`.
+ */
+test("Transactions worker sends a bounded number of index calls per sync", async ({
+  page,
+  context,
+}) => {
+  const ledgerCanisterId = await dfxCanisterId("ckred_ledger");
+  const indexCanisterId = await dfxCanisterId("ckred_index");
+
+  const indexCalls: IndexCall[] = [];
+
+  // The worker runs in a dedicated web worker. Chromium reports its requests on
+  // the page that owns it, so the context listener sees them.
+  context.on("request", (request: Request) => {
+    const url = request.url();
+
+    if (!url.includes(`/canister/${indexCanisterId}/`)) {
+      return;
+    }
+
+    // The agent sends CBOR. The method name is a text string inside it, so the
+    // ASCII bytes appear verbatim in the body.
+    const body = request.postDataBuffer();
+
+    if (body === null || !body.includes("get_account_transactions")) {
+      return;
+    }
+
+    indexCalls.push({ at: Date.now() });
+  });
+
+  await page.goto("/tokens");
+  await disableCssAnimations(page);
+  await signInWithNewUser({ page, context });
+
+  const pageElement = PlaywrightPageObjectElement.fromPage(page);
+  const appPo = new AppPo(pageElement);
+  const tokensPagePo = appPo.getTokensPo().getTokensPagePo();
+
+  await step("Import the test token so the wallet page has an ICRC account");
+
+  await tokensPagePo.getSettingsButtonPo().click();
+
+  const importButtonPo = tokensPagePo.getImportTokenButtonPo();
+  await importButtonPo.waitFor();
+  await importButtonPo.click();
+
+  const importTokenModalPo = tokensPagePo.getImportTokenModalPo();
+  await importTokenModalPo.waitFor();
+
+  const formPo = importTokenModalPo.getImportTokenFormPo();
+  await formPo.getLedgerCanisterInputPo().typeText(ledgerCanisterId);
+  await formPo.getIndexCanisterInputPo().typeText(indexCanisterId);
+  await formPo.getSubmitButtonPo().click();
+
+  const reviewPo = importTokenModalPo.getImportTokenReviewPo();
+  await reviewPo.waitFor();
+  expect(await reviewPo.getTokenName()).toBe(TEST_TOKEN_NAME);
+  await reviewPo.getConfirmButtonPo().click();
+
+  await step("Wait for the wallet page of the imported token");
+
+  const walletPo = appPo.getWalletPo().getIcrcWalletPo();
+  await walletPo.waitFor();
+
+  // The new user made no transaction with this token, so the list settles on
+  // the empty state. It also proves the transactions path of the page finished.
+  await expect
+    .poll(() => walletPo.hasNoTransactions(), { timeout: 60_000 })
+    .toBe(true);
+
+  await step("Let the page settle, then measure two full sync intervals");
+
+  await page.waitForTimeout(SETTLE_MILLIS);
+
+  const mark = indexCalls.length;
+
+  await page.waitForTimeout(WINDOW_MILLIS);
+
+  const callsInWindow = indexCalls.slice(mark);
+
+  await step("Every sync must stay under the page cap");
+
+  // Fails closed: if the worker stopped polling, or if the requests never
+  // reached this listener, the window is empty and this assertion fails.
+  expect(callsInWindow.length).toBeGreaterThan(0);
+
+  const syncSizes: number[] = [];
+  let previousAt: number | undefined = undefined;
+
+  for (const { at } of callsInWindow) {
+    if (previousAt === undefined || at - previousAt > BURST_GAP_MILLIS) {
+      syncSizes.push(1);
+    } else {
+      syncSizes[syncSizes.length - 1] += 1;
+    }
+
+    previousAt = at;
+  }
+
+  expect(Math.max(...syncSizes)).toBeLessThanOrEqual(
+    DEFAULT_INDEX_TRANSACTION_MAX_PAGES
+  );
+
+  await step("The page still shows the transactions list");
+
+  // A sync that never ends never posts its result, and the list would fall back
+  // to the loading state. It must still show the empty state.
+  expect(await walletPo.hasNoTransactions()).toBe(true);
+});