-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
fix: simpler https audit #1918
Conversation
it is nice to match Chrome's/devtools' notions of security, though, and not have to update to match when it changes (e.g. Would it make sense to still do We can definitely ditch all the timeout nonsense. That's from ancient times, and if we actually needed to worry about every driver call timing out, Lighthouse as a whole would be in a ton of trouble :) |
@@ -41,9 +43,25 @@ class HTTPS extends Audit { | |||
* @return {!AuditResult} | |||
*/ | |||
static audit(artifacts) { | |||
const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS]; | |||
const insecureRecords = networkRecords | |||
.filter(record => record.scheme === 'http' && record.domain !== 'localhost') |
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'd prefer to whitelist what we consider secure, rather than building up a blacklist.
check out https://www.chromium.org/Home/chromium-security/security-faq#TOC-Which-origins-are-secure-
i think we only want to do https/wss and localhost for now.
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.
done
lighthouse-core/test/runner-test.js
Outdated
@@ -46,7 +46,7 @@ describe('Runner', () => { | |||
const url = 'https://example.com'; | |||
const config = new Config({ | |||
audits: [ | |||
'is-on-https' | |||
'theme-color-meta' |
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.
im just about to nuke this guy. ;) can you use another
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.
😢
basically the |
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.
two nits, but otherwise im happy with this.
lighthouse-core/test/runner-test.js
Outdated
HTTPS: { | ||
value: true | ||
} | ||
ViewportDimensions: '#ffffff' |
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.
white dimensions? that's kinda fun.
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.
heh, this turns out this value is meaningless to tests then :)
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's not testing that the content-width
didn't throw an error, though, just that the artifacts make it through. Should still update the assertion value to something less confusing for the next reader
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.
done
let displayValue = ''; | ||
if (insecureRecords.length > 1) { | ||
displayValue = `${insecureRecords.length} insecure requests found`; | ||
} else if (insecureRecords.length === 1) { |
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.
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.
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.
What about one string to rule them all:
if (insecureRecords.length) {
displayValue = `Insecure requests: ${insecureRecords.length}`;
}
We should handle the pluralization though. #polish
lighthouse-core/test/runner-test.js
Outdated
@@ -153,17 +151,17 @@ describe('Runner', () => { | |||
const config = new Config({ | |||
audits: [ | |||
// requires the HTTPS artifact |
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.
ViewportDimensions
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.
done
lighthouse-core/test/runner-test.js
Outdated
HTTPS: { | ||
value: true | ||
} | ||
ViewportDimensions: '#ffffff' |
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's not testing that the content-width
didn't throw an error, though, just that the artifacts make it through. Should still update the assertion value to something less confusing for the next reader
fixes #1904
Moves the HTTPS audit to a simpler method just ensuring no requests were loaded over
http
more debuggable and informative to the user as well which URLs need to be https. Also considers localhost to be secure to fix #1175