Bug 1733558 - stop duplicating append redirect history entry logic everywhere, r=ckerschb,necko-reviewers,valentin
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 04 Oct 2021 13:24:15 +0000
changeset 594462 c04e9ee53e5383f79086e0c94f0c2e7ee9083ad1
parent 594461 0c8983f9c20e68dea55edaed28281a8c6c47220e
child 594463 1364234d0a3766886eb37b00de2baa1cf230e6e3
push id150791
push usergijskruitbosch@gmail.com
push dateMon, 04 Oct 2021 13:26:37 +0000
treeherderautoland@c04e9ee53e53 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersckerschb, necko-reviewers, valentin
bugs1733558
milestone94.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 1733558 - stop duplicating append redirect history entry logic everywhere, r=ckerschb,necko-reviewers,valentin Differential Revision: https://phabricator.services.mozilla.com/D127251
dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp
netwerk/base/LoadInfo.cpp
netwerk/base/TRRLoadInfo.cpp
netwerk/base/nsBaseChannel.cpp
netwerk/base/nsILoadInfo.idl
netwerk/ipc/DocumentLoadListener.cpp
netwerk/protocol/http/HttpBaseChannel.cpp
netwerk/protocol/http/HttpBaseChannel.h
netwerk/protocol/http/HttpChannelChild.cpp
--- a/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp
+++ b/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp
@@ -21,17 +21,16 @@
 #include <stdlib.h>
 #include "nsContentSecurityManager.h"
 #include "nsContentSecurityUtils.h"
 #include "nsContentUtils.h"
 #include "nsIContentPolicy.h"
 #include "nsILoadInfo.h"
 #include "nsNetUtil.h"
 #include "nsStringFwd.h"
-#include "mozilla/nsRedirectHistoryEntry.h"
 
 using namespace mozilla;
 using namespace TelemetryTestHelpers;
 
 extern Atomic<bool, mozilla::Relaxed> sJSHacksChecked;
 extern Atomic<bool, mozilla::Relaxed> sJSHacksPresent;
 extern Atomic<bool, mozilla::Relaxed> sCSSHacksChecked;
 extern Atomic<bool, mozilla::Relaxed> sCSSHacksPresent;
@@ -183,20 +182,22 @@ TEST_F(TelemetryTestFixture, UnexpectedP
     if (currentTest.urlstring.EqualsASCII(
             "moz-extension://abcdefab-1234-4321-0000-abcdefabcdef/js/"
             "assets.js")) {
       nsCOMPtr<nsIURI> redirUri;
       NS_NewURI(getter_AddRefs(redirUri),
                 "https://www.analytics.example/analytics.js"_ns);
       nsCOMPtr<nsIPrincipal> redirPrincipal =
           BasePrincipal::CreateContentPrincipal(redirUri, OriginAttributes());
-      nsCOMPtr<nsIRedirectHistoryEntry> entry =
-          new net::nsRedirectHistoryEntry(redirPrincipal, nullptr, ""_ns);
+      nsCOMPtr<nsIChannel> redirectChannel;
+      Unused << service->NewChannelFromURI(redirUri, nullptr, redirPrincipal,
+                                           nullptr, 0, currentTest.contentType,
+                                           getter_AddRefs(redirectChannel));
 
-      mockLoadInfo->AppendRedirectHistoryEntry(entry, false);
+      mockLoadInfo->AppendRedirectHistoryEntry(redirectChannel, false);
     }
 
     // this will record the event
     nsContentSecurityManager::MeasureUnexpectedPrivilegedLoads(
         mockLoadInfo, uri, currentTest.remoteType);
 
     // let's inspect the recorded events
 
--- a/netwerk/base/LoadInfo.cpp
+++ b/netwerk/base/LoadInfo.cpp
@@ -25,16 +25,18 @@
 #include "mozilla/StaticPrefs_security.h"
 #include "mozIThirdPartyUtil.h"
 #include "ThirdPartyUtil.h"
 #include "nsFrameLoader.h"
 #include "nsFrameLoaderOwner.h"
 #include "nsIContentSecurityPolicy.h"
 #include "nsIDocShell.h"
 #include "mozilla/dom/Document.h"
+#include "nsIHttpChannel.h"
+#include "nsIHttpChannelInternal.h"
 #include "nsIInterfaceRequestorUtils.h"
 #include "nsIScriptElement.h"
 #include "nsISupportsImpl.h"
 #include "nsISupportsUtils.h"
 #include "nsIXPConnect.h"
 #include "nsDocShell.h"
 #include "nsGlobalWindow.h"
 #include "nsMixedContentBlocker.h"
@@ -1321,24 +1323,51 @@ LoadInfo::SetInitialSecurityCheckDone(bo
 
 NS_IMETHODIMP
 LoadInfo::GetInitialSecurityCheckDone(bool* aResult) {
   *aResult = mInitialSecurityCheckDone;
   return NS_OK;
 }
 
 NS_IMETHODIMP
-LoadInfo::AppendRedirectHistoryEntry(nsIRedirectHistoryEntry* aEntry,
+LoadInfo::AppendRedirectHistoryEntry(nsIChannel* aChannel,
                                      bool aIsInternalRedirect) {
-  NS_ENSURE_ARG(aEntry);
+  NS_ENSURE_ARG(aChannel);
   MOZ_ASSERT(NS_IsMainThread());
 
-  mRedirectChainIncludingInternalRedirects.AppendElement(aEntry);
+  nsCOMPtr<nsIPrincipal> uriPrincipal;
+  nsIScriptSecurityManager* sm = nsContentUtils::GetSecurityManager();
+  sm->GetChannelURIPrincipal(aChannel, getter_AddRefs(uriPrincipal));
+
+  nsCOMPtr<nsIURI> referrer;
+  nsCString remoteAddress;
+
+  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel));
+  if (httpChannel) {
+    nsCOMPtr<nsIReferrerInfo> referrerInfo;
+    Unused << httpChannel->GetReferrerInfo(getter_AddRefs(referrerInfo));
+    if (referrerInfo) {
+      referrer = referrerInfo->GetComputedReferrer();
+    }
+  }
+
+  // ClassifierDummyChannel implements this, but not nsIHttpChannel,
+  // whereas NullHttpChannel implements nsIHttpChannel but not this, so
+  // we can't make assumptions by nesting these QIs.
+  nsCOMPtr<nsIHttpChannelInternal> intChannel(do_QueryInterface(aChannel));
+  if (intChannel) {
+    Unused << intChannel->GetRemoteAddress(remoteAddress);
+  }
+
+  nsCOMPtr<nsIRedirectHistoryEntry> entry =
+      new nsRedirectHistoryEntry(uriPrincipal, referrer, remoteAddress);
+
+  mRedirectChainIncludingInternalRedirects.AppendElement(entry);
   if (!aIsInternalRedirect) {
-    mRedirectChain.AppendElement(aEntry);
+    mRedirectChain.AppendElement(entry);
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 LoadInfo::GetRedirects(JSContext* aCx, JS::MutableHandle<JS::Value> aRedirects,
                        const RedirectHistoryArray& aArray) {
   JS::Rooted<JSObject*> redirects(aCx,
--- a/netwerk/base/TRRLoadInfo.cpp
+++ b/netwerk/base/TRRLoadInfo.cpp
@@ -386,17 +386,17 @@ TRRLoadInfo::SetInitialSecurityCheckDone
 }
 
 NS_IMETHODIMP
 TRRLoadInfo::GetInitialSecurityCheckDone(bool* aResult) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
-TRRLoadInfo::AppendRedirectHistoryEntry(nsIRedirectHistoryEntry* aEntry,
+TRRLoadInfo::AppendRedirectHistoryEntry(nsIChannel* aChannelToDeriveFrom,
                                         bool aIsInternalRedirect) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 TRRLoadInfo::GetRedirectChainIncludingInternalRedirects(
     JSContext* aCx, JS::MutableHandle<JS::Value> aChain) {
   return NS_ERROR_NOT_IMPLEMENTED;
--- a/netwerk/base/nsBaseChannel.cpp
+++ b/netwerk/base/nsBaseChannel.cpp
@@ -73,29 +73,21 @@ nsresult nsBaseChannel::Redirect(nsIChan
   // make a copy of the loadinfo, append to the redirectchain
   // and set it on the new channel
   nsSecurityFlags secFlags =
       mLoadInfo->GetSecurityFlags() & ~nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
   nsCOMPtr<nsILoadInfo> newLoadInfo =
       static_cast<net::LoadInfo*>(mLoadInfo.get())
           ->CloneWithNewSecFlags(secFlags);
 
-  nsCOMPtr<nsIPrincipal> uriPrincipal;
-  nsIScriptSecurityManager* sm = nsContentUtils::GetSecurityManager();
-  sm->GetChannelURIPrincipal(this, getter_AddRefs(uriPrincipal));
   bool isInternalRedirect =
       (redirectFlags & (nsIChannelEventSink::REDIRECT_INTERNAL |
                         nsIChannelEventSink::REDIRECT_STS_UPGRADE));
 
-  // nsBaseChannel hst no thing to do with HttpBaseChannel, we would not care
-  // about referrer and remote address in this case
-  nsCOMPtr<nsIRedirectHistoryEntry> entry =
-      new net::nsRedirectHistoryEntry(uriPrincipal, nullptr, ""_ns);
-
-  newLoadInfo->AppendRedirectHistoryEntry(entry, isInternalRedirect);
+  newLoadInfo->AppendRedirectHistoryEntry(this, isInternalRedirect);
 
   // Ensure the channel's loadInfo's result principal URI so that it's
   // either non-null or updated to the redirect target URI.
   // We must do this because in case the loadInfo's result principal URI
   // is null, it would be taken from OriginalURI of the channel.  But we
   // overwrite it with the whole redirect chain first URI before opening
   // the target channel, hence the information would be lost.
   // If the protocol handler that created the channel wants to use
--- a/netwerk/base/nsILoadInfo.idl
+++ b/netwerk/base/nsILoadInfo.idl
@@ -852,22 +852,21 @@ interface nsILoadInfo : nsISupports
   [noscript, infallible] readonly attribute boolean serviceWorkerTaintingSynthesized;
 
   /**
    * Whenever a channel gets redirected, append the redirect history entry of
    * the channel which contains principal referrer and remote address [before
    * the channels got redirected] to the loadinfo, so that at every point this
    * array provides us information about all the redirects this channel went
    * through.
-   * @param entry, the nsIRedirectHistoryEntry before the channel
-   *         got redirected.
+   * @param channelToDeriveFrom the channel being redirected
    * @param aIsInternalRedirect should be true if the channel is going
    *        through an internal redirect, otherwise false.
    */
-  void appendRedirectHistoryEntry(in nsIRedirectHistoryEntry entry,
+  void appendRedirectHistoryEntry(in nsIChannel channelToDeriveFrom,
                                   in boolean isInternalRedirect);
 
   /**
    * An array of nsIRedirectHistoryEntry which stores redirects associated
    * with this channel. This array is filled whether or not the channel has
    * ever been opened. The last element of the array is associated with the
    * most recent redirect. Please note, that this array *includes* internal
    * redirects.
--- a/netwerk/ipc/DocumentLoadListener.cpp
+++ b/netwerk/ipc/DocumentLoadListener.cpp
@@ -1394,24 +1394,17 @@ void DocumentLoadListener::SerializeRedi
     // docshell/test/mochitest/test_forceinheritprincipal_overrule_owner.html
     if (principalToInherit) {
       redirectLoadInfo->SetPrincipalToInherit(principalToInherit);
     }
   } else {
     redirectLoadInfo =
         static_cast<mozilla::net::LoadInfo*>(channelLoadInfo.get())->Clone();
 
-    nsCOMPtr<nsIPrincipal> uriPrincipal;
-    nsIScriptSecurityManager* sm = nsContentUtils::GetSecurityManager();
-    sm->GetChannelURIPrincipal(mChannel, getter_AddRefs(uriPrincipal));
-
-    nsCOMPtr<nsIRedirectHistoryEntry> entry =
-        new nsRedirectHistoryEntry(uriPrincipal, nullptr, ""_ns);
-
-    redirectLoadInfo->AppendRedirectHistoryEntry(entry, true);
+    redirectLoadInfo->AppendRedirectHistoryEntry(mChannel, true);
   }
 
   const Maybe<ClientInfo>& reservedClientInfo =
       channelLoadInfo->GetReservedClientInfo();
   if (reservedClientInfo) {
     redirectLoadInfo->SetReservedClientInfo(*reservedClientInfo);
   }
 
--- a/netwerk/protocol/http/HttpBaseChannel.cpp
+++ b/netwerk/protocol/http/HttpBaseChannel.cpp
@@ -3714,40 +3714,16 @@ void HttpBaseChannel::StealConsoleReport
     nsTArray<net::ConsoleReportCollected>& aReports) {
   mReportCollector->StealConsoleReports(aReports);
 }
 
 void HttpBaseChannel::ClearConsoleReports() {
   mReportCollector->ClearConsoleReports();
 }
 
-nsIPrincipal* HttpBaseChannel::GetURIPrincipal() {
-  if (mPrincipal) {
-    return mPrincipal;
-  }
-
-  nsIScriptSecurityManager* securityManager =
-      nsContentUtils::GetSecurityManager();
-
-  if (!securityManager) {
-    LOG(("HttpBaseChannel::GetURIPrincipal: No security manager [this=%p]",
-         this));
-    return nullptr;
-  }
-
-  securityManager->GetChannelURIPrincipal(this, getter_AddRefs(mPrincipal));
-  if (!mPrincipal) {
-    LOG(("HttpBaseChannel::GetURIPrincipal: No channel principal [this=%p]",
-         this));
-    return nullptr;
-  }
-
-  return mPrincipal;
-}
-
 bool HttpBaseChannel::IsNavigation() {
   return LoadForceMainDocumentChannel() || (mLoadFlags & LOAD_DOCUMENT_URI);
 }
 
 bool HttpBaseChannel::BypassServiceWorker() const {
   return mLoadFlags & LOAD_BYPASS_SERVICE_WORKER;
 }
 
@@ -3934,27 +3910,17 @@ already_AddRefed<nsILoadInfo> HttpBaseCh
     // allowed to go ahead and not trip infinite-loop protection
     // (see bug 1717314 for context).
     if (!aNewURI->SchemeIs("http") && !aNewURI->SchemeIs("https")) {
       newLoadInfo->SetLoadTriggeredFromExternal(false);
     }
     newLoadInfo->ResetSandboxedNullPrincipalID();
   }
 
-  nsCString remoteAddress;
-  Unused << GetRemoteAddress(remoteAddress);
-  nsCOMPtr<nsIURI> referrer;
-  if (mReferrerInfo) {
-    referrer = mReferrerInfo->GetComputedReferrer();
-  }
-
-  nsCOMPtr<nsIRedirectHistoryEntry> entry =
-      new nsRedirectHistoryEntry(GetURIPrincipal(), referrer, remoteAddress);
-
-  newLoadInfo->AppendRedirectHistoryEntry(entry, isInternalRedirect);
+  newLoadInfo->AppendRedirectHistoryEntry(this, isInternalRedirect);
 
   return newLoadInfo.forget();
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpChannel::nsITraceableChannel
 //-----------------------------------------------------------------------------
 
--- a/netwerk/protocol/http/HttpBaseChannel.h
+++ b/netwerk/protocol/http/HttpBaseChannel.h
@@ -570,19 +570,16 @@ class HttpBaseChannel : public nsHashPro
                                   NS_GET_TEMPLATE_IID(T),
                                   getter_AddRefs(aResult));
   }
 
   // Redirect tracking
   // Checks whether or not aURI and mOriginalURI share the same domain.
   virtual bool SameOriginWithOriginalUri(nsIURI* aURI);
 
-  // GetPrincipal Returns the channel's URI principal.
-  nsIPrincipal* GetURIPrincipal();
-
   [[nodiscard]] bool BypassServiceWorker() const;
 
   // Returns true if this channel should intercept the network request and
   // prepare for a possible synthesized response instead.
   bool ShouldIntercept(nsIURI* aURI = nullptr);
 
   // Callback on main thread when NS_AsyncCopy() is finished populating
   // the new mUploadStream.
--- a/netwerk/protocol/http/HttpChannelChild.cpp
+++ b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -1532,27 +1532,17 @@ void HttpChannelChild::Redirect3Complete
   CleanupRedirectingChannel(rv);
 }
 
 void HttpChannelChild::CleanupRedirectingChannel(nsresult rv) {
   // Redirecting to new channel: shut this down and init new channel
   if (mLoadGroup) mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED);
 
   if (NS_SUCCEEDED(rv)) {
-    nsCString remoteAddress;
-    Unused << GetRemoteAddress(remoteAddress);
-    nsCOMPtr<nsIURI> referrer;
-    if (mReferrerInfo) {
-      referrer = mReferrerInfo->GetComputedReferrer();
-    }
-
-    nsCOMPtr<nsIRedirectHistoryEntry> entry =
-        new nsRedirectHistoryEntry(GetURIPrincipal(), referrer, remoteAddress);
-
-    mLoadInfo->AppendRedirectHistoryEntry(entry, false);
+    mLoadInfo->AppendRedirectHistoryEntry(this, false);
   } else {
     NS_WARNING("CompleteRedirectSetup failed, HttpChannelChild already open?");
   }
 
   // Release ref to new channel.
   mRedirectChannelChild = nullptr;
 
   NotifyOrReleaseListeners(rv);