283733 - accessing a relative anchor in a secure page removes the locked icon and yellow background UI

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[sairuh (rarely reading bugmail)]

posting from Asa's blog:

1. If I enter the URL: https://agia.fsf.org/order/
I get the padlock in the bottom and the address bar is yellow.

2. Now if I click on one of the #refs, eg the link "Software",
I lose the padlock and the yellow address bar.

3. Pasting the full url https://agia.fsf.org/order/#software
gives the secure feedback, but clicking another #ref
loses it again.

This is with 1.0.1 en-us Linux

------------------------------------

I was able to reproduce this somewhat using a welsh (cy-GB, have been doing l10n
testing, I hope this doesn't matter) 20050223-1.0.1 build. (I can recheck later
with en-US.)

Anyhow, I get the same misbehavior at step 2 --but strangely, step 3 for me
doesn't restore the locked padlock or yellow background color in the address bar.

I tested this using a ffox 1.0 build (mac again), and this was not a problem.

Comment 1

[Darin Fisher]

yikes... this sounds pretty bad.

Comment 2

[Jesse Ruderman]

This is public (http://weblogs.mozillazine.org/asa/archives/007626.html) and not
a security hole.  Why is it marked as security-sensitive?

Comment 3

[Darin Fisher]

Are people seeing this on the trunk, or is it only affecting Firefox 1.0.1?

Comment 4

[sairuh (rarely reading bugmail)]

I cannot reproduce this using 2005021409-trunk ffox bits (linux fc3). I'll grab
a more recent build...

Comment 5

[sairuh (rarely reading bugmail)]

hrm, I do see this using 2005022511-trunk ffox bits.

Jay had email some https / location bar / security / padlock bugs with checkins
which might be suspect:

https://bugzilla.mozilla.org/show_bug.cgi?id=258048
https://bugzilla.mozilla.org/show_bug.cgi?id=268483
https://bugzilla.mozilla.org/show_bug.cgi?id=277564
https://bugzilla.mozilla.org/show_bug.cgi?id=283201

Comment 6

[Tracy Walker [:tracy]]

Testing Windows, this regressed on the 1.0.1 branch between
2005-02-14-06-aviary1.0.1 and 2005-02-15-06-aviary1.0.1. Sarah, please check the
trunk build from the 15th.

Comment 7

[sairuh (rarely reading bugmail)]

using ffox trunk builds on linux fc3, this regressed between:

* 2005021510-trunk (works)
* 2005021609-trunk (broken)

another observation: after clicking a relative link (then noticing the locked UI
go away), reloading (ctrl+R) the page restores the locked UI.

Comment 8

[Darin Fisher]

Thanks for narrowing the regression window.  It looks like the patch for bug
258048 is the likely cause of this bug.  Investigating...

Comment 9

[Darin Fisher]

OK, the problem here is quite simple.  We are getting a null request in the
OnLocationChange event generated for navigations to https://blah.com/#ref

I could attempt to fix this bug by skipping the lock icon updating logic when we
have a null request (or perhaps when we have a null channel).  What do people
think of that solution?

It may also be wrong that we are sending an OnLocationChange in this case (since
there is no corresponding request), but then again... I think that might be
necessary to ensure that the URL bar is updated properly.  I wonder what other
cases may result in a null request.  Is it always valid to ignore those and
assume that they have the same security level?

Comment 10

[Darin Fisher]

Attachment: v1 patch

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

This seems reasonable to me. r=jst

Comment 12

[Daniel Veditz [:dveditz]]

Clearing the security flag for reasons Jesse noted in comment 2

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

sr- This regresses recently fixed bug 258048 and bug 277564

As far as I can tell aRequest is *always* null when passed to OnLocationChange.

Comment 14

[Daniel Veditz [:dveditz]]

-> PSM

Comment 15

[Darin Fisher]

dveditz: no!  if you recall, i fixed the regression where aRequest was always
null.  that regression was caused by a bad merge to nsDocShell.  i tested this
very carefully, and aRequest is only null in special cases such as this one
where there is not a network request.  otherwise, due to johnny's patch, there
is a non-null network request.  please re-consider the patch.

Comment 16

[Darin Fisher]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

With this patch applied I could not reproduce bug 258048.  Did you find that
you could?

restoring r=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

Isn't aRequest null on tab switches?  Does the lock icon update properly on tab
switch with this change?

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

ah, *docshell*. I made sure to cvs up in security before testing your patch,
but forgot the late-breaking docshell change. :-(

Sorry about that, it of course works as advertised (*phew*, I was wondering how
in the world the other patches worked on a null request).

sr=dveditz

Comment 19

[Daniel Veditz [:dveditz]]

I think this should block 1.7.6 just to get it right, but since it shows a
secure site as non-secure rather than the other way around it's not a security
hole that requires a firefox respin.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

requesting 1.7.6 approval, and 1.0.1 branch for the heck of it just in case
there's a 1.0.2

Comment 21

[Darin Fisher]

(In reply to comment #17)
> Isn't aRequest null on tab switches?  Does the lock icon update properly on tab
> switch with this change?

No, nsSecureBrowserUI::OnLocationChange is not called when tabs are switched. 
If that is using nsIWebProgressListener, then it must depend on something
special being done by tabbrowser.xml for nsIWebProgressListener's attached
directly to it.  I suspect that the tabbrowser stores the state of each browser,
and notifies its listeners when things change.  The nsSecureBrowserUI is
attached to each browser.

Comment 22

[Darin Fisher]

I think it would be wise to consider respinning for this fix since it can give
users false confidence in the browser.  It may for example make some ecommerce
site appear insecure, which might discourage customers, etc.  That seems like
something worth fixing.  Wasn't there a respin planned for the installer problem?

Comment 23

[Chase Phillips]

(In reply to comment #22)
> Wasn't there a respin planned for the installer problem?

The respin was for 1.0.1 and included no app-level changes, only installer
changes.  Happened last night, files have been QA'd, signed, and are ready to be
pushed onto the site.

Comment 24

[Darin Fisher]

ok, fair enough.  i don't know how best to handle this.

Comment 25

[Darin Fisher]

fixed-on-trunk

Comment 26

[Boris Zbarsky [:bzbarsky]]

The tabbrowser code is at
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#572

Looks like the security UI is just not a listener on the <tabbrowser>, if this
code doesn't break us....

Comment 27

[Darin Fisher]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

1.0.1 has shipped; requesting approval for aviary 1.0.2

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 175629 [details] [diff] [review]
v1 patch

approved.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can someone land this fix on the appropriate branches?

Comment 30

[Darin Fisher]

yes, i will do so shortly.

Comment 31

[Darin Fisher]

fixed-aviary1.0.1, fixed1.7.6

Comment 32

[OstGote!]

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

Comment 33

[OstGote!]

(In reply to comment #31)
> fixed-aviary1.0.1, fixed1.7.6

Hmm fixed-aviary1.0.2 I guess.

Comment 34

[sairuh (rarely reading bugmail)]

tested with 2005031011-1.7 mozilla bits on linux fc3: looks good, the locked
padlock in the statusbar doesn't disappear when I click a relative anchor in the
secure page.

Comment 35

[sairuh (rarely reading bugmail)]

mistakenly removed fixed1.7.6 --pardon the bugspam. set your filter/quicksearch
to "ZippidityDooDahHey" to catch these for easy removal/etc/

Comment 36

[sairuh (rarely reading bugmail)]

also verified fixed in

- firefox 1.0.2 branch 20050314 build on Mac OS X 10.3.8

- firefox trunk 20050314 builds on Mac and Win XPsp2.