Bug 1407428: Hand out a const array reference for expanded principal whiteList. r=krizsa
authorKris Maglione <maglione.k@gmail.com>
Tue, 10 Oct 2017 15:00:16 -0700
changeset 386320 28a28f017a5f8be72f91b3efb4e820abd16b949c
parent 386319 4c3ce57ff29ce41a3bcbf452d2972ab97bb627f4
child 386321 bfdd78172197790aae1fe8b307602e1a92f27072
push id96214
push usermaglione.k@gmail.com
push dateMon, 16 Oct 2017 00:19:56 +0000
treeherdermozilla-inbound@28a28f017a5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskrizsa
bugs1407428
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
@@ -2494,26 +2494,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 && 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;
 }