1334201 - Site Identity doorhanger says "Logins entered on this page could be compromised", for secure tab opened by insecure page

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR: (Same as in bug 1333687)
 1. Visit http://www.wedsure.com/  (note: HTTP)
 2. Click "Sign in" at top right of page
  --> A new tab gets opened, at https://service.rvnuccio.com/admin/auth/login
     (note: this new tab is HTTPS)
 3. Click the site identity button (at left of URL bar), and see what happens.

ACTUAL RESULTS:
The UI is self-contradictory.
 - The site has a green EV area showing a green lock, and the site identity doorhanger says "Secure Connection". (Hooray, security.)
 - BUT, inside the site identity doorhanger, there is an image of a *broken lock*, and there's a warning saying "Logins entered on this page could be compromised"

EXPECTED RESULTS:
- UI should be consistent about security state.
- And in particular, we probably shouldn't be showing the broken lock & "compromised" warning text in this scenario.

Comment 1

[Daniel Holbert [:dholbert]]

mozregression says this regressed from bug 1269568.

(Note: This is similar to 1329940, but that [fixed] bug is about the username/password fields, whereas this bug is about the doorhanger.  The doorhanger still shows the "ACTUAL RESULTS" noted in comment 0, in latest Nightly which has bug 1329940's fix.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug (with contradictory UI highlighted)

Comment 3

[Tanvi Vyas[:tanvi]]

[Tracking Requested - why for this release]: In-context insecure password warning feature goes out in FF 52.

The in-context warning also appears.

Kate or Jonathan, do you know how this is different than bug 1329940?  Can you dig into this?

Comment 4

[Jonathan Watt [:jwatt]]

Seems like there are a couple more places under toolkit that should maybe be converted:

https://dxr.mozilla.org/mozilla-central/search?q=isSecureContext+path%3A&redirect=false

Matt, do you agree these should also use isSecureContextIfOpenerIgnored? (Preferably encapsulated in a helper function somewhere, with the big comment that was added above the |let isSafePage| line in bug 1329940 moved to document that function.)

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> Seems like there are a couple more places under toolkit that should maybe be
> converted:
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=isSecureContext+path%3A&redirect=false

This query shows me not results.  Can you update the link?

Kate or Jonathan, would one of you be able to take this bug and fix it this week?

Comment 6

[Johann Hofmann [:johannh]]

Seems it's urgent to get this into 52, so I'll steal this :)

Leaving ni for Matt but I assume we'll just use isSecureContextIfOpenerIgnored for any site identity UI for now.

Comment 7

[Tanvi Vyas[:tanvi]]

I'd rather be more conservative (show the warning less often) than show the warning on pages where it could appear confusing.  We can always tighten things later, but if things are overly tight from the beginning and we don't modify the UX to communicate what's happening, it will lead to user confusion.

Comment 8

[Johann Hofmann [:johannh]]

Attachment: Bug 1334201 - Ignore window.opener for insecure form warnings.

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

Comment 9

[Johann Hofmann [:johannh]]

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

Comment 10

[Johann Hofmann [:johannh]]

Only a browser test for now since I'm running into the same limitations as in bug 1313005, which I'm still not sure how to solve. This time it's even trickier because we have to open an HTTPS page from an HTTP page.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8831971 [details]
Bug 1334201 - Ignore window.opener for insecure form warnings.

https://reviewboard.mozilla.org/r/108430/#review109654

Thanks. I thought we had this unified already when I reviewed Kate's patch.

Comment 12

[Jonathan Watt [:jwatt]]

LGTM FWIW.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/447b349f9143
Ignore window.opener for insecure form warnings. r=MattN

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/447b349f9143

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8831971 [details]
Bug 1334201 - Ignore window.opener for insecure form warnings.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1269568
[User impact if declined]: HTTPS pages that were opened from HTTP pages will be shown as insecure in the site identity panel even though there is no actual risk to the user
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: None (bug 1329940 was already uplifted)
[Is the change risky?]: No
[Why is the change risky/not risky?]: The bulk of the work was done in bug 1329940, this bug amends only a few lines where the new attribute was not replaced correctly
[String changes made/needed]: None

Comment 16

[Gerry Chang [:gchang]]

Hi Andrei,
Hello Loic, could you help verify if this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 17

[Andrei Vaida [:avaida]]

Forwarding this to Paul, who's taking care of security related bug fixes in iterations.

Comment 18

[Julien Cristau [:jcristau]]

Looks fixed for me with the steps from comment 0 in today's nightly.

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8831971 [details]
Bug 1334201 - Ignore window.opener for insecure form warnings.

remove doorhanger security warning for tabs with insecure opener, beta52+, aurora53+

Comment 20

[Johann Hofmann [:johannh]]

(In reply to Julien Cristau [:jcristau] from comment #18)
> Looks fixed for me with the steps from comment 0 in today's nightly.

Yes, I can confirm, thanks!

Comment 21

[Paul Silaghi, QA [:pauly]]

Reproduced on 54.0a1 (2017-02-01).
Verified fixed 54.0a1 (2017-02-02) Win 7, OS X 10.11.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b1e50873a30

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/80636f3bf46c

Comment 24

[Andrei Vaida [:avaida]]

Paul, could you please verify this on beta 52 as well?

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 53.0a2 (2017-02-06), 52b4 Win 7, Ubuntu 14.04, OS X 10.11.