fix(auth): limit the sign-out toast message to a known allowlist

CHANGELOG-Nns-Dapp-unreleased.md

@@ -35,6 +35,9 @@ proposal is successful, the changes it released will be moved from this file to
 
 #### Security
 
+- The sign-out message in the URL is now limited to known messages. Before, a
+  crafted link could show any text as an official toast.
+
 #### Not Published
 
 ### Operations

frontend/src/lib/services/auth.services.ts

@@ -2,7 +2,7 @@ import { browser } from "$app/environment";
 import { authStore } from "$lib/stores/auth.store";
 import { startBusy } from "$lib/stores/busy.store";
 import { toastsError, toastsShow } from "$lib/stores/toasts.store";
-import type { ToastMsg } from "$lib/types/toast";
+import type { I18nKeys } from "$lib/utils/i18n.utils";
 import { replaceHistory } from "$lib/utils/route.utils";
 import { registerCleanupForTesting } from "$lib/utils/test-support.utils";
 import type { ToastLevel } from "@dfinity/gix-components";
@@ -13,6 +13,18 @@ import { get } from "svelte/store";
 const msgParam = "msg";
 const levelParam = "level";
 
+// The only messages that a sign-out can carry in the url. The app owns the
+// level, so the url cannot pick the styling of the toast.
+const LOGOUT_MSGS = {
+  "error.missing_identity": "error",
+  "warning.auth_sign_out": "warn",
+} as const satisfies Partial<Record<I18nKeys, ToastLevel>>;
+
+export type LogoutMsgKey = keyof typeof LOGOUT_MSGS;
+
+const isLogoutMsgKey = (msg: string): msg is LogoutMsgKey =>
+  Object.hasOwn(LOGOUT_MSGS, msg);
+
 let logoutInProgress = false;
 
 registerCleanupForTesting(() => {
@@ -35,11 +47,7 @@ export const login = async () => {
   await authStore.signIn(onError);
 };
 
-export const logout = async ({
-  msg = undefined,
-}: {
-  msg?: Pick<ToastMsg, "labelKey" | "level">;
-}) => {
+export const logout = async ({ msg = undefined }: { msg?: LogoutMsgKey }) => {
   // Prevent re-entrant logout calls. When authStore.signOut() sets identity
   // to null, reactive cascades can cause multiple services to detect the
   // missing identity and each independently call logout() again, appending
@@ -89,7 +97,7 @@ export const getAuthenticatedIdentity = async (): Promise<Identity> => {
 
     if (!identity) {
       await logout({
-        msg: { labelKey: "error.missing_identity", level: "error" },
+        msg: "error.missing_identity",
       });
 
       // We do not resolve on purpose. logout() does reload the browser
@@ -101,17 +109,14 @@ export const getAuthenticatedIdentity = async (): Promise<Identity> => {
 };
 
 /**
- * If a message was provided to the logout process - e.g. a message informing the logout happened because the session timed-out - append the information to the url as query params
+ * If a message was provided to the logout process - e.g. a message informing the logout happened because the session timed-out - append the key to the url as a query param
  */
-const appendMsgToUrl = (msg: Pick<ToastMsg, "labelKey" | "level">) => {
-  const { labelKey, level } = msg;
-
+const appendMsgToUrl = (msg: LogoutMsgKey) => {
   if (!browser) return;
 
   const url: URL = new URL(window.location.href);
 
-  url.searchParams.set(msgParam, encodeURI(labelKey));
-  url.searchParams.set(levelParam, level);
+  url.searchParams.set(msgParam, msg);
 
   replaceHistory(url);
 };
@@ -128,15 +133,13 @@ export const displayAndCleanLogoutMsg = () => {
 
   const msg: string | null = urlParams.get(msgParam);
 
-  if (msg === null) {
+  if (msg === null && !urlParams.has(levelParam)) {
     return;
   }
 
-  // For simplicity reason we assume the level pass as query params is one of the type ToastLevel
-  const level: ToastLevel =
-    (urlParams.get(levelParam) as ToastLevel | null) ?? "success";
-
-  toastsShow({ labelKey: msg, level });
+  if (msg !== null && isLogoutMsgKey(msg)) {
+    toastsShow({ labelKey: msg, level: LOGOUT_MSGS[msg] });
+  }
 
   cleanUpMsgUrl();
 };

frontend/src/lib/services/worker-auth.services.ts

@@ -20,10 +20,7 @@ export const initAuthWorker = async (): Promise<AuthWorker> => {
     switch (msg) {
       case "nnsSignOut":
         await logout({
-          msg: {
-            labelKey: "warning.auth_sign_out",
-            level: "warn",
-          },
+          msg: "warning.auth_sign_out",
         });
         return;
       case "nnsDelegationRemainingTime":

frontend/src/tests/e2e/logout-msg.spec.ts

@@ -0,0 +1,93 @@
+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";
+
+// The global expect timeout is 0, which means "wait forever", so every poll
+// below sets its own timeout.
+const POLL_TIMEOUT = 30_000;
+
+// Playwright cannot import en.json here, so the texts are copied.
+// "error.missing_identity" in frontend/src/lib/i18n/en.json.
+const missingIdentityText =
+  "The operation cannot be executed without any identity.";
+// "warning.auth_sign_out" in frontend/src/lib/i18n/en.json.
+const authSignOutText =
+  "You have been logged out because your session has expired.";
+
+const craftedMsg = "Send your ICP to this address to recover your account";
+
+// initAppAuth deletes both parameters on every page load.
+const waitForCleanUrl = async (page: Page) => {
+  await expect
+    .poll(() => new URL(page.url()).searchParams.get("msg"), {
+      timeout: POLL_TIMEOUT,
+    })
+    .toBeNull();
+  expect(new URL(page.url()).searchParams.get("level")).toBeNull();
+};
+
+const getToastMessages = (appPo: AppPo): Promise<string[]> =>
+  appPo.getToastsPo().getMessages();
+
+const getToastClasses = (appPo: AppPo): Promise<string[] | null> =>
+  appPo.getToastsPo().getToastPo().root.getClasses();
+
+test("Test the msg url parameter", async ({ page }) => {
+  const appPo = new AppPo(PlaywrightPageObjectElement.fromPage(page));
+
+  await step("A msg that is not in the allowlist shows no toast");
+  await page.goto(
+    `/accounts?msg=${encodeURIComponent(craftedMsg)}&level=error`
+  );
+  await appPo.getSignInPo().waitFor();
+  await waitForCleanUrl(page);
+  expect(await getToastMessages(appPo)).toEqual([]);
+
+  await step("A msg in the allowlist shows its own text and its own level");
+  await page.goto("/accounts?msg=error.missing_identity&level=success");
+  await appPo.getSignInPo().waitFor();
+  await expect
+    .poll(() => getToastMessages(appPo), { timeout: POLL_TIMEOUT })
+    .toEqual([missingIdentityText]);
+  // The url asked for "success". The app owns the level, so the toast is an
+  // error.
+  expect(await getToastClasses(appPo)).toContain("error");
+  expect(await getToastClasses(appPo)).not.toContain("success");
+  await waitForCleanUrl(page);
+});
+
+test("Test the toast after an automatic sign out", async ({
+  page: page1,
+  context,
+}) => {
+  await page1.goto("/accounts");
+  await expect(page1).toHaveTitle("Account | Network Nervous System");
+  const appPo1 = new AppPo(PlaywrightPageObjectElement.fromPage(page1));
+
+  const page2 = await context.newPage();
+  await page2.goto("/accounts");
+  await expect(page2).toHaveTitle("Account | Network Nervous System");
+  const appPo2 = new AppPo(PlaywrightPageObjectElement.fromPage(page2));
+
+  await signInWithNewUser({ page: page1, context });
+  await appPo1.getAccountsPo().waitFor();
+
+  await page2.reload();
+  await appPo2.getAccountsPo().waitFor();
+
+  await step("Sign out in the first tab");
+  await appPo1.getAccountMenuPo().openMenu();
+  await appPo1.getAccountMenuPo().clickLogout();
+  await appPo1.getSignInPo().waitFor();
+
+  await step("The second tab shows the session expiry toast");
+  // The auth worker of the second tab sees the missing delegation, calls
+  // logout with "warning.auth_sign_out" and reloads the page.
+  await appPo2.getSignInPo().waitFor();
+  await expect
+    .poll(() => getToastMessages(appPo2), { timeout: POLL_TIMEOUT })
+    .toContain(authSignOutText);
+  expect(await getToastClasses(appPo2)).toContain("warn");
+  await waitForCleanUrl(page2);
+});