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 <saulifshin.cs@gmail.com>
2026-09-02 21:03:34 +03:00 · 2026-08-14 18:07:42 +01:00
parent bff1ea916d
commit 6143610608
10 changed files with 270 additions and 19 deletions
@@ -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;
      }
    }
@@ -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<LinkLayerProps> = ({
            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<LinkLayerProps> = ({
          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 (
            <a
@@ -524,7 +524,9 @@ export const LinkLayer: React.FC<LinkLayerProps> = ({
                  linkElementRefs.current.delete(annotationLink.id);
                }
              }}
-              href="#"
+              href={externalHref ?? "#"}
+              target={externalHref ? "_blank" : undefined}
+              rel={externalHref ? "noopener noreferrer" : undefined}
              onClick={(e) => {
                e.preventDefault();
                e.stopPropagation();
@@ -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);
    }
@@ -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,<script>alert(1)</script>",
+    "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();
+  });
+});
@@ -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<void>;
+
+export const openExternalTab: OpenExternalTab = async (
+  url: string,
+): Promise<void> => {
+  const safeHref = getExternalHref(url);
+  if (!safeHref) {
+    console.warn("[openExternalTab] Refused to open unsafe URL:", url);
+    return;
+  }
+  window.open(safeHref, "_blank", "noopener,noreferrer");
+};
@@ -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,<script>alert(1)</script>"),
+    ).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();
+  });
+});
@@ -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;
+}
@@ -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();
+    },
+  );
+});
@@ -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<void> => {
+  const safeHref = getExternalHref(url);
+  if (!safeHref) {
+    console.warn("[openExternalTab] Refused to open unsafe URL:", url);
+    return;
+  }
+  await openExternal(safeHref);
+};
@@ -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,