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

core(is-on-https): add mixed-content resolution #10975

Merged
merged 23 commits into from
Jul 10, 2020
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
44 changes: 38 additions & 6 deletions lighthouse-core/audits/is-on-https.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ const UIStrings = {
}`,
/** Label for a column in a data table; entries in the column will be the URLs of insecure (non-HTTPS) network requests. */
columnInsecureURL: 'Insecure URL',
/** Label for a column in a data table; entries in the column will be how the browser handled insecure (non-HTTPS) network requests. */
columnResolution: 'Resolution',
/** Value for the resolution column in a data table; denotes that the insecure URL was blocked by the browser. */
blocked: 'Blocked',
/** Value for the resolution column in a data table; denotes that the insecure URL may be blocked by the browser in the future. */
warning: 'Warned',
/** Value for the resolution column in a data table; denotes that the insecure URL was upgraded to a secure request by the browser. */
upgraded: 'Automatically Upgraded',
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
Expand All @@ -48,7 +56,7 @@ class HTTPS extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['devtoolsLogs'],
requiredArtifacts: ['devtoolsLogs', 'InspectorIssues'],
};
}

Expand All @@ -74,18 +82,42 @@ class HTTPS extends Audit {
.filter(record => !HTTPS.isSecureRecord(record))
.map(record => URL.elideDataURI(record.url));

/** @type {Array<{url: string, resolution?: string}>} */
const items = Array.from(new Set(insecureURLs)).map(url => ({url}));

let displayValue = '';
if (items.length > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: items.length});
}

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
{key: 'url', itemType: 'url', text: str_(UIStrings.columnInsecureURL)},
];

// Mixed-content issues aren't emitted until M84.
if (artifacts.InspectorIssues.mixedContent.length) {
headings.push(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so does anyone hate this

Copy link
Member

Choose a reason for hiding this comment

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

so does anyone hate this

seems fine for < m84, but it is a little weird after that to have an optional column.

I assume if there are items caught by the HTTPS.isSecureRecord filter, they should (almost?) always have a mixed Content issue associated with them. Would it be terrible to give every entry a default resolution: undefined then, since they'll (almost?) always have one anyways ≥ m84?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's my expectation.

Also, I'll file an issue to follow up and remove the isSecureRecord part completely once M84 lands.

Would it be terrible to give every entry a default resolution: undefined then

sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I'll file an issue to follow up and remove the isSecureRecord part completely make the header always added once M84 lands.

Copy link
Member

Choose a reason for hiding this comment

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

cc #10976 some other m84 stuff.

Copy link
Member

Choose a reason for hiding this comment

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

bump on

I'll file an issue to follow up and make the header always added once M84 lands.

so it's not lost in the middle of this review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

{key: 'resolution', itemType: 'text', text: str_(UIStrings.columnResolution)});
for (const details of artifacts.InspectorIssues.mixedContent) {
let item = items.find(item => item.url === details.insecureURL);
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
if (!item) {
item = {url: details.insecureURL};
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
items.push(item);
}

if (details.resolutionStatus === 'MixedContentAutomaticallyUpgraded') {
item.resolution = UIStrings.upgraded;
} else if (details.resolutionStatus === 'MixedContentBlocked') {
item.resolution = UIStrings.blocked;
} else if (details.resolutionStatus === 'MixedContentWarning') {
item.resolution = UIStrings.warning;
} else {
item.resolution = details.resolutionStatus;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

let displayValue = '';
if (items.length > 0) {
displayValue = str_(UIStrings.displayValue, {itemCount: items.length});
}

return {
score: Number(items.length === 0),
displayValue,
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -794,9 +794,15 @@
"lighthouse-core/audits/installable-manifest.js | title": {
"message": "Web app manifest meets the installability requirements"
},
"lighthouse-core/audits/is-on-https.js | blocked": {
"message": "Blocked"
},
"lighthouse-core/audits/is-on-https.js | columnInsecureURL": {
"message": "Insecure URL"
},
"lighthouse-core/audits/is-on-https.js | columnResolution": {
"message": "Resolution"
},
"lighthouse-core/audits/is-on-https.js | description": {
"message": "All sites should be protected with HTTPS, even ones that don't handle sensitive data. This includes avoiding [mixed content](https://developers.google.com/web/fundamentals/security/prevent-mixed-content/what-is-mixed-content), where some resources are loaded over HTTP despite the initial request being servedover HTTPS. HTTPS prevents intruders from tampering with or passively listening 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://web.dev/is-on-https)."
},
Expand All @@ -809,6 +815,12 @@
"lighthouse-core/audits/is-on-https.js | title": {
"message": "Uses HTTPS"
},
"lighthouse-core/audits/is-on-https.js | upgraded": {
"message": "Automatically Upgraded"
},
"lighthouse-core/audits/is-on-https.js | warning": {
"message": "Warned"
},
"lighthouse-core/audits/largest-contentful-paint-element.js | description": {
"message": "This is the element that was identified as the Largest Contentful Paint. [Learn More](https://web.dev/lighthouse-largest-contentful-paint)"
},
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json
Original file line number Diff line number Diff line change
Expand Up @@ -794,9 +794,15 @@
"lighthouse-core/audits/installable-manifest.js | title": {
"message": "Ŵéb̂ áp̂ṕ m̂án̂íf̂éŝt́ m̂éêt́ŝ t́ĥé îńŝt́âĺl̂áb̂íl̂ít̂ý r̂éq̂úîŕêḿêńt̂ś"
},
"lighthouse-core/audits/is-on-https.js | blocked": {
"message": "B̂ĺôćk̂éd̂"
},
"lighthouse-core/audits/is-on-https.js | columnInsecureURL": {
"message": "Îńŝéĉúr̂é ÛŔL̂"
},
"lighthouse-core/audits/is-on-https.js | columnResolution": {
"message": "R̂éŝól̂út̂íôń"
},
"lighthouse-core/audits/is-on-https.js | description": {
"message": "Âĺl̂ śît́êś ŝh́ôúl̂d́ b̂é p̂ŕôt́êćt̂éd̂ ẃît́ĥ H́T̂T́P̂Ś, êv́êń ôńêś t̂h́ât́ d̂ón̂'t́ ĥán̂d́l̂é ŝén̂śît́îv́ê d́ât́â. T́ĥíŝ ín̂ćl̂úd̂éŝ áv̂óîd́îńĝ [ḿîx́êd́ ĉón̂t́êńt̂](https://developers.google.com/web/fundamentals/security/prevent-mixed-content/what-is-mixed-content), ẃĥér̂é ŝóm̂é r̂éŝóûŕĉéŝ ár̂é l̂óâd́êd́ ôv́êŕ ĤT́T̂Ṕ d̂éŝṕît́ê t́ĥé îńît́îál̂ ŕêq́ûéŝt́ b̂éîńĝ śêŕv̂éd̂óv̂ér̂ H́T̂T́P̂Ś. ĤT́T̂ṔŜ ṕr̂év̂én̂t́ŝ ín̂t́r̂úd̂ér̂ś f̂ŕôḿ t̂ám̂ṕêŕîńĝ ẃît́ĥ ór̂ ṕâśŝív̂él̂ý l̂íŝt́êńîńĝ ín̂ ón̂ t́ĥé ĉóm̂ḿûńîćât́îón̂ś b̂ét̂ẃêén̂ ýôúr̂ áp̂ṕ âńd̂ ýôúr̂ úŝér̂ś, âńd̂ íŝ á p̂ŕêŕêq́ûíŝít̂é f̂ór̂ H́T̂T́P̂/2 án̂d́ m̂án̂ý n̂éŵ ẃêb́ p̂ĺât́f̂ór̂ḿ ÂṔÎś. [L̂éâŕn̂ ḿôŕê](https://web.dev/is-on-https)."
},
Expand All @@ -809,6 +815,12 @@
"lighthouse-core/audits/is-on-https.js | title": {
"message": "Ûśêś ĤT́T̂ṔŜ"
},
"lighthouse-core/audits/is-on-https.js | upgraded": {
"message": "Âút̂óm̂át̂íĉál̂ĺŷ Úp̂ǵr̂ád̂éd̂"
},
"lighthouse-core/audits/is-on-https.js | warning": {
"message": "Ŵár̂ńêd́"
},
"lighthouse-core/audits/largest-contentful-paint-element.js | description": {
"message": "T̂h́îś îś t̂h́ê él̂ém̂én̂t́ t̂h́ât́ ŵáŝ íd̂én̂t́îf́îéd̂ áŝ t́ĥé L̂ár̂ǵêśt̂ Ćôńt̂én̂t́f̂úl̂ Ṕâín̂t́. [L̂éâŕn̂ Ḿôŕê](https://web.dev/lighthouse-largest-contentful-paint)"
},
Expand Down
22 changes: 21 additions & 1 deletion lighthouse-core/test/audits/is-on-https-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.
/* eslint-env jest */

describe('Security: HTTPS audit', () => {
function getArtifacts(networkRecords) {
function getArtifacts(networkRecords, mixedContentIssues) {
const devtoolsLog = networkRecordsToDevtoolsLog(networkRecords);
return {
devtoolsLogs: {[Audit.DEFAULT_PASS]: devtoolsLog},
InspectorIssues: {mixedContent: mixedContentIssues || []},
};
}

Expand All @@ -42,6 +43,7 @@ describe('Security: HTTPS audit', () => {
assert.strictEqual(result.score, 0);
expect(result.displayValue).toBeDisplayString('1 insecure request found');
assert.deepEqual(result.extendedInfo.value[0], {url: 'http://insecure.com/image.jpeg'});
Copy link
Member

Choose a reason for hiding this comment

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

result.extendedInfo

guh we really have to land #10779 :)

assert.strictEqual(result.details.headings.length, 1);
});
});

Expand All @@ -55,6 +57,24 @@ describe('Security: HTTPS audit', () => {
});
});

it('augmented with mixed-content InspectorIssues', async () => {
const networkRecords = [
{url: 'https://google.com/', parsedURL: {scheme: 'https', host: 'google.com'}},
{url: 'http://localhost/image.jpeg', parsedURL: {scheme: 'http', host: 'localhost'}},
{url: 'https://google.com/', parsedURL: {scheme: 'https', host: 'google.com'}},
];
const mixedContentIssues = [
{insecureURL: 'http://localhost/image.jpeg', resolutionStatus: 'MixedContentBlocked'},
];
const artifacts = getArtifacts(networkRecords, mixedContentIssues);
const result = await Audit.audit(artifacts, {computedCache: new Map()});

expect(result.details.headings).toHaveLength(2);
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
expect(result.details.items[0]).toMatchObject({url: 'http://localhost/image.jpeg'});
expect(result.details.items[0].resolution).toBeDisplayString('Blocked');
Copy link
Member

Choose a reason for hiding this comment

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

super-nit: can be fancy and do

Suggested change
expect(result.details.items[0]).toMatchObject({url: 'http://localhost/image.jpeg'});
expect(result.details.items[0].resolution).toBeDisplayString('Blocked');
expect(result.details.items[0]).toMatchObject({
url: 'http://localhost/image.jpeg',
resolution: expect.toBeDisplayString('Blocked'),
});

(or it could be s/toMatchObject/toEqual if both are there?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh i missed the fancy

neat

expect(result.score).toBe(0);
});

describe('#isSecureRecord', () => {
it('correctly identifies insecure records', () => {
assert.strictEqual(Audit.isSecureRecord({parsedURL: {scheme: 'http', host: 'google.com'}}),
Expand Down
Loading