159424 - Objects cannot be passed between windows. Scope for objects seems to be changed.

Status: RESOLVED FIXED

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

Description

[Mihail Daskalov]

Hi, 
I cannot be sure this is a bug, or correct behaviour, but a form with Javascript
that previously worked now doesn't work.
See testcases that I will upload soon.

Last it worked with nightly builds before Jul,16 (this is not exact date when it
stopped working), and doesn't work with Mozilla 1.1b.
For sure it worked with 2002053012 build
and 2002070608 nightly build.
The first build I noticed the problem in was 2002071608.
So it broke somewhere between 7th and 16th of July.

This might be a security enhancement or something else. If this is the case
sorry for the spam but I couldn't find anything similar in bugzilla.

Not the description of the problem:
If I declare a variable in javascript in a document and then open a window,
the from the newly opened window I cannot directly access the variable defined
in the opener.
This is breaking a lot of scripts.
The variable defined can still be accessed as opener.variable, but not as
variable, or top.variable.
In previous builds, and also in NS4 and IE4-IE6 this was possible.

Someone more knowledgable about JavaScript,ECMA etc. standards please check this.

I will upload a testcase where it works (with opener), and one where it doesn't
(without any prefix).

Comment 1

[Mihail Daskalov]

Attachment: Testcase which worked with older builds, IE, NS4 but not with recent Mozillas. Unzip and open form.html

Comment 2

[Mihail Daskalov]

Attachment: This works is a testcase to see the expected behaviour of the previous one.

Comment 3

[Phil Schwartau]

Browser, not engine ---> DOM Level 0.
Nice testcases!

Confirming bug with Mozilla trunk binary 20020723xx. When I try
the first testcase in this build, I get the following error in
the JS Console from clicking links in the child window "lov.html":


Error: top.ifield1 has no properties
Source File: 
file:///C:/WINNT/Profiles/pschwartau/Desktop/159424/moz_testcase/lov.html
Line: 11


The identifier |ifield1| is defined in the parent window "form.html":


<script type="text/javascript">
var ifield1;
var ifield2;
var chooser;

function choose_value()
{
  ifield1 = document.forms[0].realvalue; 
  ifield2 = document.forms[0].display_value;

  chooser = window.open('lov.html','chooser',
  'toolbar=no,menubar=no,scrollbars=yes,resizable=yes,width=500,height=200');

  chooser.ifield1 = ifield1; 
  chooser.ifield2 = ifield2;
  return true;
}
</script>


<form METHOD=GET>
<table>
<tr>
<td width="80%">
<input type="HIDDEN" name=realvalue value="">    <<<----- the "realvalue" elt
              
            etc.
            etc.



When I try the same steps with a build from July 1, I get no errors
and everything works as expected -

Comment 4

[Phil Schwartau]

Here is the code from testcase #1, that used to work but now is erroring:

function select(f,v)
{
  top.ifield1.value = f;
  top.ifield2.value = v;
  top.close();
  return true;
}


Here is the code from testcase #2 (that is not erroring):

function select(f,v)
{
  opener.ifield1.value = f;
  opener.ifield2.value = v;
  top.close();
  return true;
}

Comment 5

[Mihail Daskalov]

Actually I don't know which way it should work, and cannot determine where is
the problem. It might be two things.

<script type="text/javascript">
var ifield1; (is this the variable that allows you assigning "1" to the hidden
input using opener.ifield1.value=f)
var ifield2; 
var chooser;

function choose_value()
{
  ifield1 = document.forms[0].realvalue; 
  ifield2 = document.forms[0].display_value;

  chooser = window.open('lov.html','chooser',
  'toolbar=no,menubar=no,scrollbars=yes,resizable=yes,width=500,height=200');

  chooser.ifield1 = ifield1; (I think the should create a variable visible in
the default scope in lov.html, so that it could be used as ifield1.value=f;)
  chooser.ifield2 = ifield2;
  return true;
}
</script>


1. chooser.ifield1 = ifield1 doesn't create ifield1 visible in chooser.

or

2. var ifield1; doesn't create a variable which is accessible as top.ifield1


The 1. situation seems more probable to me.

Regards

Comment 6

[Johnny Stenback (:jst)]

Peter, could you investigate please?

Comment 7

[Mihail Daskalov]

Hi, this bug is breaking a lot of applications,
for example WEBMIN - Web Administration package,
many other web pages where you have LOV functionality,
including Oracle's Business Components for Java JSP pages.

Any idea what is causing this?

Comment 8

[Mihail Daskalov]

Can this be something related to 
BUG 154930
and similar to 
BUG 160339


http://bugzilla.mozilla.org/show_bug.cgi?id=160339
http://bugzilla.mozilla.org/show_bug.cgi?id=154930

I noticed that the time of fix of 154930 is somewhere around jul,15th so it
could be that fixing bug 154930 caused bug 159424

Comment 9

[Bob Clary [:bc] (inactive)]

I don't think bug 154930 caused this bug since bug 154930 has been fixed on
branch and trunk and the test case for this bug wfm in 1.0.1/20020801/win2k but
does not work in 1.1b/20020803/win2k

Comment 10

[Mihail Daskalov]

I created a new much simpler testcase which definitly points where the problem is.

if you do
chooser = window.open('lov.html',  'chooser');
then
chooser.passed_value = something
doesn't work.

it is available on http://217.75.131.39:7070/mozilla_demo/form.html.

I lookup on NSGlobalWindow.cpp in lxr but couldn't definitly find out what
caused the problem.
I even don't have any idea.
But for sure this is causing problems with various applications.

Comment 11

[Phil Schwartau]

Using Mozilla 1.0-branch binary 20020806xx on WinNT.
Interesting:

When I try the testcase http://217.75.131.39:7070/mozilla_demo/form.html,
I get this error in the JS Console:

         Error: uncaught exception:
         Permission denied to set property Window.passed_value

This causes subsequent errors like

         Error: passed_value is not defined
         Source File: http://217.75.131.39:7070/mozilla_demo/lov.html
         Line: 11

         etc.

BUT: when I save both form.html and lov.html locally, the testcase
works perfectly without any errors. 

So this looks like some sort of security issue -(?).
cc'ing mstoltz, bzbarsky to find out -

Comment 12

[Phil Schwartau]

Very strange. Now http://217.75.131.39:7070/mozilla_demo/form.html
works for me, using the same build as in my last comment! It only
failed that once. 

However, with a trunk build from 2002-08-05, the testcase fails
consistently for me . Note, however, I do not get the "Permission denied"
error first in this build, just "passed_value is not defined" errors.

Something very strange going on here -

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm... Could the problem here be that we are setting the value before the
document in the other window has loaded?  And then the document load clobbers
the value?

Comment 14

[Johnny Stenback (:jst)]

Sounds possible.

Comment 15

[Mihail Daskalov]

Very strange behaviour.
When debugging through Javascript debugger averything works just fine!!!!
This is in build 2002081204

Without debugging it doesn't work.

I step through the code which does 
chooser.passed_value = document.forms[0].i;

Comment 16

[Mihail Daskalov]

I have downloaded the source code through cvs last night 01:00 GMT, and managed
to compile it on a SuSE Linux 7.2 box, with the only option in .mozconfig 
"ac_add_options --enable-crypto".
I run the build and enabled Javascript Strict warning. Then when I got to
http://217.75.131.39:7070/mozilla_demo/form.html 
after when opening the second window I see the following on the console:

----------------------------------------------------------------------------
Document http://esrv2.technologica.bg/mozilla_demo/form.html loaded successfully
JavaScript strict warning: 
http://esrv2.technologica.bg/mozilla_demo/form.html line 13: assignment to
undeclared variable chooser

we don't handle eBorderStyle_close yet... please fix me
WEBSHELL+ = 4
WEBSHELL+ = 5
Document http://esrv2.technologica.bg/mozilla_demo/lov.html loaded successfully
----------------------------------------------------------------------------

And it still doesn't work.
Is this a DOM problem, or Javascript Engine problem?
Or both?

If I run it again this message disappears. I mean if I press the "Open second
window" button again, reloading the page and pressing the button again,
go to another URL, and then return back and press the button again.

Somehow I managed to get this message again without restarting the browser,
but I couldn't reproduce it.

Comment 17

[Phil Schwartau]

Turning on JavaScript strict warnings will show you a lot of
extra messages that are not errors: just warnings.

When I go to http://217.75.131.39:7070/mozilla_demo/form.html
with JavaScript strict warnings on, I never get any warnings.

I tried looking at line 13 in the code for the warning you
got at http://esrv2.technologica.bg/mozilla_demo/form.html,
but I cannot access that site. The browser says:

      esrv2.technologica.bg could not be found. 
      Please check the name and try again

The code at http://217.75.131.39:7070/mozilla_demo/form.html
does seem to have a var declaration for |chooser|, so I can't see
why you would ever get that warning -

Comment 18

[Mihail Daskalov]

meanwhile I changed the code, thats why you don't see it now.
There was a stupid typo, but correcting it doesn't help.

Any advices how to debug, or which newsgroup to use for asking questions.

Unfortunately I don't have access to IRC   :-( 

Comment 19

[Mihail Daskalov]

Some more testcases which I don't know why, but they work ...

see 
http://217.75.131.39:7070/mozilla_demo/f2.html
and 
http://217.75.131.39:7070/mozilla_demo/f4.html

they both open f3.html in another window, passing two values to the window.
The second value is always succesfully passed, and the first one is only passed
in f4.html.
The f2.html is unable to pass the value, and f4.html is able to do it.

The difference is that in
f2.html I've put the following line
alert(x.document); // this makes it not work

and in f4.html I've put the following line
// this makes it work alert(x.document);


This shows it has something similar to Boris's comment 13
but it's rather the opposite !?

I would suppose when I have alert(x.document) this to allow for more time for
the document to load, but it's rather the opposite.
I can't say what's going on.

Comment 20

[Mihail Daskalov]

Something else, almost proving comment 13
save the files locally and start f4.html.

When you click the check button you'll see that it initially displays the passed
value, and then in a while it disappears, and then it's just not there.

I cannot see this behavious through http protocol
This is my experience with build 2002081204 on Windows NT,
and with cvs build on linux from today.

Comment 21

[georgi - hopefully not receiving bugspam]

Is the patch for bug 134315 causing this?
The patch for 134315 was commited around the time the problem started.

Comment 22

[Mihail Daskalov]

I am happy to see that this bug doesn't exist in 1.0 branch build ID: 2002082106
on Window NT

Comment 23

[Mihail Daskalov]

adding a few keywords (nominations for fix in the next Netscape release -
primarily).

Comment 24

[Peter Van der Beken [:peterv]]

This is definitely caused by the fix for bug 134315.

Comment 25

[Peter Van der Beken [:peterv]]

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

Comment 26

[Phil Schwartau]

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

Comment 27

[Mitchell Stoltz (not reading bugmail)]

While there are apparently some site authors wo depend on being able to set
variables on a window object as it's opening, and having those values available
after a URL has loaded in that window, I think allowing that in all cases could
be dangerous. A malicious page could do something like this:
w = window.open("http://some site where the victim has a login cookie");
w.ImportantVar= "new value which changes the behavior of the page";

or redefine the String class to a class that leaks sensitive information to the
attacker.

I think we should do a same-origin check here, like so:
When window.open is called, save the principal of the calling script in a
variable on the window being opened. Then, when content arrives and
SetNewDocument is called, compare the cached principal to the principal of the
document being loaded. If they match, don't clear the scope. Otherwise, proceed
with the existing behavior.

I can implement this when I get the time, so I'm reassigning the bug to me.
Peter, if you have the time and desire to fix this sooner, feel free to take it
back.

Comment 28

[Mihail Daskalov]

Is there chance to get this in Mozilla 1.2 release?

Comment 29

[R.K.Aa.]

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

Comment 30

[Phil Schwartau]

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

Comment 31

[Mitchell Stoltz (not reading bugmail)]

Clearing milestone for now.

Comment 32

[Mihail Daskalov]

as 1.4 will replace the 1.0 branch as a stable release I think this bug should
be fixed by 1.4b.

This bug is not present in the Mozilla 1.0 branch.

Comment 33

[Samir Gehani]

adt: nsbeta1-

Comment 34

[Asa Dotzler [:asa]]

mitch, drivers would like to get this for the long-lived 1.4 branch. Will you be
able to get to it for 1.4?

Comment 35

[Mitchell Stoltz (not reading bugmail)]

I will try to have a patch tomorow.

Comment 36

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - work in progress, doesn't quite work

in NSGlobalWindow::SetNewDocument, I need to get the principal of the new
document being loaded, but the new document is not passed in here. I could add
a parameter for that, but I'm not sure that's the best solution. Look for
'//XXX Problem' in the patch above.

Comment 37

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - this one works.

As a bonus, this patch also makes window.opener settable only to null, which
will avoid some problems down the road.

Comment 38

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 122266 [details] [diff] [review]
Patch - this one works.

>Index: src/base/nsGlobalWindow.cpp
>===================================================================

>+        NS_ASSERTION(sSecMan, "No Security Manager Found!");
>+        nsCOMPtr<nsIPrincipal> principal;
>+        sSecMan->GetSubjectPrincipal(getter_AddRefs(principal));

This assertion doesn't really help. Either assert and avoid the crash, or let
it crash without the assertion.

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 122266 [details] [diff] [review]
Patch - this one works.

- In GlobalWindowImpl::SetOpener():

+  if (aOpener) {
+    // check if we were called from a privileged chrome script.
+    // If not, opener is settable only to null.
+    NS_ENSURE_TRUE(sSecMan, NS_ERROR_FAILURE);
+    PRBool inChrome;
+    nsresult rv = sSecMan->SubjectPrincipalIsSystem(&inChrome);
+    if (NS_FAILED(rv) || !inChrome) {
+      return NS_OK;
+    }
+  }

You could simplyfie that by ising the helper IsCallerChrome(), i.e.

+  // check if we were called from a privileged chrome script.
+  // If not, opener is settable only to null.
+  if (aOpener && !IsCallerChrome()) {
+    return NS_OK;
+  }

Other than that, sr=jst.

Comment 40

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch 3 - addresses review comments

Comment 41

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 122330 [details] [diff] [review]
Patch 3 - addresses review comments

Requesting 1.4b approval. This patch slightly relaxes the rules for when global
object scope is cleared when a new window is opened. As this makes security
slightly more permissive, the chance of regression is low. At worst, in case of
unforseen error, this code defaults to the previous existing behavior (clearing
the scope) causing a few scripts to fail.

Comment 42

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 122266 [details] [diff] [review]
Patch - this one works.

Johnny cleared my r=, adding it back.

Comment 43

[(not reading, please use [e-mail] instead)]

Comment on attachment 122330 [details] [diff] [review]
Patch 3 - addresses review comments

do heikki's and jst's reviews carry over to this patch?

this one is setting off my potential-regression-radar

can you give me examples of websites that this will fix?

Comment 44

[Zibi Braniecki [:zbraniecki][:gandalf]]

According to Comment #37 bug 184618 will be fixed with this patch.

Comment 45

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 122330 [details] [diff] [review]
Patch 3 - addresses review comments

Yes, this also has r=heikki

Comment 46

[Mitchell Stoltz (not reading bugmail)]

Seth,
   I don't know which sites this fixes, but I assume drivers do since they
marked this as blocking1.4b.

Comment 47

[Asa Dotzler [:asa]]

Comment on attachment 122330 [details] [diff] [review]
Patch 3 - addresses review comments

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 48

[John Morrison]

An example on a popular commerce site is http://www.travelocity.com/ -- click
open the calendar popup and try to use the date widget. It's broken.

Comment 49

[Mitchell Stoltz (not reading bugmail)]

Fix checked in.

Comment 50

[Kathleen :Brade]

This fix may have caused a regression for bug 206317.

Comment 51

[Ian Neal]

This fix may have caused the regression in bug 211719

Comment 52

[Ian Neal]

Having backed out this bug's patch locally it does get rid of the regression in
bug 211719