Closed
Bug 258405
Opened 20 years ago
Closed 17 years ago
New measure to prevent XUL spoofing also allows other custom toolbars to appear, not only the location bar.
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 3 beta3
People
(Reporter: markus.lindstrom, Assigned: ventnor.bugzilla)
References
Details
Attachments
(1 file, 11 obsolete files)
946 bytes,
patch
|
asaf
:
review+
mconnor
:
approval1.9+
|
Details | Diff | Splinter Review |
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 2•20 years ago
|
||
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.
Updated•20 years ago
|
Attachment #158625 -
Flags: review?(bugs)
Comment 3•20 years ago
|
||
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.
Flags: blocking-aviary1.0PR?
Comment 4•20 years ago
|
||
There is a better solution in Bug 22183. See Bug 22183 comment #163
Comment 5•20 years ago
|
||
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•20 years ago
|
||
Maybe you should combine that patch with yours, as it would ease applying it,
and might get more attention in a less cluttered bug.
Flags: blocking-aviary1.0?
Comment 7•20 years ago
|
||
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.
Updated•20 years ago
|
Attachment #158642 -
Flags: review?(bugs)
Updated•20 years ago
|
Attachment #158625 -
Attachment is obsolete: true
Attachment #158625 -
Flags: review?(bugs)
Comment 8•20 years ago
|
||
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.
Flags: blocking-aviary1.0? → blocking-aviary1.0+
Comment 10•20 years ago
|
||
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.
Attachment #158642 -
Flags: review?(bugs) → review-
Comment 11•20 years ago
|
||
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.
Updated•20 years ago
|
Attachment #158642 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #158687 -
Flags: review?(bugs)
Comment 12•20 years ago
|
||
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•20 years ago
|
||
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.
Updated•20 years ago
|
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
Updated•19 years ago
|
Assignee: bugs → nobody
QA Contact: bugzilla → toolbars
Comment 14•19 years ago
|
||
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.
Attachment #158687 -
Attachment is obsolete: true
Attachment #158687 -
Flags: review?(bugs)
Assignee | ||
Comment 15•18 years ago
|
||
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.
Assignee | ||
Comment 16•18 years ago
|
||
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...
Attachment #255663 -
Attachment is obsolete: true
Attachment #255663 -
Flags: review?(mano)
Assignee | ||
Comment 17•18 years ago
|
||
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.
Attachment #255666 -
Flags: review?(mano)
Assignee | ||
Comment 18•18 years ago
|
||
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.
Attachment #255666 -
Attachment is obsolete: true
Attachment #255674 -
Flags: review?(mano)
Attachment #255666 -
Flags: review?(mano)
Assignee | ||
Comment 19•18 years ago
|
||
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.
Attachment #255674 -
Attachment is obsolete: true
Attachment #255738 -
Flags: review?(mano)
Attachment #255674 -
Flags: review?(mano)
Comment 20•18 years ago
|
||
hrm, couldn't you use the collapsed attribute though (like the context menu item would do)?
Assignee | ||
Comment 21•18 years ago
|
||
(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)
Assignee | ||
Comment 22•18 years ago
|
||
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•18 years ago
|
||
(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?
Assignee | ||
Comment 24•18 years ago
|
||
(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•18 years ago
|
||
attribute (see xul.css and the full screen implementation for reference), has the same effect as the collapsed attribute (but not persisted).
Assignee | ||
Comment 26•18 years ago
|
||
Thank you Asaf. Now I guess it's time for you to go into nitpicking mode :-)
Attachment #255738 -
Attachment is obsolete: true
Attachment #255746 -
Flags: review?(mano)
Attachment #255738 -
Flags: review?(mano)
Comment 27•18 years ago
|
||
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.
Attachment #255746 -
Flags: review?(mano)
Assignee | ||
Comment 28•18 years ago
|
||
(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•18 years ago
|
||
urlbarClone.className = "chromeclass-toolbar";
urlbarClone.id = "antiphish-urlbar";
Assignee | ||
Comment 30•18 years ago
|
||
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.
Attachment #255746 -
Attachment is obsolete: true
Attachment #255871 -
Flags: review?(mano)
Comment 31•18 years ago
|
||
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?
Assignee | ||
Comment 32•18 years ago
|
||
(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.
Assignee | ||
Comment 33•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 35•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
And while the js solution works, it makes some toolkit code not reflect reality.
Assignee | ||
Comment 38•18 years ago
|
||
(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.
Assignee | ||
Comment 39•18 years ago
|
||
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.
Attachment #255871 -
Attachment is obsolete: true
Attachment #263447 -
Flags: review?(mano)
Attachment #255871 -
Flags: review?(mano)
Assignee | ||
Updated•18 years ago
|
Attachment #263447 -
Attachment is obsolete: true
Attachment #263447 -
Flags: review?(mano)
Assignee | ||
Comment 40•18 years ago
|
||
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.
Attachment #266164 -
Flags: review?(mano)
Comment 41•18 years ago
|
||
These belong to browser.css (in browser/), I think, let's get the other bug fixed first though.
Assignee | ||
Updated•18 years ago
|
Attachment #266164 -
Attachment is obsolete: true
Attachment #266164 -
Flags: review?(mano)
Assignee | ||
Comment 42•17 years ago
|
||
That bug is fixed, so lets fix this one.
Attachment #292886 -
Flags: review?(mano)
Comment 43•17 years ago
|
||
Comment on attachment 292886 [details] [diff] [review]
Updated patch
looks good, r=mano.
Attachment #292886 -
Flags: review?(mano) → review+
Assignee | ||
Updated•17 years ago
|
Attachment #292886 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #292886 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Comment 44•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
You need to log in
before you can comment on or make changes to this bug.
Description
•