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

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P5)

Description

[Gérard Talbot]

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

Comment 1

[Gérard Talbot]

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.

Comment 2

[Gérard Talbot]

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

Comment 3

[Jesse Ruderman]

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.

Comment 4

[Martijn Wargers (dead)]

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

Comment 5

[Martijn Wargers (dead)]

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.

Comment 6

[Martijn Wargers (dead)]

Ok, that doesn't work. Firefox crashes when I make those properties readonly, so
I guess it is needed for xul.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase

Comment 8

[Martijn Wargers (dead)]

Attachment: 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.

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase

This testcase also includes screen.top and screen.left which becomes also readonly by the patch.

Comment 10

[Martijn Wargers (dead)]

Oh, sorry screen.top and screen.left already throw when setting. But still useful for testing.

Comment 11

[Martijn Wargers (dead)]

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

Comment 12

[Martijn Wargers (dead)]

Attachment: mochikit testcase

Comment 13

[Martijn Wargers (dead)]

Attachment: mochikit testcase

Also testing the dom.disable_window_move_resize pref now.

Comment 14

[Johnny Stenback (: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).

Comment 15

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 16

[David]

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 17

[Johnny Stenback (:jst)]

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...

Comment 18

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 19

[Anne (:annevk)]

This got fixed at some point. Maybe when converting stuff to WebIDL.

Comment 20

[Jari Jalkanen [:jari]]

Attachment: Bug 1769057 - Fix static analysis issues. r=#dom-storage-reviewers

Comment 21

[Phabricator Automation]

Comment on attachment 9301537 [details]
Bug 1769057 - Fix static analysis issues. r=#dom-storage-reviewers

Revision D161025 was moved to bug 1769057. Setting attachment 9301537 [details] to obsolete.