Revert "webview: make extra header handling more sensible."

This reverts commit 6e46cca3.

Reason for revert: Breaking auth flow b/156844354

Original change's description:
> webview: make extra header handling more sensible.
>
> Change how extra headers provided through loadUrl(url, extra_headers)
> are handled in WebView:
>
> 1) Remove any extra headers from the request if the request is
>    redirected to a different origin, since they might be sensitive.
>
> 2) Don't attempt to add any extra headers for the redirect target URL
>    when we encounter a redirect; this is likely to be surprising and
>    unwanted.
>
> 3) Record metrics on when we add headers and what was done with them on
>    redirect.
>
> 4) Add an additional test verifying that the extra headers are cleared
>    if the app loads the same URL again via loadUrl(url).
>
> Bug: 1038002
> Change-Id: Ib39e2938f7b76d212cd20773aab56da138088b63
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999229
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
> Commit-Queue: Richard Coles <torne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#747517}

TBR=isherman@chromium.org,torne@chromium.org,tobiasjs@chromium.org

Bug: 1038002
Change-Id: I18791d4ef448d1ed9bcd4f0b02d4b8884018e8c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2213114
Commit-Queue: Ben Mason <benmason@chromium.org>
Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org>
Reviewed-by: Ben Mason <benmason@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771390}

android_webview/browser/network_service/aw_url_loader_throttle.cc

@@ -5,30 +5,10 @@
 #include "android_webview/browser/network_service/aw_url_loader_throttle.h"
 
 #include "android_webview/browser/aw_resource_context.h"
-#include "base/metrics/histogram_macros.h"
-#include "base/strings/string_util.h"
-#include "net/http/http_request_headers.h"
+#include "net/http/http_response_headers.h"
 
 namespace android_webview {
 
-namespace {
-
-// These values are logged to UMA. Entries should not be renumbered and
-// numeric values should never be reused. Please keep in sync with
-// "WebViewExtraHeaders" in src/tools/metrics/histograms/enums.xml.
-enum class ExtraHeaders {
-  kAddedInStartRequest = 0,
-  kKeptOnSameOriginRedirect = 1,
-  kRemovedOnCrossOriginRedirect = 2,
-  kMaxValue = kRemovedOnCrossOriginRedirect
-};
-
-void RecordExtraHeadersUMA(ExtraHeaders value) {
-  UMA_HISTOGRAM_ENUMERATION("Android.WebView.ExtraHeaders", value);
-}
-
-}  // namespace
-
 AwURLLoaderThrottle::AwURLLoaderThrottle(AwResourceContext* aw_resource_context)
     : aw_resource_context_(aw_resource_context) {}
 
@@ -37,10 +17,6 @@ AwURLLoaderThrottle::~AwURLLoaderThrottle() = default;
 void AwURLLoaderThrottle::WillStartRequest(network::ResourceRequest* request,
                                            bool* defer) {
   AddExtraHeadersIfNeeded(request->url, &request->headers);
-  if (!added_headers_.empty()) {
-    original_origin_ = url::Origin::Create(request->url);
-    RecordExtraHeadersUMA(ExtraHeaders::kAddedInStartRequest);
-  }
 }
 
 void AwURLLoaderThrottle::WillRedirectRequest(
@@ -50,19 +26,7 @@ void AwURLLoaderThrottle::WillRedirectRequest(
     std::vector<std::string>* to_be_removed_request_headers,
     net::HttpRequestHeaders* modified_request_headers,
     net::HttpRequestHeaders* modified_cors_exempt_request_headers) {
-  if (!added_headers_.empty()) {
-    if (original_origin_.CanBeDerivedFrom(redirect_info->new_url)) {
-      RecordExtraHeadersUMA(ExtraHeaders::kKeptOnSameOriginRedirect);
-    } else {
-      // Cross-origin redirect. Remove the headers we added.
-      to_be_removed_request_headers->insert(
-          to_be_removed_request_headers->end(),
-          std::make_move_iterator(added_headers_.begin()),
-          std::make_move_iterator(added_headers_.end()));
-      added_headers_.clear();
-      RecordExtraHeadersUMA(ExtraHeaders::kRemovedOnCrossOriginRedirect);
-    }
-  }
+  AddExtraHeadersIfNeeded(redirect_info->new_url, modified_request_headers);
 }
 
 void AwURLLoaderThrottle::AddExtraHeadersIfNeeded(
@@ -74,13 +38,8 @@ void AwURLLoaderThrottle::AddExtraHeadersIfNeeded(
 
   net::HttpRequestHeaders temp_headers;
   temp_headers.AddHeadersFromString(extra_headers);
-  for (net::HttpRequestHeaders::Iterator it(temp_headers); it.GetNext();) {
-    if (headers->HasHeader(it.name()))
-      continue;
-
-    headers->SetHeader(it.name(), it.value());
-    added_headers_.push_back(it.name());
-  }
+  for (net::HttpRequestHeaders::Iterator it(temp_headers); it.GetNext();)
+    headers->SetHeaderIfMissing(it.name(), it.value());
 }
 
 }  // namespace android_webview

android_webview/browser/network_service/aw_url_loader_throttle.h

@@ -10,7 +10,6 @@
 
 #include "base/macros.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
-#include "url/origin.h"
 
 class GURL;
 
@@ -42,8 +41,6 @@ class AwURLLoaderThrottle : public blink::URLLoaderThrottle {
                                net::HttpRequestHeaders* headers);
 
   AwResourceContext* aw_resource_context_;
-  std::vector<std::string> added_headers_;
-  url::Origin original_origin_;
 
   DISALLOW_COPY_AND_ASSIGN(AwURLLoaderThrottle);
 };

android_webview/javatests/src/org/chromium/android_webview/test/LoadUrlTest.java

@@ -255,19 +255,6 @@ public class LoadUrlTest {
         }
     }
 
-    /**
-     * Make a test server URL look like it is a different origin.
-     */
-    private static String toDifferentOriginUrl(String url) {
-        if (url.contains("localhost")) {
-            return url.replace("localhost", "127.0.0.1");
-        } else if (url.contains("127.0.0.1")) {
-            return url.replace("127.0.0.1", "localhost");
-        } else {
-            throw new RuntimeException("Can't convert url " + url + " to different origin");
-        }
-    }
-
     @Test
     @SmallTest
     @Feature({"AndroidWebView"})
@@ -292,7 +279,7 @@ public class LoadUrlTest {
         int onReceivedTitleCallCount = onReceivedTitleHelper.getCallCount();
         loadUrlWithExtraHeadersSync(awContents, contentsClient.getOnPageFinishedHelper(), url2,
                 createHeadersMap(extraHeaders));
-        // Verify that extra headers are passed to the loaded url, but not to the image subresource.
+        // Verify that extra headers are passed to the loaded url.
         validateHeadersValue(awContents, contentsClient, extraHeaders, true);
         onReceivedTitleHelper.waitForCallback(onReceivedTitleCallCount);
         Assert.assertEquals("5", onReceivedTitleHelper.getTitle());
@@ -342,30 +329,6 @@ public class LoadUrlTest {
         validateHeadersValue(awContents, contentsClient, extraHeaders, true);
     }
 
-    @Test
-    @SmallTest
-    @Feature({"AndroidWebView"})
-    public void testLoadWithoutExtraHeadersClearsState() throws Throwable {
-        final TestAwContentsClient contentsClient = new TestAwContentsClient();
-        final AwTestContainerView testContainerView =
-                mActivityTestRule.createAwTestContainerViewOnMainSync(contentsClient);
-        final AwContents awContents = testContainerView.getAwContents();
-        AwActivityTestRule.enableJavaScriptOnUiThread(awContents);
-        String[] extraHeaders = {
-                "X-ExtraHeaders1", "extra-header-data1", "x-extraHeaders2", "EXTRA-HEADER-DATA2"};
-        final String url =
-                mTestServer.getURL("/echoheader?" + extraHeaders[0] + "&" + extraHeaders[2]);
-
-        loadUrlWithExtraHeadersSync(awContents, contentsClient.getOnPageFinishedHelper(), url,
-                createHeadersMap(extraHeaders));
-        validateHeadersValue(awContents, contentsClient, extraHeaders, true);
-
-        // Load the same URL again without the extra headers specified.
-        mActivityTestRule.loadUrlSync(awContents, contentsClient.getOnPageFinishedHelper(), url);
-        // Check they're not still there.
-        validateHeadersValue(awContents, contentsClient, extraHeaders, false);
-    }
-
     @Test
     @SmallTest
     @Feature({"AndroidWebView"})
@@ -400,69 +363,6 @@ public class LoadUrlTest {
                 awContents, contentsClient, extraHeaders, echoRedirectedUrlHeader, false);
     }
 
-    @Test
-    @SmallTest
-    @Feature({"AndroidWebView"})
-    public void testCrossOriginRedirectWithExtraHeaders() throws Throwable {
-        final TestAwContentsClient contentsClient = new TestAwContentsClient();
-        final AwTestContainerView testContainerView =
-                mActivityTestRule.createAwTestContainerViewOnMainSync(contentsClient);
-        final AwContents awContents = testContainerView.getAwContents();
-        final String echoRedirectedUrlHeader = "echo header";
-        final String echoInitialUrlHeader = "data content";
-
-        AwActivityTestRule.enableJavaScriptOnUiThread(awContents);
-
-        String[] extraHeaders = {
-                "X-ExtraHeaders1", "extra-header-data1", "x-extraHeaders2", "EXTRA-HEADER-DATA2"};
-        final String redirectedUrl =
-                toDifferentOriginUrl(mTestServer.getURL("/echoheader-and-set-data?header="
-                        + extraHeaders[0] + "&header=" + extraHeaders[2]));
-        final String initialUrl =
-                mTestServer.getURL("/server-redirect-echoheader?url=" + encodeUrl(redirectedUrl)
-                        + "&header=" + extraHeaders[0] + "&header=" + extraHeaders[2]);
-        loadUrlWithExtraHeadersSync(awContents, contentsClient.getOnPageFinishedHelper(),
-                initialUrl, createHeadersMap(extraHeaders));
-        validateHeadersFromJson(
-                awContents, contentsClient, extraHeaders, echoInitialUrlHeader, true);
-        // Check that the headers were removed when the request was redirected to another origin.
-        validateHeadersFromJson(
-                awContents, contentsClient, extraHeaders, echoRedirectedUrlHeader, false);
-    }
-
-    @Test
-    @SmallTest
-    @Feature({"AndroidWebView"})
-    public void testRedirectToPreviousExtraHeaders() throws Throwable {
-        final TestAwContentsClient contentsClient = new TestAwContentsClient();
-        final AwTestContainerView testContainerView =
-                mActivityTestRule.createAwTestContainerViewOnMainSync(contentsClient);
-        final AwContents awContents = testContainerView.getAwContents();
-        final String echoRedirectedUrlHeader = "echo header";
-
-        AwActivityTestRule.enableJavaScriptOnUiThread(awContents);
-
-        String[] extraHeaders = {
-                "X-ExtraHeaders1", "extra-header-data1", "x-extraHeaders2", "EXTRA-HEADER-DATA2"};
-        final String redirectedUrl = mTestServer.getURL("/echoheader-and-set-data?header="
-                + extraHeaders[0] + "&header=" + extraHeaders[2]);
-        final String initialUrl =
-                mTestServer.getURL("/server-redirect-echoheader?url=" + encodeUrl(redirectedUrl));
-
-        // First load the redirect target URL with extra headers
-        loadUrlWithExtraHeadersSync(awContents, contentsClient.getOnPageFinishedHelper(),
-                redirectedUrl, createHeadersMap(extraHeaders));
-        validateHeadersFromJson(
-                awContents, contentsClient, extraHeaders, echoRedirectedUrlHeader, true);
-
-        // Now load the initial URL without any extra headers and let it redirect;
-        // the extra headers should not be added to the redirected request.
-        mActivityTestRule.loadUrlSync(
-                awContents, contentsClient.getOnPageFinishedHelper(), initialUrl);
-        validateHeadersFromJson(
-                awContents, contentsClient, extraHeaders, echoRedirectedUrlHeader, false);
-    }
-
     @Test
     @SmallTest
     @Feature({"AndroidWebView"})