Merge pull request #17984 from Snuffleupagus/issue-17981

Validate explicit destinations on the worker-thread to prevent `DataCloneError` (issue 17981)
mozilla · Apr 24, 2024 · 885dd72 · 885dd72
2 parents 7d1eabe + 7206d0a
commit 885dd72
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 34 deletions.
diff --git a/src/core/catalog.js b/src/core/catalog.js
@@ -52,11 +52,57 @@ import { GlobalImageCache } from "./image_utils.js";
 import { MetadataParser } from "./metadata_parser.js";
 import { StructTreeRoot } from "./struct_tree.js";
 
-function fetchDestination(dest) {
+function isValidExplicitDest(dest) {
+  if (!Array.isArray(dest) || dest.length < 2) {
+    return false;
+  }
+  const [page, zoom, ...args] = dest;
+  if (!(page instanceof Ref) && !Number.isInteger(page)) {
+    return false;
+  }
+  if (!(zoom instanceof Name)) {
+    return false;
+  }
+  let allowNull = true;
+  switch (zoom.name) {
+    case "XYZ":
+      if (args.length !== 3) {
+        return false;
+      }
+      break;
+    case "Fit":
+    case "FitB":
+      return args.length === 0;
+    case "FitH":
+    case "FitBH":
+    case "FitV":
+    case "FitBV":
+      if (args.length !== 1) {
+        return false;
+      }
+      break;
+    case "FitR":
+      if (args.length !== 4) {
+        return false;
+      }
+      allowNull = false;
+      break;
+    default:
+      return false;
+  }
+  for (const arg of args) {
+    if (!(typeof arg === "number" || (allowNull && arg === null))) {
+      return false;
+    }
+  }
+  return true;
+}
+
+function fetchDest(dest) {
   if (dest instanceof Dict) {
     dest = dest.get("D");
   }
-  return Array.isArray(dest) ? dest : null;
+  return isValidExplicitDest(dest) ? dest : null;
 }
 
 function fetchRemoteDest(action) {
@@ -67,7 +113,7 @@ function fetchRemoteDest(action) {
     }
     if (typeof dest === "string") {
       return stringToPDFString(dest);
-    } else if (Array.isArray(dest)) {
+    } else if (isValidExplicitDest(dest)) {
       return JSON.stringify(dest);
     }
   }
@@ -641,14 +687,14 @@ class Catalog {
       dests = Object.create(null);
     if (obj instanceof NameTree) {
       for (const [key, value] of obj.getAll()) {
-        const dest = fetchDestination(value);
+        const dest = fetchDest(value);
         if (dest) {
           dests[stringToPDFString(key)] = dest;
         }
       }
     } else if (obj instanceof Dict) {
       obj.forEach(function (key, value) {
-        const dest = fetchDestination(value);
+        const dest = fetchDest(value);
         if (dest) {
           dests[key] = dest;
         }
@@ -660,7 +706,7 @@ class Catalog {
   getDestination(id) {
     const obj = this._readDests();
     if (obj instanceof NameTree) {
-      const dest = fetchDestination(obj.get(id));
+      const dest = fetchDest(obj.get(id));
       if (dest) {
         return dest;
       }
@@ -672,7 +718,7 @@ class Catalog {
         return allDest;
       }
     } else if (obj instanceof Dict) {
-      const dest = fetchDestination(obj.get(id));
+      const dest = fetchDest(obj.get(id));
       if (dest) {
         return dest;
       }
@@ -1703,7 +1749,7 @@ class Catalog {
       }
       if (typeof dest === "string") {
         resultObj.dest = stringToPDFString(dest);
-      } else if (Array.isArray(dest)) {
+      } else if (isValidExplicitDest(dest)) {
         resultObj.dest = dest;
       }
     }

diff --git a/src/display/api.js b/src/display/api.js
@@ -2933,10 +2933,9 @@ class WorkerTransport {
   getPageIndex(ref) {
     if (
       typeof ref !== "object" ||
-      ref === null ||
-      !Number.isInteger(ref.num) ||
+      !Number.isInteger(ref?.num) ||
       ref.num < 0 ||
-      !Number.isInteger(ref.gen) ||
+      !Number.isInteger(ref?.gen) ||
       ref.gen < 0
     ) {
       return Promise.reject(new Error("Invalid pageIndex request."));

diff --git a/test/pdfs/issue17981.pdf.link b/test/pdfs/issue17981.pdf.link
@@ -0,0 +1 @@
+https://github.com/mozilla/pdf.js/files/15061082/unhandled_get_annotations.pdf
diff --git a/test/test_manifest.json b/test/test_manifest.json
@@ -2111,6 +2111,16 @@
     "lastPage": 1,
     "type": "eq"
   },
+  {
+    "id": "issue17981",
+    "file": "pdfs/issue17981.pdf",
+    "md5": "3fd6d2bdcad8ff7cc5c0575da0b70266",
+    "rounds": 1,
+    "link": true,
+    "lastPage": 1,
+    "type": "eq",
+    "annotations": true
+  },
   {
     "id": "issue9105_other",
     "file": "pdfs/issue9105_other.pdf",

diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js
@@ -454,10 +454,7 @@ class PDFLinkService {
       }
     } catch {}
 
-    if (
-      typeof dest === "string" ||
-      PDFLinkService.#isValidExplicitDestination(dest)
-    ) {
+    if (typeof dest === "string" || PDFLinkService.#isValidExplicitDest(dest)) {
       this.goToDestination(dest);
       return;
     }
@@ -549,59 +546,53 @@ class PDFLinkService {
     return this.#pagesRefCache.get(refStr) || null;
   }
 
-  static #isValidExplicitDestination(dest) {
-    if (!Array.isArray(dest)) {
-      return false;
-    }
-    const destLength = dest.length;
-    if (destLength < 2) {
+  static #isValidExplicitDest(dest) {
+    if (!Array.isArray(dest) || dest.length < 2) {
       return false;
     }
-    const page = dest[0];
+    const [page, zoom, ...args] = dest;
     if (
       !(
         typeof page === "object" &&
-        Number.isInteger(page.num) &&
-        Number.isInteger(page.gen)
+        Number.isInteger(page?.num) &&
+        Number.isInteger(page?.gen)
       ) &&
-      !(Number.isInteger(page) && page >= 0)
+      !Number.isInteger(page)
     ) {
       return false;
     }
-    const zoom = dest[1];
-    if (!(typeof zoom === "object" && typeof zoom.name === "string")) {
+    if (!(typeof zoom === "object" && typeof zoom?.name === "string")) {
       return false;
     }
     let allowNull = true;
     switch (zoom.name) {
       case "XYZ":
-        if (destLength !== 5) {
+        if (args.length !== 3) {
           return false;
         }
         break;
       case "Fit":
       case "FitB":
-        return destLength === 2;
+        return args.length === 0;
       case "FitH":
       case "FitBH":
       case "FitV":
       case "FitBV":
-        if (destLength !== 3) {
+        if (args.length !== 1) {
           return false;
         }
         break;
       case "FitR":
-        if (destLength !== 6) {
+        if (args.length !== 4) {
           return false;
         }
         allowNull = false;
         break;
       default:
         return false;
     }
-    for (let i = 2; i < destLength; i++) {
-      const param = dest[i];
-      if (!(typeof param === "number" || (allowNull && param === null))) {
+    for (const arg of args) {
+      if (!(typeof arg === "number" || (allowNull && arg === null))) {
         return false;
       }
     }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		https://github.com/mozilla/pdf.js/files/15061082/unhandled_get_annotations.pdf