298064 - nsContentUtils::GetDocumentFromCaller() is broken.

Status: VERIFIED FIXED

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

Description

[Johnny Stenback (:jst)]

Right now nsContentUtils::GetDocumentFromCaller() is broken, it now gets the
document from the calling context, which may or may not be what you want, what
you most likely always want is the document where the calling code came from.

We use the caller document in document.open() etc to propagate security info
etc, and that really should be that of the calling code, not the calling context.

Comment 1

[Daniel Veditz [:dveditz]]

Luckily it's not used much
http://lxr.mozilla.org/mozilla/search?string=GetDocumentFromCaller

Comment 2

[Mike Shaver (:shaver emeritus)]

Blocking beta, but not branch.

Comment 3

[Mike Schroepfer]

Peter - can you take a look?

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

This seems to do the trick (thanks to jst for setting me on the right track).
Brendan: are we doing the right thing here by using
JS_GetScopeChain/JS_GetParent, it does work but is it correct?
Jst: shouldn't the code to get callerPrincipal be moved out of the if at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
?

Comment 5

[Johnny Stenback (:jst)]

(In reply to comment #4)
> Created an attachment (id=193916) [edit]
> v1
> 
> This seems to do the trick (thanks to jst for setting me on the right track).
> Brendan: are we doing the right thing here by using
> JS_GetScopeChain/JS_GetParent, it does work but is it correct?
> Jst: shouldn't the code to get callerPrincipal be moved out of the if at
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
> ?

Yeah, seems like it should be. Wanna do a double whammy and include that in this
fix as well? :)

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to comment #5)
> (In reply to comment #4)
> > Jst: shouldn't the code to get callerPrincipal be moved out of the if at
> >
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.622#1855
> > ?
> 
> Yeah, seems like it should be. Wanna do a double whammy and include that in this
> fix as well? :)

Sure, I'll try to attach a new patch with that included later tonight.

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Testcase file B

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Testcase file A

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 193935 [details] [diff] [review]
v1.1

Ok, you don't want the scope chain, if we are willing to break compatibility
with Gecko here in the interest of sanity and (perhaps, need to test; also
likely irrelevant) Netscape 2-4.

You want to change GetDocumentFromCaller to take a JSObject *obj argument that
it uses as the start of the loop up the parent chain to a global object.  This
obj param should be the Image constructor, e.g., being called.

That is, var img = new Image will work as with your patch, but var img2 = new
otherWindow.Image will work as it should, not as it does today (which I believe
involves using the wrong document's nodeinfo list -- please correct me if I am
wrong).

Need a third testcase covering the img2 case and somehow detects the difference
in results, and some results from various browsers.

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (From update of attachment 193935 [details] [diff] [review] [edit])
> Ok, you don't want the scope chain, if we are willing to break compatibility
> with Gecko here in the interest of sanity and (perhaps, need to test; also
> likely irrelevant) Netscape 2-4.

Of course, your patch to use the scope chain is incompatible with the broken,
longstanding Gecko implementation anyway.

Still, it'd be good to have a testcase and results from various browsers.  We
may choose to break compat here because no one will care.  I'm keen to see how
you'd detect the difference in which document's nodeinfo list was used.

/be

Comment 12

[Peter Van der Beken [:peterv]]

So I either need to walk the JS stack until I find a function or a constructor
and use the function object or the constructor object, or I need to pass those
objects on from where I can reach them.
The first solution works if I add a JS_GetFrameConstructorObject to
jsdbgapi.h/cpp which does |return fp->argv && (fp->flags & JSFRAME_CONSTRUCTING)
!= 0 ? JSVAL_TO_OBJECT(fp->argv[-2]) : NULL;|. The other solution works too, I
can get at argv[-2] from nsHTMLImageElement::Initialize and
nsHTMLOptionElement::Initialize, but it means that we have to create elements
without a nodeinfo (Initialize gets called after creation). I'd rather not do
that (and avoid the "No nsINodeInfo passed to nsGenericElement, PREPARE TO
CRASH!!!" assertion), so I'm leaning towards the first solution. I need to know
whether adding JS_GetFrameConstructorObject would be ok though.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> I need to know
> whether adding JS_GetFrameConstructorObject would be ok though.

I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
or condition it with JS_IsConstructing?

/be

Comment 14

[Brendan Eich [:brendan]]

Really, it doesn't matter whether the JS callee is a constructor or not.  If you
are hacking a native method reached from a new Image or Image() call or
whatever, and it's creating a new Image object, you want it to use static scoping.

/be

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to comment #13)
> I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
> or condition it with JS_IsConstructing?

Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming
through a constructor (like from new Image()) fp->fun is null.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Testcase file B

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Testcase file A

IE 6, Opera 8 and Safari 2.0 all pass this one, and with my patch (as described
in comment 12) Firefox does too. It's not trivial to test this in older
browsers, I guess it could be done but do we really care given these results?

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Testcase file A

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #15)
> (In reply to comment #13)
> > I don't see the need, given JS_GetFrameFunctionObject -- why not use just that,
> > or condition it with JS_IsConstructing?
> 
> Because JS_GetFrameFunctionObject checks |fp->argv && fp->fun| and when coming
> through a constructor (like from new Image()) fp->fun is null.

Oh duh -- XPConnect makes its own callable objects.  Ok, all we really need here
is fp->argv[-2], with safety null-checking for fp->argv.  Don't conflate it with
JS_IsConstructing, call it JS_GetFrameCalleeObject and add it to the patch?

/be

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 194235 [details] [diff] [review]
v2

Thanks, looks good, only thought:

Isn't there a subroutine to get the global object by walking up the parent
chain?	Maybe jst knows.

/be

Comment 22

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> Isn't there a subroutine to get the global object by walking up the parent
> chain?	Maybe jst knows.

There's nsJSUtils::GetStaticScriptGlobal, it would add one QI (it returns
nsIScriptGlobalObject and we want nsPIDOMWindow). Jst?

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 194235 [details] [diff] [review]
v2

Yeah, use nsJSUtils here, one more QI won't hurt much. r=jst

Comment 24

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Here's the patch that I checked in on the trunk. Carrying over r=jst,
sr=brendan and asking for branch approval.

Comment 25

[Bob Clary [:bc] (inactive)]

I'll verify this with tomorrow's builds.

Comment 26

[Bob Clary [:bc] (inactive)]

(In reply to comment #18)
> Created an attachment (id=194172) [edit]
> Testcase file A
> 

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050903 Firefox/1.6a1

I'll try to verify on Linux and Mac later.

Comment 27

[Daniel Veditz [:dveditz]]

This appears to have caused bug 312363, showing mixed content if a child frame has been document.written() into as seen in a real-world bank site.

Comment 28

[Johnny Stenback (:jst)]

The nsContentUtils.cpp changes that went in with this fix was backed out on the branch. See bug 312363. (removing fixed1.8 keyword)

Comment 29

[Brendan Eich [:brendan]]

Should this bug be reopened, and used for patching to fix the two callers (for new Image and new Option from JS) that need fixing?  Bug 312363 is about document.open and it is fixed.

/be

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Brendan, this patch is still in on the trunk. I had the intention of leaving things that way and simply fixing document.open on the trunk (I'm about to attach a new patch to that bug).

Comment 31

[Bob Clary [:bc] (inactive)]

fyi, failed ff 1.0.8/winxp/20060221

Comment 32

[Bob Clary [:bc] (inactive)]

This is not fixed in bon echo 1.8 20060910/winxp. 

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #32)
> This is not fixed in bon echo 1.8 20060910/winxp. 

Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want this fixed on the trunk, the trunk patch in bug 312363 should be nominated/checked into the 1.8 branch.

Comment 34

[Mike Schroepfer]

Bkap/Brendan - what do we need to do here?

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #33)
> (In reply to comment #32)
> > This is not fixed in bon echo 1.8 20060910/winxp. 
> 
> Yeah, the patch was backed out from the 1.8 branch (see comment 28). If we want
> this fixed on the trunk,

I think Blake meant "branch" here.

> the trunk patch in bug 312363 should be
> nominated/checked into the 1.8 branch.

That sounds like the thing to do for 1.8.  Blake, can you confirm?

/be

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #35)
> That sounds like the thing to do for 1.8.  Blake, can you confirm?

Yeah, that's what I meant.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you nominate the appropriate patch?  It's marked fixed1.8.

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #37)
> Could you nominate the appropriate patch?  It's marked fixed1.8.

It's marked fixed1.8 because we backed out the offending patch from the branch. I'll nominate the correct patch now (checking to make sure that it applies correctly first).

Comment 39

[Mike Schroepfer]

Plusing the blocking flag on this so we don't lose it.  Sounds like we want the patch in 312363...

Comment 40

[Mike Schroepfer]

Moving this out to 1.8.1.1 since we can't seem to get a straight answer on what the right approach here is...

Comment 41

[Daniel Veditz [:dveditz]]

mrbkap: sounds like we want this one, or some kind of fix, but what patch? Please ask for approval on what we need.

Would we want this for 1.8.0.9 too?

Comment 42

[Jay Patel [:jay]]

Removing blocking flags, peterv wants us to take the fix in bug 312363.  I will update that bug with the flags.

Comment 43

[Peter Van der Beken [:peterv]]

Fixed on branches by the check-in of attachment 238242 [details] [diff] [review] from bug 312363.

Comment 44

[juan becerra [:juanb]]

Verified no crash using testcase on comment #18 with the following:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061128 Minefield/3.0a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre
and 1.8.0.9 on Intelmac, 1.8.1.1 linux 

Comment 45

[Boris Zbarsky [:bzbarsky]]

I checked in a test for this in bug 445004.