1083422 - Add triggeringPrincipal to nsILoadInfo

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

In bug 1038756, we updated the NS_NewChannel API to pass in a loadingNode (when available) OR a loadingPrincipal. 

We also created an NS_NewChannelInternal method that passed in both.  We call NS_NewChannelInternal when:
* We have a loadingNode and a loadingPrincipal
* The origin of the principal attached to the loadingNode may not be the same as the origin of the loadingPrincipal.

In those cases, what we probably want is an NS_NewChannelWithTriggeringPrincipal() method.  This method would take the loadingNode and a triggeringPrincipal.  The loadingNode tells us where the content is being loaded into and the triggeringPrincipal is the principal of the thing that triggered the load.  In a lot of cases, the principals associated with these are the same (and hence, we use NS_NewChannel most of the time).  But in some cases they differ (ex: a stylesheet may import a subresource; the stylesheet principal is the triggeringPrincipal and the document that the stylesheet is being applied to is the loadingNode for that subresource).  We should represent the difference explicitly when setting loadInfo.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_1_loadinfo_changes.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_2_netutil_changes.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_3_update_callsites.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, you wanna take a first look and see if we are missing something?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: part_4_e10s.patch

We should also support e10s and propagate the triggeringPrincipal if available.

Comment 6

[Tanvi Vyas[:tanvi]]

Comment on attachment 8506455 [details] [diff] [review]
part_1_loadinfo_changes.patch

+NS_IMETHODIMP
+LoadInfo::GetTriggeringPrincipal(nsIPrincipal** aTriggeringPrincipal)
+{
+  if (mTriggeringPrincipal) {
+    NS_ADDREF(*aTriggeringPrincipal = mTriggeringPrincipal);
+    return NS_OK;
+  }
+  // if no triggeringPrincipal, return the loadingPrincipal
+  return GetLoadingPrincipal(aTriggeringPrincipal);
+}

What happens if a caller sets a triggeringPrincipal and a loadingNode who's principal is the same as the triggeringPrincipal?  In that case, we won't know if the caller passed in both triggeringPrincipal and loadingNode, or if they just passed in a loadingNode.

I would rather do this another way.  If a caller passes in a triggeringPrincipal, set it.  Otherwise let it be null.  That way we are fully aware of whether a caller set it or not.

Still reviewing, so not clearing the needinfo flag yet.

Comment 7

[Tanvi Vyas[:tanvi]]

Comment on attachment 8506457 [details] [diff] [review]
part_2_netutil_changes.patch

We no longer need NS_NewChannelInternal() that takes a node and a principal - https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#249.  We can replace this with NS_NewChannelWithTriggeringPrincipal().

As for NS_NewChannelInternal() that takes loadinfo - https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#200 - we can either change that function name to NS_NewChannelWithTriggeringPrincipal() or leave it as is.  Perhaps leaving it as is is best, since some callers may want to pass in LoadInfo but won't necessary have set a triggeringPrincipal.

Comment 8

[Tanvi Vyas[:tanvi]]

Comment on attachment 8506459 [details] [diff] [review]
part_3_update_callsites.patch

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -10215,25 +10215,25 @@ nsDocShell::DoURILoad(nsIURI * aURI,
>     if (inherit) {
>       securityFlags |= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
>     }
>     if (isSandBoxed) {
>       securityFlags |= nsILoadInfo::SEC_SANDBOXED;
>     }
> 
>     if (!isSrcdoc) {
>-        rv = NS_NewChannelInternal(getter_AddRefs(channel),
>-                                   aURI,
>-                                   requestingNode,
>-                                   requestingPrincipal,
>-                                   securityFlags,
>-                                   aContentPolicyType,
>-                                   nullptr,   // loadGroup
>-                                   static_cast<nsIInterfaceRequestor*>(this),
>-                                   loadFlags);
>+        rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),
>+                                                  aURI,
>+                                                  requestingNode,
>+                                                  requestingPrincipal,
>+                                                  securityFlags,
>+                                                  aContentPolicyType,
>+                                                  nullptr,   // loadGroup
>+                                                  static_cast<nsIInterfaceRequestor*>(this),
>+                                                  loadFlags);
> 
Perhaps we should s/requestingPrincipal/triggeringPrincipal here.  Also in dom/xbl/nsXBLService.cpp and image/src/imgLoader.cpp.

Comment 9

[Tanvi Vyas[:tanvi]]

Comment on attachment 8506455 [details] [diff] [review]
part_1_loadinfo_changes.patch

Per comment 6.

Comment 10

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

> What happens if a caller sets a triggeringPrincipal and a loadingNode who's
> principal is the same as the triggeringPrincipal?  In that case, we won't
> know if the caller passed in both triggeringPrincipal and loadingNode, or if
> they just passed in a loadingNode.
> 
> I would rather do this another way.  If a caller passes in a
> triggeringPrincipal, set it.  Otherwise let it be null.  That way we are
> fully aware of whether a caller set it or not.

Keep in mind that the more different things that we allow an nsILoadInfo to contain, the more different things we force all security policy implemwntations to deal with. Including policies implemented by addons which are unlikely to think of every scenario.

We should only allow telling passing in the same triggering principal from passing a null principal if those things are meaningfully different. Which I don't think they are.

So we should either do what the patch does, or we should check if the triggering principal is the same as the loading principal, and if so set the triggering principal to null.

That makes the life easier for all security policies, and we'll end up with fewer security bugs.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> So we should either do what the patch does, or we should check if the
> triggering principal is the same as the loading principal, and if so set the
> triggering principal to null.

I think that makes the most sense given that e10s has to query the triggeringPrincipal for passing it around between child and parent. By falling back to the loadingPrincipal in case the triggeringPrincipal is null, we would have slighlty different values in regular and e10s mode. I think it's best if we set the triggering Principal to null if it's not different from the loadingPrincipal.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> Comment on attachment 8506457 [details] [diff] [review]
> part_2_netutil_changes.patch
> 
> We no longer need NS_NewChannelInternal() that takes a node and a principal

Agreed, we should eliminate NS_NewChannelInternal completely since the name is partially misleading.
I filed bug 1085450, where we can do that split and either call NS_NewChannelWithTriggeringPrincipal or also NS_NewChannelWithLoadInfo which is also important for e10s where we have to propagate loadInfo between child and parent.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #8)
> Comment on attachment 8506459 [details] [diff] [review]
> part_3_update_callsites.patch
> 
> >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
> >--- a/docshell/base/nsDocShell.cpp
> >+++ b/docshell/base/nsDocShell.cpp
>
> Perhaps we should s/requestingPrincipal/triggeringPrincipal here.  Also in
> dom/xbl/nsXBLService.cpp and image/src/imgLoader.cpp.

I did replace it nsXBLService.cpp and and also in the imgLoader.cpp, not sure if it makes sense in nsDocShell.cpp. See the following snippet in nsDocshell.cpp

> nsCOMPtr<nsILoadInfo> loadInfo =
>   new LoadInfo(requestingPrincipal,
>                nullptr, // triggeringPrincipal
>                requestingNode,
>                securityFlags,
>                aContentPolicyType);
> channel->SetLoadInfo(loadInfo);

where we then would have triggeringPrincipal as the first argument and a nullPtr as triggeringPrincipal as the second argument to loadInfo.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_2_netutil_changes.patch

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_3_callsites.patch

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_4_e10s.patch

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_5_consumers.patch

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: tmp_other_consumers_of_loadingprincipal.patch

Jonas, I think we only have to update nsJSProtocolHandler.cpp to use TriggeringPrincipal before falling back to the LoadingPrincipal. To make sure I created that list of all consumers of LoadingPrincipal. Can you please verify that we are not missing something?

Comment 20

[Boris Zbarsky [:bzbarsky]]

It'll likely take me a few days to get to this; I'm behind on reviews.

Comment 21

[Jason Duell]

Comment on attachment 8506546 [details] [diff] [review]
part_4_e10s.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +339,5 @@
> +                               loadInfo,
> +                               aLoadGroup,
> +                               aCallbacks,
> +                               aLoadFlags,
> +                               aIoService);

I'm not much of a fan of one function arg per line style here, and we don't use it much in necko.  Just mush into as few (80 char) lines as possible.

Comment 22

[Jason Duell]

Comment on attachment 8508242 [details] [diff] [review]
trig_2_netutil_changes.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +307,5 @@
> +                               loadInfo,
> +                               aLoadGroup,
> +                               aCallbacks,
> +                               aLoadFlags,
> +                               aIoService);

just lump into 80-char lines

@@ +339,5 @@
> +                               loadInfo,
> +                               aLoadGroup,
> +                               aCallbacks,
> +                               aLoadFlags,
> +                               aIoService);

ditto

Comment 23

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

Comment on attachment 8508246 [details] [diff] [review]
trig_5_consumers.patch

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

bz needs to look at this.

Comment 24

[Tanvi Vyas[:tanvi]]

Comment on attachment 8508241 [details] [diff] [review]
trig_1_loadinfo_changes.patch

>+  /**
>+   * The triggeringPrincipal is the principal that triggerd the load.
>+   * Most likely the triggeringPrincipal and the loadingPrincipal are the same,
>+   * hence we return the loadingPrincipal in case the triggeringPrincipal is not
>+   * set explicitly.
This last part is no longer true with your new patch.

>+   * In some cases the loadingPrincipal and the triggeringPrincipal are different
>+   * however, e.g. a stylesheet may import a subresource. In that case the
>+   * stylesheet principal is the triggeringPrincipal and the doucment that loads
document

>+   * the stylesheet provides a loadingContext and hence the loadingPrincipal.
>+   *
>+   * The triggeringPrincipal can be null, in which case the loadingPrincipal is
>+   * returned.
Take out the "in which case the loadingPrincipal is returned".

Comment 25

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> (In reply to Tanvi Vyas [:tanvi] from comment #7)
> > Comment on attachment 8506457 [details] [diff] [review]
> > part_2_netutil_changes.patch
> > 
> > We no longer need NS_NewChannelInternal() that takes a node and a principal
> 
> Agreed, we should eliminate NS_NewChannelInternal completely since the name
> is partially misleading.
> I filed bug 1085450, where we can do that split and either call
> NS_NewChannelWithTriggeringPrincipal or also NS_NewChannelWithLoadInfo which
> is also important for e10s where we have to propagate loadInfo between child
> and parent.

* Why do we need another bug for removing NS_NewChannelInternal(node, principal)?  Why not just replace NS_NewChannelInteral with NS_NewChannelWithTriggeringPrincipal directly: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#249

For https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#200, you can change the name to NS_NewChannelWithLoadInfo() (either in this bug or the new bug).

* This function isn't needed:
+inline nsresult
+NS_NewChannelWithTriggeringPrincipal(nsIChannel**           outChannel,
+                                     nsIURI*                aUri,
+                                     nsIPrincipal*          aRequestingPrincipal,
+                                     nsIPrincipal*          aTriggeringPrincipal,
+                                     nsSecurityFlags        aSecurityFlags,
+                                     nsContentPolicyType    aContentPolicyType,
+                                     nsILoadGroup*          aLoadGroup = nullptr,
+                                     nsIInterfaceRequestor* aCallbacks = nullptr,
+                                     nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
+                                     nsIIOService*          aIoService = nullptr)
+{

When we create a channel, the combinations of values to pass in are:
* requestingNode - covered in NS_NewChannel(node)
* requestingPrincipal - covered in NS_NewChannel(principal) 
* triggeringPrincipal and requestingNode - will be covered in NS_NewChannelWithTriggeringPrincipal() (replacement for NS_NewChannelInternal)
* loadInfo - currently is NS_NewChannelInternal(loadInfo), and can be renamed to NS_NewChannelWithLoadInfo (either in this bug or in bug 1085450)

The combination of requestingPrincipal and triggeringPrincipal doesn't make sense to me.  We can discuss further in our meeting today.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #25)

> * Why do we need another bug for removing NS_NewChannelInternal(node,
> principal)?  Why not just replace NS_NewChannelInteral with
> NS_NewChannelWithTriggeringPrincipal directly:
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.
> h#249

I don't want to make this bug too confusing, by e.g. changing nsCrossSiteListenerProxy calls that are not related to the triggeringPrincipal [1]. Just easier to do it in a separate bug.

[1]  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCrossSiteListenerProxy.cpp#11220
 
> For
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.
> h#200, you can change the name to NS_NewChannelWithLoadInfo() (either in
> this bug or the new bug).
> 
> * This function isn't needed:
> +inline nsresult
> +NS_NewChannelWithTriggeringPrincipal(nsIChannel**           outChannel,
> +                                     nsIURI*                aUri,
> +                                     nsIPrincipal*         
> aRequestingPrincipal,
> +                                     nsIPrincipal*         
> aTriggeringPrincipal,
> +                                     nsSecurityFlags        aSecurityFlags,
> +                                     nsContentPolicyType   
> aContentPolicyType,
> +                                     nsILoadGroup*          aLoadGroup =
> nullptr,
> +                                     nsIInterfaceRequestor* aCallbacks =
> nullptr,
> +                                     nsLoadFlags            aLoadFlags =
> nsIRequest::LOAD_NORMAL,
> +                                     nsIIOService*          aIoService =
> nullptr)
> +{

Doesn't make sense at first, but is needed. We need it in e10s ::DoAsyncOpen()
https://bugzilla.mozilla.org/attachment.cgi?id=8508245&action=diff
otherwise we would have to create the loadInfo in ::DoAsyncOpen() which I tried to avoid.
 
> When we create a channel, the combinations of values to pass in are:
> * requestingNode - covered in NS_NewChannel(node)
> * requestingPrincipal - covered in NS_NewChannel(principal) 
> * triggeringPrincipal and requestingNode - will be covered in
> NS_NewChannelWithTriggeringPrincipal() (replacement for
> NS_NewChannelInternal)
> * loadInfo - currently is NS_NewChannelInternal(loadInfo), and can be
> renamed to NS_NewChannelWithLoadInfo (either in this bug or in bug 1085450)

Yes, those are the combinations which we should try to use.
 
> The combination of requestingPrincipal and triggeringPrincipal doesn't make
> sense to me.  We can discuss further in our meeting today.

As I said, e10s. Unfortunate, but I think that's the best option we have.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #24)
> Comment on attachment 8508241 [details] [diff] [review]
> trig_1_loadinfo_changes.patch
> 
> >+  /**
> >+   * The triggeringPrincipal is the principal that triggerd the load.
> >+   * Most likely the triggeringPrincipal and the loadingPrincipal are the same,
> >+   * hence we return the loadingPrincipal in case the triggeringPrincipal is not
> >+   * set explicitly.
> This last part is no longer true with your new patch.
> 
> >+   * In some cases the loadingPrincipal and the triggeringPrincipal are different
> >+   * however, e.g. a stylesheet may import a subresource. In that case the
> >+   * stylesheet principal is the triggeringPrincipal and the doucment that loads
> document
> 
> >+   * the stylesheet provides a loadingContext and hence the loadingPrincipal.
> >+   *
> >+   * The triggeringPrincipal can be null, in which case the loadingPrincipal is
> >+   * returned.
> Take out the "in which case the loadingPrincipal is returned".

Oh yes, good catch. Updated the code but not the comment - will incorporate those changes.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8508241 [details] [diff] [review]
trig_1_loadinfo_changes.patch

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

Just adding a note since we discussed that part in the meeting today.

::: docshell/base/LoadInfo.cpp
@@ +19,5 @@
>                     nsINode* aLoadingContext,
>                     nsSecurityFlags aSecurityFlags,
>                     nsContentPolicyType aContentPolicyType)
> +  : mLoadingPrincipal(aLoadingPrincipal)
> +  , mTriggeringPrincipal(aTriggeringPrincipal)

Note to myself: We should only store the TriggeringPrincipal if it's different from the LoadingPrincipal.

Comment 29

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

Comment on attachment 8508249 [details] [diff] [review]
tmp_other_consumers_of_loadingprincipal.patch

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

Otherwise this looks good to me.

::: caps/nsScriptSecurityManager.cpp
@@ +332,5 @@
>              return NS_OK;
>          }
>  
>          if (loadInfo->GetForceInheritPrincipal()) {
> +            // No need to be updated!

Actually, this might want to be TriggeringPrincipal. Bz should look at this.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8508243 [details] [diff] [review]
trig_3_callsites.patch

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

Note to myself: Tanvi just brought to my attention that we forgot to change this callsite:
https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#1562

Comment 31

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

Comment on attachment 8508249 [details] [diff] [review]
tmp_other_consumers_of_loadingprincipal.patch

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

Otherwise this looks good to me.

::: caps/nsScriptSecurityManager.cpp
@@ +332,5 @@
>              return NS_OK;
>          }
>  
>          if (loadInfo->GetForceInheritPrincipal()) {
> +            // No need to be updated!

Actually, this might want to be TriggeringPrincipal. Bz should look at this.

@@ +337,1 @@
>              NS_ADDREF(*aPrincipal = loadInfo->LoadingPrincipal());

bz says that this should be TriggeringPrincipal. Which is quite important and a security problem if we get it wrong.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8508243 [details] [diff] [review]
trig_3_callsites.patch

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

::: content/base/src/WebSocket.cpp
@@ +1404,4 @@
>                   doc,
>                   nsILoadInfo::SEC_NORMAL,
>                   nsIContentPolicy::TYPE_WEBSOCKET);
>    rv = wsChannel->SetLoadInfo(loadInfo);

Note to myself: Probably this should be
new LoadInfo(doc->NodePrincipal(),
             mPrincipal,
             doc,
             ...

::: docshell/base/nsDocShell.cpp
@@ +10289,3 @@
>                             requestingNode,
>                             securityFlags,
>                             aContentPolicyType);

Note to myself: this should be
new Loadinfo(requestingNode->NodePrincipal,
             requestingPrincipal, // or better triggeringPrincipal
             requestingNode,
             ...

Comment 33

[Tanvi Vyas[:tanvi]]

Comment on attachment 8508242 [details] [diff] [review]
trig_2_netutil_changes.patch

Talked to Christoph  about a few places where triggeringPrincipal and loadingPrincipal got swapped.  Looks like he just caught similar issues in other patches in comment 32.

>Bug 1083422 - Add triggering Principal to nsILoadInfo - netutil changes (r=mcmanus)
>
>diff --git a/netwerk/base/public/nsNetUtil.h b/netwerk/base/public/nsNetUtil.h
>--- a/netwerk/base/public/nsNetUtil.h
>+++ b/netwerk/base/public/nsNetUtil.h
>@@ -257,31 +257,97 @@ NS_NewChannelInternal(nsIChannel**      
>                       nsIInterfaceRequestor* aCallbacks = nullptr,
>                       nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
>                       nsIIOService*          aIoService = nullptr)
> {
>   NS_ASSERTION(aRequestingPrincipal, "Can not create channel without a requesting Principal!");
> 
>   nsCOMPtr<nsILoadInfo> loadInfo =
>     new mozilla::LoadInfo(aRequestingPrincipal,
>+                          nullptr, // aTriggeringPrincipal
>                           aRequestingNode,
>                           aSecurityFlags,
>                           aContentPolicyType);
Technically, no one should be calling this anymore since we have NS_NewChannelWithTriggeringPrincipal() in this bug.  But in case they do, this should be:
     new mozilla::LoadInfo(aRequestingNode->NodePrincipal()
                           aTriggeringPrincipal, //the principal that was passed in
                           aRequestingNode,
                           aSecurityFlags,

>   if (!loadInfo) {
>     return NS_ERROR_UNEXPECTED;
>   }
>   return NS_NewChannelInternal(outChannel,
>                                aUri,
>                                loadInfo,
>                                aLoadGroup,
>                                aCallbacks,
>                                aLoadFlags,
>                                aIoService);
> }




>@@ -426,16 +492,17 @@ NS_OpenURIInternal(nsIStreamListener*   
>                    nsIInterfaceRequestor* aCallbacks = nullptr,
>                    nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
>                    nsIIOService*          aIoService = nullptr)
> {
>   NS_ASSERTION(aRequestingPrincipal, "Can not create channel without a requesting Principal!");
> 
>   nsCOMPtr<nsILoadInfo> loadInfo =
>     new mozilla::LoadInfo(aRequestingPrincipal,
>+                          nullptr, // aTriggeringPrincipal
>                           aRequestingNode,
>                           aSecurityFlags,
>                           aContentPolicyType);
Same here.



>@@ -626,16 +693,17 @@ NS_NewInputStreamChannelInternal(nsIChan
> 
>   if (!aContentCharset.IsEmpty()) {
>     rv = channel->SetContentCharset(aContentCharset);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
> 
>   nsCOMPtr<nsILoadInfo> loadInfo =
>     new mozilla::LoadInfo(aRequestingPrincipal,
>+                          nullptr, // aTriggeringPrincipal
>                           aRequestingNode,
>                           aSecurityFlags,
>                           aContentPolicyType);
And same here.

Comment 34

[Boris Zbarsky [:bzbarsky]]

I'm not going to get to this until Tuesday, sorry.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #34)
> I'm not going to get to this until Tuesday, sorry.

Thanks for the update - in that case I will unbitrot the patches and incorporate feedback and suggestions already made in this bug. Will be up Monday night and ready for you to review on Tuesday. Thanks!

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

Here is what I addressed in this patch:
* triggeringPrincipal is only set if different from the loadingPrincipal
* comments in the *.idl above 'triggeringPrincipal'
* unbitroted patch

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_2_netutil_changes.patch

Here is what I addressed in this patch:
* To make it not completely confusing, I added an argument 'triggeringPrincipal' to all the internal functions. If e.g. we call NS_NewChannel[Node] then this function
is responsible for passing the right arguments to NS_newChannelinternal which takes all 3 args (Node, LoadingPrincipal, TriggeringPrincipal).
* unbitroted patch

Carrying over r+ from jduell.

Tanvi, wanna take a look at this again?

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_3_callsites.patch

Here is what I addressed in this patch:
* WebSocket.cpp so mPrincipal is the triggeringPrincipal instead of the loadingPrincipal
* nsDocShell.cpp to use the (node, loadingPrincipal, triggeringPrincipal) when creating a new LoadInfo
* unbitroted patch

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_4_e10s.patch

Here is what I addressed in this patch:
* unbitroted patch

Carrying over r+ from jduell.

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_5_consumers.patch

Here is what I addressed in this patch:
* unbitroted patch

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Attachment: tmp_other_loadinfo_consumers.patch

Jonas said you probably want to look that over. Can you make sure we caught all the cases where we have to update consumers in patch trig_5_consumers.patch? Here is a list with comments where the other consumers of loadInfo occur in the code.

Comment 42

[Tanvi Vyas[:tanvi]]

Comment on attachment 8516258 [details] [diff] [review]
trig_2_netutil_changes.patch

We don't want to add a third parameter to NS_OpenURIInternal() and friends.  Adding triggeringPrincipal to NS_OpenURI (and friends) is part of bug 1085450.  There, we have NS_OpenURIWithTriggeringPrincipal that takes a node and a triggeringPrincipal.  Asking callers to pass a node, triggering principal, and requesting principal is too confusing and error prone.

We also have an NS_OpenURIWithTriggeringAndLoadingPrincipal().  This takes both the requestingPrincipal and the triggeringPrincipal.  We had to add this case for e10s, where we can't pass along the Node, so take the principal from the node and pass that instead.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1085450#c2 and line 374 onwards here https://bugzilla.mozilla.org/attachment.cgi?id=8514609&action=diff#a/netwerk/base/public/nsNetUtil.h_sec4

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #42)
> Comment on attachment 8516258 [details] [diff] [review]
> trig_2_netutil_changes.patch
> 
> We don't want to add a third parameter to NS_OpenURIInternal() and friends. 
> Adding triggeringPrincipal to NS_OpenURI (and friends) is part of bug
> 1085450.  There, we have NS_OpenURIWithTriggeringPrincipal that takes a node
> and a triggeringPrincipal.  Asking callers to pass a node, triggering
> principal, and requesting principal is too confusing and error prone.

There is no call to NS_OpenURIInternal() from outside nsNetutil.h and I think we don't want one central function that does all the heavy lifting. We do the same thing for NS_NewChannel, where we have on taking a node, one with a principal, ... and all of them are forwared to NS_NewChannelInternal in the end.

> We also have an NS_OpenURIWithTriggeringAndLoadingPrincipal().  This takes
> both the requestingPrincipal and the triggeringPrincipal.  We had to add
> this case for e10s, where we can't pass along the Node, so take the
> principal from the node and pass that instead.

I added NS_OpenURIWithTriggeringPrincipal, which passes a node and a triggeringPrincpial and internally calls NS_OpenURIInternal which takes 3 args (the node, the loadingPrincipal, and the requestingPrincipal). We should try to not end up having too many different functions, which is probably going to be really confusing for calles for NS_newChannel and friends. We can again simplify in bug 1085450. For now, I would like to keep things as they are.

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

Since bug 971432 completely bitrotted changes for this bug and since this is security critical stuff touching code in docShell I though it's a good idea to re-export all of the patches before review.

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_2_netutil_changes.patch

Carrying over r+ from jduell.

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_3_callsites.patch

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_4_e10s.patch

Carrying over r+ from jduell.

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_5_consumers.patch

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: tmp_other_consumers_of_loadingprincipal.patch

Comment 50

[Tanvi Vyas[:tanvi]]

Comment on attachment 8518403 [details] [diff] [review]
trig_3_callsites.patch

In Loader.cpp and imgLoader.cpp, we called the internal methods because they are cases where the triggeringPrincipal and the principal of the requestingNode are different.  If we don't have the requestingNode available, we should go find it instead of falling back to NS_NewChannel and NS_OpenURI.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1085450#c9 for more details.  If "aLoadData->mRequestingNode" and "requestingNode" are null, we should ask someone who is familiar with the code how we can get the node.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8518398 [details] [diff] [review]
trig_1_loadinfo_changes.patch

>+++ b/docshell/base/LoadInfo.cpp
>+  NS_ADDREF(*aTriggeringPrincipal = mTriggeringPrincipal);

That's going to crash if mTriggeringPrincipal is null.  NS_IF_ADDREF, and add a test that would have caught this.

>+++ b/docshell/base/nsILoadInfo.idl
>+   * in which case triggeringPrincipal returns a nullptr.

"returns null".

>+   * triggeringPrincipal returns a nullptr.

And here.

The decision to have triggering principal be null instead of == loadingPrincipal is a bit odd to me.  It means consumers have to worry about it being possibly null in which case they need to fall back to loadingPrincipal themselves, no?

The discussion in this bug doesn't make it clear why we went this direction instead of just setting triggeringPrincipal = loadingPrincipal if !triggeringPrincipal...

r=me modulo that

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8518403 [details] [diff] [review]
trig_3_callsites.patch

>+++ b/docshell/base/nsDocShell.cpp
>+        new mozilla::LoadInfo(requestingNode->NodePrincipal(),

What guarantees requestingNode is not null here?  I don't think anything does.

>+              new LoadInfo(requestingNode->NodePrincipal(),

Likewise.

Also, might be good to consistently not use "mozilla::" here.

>+                                            requestingNode->NodePrincipal(),

same issue here.

>+++ b/dom/plugins/base/nsPluginHost.cpp
>+  rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),

That function assumes it's passed a non-null node.  Why do you think doc is non-null?

>+++ b/dom/plugins/base/nsPluginStreamListenerPeer.cpp
>+  rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),

And here.

>+++ b/dom/xbl/nsXBLService.cpp
>+  rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),

And here for aBoundDocument.  Which can explicitly be null per the nice comment above, no?

>+++ b/image/src/imgLoader.cpp

Ah, here the node is null-checked, good.  ;)

That said, having to branch on it like this is annoying.  I'd almost rather NS_NewChannelWithTriggeringPrincipal/NS_OpenURIWithTriggeringPrincipal accepted a null node and did the right thing instead of forcing all consumers to copy this branch code.

>+++ b/layout/style/Loader.cpp
>@@ -1555,30 +1569,30 @@ Loader::LoadSheet(SheetLoadData* aLoadDa
>+  rv = NS_NewChannelWithTriggeringPrincipal(getter_AddRefs(channel),

Why is aLoadData->mRequestingNode non-null?

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8518406 [details] [diff] [review]
trig_5_consumers.patch

Yeah, this is exactly why I think the API added in part 1 is broken.  :(

r=me given that API, I guess.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8518406 [details] [diff] [review]
trig_5_consumers.patch

Though, wait.  Why do we want the triggering principal here, not the loading principal?  That seems fishy.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8518407 [details] [diff] [review]
tmp_other_consumers_of_loadingprincipal.patch

It's hard to evaluate this without understanding when the two principals are different.  Apart from the stylesheet cases (when loading == document and triggering == sheet, right?) and whatever the heck is going on in docshell, are there other cases where they differ?

Comment 56

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

> The decision to have triggering principal be null instead of == loadingPrincipal is a bit odd to me.

We have been discussing different options, but couldn't really find the best solution. We decided to let triggeringPrincipal be null if not set explicitly so consumers could find out whether it was set explicitly or not by quering the triggeringPrincipal. Now that I think about it again, I lean more towards setting the triggeringPrincipal = loadingPrincipal if not set explicitly so callers don't have to worry about it. All these principals are probably already confusing enough for consumers. If Jonas and Tanvi think differently, we can obviously discuss that again.

Here is a list of what I changed:
* Set triggeringPrincipal = loadingPrincipal in the constructor of Loadinfo if triggeringPrincipal is not provided explicitly.
* Added a MOZ_ASSERT in the constructor that mTriggeringPrincipal is not null, hence didn't update NS_IF_ADDREF in GetTriggeringPrincipal, rather leaving NS_ADDREF. Hence, I also haven't added a testcase for that. Good catch however, because we were facing the same problem in GetLoadingDocument, where I added an NS_ENSURE_ARG_POINTER check.
* I will file a follow up to add a (compiled code) test. As we keep enhancing LoadInfo we should really add some testcases.
* Adapted comments accordingly.

Carrying over r+ from bz.

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_2_netutil_changes.patch

> I'd almost rather NS_NewChannelWithTriggeringPrincipal/NS_OpenURIWithTriggeringPrincipal accepted a null node and did the right thing instead of forcing all consumers to copy this branch code.

That makes sense to me, having all these callsites checks is annoying. I don't like branching eveverywhere. I incorporated that change and use the triggeringPrincipal as the loadingPrincipal if the node is null.

Should we have a node everywhere? Is it worth filing a follow up and investigate all these callsites?

Still carrying over r+ from jduell since that was a minor change.

Comment 58

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_3_callsites.patch

Here is a list of what I changed:
* Since we changed NS_NewChannelWithTriggeringPrincipal/NS_OpenURIWithTriggeringPrincipal we can leave the callsites as they are right now. No need to introduce branchy-code if the requestingNode does not exist.
* Consistently not using "mozilla::" when creating a new LoadInfo

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_4_e10s.patch

Just rebased, carrying over r+ from jduell.

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_5_consumers.patch

> Yeah, this is exactly why I think the API added in part 1 is broken.  :(
Looks better now.

> Though, wait.  Why do we want the triggering principal here, not the loading principal?  That seems fishy.

I am not sure if we want the triggeringPrincipal or the loadingPrincipal in that case. I can tell for sure that
> /tests/dom/base/test/csp/test_CSP_inlinescript.html
fails if we use loadingPrincipal. Here are the two principals when evaluating the inline script:

> TriggeringPrincipal: http://mochi.test:8888/tests/dom/base/test/csp/file_CSP_inlinescript_main_allowed.html
> LoadingPrincipal:    http://mochi.test:8888/tests/dom/base/test/csp/test_CSP_inlinescript.html

If we use the LoadingPrincipal it cannot find the appropriate Content Scurity Policy and hence the test fails.

Not carrying over r+, giving bz a chance to veto again.

Comment 61

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #55)
> It's hard to evaluate this without understanding when the two principals are different.
> Apart from the stylesheet cases (when loading == document and triggering == sheet, right?) and whatever the heck is going on in docshell, are there other cases where they differ?

Well, we do have code where we call NS_newChannelWithTriggeringPrincipal in many cases (see trig_3_callsites.patch). Agreed, in most of those cases we just don't have a node available and hence use a triggeringPrincipal. I guess figuring out all of these cases would need further investigation. Can we land this and file a followup?

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Good catch however, because we were facing the same problem in GetLoadingDocument

No, that's a totally different problem.  Please take out that NS_ENSURE_ARG_POINTER check, because if someone passes null there we should in fact crash to make them realize they made a huge mistake.

> I will file a follow up to add a (compiled code) test.

For what, exactly?

> Should we have a node everywhere?

Possibly.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8519687 [details] [diff] [review]
trig_5_consumers.patch

r=me

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8519685 [details] [diff] [review]
trig_3_callsites.patch

r=me

Comment 65

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #62)
> > Good catch however, because we were facing the same problem in GetLoadingDocument
> 
> No, that's a totally different problem.  Please take out that
> NS_ENSURE_ARG_POINTER check, because if someone passes null there we should
> in fact crash to make them realize they made a huge mistake.

Ok, will do.

> > I will file a follow up to add a (compiled code) test.
> 
> For what, exactly?

I think we should add some testcases around the LoadInfo, e.g. setting/retrieving the loadInfo on channels including setting/retrieving values within the loadInfo. I will discuss that also with Jonas and Tanvi in our meeting, what tests will be appropriate.

Comment 66

[Boris Zbarsky [:bzbarsky]]

I agree, but I don't see why we need compiled code tests.

Comment 67

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

Just took out NS_ENSURE_ARG_POINTER as bz suggested.
Carrying over r+ from bz.

Comment 68

[Tanvi Vyas[:tanvi]]

If we are call NS_NewChannelWithTriggeringPrincipal, I think we should have a node.  Why would we have cases where we don't have a node but we do have a triggering principal?  Shouldn't we just call NS_NewChannel with a loadingPrincipal in that case?

The loadingNode and loadingPrincipal tell us where the resource is being loaded into, and the triggeringPrincipal tells us who initiated the load.  If we do in fact have a separate triggeringPrincipal (which in most case we don't), then there should be a document to go along with it.  If we do not, then it's probably not a triggeringPrincipal but a loadingPrincipal.

Comment 69

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky [:bz] from comment #51)
> Comment on attachment 8518398 [details] [diff] [review]
> trig_1_loadinfo_changes.patch
>
> The decision to have triggering principal be null instead of ==
> loadingPrincipal is a bit odd to me.  It means consumers have to worry about
> it being possibly null in which case they need to fall back to
> loadingPrincipal themselves, no?
>
> The discussion in this bug doesn't make it clear why we went this direction
> instead of just setting triggeringPrincipal = loadingPrincipal if
> !triggeringPrincipal...
>
> r=me modulo that

We discussed this with Jonas and had a few options.  I really wanted to have a way to extract from the loadInfo whether the caller explicitly set a triggeringPrincipal, since this only happens in a handful of cases.  If we set the the triggeringPrincipal=loadingPrincipal when it's not passed in, we don't know whether the caller passed in a triggeringPrincipal or whether they passed in the same triggerPrincipal and loadingPrincipal (which shouldn't happen).  If they are the same, the caller should have called NS_NewChannel instead of NS_NewChannelWithTriggeringPrincipal().

Comment 70

[Boris Zbarsky [:bzbarsky]]

> I really wanted to have a way to extract from the loadInfo whether the caller explicitly
> set a triggeringPrincipal

Except you couldn't do that with the previous patch, since null triggeringPrincipal could either mean the caller didn't set it _or_ caller set it to the same thing as loadingPrincipal.

> If they are the same, the caller should have called NS_NewChannel instead of
> NS_NewChannelWithTriggeringPrincipal()

This seems like a pretty major annoyance for callers: they have to check their principals.

The simplest thing to get what you want would be to simply have callers pass a loading principal and maybe a triggering one and save both pointers as-is. Then you can tell exactly what the caller passed in, if that's what's really desired here.

Comment 71

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_6_more_consumer_updates.patch

Boris, it seems that Jonas was right! When pushing to try [1] I realized that we probably have to update all consumers of loadInfo where we call GetForceInheritPrincipal() [2] to use the TriggeringPrincipal rather then the LoadingPrincipal. I updated those places and tested locally all of the failing tests - all of them pass. I pushed to try again to make sure I am not missing anything [3].

Further, there was also another test failing:
* dom/workers/test/test_webSocket_sharedWorker.html
where the doc is null in WebSocket.cpp when we create a LoadInfo. I fixed this by doing what we do in patch trig_3_callsites, where we use the triggeringPrincipal as the loadingPrincipal in case the node is null.

Once reviewed I can fold those changes into the other patches.

[1] https://tbpl.mozilla.org/?tree=Try&rev=d7e97820f531
[2] http://mxr.mozilla.org/mozilla-central/search?string=GetForceInheritPrincipal%28%29
[3] https://tbpl.mozilla.org/?tree=Try&rev=0775699a3266

Comment 72

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_7_default_args_in_e10s.patch

> [Child 1850] ###!!! ABORT: unknown union type: file /builds/slave/try-lx-00000000000000000000000/build/obj-firefox/ipc/ipdl/PWyciwygChannelChild.cpp, line 909

Isn't something we where touching with this bug. Providing default arguments for the principalInfo, apparently it can't really handle uninitialized values.
Passing tests that where failing locally. Since we are not really touching e10s code here, I think that should resolve our problems.
Once reviewed, I can fold this patch into the other e10s patch in this bug.

Jason, any objections/comments?

Comment 73

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #71)
> [3] https://tbpl.mozilla.org/?tree=Try&rev=0775699a3266

No need to consume more resources on this one - stopping try. I think we have seen enough.

Comment 74

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #56)
> Now that I think about it again, I lean more towards setting the
> triggeringPrincipal = loadingPrincipal if not set
> explicitly so callers don't have to worry about it.

I agree.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #57)
> Should we have a node everywhere? Is it worth filing a follow up and
> investigate all these callsites?

I know of only two valid situations when it's valid to have a null node:
* When chrome code that is not related to a chrome document is initiating a network request.
* When a Worker is initiating a network request.

For the former the loadingPrincipal and triggeringPrincipal should both be the system principal.

For the latter we ideally should pass in something like a WorkerPrivateParent::LoadInfo [1] which represents a Worker but which is accessible from the main thread.

So ideally the LoadInfo ctor should assert that it's either getting a Node or a WorkerPrivateParent::LoadInfo, or that the loadingPrincipal and triggeringPrincipal both are system principals.

But I'm not sure if we are able to do that quite yet. But that's where we should get eventually.

[1] http://hg.mozilla.org/mozilla-central/file/e30de6729aff/dom/workers/WorkerPrivate.h#l148


(In reply to Tanvi Vyas [:tanvi] from comment #69)
> We discussed this with Jonas and had a few options.  I really wanted to have
> a way to extract from the loadInfo whether the caller explicitly set a
> triggeringPrincipal, since this only happens in a handful of cases.

I definitely see the logic in wanting to reflect exactly what principals the caller set. However from a semantic point of view there's always a triggering principal. I.e. *someone* always caused a given channel to load. Likewise there's always a loading principal since we always load a given channel into something.

So it makes life simpler for consumers of LoadInfo if we make the LoadInfo reflect those semantics, rather than allow people that create channels to pass in whatever they want and then ask any consumers of LoadInfo to just deal. That is similar to the situation we have with nsIContentPolicy right now and it's not a pretty situation :)

It's better that we instead standardize and say this is what you *must* pass in in order to create a channel. We get to set the rules, so lets make the rules be something that makes for a good Gecko.

Comment 75

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #74)
> (In reply to Tanvi Vyas [:tanvi] from comment #69)
> > We discussed this with Jonas and had a few options.  I really wanted to have
> > a way to extract from the loadInfo whether the caller explicitly set a
> > triggeringPrincipal, since this only happens in a handful of cases.
> 
> I definitely see the logic in wanting to reflect exactly what principals the
> caller set. However from a semantic point of view there's always a
> triggering principal. I.e. *someone* always caused a given channel to load.
> Likewise there's always a loading principal since we always load a given
> channel into something.
> 
Okay, in that case we should do the following:
1) Ensure that we always have a loadingPrincipal (either passed in by the caller or extracted from the loadingNode)
2) If the caller passes in a triggeringPrincipal, use that as triggeringPrincipal
3) If a caller does not pass in a triggeringPrincipal, use the loadingPrincipal as the triggeringPrincipal.

Consumers can then decide to use the triggeringPrincipal or the loadingPrincipal without having to do a null check and a fallback.

The reason for number 1 is that we should be able to rely on the loadingPrincipal actually being the loadingPrincipal.  If a caller calls NS_NewChannelWithTriggeringPrincipal and passes in a triggeringPrincipal of a stylesheet, I don't want to use that as loadingPrincipal.

Comment 76

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #75)
> Okay, in that case we should do the following:
> 1) Ensure that we always have a loadingPrincipal (either passed in by the
> caller or extracted from the loadingNode)
> 2) If the caller passes in a triggeringPrincipal, use that as
> triggeringPrincipal
> 3) If a caller does not pass in a triggeringPrincipal, use the
> loadingPrincipal as the triggeringPrincipal.

I think that is exactly what the current patches do. No?

Comment 77

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #76)
> (In reply to Tanvi Vyas [:tanvi] from comment #75)
> > Okay, in that case we should do the following:
> > 1) Ensure that we always have a loadingPrincipal (either passed in by the
> > caller or extracted from the loadingNode)

> I think that is exactly what the current patches do. No?


Here we set the loadingPrincipal to the triggeringPrincipal if the caller didn't pass in a node:
https://bugzilla.mozilla.org/attachment.cgi?id=8519684&action=diff#a/netwerk/base/public/nsNetUtil.h_sec2

Comment 78

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #77)
> Here we set the loadingPrincipal to the triggeringPrincipal if the caller
> didn't pass in a node:
> https://bugzilla.mozilla.org/attachment.cgi?id=8519684&action=diff#a/netwerk/
> base/public/nsNetUtil.h_sec2

Look at the specific cases Boris mentioned in Comment 52:
> That said, having to branch on it like this is annoying.  I'd almost rather
> NS_NewChannelWithTriggeringPrincipal/NS_OpenURIWithTriggeringPrincipal accepted
> a null node and did the right thing instead of forcing all consumers to copy this branch code.

So, in DocShell for example, we now for a fact that we don't always have a node available, in which case both principals end up being the same anyway. Rather than branching on the node, we accept a null node and set the loadingPrincipal = triggeringPrincipal.

Comment 79

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

(In reply to Tanvi Vyas [:tanvi] from comment #75)
> Okay, in that case we should do the following:
> 1) Ensure that we always have a loadingPrincipal (either passed in by the
> caller or extracted from the loadingNode)
> 2) If the caller passes in a triggeringPrincipal, use that as
> triggeringPrincipal
> 3) If a caller does not pass in a triggeringPrincipal, use the
> loadingPrincipal as the triggeringPrincipal.

Sounds great! Definitely agree.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8520800 [details] [diff] [review]
trig_6_more_consumer_updates.patch

Yay testing.  r=me

Comment 81

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_6_callsite_updates.patch

In our meeting today (Jonas, Tanvi, and I) decided to rather update callsites and create channels differently based on what information is available on the callsite. E.g, if a node is available we rather call NS_newChannel*Node*(), etc. The idea behind that is that we rather go back and change individual callsites than changing the core of the new API after people already started using the new API in which case we would have to revisit all of the callsites.
For example: Ideally we should always have a node available in XBLService.cpp, so right no we branch on the node. In follow ups we are going to revisit those callsites and make sure we have a node available.

In this patch we brought back assertions in nsNetutil.h making sure we pass the required arguments and update callsites accordingly. NewChannelWithTriggeringPrincipal and also OpenURIWithTriggeringPrincipal are not falling back to use the triggeringPrincipal as the loadingPrincipal in case no node is passed as an argument. Once reviewed and approved I will qfold the code in the apprpriate patches before landing.

As agreed in the meeting, Jonas is going to look that over first. Once he greenlights it, we still would like the final approval from Boris!

Comment 82

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

Comment on attachment 8521979 [details] [diff] [review]
trig_6_callsite_updates.patch

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

r=me, though bz needs to review this too.

::: dom/plugins/base/nsPluginHost.cpp
@@ +3085,5 @@
> +                       nullptr,  // aLoadGroup
> +                       listenerPeer);
> +  }
> +  else {
> +    principal = do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);

Add a comment here saying that if this happens that we really have no idea where this load is coming from, and that we probably should do something better than use a null principal.

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ +652,5 @@
> +  nsCOMPtr<nsIChannel> channel;
> +  if (doc) {
> +    rv = NS_NewChannel(getter_AddRefs(channel),
> +                       mURL,
> +                       doc,

Get the element using owner->GetDOMElement(), QI to nsINode, and pass that in as the node.

@@ +660,5 @@
> +                       callbacks);
> +  }
> +  else {
> +    nsCOMPtr<nsIPrincipal> principal =
> +      do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);

Same as in nsPluginHost.cpp

Comment 83

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

Also, you should push to try to see if any of those assertions fire.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521979 [details] [diff] [review]
trig_6_callsite_updates.patch

Please MOZ_ASSERT the system principal bits.  And probably the aRequestingNode bits in nsNetUtil.

r=me

Comment 85

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #83)
> Also, you should push to try to see if any of those assertions fire.

Thanks Boris and Jonas; Let's see if our assumptions hold:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a25e4a0b87d8
or
  https://tbpl.mozilla.org/?tree=Try&rev=a25e4a0b87d8

Comment 86

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_1_loadinfo_changes.patch

Here is a series of new/updated/unbitroted and qfolded patches.
Carrying over r+ from bz.

Comment 87

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_2_netutil_changes.patch

Carrying over r+ from jduell.

Comment 88

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_3_callsites.patch

Added missing include (nsIDomElement.h) so that patches compile on TBPL.
Carrying over r+ from bz and sicking.

Comment 89

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_4_e10s.patch

Carrying over r+ from jduell.

Comment 90

[Christoph Kerschbaumer [:ckerschb]]

Attachment: trig_5_consumers.patch

Carrying over r+ from bz.

Comment 91

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63a25e5073d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5043803c8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b292b91c6755
https://hg.mozilla.org/integration/mozilla-inbound/rev/b746586983f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9da209fe2f2

Comment 92

[Tanvi Vyas[:tanvi]]

Comment on attachment 8523034 [details] [diff] [review]
trig_3_callsites.patch

Since this bug has already landed, perhaps these comments can be fixed in a followup bug.

>Bug 1083422 - Add triggering Principal to nsILoadInfo - update callsites (r=bz,sicking)
>
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>     if (!isSrcdoc) {
>       nsCOMPtr<nsILoadInfo> loadInfo =
>-        new mozilla::LoadInfo(requestingPrincipal,
>-                              requestingNode,
>-                              securityFlags,
>-                              aContentPolicyType,
>-                              aBaseURI);
>+        new LoadInfo(requestingNode ?
>+                       requestingNode->NodePrincipal() : triggeringPrincipal.get(),
>+                     triggeringPrincipal,
>+                     requestingNode,
>+                     securityFlags,
>+                     aContentPolicyType,
>+                     aBaseURI);

We shouldn't be using the triggeringPrincipal as the requestingPrincipal here.  Instead we should do what we have done in other cases - if there is no requestingNode, assert that triggeringPrincipal is systemPrincipal and then pass systemPrincipal as the loadingPrincipal to NS_NewChannel.

>         rv = NS_NewChannelInternal(getter_AddRefs(channel),
>                                    aURI,
>                                    loadInfo,
>                                    nullptr,   // loadGroup
>                                    static_cast<nsIInterfaceRequestor*>(this),
>                                    loadFlags);
> 
>         if (NS_FAILED(rv)) {
>@@ -10330,30 +10332,34 @@ nsDocShell::DoURILoad(nsIURI * aURI,
> 
>         if (isViewSource) {
>             nsViewSourceHandler *vsh = nsViewSourceHandler::GetInstance();
>             NS_ENSURE_TRUE(vsh,NS_ERROR_FAILURE);
> 
>             rv = vsh->NewSrcdocChannel(aURI, aSrcdoc, getter_AddRefs(channel));
>             NS_ENSURE_SUCCESS(rv, rv);
>             nsCOMPtr<nsILoadInfo> loadInfo =
>-              new LoadInfo(requestingPrincipal,
>+              new LoadInfo(requestingNode ?
>+                             requestingNode->NodePrincipal() : triggeringPrincipal.get(),
>+                           triggeringPrincipal,
>                            requestingNode,
>                            securityFlags,
>                            aContentPolicyType,
>                            aBaseURI);
Same here.
>             channel->SetLoadInfo(loadInfo);
>         }
>         else {
>             rv = NS_NewInputStreamChannelInternal(getter_AddRefs(channel),
>                                                   aURI,
>                                                   aSrcdoc,
>                                                   NS_LITERAL_CSTRING("text/html"),
>                                                   requestingNode,
>-                                                  requestingPrincipal,
>+                                                  requestingNode ?
>+                                                    requestingNode->NodePrincipal() : triggeringPrincipal.get(),
>+                                                  triggeringPrincipal,
>                                                   securityFlags,
>                                                   aContentPolicyType,
>                                                   true,
>                                                   aBaseURI);
And here.

>             NS_ENSURE_SUCCESS(rv, rv);
>         }
>     }
> 
>diff --git a/dom/base/WebSocket.cpp b/dom/base/WebSocket.cpp
>--- a/dom/base/WebSocket.cpp
>+++ b/dom/base/WebSocket.cpp
>@@ -1465,17 +1465,19 @@ WebSocketImpl::InitializeConnection()
>   // was not set during channel creation.
>   nsCOMPtr<nsIDocument> doc = do_QueryReferent(mOriginDocument);
> 
>   // mOriginDocument has to be release on main-thread because WeakReferences
>   // are not thread-safe.
>   mOriginDocument = nullptr;
> 
>   nsCOMPtr<nsILoadInfo> loadInfo =
>-    new LoadInfo(mPrincipal,
>+    new LoadInfo(doc ?
>+                   doc->NodePrincipal() : mPrincipal.get(),
>+                 mPrincipal,
>                  doc,
>                  nsILoadInfo::SEC_NORMAL,
>                  nsIContentPolicy::TYPE_WEBSOCKET);
And here.  If there is no doc, assert that mPrincipal is system and pass it as the requestingPrincipal.

>   rv = wsChannel->SetLoadInfo(loadInfo);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (!mRequestedProtocolList.IsEmpty()) {
>     rv = wsChannel->SetProtocol(mRequestedProtocolList);


>diff --git a/dom/xbl/nsXBLService.cpp b/dom/xbl/nsXBLService.cpp
>--- a/dom/xbl/nsXBLService.cpp
>+++ b/dom/xbl/nsXBLService.cpp
>@@ -1068,28 +1068,37 @@ nsXBLService::FetchBindingDocument(nsICo
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Open channel
>   // Note: There are some cases where aOriginPrincipal and aBoundDocument are purposely
>   // set to null (to bypass security checks) when calling LoadBindingDocumentInfo() which calls
>   // FetchBindingDocument().  LoadInfo will end up with no principal or node in those cases,
>   // so we use systemPrincipal.  This achieves the same result of bypassing security checks,
>   // but it gives the wrong information to potential future consumers of loadInfo.
>-  nsCOMPtr<nsIPrincipal> requestingPrincipal = aOriginPrincipal ? aOriginPrincipal
>-                                                                : nsContentUtils::GetSystemPrincipal();
>   nsCOMPtr<nsIChannel> channel;
>-  // Note that we are calling NS_NewChannelInternal here with both a node and a principal.
>-  // This is because the principal and node could be different.
Add this comment back in

>-  rv = NS_NewChannelInternal(getter_AddRefs(channel),
>-                             aDocumentURI,
>-                             aBoundDocument,
>-                             requestingPrincipal,
>-                             nsILoadInfo::SEC_NORMAL,
>-                             nsIContentPolicy::TYPE_OTHER,
>-                             loadGroup);
>+
>+  if (aOriginPrincipal) {


>diff --git a/image/src/imgLoader.cpp b/image/src/imgLoader.cpp
>--- a/image/src/imgLoader.cpp
>+++ b/image/src/imgLoader.cpp
>@@ -658,48 +658,65 @@ static nsresult NewImageChannel(nsIChann

>+  else {
>+    // either we are loading something inside a document, in which case
>+    // we should always have a requestingNode, or we are loading something
>+    // outside a document, in which case the triggeringPrincipal
>+    // should always be the systemPrincipal.
>+    MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(triggeringPrincipal));
>+    rv = NS_NewChannel(aResult,
>+                       aURI,
>+                       triggeringPrincipal,
triggeringPrincipal, //aRequestingPrincipal 

(or something to indicate that we are passing triggeringPrincipal into the requestingPrincipal parameter)

>+                       securityFlags,
>+                       nsIContentPolicy::TYPE_IMAGE,
>+                       nullptr,   // loadGroup
>+                       callbacks,
>+                       aLoadFlags);
>+  }
 
>diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp
>--- a/layout/style/Loader.cpp
>+++ b/layout/style/Loader.cpp

>+    else {
>+      // either we are loading something inside a document, in which case
>+      // we should always have a requestingNode, or we are loading something
>+      // outside a document, in which case the triggeringPrincipal
>+      // should always be the systemPrincipal.
>+      MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(triggeringPrincipal));
>+      rv = NS_OpenURI(getter_AddRefs(stream),
>+                      aLoadData->mURI,
>+                      triggeringPrincipal,
triggeringPrincipal, //aRequestingPrincipal 

(or something to indicate that we are passing triggeringPrincipal into the requestingPrincipal parameter)

>+                      nsILoadInfo::SEC_NORMAL,
>+                      nsIContentPolicy::TYPE_OTHER,
>+                      nullptr,   // aLoadGroup
>+                      nullptr,   // aCallbacks
>+                      nsIRequest::LOAD_NORMAL,
>+                      nullptr,   // aIoService
>+                      getter_AddRefs(channel));
>+    }

>@@ -1555,30 +1574,48 @@ Loader::LoadSheet(SheetLoadData* aLoadDa
>+  else {
>+    // either we are loading something inside a document, in which case
>+    // we should always have a requestingNode, or we are loading something
>+    // outside a document, in which case the triggeringPrincipal
>+    // should always be the systemPrincipal.
>+    MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(triggeringPrincipal));
>+    rv = NS_NewChannel(getter_AddRefs(channel),
>+                       aLoadData->mURI,
>+                       triggeringPrincipal,
same here: triggeringPrincipal, //aRequestingPrincipal 
>+                       securityFlags,
>+                       nsIContentPolicy::TYPE_STYLESHEET,
>+                       loadGroup,
>+                       nullptr,   // aCallbacks
>+                       nsIChannel::LOAD_NORMAL |
>+                       nsIChannel::LOAD_CLASSIFY_URI);
>+  }

Comment 93

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

I'm personally much less sure that we can assert that in the docshell case. In the other cases we're reasonably confident that the only time when we're lacking a loading node is when the load is coming from the system rather than from a webpage. However the docshell code is so complex and crufty that *I* am much less certain that that's the case there.

Someone that knows the docshell code better should weigh in though. And of course, if we can improve the docshell code that's ideal.

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63a25e5073d3
https://hg.mozilla.org/mozilla-central/rev/ba5043803c8e
https://hg.mozilla.org/mozilla-central/rev/b292b91c6755
https://hg.mozilla.org/mozilla-central/rev/b746586983f4
https://hg.mozilla.org/mozilla-central/rev/b9da209fe2f2