152256 - window.(menubar|toolbar|locationbar|...).visible don't require write privelege

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[John Morrison]

Traditionally, the 'visible' various toolbar elements of the UI required the
UniversalBrowserWrite privelege to be modified (i.e., hidden). I happened to
notice that this is no longer a requirement (and it wasn't in 6.2 either).
Not sure when this changed.

I'm not sure how worked up to get about this, since the same effect can be
accomplished with the following (although Nav4.x doesn't allow this either):
    <script>
      window.open("http://www.mozilla.org/", "", 
                  "locationbar=no,statusbar=no,menubar=no");
      window.close();
    </script>

Example:
-----
<script>
  // scrollbars has no effect (neither sets visibility nor flags error)
  //window.setTimeout("window.scrollbars.visible  = true;",  8 * delay);
  var delay = 1000;
  window.setTimeout("window.menubar.visible     = false;", 1 * delay);
  window.setTimeout("window.toolbar.visible     = false;", 2 * delay);
  window.setTimeout("window.locationbar.visible = false;", 3 * delay);
  window.setTimeout("window.personalbar.visible = false;", 4 * delay);
  window.setTimeout("window.statusbar.visible   = false;", 5 * delay);
  window.setTimeout("window.statusbar.visible   = true;",  6 * delay);
  window.setTimeout("window.personalbar.visible = true;",  7 * delay);
  window.setTimeout("window.locationbar.visible = true;",  8 * delay);
  window.setTimeout("window.toolbar.visible     = true;",  9 * delay);
  window.setTimeout("window.menubar.visible     = true;", 10 * delay);
</script>

Comment 1

[John Morrison]

Attachment: testcase

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

happens on linux too

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Well, we just added a hidden pref to always show the status bar, regardless of
window configuration. As for the rest, as you point out, a script can accomplish
the same thing through window.open options, so I agree this probably isn't a big
deal, but let's get some other opinions.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, this bypasses the preference that we just added.  It focused in on
only preventing sites from doing window.open(url, name, 'statusbar=no'), but a
site is still able to open a window, and then through JS if they get a reference
to the opened window, they can easily do window.statusbar.visible=false to hide
it...

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Statusbar fix v.1.0

Part 2 of bug 107949.  :-)  Eventually, we'll need to tie this up to the other
hidden prefs, but for now since we actually expose the statusbar pref, and we
want this as its security related, I am just preventing sites from doing
window.statusbar.visible = false; IFF the user has disabled it in their prefs.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Fix v.1.1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 88257 [details] [diff] [review]
Fix v.1.1

>+#include "nsIPref.h"

This is deprecated; how about including nsIPrefBranch.h/nsIPrefService.h
instead?

>+    nsCOMPtr<nsIPrefBranch> prefBranch;

Move this down into the |if| where it's actually used?

>+        prefBranch->GetBoolPref("status", &forceEnable);

Check the return value?

>+        if (forceEnable && !GlobalWindowImpl::IsCallerChrome()) {
>+          aVisible = PR_TRUE;

Why not just an early return?

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 1.2

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 88259 [details] [diff] [review]
1.2

r=bzbarsky

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 88259 [details] [diff] [review]
1.2

sr=jst

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Statusbar patch landed on the trunk.

Mitch, can you hook me up with the approvals needed for the branch, as an
extension of the other fix?  :)

Comment 12

[Jaime Rodriguez, Jr.]

removing adt1.0.1, until this has been reviewed by the Nav Triage team.

Comment 13

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 14

[Paul Wyskoczka]

adding nsbeta1+ per nav triage team.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

attachment #88259 [details] [diff] [review] has landed on the branch a while ago.  just forgot to mark it.
 This bug is still to be left open as there are other cases that we need to handle.

Comment 16

[lchiang]

bsharma - can you verify this bug fix in 1.01 branch?  When verified, pls
replace fixed1.0.1 keyword with verified1.0.1.  Thanks.

Comment 17

[bsharma]

Verified on 2002-08-24- branch build on Win 2000.

The test case loads fine.

Comment 18

[Paul Wyskoczka]

adt:  Has this been checked into the trunk?

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes, attachment 88259 [details] [diff] [review] landed on the trunk.  This bug has remained open still for
futher work.  Futuring.

Comment 20

[Mitchell Stoltz (not reading bugmail)]

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

Comment 21

[Mitchell Stoltz (not reading bugmail)]

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

Comment 22

[Mitchell Stoltz (not reading bugmail)]

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

Comment 23

[Josh Birnbaum]

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

Comment 24

[Hixie (not reading bugmail)]

caillon: Can you summarise what needs to be done for this bug to be considered
fixed?

Comment 25

[Dan M]

Attachment: require UniversalBrowserWrite to set *bar visibility

The patch previously checked in to address the statusbar is incorrect. The
"spec," such as it is, calls for UniversalBrowserWrite privileges to modify
*bar visibility. See for example
http://devedge.netscape.com/library/manuals/2000/javascript/1.3/reference/window.html#1202556.


Clearly the more functional UBW privilege check should be invoked. It's
arguable whether the disable_window_open_feature prefs should also be checked.
I say no.

This patch backs out (most of) the previous patch and replaces it with an
implementation which covers all the *bars in the way required by the "spec."

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 143231 [details] [diff] [review]
require UniversalBrowserWrite to set *bar visibility

Aha!  I was looking through devedge when I first wrote this, and never saw that
documented.  Makes much more sense.  I'll steal this review request since I did
the part you are backing out.  r=me.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, do we still need the nsIPref*.h includes?

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 143231 [details] [diff] [review]
require UniversalBrowserWrite to set *bar visibility

What caillon just said.

sr=jst

Comment 29

[Dan M]

Checked in for 1.7b with Pref includes also removed

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

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