window.screenX and window.screenY should be read-only values.

NEW
Unassigned

Status

()

Core
DOM: Core & HTML
15 years ago
5 years ago

People

(Reporter: Gérard Talbot, Unassigned)

Tracking

({dom0})

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
The only way to reposition or move a window (main, child or secondary) should be
via the moveTo(x_coordinate, y_coordinate) or the moveBy(deltaX, deltaY)
methods. The only exception of setting top/left/screenX/screenY values is in the
window features string list in the window.open() method for/when creating a
child|secondary window.

Right now, a javascript function such as
function move(X,Y){
window.screenX = eval(X);
window.screenY = eval(Y);
}
which can be found at
http://bugzilla.mozilla.org/attachment.cgi?id=93792&action=view 
makes it possible to set the screenX and screenY values and then
moves/repositions the window.

Known documentation clearly indicates
"Security:
Setting the value of the screenY property requires the UniversalBrowserWrite
privilege."
found at
http://developer.netscape.com/docs/manuals/js/client/jsref/window.htm#1203394

Even MSIE does not make screenLeft and screenTop values settable
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/scrollleft.asp
http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/scrolltop.asp
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

15 years ago
Keywords: 4xp
(Reporter)

Comment 1

15 years ago
I read again the Client-Side JavaScript Guide and Client-Side JavaScript
Reference. Although nowhere could I explicitly read that window.screenX and
window.screenY are read-only values, nevertheless the whole spirit of the
documentation suggests these are read-only values, only gettable values, not
directly settable values. The same spirit also applies as well to other
[private] window object properties/members such as outerWidth, outerHeight,
innerWidth, innerHeight, closed, etc..

In such typical object-oriented environment, object encapsulation clearly
indicates to use proper [and pre-defined] methods to modify such private
members. If window.screenX and window.screenY are not 100% pure read-only
values, then the very least is that they should NOT be directly settable like a
custom user-defined variable. The ONLY way to set, reset window.screenX and
window.screenY values should be via the moveTo() and moveBy() public methods.
Even in an environment with expanded security priviledges.

One obvious idea of promoting exclusive usage of moveTo() and moveBy()
predefined methods is usability and security oriented: to prevent and avoid
[any] part of the content area of windows being [re-]positioned/relocated off
the screen view, outside the view of the user.
(Reporter)

Updated

15 years ago
Keywords: dom0
(Reporter)

Comment 2

15 years ago
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowInternal.idl#99

Comment 3

15 years ago
Allowing scripts to change screenX and screenY is not a security issue, because
the setter can check the same conditions that moveTo and moveBy check.

I think Mozilla should not support setting screenX and screenY because:
1. Changing the coordinates separately might be slower/uglier on some platforms.
 (I haven't tested any.)
2. Allowing it could confuse web developers who test Mozilla first and older
browsers later.

Updated

13 years ago
Assignee: danm.moz → nobody
I think the relevant code is here:
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1818
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#1862

Just removing that code would not cause any problems, afaics:
http://lxr.mozilla.org/seamonkey/search?string=setscreen
Humm, no, that code might be useful for xul windows, maybe, need to investigate
Probably this bug can be fixed here:
http://lxr.mozilla.org/seamonkey/source/dom/public/idl/base/nsIDOMWindowInternal.idl#104
by making those properties readonly.
Ok, that doesn't work. Firefox crashes when I make those properties readonly, so
I guess it is needed for xul.
Created attachment 268236 [details]
testcase
Created attachment 268238 [details] [diff] [review]
patch

This makes screenX and screenY only available for privileged content.
And this makes Mozilla throw when trying to set it from unprivileged content.
That's what IE7 is doing when trying to set window.screenLeft.
Created attachment 268245 [details]
testcase

This testcase also includes screen.top and screen.left which becomes also readonly by the patch.
Attachment #268236 - Attachment is obsolete: true
Oh, sorry screen.top and screen.left already throw when setting. But still useful for testing.
Ugh, the devmo documentation for screen.left and screen.top was misleading :(
I've improved it now:
http://developer.mozilla.org/en/docs/DOM:window.screen
http://developer.mozilla.org/en/docs/DOM:window.screen.left
http://developer.mozilla.org/en/docs/DOM:window.screen.top

Updated

10 years ago
Attachment #268238 - Flags: review?(jst)
Created attachment 268247 [details] [diff] [review]
mochikit testcase
Attachment #268247 - Flags: review?(jst)
Created attachment 268254 [details] [diff] [review]
mochikit testcase

Also testing the dom.disable_window_move_resize pref now.
Attachment #268247 - Attachment is obsolete: true
Attachment #268247 - Flags: review?(jst)

Updated

10 years ago
Attachment #268254 - Flags: review?(jst)
Comment on attachment 268254 [details] [diff] [review]
mochikit testcase

This seems like it could break pages out there that rely on this by now. Not sure how much that matters, but I think if we change this we should make the properties "readonly replaceable", which we do for other readonly properties already (see IsReadonlyReplaceable() in nsDOMClassInfo.h. That way the properties remain settable, but they don't actually attempt to set the underlying screen x and y. W/o something like that we'd end up throwing JS exceptions in code that sets these properties, which means the script would terminate (unless they catch exceptions etc, which is unlikely).
Attachment #268254 - Flags: review?(jst) → review-
Filter on "Nobody_NScomTLD_20080620"
QA Contact: desale → general

Comment 16

7 years ago
It would be awesome if these settings should be read-only.  For me it is unclear that screenX, and screenY should be reserved keywords.  This also exists as a bug in the latest version of processing.js! (0.9.1), with the lines:

This is a snippet of the code from processing.js
var p = this;
/* snip */
p.screenX = function screenX( x, y, z ) { /* snip */ };
p.screenY = function screenY( x, y, z ) { /* snip */ };

Just executing that code, anywhere, the simple act of defining those functions, will move your browser window!  (if the permission is enabled).  Seems like a nasty bug.  Especially since there isn't a window.onMove event to subscribe to?
Comment on attachment 268238 [details] [diff] [review]
patch

Sorry for letting this slip through the cracks :(. r- since we should fix this by making these properties readonly replaceable, or fix this with the new bindings once we use them for window. And this is something we should take up with the specs as well and make sure we're all in agreement...
Attachment #268238 - Flags: review?(jst) → review-
You need to log in before you can comment on or make changes to this bug.