AW NS: correctly implement cookie blocking for HTTP

Although we previously implemented cookie blocking in
http://crrev.com/c/1507286, this design did not correctly handle the
case where the application has decided to block only third party cookies
and the request redirects (such that it changes first-party-ness).

After the previous CL landed, we realized the design to use load_flags
had a significant consequence: this can subtly affect HTTP
authentication, since the load_flags opt the request into "privacy
mode", and net layer may pool this into a socket with other requests in
privacy mode. This might lead to putting requests we don't trust with
credentials into an already-authenticated socket (which would have
security consequences).

This mostly reverts the previous CL (because of the aforementioned
problems with load_flags) and instead propagates WebView's cookie policy
into the NetworkService via URLLoader options (and, new
WebSocket options). The NetworkServiceNetworkDelegate checks these
settings and either blocks or allows cookies.

Design: http://go/wv-ns-cookie-apis#heading=h.2h285wvuvqal
Bug: 941337, 941260
Test: $ run_webview_instrumentation_test_apk \
Test: --enable-features=NetworkService,NetworkServiceInProcess \
Test: -f CookieManagerTest.*
Test: $ out/Default/services_unittests --gtest_filter=URLLoaderTest.* \
Test: :NetworkContextTest.*Cookies
Cq-Include-Trybots: luci.chromium.try:android_mojo
Change-Id: I533886347441ae369b925574f344dd65801509e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1542726
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#646986}

android_webview/browser/aw_content_browser_client.cc

@@ -983,6 +983,26 @@ bool AwContentBrowserClient::WillCreateURLLoaderFactory(
   return true;
 }
 
+void AwContentBrowserClient::WillCreateWebSocket(
+    content::RenderFrameHost* frame,
+    network::mojom::WebSocketRequest* request,
+    network::mojom::AuthenticationHandlerPtr* auth_handler,
+    network::mojom::TrustedHeaderClientPtr* header_client,
+    uint32_t* options) {
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderFrameHost(frame);
+  AwContents* aw_contents = AwContents::FromWebContents(web_contents);
+
+  bool global_cookie_policy =
+      AwCookieAccessPolicy::GetInstance()->GetShouldAcceptCookies();
+  bool third_party_cookie_policy = aw_contents->AllowThirdPartyCookies();
+  if (!global_cookie_policy) {
+    *options |= network::mojom::kWebSocketOptionBlockAllCookies;
+  } else if (!third_party_cookie_policy) {
+    *options |= network::mojom::kWebSocketOptionBlockThirdPartyCookies;
+  }
+}
+
 std::string AwContentBrowserClient::GetProduct() const {
   return android_webview::GetProduct();
 }

android_webview/browser/aw_content_browser_client.h

@@ -227,6 +227,12 @@ class AwContentBrowserClient : public content::ContentBrowserClient {
       network::mojom::URLLoaderFactoryRequest* factory_request,
       network::mojom::TrustedURLLoaderHeaderClientPtrInfo* header_client,
       bool* bypass_redirect_checks) override;
+  void WillCreateWebSocket(
+      content::RenderFrameHost* frame,
+      network::mojom::WebSocketRequest* request,
+      network::mojom::AuthenticationHandlerPtr* authentication_handler,
+      network::mojom::TrustedHeaderClientPtr* header_client,
+      uint32_t* options) override;
   std::string GetProduct() const override;
   std::string GetUserAgent() const override;
   ContentBrowserClient::WideColorGamutHeuristic GetWideColorGamutHeuristic()

android_webview/browser/aw_contents.cc

@@ -793,6 +793,12 @@ FindHelper* AwContents::GetFindHelper() {
   return find_helper_.get();
 }
 
+bool AwContents::AllowThirdPartyCookies() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  AwSettings* aw_settings = AwSettings::FromWebContents(web_contents_.get());
+  return aw_settings->GetAllowThirdPartyCookies();
+}
+
 void AwContents::OnFindResultReceived(int active_ordinal,
                                       int match_count,
                                       bool finished) {

android_webview/browser/aw_cookie_access_policy.cc

@@ -13,7 +13,6 @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/websocket_handshake_request_info.h"
 #include "net/base/net_errors.h"
-#include "services/network/public/cpp/resource_request.h"
 
 using base::AutoLock;
 using content::BrowserThread;
@@ -48,21 +47,6 @@ void AwCookieAccessPolicy::SetShouldAcceptCookies(bool allow) {
   accept_cookies_ = allow;
 }
 
-bool AwCookieAccessPolicy::ShouldAllowCookiesForRequest(
-    const network::ResourceRequest& request,
-    int process_id) {
-  // process_id == 0 means the render_frame_id is actually a valid
-  // frame_tree_node_id, otherwise use it as a valid render_frame_id.
-  int frame_tree_node_id = process_id
-                               ? content::RenderFrameHost::kNoFrameTreeNodeId
-                               : request.render_frame_id;
-  bool global = GetShouldAcceptCookies();
-  bool third_party = GetShouldAcceptThirdPartyCookies(
-      process_id, request.render_frame_id, frame_tree_node_id);
-  return AwStaticCookiePolicy(global, third_party)
-      .AllowGet(request.url, request.site_for_cookies);
-}
-
 bool AwCookieAccessPolicy::GetShouldAcceptThirdPartyCookies(
     int render_process_id,
     int render_frame_id,

android_webview/browser/aw_cookie_access_policy.h

@@ -16,10 +16,6 @@ namespace content {
 class ResourceContext;
 }
 
-namespace network {
-struct ResourceRequest;
-}
-
 class GURL;
 
 namespace android_webview {
@@ -35,10 +31,6 @@ class AwCookieAccessPolicy {
   bool GetShouldAcceptCookies();
   void SetShouldAcceptCookies(bool allow);
 
-  // Should cookies be readable/writable for |request|?
-  bool ShouldAllowCookiesForRequest(const network::ResourceRequest& request,
-                                    int process_id);
-
   // Can we read/write third party cookies?
   // |render_process_id| and |render_frame_id| must be valid.
   // Navigation requests are not associated with a renderer process. In this

android_webview/browser/aw_settings.cc

@@ -71,6 +71,7 @@ AwSettings::AwSettings(JNIEnv* env,
     : WebContentsObserver(web_contents),
       renderer_prefs_initialized_(false),
       javascript_can_open_windows_automatically_(false),
+      allow_third_party_cookies_(false),
       aw_settings_(env, obj) {
   web_contents->SetUserData(kAwSettingsUserDataKey,
                             std::make_unique<AwSettingsUserData>(this));
@@ -93,6 +94,10 @@ bool AwSettings::GetJavaScriptCanOpenWindowsAutomatically() {
   return javascript_can_open_windows_automatically_;
 }
 
+bool AwSettings::GetAllowThirdPartyCookies() {
+  return allow_third_party_cookies_;
+}
+
 void AwSettings::Destroy(JNIEnv* env, const JavaParamRef<jobject>& obj) {
   delete this;
 }
@@ -143,6 +148,7 @@ void AwSettings::UpdateEverythingLocked(JNIEnv* env,
   UpdateRendererPreferencesLocked(env, obj);
   UpdateOffscreenPreRasterLocked(env, obj);
   UpdateWillSuppressErrorStateLocked(env, obj);
+  UpdateCookiePolicyLocked(env, obj);
 }
 
 void AwSettings::UpdateUserAgentLocked(JNIEnv* env,
@@ -261,6 +267,15 @@ void AwSettings::UpdateRendererPreferencesLocked(
   }
 }
 
+void AwSettings::UpdateCookiePolicyLocked(JNIEnv* env,
+                                          const JavaParamRef<jobject>& obj) {
+  if (!web_contents())
+    return;
+
+  allow_third_party_cookies_ =
+      Java_AwSettings_getAcceptThirdPartyCookiesLocked(env, obj);
+}
+
 void AwSettings::UpdateOffscreenPreRasterLocked(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {

android_webview/browser/aw_settings.h

@@ -35,6 +35,7 @@ class AwSettings : public content::WebContentsObserver {
   ~AwSettings() override;
 
   bool GetJavaScriptCanOpenWindowsAutomatically();
+  bool GetAllowThirdPartyCookies();
 
   // Called from Java. Methods with "Locked" suffix require that the settings
   // access lock is held during their execution.
@@ -65,6 +66,9 @@ class AwSettings : public content::WebContentsObserver {
   void UpdateRendererPreferencesLocked(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
+  void UpdateCookiePolicyLocked(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj);
   void UpdateOffscreenPreRasterLocked(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
@@ -82,6 +86,7 @@ class AwSettings : public content::WebContentsObserver {
 
   bool renderer_prefs_initialized_;
   bool javascript_can_open_windows_automatically_;
+  bool allow_third_party_cookies_;
 
   JavaObjectWeakGlobalRef aw_settings_;
 };

android_webview/browser/network_service/aw_proxying_url_loader_factory.cc

@@ -292,12 +292,6 @@ void InterceptedRequest::Restart() {
 
   request_.load_flags =
       UpdateLoadFlags(request_.load_flags, io_thread_client.get());
-  if (!AwCookieAccessPolicy::GetInstance()->ShouldAllowCookiesForRequest(
-          request_, process_id_)) {
-    request_.load_flags |= net::LOAD_DO_NOT_SAVE_COOKIES;
-    request_.load_flags |= net::LOAD_DO_NOT_SEND_COOKIES;
-  }
-
   if (ShouldNotInterceptRequest()) {
     // equivalent to no interception
     InterceptResponseReceived(nullptr);
@@ -759,6 +753,25 @@ void AwProxyingURLLoaderFactory::CreateLoaderAndStart(
   network::mojom::URLLoaderFactoryPtr target_factory_clone;
   target_factory_->Clone(MakeRequest(&target_factory_clone));
 
+  bool global_cookie_policy =
+      AwCookieAccessPolicy::GetInstance()->GetShouldAcceptCookies();
+  // process_id == 0 means the render_frame_id is actually a valid
+  // frame_tree_node_id, otherwise use it as a valid render_frame_id.
+  int frame_tree_node_id = process_id_
+                               ? content::RenderFrameHost::kNoFrameTreeNodeId
+                               : request.render_frame_id;
+  bool third_party_cookie_policy =
+      AwCookieAccessPolicy::GetInstance()->GetShouldAcceptThirdPartyCookies(
+          process_id_, request.render_frame_id, frame_tree_node_id);
+  if (!global_cookie_policy) {
+    options |= network::mojom::kURLLoadOptionBlockAllCookies;
+  } else if (!third_party_cookie_policy && !request.url.SchemeIsFile()) {
+    // Special case: if the application has asked that we allow file:// scheme
+    // URLs to set cookies, we need to avoid setting a cookie policy (as file://
+    // scheme URLs are third-party to everything).
+    options |= network::mojom::kURLLoadOptionBlockThirdPartyCookies;
+  }
+
   // manages its own lifecycle
   // TODO(timvolodine): consider keeping track of requests.
   InterceptedRequest* req = new InterceptedRequest(

android_webview/java/src/org/chromium/android_webview/AwSettings.java

@@ -226,6 +226,10 @@ void maybePostOnUiThread(Runnable r) {
         void updateWebkitPreferencesLocked() {
             runOnUiThreadBlockingAndLocked(() -> updateWebkitPreferencesOnUiThreadLocked());
         }
+
+        void updateCookiePolicyLocked() {
+            runOnUiThreadBlockingAndLocked(() -> updateCookiePolicyOnUiThreadLocked());
+        }
     }
 
     interface ZoomSupportChangeListener {
@@ -353,6 +357,7 @@ public void setAcceptThirdPartyCookies(boolean accept) {
         if (TRACE) Log.i(LOGTAG, "setAcceptThirdPartyCookies=" + accept);
         synchronized (mAwSettingsLock) {
             mAcceptThirdPartyCookies = accept;
+            mEventHandler.updateCookiePolicyLocked();
         }
     }
 
@@ -376,6 +381,12 @@ public boolean getAcceptThirdPartyCookies() {
         }
     }
 
+    @CalledByNative
+    private boolean getAcceptThirdPartyCookiesLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
+        return mAcceptThirdPartyCookies;
+    }
+
     /**
      * Return whether Safe Browsing has been enabled for the current WebView
      * @return true if SafeBrowsing is enabled
@@ -1827,6 +1838,14 @@ private void updateWebkitPreferencesOnUiThreadLocked() {
         }
     }
 
+    private void updateCookiePolicyOnUiThreadLocked() {
+        assert mEventHandler.mHandler != null;
+        ThreadUtils.assertOnUiThread();
+        if (mNativeAwSettings != 0) {
+            nativeUpdateCookiePolicyLocked(mNativeAwSettings);
+        }
+    }
+
     private native long nativeInit(WebContents webContents);
 
     private native void nativeDestroy(long nativeAwSettings);
@@ -1852,4 +1871,6 @@ private void updateWebkitPreferencesOnUiThreadLocked() {
     private native void nativeUpdateOffscreenPreRasterLocked(long nativeAwSettings);
 
     private native void nativeUpdateWillSuppressErrorStateLocked(long nativeAwSettings);
+
+    private native void nativeUpdateCookiePolicyLocked(long nativeAwSettings);
 }

chrome/browser/chrome_content_browser_client.cc

@@ -4969,7 +4969,8 @@ void ChromeContentBrowserClient::WillCreateWebSocket(
     content::RenderFrameHost* frame,
     network::mojom::WebSocketRequest* request,
     network::mojom::AuthenticationHandlerPtr* auth_handler,
-    network::mojom::TrustedHeaderClientPtr* header_client) {
+    network::mojom::TrustedHeaderClientPtr* header_client,
+    uint32_t* options) {
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   auto* web_request_api =
       extensions::BrowserContextKeyedAPIFactory<extensions::WebRequestAPI>::Get(

chrome/browser/chrome_content_browser_client.h

@@ -490,7 +490,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       content::RenderFrameHost* frame,
       network::mojom::WebSocketRequest* request,
       network::mojom::AuthenticationHandlerPtr* auth_handler,
-      network::mojom::TrustedHeaderClientPtr* header_client) override;
+      network::mojom::TrustedHeaderClientPtr* header_client,
+      uint32_t* options) override;
   void OnNetworkServiceCreated(
       network::mojom::NetworkService* network_service) override;
   network::mojom::NetworkContextPtr CreateNetworkContext(

chrome/browser/extensions/api/web_request/web_request_apitest.cc

@@ -1559,6 +1559,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
       ->CreateWebSocket(std::move(request), network::mojom::kBrowserProcessId,
                         host->GetProcess()->GetID(),
                         url::Origin::Create(GURL("http://example.com")),
+                        network::mojom::kWebSocketOptionNone,
                         std::move(auth_handler), nullptr);
   web_socket.reset();
 }

content/browser/frame_host/render_frame_host_impl.cc

@@ -5670,14 +5670,15 @@ void RenderFrameHostImpl::CreateWebSocket(
   network::mojom::AuthenticationHandlerPtr auth_handler;
 
   network::mojom::TrustedHeaderClientPtr header_client;
+  uint32_t options = network::mojom::kWebSocketOptionNone;
   GetContentClient()->browser()->WillCreateWebSocket(
-      this, &request, &auth_handler, &header_client);
+      this, &request, &auth_handler, &header_client, &options);
 
   // This is to support usage of WebSockets in cases in which there is an
   // associated RenderFrame. This is important for showing the correct security
   // state of the page and also honoring user override of bad certificates.
   WebSocketManager::CreateWebSocket(
-      process_->GetID(), routing_id_, last_committed_origin_,
+      process_->GetID(), routing_id_, last_committed_origin_, options,
       std::move(auth_handler), std::move(header_client), std::move(request));
 }
 

content/browser/renderer_interface_binders.cc

@@ -218,6 +218,7 @@ void RendererInterfaceBinders::CreateWebSocket(
   // TODO(jam): is it ok to not send extraHeaders for sockets created from
   // shared and service workers?
   WebSocketManager::CreateWebSocket(host->GetID(), MSG_ROUTING_NONE, origin,
+                                    network::mojom::kWebSocketOptionNone,
                                     nullptr, nullptr, std::move(request));
 }
 

content/browser/websockets/websocket_manager.cc

@@ -166,6 +166,7 @@ void WebSocketManager::CreateWebSocket(
     int process_id,
     int frame_id,
     url::Origin origin,
+    uint32_t options,
     network::mojom::AuthenticationHandlerPtr auth_handler,
     network::mojom::TrustedHeaderClientPtr header_client,
     network::mojom::WebSocketRequest request) {
@@ -179,7 +180,7 @@ void WebSocketManager::CreateWebSocket(
     network::mojom::NetworkContext* network_context =
         storage_partition->GetNetworkContext();
     network_context->CreateWebSocket(std::move(request), process_id, frame_id,
-                                     origin, std::move(auth_handler),
+                                     origin, options, std::move(auth_handler),
                                      std::move(header_client));
     return;
   }
@@ -207,7 +208,7 @@ void WebSocketManager::CreateWebSocket(
       FROM_HERE, {BrowserThread::IO},
       base::BindOnce(&WebSocketManager::DoCreateWebSocket,
                      base::Unretained(handle->manager()), frame_id,
-                     std::move(origin), std::move(request)));
+                     std::move(origin), options, std::move(request)));
 }
 
 WebSocketManager::WebSocketManager(int process_id,
@@ -240,6 +241,7 @@ WebSocketManager::~WebSocketManager() {
 void WebSocketManager::DoCreateWebSocket(
     int frame_id,
     url::Origin origin,
+    uint32_t options,
     network::mojom::WebSocketRequest request) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ -264,7 +266,7 @@ void WebSocketManager::DoCreateWebSocket(
   impls_.insert(DoCreateWebSocketInternal(
       std::make_unique<Delegate>(this), std::move(request),
       throttler_.IssuePendingConnectionTracker(), process_id_, frame_id,
-      std::move(origin), throttler_.CalculateDelay()));
+      std::move(origin), options, throttler_.CalculateDelay()));
 
   if (!throttling_period_timer_.IsRunning()) {
     throttling_period_timer_.Start(
@@ -288,11 +290,12 @@ std::unique_ptr<network::WebSocket> WebSocketManager::DoCreateWebSocketInternal(
     int child_id,
     int frame_id,
     url::Origin origin,
+    uint32_t options,
     base::TimeDelta delay) {
   return std::make_unique<network::WebSocket>(
       std::move(delegate), std::move(request), nullptr, nullptr,
       std::move(pending_connection_tracker), child_id, frame_id,
-      std::move(origin), delay);
+      std::move(origin), options, delay);
 }
 
 net::URLRequestContext* WebSocketManager::GetURLRequestContext() {