core(driver): exit early when testing page with insecure certificate (#…

…6608)

previously tests of pages with insecure certificates ran until the page load timeout

clients/extension/scripts/popup.js

@@ -26,6 +26,8 @@ const NON_BUG_ERROR_MESSAGES = {
       'are trying to review.',
   'Cannot access contents of the page': 'Lighthouse can only audit URLs that start' +
       ' with http:// or https://.',
+  'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have valid' +
+      ' security credentials.',
   'INVALID_URL': 'Lighthouse can only audit URLs that start' +
       ' with http:// or https://.',
 };

lighthouse-cli/run.js

@@ -23,6 +23,7 @@ const opn = require('opn');
 const _RUNTIME_ERROR_CODE = 1;
 const _PROTOCOL_TIMEOUT_EXIT_CODE = 67;
 const _PAGE_HUNG_EXIT_CODE = 68;
+const _INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69;
 
 /**
  * exported for testing
@@ -77,6 +78,12 @@ function showPageHungError(err) {
   process.exit(_PAGE_HUNG_EXIT_CODE);
 }
 
+/** @param {LH.LighthouseError} err */
+function showInsecureDocumentRequestError(err) {
+  console.error('Insecure document request:', err.friendlyMessage);
+  process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE);
+}
+
 /**
  * @param {LH.LighthouseError} err
  */
@@ -98,6 +105,8 @@ function handleError(err) {
     showProtocolTimeoutError();
   } else if (err.code === 'PAGE_HUNG') {
     showPageHungError(err);
+  } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') {
+    showInsecureDocumentRequestError(err);
   } else {
     showRuntimeError(err);
   }

lighthouse-cli/test/smokehouse/error-expectations.js

@@ -15,4 +15,10 @@ module.exports = [
     errorCode: 'PAGE_HUNG',
     audits: {},
   },
+  {
+    requestedUrl: 'https://expired.badssl.com',
+    finalUrl: 'https://expired.badssl.com',
+    errorCode: 'INSECURE_DOCUMENT_REQUEST',
+    audits: {},
+  },
 ];

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -32,6 +32,7 @@ const log = require('lighthouse-logger');
 
 const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
 const PAGE_HUNG_EXIT_CODE = 68;
+const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69;
 const RETRIES = 3;
 const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/;
 
@@ -103,7 +104,9 @@ function runLighthouse(url, configPath, isDebug) {
   if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) {
     console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`);
     process.exit(1);
-  } else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE) {
+  } else if (runResults.status !== 0
+     && runResults.status !== PAGE_HUNG_EXIT_CODE
+     && runResults.status !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE) {
     console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`);
     console.error(runResults.stderr);
     process.exit(runResults.status);
@@ -118,6 +121,10 @@ function runLighthouse(url, configPath, isDebug) {
     return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}};
   }
 
+  if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) {
+    return {requestedUrl: url, finalUrl: url, errorCode: 'INSECURE_DOCUMENT_REQUEST', audits: {}};
+  }
+
   const lhr = fs.readFileSync(outputPath, 'utf8');
   if (isDebug) {
     console.log('LHR output available at: ', outputPath);

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
@@ -546,14 +539,17 @@ class Driver {
       /** @param {LH.Crdp.Page.LifecycleEventEvent} e */
       const lifecycleListener = e => {
         if (e.name === 'firstContentfulPaint') {
+          cancel();
           resolve();
-          this.off('Page.lifecycleEvent', lifecycleListener);
         }
       };
 
       this.on('Page.lifecycleEvent', lifecycleListener);
 
+      let canceled = false;
       cancel = () => {
+        if (canceled) return;
+        canceled = true;
         this.off('Page.lifecycleEvent', lifecycleListener);
       };
     });
@@ -634,7 +630,10 @@ class Driver {
       networkStatusMonitor.on('network-2-busy', logStatus);
 
       this.once('Page.domContentEventFired', domContentLoadedListener);
+      let canceled = false;
       cancel = () => {
+        if (canceled) return;
+        canceled = true;
         idleTimeout && clearTimeout(idleTimeout);
         this.off('Page.domContentEventFired', domContentLoadedListener);
         networkStatusMonitor.removeListener('network-2-busy', onBusy);
@@ -666,7 +665,7 @@ class Driver {
 
     /** @type {NodeJS.Timer|undefined} */
     let lastTimeout;
-    let cancelled = false;
+    let canceled = false;
 
     const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`;
     /**
@@ -675,9 +674,9 @@ class Driver {
      * @return {Promise<void>}
      */
     async function checkForQuiet(driver, resolve) {
-      if (cancelled) return;
+      if (canceled) return;
       const timeSinceLongTask = await driver.evaluateAsync(checkForQuietExpression);
-      if (cancelled) return;
+      if (canceled) return;
 
       if (typeof timeSinceLongTask === 'number') {
         if (timeSinceLongTask >= waitForCPUQuiet) {
@@ -698,9 +697,10 @@ class Driver {
     const promise = new Promise((resolve, reject) => {
       checkForQuiet(this, resolve).catch(reject);
       cancel = () => {
-        cancelled = true;
+        if (canceled) return;
+        canceled = true;
         if (lastTimeout) clearTimeout(lastTimeout);
-        reject(new Error('Wait for CPU idle cancelled'));
+        reject(new Error('Wait for CPU idle canceled'));
       };
     });
 
@@ -731,7 +731,10 @@ class Driver {
       };
       this.once('Page.loadEventFired', loadListener);
 
+      let canceled = false;
       cancel = () => {
+        if (canceled) return;
+        canceled = true;
         this.off('Page.loadEventFired', loadListener);
         loadTimeout && clearTimeout(loadTimeout);
       };
@@ -743,6 +746,49 @@ class Driver {
     };
   }
 
+  /**
+   * Return a promise that resolves when an insecure security state is encountered
+   * and a method to cancel internal listeners.
+   * @return {{promise: Promise<string>, cancel: function(): void}}
+   * @private
+   */
+  _monitorForInsecureState() {
+    /** @type {(() => void)} */
+    let cancel = () => {
+      throw new Error('_monitorForInsecureState.cancel() called before it was defined');
+    };
+
+    const promise = new Promise((resolve, reject) => {
+      /**
+       * @param {LH.Crdp.Security.SecurityStateChangedEvent} event
+       */
+      const securityStateChangedListener = ({securityState, explanations}) => {
+        if (securityState === 'insecure') {
+          cancel();
+          const insecureDescriptions = explanations
+            .filter(exp => exp.securityState === 'insecure')
+            .map(exp => exp.description);
+          resolve(insecureDescriptions.join(' '));
+        }
+      };
+      let canceled = false;
+      cancel = () => {
+        if (canceled) return;
+        canceled = true;
+        this.off('Security.securityStateChanged', securityStateChangedListener);
+        // TODO(@patrickhulce): cancel() should really be a promise itself to handle things like this
+        this.sendCommand('Security.disable').catch(() => {});
+      };
+      this.on('Security.securityStateChanged', securityStateChangedListener);
+      this.sendCommand('Security.enable').catch(() => {});
+    });
+
+    return {
+      promise,
+      cancel,
+    };
+  }
+
   /**
    * Returns whether the page appears to be hung.
    * @return {Promise<boolean>}
@@ -765,6 +811,7 @@ class Driver {
   /**
    * Returns a promise that resolves when:
    * - All of the following conditions have been met:
+   *    - page has no security issues
    *    - pauseAfterLoadMs milliseconds have passed since the load event.
    *    - networkQuietThresholdMs milliseconds have passed since the last network request that exceeded
    *      2 inflight requests (network-2-quiet has been reached).
@@ -793,6 +840,13 @@ class Driver {
     // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle.
     let waitForCPUIdle = this._waitForNothing();
 
+    const monitorForInsecureState = this._monitorForInsecureState();
+    const securityCheckPromise = monitorForInsecureState.promise.then(securityMessages => {
+      return function() {
+        throw new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages});
+      };
+    });
+
     // Wait for both load promises. Resolves on cleanup function the clears load
     // timeout timer.
     const loadPromise = Promise.all([
@@ -805,7 +859,6 @@ class Driver {
     }).then(() => {
       return function() {
         log.verbose('Driver', 'loadEventFired and network considered idle');
-        maxTimeoutHandle && clearTimeout(maxTimeoutHandle);
       };
     });
 
@@ -816,11 +869,6 @@ class Driver {
     }).then(_ => {
       return async () => {
         log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...');
-        waitForFCP.cancel();
-        waitForLoadEvent.cancel();
-        waitForNetworkIdle.cancel();
-        waitForCPUIdle.cancel();
-
         if (await this.isPageHung()) {
           log.warn('Driver', 'Page appears to be hung, killing JavaScript...');
           await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true});
@@ -830,11 +878,20 @@ class Driver {
       };
     });
 
-    // Wait for load or timeout and run the cleanup function the winner returns.
+    // Wait for security issue, load or timeout and run the cleanup function the winner returns.
     const cleanupFn = await Promise.race([
+      securityCheckPromise,
       loadPromise,
       maxTimeoutPromise,
     ]);
+
+    maxTimeoutHandle && clearTimeout(maxTimeoutHandle);
+    waitForFCP.cancel();
+    waitForLoadEvent.cancel();
+    waitForNetworkIdle.cancel();
+    waitForCPUIdle.cancel();
+    monitorForInsecureState.cancel();
+
     await cleanupFn();
   }
 
@@ -1007,26 +1064,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

@@ -110,7 +110,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,23 +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 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.
@@ -312,8 +294,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

@@ -992,8 +992,8 @@
     "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})",
-    "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed."
+    "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. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load."
   },
   "lighthouse-core/lib/lh-error.js | pageLoadFailedWithDetails": {
     "message": "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})",

lighthouse-core/lib/lh-error.js

@@ -21,8 +21,8 @@ const UIStrings = {
   pageLoadFailedWithStatusCode: '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. (Status code: {statusCode})',
   /** 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})',
+  /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load. */
+  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. */