Flag cookies that would be (but are not yet) blocked due to SameSite
Identify cookies on get/set in URLRequestHttpJob that come up as
CookieInclusionStatus::INCLUDE, but would be blocked if
SameSiteByDefaultCookies or CookiesWithoutSameSiteMustBeSecure were
enabled, and copy them into the list of excluded cookies, so that
appropriate console messages can be displayed about the impending
deprecation.
This only covers get/set via HTTP headers. Get/set via document.cookie
will be handled separately.
Bug: 966576
Change-Id: I7edffc66956743cdad3bee6eb13e342eb3e07ea1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625841
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664955}
diff --git a/net/cookies/cookie_util.cc b/net/cookies/cookie_util.cc
index 209bef9..a9b98ab 100644
--- a/net/cookies/cookie_util.cc
+++ b/net/cookies/cookie_util.cc
@@ -467,6 +467,28 @@
return CookieOptions::SameSiteCookieContext::CROSS_SITE;
}
+CanonicalCookie::CookieInclusionStatus CookieWouldBeExcludedDueToSameSite(
+ const CanonicalCookie& cookie,
+ const CookieOptions& options) {
+ // Check if cookie would be excluded under SameSiteByDefaultCookies.
+ bool cross_site_context = options.same_site_cookie_context() ==
+ CookieOptions::SameSiteCookieContext::CROSS_SITE;
+ if (cross_site_context && cookie.SameSite() == CookieSameSite::UNSPECIFIED) {
+ DCHECK_EQ(CookieSameSite::NO_RESTRICTION, cookie.GetEffectiveSameSite());
+ return CanonicalCookie::CookieInclusionStatus::
+ EXCLUDE_SAMESITE_UNSPECIFIED_TREATED_AS_LAX;
+ }
+
+ // Check if cookie would be excluded under CookiesWithoutSameSiteMustBeSecure.
+ if (cookie.SameSite() == CookieSameSite::NO_RESTRICTION &&
+ !cookie.IsSecure()) {
+ return CanonicalCookie::CookieInclusionStatus::
+ EXCLUDE_SAMESITE_NONE_INSECURE;
+ }
+
+ return CanonicalCookie::CookieInclusionStatus::INCLUDE;
+}
+
base::OnceCallback<void(const CookieList&, const CookieStatusList&)>
IgnoreCookieStatusList(base::OnceCallback<void(const CookieList&)> callback) {
return base::BindOnce(
diff --git a/net/cookies/cookie_util.h b/net/cookies/cookie_util.h
index f07dc3f..89a9c25 100644
--- a/net/cookies/cookie_util.h
+++ b/net/cookies/cookie_util.h
@@ -135,6 +135,18 @@
ComputeSameSiteContextForScriptSet(const GURL& url,
const GURL& site_for_cookies);
+// Checks whether a cookie would be excluded due to SameSite restrictions,
+// assuming SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
+// were turned on. This should be called on a cookie that is in fact included,
+// (presumably because SameSiteByDefaultCookies and
+// CookiesWithoutSameSiteMustBeSecure are not actually enabled). If the
+// return value is not INCLUDE, the cookie should be added to the excluded
+// cookies list so that an appropriate warning message can be shown in the
+// console.
+NET_EXPORT CanonicalCookie::CookieInclusionStatus
+CookieWouldBeExcludedDueToSameSite(const CanonicalCookie& cookie,
+ const CookieOptions& options);
+
// Takes a OnceCallback with only a CookieList and binds it to a callback that
// also accepts a CookieStatusList, making it compatible with
// CookieStore::GetCookieListCallback.
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index f7e8b8f..94883ec 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -636,13 +636,14 @@
cookie_store->GetCookieListWithOptionsAsync(
request_->url(), options,
base::Bind(&URLRequestHttpJob::SetCookieHeaderAndStart,
- weak_factory_.GetWeakPtr()));
+ weak_factory_.GetWeakPtr(), options));
} else {
StartTransaction();
}
}
void URLRequestHttpJob::SetCookieHeaderAndStart(
+ const CookieOptions& options,
const CookieList& cookie_list,
const CookieStatusList& excluded_list) {
DCHECK(request_->not_sent_cookies().empty());
@@ -668,6 +669,25 @@
}
}
+ // Copy any cookies that would not be sent under SameSiteByDefaultCookies
+ // and/or CookiesWithoutSameSiteMustBeSecure, into the |excluded_cookies| list
+ // so that we can display appropriate console warning messages about them.
+ // I.e. they are still included in the Cookie header, but they are *also*
+ // copied into |excluded_cookies| with the CookieInclusionStatus that *would*
+ // apply. This special-casing will go away once SameSiteByDefaultCookies and
+ // CookiesWithoutSameSiteMustBeSecure are on by default, as the affected
+ // cookies will just be excluded in the first place.
+ for (const CanonicalCookie& cookie : cookie_list) {
+ CanonicalCookie::CookieInclusionStatus
+ include_but_maybe_would_exclude_status =
+ cookie_util::CookieWouldBeExcludedDueToSameSite(cookie, options);
+ if (include_but_maybe_would_exclude_status !=
+ CanonicalCookie::CookieInclusionStatus::INCLUDE) {
+ excluded_cookies.push_back(
+ {cookie, include_but_maybe_would_exclude_status});
+ }
+ }
+
request_->set_not_sent_cookies(std::move(excluded_cookies));
StartTransaction();
@@ -734,14 +754,14 @@
&returned_status);
if (returned_status != CanonicalCookie::CookieInclusionStatus::INCLUDE) {
- OnSetCookieResult(base::nullopt, std::move(cookie_string),
+ OnSetCookieResult(options, base::nullopt, std::move(cookie_string),
returned_status);
continue;
}
if (!CanSetCookie(*cookie, &options)) {
OnSetCookieResult(
- base::make_optional<CanonicalCookie>(*cookie),
+ options, base::make_optional<CanonicalCookie>(*cookie),
std::move(cookie_string),
CanonicalCookie::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES);
continue;
@@ -749,9 +769,10 @@
request_->context()->cookie_store()->SetCookieWithOptionsAsync(
request_->url(), cookie_string, options,
- base::BindOnce(
- &URLRequestHttpJob::OnSetCookieResult, weak_factory_.GetWeakPtr(),
- base::make_optional<CanonicalCookie>(*cookie), cookie_string));
+ base::BindOnce(&URLRequestHttpJob::OnSetCookieResult,
+ weak_factory_.GetWeakPtr(), options,
+ base::make_optional<CanonicalCookie>(*cookie),
+ cookie_string));
}
// Removing the 1 that |num_cookie_lines_left| started with, signifing that
// loop has been exited.
@@ -762,12 +783,32 @@
}
void URLRequestHttpJob::OnSetCookieResult(
+ const CookieOptions& options,
base::Optional<CanonicalCookie> cookie,
std::string cookie_string,
CanonicalCookie::CookieInclusionStatus status) {
if (status != CanonicalCookie::CookieInclusionStatus::INCLUDE) {
cs_status_list_.emplace_back(std::move(cookie), std::move(cookie_string),
status);
+ } else {
+ DCHECK(cookie.has_value());
+ // Even if the status is INCLUDE, copy any cookies that would not be set
+ // under SameSiteByDefaultCookies and/or CookiesWithoutSameSiteMustBeSecure
+ // into |cs_status_list_| so that we can display appropriate console warning
+ // messages about them. I.e. they are still set, but they are *also* copied
+ // into |cs_status_list_| with the CookieInclusionStatus that *would* apply.
+ // This special-casing will go away once SameSiteByDefaultCookies and
+ // CookiesWithoutSameSiteMustBeSecure are on by default, as the affected
+ // cookies will just be excluded in the first place.
+ CanonicalCookie::CookieInclusionStatus
+ include_but_maybe_would_exclude_status =
+ cookie_util::CookieWouldBeExcludedDueToSameSite(cookie.value(),
+ options);
+ if (include_but_maybe_would_exclude_status !=
+ CanonicalCookie::CookieInclusionStatus::INCLUDE) {
+ cs_status_list_.emplace_back(std::move(cookie), std::move(cookie_string),
+ include_but_maybe_would_exclude_status);
+ }
}
num_cookie_lines_left_--;
diff --git a/net/url_request/url_request_http_job.h b/net/url_request/url_request_http_job.h
index ff2a817..b02453a 100644
--- a/net/url_request/url_request_http_job.h
+++ b/net/url_request/url_request_http_job.h
@@ -149,11 +149,13 @@
void DoneWithRequest(CompletionCause reason);
// Callback functions for Cookie Monster
- void SetCookieHeaderAndStart(const CookieList& cookie_list,
+ void SetCookieHeaderAndStart(const CookieOptions& options,
+ const CookieList& cookie_list,
const CookieStatusList& excluded_list);
// Another Cookie Monster callback
- void OnSetCookieResult(base::Optional<CanonicalCookie> cookie,
+ void OnSetCookieResult(const CookieOptions& options,
+ base::Optional<CanonicalCookie> cookie,
std::string cookie_string,
CanonicalCookie::CookieInclusionStatus status);
int num_cookie_lines_left_;
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 9cd7315..1b646f5 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -8494,6 +8494,7 @@
std::unique_ptr<URLRequest> r(
context.CreateRequest(url_requiring_auth, DEFAULT_PRIORITY, &d,
TRAFFIC_ANNOTATION_FOR_TESTS));
+ r->set_site_for_cookies(url_requiring_auth);
r->Start();
d.RunUntilComplete();
@@ -8522,6 +8523,7 @@
std::unique_ptr<URLRequest> r(context.CreateRequest(
url_with_identity, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ r->set_site_for_cookies(url_with_identity);
r->Start();
d.RunUntilComplete();
@@ -8548,9 +8550,10 @@
// Make sure cookies blocked from being stored are caught.
{
TestDelegate d;
+ GURL test_url = test_server.GetURL("/set-cookie?not_stored_cookie=true");
std::unique_ptr<URLRequest> req(context.CreateRequest(
- test_server.GetURL("/set-cookie?not_stored_cookie=true"),
- DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ test_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ req->set_site_for_cookies(test_url);
req->Start();
d.RunUntilComplete();
@@ -8564,18 +8567,20 @@
// Set a cookie to be blocked later
{
TestDelegate d;
+ GURL test_url = test_server.GetURL("/set-cookie?not_sent_cookies=true");
std::unique_ptr<URLRequest> req(context.CreateRequest(
- test_server.GetURL("/set-cookie?not_sent_cookies=true"),
- DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ test_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ req->set_site_for_cookies(test_url);
req->Start();
d.RunUntilComplete();
}
{
TestDelegate d;
// Make sure cookies blocked from being sent are caught.
+ GURL test_url = test_server.GetURL("/echoheader?Cookie");
std::unique_ptr<URLRequest> req(context.CreateRequest(
- test_server.GetURL("/echoheader?Cookie"), DEFAULT_PRIORITY, &d,
- TRAFFIC_ANNOTATION_FOR_TESTS));
+ test_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
+ req->set_site_for_cookies(test_url);
req->Start();
d.RunUntilComplete();
@@ -8613,6 +8618,7 @@
std::unique_ptr<URLRequest> request(
context.CreateRequest(url_requiring_auth, DEFAULT_PRIORITY, &delegate,
TRAFFIC_ANNOTATION_FOR_TESTS));
+ request->set_site_for_cookies(url_requiring_auth);
request->Start();
delegate.RunUntilAuthRequired();
@@ -8659,6 +8665,7 @@
std::unique_ptr<URLRequest> request(
context.CreateRequest(url_requiring_auth_wo_cookies, DEFAULT_PRIORITY,
&delegate, TRAFFIC_ANNOTATION_FOR_TESTS));
+ request->set_site_for_cookies(url_requiring_auth_wo_cookies);
request->Start();
delegate.RunUntilAuthRequired();
@@ -8992,6 +8999,7 @@
context.CreateRequest(original_url, DEFAULT_PRIORITY, &delegate,
TRAFFIC_ANNOTATION_FOR_TESTS));
+ request->set_site_for_cookies(original_url);
request->Start();
delegate.RunUntilRedirect();
@@ -9043,6 +9051,7 @@
context.CreateRequest(original_url_wo_cookie, DEFAULT_PRIORITY,
&delegate, TRAFFIC_ANNOTATION_FOR_TESTS));
+ request->set_site_for_cookies(original_url_wo_cookie);
request->Start();
delegate.RunUntilRedirect();
