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

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;
 }
 

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<uint8_t>&& aDataForSniffer, const Maybe<nsCString>& aAltSvcUsed,
-    const bool& aDataToChildProcess) {
+    const bool& aDataToChildProcess, const bool& aRestarted) {
   mEventQ->RunOrEnqueue(new NeckoTargetChannelFunctionEvent(
       this, [self = UnsafePtr<HttpTransactionParent>(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<uint8_t>&& aDataForSniffer, const Maybe<nsCString>& aAltSvcUsed,
-    const bool& aDataToChildProcess) {
+    const bool& aDataToChildProcess, const bool& aRestarted) {
   LOG(("HttpTransactionParent::DoOnStartRequest [this=%p aStatus=%" PRIx32
        "]\n",
        this, static_cast<uint32_t>(aStatus)));
@@ -484,6 +492,7 @@ void HttpTransactionParent::DoOnStartRequest(
 
   mProxyConnectResponseCode = aProxyConnectResponseCode;
   mDataForSniffer = std::move(aDataForSniffer);
+  mRestarted = aRestarted;
 
   nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
   MOZ_ASSERT(httpChannel, "mChannel is expected to implement nsIHttpChannel");

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<uint8_t>&& aDataForSniffer, const Maybe<nsCString>& 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<nsHttpResponseHead>& aResponseHead,
-                        const nsCString& aSecurityInfoSerialization,
-                        const bool& aProxyConnectFailed,
-                        const TimingStructArgs& aTimings,
-                        const int32_t& aProxyConnectResponseCode,
-                        nsTArray<uint8_t>&& aDataForSniffer,
-                        const Maybe<nsCString>& aAltSvcUsed,
-                        const bool& aDataToChildProcess);
+  void DoOnStartRequest(
+      const nsresult& aStatus, const Maybe<nsHttpResponseHead>& aResponseHead,
+      const nsCString& aSecurityInfoSerialization,
+      const bool& aProxyConnectFailed, const TimingStructArgs& aTimings,
+      const int32_t& aProxyConnectResponseCode,
+      nsTArray<uint8_t>&& aDataForSniffer, const Maybe<nsCString>& 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;
 

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
 

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,

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.

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();
     }
   }
 

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<nsISeekableStream> 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() {

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<nsHttpHeaderArray> mForTakeResponseTrailers;
   bool mResponseTrailersTaken;
 
+  // Set when this transaction was restarted by call to Restart().  Used to tell
+  // the http channel to reset proxy authentication.
+  Atomic<bool> mRestarted;
+
   // The time when the transaction was submitted to the Connection Manager
   TimeStamp mPendingTime;
 

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();
 };