From 4ab4efd42f4e868d7813f68fc1eb265fb4bc12a9 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 31 Jul 2021 22:15:30 +0200 Subject: [PATCH] Change the `parseQueryString` function to return a `Map` rather than an Object (issue 13829) Even though the code as-is *should* be safe, given that we're using an Object with a `null` prototype, it cannot hurt to change this to a Map to prevent any issues (since we're parsing unknown and potentially unsafe data). Overall I also think that these changes improve the `parseQueryString` call-sites, since we now have a proper way of checking for the existence of a particular key (and don't have to use `in` which stringifies the keys in the Object). This patch also changes the default, when no `value` exists, from `null` to an empty string since the use of `decodeURIComponent` currently can modify the value in a somewhat surprising way (at least to me). Note how `decodeURIComponent(null) === "null"` which is unlikely to be what you actually want, whereas `decodeURIComponent("") === ""` which seems much more helpful. --- web/app.js | 47 ++++++++++++++++++++++------------------- web/pdf_history.js | 6 +++--- web/pdf_link_service.js | 22 +++++++++---------- web/ui_utils.js | 14 ++++++------ 4 files changed, 47 insertions(+), 42 deletions(-) diff --git a/web/app.js b/web/app.js index 5683273d9..4977827ae 100644 --- a/web/app.js +++ b/web/app.js @@ -329,35 +329,38 @@ const PDFViewerApplication = { if (!hash) { return undefined; } - const hashParams = parseQueryString(hash), + const params = parseQueryString(hash), waitOn = []; - if ("disableworker" in hashParams && hashParams.disableworker === "true") { + if (params.get("disableworker") === "true") { waitOn.push(loadFakeWorker()); } - if ("disablerange" in hashParams) { - AppOptions.set("disableRange", hashParams.disablerange === "true"); + if (params.has("disablerange")) { + AppOptions.set("disableRange", params.get("disablerange") === "true"); } - if ("disablestream" in hashParams) { - AppOptions.set("disableStream", hashParams.disablestream === "true"); + if (params.has("disablestream")) { + AppOptions.set("disableStream", params.get("disablestream") === "true"); } - if ("disableautofetch" in hashParams) { + if (params.has("disableautofetch")) { AppOptions.set( "disableAutoFetch", - hashParams.disableautofetch === "true" + params.get("disableautofetch") === "true" ); } - if ("disablefontface" in hashParams) { - AppOptions.set("disableFontFace", hashParams.disablefontface === "true"); + if (params.has("disablefontface")) { + AppOptions.set( + "disableFontFace", + params.get("disablefontface") === "true" + ); } - if ("disablehistory" in hashParams) { - AppOptions.set("disableHistory", hashParams.disablehistory === "true"); + if (params.has("disablehistory")) { + AppOptions.set("disableHistory", params.get("disablehistory") === "true"); } - if ("verbosity" in hashParams) { - AppOptions.set("verbosity", hashParams.verbosity | 0); + if (params.has("verbosity")) { + AppOptions.set("verbosity", params.get("verbosity") | 0); } - if ("textlayer" in hashParams) { - switch (hashParams.textlayer) { + if (params.has("textlayer")) { + switch (params.get("textlayer")) { case "off": AppOptions.set("textLayerMode", TextLayerMode.DISABLE); break; @@ -365,24 +368,24 @@ const PDFViewerApplication = { case "shadow": case "hover": const viewer = this.appConfig.viewerContainer; - viewer.classList.add("textLayer-" + hashParams.textlayer); + viewer.classList.add(`textLayer-${params.get("textlayer")}`); break; } } - if ("pdfbug" in hashParams) { + if (params.has("pdfbug")) { AppOptions.set("pdfBug", true); AppOptions.set("fontExtraProperties", true); - const enabled = hashParams.pdfbug.split(","); + const enabled = params.get("pdfbug").split(","); waitOn.push(loadAndEnablePDFBug(enabled)); } // It is not possible to change locale for the (various) extension builds. if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("!PRODUCTION || GENERIC")) && - "locale" in hashParams + params.has("locale") ) { - AppOptions.set("locale", hashParams.locale); + AppOptions.set("locale", params.get("locale")); } if (waitOn.length === 0) { @@ -2167,7 +2170,7 @@ function webViewerInitialized() { if (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) { const queryString = document.location.search.substring(1); const params = parseQueryString(queryString); - file = "file" in params ? params.file : AppOptions.get("defaultUrl"); + file = params.get("file") ?? AppOptions.get("defaultUrl"); validateFileURL(file); } else if (PDFJSDev.test("MOZCENTRAL")) { file = window.location.href; diff --git a/web/pdf_history.js b/web/pdf_history.js index 042338082..4a49ee5a7 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -544,8 +544,8 @@ class PDFHistory { const hash = unescape(getCurrentHash()).substring(1); const params = parseQueryString(hash); - const nameddest = params.nameddest || ""; - let page = params.page | 0; + const nameddest = params.get("nameddest") || ""; + let page = params.get("page") | 0; if (!this._isValidPage(page) || (checkNameddest && nameddest.length > 0)) { page = null; @@ -753,7 +753,7 @@ function isDestHashesEqual(destHash, pushHash) { if (destHash === pushHash) { return true; } - const { nameddest } = parseQueryString(destHash); + const nameddest = parseQueryString(destHash).get("nameddest"); if (nameddest === pushHash) { return true; } diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index e73c11fe1..490e0116f 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -263,20 +263,20 @@ class PDFLinkService { let pageNumber, dest; if (hash.includes("=")) { const params = parseQueryString(hash); - if ("search" in params) { + if (params.has("search")) { this.eventBus.dispatch("findfromurlhash", { source: this, - query: params.search.replace(/"/g, ""), - phraseSearch: params.phrase === "true", + query: params.get("search").replace(/"/g, ""), + phraseSearch: params.get("phrase") === "true", }); } // borrowing syntax from "Parameters for Opening PDF Files" - if ("page" in params) { - pageNumber = params.page | 0 || 1; + if (params.has("page")) { + pageNumber = params.get("page") | 0 || 1; } - if ("zoom" in params) { + if (params.has("zoom")) { // Build the destination array. - const zoomArgs = params.zoom.split(","); // scale,left,top + const zoomArgs = params.get("zoom").split(","); // scale,left,top const zoomArg = zoomArgs[0]; const zoomArgNumber = parseFloat(zoomArg); @@ -336,16 +336,16 @@ class PDFLinkService { } else if (pageNumber) { this.page = pageNumber; // simple page } - if ("pagemode" in params) { + if (params.has("pagemode")) { this.eventBus.dispatch("pagemode", { source: this, - mode: params.pagemode, + mode: params.get("pagemode"), }); } // Ensure that this parameter is *always* handled last, in order to // guarantee that it won't be overridden (e.g. by the "page" parameter). - if ("nameddest" in params) { - this.goToDestination(params.nameddest); + if (params.has("nameddest")) { + this.goToDestination(params.get("nameddest")); } } else { // Named (or explicit) destination. diff --git a/web/ui_utils.js b/web/ui_utils.js index 672c56f69..985da3f8a 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -180,16 +180,18 @@ function watchScroll(viewAreaElement, callback) { } /** - * Helper function to parse query string (e.g. ?param1=value&parm2=...). + * Helper function to parse query string (e.g. ?param1=value¶m2=...). + * @param {string} + * @returns {Map} */ function parseQueryString(query) { const parts = query.split("&"); - const params = Object.create(null); + const params = new Map(); for (let i = 0, ii = parts.length; i < ii; ++i) { - const param = parts[i].split("="); - const key = param[0].toLowerCase(); - const value = param.length > 1 ? param[1] : null; - params[decodeURIComponent(key)] = decodeURIComponent(value); + const param = parts[i].split("="), + key = param[0].toLowerCase(), + value = param.length > 1 ? param[1] : ""; + params.set(decodeURIComponent(key), decodeURIComponent(value)); } return params; }