258405 - New measure to prevent XUL spoofing also allows other custom toolbars to appear, not only the location bar.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Markus Lindström]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040907 Firefox/1.0 PR (NOT FINAL)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040907 Firefox/1.0 PR (NOT FINAL)

Currently, when a pop-up is created, the Location Bar appears by default on top
of it, as a way to prevent XUL spoofing. The problem is that, while the Location
Bar is the only one supposed to appear, if any other custom toolbar is installed
(Web Developer in my case, and I assume it'll happen with others such as
Googlebar as well, but also any manually created one), it will also make its way
into the pop-up's UI. This takes up a great deal of space, and should be prevented.

Reproducible: Always
Steps to Reproduce:
1. Open pop-up.
2. Look at the top of it.

Actual Results:  
Location Bar and custom toolbars (both extension made or manually made) appear.

Expected Results:  
Only the Location Bar should be present.

Comment 1

[Peter van der Woude [:Peter6]]

dupe of bug 22183 ?

Comment 2

[Neil Williamson]

Attachment: Simple CSS patch

Currently, items with class="chromeclass-toolbar" are hidden only if
location=no AND toolbar=no.  This patch also hides them when location=yes and
toolbar=no, with the exception of the primary toolbar
(class="toolbar-primary"); this not only prevents custom toolbars from
appearing, but the tab bar as well.

Combined with Ben Basson's patch (attachment 158192 [details] [diff] [review] on bug 22183), the
remaining primary toolbar will certainly contain the location bar; without that
patch, the location bar won't appear unless it's left in the primary toolbar,
as it is by default.  That's a problem even without my patch, as the location
bar can still be placed in the menu bar, and the menu bar is still correctly
hidden.

Comment 3

[Neil Williamson]

Requesting blocking1.0PR, since there's a patch and most popup windows look
pretty ugly and broken for all users with custom toolbars without it.

Comment 4

[Robert Parenton]

There is a better solution in Bug 22183.  See Bug 22183 comment #163

Comment 5

[Neil Williamson]

I tested that patch before attempting this one, and found that it did not hide
custom toolbars or the tab bar.  It did, however, ensure that the location bar
appeared in the primary toolbar (and hid any remaining separators, extension
buttons etc. within that toolbar); that behaviour is required for this patch to
work as expected.

Comment 6

[Robert Parenton]

Maybe you should combine that patch with yours, as it would ease applying it,
and might get more attention in a less cluttered bug.

Comment 7

[Neil Williamson]

Attachment: Combined fix: hide all custom toolbars and tab bar, ensure only the URL bar is present in the remaining toolbar and disable context menu and collapsing.

The previous patch with Ben Basson's (cusser at ntlworld.com) patch from bug
22183 comment 163 added on.

This should ensure that the location bar is the only toolbar chrome on the
window, as was presumably the original intention.

I'm not sure whether I should obsolete the previous, standalone patch for my
part of the fix or move the review request over to this one; I'll leave that
decision to someone with more experience.

Comment 8

[Asa Dotzler [:asa]]

I'm pinging the Aviary team to get a reply on the PR blocker nomination. We
certainly want to fix this before 1.0 so I'm tentatively setting that flag to
blocking.

Comment 9

[Asa Dotzler [:asa]]

OK, not for PR. 

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 158642 [details] [diff] [review]
Combined fix: hide all custom toolbars and tab bar, ensure only the URL bar is present in the remaining toolbar and disable context menu and collapsing.

How does this patch affect things like navigating in a frame - where the status
listener uses getElementById to find the *one* URL bar to update, etc. Now you
have two - the original and a cloned one. You need to tear the old one out of
the document. 

Alternate toolbars should be created with the class
chromeclass-toolbar-additional (the most benign patch is probably to
customizeToolbar.js)... extension authors will have to patch their extensions.

Comment 11

[Neil Williamson]

Attachment: Revised patch based on Ben's feedback

This patch replaces the old nav bar's node with the new one, rather than just
collapsing it, so there's only one nav bar again.

I also had a look in customizeToolbar.js, traced the custom toolbar class
definition back to bindings/toolbar.xml and replaced it with
chromeclass-toolbar-additional as advised.  I did the same for
bindings/tabbrowser.xml.

The only things not addressed are extensions' own toolbars.

Comment 12

[Neil Williamson]

I thought about this again this morning, and the latest patch I submitted
doesn't actually manually take the URLbar out - rather, it takes out the main
toolbar, and it's not guaranteed that the URLbar is in there (which is part of
the point of this bug).

With that in mind, I had DOMi check on some would-be chromeless windows with the
URLbar located in other places (custom toolbar, menubar), but there was only
ever the one URLbar in the main navbar with this patch, no matter where I put it
in Customize Toolbars.  I can only assume that appendChild() rips the URLbar out
of its original position amd moves it to the new position, rather than
duplicating it.  (I'm assuming that scripts using getElementById wouldn't find
anything that DOMi doesn't, since DOMi did show both the original and the cloned
navbar node using the old patch.)

Nevertheless, this still looks a nicer solution than the previous patch since it
retains the navbar's correct node position rather than re-adding it as the last
child of #navigator-toolbox.

Comment 13

[Robert Parenton]

Even though the status bar is now forced instead of the location bar, we should
probably still try to fix this, just so the code isn't broken, unless it is
going to get backed out.

Comment 14

[Phil Ringnalda (:philor)]

Comment on attachment 158687 [details] [diff] [review]
Revised patch based on Ben's feedback

Not only has this bit-rotted over a year and a half, but if this worked at the time, it doesn't seem to now: when I tried the same approach updated to trunk, it had the nasty effect of killing addressbar autocomplete in all subsequent windows from then on.

Comment 15

[Michael Ventnor]

Attachment: Remove custom toolbars from popup windows

This seems to do the Right Thing with less code. It checks the ID's of the toolbars and if it doesn't match that of the menubar or main toolbar it will be removed. Changing properties on toolbars seems to make the change across windows and sessions which is why I'm removing them rather than setting them to hidden, removing them only affects the popup.

I love the DOM Inspector.

Comment 16

[Michael Ventnor]

Comment on attachment 255663 [details] [diff] [review]
Remove custom toolbars from popup windows

I just remembered that not everyone has the default Firefox layout so if the URL bar is is any place but the main toolbar it won't appear...

Comment 17

[Michael Ventnor]

Attachment: Make stripped-down navigator toolbox

This takes a similar aspect to the two-year-old patches. It works perfectly in all the custom toolbar combinations I've tried.

Interestingly, replaceChild doesn't work properly so I have to use insertBefore then removeChild.

Comment 18

[Michael Ventnor]

Attachment: v2: Don't allow users to customize crippled toolbox

This was the one thing that was worrying me when I made the last patch, now I figured out how to prevent that. Also, updated a comment where I should have.

Comment 19

[Michael Ventnor]

Attachment: Use display:none to hide the toolbars

OK, now that I've had some sleep I can put my earlier ramblings into English.

Setting the "hidden" attribute will cause a custom toolbar to disappear, forever. Checking the toolbar in the customize context menu doesn't bring it back in any window.
Originally I thought of just removing all the custom toolbars but then I thought that this would cause mass breakage on any extension that likes to put its options anywhere it can find (eg. web developer toolbar).
After some thought and sleep, I decided to use the same method in xul.css. Setting display:none will cause much less hassle for extension developers than completely removing the toolbar from the popup.

Comment 20

[Asaf Romano (gone)]

hrm, couldn't you use the collapsed attribute though (like the context menu item would do)?

Comment 21

[Michael Ventnor]

(In reply to comment #20)
> hrm, couldn't you use the collapsed attribute though (like the context menu
> item would do)?
> 

Wouldn't that make it across windows and sessions like the hidden attribute did?
(I'll try it now)

Comment 22

[Michael Ventnor]

The good news is using "collapsed" doesn't do damage like hidden does. However its slower than setting the display style, I could see the toolbars for a millisecond before they disappeared. This didn't happen with the display attribute.

Comment 23

[Asaf Romano (gone)]

(In reply to comment #21)
> (In reply to comment #20)
> > hrm, couldn't you use the collapsed attribute though (like the context menu
> > item would do)?
> > 
> 
> Wouldn't that make it across windows and sessions like the hidden attribute
> did?
> (I'll try it now)
> 

hrm, maybe moz-collapsed then?

(In reply to comment #22)
> The good news is using "collapsed" doesn't do damage like hidden does. However
> its slower than setting the display style, I could see the toolbars for a
> millisecond before they disappeared. This didn't happen with the display
> attribute.
> 

Do you also see that if you move this code to prepareForStartup?

Comment 24

[Michael Ventnor]

(In reply to comment #22)
> The good news is using "collapsed" doesn't do damage like hidden does. However
> its slower than setting the display style, I could see the toolbars for a
> millisecond before they disappeared. This didn't happen with the display
> attribute.
> 

I take that back. Using collapsed causes custom toolbars to not appear when you open a new window from a normal browser window. I had to open them up again from the context menu.
What's the moz-collapsed attribute you mean? Is it a normal attribute or a CSS style like the other moz-* attributes I've seen?

Comment 25

[Asaf Romano (gone)]

attribute (see xul.css and the full screen implementation for reference), has the same effect as the collapsed attribute (but not persisted).

Comment 26

[Michael Ventnor]

Attachment: Use moz-collapsed

Thank you Asaf. Now I guess it's time for you to go into nitpicking mode :-)

Comment 27

[Asaf Romano (gone)]

Comment on attachment 255746 [details] [diff] [review]
Use moz-collapsed

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.765
>diff -u -8 -p -r1.765 browser.js
>--- browser/base/content/browser.js	19 Feb 2007 04:24:58 -0000	1.765
>+++ browser/base/content/browser.js	20 Feb 2007 02:52:20 -0000
>@@ -953,42 +953,62 @@ function prepareForStartup()
>   // hook up UI through progress listener
>   gBrowser.addProgressListener(window.XULBrowserWindow, Components.interfaces.nsIWebProgress.NOTIFY_ALL);
> 
>   // Initialize the feedhandler
>   FeedHandler.init();
> 
>   // Initialize the searchbar
>   BrowserSearch.init();
>+
>+  var chromeHidden = document.documentElement.getAttribute("chromehidden");
>+  if (gURLBar && chromeHidden.indexOf("toolbar") != -1 && chromeHidden.indexOf("location") == -1) {

hrm, why did you add the "location" check?

>+    gURLBar.setAttribute("readonly", "true");
>+    gURLBar.setAttribute("enablehistory", "false");
>+    var goButtonStack = document.getElementById("go-button-stack");
>+    if (goButtonStack)
>+      goButtonStack.setAttribute("hidden", "true");
>+
>+    // Hide all toolbars and leave nothing but the url bar (and the menu bar if wanted)
>+    var navToolbox = document.getElementById("navigator-toolbox");
>+    var allToolbars = navToolbox.childNodes;
>+    for (var i = 0; i < allToolbars.length; i++) {
>+      allToolbars[i].setAttribute("moz-collapsed", "true");
>+    }
>+    if (chromeHidden.indexOf("menubar") == -1) {
>+      var menubar = document.getElementById("toolbar-menubar");
>+      menubar.removeAttribute("moz-collapsed");
>+      menubar.removeAttribute("context");
>+      menubar.removeAttribute("customizable");

what's the purpose of removing the customizable attribute?

>+    }
>+    var urlbarClone = document.createElementNS(
>+                      "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",

use kXULNS

>+                      "toolbar");
>+    urlbarClone.appendChild(document.getElementById("urlbar-container"));
>+    urlbarClone.setAttribute("class", "chromeclass-toolbar");
>+    urlbarClone.setAttribute("id", "antiphish-urlbar");

prefer the class and id properties.

Comment 28

[Michael Ventnor]

(In reply to comment #27)
> hrm, why did you add the "location" check?

My original thought was that almost all of this code is modifying the location bar. If we aren't going to display it anyway then why waste all this processing?
But now I realise that we have the menubar to handle as well so I could restructure those checks.

> what's the purpose of removing the customizable attribute?

I guess it can be removed now, it was left over from my previous method of removing the toolbars plus I didn't want them to be able to customize a chrome-crippled popup.
 
> prefer the class and id properties.

Sorry, can you please clarify this?

Comment 29

[Asaf Romano (gone)]

urlbarClone.className = "chromeclass-toolbar";
urlbarClone.id = "antiphish-urlbar";

Comment 30

[Michael Ventnor]

Attachment: Use moz-collapsed 2

Yes, I forgot about those. You know you're a geek when you understand codespeak but not English.

I moved the gURLBar check because from where it was it was causing problems for people with no URL bar. By changing the position of this check, it not only allows their custom toolbars to be hidden also but fixes a bug where a blank "nav-bar" is left behind in popups where toolbar=no but location=yes. I also moved the chromeHidden location check to there so we don't waste cycles on an invisible location bar.

Comment 31

[Asaf Romano (gone)]

I would argue that we should show the readonly location bar even if the normal location bar has been removed... how does IE7 handle this?

Comment 32

[Michael Ventnor]

(In reply to comment #31)
> I would argue that we should show the readonly location bar even if the normal
> location bar has been removed... how does IE7 handle this?
> 

I don't know, IE7 doesn't let you remove the location bar.
I would argue that the people who choose to hide the location bar are the ones that don't understand it, so even if we did force a location bar on them it still wouldn't save them.
The reason they removed the location bar is because they DON'T want to see it, I don't think we should go against their will in any case.

Comment 33

[Michael Ventnor]

I don't think that would work either. When I tried to make a clone of the URL bar it always ends up blank (defeating the purpose :P ).

Are you going to mark a review, Mano? Do you have any further suggestions?

Comment 34

[Asaf Romano (gone)]

We need to figure out what's the purpose of chromeclass-toolbar-additional with this patch or whether comment 10 is a better solution.

While this does work, changing the rule(s) in http://lxr.mozilla.org/seamonkey/source/toolkit/content/xul.css#41 is cheaper and also less confusing(!) Note SeaMonkey depends on the current rules set too.

Comment 35

[Michael Ventnor]

(In reply to comment #34)
> We need to figure out what's the purpose of chromeclass-toolbar-additional with
> this patch or whether comment 10 is a better solution.

The problem with that though is that everyone who develops a toolbar extension would need to be made aware of this bug and make sure they use a specific class. With my method everything is preemptive and requires no work on the extension author's part.

> While this does work, changing the rule(s) in
> http://lxr.mozilla.org/seamonkey/source/toolkit/content/xul.css#41 is cheaper
> and also less confusing(!) Note SeaMonkey depends on the current rules set too.

By that do you mean SeaMonkey also uses xul.css (even though its in toolkit)? Also in order to do that I think I'd need to introduce a new class (one that prevents the default toolbars from being hidden so they can be dealt with later) as well as rules that fight with each other so I get the right result.

I still think my JS method is better but I'll give it a shot.

Comment 36

[Asaf Romano (gone)]

Why would you need a new class? It's ok for the toolbar itself to be hidden... you're moving the location bar to a separate toolbar which wouldn't have the chromeclass-toolbar set.

You may want to discuss this Neil (both) before implementing my 2:00 AM approach. :)

Suite-runner uses toolkit's xul.css.

Comment 37

[Asaf Romano (gone)]

And while the js solution works, it makes some toolkit code not reflect reality.

Comment 38

[Michael Ventnor]

(In reply to comment #36)
> Why would you need a new class? It's ok for the toolbar itself to be hidden...
> you're moving the location bar to a separate toolbar which wouldn't have the
> chromeclass-toolbar set.

Now you're confusing me. I thought you wanted me to develop a CSS solution to completely replace my JS solution :/

(In reply to comment #37)
> And while the js solution works, it makes some toolkit code not reflect
> reality.
> 

How so? The new toolbar directly imports the URL Bar thus keeping all the properties. Is there code which uses the URL Bar to manipulate its siblings? That would be a problem, but so far I haven't encountered it yet.

Comment 39

[Michael Ventnor]

Attachment: Update to tip (among other things)

This patch will fix the location microbar not displaying on full-screen. It will also adopt the throbber should it be on a hidden toolbar. The throbber provides very useful activity feedback and it should apply to popups too.

Mano, I can't think of a non-hackish way of doing this in CSS. And so far I haven't encountered a single problem with this method.

Comment 40

[Michael Ventnor]

Attachment: Pure-CSS approach

Now that bug 337344 has a definitive goal (and patch) I need to accommodate that patch's behaviour, by going with a purely CSS approach.

This will do the right thing on toolbars that haven't set the right classes (extension toolbars). I've tested and this works perfectly on Firefox. I'm pretty certain that Seamonkey would behave well too.

Comment 41

[Asaf Romano (gone)]

These belong to browser.css (in browser/), I think, let's get the other bug fixed  first though.

Comment 42

[Michael Ventnor]

Attachment: Updated patch

That bug is fixed, so lets fix this one.

Comment 43

[Asaf Romano (gone)]

Comment on attachment 292886 [details] [diff] [review]
Updated patch

looks good, r=mano.

Comment 44

[Reed Loden [:reed]]

Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.45; previous revision: 1.44
done