1674341 - Test SiteIdentity when auto-upgrading mixed content

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P2)

Description

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Within Bug 1633743 we are considering to auto upgrade mixed content (image, audio, video) which in turn renders the doorhanger obsolete. We have the pref security.mixed_content.upgrade_display_content and I think it's best if we just hide the doorhanger behind that pref.

If true -> disable doorhanger
If false -> leave things unchanged (as currently in the tree)

Comment 1

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Attachment: bug_1674341_mixed_content_ui.png

Evaluating all the code in question, I don't even think it's necessary to bind the UI on the pref because the page in general appears as secure with no insecure display content loaded if we are upgrading all display content to use HTTPS -> see attached screenshot.

The reason for that is, if the pref security_mixed_content_upgrade_display_content is set to true, then the MixedContentBlocker already accepts the load with the promise all content will be upgraded to https and not recording any mixed content state (nsIWebProgressListener::STATE_LOADED_MIXED_DISPLAY_CONTENT) which would later cause the UI to appear in the first place.

Johann, I think we can mark this bug as a WONTFIX in the end. Or is there anything I am missing?

Comment 2

[Johann Hofmann [:johannh]]

Yes, that seems like a correct analysis, no UI should appear in this case, which seems fine. Out of interest, what happens when upgrading is not possible? Are we blocking the load then or allowing the mixed content? Does the right UI show in that case?

Comment 3

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

(In reply to Johann Hofmann [:johannh] from comment #2)

Yes, that seems like a correct analysis, no UI should appear in this case, which seems fine. Out of interest, what happens when upgrading is not possible? Are we blocking the load then or allowing the mixed content? Does the right UI show in that case?

Basically we are just rewriting the URL from http: to https:; if loading works, then all good, if not, then you would have a broken image. There is no fallback to http: and the UI wouldn't be updated in any way.

I am currently working on a test which flips the pref and (a) ensures the is mixed content site identity state, and (b) not update to the site identity state in case we are auto upgrading. So this bug is not quite a WONTFIX - let's land that test in here.

Comment 4

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

Attachment: Bug 1674341: Test SiteIdentity when auto-upgrading mixed content.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)

(In reply to Johann Hofmann [:johannh] from comment #2)

Yes, that seems like a correct analysis, no UI should appear in this case, which seems fine. Out of interest, what happens when upgrading is not possible? Are we blocking the load then or allowing the mixed content? Does the right UI show in that case?

Basically we are just rewriting the URL from http: to https:; if loading works, then all good, if not, then you would have a broken image. There is no fallback to http: and the UI wouldn't be updated in any way.

Currently we allow the user to opt out of breaking mixed active content per-site (go to https://mixed-script.badssl.com/ or https://very.badssl.com/ and click the lock icon), but apparently that doesn't work for mixed passive auto-upgrade? That feels a bit strange to me.

We know from Telemetry a while back that this UI is not used a lot. We should decide to either remove it altogether or integrate it with mixed passive content. I think it depends on how much breakage we expect from upgrading mixed passive.

Do you think it's worth adapting our existing mixed content exceptions to also cover breaking passive mixed content?

Comment 6

[Christoph Kerschbaumer [:ckerschb, back Sept 8th]]

(In reply to Johann Hofmann [:johannh] from comment #5)

Currently we allow the user to opt out of breaking mixed active content per-site (go to https://mixed-script.badssl.com/ or https://very.badssl.com/ and click the lock icon), but apparently that doesn't work for mixed passive auto-upgrade? That feels a bit strange to me.

Your assessment is correct. That UI does not touch the passive upgrading mechanism - from a technical perspective we could integrate that of course - the questions is - do we want that?

We know from Telemetry a while back that this UI is not used a lot. We should decide to either remove it altogether or integrate it with mixed passive content.

Personally I am for removing that UI alltogether and eliminating complexity.

Do you think it's worth adapting our existing mixed content exceptions to also cover breaking passive mixed content?

There is definitely breakge from auto-upgrading mixed content where end users will experience a broken image and there is not way they can opt out of that at the moment. From that perspective it would be good to have some sort of opt-out mechanism for end users.

What's your take?

Comment 7

[Johann Hofmann [:johannh]]

Yes, so, I think this UI is probably not very useful for mixed active content right now and could be removed. I'm also a bit skeptical whether, in its current form, it will have any impact on user recovery from auto-upgrading breakage. It's very hidden and generally this is hard to explain to users.

However, the good (and bad) thing about having any sort of UI, even hidden, is that users can get directions from others on how to disable the protection to fix e.g. sites that are slow to adapt to the change. If other browsers allow this then we need to have it, to guard against "to view images on this site, first switch to Chrome, then click Settings, etc.".

As an alternative we could show contextual hints (i.e. placeholders for removed images) that allow the user to opt-out when they can't see an image. This has the problem that not all breakage is "I can't see this image", it could also be around onload handlers not firing etc.

Overall I feel like we should probably wire up upgrading with the exception mechanism, if only to give Nightly users an opt out mechanism. This could also support future UI explorations.

But I know you wanted to file a separate bug for UI for auto-upgrading so we should probably discuss it there.

We'll definitely also need good enterprise policy support for this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79932a0ca8a7
Test SiteIdentity when auto-upgrading mixed content. r=johannh

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/79932a0ca8a7