1063837 - xhr and eventsource shouldn't set the nullprincipal in loadInfo

Status: RESOLVED FIXED

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

Description

[Tanvi Vyas[:tanvi]]

When we have systemPrincipal, nsXMLHttpRequest::OnStartRequest() sets the nullPrincipal on the channel because of bug 338583.  This is because didn't want GetChannelPrincipal to return the system principal.  But now that we have a sandbox flag in loadInfo that GetChannelResultPrincipal uses, we should preserve the correct principal and just set the sandbox flag (see bug 965413 and 1062529).

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1960

The code here attempts to copy some of the xhr code, and hence ends up with the same issue.  In this case, there is no reason to set a nullPrincipal, so we should remove lines 374-387.
https://mxr.mozilla.org/mozilla-central/source/content/base/src/EventSource.cpp#372

Comment 1

[Tanvi Vyas[:tanvi]]

Jonas, you mention that the tricky part is what to do here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2129

With this change, even if this IsSystemXHR(), we will set documentPrincipal to mPrincipal / system.  That means the principal set on mResponseXML will be different than what it is today.  Is that okay?  Will that cause compatibility issues?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This code sets the principal a total of three times :)

NS_NewDOMDocument sets the principal to mPrincipal, which is the wrong principal.

Then mResponseXML->SetPrincipal() sets the principal to the right principal.

Then mResponseXML->StartDocumentLoad() sets the principal again by getting the principal off of the channel.

Lovely.


We should get the principal from the channel by calling GetChannelResultPrincipal(), and then passing that principal to NS_NewDOMDocument.

Then we should remove the SetPrincipal() call as it is redundant.

The mResponseXML->StartDocumentLoad() call is fine as it's needed for many other reasons.

That way we'll retain current behavior.

Comment 3

[Tanvi Vyas[:tanvi]]

Attachment: Bug1063837-09-10-14.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> NS_NewDOMDocument sets the principal to mPrincipal, which is the wrong
> principal.
Why do you say NS_NewDOMDocument setting mPrincipal is the wrong principal?  We should change this to use the channel to get the principal, but mPrincipal is what we set on the channel earlier anyway.  Just wanted to confirm with you that there isn't some other principal we should be using here instead.


> Then mResponseXML->SetPrincipal() sets the principal to the right principal.
> 
> Then mResponseXML->StartDocumentLoad() sets the principal again by getting
> the principal off of the channel.
> 
I don't see where this happens.  Does this call point to http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2521?  In that method, we aren't using the channel to get the principal (currently or in bug 1038756).

> We should get the principal from the channel by calling
> GetChannelResultPrincipal(), and then passing that principal to
> NS_NewDOMDocument.
> 
This won't work, because of http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2116.  If we call GetChannelResultPrincipal, we will get the nullPrincipal, and per the comment, we don't want the nullPrincipal.  We want the actual principal, so we need to get it from loadinfo.

Here is a work in progress patch with changes for nsXMLHttpRequest.

Comment 4

[Tanvi Vyas[:tanvi]]

Oh, I should also note that this patch is on top of bug 1038756.  Specifically, you need https://bugzilla.mozilla.org/attachment.cgi?id=8486686&action=diff#a/content/base/src/nsXMLHttpRequest.cpp_sec2

Comment 5

[Tanvi Vyas[:tanvi]]

I spoke to Jonas and we discussed comment 3.

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> > NS_NewDOMDocument sets the principal to mPrincipal, which is the wrong
> > principal.
> Why do you say NS_NewDOMDocument setting mPrincipal is the wrong principal? 
> We should change this to use the channel to get the principal, but
> mPrincipal is what we set on the channel earlier anyway.  Just wanted to
> confirm with you that there isn't some other principal we should be using
> here instead.
> 

Sometimes we use the document (if it exists) and if not we use mPrincipal.  They are same origin but not necessary the same principal instance.


> > Then mResponseXML->StartDocumentLoad() sets the principal again by getting
> > the principal off of the channel.
> > 
> I don't see where this happens.  Does this call point to
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.
> cpp#2521?  In that method, we aren't using the channel to get the principal
> (currently or in bug 1038756).

StartDocumentLoad() calls Reset() which calls GetChannelResultPrincipal.

> 
> > We should get the principal from the channel by calling
> > GetChannelResultPrincipal(), and then passing that principal to
> > NS_NewDOMDocument.
> > 
> This won't work, because of
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsXMLHttpRequest.cpp#2116.  If we call GetChannelResultPrincipal, we will
> get the nullPrincipal, and per the comment, we don't want the nullPrincipal.
> We want the actual principal, so we need to get it from loadinfo.
> 

Jonas says the comment that says using the nullprincipal won't work is wrong these days.  So we can just use GetChannelReulstPrincipal() even though it will sometimes give you a nullprincipal.

New patch coming.

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: Bug1063837-09-10-14B.patch

Updated patch for review.  Includes nsXMLHttpRequest and EventSource.cpp

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8487571 [details] [diff] [review]
Bug1063837-09-10-14B.patch

Review of attachment 8487571 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, though Olli might want to review given that he had opinions on this stuff.

Do a tryserver push for at least one platform first though, to make sure that I'm not wrong about the baseuri stuff.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8487571 [details] [diff] [review]
Bug1063837-09-10-14B.patch

We need a test for this, if we don't have such yet.

Since nodes have .nodePrincipal property in chrome context, testing this
should be rather simple. Just initiate an XHR and check that the
responseXML has null-principal.

Comment 9

[Tanvi Vyas[:tanvi]]

Pushed patch for this bug to try, on top of the patches in Bug 1038756.
https://tbpl.mozilla.org/?tree=Try&rev=baf0523c2ed3

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: Bug1063837-test-09-11-14.patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8488235 [details] [diff] [review]
Bug1063837-test-09-11-14.patch

Could you use async XHR. We're trying to get rid of the sync one.

So 

xhr.open("GET", location);
xhr.onload = function() {
  ok(xhr.responseXML, "We should have response content!");
  var principal = xhr.responseXML.nodePrincipal;
  ok(principal.URI.schemeIs("moz-nullprincipal"), "The response document should have a null principal");
  SimpleTest.finish();
}
xhr.send();

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Bug1063837-test-09-11-14B.patch

Changed to async xhr.  Carrying over r+.

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: Bug1063837-09-22-14.patch

Unbitrotted patch.  Updated some comments.  Carrying over r+.

Comment 14

[Tanvi Vyas[:tanvi]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84c5652507ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/de9ba02af7a6

Note that the test that the bc failed in the earlier try run was because of a bug in 1038756 which has been fixed.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84c5652507ce
https://hg.mozilla.org/mozilla-central/rev/de9ba02af7a6