From 2661d0623bb002eedc8948e190ef9419a3593646 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Wed, 27 Nov 2024 12:33:39 +0100 Subject: [PATCH 1/2] Ensure that a missing/invalid "Content-Range" header is handled in `PDFNetworkStream` (issue 19075) In the event that the "Content-Range" header is missing/invalid, loading will now be aborted rather than hanging indefinitely. --- src/display/network.js | 15 +++++++++----- test/unit/network_spec.js | 43 ++++++++++++++++++++++++++++++++++++++- test/webserver.mjs | 13 +++++++++++- 3 files changed, 64 insertions(+), 7 deletions(-) diff --git a/src/display/network.js b/src/display/network.js index 3beb3257bd546..8f08e27c5f51c 100644 --- 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, @@ -161,10 +161,15 @@ class NetworkManager { 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, diff --git a/test/unit/network_spec.js b/test/unit/network_spec.js index 367e525601ed9..ebcb1a89103ac 100644 --- 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 index fffd8d72aa5ec..dd2ae735ecbd1 100644 --- 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); } From eff8ede33ec8d0e338ca50d6fa8dad45ad54f481 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 1 Dec 2024 11:36:18 +0100 Subject: [PATCH 2/2] Make the `onError` callback required in `NetworkManager` This helps ensure that loading errors are always handled correctly, and note that both `PDFNetworkStreamFullRequestReader` and `PDFNetworkStreamRangeRequestReader` already provided such a callback. --- src/display/network.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/display/network.js b/src/display/network.js index 8f08e27c5f51c..930c2d39639a5 100644 --- a/src/display/network.js +++ b/src/display/network.js @@ -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,7 +152,7 @@ class NetworkManager { !ok_response_on_range_request && xhrStatus !== pendingRequest.expectedStatus ) { - pendingRequest.onError?.(xhr.status); + pendingRequest.onError(xhr.status); return; } @@ -168,7 +167,7 @@ class NetworkManager { }); } else { warn(`Missing or invalid "Content-Range" header.`); - pendingRequest.onError?.(0); + pendingRequest.onError(0); } } else if (chunk) { pendingRequest.onDone({ @@ -176,7 +175,7 @@ class NetworkManager { chunk, }); } else { - pendingRequest.onError?.(xhr.status); + pendingRequest.onError(xhr.status); } }