diff --git a/frontend/editor/src/core/components/viewer/BookmarkSidebar.tsx b/frontend/editor/src/core/components/viewer/BookmarkSidebar.tsx index 9a590587ee..9c92730077 100644 --- a/frontend/editor/src/core/components/viewer/BookmarkSidebar.tsx +++ b/frontend/editor/src/core/components/viewer/BookmarkSidebar.tsx @@ -17,6 +17,8 @@ import { useFileContext } from "@app/contexts/FileContext"; import { isStirlingFile, type FileId } from "@app/types/fileContext"; import { createStirlingFilesAndStubs } from "@app/services/fileStubHelpers"; import apiClient from "@app/services/apiClient"; +import { openExternalTab } from "@app/platform/openExternalTab"; +import { getExternalHref } from "@app/utils/externalUrl"; import { PdfBookmarkObject, PdfActionType } from "@embedpdf/models"; import { useTranslation } from "react-i18next"; import BookmarksIcon from "@mui/icons-material/BookmarksRounded"; @@ -74,6 +76,17 @@ const resolvePageNumber = (bookmark: PdfBookmarkObject): number | null => { return null; }; +// Bookmark targets are PDF-supplied, so sanitise before opening. Local paths +// from LaunchAppOrOpenFile fail the allowlist - a browser blocks them anyway. +const openBookmarkTarget = (rawUrl: string): void => { + const href = getExternalHref(rawUrl); + if (!href) { + console.warn("[BookmarkSidebar] Blocked unsafe URL:", rawUrl); + return; + } + void openExternalTab(href); +}; + export const BookmarkSidebar = ({ visible, thumbnailVisible, @@ -515,12 +528,12 @@ export const BookmarkSidebar = ({ const action = target.action; if (action.type === PdfActionType.URI && action.uri) { event.preventDefault(); - window.open(action.uri, "_blank", "noopener"); + openBookmarkTarget(action.uri); return; } if (action.type === PdfActionType.LaunchAppOrOpenFile && action.path) { event.preventDefault(); - window.open(action.path, "_blank", "noopener"); + openBookmarkTarget(action.path); return; } } diff --git a/frontend/editor/src/core/components/viewer/LinkLayer.tsx b/frontend/editor/src/core/components/viewer/LinkLayer.tsx index e1a2aeb8f0..e68c541fd8 100644 --- a/frontend/editor/src/core/components/viewer/LinkLayer.tsx +++ b/frontend/editor/src/core/components/viewer/LinkLayer.tsx @@ -19,6 +19,9 @@ import { import { Z_INDEX_VIEWER_FLOATING_MENU } from "@app/styles/zIndex"; import { Button } from "@app/ui/Button"; import { ActionIcon } from "@app/ui/ActionIcon"; +import { openExternalTab } from "@app/platform/openExternalTab"; +import { getExternalHref } from "@app/utils/externalUrl"; + // --------------------------------------------------------------------------- // Inline SVG icons (thin-stroke, modern) // --------------------------------------------------------------------------- @@ -401,19 +404,11 @@ export const LinkLayer: React.FC = ({ behavior: "smooth", }); } else if (action.type === PdfActionType.URI) { - const uri = action.uri; - try { - const url = new URL(uri, window.location.href); - if (["http:", "https:", "mailto:"].includes(url.protocol)) { - window.open(uri, "_blank", "noopener,noreferrer"); - } else { - console.warn( - "[LinkLayer] Blocked unsafe URL protocol:", - url.protocol, - ); - } - } catch { - window.open(uri, "_blank", "noopener,noreferrer"); + const href = getExternalHref(action.uri); + if (href) { + void openExternalTab(href); + } else { + console.warn("[LinkLayer] Blocked unsafe URL:", action.uri); } } } @@ -513,6 +508,11 @@ export const LinkLayer: React.FC = ({ const top = annotationLink.rect.origin.y * scale; const width = annotationLink.rect.size.width * scale; const height = annotationLink.rect.size.height * scale; + const externalHref = + annotationLink.target?.type === "action" && + annotationLink.target.action.type === PdfActionType.URI + ? getExternalHref(annotationLink.target.action.uri) + : null; return ( = ({ linkElementRefs.current.delete(annotationLink.id); } }} - href="#" + href={externalHref ?? "#"} + target={externalHref ? "_blank" : undefined} + rel={externalHref ? "noopener noreferrer" : undefined} onClick={(e) => { e.preventDefault(); e.stopPropagation(); diff --git a/frontend/editor/src/core/components/viewer/useAnnotationMenuHandlers.ts b/frontend/editor/src/core/components/viewer/useAnnotationMenuHandlers.ts index bf14b559f4..3e9e048aaf 100644 --- a/frontend/editor/src/core/components/viewer/useAnnotationMenuHandlers.ts +++ b/frontend/editor/src/core/components/viewer/useAnnotationMenuHandlers.ts @@ -14,6 +14,8 @@ import type { AnnotationPatch, } from "@app/components/viewer/viewerTypes"; import type { ScrollActions } from "@app/contexts/viewer/viewerActions"; +import { openExternalTab } from "@app/platform/openExternalTab"; +import { getExternalHref } from "@app/utils/externalUrl"; export type AnnotationType = | "textMarkup" @@ -370,7 +372,15 @@ export function useAnnotationMenuHandlers({ const onGoToLink = useCallback(() => { if (!firstLinkTarget) return; if (firstLinkTarget.type === "uri") { - window.open(firstLinkTarget.uri, "_blank", "noopener,noreferrer"); + const href = getExternalHref(firstLinkTarget.uri); + if (href) { + void openExternalTab(href); + } else { + console.warn( + "[useAnnotationMenuHandlers] Blocked unsafe URL:", + firstLinkTarget.uri, + ); + } } else { scrollActions.scrollToPage(firstLinkTarget.pageIndex + 1); } diff --git a/frontend/editor/src/core/platform/openExternalTab.test.ts b/frontend/editor/src/core/platform/openExternalTab.test.ts new file mode 100644 index 0000000000..ccbbe4d818 --- /dev/null +++ b/frontend/editor/src/core/platform/openExternalTab.test.ts @@ -0,0 +1,45 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { openExternalTab } from "@app/platform/openExternalTab"; +import { expectConsole } from "@app/tests/failOnConsole"; + +describe("openExternalTab (core/web)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + // Opening in a new tab is the point of this seam: @app/platform/openExternal + // navigates the current tab on saas, which would tear the user out of the PDF. + test("opens alongside the app rather than navigating it away", async () => { + const openSpy = vi + .spyOn(window, "open") + .mockImplementation(() => null as Window | null); + const originalHref = window.location.href; + + await openExternalTab("https://example.com/"); + + expect(openSpy).toHaveBeenCalledWith( + "https://example.com/", + "_blank", + "noopener,noreferrer", + ); + expect(window.location.href).toBe(originalHref); + }); + + // The seam is the sink, so it must not depend on callers having sanitised: + // window.open on a javascript: URL would execute it in our own origin. + test.each([ + "javascript:alert(1)", + " javascript:alert(1)", + "data:text/html,", + "file:///etc/passwd", + ])("refuses to open %s even if a caller skips sanitising", async (url) => { + const openSpy = vi + .spyOn(window, "open") + .mockImplementation(() => null as Window | null); + expectConsole.warn(/Refused to open unsafe URL/); + + await openExternalTab(url); + + expect(openSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/editor/src/core/platform/openExternalTab.ts b/frontend/editor/src/core/platform/openExternalTab.ts new file mode 100644 index 0000000000..3a8d21d74a --- /dev/null +++ b/frontend/editor/src/core/platform/openExternalTab.ts @@ -0,0 +1,27 @@ +/** + * core/web implementation of the @app/platform/openExternalTab seam. + * + * Distinct from @app/platform/openExternal: that seam is for "leave and return" + * redirects (Stripe checkout), so its saas impl navigates the CURRENT tab. A PDF + * link must never do that — it would tear the user out of their document — so + * this seam always opens alongside the app. Desktop shadows it to escape the + * Tauri webview; saas/proprietary fall through to this window.open. + * + * Callers are expected to sanitise, but this is the sink that actually hands the + * URL to the browser, so it re-checks rather than trusting them: window.open on + * a `javascript:` URL executes it in our own origin. + */ +import { getExternalHref } from "@app/utils/externalUrl"; + +export type OpenExternalTab = (url: string) => Promise; + +export const openExternalTab: OpenExternalTab = async ( + url: string, +): Promise => { + const safeHref = getExternalHref(url); + if (!safeHref) { + console.warn("[openExternalTab] Refused to open unsafe URL:", url); + return; + } + window.open(safeHref, "_blank", "noopener,noreferrer"); +}; diff --git a/frontend/editor/src/core/utils/externalUrl.test.ts b/frontend/editor/src/core/utils/externalUrl.test.ts new file mode 100644 index 0000000000..c59a1c5926 --- /dev/null +++ b/frontend/editor/src/core/utils/externalUrl.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "vitest"; +import { getExternalHref, toSafeExternalUrl } from "@app/utils/externalUrl"; + +describe("externalUrl", () => { + test("accepts http/https/mailto URLs", () => { + expect(toSafeExternalUrl("https://example.com/test")?.href).toBe( + "https://example.com/test", + ); + expect(toSafeExternalUrl("http://example.com/test")?.href).toBe( + "http://example.com/test", + ); + expect(toSafeExternalUrl("mailto:test@example.com")?.href).toBe( + "mailto:test@example.com", + ); + }); + + test("rejects unsafe protocols", () => { + expect(toSafeExternalUrl("javascript:alert(1)")).toBeNull(); + expect(toSafeExternalUrl("file:///etc/passwd")).toBeNull(); + expect(toSafeExternalUrl("ftp://example.com")).toBeNull(); + expect( + toSafeExternalUrl("data:text/html,"), + ).toBeNull(); + expect(toSafeExternalUrl("vbscript:msgbox(1)")).toBeNull(); + }); + + test("rejects unparseable input instead of opening it blind", () => { + expect(toSafeExternalUrl("")).toBeNull(); + expect(toSafeExternalUrl("http://[")).toBeNull(); + }); + + test("is not fooled by casing or leading whitespace", () => { + expect(toSafeExternalUrl("JavaScript:alert(1)")).toBeNull(); + expect(toSafeExternalUrl(" javascript:alert(1)")).toBeNull(); + expect(toSafeExternalUrl("HTTPS://example.com")?.protocol).toBe("https:"); + }); + + test("normalizes relative URLs against current origin", () => { + expect(getExternalHref("/docs/help")?.endsWith("/docs/help")).toBe(true); + }); + + test("getExternalHref returns null for blocked URLs", () => { + expect(getExternalHref("javascript:alert(1)")).toBeNull(); + }); +}); diff --git a/frontend/editor/src/core/utils/externalUrl.ts b/frontend/editor/src/core/utils/externalUrl.ts new file mode 100644 index 0000000000..cf8638b5a1 --- /dev/null +++ b/frontend/editor/src/core/utils/externalUrl.ts @@ -0,0 +1,32 @@ +/** + * Sanitisation for URLs that come out of a PDF (link annotations, bookmark + * actions). PDF-supplied URIs are untrusted input, so everything that opens one + * funnels through here first and drops anything outside the allowlist. + * + * Pure helpers only — opening the URL is a platform concern and lives behind + * the @app/platform/openExternalTab seam. + */ +const ALLOWED_EXTERNAL_PROTOCOLS = new Set(["http:", "https:", "mailto:"]); + +function getExternalUrlBase(): string | undefined { + // Relative URIs resolve against the app's own location, matching how the + // viewer has always treated them. No DOM => absolute URLs only. + return typeof window !== "undefined" ? window.location?.href : undefined; +} + +/** Parses `rawUrl` and returns it only if its protocol is on the allowlist. */ +export function toSafeExternalUrl(rawUrl: string): URL | null { + // An empty URI would otherwise resolve to the app's own page via the base. + if (!rawUrl?.trim()) return null; + try { + const parsed = new URL(rawUrl, getExternalUrlBase()); + return ALLOWED_EXTERNAL_PROTOCOLS.has(parsed.protocol) ? parsed : null; + } catch { + return null; + } +} + +/** Normalised href for a safe external URL, or null if it is not safe to open. */ +export function getExternalHref(rawUrl: string): string | null { + return toSafeExternalUrl(rawUrl)?.href ?? null; +} diff --git a/frontend/editor/src/desktop/platform/openExternalTab.test.ts b/frontend/editor/src/desktop/platform/openExternalTab.test.ts new file mode 100644 index 0000000000..e45ee1bf47 --- /dev/null +++ b/frontend/editor/src/desktop/platform/openExternalTab.test.ts @@ -0,0 +1,43 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; + +const shellOpenMock = vi.fn(); + +vi.mock("@tauri-apps/plugin-shell", () => ({ + open: (url: string) => shellOpenMock(url), +})); + +import { openExternalTab } from "@app/platform/openExternalTab"; +import { expectConsole } from "@app/tests/failOnConsole"; + +describe("openExternalTab (desktop/Tauri)", () => { + afterEach(() => { + vi.restoreAllMocks(); + shellOpenMock.mockReset(); + }); + + // Regression for #6272: window.open traps the link inside the Tauri webview, + // so a PDF link opened a blank in-app window instead of the user's browser. + test("hands the URL to the OS instead of the webview", async () => { + const openSpy = vi + .spyOn(window, "open") + .mockImplementation(() => null as Window | null); + + await openExternalTab("https://example.com/"); + + expect(shellOpenMock).toHaveBeenCalledWith("https://example.com/"); + expect(openSpy).not.toHaveBeenCalled(); + }); + + // Worse than the web case: an unvalidated scheme here reaches an OS handler + // rather than staying inside a browser. + test.each(["javascript:alert(1)", "file:///etc/passwd", "ftp://example.com"])( + "refuses to hand %s to the OS", + async (url) => { + expectConsole.warn(/Refused to open unsafe URL/); + + await openExternalTab(url); + + expect(shellOpenMock).not.toHaveBeenCalled(); + }, + ); +}); diff --git a/frontend/editor/src/desktop/platform/openExternalTab.ts b/frontend/editor/src/desktop/platform/openExternalTab.ts new file mode 100644 index 0000000000..e7a4b5865b --- /dev/null +++ b/frontend/editor/src/desktop/platform/openExternalTab.ts @@ -0,0 +1,24 @@ +/** + * desktop (Tauri) implementation of the @app/platform/openExternalTab seam. + * + * window.open would trap the URL inside our own webview, so hand it to the OS. + * Delegates to the openExternal seam rather than calling the Tauri shell plugin + * again — on desktop "new tab" and "system browser" are the same action. + * + * Re-checks the URL for the same reason the core impl does, and more so: here it + * reaches an OS handler, so an unvalidated scheme is not confined to a browser. + */ +import { openExternal } from "@app/platform/openExternal"; +import type { OpenExternalTab } from "@core/platform/openExternalTab"; +import { getExternalHref } from "@core/utils/externalUrl"; + +export const openExternalTab: OpenExternalTab = async ( + url: string, +): Promise => { + const safeHref = getExternalHref(url); + if (!safeHref) { + console.warn("[openExternalTab] Refused to open unsafe URL:", url); + return; + } + await openExternal(safeHref); +}; diff --git a/frontend/editor/vitest.config.ts b/frontend/editor/vitest.config.ts index 5d7a00bccf..16d457bbd3 100644 --- a/frontend/editor/vitest.config.ts +++ b/frontend/editor/vitest.config.ts @@ -1,6 +1,11 @@ import { defineConfig } from "vitest/config"; import react from "@vitejs/plugin-react-swc"; import tsconfigPaths from "vite-tsconfig-paths"; + +// Projects do NOT inherit the root test.testTimeout, so every project silently +// ran at vitest's 5s default. Spread this into each one instead. +const TIMEOUTS = { testTimeout: 10000, hookTimeout: 10000 }; + export default defineConfig({ test: { globals: true, @@ -12,8 +17,7 @@ export default defineConfig({ "src/**/*.spec.ts", // Exclude Playwright E2E tests "src/tests/test-fixtures/**", ], - testTimeout: 10000, - hookTimeout: 10000, + ...TIMEOUTS, coverage: { reporter: ["text", "json", "html"], exclude: [ @@ -30,6 +34,7 @@ export default defineConfig({ { test: { name: "core", + ...TIMEOUTS, include: ["src/core/**/*.test.{ts,tsx}"], environment: "jsdom", globals: true, @@ -48,6 +53,7 @@ export default defineConfig({ { test: { name: "portal", + ...TIMEOUTS, include: ["src/portal/**/*.test.{ts,tsx}"], environment: "jsdom", globals: true, @@ -68,6 +74,7 @@ export default defineConfig({ { test: { name: "proprietary", + ...TIMEOUTS, include: ["src/proprietary/**/*.test.{ts,tsx}"], environment: "jsdom", globals: true, @@ -86,6 +93,7 @@ export default defineConfig({ { test: { name: "desktop", + ...TIMEOUTS, include: ["src/desktop/**/*.test.{ts,tsx}"], environment: "jsdom", globals: true, @@ -104,6 +112,7 @@ export default defineConfig({ { test: { name: "saas", + ...TIMEOUTS, // src/saas = editor-saas layer; src/portal-saas = the portal's saas // overrides (sibling to src/portal). Both build under the saas flavor, // so both resolve @portal via the saas cascade (tsconfig.saas.vite.json). @@ -128,6 +137,7 @@ export default defineConfig({ { test: { name: "prototypes", + ...TIMEOUTS, include: ["src/prototypes/**/*.test.{ts,tsx}"], environment: "jsdom", globals: true,