diff --git a/CHANGELOG-Nns-Dapp-unreleased.md b/CHANGELOG-Nns-Dapp-unreleased.md index 31087ae031..7d4c8d441e 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 8501739a63..1cce44d8ca 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,17 @@ 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); + // 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); }; @@ -128,15 +136,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(); }; diff --git a/frontend/src/lib/services/worker-auth.services.ts b/frontend/src/lib/services/worker-auth.services.ts index 8ddd3ab243..c09c85786e 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/e2e/logout-msg.spec.ts b/frontend/src/tests/e2e/logout-msg.spec.ts new file mode 100644 index 0000000000..874a3c9fb5 --- /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); +}); diff --git a/frontend/src/tests/lib/services/auth.services.spec.ts b/frontend/src/tests/lib/services/auth.services.spec.ts index 1407512cbb..7cd6d2cc17 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,44 @@ 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(); + }); + + 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(); }); @@ -139,12 +175,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, @@ -158,15 +199,147 @@ 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=test.key&level=warn" }, + 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, + value: { ...location }, + }); + + 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; + 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, + 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 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"); + + 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, @@ -191,9 +364,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);