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,53 @@ class Driver {
     });
   }
 
+  /**
+   * Rejects if the security state is insecure.
+   * @return {{promise: Promise<void>, cancel: function(): void}}
+   * @private
+   */
+  _waitForSecurityCheck() {
+    /** @type {(() => void)} */
+    let cancel = () => {
+      throw new Error('_waitForSecurityCheck.cancel() called before it was defined');
+    };
+
+    const promise = new Promise(async (resolve, reject) => {
+      /**
+       * @param {LH.Crdp.Security.SecurityStateChangedEvent} event
+       */
+      const securityStateChangedListener = ({securityState, explanations}) => {
+        this.sendCommand('Security.disable');
+        if (securityState === 'insecure') {
+          const insecureDescriptions = explanations
+            .filter(exp => exp.securityState === 'insecure')
+            .map(exp => exp.description);
+          const err = new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {
+            securityMessages: insecureDescriptions.join(' '),
+          });
+          reject(err);
+        } else {
+          resolve();
+        }
+      };
+
+      // wait for Security.enable to resolve, so we skip whatever state change
+      // events are in the pipeline
+      await this.sendCommand('Security.enable');
+      this.once('Security.securityStateChanged', securityStateChangedListener);
+
+      cancel = () => {
+        this.off('Security.securityStateChanged', securityStateChangedListener);
+        this.sendCommand('Security.disable');
+      };
+    });
+
+    return {
+      promise,
+      cancel,
+    };
+  }
+
   /**
    * Returns a promise that resolve when a frame has been navigated.
    * Used for detecting that our about:blank reset has been completed.
@@ -741,6 +781,9 @@ class Driver {
     /** @type {NodeJS.Timer|undefined} */
     let maxTimeoutHandle;
 
+
+    // Listener for security state change. Rejects if security issue is found.
+    const waitForSecurityCheck = this._waitForSecurityCheck();
     // Listener for onload. Resolves pauseAfterLoadMs ms after load.
     const waitForLoadEvent = this._waitForLoadEvent(pauseAfterLoadMs);
     // Network listener. Resolves when the network has been idle for networkQuietThresholdMs.
@@ -752,6 +795,7 @@ class Driver {
     // Wait for both load promises. Resolves on cleanup function the clears load
     // timeout timer.
     const loadPromise = Promise.all([
+      waitForSecurityCheck.promise,
       waitForLoadEvent.promise,
       waitForNetworkIdle.promise,
     ]).then(() => {
@@ -771,6 +815,7 @@ class Driver {
     }).then(_ => {
       return async () => {
         log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...');
+        waitForSecurityCheck.cancel();
         waitForLoadEvent.cancel();
         waitForNetworkIdle.cancel();
         waitForCPUIdle && waitForCPUIdle.cancel();
@@ -959,26 +1004,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.
@@ -311,8 +293,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/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/test/gather/driver-test.js

@@ -99,6 +99,8 @@ connection.sendCommand = function(command, params) {
     case 'Tracing.start':
     case 'ServiceWorker.enable':
     case 'ServiceWorker.disable':
+    case 'Security.enable':
+    case 'Security.disable':
     case 'Network.setExtraHTTPHeaders':
     case 'Network.emulateNetworkConditions':
     case 'Emulation.setCPUThrottlingRate':
@@ -221,6 +223,12 @@ describe('Browser Driver', () => {
     }
     const replayConnection = new ReplayConnection();
     const driver = new Driver(replayConnection);
+    driver._waitForSecurityCheck = () => {
+      return {
+        promise: Promise.resolve(),
+        cancel: () => {},
+      };
+    };
 
     // Redirect in log will go through
     const startUrl = 'http://en.wikipedia.org/';
@@ -535,4 +543,48 @@ describe('Multiple tab check', () => {
       });
     });
   });
+
+  describe('._waitForSecurityCheck', () => {
+    it('returns nothing when page is secure', async () => {
+      const secureSecurityState = {
+        securityState: 'secure',
+      };
+      driverStub.once = createOnceStub({
+        'Security.securityStateChanged': secureSecurityState,
+      });
+      await driverStub._waitForSecurityCheck().promise;
+    });
+
+    it('returns an error when page is insecure', async () => {
+      const insecureSecurityState = {
+        explanations: [
+          {
+            description: 'reason 1.',
+            securityState: 'insecure',
+          },
+          {
+            description: 'blah.',
+            securityState: 'info',
+          },
+          {
+            description: 'reason 2.',
+            securityState: 'insecure',
+          },
+        ],
+        securityState: 'insecure',
+      };
+      driverStub.once = createOnceStub({
+        'Security.securityStateChanged': insecureSecurityState,
+      });
+      try {
+        await driverStub._waitForSecurityCheck().promise;
+        assert.fail('_waitForSecurityCheck should have rejected');
+      } catch (err) {
+        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,6 @@ const fakeDriver = {
   setExtraHTTPHeaders() {
     return Promise.resolve();
   },
-  listenForSecurityStateChanges() {
-    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 () => {