Merge pull request mozilla#19114 from Snuffleupagus/issue-19075

Ensure that a missing/invalid "Content-Range" header is handled in `PDFNetworkStream` (issue 19075)
timvandermeij · Dec 6, 2024 · 60eba28 · 60eba28
2 parents e04b62b + eff8ede
commit 60eba28
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 15 deletions.
diff --git a/src/display/network.js b/src/display/network.js
@@ -13,7 +13,7 @@
  * limitations under the License.
  */
 
-import { assert, stringToBytes } from "../shared/util.js";
+import { assert, stringToBytes, warn } from "../shared/util.js";
 import {
   createHeaders,
   createResponseStatusError,
@@ -85,11 +85,10 @@ class NetworkManager {
     }
     xhr.responseType = "arraybuffer";
 
-    if (args.onError) {
-      xhr.onerror = function (evt) {
-        args.onError(xhr.status);
-      };
-    }
+    assert(args.onError, "Expected `onError` callback to be provided.");
+    xhr.onerror = () => {
+      args.onError(xhr.status);
+    };
     xhr.onreadystatechange = this.onStateChange.bind(this, xhrId);
     xhr.onprogress = this.onProgress.bind(this, xhrId);
 
@@ -137,7 +136,7 @@ class NetworkManager {
 
     // Success status == 0 can be on ftp, file and other protocols.
     if (xhr.status === 0 && this.isHttp) {
-      pendingRequest.onError?.(xhr.status);
+      pendingRequest.onError(xhr.status);
       return;
     }
     const xhrStatus = xhr.status || OK_RESPONSE;
@@ -153,25 +152,30 @@ class NetworkManager {
       !ok_response_on_range_request &&
       xhrStatus !== pendingRequest.expectedStatus
     ) {
-      pendingRequest.onError?.(xhr.status);
+      pendingRequest.onError(xhr.status);
       return;
     }
 
     const chunk = getArrayBuffer(xhr);
     if (xhrStatus === PARTIAL_CONTENT_RESPONSE) {
       const rangeHeader = xhr.getResponseHeader("Content-Range");
       const matches = /bytes (\d+)-(\d+)\/(\d+)/.exec(rangeHeader);
-      pendingRequest.onDone({
-        begin: parseInt(matches[1], 10),
-        chunk,
-      });
+      if (matches) {
+        pendingRequest.onDone({
+          begin: parseInt(matches[1], 10),
+          chunk,
+        });
+      } else {
+        warn(`Missing or invalid "Content-Range" header.`);
+        pendingRequest.onError(0);
+      }
     } else if (chunk) {
       pendingRequest.onDone({
         begin: 0,
         chunk,
       });
     } else {
-      pendingRequest.onError?.(xhr.status);
+      pendingRequest.onError(xhr.status);
     }
   }
 

diff --git a/test/unit/network_spec.js b/test/unit/network_spec.js
@@ -13,7 +13,10 @@
  * limitations under the License.
  */
 
-import { AbortException } from "../../src/shared/util.js";
+import {
+  AbortException,
+  UnexpectedResponseException,
+} from "../../src/shared/util.js";
 import { PDFNetworkStream } from "../../src/display/network.js";
 import { testCrossOriginRedirects } from "./common_pdfstream_tests.js";
 import { TestPdfsServer } from "./test_utils.js";
@@ -118,6 +121,44 @@ describe("network", function () {
     expect(fullReaderCancelled).toEqual(true);
   });
 
+  it(`handle reading ranges with missing/invalid "Content-Range" header`, async function () {
+    async function readRanges(mode) {
+      const rangeSize = 32768;
+      const stream = new PDFNetworkStream({
+        url: `${pdf1}?test-network-break-ranges=${mode}`,
+        length: pdf1Length,
+        rangeChunkSize: rangeSize,
+        disableStream: true,
+        disableRange: false,
+      });
+
+      const fullReader = stream.getFullReader();
+
+      await fullReader.headersReady;
+      // Ensure that range requests are supported.
+      expect(fullReader.isRangeSupported).toEqual(true);
+      // We shall be able to close the full reader without issues.
+      fullReader.cancel(new AbortException("Don't need fullReader."));
+
+      const rangeReader = stream.getRangeReader(
+        pdf1Length - rangeSize,
+        pdf1Length
+      );
+
+      try {
+        await rangeReader.read();
+
+        // Shouldn't get here.
+        expect(false).toEqual(true);
+      } catch (ex) {
+        expect(ex instanceof UnexpectedResponseException).toEqual(true);
+        expect(ex.status).toEqual(0);
+      }
+    }
+
+    await Promise.all([readRanges("missing"), readRanges("invalid")]);
+  });
+
   describe("Redirects", function () {
     beforeAll(async function () {
       await TestPdfsServer.ensureStarted();

diff --git a/test/webserver.mjs b/test/webserver.mjs
@@ -177,6 +177,7 @@ class WebServer {
       this.#serveFileRange(
         response,
         localURL,
+        url.searchParams,
         fileSize,
         start,
         isNaN(end) ? fileSize : end + 1
@@ -307,7 +308,7 @@ class WebServer {
     stream.pipe(response);
   }
 
-  #serveFileRange(response, fileURL, fileSize, start, end) {
+  #serveFileRange(response, fileURL, searchParams, fileSize, start, end) {
     if (end > fileSize || start > end) {
       response.writeHead(416);
       response.end();
@@ -330,6 +331,16 @@ class WebServer {
       "Content-Range",
       `bytes ${start}-${end - 1}/${fileSize}`
     );
+
+    // Support test in `test/unit/network_spec.js`.
+    switch (searchParams.get("test-network-break-ranges")) {
+      case "missing":
+        response.removeHeader("Content-Range");
+        break;
+      case "invalid":
+        response.setHeader("Content-Range", "bytes abc-def/qwerty");
+        break;
+    }
     response.writeHead(206);
     stream.pipe(response);
   }