From dd394d57a23a2ddcf2c10184f3ffab8400b03214 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Mon, 4 Oct 2021 13:24:15 +0000 Subject: [PATCH] Bug 1733558 - stop duplicating append redirect history entry logic everywhere, r=ckerschb,necko-reviewers,valentin Differential Revision: https://phabricator.services.mozilla.com/D127251 --- .../gtest/TestUnexpectedPrivilegedLoads.cpp | 9 +++-- netwerk/base/LoadInfo.cpp | 37 +++++++++++++++++-- netwerk/base/TRRLoadInfo.cpp | 2 +- netwerk/base/nsBaseChannel.cpp | 10 +---- netwerk/base/nsILoadInfo.idl | 5 +-- netwerk/ipc/DocumentLoadListener.cpp | 9 +---- netwerk/protocol/http/HttpBaseChannel.cpp | 36 +----------------- netwerk/protocol/http/HttpBaseChannel.h | 3 -- netwerk/protocol/http/HttpChannelChild.cpp | 12 +----- 9 files changed, 45 insertions(+), 78 deletions(-) diff --git a/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp b/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp index 9498f736cfd0..271ba50a8201 100644 --- a/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp +++ b/dom/security/test/gtest/TestUnexpectedPrivilegedLoads.cpp @@ -26,7 +26,6 @@ #include "nsILoadInfo.h" #include "nsNetUtil.h" #include "nsStringFwd.h" -#include "mozilla/nsRedirectHistoryEntry.h" using namespace mozilla; using namespace TelemetryTestHelpers; @@ -188,10 +187,12 @@ TEST_F(TelemetryTestFixture, UnexpectedPrivilegedLoadsTelemetryTest) { "https://www.analytics.example/analytics.js"_ns); nsCOMPtr redirPrincipal = BasePrincipal::CreateContentPrincipal(redirUri, OriginAttributes()); - nsCOMPtr entry = - new net::nsRedirectHistoryEntry(redirPrincipal, nullptr, ""_ns); + nsCOMPtr 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 diff --git a/netwerk/base/LoadInfo.cpp b/netwerk/base/LoadInfo.cpp index e9b0d96363f3..4abc375f6ddd 100644 --- a/netwerk/base/LoadInfo.cpp +++ b/netwerk/base/LoadInfo.cpp @@ -30,6 +30,8 @@ #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" @@ -1326,14 +1328,41 @@ LoadInfo::GetInitialSecurityCheckDone(bool* aResult) { } 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 uriPrincipal; + nsIScriptSecurityManager* sm = nsContentUtils::GetSecurityManager(); + sm->GetChannelURIPrincipal(aChannel, getter_AddRefs(uriPrincipal)); + + nsCOMPtr referrer; + nsCString remoteAddress; + + nsCOMPtr httpChannel(do_QueryInterface(aChannel)); + if (httpChannel) { + nsCOMPtr 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 intChannel(do_QueryInterface(aChannel)); + if (intChannel) { + Unused << intChannel->GetRemoteAddress(remoteAddress); + } + + nsCOMPtr entry = + new nsRedirectHistoryEntry(uriPrincipal, referrer, remoteAddress); + + mRedirectChainIncludingInternalRedirects.AppendElement(entry); if (!aIsInternalRedirect) { - mRedirectChain.AppendElement(aEntry); + mRedirectChain.AppendElement(entry); } return NS_OK; } diff --git a/netwerk/base/TRRLoadInfo.cpp b/netwerk/base/TRRLoadInfo.cpp index d262ca6f5786..98f1ec085c58 100644 --- a/netwerk/base/TRRLoadInfo.cpp +++ b/netwerk/base/TRRLoadInfo.cpp @@ -391,7 +391,7 @@ TRRLoadInfo::GetInitialSecurityCheckDone(bool* aResult) { } NS_IMETHODIMP -TRRLoadInfo::AppendRedirectHistoryEntry(nsIRedirectHistoryEntry* aEntry, +TRRLoadInfo::AppendRedirectHistoryEntry(nsIChannel* aChannelToDeriveFrom, bool aIsInternalRedirect) { return NS_ERROR_NOT_IMPLEMENTED; } diff --git a/netwerk/base/nsBaseChannel.cpp b/netwerk/base/nsBaseChannel.cpp index b37298be7732..abbb050c4e50 100644 --- a/netwerk/base/nsBaseChannel.cpp +++ b/netwerk/base/nsBaseChannel.cpp @@ -78,19 +78,11 @@ nsresult nsBaseChannel::Redirect(nsIChannel* newChannel, uint32_t redirectFlags, static_cast(mLoadInfo.get()) ->CloneWithNewSecFlags(secFlags); - nsCOMPtr 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 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. diff --git a/netwerk/base/nsILoadInfo.idl b/netwerk/base/nsILoadInfo.idl index bc1aea373e83..78d7833668c5 100644 --- a/netwerk/base/nsILoadInfo.idl +++ b/netwerk/base/nsILoadInfo.idl @@ -857,12 +857,11 @@ interface nsILoadInfo : nsISupports * 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); /** diff --git a/netwerk/ipc/DocumentLoadListener.cpp b/netwerk/ipc/DocumentLoadListener.cpp index 50b9def515e7..5ff3192e954d 100644 --- a/netwerk/ipc/DocumentLoadListener.cpp +++ b/netwerk/ipc/DocumentLoadListener.cpp @@ -1399,14 +1399,7 @@ void DocumentLoadListener::SerializeRedirectData( redirectLoadInfo = static_cast(channelLoadInfo.get())->Clone(); - nsCOMPtr uriPrincipal; - nsIScriptSecurityManager* sm = nsContentUtils::GetSecurityManager(); - sm->GetChannelURIPrincipal(mChannel, getter_AddRefs(uriPrincipal)); - - nsCOMPtr entry = - new nsRedirectHistoryEntry(uriPrincipal, nullptr, ""_ns); - - redirectLoadInfo->AppendRedirectHistoryEntry(entry, true); + redirectLoadInfo->AppendRedirectHistoryEntry(mChannel, true); } const Maybe& reservedClientInfo = diff --git a/netwerk/protocol/http/HttpBaseChannel.cpp b/netwerk/protocol/http/HttpBaseChannel.cpp index dc32f25e0d1f..dcc44ef7e027 100644 --- a/netwerk/protocol/http/HttpBaseChannel.cpp +++ b/netwerk/protocol/http/HttpBaseChannel.cpp @@ -3719,30 +3719,6 @@ 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); } @@ -3939,17 +3915,7 @@ already_AddRefed HttpBaseChannel::CloneLoadInfoForRedirect( newLoadInfo->ResetSandboxedNullPrincipalID(); } - nsCString remoteAddress; - Unused << GetRemoteAddress(remoteAddress); - nsCOMPtr referrer; - if (mReferrerInfo) { - referrer = mReferrerInfo->GetComputedReferrer(); - } - - nsCOMPtr entry = - new nsRedirectHistoryEntry(GetURIPrincipal(), referrer, remoteAddress); - - newLoadInfo->AppendRedirectHistoryEntry(entry, isInternalRedirect); + newLoadInfo->AppendRedirectHistoryEntry(this, isInternalRedirect); return newLoadInfo.forget(); } diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h index 5495599ed27d..152cdb519720 100644 --- a/netwerk/protocol/http/HttpBaseChannel.h +++ b/netwerk/protocol/http/HttpBaseChannel.h @@ -575,9 +575,6 @@ class HttpBaseChannel : public nsHashPropertyBag, // 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 diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp index 249a5330d2bf..90dc24eeb304 100644 --- a/netwerk/protocol/http/HttpChannelChild.cpp +++ b/netwerk/protocol/http/HttpChannelChild.cpp @@ -1537,17 +1537,7 @@ void HttpChannelChild::CleanupRedirectingChannel(nsresult rv) { if (mLoadGroup) mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED); if (NS_SUCCEEDED(rv)) { - nsCString remoteAddress; - Unused << GetRemoteAddress(remoteAddress); - nsCOMPtr referrer; - if (mReferrerInfo) { - referrer = mReferrerInfo->GetComputedReferrer(); - } - - nsCOMPtr entry = - new nsRedirectHistoryEntry(GetURIPrincipal(), referrer, remoteAddress); - - mLoadInfo->AppendRedirectHistoryEntry(entry, false); + mLoadInfo->AppendRedirectHistoryEntry(this, false); } else { NS_WARNING("CompleteRedirectSetup failed, HttpChannelChild already open?"); }