From 141e8f5d72ec279f26064b44d3c8d9c7765080a0 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Mon, 3 Aug 2020 18:31:36 +0000 Subject: [PATCH] Bug 1609410 - Clear used proxy identity in nsHttpChannelAuthProvider to prevent authentication prompt pop-up on transaction internal restart, r=kershaw,necko-reviewers Differential Revision: https://phabricator.services.mozilla.com/D84533 --- .../protocol/http/HttpTransactionChild.cpp | 3 +- .../protocol/http/HttpTransactionParent.cpp | 25 +++++++++---- netwerk/protocol/http/HttpTransactionParent.h | 19 +++++----- netwerk/protocol/http/HttpTransactionShell.h | 5 ++- netwerk/protocol/http/PHttpTransaction.ipdl | 3 +- netwerk/protocol/http/nsHttpChannel.cpp | 8 ++++ .../http/nsHttpChannelAuthProvider.cpp | 9 ++++- netwerk/protocol/http/nsHttpTransaction.cpp | 37 ++++++++++++++++--- netwerk/protocol/http/nsHttpTransaction.h | 5 +++ .../http/nsIHttpChannelAuthProvider.idl | 7 ++++ 10 files changed, 94 insertions(+), 27 deletions(-) diff --git a/netwerk/protocol/http/HttpTransactionChild.cpp b/netwerk/protocol/http/HttpTransactionChild.cpp index f8825d602b8f..5535253af878 100644 --- a/netwerk/protocol/http/HttpTransactionChild.cpp +++ b/netwerk/protocol/http/HttpTransactionChild.cpp @@ -496,7 +496,8 @@ HttpTransactionChild::OnStartRequest(nsIRequest* aRequest) { mTransaction->ProxyConnectFailed(), ToTimingStructArgs(mTransaction->Timings()), proxyConnectResponseCode, dataForSniffer, - optionalAltSvcUsed, !!mDataBridgeParent); + optionalAltSvcUsed, !!mDataBridgeParent, + mTransaction->TakeRestartedState()); return NS_OK; } diff --git a/netwerk/protocol/http/HttpTransactionParent.cpp b/netwerk/protocol/http/HttpTransactionParent.cpp index 28e373079cd5..6b4342770dd7 100644 --- a/netwerk/protocol/http/HttpTransactionParent.cpp +++ b/netwerk/protocol/http/HttpTransactionParent.cpp @@ -92,7 +92,8 @@ HttpTransactionParent::HttpTransactionParent(bool aIsDocumentLoad) mProxyConnectResponseCode(0), mChannelId(0), mDataSentToChildProcess(false), - mIsDocumentLoad(aIsDocumentLoad) { + mIsDocumentLoad(aIsDocumentLoad), + mRestarted(false) { LOG(("Creating HttpTransactionParent @%p\n", this)); this->mSelfAddr.inet = {}; @@ -368,6 +369,12 @@ nsISupports* HttpTransactionParent::SecurityInfo() { return mSecurityInfo; } bool HttpTransactionParent::ProxyConnectFailed() { return mProxyConnectFailed; } +bool HttpTransactionParent::TakeRestartedState() { + bool result = mRestarted; + mRestarted = false; + return result; +} + void HttpTransactionParent::DontReuseConnection() { MOZ_ASSERT(NS_IsMainThread()); Unused << SendDontReuseConnection(); @@ -418,17 +425,18 @@ mozilla::ipc::IPCResult HttpTransactionParent::RecvOnStartRequest( const bool& aProxyConnectFailed, const TimingStructArgs& aTimings, const int32_t& aProxyConnectResponseCode, nsTArray&& aDataForSniffer, const Maybe& aAltSvcUsed, - const bool& aDataToChildProcess) { + const bool& aDataToChildProcess, const bool& aRestarted) { mEventQ->RunOrEnqueue(new NeckoTargetChannelFunctionEvent( this, [self = UnsafePtr(this), aStatus, aResponseHead, aSecurityInfoSerialization, aProxyConnectFailed, aTimings, aProxyConnectResponseCode, aDataForSniffer = CopyableTArray{std::move(aDataForSniffer)}, - aAltSvcUsed, aDataToChildProcess]() mutable { - self->DoOnStartRequest( - aStatus, aResponseHead, aSecurityInfoSerialization, - aProxyConnectFailed, aTimings, aProxyConnectResponseCode, - std::move(aDataForSniffer), aAltSvcUsed, aDataToChildProcess); + aAltSvcUsed, aDataToChildProcess, aRestarted]() mutable { + self->DoOnStartRequest(aStatus, aResponseHead, + aSecurityInfoSerialization, aProxyConnectFailed, + aTimings, aProxyConnectResponseCode, + std::move(aDataForSniffer), aAltSvcUsed, + aDataToChildProcess, aRestarted); })); return IPC_OK(); } @@ -457,7 +465,7 @@ void HttpTransactionParent::DoOnStartRequest( const bool& aProxyConnectFailed, const TimingStructArgs& aTimings, const int32_t& aProxyConnectResponseCode, nsTArray&& aDataForSniffer, const Maybe& aAltSvcUsed, - const bool& aDataToChildProcess) { + const bool& aDataToChildProcess, const bool& aRestarted) { LOG(("HttpTransactionParent::DoOnStartRequest [this=%p aStatus=%" PRIx32 "]\n", this, static_cast(aStatus))); @@ -484,6 +492,7 @@ void HttpTransactionParent::DoOnStartRequest( mProxyConnectResponseCode = aProxyConnectResponseCode; mDataForSniffer = std::move(aDataForSniffer); + mRestarted = aRestarted; nsCOMPtr httpChannel = do_QueryInterface(mChannel); MOZ_ASSERT(httpChannel, "mChannel is expected to implement nsIHttpChannel"); diff --git a/netwerk/protocol/http/HttpTransactionParent.h b/netwerk/protocol/http/HttpTransactionParent.h index 6935b488c17d..5a9e258cebc6 100644 --- a/netwerk/protocol/http/HttpTransactionParent.h +++ b/netwerk/protocol/http/HttpTransactionParent.h @@ -53,7 +53,7 @@ class HttpTransactionParent final : public PHttpTransactionParent, const bool& aProxyConnectFailed, const TimingStructArgs& aTimings, const int32_t& aProxyConnectResponseCode, nsTArray&& aDataForSniffer, const Maybe& aAltSvcUsed, - const bool& aDataToChildProcess); + const bool& aDataToChildProcess, const bool& aRestarted); mozilla::ipc::IPCResult RecvOnTransportStatus( const nsresult& aStatus, const int64_t& aProgress, const int64_t& aProgressMax, @@ -90,15 +90,13 @@ class HttpTransactionParent final : public PHttpTransactionParent, void GetStructFromInfo(nsHttpConnectionInfo* aInfo, HttpConnectionInfoCloneArgs& aArgs); - void DoOnStartRequest(const nsresult& aStatus, - const Maybe& aResponseHead, - const nsCString& aSecurityInfoSerialization, - const bool& aProxyConnectFailed, - const TimingStructArgs& aTimings, - const int32_t& aProxyConnectResponseCode, - nsTArray&& aDataForSniffer, - const Maybe& aAltSvcUsed, - const bool& aDataToChildProcess); + void DoOnStartRequest( + const nsresult& aStatus, const Maybe& aResponseHead, + const nsCString& aSecurityInfoSerialization, + const bool& aProxyConnectFailed, const TimingStructArgs& aTimings, + const int32_t& aProxyConnectResponseCode, + nsTArray&& aDataForSniffer, const Maybe& aAltSvcUsed, + const bool& aDataToChildProcess, const bool& aRestarted); void DoOnDataAvailable(const nsCString& aData, const uint64_t& aOffset, const uint32_t& aCount); void DoOnStopRequest( @@ -141,6 +139,7 @@ class HttpTransactionParent final : public PHttpTransactionParent, uint64_t mChannelId; bool mDataSentToChildProcess; bool mIsDocumentLoad; + bool mRestarted; TimeStamp mRedirectStart; TimeStamp mRedirectEnd; diff --git a/netwerk/protocol/http/HttpTransactionShell.h b/netwerk/protocol/http/HttpTransactionShell.h index 99aad8d9418a..6f122e665402 100644 --- a/netwerk/protocol/http/HttpTransactionShell.h +++ b/netwerk/protocol/http/HttpTransactionShell.h @@ -151,6 +151,8 @@ class HttpTransactionShell : public nsISupports { virtual nsHttpTransaction* AsHttpTransaction() = 0; virtual HttpTransactionParent* AsHttpTransactionParent() = 0; + + virtual bool TakeRestartedState() = 0; }; NS_DEFINE_STATIC_IID_ACCESSOR(HttpTransactionShell, HTTPTRANSACTIONSHELL_IID) @@ -204,7 +206,8 @@ NS_DEFINE_STATIC_IID_ACCESSOR(HttpTransactionShell, HTTPTRANSACTIONSHELL_IID) virtual int32_t GetProxyConnectResponseCode() override; \ virtual bool DataSentToChildProcess() override; \ virtual nsHttpTransaction* AsHttpTransaction() override; \ - virtual HttpTransactionParent* AsHttpTransactionParent() override; + virtual HttpTransactionParent* AsHttpTransactionParent() override; \ + virtual bool TakeRestartedState() override; } // namespace net } // namespace mozilla diff --git a/netwerk/protocol/http/PHttpTransaction.ipdl b/netwerk/protocol/http/PHttpTransaction.ipdl index fe72065709a1..bd133bc29942 100644 --- a/netwerk/protocol/http/PHttpTransaction.ipdl +++ b/netwerk/protocol/http/PHttpTransaction.ipdl @@ -45,7 +45,8 @@ parent: int32_t proxyConnectResponseCode, uint8_t[] dataForSniffer, nsCString? altSvcUsed, - bool dataToChildProcess); + bool dataToChildProcess, + bool restarted); async OnTransportStatus(nsresult status, int64_t progress, int64_t progressMax, diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index d3138e6ed45a..cac99c7d5e27 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -2725,6 +2725,7 @@ nsresult nsHttpChannel::ContinueProcessResponse3(nsresult rv) { rv = NS_OK; uint32_t httpStatus = mResponseHead->Status(); + bool transactionRestarted = mTransaction->TakeRestartedState(); // handle different server response categories. Note that we handle // caching or not caching of error pages in @@ -2816,6 +2817,13 @@ nsresult nsHttpChannel::ContinueProcessResponse3(nsresult rv) { break; case 401: case 407: + if (MOZ_UNLIKELY(httpStatus == 407 && transactionRestarted)) { + // The transaction has been internally restarted. We want to + // authenticate to the proxy again, so reuse either cached credentials + // or use default credentials for NTLM/Negotiate. This prevents + // considering the previously used creadentials as invalid. + mAuthProvider->ClearProxyIdent(); + } if (MOZ_UNLIKELY(mCustomAuthHeader) && httpStatus == 401) { // When a custom auth header fails, we don't want to try // any cached credentials, nor we want to ask the user. diff --git a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp b/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp index 2f0136cfc032..ea5731dc0409 100644 --- a/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp +++ b/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp @@ -448,6 +448,13 @@ nsresult nsHttpChannelAuthProvider::UpdateCache( return rv; } +NS_IMETHODIMP nsHttpChannelAuthProvider::ClearProxyIdent() { + LOG(("nsHttpChannelAuthProvider::ClearProxyIdent [this=%p]\n", this)); + + mProxyIdent.Clear(); + return NS_OK; +} + nsresult nsHttpChannelAuthProvider::PrepareForAuthentication(bool proxyAuth) { LOG( ("nsHttpChannelAuthProvider::PrepareForAuthentication " @@ -742,7 +749,7 @@ nsresult nsHttpChannelAuthProvider::GetCredentialsForChallenge( // - if we didn't clear the proxy identity, it would be considered // as non-valid and we would ask the user again ; clearing it forces // use of the cached identity and not asking the user again - mProxyIdent.Clear(); + ClearProxyIdent(); } } diff --git a/netwerk/protocol/http/nsHttpTransaction.cpp b/netwerk/protocol/http/nsHttpTransaction.cpp index 72b124a31aaf..b3d314befa77 100644 --- a/netwerk/protocol/http/nsHttpTransaction.cpp +++ b/netwerk/protocol/http/nsHttpTransaction.cpp @@ -122,6 +122,7 @@ nsHttpTransaction::nsHttpTransaction() mResponseHeadTaken(false), mForTakeResponseTrailers(nullptr), mResponseTrailersTaken(false), + mRestarted(false), mTopLevelOuterContentWindowId(0), mSubmittedRatePacing(false), mPassedRatePacing(false), @@ -1359,6 +1360,17 @@ nsresult nsHttpTransaction::Restart() { LOG(("restarting transaction @%p\n", this)); mTunnelProvider = nullptr; + if (mRequestHead) { + // Dispatching on a new connection better w/o an ambient connection proxy + // auth request header to not confuse the proxy authenticator. + nsAutoCString proxyAuth; + if (NS_SUCCEEDED( + mRequestHead->GetHeader(nsHttp::Proxy_Authorization, proxyAuth)) && + IsStickyAuthSchemeAt(proxyAuth)) { + Unused << mRequestHead->ClearHeader(nsHttp::Proxy_Authorization); + } + } + // rewind streams in case we already wrote out the request nsCOMPtr seekable = do_QueryInterface(mRequestStream); if (seekable) seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0); @@ -1391,10 +1403,18 @@ nsresult nsHttpTransaction::Restart() { // Reset mDoNotRemoveAltSvc for the next try. mDoNotRemoveAltSvc = false; + mRestarted = true; return gHttpHandler->InitiateTransaction(this, mPriority); } +bool nsHttpTransaction::TakeRestartedState() { + // This return true if the transaction has been restarted internally. Used to + // let the consuming nsHttpChannel reset proxy authentication. The flag is + // reset to false by this method. + return mRestarted.exchange(false); +} + char* nsHttpTransaction::LocateHttpStart(char* buf, uint32_t len, bool aAllowPartialMatch) { MOZ_ASSERT(!aAllowPartialMatch || mLineBuf.IsEmpty()); @@ -2095,6 +2115,15 @@ void nsHttpTransaction::CheckForStickyAuthSchemeAt(nsHttpAtom const& header) { return; } + if (IsStickyAuthSchemeAt(auth)) { + LOG((" connection made sticky")); + // This is enough to make this transaction keep it's current connection, + // prevents the connection from being released back to the pool. + mCaps |= NS_HTTP_STICKY_CONNECTION; + } +} + +bool nsHttpTransaction::IsStickyAuthSchemeAt(nsACString const& auth) { Tokenizer p(auth); nsAutoCString schema; while (p.ReadWord(schema)) { @@ -2116,11 +2145,7 @@ void nsHttpTransaction::CheckForStickyAuthSchemeAt(nsHttpAtom const& header) { nsresult rv = authenticator->GetAuthFlags(&flags); if (NS_SUCCEEDED(rv) && (flags & nsIHttpAuthenticator::CONNECTION_BASED)) { - LOG((" connection made sticky, found %s auth shema", schema.get())); - // This is enough to make this transaction keep it's current connection, - // prevents the connection from being released back to the pool. - mCaps |= NS_HTTP_STICKY_CONNECTION; - break; + return true; } } @@ -2128,6 +2153,8 @@ void nsHttpTransaction::CheckForStickyAuthSchemeAt(nsHttpAtom const& header) { p.SkipUntil(Tokenizer::Token::NewLine()); p.SkipWhites(Tokenizer::INCLUDE_NEW_LINE); } + + return false; } const TimingStruct nsHttpTransaction::Timings() { diff --git a/netwerk/protocol/http/nsHttpTransaction.h b/netwerk/protocol/http/nsHttpTransaction.h index 643f18836479..dc70971f13a0 100644 --- a/netwerk/protocol/http/nsHttpTransaction.h +++ b/netwerk/protocol/http/nsHttpTransaction.h @@ -182,6 +182,7 @@ class nsHttpTransaction final : public nsAHttpTransaction, // connection from very start of the authentication process. void CheckForStickyAuthScheme(); void CheckForStickyAuthSchemeAt(nsHttpAtom const& header); + bool IsStickyAuthSchemeAt(nsACString const& auth); // Called from WriteSegments. Checks for conditions whether to throttle // reading the content. When this returns true, WriteSegments returns @@ -341,6 +342,10 @@ class nsHttpTransaction final : public nsAHttpTransaction, UniquePtr mForTakeResponseTrailers; bool mResponseTrailersTaken; + // Set when this transaction was restarted by call to Restart(). Used to tell + // the http channel to reset proxy authentication. + Atomic mRestarted; + // The time when the transaction was submitted to the Connection Manager TimeStamp mPendingTime; diff --git a/netwerk/protocol/http/nsIHttpChannelAuthProvider.idl b/netwerk/protocol/http/nsIHttpChannelAuthProvider.idl index d482192aa059..73282a4beff7 100644 --- a/netwerk/protocol/http/nsIHttpChannelAuthProvider.idl +++ b/netwerk/protocol/http/nsIHttpChannelAuthProvider.idl @@ -76,4 +76,11 @@ interface nsIHttpChannelAuthProvider : nsICancelable * weak references. */ [must_use] void disconnect(in nsresult status); + + /** + * Clear the proxy ident to not consider it invalid on re-athentication. + * Called when the channel finds out its transaction has been internally + * restarted. + */ + void clearProxyIdent(); };