-
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 4 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 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 || []}, | ||||||||||||||
}; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
@@ -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'}); | ||||||||||||||
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.
guh we really have to land #10779 :) |
||||||||||||||
assert.strictEqual(result.details.headings.length, 1); | ||||||||||||||
}); | ||||||||||||||
}); | ||||||||||||||
|
||||||||||||||
|
@@ -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'); | ||||||||||||||
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. super-nit: can be fancy and do
Suggested change
(or it could be 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 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'}}), | ||||||||||||||
|
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.
so does anyone hate this
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.
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 amixed Content
issue associated with them. Would it be terrible to give every entry a defaultresolution: undefined
then, since they'll (almost?) always have one anyways ≥ m84?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.
That's my expectation.
Also, I'll file an issue to follow up and remove the
isSecureRecord
part completely once M84 lands.sure.
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.
Also, I'll file an issue to follow up and
remove the isSecureRecord part completelymake the header always added once M84 lands.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.
cc #10976 some other m84 stuff.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
#11076