core: INSECURE_DOCUMENT_REQUEST errors still return lhr

lighthouse-core/gather/driver.js

@@ -81,13 +81,6 @@ class Driver {
       this._eventEmitter.emit(event.method, event.params);
     });
 
-    /**
-     * Used for monitoring network status events during gotoURL.
-     * @type {?LH.Crdp.Security.SecurityStateChangedEvent}
-     * @private
-     */
-    this._lastSecurityState = null;
-
     /**
      * @type {number}
      * @private
@@ -512,6 +505,35 @@ class Driver {
     });
   }
 
+  /**
+   * Returns LHError if the security state is insecure.
+   * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState
+   * @returns {LHError|undefined}
+   */
+  checkForSecurityIssues({securityState, explanations}) {
+    if (securityState === 'insecure') {
+      const insecureDescriptions = explanations
+        .filter(exp => exp.securityState === 'insecure')
+        .map(exp => exp.description);
+      return new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {
+        securityMessages: insecureDescriptions.join(' '),
+      });
+    }
+  }
+
+  waitForSecurityIssuesCheck() {
+    return new Promise((resolve, reject) => {
+      this.once('Security.securityStateChanged', state => {
+        const err = this.checkForSecurityIssues(state);
+        if (err) {
+          reject(err);
+        } else {
+          resolve();
+        }
+      });
+    });
+  }
+
   /**
    * Returns a promise that resolve when a frame has been navigated.
    * Used for detecting that our about:blank reset has been completed.
@@ -895,6 +917,13 @@ class Driver {
     // No timeout needed for Page.navigate. See #6413.
     const waitforPageNavigateCmd = this._innerSendCommand('Page.navigate', {url});
 
+    try {
+      await this.sendCommand('Security.enable');
+      await this.waitForSecurityIssuesCheck();
+    } finally {
+      this.sendCommand('Security.disable');
+    }
+
     if (waitForNavigated) {
       await this._waitForFrameNavigated();
     } else if (waitForLoad) {
@@ -959,26 +988,6 @@ class Driver {
     return result.body;
   }
 
-  async listenForSecurityStateChanges() {
-    this.on('Security.securityStateChanged', state => {
-      this._lastSecurityState = state;
-    });
-    await this.sendCommand('Security.enable');
-  }
-
-  /**
-   * @return {LH.Crdp.Security.SecurityStateChangedEvent}
-   */
-  getSecurityState() {
-    if (!this._lastSecurityState) {
-      // happens if 'listenForSecurityStateChanges' is not called,
-      // or if some assumptions about the Security domain are wrong
-      throw new Error('Expected a security state.');
-    }
-
-    return this._lastSecurityState;
-  }
-
   /**
    * @param {string} name The name of API whose permission you wish to query
    * @return {Promise<string>} The state of permissions, resolved in a promise.

lighthouse-core/gather/gather-runner.js

@@ -109,7 +109,6 @@ class GatherRunner {
     await driver.cacheNatives();
     await driver.registerPerformanceObserver();
     await driver.dismissJavaScriptDialogs();
-    await driver.listenForSecurityStateChanges();
     if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl);
     log.timeEnd(status);
   }
@@ -172,23 +171,6 @@ class GatherRunner {
     }
   }
 
-  /**
-   * Throws an error if the security state is insecure.
-   * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState
-   * @throws {LHError}
-   */
-  static assertNoSecurityIssues({securityState, explanations}) {
-    if (securityState === 'insecure') {
-      const insecureDescriptions = explanations
-        .filter(exp => exp.securityState === 'insecure')
-        .map(exp => exp.description);
-      throw new LHError(
-        LHError.errors.INSECURE_DOCUMENT_REQUEST,
-        {securityMessages: insecureDescriptions.join(' ')}
-      );
-    }
-  }
-
   /**
    * Calls beforePass() on gatherers before tracing
    * has started and before navigation to the target page.
@@ -256,8 +238,11 @@ class GatherRunner {
     if (recordTrace) await driver.beginTrace(settings);
 
     // Navigate.
-    await GatherRunner.loadPage(driver, passContext);
-    log.timeEnd(status);
+    try {
+      await GatherRunner.loadPage(driver, passContext);
+    } finally {
+      log.timeEnd(status);
+    }
 
     const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`};
     log.time(pStatus, 'verbose');
@@ -311,8 +296,6 @@ class GatherRunner {
     const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
     log.timeEnd(status);
 
-    this.assertNoSecurityIssues(driver.getSecurityState());
-
     let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords);
     // If the driver was offline, a page load error is expected, so do not save it.
     if (!driver.online) pageLoadError = undefined;

lighthouse-core/lib/file-namer.js

@@ -16,11 +16,11 @@
  * Generate a filenamePrefix of hostname_YYYY-MM-DD_HH-MM-SS
  * Date/time uses the local timezone, however Node has unreliable ICU
  * support, so we must construct a YYYY-MM-DD date format manually. :/
- * @param {{finalUrl: string, fetchTime: string}} lhr
+ * @param {{requestedUrl: string, finalUrl: string, fetchTime: string}} lhr
  * @return {string}
  */
 function getFilenamePrefix(lhr) {
-  const hostname = new (getUrlConstructor())(lhr.finalUrl).hostname;
+  const hostname = new (getUrlConstructor())(lhr.finalUrl || lhr.requestedUrl).hostname;
   const date = (lhr.fetchTime && new Date(lhr.fetchTime)) || new Date();
 
   const timeStr = date.toLocaleTimeString('en-US', {hour12: false});

lighthouse-core/lib/i18n/en-US.json

@@ -952,7 +952,7 @@
     "description": "Error message explaining that Lighthouse couldn't complete because the page has stopped responding to its instructions."
   },
   "lighthouse-core/lib/lh-error.js | pageLoadFailedInsecure": {
-    "message": "The URL you have provided does not have valid security credentials. ({securityMessages})",
+    "message": "The URL you have provided does not have valid security credentials. {securityMessages}",
     "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed."
   },
   "lighthouse-core/lib/lh-error.js | pageLoadFailedWithDetails": {

lighthouse-core/lib/lh-error.js

@@ -22,7 +22,7 @@ const UIStrings = {
   /** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */
   pageLoadFailedWithDetails: 'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: {errorDetails})',
   /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */
-  pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. ({securityMessages})',
+  pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}',
   /** Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted. */
   internalChromeError: 'An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.',
   /** Error message explaining that fetching the resources of the webpage has taken longer than the maximum time. */

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -461,6 +461,7 @@ class ReportUIFeatures {
    */
   _saveFile(blob) {
     const filename = getFilenamePrefix({
+      requestedUrl: this.json.requestedUrl,
       finalUrl: this.json.finalUrl,
       fetchTime: this.json.fetchTime,
     });

lighthouse-core/test/gather/driver-test.js

@@ -221,6 +221,7 @@ describe('Browser Driver', () => {
     }
     const replayConnection = new ReplayConnection();
     const driver = new Driver(replayConnection);
+    driver.waitForSecurityIssuesCheck = () => Promise.resolve();
 
     // Redirect in log will go through
     const startUrl = 'http://en.wikipedia.org/';
@@ -535,4 +536,39 @@ describe('Multiple tab check', () => {
       });
     });
   });
+
+  describe('.checkForSecurityIssues', () => {
+    it('returns nothing when page is secure', () => {
+      const secureSecurityState = {
+        securityState: 'secure',
+      };
+      const err = driverStub.checkForSecurityIssues(secureSecurityState);
+      assert.equal(err, undefined);
+    });
+
+    it('returns an error when page is insecure', () => {
+      const insecureSecurityState = {
+        explanations: [
+          {
+            description: 'reason 1.',
+            securityState: 'insecure',
+          },
+          {
+            description: 'blah.',
+            securityState: 'info',
+          },
+          {
+            description: 'reason 2.',
+            securityState: 'insecure',
+          },
+        ],
+        securityState: 'insecure',
+      };
+      const err = driverStub.checkForSecurityIssues(insecureSecurityState);
+      assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST');
+      assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST');
+      /* eslint-disable-next-line max-len */
+      expect(err.friendlyMessage).toBeDisplayString('The URL you have provided does not have valid security credentials. reason 1. reason 2.');
+    });
+  });
 });

lighthouse-core/test/gather/fake-driver.js

@@ -81,14 +81,9 @@ const fakeDriver = {
   setExtraHTTPHeaders() {
     return Promise.resolve();
   },
-  listenForSecurityStateChanges() {
+  waitForSecurityIssuesCheck() {
     return Promise.resolve();
   },
-  getSecurityState() {
-    return Promise.resolve({
-      securityState: 'secure',
-    });
-  },
 };
 
 module.exports = fakeDriver;

lighthouse-core/test/gather/gather-runner-test.js

@@ -322,7 +322,6 @@ describe('GatherRunner', function() {
       clearDataForOrigin: createCheck('calledClearStorage'),
       blockUrlPatterns: asyncFunc,
       setExtraHTTPHeaders: asyncFunc,
-      listenForSecurityStateChanges: asyncFunc,
     };
 
     return GatherRunner.setupDriver(driver, {settings: {}}).then(_ => {
@@ -382,7 +381,6 @@ describe('GatherRunner', function() {
       clearDataForOrigin: createCheck('calledClearStorage'),
       blockUrlPatterns: asyncFunc,
       setExtraHTTPHeaders: asyncFunc,
-      listenForSecurityStateChanges: asyncFunc,
     };
 
     return GatherRunner.setupDriver(driver, {
@@ -696,44 +694,6 @@ describe('GatherRunner', function() {
     });
   });
 
-  describe('#assertNoSecurityIssues', () => {
-    it('succeeds when page is secure', () => {
-      const secureSecurityState = {
-        securityState: 'secure',
-      };
-      GatherRunner.assertNoSecurityIssues(secureSecurityState);
-    });
-
-    it('fails when page is insecure', () => {
-      const insecureSecurityState = {
-        explanations: [
-          {
-            description: 'reason 1',
-            securityState: 'insecure',
-          },
-          {
-            description: 'blah.',
-            securityState: 'info',
-          },
-          {
-            description: 'reason 2',
-            securityState: 'insecure',
-          },
-        ],
-        securityState: 'insecure',
-      };
-      try {
-        GatherRunner.assertNoSecurityIssues(insecureSecurityState);
-        assert.fail('expected INSECURE_DOCUMENT_REQUEST LHError');
-      } catch (err) {
-        assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST');
-        assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST');
-        expect(err.friendlyMessage)
-          .toBeDisplayString(/The URL.*security credentials.*reason 1 reason 2/);
-      }
-    });
-  });
-
   describe('artifact collection', () => {
     // Make sure our gatherers never execute in parallel
     it('runs gatherer lifecycle methods strictly in sequence', async () => {

lighthouse-viewer/app/src/github-api.js

@@ -38,6 +38,7 @@ class GithubApi {
     return this._auth.getAccessToken()
       .then(accessToken => {
         const filename = getFilenamePrefix({
+          requestedUrl: jsonFile.requestedUrl,
           finalUrl: jsonFile.finalUrl,
           fetchTime: jsonFile.fetchTime,
         });