parse URLs in Web App Manifest relative to manifest itself

GoogleChrome · Aug 24, 2016 · f45ae69 · f45ae69
1 parent 35eb5a1
commit f45ae69
Show file tree

Hide file tree

Showing 16 changed files with 505 additions and 126 deletions.
diff --git a/lighthouse-core/audits/cache-start-url.js b/lighthouse-core/audits/cache-start-url.js
@@ -17,7 +17,6 @@
 
 'use strict';
 
-const url = require('url');
 const Audit = require('./audit');
 
 class CacheStartUrl extends Audit {
@@ -41,7 +40,6 @@ class CacheStartUrl extends Audit {
     let cacheHasStartUrl = false;
     const manifest = artifacts.Manifest && artifacts.Manifest.value;
     const cacheContents = artifacts.CacheContents;
-    const baseURL = artifacts.URL;
 
     if (!(manifest && manifest.start_url && manifest.start_url.value)) {
       return CacheStartUrl.generateAuditResult({
@@ -58,7 +56,7 @@ class CacheStartUrl extends Audit {
     }
 
     // Remove any UTM strings.
-    const startURL = url.resolve(baseURL, manifest.start_url.value).toString();
+    const startURL = manifest.start_url.value;
     const altStartURL = startURL
         .replace(/\?utm_([^=]*)=([^&]|$)*/, '')
         .replace(/\?$/, '');

diff --git a/lighthouse-core/gather/gatherers/manifest.js b/lighthouse-core/gather/gatherers/manifest.js
@@ -54,7 +54,7 @@ class Manifest extends Gatherer {
           return;
         }
 
-        this.artifact = manifestParser(response.data);
+        this.artifact = manifestParser(response.data, response.url, options.url);
       }, _ => {
         this.artifact = Manifest._errorManifest('Unable to retrieve manifest');
         return;

diff --git a/lighthouse-core/lib/manifest-parser.js b/lighthouse-core/lib/manifest-parser.js
@@ -16,6 +16,7 @@
  */
 'use strict';
 
+const url = require('url');
 const validateColor = require('./web-inspector').Color.parse;
 
 const ALLOWED_DISPLAY_VALUES = [
@@ -61,13 +62,6 @@ function parseString(raw, trim) {
   };
 }
 
-function parseURL(raw) {
-  // TODO: resolve url using baseURL
-  // var baseURL = args.baseURL;
-  // new URL(parseString(raw).value, baseURL);
-  return parseString(raw, true);
-}
-
 function parseColor(raw) {
   const color = parseString(raw);
 
@@ -94,10 +88,69 @@ function parseShortName(jsonInput) {
   return parseString(jsonInput.short_name, true);
 }
 
-function parseStartUrl(jsonInput) {
-  // TODO: parse url using manifest_url as a base (missing).
-  // start_url must be same-origin as Document of the top-level browsing context.
-  return parseURL(jsonInput.start_url);
+/**
+ * Returns whether the urls are of the same origin. See https://html.spec.whatwg.org/#same-origin
+ * @param {string} url1
+ * @param {string} url2
+ * @return {boolean}
+ */
+function checkSameOrigin(url1, url2) {
+  const parsed1 = url.parse(url1);
+  const parsed2 = url.parse(url2);
+
+  return parsed1.protocol === parsed2.protocol &&
+      parsed1.hostname === parsed2.hostname &&
+      parsed1.port === parsed2.port;
+}
+
+/**
+ * https://w3c.github.io/manifest/#start_url-member
+ */
+function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
+  const raw = jsonInput.start_url;
+
+  // 8.10(3) - discard the empty string and non-strings.
+  if (raw === '') {
+    return {
+      raw,
+      value: documentUrl,
+      debugString: 'ERROR: start_url string empty'
+    };
+  }
+  const parsedAsString = parseString(raw);
+  if (!parsedAsString.value) {
+    parsedAsString.value = documentUrl;
+    return parsedAsString;
+  }
+
+  // 8.10(4) - construct URL with raw as input and manifestUrl as the base.
+  let startUrl;
+  try {
+    // TODO(bckenny): need better URL constructor to do this properly. See
+    // https://github.com/GoogleChrome/lighthouse/issues/602
+    startUrl = url.resolve(manifestUrl, raw);
+  } catch (e) {
+    // 8.10(5) - discard invalid URLs.
+    return {
+      raw,
+      value: documentUrl,
+      debugString: 'ERROR: invalid start_url relative to ${manifestUrl}'
+    };
+  }
+
+  // 8.10(6) - discard start_urls that are not same origin as documentUrl.
+  if (!checkSameOrigin(startUrl, documentUrl)) {
+    return {
+      raw,
+      value: documentUrl,
+      debugString: 'ERROR: start_url must be same-origin as document'
+    };
+  }
+
+  return {
+    raw,
+    value: startUrl
+  };
 }
 
 function parseDisplay(jsonInput) {
@@ -130,9 +183,20 @@ function parseOrientation(jsonInput) {
   return orientation;
 }
 
-function parseIcon(raw) {
-  // TODO: pass manifest url as base.
-  let src = parseURL(raw.src);
+function parseIcon(raw, manifestUrl) {
+  // 9.4(3)
+  const src = parseString(raw.src, true);
+  // 9.4(4) - discard if trimmed value is the empty string.
+  if (src.value === '') {
+    src.value = undefined;
+  }
+  if (src.value) {
+    // TODO(bckenny): need better URL constructor to do this properly. See
+    // https://github.com/GoogleChrome/lighthouse/issues/602
+    // 9.4(4) - construct URL with manifest URL as the base
+    src.value = url.resolve(manifestUrl, src.value);
+  }
+
   let type = parseString(raw.type, true);
 
   let density = {
@@ -167,7 +231,7 @@ function parseIcon(raw) {
   };
 }
 
-function parseIcons(jsonInput) {
+function parseIcons(jsonInput, manifestUrl) {
   const raw = jsonInput.icons;
   let value;
 
@@ -187,8 +251,15 @@ function parseIcons(jsonInput) {
     };
   }
 
-  // TODO(bckenny): spec says to skip icons missing `src`. Warn instead?
-  value = raw.filter(icon => !!icon.src).map(parseIcon);
+  // TODO(bckenny): spec says to skip icons missing `src`, so debug messages on
+  // individual icons are lost. Warn instead?
+  value = raw
+    // 9.6(3)(1)
+    .filter(icon => icon.src !== undefined)
+    // 9.6(3)(2)(1)
+    .map(icon => parseIcon(icon, manifestUrl))
+    // 9.6(3)(2)(2)
+    .filter(parsedIcon => parsedIcon.value.src.value !== undefined);
 
   return {
     raw,
@@ -200,15 +271,27 @@ function parseIcons(jsonInput) {
 function parseApplication(raw) {
   let platform = parseString(raw.platform, true);
   let id = parseString(raw.id, true);
-  // TODO: pass manfiest url as base.
-  let url = parseURL(raw.url);
+
+  // 10.2.(2) and 10.2.(3)
+  const appUrl = parseString(raw.url, true);
+  if (appUrl.value) {
+    try {
+      // TODO(bckenny): need better URL constructor to do this properly. See
+      // https://github.com/GoogleChrome/lighthouse/issues/602
+      // 10.2.(4) - attempt to construct URL.
+      appUrl.value = url.parse(appUrl.value).href;
+    } catch (e) {
+      appUrl.value = undefined;
+      appUrl.debugString = 'ERROR: invalid application URL ${raw.url}';
+    }
+  }
 
   return {
     raw,
     value: {
       platform,
       id,
-      url
+      url: appUrl
     },
     debugString: undefined
   };
@@ -234,8 +317,12 @@ function parseRelatedApplications(jsonInput) {
     };
   }
 
-  // TODO(bckenny): spec says to skip apps missing `platform`. Warn instead?
-  value = raw.filter(application => !!application.platform).map(parseApplication);
+  // TODO(bckenny): spec says to skip apps missing `platform`, so debug messages
+  // on individual apps are lost. Warn instead?
+  value = raw
+    .filter(application => !!application.platform)
+    .map(parseApplication)
+    .filter(parsedApp => !!parsedApp.value.id.value || !!parsedApp.value.url.value);
 
   return {
     raw,
@@ -273,7 +360,18 @@ function parseBackgroundColor(jsonInput) {
   return parseColor(jsonInput.background_color);
 }
 
-function parse(string) {
+/**
+ * Parse a manifest from the given inputs.
+ * @param {string} string Manifest JSON string.
+ * @param {string} manifestUrl URL of manifest file.
+ * @param {string} documentUrl URL of document containing manifest link element.
+ * @return {!ManifestNode<(!Manifest|undefined)>}
+ */
+function parse(string, manifestUrl, documentUrl) {
+  if (manifestUrl === undefined || documentUrl === undefined) {
+    throw new Error('Manifest and document URLs required for manifest parsing.');
+  }
+
   let jsonInput;
 
   try {
@@ -290,10 +388,10 @@ function parse(string) {
   let manifest = {
     name: parseName(jsonInput),
     short_name: parseShortName(jsonInput),
-    start_url: parseStartUrl(jsonInput),
+    start_url: parseStartUrl(jsonInput, manifestUrl, documentUrl),
     display: parseDisplay(jsonInput),
     orientation: parseOrientation(jsonInput),
-    icons: parseIcons(jsonInput),
+    icons: parseIcons(jsonInput, manifestUrl),
     related_applications: parseRelatedApplications(jsonInput),
     prefer_related_applications: parsePreferRelatedApplications(jsonInput),
     theme_color: parseThemeColor(jsonInput),

diff --git a/lighthouse-core/test/audits/background-color-test.js b/lighthouse-core/test/audits/background-color-test.js
@@ -17,7 +17,20 @@ const Audit = require('../../audits/manifest-background-color.js');
 const assert = require('assert');
 const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
 const manifestParser = require('../../lib/manifest-parser');
-const exampleManifest = manifestParser(manifestSrc);
+const exampleManifest = manifestParser(manifestSrc, 'https://example.com/', 'https://example.com/');
+
+const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json';
+const EXAMPLE_DOC_URL = 'https://example.com/index.html';
+
+/**
+ * Simple manifest parsing helper when the manifest URLs aren't material to the
+ * test. Uses example.com URLs for testing.
+ * @param {string} manifestSrc
+ * @return {!ManifestNode<(!Manifest|undefined)>}
+ */
+function noUrlManifestParser(manifestSrc) {
+  return manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
+}
 
 /* global describe, it*/
 
@@ -32,14 +45,14 @@ describe('Manifest: background color audit', () => {
 
   it('fails when an empty manifest is present', () => {
     const artifacts = {
-      Manifest: manifestParser('{}')
+      Manifest: noUrlManifestParser('{}')
     };
     return assert.equal(Audit.audit(artifacts).rawValue, false);
   });
 
   it('fails when a minimal manifest contains no background_color', () => {
     const artifacts = {
-      Manifest: manifestParser(JSON.stringify({
+      Manifest: noUrlManifestParser(JSON.stringify({
         start_url: '/'
       }))
     };
@@ -50,7 +63,7 @@ describe('Manifest: background color audit', () => {
 
   it('fails when a minimal manifest contains an invalid background_color', () => {
     const artifacts = {
-      Manifest: manifestParser(JSON.stringify({
+      Manifest: noUrlManifestParser(JSON.stringify({
         background_color: 'no'
       }))
     };
@@ -61,7 +74,7 @@ describe('Manifest: background color audit', () => {
 
   it('succeeds when a minimal manifest contains a valid background_color', () => {
     const artifacts = {
-      Manifest: manifestParser(JSON.stringify({
+      Manifest: noUrlManifestParser(JSON.stringify({
         background_color: '#FAFAFA'
       }))
     };

diff --git a/lighthouse-core/test/audits/cache-start-url-test.js b/lighthouse-core/test/audits/cache-start-url-test.js
@@ -17,10 +17,10 @@ const Audit = require('../../audits/cache-start-url.js');
 const assert = require('assert');
 const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
 const manifestParser = require('../../lib/manifest-parser');
-const exampleManifest = manifestParser(manifestSrc);
 const CacheContents = ['https://another.example.com/', 'https://example.com/'];
 const URL = 'https://example.com';
 const AltURL = 'https://example.com/?utm_source=http203';
+const exampleManifest = manifestParser(manifestSrc, URL, URL);
 
 /* global describe, it*/
 
@@ -43,18 +43,6 @@ describe('Cache: start_url audit', () => {
     assert.equal(output.debugString, 'No cache or URL detected');
   });
 
-  // Need to disable camelcase check for dealing with start_url.
-  /* eslint-disable camelcase */
-  it('fails when a manifest contains no start_url', () => {
-    const artifacts = {
-      Manifest: manifestParser('{}')
-    };
-    const output = Audit.audit(artifacts);
-    assert.equal(output.rawValue, false);
-    assert.equal(output.debugString, 'start_url not present in Manifest');
-  });
-  /* eslint-enable camelcase */
-
   it('succeeds when given a manifest with a start_url, cache contents, and a URL', () => {
     return assert.equal(Audit.audit({
       Manifest: exampleManifest,

diff --git a/lighthouse-core/test/audits/display-test.js b/lighthouse-core/test/audits/display-test.js
@@ -18,7 +18,9 @@ const manifestParser = require('../../lib/manifest-parser');
 const assert = require('assert');
 
 const manifestSrc = JSON.stringify(require('../fixtures/manifest.json'));
-const exampleManifest = manifestParser(manifestSrc);
+const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json';
+const EXAMPLE_DOC_URL = 'https://example.com/index.html';
+const exampleManifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
 
 /* global describe, it*/
 
@@ -38,7 +40,7 @@ describe('Mobile-friendly: display audit', () => {
 
   it('falls back to the successful default when there is no manifest display property', () => {
     const artifacts = {
-      Manifest: manifestParser('{}')
+      Manifest: manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL)
     };
     const output = Audit.audit(artifacts);
 
@@ -51,7 +53,7 @@ describe('Mobile-friendly: display audit', () => {
     const artifacts = {
       Manifest: manifestParser(JSON.stringify({
         display: 'standalone'
-      }))
+      }), EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL)
     };
     const output = Audit.audit(artifacts);
     assert.equal(output.score, true);