Skip to content

Commit

Permalink
Add Partitioned cookie support to CookieStore API
Browse files Browse the repository at this point in the history
This CL adds support for Partitioned cookies* to the CookieStore API.
It implements the changes proposed in
WICG/cookie-store#206 in Chromium.

Cookie partitioning will only be active and the `partitioned` field
is only considered by `CookieStore.set` or `CookieStore.delete` when
the --partitioned-cookies flag is set.

*: see https://github.com/WICG/CHIPS

Bug: 1225444
Change-Id: I4bdde833c28fa192bb0b5e011a6005a45029b481
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3212611
Commit-Queue: Dylan Cutler <dylancutler@google.com>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#937656}
  • Loading branch information
DCtheTall authored and Chromium LUCI CQ committed Nov 3, 2021
1 parent c9cdc43 commit 4a461f1
Show file tree
Hide file tree
Showing 20 changed files with 235 additions and 8 deletions.
11 changes: 9 additions & 2 deletions net/cookies/cookie_partition_key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ namespace net {
CookiePartitionKey::CookiePartitionKey() = default;

CookiePartitionKey::CookiePartitionKey(const SchemefulSite& site)
: site_(site) {}
: site_(site), from_script_(false) {}

CookiePartitionKey::CookiePartitionKey(const GURL& url)
: site_(SchemefulSite(url)) {}
: site_(SchemefulSite(url)), from_script_(false) {}

CookiePartitionKey::CookiePartitionKey(bool from_script)
: from_script_(from_script) {}

CookiePartitionKey::CookiePartitionKey(const CookiePartitionKey& other) =
default;
Expand Down Expand Up @@ -52,6 +55,10 @@ bool CookiePartitionKey::Serialize(const absl::optional<CookiePartitionKey>& in,
}
if (!base::FeatureList::IsEnabled(features::kPartitionedCookies))
return false;

// We should not try to serialize a partition key created by a renderer.
DCHECK(!in->from_script_);

if (in->site_.GetURL().SchemeIsFile()) {
out = in->site_.SerializeFileSiteWithHost();
return true;
Expand Down
20 changes: 20 additions & 0 deletions net/cookies/cookie_partition_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,37 @@ class NET_EXPORT CookiePartitionKey {
return CookiePartitionKey(site);
}

// Create a new CookiePartitionKey in a script running in a renderer. We do
// not trust the renderer to provide us with a cookie partition key, so we let
// the renderer use this method to indicate the cookie is partitioned but the
// key still needs to be determined.
//
// When the browser is ingesting cookie partition keys from the renderer,
// either the `from_script_` flag should be set or the cookie partition key
// should match the browser's. Otherwise the renderer may be compromised.
//
// TODO(crbug.com/1225444) Consider removing this factory method and
// `from_script_` flag when BlinkStorageKey is available in
// ServiceWorkerGlobalScope.
static absl::optional<CookiePartitionKey> FromScript() {
return absl::make_optional(CookiePartitionKey(true));
}

// Temporary method, used to mark the places where we need to supply the
// cookie partition key to CanonicalCookie::Create.
static absl::optional<CookiePartitionKey> Todo() { return absl::nullopt; }

const SchemefulSite& site() const { return site_; }

bool from_script() const { return from_script_; }

private:
explicit CookiePartitionKey(const SchemefulSite& site);
explicit CookiePartitionKey(const GURL& url);
explicit CookiePartitionKey(bool from_script);

SchemefulSite site_;
bool from_script_;
};

} // namespace net
Expand Down
9 changes: 9 additions & 0 deletions net/cookies/cookie_partition_key_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ TEST_P(CookiePartitionKeyTest, FromNetworkIsolationKey) {
bool partitioned_cookies_enabled = PartitionedCookiesEnabled();
EXPECT_EQ(partitioned_cookies_enabled, got.has_value());
if (partitioned_cookies_enabled) {
EXPECT_FALSE(got->from_script());
EXPECT_EQ(CookiePartitionKey::FromURLForTesting(top_level_site.GetURL()),
got.value());
}
Expand All @@ -130,6 +131,14 @@ TEST_P(CookiePartitionKeyTest, FromWire) {
auto want = CookiePartitionKey::FromURLForTesting(GURL("https://foo.com"));
auto got = CookiePartitionKey::FromWire(want.site());
EXPECT_EQ(want, got);
EXPECT_FALSE(got.from_script());
}

TEST_P(CookiePartitionKeyTest, FromScript) {
auto key = CookiePartitionKey::FromScript();
EXPECT_TRUE(key);
EXPECT_TRUE(key->from_script());
EXPECT_TRUE(key->site().opaque());
}

} // namespace net
Expand Down
4 changes: 4 additions & 0 deletions services/network/public/cpp/cookie_manager_mojom_traits.cc
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,10 @@ bool StructTraits<network::mojom::CookiePartitionKeyDataView,
net::CookiePartitionKey>::
Read(network::mojom::CookiePartitionKeyDataView partition_key,
net::CookiePartitionKey* out) {
if (partition_key.from_script()) {
*out = net::CookiePartitionKey::FromScript().value();
return true;
}
net::SchemefulSite site;
if (!partition_key.ReadSite(&site))
return false;
Expand Down
4 changes: 4 additions & 0 deletions services/network/public/cpp/cookie_manager_mojom_traits.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ struct StructTraits<network::mojom::CookiePartitionKeyDataView,
static const net::SchemefulSite& site(const net::CookiePartitionKey& cpk) {
return cpk.site();
}
static bool from_script(const net::CookiePartitionKey& cpk) {
return cpk.from_script();
}

static bool Read(network::mojom::CookiePartitionKeyDataView partition_key,
net::CookiePartitionKey* out);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,16 @@ TEST(CookieManagerTraitsTest, Roundtrips_PartitionKey) {
EXPECT_TRUE(mojo::test::SerializeAndDeserialize<mojom::CanonicalCookie>(
*original, copied));
EXPECT_EQ(original->PartitionKey(), copied.PartitionKey());
EXPECT_FALSE(copied.PartitionKey()->from_script());

original = net::CanonicalCookie::CreateUnsafeCookieForTesting(
"__Host-A", "B", "x.y", "/", base::Time(), base::Time(), base::Time(),
true, false, net::CookieSameSite::NO_RESTRICTION,
net::COOKIE_PRIORITY_LOW, false, net::CookiePartitionKey::FromScript(),
net::CookieSourceScheme::kSecure, 8433);
EXPECT_TRUE(mojo::test::SerializeAndDeserialize<mojom::CanonicalCookie>(
*original, copied));
EXPECT_TRUE(copied.PartitionKey()->from_script());
}

TEST(CookieManagerTraitsTest, Roundtrips_CookiePartitionKeychain) {
Expand Down
4 changes: 4 additions & 0 deletions services/network/public/mojom/cookie_manager.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ struct CookieOptions {

struct CookiePartitionKey {
SchemefulSite site;
// Indicates the CookiePartitionKey is a placeholder indicating that the
// cookie should be partitioned, but it was created in the renderer so we
// don't know what its partition key is yet.
bool from_script = false;
};

struct CookiePartitionKeychain {
Expand Down
32 changes: 31 additions & 1 deletion services/network/restricted_cookie_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -534,11 +534,41 @@ void RestrictedCookieManager::SetCanonicalCookie(
GURL::SchemeIsCryptographic(origin_.scheme())
? net::CookieSourceScheme::kSecure
: net::CookieSourceScheme::kNonSecure;

// If the renderer's cookie has a partition key that was not created using
// CookiePartitionKey::FromScript, then the cookie's partition key should be
// equal to RestrictedCookieManager's partition key.
absl::optional<net::CookiePartitionKey> cookie_partition_key =
cookie.PartitionKey();
if (cookie_partition_key) {
// RestrictedCookieManager having a null partition key strictly implies the
// feature is disabled. If that is the case, we treat the cookie as
// unpartitioned.
if (!cookie_partition_key_) {
cookie_partition_key = absl::nullopt;
} else {
bool cookie_partition_key_ok =
cookie.PartitionKey()->from_script() ||
cookie.PartitionKey().value() == cookie_partition_key_.value();
UMA_HISTOGRAM_BOOLEAN("Net.RestrictedCookieManager.CookiePartitionKeyOK",
cookie_partition_key_ok);
if (!cookie_partition_key_ok) {
mojo::ReportBadMessage(
"RestrictedCookieManager: unexpected cookie partition key");
std::move(callback).Run(false);
return;
}
if (cookie.PartitionKey()->from_script()) {
cookie_partition_key = cookie_partition_key_;
}
}
}

auto sanitized_cookie = net::CanonicalCookie::FromStorage(
cookie.Name(), cookie.Value(), cookie.Domain(), cookie.Path(), now,
cookie.ExpiryDate(), now, cookie.IsSecure(), cookie.IsHttpOnly(),
cookie.SameSite(), cookie.Priority(), cookie.IsSameParty(),
cookie.PartitionKey(), source_scheme, origin_.port());
cookie_partition_key, source_scheme, origin_.port());
DCHECK(sanitized_cookie);
net::CanonicalCookie cookie_copy = *sanitized_cookie;

Expand Down
76 changes: 76 additions & 0 deletions services/network/restricted_cookie_manager_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1643,6 +1643,35 @@ TEST_P(RestrictedCookieManagerTest, PartitionedCookies) {
}
}

TEST_P(RestrictedCookieManagerTest, PartitionKeyFromScript) {
const GURL kCookieURL("https://example.com");
const GURL kTopFrameURL("https://foo.com");
const net::SiteForCookies kSiteForCookies =
net::SiteForCookies::FromUrl(kTopFrameURL);
const url::Origin kTopFrameOrigin = url::Origin::Create(kTopFrameURL);
const net::IsolationInfo kIsolationInfo =
net::IsolationInfo::CreateForInternalRequest(kTopFrameOrigin);
service_->OverrideIsolationInfoForTesting(kIsolationInfo);

EXPECT_TRUE(sync_service_->SetCanonicalCookie(
*net::CanonicalCookie::Create(
kCookieURL,
"__Host-foo=bar; Secure; SameSite=None; Path=/; Partitioned",
base::Time::Now(), absl::nullopt /* server_time */,
net::CookiePartitionKey::FromScript()),
kCookieURL, kSiteForCookies, kTopFrameOrigin));

auto options = mojom::CookieManagerGetOptions::New();
options->name = "";
options->match_type = mojom::CookieMatchType::STARTS_WITH;

net::CookieList cookies = sync_service_->GetAllForUrl(
kCookieURL, kSiteForCookies, kTopFrameOrigin, std::move(options));
ASSERT_EQ(1u, cookies.size());
EXPECT_FALSE(cookies[0].IsPartitioned());
EXPECT_EQ("__Host-foo", cookies[0].Name());
}

// Test Partitioned cookie behavior when feature is enabled.
TEST_P(PartitionedCookiesEnabledRestrictedCookieManagerTest,
PartitionedCookies) {
Expand Down Expand Up @@ -1739,6 +1768,53 @@ TEST_P(PartitionedCookiesEnabledRestrictedCookieManagerTest,
second_listener->WaitForChange();
ASSERT_THAT(listener->observed_changes(), testing::SizeIs(0));
}

{ // Test that a cookie cannot be set with a different partition key than
// RestrictedCookieManager's.
service_->OverrideIsolationInfoForTesting(kIsolationInfo);
ExpectBadMessage();
EXPECT_FALSE(sync_service_->SetCanonicalCookie(
*net::CanonicalCookie::Create(
kCookieURL,
"__Host-foo=bar; Secure; SameSite=None; Path=/; Partitioned",
base::Time::Now(), absl::nullopt /* server_time */,
absl::make_optional(net::CookiePartitionKey::FromURLForTesting(
GURL("https://foo.bar.com")))),
kCookieURL, kSiteForCookies, kTopFrameOrigin));
}
}

TEST_P(PartitionedCookiesEnabledRestrictedCookieManagerTest,
PartitionKeyFromScript) {
const GURL kCookieURL("https://example.com");
const GURL kTopFrameURL("https://foo.com");
const net::SiteForCookies kSiteForCookies =
net::SiteForCookies::FromUrl(kTopFrameURL);
const url::Origin kTopFrameOrigin = url::Origin::Create(kTopFrameURL);
const net::IsolationInfo kIsolationInfo =
net::IsolationInfo::CreateForInternalRequest(kTopFrameOrigin);

service_->OverrideIsolationInfoForTesting(kIsolationInfo);
EXPECT_TRUE(sync_service_->SetCanonicalCookie(
*net::CanonicalCookie::Create(
kCookieURL,
"__Host-foo=bar; Secure; SameSite=None; Path=/; Partitioned",
base::Time::Now(), absl::nullopt /* server_time */,
net::CookiePartitionKey::FromScript()),
kCookieURL, kSiteForCookies, kTopFrameOrigin));

auto options = mojom::CookieManagerGetOptions::New();
options->name = "";
options->match_type = mojom::CookieMatchType::STARTS_WITH;

net::CookieList cookies = sync_service_->GetAllForUrl(
kCookieURL, kSiteForCookies, kTopFrameOrigin, std::move(options));
ASSERT_EQ(1u, cookies.size());
EXPECT_TRUE(cookies[0].IsPartitioned());
EXPECT_EQ(cookies[0].PartitionKey(),
net::CookiePartitionKey::FromNetworkIsolationKey(
kIsolationInfo.network_isolation_key()));
EXPECT_EQ("__Host-foo", cookies[0].Name());
}

INSTANTIATE_TEST_SUITE_P(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ CookieListItem* CookieChangeEvent::ToCookieListItem(
canonical_cookie.ExpiryDate().ToDoubleT()));
}
}

list_item->setPartitioned(canonical_cookie.IsPartitioned());

return list_item;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ dictionary CookieInit {
USVString path = "/";
DOMTimeStamp? expires = null;
CookieSameSite sameSite = "strict";
[RuntimeEnabled=PartitionedCookies] boolean partitioned = false;
};
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ dictionary CookieListItem {
DOMTimeStamp? expires;
boolean secure;
CookieSameSite sameSite;
[RuntimeEnabled=PartitionedCookies] boolean partitioned;
};

typedef sequence<CookieListItem> CookieList;
11 changes: 9 additions & 2 deletions third_party/blink/renderer/modules/cookie_store/cookie_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,20 @@ std::unique_ptr<net::CanonicalCookie> ToCanonicalCookie(
same_site = net::CookieSameSite::NO_RESTRICTION;
}

absl::optional<net::CookiePartitionKey> cookie_partition_key = absl::nullopt;
if (options->partitioned()) {
// We don't trust the renderer to determine the cookie partition key, so we
// use this factory to indicate we are using a temporary value here.
cookie_partition_key = net::CookiePartitionKey::FromScript();
}

// TODO(crbug.com/1144187): Add support for SameParty attribute.
// TODO(crbug.com/1225444): Add support for Partitioned attrbute.
return net::CanonicalCookie::CreateSanitizedCookie(
cookie_url, name.Utf8(), value.Utf8(), domain.Utf8(), path.Utf8(),
base::Time() /*creation*/, expires, base::Time() /*last_access*/,
true /*secure*/, false /*http_only*/, same_site,
net::CookiePriority::COOKIE_PRIORITY_DEFAULT, false /*same_party*/,
absl::nullopt /*partition_key*/);
cookie_partition_key);
}

const KURL DefaultCookieURL(ExecutionContext* execution_context) {
Expand Down Expand Up @@ -333,6 +339,7 @@ ScriptPromise CookieStore::Delete(ScriptState* script_state,
set_options->setDomain(options->domain());
set_options->setPath(options->path());
set_options->setSameSite("strict");
set_options->setPartitioned(options->partitioned());
return DoWrite(script_state, set_options, exception_state);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ dictionary CookieStoreDeleteOptions {
required USVString name;
USVString? domain = null;
USVString path = "/";
[RuntimeEnabled=PartitionedCookies] boolean partitioned = false;
};
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ This is a testharness.js-based test.
PASS Test partitioned cookies
PASS Partitioned cookies accessible on the top-level site they are created in via HTTP
PASS Partitioned cookies accessible on the top-level site they are created in via DOM
PASS Partitioned cookies accessible on the top-level site they are created in via CookieStore
PASS Cross-site window opened correctly
FAIL Partitioned cookies are not accessible on a different top-level site via HTTP assert_equals: Expected __Host-pchttp to not be available on a different top-level site expected false but got true
PASS Opening embedded cookie site frame
FAIL Partitioned cookies are not accessible on a different top-level site via DOM assert_equals: Expected __Host-pchttp to not be available on a different top-level site expected false but got true
FAIL Partitioned cookies are not accessible on a different top-level site via CookieStore assert_equals: Expected __Host-pchttp to not be available on a different top-level site expected false but got true
Harness: the test ran to completion.

Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,30 @@
const domCookieName = '__Host-pcdom';
document.cookie = `${domCookieName}=foobar;${attributes}`;

// Set another partitioned cookie using the CookieStore API.
const cookieStoreCookieName = '__Host-pccookistore';
await cookieStore.set({
name: cookieStoreCookieName,
value: 'foobar',
path: '/',
sameSite: 'none',
partitioned: true,
});

// Verify that the cookies are sent in requests from this top-level site.
testHttpPartitionedCookies({
origin: self.origin,
cookieNames: [httpCookieName, domCookieName],
cookieNames: [httpCookieName, domCookieName, cookieStoreCookieName],
expectsCookie: true,
});

// Verify that the cookies are exposed to the DOM on this top-level site.
testDomPartitionedCookies({
cookieNames: [httpCookieName, domCookieName],
cookieNames: [httpCookieName, domCookieName, cookieStoreCookieName],
expectsCookie: true,
});
testCookieStorePartitionedCookies({
cookieNames: [httpCookieName, domCookieName, cookieStoreCookieName],
expectsCookie: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,15 @@
<body>
<script>

const cookieNames = ['__Host-pchttp', '__Host-pcdom', '__Host-pccookiestore'];

testDomPartitionedCookies({
cookieNames: ['__Host-pchttp', '__Host-pcdom'],
cookieNames,
expectsCookie: false,
});

testCookieStorePartitionedCookies({
cookieNames,
expectsCookie: false,
});

Expand Down
Loading

0 comments on commit 4a461f1

Please sign in to comment.