INSECURE_DOCUMENT_REQUEST errors still return lhr (#6595)

lighthouse-core/gather/driver.js

@@ -497,6 +497,35 @@ class Driver {
     });
   }
 
+  /**
+   * Rejects if the security state is insecure.
+   * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState
+   * @returns {LHError|undefined}
+   */
+  checkForSecurityIssues({securityState, explanations}) {
+    if (securityState === 'insecure') {
+      const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST};
+      const insecureDescriptions = explanations
+        .filter(exp => exp.securityState === 'insecure')
+        .map(exp => exp.description);
+      errorDef.message += ` ${insecureDescriptions.join(' ')}`;
+      return new LHError(errorDef);
+    }
+  }
+
+  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.
@@ -883,7 +912,19 @@ class Driver {
     this.sendCommand('Page.enable');
     this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
     this.setNextProtocolTimeout(30 * 1000);
-    this.sendCommand('Page.navigate', {url});
+    this.sendCommand('Page.navigate', {url}).catch(err => {
+      // can timeout on security issue
+      if (err.code !== 'PROTOCOL_TIMEOUT') {
+        throw err;
+      }
+    });
+
+    try {
+      await this.sendCommand('Security.enable');
+      await this.waitForSecurityIssuesCheck();
+    } finally {
+      this.sendCommand('Security.disable');
+    }
 
     if (waitForNavigated) {
       await this._waitForFrameNavigated();
@@ -948,26 +989,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);
   }
@@ -173,22 +172,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 errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST};
-      const insecureDescriptions = explanations
-        .filter(exp => exp.securityState === 'insecure')
-        .map(exp => exp.description);
-      errorDef.message += ` ${insecureDescriptions.join(' ')}`;
-      throw new LHError(errorDef);
-    }
-  }
-
   /**
    * Calls beforePass() on gatherers before tracing
    * has started and before navigation to the target page.
@@ -256,8 +239,13 @@ class GatherRunner {
     if (recordTrace) await driver.beginTrace(settings);
 
     // Navigate.
-    await GatherRunner.loadPage(driver, passContext);
-    log.timeEnd(status);
+    try {
+      await GatherRunner.loadPage(driver, passContext);
+    } catch (err) {
+      throw err;
+    } finally {
+      log.timeEnd(status);
+    }
 
     const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`};
     log.time(pStatus, 'verbose');
@@ -311,8 +299,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;
@@ -466,7 +452,21 @@ class GatherRunner {
           await GatherRunner.loadBlank(driver, passConfig.blankPage);
         }
         await GatherRunner.beforePass(passContext, gathererResults);
-        await GatherRunner.pass(passContext, gathererResults);
+        try {
+          await GatherRunner.pass(passContext, gathererResults);
+        } catch (err) {
+          if (err.code === LHError.errors.INSECURE_DOCUMENT_REQUEST.code) {
+            log.error('GatherRunner.pass', err.message);
+            for (const gathererResult of Object.values(gathererResults)) {
+              if (gathererResult) {
+                gathererResult.push(err);
+              }
+            }
+            break;
+          }
+
+          throw err;
+        }
         const passData = await GatherRunner.afterPass(passContext, gathererResults);
 
         // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts.

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/report/html/renderer/report-ui-features.js

@@ -462,6 +462,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

@@ -219,6 +219,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/';
@@ -507,4 +508,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 */
+      assert.equal(err.friendlyMessage, '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, {
@@ -693,44 +691,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');
-        /* eslint-disable-next-line max-len */
-        assert.equal(err.friendlyMessage, 'The URL you have provided does not have valid 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,
         });