Closed Bug 337344 Opened 19 years ago Closed 17 years ago

Disable location bar hiding by default, to make chrome spoofing harder

Categories

(Firefox :: Address Bar, enhancement)

2.0 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: dveditz, Assigned: johnath)

References

Details

(Whiteboard: [sg:want P2] anti-spoofing 181b1+)

Attachments

(2 files, 9 obsolete files)

977 bytes, application/vnd.mozilla.xul+xml
Details
3.28 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
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.
Flags: blocking-firefox2?
Whiteboard: [sg:want] anti-spoofing
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
Flags: blocking-firefox2? → blocking-firefox2+
Depends on: 258405
Depends on: 338887
Depends on: 339395
Depends on: 339192
No longer depends on: 338887
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.
Target Milestone: --- → Firefox 2 beta1
Hey Mike, here's one to pass to the interns ;-)
Assignee: bugs → mconnor
(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)| |---------------------------------------------------------------------| | | . . . . . .
Indeed!
Assignee: mconnor → jwalden+bmo
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?
(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."
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.
Whiteboard: [sg:want] anti-spoofing → [sg:want] anti-spoofing 181b1+
Attached patch Flip the pref (obsolete) — Splinter Review
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.
Attachment #227346 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
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 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.
Attachment #227346 - Flags: review?(mconnor) → review-
Whiteboard: [sg:want] anti-spoofing 181b1+ → [sg:want] anti-spoofing 181b1+ [needs new patch]
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! :)
Assignee: jwalden+bmo → dveditz
Status: ASSIGNED → NEW
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
*** Bug 348373 has been marked as a duplicate of this bug. ***
*** Bug 361742 has been marked as a duplicate of this bug. ***
Whiteboard: [sg:want] anti-spoofing 181b1+ [needs new patch] → [sg:want P2] anti-spoofing 181b1+ [needs new patch]
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.
it'll break the external link handling code that bypasses popups by checking chromehidden. Beltzner's solution is preferred anyway, per comment 11
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?
Depends on: 370785
Did you try it on the Mac? WFM fine on windows, but mconnor demonstrated some problems on a Mac.
(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.
Target Milestone: Firefox 2 beta2 → Future
Target Milestone: Future → ---
(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.
(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.
(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?)
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.
(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?
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).
Me and my silly mistakes: bug 370555
(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.
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.
(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.
(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
Flags: blocking-firefox3?
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.
Assignee: dveditz → johnath
Flags: blocking-firefox3? → blocking-firefox3+
Summary: Change default dom.disable_window_open_feature.location to true → Redesign location microbar and change default for dom.disable_window_open_feature.location to true
Blocks: 378554
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.
Attachment #227346 - Attachment is obsolete: true
Attachment #265983 - Flags: review?
Attachment #265983 - Flags: review? → review?(mconnor)
Attached image Screencap of default presentation now (obsolete) —
Attachment #265985 - Flags: ui-review?
Attachment #265985 - Flags: ui-review? → ui-review?(beltzner)
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.
> 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.
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.
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.
Target Milestone: --- → Firefox 3 alpha6
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.
Attachment #265983 - Attachment is obsolete: true
Attachment #266373 - Flags: review?(mconnor)
Attachment #265983 - Flags: review?(mconnor)
Whiteboard: [sg:want P2] anti-spoofing 181b1+ [needs new patch] → [sg:want P2] anti-spoofing 181b1+
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.
Attachment #266373 - Attachment is obsolete: true
Attachment #266380 - Flags: review?(mconnor)
Attachment #266373 - Flags: review?(mconnor)
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.
It can't be spoofed because the top of the window is always either a real normal location bar or a real microbar.
(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.
Status: NEW → ASSIGNED
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
Attachment #266380 - Flags: review?(mconnor) → review-
Attached patch Post-review patch (obsolete) — Splinter Review
Attachment #266380 - Attachment is obsolete: true
Attachment #267309 - Flags: review?(mconnor)
(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 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
Attachment #267309 - Flags: review?(mconnor) → review+
Attached patch Post-nit patch (obsolete) — Splinter Review
Nits addressed, added my name to browser.js contributors list too.
Attachment #267309 - Attachment is obsolete: true
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.
Attachment #265985 - Flags: ui-review?(beltzner) → ui-review+
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.
Attachment #267411 - Attachment is obsolete: true
Updating summary to reflect the direction this bug eventually took.
Summary: Redesign location microbar and change default for dom.disable_window_open_feature.location to true → Inform users with notificationbar message on chromeless windows, to prevent chrome spoofing
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?
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.
No longer depends on: 258405, 339192, 339395, 370785
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.
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.
(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?
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.)
> 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...
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.
> 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... ;)
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
(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.
I don't think we support popup transparency across all our platforms (though I could be wrong).
(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?
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.
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.
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.
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?
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.
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.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Target Milestone: Firefox 3 M7 → Firefox 3 M8
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")
Attachment #265985 - Attachment is obsolete: true
Attachment #267456 - Attachment is obsolete: true
Attachment #270106 - Attachment is obsolete: true
Attachment #277763 - Flags: review?(mconnor)
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?
(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 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.
Attachment #277763 - Flags: review?(mconnor) → review+
I've confirmed that this patch passes mochitest as well. Setting checkin-needed.
Keywords: checkin-needed
Summary: Inform users with notificationbar message on chromeless windows, to prevent chrome spoofing → Disable location bar hiding by default, to make chrome spoofing harder
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 393900
Blocks: 397695
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).
(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.
Depends on: 408678
Depends on: 445542
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.
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.
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.
Blocks: 485237
Blocks: 501762
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: