Bug 1874132 - remove auth headers from preflight request for cross origin requests. r=necko-reviewers,valentin

Differential Revision: https://phabricator.services.mozilla.com/D204608

netwerk/protocol/http/HttpBaseChannel.cpp

@@ -4973,7 +4973,7 @@ nsresult HttpBaseChannel::SetupReplacementChannel(nsIURI* newURI,
     httpInternal->SetLastRedirectFlags(redirectFlags);
 
     if (LoadRequireCORSPreflight()) {
-      httpInternal->SetCorsPreflightParameters(mUnsafeHeaders, false);
+      httpInternal->SetCorsPreflightParameters(mUnsafeHeaders, false, false);
     }
   }
 
@@ -5847,17 +5847,20 @@ void HttpBaseChannel::EnsureBrowserId() {
 
 void HttpBaseChannel::SetCorsPreflightParameters(
     const nsTArray<nsCString>& aUnsafeHeaders,
-    bool aShouldStripRequestBodyHeader) {
+    bool aShouldStripRequestBodyHeader, bool aShouldStripAuthHeader) {
   MOZ_RELEASE_ASSERT(!LoadRequestObserversCalled());
 
   StoreRequireCORSPreflight(true);
   mUnsafeHeaders = aUnsafeHeaders.Clone();
-  if (aShouldStripRequestBodyHeader) {
+  if (aShouldStripRequestBodyHeader || aShouldStripAuthHeader) {
     mUnsafeHeaders.RemoveElementsBy([&](const nsCString& aHeader) {
-      return aHeader.LowerCaseEqualsASCII("content-type") ||
+      return (aShouldStripRequestBodyHeader &&
+              (aHeader.LowerCaseEqualsASCII("content-type") ||
                aHeader.LowerCaseEqualsASCII("content-encoding") ||
                aHeader.LowerCaseEqualsASCII("content-language") ||
-             aHeader.LowerCaseEqualsASCII("content-location");
+               aHeader.LowerCaseEqualsASCII("content-location"))) ||
+             (aShouldStripAuthHeader &&
+              aHeader.LowerCaseEqualsASCII("authorization"));
     });
   }
 }

netwerk/protocol/http/HttpBaseChannel.h

@@ -319,7 +319,7 @@ class HttpBaseChannel : public nsHashPropertyBag,
   NS_IMETHOD GetProxyURI(nsIURI** proxyURI) override;
   virtual void SetCorsPreflightParameters(
       const nsTArray<nsCString>& unsafeHeaders,
-      bool aShouldStripRequestBodyHeader) override;
+      bool aShouldStripRequestBodyHeader, bool aShouldStripAuthHeader) override;
   virtual void SetAltDataForChild(bool aIsForChild) override;
   virtual void DisableAltDataCache() override {
     StoreDisableAltDataCache(true);

netwerk/protocol/http/HttpChannelParent.cpp

@@ -587,7 +587,7 @@ bool HttpChannelParent::DoAsyncOpen(
 
   if (aCorsPreflightArgs.isSome()) {
     const CorsPreflightArgs& args = aCorsPreflightArgs.ref();
-    httpChannel->SetCorsPreflightParameters(args.unsafeHeaders(), false);
+    httpChannel->SetCorsPreflightParameters(args.unsafeHeaders(), false, false);
   }
 
   nsCOMPtr<nsIInputStream> stream = DeserializeIPCStream(uploadStream);
@@ -896,7 +896,7 @@ mozilla::ipc::IPCResult HttpChannelParent::RecvRedirect2Verify(
         MOZ_RELEASE_ASSERT(newInternalChannel);
         const CorsPreflightArgs& args = aCorsPreflightArgs.ref();
         newInternalChannel->SetCorsPreflightParameters(args.unsafeHeaders(),
-                                                       false);
+                                                       false, false);
       }
 
       if (aReferrerInfo) {

netwerk/protocol/http/ObliviousHttpChannel.cpp

@@ -419,10 +419,10 @@ void ObliviousHttpChannel::SetAltDataForChild(bool aIsForChild) {
 
 void ObliviousHttpChannel::SetCorsPreflightParameters(
     nsTArray<nsTString<char>> const& aUnsafeHeaders,
-    bool aShouldStripRequestBodyHeader) {
+    bool aShouldStripRequestBodyHeader, bool aShouldStripAuthHeader) {
   if (mInnerChannelInternal) {
     mInnerChannelInternal->SetCorsPreflightParameters(
-        aUnsafeHeaders, aShouldStripRequestBodyHeader);
+        aUnsafeHeaders, aShouldStripRequestBodyHeader, aShouldStripAuthHeader);
   }
 }
 

netwerk/protocol/http/nsCORSListenerProxy.cpp

@@ -441,7 +441,8 @@ nsresult nsCORSListenerProxy::Init(nsIChannel* aChannel,
       getter_AddRefs(mOuterNotificationCallbacks));
   aChannel->SetNotificationCallbacks(this);
 
-  nsresult rv = UpdateChannel(aChannel, aAllowDataURI, UpdateType::Default);
+  nsresult rv =
+      UpdateChannel(aChannel, aAllowDataURI, UpdateType::Default, false);
   if (NS_FAILED(rv)) {
     {
       MutexAutoLock lock(mMutex);
@@ -765,7 +766,7 @@ nsCORSListenerProxy::AsyncOnChannelRedirect(
     // data URIs should have been blocked before we got to the internal
     // redirect.
     rv = UpdateChannel(aNewChannel, DataURIHandling::Allow,
-                       UpdateType::InternalOrHSTSRedirect);
+                       UpdateType::InternalOrHSTSRedirect, false);
     if (NS_FAILED(rv)) {
       NS_WARNING(
           "nsCORSListenerProxy::AsyncOnChannelRedirect: "
@@ -835,6 +836,13 @@ nsCORSListenerProxy::AsyncOnChannelRedirect(
     }
 
     bool rewriteToGET = false;
+    // We need to strip auth header from preflight request for
+    // cross-origin redirects.
+    // See Bug 1874132
+    bool stripAuthHeader =
+        StaticPrefs::network_fetch_redirect_stripAuthHeader() &&
+        NS_ShouldRemoveAuthHeaderOnRedirect(aOldChannel, aNewChannel, aFlags);
+
     nsCOMPtr<nsIHttpChannel> oldHttpChannel = do_QueryInterface(aOldChannel);
     if (oldHttpChannel) {
       nsAutoCString method;
@@ -843,9 +851,10 @@ nsCORSListenerProxy::AsyncOnChannelRedirect(
                                                              &rewriteToGET);
     }
 
-    rv = UpdateChannel(aNewChannel, DataURIHandling::Disallow,
+    rv = UpdateChannel(
-                       rewriteToGET ? UpdateType::StripRequestBodyHeader
+        aNewChannel, DataURIHandling::Disallow,
-                                    : UpdateType::Default);
+        rewriteToGET ? UpdateType::StripRequestBodyHeader : UpdateType::Default,
+        stripAuthHeader);
     if (NS_FAILED(rv)) {
       NS_WARNING(
           "nsCORSListenerProxy::AsyncOnChannelRedirect: "
@@ -930,7 +939,10 @@ bool CheckInsecureUpgradePreventsCORS(nsIPrincipal* aRequestingPrincipal,
 
 nsresult nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel,
                                             DataURIHandling aAllowDataURI,
-                                            UpdateType aUpdateType) {
+                                            UpdateType aUpdateType,
+                                            bool aStripAuthHeader) {
+  MOZ_ASSERT_IF(aUpdateType == UpdateType::InternalOrHSTSRedirect,
+                !aStripAuthHeader);
   nsCOMPtr<nsIURI> uri, originalURI;
   nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1020,7 +1032,7 @@ nsresult nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel,
 
   // Check if we need to do a preflight, and if so set one up. This must be
   // called once we know that the request is going, or has gone, cross-origin.
-  rv = CheckPreflightNeeded(aChannel, aUpdateType);
+  rv = CheckPreflightNeeded(aChannel, aUpdateType, aStripAuthHeader);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // It's a cross site load
@@ -1087,7 +1099,8 @@ nsresult nsCORSListenerProxy::UpdateChannel(nsIChannel* aChannel,
 }
 
 nsresult nsCORSListenerProxy::CheckPreflightNeeded(nsIChannel* aChannel,
-                                                   UpdateType aUpdateType) {
+                                                   UpdateType aUpdateType,
+                                                   bool aStripAuthHeader) {
   // If this caller isn't using AsyncOpen, or if this *is* a preflight channel,
   // then we shouldn't initiate preflight for this channel.
   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->LoadInfo();
@@ -1154,7 +1167,7 @@ nsresult nsCORSListenerProxy::CheckPreflightNeeded(nsIChannel* aChannel,
 
   internal->SetCorsPreflightParameters(
       headers.IsEmpty() ? loadInfoHeaders : headers,
-      aUpdateType == UpdateType::StripRequestBodyHeader);
+      aUpdateType == UpdateType::StripRequestBodyHeader, aStripAuthHeader);
 
   return NS_OK;
 }

netwerk/protocol/http/nsCORSListenerProxy.h

@@ -92,10 +92,12 @@ class nsCORSListenerProxy final : public nsIInterfaceRequestor,
 
   [[nodiscard]] nsresult UpdateChannel(nsIChannel* aChannel,
                                        DataURIHandling aAllowDataURI,
-                                       UpdateType aUpdateType);
+                                       UpdateType aUpdateType,
+                                       bool aStripAuthHeader);
   [[nodiscard]] nsresult CheckRequestApproved(nsIRequest* aRequest);
   [[nodiscard]] nsresult CheckPreflightNeeded(nsIChannel* aChannel,
-                                              UpdateType aUpdateType);
+                                              UpdateType aUpdateType,
+                                              bool aStripAuthHeader);
 
   nsCOMPtr<nsIStreamListener> mOuterListener;
   // The principal that originally kicked off the request

netwerk/protocol/http/nsIHttpChannelInternal.idl

@@ -381,7 +381,8 @@ interface nsIHttpChannelInternal : nsISupports
      */
     [noscript, notxpcom, nostdcall]
     void setCorsPreflightParameters(in CStringArrayRef unsafeHeaders,
-                                    in boolean shouldStripRequestBodyHeader);
+                                    in boolean shouldStripRequestBodyHeader,
+                                    in boolean shouldStripAuthHeader);
 
     [noscript, notxpcom, nostdcall]
     void setAltDataForChild(in boolean aIsForChild);

netwerk/protocol/viewsource/nsViewSourceChannel.cpp

@@ -1048,9 +1048,9 @@ NS_IMETHODIMP nsViewSourceChannel::GetDocumentCharacterSet(
 // Have to manually forward SetCorsPreflightParameters since it's [notxpcom]
 void nsViewSourceChannel::SetCorsPreflightParameters(
     const nsTArray<nsCString>& aUnsafeHeaders,
-    bool aShouldStripRequestBodyHeader) {
+    bool aShouldStripRequestBodyHeader, bool aShouldStripAuthHeader) {
   mHttpChannelInternal->SetCorsPreflightParameters(
-      aUnsafeHeaders, aShouldStripRequestBodyHeader);
+      aUnsafeHeaders, aShouldStripRequestBodyHeader, aShouldStripAuthHeader);
 }
 
 void nsViewSourceChannel::SetAltDataForChild(bool aIsForChild) {

testing/web-platform/tests/fetch/api/credentials/authentication-redirection.any.js

@@ -24,6 +24,6 @@ promise_test(async test => {
 }, "getAuthorizationHeaderValue - same origin redirection");
 
 promise_test(async (test) => {
-  const result = await getAuthorizationHeaderValue(get_host_info().HTTPS_REMOTE_ORIGIN + "/fetch/api/resources/redirect.py?allow_headers=Authorization&location=" + encodeURIComponent(get_host_info().HTTPS_ORIGIN + "/fetch/api/resources/dump-authorization-header.py"));
+  const result = await getAuthorizationHeaderValue(get_host_info().HTTPS_REMOTE_ORIGIN + "/fetch/api/resources/redirect.py?allow_headers=Authorization&location=" + encodeURIComponent(get_host_info().HTTPS_ORIGIN + "/fetch/api/resources/dump-authorization-header.py?strip_auth_header=true"));
   assert_equals(result, "none");
 }, "getAuthorizationHeaderValue - cross origin redirection");

testing/web-platform/tests/fetch/api/resources/dump-authorization-header.py

@@ -2,6 +2,11 @@ def main(request, response):
     headers = [(b"Content-Type", "text/html"),
                (b"Cache-Control", b"no-cache")]
 
+    if (request.GET.first(b"strip_auth_header", False) and request.method == "OPTIONS" and
+        b"authorization" in request.headers.get(b"Access-Control-Request-Headers", b"").lower()):
+        # Auth header should not be sent for preflight after cross-origin redirect.
+        return 500, headers, "fail"
+
     if b"Origin" in request.headers:
         headers.append((b"Access-Control-Allow-Origin", request.headers.get(b"Origin", b"")))
         headers.append((b"Access-Control-Allow-Credentials", b"true"))