148610 - Lock icon should be updated as early as possible

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Kai Engert [:KaiE:]]

Start by opening a standard page loaded over http.
Then go to a https address.

Actual behaviour:
=================
While the https page is being loaded, the lock icon will stay in its state until
the page has been fully loaded.

Go to https://www.kuix.de/misc/test16/nothing.html to see the problem.
That page uses a ten second delay before the contents will be fully delivered.
The lock icon stays in the unsecure state during the delay.

Expected behaviour:
===================
The lock icon should indicate the known security state as early as possible.
In the above test case, the lock icon should indicate the secure state before
the 10 second timeout is over.


It was argued in the past, what we are currently doing is ok, because we should
not show the new state until it is completely known.

I think we should go away from that approach.

I think it is reasonable to show the new security state as soon as data is being
transfered for a new page.

This new mode does not need to be perfect yet, it is ok if we show the best we
yet know.

While the page continues being loaded, we will continue to check what the state
of the loaded page is. If we detect a change, we can just change the UI again.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Suggested Fix

This patch seems to work for me.

Comment 2

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 3

[Stephane Saux]

We need to bake this on the trunk. I mark it adt1 RTM because any lock icon
issue is guranteed to touch everybody using this browser. The patch was
discussed with evangelism, and at least one large bank has raised the issue.

Comment 4

[bill]

What happens when an SSL site has content that's both SSL and non-SSL?  It
sounds like the icon would show "locked", then flip to "unlocked" once non-SSL
content was parsed.

Comment 5

[Javier Delgadillo]

Comment on attachment 85953 [details] [diff] [review]
Suggested Fix

r=javi

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Re: comment #4.  That behavior sounds reasonable to me.  It seems reasonable
to start by assuming that an https web page is/will-be secure, and so show
the lock as locked when we start loading the page.  And then if we find,
while loading the page, that there are insecure components, unlock the lock.

Communicator behaves much the same way as N6 does with respect to this issue.
There have been many complaints about this behavior over the years.  
In this case, I think that "that's how Communicator did it" is not a very 
good reaason for N6 to have the current lock behavior.

Of course, any change to the lock behavior is risky.  If someone undertakes
to make this change, the result must be thoroughly tested to ensure that 
we don't create new situations in which the lock state is wrong.

Comment 7

[Kai Engert [:KaiE:]]

shadow: What you assume is correct.

Comment 8

[Stephane Saux]

I still don't mind making this change on the trunk. Given that the current
behavior is the same as that on N4, and that changing this behavior is
inherently risky, I don't know if it's a good idea to put it on the branch.

If the behavior is validated on the trunk, and there's a strong push from
evangelism folks to include this change, then we will try and get it on the branch.

Comment 9

[selmer (gone)]

Still need sr=.  I agree with Stephane's comments about it being pretty late to
make a change like this.  I'd rather get beat up about showing the proper state
a little late than take chances at being beat up about showing the *wrong* state
(which would be a bona-fide pull-it-off-the-wire type of bug and could slip the
schedule.)

My recommendation would be to get this on the trunk and baking and then release
it in Buffy.

Comment 10

[Kai Engert [:KaiE:]]

Rick, can you please review?

Comment 11

[Kai Engert [:KaiE:]]

Please remember:

The lock icon does tell the user what happened in the past. If everything
received in the past, while loading the current page, was secure, why not show
that information?

The lock icon does not make any statement whether the future will be secure. It
only says, whether the data shown on the page is secure or not.

Loading a page can take a long time to complete. There are pages, that contain
some slow content, which can cause the user to look at a secure page, and we
display an insecure lock icon - and that is incorrect, too. Also remember users
on slow modems.


Please note there is not only "going from insecure to secure", but the much
worse "going from insecure to secure" scenario:


Imagine a user is looking at a secure web page.
Next the user clicks on an insecure link, and the insecure page begins to load.

If there is a delay in loading the insecure page, because of a stale image, we
are currently still showing the secure lock icon! The user might continue to
assume the shown page is secure, although it is not!

I think that's a good argument for updating the lock icon as early as possible.


I have prepared a demonstration.
Go to:
  https://www.kuix.de/misc/test18/
Accept the server certificate.
The lock icon will switch to secure.

Then lick the link. The next page will load from an insecure http connetion, the
word "hello" you are reading on screen will have been received insecurely.

This test page has sub content which will not load before a 60 second delay.
During that time, the lock icon will still indicate a secure page.

With the patch attached to this bug, the lock icon will immediately switch to
the insecure mode.


It was said, it is a bad thing to show the user a "bad security" icon, even
though everything on the current page has been received securely. It was said,
"it's better to wait to be sure".

But why wait when there has been nothing insecurely received yet?


Today's web content is mostly complex, and it is easy to come into a situation,
where one image loading from an advertisement server is stale. Imagine a banking
site where this happens. The user would always think the page is "not secure",
although nothing insecurely has been transfered.

In addition, as soon as something insecure will be received, we will switch the
lock icon, and if the user did not disable the warnings, he will even get
another warning dialog box, informing about the changed situation.

Comment 12

[Kai Engert [:KaiE:]]

I think this is a regression.

In previous versions of Mozilla (like 0.9.4 based, N6.2.3), the lock icon did
show incorrect states. However, it did switch the lock icon at an early time.
The test case I'm listing in the previous section causes the lock icon to switch
very early in that old version of Mozilla.

What we see now is a result of the new lock icon tracking implementation, as it
was done with the patch in bug 130949.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: More conservative patch, v2

To minimize risk in this patch, I'm no longer introducing a new point of time
to obtain the security state from the channel.

The basic change is: We set our internal security state more often.
- after the top level document has been loaded, the security state of that
document will be checked, and calls the already existent code to update the
security UI
- we no longer wait for all subdocuments to load completely, but every time a
subdocument has been loaded (or loading has been canceled), we check the state
for changes again.

Comment 14

[Javier Delgadillo]

Comment on attachment 87214 [details] [diff] [review]
More conservative patch, v2

r=javi

Comment 15

[rpotts (gone)]

Comment on attachment 87214 [details] [diff] [review]
More conservative patch, v2

sr=[e-mail]

Comment 16

[scottputterman]

Let's get this on the trunk.  junruh, can you verify this after it lands?

Comment 17

[Kai Engert [:KaiE:]]

Checked in to trunk, marking fixed.

Finally, my server is online again, so can now again actually access the
prepared testcases.

When verifying, you can compare the behaviour of the 4 testcases at
  https://www.kuix.de/misc/test19/
with previous and new builds.

Comment 18

[John Unruh]

Verified on 6/17 Win2000 trunk. The lock icon behavior looks good to me.

Comment 19

[Katsuhiko Momoi]

Is thi schange in the branch? I want to make sure that this gest into the
branch for the next commercial release. This is such an important change
that is being requsted by several banks in Japan. Thanks.

Comment 20

[Paul Wyskoczka]

Marking adt1.0.1+ on behalf of the adt for checkin to the 1.0 branch.  Please
get drivers approval before checking in.

Comment 21

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 22

[Kai Engert [:KaiE:]]

Checked in to 1_0 branch.

Comment 23

[Jaime Rodriguez, Jr.]

junruh - can you pls verify this fix on the 1.0 branch? thanks!

Comment 24

[John Unruh]

Verified on 6/20 branch.

Comment 25

[Kai Engert [:KaiE:]]

*** Bug 127210 has been marked as a duplicate of this bug. ***

Comment 26

[Boris Zbarsky [:bzbarsky]]

This patch caused regresion bug 154084, which makes navigation on some secure
sites completely unbearable.

Comment 27

[Kai Engert [:KaiE:]]

Indeed. Please see http://bugzilla.mozilla.org/show_bug.cgi?id=154084#c9 for the
explanation and the same bug for the fix.

Comment 28

[Kai Engert [:KaiE:]]

*** Bug 78832 has been marked as a duplicate of this bug. ***