Repository: GoogleChrome/lighthouse (js)

Issue:
Don't allow running LH against file:// URLs
We should block these entirely

Pull request:
cli(run): prevent file:// from running, return error
Per #5924, this prevents `file://` urls from running in LH. 
![image](https://user-images.githubusercontent.com/643503/44824656-ae853680-abbb-11e8-87d6-3b5e1bdd89f6.png)

A few things:

1. The regex doesn't actually look for `file`, rather it checks to verify if the URL is a valid form. To see what the regex matches, I put up https://regexr.com/3um70 for easy glance (which is pretty much just an extended version of Mathias' list from https://mathiasbynens.be/demo/url-regex)
2. The regex does make an exception for `chrome://` urls
3. I placed this in `run` as opposed to running checks in `cli-flags` mostly because it seemed the cleaner route. Re-uses `handleError` in this case (though the error message placement probably needs to move) 
4. I added a specific test that utilizes `jest.spyOn` and a mock for the `process.exit()` call that `handleError` makes when failing on `file:///` (though the console.error will still appear when running tests):
![image](https://user-images.githubusercontent.com/643503/44824168-0c644f00-abb9-11e8-901c-7fe619fe8e0e.png)

Resolves #5924.

Patch:
diff --git a/docs/recipes/custom-audit/package.json b/docs/recipes/custom-audit/package.json
index 5950d25c4..37d27dac0 100644
--- a/docs/recipes/custom-audit/package.json
+++ b/docs/recipes/custom-audit/package.json
@@ -3,6 +3,6 @@
   "private": true,
   "scripts": {},
   "devDependencies": {
-    "lighthouse": "^2.x"
+    "lighthouse": "^3.x"
   }
 }
diff --git a/lighthouse-core/index.js b/lighthouse-core/index.js
index 52c6b8392..da7b8b183 100644
--- a/lighthouse-core/index.js
+++ b/lighthouse-core/index.js
@@ -10,6 +10,9 @@ const log = require('lighthouse-logger');
 const ChromeProtocol = require('./gather/connections/cri.js');
 const Config = require('./config/config');
 
+const URL = require('./lib/url-shim.js');
+const LHError = require('./lib/lh-error.js');
+
 /** @typedef {import('./gather/connections/connection.js')} Connection */
 
 /*
@@ -36,6 +39,11 @@ const Config = require('./config/config');
  * @return {Promise<LH.RunnerResult|undefined>}
  */
 async function lighthouse(url, flags = {}, configJSON, connection) {
+  // verify the url is valid and that protocol is allowed
+  if (url && (!URL.isValid(url) || !URL.isProtocolAllowed(url))) {
+    throw new LHError(LHError.errors.INVALID_URL);
+  }
+
   // set logging preferences, assume quiet
   flags.logLevel = flags.logLevel || 'error';
   log.setLevel(flags.logLevel);
diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js
index 9409f0db7..1b623fcf0 100644
--- a/lighthouse-core/lib/lh-error.js
+++ b/lighthouse-core/lib/lh-error.js
@@ -118,6 +118,12 @@ const ERRORS = {
     code: 'REQUEST_CONTENT_TIMEOUT',
     message: strings.requestContentTimeout,
   },
+
+  // URL parsing failures
+  INVALID_URL: {
+    code: 'INVALID_URL',
+    message: strings.urlInvalid,
+  },
 };
 
 /** @type {Record<keyof typeof ERRORS, LighthouseErrorDefinition>} */
diff --git a/lighthouse-core/lib/strings.js b/lighthouse-core/lib/strings.js
index 614bfda60..d6f56f5a1 100644
--- a/lighthouse-core/lib/strings.js
+++ b/lighthouse-core/lib/strings.js
@@ -13,4 +13,5 @@ module.exports = {
   pageLoadFailed: `Your page failed to load. Verify that the URL is valid and re-run Lighthouse.`,
   internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`,
   requestContentTimeout: 'Fetching resource content has exceeded the allotted time',
+  urlInvalid: `The URL you have provided appears to be invalid.`,
 };
diff --git a/lighthouse-core/lib/url-shim.js b/lighthouse-core/lib/url-shim.js
index aaeb79ea7..8327efff3 100644
--- a/lighthouse-core/lib/url-shim.js
+++ b/lighthouse-core/lib/url-shim.js
@@ -23,6 +23,11 @@ const listOfTlds = [
   'com', 'co', 'gov', 'edu', 'ac', 'org', 'go', 'gob', 'or', 'net', 'in', 'ne', 'nic', 'gouv',
   'web', 'spb', 'blog', 'jus', 'kiev', 'mil', 'wi', 'qc', 'ca', 'bel', 'on',
 ];
+
+const allowedProtocols = [
+  'https:', 'http:', 'chrome:',
+];
+
 /**
  * There is fancy URL rewriting logic for the chrome://settings page that we need to work around.
  * Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages.
@@ -184,6 +189,20 @@ class URLShim extends URL {
       return false;
     }
   }
+
+  /**
+   * Determine if the url has a protocol that we're able to test
+   * @param {string} url
+   * @return {boolean}
+   */
+  static isProtocolAllowed(url) {
+    try {
+      const parsed = new URL(url);
+      return allowedProtocols.includes(parsed.protocol);
+    } catch (e) {
+      return false;
+    }
+  }
 }
 
 URLShim.URL = URL;
diff --git a/lighthouse-core/report/html/renderer/util.js b/lighthouse-core/report/html/renderer/util.js
index 856f4cc20..a11a6f6dc 100644
--- a/lighthouse-core/report/html/renderer/util.js
+++ b/lighthouse-core/report/html/renderer/util.js
@@ -142,10 +142,7 @@ class Util {
       case 'numeric':
       case 'binary':
       default:
-        // Numeric audits that are within PASS_THRESHOLD will still show up with failing.
-        // For opportunities, we want to have them show up with other failing for contrast.
-        // For diagnostics, we sort by score so they'll be lowest priority.
-        return Number(audit.score) === 1;
+        return Number(audit.score) >= RATINGS.PASS.minScore;
     }
   }
 
diff --git a/lighthouse-core/report/html/report-styles.css b/lighthouse-core/report/html/report-styles.css
index 0df1d2fca..7bba98095 100644
--- a/lighthouse-core/report/html/report-styles.css
+++ b/lighthouse-core/report/html/report-styles.css
@@ -150,12 +150,15 @@
   display: none !important;
 }
 
+.lh-root details > summary {
+  cursor: pointer;
+}
+
 .lh-audit-group a,
 .lh-category-header__description a {
   color: #0c50c7;
 }
 
-
 .lh-audit__description {
   --inner-audit-left-padding: calc(var(--text-indent) + var(--lh-audit-index-width) + 2 * var(--audit-item-gap));
   --inner-audit-right-padding: calc(var(--text-indent) + 2px);
@@ -299,7 +302,6 @@
 .lh-audit__header {
   display: flex;
   padding: var(--lh-audit-vpadding) var(--text-indent);
-  cursor: pointer;
 }
 
 .lh-audit--load-opportunity .lh-audit__header {


Test patch:
diff --git a/lighthouse-core/test/index-test.js b/lighthouse-core/test/index-test.js
index faccc5471..99190946f 100644
--- a/lighthouse-core/test/index-test.js
+++ b/lighthouse-core/test/index-test.js
@@ -52,7 +52,7 @@ describe('Module Tests', function() {
   });
 
   it('should throw an error when the second parameter is not an object', function() {
-    return lighthouse('SOME_URL', 'flags')
+    return lighthouse('chrome://version', 'flags')
       .then(() => {
         throw new Error('Should not have resolved when second arg is not an object');
       }, err => {
@@ -61,7 +61,7 @@ describe('Module Tests', function() {
   });
 
   it('should throw an error when the config is invalid', function() {
-    return lighthouse('SOME_URL', {}, {})
+    return lighthouse('chrome://version', {}, {})
       .then(() => {
         throw new Error('Should not have resolved when second arg is not an object');
       }, err => {
@@ -70,7 +70,7 @@ describe('Module Tests', function() {
   });
 
   it('should throw an error when the config contains incorrect audits', function() {
-    return lighthouse('SOME_URL', {}, {
+    return lighthouse('chrome://version', {}, {
       passes: [{
         gatherers: [
           'viewport',
@@ -87,6 +87,24 @@ describe('Module Tests', function() {
       });
   });
 
+  it('should throw an error when the url is invalid', function() {
+    return lighthouse('https:/i-am-not-valid', {}, {})
+      .then(() => {
+        throw new Error('Should not have resolved when url is invalid');
+      }, err => {
+        assert.ok(err);
+      });
+  });
+
+  it('should throw an error when the url is invalid protocol (file:///)', function() {
+    return lighthouse('file:///a/fake/index.html', {}, {})
+      .then(() => {
+        throw new Error('Should not have resolved when url is file:///');
+      }, err => {
+        assert.ok(err);
+      });
+  });
+
   it('should return formatted LHR when given no categories', function() {
     const exampleUrl = 'https://www.reddit.com/r/nba';
     return lighthouse(exampleUrl, {
diff --git a/lighthouse-core/test/lib/url-shim-test.js b/lighthouse-core/test/lib/url-shim-test.js
index 9ba2f0a1d..236127de7 100644
--- a/lighthouse-core/test/lib/url-shim-test.js
+++ b/lighthouse-core/test/lib/url-shim-test.js
@@ -36,6 +36,18 @@ describe('URL Shim', () => {
     assert.equal(URL.isValid('eval(<context>):45:16'), false);
   });
 
+  it('safely identifies allowed URL protocols', () => {
+    assert.ok(URL.isProtocolAllowed('http://google.com/'));
+    assert.ok(URL.isProtocolAllowed('https://google.com/'));
+    assert.ok(URL.isProtocolAllowed('chrome://version'));
+  });
+
+  it('safely identifies disallowed URL protocols', () => {
+    assert.equal(URL.isProtocolAllowed('file:///i/am/a/fake/file.html'), false);
+    assert.equal(URL.isProtocolAllowed('ftp://user:password@private.ftp.example.com/index.html'), false);
+    assert.equal(URL.isProtocolAllowed('gopher://underground:9090/path'), false);
+  });
+
   it('safely identifies same hosts', () => {
     const urlA = 'https://5321212.fls.net/page?query=string#hash';
     const urlB = 'http://5321212.fls.net/deeply/nested/page';
diff --git a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
index 8650cbf77..15781d576 100644
--- a/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/performance-category-renderer-test.js
@@ -140,7 +140,7 @@ describe('PerfCategoryRenderer', () => {
     const diagnosticSection = categoryDOM.querySelectorAll('.lh-category > .lh-audit-group')[2];
 
     const diagnosticAudits = category.auditRefs.filter(audit => audit.group === 'diagnostics' &&
-        audit.result.score !== 1 && audit.result.scoreDisplayMode !== 'not-applicable');
+        !Util.showAsPassed(audit.result));
     const diagnosticElements = diagnosticSection.querySelectorAll('.lh-audit');
     assert.equal(diagnosticElements.length, diagnosticAudits.length);
   });
@@ -150,8 +150,7 @@ describe('PerfCategoryRenderer', () => {
     const passedSection = categoryDOM.querySelector('.lh-category > .lh-passed-audits');
 
     const passedAudits = category.auditRefs.filter(audit =>
-        audit.group && audit.group !== 'metrics' &&
-        (audit.result.score === 1 || audit.result.scoreDisplayMode === 'not-applicable'));
+        audit.group && audit.group !== 'metrics' && Util.showAsPassed(audit.result));
     const passedElements = passedSection.querySelectorAll('.lh-audit');
     assert.equal(passedElements.length, passedAudits.length);
   });