chromium/src
0
Fork 0
Code Activity

[cookies] Remove defaults from CanonicalCookie::CreateSanitizedCookie

Browse Source 
There is a confusing pattern of defaults on cookie constructors, and
this is somewhat dangerous as we do not want cookies to be created in
production code without all values being explicitly set (especially as
new values are added). No test-only function was added given there's
only one value to add a default too it doesn't seem needed.

Bug: 332939148
Change-Id: I694660113462c19e15580fdf46564a556ba97d8f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5425079
Reviewed-by: Danil Somsikov <dsv@chromium.org>
Reviewed-by: Monica Basta <msalama@chromium.org>
Reviewed-by: Dylan Cutler <dylancutler@google.com>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Reviewed-by: Danila Kuzmin <dkuzmin@google.com>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Auto-Submit: Ari Chivukula <arichiv@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Andrey Davydov <andreydav@google.com>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1283986}
This commit is contained in:
Ari Chivukula
2024-04-08 18:52:10 +00:00
committed by Chromium LUCI CQ
parent 02c7febeed
commit 87c035bc50
22 changed files with 51 additions and 42 deletions

4
chrome/browser/ash/android_sms/android_sms_app_setup_controller_impl.cc

@ -116,7 +116,7 @@ void AndroidSmsAppSetupControllerImpl::SetUpApp(const GURL& app_url,
base::Time::Now() /* last_access_time */,
!net::IsLocalhost(app_url) /* secure */, false /* http_only */,
net::CookieSameSite::STRICT_MODE, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt /* partition_key */);
std::nullopt /* partition_key */, /*status=*/nullptr);
// TODO(crbug.com/1069974): The cookie source url must be faked here because
// otherwise, this would fail to set a secure cookie if |app_url| is insecure.
// Consider instead to use url::Replacements to force the scheme to be https.
@ -328,7 +328,7 @@ void AndroidSmsAppSetupControllerImpl::SetMigrationCookie(
base::Time::Now() /* last_access_time */,
!net::IsLocalhost(app_url) /* secure */, false /* http_only */,
net::CookieSameSite::STRICT_MODE, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt /* partition_key */);
std::nullopt /* partition_key */, /*status=*/nullptr);
// TODO(crbug.com/1069974): The cookie source url must be faked here because
// otherwise, this would fail to set a secure cookie if |app_url| is insecure.
// Consider instead to use url::Replacements to force the scheme to be https.

6
chrome/browser/ash/login/profile_auth_data_unittest.cc

@ -233,7 +233,7 @@ void ProfileAuthDataTest::PopulateBrowserContext(
kSAMLIdPCookieDomainWithWildcard, std::string(), base::Time(),
base::Time(), base::Time(), true, false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt),
std::nullopt, /*status=*/nullptr),
GURL(kSAMLIdPCookieURL), options, base::DoNothing());
cookies->SetCanonicalCookie(
@ -241,7 +241,7 @@ void ProfileAuthDataTest::PopulateBrowserContext(
GURL(kSAMLIdPCookieURL), kCookieName, cookie_value, std::string(),
std::string(), base::Time(), base::Time(), base::Time(), true, false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt),
std::nullopt, /*status=*/nullptr),
GURL(kSAMLIdPCookieURL), options, base::DoNothing());
cookies->SetCanonicalCookie(
@ -249,7 +249,7 @@ void ProfileAuthDataTest::PopulateBrowserContext(
GURL(kGAIACookieURL), kCookieName, cookie_value, std::string(),
std::string(), base::Time(), base::Time(), base::Time(), true, false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt),
std::nullopt, /*status=*/nullptr),
GURL(kGAIACookieURL), options, base::DoNothing());
}

2
chrome/browser/ash/login/saml/saml_lockscreen_browsertest.cc

@ -1065,7 +1065,7 @@ class SAMLCookieTransferTest : public SamlUnlockTest {
/*last_access_time=*/base::Time(), /*secure=*/true,
/*http_only=*/false, net::CookieSameSite::NO_RESTRICTION,
net::COOKIE_PRIORITY_DEFAULT,
/*partition_key=*/std::nullopt),
/*partition_key=*/std::nullopt, /*status=*/nullptr),
fake_saml_idp()->GetSamlPageUrl(), options, base::DoNothing());
ExpectCookieInUserProfile(kRandomCookieName, kRandomCookieValue);
}

2
chrome/browser/browsing_data/counters/site_data_counting_helper_unittest.cc

@ -58,7 +58,7 @@ class SiteDataCountingHelperTest : public testing::Test {
url, "name", "A=1", url.host(), url.path(), creation_time,
base::Time(), creation_time, url.SchemeIsCryptographic(), false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt);
std::nullopt, /*status=*/nullptr);
net::CookieOptions options;
options.set_include_httponly();
cookie_manager->SetCanonicalCookie(

2
chrome/browser/enterprise/signin/token_managed_profile_creation_delegate_unittest.cc

@ -50,7 +50,7 @@ void CreateCookies(
url, name, "A=" + name, url.host(), url.path(), base::Time::Now(),
base::Time::Max(), base::Time::Now(), url.SchemeIsCryptographic(),
false, net::CookieSameSite::NO_RESTRICTION,
net::COOKIE_PRIORITY_DEFAULT, std::nullopt);
net::COOKIE_PRIORITY_DEFAULT, std::nullopt, /*status=*/nullptr);
cookie_manager->SetCanonicalCookie(
*cookie, url, net::CookieOptions::MakeAllInclusive(),
base::BindLambdaForTesting(

7
chrome/browser/extensions/api/cookies/cookies_api.cc

@ -494,14 +494,15 @@ ExtensionFunction::ResponseAction CookiesSetFunction::Run() {
parsed_args_->details.value.value_or(std::string()), //
parsed_args_->details.domain.value_or(std::string()), //
parsed_args_->details.path.value_or(std::string()), //
base::Time(), //
/*creation_time=*/base::Time(), //
expiration_time, //
base::Time(), //
/*last_access_time=*/base::Time(), //
parsed_args_->details.secure.value_or(false), //
parsed_args_->details.http_only.value_or(false), //
same_site, //
net::COOKIE_PRIORITY_DEFAULT, //
partition_key));
partition_key, //
/*status=*/nullptr));
if (!cc) {
// Return error through callbacks so that the proper error message
// is generated.

2
chrome/browser/signin/bound_session_credentials/bound_session_test_cookie_manager.cc

@ -24,7 +24,7 @@ net::CanonicalCookie BoundSessionTestCookieManager::CreateCookie(
/*last_access_time=*/base::Time::Now(), /*secure=*/true,
/*http_only=*/true, net::CookieSameSite::UNSPECIFIED,
net::CookiePriority::COOKIE_PRIORITY_HIGH,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
}
size_t BoundSessionTestCookieManager::GetNumberOfDeleteCookiesCallbacks() {

2
chrome/browser/signin/bound_session_credentials/fake_bound_session_refresh_cookie_fetcher.cc

@ -139,7 +139,7 @@ FakeBoundSessionRefreshCookieFetcher::CreateFakeCookie(
/*last_access_time=*/now, /*secure=*/true,
/*http_only=*/true, net::CookieSameSite::UNSPECIFIED,
net::CookiePriority::COOKIE_PRIORITY_HIGH,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
DCHECK(new_cookie);
return new_cookie;

2
chrome/browser/signin/dice_signed_in_profile_creator_unittest.cc

@ -68,7 +68,7 @@ void CreateCookies(
url.path(), base::Time::Now(), base::Time::Max(), base::Time::Now(),
url.SchemeIsCryptographic(), false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt);
std::nullopt, /*status=*/nullptr);
cookie_manager->SetCanonicalCookie(
*cookie, url, net::CookieOptions::MakeAllInclusive(),
base::BindLambdaForTesting(

2
components/signin/core/browser/consistency_cookie_manager.cc

@ -116,7 +116,7 @@ ConsistencyCookieManager::CreateConsistencyCookie(const std::string& value) {
/*path=*/"/", /*creation=*/now, /*expiration=*/expiry,
/*last_access=*/now, /*secure=*/true, /*httponly=*/false,
net::CookieSameSite::STRICT_MODE, net::COOKIE_PRIORITY_DEFAULT,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
}
// static

2
components/signin/internal/identity_manager/gaia_cookie_manager_service.cc

@ -569,7 +569,7 @@ void GaiaCookieManagerService::ForceOnCookieChangeProcessing() {
"." + google_url.host(), "/", base::Time(), base::Time(),
base::Time(), true /* secure */, false /* httponly */,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt /* cookie_partition_key */);
std::nullopt /* cookie_partition_key */, /*status=*/nullptr);
OnCookieChange(
net::CookieChangeInfo(*cookie, net::CookieAccessResult(),
net::CookieChangeCause::UNKNOWN_DELETION));

2
components/signin/internal/identity_manager/gaia_cookie_manager_service_unittest.cc

@ -110,7 +110,7 @@ net::CanonicalCookie GetTestCookie(const GURL& url, const std::string& name) {
/*expiration_time=*/base::Time(), /*last_access_time=*/base::Time(),
/*secure=*/true, /*http_only=*/false,
net::CookieSameSite::NO_RESTRICTION, net::COOKIE_PRIORITY_DEFAULT,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
return *cookie;
}

3
components/signin/ios/browser/account_consistency_service.mm

@ -492,7 +492,8 @@ void AccountConsistencyService::SetChromeConnectedCookieWithUrl(
/*secure=*/true,
/*httponly=*/false, net::CookieSameSite::LAX_MODE,
net::COOKIE_PRIORITY_DEFAULT,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt,
/*status=*/nullptr);
net::CookieOptions options;
options.set_include_httponly();
options.set_same_site_cookie_context(

2
content/browser/devtools/protocol/network_handler.cc

@ -450,7 +450,7 @@ MakeCookieFromProtocolValues(const std::string& name,
net::CanonicalCookie::CreateSanitizedCookie(
url, name, value, normalized_domain, path, base::Time(),
expiration_date, base::Time(), secure, http_only, css, cp,
cookie_partition_key);
cookie_partition_key, /*status=*/nullptr);
if (!cookie)
return Response::InvalidParams("Sanitizing cookie failed");

2
fuchsia_web/webengine/browser/cookie_manager_impl_unittest.cc

@ -46,7 +46,7 @@ std::unique_ptr<net::CanonicalCookie> CreateCookie(base::StringPiece name,
/*secure=*/true,
/*httponly*/ false, net::CookieSameSite::NO_RESTRICTION,
net::COOKIE_PRIORITY_MEDIUM,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
}
class CookieManagerImplTest : public testing::Test {

2
google_apis/gaia/oauth2_mint_token_flow.cc

@ -490,7 +490,7 @@ bool OAuth2MintTokenFlow::ParseRemoteConsentResponse(
is_http_only ? *is_http_only : false,
net::StringToCookieSameSite(same_site ? *same_site : ""),
net::COOKIE_PRIORITY_DEFAULT,
/* partition_key */ std::nullopt);
/* partition_key */ std::nullopt, /*status=*/nullptr);
cookies.push_back(*cookie);
}
}

4
google_apis/gaia/oauth2_mint_token_flow_unittest.cc

@ -119,13 +119,13 @@ static RemoteConsentResolutionData CreateRemoteConsentResolutionData() {
resolution_data.url, "test_name", "test_value", "test.com", "/",
base::Time(), base::Time(), base::Time(), false, true,
net::CookieSameSite::LAX_MODE, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt));
std::nullopt, /*status=*/nullptr));
resolution_data.cookies.push_back(
*net::CanonicalCookie::CreateSanitizedCookie(
resolution_data.url, "test_name2", "test_value2", "test.com", "/",
base::Time(), base::Time(), base::Time(), false, false,
net::CookieSameSite::UNSPECIFIED, net::COOKIE_PRIORITY_DEFAULT,
std::nullopt));
std::nullopt, /*status=*/nullptr));
return resolution_data;
}

5
net/cookies/canonical_cookie.h

@ -124,6 +124,9 @@ class NET_EXPORT CanonicalCookie : public CookieBase {
// cannot be sanitized. If `status` is provided it will have any relevant
// CookieInclusionStatus rejection reasons set. If a cookie is created,
// `cookie->IsCanonical()` will be true.
//
// NOTE: Do not add any defaults to this constructor, we want every caller to
// understand and choose their inputs.
static std::unique_ptr<CanonicalCookie> CreateSanitizedCookie(
const GURL& url,
const std::string& name,
@ -138,7 +141,7 @@ class NET_EXPORT CanonicalCookie : public CookieBase {
CookieSameSite same_site,
CookiePriority priority,
std::optional<CookiePartitionKey> partition_key,
CookieInclusionStatus* status = nullptr);
CookieInclusionStatus* status);
// FromStorage is a factory method which is meant for creating a new
// CanonicalCookie using properties of a previously existing cookie

2
net/cookies/canonical_cookie_fuzzer.cc

@ -66,7 +66,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
url, name, value, domain, path, creation, expiration, last_access,
data_provider.ConsumeBool() /* secure */,
data_provider.ConsumeBool() /* httponly */, same_site, priority,
partition_key);
partition_key, /*status=*/nullptr);
if (sanitized_cookie) {
CHECK(sanitized_cookie->IsCanonical());

2
net/cookies/canonical_cookie_unittest.cc

@ -982,7 +982,7 @@ TEST(CanonicalCookieTest, CreateWithLastUpdate) {
creation_time, /*secure=*/true,
/*http_only=*/false, CookieSameSite::NO_RESTRICTION,
COOKIE_PRIORITY_DEFAULT,
/*partition_key=*/std::nullopt);
/*partition_key=*/std::nullopt, /*status=*/nullptr);
ASSERT_TRUE(cookie.get());
EXPECT_TRUE((base::Time::Now() - cookie->LastUpdateDate()).magnitude() <
base::Seconds(1));

24
net/cookies/cookie_monster_unittest.cc

@ -7145,8 +7145,8 @@ TEST_F(CookieMonsterTest, FromStorageCookieCreated300DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
new_creation, new_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_THAT(
GetAllCookies(cookie_monster.get()),
@ -7188,8 +7188,8 @@ TEST_F(CookieMonsterTest, FromStorageCookieCreated500DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
new_creation, new_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_THAT(
GetAllCookies(cookie_monster.get()),
@ -7216,8 +7216,8 @@ TEST_F(CookieMonsterTest, SanitizedCookieCreated300DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
original_creation, original_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_THAT(
GetAllCookies(cookie_monster.get()),
@ -7232,8 +7232,8 @@ TEST_F(CookieMonsterTest, SanitizedCookieCreated300DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
new_creation, new_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_THAT(
GetAllCookies(cookie_monster.get()),
@ -7260,8 +7260,8 @@ TEST_F(CookieMonsterTest, SanitizedCookieCreated500DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
original_creation, original_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_TRUE(GetAllCookies(cookie_monster.get()).empty());
@ -7273,8 +7273,8 @@ TEST_F(CookieMonsterTest, SanitizedCookieCreated500DaysAgoThenUpdatedNow) {
CanonicalCookie::CreateSanitizedCookie(
https_www_foo_.url(), "A", "B", https_www_foo_.url().host(), "/",
new_creation, new_expiry, base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
std::nullopt),
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr),
https_www_foo_.url(), false));
EXPECT_THAT(GetAllCookies(cookie_monster.get()),
ElementsAre(MatchesCookieNameValueCreationExpiry(

12
net/cookies/cookie_store_unittest.h

@ -477,7 +477,8 @@ TYPED_TEST_P(CookieStoreTest, FilterTest) {
std::unique_ptr<CanonicalCookie> cc(CanonicalCookie::CreateSanitizedCookie(
this->www_foo_foo_.url(), "A", "B", std::string(), "/foo", one_hour_ago,
one_hour_from_now, base::Time(), false, false,
CookieSameSite::STRICT_MODE, COOKIE_PRIORITY_DEFAULT, std::nullopt));
CookieSameSite::STRICT_MODE, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr));
ASSERT_TRUE(cc);
EXPECT_TRUE(this->SetCanonicalCookie(
cs, std::move(cc), this->www_foo_foo_.url(), true /*modify_httponly*/));
@ -487,7 +488,8 @@ TYPED_TEST_P(CookieStoreTest, FilterTest) {
cc = CanonicalCookie::CreateSanitizedCookie(
this->www_foo_bar_.url(), "C", "D", this->www_foo_bar_.domain(), "/bar",
two_hours_ago, base::Time(), one_hour_ago, false, true,
CookieSameSite::STRICT_MODE, COOKIE_PRIORITY_DEFAULT, std::nullopt);
CookieSameSite::STRICT_MODE, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr);
ASSERT_TRUE(cc);
EXPECT_TRUE(this->SetCanonicalCookie(
cs, std::move(cc), this->www_foo_bar_.url(), true /*modify_httponly*/));
@ -499,7 +501,8 @@ TYPED_TEST_P(CookieStoreTest, FilterTest) {
cc = CanonicalCookie::CreateSanitizedCookie(
this->http_www_foo_.url(), "E", "F", std::string(), std::string(),
base::Time(), base::Time(), base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt);
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr);
ASSERT_TRUE(cc);
EXPECT_FALSE(this->SetCanonicalCookie(
cs, std::move(cc), this->http_www_foo_.url(), true /*modify_httponly*/));
@ -507,7 +510,8 @@ TYPED_TEST_P(CookieStoreTest, FilterTest) {
cc = CanonicalCookie::CreateSanitizedCookie(
this->https_www_foo_.url(), "E", "F", std::string(), std::string(),
base::Time(), base::Time(), base::Time(), true, false,
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt);
CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT, std::nullopt,
/*status=*/nullptr);
ASSERT_TRUE(cc);
EXPECT_TRUE(this->SetCanonicalCookie(
cs, std::move(cc), this->https_www_foo_.url(), true /*modify_httponly*/));