fix: simpler https audit (#1918)

* fix: simpler https audit
* feedback
* test cleanup

lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html

@@ -353,5 +353,7 @@ if (location.search === '') {
 <!-- PASS: not in header, so it does not block rendering. zone.js is loaded
      by the static-server and provides a polyfill for Promise. -->
 <script src="/zone.js"></script>
+<!-- FAIL(is-on-https): requires a non-localhost http file -->
+<script src="http://ajax.googleapis.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
 </body>
 </html>

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -200,7 +200,7 @@ module.exports = [
     url: 'http://localhost:10200/online-only.html',
     audits: {
       'is-on-https': {
-        score: false
+        score: true
       },
       'uses-http2': {
         score: false

lighthouse-cli/test/smokehouse/offline-local/offline-expectations.js

@@ -27,7 +27,7 @@ module.exports = [
     url: 'http://localhost:10200/online-only.html',
     audits: {
       'is-on-https': {
-        score: false
+        score: true
       },
       'redirects-http': {
         score: false
@@ -90,7 +90,7 @@ module.exports = [
     url: 'http://localhost:10503/offline-ready.html',
     audits: {
       'is-on-https': {
-        score: false
+        score: true
       },
       'redirects-http': {
         score: false

lighthouse-cli/test/smokehouse/pwa-config.js

@@ -25,7 +25,6 @@ module.exports = {
     recordTrace: true,
     gatherers: [
       'url',
-      'https',
       'theme-color',
       'manifest',
       // https://github.com/GoogleChrome/lighthouse/issues/566

lighthouse-core/audits/is-on-https.js

@@ -17,6 +17,11 @@
 'use strict';
 
 const Audit = require('./audit');
+const Formatter = require('../report/formatter');
+const URL = require('../lib/url-shim');
+
+const SECURE_SCHEMES = ['data', 'https', 'wss'];
+const SECURE_DOMAINS = ['localhost', '127.0.0.1'];
 
 class HTTPS extends Audit {
   /**
@@ -32,18 +37,42 @@ class HTTPS extends Audit {
           'in on the communications between your app and your users, and is a prerequisite for ' +
           'HTTP/2 and many new web platform APIs. ' +
           '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/https).',
-      requiredArtifacts: ['HTTPS']
+      requiredArtifacts: ['networkRecords']
     };
   }
 
+  /**
+   * @param {{scheme: string, domain: string}} record
+   * @return {boolean}
+   */
+  static isSecureRecord(record) {
+    return SECURE_SCHEMES.includes(record.scheme) || SECURE_DOMAINS.includes(record.domain);
+  }
+
   /**
    * @param {!Artifacts} artifacts
    * @return {!AuditResult}
    */
   static audit(artifacts) {
+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+    const insecureRecords = networkRecords
+        .filter(record => !HTTPS.isSecureRecord(record))
+        .map(record => ({url: URL.getDisplayName(record.url, {preserveHost: true})}));
+
+    let displayValue = '';
+    if (insecureRecords.length > 1) {
+      displayValue = `${insecureRecords.length} insecure requests found`;
+    } else if (insecureRecords.length === 1) {
+      displayValue = `${insecureRecords.length} insecure request found`;
+    }
+
     return {
-      rawValue: artifacts.HTTPS.value,
-      debugString: artifacts.HTTPS.debugString
+      rawValue: insecureRecords.length === 0,
+      displayValue,
+      extendedInfo: {
+        formatter: Formatter.SUPPORTED_FORMATS.URL_LIST,
+        value: insecureRecords
+      }
     };
   }
 }

lighthouse-core/config/default.js

@@ -8,7 +8,6 @@ module.exports = {
     "useThrottling": true,
     "gatherers": [
       "url",
-      "https",
       "viewport",
       "viewport-dimensions",
       "theme-color",

lighthouse-core/gather/gatherers/https.js

@@ -1,67 +0,0 @@
-/**
- * @license
- * Copyright 2016 Google Inc. All rights reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-'use strict';
-
-const Gatherer = require('./gatherer');
-
-/**
- * @fileoverview Determines the security level of the page.
- * @see https://chromedevtools.github.io/debugger-protocol-viewer/tot/Security/#type-SecurityState
- */
-
-class HTTPS extends Gatherer {
-
-  constructor() {
-    super();
-    this._noSecurityChangesTimeout = undefined;
-  }
-
-  afterPass(options) {
-    // Allow override for faster testing.
-    const timeout = options._testTimeout || 10000;
-
-    const securityPromise = options.driver.getSecurityState()
-      .then(state => {
-        return {
-          value: state.schemeIsCryptographic
-        };
-      });
-
-    let noSecurityChangesTimeout;
-    const timeoutPromise = new Promise((resolve, reject) => {
-      // Set up a timeout for ten seconds in case we don't get any
-      // security events at all. If that happens, bail.
-      noSecurityChangesTimeout = setTimeout(_ => {
-        reject(new Error('Timed out waiting for page security state.'));
-      }, timeout);
-    });
-
-    return Promise.race([
-      securityPromise,
-      timeoutPromise
-    ]).then(result => {
-      // Clear timeout. No effect if it won, no need to wait if it lost.
-      clearTimeout(noSecurityChangesTimeout);
-      return result;
-    }).catch(err => {
-      clearTimeout(noSecurityChangesTimeout);
-      throw err;
-    });
-  }
-}
-
-module.exports = HTTPS;

lighthouse-core/test/audits/is-on-https-test.js

@@ -21,24 +21,56 @@ const assert = require('assert');
 /* eslint-env mocha */
 
 describe('Security: HTTPS audit', () => {
-  it('fails when not on HTTPS', () => {
-    const debugString = 'Error string';
-    const result = Audit.audit({
-      HTTPS: {
-        value: false,
-        debugString
-      }
-    });
+  function getArtifacts(networkRecords) {
+    return {networkRecords: {defaultPass: networkRecords}};
+  }
+
+  it('fails when there is more than one insecure record', () => {
+    const result = Audit.audit(getArtifacts([
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+      {url: 'http://insecure.com/image.jpeg', scheme: 'http', domain: 'insecure.com'},
+      {url: 'http://insecure.com/image2.jpeg', scheme: 'http', domain: 'insecure.com'},
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+    ]));
     assert.strictEqual(result.rawValue, false);
-    assert.strictEqual(result.debugString, debugString);
+    assert.ok(result.displayValue.includes('requests found'));
+    assert.strictEqual(result.extendedInfo.value.length, 2);
   });
 
-  it('passes when on HTTPS', () => {
-    const result = Audit.audit({
-      HTTPS: {
-        value: true
-      }
-    });
+  it('fails when there is one insecure record', () => {
+    const result = Audit.audit(getArtifacts([
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+      {url: 'http://insecure.com/image.jpeg', scheme: 'http', domain: 'insecure.com'},
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+    ]));
+    assert.strictEqual(result.rawValue, false);
+    assert.ok(result.displayValue.includes('request found'));
+    assert.deepEqual(result.extendedInfo.value[0], {url: 'insecure.com/image.jpeg'});
+  });
+
+  it('passes when all records are secure', () => {
+    const result = Audit.audit(getArtifacts([
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+      {url: 'http://localhost/image.jpeg', scheme: 'http', domain: 'localhost'},
+      {url: 'https://google.com/', scheme: 'https', domain: 'google.com'},
+    ]));
+
     assert.strictEqual(result.rawValue, true);
   });
+
+  describe('#isSecureRecord', () => {
+    it('correctly identifies insecure records', () => {
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'http', domain: 'google.com'}), false);
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'http', domain: '54.33.21.23'}), false);
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'ws', domain: 'my-service.com'}), false);
+      assert.strictEqual(Audit.isSecureRecord({scheme: '', domain: 'google.com'}), false);
+    });
+
+    it('correctly identifies secure records', () => {
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'http', domain: 'localhost'}), true);
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'https', domain: 'google.com'}), true);
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'wss', domain: 'my-service.com'}), true);
+      assert.strictEqual(Audit.isSecureRecord({scheme: 'data', domain: ''}), true);
+    });
+  });
 });

lighthouse-core/test/config/config-test.js

@@ -138,7 +138,6 @@ describe('Config', () => {
       passes: [{
         gatherers: [
           'url',
-          'https',
           'viewport'
         ]
       }],
@@ -146,7 +145,7 @@ describe('Config', () => {
     };
 
     const _ = new Config(configJSON);
-    assert.equal(configJSON.passes[0].gatherers.length, 3);
+    assert.equal(configJSON.passes[0].gatherers.length, 2);
   });
 
   it('contains new copies of auditResults and aggregations', () => {

lighthouse-core/test/gather/gatherers/https-test.js

@@ -1,78 +0,0 @@
-/**
- * Copyright 2016 Google Inc. All rights reserved.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-'use strict';
-
-/* eslint-env mocha */
-
-const HTTPSGather = require('../../../gather/gatherers/https');
-const assert = require('assert');
-let httpsGather;
-
-describe('HTTPS gatherer', () => {
-  // Reset the Gatherer before each test.
-  beforeEach(() => {
-    httpsGather = new HTTPSGather();
-  });
-
-  it('returns an artifact', () => {
-    return httpsGather.afterPass({
-      driver: {
-        getSecurityState() {
-          return Promise.resolve({
-            schemeIsCryptographic: true
-          });
-        }
-      }
-    }).then(artifact => {
-      assert.deepEqual(artifact.value, true);
-    });
-  });
-
-  it('throws an error on driver failure', () => {
-    return httpsGather.afterPass({
-      driver: {
-        getSecurityState() {
-          return Promise.reject('such a fail');
-        }
-      }
-    }).then(
-      _ => assert.ok(false),
-      _ => assert.ok(true));
-  });
-
-  it('throws an error on driver timeout', () => {
-    const fastTimeout = 50;
-    const slowResolve = 200;
-
-    return httpsGather.afterPass({
-      driver: {
-        getSecurityState() {
-          return new Promise((resolve, reject) => {
-            // Resolve slowly, after the timeout for waiting on the security
-            // state has fired.
-            setTimeout(_ => resolve({
-              schemeIsCryptographic: true
-            }), slowResolve);
-          });
-        }
-      },
-
-      _testTimeout: fastTimeout
-    }).then(
-      _ => assert.ok(false),
-      _ => assert.ok(true));
-  });
-});

lighthouse-core/test/runner-test.js

@@ -30,10 +30,10 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       passes: [{
-        gatherers: ['https']
+        gatherers: ['viewport-dimensions']
       }],
       audits: [
-        'is-on-https'
+        'content-width'
       ]
     });
 
@@ -46,7 +46,7 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       audits: [
-        'is-on-https'
+        'content-width'
       ]
     });
 
@@ -62,20 +62,18 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       audits: [
-        'is-on-https'
+        'content-width'
       ],
 
       artifacts: {
-        HTTPS: {
-          value: true
-        }
+        ViewportDimensions: {}
       }
     });
 
     return Runner.run({}, {url, config}).then(results => {
       // Mostly checking that this did not throw, but check representative values.
       assert.equal(results.initialUrl, url);
-      assert.strictEqual(results.audits['is-on-https'].rawValue, true);
+      assert.strictEqual(results.audits['content-width'].rawValue, true);
     });
   });
 
@@ -152,18 +150,18 @@ describe('Runner', () => {
       const url = 'https://example.com';
       const config = new Config({
         audits: [
-          // requires the HTTPS artifact
-          'is-on-https'
+          // requires the ViewportDimensions artifact
+          'content-width'
         ],
 
         artifacts: {}
       });
 
       return Runner.run({}, {url, config}).then(results => {
-        const auditResult = results.audits['is-on-https'];
+        const auditResult = results.audits['content-width'];
         assert.strictEqual(auditResult.rawValue, null);
         assert.strictEqual(auditResult.error, true);
-        assert.ok(auditResult.debugString.includes('HTTPS'));
+        assert.ok(auditResult.debugString.includes('ViewportDimensions'));
       });
     });
 
@@ -174,19 +172,19 @@ describe('Runner', () => {
       const url = 'https://example.com';
       const config = new Config({
         audits: [
-          'is-on-https'
+          'content-width'
         ],
 
         artifacts: {
           // Error objects don't make it through the Config constructor due to
           // JSON.stringify/parse step, so populate with test error below.
-          HTTPS: null
+          ViewportDimensions: null
         }
       });
-      config.artifacts.HTTPS = artifactError;
+      config.artifacts.ViewportDimensions = artifactError;
 
       return Runner.run({}, {url, config}).then(results => {
-        const auditResult = results.audits['is-on-https'];
+        const auditResult = results.audits['content-width'];
         assert.strictEqual(auditResult.rawValue, null);
         assert.strictEqual(auditResult.error, true);
         assert.ok(auditResult.debugString.includes(errorMessage));
@@ -277,7 +275,7 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       passes: [{
-        gatherers: ['https']
+        gatherers: ['viewport-dimensions']
       }]
     });
 
@@ -293,7 +291,7 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       auditResults: [{
-        name: 'is-on-https',
+        name: 'content-width',
         rawValue: true,
         score: true,
         displayValue: ''
@@ -308,7 +306,7 @@ describe('Runner', () => {
           name: 'name',
           description: 'description',
           audits: {
-            'is-on-https': {
+            'content-width': {
               expectedValue: true,
               weight: 1
             }
@@ -320,7 +318,7 @@ describe('Runner', () => {
     return Runner.run(null, {url, config, driverMock}).then(results => {
       // Mostly checking that this did not throw, but check representative values.
       assert.equal(results.initialUrl, url);
-      assert.strictEqual(results.audits['is-on-https'].rawValue, true);
+      assert.strictEqual(results.audits['content-width'].rawValue, true);
     });
   });
 
@@ -328,7 +326,7 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       auditResults: [{
-        name: 'is-on-https',
+        name: 'content-width',
         rawValue: true,
         score: true,
         displayValue: ''
@@ -343,7 +341,7 @@ describe('Runner', () => {
           name: 'name',
           description: 'description',
           audits: {
-            'is-on-https': {
+            'content-width': {
               expectedValue: true,
               weight: 1
             }
@@ -356,9 +354,9 @@ describe('Runner', () => {
       assert.ok(results.lighthouseVersion);
       assert.ok(results.generatedTime);
       assert.equal(results.initialUrl, url);
-      assert.equal(results.audits['is-on-https'].name, 'is-on-https');
+      assert.equal(results.audits['content-width'].name, 'content-width');
       assert.equal(results.aggregations[0].score[0].overall, 1);
-      assert.equal(results.aggregations[0].score[0].subItems[0], 'is-on-https');
+      assert.equal(results.aggregations[0].score[0].subItems[0], 'content-width');
     });
   });
 
@@ -390,20 +388,17 @@ describe('Runner', () => {
 
   it('results include artifacts when given artifacts and audits', () => {
     const url = 'https://example.com';
+    const ViewportDimensions = {innerHeight: 10, innerWidth: 10};
     const config = new Config({
       audits: [
-        'is-on-https'
+        'content-width'
       ],
 
-      artifacts: {
-        HTTPS: {
-          value: true
-        }
-      }
+      artifacts: {ViewportDimensions}
     });
 
     return Runner.run({}, {url, config}).then(results => {
-      assert.strictEqual(results.artifacts.HTTPS.value, true);
+      assert.deepEqual(results.artifacts.ViewportDimensions, ViewportDimensions);
 
       for (const method of Object.keys(computedArtifacts)) {
         assert.ok(results.artifacts.hasOwnProperty(method));
@@ -415,17 +410,17 @@ describe('Runner', () => {
     const url = 'https://example.com';
     const config = new Config({
       passes: [{
-        gatherers: ['https']
+        gatherers: ['viewport-dimensions']
       }],
 
       audits: [
-        'is-on-https'
+        'content-width'
       ]
     });
 
     return Runner.run(null, {url, config, driverMock}).then(results => {
       // Check whether non-computedArtifacts attributes are returned
-      assert.ok(results.artifacts.HTTPS);
+      assert.ok(results.artifacts.ViewportDimensions);
 
       for (const method of Object.keys(computedArtifacts)) {
         assert.ok(results.artifacts.hasOwnProperty(method));