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

connorjclark · 2020-06-16T18:12:04Z

node lighthouse-cli https://very.badssl.com/ --view

The mixed-content Issue won't be emitted until M84 (next month), so this new column is added as a progressive enhancement.

lighthouse-core/audits/is-on-https.js

paulirish · 2020-06-16T21:29:38Z

lighthouse-core/test/results/sample_v2.json

-      }
+      "score": null,
+      "scoreDisplayMode": "error",
+      "errorMessage": "Required InspectorIssues gatherer did not run."


hmmm

should we use optionalArtifacts again?
seems good.

but also we can add a stub to the artifacts fixtures

No, I'll just fix the sample artifacts.

don't think optional artifacts fits here. it's meant to allow flexibility in which gatherers can run today, based only on what gatherers are in the config. since InspectorIssues is in the default config, it won't benefit.

connorjclark · 2020-06-16T21:38:21Z

lighthouse-core/audits/is-on-https.js

      /** @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(


so does anyone hate this

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?

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.

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

cc #10976 some other m84 stuff.

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.

brendankenny · 2020-06-19T18:53:27Z

lighthouse-core/audits/is-on-https.js

      /** @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(


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?

lighthouse-core/audits/is-on-https.js

brendankenny · 2020-06-19T18:59:02Z

lighthouse-core/test/audits/is-on-https-test.js

@@ -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'});


result.extendedInfo

guh we really have to land #10779 :)

lighthouse-core/audits/is-on-https.js

lighthouse-core/test/audits/is-on-https-test.js

lighthouse-core/audits/is-on-https.js

brendankenny · 2020-06-19T19:15:52Z

lighthouse-core/test/audits/is-on-https-test.js

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


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?)

oh i missed the fancy

neat

brendankenny · 2020-06-19T19:23:18Z

oh, https://web.dev/is-on-https needs updating as well. Maybe there are devtools docs we can utilize for the resolution types?

cc @kaycebasques

connorjclark · 2020-06-19T19:36:42Z

the http jquery script doesn't emit a mixed-content issue ... I guess because the domain is localhost so it's not considered insecure.

This audit currently checks if the resource is on secure domain, but shouldn't we check if the finalURL is? if on localhost shouldn't this audit always pass?

brendankenny · 2020-06-19T19:43:07Z

This audit currently checks if the resource is on secure domain, but shouldn't we check if the finalURL is? so if on localhost, this audit always passes?

good question. It's possible this is intentional (you don't get penalized for loading resources from localhost because what can you do about that during local development, but you do get penalized developing in localhost but loading resources that will be insecure once you deploy your site into production), but I don't remember. @paulirish @patrickhulce?

If it is intentional we should make sure to write it down this time :)

patrickhulce · 2020-06-19T19:51:34Z

This audit currently checks if the resource is on secure domain, but shouldn't we check if the finalURL is? if on localhost shouldn't this audit always pass?

It's possible this is intentional (you don't get penalized for loading resources from localhost because what can you do about that during local development, but you do get penalized developing in localhost but loading resources that will be insecure once you deploy your site into production)

I don't understand these questions and I'm not sure what we would document being intentional so I'm guessing that things I thought were unambiguous about this audit aren't actually shared by anyone else 😆

I always thought that the is-on-https audit is about flagging requests the navigation is causing that aren't made to a secure origin. With that lens, it immediately follows that http://localhost shouldn't be flagged because they're made to a secure origin while any requests to a http://example.com are obviously not made to a secure origin just because they're requested by a page that's on localhost. Does that answer the questions above or am I still missing something?

brendankenny · 2020-06-19T19:56:50Z

Does that answer the questions above or am I still missing something?

No, I think that's it :) It does fall out as a natural consequence when thought about it that way, I guess it's just coming up now because the MixedContentIssues don't think that it's an issue.

connorjclark · 2020-06-19T20:36:21Z

Ok, thanks for framing the audit patrick. I think the issue was the assumption that mixed content issues would 1:1 replace this audit. That is clearly not the case.

So we now know that there are items we want to surface that wont have a mixed content issue. For resolution, let's just put allowed or something? I think we'll only have this edge case for localhost.

paulirish · 2020-07-07T22:11:20Z

lighthouse-core/audits/is-on-https.js

+  /** 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',


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"

paulirish · 2020-07-07T22:12:37Z

lighthouse-core/audits/is-on-https.js

      /** @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(


cc #10976 some other m84 stuff.

paulirish · 2020-07-07T22:16:59Z

lighthouse-core/gather/gatherers/inspector-issues.js

@@ -33,7 +33,9 @@ class InspectorIssues extends Gatherer {
  async beforePass(passContext) {
    const driver = passContext.driver;
    driver.on('Audits.issueAdded', this._onIssueAdded);
+    // try {


we can remove these all now tho?

It breaks appveyor rn :/

but yeah once #10976 lands we can set a min chrome for the is-on-https expectation.

paulirish · 2020-07-07T22:22:32Z

lighthouse-core/gather/gatherers/inspector-issues.js

-    await driver.sendCommand('Audits.enable');
+    try {
+      await driver.sendCommand('Audits.enable');
+    } catch (_) {} // Fails if Chrome is old.


"Fails if Chrome is older than mXX"

not a huge deal but my brain associates "old"/age with higher numbers, but here, older means lower. 😖

maybe we just stick to if Chrome is < m82 or w/e

paulirish

(marking)

brendankenny

LGTM!

maybe add a smoke test entry? dbw has a "resolution": "Allowed" for its 1 item

brendankenny · 2020-07-08T19:34:49Z

lighthouse-core/audits/is-on-https.js

+  /** 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',


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).

brendankenny · 2020-07-08T19:46:42Z

lighthouse-core/audits/is-on-https.js

      /** @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(


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.

brendankenny · 2020-07-08T19:47:52Z

lighthouse-core/audits/is-on-https.js

+      // (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;


Suggested change

if (!item.resolution) item.resolution = UIStrings.allowed;

if (!item.resolution) item.resolution = str_(UIStrings.allowed);

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

e69edce

connorjclark requested a review from a team as a code owner June 16, 2020 18:12

connorjclark requested review from paulirish and removed request for a team June 16, 2020 18:12

googlebot added the cla: yes label Jun 16, 2020

devtools-bot assigned paulirish Jun 16, 2020

devtools-bot added the waiting4reviewer label Jun 16, 2020

paulirish requested changes Jun 16, 2020

View reviewed changes

fix

b2c024c

vercel bot deployed to Preview June 16, 2020 21:37 View deployment

connorjclark commented Jun 16, 2020

View reviewed changes

connorjclark requested a review from paulirish June 17, 2020 01:40

connorjclark added 2 commits June 19, 2020 11:20

Merge remote-tracking branch 'origin/master' into https-mixed-content

a5884fe

fixtest

551f5d8

vercel bot deployed to Preview June 19, 2020 18:23 View deployment

brendankenny reviewed Jun 19, 2020

View reviewed changes

fix

8924930

vercel bot deployed to Preview June 19, 2020 19:18 View deployment

test

afc8539

vercel bot deployed to Preview June 19, 2020 19:26 View deployment

undf

5ac5c6a

vercel bot deployed to Preview June 19, 2020 19:28 View deployment

string mapping

d200103

vercel bot deployed to Preview June 19, 2020 22:54 View deployment

expect

25ab53e

vercel bot deployed to Preview June 24, 2020 21:18 View deployment

ugh

6969147

vercel bot deployed to Preview June 24, 2020 21:54 View deployment

ok

0c751b9

vercel bot deployed to Preview June 24, 2020 22:13 View deployment

hm

ad4b662

vercel bot deployed to Preview June 24, 2020 22:22 View deployment

dfsdfj

fc2f500

vercel bot deployed to Preview June 24, 2020 22:25 View deployment

ahh

bd8d131

vercel bot deployed to Preview June 24, 2020 22:41 View deployment

Merge remote-tracking branch 'origin/master' into https-mixed-content

2dae0c7

vercel bot deployed to Preview July 1, 2020 00:50 View deployment

paulirish removed the 6.1 label Jul 7, 2020

paulirish reviewed Jul 7, 2020

View reviewed changes

paulirish requested changes Jul 7, 2020

View reviewed changes

rm try

ed7d921

vercel bot deployed to Preview July 7, 2020 23:29 View deployment

column

180e96e

vercel bot deployed to Preview July 7, 2020 23:54 View deployment

paulirish approved these changes Jul 8, 2020

View reviewed changes

brendankenny approved these changes Jul 8, 2020

View reviewed changes

pr

a46830a

vercel bot deployed to Preview July 9, 2020 15:30 View deployment

sample

8a55597

vercel bot deployed to Preview July 9, 2020 15:54 View deployment

connorjclark merged commit 5acd30f into master Jul 10, 2020

connorjclark deleted the https-mixed-content branch July 10, 2020 23:31

connorjclark mentioned this pull request Jul 10, 2020

Alignment with new DevTools Mixed Content issues #10615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

connorjclark commented Jun 16, 2020

paulirish Jun 16, 2020

connorjclark Jun 16, 2020 •

edited

Loading

connorjclark Jun 16, 2020

brendankenny Jun 19, 2020

connorjclark Jun 19, 2020

connorjclark Jun 24, 2020

paulirish Jul 7, 2020

brendankenny Jul 8, 2020

connorjclark Jul 9, 2020

brendankenny Jun 19, 2020

brendankenny Jun 19, 2020

brendankenny Jun 19, 2020

connorjclark Jun 24, 2020

brendankenny commented Jun 19, 2020

connorjclark commented Jun 19, 2020 •

edited

Loading

brendankenny commented Jun 19, 2020

patrickhulce commented Jun 19, 2020

brendankenny commented Jun 19, 2020

connorjclark commented Jun 19, 2020

paulirish Jul 7, 2020

paulirish Jul 7, 2020

paulirish Jul 7, 2020

connorjclark Jul 7, 2020

paulirish Jul 7, 2020

paulirish left a comment

brendankenny left a comment

brendankenny Jul 8, 2020

brendankenny Jul 8, 2020

brendankenny Jul 8, 2020

connorjclark Jul 8, 2020

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

	if (!item.resolution) item.resolution = UIStrings.allowed;
	if (!item.resolution) item.resolution = str_(UIStrings.allowed);

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

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

Conversation

connorjclark commented Jun 16, 2020

Choose a reason for hiding this comment

connorjclark Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 19, 2020

connorjclark commented Jun 19, 2020 • edited Loading

brendankenny commented Jun 19, 2020

patrickhulce commented Jun 19, 2020

brendankenny commented Jun 19, 2020

connorjclark commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jun 16, 2020 •

edited

Loading

connorjclark commented Jun 19, 2020 •

edited

Loading