Skip to content

Commit

Permalink
Reland "Make Clear-Site-Data: "cookies" respect third-party cookie bl…
Browse files Browse the repository at this point in the history
…ocking"

The failure in the rollback [1] is because one cookie was not cleared when the first CSD header is sent in clear_site_data_handler_browsertest.cc. This should only be possible if 3P cookie blocking is already enabled at the beginning of the test.

To patch this, I explicitly disable 3PCB at the start of the test.

To be conservative, I also explicitly enable the partitioned cookies feature so that it does not rely on the environment's default. Right now, partitioned cookies are enabled via the field trial testing config.

[1]: https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/31221/overview

This is a reland of commit d27d506

Original change's description:
> Make Clear-Site-Data: "cookies" respect third-party cookie blocking
>
> This will prevent Clear-Site-Data from being abused for cross-site
> tracking [1] when partitioned cookies are enabled by default.
>
> [1]: privacycg/storage-partitioning#11
>
> Old behavior: Clear-Site-Data could clear unpartitioned cookies from any context.
>
> New behavior: Clear-Site-Data cannot clear unpartitioned cookies if it came from a response where 3P cookie blocking applies.
>
> In both cases, CSD will delete partitioned cookies in the current partition.
>
> Low-Coverage-Reason:Some files have trial changes that do not impact behavior.
>
> Bug: 1416655
> Change-Id: Ieed1e050f8f376b7d7704b4948c8f59adc21a17f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4312585
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Andrey Zaytsev <andzaytsev@google.com>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Reviewed-by: Daniel Murphy <dmurph@chromium.org>
> Commit-Queue: Dylan Cutler <dylancutler@google.com>
> Reviewed-by: Christian Dullweber <dullweber@chromium.org>
> Reviewed-by: Oleh Lamzin <lamzin@google.com>
> Cr-Commit-Position: refs/heads/main@{#1118080}

Bug: 1416655
Change-Id: I10e54ba18587c7c9a838e79dd52a535b5e8ff751
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4346675
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Dylan Cutler <dylancutler@google.com>
Reviewed-by: Oleh Lamzin <lamzin@google.com>
Reviewed-by: Andrey Zaytsev <andzaytsev@google.com>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1119580}
  • Loading branch information
DCtheTall authored and tkiela1 committed Mar 20, 2023
1 parent 0329913 commit 72ac550
Show file tree
Hide file tree
Showing 31 changed files with 362 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ void WilcoDtcSupportdNetworkContextImpl::OnClearSiteData(
const std::string& header_value,
int32_t load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
bool partitioned_state_allowed_only,
OnClearSiteDataCallback callback) {
std::move(callback).Run();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class WilcoDtcSupportdNetworkContextImpl
const std::string& header_value,
int32_t load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
bool partitioned_state_allowed_only,
OnClearSiteDataCallback callback) override;
void OnLoadingStateUpdate(network::mojom::LoadInfoPtr info,
OnLoadingStateUpdateCallback callback) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@ class BrowsingDataRemoverWithPasswordsAccountStorageBrowserTest
/*avoid_closing_connections=*/true,
/*cookie_partition_key=*/cookie_partition_key,
/*storage_key=*/storage_key,
/*partitioned_state_allowed_only=*/false,
/*callback=*/loop.QuitClosure());
loop.Run();
}
Expand Down Expand Up @@ -972,6 +973,7 @@ class BrowsingDataRemoverStorageBucketsBrowserTest
/*avoid_closing_connections=*/true,
/*cookie_partition_key=*/absl::nullopt,
/*storage_key=*/storage_key,
/*partitioned_state_allowed_only=*/false,
/*callback=*/loop.QuitClosure());
loop.Run();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ void WebAppUninstallDialogDelegateView::ClearWebAppSiteData() {
/*storage_buckets_to_remove=*/{},
/*avoid_closing_connections=*/false,
/*cookie_partition_key=*/absl::nullopt,
/*storage_key=*/absl::nullopt, base::DoNothing());
/*storage_key=*/absl::nullopt,
/*partitioned_state_allowed_only=*/false,
base::DoNothing());
}

void WebAppUninstallDialogDelegateView::ProcessAutoConfirmValue() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -772,16 +772,17 @@ void WebAppPublisherHelper::UninstallWebApp(
constexpr bool kClearCache = true;
constexpr bool kAvoidClosingConnections = false;

content::ClearSiteData(base::BindRepeating(
[](content::BrowserContext* browser_context) {
return browser_context;
},
base::Unretained(profile())),
origin, kClearCookies, kClearStorage, kClearCache,
/*storage_buckets_to_remove=*/{},
kAvoidClosingConnections,
/*cookie_partition_key=*/absl::nullopt,
/*storage_key=*/absl::nullopt, base::DoNothing());
content::ClearSiteData(
base::BindRepeating(
[](content::BrowserContext* browser_context) {
return browser_context;
},
base::Unretained(profile())),
origin, kClearCookies, kClearStorage, kClearCache,
/*storage_buckets_to_remove=*/{}, kAvoidClosingConnections,
/*cookie_partition_key=*/absl::nullopt,
/*storage_key=*/absl::nullopt,
/*partitioned_state_allowed_only=*/false, base::DoNothing());
}

void WebAppPublisherHelper::SetIconEffect(const std::string& app_id) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,10 @@ bool BrowsingDataFilterBuilderImpl::MatchesAllOriginsAndDomains() {
return mode_ == Mode::kPreserve && origins_.empty() && domains_.empty();
}

void BrowsingDataFilterBuilderImpl::SetPartitionedStateAllowedOnly(bool value) {
partitioned_state_only_ = value;
}

base::RepeatingCallback<bool(const GURL&)>
BrowsingDataFilterBuilderImpl::BuildUrlFilter() {
if (MatchesAllOriginsAndDomains())
Expand Down Expand Up @@ -274,6 +278,8 @@ BrowsingDataFilterBuilderImpl::BuildCookieDeletionFilter() {
deletion_filter->cookie_partition_key_collection =
cookie_partition_key_collection_;

deletion_filter->partitioned_state_only = partitioned_state_only_;

switch (mode_) {
case Mode::kDelete:
deletion_filter->including_domains.emplace(domains_.begin(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class CONTENT_EXPORT BrowsingDataFilterBuilderImpl
bool MatchesWithSavedStorageKey(
const blink::StorageKey& other_key) const override;
bool MatchesAllOriginsAndDomains() override;
void SetPartitionedStateAllowedOnly(bool value) override;
base::RepeatingCallback<bool(const GURL&)> BuildUrlFilter() override;
content::StoragePartition::StorageKeyMatcherFunction BuildStorageKeyFilter()
override;
Expand Down Expand Up @@ -68,6 +69,7 @@ class CONTENT_EXPORT BrowsingDataFilterBuilderImpl
net::CookiePartitionKeyCollection cookie_partition_key_collection_ =
net::CookiePartitionKeyCollection::ContainsAll();
absl::optional<blink::StorageKey> storage_key_ = absl::nullopt;
bool partitioned_state_only_ = false;
};

} // content
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -869,4 +869,51 @@ TEST(BrowsingDataFilterBuilderImplTest, GetRegisterableDomains) {
UnorderedElementsAre(kGoogleDomain, kLongETLDDomain));
}

TEST(BrowsingDataFilterBuilderImplTest, ExcludeUnpartitionedCookies) {
BrowsingDataFilterBuilderImpl builder(
BrowsingDataFilterBuilderImpl::Mode::kDelete);

builder.SetPartitionedStateAllowedOnly(true);

CookieDeletionInfo delete_info =
network::DeletionFilterToInfo(builder.BuildCookieDeletionFilter());

// Unpartitioned cookie should NOT match.
std::unique_ptr<net::CanonicalCookie> cookie = net::CanonicalCookie::Create(
GURL("https://www.cookie.com/"),
"__Host-A=B; Secure; SameSite=None; Path=/;", base::Time::Now(),
absl::nullopt, absl::nullopt);
EXPECT_TRUE(cookie);
EXPECT_FALSE(delete_info.Matches(
*cookie, net::CookieAccessParams{
net::CookieAccessSemantics::NONLEGACY, false,
net::CookieSamePartyStatus::kNoSamePartyEnforcement}));

// Partitioned cookie should match.
cookie = net::CanonicalCookie::Create(
GURL("https://www.cookie.com/"),
"__Host-A=B; Secure; SameSite=None; Path=/; Partitioned;",
base::Time::Now(), absl::nullopt,
net::CookiePartitionKey::FromURLForTesting(
GURL("https://toplevelsite.com")));
EXPECT_TRUE(cookie);
EXPECT_TRUE(delete_info.Matches(
*cookie, net::CookieAccessParams{
net::CookieAccessSemantics::NONLEGACY, false,
net::CookieSamePartyStatus::kNoSamePartyEnforcement}));

// Nonced partitioned cookie should match.
cookie = net::CanonicalCookie::Create(
GURL("https://www.cookie.com/"),
"__Host-A=B; Secure; SameSite=None; Path=/;", base::Time::Now(),
absl::nullopt,
net::CookiePartitionKey::FromURLForTesting(
GURL("https://toplevelsite.com"), base::UnguessableToken::Create()));
EXPECT_TRUE(cookie);
EXPECT_TRUE(delete_info.Matches(
*cookie, net::CookieAccessParams{
net::CookieAccessSemantics::NONLEGACY, false,
net::CookieSamePartyStatus::kNoSamePartyEnforcement}));
}

} // namespace content
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,36 @@ IN_PROC_BROWSER_TEST_F(CookiesBrowsingDataRemoverImplBrowserTest,
EXPECT_EQ("F", cookies[2].Name());
}

IN_PROC_BROWSER_TEST_F(CookiesBrowsingDataRemoverImplBrowserTest,
ClearSiteData_PartitionedStateAllowedOnly) {
// Unpartitioned cookie should not be removed when third-party cookie blocking
// applies to the request that sent Clear-Site-Data.
ASSERT_TRUE(SetCookie(GURL("https://a.com"), "A=0; secure;",
/*cookie_partition_key=*/absl::nullopt));
// Partitioned cookies should still be removed.
ASSERT_TRUE(SetCookie(
GURL("https://a.com"), "B=1; secure; partitioned",
net::CookiePartitionKey::FromURLForTesting(GURL("https://b.com"))));
// Nonced partitioned cookies should still be removed.
ASSERT_TRUE(
SetCookie(GURL("https://a.com"), "C=2; secure;",
net::CookiePartitionKey::FromURLForTesting(
GURL("https://b.com"), base::UnguessableToken::Create())));

std::unique_ptr<BrowsingDataFilterBuilder> builder(
BrowsingDataFilterBuilder::Create(
BrowsingDataFilterBuilder::Mode::kDelete));
builder->AddRegisterableDomain("a.com");
builder->SetPartitionedStateAllowedOnly(true);

RemoveWithFilterAndWait(BrowsingDataRemover::DATA_TYPE_COOKIES,
std::move(builder));

auto cookies = GetAllCookies();
EXPECT_EQ(1u, cookies.size());
EXPECT_EQ("A", cookies[0].Name());
}

namespace {
// Provide BrowsingDataRemoverImplTrustTokenTest the Trust Tokens
// feature as a mixin so that it gets set before the superclass initializes
Expand Down
9 changes: 7 additions & 2 deletions content/browser/browsing_data/clear_site_data_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,12 @@ void ClearSiteDataHandler::HandleHeader(
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback) {
ClearSiteDataHandler handler(browser_context_getter, web_contents_getter, url,
header_value, load_flags, cookie_partition_key,
storage_key, std::move(callback),
storage_key, partitioned_state_allowed_only,
std::move(callback),
std::make_unique<ConsoleMessagesDelegate>());
handler.HandleHeaderAndOutputConsoleMessages();
}
Expand All @@ -134,6 +136,7 @@ ClearSiteDataHandler::ClearSiteDataHandler(
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback,
std::unique_ptr<ConsoleMessagesDelegate> delegate)
: browser_context_getter_(browser_context_getter),
Expand All @@ -143,6 +146,7 @@ ClearSiteDataHandler::ClearSiteDataHandler(
load_flags_(load_flags),
cookie_partition_key_(cookie_partition_key),
storage_key_(storage_key),
partitioned_state_allowed_only_(partitioned_state_allowed_only),
callback_(std::move(callback)),
delegate_(std::move(delegate)) {
DCHECK(browser_context_getter_);
Expand Down Expand Up @@ -336,7 +340,8 @@ void ClearSiteDataHandler::ExecuteClearingTask(
ClearSiteData(browser_context_getter_, origin, clear_cookies, clear_storage,
clear_cache, storage_buckets_to_remove,
true /*avoid_closing_connections*/, cookie_partition_key_,
storage_key_, std::move(callback));
storage_key_, partitioned_state_allowed_only_,
std::move(callback));
}

// static
Expand Down
20 changes: 15 additions & 5 deletions content/browser/browsing_data/clear_site_data_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class CONTENT_EXPORT ClearSiteDataHandler {
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback);

// Exposes ParseHeader() publicly for testing.
Expand All @@ -104,6 +105,7 @@ class CONTENT_EXPORT ClearSiteDataHandler {
int load_flags,
const absl::optional<net::CookiePartitionKey>& cookie_partition_key,
const absl::optional<blink::StorageKey>& storage_key,
bool partitioned_state_allowed_only,
base::OnceClosure callback,
std::unique_ptr<ConsoleMessagesDelegate> delegate);
virtual ~ClearSiteDataHandler();
Expand Down Expand Up @@ -163,27 +165,35 @@ class CONTENT_EXPORT ClearSiteDataHandler {
return storage_key_;
}

bool PartitionedStateOnlyForTesting() const {
return partitioned_state_allowed_only_;
}

private:
// Required to clear the data.
base::RepeatingCallback<BrowserContext*()> browser_context_getter_;
base::RepeatingCallback<WebContents*()> web_contents_getter_;

// Target URL whose data will be cleared.
GURL url_;
const GURL url_;

// Raw string value of the 'Clear-Site-Data' header.
std::string header_value_;
const std::string header_value_;

// Load flags of the current request, used to check cookie policies.
int load_flags_;
const int load_flags_;

// The cookie partition key for which we need to clear partitioned cookies
// when we receive the Clear-Site-Data header.
absl::optional<net::CookiePartitionKey> cookie_partition_key_;
const absl::optional<net::CookiePartitionKey> cookie_partition_key_;

// The storage key for which we need to clear partitioned storage when we
// receive the Clear-Site-Data header.
absl::optional<blink::StorageKey> storage_key_;
const absl::optional<blink::StorageKey> storage_key_;

// If third-party cookie blocking is enabled and applies to the response that
// sent Clear-Site-Data.
const bool partitioned_state_allowed_only_;

// Used to notify that the clearing has completed. Callers could resuming
// loading after this point.
Expand Down
Loading

0 comments on commit 72ac550

Please sign in to comment.