Closed Bug 358438 Opened 15 years ago Closed 14 years ago

Mixed content site incorrectly reported as being secure on back/forward

Categories

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

x86
Windows XP

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: help, Assigned: mayhemer)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [sg:moderate spoof] [kerh-bra] mixed content looks secure)

Attachments

(5 files, 10 obsolete files)

9.61 KB, text/plain
Details
4.02 KB, text/plain
Details
30.98 KB, patch
Details | Diff | Splinter Review
30.45 KB, patch
mayhemer
: review+
rrelyea
: superreview+
Details | Diff | Splinter Review
32.31 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0

When you visit a site that has not got a valid certificate and the padlock in the address bar is open and "Contains Unauthenticated Content". If you go somewhere else and then click back, the bar is now yellow and locked padlock suggesting site ok. This is incorrect.

Reproducible: Always

Steps to Reproduce:
1. Go to https://www.madmash.com/ - Unlocked Padlock in Bar
2. Go to http://news.bbc.co.uk or whatever
3. Click BACK button, Locked padlock in bar.

Actual Results:  
Padlock locked.

Expected Results:  
Padlock still unlocked.
Flags: blocking1.9?
Flags: blocking1.8.1.1?
I don't get an "open" padlock, I get a slashed one indicating "mixed" content. When i go to an SSL site and then hit back I get an initial padlock that switches to mixed content when some of the non-SSL images render.

Non-SSL sites have no padlock at all, there should never be an "unlocked" icon ever in Firefox.
Whiteboard: [sg:needinfo]
apologies - dveditz@cruzio.com is correct.
Marking INVALID based on comment #2.

Gerv
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → INVALID
whoa, pardner! We clarified some terminology, but both the reporter and I agree on the symptoms, and the symptoms are a problem:

1) on a mixed content site (we have plans to kill this state)
2) go elsewhere, hit back
3) same content as 1, but no longer showing the "mixed" state.

I'm fairly sure this is because we switch into mixed state based on loading insecure content. When we hit back, however, we now load the content from the bfcache -- we don't "load" the insecure content because we already loaded and incorporated it into the saved (cached) content.

Not sure whether to blame security UI or the bfcache code (we should save the security state in the cache, probably). Another approach is to move forward with the eventual plan to kill off mixed content, and then this case doesn't even come up.

I don't think either fix is realistic for 1.8.1.1
Status: RESOLVED → UNCONFIRMED
Component: Security → Security: UI
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Keywords: regression
Product: Firefox → Core
Resolution: INVALID → ---
Whiteboard: [sg:needinfo] → [sg:low spoof] mixed content looks secure
Version: unspecified → 1.8 Branch
Confirming. Dan, thanks for your analysis.
Assignee: nobody → kengert
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #4)
> Not sure whether to blame security UI or the bfcache code (we should save the
> security state in the cache, probably). 

This is a regression that was introduced by changes outside of the security UI code.
In my testing, the lock icon tracking code does no longer receive events about loading the inner content of the page. None for JS, none for CSS.

As you raise the blame question: I'd blame the change that introduced new document loading, without ensuring that the web progress notification messages still worked as they were before.


> Another approach is to move forward
> with the eventual plan to kill off mixed content, and then this case doesn't
> even come up.

This will not be sufficient. Even in that scenario we'll need to be able to detect that insecure content and block / hide it.
For testing I used:
- go to https://kuix.de/misc/test17/index3.php
- go to about:blank
- go back

I used NSPR_LOG_MODULES="nsSecureBrowserUI:5" to capture the events we get on initial and second loading of that page.

From the attached files you can see, the second time the security UI tracking code gets much less tracking information.

We used to get the same amount of notifications in both scenarios.
This regression probably has been caused by changed around bug 292971.

With the new caching, and the new lack of progress notifications, it appears, we never go into the mixed-mode state when navigating through history.

Looking at the checkins made to file nsSecureBrowserUI.h, you had initially attempted to store the security state and restore the state when navigating. But you backed it out without any solution. Was this an oversight?

I suspect it was not an oversight. In bug 292971 comment 8 Brian said:
> Reverted the changes to the PasswordManager and SecureBrowserUI classes that
> were needed to account for lack of WebProgressListener notifications.

Did you believe the notifications are completely working?

While we get some notifications for the topmost document, we do not seem to get any events for the inner contents, which would be necessary to detect the mixed state.
Blocks: lockicon
Summary: padlock comes up when site previously had open padlock by clicking back onto page → Mixed content site incorrectly reported as being secure on back/forward
Whiteboard: [sg:low spoof] mixed content looks secure → [sg:low spoof] [kerh-bra] mixed content looks secure
raising severity to moderate, malicious injected code could easily use this to hide the fact that it's insecure.
 1 - load page in "mixed" state due to evil code
 2 - evil code loads a page under its control (either its own
     or any random site with an XSS flaw)
 3 - the controlled page issues a back()
 4 - profit! (original hacked page now looks secure)

Who owns bfcache now?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Whiteboard: [sg:low spoof] [kerh-bra] mixed content looks secure → [sg:moderate spoof] [kerh-bra] mixed content looks secure
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
I think it's unfortunate this functional regression in security has never been addressed. The original code should have been backed out.

I was reminded about this bug when I heard about new bug 390168.

As of today, I can not reproduce this one. But it seems likely to be hidden by the new bug 390168.
let's re-test this bug after the new regression bug 390168 got fixed.
Depends on: 390168
I found a simpler test which proves we still have this bug.

- go to https://kuix.de/misc/test17/insecure-script-and-css.php

- notice mixed lock icon
  (why does Firefox show a yellow bar? SeaMonkey does not, which is better)

- go to some other page, click the link

- click back

You get a secure lock icon.
Bug is still confirmed.
Duplicate of this bug: 368837
Seems pointless to do EV if we're going to have bugs like this that let sites circumvent it...
Flags: blocking1.9?
Marking as blocking.  Can still reproduce this on trunk.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Still reproducible on trunk.  Have we been able to make any progress toward a fix?
Kai, any chance that 402574 offers any insight here?  It's certainly pretty undesirable behaviour.  :)
Flags: tracking1.9+ → wanted1.9.0.x+
(In reply to comment #19)
> Kai, any chance that 402574 offers any insight here?

No. Bug 402574 was a trivial bug in state tracking.

But this bug/regression exists, because the networking backend no longer sends the notification the security tracking relies on.

See comment 8 and comment 9.
I'm replacing the old test
  ( https://kuix.de/misc/test17/insecure-script-and-css.php )
with a more impressive one.

Go to https://kuix.de/misc/test17/358438.php

For a short second the page is shown with mixed content.
Then it goes to fully secure.
I notice, when going back, the web progress notification contains a new flag, STATE_RESTORING.

Is there any way PSM could stick the "mixed content" information into the cache?

Or alternatively, is there any way that PSM could iterate over the element objects that are associated to the cached page, and extract security information for each of them, and go to mixed mode if that fails?

(Well, the second proposal might not work, because we'd fail to extract security information for images, probably. Because we don't detect insecure images on page loading, going back would report more pages (correctly) as having a mixed content.)
If you see mixed content for a given origin, you might as well continue to show the mixed content indication because if an attacker substitutes a malicious script in place of the insecure script, he can script/control any page in that origin.  See Bug 403590.

If you remember the mixed content information per origin, there isn't any need to iterate over the objects again, and back/forward will work correctly.
> Is there any way PSM could stick the "mixed content" information into the
> cache?

Do you have a cache entry descriptor?  You can store arbitrary metadata on those...

Alternately, could you store something on the security info?
I still haven't been able to focus on this bug.

I'm adding Honza to the CC list, who has recently started to work on PSM and told me he is looking for priority bugs he could help with. I discussed this bug with him, and he is willing to give it a try.

Honza, this might be tricky to solve, so thanks a lot for your attempt!

Answering Boris' comment 24, I'm not aware of cache entry descriptors (to the bfcache), no idea where I could get them from.

You also asked whether I could store something in the security info. Do you think the security info gets serialized into the bfcache? I assume it does not. Well, at least the combined state gets calculated by nsSecureBrowserUIImpl each time you go backward/forward.
The security info hangs off the document channel, doesn't it?
Bad news: I cannot read the cache entry when the page is being restored from bfcache because the cache entry inside the channel was already closed and released. It is loaded (and available) only when the channel is open for network access between OnStart and OnStopRequest. But during fast back load the channel is passed just in inactive form for compatibility. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp#5358 

I can load the cache entry directly from the cache session (but it is IMHO little bit misplaced to be called from the security UI).

Other way is to cache this inside of the security UI mapped to the URI and hold this during life time of the session - same as bfcache life time is. It is probably better way then storing this to the cache entry that is (even more) persistent among sessions.

BTW: I would like to ask why the particular notification for subcontent were actually removed? Is there any concrete reference why this were done?

FYI: This bug is irreproducible with Firebug 1.1.0b10. From the security UI logs it seems the bfcache is turned off when Firebug is enabled although they claim bfcache is untouched: http://www.getfirebug.com/relnotes.html, Beta 9, #2. I will check this with Firebug developers.

FYI2: This bug is irreproducible with this URL: https://www1.mail.volny.cz/
I will check why, maybe it can help us.

For now I am going to dive deeper to the bfcache code and see if we can get some notification that will not break anything but give the complete information.
Assignee: kengert → honzab
> Other way is to cache this inside of the security UI

Or in the channel's securityinfo, right?

> I would like to ask why the particular notification for subcontent were
> actually removed?

Because the whole point of bfcache is that the subcontent is not actually loaded again.  Which means there is nothing to send the notifications...

> see if we can get some notification that will not break anything

That would be much much more difficult than caching the "final" security state in the security info, I suspect.
Attached patch Draft fix 1 (obsolete) — Splinter Review
This is first working draft. I changed nsITransportSecurityInfo interface to make the securityState attribute mutable. I override the value when notifying UI and next time on reload this new "final" value is read from there. 

I am not sure this is the right way, but it seems to me this is the most correct way to store the info, anyway.

Also I am not sure where to throw away (and if even to) reference to a top level security info. I have tested there is no leak when reference is released just this way.

Tested with test cases at comment 0, comment 8 and comment 21.
Attachment #308626 - Flags: review?(kengert)
Attachment #308626 - Flags: review?(bzbarsky)
I don't really know most of this code well enough to review.

I do think that the public API method should only allow downgrading the security state, as a safety precaution.  Right now it seems to also allow upgrading it.
not limited to 1.8 branch
Version: 1.8 Branch → Trunk
Attached patch Final fix 1 (obsolete) — Splinter Review
I added check to allow only downgrade.

- When state is SECURED we may set only *exactly same strength* of SECURED. Other options are BROKEN or INSECURE, security strength bits will then be discarded.
- When state is BROKEN then just BROKEN and INSECURE might be set. Security strength bits will be discarded.
- When state is INSECURE then just INSECURE might be set. Security strength bits will be discarded.
Attachment #308626 - Attachment is obsolete: true
Attachment #309083 - Flags: review?(kengert)
Attachment #308626 - Flags: review?(kengert)
Attachment #308626 - Flags: review?(bzbarsky)
Attached patch Final fix 1.1 (obsolete) — Splinter Review
Maybe more human-readable logic of the restriction code.
Attachment #309083 - Attachment is obsolete: true
Attachment #309084 - Flags: review?(kengert)
Attachment #309083 - Flags: review?(kengert)
I think you are on the right path.

Your logic seems simple and straight forward.

However; it looks like you didn't discard the security bits for:
case nsIWebProgressListener::STATE_IS_INSECURE:
(unless that isn't needed or is already handled in other code)
(In reply to comment #34)
> I think you are on the right path.
> 
> Your logic seems simple and straight forward.
> 
> However; it looks like you didn't discard the security bits for:
> case nsIWebProgressListener::STATE_IS_INSECURE:
> (unless that isn't needed or is already handled in other code)
> 

I expect that in that case there are already no level flags set. The only place the flags could be set is through SetSecurityState module-only public method. If we really want to be clear and do not trust our self :) then the check should additionally be made right there.
Status: NEW → ASSIGNED
If it's guaranteed to be true, it's probably worth an assertion....
Attached patch Final fix 1.2 (obsolete) — Splinter Review
Attachment #309084 - Attachment is obsolete: true
Attachment #310045 - Flags: review?(kengert)
Attachment #309084 - Flags: review?(kengert)
Comment on attachment 310045 [details] [diff] [review]
Final fix 1.2

Honza, good work!

I must confess, it took me quite a while to understand WHY exactly this patch is working.

I'm learning:
- you rely on the fact that the channels associated to the load requests store the original security state bits. I guess this works because the cache serializes the state, I remember such code got added last year.

- instead of storing a downgraded state only in the temporary state of nsSecureBrowserUI, you permanently store a downgraded state for the toplevel document in the channel.


I have several comments to improve the patch, in the hope it still works as intended.

I notice that fix v1.2 causes us to call the downgrade function many times. I think we should limit it to contexts where a downgrade is possible. (We should not call it each time we get a location change notification for a page element.)


I also propose some more comments, the use of locks for the member variables, and to simplify the downgrade function itself.


>Index: minimo/components/ssl/nsNativeSSL.cpp
> 
>+NS_IMETHODIMP
>+nsWINCESSLSocketInfo::DowngradeSecurityState(PRUint32 aNewStatus)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

It seems that your implementation of DowngradeSecurityState depends only on
the parameter and on member variable mSecurityState, which is available
in minimo, too. So for completeness, once we're done, we should consider
to simply duplicate that code in the minimo sources, too.


>Index: netwerk/socket/base/nsITransportSecurityInfo.idl
>===================================================================
> 
>-[scriptable, uuid(0d0a6b62-d4a9-402e-a197-6bc6e358fec9)]
>+[scriptable, uuid(F0B4F0A9-02F9-41d8-9658-88814EA819A3)]
> interface nsITransportSecurityInfo : nsISupports {
>     readonly attribute unsigned long    securityState;
>     readonly attribute wstring          shortSecurityDescription;
>     readonly attribute wstring          errorMessage;
>+
>+    void downgradeSecurityState(in unsigned long aNewState);
> };


I think the function to downgrade the state must be able to update all state members, including error message and description.

With the test case, and using "mouse over" the broken lock icon, I still get the message "authenticated by <CA>". When we downgrade to mixed state, we should update the information strings to the usual "contains unauthenticated content" popup message.

We could postpone this fix to a later time, however, we should make sure we at least allow the interface to support it. I propose we allow 3 parameters to the function, and allow "zero length strings" to indicate "keep strings unchanged" (for now)


>+  PRUint32 securityState = mSecurityState & securityStateMask;
>+  PRUint32 newState = aNewState & securityStateMask;
>+
>+  switch (securityState)
>+  {
>+    case nsIWebProgressListener::STATE_IS_SECURE:
>+      if (newState == nsIWebProgressListener::STATE_IS_SECURE) {

Now you know that both securityState and newState are equal to STATE_IS_SECURE, so the following check will never be true, right?


>+        if (aNewState != mSecurityState)
>+          return NS_ERROR_NOT_AVAILABLE;


I think we don't need to return a failure in this function, unless you know a good reason I propose to always return NS_OK.
Attachment #310045 - Flags: review?(kengert) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Comment on attachment 310321 [details] [diff] [review]
Patch v2

Honza, what do you think about my changes to your patch?
Attachment #310321 - Flags: review?(honzab)
Attached patch 1.8 branch patch (obsolete) — Splinter Review
I was curious whether the same fix works on the 1.8 branch in Firefox 2, too.
It was necessary to backport the patch.
It seems to work.
(In reply to comment #38)
> - you rely on the fact that the channels associated to the load requests store
> the original security state bits. I guess this works because the cache
> serializes the state, I remember such code got added last year.

bfcache is not the same as the normal cache. also, before that serialization fix last year, we didn't store SSL pages on disk, and in memory we just kept the original securityState object, so modifications to it will be kept.
Comment on attachment 310321 [details] [diff] [review]
Patch v2

(In reply to comment #38)
> (From update of attachment 310045 [details] [diff] [review])
> - you rely on the fact that the channels associated to the load requests store
> the original security state bits. I guess this works because the cache
> serializes the state, I remember such code got added last year.
> 
> - instead of storing a downgraded state only in the temporary state of
> nsSecureBrowserUI, you permanently store a downgraded state for the toplevel
> document in the channel.
> 

Exactly. And with the bfcache assistance the channel is held and passed to callbacks during restore. Channel then keeps flags as long as bfcache holds the page (exactly what we need).

> I notice that fix v1.2 causes us to call the downgrade function many times. I
> think we should limit it to contexts where a downgrade is possible. (We should
> not call it each time we get a location change notification for a page
> element.)
> 

Agree but IMHO it is not a big performance problem.

> 
> I also propose some more comments, the use of locks for the member variables,
> and to simplify the downgrade function itself.
> 

I forget about any synchronization, right :)

> 
> >Index: minimo/components/ssl/nsNativeSSL.cpp
> > 
> >+NS_IMETHODIMP
> >+nsWINCESSLSocketInfo::DowngradeSecurityState(PRUint32 aNewStatus)
> >+{
> >+  return NS_ERROR_NOT_IMPLEMENTED;
> >+}
> 
> It seems that your implementation of DowngradeSecurityState depends only on
> the parameter and on member variable mSecurityState, which is available
> in minimo, too. So for completeness, once we're done, we should consider
> to simply duplicate that code in the minimo sources, too.
> 

Yes, agree.

> 
> >Index: netwerk/socket/base/nsITransportSecurityInfo.idl
> >===================================================================
> > 
> >-[scriptable, uuid(0d0a6b62-d4a9-402e-a197-6bc6e358fec9)]
> >+[scriptable, uuid(F0B4F0A9-02F9-41d8-9658-88814EA819A3)]
> > interface nsITransportSecurityInfo : nsISupports {
> >     readonly attribute unsigned long    securityState;
> >     readonly attribute wstring          shortSecurityDescription;
> >     readonly attribute wstring          errorMessage;
> >+
> >+    void downgradeSecurityState(in unsigned long aNewState);
> > };
> 
> 
> I think the function to downgrade the state must be able to update all state
> members, including error message and description.
> 
> With the test case, and using "mouse over" the broken lock icon, I still get
> the message "authenticated by <CA>". When we downgrade to mixed state, we
> should update the information strings to the usual "contains unauthenticated
> content" popup message.
> 

Thanks for this hit! It is very important. The problem isn't raising because we don't clear the message. It is build by nsSecureBrowserUIImpl::GetTooltipText that returns "SecurityButtonMixedContentTooltipText" text when state is exactly lis_mixed_security. 

BUT: on restore (when navigating back) the state is lis_broken_security because the top level document is 'insecure'. Then security state gets to lis_broken_security instead of lis_mixed_security, see nsSecureBrowserUIImpl::UpdateMyFlags, if (mNewToplevelSecurityState & STATE_IS_BROKEN) branch.

Thus, this whole patch and concept is probably completely wrong because we produce incorrect security state on restore.

Personally I tend to cache the state in nsSecureBrowserUIImpl using hash table. It is probably the only way we can do it correctly w/o any magic to fit into the current code, change API and so on.

Not sure other comments are relevant anymore...

> We could postpone this fix to a later time, however, we should make sure we at
> least allow the interface to support it. I propose we allow 3 parameters to the
> function, and allow "zero length strings" to indicate "keep strings unchanged"
> (for now)
> 
> 
> >+  PRUint32 securityState = mSecurityState & securityStateMask;
> >+  PRUint32 newState = aNewState & securityStateMask;
> >+
> >+  switch (securityState)
> >+  {
> >+    case nsIWebProgressListener::STATE_IS_SECURE:
> >+      if (newState == nsIWebProgressListener::STATE_IS_SECURE) {
> 
> Now you know that both securityState and newState are equal to STATE_IS_SECURE,
> so the following check will never be true, right?
> 

I don't want to allow change of security level flags. aNewState and mSecurityState might differ. Question is what to do in that case - ignore the difference or throw error.

However, downgrading the security level should probably also be allowed. When some parts of the page are loaded securely but with lower level (key < 90 bits) then the whole page info should be displayed with this level, shouldn't be? It's the same when some part (even a small picture) is not loaded from a secure site - we get BROKEN state then.

> 
> >+        if (aNewState != mSecurityState)
> >+          return NS_ERROR_NOT_AVAILABLE;
> 
> 
> I think we don't need to return a failure in this function, unless you know a
> good reason I propose to always return NS_OK.
> 

The change in DowngradeSecurityState you provide (which is more clear then my original way :)) allows set of security level flags even the IS_SECURE flag is not set. This IMO should not be allowed. Thus we should probably use mSecurityState = newState (the masked one) or better, call SetSecurityState(aNewState) instead.
Attachment #310321 - Flags: review?(honzab) → review-
Attached patch v3 (incomplete) (obsolete) — Splinter Review
I store the number of secure/non-secure/broken-secure sub-documents in the channel's security info using new interface dedicated to it. I also reflect the nsIWebProgressListener::STATE_RESTORING flag to really restore only when restoring and save only on full reload.

We might thing of a better place to store the numbers (currently in nsSecureBrowserUIImpl::UpdateSubrequestMembers). Should be stored on finish of page load but I am not able to find the correct place.
Attachment #310045 - Attachment is obsolete: true
Attachment #310321 - Attachment is obsolete: true
Attachment #310369 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Same as the previous one but added missing interface.
Attachment #310802 - Attachment is obsolete: true
Attachment #311621 - Flags: review?(kengert)
Comment on attachment 311621 [details] [diff] [review]
v3

Honza, thanks a lot, this patch was very inspiring, and you're on the right track towards the solution.

I'm sorry that it took me so long to come back to this bug, but reviewing this patch was not trivial, I wanted to make sure I really understand it.

Unfortunately, when I review complicated code, I often find that I better write the new myself, instead of describing proposed changes :-/ Sorry.

So, I'll again attach a new patch.

I'm giving you r- for this patch for two major reasons:

- I'm worried that your proposed use of mRestoring is dangerous.

I can't predict the order of calls on OnStateChange and onLocationChange, but you try to transport the "restoring" state info the other function.

I think we should save and restored the state at the same time where your code decides to save the state.

- Your proposed patch would be "expensive" at runtime. You proposed to update the counts for every element that gets loaded, which can be a lot, and we'd do 4 XPCOM calls for each such change. I think it's worth it to optimize and do it only when changing between pages.
Attachment #311621 - Flags: review?(kengert) → review-
Attached patch merged v3 (obsolete) — Splinter Review
I want to demonstrate the steps that brought me to the new patch.
FYI, you produced your v3 patch while bug 420187 was checked in, but it was soon after backed out.
This required me to merge.
So, here is a version of v3 that still applies to the most recent trunk.
Attachment #311621 - Attachment is obsolete: true
Attached patch merged v3 rearranged (obsolete) — Splinter Review
So, bugzilla is unable to diff "v3" against "merged v3", but a tool like "tkdiff" should help you compare the files.

Now, this "merged v3 rearranged" is the same as "merged v3", but has the order of files changed. You can use it to compare it to the "v4" that I'll attach next.
Attachment #314804 - Attachment is obsolete: true
Attached patch patch v4Splinter Review
This is my new proposed fix.
However, I'll attach a version that is easier to review.
Attachment #314806 - Attachment is obsolete: true
Honza, what do you think?
Attachment #314808 - Flags: review?(honzab)
Attachment #310802 - Attachment description: v3 → v3 (incomplete)
Comment on attachment 314808 [details] [diff] [review]
patch v4, not showing whitespace changes

Yes, this is quit good! I tested this again will all STRs and it works perfectly. 

Please, just take a look at indenting in nsSecureBrowserUIImpl::OnStateChange.
Attachment #314808 - Flags: review?(honzab) → review+
(In reply to comment #51)
> 
> Please, just take a look at indenting in nsSecureBrowserUIImpl::OnStateChange.

The whitespace fixes are already included in attachment 314807 [details] [diff] [review].
The fix for this bug overlaps with the fix for bug 383369 (which is a better fix for bug 335801).

This patch is a merged version of bug 358438 attachment 314807 [details] [diff] [review] (patch v4) and bug 383369 attachment 315028 [details] [diff] [review] (patch v1).

In my testing, this merged patch fixes all of bug 335801, bug 383369, bug 358438 (this one). I've tested using the URLs listed in those bugs, which are:
http://kuix.de/misc/test335801/
http://kuix.de/misc/test383369/
https://kuix.de/misc/test17/358438.php

Now I'm going to work on an updated patch for bug 420187, which will overlap as well :-(
Attachment #314808 - Flags: superreview?(rrelyea)
Comment on attachment 314808 [details] [diff] [review]
patch v4, not showing whitespace changes

r+ rrelyea
Attachment #314808 - Flags: superreview?(rrelyea) → superreview+
I've checked in a merged version of the latest patch together with the fix for bug 420187.

Marking fixed.

Honza, thanks a lot for your help!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
Flags: wanted1.9.0.x+
This bug can be opened now, can it not?  The fix is now in every gecko version we support.
Group: core-security
Although this is fixed in FIREFOX.

My original bug report was for Seamonkey.

As of SeaMonkey 1.1.17 (Windows) - it is still not fixed.
Paul FM, see comment 57 (and check the Gecko version in your Seamonkey build).
Gecko/20090605 Installed with Seamonkey 1.1.17 (zip file install).

I run Firefox on the same machine.


I just want to make sure that this bug fix gets into the next (currently alpha) version of Seamonkey.
> Gecko/20090605

I really did mean the version, not the build date.  The part after "rv:".

> I just want to make sure that this bug fix gets into the next (currently alpha)

If it's shipping off of Gecko 1.9.0 or later (which I believe it is), it'll pick this fix up automatically.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.