From 0080ad38026503182966d34a78fa1b76bd1c994b Mon Sep 17 00:00:00 2001 From: Yusef Habib Fernandez Date: Fri, 4 Sep 2026 13:20:17 +0200 Subject: [PATCH 1/5] fix(auth): limit the sign-out message in the url to an allowlist displayAndCleanLogoutMsg passed the msg query parameter straight to toastsShow as a labelKey. translate returns the raw key when the catalog has no match, so a crafted link showed any text as an official toast. The level parameter set the toast color with no check. The url now carries only a key from LOGOUT_MSGS. The map owns the level, so the url cannot pick the styling. An unknown key shows no toast and the url is still cleaned. --- CHANGELOG-Nns-Dapp-unreleased.md | 3 + frontend/src/lib/services/auth.services.ts | 40 ++++--- .../src/lib/services/worker-auth.services.ts | 5 +- .../tests/lib/services/auth.services.spec.ts | 107 ++++++++++++++++-- 4 files changed, 126 insertions(+), 29 deletions(-) diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 31087ae0319..7d4c8d441e3 100644 --- a/CHANGELOG-Nns-Dapp-unreleased.md +++ b/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 diff --git a/frontend/src/lib/services/auth.services.ts b/frontend/src/lib/services/auth.services.ts index 8501739a63d..8e9f54ef7ea 100644 --- a/frontend/src/lib/services/auth.services.ts +++ b/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>; + +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; -}) => { +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 => { 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 => { }; /** - * 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) => { - 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); }; @@ -132,11 +137,12 @@ export const displayAndCleanLogoutMsg = () => { 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"; + if (!isLogoutMsgKey(msg)) { + cleanUpMsgUrl(); + return; + } - toastsShow({ labelKey: msg, level }); + toastsShow({ labelKey: msg, level: LOGOUT_MSGS[msg] }); cleanUpMsgUrl(); }; diff --git a/frontend/src/lib/services/worker-auth.services.ts b/frontend/src/lib/services/worker-auth.services.ts index 8ddd3ab2433..c09c85786e4 100644 --- a/frontend/src/lib/services/worker-auth.services.ts +++ b/frontend/src/lib/services/worker-auth.services.ts @@ -20,10 +20,7 @@ export const initAuthWorker = async (): Promise => { switch (msg) { case "nnsSignOut": await logout({ - msg: { - labelKey: "warning.auth_sign_out", - level: "warn", - }, + msg: "warning.auth_sign_out", }); return; case "nnsDelegationRemainingTime": diff --git a/frontend/src/tests/lib/services/auth.services.spec.ts b/frontend/src/tests/lib/services/auth.services.spec.ts index 1407512cbbb..59d50260576 100644 --- a/frontend/src/tests/lib/services/auth.services.spec.ts +++ b/frontend/src/tests/lib/services/auth.services.spec.ts @@ -8,6 +8,7 @@ import { authStore } from "$lib/stores/auth.store"; import * as busyStore from "$lib/stores/busy.store"; import * as routeUtils from "$lib/utils/route.utils"; import { mockIdentity } from "$tests/mocks/auth.store.mock"; +import en from "$tests/mocks/i18n.mock"; import { toastsStore } from "@dfinity/gix-components"; import { AuthClient, IdbStorage } from "@icp-sdk/auth/client"; import { AnonymousIdentity } from "@icp-sdk/core/agent"; @@ -107,9 +108,13 @@ describe("auth-services", () => { it("should add msg to url", async () => { const spy = vi.spyOn(routeUtils, "replaceHistory"); - await logout({ msg: { labelKey: "test.key", level: "warn" } }); + await logout({ msg: "warning.auth_sign_out" }); - expect(spy).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledTimes(1); + + const url = spy.mock.calls[0][0]; + expect(url.searchParams.get("msg")).toEqual("warning.auth_sign_out"); + expect(url.searchParams.get("level")).toBeNull(); spy.mockClear(); }); @@ -139,12 +144,17 @@ describe("auth-services", () => { Object.defineProperty(window, "location", { writable: true, - value: { ...location, search: "msg=test.key&level=warn" }, + value: { ...location, search: "msg=warning.auth_sign_out" }, }); await displayAndCleanLogoutMsg(); - expect(spy).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledWith( + expect.objectContaining({ + level: "warn", + text: en.warning.auth_sign_out, + }) + ); Object.defineProperty(window, "location", { writable: true, @@ -161,7 +171,7 @@ describe("auth-services", () => { Object.defineProperty(window, "location", { writable: true, - value: { ...location, search: "msg=test.key&level=warn" }, + value: { ...location, search: "msg=warning.auth_sign_out" }, }); await displayAndCleanLogoutMsg(); @@ -176,6 +186,87 @@ describe("auth-services", () => { spy.mockClear(); }); + it("should ignore an unknown msg from url", async () => { + const toastSpy = vi.spyOn(toastsStore, "show"); + const historySpy = vi.spyOn(routeUtils, "replaceHistory"); + + const location = window.location; + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location, search: "msg=Send%20funds%20now&level=error" }, + }); + + await displayAndCleanLogoutMsg(); + + expect(toastSpy).not.toHaveBeenCalled(); + expect(historySpy).toHaveBeenCalledTimes(1); + + const url = historySpy.mock.calls[0][0]; + expect(url.searchParams.get("msg")).toBeNull(); + expect(url.searchParams.get("level")).toBeNull(); + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location }, + }); + + toastSpy.mockClear(); + historySpy.mockClear(); + }); + + it("should ignore the level from url", async () => { + const spy = vi.spyOn(toastsStore, "show"); + + const location = window.location; + + Object.defineProperty(window, "location", { + writable: true, + value: { + ...location, + search: "msg=error.missing_identity&level=success", + }, + }); + + await displayAndCleanLogoutMsg(); + + expect(spy).toHaveBeenCalledWith( + expect.objectContaining({ + level: "error", + text: en.error.missing_identity, + }) + ); + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location }, + }); + + spy.mockClear(); + }); + + it("should ignore a prototype key from url", async () => { + const spy = vi.spyOn(toastsStore, "show"); + + const location = window.location; + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location, search: "msg=constructor" }, + }); + + await displayAndCleanLogoutMsg(); + + expect(spy).not.toHaveBeenCalled(); + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location }, + }); + + spy.mockClear(); + }); + it("should display a busy screen", async () => { const spy = vi.spyOn(busyStore, "startBusy"); @@ -191,9 +282,9 @@ describe("auth-services", () => { const signOutSpy = vi.spyOn(authStore, "signOut"); await Promise.all([ - logout({ msg: { labelKey: "warning.auth_sign_out", level: "warn" } }), - logout({ msg: { labelKey: "warning.auth_sign_out", level: "warn" } }), - logout({ msg: { labelKey: "error.missing_identity", level: "error" } }), + logout({ msg: "warning.auth_sign_out" }), + logout({ msg: "warning.auth_sign_out" }), + logout({ msg: "error.missing_identity" }), ]); expect(signOutSpy).toHaveBeenCalledTimes(1); From 6ba71beb72939f0eb88747fac328fc326b195c61 Mon Sep 17 00:00:00 2001 From: Yusef Habib Fernandez Date: Fri, 4 Sep 2026 13:33:36 +0200 Subject: [PATCH 2/5] test(auth): add an e2e spec for the sign-out message allowlist The spec checks three things in the browser. A msg that is not in the allowlist shows no toast. An allowlisted msg keeps the level of the app, not the level in the url. The automatic sign-out in a second tab still shows the session expiry toast. --- frontend/src/tests/e2e/logout-msg.spec.ts | 93 +++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 frontend/src/tests/e2e/logout-msg.spec.ts diff --git a/frontend/src/tests/e2e/logout-msg.spec.ts b/frontend/src/tests/e2e/logout-msg.spec.ts new file mode 100644 index 00000000000..874a3c9fb5f --- /dev/null +++ b/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 => + appPo.getToastsPo().getMessages(); + +const getToastClasses = (appPo: AppPo): Promise => + 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); +}); From c1ce87bc53730769fb4fff116001ad5aedeee200 Mon Sep 17 00:00:00 2001 From: Yusef Habib Fernandez Date: Fri, 4 Sep 2026 13:41:21 +0200 Subject: [PATCH 3/5] test(auth): make the url assertions in the sign-out msg tests real The mocked window.location set only `search`. `cleanUpMsgUrl` builds its url from `href`, so the url assertions held whatever the code did. The mock now sets `href` with the same query. --- .../tests/lib/services/auth.services.spec.ts | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/frontend/src/tests/lib/services/auth.services.spec.ts b/frontend/src/tests/lib/services/auth.services.spec.ts index 59d50260576..887c9c6b18f 100644 --- a/frontend/src/tests/lib/services/auth.services.spec.ts +++ b/frontend/src/tests/lib/services/auth.services.spec.ts @@ -168,15 +168,25 @@ describe("auth-services", () => { const spy = vi.spyOn(routeUtils, "replaceHistory"); const location = window.location; + const search = "msg=warning.auth_sign_out&level=warn"; + // cleanUpMsgUrl builds its url from href, so href must carry the query. Object.defineProperty(window, "location", { writable: true, - value: { ...location, search: "msg=warning.auth_sign_out" }, + value: { + ...location, + href: `https://nns.internetcomputer.org/accounts?${search}`, + search, + }, }); await displayAndCleanLogoutMsg(); - expect(spy).toHaveBeenCalled(); + expect(spy).toHaveBeenCalledTimes(1); + + const url = spy.mock.calls[0][0]; + expect(url.searchParams.get("msg")).toBeNull(); + expect(url.searchParams.get("level")).toBeNull(); Object.defineProperty(window, "location", { writable: true, @@ -191,10 +201,16 @@ describe("auth-services", () => { const historySpy = vi.spyOn(routeUtils, "replaceHistory"); const location = window.location; + const search = "msg=Send%20funds%20now&level=error"; + // cleanUpMsgUrl builds its url from href, so href must carry the query. Object.defineProperty(window, "location", { writable: true, - value: { ...location, search: "msg=Send%20funds%20now&level=error" }, + value: { + ...location, + href: `https://nns.internetcomputer.org/accounts?${search}`, + search, + }, }); await displayAndCleanLogoutMsg(); From 60f6e9843c119433beb7023325ada428f37c6264 Mon Sep 17 00:00:00 2001 From: Yusef Habib Fernandez Date: Fri, 4 Sep 2026 13:56:50 +0200 Subject: [PATCH 4/5] fix(auth): clean a bare level url param with no msg displayAndCleanLogoutMsg only cleaned the url when msg was present. A url with only level stayed untouched. It now cleans the url whenever msg or level is present. --- frontend/src/lib/services/auth.services.ts | 9 ++--- .../tests/lib/services/auth.services.spec.ts | 35 +++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/frontend/src/lib/services/auth.services.ts b/frontend/src/lib/services/auth.services.ts index 8e9f54ef7ea..96acedfc53e 100644 --- a/frontend/src/lib/services/auth.services.ts +++ b/frontend/src/lib/services/auth.services.ts @@ -133,17 +133,14 @@ export const displayAndCleanLogoutMsg = () => { const msg: string | null = urlParams.get(msgParam); - if (msg === null) { + if (msg === null && !urlParams.has(levelParam)) { return; } - if (!isLogoutMsgKey(msg)) { - cleanUpMsgUrl(); - return; + if (msg !== null && isLogoutMsgKey(msg)) { + toastsShow({ labelKey: msg, level: LOGOUT_MSGS[msg] }); } - toastsShow({ labelKey: msg, level: LOGOUT_MSGS[msg] }); - cleanUpMsgUrl(); }; diff --git a/frontend/src/tests/lib/services/auth.services.spec.ts b/frontend/src/tests/lib/services/auth.services.spec.ts index 887c9c6b18f..643117e9464 100644 --- a/frontend/src/tests/lib/services/auth.services.spec.ts +++ b/frontend/src/tests/lib/services/auth.services.spec.ts @@ -231,6 +231,41 @@ describe("auth-services", () => { historySpy.mockClear(); }); + it("should clean a bare level from url with no msg", async () => { + const toastSpy = vi.spyOn(toastsStore, "show"); + const historySpy = vi.spyOn(routeUtils, "replaceHistory"); + + const location = window.location; + const search = "level=error"; + + // cleanUpMsgUrl builds its url from href, so href must carry the query. + Object.defineProperty(window, "location", { + writable: true, + value: { + ...location, + href: `https://nns.internetcomputer.org/accounts?${search}`, + search, + }, + }); + + await displayAndCleanLogoutMsg(); + + expect(toastSpy).not.toHaveBeenCalled(); + expect(historySpy).toHaveBeenCalledTimes(1); + + const url = historySpy.mock.calls[0][0]; + expect(url.searchParams.get("msg")).toBeNull(); + expect(url.searchParams.get("level")).toBeNull(); + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location }, + }); + + toastSpy.mockClear(); + historySpy.mockClear(); + }); + it("should ignore the level from url", async () => { const spy = vi.spyOn(toastsStore, "show"); From 3e25ff34bc2b73e53d2fd5bec2858c833f99417d Mon Sep 17 00:00:00 2001 From: Yusef Habib Fernandez Date: Fri, 4 Sep 2026 14:21:33 +0200 Subject: [PATCH 5/5] fix(auth): strip a pre-existing level param when writing msg to url appendMsgToUrl set msg but left an already-present level param in place. It now deletes level before it sets msg, so a crafted or legacy url cannot carry level into the post-logout url. --- frontend/src/lib/services/auth.services.ts | 3 ++ .../tests/lib/services/auth.services.spec.ts | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/frontend/src/lib/services/auth.services.ts b/frontend/src/lib/services/auth.services.ts index 96acedfc53e..1cce44d8ca5 100644 --- a/frontend/src/lib/services/auth.services.ts +++ b/frontend/src/lib/services/auth.services.ts @@ -116,6 +116,9 @@ const appendMsgToUrl = (msg: LogoutMsgKey) => { const url: URL = new URL(window.location.href); + // Drop a pre-existing level param, so the url never carries an untrusted + // level value, even one left over from a crafted or legacy link. + url.searchParams.delete(levelParam); url.searchParams.set(msgParam, msg); replaceHistory(url); diff --git a/frontend/src/tests/lib/services/auth.services.spec.ts b/frontend/src/tests/lib/services/auth.services.spec.ts index 643117e9464..7cd6d2cc177 100644 --- a/frontend/src/tests/lib/services/auth.services.spec.ts +++ b/frontend/src/tests/lib/services/auth.services.spec.ts @@ -119,6 +119,37 @@ describe("auth-services", () => { spy.mockClear(); }); + it("should drop a pre-existing level param when adding msg to url", async () => { + const spy = vi.spyOn(routeUtils, "replaceHistory"); + + const location = window.location; + const search = "level=success"; + + Object.defineProperty(window, "location", { + writable: true, + value: { + ...location, + href: `https://nns.internetcomputer.org/accounts?${search}`, + search, + }, + }); + + await logout({ msg: "warning.auth_sign_out" }); + + expect(spy).toHaveBeenCalledTimes(1); + + const url = spy.mock.calls[0][0]; + expect(url.searchParams.get("msg")).toEqual("warning.auth_sign_out"); + expect(url.searchParams.get("level")).toBeNull(); + + Object.defineProperty(window, "location", { + writable: true, + value: { ...location }, + }); + + spy.mockClear(); + }); + it("should not add msg to url", async () => { const spy = vi.spyOn(routeUtils, "replaceHistory");