Closed Bug 1305676 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Site Identity, defect, P3)

51 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: Gijs, Assigned: johannh, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [fxprivacy])

Attachments

(1 file, 2 obsolete files)

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.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jhofmann)
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?
Flags: needinfo?(jhofmann)
(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.
Flags: needinfo?(jhofmann)
> 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.
Flags: needinfo?(jhofmann)
(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.
Flags: needinfo?(paolo.mozmail)
Whiteboard: [fxprivacy]
No longer depends on: MixedContentBlocker
Keywords: good-first-bug
Priority: -- → P3
Kate would like to take a look at this one.
Assignee: nobody → katika2987
Attached patch passive-overrides-active.patch (obsolete) — Splinter Review
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 :)
Attachment #8805776 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8805776 - Flags: review?(gijskruitbosch+bugs) → review+
Status: NEW → ASSIGNED
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!
Flags: needinfo?(katika2987)
Since this was only a tiny change and I'd like to see this bug closed I updated and rebased it myself. :)
Flags: needinfo?(katika2987)
Assignee: katika2987 → jhofmann
Attachment #8805776 - Attachment is obsolete: true
Attachment #8827201 - Attachment is obsolete: true
Attachment #8827201 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8827202 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb28710026a883c4b384c50720393bcc87e3581
Bug 1305676 - Fix passive mixed content indicator overriding active mixed content indicator. r=Gijs
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb28710026a
Fix passive mixed content indicator overriding active mixed content indicator. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/efb28710026a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.