diff --git a/lighthouse-core/lib/manifest-parser.js b/lighthouse-core/lib/manifest-parser.js index 763910cfd4b0..64d4fd06b52d 100644 --- a/lighthouse-core/lib/manifest-parser.js +++ b/lighthouse-core/lib/manifest-parser.js @@ -16,7 +16,7 @@ const ALLOWED_DISPLAY_VALUES = [ ]; /** * All display-mode fallbacks, including when unset, lead to default display mode 'browser'. - * @see https://w3c.github.io/manifest/#dfn-default-display-mode + * @see https://www.w3.org/TR/2016/WD-appmanifest-20160825/#dfn-default-display-mode */ const DEFAULT_DISPLAY_MODE = 'browser'; @@ -111,7 +111,7 @@ function checkSameOrigin(url1, url2) { } /** - * https://w3c.github.io/manifest/#start_url-member + * https://www.w3.org/TR/2016/WD-appmanifest-20160825/#start_url-member * @param {*} jsonInput * @param {string} manifestUrl * @param {string} documentUrl @@ -151,7 +151,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) { return { raw, value: documentUrl, - warning: 'ERROR: invalid start_url relative to ${manifestUrl}', + warning: `ERROR: invalid start_url relative to ${manifestUrl}`, }; } @@ -218,6 +218,7 @@ function parseOrientation(jsonInput) { } /** + * @see https://www.w3.org/TR/2016/WD-appmanifest-20160825/#src-member * @param {*} raw * @param {string} manifestUrl */ @@ -229,8 +230,14 @@ function parseIcon(raw, manifestUrl) { src.value = undefined; } if (src.value) { - // 9.4(4) - construct URL with manifest URL as the base - src.value = new URL(src.value, manifestUrl).href; + try { + // 9.4(4) - construct URL with manifest URL as the base + src.value = new URL(src.value, manifestUrl).href; + } catch (_) { + // 9.4 "This algorithm will return a URL or undefined." + src.warning = `ERROR: invalid icon url will be ignored: '${raw.src}'`; + src.value = undefined; + } } const type = parseString(raw.type, true); @@ -301,20 +308,31 @@ function parseIcons(jsonInput, manifestUrl) { }; } - // TODO(bckenny): spec says to skip icons missing `src`, so debug messages on - // individual icons are lost. Warn instead? - const value = raw + const parsedIcons = raw // 9.6(3)(1) .filter(icon => icon.src !== undefined) // 9.6(3)(2)(1) - .map(icon => parseIcon(icon, manifestUrl)) + .map(icon => parseIcon(icon, manifestUrl)); + + // NOTE: we still lose the specific message on these icons, but it's not possible to surface them + // without a massive change to the structure and paradigms of `manifest-parser`. + const ignoredIconsWithWarnings = parsedIcons + .filter(icon => { + const possibleWarnings = [icon.warning, icon.value.type.warning, icon.value.src.warning, + icon.value.sizes.warning, icon.value.density.warning].filter(Boolean); + const hasSrc = !!icon.value.src.value; + return !!possibleWarnings.length && !hasSrc; + }); + + const value = parsedIcons // 9.6(3)(2)(2) .filter(parsedIcon => parsedIcon.value.src.value !== undefined); return { raw, value, - warning: undefined, + warning: ignoredIconsWithWarnings.length ? + 'WARNING: Some icons were ignored due to warnings.' : undefined, }; } @@ -333,7 +351,7 @@ function parseApplication(raw) { appUrl.value = new URL(appUrl.value).href; } catch (e) { appUrl.value = undefined; - appUrl.warning = 'ERROR: invalid application URL ${raw.url}'; + appUrl.warning = `ERROR: invalid application URL ${raw.url}`; } } @@ -460,10 +478,21 @@ function parse(string, manifestUrl, documentUrl) { }; /* eslint-enable camelcase */ + /** @type {string|undefined} */ + let manifestUrlWarning; + try { + const manifestUrlParsed = new URL(manifestUrl); + if (!manifestUrlParsed.protocol.startsWith('http')) { + manifestUrlWarning = `WARNING: manifest URL not available over a valid network protocol`; + } + } catch (_) { + manifestUrlWarning = `ERROR: invalid manifest URL: '${manifestUrl}'`; + } + return { raw: string, value: manifest, - warning: undefined, + warning: manifestUrlWarning, }; } diff --git a/lighthouse-core/test/lib/manifest-parser-test.js b/lighthouse-core/test/lib/manifest-parser-test.js index 9f4fa38e405b..9a93de8a1940 100644 --- a/lighthouse-core/test/lib/manifest-parser-test.js +++ b/lighthouse-core/test/lib/manifest-parser-test.js @@ -11,6 +11,7 @@ const manifestStub = require('../fixtures/manifest.json'); const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json'; const EXAMPLE_DOC_URL = 'https://example.com/index.html'; +const EXAMPLE_MANIFEST_BLOB_URL = 'blob:https://example.com/manifest.json'; /** * Simple manifest parsing helper when the manifest URLs aren't material to the @@ -27,32 +28,48 @@ function noUrlManifestParser(manifestSrc) { describe('Manifest Parser', function() { it('should not parse empty string input', function() { const parsedManifest = noUrlManifestParser(''); - assert.ok(parsedManifest.warning); + expect(parsedManifest.warning) + .toEqual('ERROR: file isn\'t valid JSON: SyntaxError: Unexpected end of JSON input'); }); it('accepts empty dictionary', function() { const parsedManifest = noUrlManifestParser('{}'); - assert(!parsedManifest.warning); - assert.equal(parsedManifest.value.name.value, undefined); - assert.equal(parsedManifest.value.short_name.value, undefined); - assert.equal(parsedManifest.value.start_url.value, EXAMPLE_DOC_URL); - assert.equal(parsedManifest.value.display.value, 'browser'); - assert.equal(parsedManifest.value.orientation.value, undefined); - assert.equal(parsedManifest.value.theme_color.value, undefined); - assert.equal(parsedManifest.value.background_color.value, undefined); - assert.ok(Array.isArray(parsedManifest.value.icons.value)); - assert.ok(parsedManifest.value.icons.value.length === 0); + expect(parsedManifest.warning).toBeUndefined(); + expect(parsedManifest.value.name.value).toBe(undefined); + expect(parsedManifest.value.short_name.value).toBe(undefined); + expect(parsedManifest.value.start_url.value).toBe(EXAMPLE_DOC_URL); + expect(parsedManifest.value.display.value).toBe('browser'); + expect(parsedManifest.value.orientation.value).toBe(undefined); + expect(parsedManifest.value.theme_color.value).toBe(undefined); + expect(parsedManifest.value.background_color.value).toBe(undefined); + expect(parsedManifest.value.icons.value).toHaveLength(0); // TODO: // related_applications // prefer_related_applications }); + it('should warn on invalid manifest parser URL', function() { + const parsedManifest = manifestParser('{}', 'not a URL', EXAMPLE_DOC_URL); + expect(parsedManifest.warning) + .toEqual('ERROR: invalid manifest URL: \'not a URL\''); + }); + + it('should warn on valid but non-(HTTP|HTTPS) manifest parser URL', function() { + const parsedManifest = manifestParser('{}', EXAMPLE_MANIFEST_BLOB_URL, EXAMPLE_DOC_URL); + expect(parsedManifest.warning) + .toEqual('WARNING: manifest URL not available over a valid network protocol'); + }); + describe('icon parsing', function() { // 9.7 it('gives an empty array and an error for erroneous icons entry', () => { - const parsedManifest = manifestParser('{"icons": {"16": "img/icons/icon16.png"}}', - EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert.ok(!parsedManifest.warning); + const parsedManifest = manifestParser( + '{"icons": {"16": "img/icons/icon16.png"}}', + EXAMPLE_MANIFEST_URL, + EXAMPLE_DOC_URL + ); + + expect(parsedManifest.warning).toBeUndefined(); const icons = parsedManifest.value.icons; assert.ok(Array.isArray(icons.value)); assert.equal(icons.value.length, 0); @@ -60,9 +77,8 @@ describe('Manifest Parser', function() { }); it('gives an empty array and no error for missing icons entry', () => { - const parsedManifest = manifestParser('{}', - EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert.ok(!parsedManifest.warning); + const parsedManifest = manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); + expect(parsedManifest.warning).toBeUndefined(); const icons = parsedManifest.value.icons; assert.ok(Array.isArray(icons.value)); assert.equal(icons.value.length, 0); @@ -70,9 +86,12 @@ describe('Manifest Parser', function() { }); it('parses basic string', function() { - const parsedManifest = manifestParser('{"icons": [{"src": "192.png", "sizes": "192x192"}]}', - EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL); - assert(!parsedManifest.warning); + const parsedManifest = manifestParser( + '{"icons": [{"src": "192.png", "sizes": "192x192"}]}', + EXAMPLE_MANIFEST_URL, + EXAMPLE_DOC_URL + ); + expect(parsedManifest.warning).toBeUndefined(); const icons = parsedManifest.value.icons; assert(!icons.warning); const icon192 = icons.value[0]; @@ -81,9 +100,12 @@ describe('Manifest Parser', function() { }); it('finds four icons in the stub manifest', function() { - const parsedManifest = manifestParser(JSON.stringify(manifestStub), EXAMPLE_MANIFEST_URL, - EXAMPLE_DOC_URL); - assert(!parsedManifest.warning); + const parsedManifest = manifestParser( + JSON.stringify(manifestStub), + EXAMPLE_MANIFEST_URL, + EXAMPLE_DOC_URL + ); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.icons.value.length, 4); }); @@ -110,6 +132,19 @@ describe('Manifest Parser', function() { assert.equal(icons.value.length, 0); }); + it('parses icons and discards any with invalid base URL values', () => { + const manifestSrc = JSON.stringify({ + icons: [{ + src: '/valid/path', + }], + }); + const parsedManifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_BLOB_URL, + EXAMPLE_DOC_URL); + const icons = parsedManifest.value.icons; + expect(icons.value).toHaveLength(0); + expect(icons.warning).toEqual('WARNING: Some icons were ignored due to warnings.'); + }); + it('parses icons and discards any with undefined or empty string src values', () => { const manifestSrc = JSON.stringify({ icons: [{ @@ -139,23 +174,23 @@ describe('Manifest Parser', function() { describe('name parsing', function() { it('parses basic string', function() { const parsedManifest = noUrlManifestParser('{"name":"foo"}'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.name.value, 'foo'); }); it('trims whitespaces', function() { const parsedManifest = noUrlManifestParser('{"name":" foo "}'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.name.value, 'foo'); }); it('doesn\'t parse non-string', function() { let parsedManifest = noUrlManifestParser('{"name": {} }'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.name.value, undefined); parsedManifest = noUrlManifestParser('{"name": 42 }'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.name.value, undefined); }); }); @@ -163,23 +198,23 @@ describe('Manifest Parser', function() { describe('short_name parsing', function() { it('parses basic string', function() { const parsedManifest = noUrlManifestParser('{"short_name":"foo"}'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.short_name.value, 'foo'); }); it('trims whitespaces', function() { const parsedManifest = noUrlManifestParser('{"short_name":" foo "}'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.short_name.value, 'foo'); }); it('doesn\'t parse non-string', function() { let parsedManifest = noUrlManifestParser('{"short_name": {} }'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.short_name.value, undefined); parsedManifest = noUrlManifestParser('{"short_name": 42 }'); - assert(!parsedManifest.warning); + expect(parsedManifest.warning).toBeUndefined(); assert.equal(parsedManifest.value.short_name.value, undefined); }); });