test_evalInSandbox.xul appears to be partly bogus

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 545554 [details] [diff] [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).

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.
Attachment #545554 - Flags: review?(peterv)
(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?
> +  <iframe type="chrome"

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

Comment 3

6 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?
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.... :(
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....
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.
Attachment #545554 - Flags: review?(peterv) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 553600 [details] [diff] [review]
Proposed fix
Assignee: nobody → mrbkap
Attachment #545554 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #553600 - Flags: review?(peterv)
Attachment #553600 - Flags: review?(peterv) → review+
http://hg.mozilla.org/mozilla-central/rev/df5afd3a325d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.