Bug 1303943 - Don't inherit principal for external loads and update documentation for principalToInherit. r=bz

docshell/base/nsDocShell.cpp

@@ -9738,6 +9738,20 @@ nsDocShell::InternalLoad(nsIURI* aURI,
                          nsIDocShell** aDocShell,
                          nsIRequest** aRequest)
 {
+  // In most cases both principals (aTriggeringPrincipal and aPrincipalToInherit)
+  // are both null or both non-null. For the exceptional cases let's make sure that:
+  // * if aTriggeringPrincipal is null then either aPrincipalToInherit is null or
+  //   it's a NullPrincipal
+  // * if aPrincipalToInherit is null then either aTriggeringPrincipal is null or
+  //   it's a NullPrincipal or INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set.
+  MOZ_ASSERT(aTriggeringPrincipal ||
+             (!aPrincipalToInherit ||
+              aPrincipalToInherit->GetIsNullPrincipal()));
+  MOZ_ASSERT(aPrincipalToInherit ||
+             (!aTriggeringPrincipal ||
+              aTriggeringPrincipal->GetIsNullPrincipal() ||
+              (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL)));
+
   nsresult rv = NS_OK;
   mOriginalUriString.Truncate();
 
@@ -9929,12 +9943,26 @@ nsDocShell::InternalLoad(nsIURI* aURI,
   {
     bool inherits;
     // One more twist: Don't inherit the principal for external loads.
-    if (aLoadType != LOAD_NORMAL_EXTERNAL && !principalToInherit &&
-        (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL) &&
+    if (!principalToInherit && 
         NS_SUCCEEDED(nsContentUtils::URIInheritsSecurityContext(aURI,
                                                                 &inherits)) &&
         inherits) {
-      principalToInherit = GetInheritedPrincipal(true);
+      if (aLoadType != LOAD_NORMAL_EXTERNAL && 
+          (aFlags & INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL)) {
+        principalToInherit = GetInheritedPrincipal(true);
+      }
+
+      // In case we don't have a principalToInherit and the TriggeringPrincipal
+      // either already is a SystemPrincipal or would fall back to become
+      // a SystemPrincipal within the loadInfo then we should explicitly set
+      // the principalToInherit to a freshly created NullPrincipal.
+      if (!principalToInherit && 
+          (nsContentUtils::IsSystemPrincipal(aTriggeringPrincipal) ||
+           (!aTriggeringPrincipal && !aReferrer))) {
+        // We're going to default to inheriting our system triggering principal, 
+        // more or less by accident.  This doesn't seem like a good idea.
+        principalToInherit = nsNullPrincipal::CreateWithInheritedAttributes(this);
+      }
     }
   }
 
@@ -12300,7 +12328,7 @@ nsDocShell::AddToSessionHistory(nsIURI* aURI, nsIChannel* aChannel,
             pAttrs.InheritFromNecko(nAttrs);
             principalToInherit = nsNullPrincipal::Create(pAttrs);
           }
-        } else if (loadInfo->GetForceInheritPrincipal()) {
+        } else {
           principalToInherit = loadInfo->PrincipalToInherit();
         }
       }

docshell/base/nsIDocShell.idl

@@ -132,8 +132,35 @@ interface nsIDocShell : nsIDocShellTreeItem
    *                               ignored.
    * @param aReferrer            - Referring URI
    * @param aReferrerPolicy      - Referrer policy
-   * @param aTriggeringPrincipal - Principal that initiated that load
-   * @param aPrincipalToInherit  - Principal to be inherited for that load
+   * @param aTriggeringPrincipal - Principal that initiated that load. If passing
+   *                               null for this argument, then internally a
+   *                               principal is created from aReferrer. If
+   *                               aReferrer is also null, then the
+   *                               triggeringPrincipal defaults to the
+   *                               SystemPrincipal. Please note that this is the
+   *                               principal that is used for security checks. If
+   *                               the argument aURI is provided by the web, then
+   *                               please pass an explicit triggeringPrincipal to
+   *                               avoid the fallback to SystemPrincipal and
+   *                               hence a potential security risk.
+   *                               If aTriggeringPrincipal is null then either
+   *                               aPrincipalToInherit is null or it's
+   *                               a NullPrincipal.
+   * @param aPrincipalToInherit  - Principal to be inherited for that load. If
+   *                               passing null for this argument, then internally
+   *                               the triggeringPrincipal is also used for the
+   *                               principalToInherit. There are cases where those
+   *                               two principals need to be different though.
+   *                               E.g. the system might initiate a load for
+   *                               'about:blank', hence SystemPrincipal is passed
+   *                               for aTriggeringPrincipal. But the principal to
+   *                               be inherited for that load should be a
+   *                               NullPrincipal and not the SystemPrincipal.
+   *                               In that case, please pass a non null
+   *                               principalToInherit.
+   *                               If aPrincipalToInherit is null then either
+   *                               aTriggeringPrincipal is null or
+   *                               INTERNAL_LOAD_FLAGS_INHERIT_PRINCIPAL is set.
    * @param aFlags               - Any of the load flags defined within above.
    * @param aStopActiveDoc       - Flag indicating whether loading the current
    *                               document should be stopped.