From 6143610608fd78d6b94b2aaaa2366fbbd6ee8932 Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:07:42 +0100 Subject: [PATCH] fix(frontend): preserve pdf link targets in desktop viewer (#7235) Same PR as #6396, just with the conflicts resolved and some fixes on top. Original commit by @saul1310 is preserved as-is; everything else is a follow-up commit. Refs #6272 ## Conflicts #6396 was written before the frontend was restructured, so all four files it touched moved (`frontend/src/**` -> `frontend/editor/src/**`) and `LinkLayer.tsx` had drifted. Cherry-picked with rename detection and re-resolved against current `main`. ## Fixes on top - **Reuse the existing platform seam instead of adding a second one.** `main` already has `@app/platform/*` seams with per-flavour implementations; #6396 added a parallel `@app/utils/openExternalUrl` core+desktop pair that re-implemented the Tauri shell call already in `desktop/platform/openExternal.ts`. Split into a pure sanitiser (`@app/utils/externalUrl`) and a platform seam (`@app/platform/openExternalTab`), with the desktop impl delegating to the existing `openExternal`. - **Kept PDF links off the `openExternal` seam.** That seam is for leave-and-return redirects (Stripe) and its saas impl is `window.location.assign` - routing PDF links through it would navigate the whole app away from the user's document. `openExternalTab` always opens alongside the app; desktop shadows it to escape the webview. - **Fixed the same defect in two sibling call sites** that #6396 didn't cover: `BookmarkSidebar` (bookmark URI / LaunchAppOrOpenFile actions) and `useAnnotationMenuHandlers` (annotation menu "go to link"). Both called `window.open` on an unsanitised PDF-supplied URI, so on desktop they trapped the link in the webview exactly like the viewer did. - **Dropped the unguarded fallback.** The old code fell back to `window.open(uri)` when `new URL()` threw, so an unparseable URI bypassed the allowlist entirely. It is now blocked. - Empty/whitespace URIs are blocked rather than silently resolving to the app's own page via the base URL. - Tests: sanitiser cases (casing, leading whitespace, `data:`, `vbscript:`, unparseable), a core seam test asserting new-tab-not-navigate, and a desktop seam regression test asserting the URL goes to the OS rather than `window.open`. - **`openExternalTab` now re-validates its own input.** Every caller sanitises first, so nothing reached it unvalidated - but it is the sink that hands a URL to `window.open` (executes `javascript:` in our origin) or to an OS handler on desktop, and its safety shouldn't depend on callers remembering. Both impls fail closed, with tests that call them directly with `javascript:`/`data:`/`file:`/`ftp:`. ## Unrelated fix included (flagged deliberately) The last commit fixes `frontend/editor/vitest.config.ts`: `testTimeout: 10000` was set on the root `test` block, but tests all run under `projects`, which do not inherit it - so the whole suite has silently been running at vitest's 5s default. This is not cosmetic. It made `task check` fail intermittently on unrelated portal specs (`demoData`, `ConnectionModal`); the ConsignO test takes 2966ms with only the portal project running, i.e. 59% of a budget it was never meant to have, so any CPU contention tips it over. Proven with an identical 6.5s probe test: times out at 5000ms on the old config, passes at 6512ms on the fixed one. Happy to split this into its own PR if preferred - it is here because the gate could not be trusted without it. ## Validation Typecheck passes for all 7 build flavours (core, proprietary, saas, desktop, cloud, prototypes, portal); ESLint, Prettier, dpdm and the full 1662-test vitest suite pass. Driven live against the dev server + backend with a PDF carrying five URI annotations (https, `javascript:`, mailto, `file:`, relative). 14/14 behavioural checks pass on this branch; 5 of them fail on `main`: | check | main | this PR | | --- | --- | --- | | safe https link exposes real href (copy-link) | `href="#"` | `https://example.com/safe-link?a=1` | | link opens in new tab / tabnabbing-proof | no `target`/`rel` | `_blank` + `noopener noreferrer` | | mailto link exposes real href | `href="#"` | `mailto:test@example.com` | | relative URI resolved against app origin | `href="#"` | resolved | | `javascript:` / `file:` never reach href | blocked | blocked | | clicking blocked link doesn't execute or navigate | ok | ok | | clicking safe link opens new tab at source URL | - | ok, app not navigated away | --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Testing (if applicable) - [x] I have run `task check` to verify linters, typechecks, and tests pass - [x] I have tested my changes locally --------- Co-authored-by: Saul --- .../components/viewer/BookmarkSidebar.tsx | 17 ++++++- .../src/core/components/viewer/LinkLayer.tsx | 30 +++++++------ .../viewer/useAnnotationMenuHandlers.ts | 12 ++++- .../src/core/platform/openExternalTab.test.ts | 45 +++++++++++++++++++ .../src/core/platform/openExternalTab.ts | 27 +++++++++++ .../editor/src/core/utils/externalUrl.test.ts | 45 +++++++++++++++++++ frontend/editor/src/core/utils/externalUrl.ts | 32 +++++++++++++ .../desktop/platform/openExternalTab.test.ts | 43 ++++++++++++++++++ .../src/desktop/platform/openExternalTab.ts | 24 ++++++++++ frontend/editor/vitest.config.ts | 14 +++++- 10 files changed, 270 insertions(+), 19 deletions(-) create mode 100644 frontend/editor/src/core/platform/openExternalTab.test.ts create mode 100644 frontend/editor/src/core/platform/openExternalTab.ts create mode 100644 frontend/editor/src/core/utils/externalUrl.test.ts create mode 100644 frontend/editor/src/core/utils/externalUrl.ts create mode 100644 frontend/editor/src/desktop/platform/openExternalTab.test.ts create mode 100644 frontend/editor/src/desktop/platform/openExternalTab.ts 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,