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

@@ -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
@@ -765,6 +758,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).
@@ -792,6 +786,40 @@ class Driver {
     const waitForNetworkIdle = this._waitForNetworkIdle(networkQuietThresholdMs);
     // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle.
     let waitForCPUIdle = this._waitForNothing();
+    const cancelAll = () => {
+      waitForFCP.cancel();
+      waitForLoadEvent.cancel();
+      waitForNetworkIdle.cancel();
+      waitForCPUIdle.cancel();
+    };
+
+    // Promise that only rejects when an insecure security state is encountered
+    let securityCheckCleanup = () => {};
+    const securityCheckPromise = new Promise((_, reject) => {
+      /**
+       * @param {LH.Crdp.Security.SecurityStateChangedEvent} event
+       */
+      const securityStateChangedListener = ({securityState, explanations}) => {
+        if (securityState === 'insecure') {
+          maxTimeoutHandle && clearTimeout(maxTimeoutHandle);
+          securityCheckCleanup();
+          cancelAll();
+          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);
+        }
+      };
+      securityCheckCleanup = () => {
+        this.off('Security.securityStateChanged', securityStateChangedListener);
+        this.sendCommand('Security.disable').catch(() => {});
+      };
+      this.on('Security.securityStateChanged', securityStateChangedListener);
+      this.sendCommand('Security.enable').catch(() => {});
+    });
 
     // Wait for both load promises. Resolves on cleanup function the clears load
     // timeout timer.
@@ -816,10 +844,7 @@ 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();
+        cancelAll();
 
         if (await this.isPageHung()) {
           log.warn('Driver', 'Page appears to be hung, killing JavaScript...');
@@ -832,9 +857,11 @@ class Driver {
 
     // Wait for load or timeout and run the cleanup function the winner returns.
     const cleanupFn = await Promise.race([
+      securityCheckPromise,
       loadPromise,
       maxTimeoutPromise,
     ]);
+    securityCheckCleanup();
     await cleanupFn();
   }
 
@@ -1007,26 +1034,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,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]);
     }
@@ -95,13 +98,18 @@ connection.sendCommand = function(command, params) {
     case 'Network.getResponseBody':
       return new Promise(res => setTimeout(res, MAX_WAIT_FOR_PROTOCOL + 20));
     case 'Page.enable':
+    case 'Page.navigate':
+    case 'Page.setLifecycleEventsEnabled':
     case 'Network.enable':
     case 'Tracing.start':
     case 'ServiceWorker.enable':
     case 'ServiceWorker.disable':
+    case 'Security.enable':
+    case 'Security.disable':
     case 'Network.setExtraHTTPHeaders':
     case 'Network.emulateNetworkConditions':
     case 'Emulation.setCPUThrottlingRate':
+    case 'Emulation.setScriptExecutionDisabled':
       return Promise.resolve({});
     case 'Tracing.end':
       return Promise.reject(new Error('tracing not started'));
@@ -535,4 +543,75 @@ describe('Multiple tab check', () => {
       });
     });
   });
+
+  describe('Security check', () => {
+    it('does not reject when page is secure', async () => {
+      const secureSecurityState = {
+        explanations: [],
+        securityState: 'secure',
+      };
+      driverStub.on = driverStub.once = createOnceStub({
+        'Security.securityStateChanged': secureSecurityState,
+        'Page.loadEventFired': {},
+        'Page.domContentEventFired': {},
+      });
+
+      const startUrl = 'https://www.example.com';
+      const loadOptions = {
+        waitForLoad: true,
+        passContext: {
+          passConfig: {
+            networkQuietThresholdMs: 1,
+          },
+          settings: {
+            maxWaitForLoad: 1,
+          },
+        },
+      };
+      await driverStub.gotoURL(startUrl, loadOptions);
+    });
+
+    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,
+      });
+
+      const startUrl = 'https://www.example.com';
+      const loadOptions = {
+        waitForLoad: true,
+        passContext: {
+          passConfig: {
+            networkQuietThresholdMs: 1,
+          },
+        },
+      };
+
+      try {
+        await driverStub.gotoURL(startUrl, loadOptions);
+        assert.fail('security check 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;