core: INSECURE_DOCUMENT_REQUEST errors still return lhr

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 _INSECRE_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(_INSECRE_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

@@ -16,6 +16,7 @@ const log = require('lighthouse-logger');
 
 const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
 const PAGE_HUNG_EXIT_CODE = 68;
+const INSECRE_DOCUMENT_REQUEST_EXIT_CODE = 69;
 const RETRIES = 3;
 const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/;
 
@@ -88,7 +89,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 !== INSECRE_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);
@@ -103,6 +106,10 @@ function runLighthouse(url, configPath, isDebug) {
     return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}};
   }
 
+  if (runResults.status === INSECRE_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

@@ -32,6 +32,8 @@ const DEFAULT_CPU_QUIET_THRESHOLD = 0;
 // Controls how long to wait for a response after sending a DevTools protocol command.
 const DEFAULT_PROTOCOL_TIMEOUT = 30000;
 
+const SECURE_SCHEMES = ['data', 'https', 'wss', 'blob', 'chrome', 'chrome-extension', 'about'];
+
 /**
  * @typedef {LH.Protocol.StrictEventEmitter<LH.CrdpEvents>} CrdpEventEmitter
  */
@@ -81,13 +83,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 +507,61 @@ class Driver {
     });
   }
 
+  /**
+   * Listener that resolves or rejects on the first interesting security state change.
+   * If the first change is secure, we resolve.
+   * Otherwise, we reject.
+   * We can expect the security state to always change because this function
+   * is only used to move about:blank (neutral) -> the target url (something not neutral).
+   * @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}) => {
+        // ignore until there is something meaningful to derive security from
+        if (securityState === 'neutral' && !explanations.length) {
+          return;
+        }
+
+        if (securityState === 'insecure') {
+          cancel();
+          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 {
+          cancel();
+          resolve();
+        }
+      };
+
+      this.on('Security.securityStateChanged', securityStateChangedListener);
+      this.sendCommand('Security.enable').catch(() => {});
+
+      cancel = () => {
+        this.off('Security.securityStateChanged', securityStateChangedListener);
+        this.sendCommand('Security.disable').catch(() => {});
+      };
+    });
+
+    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.
@@ -723,6 +773,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).
@@ -741,6 +792,14 @@ class Driver {
     /** @type {NodeJS.Timer|undefined} */
     let maxTimeoutHandle;
 
+    // Noop if offline or not https
+    // https: => https
+    const protocol = new URL(this._monitoredUrl || '').protocol.replace(/:$/, '');
+    const isSecureProtocol = SECURE_SCHEMES.includes(protocol);
+    const waitForSecurityCheck = this.online && isSecureProtocol ? this._waitForSecurityCheck() : {
+      promise: Promise.resolve(),
+      cancel: () => {},
+    };
     // 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 +811,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 +831,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 +1020,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

@@ -992,7 +992,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

@@ -22,7 +22,10 @@ const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog.
 const MAX_WAIT_FOR_PROTOCOL = 20;
 
 function createOnceStub(events) {
-  return (eventName, cb) => {
+  return async (eventName, cb) => {
+    // wait a tick b/c real events never fire immediately
+    await Promise.resolve();
+
     if (events[eventName]) {
       return cb(events[eventName]);
     }
@@ -99,6 +102,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 +226,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 +546,48 @@ describe('Multiple tab check', () => {
       });
     });
   });
+
+  describe('._waitForSecurityCheck', () => {
+    it('does not reject when page is secure', async () => {
+      const secureSecurityState = {
+        securityState: 'secure',
+      };
+      driverStub.on = createOnceStub({
+        'Security.securityStateChanged': secureSecurityState,
+      });
+      await driverStub._waitForSecurityCheck().promise;
+    });
+
+    it('rejects 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.on = 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;