119969 - Confusing lock icon behavior when downloading a crl over ssl.

Status: RESOLVED WORKSFORME

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

Description

[Stephane Saux]

To reproduce:

1) load an insecure page (say bugzilla.mozilla.org). Lock icon is open.
2) copy and paste
https://www.thawte.com/cgi/lifecycle/getcrl.crl?skeyid=sd%84%e6m%7blI%bds%3d%5d%e0%d9%19%84%12Ip%a2
(or just clicking might also trigger the bug, I'm not sure).
3) you get a lock icon, even though the page displayed remains the old one.

enter a bug number and submit the form, you get the "leaving secure site" dialog
box.

What should the proper behavior be?  Is this even a bug?
The lock icon agrees with the URL box, but it's faulty to assume that the page
content displayed is secure.

Comment 1

[Stephane Saux]

Ok so if you click on the link (rather than copy and paste it in the url box),
the lock icon disagrees with both the content displayed and the url.

Comment 2

[[not reading bugmail]]

see also bug 118328.

Comment 3

[Kai Engert [:KaiE:]]

-> me

Comment 4

[Kai Engert [:KaiE:]]

The special case we have here is a URL that produces a document that will not be
shown in the content area, but will be handled by a separate content handler.

When I tested this special case, I saw we received notifications with the
additional flag 
  LOAD_RETARGETED_DOCUMENT_URI


I understand that a START + LOAD_DOCUMENT_URI notification will be sent, before
it is known that retargeting will happen.

However, is it safe to assume, 
- START + LOAD_DOCUMENT_URI
- no notifications for any sub content prior to:
- LOAD_RETARGETED_DOCUMENT_URI
- now notifications for sub content could arrive
  (although I'd ideally assume, they won't arrive in our window at all)

If we can assume the above, fixing this bug would be simple.
After having received the
  LOAD_RETARGETED_DOCUMENT_URI
we would simply ignore any events, until we see
  STOP + LOAD_DOCUMENT_URI

Well, at least we will have to ignore them for setting the lock icon.
We will however, still require to show some security state transition warnings,
if enabled.

However, if we are going to show security transition warnings, those warnings
must be treated as being "isolated" from the shown content (previous content of
the window), since the new URL doesn't replace it, and since we are not updating
the shown lock icon.

That is, while we show a warning, the next event that will happen in the
unchanged document window, will behave as if the CRL would have never been
loaded. The check, whether a transition is being made, will be based on the
security shown window content and the shown lock icon.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Kai, I'm not really up on the progress notifications angle of things yet... what
you describe sounds reasonable, but Darin or Adam should know more about it...

Comment 6

[Kai Engert [:KaiE:]]

While working on a related issues, I today had a chat with bz, and I learned
there is a much simpler approach to fixing this bug.

Please correct me, but I think the following is true:
- whenever a "toplevel" document gets loaded, 
  that causes different content to be shown in the content
  area, nsIWebProgressListener::OnLocationChange
  gets called

- the call to OnLocationChange will happen early in the process
  of loading the document, before the final STOP notifications
  are sent out to nsIWebProgressListener::OnStateChange

- when downloading a CRL (or a zip) that does get shown in the content
  are, OnLocationChange does not get called! (that's good)

With this new knowledge, I'm suggesting the following approach, which seems to
work for me (patch coming up):

- the lock icon tracking code already recognizes when a load starts
- whenever a new load starts, set a flag which says
  "we do not yet know whether the load will change the location of the
   shown document"
- when we received OnLocationChange,
  set that flag to "we now know this load will change the location"
- continue to track all security states
- when it is time to check for updating the lock icon,
  only change the lock icon and the state,
  if we know the location changes


Before we check this patch in, we should do careful testing.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 119460 [details] [diff] [review]
Patch v1

Sorry, this patch is too simple.

This patch would cause bug 154240 to appear again...

Comment 9

[Kai Engert [:KaiE:]]

This is now WORKSFORME (with Mozilla trunk).

Comment 10

[Kai Engert [:KaiE:]]

Oops, sorry, reopening!

Here is a new test case: (I copied a CRL from Thawte to my server to make it downlaodable using https)

https://kuix.de/misc/test49/BTClass1Individual.crl

Comment 11

[Kai Engert [:KaiE:]]

However, the latest patch in bug 62178 fixes this bug, I just tested.

Comment 12

[Kai Engert [:KaiE:]]

This is WORKSFORME.

- go to a plain text site
- go to https://kuix.de/misc/test49/BTClass1Individual.crl
- cancel the crl import

The initial page still is still shown as a plain page.

I remember this got fixed a while ago.