Last Comment Bug 156452 - Accessing cross-domain document structure via TreeWalker
: Accessing cross-domain document structure via TreeWalker
[ADT1 RTM] [ETA 08/02] security
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla1.0.1
Assigned To: Jonas Sicking (:sicking)
: bsharma
Depends on: 159348 159468
Blocks: 143047 158167 158202 158340 1.0.1
  Show dependency treegraph
Reported: 2002-07-09 06:56 PDT by Bradley Baetz (:bbaetz)
Modified: 2013-04-04 13:53 PDT (History)
28 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (961 bytes, text/html)
2002-07-09 07:01 PDT, Bradley Baetz (:bbaetz)
no flags Details
patch to fix (5.05 KB, patch)
2002-07-11 02:59 PDT, Jonas Sicking (:sicking)
security-bugs: review+
jst: superreview+
Details | Diff | Review
fixes jsts comment (5.06 KB, patch)
2002-07-12 00:20 PDT, Jonas Sicking (:sicking)
jonas: review+
jonas: superreview+
chofmann: approval+
Details | Diff | Review
Plug more holes... (12.25 KB, patch)
2002-07-15 18:55 PDT, Johnny Stenback (:jst,
no flags Details | Diff | Review
...and yet more (requires attachment 91442) (8.58 KB, patch)
2002-07-16 00:55 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
combined and refined (25.01 KB, patch)
2002-07-16 02:23 PDT, Jonas Sicking (:sicking)
jst: review+
Details | Diff | Review
fixes bzs comments (42.89 KB, patch)
2002-07-16 03:47 PDT, Jonas Sicking (:sicking)
bzbarsky: superreview+
Details | Diff | Review
Patch to disable the security checks temporarily (1.36 KB, patch)
2002-07-16 08:42 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
Fix the performance issues (15.79 KB, patch)
2002-07-16 11:38 PDT, Johnny Stenback (:jst,
jonas: review+
bzbarsky: superreview+
Details | Diff | Review
diff -w of the above (10.74 KB, patch)
2002-07-16 11:39 PDT, Johnny Stenback (:jst,
no flags Details | Diff | Review
XSLT/XPath changes (6.52 KB, patch)
2002-07-16 21:36 PDT, Jonas Sicking (:sicking)
peterv: review+
jst: superreview+
Details | Diff | Review
combines 91527 and 91597 (23.52 KB, patch)
2002-07-17 20:42 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
*this* is really what i ment to attach (23.69 KB, patch)
2002-07-17 21:24 PDT, Jonas Sicking (:sicking)
jst: review+
bzbarsky: superreview+
Details | Diff | Review
fixes bzs comments (25.11 KB, patch)
2002-07-17 22:54 PDT, Jonas Sicking (:sicking)
jonas: review+
bzbarsky: superreview+
Details | Diff | Review
testcase for some of the exploits fixed (654 bytes, text/html)
2002-07-18 20:46 PDT, Jonas Sicking (:sicking)
no flags Details
another testcase (581 bytes, text/html)
2002-07-18 20:50 PDT, Jonas Sicking (:sicking)
no flags Details
fix XUL problem (20.39 KB, patch)
2002-07-22 12:50 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
fix XUL problem version 2 (22.35 KB, patch)
2002-07-22 16:03 PDT, Jonas Sicking (:sicking)
jst: superreview+
Details | Diff | Review
Fix bzs comments (23.16 KB, patch)
2002-07-22 18:48 PDT, Jonas Sicking (:sicking)
bzbarsky: review+
jonas: superreview+
asa: approval+
Details | Diff | Review
patch for branch (49.51 KB, patch)
2002-07-23 03:06 PDT, Jonas Sicking (:sicking)
bzbarsky: review+
chofmann: approval+
Details | Diff | Review
demo that reads text from a form field using appendChild and dom 0 (763 bytes, text/html)
2002-07-23 17:52 PDT, Jesse Ruderman
no flags Details
Fix the DOM inspector regression (7.11 KB, patch)
2002-07-23 20:14 PDT, Johnny Stenback (:jst,
jonas: review+
bzbarsky: superreview+
chofmann: approval+
Details | Diff | Review
Complete diff for the branch. (39.11 KB, patch)
2002-07-24 00:27 PDT, Johnny Stenback (:jst,
no flags Details | Diff | Review
Patch for the branch, including the fix for bug 159468 (40.11 KB, patch)
2002-07-26 16:33 PDT, Johnny Stenback (:jst,
no flags Details | Diff | Review
stacktrace of new principal code (6.35 KB, text/plain)
2002-07-26 16:47 PDT, Jonas Sicking (:sicking)
no flags Details
patch for branch, including fix for bug 159348 (62.34 KB, patch)
2002-07-31 18:39 PDT, Jonas Sicking (:sicking)
jonas: review+
jonas: superreview+
rjesup: approval+
Details | Diff | Review

Description Bradley Baetz (:bbaetz) 2002-07-09 06:56:57 PDT
Its possible to use the tree walker to get at the document structure of a
document in another domain. I'll attach a testcase (based on sicking's original

I don't think its possible to see data/attributes from the nodes without finding
a separate bug somehow - I get permission denied errors whenever I try to access
anything on the nodes themselves, apart from .toString. I haven't tried anything
fancy, though.

Is it legal to call document.createTreeWalker(someOtherDocument, ...) in the
first place?
Comment 1 Bradley Baetz (:bbaetz) 2002-07-09 07:01:29 PDT
Created attachment 90607 [details]

This is basically sicking's original testcase, modified. To use, wait until the
contents of the <iframe> have loaded, then click |begin|. Then traverse the DOM
tree using the other buttons.

Can anyone manage to get useful information out of this?
Comment 2 Johnny Stenback (:jst, 2002-07-09 09:50:57 PDT
Hmm, I can't see how this would be directly exploitable, but we should block it
in any case. This could be fixed the same way bug 147754 was fixed.
Comment 3 Jonas Sicking (:sicking) 2002-07-09 11:58:18 PDT
How can the user get hold of the node to initialize the treewalker to in the
first place? I.e. shouldn't it fail already at the
Comment 4 Johnny Stenback (:jst, 2002-07-09 12:54:02 PDT
No, accessing .contentDocument should be allowed since there are properties on
the document in the iframe that anyone should be able to set (like
.contentDocument.location = "http://...").
Comment 5 Jonas Sicking (:sicking) 2002-07-11 02:59:40 PDT
Created attachment 90934 [details] [diff] [review]
patch to fix

This patch performs same-origin tests whenever we create a treewalker of set
the contentnode in a treewalker. Those two should be the only methods of
setting the node that the walker walks
Comment 6 Mitchell Stoltz (not reading bugmail) 2002-07-11 19:36:09 PDT
Comment on attachment 90934 [details] [diff] [review]
patch to fix

Perfect. The |if (doc1 == doc2)| fast return path is a good thing, and your use
of nsScriptSecurityManager looks fine. r=mstoltz.
Comment 7 Johnny Stenback (:jst, 2002-07-11 20:02:13 PDT
Comment on attachment 90934 [details] [diff] [review]
patch to fix

+  nsresult rv = nsContentUtils::CheckSameOrigin(this, aRoot);

Don't use NS_ENSURE_SUCCESS() in places where you know the call can fail for
valid reasons, like in this case. if (NS_FAILED(rv)) { return NS_OK; } in
stead, but on separate lines...

sr=jst if you change that.
Comment 8 Jesse Ruderman 2002-07-11 22:56:33 PDT
What other DOM stuff uses this akward syntax?  I know that at least one other
function, document.getAnonymousNodes, uses it.
Comment 9 Jonas Sicking (:sicking) 2002-07-12 00:20:53 PDT
Created attachment 91079 [details] [diff] [review]
fixes jsts comment
Comment 10 Jonas Sicking (:sicking) 2002-07-12 00:32:04 PDT
Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

carrying over r/sr
Comment 11 Johnny Stenback (:jst, 2002-07-12 00:38:07 PDT
Hmm, what if script from domain X gets access to a document from domain Y and is
able to reach an element in the document from domain Y, could it then do
document.body.appendChild() to steal the node from the other document?

There's also document.importNode(), and maybe others...
Comment 12 Johnny Stenback (:jst, 2002-07-12 00:42:31 PDT
We might want new bugs for those issues if they're real issues... Seems like all
properties of document objects that expose elements are protected, but who knows
if there are other ways to reach an element in a window to which you're not
reall supposed to have access to... Belts and suspenders, we need some of that
here it seems...
Comment 13 Jesse Ruderman 2002-07-12 19:22:45 PDT
Sorry for the incoherent comment earlier.  What I meant was, "what other DOM
functions are accessed using document.functionName but don't necessarily operate
on the document itself?"  A better question is, "what other DOM functions can
take a document as a parameter and might need a security check on that

After looking at the traversal spec and the testcase and scratching my head for
a few minutes: Eek! I didn't know Document inherited from Node.  That blows away
my assumption that a script can get access to another document's document object
but not its nodes.  That means we have a lot of functions to consider and/or add
security checks to.
Comment 14 Jonas Sicking (:sicking) 2002-07-13 04:45:31 PDT
Comment 15 Jonas Sicking (:sicking) 2002-07-15 11:09:08 PDT
adding keywords, important security bug. nsbeta1+'ed with heikkis blessing
Comment 16 chris hofmann 2002-07-15 12:08:12 PDT
Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

a=chofmann for the 1.1b trunk...  lets see how if goes there and then look at
getting on the branch.
Comment 17 Jonas Sicking (:sicking) 2002-07-15 16:59:37 PDT
Comment on attachment 91079 [details] [diff] [review]
fixes jsts comment

checked in on trunk
Comment 18 Paul Wyskoczka 2002-07-15 17:02:50 PDT
Reducing to adt2 per adt.
Comment 19 lchiang 2002-07-15 17:48:21 PDT
gerardok or bsharma - can you take over as the QA contact and verify the bug fix
with the attached testcase on the trunk builds (after July 15)?   Also, will
need verification once the fix is in the branch.  Thanks.
Comment 20 Johnny Stenback (:jst, 2002-07-15 18:55:31 PDT
Created attachment 91442 [details] [diff] [review]
Plug more holes...
Comment 21 Jonas Sicking (:sicking) 2002-07-16 00:55:29 PDT
Created attachment 91479 [details] [diff] [review]
...and yet more (requires attachment 91442 [details] [diff] [review])
Comment 22 Jonas Sicking (:sicking) 2002-07-16 02:23:18 PDT
Created attachment 91485 [details] [diff] [review]
combined and refined
Comment 23 Johnny Stenback (:jst, 2002-07-16 02:29:34 PDT
Comment on attachment 91485 [details] [diff] [review]
combined and refined

r=jst for the parts I didn't write myself.
Comment 24 Boris Zbarsky [:bz] 2002-07-16 03:28:45 PDT
Comment on attachment 91485 [details] [diff] [review]
combined and refined

>     aNode1->GetOwnerDocument(getter_AddRefs(domDoc1));
>     doc1 = do_QueryInterface(domDoc1);
>     if (!doc1) {
>-      return NS_ERROR_FAILURE;
>+      // aNode1 is not part of a document, let any caller access it.
>+      return NS_OK;
>     }

I would be happier with:

  if (!domDoc1) {
    // aNode1 is not part of a document, let any caller access it.
    return NS_OK;
  doc1 = do_QueryInteface(domDoc1);
  NS_ASSERTION(doc1, "We have an nsIDOMDocument that's not QIable to

or something along those lines.  That way a QI failure won't suddenly open up
access to stuff.

Same thing for doc2, of course, and same in nsContentUtils::CanCallerAccess

> +    if(NS_FAILED(rv)) {

Space after the "if", please?  You have this in a few places.

> nsDocument::AddBinding(nsIDOMElement* aContent, const nsAString& aURL)

How about fixing nsDocument::RemoveBinding too?

>+  if (old_doc != mDocument) {
>+    if(!nsContentUtils::CanCallerAccess(aNewChild)) {

This strikes me as a good place to use boolean operators instead of nested
ifs... ;)

>+  if (old_doc != mDocument) {
>+    if(!nsContentUtils::CanCallerAccess(aNewChild)) {

Here too.

>+MBool URIUtils::CanCallerAccess(nsIDOMNode *aNode)

Same comments apply here as to nsContentUtils::CanCallerAccess


#else /* TX_EXE */

> #endif

#endif /* TX_EXE */

Makes things more readable, imo.
Comment 25 Jonas Sicking (:sicking) 2002-07-16 03:47:46 PDT
Created attachment 91491 [details] [diff] [review]
fixes bzs comments
Comment 26 Boris Zbarsky [:bz] 2002-07-16 03:50:38 PDT
Comment on attachment 91491 [details] [diff] [review]
fixes bzs comments

Comment 27 Jonas Sicking (:sicking) 2002-07-16 03:50:49 PDT
also changed the transformiix stuff to
    if (!URIUtils::CanCallerAccess(aSourceDOM) ||
        !URIUtils::CanCallerAccess(aStyleDOM) ||
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-07-16 08:20:43 PDT
This patch increased new window time on tinderbox by around 20%-25% on the
mid-speed tinderboxes and by 47% on the fastest tinderbox (comet).  (The slow
ones haven't cycled yet.)  That seems a bit excessive.  Any thoughts on how to
fix it?
Comment 29 Peter Van der Beken [:peterv] 2002-07-16 08:42:40 PDT
Created attachment 91507 [details] [diff] [review]
Patch to disable the security checks temporarily
Comment 30 Peter Van der Beken [:peterv] 2002-07-16 08:50:46 PDT
Also, please correct the indentation in txURIUtils (4 spaces instead of 2) and
share CanCallerAccess (for example exposing it through the security manager)
instead of implementing it twice.
Comment 31 Peter Van der Beken [:peterv] 2002-07-16 08:55:56 PDT
I checked in attachment 91507 [details] [diff] [review].
Comment 32 Johnny Stenback (:jst, 2002-07-16 09:35:15 PDT
Ugh, that sucks... I'll have a look and see why we end up hitting this new
security check so often, updating my tree now...
Comment 33 Johnny Stenback (:jst, 2002-07-16 11:38:24 PDT
Created attachment 91527 [details] [diff] [review]
Fix the performance issues

Looks like the performance hit was due to the security checks in
nsXULCommandDispatcher::AddCommandDispatcher() and
nsXULDocument::AddBroadcastListenerFor(), so I changed those to use the faster
version of the security check which is more or less a nop in the common case
(every case during startup), and I also optimized the checks in
nsGenericElement a bit more. Also, I added a cache for the security manager
pointer to nsContentUtils to avoid hitting the component manager for every
security check.
Comment 34 Johnny Stenback (:jst, 2002-07-16 11:39:47 PDT
Created attachment 91528 [details] [diff] [review]
diff -w of the above
Comment 35 Johnny Stenback (:jst, 2002-07-16 14:35:03 PDT
CanCallerAccess(nsIDOMNode *) does IMO *not* belong in caps. Had it been more
than a trivial ammount of code we could share it, but given how little code it
is I don't think it's worth it.
Comment 36 Jonas Sicking (:sicking) 2002-07-16 16:05:34 PDT
+    if (!doc2) {
+      // aNode is neither a nsIContent nor an nsIDocument, something
+      // weird is going on...
+      NS_ERROR("aNode is neither an nsIContent nor an nsIDocument!");
+      return NS_ERROR_UNEXPECTED;

Why not do the same as for aNode1 and say that this is a faked node? I.e. return
NS_ERROR_DOM_SECURITY_ERR. Same in nsContentUtils::CanCallerAccess

Shouldn't you release the security-manager somewhere? though i can't find any
place where sXPConnect is release either...

I'll try to come up with some code to speed up the XSLT stuff
Comment 37 Jonas Sicking (:sicking) 2002-07-16 16:11:31 PDT
other then that r=sicking
Comment 38 Johnny Stenback (:jst, 2002-07-16 16:49:26 PDT
since we don't know for a fact that the case where that code would be triggerd
is a security error, it might be, but it could be for some other reason as well.

And yes, the cached security manager needs to be released in
nsContentUtils::Shutdown() (where sXPConnect is released too btw). Consider that
change made...
Comment 39 Jonas Sicking (:sicking) 2002-07-16 17:03:49 PDT
Comment on attachment 91527 [details] [diff] [review]
Fix the performance issues

ok, r=sicking
Comment 40 Boris Zbarsky [:bz] 2002-07-16 17:16:00 PDT
Comment on attachment 91527 [details] [diff] [review]
Fix the performance issues

It may be a good idea to move the |if (!sSecurityManager)| checks up to before
we do all that document-getting.  Doesn't affect us, but may affect some
embedding apps.

other than that, sr=bzbarsky (assuming that change to Shutdown(), of course).
Comment 41 Johnny Stenback (:jst, 2002-07-16 17:24:49 PDT
No, I intentionally put the if (!sSecurityManager) check far down in the method
so that we don't bother doing that check in the fast path (i.e. get the document
pointers and compare, and return early in the common case). It only matters for
embedders who choose to not have a security manager, which would be a really bad
decision IMO.
Comment 42 Jonas Sicking (:sicking) 2002-07-16 21:36:19 PDT
Created attachment 91597 [details] [diff] [review]
XSLT/XPath changes
Comment 43 Johnny Stenback (:jst, 2002-07-16 22:38:35 PDT
Comment on attachment 91597 [details] [diff] [review]
XSLT/XPath changes

Comment 44 Peter Van der Beken [:peterv] 2002-07-17 02:38:46 PDT
Comment on attachment 91597 [details] [diff] [review]
XSLT/XPath changes

>Index: extensions/transformiix/build/XSLTProcessorModule.cpp
>RCS file: /cvsroot/mozilla/extensions/transformiix/build/XSLTProcessorModule.cpp,v
>retrieving revision 1.21
>diff -u -r1.21 XSLTProcessorModule.cpp
>--- extensions/transformiix/build/XSLTProcessorModule.cpp	15 May 2002 18:53:45 -0000	1.21
>+++ extensions/transformiix/build/XSLTProcessorModule.cpp	17 Jul 2002 04:27:03 -0000

>@@ -192,7 +195,17 @@
>         return NS_ERROR_OUT_OF_MEMORY;
>     if (!txHTMLAtoms::init())
>         return NS_ERROR_OUT_OF_MEMORY;
>+    rv = nsServiceManager::GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID,
>+                                      nsIScriptSecurityManager::GetIID(),
>+                                      (nsISupports **)&sSecurityManager);
>+    if (NS_FAILED(rv)) {
>+        sSecurityManager = nsnull;
>+        return rv;
>+    }

Please use CallGetService.

Comment 45 Jonas Sicking (:sicking) 2002-07-17 20:42:13 PDT
Created attachment 91748 [details] [diff] [review]
combines 91527 and 91597

combines 91527 and 91597 and makes a small change to ::CheckSameOrigin as
discussed on irc
Comment 46 Jonas Sicking (:sicking) 2002-07-17 20:49:18 PDT
arg, forgot to do petervs change, The new code is

rv = CallGetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &sSecurityManager);
Comment 47 Jonas Sicking (:sicking) 2002-07-17 21:24:28 PDT
Created attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

apparently i had forgot to save in a few more places, this should be the real
Comment 48 Boris Zbarsky [:bz] 2002-07-17 21:47:13 PDT
Comment on attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

>+  /*
>+   * Checks if two nodes comes from the same origin. aTrustedNode is

Make that two '*' chars on the first line so doxygen picks it up?

"whether", not "if".

>+   * considered 'safe' in that a user can operate on it and that it isn't
>+   * isn't a js-object that implements nsIDOMNode.

Take out one "isn't".

>+ * Never call this function with the first node provided by script, it
>+ * must always be known to be a 'real' node!

Could we add a debug-only assertion that aTrustedNode is either an nsIDocument
or nsIContent?	Just to catch people who don't read comments?

More importantly, please add NS_PRECONDITION(aTrustedNode, "There must be a
trusted node"), since you dereference it unconditionally when it's not an

You never release sScriptSecurityManager in nsContentUtils::Shutdown()

>Index: content/base/src/nsDOMAttributeMap.cpp

Replace the code you removed from here with "// XXX" comments to readd it once
GetOwnerDocument gets fixed on Attr nodes.  File a bug to that effect, make it
depend on the relevant Attr node bug.

>+extern nsIScriptSecurityManager *sSecurityManager;

This may make more sense as gSecurityManager, since it's not static....  But
whatever the module convention is. ;)

sr=bzbarsky with those changes
Comment 49 Johnny Stenback (:jst, 2002-07-17 22:40:23 PDT
Comment on attachment 91753 [details] [diff] [review]
*this* is really what i ment to attach

Comment 50 Jonas Sicking (:sicking) 2002-07-17 22:54:02 PDT
Created attachment 91757 [details] [diff] [review]
fixes bzs comments

this also fixes nsXULElement::GetOwnerDocument which is needed to not regress
some xul stuff, got r=jst over irc for that
Comment 51 Jonas Sicking (:sicking) 2002-07-17 22:54:34 PDT
Comment on attachment 91757 [details] [diff] [review]
fixes bzs comments

bringing forward r=jst
Comment 52 Boris Zbarsky [:bz] 2002-07-17 22:59:47 PDT
Comment on attachment 91757 [details] [diff] [review]
fixes bzs comments

sr=bzbarsky if	you fix the nsContentUtils.h comment to fix that in
Comment 53 Kathleen Brade 2002-07-18 06:28:46 PDT
Apologies if I'm confused about the destination of this patch; however, if this
latest patch is intended for the branch, it is incomplete.  It needs the
Composer fix in editor.js (bug 157851; patch 91702) unless that issue has been
addressed in some other way.
Comment 54 Jonas Sicking (:sicking) 2002-07-18 14:23:42 PDT
brade: The patch from that bug should not be needed any more. However it's still
a good thing that it landed on trunk since the new code is faster, cleaner and
Comment 55 Jonas Sicking (:sicking) 2002-07-18 18:06:13 PDT
the checkin for this caused bug 158202, should be fixed soon
Comment 56 Jonas Sicking (:sicking) 2002-07-18 18:13:38 PDT
XUL prototypes are also causing us trouble, we havn't yet ran into a regression
from this, but there is a possibility for it. We're looking into how to fix this
Comment 57 Jonas Sicking (:sicking) 2002-07-18 20:46:18 PDT
Created attachment 91903 [details]
testcase for some of the exploits fixed
Comment 58 Jonas Sicking (:sicking) 2002-07-18 20:50:22 PDT
Created attachment 91904 [details]
another testcase

clicking "dump string contents" will dump the string contents of the url in the
iframe. I even whitespace-normalised it so that it would be easier to read :)

With the patch we throw a security exception.

In attachment 91903 [details] pressing "clone contents" will clone all contents of the
iframe into the main document. Again, the patch makes us throw a security
exception instead.
Comment 59 Daniel Veditz [:dveditz] 2002-07-19 12:36:53 PDT
crash bug 158340 is being blamed on this
Comment 60 Mitchell Stoltz (not reading bugmail) 2002-07-19 12:46:26 PDT
Raising priority to ADT1 based on jst's comments in email
Comment 61 timeless 2002-07-19 12:49:54 PDT
this is blamed for the outstanding smoketest blocker: bug 158167 (not a crash)
Comment 62 Jaime Rodriguez, Jr. 2002-07-20 12:33:06 PDT
Removing adt1.0.1, as this has been said to cause smoketest blocker bug 158167.  

It seems that this has caused a couple of regressions each time it has landed on
the trunk. what steps can we take to make sure taking this fix this late date
does run the risk of breaking something else? 
Comment 63 Jonas Sicking (:sicking) 2002-07-22 12:50:34 PDT
Created attachment 92260 [details] [diff] [review]
fix XUL problem

This fixes a know XUL problem. I'm not sure if this problem actually has
regressed anything in mozilla (I havn't heard of anyone reporting a problem
that could be traced back to this) but better stay on the safe side...
Comment 64 Jonas Sicking (:sicking) 2002-07-22 16:03:34 PDT
Created attachment 92292 [details] [diff] [review]
fix XUL problem version 2

addressed comments from jst
Comment 65 Johnny Stenback (:jst, 2002-07-22 16:18:45 PDT
Comment on attachment 92292 [details] [diff] [review]
fix XUL problem version 2

Comment 66 Boris Zbarsky [:bz] 2002-07-22 17:05:02 PDT
Comment on attachment 92292 [details] [diff] [review]
fix XUL problem version 2

>+  /*

Two stars on this first line, please (doxygen syntax).

>+   * Sets the url of the nodeinfo manager. This should only be called when
>+   * this nodeinfomanager isn't connected to an nsIDocument.

Pick a spelling of "nodeinfo manager" and stick with it?  ;)

>+   * Get hold of each nodes document or uri
>+   */

"node's", not "nodes"

>+      // In theory this should never happen. But since theory and reality is
>+      // different for XUL elements we'll try to get the URI from the

"are different"

>+      nsCOMPtr<nsINodeInfoManager> nimgr;
>+      ni->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+      nimgr->GetDocumentURL(getter_AddRefs(trustedUri));

Can getting nimgr fail or give null here?

>+      nsCOMPtr<nsINodeInfoManager> nimgr;
>+      ni->GetNodeInfoManager(*getter_AddRefs(nimgr));
>+      nimgr->GetDocumentURL(getter_AddRefs(unTrustedUri));

Same here.

>+  if (!trustedUri) {
>+    trustedDoc->GetDocumentURL(getter_AddRefs(trustedUri));

Why is trustedDoc not null here?  What if the nodeinfo manager just gave us a
null URI?

>+  if (!unTrustedUri) {
>+    unTrustedDoc->GetDocumentURL(getter_AddRefs(unTrustedUri));

Same here.

>@@ -552,7 +552,6 @@
>     }
>   }
>-  NS_IF_RELEASE(mDocumentURL);
>   NS_IF_RELEASE(mPrincipal);
>   mDocumentLoadGroup = nsnull;
>@@ -631,6 +630,8 @@
>   if (mNodeInfoManager) {
>     mNodeInfoManager->DropDocumentReference();
>   }
>+  NS_IF_RELEASE(mDocumentURL);
> }

Add a comment explaining why this change was needed.

> nsNodeInfoManager::DropDocumentReference()
> {
>+  if (mDocument) {

Is this ever actually called when mDocument is null?  (Not that I mind the
null-check, but if an assertion would do then we should do that instead.)

>+nsNodeInfoManager::GetDocumentURL(nsIURI** aURL)

Please assert that !mDocument || !mDocumentURL.  It would be even better if we
could assert that (mDocument || mDocumentURL) && (!mDocument ||
!mDocumentURL).... (but I doubt we can do that).

Please clear mDocumentURL in Init() as we discussed.
Comment 67 Johnny Stenback (:jst, 2002-07-22 17:20:12 PDT
An nsINodeInfo will never give back a null node info manager, and it will never
fail to give you a node info manager (unless someone uses a node info that was
never initialized, but we don't need to protect against code that broken).
Comment 68 Jonas Sicking (:sicking) 2002-07-22 18:48:24 PDT
Created attachment 92316 [details] [diff] [review]
Fix bzs comments
Comment 69 Jonas Sicking (:sicking) 2002-07-22 19:19:02 PDT
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

bring forward jsts sr
Comment 70 Boris Zbarsky [:bz] 2002-07-22 19:30:16 PDT
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

r=bzbarsky and all that.
Comment 71 Jonas Sicking (:sicking) 2002-07-23 03:06:10 PDT
Created attachment 92372 [details] [diff] [review]
patch for branch

This is a patch for branch which is a and is a merge of all patches that has
landed in relation to this. Including the patches in bug 158202 and bug 158167
Comment 72 Asa Dotzler [:asa] 2002-07-23 14:40:49 PDT
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

a=asa (on behalf of drivers) for checkin to 1.1
Comment 73 Boris Zbarsky [:bz] 2002-07-23 15:37:11 PDT
Comment on attachment 92372 [details] [diff] [review]
patch for branch

Comment 74 Jonas Sicking (:sicking) 2002-07-23 16:32:49 PDT
Another regression found: bug 158766. Jst is working on it.
Comment 75 Jesse Ruderman 2002-07-23 17:52:23 PDT
Created attachment 92482 [details]
demo that reads text from a form field using appendChild and dom 0
Comment 76 Mitchell Stoltz (not reading bugmail) 2002-07-23 18:04:55 PDT
The testcase Jesse just posted in comment 75 reads the contents of a form on a
page from another host using only the core DOM function appendChild. This is
very serious and directly exploitable, so it's a stop-ship.
Comment 77 Johnny Stenback (:jst, 2002-07-23 20:14:16 PDT
Created attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

This fixes the last known regression caused by this whole security fix.

This change makes sure that the correct JS context is pushed onto the JS stack
when we're focusing and blurring elements from native code. This change does
that by creating a new little JS context "pusher" class that uses existing and
well tested code to do the pushing and popping of the JS context when event
handlers are called. So the bulk of this change is to move exisiting code into
a new class and make the code that used the moved code use the new class, and
then also use the new class in the code that deals with focus and blur.
Comment 78 Jonas Sicking (:sicking) 2002-07-23 20:46:17 PDT
Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

Comment 79 Boris Zbarsky [:bz] 2002-07-23 20:47:27 PDT
Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

Comment 80 Jonas Sicking (:sicking) 2002-07-23 20:54:15 PDT
Comment on attachment 92316 [details] [diff] [review]
Fix bzs comments

checked in earlier today
Comment 81 chris hofmann 2002-07-23 22:08:32 PDT
Comment on attachment 92372 [details] [diff] [review]
patch for branch

a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking into the branch
Comment 82 chris hofmann 2002-07-23 22:09:44 PDT
Comment on attachment 92515 [details] [diff] [review]
Fix the DOM inspector regression

a=chofmann for branch and trunk.
Comment 83 Johnny Stenback (:jst, 2002-07-23 23:22:50 PDT
DOM inspector regression fixed on the trunk.
Comment 84 Johnny Stenback (:jst, 2002-07-24 00:27:22 PDT
Created attachment 92542 [details] [diff] [review]
Complete diff for the branch.

This is a diff against the branch, this includes all the above changes,
including all regression fixes.
Comment 85 scottputterman 2002-07-24 09:03:46 PDT
Bindu, can you verify this on the trunk?
Comment 86 scottputterman 2002-07-24 11:39:43 PDT
changing the branch landing eta to 7/29 since we'll be letting this bake on the
trunk for a few more days.
Comment 87 Jaime Rodriguez, Jr. 2002-07-25 17:14:02 PDT
jst mentioned that there was another regression related to this checkin.
therefore, i am removing adt1.0.1 nomination until the new regression is
addressed. pls renominate when you think the new patch is ready for the branch.
Comment 88 Johnny Stenback (:jst, 2002-07-26 01:06:14 PDT
The new regression is bug 159348. A patch exists (by sicking and mstoltz), but
it's not attached to a bug yet, coming soon.
Comment 89 Johnny Stenback (:jst, 2002-07-26 15:58:36 PDT
... and the saga continues, one of my fixes for one of the regressions here
caused yet one more regression. Bug 159468 is the newest bug in this sad story
(fix attached)... :-(
Comment 90 Johnny Stenback (:jst, 2002-07-26 16:33:34 PDT
Created attachment 92973 [details] [diff] [review]
Patch for the branch, including the fix for bug 159468
Comment 91 Jonas Sicking (:sicking) 2002-07-26 16:47:03 PDT
Created attachment 92977 [details]
stacktrace of new principal code
Comment 92 Jaime Rodriguez, Jr. 2002-07-31 11:02:29 PDT
bsharma: Bindu, looks like the patch for the remaining regression has been
checked into the trunk. Can you pls run DOM tests, and check around for any
possible regressions that might be caused by this fix. If there are none, pls
verify this bug as fixed. thanks!

sicking: Jonas, now that the last regression has been patched, can you resolve
this bug as fixed, and run a diff for 1.0 branch through the review and approval
gauntlet, so that we are ready to check this one in tonight? thanks!
Comment 93 bsharma 2002-07-31 12:21:10 PDT
Verified on 2002-07-31-trunk build in Win 2000.

Ran following tests and they all give an exception.

1. Test case in comment #1
2. Test case in comment #57 and #58
3. Test case in comment #75.
4. Tests in bug 90757

Comment 94 bsharma 2002-07-31 14:21:51 PDT
Adding Siva to the list. He has some DOM tests that he can run.
Comment 95 Jonas Sicking (:sicking) 2002-07-31 14:58:49 PDT
marking this one fixed on request from adt and that it actually is fixed on the
Comment 96 Jonas Sicking (:sicking) 2002-07-31 18:39:36 PDT
Created attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348
Comment 97 Jaime Rodriguez, Jr. 2002-07-31 18:42:54 PDT
can the new patch go as is, or will we need to get new reviews?
Comment 98 Jonas Sicking (:sicking) 2002-07-31 18:45:46 PDT
Comment on attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348

This has got r=bz and sr=jst
Comment 99 Jaime Rodriguez, Jr. 2002-07-31 18:50:44 PDT
adt1.0.1+ (on ADT's behalf) apporval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then replace "mozilla1.0.1+" with
"fixed1.0.1". thanks!

Siva and Bindu: Let's finish the test on the trunk tomorrow, and then focus our
efforts on the branch. Once you have run all your tests on the branch, feel this
is fixed, and that no new regressions were caused by this fix, then pls replace
"mozilla1.0.1+" with "fixed1.0.1". thanks!
Comment 100 Jonas Sicking (:sicking) 2002-07-31 18:58:09 PDT
I'm confused. Who is supposed to change mozilla1.0.1+ to fixed1.0.1? Me or
Comment 101 Jaime Rodriguez, Jr. 2002-07-31 19:01:58 PDT
doh! sorry my bad. apologies all around. 

jonas, you are *of course* suppose to replace "mozilla1.0.1+" with "fixed1.0.1".

once qa verifies the fix on the 1.0 branch, they will replace "fixed1.0.1", with

again, my apologies.
Comment 102 Randell Jesup [:jesup] 2002-08-01 08:18:23 PDT
Comment on attachment 93512 [details] [diff] [review]
patch for branch, including fix for bug 159348

re-approval for branch; change mozilla1.0.1+ to fixed1.0.1 when checked in. 
Please watch Txul, Ts, and Tp after this is checked in for regressions on
Comment 103 Sivakiran Tummala 2002-08-01 13:26:22 PDT
verfied on trunk.
Comment 104 bsharma 2002-08-01 14:01:13 PDT
Verified on 2002-08-01-branch on Win 2000.

1. Test case in Comment #1: Dialog bix appears with the Object names when
clicked on several buttons.
2. Test case in Comment #57: The another Google is opened in the lower frame
when clicked on the Clone contents button. But what I typed in the search box is
not displayed.
3. Test case in Comment #58: When clicked on "dump string content" button, the
contents of Google window are opened in the lower frame.
4. Test case in Comment #75: Typed search string in the Google window, when
clicked on the button, an alert is appeared with the test string from Google
window in it.
4. Tests in bug 90757, all the testse are passed.
Comment 105 Jonas Sicking (:sicking) 2002-08-01 14:14:40 PDT
bsharma: what branch-build did you test this with? This was checked in on the
branch just an hour ago.
Comment 106 Sivakiran Tummala 2002-08-01 14:16:00 PDT
it is to be verified on branch only after fixed1.0.1 is added to the keyword
list. I think they are still waiting for the drivers approval to move the patch
on to branch. I am marking this bug Resolved/verified fixed again.
Comment 107 Sivakiran Tummala 2002-08-01 14:17:38 PDT
marking verified, needs to verify on branch after mozilla1.0.1 is removed and
fixed1.0.1 is added.
Comment 108 Jonas Sicking (:sicking) 2002-08-01 15:57:03 PDT
checked in and cycled green
Comment 109 Sivakiran Tummala 2002-08-02 15:03:52 PDT
verified on branch 08-02-06-1.0. adding keyword verified1.0.1

Note You need to log in before you can comment on or make changes to this bug.