Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: simpler https audit #1918

Merged
merged 5 commits into from
Mar 28, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -353,5 +353,7 @@ <h2>Do better web tester page</h2>
<!-- 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>
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -114,7 +114,7 @@ module.exports = [
url: 'http://localhost:10503/offline-ready.html',
audits: {
'is-on-https': {
score: false
score: true
},
'redirects-http': {
score: false
Expand Down
1 change: 0 additions & 1 deletion lighthouse-cli/test/smokehouse/pwa-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ module.exports = {
recordTrace: true,
gatherers: [
'url',
'https',
'manifest',
// https://github.com/GoogleChrome/lighthouse/issues/566
// 'cache-contents'
Expand Down
35 changes: 32 additions & 3 deletions lighthouse-core/audits/is-on-https.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we're setting the same displayValue in both cases.
Regardless, I'm +1 on ignoring the pluralization issue and keeping the impl simple.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about one string to rule them all:

if (insecureRecords.length) {
  displayValue = `Insecure requests: ${insecureRecords.length}`;
}

We should handle the pluralization though. #polish

displayValue = `${insecureRecords.length} insecure requests found`;
}

return {
rawValue: artifacts.HTTPS.value,
debugString: artifacts.HTTPS.debugString
rawValue: insecureRecords.length === 0,
displayValue,
extendedInfo: {
formatter: Formatter.SUPPORTED_FORMATS.URL_LIST,
value: insecureRecords
}
};
}
}
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/config/default.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"useThrottling": true,
"gatherers": [
"url",
"https",
"viewport",
"viewport-dimensions",
"theme-color",
Expand Down
67 changes: 0 additions & 67 deletions lighthouse-core/gather/gatherers/https.js

This file was deleted.

49 changes: 34 additions & 15 deletions lighthouse-core/test/audits/is-on-https-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,43 @@ 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 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.strictEqual(result.debugString, debugString);
assert.deepEqual(result.extendedInfo.value[0], {url: 'insecure.com/image.jpeg'});
});

it('passes when on HTTPS', () => {
const result = Audit.audit({
HTTPS: {
value: true
}
});
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);
});
});
});
3 changes: 1 addition & 2 deletions lighthouse-core/test/config/config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,15 +138,14 @@ describe('Config', () => {
passes: [{
gatherers: [
'url',
'https',
'viewport'
]
}],
audits: ['is-on-https']
};

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', () => {
Expand Down
78 changes: 0 additions & 78 deletions lighthouse-core/test/gather/gatherers/https-test.js

This file was deleted.

Loading