From 2be8036eb75f71fc97321d0fcbfb28d1288bc28e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 24 Feb 2022 11:20:27 +0100 Subject: [PATCH 1/2] [api-minor] Reduce duplication in the "gets non-existent page" unit-test --- src/display/api.js | 2 +- test/unit/api_spec.js | 47 ++++++++++++++----------------------------- 2 files changed, 16 insertions(+), 33 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index de847bb50..0fd2f71e5 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -2848,7 +2848,7 @@ class WorkerTransport { pageNumber <= 0 || pageNumber > this._numPages ) { - return Promise.reject(new Error("Invalid page request")); + return Promise.reject(new Error("Invalid page request.")); } const pageIndex = pageNumber - 1, diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 95cc8461c..54c1ef885 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -815,40 +815,23 @@ describe("api", function () { }); it("gets non-existent page", async function () { - let outOfRangePromise = pdfDocument.getPage(100); - let nonIntegerPromise = pdfDocument.getPage(2.5); - let nonNumberPromise = pdfDocument.getPage("1"); + const pageNumbers = [ + /* outOfRange = */ 100, + /* nonInteger = */ 2.5, + /* nonNumber = */ "1", + ]; - outOfRangePromise = outOfRangePromise.then( - function () { - throw new Error("shall fail for out-of-range pageNumber parameter"); - }, - function (reason) { - expect(reason instanceof Error).toEqual(true); - } - ); - nonIntegerPromise = nonIntegerPromise.then( - function () { - throw new Error("shall fail for non-integer pageNumber parameter"); - }, - function (reason) { - expect(reason instanceof Error).toEqual(true); - } - ); - nonNumberPromise = nonNumberPromise.then( - function () { - throw new Error("shall fail for non-number pageNumber parameter"); - }, - function (reason) { - expect(reason instanceof Error).toEqual(true); - } - ); + for (const pageNumber of pageNumbers) { + try { + await pdfDocument.getPage(pageNumber); - await Promise.all([ - outOfRangePromise, - nonIntegerPromise, - nonNumberPromise, - ]); + // Shouldn't get here. + expect(false).toEqual(true); + } catch (reason) { + expect(reason instanceof Error).toEqual(true); + expect(reason.message).toEqual("Invalid page request."); + } + } }); it("gets page, from /Pages tree with circular reference", async function () { From 172d00759873c46ddbef391634a0b2a382e7d873 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Thu, 24 Feb 2022 12:01:51 +0100 Subject: [PATCH 2/2] [api-minor] Add validation for the `PDFDocumentProxy.getPageIndex` method Currently we'll happily attempt to send any argument passed to this method over to the worker-thread, without doing any sort of validation. That could obviously be quite bad, since there's first of all no protection against sending unclonable data. Secondly, it's also possible to pass data that will cause the `Ref.get` call in the worker-thread to fail immediately. In order to address all of these issues, we'll now properly validate the argument passed to `PDFDocumentProxy.getPageIndex` and when necessary reject already on the main-thread instead. --- src/core/worker.js | 4 ++-- src/display/api.js | 13 ++++++++++++- test/unit/api_spec.js | 37 +++++++++++++++++++++++++++---------- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/core/worker.js b/src/core/worker.js index eb819a2df..5ec40561d 100644 --- a/src/core/worker.js +++ b/src/core/worker.js @@ -454,8 +454,8 @@ class WorkerMessageHandler { }); }); - handler.on("GetPageIndex", function wphSetupGetPageIndex({ ref }) { - const pageRef = Ref.get(ref.num, ref.gen); + handler.on("GetPageIndex", function wphSetupGetPageIndex(data) { + const pageRef = Ref.get(data.num, data.gen); return pdfManager.ensureCatalog("getPageIndex", [pageRef]); }); diff --git a/src/display/api.js b/src/display/api.js index 0fd2f71e5..241db612e 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -2879,8 +2879,19 @@ class WorkerTransport { } getPageIndex(ref) { + if ( + typeof ref !== "object" || + ref === null || + !Number.isInteger(ref.num) || + ref.num < 0 || + !Number.isInteger(ref.gen) || + ref.gen < 0 + ) { + return Promise.reject(new Error("Invalid pageIndex request.")); + } return this.messageHandler.sendWithPromise("GetPageIndex", { - ref, + num: ref.num, + gen: ref.gen, }); } diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 54c1ef885..273aa4be0 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -890,18 +890,35 @@ describe("api", function () { }); it("gets invalid page index", async function () { - const ref = { num: 3, gen: 0 }; // Reference to a font dictionary. + const pageRefs = [ + /* fontRef = */ { num: 3, gen: 0 }, + /* invalidRef = */ { num: -1, gen: 0 }, + /* nonRef = */ "qwerty", + /* nullRef = */ null, + ]; - try { - await pdfDocument.getPageIndex(ref); + const expectedErrors = [ + { + exception: UnknownErrorException, + message: "The reference does not point to a /Page dictionary.", + }, + { exception: Error, message: "Invalid pageIndex request." }, + { exception: Error, message: "Invalid pageIndex request." }, + { exception: Error, message: "Invalid pageIndex request." }, + ]; - // Shouldn't get here. - expect(false).toEqual(true); - } catch (reason) { - expect(reason instanceof UnknownErrorException).toEqual(true); - expect(reason.message).toEqual( - "The reference does not point to a /Page dictionary." - ); + for (let i = 0, ii = pageRefs.length; i < ii; i++) { + try { + await pdfDocument.getPageIndex(pageRefs[i]); + + // Shouldn't get here. + expect(false).toEqual(true); + } catch (reason) { + const { exception, message } = expectedErrors[i]; + + expect(reason instanceof exception).toEqual(true); + expect(reason.message).toEqual(message); + } } });