https surfing: Clicking on a link early leads to "mixed/broken" lock icon

VERIFIED FIXED in psm2.3

Status

P2
normal
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

(Blocks: 1 bug)

Other Branch
psm2.3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM] [eta 6/24])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

17 years ago
It is possible to see a "broken/mixed security" lock icon and warning, when
surfing https pages, that are perfectly secure.

However, this does only occur, when one clicks a link, or presses "stop" or
"escape" while the page is still loading.

The reason for the wrong display comes from the fact, that for summarizing the
security info, we are currently analyzing the transfer status of the page's parts.

When the user interrupts loading too early, it is possible that loading of some
parts has begun, for whom no security information is yet available.

On stop, all those connections are stopped, too. The logic tries to extract
security information, but is unable to do so. Therefore it counts the objects
which were tried to load as being insecure. And having at least one insecure
object is enough for the summarized page status going to "mixed".

While this is a bug in behaviour, it is not a bug in code. Rather the code is
currently not trying to detect this kind of failure.

Expected behaviour: Interrupted loading of objects on a page should not lead to
mixed/broken security icon.

Suggested implementation:

For each loaded subpart of a displayed page, the code should take into
consideration two more properties:
a) the loading "stop reason"
b) the fact, whether content bytes have been transfered or not.

We have to prove, but actually I believe, that a) is not necessary, looking at
b) should be enough.

I suspect, security information will be unavailable for those connections, where
the SSL handshaking did not yet finish. Those connections did not yet transfer
any content data, and therefore could not lead to insecure data being displayed.
Therefore those connections should get ignored for determining page security status.

Therefore it should be enough to implement a "per-object flag" whether bytes
were received already or not yet at all. It has to be researched, whether web
progress notification (the mechanism used by PSM to track security state)
already delivers the information or not.
(Assignee)

Updated

17 years ago
Blocks: 130949

Comment 1

17 years ago
Kai.
Assignee: ssaux → kaie
Keywords: nsbeta1+
Priority: -- → P2
Whiteboard: [adt2 RTM]
Target Milestone: --- → 2.3
(Assignee)

Comment 2

17 years ago
I think I have a solution for this problem, my tests so far show that I have a
working fix.

Here are two test cases:
- https://www.kuix.de/misc/test16/something.html
- https://www.kuix.de/misc/test16/nothing.html

Both testcases are frame-documents. The left frame will not immediately, but
there will be a 10 second delay before the page is completely shown.


The first testcase "something" uses more text content in the left frame, there
will be some content shown in the page before the 10 second timeout is reached.

This testcase has already been working correctly. When you hit stop, we
correctly find security information for the frame, and the lock icon will
indicate a secure page.


The second testcase "nothing" will display nothing in the frame if you hit
cancel early.

Right now, and this is what this bug complains about, when you hit cancel before
the 10 second timeout, the page will go to a mixed/insecure state.

The reasons for the failure is indeed, as I explained in the previous comments,
that no data has been transfered over the channel yet, and we are unable to
retrieve security information.

But because there really has no data been transfered, there will nothing be
shown on the screen that could potentially been insecure. So we can ignore the
state of this frame, even though we are unable to obtain security information
for it.


I'll be attaching a patch that corrects the problem. For each part of the loaded
document, we remember whether data has been transfered or not, and ignore the
requests we don't care for.
Status: NEW → ASSIGNED
(Assignee)

Comment 3

17 years ago
Created attachment 85951 [details] [diff] [review]
Suggested Fix
(Assignee)

Comment 4

17 years ago
Created attachment 85952 [details] [diff] [review]
Same Patch, but ignoring whitespace changes for easier reviewing
(Assignee)

Comment 5

17 years ago
Javi, can you please review?

Comment 6

17 years ago
let's get this to bake on the trunk, have banks who reported being worried about
this bug give it a spin.  We should make sure that we review a build that
contains both this fix and the fix to bug 148610.
(Assignee)

Comment 7

17 years ago
Comment on attachment 85951 [details] [diff] [review]
Suggested Fix

I was told nsHashtable is not recommended for usage. I'll rework the patch to
use a better hashtable class.
Attachment #85951 - Flags: needs-work+
(Assignee)

Updated

17 years ago
Attachment #85952 - Attachment is obsolete: true
(Assignee)

Comment 8

17 years ago
More testing showed that my patch does not work correctly.

I assumed that I would see the STATE_TRANSFERRING state for any request part for
the loaded document.

But I don't see that request for subdocuments like CSS or JS documents loaded
over separate URLs. So I guess I have to use something else.
(Assignee)

Comment 9

17 years ago
Rick, do you have an idea? How could I query a nsIRequest at the time I receive
the STOP progress, whether any content bytes have been transfered or not?
(Assignee)

Updated

17 years ago
Depends on: 148981
(Assignee)

Comment 10

17 years ago
I tried several other approaches, also using OnProgress and OnStatus events.

For some reason, external stylesheets behave special, and we do not receive
progress information using either approach.

While I first thought, this would be a general behaviour of non-visual
subdocuments, I learned that referenced external JavaScript documents behave
correctly, we do receive the expected event for them.

I have filed bug 148981.


It would be nice to have a fix for 148981, in that case, the existing approach
in this bug would already work correctly.


I was thinking how we should behave for such documents, where we do not receive
progress information.

If we don't, we must decide whether we want to assume data has been transfered
or not over the channel.

If we assume there has not, but data has been actually transfered, and in fact
used an unsecure protocol, we would incorrectly display the page as being secure.

Or, if we always assume, data has been transfered, but we are unable to extract
the security info, and there has no data been transfered, then this will lead to
a mixed state icon, which effectively would mean, this bug is not fixed correctly.

Because of that, I think we should not make assumptions. If all else fails, we
don't find a solution for 148981, and we really want to fix this bug as good as
possible, then we should use the latter approach, and accept there are still
some cases that could lead to a broken icon.


One could argue: "Hey, it's only a style sheet, not content shown on the screen!
So let's ignore it".

I would argue against that: With a stylesheet it is possible to hide information
from a page. For example, a secure web page could display a warning. The hacked
non-securely transfered stylesheet could have rules to hide that warning.

Therefore I think we should not ignore the stylesheet.
(Assignee)

Comment 11

17 years ago
My previous comment might rise the impression, there is a partial solution, even
if bug 148981 is not fixed. Thinking about it more, I would like to restate,
there is no partial solution. The approach of this suggested patch only work, if
one can rely one STATE_TRANSFERRING events to arrive. If we can't rely on that,
we must assume the worst case, check for security information, and this bug will
not be fixed in any way.


To restate what our options are:

Either:
- Fix for 148981 and a statement from the owner of the document loader team,
that we can indeed rely on STATE_TRANSFERING objects to arive, for any request
where content bytes have been transfered

Or:
- Do not rely on that event. Instead, find a way to obtain the information, by
querying an object passed in to the progress listener, whether content bytes
have been transfered or not.

(Assignee)

Comment 12

17 years ago
Created attachment 86207 [details] [diff] [review]
Updated Patch / Only recommended with 148981 fixed
Attachment #85951 - Attachment is obsolete: true
(Assignee)

Comment 13

17 years ago
Created attachment 86208 [details] [diff] [review]
Updated Patch / ignoring whitespace for easier review / Only recommended with 148981 fixed
(Assignee)

Comment 14

17 years ago
The previous patch would make the redness go away, but it would be a regression
without having 148981 fixed. Because we would display a page as secure, even if
stylesheets (and potential other subdocuments) have been transfered without
security.
(Assignee)

Comment 15

17 years ago
*** Bug 125199 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

17 years ago
I'm dumping some thoughts about my patch (only of interest for those trying to
follow the implementation).

While a document gets loaded, I need to keep a list of all the requests that
were seen. The list gets cleared each time a new top level document loading starts.

For each request, I remember a bool, whether data has been transfered or not.
Actually, I'm simplifying, by only remembering those URLs, where data has
actually been transfered. The existence in the list defines that data has been
transfered for that request.

Loading a document can consist of a lot of requests.

Because I don't want to be responsible for a performance regression, or cause
other side effects by delaying the freeing of request objects, I made a
simplification in the previous version of my patch. I'm not storing references
to the objects, and I'm not storing URLs, I'm just storing the numerical pointer
values to the request objects. 

But when I did that, I blindly assumed that those pointer values would not get
reused while loading a document. But I think I should not assume that. If a
request finishes and gets deleted early, the address might get re-used.

I thought I would have to use a more complicated solution, but actually there is
a simple one: I don't need to remember an object longer than it's lifetime. I
know that I will receive a stop event before an object gets deleted. The
solution is: Remove the entry from the hash table, as soon as the state has been
looked up. That way, we only remember pointer variables during the lifetime of
request objects, and we are fine by making that simplification.

The next issue with my previous patch is that I limited the detection to
sub-documents only. However, as seen in bug 125199 - see also the testcase at
https://www.kuix.de/misc/test21/ - it makes sense to include the top level
document case.

I will attach a new patch that implements the above changes.
(Assignee)

Comment 17

17 years ago
Created attachment 88228 [details] [diff] [review]
Patch v3 / needs 148981
Attachment #86207 - Attachment is obsolete: true
(Assignee)

Comment 18

17 years ago
Created attachment 88229 [details] [diff] [review]
Patch v3 / ignoring whitespace / needs 148981
Attachment #86208 - Attachment is obsolete: true
(Assignee)

Comment 19

17 years ago
javi: Can you please review?

jkeiser: You asked me to use the PLD hash table. Can you please have a look
whether I'm using it correctly?

Thanks!

Comment 20

17 years ago
Comment on attachment 88228 [details] [diff] [review]
Patch v3 / needs 148981

r=javi
Attachment #88228 - Flags: review+
Looks good to me, except the ADD and REMOVE don't require a lookup first.  ADD
will  do a lookup automatically and REMOVE will just do nothing if the entry
isn't there.
(Assignee)

Comment 22

17 years ago
Created attachment 88393 [details] [diff] [review]
Patch v4 / needs 148981

Slightly updated patch, only removing the unnecessary lookup-before-add.

I think I can't remove the lookup-before-remove, because I need the information
whether the key was contained in the table or not.
Attachment #88228 - Attachment is obsolete: true
(Assignee)

Comment 23

17 years ago
Created attachment 88394 [details] [diff] [review]
Patch v4 / ignoring whitespace / needs 148981

Same patch, ignoring whitespace.
Attachment #88229 - Attachment is obsolete: true
(Assignee)

Comment 24

17 years ago
Comment on attachment 88393 [details] [diff] [review]
Patch v4 / needs 148981

carrying forward r=javi.
Attachment #88393 - Flags: review+
(Assignee)

Comment 25

17 years ago
I just retested, the patch from this bug, combined with the patch from bug
148981, also corrects the issues with german TOPSITE https://freemail.web.de, as
reported in duplicate bug 125199.

Comment 26

17 years ago
Comment on attachment 88394 [details] [diff] [review]
Patch v4 / ignoring whitespace / needs 148981

sr=rpotts@netscape.com
Attachment #88394 - Flags: superreview+

Updated

17 years ago
Whiteboard: [adt2 RTM] → [adt2 RTM] [eta 6/24]
(Assignee)

Comment 27

17 years ago
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Keywords: adt1.0.1, mozilla1.0.1
Resolution: --- → FIXED

Comment 28

17 years ago
junruh - can you pls verify this fix on the trunk, then resolve this bug as
verified? thanks!
Blocks: 143047

Comment 29

17 years ago
Verified on 6/21 trunk. The lock icon behavior has never been more accurate.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 30

17 years ago
Comment on attachment 88393 [details] [diff] [review]
Patch v4 / needs 148981

To avoid confusion during the approval phase, carrying over sr= from the
white-space-ignoring patch to this one.
Attachment #88393 - Flags: superreview+
(Assignee)

Comment 31

17 years ago
Comment on attachment 88394 [details] [diff] [review]
Patch v4 / ignoring whitespace / needs 148981

Marking this patch as obsolete, since it was only a helper for easier
reviewing.
Attachment #88394 - Attachment is obsolete: true

Comment 32

17 years ago
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers approval. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1 → adt1.0.1+, approval

Updated

17 years ago
Attachment #88393 - Flags: approval+

Comment 33

17 years ago
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
(Assignee)

Comment 34

17 years ago
Checked in to 1_0 branch.
Keywords: adt1.0.1+, mozilla1.0.1+ → fixed1.0.1

Comment 35

17 years ago
Verified on 6/25 branch.
Keywords: fixed1.0.1 → verified1.0.1
(Assignee)

Comment 36

17 years ago
*** Bug 100787 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 37

16 years ago
*** Bug 153962 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.