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

Reland the change to extra header handling with the actual behaviour
change behind a disabled-by-default base::Feature. UMA metrics are
collected even when the feature is disabled, simulating what would have
happened had the feature been enabled, so we can better gauge the
compatibility impact on applications.

Bug: 1038002
Change-Id: I3d722060cc4526a14adac076bbf4eec0a0992b70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2229420
Commit-Queue: Richard Coles <torne@chromium.org>
Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775115}

android_webview/browser/network_service/aw_url_loader_throttle.cc

@@ -5,10 +5,32 @@
 #include "android_webview/browser/network_service/aw_url_loader_throttle.h"
 
 #include "android_webview/browser/aw_resource_context.h"
-#include "net/http/http_response_headers.h"
+#include "android_webview/common/aw_features.h"
+#include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/strings/string_util.h"
+#include "net/http/http_request_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) {}
 
@@ -17,6 +39,10 @@ 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(
@@ -26,7 +52,33 @@ void AwURLLoaderThrottle::WillRedirectRequest(
     std::vector<std::string>* to_be_removed_request_headers,
     net::HttpRequestHeaders* modified_request_headers,
     net::HttpRequestHeaders* modified_cors_exempt_request_headers) {
-  AddExtraHeadersIfNeeded(redirect_info->new_url, modified_request_headers);
+  bool same_origin_only = base::FeatureList::IsEnabled(
+      features::kWebViewExtraHeadersSameOriginOnly);
+
+  if (!added_headers_.empty()) {
+    if (original_origin_.CanBeDerivedFrom(redirect_info->new_url)) {
+      RecordExtraHeadersUMA(ExtraHeaders::kKeptOnSameOriginRedirect);
+    } else {
+      // Cross-origin redirect. Remove the headers we added if the feature is
+      // enabled. added_headers_ is still cleared either way so that the metrics
+      // will reflect what would have happened if the feature was enabled.
+      if (same_origin_only) {
+        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);
+    }
+  }
+
+  if (!same_origin_only) {
+    // The original behaviour added more headers if the redirect target had
+    // previously been loaded with extra headers; this is weird/surprising, so
+    // it's skipped when the feature is enabled.
+    AddExtraHeadersIfNeeded(redirect_info->new_url, modified_request_headers);
+  }
 }
 
 void AwURLLoaderThrottle::AddExtraHeadersIfNeeded(
@@ -38,8 +90,13 @@ void AwURLLoaderThrottle::AddExtraHeadersIfNeeded(
 
   net::HttpRequestHeaders temp_headers;
   temp_headers.AddHeadersFromString(extra_headers);
-  for (net::HttpRequestHeaders::Iterator it(temp_headers); it.GetNext();)
-    headers->SetHeaderIfMissing(it.name(), it.value());
+  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());
+  }
 }
 
 }  // namespace android_webview

android_webview/browser/network_service/aw_url_loader_throttle.h

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "third_party/blink/public/common/loader/url_loader_throttle.h"
+#include "url/origin.h"
 
 class GURL;
 
@@ -41,6 +42,8 @@ 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/common/aw_features.cc

@@ -23,6 +23,11 @@ const base::Feature kWebViewCollectNonembeddedMetrics{
 const base::Feature kWebViewConnectionlessSafeBrowsing{
     "WebViewConnectionlessSafeBrowsing", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Only allow extra headers added via loadUrl() to be sent to the original
+// origin; strip them from the request if a cross-origin redirect occurs.
+const base::Feature kWebViewExtraHeadersSameOriginOnly{
+    "WebViewExtraHeadersSameOriginOnly", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Sniff the content stream to guess the MIME type when the application doesn't
 // tell us the MIME type explicitly.
 //

android_webview/common/aw_features.h

@@ -17,6 +17,7 @@ namespace features {
 extern const base::Feature kWebViewBrotliSupport;
 extern const base::Feature kWebViewCollectNonembeddedMetrics;
 extern const base::Feature kWebViewConnectionlessSafeBrowsing;
+extern const base::Feature kWebViewExtraHeadersSameOriginOnly;
 extern const base::Feature kWebViewSniffMimeType;
 extern const base::Feature kWebViewWideColorGamutSupport;
 

android_webview/java/src/org/chromium/android_webview/common/AwFeatures.java

@@ -17,5 +17,8 @@ public final class AwFeatures {
     public static final String WEBVIEW_CONNECTIONLESS_SAFE_BROWSING =
             "WebViewConnectionlessSafeBrowsing";
 
+    public static final String WEBVIEW_EXTRA_HEADERS_SAME_ORIGIN_ONLY =
+            "WebViewExtraHeadersSameOriginOnly";
+
     private AwFeatures() {}
 }

android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java

@@ -68,5 +68,8 @@ public final class ProductionSupportedFlagList {
                     "WebViewBrotliSupport", "Enables brotli compression support in WebView."),
             Flag.baseFeature(
                     "AppCache", "Controls AppCache to facilitate testing against future removal."),
+            Flag.baseFeature(AwFeatures.WEBVIEW_EXTRA_HEADERS_SAME_ORIGIN_ONLY,
+                    "Only allow extra headers added via loadUrl() to be sent to the same origin "
+                            + "as the original request."),
     };
 }

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

@@ -21,9 +21,11 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import org.chromium.android_webview.AwContents;
+import org.chromium.android_webview.common.AwFeatures;
 import org.chromium.android_webview.test.util.CommonResources;
 import org.chromium.android_webview.test.util.JSUtils;
 import org.chromium.base.test.util.CallbackHelper;
+import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Feature;
 import org.chromium.content_public.browser.test.util.HistoryUtils;
 import org.chromium.net.test.util.TestWebServer;
@@ -255,6 +257,19 @@ 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"})
@@ -279,7 +294,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.
+        // Verify that extra headers are passed to the loaded url, but not to the image subresource.
         validateHeadersValue(awContents, contentsClient, extraHeaders, true);
         onReceivedTitleHelper.waitForCallback(onReceivedTitleCallCount);
         Assert.assertEquals("5", onReceivedTitleHelper.getTitle());
@@ -329,6 +344,30 @@ 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"})
@@ -363,6 +402,73 @@ public class LoadUrlTest {
                 awContents, contentsClient, extraHeaders, echoRedirectedUrlHeader, false);
     }
 
+    @Test
+    @SmallTest
+    @Feature({"AndroidWebView"})
+    @CommandLineFlags.Add("enable-features=" + AwFeatures.WEBVIEW_EXTRA_HEADERS_SAME_ORIGIN_ONLY)
+    // TODO(crbug.com/1038002) remove flag when enabled by default
+    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"})
+    @CommandLineFlags.Add("enable-features=" + AwFeatures.WEBVIEW_EXTRA_HEADERS_SAME_ORIGIN_ONLY)
+    // TODO(crbug.com/1038002) remove flag when enabled by default
+    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"})