1305676 - In some circumstances passive mixed content indicator will override active mixed content indicator

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[:Gijs (he/him)]

http://searchfox.org/mozilla-central/rev/05ed82e50b45df5aa5a8fad219dece1b56757261/browser/themes/shared/identity-block/identity-block.inc.css#206-222

#urlbar[pageproxystate="valid"] > #identity-box.certUserOverridden > #connection-icon {
  list-style-image: var(--connection-icon-mixed-passive-loaded);
  visibility: visible;
}

#urlbar[pageproxystate="valid"] > #identity-box.insecureLoginForms > #connection-icon,
#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveContent > #connection-icon {
  list-style-image: var(--connection-icon-mixed-active-loaded);
  visibility: visible;
}

#urlbar[pageproxystate="valid"] > #identity-box.weakCipher > #connection-icon,
#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContent > #connection-icon,
#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContentLoadedActiveBlocked > #connection-icon {
  list-style-image: var(--connection-icon-mixed-passive-loaded);
  visibility: visible;
}


(bug 1304363 moves some of this into %defines instead of variables, in a different file, but the gist of it is the same, cf. http://hg.mozilla.org/integration/mozilla-inbound/file/c2596f38f83f/browser/themes/shared/identity-block/icons.inc.css#l46 )

This is confusing, because we're basically saying that when displaying the unmodified URL for the page (pageproxystate=valid), for the connection icon, use:

- the passive mixed content icon if the cert has been user-overridden,
- unless the page has mixed active content or insecure forms, in which case use the active mixed content icon,
- unless the page uses a weak cipher, has mixed display content or blocked active mixed display content (??) in which case use the passive mixed content icon again.

That doesn't make any sense.

I *think* all the selectors for the passive mixed content icon should come first (and be combined), followed by a selector for the active mixed content cases.

If that is somehow not right, there really really needs to be a comment in the CSS that explains this... odd... sequence of rules, why there are separate ones, and why they override each other in the way that they do.

Paolo or Johann, can you take a look at what should happen here? Seems you wrote the rules in question modulo the CSS refactoring that Dão has been doing.

Comment 1

[Johann Hofmann [:johannh]]

Yup, I'd spontaneously claim you're right about this. The original ordering might have been there from when we still had the green lock with the grey triangle and nobody cared to consolidate the rules. This shouldn't have any user-facing effect since I think .mixedActiveContent isn't set together with .mixedDisplayContent but we should definitely fix it. I can't come up with a scenario where the weaker security indicator should have priority.

Could be a good first bug?

Comment 2

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #1)
> Yup, I'd spontaneously claim you're right about this. The original ordering
> might have been there from when we still had the green lock with the grey
> triangle and nobody cared to consolidate the rules. This shouldn't have any
> user-facing effect since I think .mixedActiveContent isn't set together with
> .mixedDisplayContent

I went and looked for the code where this is set. AFAICT insecureLoginForms can be set in addition to any of the passive mixed content ones from the second selector, right?

> but we should definitely fix it. I can't come up with a
> scenario where the weaker security indicator should have priority.

WFM. Then we can just unify the latter passive icon block with the former. And yes, that could be a good first bug.

Comment 3

[Johann Hofmann [:johannh]]

> I went and looked for the code where this is set. AFAICT insecureLoginForms can be set in addition to any of the passive mixed > content ones from the second selector, right?

The only case where this could happen is when loading an HTTPS page that has mixed display content and a form with an HTTP action, so that .mixedDisplayContent overrides the .insecureLoginForms. However, we don't show a warning for this case right now (Bug 1231914) so I don't think this is a user-facing bug. Also, we know from telemetry that HTTP forms on HTTPS pages are virtually nonexistent.

Comment 4

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #1)
> The original ordering
> might have been there from when we still had the green lock with the grey
> triangle and nobody cared to consolidate the rules.

This might well be the case, I've found this early version with the same ordering when we had three different icons:

http://searchfox.org/mozilla-central/rev/7a511699aaef3a48f09639e83e6e35b8c401ea64/browser/themes/shared/identity-block/identity-block.inc.css#76-88

> I can't come up with a
> scenario where the weaker security indicator should have priority.

We have regression tests for the icon appearance, so it's easy to verify whether changing the order breaks any existing test case. The weakCipher rule should probably have lower priority than the others, I'm not sure how easy it is to add a test case for that.

Comment 5

[:Gijs (he/him)]

To fix this bug, you'd need to remove this rule and add its selectors:

https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/browser/themes/shared/identity-block/icons.inc.css#57-59

to this rule:

https://dxr.mozilla.org/mozilla-central/rev/7c576fe3279d87543f0a03b844eba7bc215e17f1/browser/themes/shared/identity-block/icons.inc.css#46-49

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

Kate would like to take a look at this one.

Comment 7

[katecastellano]

Attachment: passive-overrides-active.patch

Comment 8

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83fec92ad8ca

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8805776 [details] [diff] [review]
passive-overrides-active.patch

Kate, I think you forgot to flag Gijs for review. I just did that for you, I hope that was correct :)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8805776 [details] [diff] [review]
passive-overrides-active.patch

Review of attachment 8805776 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but see suggestions below.

::: browser/themes/shared/identity-block/icons.inc.css
@@ +35,3 @@
>    list-style-image: url(chrome://browser/skin/tracking-protection-16.svg#disabled@iconVariant@);
>  }
>  

The extra blank line here makes sense to me, separating the tracking protection styling from this block of connection-icon rules, so I would personally leave it.

@@ +44,5 @@
>  
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.certUserOverridden > #connection-icon@selectorSuffix@,
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.weakCipher > #connection-icon@selectorSuffix@,
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContent > #connection-icon@selectorSuffix@,
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContentLoadedActiveBlocked > #connection-icon@selectorSuffix@ {

Nit: if you add the moved selectors before the existing one you can avoid touching blame for the line with the original selector. Not sure how much it matters in this case.

Comment 11

[Johann Hofmann [:johannh]]

Kate, would you like to implement the two small suggestions by Gijs above and attach another patch? As Gijs mentioned they're not strictly required but it'd probably make sense to do.

Let me know if you have any questions!

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1305676 - Fix passive mixed content indicator overriding active mixed content indicator.

Review commit: https://reviewboard.mozilla.org/r/104958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104958/

Comment 13

[Johann Hofmann [:johannh]]

Since this was only a tiny change and I'd like to see this bug closed I updated and rebased it myself. :)

Comment 14

[Johann Hofmann [:johannh]]

Attachment: Fix passive mixed content indicator overriding active mixed content indicator

MozReview-Commit-ID: 2QnVMDVf2G1

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8827202 [details] [diff] [review]
Fix passive mixed content indicator overriding active mixed content indicator

Sorry for the mess, didn't realize this wasn't a reviewboard patch.

Comment 16

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efb28710026a883c4b384c50720393bcc87e3581
Bug 1305676 - Fix passive mixed content indicator overriding active mixed content indicator. r=Gijs

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb28710026a
Fix passive mixed content indicator overriding active mixed content indicator. r=Gijs

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/efb28710026a