From 01ab15a6f17b2f71cd1403f84aae2d09002cccda Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 16 May 2016 16:28:25 +0200 Subject: [PATCH 1/2] [api-minor] Let `Catalog_getPageIndex` check that the `Ref` actually points to a /Page dictionary Currently the `getPageIndex` method will happily return `0`, even if the `Ref` parameter doesn't actually point to a proper /Page dictionary. Having the API trust that the consumer is doing the right thing seems error-prone, hence this patch which adds a check for this case. Given that the `Catalog_getPageIndex` method isn't used in any hot part of the codebase, this extra check shouldn't be a problem. (Note: in the standard viewer, it is only ever used from `PDFLinkService_navigateTo` if a destination needs to be resolved during document loading, which isn't common enough to be an issue IMHO.) --- src/core/obj.js | 13 ++++++++++--- src/core/primitives.js | 5 +++++ src/display/api.js | 7 ++++++- test/unit/api_spec.js | 10 ++++++++++ test/unit/primitives_spec.js | 16 +++++++++++++++- 5 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/core/obj.js b/src/core/obj.js index 28f28e21d..b104f58fe 100644 --- a/src/core/obj.js +++ b/src/core/obj.js @@ -56,6 +56,7 @@ var isName = corePrimitives.isName; var isCmd = corePrimitives.isCmd; var isDict = corePrimitives.isDict; var isRef = corePrimitives.isRef; +var isRefsEqual = corePrimitives.isRefsEqual; var isStream = corePrimitives.isStream; var CipherTransformFactory = coreCrypto.CipherTransformFactory; var Lexer = coreParser.Lexer; @@ -522,7 +523,7 @@ var Catalog = (function CatalogClosure() { return capability.promise; }, - getPageIndex: function Catalog_getPageIndex(ref) { + getPageIndex: function Catalog_getPageIndex(pageRef) { // The page tree nodes have the count of all the leaves below them. To get // how many pages are before we just have to walk up the tree and keep // adding the count of siblings to the left of the node. @@ -531,15 +532,21 @@ var Catalog = (function CatalogClosure() { var total = 0; var parentRef; return xref.fetchAsync(kidRef).then(function (node) { + if (isRefsEqual(kidRef, pageRef) && !isDict(node, 'Page') && + !(isDict(node) && !node.has('Type') && node.has('Contents'))) { + throw new Error('The reference does not point to a /Page Dict.'); + } if (!node) { return null; } + assert(isDict(node), 'node must be a Dict.'); parentRef = node.getRaw('Parent'); return node.getAsync('Parent'); }).then(function (parent) { if (!parent) { return null; } + assert(isDict(parent), 'parent must be a Dict.'); return parent.getAsync('Kids'); }).then(function (kids) { if (!kids) { @@ -549,7 +556,7 @@ var Catalog = (function CatalogClosure() { var found = false; for (var i = 0; i < kids.length; i++) { var kid = kids[i]; - assert(isRef(kid), 'kids must be a ref'); + assert(isRef(kid), 'kid must be a Ref.'); if (kid.num === kidRef.num) { found = true; break; @@ -585,7 +592,7 @@ var Catalog = (function CatalogClosure() { }); } - return next(ref); + return next(pageRef); } }; diff --git a/src/core/primitives.js b/src/core/primitives.js index 730bacb1f..ee4d5e4ea 100644 --- a/src/core/primitives.js +++ b/src/core/primitives.js @@ -290,6 +290,10 @@ function isRef(v) { return v instanceof Ref; } +function isRefsEqual(v1, v2) { + return v1.num === v2.num && v1.gen === v2.gen; +} + function isStream(v) { return typeof v === 'object' && v !== null && v.getBytes !== undefined; } @@ -304,5 +308,6 @@ exports.isCmd = isCmd; exports.isDict = isDict; exports.isName = isName; exports.isRef = isRef; +exports.isRefsEqual = isRefsEqual; exports.isStream = isStream; })); diff --git a/src/display/api.js b/src/display/api.js index 77edfc2fd..cfdfb1a9f 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -1683,7 +1683,12 @@ var WorkerTransport = (function WorkerTransportClosure() { }, getPageIndex: function WorkerTransport_getPageIndexByRef(ref) { - return this.messageHandler.sendWithPromise('GetPageIndex', { ref: ref }); + return this.messageHandler.sendWithPromise('GetPageIndex', { ref: ref }). + then(function (pageIndex) { + return pageIndex; + }, function (reason) { + return Promise.reject(new Error(reason)); + }); }, getAnnotations: function WorkerTransport_getAnnotations(pageIndex, intent) { diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 66b6f2ede..415b527be 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -360,6 +360,16 @@ describe('api', function() { done.fail(reason); }); }); + it('gets invalid page index', function (done) { + var ref = { num: 3, gen: 0 }; // Reference to a font dictionary. + var promise = doc.getPageIndex(ref); + promise.then(function () { + done.fail('shall fail for invalid page reference.'); + }, function (data) { + expect(data instanceof Error).toEqual(true); + done(); + }); + }); it('gets destinations, from /Dests dictionary', function(done) { var promise = doc.getDestinations(); diff --git a/test/unit/primitives_spec.js b/test/unit/primitives_spec.js index a19801feb..be9354c47 100644 --- a/test/unit/primitives_spec.js +++ b/test/unit/primitives_spec.js @@ -1,5 +1,5 @@ /* globals expect, it, describe, beforeEach, Name, Dict, Ref, RefSet, Cmd, - jasmine, isDict */ + jasmine, isDict, isRefsEqual */ 'use strict'; @@ -158,4 +158,18 @@ describe('primitives', function() { expect(isDict(dict, 'Page')).toEqual(true); }); }); + + describe('isRefsEqual', function () { + it('should handle different Refs pointing to the same object', function () { + var ref1 = new Ref(1, 0); + var ref2 = new Ref(1, 0); + expect(isRefsEqual(ref1, ref2)).toEqual(true); + }); + + it('should handle Refs pointing to different objects', function () { + var ref1 = new Ref(1, 0); + var ref2 = new Ref(2, 0); + expect(isRefsEqual(ref1, ref2)).toEqual(false); + }); + }); }); From b354682dd64df36859e1f4e8ca14816d8204d02e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 15 May 2016 12:12:47 +0200 Subject: [PATCH 2/2] [api-minor] Let `LinkAnnotation`/`PDFLinkService_getDestinationHash` return a stringified version of the destination array for explicit destinations Currently for explicit destinations, compared to named destinations, we manually try to build a hash that often times is a quite poor representation of the *actual* destination. (Currently this only, kind of, works for `\XYZ` destinations.) For PDF files using explicit destinations, this can make it difficult/impossible to obtain a link to a specific section of the document through the URL. Note that in practice most PDF files, especially newer ones, use named destinations and these are thus unnaffected by this patch. This patch also fixes an existing issue in `PDFLinkService_getDestinationHash`, where a named destination consisting of only a number would not be handled correctly. With the added, and already existing, type checks in place for destinations, I really don't think that this patch exposes any "sensitive" internal destination code not already accessible through normal hash parameters. *Please note:* Just trying to improve the algorithm that generates the hash is unfortunately not possible in general, since there are a number of cases where it will simply never work well. - First of all, note that `getDestinationHash` currently relies on the `_pagesRefCache`, hence it's possible that the hash returned is empty during e.g. ranged/streamed loading of a PDF file. - Second of all, the currently computed hash is actually dependent on the document rotation. With named destinations, the fetched internal destination array is rotational invariant (as it should be), but this will not hold in general for the hash. We can easily avoid this issue by using a stringified destination array. - Third of all, note that according to the PDF specification[1], `GoToR` destinations may actually contain explicit destination arrays. Since we cannot really construct a hash in `annotation.js`, we currently have no good way to support those. Even though this case seems *very* rare in practice (I've not actually seen such a PDF file), it's in the specification, and this patch allows us to support that for "free". --- [1] http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/PDF32000_2008.pdf#G11.1951685 --- src/core/annotation.js | 12 ++- test/unit/annotation_layer_spec.js | 58 ++++++++++++++- web/app.js | 6 +- web/pdf_link_service.js | 116 +++++++++++++++++++++-------- web/pdf_viewer.js | 2 + 5 files changed, 155 insertions(+), 39 deletions(-) diff --git a/src/core/annotation.js b/src/core/annotation.js index 55c7531a9..64a406c80 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -743,9 +743,17 @@ var LinkAnnotation = (function LinkAnnotationClosure() { if (isName(remoteDest)) { remoteDest = remoteDest.name; } - if (isString(remoteDest) && isString(url)) { + if (isString(url)) { var baseUrl = url.split('#')[0]; - url = baseUrl + '#' + remoteDest; + if (isString(remoteDest)) { + // In practice, a named destination may contain only a number. + // If that happens, use the '#nameddest=' form to avoid the link + // redirecting to a page, instead of the correct destination. + url = baseUrl + '#' + + (/^\d+$/.test(remoteDest) ? 'nameddest=' : '') + remoteDest; + } else if (isArray(remoteDest)) { + url = baseUrl + '#' + JSON.stringify(remoteDest); + } } } // The 'NewWindow' property, equal to `LinkTarget.BLANK`. diff --git a/test/unit/annotation_layer_spec.js b/test/unit/annotation_layer_spec.js index 31911bcb9..3a8ffea38 100644 --- a/test/unit/annotation_layer_spec.js +++ b/test/unit/annotation_layer_spec.js @@ -268,8 +268,9 @@ describe('Annotation layer', function() { var actionDict = new Dict(); actionDict.set('Type', Name.get('Action')); actionDict.set('S', Name.get('GoToR')); - actionDict.set('F', '../../0021/002156/215675E.pdf'); - actionDict.set('D', '15'); + actionDict.set('F', '../../0013/001346/134685E.pdf'); + actionDict.set('D', '4.3'); + actionDict.set('NewWindow', true); var annotationDict = new Dict(); annotationDict.set('Type', Name.get('Annot')); @@ -283,7 +284,58 @@ describe('Annotation layer', function() { var data = annotation.data; expect(data.annotationType).toEqual(AnnotationType.LINK); - expect(data.url).toBeUndefined(); + expect(data.url).toBeUndefined(); // ../../0013/001346/134685E.pdf#4.3 + expect(data.dest).toBeUndefined(); + expect(data.newWindow).toEqual(true); + }); + + it('should correctly parse a GoToR action, with named destination', + function() { + var actionDict = new Dict(); + actionDict.set('Type', Name.get('Action')); + actionDict.set('S', Name.get('GoToR')); + actionDict.set('F', 'http://www.example.com/test.pdf'); + actionDict.set('D', '15'); + + var annotationDict = new Dict(); + annotationDict.set('Type', Name.get('Annot')); + annotationDict.set('Subtype', Name.get('Link')); + annotationDict.set('A', actionDict); + + var xrefMock = new XrefMock([annotationDict]); + var annotationRef = new Ref(495, 0); + + var annotation = annotationFactory.create(xrefMock, annotationRef); + var data = annotation.data; + expect(data.annotationType).toEqual(AnnotationType.LINK); + + expect(data.url).toEqual('http://www.example.com/test.pdf#nameddest=15'); + expect(data.dest).toBeUndefined(); + expect(data.newWindow).toBeFalsy(); + }); + + it('should correctly parse a GoToR action, with explicit destination array', + function() { + var actionDict = new Dict(); + actionDict.set('Type', Name.get('Action')); + actionDict.set('S', Name.get('GoToR')); + actionDict.set('F', 'http://www.example.com/test.pdf'); + actionDict.set('D', [14, Name.get('XYZ'), null, 298.043, null]); + + var annotationDict = new Dict(); + annotationDict.set('Type', Name.get('Annot')); + annotationDict.set('Subtype', Name.get('Link')); + annotationDict.set('A', actionDict); + + var xrefMock = new XrefMock([annotationDict]); + var annotationRef = new Ref(489, 0); + + var annotation = annotationFactory.create(xrefMock, annotationRef); + var data = annotation.data; + expect(data.annotationType).toEqual(AnnotationType.LINK); + + expect(data.url).toEqual('http://www.example.com/test.pdf#' + + '[14,{"name":"XYZ"},null,298.043,null]'); expect(data.dest).toBeUndefined(); expect(data.newWindow).toBeFalsy(); }); diff --git a/web/app.js b/web/app.js index 155d01c36..9b8361146 100644 --- a/web/app.js +++ b/web/app.js @@ -857,6 +857,7 @@ var PDFViewerApplication = { pdfViewer.setDocument(pdfDocument); var firstPagePromise = pdfViewer.firstPagePromise; var pagesPromise = pdfViewer.pagesPromise; + var onePageRendered = pdfViewer.onePageRendered; this.pageRotation = 0; @@ -962,9 +963,8 @@ var PDFViewerApplication = { } }); - // outline depends on pagesRefMap - var promises = [pagesPromise, this.animationStartedPromise]; - Promise.all(promises).then(function() { + Promise.all([onePageRendered, this.animationStartedPromise]).then( + function() { pdfDocument.getOutline().then(function(outline) { self.pdfOutlineViewer.render({ outline: outline }); }); diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index 9228dd914..57ece6429 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -29,6 +29,11 @@ var parseQueryString = uiUtils.parseQueryString; +var PageNumberRegExp = /^\d+$/; +function isPageNumber(str) { + return PageNumberRegExp.test(str); +} + /** * @typedef {Object} PDFLinkServiceOptions * @property {EventBus} eventBus - The application event bus. @@ -40,7 +45,7 @@ var parseQueryString = uiUtils.parseQueryString; * @class * @implements {IPDFLinkService} */ -var PDFLinkService = (function () { +var PDFLinkService = (function PDFLinkServiceClosure() { /** * @constructs PDFLinkService * @param {PDFLinkServiceOptions} options @@ -100,7 +105,7 @@ var PDFLinkService = (function () { var self = this; var goToDestination = function(destRef) { - // dest array looks like that: + // dest array looks like that: var pageNumber = destRef instanceof Object ? self._pagesRefCache[destRef.num + ' ' + destRef.gen + ' R'] : (destRef + 1); @@ -150,30 +155,15 @@ var PDFLinkService = (function () { */ getDestinationHash: function PDFLinkService_getDestinationHash(dest) { if (typeof dest === 'string') { - return this.getAnchorUrl('#' + escape(dest)); + // In practice, a named destination may contain only a number. + // If that happens, use the '#nameddest=' form to avoid the link + // redirecting to a page, instead of the correct destination. + return this.getAnchorUrl( + '#' + (isPageNumber(dest) ? 'nameddest=' : '') + escape(dest)); } if (dest instanceof Array) { - var destRef = dest[0]; // see navigateTo method for dest format - var pageNumber = destRef instanceof Object ? - this._pagesRefCache[destRef.num + ' ' + destRef.gen + ' R'] : - (destRef + 1); - if (pageNumber) { - var pdfOpenParams = this.getAnchorUrl('#page=' + pageNumber); - var destKind = dest[1]; - if (typeof destKind === 'object' && 'name' in destKind && - destKind.name === 'XYZ') { - var scale = (dest[4] || this.pdfViewer.currentScaleValue); - var scaleNumber = parseFloat(scale); - if (scaleNumber) { - scale = scaleNumber * 100; - } - pdfOpenParams += '&zoom=' + scale; - if (dest[2] || dest[3]) { - pdfOpenParams += ',' + (dest[2] || 0) + ',' + (dest[3] || 0); - } - } - return pdfOpenParams; - } + var str = JSON.stringify(dest); + return this.getAnchorUrl('#' + escape(str)); } return this.getAnchorUrl(''); }, @@ -192,6 +182,7 @@ var PDFLinkService = (function () { * @param {string} hash */ setHash: function PDFLinkService_setHash(hash) { + var pageNumber, dest; if (hash.indexOf('=') >= 0) { var params = parseQueryString(hash); // borrowing syntax from "Parameters for Opening PDF Files" @@ -202,7 +193,6 @@ var PDFLinkService = (function () { this.navigateTo(params.nameddest); return; } - var pageNumber, dest; if ('page' in params) { pageNumber = (params.page | 0) || 1; } @@ -252,13 +242,23 @@ var PDFLinkService = (function () { mode: params.pagemode }); } - } else if (/^\d+$/.test(hash)) { // page number - this.page = hash; - } else { // named destination - if (this.pdfHistory) { - this.pdfHistory.updateNextHashParam(unescape(hash)); + } else if (isPageNumber(hash)) { // Page number. + this.page = hash | 0; + } else { // Named (or explicit) destination. + dest = unescape(hash); + try { + dest = JSON.parse(dest); + } catch (ex) {} + + if (typeof dest === 'string' || isValidExplicitDestination(dest)) { + if (this.pdfHistory) { + this.pdfHistory.updateNextHashParam(dest); + } + this.navigateTo(dest); + return; } - this.navigateTo(unescape(hash)); + console.error('PDFLinkService_setHash: \'' + unescape(hash) + + '\' is not a valid destination.'); } }, @@ -316,6 +316,60 @@ var PDFLinkService = (function () { } }; + function isValidExplicitDestination(dest) { + if (!(dest instanceof Array)) { + return false; + } + var destLength = dest.length, allowNull = true; + if (destLength < 2) { + return false; + } + var page = dest[0]; + if (!(typeof page === 'object' && + typeof page.num === 'number' && (page.num | 0) === page.num && + typeof page.gen === 'number' && (page.gen | 0) === page.gen) && + !(typeof page === 'number' && (page | 0) === page && page >= 0)) { + return false; + } + var zoom = dest[1]; + if (!(typeof zoom === 'object' && typeof zoom.name === 'string')) { + return false; + } + switch (zoom.name) { + case 'XYZ': + if (destLength !== 5) { + return false; + } + break; + case 'Fit': + case 'FitB': + return destLength === 2; + case 'FitH': + case 'FitBH': + case 'FitV': + case 'FitBV': + if (destLength !== 3) { + return false; + } + break; + case 'FitR': + if (destLength !== 6) { + return false; + } + allowNull = false; + break; + default: + return false; + } + for (var i = 2; i < destLength; i++) { + var param = dest[i]; + if (!(typeof param === 'number' || (allowNull && param === null))) { + return false; + } + } + return true; + } + return PDFLinkService; })(); diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 1a79b3745..d60d13cfb 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -587,6 +587,8 @@ var PDFViewer = (function pdfViewer() { scale = Math.min(Math.abs(widthScale), Math.abs(heightScale)); break; default: + console.error('PDFViewer_scrollPageIntoView: \'' + dest[1].name + + '\' is not a valid destination type.'); return; }