Blake Kaplan (:mrbkap) (inactive) Assignee
	Description • 13 years ago

In bug 533596, comment 26, Henri pointed to js/src/xpconnect/tests/chrome/test_evalInSandbox.xul (note that the bug number in the test is actually bogus) and asks why it fails with his patch for bug 543435. I debugged it today and found that the test is (partly bogus).

The part of the test that's failing appears to want to test a chrome sandbox. The way it does that is to load a data: URL in an iframe in a chrome document. Unfortunately, it appears that there's a copy/paste error in the test which makes the iframe type=content. This doesn't appear to affect current trunk.

However, even when I make the iframe type=chrome, the test fails with the patch for bug 543435. Henri, I think the remainder of the fix is figuring out why your changes leave the iframe with a content principal. Either way, we should probably take this fix.

As a note, if it's too hard to fix the data: URL behavior, we could also change the test to use an explicit chrome: URL.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 1 • 13 years ago

(In reply to comment #0)
> Created attachment 545554 [details] [diff] [review] [review]
> Proposed fix
> 
> In bug 533596, comment 26, Henri pointed to
> js/src/xpconnect/tests/chrome/test_evalInSandbox.xul (note that the bug
> number in the test is actually bogus) and asks why it fails with his patch
> for bug 543435. I debugged it today and found that the test is (partly
> bogus).

Thank you!

> However, even when I make the iframe type=chrome, the test fails with the
> patch for bug 543435. Henri, I think the remainder of the fix is figuring
> out why your changes leave the iframe with a content principal. Either way,
> we should probably take this fix.
> 
> As a note, if it's too hard to fix the data: URL behavior, we could also
> change the test to use an explicit chrome: URL.

I'm not at all familiar with what *should* be happening. Do you mean that a data: URL should load with a chrome principal but the initial about:blank interferes with the principal selection?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 2 • 13 years ago

> +  <iframe type="chrome"

Are you sure that type="chrome" has an effect on HTML iframes? smaug told me on IRC that it doesn't.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 13 years ago

Oh, that's a good point. But I think HTML iframes always inherit the principal of their owner document, in which case the data: URL should have chrome privileges anyway, right?

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 13 years ago

HTML iframes always inherit their parent docshell type.  So those iframes have the same type of docshell as the parent page loading them.

However that page has a _content_ type docshell, because this is a browser-chrome test and those are run in a content-type docshell (which sucks and all, but there we are).  I was pretty sure we had a bug on that, but I can't find it right now; there are workarounds for it all over the docshell chrome tests, opening new windows which actually have chrome-type docshells to run the test in....

So in this case we have a situation where the loadinfo passed to the docshell has the system principal (the NodePrincipal() of that <iframe>) as the owner, but the load is happening in a content docshell.

Now we get to the code at <http://hg.mozilla.org/mozilla-central/file/887fad3ebc0b/docshell/base/nsDocShell.cpp#l1405>.  ownerPrincipal is system, we're a typeContent docshell, so we null out the owner and set inheritOwner to true.  That means we pass INTERNAL_LOAD_FLAGS_INHERIT_OWNER to InternalLoad.

We're loading a data: URI, which inherits the security context, so this actually has an effect we end up calling GetInheritedPrincipal(PR_TRUE) at http://hg.mozilla.org/mozilla-central/file/887fad3ebc0b/docshell/base/nsDocShell.cpp#l8178

Now GetInheritedPrincipal(PR_TRUE) means we use the principal of the currently loaded document, if any.  If there isn't one, we us the principal of our same-type parent, if any.  If there isn't one, we just go ahead and EnsureContentViewer and use the principal of the resulting document.

Now before Henri's change, I bet we did NOT have a current document, and our parent docshell is same-type, so we used the parent document's principal, which is system.  But with Henri's change we always have an about:blank in the docshell, so we inherit whatever principal that has.

Looking at the patch in bug 543435, that will be a null principal due to the code added to EnsureContentViewer.  That's the right behavior if we want the synthetic about:blank match the behavior of actual about:blank loads; that's why that hunk was added.  But it breaks this particular case.  Note that four required parts of the breakage are that (1) a system-principal page (2) loaded in a content-type docshell has a subframe (3) whose initial src value (4) points to a javascript: or data: URI,

I believe the test in question will fail even without Henri's patch if it's changed to wait until after the initial about:blank loads in the subframe before doing the data: load.  Worth verifying.

Fixing the test is straightforward: it needs to open a new chrome-type window to run itself in, or use a chrome:// URI, or whatever.

An interesting question is how we feel about the compat impact of this on the various abusers who're loading system-principal documents in content-type docshells.... :(

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 13 years ago

One option is to make the "inherit from current document" case of GetInheritedPrincipal initially pretend there's no current document if the current document is the initial about:blank....

	Peter Van der Beken [:peterv]
	Comment 6 • 13 years ago

Comment on attachment 545554 [details] [diff] [review]
Proposed fix

Blake, I'm assuming we don't want this in light of comment 4. Rerequest review if I'm mistaken.

	Marco Bonardo [:mak]
	Comment 8 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/df5afd3a325d