From 26bc3a959f63743f34284cd8111e72045044bef0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcos=20C=C3=A1ceres?= Date: Thu, 15 Aug 2019 12:55:35 +0000 Subject: [PATCH] Bug 1559412 - Support web app manifest ImageResource.purpose r=baku implementation of purpose member Differential Revision: https://phabricator.services.mozilla.com/D38627 --HG-- extra : moz-landing-system : lando --- dom/locales/en-US/chrome/dom/dom.properties | 6 + dom/manifest/ImageObjectProcessor.jsm | 114 ++++++++++++++++-- dom/manifest/ManifestObtainer.jsm | 12 +- dom/manifest/ValueExtractor.jsm | 14 ++- dom/manifest/test/mochitest.ini | 1 + .../test_ImageObjectProcessor_purpose.html | 100 +++++++++++++++ .../test/test_ManifestProcessor_warnings.html | 58 ++++++++- 7 files changed, 289 insertions(+), 16 deletions(-) create mode 100644 dom/manifest/test/test_ImageObjectProcessor_purpose.html diff --git a/dom/locales/en-US/chrome/dom/dom.properties b/dom/locales/en-US/chrome/dom/dom.properties index 07b9fdc8afc8..c8d083aa6b09 100644 --- a/dom/locales/en-US/chrome/dom/dom.properties +++ b/dom/locales/en-US/chrome/dom/dom.properties @@ -250,6 +250,12 @@ ManifestInvalidCSSColor=%1$S: %2$S is not a valid CSS color. ManifestLangIsInvalid=%1$S: %2$S is not a valid language code. # LOCALIZATION NOTE: %1$S is the name of the parent property whose value is invalid (e.g., "icons"). %2$S is the index of the image object that is invalid (from 0). %3$S is the name of actual member that is invalid. %4$S is the invalid value. E.g. "icons item at index 2 is invalid. The src member is an invalid URL http://:Invalid" ManifestImageURLIsInvalid=%1$S item at index %2$S is invalid. The %3$S member is an invalid URL %4$S +# LOCALIZATION NOTE: %1$S is the name of the parent property that that contains the unusable image object (e.g., "icons"). %2$S is the index of the image object that is unusable (from 0). E.g. "icons item at index 2 lacks a usable purpose. It will be ignored." +ManifestImageUnusable=%1$S item at index %2$S lacks a usable purpose. It will be ignored. +# LOCALIZATION NOTE: %1$S is the name of the parent property that contains the unsupported value (e.g., "icons"). %2$S is the index of the image object that has the unsupported value (from 0). %3$S are the unknown purposes. E.g. "icons item at index 2 includes unsupported purpose(s): a b." +ManifestImageUnsupportedPurposes=%1$S item at index %2$S includes unsupported purpose(s): %3$S. +# LOCALIZATION NOTE: %1$S is the name of the parent property that has a repeated purpose (e.g., "icons"). %2$S is the index of the image object that has the repeated purpose (from 0). %3$S is the repeated purposes. E.g. "icons item at index 2 includes repeated purpose(s): a b." +ManifestImageRepeatedPurposes=%1$S item at index %2$S includes repeated purpose(s): %3$S. PatternAttributeCompileFailure=Unable to check because the pattern is not a valid regexp: %S # LOCALIZATION NOTE: Do not translate "postMessage" or DOMWindow. %S values are origins, like https://domain.com:port TargetPrincipalDoesNotMatch=Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘%S’) does not match the recipient window’s origin (‘%S’). diff --git a/dom/manifest/ImageObjectProcessor.jsm b/dom/manifest/ImageObjectProcessor.jsm index b9b0961987fb..323096cd5642 100644 --- a/dom/manifest/ImageObjectProcessor.jsm +++ b/dom/manifest/ImageObjectProcessor.jsm @@ -34,6 +34,8 @@ function ImageObjectProcessor(aErrors, aExtractor, aBundle) { this.domBundle = aBundle; } +const iconPurposes = Object.freeze(["any", "maskable"]); + // Static getters Object.defineProperties(ImageObjectProcessor, { decimals: { @@ -64,20 +66,111 @@ ImageObjectProcessor.prototype.process = function( const images = []; const value = extractor.extractValue(spec); if (Array.isArray(value)) { - // Filter out images whose "src" is not useful. value - .filter((item, index) => !!processSrcMember(item, aBaseURL, index)) .map(toImageObject) + // Filter out images that resulted in "failure", per spec. + .filter(image => image) .forEach(image => images.push(image)); } return images; - function toImageObject(aImageSpec) { - return { - src: processSrcMember(aImageSpec, aBaseURL), - type: processTypeMember(aImageSpec), - sizes: processSizesMember(aImageSpec), + function toImageObject(aImageSpec, index) { + let img; // if "failure" happens below, we return undefined. + try { + // can throw + const src = processSrcMember(aImageSpec, aBaseURL, index); + // can throw + const purpose = processPurposeMember(aImageSpec, index); + const type = processTypeMember(aImageSpec); + const sizes = processSizesMember(aImageSpec); + img = { + src, + purpose, + type, + sizes, + }; + } catch (err) { + /* Errors are collected by each process* function */ + } + return img; + } + + function processPurposeMember(aImage, index) { + const spec = { + objectName: "image", + object: aImage, + property: "purpose", + expectedType: "string", + trim: true, + throwTypeError: true, }; + + // Type errors are treated at "any"... + let value; + try { + value = extractor.extractValue(spec); + } catch (err) { + return ["any"]; + } + + // Was only whitespace... + if (!value) { + return ["any"]; + } + + const keywords = value.split(/\s+/); + + // Emtpy is treated as "any"... + if (keywords.length === 0) { + return ["any"]; + } + + // We iterate over keywords and classify them into: + const purposes = new Set(); + const unknownPurposes = new Set(); + const repeatedPurposes = new Set(); + + for (const keyword of keywords) { + const canonicalKeyword = keyword.toLowerCase(); + + if (purposes.has(canonicalKeyword)) { + repeatedPurposes.add(keyword); + continue; + } + + iconPurposes.includes(canonicalKeyword) + ? purposes.add(canonicalKeyword) + : unknownPurposes.add(keyword); + } + + // Tell developer about unknown purposes... + if (unknownPurposes.size) { + const warn = domBundle.formatStringFromName( + "ManifestImageUnsupportedPurposes", + [aMemberName, index, [...unknownPurposes].join(" ")] + ); + errors.push({ warn }); + } + + // Tell developer about repeated purposes... + if (repeatedPurposes.size) { + const warn = domBundle.formatStringFromName( + "ManifestImageRepeatedPurposes", + [aMemberName, index, [...repeatedPurposes].join(" ")] + ); + errors.push({ warn }); + } + + if (purposes.size === 0) { + const warn = domBundle.formatStringFromName("ManifestImageUnusable", [ + aMemberName, + index, + ]); + errors.push({ warn }); + throw new TypeError(warn); + } + + return [...purposes]; } function processTypeMember(aImage) { @@ -108,9 +201,15 @@ ImageObjectProcessor.prototype.process = function( property: "src", expectedType: "string", trim: false, + throwTypeError: true, }; const value = extractor.extractValue(spec); let url; + if (typeof value === "undefined" || value === "") { + // We throw here as the value is unusable, + // but it's not an developer error. + throw new TypeError(); + } if (value && value.length) { try { url = new URL(value, aBaseURL).href; @@ -120,6 +219,7 @@ ImageObjectProcessor.prototype.process = function( [aMemberName, index, "src", value] ); errors.push({ warn }); + throw e; } } return url; diff --git a/dom/manifest/ManifestObtainer.jsm b/dom/manifest/ManifestObtainer.jsm index 96cabd64732e..40a026cd51b1 100644 --- a/dom/manifest/ManifestObtainer.jsm +++ b/dom/manifest/ManifestObtainer.jsm @@ -69,14 +69,18 @@ var ManifestObtainer = { * Adds proprietary moz_* members to manifest. * @return {Promise} The processed manifest. */ - contentObtainManifest(aContent, aOptions = { checkConformance: false }) { + async contentObtainManifest( + aContent, + aOptions = { checkConformance: false } + ) { if (!aContent || isXULBrowser(aContent)) { const err = new TypeError("Invalid input. Expected a DOM Window."); return Promise.reject(err); } - return fetchManifest(aContent).then(response => - processResponse(response, aContent, aOptions) - ); + const response = await fetchManifest(aContent); + const result = await processResponse(response, aContent, aOptions); + const clone = Cu.cloneInto(result, aContent); + return clone; }, }; diff --git a/dom/manifest/ValueExtractor.jsm b/dom/manifest/ValueExtractor.jsm index 04f311bc5130..eca02e966ec6 100644 --- a/dom/manifest/ValueExtractor.jsm +++ b/dom/manifest/ValueExtractor.jsm @@ -28,7 +28,16 @@ ValueExtractor.prototype = { // objectName: string used to construct the developer warning. // property: the name of the property being extracted. // trim: boolean, if the value should be trimmed (used by string type). - extractValue({ expectedType, object, objectName, property, trim }) { + // throwTypeError: boolean, throw a TypeError if the type is incorrect. + extractValue(options) { + const { + expectedType, + object, + objectName, + property, + throwTypeError, + trim, + } = options; const value = object[property]; const isArray = Array.isArray(value); // We need to special-case "array", as it's not a JS primitive. @@ -40,6 +49,9 @@ ValueExtractor.prototype = { [objectName, property, expectedType] ); this.errors.push({ warn }); + if (throwTypeError) { + throw new TypeError(warn); + } } return undefined; } diff --git a/dom/manifest/test/mochitest.ini b/dom/manifest/test/mochitest.ini index b7e389c43458..eb6029cc6190 100644 --- a/dom/manifest/test/mochitest.ini +++ b/dom/manifest/test/mochitest.ini @@ -5,6 +5,7 @@ support-files = manifestLoader.html file_reg_appinstalled_event.html file_testserver.sjs +[test_ImageObjectProcessor_purpose.html] [test_ImageObjectProcessor_sizes.html] [test_ImageObjectProcessor_src.html] [test_ImageObjectProcessor_type.html] diff --git a/dom/manifest/test/test_ImageObjectProcessor_purpose.html b/dom/manifest/test/test_ImageObjectProcessor_purpose.html new file mode 100644 index 000000000000..b7092461b995 --- /dev/null +++ b/dom/manifest/test/test_ImageObjectProcessor_purpose.html @@ -0,0 +1,100 @@ + + + + + + Test for Bug + + + + + diff --git a/dom/manifest/test/test_ManifestProcessor_warnings.html b/dom/manifest/test/test_ManifestProcessor_warnings.html index da6943237ec1..04e0c2eddbd2 100644 --- a/dom/manifest/test/test_ManifestProcessor_warnings.html +++ b/dom/manifest/test/test_ManifestProcessor_warnings.html @@ -61,18 +61,68 @@ const options = {...data, checkConformance: true } ; { func: () => options.jsonText = JSON.stringify({ icons: [ - { "src": "http://exmaple.com", "sizes": "48x48"}, + { "src": "http://example.com", "sizes": "48x48"}, { "src": "http://:Invalid", "sizes": "48x48"}, ], }), warn: "icons item at index 1 is invalid. The src member is an invalid URL http://:Invalid", }, + // testing dom.properties: ManifestImageUnusable + { + func() { + return (options.jsonText = JSON.stringify({ + icons: [ + { src: "http://example.com", purpose: "any" }, // valid + { src: "http://example.com", purpose: "banana" }, // generates error + ], + })); + }, + get warn() { + // Returns 2 warnings... array here is just to keep them organized + return [ + "icons item at index 1 includes unsupported purpose(s): banana.", + "icons item at index 1 lacks a usable purpose. It will be ignored.", + ].join(" "); + }, + }, + // testing dom.properties: ManifestImageUnsupportedPurposes + { + func() { + return (options.jsonText = JSON.stringify({ + icons: [ + { src: "http://example.com", purpose: "any" }, // valid + { src: "http://example.com", purpose: "any foo bar baz bar bar baz" }, // generates error + ], + })); + }, + warn: "icons item at index 1 includes unsupported purpose(s): foo bar baz.", + }, + // testing dom.properties: ManifestImageRepeatedPurposes + { + func() { + return (options.jsonText = JSON.stringify({ + icons: [ + { src: "http://example.com", purpose: "any" }, // valid + { + src: "http://example.com", + purpose: "any maskable any maskable maskable", // generates error + }, + ], + })); + }, + warn: "icons item at index 1 includes repeated purpose(s): any maskable.", + }, ].forEach((test, index) => { test.func(); const result = processor.process(options); - const [message] = result.moz_validation; - is(message.warn, test.warn, "Check warning."); - options.manifestURL = manifestURL; + let messages = []; + // Poking directly at "warn" triggers xray security wrapper. + for (const validationError of result.moz_validation) { + const { warn } = validationError; + messages.push(warn); + } + is(messages.join(" "), test.warn, "Check warning."); + options.manifestURL = manifestURL; options.docURL = docURL; });