Bug 1407428: Hand out a const array reference for expanded principal whiteList. r?krizsa draft
authorKris Maglione <maglione.k@gmail.com>
Tue, 10 Oct 2017 15:00:16 -0700
changeset 678201 d2a5e0862f8c964fb5a3e46b50c2e9629b218699
parent 678200 3aad26ddfedf4f2765b121d0aa9032b55c01076c
child 735262 8aba88d839b022b4b3a87c32b5ec920773d150ef
push id83852
push usermaglione.k@gmail.com
push dateTue, 10 Oct 2017 22:01:49 +0000
reviewerskrizsa
bugs1407428
milestone58.0a1
Bug 1407428: Hand out a const array reference for expanded principal whiteList. r?krizsa The current API makes the life time and ownership of the result array unclear without careful reading. The result array is always owned by the principal, and its lifetime tied to the lifetime of the principal itself. Returning a const array reference makes this clear, and should prevent callers from accidentally modifying the returned array. MozReview-Commit-ID: 3f8mhynkKAj
caps/ExpandedPrincipal.cpp
caps/nsIPrincipal.idl
caps/nsScriptSecurityManager.cpp
dom/base/nsDocument.cpp
dom/xhr/XMLHttpRequestMainThread.cpp
extensions/cookie/nsPermissionManager.cpp
ipc/glue/BackgroundUtils.cpp
js/xpconnect/src/XPCJSContext.cpp
--- a/caps/ExpandedPrincipal.cpp
+++ b/caps/ExpandedPrincipal.cpp
@@ -95,25 +95,24 @@ ExpandedPrincipal::SetDomain(nsIURI* aDo
 }
 
 bool
 ExpandedPrincipal::SubsumesInternal(nsIPrincipal* aOther,
                                     BasePrincipal::DocumentDomainConsideration aConsideration)
 {
   // If aOther is an ExpandedPrincipal too, we break it down into its component
   // nsIPrincipals, and check subsumes on each one.
-  nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aOther);
-  if (expanded) {
-    nsTArray< nsCOMPtr<nsIPrincipal> >* otherList;
-    expanded->GetWhiteList(&otherList);
-    for (uint32_t i = 0; i < otherList->Length(); ++i){
+  if (Cast(aOther)->Is<ExpandedPrincipal>()) {
+    auto* expanded = Cast(aOther)->As<ExpandedPrincipal>();
+
+    for (auto& other : expanded->WhiteList()) {
       // Use SubsumesInternal rather than Subsumes here, since OriginAttribute
       // checks are only done between non-expanded sub-principals, and we don't
       // need to incur the extra virtual call overhead.
-      if (!SubsumesInternal((*otherList)[i], aConsideration)) {
+      if (!SubsumesInternal(other, aConsideration)) {
         return false;
       }
     }
     return true;
   }
 
   // We're dealing with a regular principal. One of our principals must subsume
   // it.
@@ -146,21 +145,20 @@ ExpandedPrincipal::GetHashValue(uint32_t
 
 NS_IMETHODIMP
 ExpandedPrincipal::GetURI(nsIURI** aURI)
 {
   *aURI = nullptr;
   return NS_OK;
 }
 
-NS_IMETHODIMP
-ExpandedPrincipal::GetWhiteList(nsTArray<nsCOMPtr<nsIPrincipal> >** aWhiteList)
+const nsTArray<nsCOMPtr<nsIPrincipal>>&
+ExpandedPrincipal::WhiteList()
 {
-  *aWhiteList = &mPrincipals;
-  return NS_OK;
+  return mPrincipals;
 }
 
 NS_IMETHODIMP
 ExpandedPrincipal::GetBaseDomain(nsACString& aBaseDomain)
 {
   return NS_ERROR_NOT_AVAILABLE;
 }
 
--- a/caps/nsIPrincipal.idl
+++ b/caps/nsIPrincipal.idl
@@ -37,17 +37,17 @@ class OriginAttributes;
 %}
 
 interface nsIURI;
 interface nsIContentSecurityPolicy;
 interface nsIDOMDocument;
 
 [ptr] native JSContext(JSContext);
 [ptr] native JSPrincipals(JSPrincipals);
-[ptr] native PrincipalArray(nsTArray<nsCOMPtr<nsIPrincipal> >);
+[ref] native PrincipalArray(const nsTArray<nsCOMPtr<nsIPrincipal>>);
 [ref] native const_OriginAttributes(const mozilla::OriginAttributes);
 
 [scriptable, builtinclass, uuid(f75f502d-79fd-48be-a079-e5a7b8f80c8b)]
 interface nsIPrincipal : nsISerializable
 {
     /**
      * Returns whether the other principal is equivalent to this principal.
      * Principals are considered equal if they are the same principal, or
@@ -335,10 +335,11 @@ interface nsIPrincipal : nsISerializable
 [uuid(f3e177Df-6a5e-489f-80a7-2dd1481471d8)]
 interface nsIExpandedPrincipal : nsISupports
 {
   /**
    * An array of principals that the expanded principal subsumes.
    * Note: this list is not reference counted, it is shared, so
    * should not be changed and should only be used ephemerally.
    */
-  [noscript] readonly attribute PrincipalArray whiteList;
+  [noscript, notxpcom, nostdcall]
+  PrincipalArray WhiteList();
 };
--- a/caps/nsScriptSecurityManager.cpp
+++ b/caps/nsScriptSecurityManager.cpp
@@ -15,16 +15,17 @@
 #include "nsIServiceManager.h"
 #include "nsIScriptObjectPrincipal.h"
 #include "nsIScriptContext.h"
 #include "nsIURL.h"
 #include "nsINestedURI.h"
 #include "nspr.h"
 #include "nsJSPrincipals.h"
 #include "mozilla/BasePrincipal.h"
+#include "ExpandedPrincipal.h"
 #include "SystemPrincipal.h"
 #include "NullPrincipal.h"
 #include "DomainPolicy.h"
 #include "nsString.h"
 #include "nsCRT.h"
 #include "nsCRTGlue.h"
 #include "nsDocShell.h"
 #include "nsError.h"
@@ -663,22 +664,21 @@ nsScriptSecurityManager::CheckLoadURIWit
     if (aPrincipal == mSystemPrincipal) {
         // Allow access
         return NS_OK;
     }
 
     nsCOMPtr<nsIURI> sourceURI;
     aPrincipal->GetURI(getter_AddRefs(sourceURI));
     if (!sourceURI) {
-        nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aPrincipal);
-        if (expanded) {
-            nsTArray< nsCOMPtr<nsIPrincipal> > *whiteList;
-            expanded->GetWhiteList(&whiteList);
-            for (uint32_t i = 0; i < whiteList->Length(); ++i) {
-                nsresult rv = CheckLoadURIWithPrincipal((*whiteList)[i],
+        auto* basePrin = BasePrincipal::Cast(aPrincipal);
+        if (basePrin->Is<ExpandedPrincipal>()) {
+            auto expanded = basePrin->As<ExpandedPrincipal>();
+            for (auto& prin : expanded->WhiteList()) {
+                nsresult rv = CheckLoadURIWithPrincipal(prin,
                                                         aTargetURI,
                                                         aFlags);
                 if (NS_SUCCEEDED(rv)) {
                     // Allow access if it succeeded with one of the white listed principals
                     return NS_OK;
                 }
             }
             // None of our whitelisted principals worked.
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -2489,21 +2489,19 @@ nsDocument::MaybeDowngradePrincipal(nsIP
 
   // We can't load a document with an expanded principal. If we're given one,
   // automatically downgrade it to the last principal it subsumes (which is the
   // extension principal, in the case of extension content scripts).
   auto* basePrin = BasePrincipal::Cast(aPrincipal);
   if (basePrin->Is<ExpandedPrincipal>()) {
     auto* expanded = basePrin->As<ExpandedPrincipal>();
 
-    nsTArray<nsCOMPtr<nsIPrincipal>>* whitelist;
-    MOZ_ALWAYS_SUCCEEDS(expanded->GetWhiteList(&whitelist));
-    MOZ_ASSERT(whitelist->Length() > 0);
-
-    return do_AddRef(whitelist->LastElement().get());
+    MOZ_ASSERT(expanded->WhiteList().Length() > 0);
+
+    return do_AddRef(expanded->WhiteList().LastElement().get());
   }
 
   if (!sChromeInContentPrefCached) {
     sChromeInContentPrefCached = true;
     Preferences::AddBoolVarCache(&sChromeInContentAllowed,
                                  kChromeInContentPref, false);
   }
   if (!sChromeInContentAllowed &&
--- a/dom/xhr/XMLHttpRequestMainThread.cpp
+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
@@ -2514,26 +2514,21 @@ XMLHttpRequestMainThread::CreateChannel(
   // Using the provided principal as the triggeringPrincipal is fine, since we
   // want to be able to access any of the origins that the principal has access
   // to during the security checks, but we don't want a document to inherit an
   // expanded principal, so in that case we need to select the principal in the
   // expanded principal's whitelist that can load our URL as principalToInherit.
   nsCOMPtr<nsIPrincipal> resultingDocumentPrincipal(mPrincipal);
   nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(mPrincipal);
   if (ep) {
-    nsTArray<nsCOMPtr<nsIPrincipal>>* whitelist = nullptr;
-    ep->GetWhiteList(&whitelist);
-    if (!whitelist) {
-      return NS_ERROR_FAILURE;
-    }
     MOZ_ASSERT(!(secFlags & nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS));
     bool dataInherits = (secFlags &
       (nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS |
        nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS)) != 0;
-    for (const auto& principal : *whitelist) {
+    for (const auto& principal : ep->WhiteList()) {
       if (NS_SUCCEEDED(principal->CheckMayLoad(mRequestURL, false, dataInherits))) {
         resultingDocumentPrincipal = principal;
         break;
       }
     }
   }
 
   nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
--- a/extensions/cookie/nsPermissionManager.cpp
+++ b/extensions/cookie/nsPermissionManager.cpp
@@ -39,16 +39,17 @@
 #include "mozilla/Telemetry.h"
 #include "nsIConsoleService.h"
 #include "nsINavHistoryService.h"
 #include "nsToolkitCompsCID.h"
 #include "nsIObserverService.h"
 #include "nsPrintfCString.h"
 #include "mozilla/AbstractThread.h"
 #include "ContentPrincipal.h"
+#include "ExpandedPrincipal.h"
 
 static nsPermissionManager *gPermissionManager = nullptr;
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 static bool
 IsChildProcess()
@@ -2228,26 +2229,23 @@ nsPermissionManager::CommonTestPermissio
     return NS_OK;
   }
 
   // Set the default.
   *aPermission = nsIPermissionManager::UNKNOWN_ACTION;
 
   // For expanded principals, we want to iterate over the whitelist and see
   // if the permission is granted for any of them.
-  nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aPrincipal);
-  if (ep) {
-    nsTArray<nsCOMPtr<nsIPrincipal>>* whitelist;
-    nsresult rv = ep->GetWhiteList(&whitelist);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    for (size_t i = 0; i < whitelist->Length(); ++i) {
+  auto* basePrin = BasePrincipal::Cast(aPrincipal);
+  if (basePrin->Is<ExpandedPrincipal>()) {
+    auto ep = basePrin->As<ExpandedPrincipal>();
+    for (auto& prin : ep->WhiteList()) {
       uint32_t perm;
-      rv = CommonTestPermission(whitelist->ElementAt(i), aType, &perm,
-                                aExactHostMatch, aIncludingSession);
+      nsresult rv = CommonTestPermission(prin, aType, &perm,
+                                         aExactHostMatch, aIncludingSession);
       NS_ENSURE_SUCCESS(rv, rv);
       if (perm == nsIPermissionManager::ALLOW_ACTION) {
         *aPermission = perm;
         return NS_OK;
       } else if (perm == nsIPermissionManager::PROMPT_ACTION) {
         // Store it, but keep going to see if we can do better.
         *aPermission = perm;
       }
--- a/ipc/glue/BackgroundUtils.cpp
+++ b/ipc/glue/BackgroundUtils.cpp
@@ -187,28 +187,25 @@ PrincipalToPrincipalInfo(nsIPrincipal* a
   }
 
   if (isSystemPrincipal) {
     *aPrincipalInfo = SystemPrincipalInfo();
     return NS_OK;
   }
 
   // might be an expanded principal
-  nsCOMPtr<nsIExpandedPrincipal> expanded =
-    do_QueryInterface(aPrincipal);
+  auto* basePrin = BasePrincipal::Cast(aPrincipal);
+  if (basePrin->Is<ExpandedPrincipal>()) {
+    auto* expanded = basePrin->As<ExpandedPrincipal>();
 
-  if (expanded) {
     nsTArray<PrincipalInfo> whitelistInfo;
     PrincipalInfo info;
 
-    nsTArray< nsCOMPtr<nsIPrincipal> >* whitelist;
-    MOZ_ALWAYS_SUCCEEDS(expanded->GetWhiteList(&whitelist));
-
-    for (uint32_t i = 0; i < whitelist->Length(); i++) {
-      rv = PrincipalToPrincipalInfo((*whitelist)[i], &info);
+    for (auto& prin : expanded->WhiteList()) {
+      rv = PrincipalToPrincipalInfo(prin, &info);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
       // append that spec to the whitelist
       whitelistInfo.AppendElement(info);
     }
 
     *aPrincipalInfo =
--- a/js/xpconnect/src/XPCJSContext.cpp
+++ b/js/xpconnect/src/XPCJSContext.cpp
@@ -578,19 +578,17 @@ static bool
 IsWebExtensionContentScript(BasePrincipal* principal, nsAString& addonId)
 {
     if (!principal->Is<ExpandedPrincipal>()) {
         return false;
     }
 
     auto expanded = principal->As<ExpandedPrincipal>();
 
-    nsTArray<nsCOMPtr<nsIPrincipal>>* principals;
-    expanded->GetWhiteList(&principals);
-    for (auto prin : *principals) {
+    for (auto& prin : expanded->WhiteList()) {
         if (IsWebExtensionPrincipal(prin, addonId)) {
             return true;
         }
     }
 
     return false;
 }