Revert of Enable 'First-Party-Only' cookies by default. (patchset chr…

…omium#3 id:40001 of https://codereview.chromium.org/1032063002/) Reason for revert: Blink OWNERs would like to wait a bit before shipping: https://groups.google.com/a/chromium.org/d/msg/blink-dev/wKZBCzcNssg/nZ1CFmgddJcJ Original issue's description: > Enable 'First-Party-Only' cookies by default. > > This patch reverts https://codereview.chromium.org/940373002, and adds > back in the ~2 lines and tests that are still relevant. > > BUG=459154 > > Committed: https://crrev.com/bdd0b09b5371066ad6d6c0ef2c32f5b92595316d > Cr-Commit-Position: refs/heads/master@{#322533} TBR=cbentzel@chromium.org,jochen@chromium.org,rsleevi@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=459154 Review URL: https://codereview.chromium.org/1043403003 Cr-Commit-Position: refs/heads/master@{#323190}
Denger-Network · Apr 1, 2015 · 0513c9d · 0513c9d
1 parent 38ee68c
commit 0513c9d
Show file tree

Hide file tree

Showing 15 changed files with 170 additions and 2 deletions.
diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/base_paths.h"
+#include "base/command_line.h"
 #include "base/debug/alias.h"
 #include "base/debug/dump_without_crashing.h"
 #include "base/debug/stack_trace.h"
@@ -38,6 +39,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -57,7 +59,6 @@
 #endif
 
 #if defined(OS_CHROMEOS)
-#include "base/command_line.h"
 #include "base/sys_info.h"
 #include "chrome/common/chrome_switches.h"
 #endif
@@ -291,6 +292,9 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
       url_blacklist_manager_(NULL),
 #endif
       domain_reliability_monitor_(NULL),
+      experimental_web_platform_features_enabled_(
+          base::CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kEnableExperimentalWebPlatformFeatures)),
       first_request_(true) {
   DCHECK(enable_referrers);
   extensions_delegate_.reset(
@@ -701,6 +705,10 @@ bool ChromeNetworkDelegate::OnCanEnablePrivacyMode(
   return privacy_mode;
 }
 
+bool ChromeNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
+  return experimental_web_platform_features_enabled_;
+}
+
 bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
     const net::URLRequest& request,
     const GURL& target_url,

diff --git a/chrome/browser/net/chrome_network_delegate.h b/chrome/browser/net/chrome_network_delegate.h
@@ -176,6 +176,7 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
   bool OnCanEnablePrivacyMode(
       const GURL& url,
       const GURL& first_party_for_cookies) const override;
+  bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
   bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
       const net::URLRequest& request,
       const GURL& target_url,
@@ -217,6 +218,8 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
   // static anyway since it is based on a command-line flag.
   static bool g_never_throttle_requests_;
 
+  bool experimental_web_platform_features_enabled_;
+
   bool first_request_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegate);

diff --git a/chrome/browser/net/chrome_network_delegate_unittest.cc b/chrome/browser/net/chrome_network_delegate_unittest.cc
@@ -25,6 +25,36 @@
 #include "chrome/browser/extensions/event_router_forwarder.h"
 #endif
 
+TEST(ChromeNetworkDelegateTest, DisableFirstPartyOnlyCookiesIffFlagDisabled) {
+  BooleanPrefMember pref_member_;
+  scoped_ptr<ChromeNetworkDelegate> delegate;
+
+#if defined(ENABLE_EXTENSIONS)
+  scoped_refptr<extensions::EventRouterForwarder> forwarder =
+      new extensions::EventRouterForwarder();
+  delegate.reset(new ChromeNetworkDelegate(forwarder.get(), &pref_member_));
+#else
+  delegate.reset(new ChromeNetworkDelegate(nullptr, &pref_member_));
+#endif
+  EXPECT_FALSE(delegate->FirstPartyOnlyCookieExperimentEnabled());
+}
+
+TEST(ChromeNetworkDelegateTest, EnableFirstPartyOnlyCookiesIffFlagEnabled) {
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+  BooleanPrefMember pref_member_;
+  scoped_ptr<ChromeNetworkDelegate> delegate;
+
+#if defined(ENABLE_EXTENSIONS)
+  scoped_refptr<extensions::EventRouterForwarder> forwarder =
+      new extensions::EventRouterForwarder();
+  delegate.reset(new ChromeNetworkDelegate(forwarder.get(), &pref_member_));
+#else
+  delegate.reset(new ChromeNetworkDelegate(nullptr, &pref_member_));
+#endif
+  EXPECT_TRUE(delegate->FirstPartyOnlyCookieExperimentEnabled());
+}
+
 #if defined(ENABLE_EXTENSIONS)
 class ChromeNetworkDelegateThrottlingTest : public testing::Test {
  protected:

diff --git a/content/shell/browser/shell_network_delegate.cc b/content/shell/browser/shell_network_delegate.cc
@@ -4,6 +4,8 @@
 
 #include "content/shell/browser/shell_network_delegate.h"
 
+#include "base/command_line.h"
+#include "content/public/common/content_switches.h"
 #include "net/base/net_errors.h"
 #include "net/base/static_cookie_policy.h"
 #include "net/url_request/url_request.h"
@@ -114,4 +116,9 @@ bool ShellNetworkDelegate::OnCanThrottleRequest(
   return false;
 }
 
+bool ShellNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+}
+
 }  // namespace content
diff --git a/content/shell/browser/shell_network_delegate.h b/content/shell/browser/shell_network_delegate.h
@@ -54,6 +54,7 @@ class ShellNetworkDelegate : public net::NetworkDelegateImpl {
   bool OnCanAccessFile(const net::URLRequest& request,
                        const base::FilePath& path) const override;
   bool OnCanThrottleRequest(const net::URLRequest& request) const override;
+  bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
 
   DISALLOW_COPY_AND_ASSIGN(ShellNetworkDelegate);
 };

diff --git a/net/base/layered_network_delegate.cc b/net/base/layered_network_delegate.cc
@@ -252,6 +252,15 @@ void LayeredNetworkDelegate::OnCanEnablePrivacyModeInternal(
     const GURL& first_party_for_cookies) const {
 }
 
+bool LayeredNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
+  OnFirstPartyOnlyCookieExperimentEnabledInternal();
+  return nested_network_delegate_->FirstPartyOnlyCookieExperimentEnabled();
+}
+
+void LayeredNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabledInternal()
+    const {
+}
+
 bool LayeredNetworkDelegate::
     OnCancelURLRequestWithPolicyViolatingReferrerHeader(
         const URLRequest& request,

diff --git a/net/base/layered_network_delegate.h b/net/base/layered_network_delegate.h
@@ -80,6 +80,7 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
   bool OnCanThrottleRequest(const URLRequest& request) const final;
   bool OnCanEnablePrivacyMode(const GURL& url,
                               const GURL& first_party_for_cookies) const final;
+  bool OnFirstPartyOnlyCookieExperimentEnabled() const final;
   bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
       const URLRequest& request,
       const GURL& target_url,
@@ -152,6 +153,8 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
       const GURL& url,
       const GURL& first_party_for_cookies) const;
 
+  virtual void OnFirstPartyOnlyCookieExperimentEnabledInternal() const;
+
   virtual void OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
       const URLRequest& request,
       const GURL& target_url,

diff --git a/net/base/network_delegate.cc b/net/base/network_delegate.cc
@@ -229,6 +229,10 @@ bool NetworkDelegate::CanEnablePrivacyMode(
   return OnCanEnablePrivacyMode(url, first_party_for_cookies);
 }
 
+bool NetworkDelegate::FirstPartyOnlyCookieExperimentEnabled() const {
+  return OnFirstPartyOnlyCookieExperimentEnabled();
+}
+
 bool NetworkDelegate::CancelURLRequestWithPolicyViolatingReferrerHeader(
     const URLRequest& request,
     const GURL& target_url,

diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h
@@ -104,6 +104,10 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
   bool CanEnablePrivacyMode(const GURL& url,
                             const GURL& first_party_for_cookies) const;
 
+  // TODO(mkwst): Remove this once we decide whether or not we wish to ship
+  // first-party cookies. https://crbug.com/459154
+  bool FirstPartyOnlyCookieExperimentEnabled() const;
+
   bool CancelURLRequestWithPolicyViolatingReferrerHeader(
       const URLRequest& request,
       const GURL& target_url,
@@ -266,6 +270,13 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
       const GURL& url,
       const GURL& first_party_for_cookies) const = 0;
 
+  // Returns true if the embedder has enabled the "first-party" cookie
+  // experiment, and false otherwise.
+  //
+  // TODO(mkwst): Remove this once we decide whether or not we wish to ship
+  // first-party cookies. https://crbug.com/459154
+  virtual bool OnFirstPartyOnlyCookieExperimentEnabled() const = 0;
+
   // Called when the |referrer_url| for requesting |target_url| during handling
   // of the |request| is does not comply with the referrer policy (e.g. a
   // secure referrer for an insecure initial target).

diff --git a/net/base/network_delegate_impl.cc b/net/base/network_delegate_impl.cc
@@ -105,6 +105,10 @@ bool NetworkDelegateImpl::OnCanEnablePrivacyMode(
   return false;
 }
 
+bool NetworkDelegateImpl::OnFirstPartyOnlyCookieExperimentEnabled() const {
+  return false;
+}
+
 bool NetworkDelegateImpl::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
     const URLRequest& request,
     const GURL& target_url,

diff --git a/net/base/network_delegate_impl.h b/net/base/network_delegate_impl.h
@@ -184,6 +184,13 @@ class NET_EXPORT NetworkDelegateImpl : public NetworkDelegate {
       const GURL& url,
       const GURL& first_party_for_cookies) const override;
 
+  // Returns true if the embedder has enabled the "first-party" cookie
+  // experiment, and false otherwise.
+  //
+  // TODO(mkwst): Remove this once we decide whether or not we wish to ship
+  // first-party cookies. https://crbug.com/459154
+  bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
+
   // Called when the |referrer_url| for requesting |target_url| during handling
   // of the |request| is does not comply with the referrer policy (e.g. a
   // secure referrer for an insecure initial target).

diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
@@ -615,7 +615,14 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() {
 void URLRequestHttpJob::DoLoadCookies() {
   CookieOptions options;
   options.set_include_httponly();
-  options.set_first_party_url(request_->first_party_for_cookies());
+
+  // TODO(mkwst): Drop this `if` once we decide whether or not to ship
+  // first-party cookies: https://crbug.com/459154
+  if (network_delegate() &&
+      network_delegate()->FirstPartyOnlyCookieExperimentEnabled())
+    options.set_first_party_url(request_->first_party_for_cookies());
+  else
+    options.set_include_first_party_only();
 
   request_->context()->cookie_store()->GetCookiesWithOptionsAsync(
       request_->url(), options, base::Bind(&URLRequestHttpJob::OnCookiesLoaded,

diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc
@@ -322,6 +322,7 @@ TestNetworkDelegate::TestNetworkDelegate()
       has_load_timing_info_before_auth_(false),
       can_access_files_(true),
       can_throttle_requests_(true),
+      first_party_only_cookies_enabled_(false),
       cancel_request_with_policy_violating_referrer_(false),
       will_be_intercepted_on_next_error_(false) {
 }
@@ -603,6 +604,10 @@ bool TestNetworkDelegate::OnCanThrottleRequest(
   return can_throttle_requests_;
 }
 
+bool TestNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
+  return first_party_only_cookies_enabled_;
+}
+
 bool TestNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
     const URLRequest& request,
     const GURL& target_url,

diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h
@@ -265,6 +265,10 @@ class TestNetworkDelegate : public NetworkDelegateImpl {
   void set_can_access_files(bool val) { can_access_files_ = val; }
   bool can_access_files() const { return can_access_files_; }
 
+  void set_first_party_only_cookies_enabled(bool val) {
+    first_party_only_cookies_enabled_ = val;
+  }
+
   void set_can_throttle_requests(bool val) { can_throttle_requests_ = val; }
   bool can_throttle_requests() const { return can_throttle_requests_; }
 
@@ -325,6 +329,7 @@ class TestNetworkDelegate : public NetworkDelegateImpl {
   bool OnCanAccessFile(const URLRequest& request,
                        const base::FilePath& path) const override;
   bool OnCanThrottleRequest(const URLRequest& request) const override;
+  bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
   bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
       const URLRequest& request,
       const GURL& target_url,
@@ -370,6 +375,7 @@ class TestNetworkDelegate : public NetworkDelegateImpl {
 
   bool can_access_files_;  // true by default
   bool can_throttle_requests_;  // true by default
+  bool first_party_only_cookies_enabled_;               // false by default
   bool cancel_request_with_policy_violating_referrer_;  // false by default
   bool will_be_intercepted_on_next_error_;
 };

diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
@@ -2551,6 +2551,7 @@ TEST_F(URLRequestTest, FirstPartyOnlyCookiesEnabled) {
   // LocalHttpTestServer points).
   {
     TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(true);
     default_context_.set_network_delegate(&network_delegate);
 
     TestDelegate d;
@@ -2568,6 +2569,7 @@ TEST_F(URLRequestTest, FirstPartyOnlyCookiesEnabled) {
   // Verify that the cookie is sent for first-party requests.
   {
     TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(true);
     default_context_.set_network_delegate(&network_delegate);
     TestDelegate d;
     scoped_ptr<URLRequest> req(default_context_.CreateRequest(
@@ -2585,6 +2587,7 @@ TEST_F(URLRequestTest, FirstPartyOnlyCookiesEnabled) {
   // Verify that the cookie is not-sent for non-first-party requests.
   {
     TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(true);
     default_context_.set_network_delegate(&network_delegate);
     TestDelegate d;
     scoped_ptr<URLRequest> req(default_context_.CreateRequest(
@@ -2600,6 +2603,66 @@ TEST_F(URLRequestTest, FirstPartyOnlyCookiesEnabled) {
   }
 }
 
+TEST_F(URLRequestTest, FirstPartyOnlyCookiesDisabled) {
+  LocalHttpTestServer test_server;
+  ASSERT_TRUE(test_server.Start());
+
+  // Set up a 'First-Party-Only' cookie (on '127.0.0.1', as that's where
+  // LocalHttpTestServer points).
+  {
+    TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(false);
+    default_context_.set_network_delegate(&network_delegate);
+
+    TestDelegate d;
+    scoped_ptr<URLRequest> req(default_context_.CreateRequest(
+        test_server.GetURL(
+            "set-cookie?FirstPartyCookieToSet=1;First-Party-Only"),
+        DEFAULT_PRIORITY, &d));
+    req->Start();
+    base::RunLoop().Run();
+    EXPECT_EQ(0, network_delegate.blocked_get_cookies_count());
+    EXPECT_EQ(0, network_delegate.blocked_set_cookie_count());
+    EXPECT_EQ(1, network_delegate.set_cookie_count());
+  }
+
+  // Verify that the cookie is sent for first-party requests.
+  {
+    TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(false);
+    default_context_.set_network_delegate(&network_delegate);
+    TestDelegate d;
+    scoped_ptr<URLRequest> req(default_context_.CreateRequest(
+        test_server.GetURL("echoheader?Cookie"), DEFAULT_PRIORITY, &d));
+    req->set_first_party_for_cookies(test_server.GetURL(""));
+    req->Start();
+    base::RunLoop().Run();
+
+    EXPECT_TRUE(d.data_received().find("FirstPartyCookieToSet=1") !=
+                std::string::npos);
+    EXPECT_EQ(0, network_delegate.blocked_get_cookies_count());
+    EXPECT_EQ(0, network_delegate.blocked_set_cookie_count());
+  }
+
+  // Verify that the cookie is also sent for non-first-party requests.
+  {
+    TestNetworkDelegate network_delegate;
+    network_delegate.set_first_party_only_cookies_enabled(false);
+    default_context_.set_network_delegate(&network_delegate);
+    TestDelegate d;
+    scoped_ptr<URLRequest> req(default_context_.CreateRequest(
+        test_server.GetURL("echoheader?Cookie"), DEFAULT_PRIORITY, &d));
+    req->set_first_party_for_cookies(GURL("http://third-party.test/"));
+    req->Start();
+    base::RunLoop().Run();
+
+    EXPECT_TRUE(d.data_received().find("FirstPartyCookieToSet=1") !=
+                std::string::npos);
+    EXPECT_EQ(0, network_delegate.blocked_get_cookies_count());
+    EXPECT_EQ(0, network_delegate.blocked_set_cookie_count());
+  }
+}
+
 // FixedDateNetworkDelegate swaps out the server's HTTP Date response header
 // value for the |fixed_date| argument given to the constructor.
 class FixedDateNetworkDelegate : public TestNetworkDelegate {