From e23dca5e882e904cdad782a716657c0ae4f31e07 Mon Sep 17 00:00:00 2001 From: Sean Feng Date: Fri, 1 Oct 2021 16:58:36 +0000 Subject: [PATCH] Bug 1733503 - Don't expose redirectStart and redirectEnd when there's a cross-origin redirect for PerformanceNavigationTiming r=smaug The actual text for hiding these information is actually missing at the moment. This is the pending PR https://github.com/whatwg/html/pull/7105 which should fix it. In addition to this PR, the obsolete (obsolete, but still accurate) definition for `redirectStart` and `redirectEnd` has specified that when there's a cross-origin redirect, these timings should not be exposed. Obsolete definition: https://w3c.github.io/navigation-timing/#dom-performancetiming-redirectstart Differential Revision: https://phabricator.services.mozilla.com/D127200 --- .../PerformanceNavigationTiming.cpp | 12 ++++++ dom/performance/PerformanceNavigationTiming.h | 6 +++ dom/performance/PerformanceResourceTiming.cpp | 5 ++- dom/performance/PerformanceResourceTiming.h | 37 ++++++++++++++----- dom/performance/PerformanceTiming.cpp | 16 +++++--- dom/performance/PerformanceTiming.h | 17 +++++---- ...ain_xserver_final_original_origin.html.ini | 4 -- 7 files changed, 67 insertions(+), 30 deletions(-) delete mode 100644 testing/web-platform/meta/navigation-timing/nav2_test_redirect_chain_xserver_final_original_origin.html.ini diff --git a/dom/performance/PerformanceNavigationTiming.cpp b/dom/performance/PerformanceNavigationTiming.cpp index 328e39b511ebf..0bd774309836d 100644 --- a/dom/performance/PerformanceNavigationTiming.cpp +++ b/dom/performance/PerformanceNavigationTiming.cpp @@ -134,6 +134,18 @@ uint16_t PerformanceNavigationTiming::RedirectCount() const { return mTimingData->GetRedirectCount(); } +DOMHighResTimeStamp PerformanceNavigationTiming::RedirectStart( + Maybe& aSubjectPrincipal) const { + return PerformanceResourceTiming::RedirectStart( + aSubjectPrincipal, true /* aEnsureSameOriginAndIgnoreTAO */); +} + +DOMHighResTimeStamp PerformanceNavigationTiming::RedirectEnd( + Maybe& aSubjectPrincipal) const { + return PerformanceResourceTiming::RedirectEnd( + aSubjectPrincipal, true /* aEnsureSameOriginAndIgnoreTAO */); +} + void PerformanceNavigationTiming::UpdatePropertiesFromHttpChannel( nsIHttpChannel* aHttpChannel, nsITimedChannel* aChannel) { mTimingData->SetPropertiesFromHttpChannel(aHttpChannel, aChannel); diff --git a/dom/performance/PerformanceNavigationTiming.h b/dom/performance/PerformanceNavigationTiming.h index f76d989535629..d41581981e26d 100644 --- a/dom/performance/PerformanceNavigationTiming.h +++ b/dom/performance/PerformanceNavigationTiming.h @@ -67,6 +67,12 @@ class PerformanceNavigationTiming final : public PerformanceResourceTiming { DOMHighResTimeStamp DomComplete() const; DOMHighResTimeStamp LoadEventStart() const; DOMHighResTimeStamp LoadEventEnd() const; + + DOMHighResTimeStamp RedirectStart( + Maybe& aSubjectPrincipal) const override; + DOMHighResTimeStamp RedirectEnd( + Maybe& aSubjectPrincipal) const override; + NavigationType Type() const; uint16_t RedirectCount() const; diff --git a/dom/performance/PerformanceResourceTiming.cpp b/dom/performance/PerformanceResourceTiming.cpp index 8f81eb56934eb..a899c4b21a33d 100644 --- a/dom/performance/PerformanceResourceTiming.cpp +++ b/dom/performance/PerformanceResourceTiming.cpp @@ -114,8 +114,9 @@ bool PerformanceResourceTiming::TimingAllowedForCaller( } bool PerformanceResourceTiming::ReportRedirectForCaller( - Maybe& aCaller) const { - if (mTimingData->ShouldReportCrossOriginRedirect()) { + Maybe& aCaller, bool aEnsureSameOriginAndIgnoreTAO) const { + if (mTimingData->ShouldReportCrossOriginRedirect( + aEnsureSameOriginAndIgnoreTAO)) { return true; } diff --git a/dom/performance/PerformanceResourceTiming.h b/dom/performance/PerformanceResourceTiming.h index 6e9b0689c2179..ad9d0e4943634 100644 --- a/dom/performance/PerformanceResourceTiming.h +++ b/dom/performance/PerformanceResourceTiming.h @@ -60,24 +60,40 @@ class PerformanceResourceTiming : public PerformanceEntry { return mTimingData->FetchStartHighRes(mPerformance); } - DOMHighResTimeStamp RedirectStart( - Maybe& aSubjectPrincipal) const { - // We have to check if all the redirect URIs had the same origin (since - // there is no check in RedirectStartHighRes()) - return ReportRedirectForCaller(aSubjectPrincipal) + DOMHighResTimeStamp RedirectStart(Maybe& aSubjectPrincipal, + bool aEnsureSameOriginAndIgnoreTAO) const { + // We have to check if all the redirect URIs whether had the same origin or + // different origins with TAO headers set (since there is no check in + // RedirectStartHighRes()) + return ReportRedirectForCaller(aSubjectPrincipal, + aEnsureSameOriginAndIgnoreTAO) ? mTimingData->RedirectStartHighRes(mPerformance) : 0; } - DOMHighResTimeStamp RedirectEnd( + virtual DOMHighResTimeStamp RedirectStart( Maybe& aSubjectPrincipal) const { - // We have to check if all the redirect URIs had the same origin (since - // there is no check in RedirectEndHighRes()) - return ReportRedirectForCaller(aSubjectPrincipal) + return RedirectStart(aSubjectPrincipal, + false /* aEnsureSameOriginAndIgnoreTAO */); + } + + DOMHighResTimeStamp RedirectEnd(Maybe& aSubjectPrincipal, + bool aEnsureSameOriginAndIgnoreTAO) const { + // We have to check if all the redirect URIs whether had the same origin or + // different origins with TAO headers set (since there is no check in + // RedirectEndHighRes()) + return ReportRedirectForCaller(aSubjectPrincipal, + aEnsureSameOriginAndIgnoreTAO) ? mTimingData->RedirectEndHighRes(mPerformance) : 0; } + virtual DOMHighResTimeStamp RedirectEnd( + Maybe& aSubjectPrincipal) const { + return RedirectEnd(aSubjectPrincipal, + false /* aEnsureSameOriginAndIgnoreTAO */); + } + DOMHighResTimeStamp DomainLookupStart( Maybe& aSubjectPrincipal) const { return TimingAllowedForCaller(aSubjectPrincipal) @@ -170,7 +186,8 @@ class PerformanceResourceTiming : public PerformanceEntry { bool TimingAllowedForCaller(Maybe& aCaller) const; // Check if cross-origin redirects should be reported to the caller. - bool ReportRedirectForCaller(Maybe& aCaller) const; + bool ReportRedirectForCaller(Maybe& aCaller, + bool aEnsureSameOriginAndIgnoreTAO) const; nsString mInitiatorType; const UniquePtr mTimingData; // always non-null diff --git a/dom/performance/PerformanceTiming.cpp b/dom/performance/PerformanceTiming.cpp index cd434b22a434c..f62003b2ef5c7 100644 --- a/dom/performance/PerformanceTiming.cpp +++ b/dom/performance/PerformanceTiming.cpp @@ -109,7 +109,7 @@ PerformanceTimingData::PerformanceTimingData(nsITimedChannel* aChannel, mDecodedBodySize(0), mRedirectCount(0), mAllRedirectsSameOrigin(true), - mReportCrossOriginRedirect(true), + mAllRedirectsPassTAO(true), mSecureConnection(false), mTimingAllowed(true), mInitialized(false) { @@ -221,9 +221,7 @@ void PerformanceTimingData::SetPropertiesFromHttpChannel( } mTimingAllowed = CheckAllowedOrigin(aHttpChannel, aChannel); - bool redirectsPassCheck = false; - aChannel->GetAllRedirectsPassTimingAllowCheck(&redirectsPassCheck); - mReportCrossOriginRedirect = mTimingAllowed && redirectsPassCheck; + aChannel->GetAllRedirectsPassTimingAllowCheck(&mAllRedirectsPassTAO); aChannel->GetNativeServerTiming(mServerTiming); } @@ -292,16 +290,22 @@ uint8_t PerformanceTimingData::GetRedirectCount() const { return mRedirectCount; } -bool PerformanceTimingData::ShouldReportCrossOriginRedirect() const { +bool PerformanceTimingData::ShouldReportCrossOriginRedirect( + bool aEnsureSameOriginAndIgnoreTAO) const { if (!StaticPrefs::dom_enable_performance() || !IsInitialized() || nsContentUtils::ShouldResistFingerprinting()) { return false; } + if (!mTimingAllowed || mRedirectCount == 0) { + return false; + } + // If the redirect count is 0, or if one of the cross-origin // redirects doesn't have the proper Timing-Allow-Origin header, // then RedirectStart and RedirectEnd will be set to zero - return (mRedirectCount != 0) && mReportCrossOriginRedirect; + return aEnsureSameOriginAndIgnoreTAO ? mAllRedirectsSameOrigin + : mAllRedirectsPassTAO; } DOMHighResTimeStamp PerformanceTimingData::AsyncOpenHighRes( diff --git a/dom/performance/PerformanceTiming.h b/dom/performance/PerformanceTiming.h index 8c3eda5333c57..96813ac56edb4 100644 --- a/dom/performance/PerformanceTiming.h +++ b/dom/performance/PerformanceTiming.h @@ -147,7 +147,12 @@ class PerformanceTimingData final { // If this is false the values of redirectStart/End will be 0 This is false if // no redirects occured, or if any of the responses failed the // timing-allow-origin check in HttpBaseChannel::TimingAllowCheck - bool ShouldReportCrossOriginRedirect() const; + // + // If aEnsureSameOriginAndIgnoreTAO is false, it checks if all redirects pass + // TAO. When it is true, it checks if all redirects are same-origin and + // ignores the result of TAO. + bool ShouldReportCrossOriginRedirect( + bool aEnsureSameOriginAndIgnoreTAO) const; // Cached result of CheckAllowedOrigin. If false, security sensitive // attributes of the resourceTiming object will be set to 0 @@ -200,10 +205,7 @@ class PerformanceTimingData final { bool mAllRedirectsSameOrigin = false; - // If the resourceTiming object should have non-zero redirectStart and - // redirectEnd attributes. It is false if there were no redirects, or if any - // of the responses didn't pass the timing-allow-check - bool mReportCrossOriginRedirect = false; + bool mAllRedirectsPassTAO = false; bool mSecureConnection = false; @@ -448,7 +450,7 @@ struct IPDLParamTraits { WriteIPDLParam(aMsg, aActor, aParam.mDecodedBodySize); WriteIPDLParam(aMsg, aActor, aParam.mRedirectCount); WriteIPDLParam(aMsg, aActor, aParam.mAllRedirectsSameOrigin); - WriteIPDLParam(aMsg, aActor, aParam.mReportCrossOriginRedirect); + WriteIPDLParam(aMsg, aActor, aParam.mAllRedirectsPassTAO); WriteIPDLParam(aMsg, aActor, aParam.mSecureConnection); WriteIPDLParam(aMsg, aActor, aParam.mTimingAllowed); WriteIPDLParam(aMsg, aActor, aParam.mInitialized); @@ -532,8 +534,7 @@ struct IPDLParamTraits { &aResult->mAllRedirectsSameOrigin)) { return false; } - if (!ReadIPDLParam(aMsg, aIter, aActor, - &aResult->mReportCrossOriginRedirect)) { + if (!ReadIPDLParam(aMsg, aIter, aActor, &aResult->mAllRedirectsPassTAO)) { return false; } if (!ReadIPDLParam(aMsg, aIter, aActor, &aResult->mSecureConnection)) { diff --git a/testing/web-platform/meta/navigation-timing/nav2_test_redirect_chain_xserver_final_original_origin.html.ini b/testing/web-platform/meta/navigation-timing/nav2_test_redirect_chain_xserver_final_original_origin.html.ini deleted file mode 100644 index c78f83f5cfdf4..0000000000000 --- a/testing/web-platform/meta/navigation-timing/nav2_test_redirect_chain_xserver_final_original_origin.html.ini +++ /dev/null @@ -1,4 +0,0 @@ -[nav2_test_redirect_chain_xserver_final_original_origin.html] - [Navigation Timing 2 WPT] - expected: FAIL -