Skip to content

Commit

Permalink
Convert SameSiteCookieContext to a class
Browse files Browse the repository at this point in the history
Change the SameSiteCookieContext enum into a class containing the
samesite context as well as the type of cross-schemeness.

This Cl is intended to be as close to a no-op for consumers
as feasible.

Bug: 1055342
Change-Id: I90277cc199676d0f90bda13eae52e7f435757fbb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103289
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Steven Bingler <bingler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752895}
  • Loading branch information
Steven Bingler authored and Commit Bot committed Mar 24, 2020
1 parent 5e0196a commit 8d76c2a
Show file tree
Hide file tree
Showing 36 changed files with 1,009 additions and 688 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/android/cookies/cookies_fetcher_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ static void JNI_CookiesFetcher_RestoreCookies(
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
GetCookieServiceClient()->SetCanonicalCookie(
*cookie, "https", options,
network::mojom::CookieManager::SetCanonicalCookieCallback());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ void AndroidSmsAppSetupControllerImpl::SetUpApp(const GURL& app_url,
<< "installation.";
net::CookieOptions options;
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
pwa_delegate_->GetCookieManager(app_url, profile_)
->SetCanonicalCookie(
*net::CanonicalCookie::CreateSanitizedCookie(
Expand Down Expand Up @@ -317,7 +317,7 @@ void AndroidSmsAppSetupControllerImpl::SetMigrationCookie(
// the user try to open old client.
net::CookieOptions options;
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
pwa_delegate_->GetCookieManager(app_url, profile_)
->SetCanonicalCookie(
*net::CanonicalCookie::CreateSanitizedCookie(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ class AndroidSmsAppSetupControllerImplTest : public testing::Test {
"true" /* expected_cookie_value */,
"https" /* expected_source_scheme */,
false /* expected_modify_http_only */,
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT,
net::CookieOptions::SameSiteCookieContext::MakeInclusive(),
true /* success */);

fake_cookie_manager_->InvokePendingDeleteCookiesCallback(
Expand Down Expand Up @@ -294,7 +294,7 @@ class AndroidSmsAppSetupControllerImplTest : public testing::Test {
"true" /* expected_cookie_value */,
"https" /* expected_source_scheme */,
false /* expected_modify_http_only */,
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT,
net::CookieOptions::SameSiteCookieContext::MakeInclusive(),
true /* success */);

fake_cookie_manager_->InvokePendingDeleteCookiesCallback(
Expand Down Expand Up @@ -369,7 +369,7 @@ class AndroidSmsAppSetupControllerImplTest : public testing::Test {
migrated_to_app_url.GetContent() /* expected_cookie_value */,
"https" /* expected_source_scheme */,
false /* expected_modify_http_only */,
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT,
net::CookieOptions::SameSiteCookieContext::MakeInclusive(),
true /* success */);

fake_cookie_manager_->InvokePendingDeleteCookiesCallback(
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/chromeos/login/profile_auth_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void ImportCookies(base::RepeatingClosure completion_callback,
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
cookie_manager->SetCanonicalCookie(
cookie, "https", options,
base::BindOnce(&OnCookieSet, cookie_completion_callback));
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/api/cookies/cookies_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ ExtensionFunction::ResponseAction CookiesSetFunction::Run() {
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
DCHECK(!url_.is_empty() && url_.is_valid());
cookie_manager->SetCanonicalCookie(
*cc, url_.scheme(), options,
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/media/feeds/media_feeds_fetcher_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class MediaFeedsFetcherTest : public ChromeRenderViewHostTestHarness {
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
cookie_manager->SetCanonicalCookie(
*cc.get(), url.scheme(), options,
base::BindOnce(
Expand Down
97 changes: 59 additions & 38 deletions chrome/browser/net/samesite_cookies_policy_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,28 @@ IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
// Set a cookie from a same-site context. The cookie does not specify
// SameSite, so it may default to Lax if the SameSite features are enabled.
// Since the context used is same-site, it should always work.
EXPECT_TRUE(content::SetCookie(
profile, url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext::SAME_SITE_LAX));
EXPECT_TRUE(content::SetCookie(profile, url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX)));
EXPECT_EQ("samesite-unspecified=1", content::GetCookies(profile, url));

// Overwrite the cookie from a cross-site context. Because we have a policy
// that allows Legacy access for all domains, this will work even if the
// SameSite features are enabled. (It works regardless, if they are disabled.)
EXPECT_TRUE(content::SetCookie(
profile, url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE)));
// Cookie has the new value because we were able to successfully overwrite it.
EXPECT_EQ("samesite-unspecified=2", content::GetCookies(profile, url));
// Fetching the cookies from a cross-site context also works because of the
// policy.
EXPECT_EQ(
"samesite-unspecified=2",
content::GetCookies(
profile, url, net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_EQ("samesite-unspecified=2",
content::GetCookies(profile, url,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
}

IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
Expand All @@ -105,9 +108,10 @@ IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
// Set a cookie from a same-site context. The cookie does not specify
// SameSite, so it may default to Lax if the SameSite features are enabled.
// Since the context used is same-site, it should always work.
EXPECT_TRUE(content::SetCookie(
profile, url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext::SAME_SITE_LAX));
EXPECT_TRUE(content::SetCookie(profile, url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX)));
EXPECT_EQ("samesite-unspecified=1", content::GetCookies(profile, url));

// Overwrite the cookie from a cross-site context. Because we have a policy
Expand All @@ -116,14 +120,17 @@ IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
// enabled.)
EXPECT_FALSE(content::SetCookie(
profile, url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE)));
// Cookie still has the previous value because re-setting it failed.
EXPECT_EQ("samesite-unspecified=1", content::GetCookies(profile, url));
// Fetching the unspecified-samesite cookie from a cross-site context does not
// work because of the policy.
EXPECT_EQ("", content::GetCookies(
profile, url,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_EQ("",
content::GetCookies(profile, url,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
}

IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
Expand All @@ -150,15 +157,19 @@ IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
// Set a cookie from a same-site context. The cookie does not specify
// SameSite, so it may default to Lax if the SameSite features are enabled.
// Since the context used is same-site, it should always work.
EXPECT_TRUE(content::SetCookie(
profile, legacy_allowed_domain_url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext::SAME_SITE_LAX));
EXPECT_TRUE(content::SetCookie(profile, legacy_allowed_domain_url,
"samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX)));
EXPECT_EQ("samesite-unspecified=1",
content::GetCookies(profile, legacy_allowed_domain_url));
// Do the same on the other domain...
EXPECT_TRUE(content::SetCookie(
profile, other_domain_url, "samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext::SAME_SITE_LAX));
EXPECT_TRUE(content::SetCookie(profile, other_domain_url,
"samesite-unspecified=1",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::SAME_SITE_LAX)));
EXPECT_EQ("samesite-unspecified=1",
content::GetCookies(profile, other_domain_url));

Expand All @@ -169,34 +180,44 @@ IN_PROC_BROWSER_TEST_P(SameSiteCookiesPolicyTest,
// disabled.)
EXPECT_TRUE(content::SetCookie(
profile, legacy_allowed_domain_url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE)));
EXPECT_EQ("samesite-unspecified=2",
content::GetCookies(profile, legacy_allowed_domain_url));
EXPECT_EQ("samesite-unspecified=2",
content::GetCookies(
profile, legacy_allowed_domain_url,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
content::GetCookies(profile, legacy_allowed_domain_url,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
// For the domain that is not Legacy by policy, we expect it to work only if
// the SameSite features are disabled.
if (AreSameSiteFeaturesEnabled()) {
EXPECT_FALSE(content::SetCookie(
profile, other_domain_url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_FALSE(
content::SetCookie(profile, other_domain_url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
EXPECT_EQ("samesite-unspecified=1",
content::GetCookies(profile, other_domain_url));
EXPECT_EQ("", content::GetCookies(
profile, other_domain_url,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_EQ(
"", content::GetCookies(profile, other_domain_url,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
} else {
EXPECT_TRUE(content::SetCookie(
profile, other_domain_url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_TRUE(
content::SetCookie(profile, other_domain_url, "samesite-unspecified=2",
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
EXPECT_EQ("samesite-unspecified=2",
content::GetCookies(profile, other_domain_url));
EXPECT_EQ("samesite-unspecified=2",
content::GetCookies(
profile, other_domain_url,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE));
EXPECT_EQ(
"samesite-unspecified=2",
content::GetCookies(profile, other_domain_url,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::
ContextType::CROSS_SITE)));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ class IsolatedPrerenderTabHelperTest : public ChromeRenderViewHostTestHarness {
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
cookie_manager->SetCanonicalCookie(
*cc.get(), url.scheme(), options,
base::BindOnce(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ void OAuthMultiloginHelper::StartSettingCookies(
options.set_include_httponly();
// Permit it to set a SameSite cookie if it wants to.
options.set_same_site_cookie_context(
net::CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT);
net::CookieOptions::SameSiteCookieContext::MakeInclusive());
cookie_manager->SetCanonicalCookie(
cookie, "https", options,
mojo::WrapCallbackWithDefaultInvokeIfNotRun(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,11 @@ class SameSiteDataRemoverBrowserTest : public ContentBrowserTest {
IN_PROC_BROWSER_TEST_F(SameSiteDataRemoverBrowserTest,
TestClearDataWithStorageRemoval) {
StoragePartition* storage_partition = GetStoragePartition();
CreateCookieForTest("TestCookie", "www.google.com",
net::CookieSameSite::NO_RESTRICTION,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE,
true /* is_cookie_secure */, GetBrowserContext());
CreateCookieForTest(
"TestCookie", "www.google.com", net::CookieSameSite::NO_RESTRICTION,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE),
true /* is_cookie_secure */, GetBrowserContext());
browsing_data_browsertest_utils::AddServiceWorker(
"www.google.com", storage_partition, GetHttpsServer());

Expand All @@ -119,10 +120,11 @@ IN_PROC_BROWSER_TEST_F(SameSiteDataRemoverBrowserTest,
IN_PROC_BROWSER_TEST_F(SameSiteDataRemoverBrowserTest,
TestClearDataWithoutStorageRemoval) {
StoragePartition* storage_partition = GetStoragePartition();
CreateCookieForTest("TestCookie", "www.google.com",
net::CookieSameSite::NO_RESTRICTION,
net::CookieOptions::SameSiteCookieContext::CROSS_SITE,
true /* is_cookie_secure */, GetBrowserContext());
CreateCookieForTest(
"TestCookie", "www.google.com", net::CookieSameSite::NO_RESTRICTION,
net::CookieOptions::SameSiteCookieContext(
net::CookieOptions::SameSiteCookieContext::ContextType::CROSS_SITE),
true /* is_cookie_secure */, GetBrowserContext());
browsing_data_browsertest_utils::AddServiceWorker(
"www.google.com", storage_partition, GetHttpsServer());

Expand Down
Loading

0 comments on commit 8d76c2a

Please sign in to comment.