Last Comment Bug 337344 - Disable location bar hiding by default, to make chrome spoofing harder
: Disable location bar hiding by default, to make chrome spoofing harder
Status: RESOLVED FIXED
[sg:want P2] anti-spoofing 181b1+
:
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: 2.0 Branch
: All All
: -- enhancement with 1 vote (vote)
: Firefox 3 alpha8
Assigned To: Johnathan Nightingale [:johnath]
:
:
Mentors:
: 348373 361742 378554 411216 (view as bug list)
Depends on: 393900 408678 445542
Blocks: 378554 397695 485237 501762
  Show dependency treegraph
 
Reported: 2006-05-09 14:09 PDT by Daniel Veditz [:dveditz]
Modified: 2012-02-16 05:11 PST (History)
45 users (show)
mbeltzner: blocking‑firefox3+
mbeltzner: blocking‑firefox2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Flip the pref (1.67 KB, patch)
2006-06-27 20:03 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mconnor: review-
Details | Diff | Splinter Review
Patch to add chromeless window detection/handling (9.92 KB, patch)
2007-05-24 13:11 PDT, Johnathan Nightingale [:johnath]
no flags Details | Diff | Splinter Review
Screencap of default presentation now (123.42 KB, image/png)
2007-05-24 13:16 PDT, Johnathan Nightingale [:johnath]
mbeltzner: ui‑review+
Details
Updated patch with dveditz, ventron feedback (10.01 KB, patch)
2007-05-28 05:44 PDT, Johnathan Nightingale [:johnath]
no flags Details | Diff | Splinter Review
Fix for gBrowser has no properties error (10.15 KB, patch)
2007-05-28 07:17 PDT, Johnathan Nightingale [:johnath]
mconnor: review-
Details | Diff | Splinter Review
Post-review patch (5.90 KB, patch)
2007-06-05 12:31 PDT, Johnathan Nightingale [:johnath]
mconnor: review+
Details | Diff | Splinter Review
Post-nit patch (6.66 KB, patch)
2007-06-06 06:29 PDT, Johnathan Nightingale [:johnath]
no flags Details | Diff | Splinter Review
Updated patch post-IRC discussion (7.06 KB, patch)
2007-06-06 11:01 PDT, Johnathan Nightingale [:johnath]
no flags Details | Diff | Splinter Review
example of using a notificationbox in a popup with transparency (977 bytes, application/vnd.mozilla.xul+xml)
2007-06-08 06:49 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details
Toggle the pref and todo the mochitests (1.80 KB, patch)
2007-06-27 18:23 PDT, Johnathan Nightingale [:johnath]
no flags Details | Diff | Splinter Review
Flip the dom.disable_window_open_feature.location pref and fix the window targetting (3.28 KB, patch)
2007-08-22 12:14 PDT, Johnathan Nightingale [:johnath]
mconnor: review+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2006-05-09 14:09:17 PDT
Firefox has had support for a cool anti-phishing microbar that shows the true location of a page even on popups that disable the full-size location bar, simply by changing the pref dom.disable_open_window_feature.location to true.

When we first tried setting this before Firefox 1.0 shipped (see bug 22183 comment 120 and following) and got a lot of pushback. Now that Firefox is more established, and phishing is understood to be a serious problem by a broader range of the web audience, it may be appropriate to finally do this in Firefox-2.
Comment 1 Mike Connor [:mconnor] 2006-05-20 09:22:49 PDT
So, this is something that is better posted to d-a-f, but I'm not opposed to it, especially as IE7 seems to be doing the same thing as we had before.

plussing so it gets discussed
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2006-05-28 08:28:12 PDT
I'm for this, really, despite the fact that evidence shows that most users don't really look at (or understand) the location bar, and despite the fact that location bars can be spoofed, we can probably design something that calls a little bit more attention to itself and alerts users to the fact that the window has been opened by the browser, etc.

A similar but alternative solution might be to use a browsermessage notification which says something along the lines of:

[ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]

Just spitballin' ideas here. 
Comment 3 Ben Goodger (use ben at mozilla dot org for email) 2006-06-08 18:56:40 PDT
Hey Mike, here's one to pass to the interns ;-)
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-09 08:38:41 PDT
(In reply to comment #2)
> A similar but alternative solution might be to use a browsermessage
> notification which says something along the lines of:
> 
> [ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]

Even better:

 .---------------------------------------------------------------------.
 |Paypal: Enter your personal information                        _ [] X|
 |---------------------------------------------------------------------'
 | http://221.241.11.2/itsatrap.html                (Show Toolbars) (X)|
 |---------------------------------------------------------------------|
 |                                                                     |
 .                                                                     .
 .                                                                     .
 .                                                                     .
Comment 5 Mike Connor [:mconnor] 2006-06-09 08:39:45 PDT
Indeed!
Comment 6 Madhava Enros [:madhava] 2006-06-09 09:46:46 PDT
If this goes ahead, it would probably be useful to provide users with a quick way to suppress the microbar for trusted domains.  In non-phishing cases, chromeless windows are likely to occur in web-apps and windows where some care has been taken with the visual aesthetics;  in both cases, a recurring microbar would likely been seen as "in the way."  If users turn it off permanently for all sites, they're prey for the wolves again.  Maybe even piggybacking on the 'allow popup' whitelist?
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-09 10:48:01 PDT
(In reply to comment #6)
> If this goes ahead, it would probably be useful to provide users with a quick
> way to suppress the microbar for trusted domains.  In non-phishing cases,

I think this is a very good idea, actually. Supporting web-apps that want to control their user experience is something we should be thinking about, even though I cringe at the idea of more whitelists.

> would likely been seen as "in the way."  If users turn it off permanently for
> all sites, they're prey for the wolves again.  Maybe even piggybacking on the

Whatever we do, there should be no way to turn it off for all sites. That would be allowing the user to "shoot from hip, aim at foot."
Comment 8 Mike Schroepfer 2006-06-27 20:02:45 PDT
Even without the notification message to suppress this, we need to get it for feature parity with IE7 as a way to stomp on phishers.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2006-06-27 20:03:42 PDT
Created attachment 227346 [details] [diff] [review]
Flip the pref

I don't see a simple way to do this with the current code, so for now I'm just flipping the pref.  If someone else sees a simple way to figure out the original window parameters from which a window was opened using the current code, I'd be all ears.
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-28 15:37:46 PDT
Connor was concerned that flipping this pref will break a bunch of our code which uses "was the window opened with chrome or not" as a heuristic to determine if a window is or is not a popup (f.e. opening a link from an external app in new tab when the last-focused Firefox window was a popup window).

Maybe we can just use the notificationmessage solution. Just show:

 .---------------------------------------------------------------------.
 |Paypal: Enter your personal information                        _ [] X|
 |---------------------------------------------------------------------'
 | /!\ This is 123.123.123.123                    (Trust this site) (X)|
 |---------------------------------------------------------------------|
Comment 11 Mike Connor [:mconnor] 2006-07-03 21:24:54 PDT
Comment on attachment 227346 [details] [diff] [review]
Flip the pref

This regresses window targeting behaviour that relies on chromehidden. I think beltzner's solution is probably better overall.
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2006-07-05 12:46:03 PDT
Decided not to block on this, since it's new functionality. Dan has said that he'd be willing to look at this and either:

 - flip the chromehidden pref and fix the browser.js and antiphishing dependencies
 - implement the notificationmessage as shown here and add an indication of whether or not the site is using SSL

Thanks, Dan! :)
Comment 13 Daniel Veditz [:dveditz] 2006-08-11 20:17:35 PDT
*** Bug 348373 has been marked as a duplicate of this bug. ***
Comment 14 Jesse Ruderman 2006-11-24 19:04:34 PST
*** Bug 361742 has been marked as a duplicate of this bug. ***
Comment 15 Michael Ventnor 2007-01-06 15:48:19 PST
I've searched LXR and haven't found any code which flipping the pref may break. Does anyone know of any examples where this is the case? I might have a go at fixing them if so.
Comment 16 Mike Connor [:mconnor] 2007-01-06 23:04:48 PST
it'll break the external link handling code that bypasses popups by checking chromehidden.  Beltzner's solution is preferred anyway, per comment 11
Comment 17 Michael Ventnor 2007-02-17 17:58:11 PST
I found the code which handles external links but saw no dependency on the location bar or chromehidden (instead it checked window.toolbar.visible). I ran a window.open javascript which had everything hidden but the location bar. Not only was the location bar read-only, but Fx had no trouble opening a new full-chrome window when I typed in a URL in the Run dialog.

Did all the problems get fixed while this bug was hibernating?
Comment 18 Daniel Veditz [:dveditz] 2007-02-18 00:20:38 PST
Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some problems on a Mac.
Comment 19 Michael Ventnor 2007-02-18 00:28:06 PST
(In reply to comment #18)
> Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some
> problems on a Mac.

I don't have a Mac, sorry. This shouldn't show problems on the Mac since the code that handles external links is JS and checks for the toolbar, not the location bar, to determine if it's a popup or not.

This missed out on 2.0 obviously so -> Future.
Comment 20 Jordan M. 2007-02-18 07:13:48 PST
(In reply to comment #18)
> Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some
> problems on a Mac.
> 
Dan - Are either of the solutions to this (notification, flip) going to show up in Firefox 3?

Thanks.
Comment 21 Jordan M. 2007-02-18 07:30:15 PST
(In reply to comment #4)
> (In reply to comment #2)
> > A similar but alternative solution might be to use a browsermessage
> > notification which says something along the lines of:
> > 
> > [ This is a webpage                [ Show URL ] [ Show Toolbars ]  ]
> 
> Even better:
> 
>  .---------------------------------------------------------------------.
>  |Paypal: Enter your personal information                        _ [] X|
>  |---------------------------------------------------------------------'
>  | http://221.241.11.2/itsatrap.html                (Show Toolbars) (X)|
>  |---------------------------------------------------------------------|
>  |                                                                     |
>  .                                                                     .
>  .                                                                     .
>  .                                                                     .
> 
Wait, we're going to have an X on the window? Isn't this going to just bother users, instead of solve the actual problem? The average user will ignore the window, regardless if it said "YOU'RE BEING FOOLED" or not, they'll X out of the window. I thought we all read the phishing document with the stats on how many people ignore/don't realize these things?

I say we cut the ability to X out, as it's not going to hurt anyone to choose either to just show the location bar (then close), or a second option, show everything it hid (such as status, menu, location) (then close).
If that hurts any functionality on the user's side, I don't know what the world is coming to. If you don't want the location bar, too bad, we'll do it because it concerns security -- as someone so eloquently put it "we can't trust the user to protect themselves."

Just some ideas.
Comment 22 Jordan M. 2007-02-18 10:47:23 PST
(In reply to comment #10)
> Connor was concerned that flipping this pref will break a bunch of our code
> which uses "was the window opened with chrome or not" as a heuristic to
> determine if a window is or is not a popup (f.e. opening a link from an
> external app in new tab when the last-focused Firefox window was a popup
> window).
> 
> Maybe we can just use the notificationmessage solution. Just show:
> 
>  .---------------------------------------------------------------------.
>  |Paypal: Enter your personal information                        _ [] X|
>  |---------------------------------------------------------------------'
>  | /!\ This is 123.123.123.123                    (Trust this site) (X)|
>  |---------------------------------------------------------------------|
> 
Like this idea a bit more (sorry for all of the replies, I'm just throwing ideas out, in case we choose this-or-that), but still with the X. Not to fond of the "trust this site" being the only option, reminds me of an IE message which I just click through -- Unless the X is "don't trust this site?" Might want to clear what you're thinking up. As for the message, I was thinking more along the lines of "this site (123.123.123.123) is not the site you requested," since given the notification, I'm supposing: They requested the site (how else would we know 123.123.123.123 isn't the site they wanted? -- or are we thinking of something else?)
Comment 23 Jordan M. 2007-02-18 11:05:33 PST
After much deciphering of my previous post, these are two separate issues, unless I'm mistaken:

A.) This bug is dealing with disabled location bars, or whatnot, and we should ask: 1. If site X is the site they wanted 2. If they want to see the location bar 3. Combine the two into one message or what have you.

B.) Trusting a site would mean that you were either: Re-directed from somewhere, you requested another site, etc.

Not sure if B should even be considered a threat, seeing as by itself, it would mean the site you clicked was cracked and you got re-directed to a phony site.
Comment 24 Phil Ringnalda (:philor) 2007-02-18 11:41:03 PST
(In reply to comment #18)
>  mconnor demonstrated some problems on a Mac.

Was it a three card monte demonstration, or did he have some fix for bug 303568 that made it work in the non-disable_window_open_feature.location case?
Comment 25 Jordan M. 2007-02-18 12:26:17 PST
Allowing the menu bar and other things (e.g. toolbar) may be useful to unhide, such as in the testcase of bug 37055 as well, since we're on the topic of notification messages, if we're going to go with that (thought I'd reinforce my earlier mention of this in comment 21 with a testcase that might show more relevance).
Comment 26 Jordan M. 2007-02-18 12:26:57 PST
Me and my silly mistakes: bug 370555
Comment 27 Jordan M. 2007-02-18 15:10:37 PST
(In reply to comment #23)
> After much deciphering of my previous post, these are two separate issues,
> unless I'm mistaken:
> 
> A.) This bug is dealing with disabled location bars, or whatnot, and we should
> ask: 1. If site X is the site they wanted 2. If they want to see the location
> bar 3. Combine the two into one message or what have you.
> 
> B.) Trusting a site would mean that you were either: Re-directed from
> somewhere, you requested another site, etc.
> 
> Not sure if B should even be considered a threat, seeing as by itself, it would
> mean the site you clicked was cracked and you got re-directed to a phony site.
> 
Am I the only one who got stuck with this thinking?

The first chart: Check, makes sense
Second chart: Makes semi-sense. But, why would the pop-up window originate from somewhere else? Am I reading the chart wrong? Apparently a legit site is going to have a pop-up window which... Does absolutely nothing?
Third chart: This site is 123.123.123.123? Is that a pop-up window? And what is "trust this site?" If you type in "paypal.com" go to that page, there's going to be a pop-up about 123.123.123.123? Wouldn't that render that window pointless, seeing as the original window isn't exactly connected to the pop-up?

Can someone clarify? Because I must be confused somewhere.
Comment 28 Jordan M. 2007-02-18 17:38:04 PST
Haha, wow, I feel like an idiot now... Had nothing to do with what I thought it did...

Nevermind comments 27, 23, and 22.

Was being way too literal with the diagrams.
Comment 29 Michael Ventnor 2007-02-18 18:38:54 PST
(In reply to comment #28)
> Haha, wow, I feel like an idiot now... Had nothing to do with what I thought it
> did...
> 
> Nevermind comments 27, 23, and 22.
> 

If we had a "best bugzilla moments" site this would be almost at the top =)

Regarding this bug, I don't think a notification bar is enough. You'd have to listen for changes to the host and dynamically update the bar, not only that but the user may want to know the whole URL rather than just the host.

For the sake of code size and consistency I'd prefer if we can fix whatever's holding us back from flipping the pref then flip it. Based on what I've seen so far, and from my own private pref-flip, most of the concerns from this bug are outdated.
Comment 30 Jordan M. 2007-02-18 18:45:52 PST
(In reply to comment #29)
> (In reply to comment #28)
> > 
> 
> If we had a "best bugzilla moments" site this would be almost at the top =)
> 

Yeah, I have ADD, and I can't think of much at the same time, thus why I pretty much doubled this bug's comments.

Thanks for not slaughtering me. XP
Comment 31 Mike Beltzner [:beltzner, not reading bugmail] 2007-04-02 11:29:39 PDT
Johnathan and I will finalize the design and either come up with a patch or work with someone to do so. This blocks Firefox 3.
Comment 32 Phil Ringnalda (:philor) 2007-04-24 08:01:27 PDT
*** Bug 378554 has been marked as a duplicate of this bug. ***
Comment 33 Johnathan Nightingale [:johnath] 2007-05-24 13:11:01 PDT
Created attachment 265983 [details] [diff] [review]
Patch to add chromeless window detection/handling

Notes on this patch:

- It adds browser/base/content/chromelessWindowOverlay.js to the tree
- It adds a pref (security.warn_chromeless_window.infobar) which defaults to true.  Turning it off (about:config only, this shouldn't be easy) disables the behaviour introduced in the patch.
- It doesn't have whitelist support.  I'm reluctant to introduce *another* whitelist here, and I also don't think it would be right to piggyback on the existing popup whitelist since the meanings/trust decisions are not identical. As schrep says, even without a way to suppress it, we need this behaviour for feature parity and phish-stomping anyhow.  If people want to discuss whitelisting, I would recommend opening a separate, follow-on bug.
Comment 34 Johnathan Nightingale [:johnath] 2007-05-24 13:16:09 PDT
Created attachment 265985 [details]
Screencap of default presentation now
Comment 35 Daniel Veditz [:dveditz] 2007-05-25 18:54:46 PDT
Comment on attachment 265983 [details] [diff] [review]
Patch to add chromeless window detection/handling

>+// Show infobar on chromeless windows
>+pref("security.warn_chromeless_window.infobar", true);

Please use a browser.foo pref, security. is for back-end global security settings (primarily PSM) rather than app-specific ones.

>+  if(document.documentElement.getAttribute("chromehidden").indexOf("toolbar") != -1)

This is probably somewhat tangential, but someone could hide the location bar without hiding the toolbar.

javascript:open("http://www.mozilla.org","","location=no,toolbar=yes")

It doesn't let them add their own fake chrome so maybe it's outside the scope of this bug, but it does allow them to hide where they are. More or less, anyway, the real host would appear in the title bar but you couldn't find the rest of the URL.

Personally I like the current mini-bar, assuming it gets updated to show the new identity indicators. But it's relatively unobtrusive which means maybe it doesn't help as much. But it's not going to get as many intranet-app authors screaming at us, either. I fear those sorts of people will agitate to lock off the feature entirely if there's no way to whitelist their sites.
Comment 36 Daniel Veditz [:dveditz] 2007-05-25 19:09:32 PDT
> Personally I like the current mini-bar

Your screen-shot is way prettier and better for Firefox in general, ignore my pointless obsession with knowing the full URL at all times. I will happily continue using the dom.disable_window_open_feature.location pref but there's no need to foist that on everyone else if we've got a better-looking solution.
Comment 37 Michael Ventnor 2007-05-25 20:22:33 PDT
What if this window gets redirected to a totally different domain? We need a way to keep this microbar up-to-date at all times. Please make sure this listens for location changes, and update the microbar if the host changes.
Comment 38 Michael Ventnor 2007-05-25 20:32:47 PDT
Actually, never mind. It appears you're using the opener instead.

> +chromelessWindow.warningMessage=The web site at %S has hidden your toolbar.
> +chromelessWindow.warningNoLocation=This web site has hidden your toolbar.

s/toolbar/toolbars.
Comment 39 Johnathan Nightingale [:johnath] 2007-05-28 05:44:13 PDT
Created attachment 266373 [details] [diff] [review]
Updated patch with dveditz, ventron feedback

Dan - I like seeing the full URL too, but I agree we are in the minority.  :)  I have added a check for chromehidden location as well.  As you say, I think it's less likely, since it's a harder spoof, but it's annoying and probably deceptive, and this gives us an easy way to bring it back.

Ventron - As you mention, I am using opener, not current url (since it was the opener who hid things) but I have updated the strings as you suggested.
Comment 40 Johnathan Nightingale [:johnath] 2007-05-28 07:17:02 PDT
Created attachment 266380 [details] [diff] [review]
Fix for gBrowser has no properties error

Caught a bug where gBrowser is undefined for windows without web content (e.g. download manager, error console).  Fix to return immediately on !gBrowser, since we don't need to handle those here.
Comment 41 Mike Kowalski 2007-05-28 17:42:25 PDT
Can't this message be spoofed? Look at bug 252257. Opera has a small bar with the url, and when you click it, a full toolbar appears.
Comment 42 Jesse Ruderman 2007-05-28 17:45:01 PDT
It can't be spoofed because the top of the window is always either a real normal location bar or a real microbar.
Comment 43 Johnathan Nightingale [:johnath] 2007-05-28 23:50:58 PDT
(In reply to comment #42)
> It can't be spoofed because the top of the window is always either a real
> normal location bar or a real microbar.

Right - a site could open a chromeless popup and spoof their own infobar, but our infobar would still be present.  And as mentioned in bug 252257, spoofing matters, but it's a generic problem.  I don't see that the spoofing issue is made any worse by this fix, whereas several other spoofing scenarios are prevented. 
Comment 44 Mike Connor [:mconnor] 2007-06-05 09:15:55 PDT
Comment on attachment 266380 [details] [diff] [review]
Fix for gBrowser has no properties error

Index: browser/base/content/chromelessWindowOverlay.js

this is an odd nit, but why have a separate file?  this will just add overhead and extra add/remove listeners.

yes, browser.js is a mess, but breaking stuff into separate files just creates overhead that isn't needed.  After we wrap up Fx3 I want to get someone to split the files into manageable chunks that combine at build time.

+# The Initial Developer of the Original Code is
+#   Johnathan NIGHTINGALE <johnath@mozilla.com>
+#
+# Contributor(s):
+#

typically, aiui, you'd want to have Mozilla Corporation as the initial developer
with you listed as a contributor, but I think this should be in browser.js anyway

+  if(!gBrowser) {
+    return;
+  }

nit, missing space after if (you do this a lot, just stop please ;), braces around single line are bad

+  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
+                    getService(Components.interfaces.nsIPrefBranch);

odd style here too (align periods when not using the Cc and Ci consts
and the alternate style for those)

  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
                        .getService(Components.interfaces.nsIPrefBranch);


 
+    var bundle_browser = document.getElementById("bundle_browser");
+    var messageString;    
+    if(window.content && window.content.opener && window.content.opener.location)
+      messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
+                                                        [window.content.opener.location.host]);
+    else
+      messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");

use instead:

try {
  var messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
                                                        [window.content.opener.location.host]);
} catch (ex) {
  messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");

} 

+    var notificationName = "chromeless-info";
+    var buttons = [{
+      label: bundle_browser.getString("chromelessWindow.showToolbarsButton"),
+      accessKey: bundle_browser.getString("chromelessWindow.accessKey"),
+      popup: null,
+      callback: function() { return showToolbar(); }
+    }];

+    var notificationBox = gBrowser.getNotificationBox();
+    if (!notificationBox.getNotificationWithValue(notificationName)) {

maybe make this 

if (notificationBox.getNotificationWithValue(notificationName)) {
  Components.utils.reportError("Already have a chromeless-info notification!")
  return;
}

and move the buttons stuff after the check (not that this should ever happen, but this is just weird flow control

+      const priority = notificationBox.PRIORITY_INFO_HIGH;
+      const iconURL = "chrome://browser/skin/Info.png";

You're only using these once, why bother with the overhead here?  same with notificationName, really.

+/**
+ * Callback for "Show Toolbars" button in notification box.  Resets visibility of
+ * the go button stack and url bar, basically counteracting the chromehidden work
+ * in browser.js
+ */
+function showToolbar() {
+  
+  // Unhide the chrome elements
+  var chromehiddenStr = document.documentElement.getAttribute("chromehidden");
+  chromehiddenStr = chromehiddenStr.replace("toolbar", "")
+                                   .replace("location", "");
+  document.documentElement.setAttribute("chromehidden", chromehiddenStr);

just nuke chromehidden here, I'd think.

+  // Undo the URLBar tweaks performed in browser.js when the url bar
+  // was chromehidden
+  if(gURLBar) {
+    gURLBar.setAttribute("readonly", "false");
+    gURLBar.setAttribute("enablehistory", "true");
+  }

+  var goButtonStack = document.getElementById("go-button-stack");
+  if (goButtonStack)
+    goButtonStack.setAttribute("hidden", "false");

removeAttribute("hidden") is faster here (same with readonly, above)

+/**
+ * It isn't critical that we run during the onload sequence, so just add
+ * ourselves to the end of the event loop.
+ */
+function startup() {
+  window.removeEventListener("load", checkForChromelessWindow, false);
+  setTimeout(checkForChromelessWindow, 0);
+}
+
+window.addEventListener("load", startup, false);

just call checkForChromelessWindow from browser.js's delayedStartup, no event listeners needed

\ No newline at end of file

bad johnath, no biscuit

Index: browser/base/content/global-scripts.inc
===================================================================
RCS file: /cvsroot/mozilla/browser/base/content/global-scripts.inc,v
retrieving revision 1.13
diff -u -8 -p -r1.13 global-scripts.inc
--- browser/base/content/global-scripts.inc	5 Jan 2007 11:45:15 -0000	1.13
+++ browser/base/content/global-scripts.inc	28 May 2007 14:12:12 -0000
@@ -49,8 +49,9 @@
 #ifndef MOZ_PLACES_BOOKMARKS
 <script type="application/x-javascript" src="chrome://browser/content/bookmarks/bookmarks.js"/>
 <script type="application/x-javascript" src="chrome://browser/content/bookmarks/bookmarksMenu.js"/>
 #endif
 <script type="application/x-javascript" src="chrome://global/content/viewZoomOverlay.js"/>
 <script type="application/x-javascript" src="chrome://browser/content/browser.js"/>
 <script type="application/x-javascript" src="chrome://global/content/inlineSpellCheckUI.js"/>
 <script type="application/x-javascript" src="chrome://global/content/viewSourceUtils.js"/>
+<script type="application/x-javascript" src="chrome://browser/content/chromelessWindowOverlay.js"/>

I think we don't want this in global-scripts, we want it in browser.xul directly

Index: browser/locales/en-US/chrome/browser/browser.properties
===================================================================
RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.properties,v
retrieving revision 1.35
diff -u -8 -p -r1.35 browser.properties
--- browser/locales/en-US/chrome/browser/browser.properties	28 Mar 2007 04:37:32 -0000	1.35
+++ browser/locales/en-US/chrome/browser/browser.properties	28 May 2007 14:12:13 -0000
@@ -86,8 +86,14 @@ tabContext.undoCloseTabAccessKey=U
 menuOpenAllInTabs.label=Open All in Tabs
 menuOpenAllInTabs.accesskey=o
 
 # Block autorefresh
 refreshBlocked.goButton=Allow
 refreshBlocked.goButton.accesskey=A
 refreshBlocked.refreshLabel=%S prevented this page from automatically reloading.
 refreshBlocked.redirectLabel=%S prevented this page from automatically redirecting to another page.
+
+# Chromeless popup handling
+chromelessWindow.warningMessage=The web site at %S has hidden your toolbars.
+chromelessWindow.warningNoLocation=This web site has hidden your toolbars.
+chromelessWindow.showToolbarsButton=Show Toolbars
+chromelessWindow.accessKey=s

S, not s (accesskeys search same case, then opposite case, so this would show at the end instead of the beginning

close, but not there yet
Comment 45 Johnathan Nightingale [:johnath] 2007-06-05 12:31:34 PDT
Created attachment 267309 [details] [diff] [review]
Post-review patch
Comment 46 Johnathan Nightingale [:johnath] 2007-06-05 12:47:41 PDT
(In reply to comment #44)
> this is an odd nit, but why have a separate file?  this will just add overhead
> and extra add/remove listeners.

Sold.  Now incorporated into browser.js.

> typically, aiui, you'd want to have Mozilla Corporation as the initial
> developer
> with you listed as a contributor, but I think this should be in browser.js
> anyway

Yeah, we've covered that.  :)

> nit, missing space after if (you do this a lot, just stop please ;), braces
> around single line are bad

Fixed 5 instances of if(, and removed 2 instances of single-statement braces.  Stupid IBM style guide.

> +  var prefs = Components.classes["@mozilla.org/preferences-service;1"].
> +                    getService(Components.interfaces.nsIPrefBranch);
> 
> odd style here too (align periods when not using the Cc and Ci consts
> and the alternate style for those)

Done.

> use instead:
> 
> try {
>   var messageString =
> bundle_browser.getFormattedString("chromelessWindow.warningMessage",
>                                                        
> [window.content.opener.location.host]);
> } catch (ex) {
>   messageString =
> bundle_browser.getString("chromelessWindow.warningNoLocation");
> } 

Done. 

> +    var notificationBox = gBrowser.getNotificationBox();
> +    if (!notificationBox.getNotificationWithValue(notificationName)) {
> 
> maybe make this 
> 
> if (notificationBox.getNotificationWithValue(notificationName)) {
>   Components.utils.reportError("Already have a chromeless-info notification!")
>   return;
> }
> 
> and move the buttons stuff after the check (not that this should ever happen,
> but this is just weird flow control

Done.

> +      const priority = notificationBox.PRIORITY_INFO_HIGH;
> +      const iconURL = "chrome://browser/skin/Info.png";
> 
> You're only using these once, why bother with the overhead here?  same with
> notificationName, really.

Done for priority and iconURL.  notificationName is used twice, so left as-is.

> +  // Unhide the chrome elements
> +  var chromehiddenStr = document.documentElement.getAttribute("chromehidden");
> +  chromehiddenStr = chromehiddenStr.replace("toolbar", "")
> +                                   .replace("location", "");
> +  document.documentElement.setAttribute("chromehidden", chromehiddenStr);
> 
> just nuke chromehidden here, I'd think.

Done, though I nuked by setting to "" instead of removeAttribute'ng, because there are multiple places in the code where we do:

document.documentElement.getAttribute("chromehidden").indexOf...

so I didn't want to undefine it.

> +    goButtonStack.setAttribute("hidden", "false");
> 
> removeAttribute("hidden") is faster here (same with readonly, above)

Done for both.

> just call checkForChromelessWindow from browser.js's delayedStartup, no event
> listeners needed

Seriously, I promise we're already moved to browser.js.  :)

> Index: browser/base/content/global-scripts.inc

> I think we don't want this in global-scripts, we want it in browser.xul
> directly

Browser.js, I take it you mean, and yes, done.

> +chromelessWindow.showToolbarsButton=Show Toolbars
> +chromelessWindow.accessKey=s
> 
> S, not s (accesskeys search same case, then opposite case, so this would show
> at the end instead of the beginning

Ah, whoops.  Done.

Comment 47 Mike Connor [:mconnor] 2007-06-05 21:02:08 PDT
Comment on attachment 267309 [details] [diff] [review]
Post-review patch


+function checkForChromelessWindow() {
+
+  // Windows without web content (pref dialogs, download manager, etc) don't need
+  // any handling here.
+  if (!gBrowser)
+    return;

gBrowser is definitely defined, or the event listener call 150 lines earlier already threw.

Your option if you're really paranoid is to just replaces gBrowser below with getBrowser() but I think that's not necessary

+  if (document.documentElement.getAttribute("chromehidden").indexOf("toolbar") != -1 ||
+     document.documentElement.getAttribute("chromehidden").indexOf("location") != -1) {

off by one indent

+    var bundle_browser = document.getElementById("bundle_browser");
+    var messageString;
+    try {
+      messageString = bundle_browser.getFormattedString("chromelessWindow.warningMessage",
+                                                        [window.content.opener.location.host]);
+    } catch (ex) {
+      messageString = bundle_browser.getString("chromelessWindow.warningNoLocation");
+    }

no need to define messageString outside of the try, save yourself a line.

r=me with nits fixed
Comment 48 Johnathan Nightingale [:johnath] 2007-06-06 06:29:22 PDT
Created attachment 267411 [details] [diff] [review]
Post-nit patch

Nits addressed, added my name to browser.js contributors list too.
Comment 49 Mike Beltzner [:beltzner, not reading bugmail] 2007-06-06 07:32:47 PDT
Comment on attachment 265985 [details]
Screencap of default presentation now

ui-r+

I'm a little worried about the interaction on sites (especially web applications and gallery sites) that throw up chromeless windows appropriately, but let's start here and see where it takes us.
Comment 50 Johnathan Nightingale [:johnath] 2007-06-06 11:01:22 PDT
Created attachment 267456 [details] [diff] [review]
Updated patch post-IRC discussion

After discussion with Shaver, mconnor, and gavin in IRC, I've put together a slight modification which:

a) adds commenting around the try/catch block
b) logs the exception, if caught (we previously felt this would be log-spam, because there was a timing issue with window.content.opener being set.  The move to the end of browser.js's delayedStartup fixed that, though, so this really is exceptional)
c) Replaces the setAttribute("chromehidden", "") call with removeAttribute("chromehidden");  This was worrisome at first, since other code calls getAttribute("chromehidden").indexOf without sanity checks, but gavin confirmed that getAttr on removed attrs in XUL returns "", not null.
Comment 51 Johnathan Nightingale [:johnath] 2007-06-06 13:14:23 PDT
Updating summary to reflect the direction this bug eventually took.
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2007-06-06 13:57:11 PDT
That last patch caused a mochitest failure, for good reasons.  The effect of showing the infobar in the particular way it's done on the standard |window.open("", "", "width=X,height=Y")| call is that the window is not the size the page requested until the infobar is dismissed.  That means that user has to click away the silly thing to view the image (almost always) the site is trying to show.

Is the expected workflow really that the user will click the bar away on every single image popup (e.g. the various image things on the BBC news site, similar for NY Times, etc) window they want to view?  That seems highly suboptimal.

Frankly, I think we should show whatever site indentity indicator we settle on (url bar replacement, whatever) in all windows at all times.  That should cover this bug, no?
Comment 53 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-06-06 14:04:13 PDT
As bz mentions, this caused a failure of the test for bug 344861 (http://lxr.mozilla.org/seamonkey/source/docshell/test/test_bug344861.html). I've flipped the pref for now, to fix the orange.
Comment 54 Johnathan Nightingale [:johnath] 2007-06-07 11:23:04 PDT
By and large, I'd say the expectation is that users will mostly leave it be, much as I expect a lot of them leave the "we blocked a popup" infobar currently.  Users who want to disable the pref can do so, but really, it exists to prevent a wide-open hole in terms of chrome-spoofing or even just hidden chrome, by giving users a way out ("Show toolbars".)

With that being said, I think earlier posters in this bug are right to observe that people may want a way to whitelist this away.  As the discussion in the bug already indicates, this is something anticipated, but we were hoping to get the behaviour into the alphas and betas, and see if the real world impact was enough to justify adding yet-another-whitelist, or possibly piggybacking of an existing one.

The suggestion that we microbar it instead, that is, that we take the URL-bar plus whatever hangers-on we think appropriate, is a possibility.  In fact, we could do it by just flipping the disable-location-bar pref as the first patch did, but:

a) mconnor and others dismissed this approach above by suggesting it would break other code which watches chromehidden

b) it still puts our users in the position of having no way to get those controls back (maybe this is okay, but it is a difference from the current approach)

c) The current infobar speaks in terms of the opener when it talks about hidden chrome, since that's really the opener's decision, not the content of the current page.  Going to a URL bar obscures that.

My preference would be to turn the pref back on, open a follow-up bug to track the question of whitelisting sites, and in the meantime tell mochitest to disable the pref, since it's really concerned with whether we made the right sized window (which we did), moreso than whether we then overlaid it with something.  

I'm willing to be wrong here, but I agree with schrep in comment 8 when he says (if I'm interpreting his words appropriately) that even without a whitelist way to suppress it, this is something we need for phish-stomping, and for parity with IE7.
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2007-06-07 11:37:30 PDT
If we expect users to leave the infobar in place, then we are NOT making the right-sized window.  The right-sized window is the one that will give the content the amount of space it asked for with the user's default behavior and the resulting chrome.  Keep in mind that the infobar doesn't "overlay" the content window; it just makes it smaller.

If the testcase were testing outerWidth/outerHeight window options, then the infobar would not matter.  But that's not what it tests, and what it tests is what we don't want to regress.

So if we stick with the current approach we need to be showing the infobar early enough that the window-sizing code that runs after onload will see the right chrome size.
Comment 56 Johnathan Nightingale [:johnath] 2007-06-07 12:50:47 PDT
(In reply to comment #55)
> So if we stick with the current approach we need to be showing the infobar
> early enough that the window-sizing code that runs after onload will see the
> right chrome size.

Would doing that cause the window to be too big if they did dismiss it?

Comment 57 Nelson Bolyard (seldom reads bugmail) 2007-06-07 13:07:47 PDT
Question:

I currently try to eliminate chromeless windows with these preferences:
It works in MOST (but, sadly, not all) cases.

user_pref("dom.disable_open_during_load", true);
user_pref("dom.disable_window_open_feature.close", true);
user_pref("dom.disable_window_open_feature.directories", true);
user_pref("dom.disable_window_open_feature.location", true);
user_pref("dom.disable_window_open_feature.menubar", true);
user_pref("dom.disable_window_open_feature.minimizable", true);
user_pref("dom.disable_window_open_feature.personalbar", true);
user_pref("dom.disable_window_open_feature.resizable", true);
user_pref("dom.disable_window_open_feature.scrollbars", true);
user_pref("dom.disable_window_open_feature.titlebar", true);
user_pref("dom.disable_window_open_feature.toolbar", true);

My question is: with this new notification bar feature, 
will I see both the chrome *AND* the notification bar in otherwise
chromeless windows?  (Hope not.)
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2007-06-07 13:59:48 PDT
> Would doing that cause the window to be too big if they did dismiss it?

Yes.  I really don't see a good way around that problem, to be honest, if we're using this transient UI.  I suppose we could shrink the window in the process of closing the notification bar, but that's a bit more work.

And given the choice, I'd take a few pixels too big over a few pixels too small from a usability perspective.  But this does hinge on the relative frequencies of users leaving it as-is and dismissing it...
Comment 59 inarius 2007-06-07 15:58:04 PDT
Speaking of window size / position, wouldn't there be an easy way to circumvent this warning message by positioning the window above the screen?

If, for instance, the warning bar is 20 pixels tall, the spoofer could simply position the window with y = -20 and the first 20 pixels of the web page could be something inconspicuous, like a fake title bar.

I don't know how feasible this is... just a thought.
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2007-06-07 16:05:08 PDT
> by positioning the window above the screen?

You're only allowed to do that if you have extra privileges.  See nsWindowWatcher::SizeOpenedDocShellItem the |if (!enabled)| clause; the one that starts with the comment:

  1903     // Security check failed.  Ensure all args meet minimum reqs.

Of course a 2-minute test of window.open would have shown that a negative top doesn't work... ;)
Comment 61 Mark Finkle (:mfinkle) (use needinfo?) 2007-06-08 06:49:58 PDT
Created attachment 267713 [details]
example of using a notificationbox in a popup with transparency

Just an rudimentary example of how a notificationbox could be popped up over the content. The notification is semi-transparent so you can see some content under it. a click anywhere (on or off the notification) will dismiss the notification
Comment 62 Dão Gottwald [:dao] 2007-06-08 07:12:42 PDT
(In reply to comment #61)
> Created an attachment (id=267713) [details]
> example of using a notificationbox in a popup with transparency

Transparency doesn't seem to work for me, and even if it did, it would only be suboptimal. More important, overlaying the scroll bar seems like a real mistake.

Overall, I think flipping the Location Bar pref and fixing "window targeting behaviour that relies on chromehidden" is the obvious and right solution for this bug.
Comment 63 Boris Zbarsky [:bz] (still a bit busy) 2007-06-08 08:33:14 PDT
I don't think we support popup transparency across all our platforms (though I could be wrong).
Comment 64 Dão Gottwald [:dao] 2007-06-20 01:53:25 PDT
(In reply to comment #62)
> fixing "window targeting behaviour that relies on chromehidden"

The popups' Location Bar is readonly. How about adding that to the heuristics?
Comment 65 Johnathan Nightingale [:johnath] 2007-06-27 18:23:06 PDT
Created attachment 270106 [details] [diff] [review]
Toggle the pref and todo the mochitests

mconnor suggested turning this on for A6 so that we get some more data on the scope of the impact.  That means todo'ng the mochitests in docshell/test/test_bug344861.html as well.  

After A6 goes out the door, we bring back the mochitest and toggle the pref back off, but at least we get some more user info.
Comment 66 Jesse Ruderman 2007-06-27 18:46:57 PDT
We *know* that if we check this in with the problems the mochitests hit (comment 52), we'll break tons of sites unnecessarily.  Turning this on for A6 will just give us more noise, less testing of other changes, and less useful testing/feedback when we fix this correctly.
Comment 67 Boris Zbarsky [:bz] (still a bit busy) 2007-06-27 19:17:25 PDT
For what it's worth, the only way I'm at all OK with that patch landing is in the release freeze when we're absolutely sure nothing else will land between it and release.  And I'd want it backed out before the tree reopens.  I don't want us missing other regressions in this.

All that said, I pretty much agree with Jesse.
Comment 68 Johnathan Nightingale [:johnath] 2007-06-27 19:22:56 PDT
So the goal here was to get some data on how much breakage it actually created,
and also to see if the UI was useful enough to justify the effort of fixing the
sizing bugs that break the tests.  I do think a dismissible/whitelistable
notify bar is a better UI option than just locking the location bar on.

But I also agree that the time to do it is not on the eve of A6.

What do you think about leaving it as-is for A6, and flipping it once the tree
re-opens post-A6, to at least get it in front of the people dogfooding?  
Comment 69 Mike Connor [:mconnor] 2007-06-27 19:36:48 PDT
I guess I'm not understanding just how fragile/breakable this behaviour is, and why this test is so critical that we can't have it off for even a single checkin.

FWIW, if any content triggers the popup blocked or missing plugins notification bar, they've been broken on that test since before Firefox 1.0, so this is just making that problem global, instead of buried in edge cases.
Comment 70 Boris Zbarsky [:bz] (still a bit busy) 2007-06-27 19:57:24 PDT
I'd say that any regression test shouldn't be turned off once it's on.  This one is not special.  Given that, I'd have an even bigger problem with flipping this on trunk for a while than I do with doing it just for the release.

As for the other notifications, the most common (by far) use cases of sized windows don't hit those situations, as you noted.  So it hasn't really been a problem.  It's making every single sized window broken that's something we want to avoid.
Comment 71 Carsten Book [:Tomcat] 2007-07-03 05:16:30 PDT
*** Bug 340891 has been marked as a duplicate of this bug. ***
Comment 72 Johnathan Nightingale [:johnath] 2007-08-22 12:14:28 PDT
Created attachment 277763 [details] [diff] [review]
Flip the dom.disable_window_open_feature.location pref and fix the window targetting

This patch flips the original pref to disable hiding of the location bar.  It also fixes the behaviour in nsBrowserContentHandler.js which was depending on 

window.toolbar.visible

replacing the check with 

!win.document.documentElement.getAttribute("chromehidden")
Comment 73 Nelson Bolyard (seldom reads bugmail) 2007-08-22 13:40:22 PDT
Johnathan, does this mean that people who (like me) set set 
dom.disable_window_open_feature.location to true to disable hiding of 
the location bar must now reverse the sense of our preferences?
Comment 74 Johnathan Nightingale [:johnath] 2007-08-22 14:08:37 PDT
(In reply to comment #73)
> Johnathan, does this mean that people who (like me) set set 
> dom.disable_window_open_feature.location to true to disable hiding of 
> the location bar must now reverse the sense of our preferences?

I'm not sure I understand your question but nothing should change for you -- you've already disabled locbar hiding, it will continue to be disabled.  This just changes new profiles to also set that pref by default.  

The changes to the targetting code mean that you might notice improved behaviour for external link clicks, which previously might have opened in your popup windows (if they were top-of-z-order) and should now open in real browser windows instead.

Comment 75 Mike Connor [:mconnor] 2007-08-22 20:26:59 PDT
Comment on attachment 277763 [details] [diff] [review]
Flip the dom.disable_window_open_feature.location pref and fix the window targetting

let's get this landed.

I'm still interested in the notification, but that's not in scope anymore.
Comment 76 Johnathan Nightingale [:johnath] 2007-08-23 06:16:18 PDT
I've confirmed that this patch passes mochitest as well.  Setting checkin-needed.
Comment 77 Boris Zbarsky [:bz] (still a bit busy) 2007-08-23 10:09:59 PDT
*** Bug 338225 has been marked as a duplicate of this bug. ***
Comment 78 Dave Townsend [:mossop] 2007-08-23 11:49:18 PDT
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.197; previous revision: 1.196
done
Checking in browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.43; previous revision: 1.42
done
Comment 79 Daniel Veditz [:dveditz] 2007-08-23 20:17:21 PDT
*** Bug 378554 has been marked as a duplicate of this bug. ***
Comment 80 Bob Clary [:bc:] 2007-09-28 04:35:28 PDT
*** Bug 397873 has been marked as a duplicate of this bug. ***
Comment 81 Phil Ringnalda (:philor) 2007-10-05 15:37:37 PDT
If, as it appears, the attachment 267456 [details] [diff] [review] notification bar is going to stay in the tree preffed off, you might want to in-litmus? it, since it's quite unlikely that anyone mucking with toolbars or the addressbar will be checking the effects of their work on something so thoroughly hidden away (I'd completely forgotten it, until I was looking for things using PRIORITY_INFO_HIGH).
Comment 82 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-05 17:19:54 PDT
(In reply to comment #81)
> If, as it appears, the attachment 267456 [details] [diff] [review] notification bar is going to
> stay in the tree preffed off

It's not, see bug 397695.
Comment 83 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-07 17:40:05 PST
*** Bug 411216 has been marked as a duplicate of this bug. ***
Comment 84 t mau 2009-03-19 11:39:08 PDT
What I haven't read in this bug is discussion about other possible solutions. 

I gathered that you're trying to make sure phishing sites remain obvious to reasonably smart users. THat's great.

How about web applicatinos design? If the window.open() is fired from an explicit click by the end user, and if the new window is on the same domain as the referrer, then why can't the location bar be hidden?

I'm not familiar with how this is built, but it seems a possible way to appease web apps builders who are very much keen on controlling window size/behavior in order to deliver the best user experience.
Comment 85 Boris Zbarsky [:bz] (still a bit busy) 2009-03-19 12:17:56 PDT
I don't see how the "if the new window is on the same domain as
the referrer" helps anything.  The whole problem is windows that are on the same site as the referrer, since that site is not the site the user thinks it's looking at.
Comment 86 Daniel Veditz [:dveditz] 2009-03-23 11:52:21 PDT
And the popup can be navigated once opened, which is either going to lead to spoofing or ugly jittering as the location mini-bar opens and closes.

> I gathered that you're trying to make sure phishing sites remain obvious to
> reasonably smart users. THat's great.

That's part of the goal, but even on non-phishing sites we believe users should be able to know where they are. The users are not asking that the mini-bar go away so it's clearly not bothering them, and clever designers take that space into account. They'll have to -- IE behaves this way too.

Incidentally, random advocacy thrown into a RESOLVED FIXED bug isn't the way to get changes made. Especially for UI changes you need to convince people in our product newsgroups that it's a good change.

Note You need to log in before you can comment on or make changes to this bug.