-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Changes from 10 commits
e69edce
b2c024c
a5884fe
551f5d8
8924930
afc8539
5ac5c6a
d200103
fe5d3f4
ccd152f
71dd133
bf6006f
25ab53e
6969147
0c751b9
ad4b662
fc2f500
bd8d131
2dae0c7
ed7d921
180e96e
a46830a
8a55597
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -30,6 +30,22 @@ 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 allowed by the browser. */ | ||||||
allowed: 'Allowed', | ||||||
/** 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 resolutionToString = { | ||||||
MixedContentAutomaticallyUpgraded: UIStrings.upgraded, | ||||||
MixedContentBlocked: UIStrings.blocked, | ||||||
MixedContentWarning: UIStrings.warning, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these have to be run through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh my. and the tests don't fail.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, and I can't think of a great way to catch something like that :/ I suggested in #10630 that we could tighten some types to require i18n references and never allow strings, but it seems like it would work for things like Other than that, search for every string in test LHRs and see if they're a value in |
||||||
}; | ||||||
|
||||||
const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); | ||||||
|
@@ -48,7 +64,7 @@ class HTTPS extends Audit { | |||||
title: str_(UIStrings.title), | ||||||
failureTitle: str_(UIStrings.failureTitle), | ||||||
description: str_(UIStrings.description), | ||||||
requiredArtifacts: ['devtoolsLogs'], | ||||||
requiredArtifacts: ['devtoolsLogs', 'InspectorIssues'], | ||||||
}; | ||||||
} | ||||||
|
||||||
|
@@ -74,18 +90,43 @@ class HTTPS extends Audit { | |||||
.filter(record => !HTTPS.isSecureRecord(record)) | ||||||
.map(record => URL.elideDataURI(record.url)); | ||||||
|
||||||
const items = Array.from(new Set(insecureURLs)).map(url => ({url})); | ||||||
|
||||||
let displayValue = ''; | ||||||
if (items.length > 0) { | ||||||
displayValue = str_(UIStrings.displayValue, {itemCount: items.length}); | ||||||
} | ||||||
/** @type {Array<{url: string, resolution?: string}>} */ | ||||||
const items = Array.from(new Set(insecureURLs)).map(url => ({url, resolution: undefined})); | ||||||
|
||||||
/** @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( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so does anyone hate this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I'll file an issue to follow up and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc #10976 some other m84 stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bump on
so it's not lost in the middle of this review. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||
} | ||||||
item.resolution = | ||||||
resolutionToString[details.resolutionStatus] || details.resolutionStatus; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is kind of convoluted now but we can clean up in the m84+ follow up you mentioned :) |
||||||
} | ||||||
|
||||||
// If a resolution wasn't assigned from an InspectorIssue, then the item | ||||||
// is not blocked by the browser but we've determined it is insecure anyhow. | ||||||
// For example, if the URL is localhost, all `http` requests are valid | ||||||
// (localhost is a secure context), but we still identify `http` requests | ||||||
// as an "Allowed" insecure URL. | ||||||
for (const item of items) { | ||||||
if (!item.resolution) item.resolution = UIStrings.allowed; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bahhhhh |
||||||
} | ||||||
|
||||||
let displayValue = ''; | ||||||
if (items.length > 0) { | ||||||
displayValue = str_(UIStrings.displayValue, {itemCount: items.length}); | ||||||
} | ||||||
|
||||||
return { | ||||||
score: Number(items.length === 0), | ||||||
displayValue, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,9 @@ class InspectorIssues extends Gatherer { | |
async beforePass(passContext) { | ||
const driver = passContext.driver; | ||
driver.on('Audits.issueAdded', this._onIssueAdded); | ||
await driver.sendCommand('Audits.enable'); | ||
try { | ||
await driver.sendCommand('Audits.enable'); | ||
} catch (_) {} // Fails if Chrome is old. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you change this to an "Fails if Chrome is older than mXX" for easy future reference? (could also be a TODO to remove but that may be overkill) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be good >= 82 .... gonna comment out and see if it still fails in CI (it shouldnt..) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. appveyor fails bc it's holding on to an old version of chrome. no idea how to clear cache so I'll just push a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. blocked on #11017 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not a reason to keep our appveyor on life support, but FWIW this is because Chrome on Windows isn't a console app. See https://crbug.com/158372 for some awkward ways to get a version number from the command line there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
not a huge deal but my brain associates "old"/age with higher numbers, but here, older means lower. 😖 maybe we just stick to |
||
} | ||
|
||
/** | ||
|
@@ -46,7 +48,9 @@ class InspectorIssues extends Gatherer { | |
const networkRecords = loadData.networkRecords; | ||
|
||
driver.off('Audits.issueAdded', this._onIssueAdded); | ||
await driver.sendCommand('Audits.disable'); | ||
try { | ||
await driver.sendCommand('Audits.disable'); | ||
} catch (_) {} // Fails if Chrome is old. | ||
const artifact = { | ||
/** @type {Array<LH.Crdp.Audits.MixedContentIssueDetails>} */ | ||
mixedContent: [], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like "Allowed" may be confusing (why is Lighthouse marking it as a problem if it was allowed?) but I don't have a good alternative to suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wording wise its odd that the first column is "URL" and second column is "Resolution"
The resolution doesn't apply to that URL, but to the network request of that URL.
Adding "request" here I think helps both this and brendan's concern.
"Request allowed/blocked" ? "Request allowed with warning"?
btw here's an example of a warning case (insecure form submission): https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/loader/mixed_content_checker.cc;l=731-747;drc=5ba77dad98cf94506cf3700f9e12fcdc65fadfb6
"Request automatically upgraded to HTTPS"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not change column title
Request Resolution
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wfm
so:
Request Resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think
Request Resolution
:'Allowed'
still raises "why is Lighthouse marking it as a problem if it was allowed?" but it seems fine to wait to see if there's any actual user confusion before coming up with something new :)I only raise it because if it does come up in the future, I don't want to see us saying "oh, well it should just be allowed and not marked as a failure, then" and instead keep following @patrickhulce's reasoning in #10975 (comment).