Skip to content

Commit

Permalink
Remove kSameSiteByDefaultCookies and kCookiesWithoutSameSiteMustBeSecure
Browse files Browse the repository at this point in the history 
This change removes the above named base::Features, which are no longer
exposed via chrome://flags, and whose removal in M94 has been publicly
announced. The associated behaviors, SameSite-Lax-by-default and
SameSite=None-requires-Secure, have been standardized and launched in
Chromium as well as other browsers.

This change modifies CanonicalCookie logic to consider only the
CookieAccessSemantics when determining effective SameSite, adjusts
tests, and updates some comments.

(Schemeful Same-Site, i.e. net::features::kSchemefulSameSite, and other
cookie and SameSite-related base::Features are unaffected.)

Bug: 1211388
Change-Id: Id3ffca3a34dc64ab076c92506682ddaa5a674602
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3059260
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#906943}
  • Loading branch information
@chlily1
chlily1 authored and Chromium LUCI CQ committed Jul 30, 2021
1 parent 3b617f8 commit f98033a
Show file tree
Hide file tree
Showing 15 changed files with 264 additions and 655 deletions.
10 changes: 5 additions & 5 deletions components/content_settings/core/common/content_settings_types.h
View file
Expand Up @@ -146,11 +146,11 @@ enum class ContentSettingsType : int32_t {
WAKE_LOCK_SCREEN,
WAKE_LOCK_SYSTEM,

// Legacy SameSite cookie behavior. This disables SameSiteByDefaultCookies,
// CookiesWithoutSameSiteMustBeSecure, and SchemefulSameSite, forcing the
// legacy behavior wherein cookies that don't specify SameSite are treated as
// SameSite=None, SameSite=None cookies are not required to be Secure, and
// schemeful same-site is not active.
// Legacy SameSite cookie behavior. This disables SameSite=Lax-by-default,
// SameSite=None requires Secure, and Schemeful Same-Site, forcing the
// legacy behavior wherein 1) cookies that don't specify SameSite are treated
// as SameSite=None, 2) SameSite=None cookies are not required to be Secure,
// and 3) schemeful same-site is not active.
//
// This will also be used to revert to legacy behavior when future changes
// in cookie handling are introduced.
Expand Down
12 changes: 6 additions & 6 deletions content/browser/cookie_store/cookie_store_manager_unittest.cc
View file
Expand Up @@ -1228,7 +1228,7 @@ TEST_P(CookieStoreManagerTest, OneCookieChange) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);
}
Expand Down Expand Up @@ -1313,7 +1313,7 @@ TEST_P(CookieStoreManagerTest, CookieChangeNameStartsWith) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);

Expand All @@ -1331,7 +1331,7 @@ TEST_P(CookieStoreManagerTest, CookieChangeNameStartsWith) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);
}
Expand Down Expand Up @@ -1445,7 +1445,7 @@ TEST_P(CookieStoreManagerTest, CookieChangeUrl) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);

Expand All @@ -1462,7 +1462,7 @@ TEST_P(CookieStoreManagerTest, CookieChangeUrl) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);
}
Expand Down Expand Up @@ -1585,7 +1585,7 @@ TEST_P(CookieStoreManagerTest, HttpOnlyCookieChange) {
EXPECT_EQ(net::CookieChangeCause::INSERTED,
worker_test_helper_->changes()[0].cause);
// example.com does not have a custom access semantics setting, so it defaults
// to NONLEGACY, because the FeatureList has SameSiteByDefaultCookies enabled.
// to NONLEGACY.
EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
worker_test_helper_->changes()[0].access_result.access_semantics);
}
Expand Down
6 changes: 0 additions & 6 deletions content/public/common/content_switch_dependent_feature_overrides.cc
View file
Expand Up @@ -93,12 +93,6 @@ GetSwitchDependentFeatureOverrides(const base::CommandLine& command_line) {
{switches::kEnableExperimentalCookieFeatures,
std::cref(net::features::kCookieSameSiteConsidersRedirectChain),
base::FeatureList::OVERRIDE_ENABLE_FEATURE},
{switches::kEnableExperimentalCookieFeatures,
std::cref(net::features::kCookiesWithoutSameSiteMustBeSecure),
base::FeatureList::OVERRIDE_ENABLE_FEATURE},
{switches::kEnableExperimentalCookieFeatures,
std::cref(net::features::kSameSiteByDefaultCookies),
base::FeatureList::OVERRIDE_ENABLE_FEATURE},
{switches::kEnableExperimentalCookieFeatures,
std::cref(net::features::kSameSiteDefaultChecksMethodRigorously),
base::FeatureList::OVERRIDE_ENABLE_FEATURE},
Expand Down
6 changes: 1 addition & 5 deletions content/public/common/content_switches.cc
View file
Expand Up @@ -345,13 +345,9 @@ const char kEnableCaretBrowsing[] = "enable-caret-browsing";
// them off individually to identify the problematic feature anyway.
//
// At present this turns on:
// net::features::kCookiesWithoutSameSiteMustBeSecure
// net::features::kSameSiteByDefaultCookies
// net::features::kSameSiteDefaultChecksMethodRigorously
// It will soon also turn on:
// content_settings::kImprovedCookieControls
// content_settings::kImprovedCookieControlsForThirdPartyCookieBlocking
// net::features::kSchemefulSameSite
// net::features::kCookieSameSiteConsidersRedirectChain
const char kEnableExperimentalCookieFeatures[] =
"enable-experimental-cookie-features";

Expand Down
6 changes: 0 additions & 6 deletions net/base/features.cc
View file
Expand Up @@ -151,12 +151,6 @@ const base::FeatureParam<std::string> kPostQuantumCECPQ2InitialLetters(
const base::Feature kNetUnusedIdleSocketTimeout{
"NetUnusedIdleSocketTimeout", base::FEATURE_DISABLED_BY_DEFAULT};

const base::Feature kSameSiteByDefaultCookies{"SameSiteByDefaultCookies",
base::FEATURE_ENABLED_BY_DEFAULT};

const base::Feature kCookiesWithoutSameSiteMustBeSecure{
"CookiesWithoutSameSiteMustBeSecure", base::FEATURE_ENABLED_BY_DEFAULT};

const base::Feature kShortLaxAllowUnsafeThreshold{
"ShortLaxAllowUnsafeThreshold", base::FEATURE_DISABLED_BY_DEFAULT};

Expand Down
16 changes: 0 additions & 16 deletions net/base/features.h
View file
Expand Up @@ -236,22 +236,6 @@ NET_EXPORT extern const base::FeatureParam<std::string>
// Changes the timeout after which unused sockets idle sockets are cleaned up.
NET_EXPORT extern const base::Feature kNetUnusedIdleSocketTimeout;

// When enabled, makes cookies without a SameSite attribute behave like
// SameSite=Lax cookies by default, and requires SameSite=None to be specified
// in order to make cookies available in a third-party context. When disabled,
// the default behavior for cookies without a SameSite attribute specified is no
// restriction, i.e., available in a third-party context.
// The "Lax-allow-unsafe" mitigation allows these cookies to be sent on
// top-level cross-site requests with an unsafe (e.g. POST) HTTP method, if the
// cookie is no more than 2 minutes old.
NET_EXPORT extern const base::Feature kSameSiteByDefaultCookies;

// When enabled, cookies without SameSite restrictions that don't specify the
// Secure attribute will be rejected if set from an insecure context, or treated
// as secure if set from a secure context. This ONLY has an effect if
// SameSiteByDefaultCookies is also enabled.
NET_EXPORT extern const base::Feature kCookiesWithoutSameSiteMustBeSecure;

// When enabled, the time threshold for Lax-allow-unsafe cookies will be lowered
// from 2 minutes to 10 seconds. This time threshold refers to the age cutoff
// for which cookies that default into SameSite=Lax, which are newer than the
Expand Down
36 changes: 12 additions & 24 deletions net/cookies/canonical_cookie.cc
View file
Expand Up @@ -936,13 +936,11 @@ CookieAccessResult CanonicalCookie::IncludeForRequestURL(
break;
}

// If both SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
// are enabled, non-SameSite cookies without the Secure attribute should be
// ignored. This can apply to cookies which were created before the
// experimental options were enabled (as non-SameSite, insecure cookies cannot
// be set while the options are on).
// Unless legacy access semantics are in effect, SameSite=None cookies without
// the Secure attribute should be ignored. This can apply to cookies which
// were created before "SameSite=None requires Secure" was enabled (as
// SameSite=None insecure cookies cannot be set while the options are on).
if (params.access_semantics != CookieAccessSemantics::LEGACY &&
cookie_util::IsCookiesWithoutSameSiteMustBeSecureEnabled() &&
SameSite() == CookieSameSite::NO_RESTRICTION && !IsSecure()) {
status.AddExclusionReason(
CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE);
Expand Down Expand Up @@ -1117,11 +1115,9 @@ CookieAccessResult CanonicalCookie::IsSetPermittedInContext(
CookieInclusionStatus::EXCLUDE_HTTP_ONLY);
}

// If both SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
// are enabled, non-SameSite cookies without the Secure attribute will be
// rejected.
// Unless legacy access semantics are in effect, SameSite=None cookies without
// the Secure attribute will be rejected.
if (params.access_semantics != CookieAccessSemantics::LEGACY &&
cookie_util::IsCookiesWithoutSameSiteMustBeSecureEnabled() &&
SameSite() == CookieSameSite::NO_RESTRICTION && !IsSecure()) {
DVLOG(net::cookie_util::kVlogSetCookies)
<< "SetCookie() rejecting insecure cookie with SameSite=None.";
Expand Down Expand Up @@ -1446,24 +1442,16 @@ CookieEffectiveSameSite CanonicalCookie::GetEffectiveSameSite(
? kShortLaxAllowUnsafeMaxAge
: kLaxAllowUnsafeMaxAge);

bool should_apply_same_site_lax_by_default =
cookie_util::IsSameSiteByDefaultCookiesEnabled();
if (access_semantics == CookieAccessSemantics::LEGACY) {
should_apply_same_site_lax_by_default = false;
} else if (access_semantics == CookieAccessSemantics::NONLEGACY) {
should_apply_same_site_lax_by_default = true;
}

switch (SameSite()) {
// If a cookie does not have a SameSite attribute, the effective SameSite
// mode depends on the SameSiteByDefaultCookies setting and whether the
// cookie is recently-created.
// mode depends on the access semantics and whether the cookie is
// recently-created.
case CookieSameSite::UNSPECIFIED:
return should_apply_same_site_lax_by_default
? (IsRecentlyCreated(lax_allow_unsafe_threshold_age)
return (access_semantics == CookieAccessSemantics::LEGACY)
? CookieEffectiveSameSite::NO_RESTRICTION
: (IsRecentlyCreated(lax_allow_unsafe_threshold_age)
? CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE
: CookieEffectiveSameSite::LAX_MODE)
: CookieEffectiveSameSite::NO_RESTRICTION;
: CookieEffectiveSameSite::LAX_MODE);
case CookieSameSite::NO_RESTRICTION:
return CookieEffectiveSameSite::NO_RESTRICTION;
case CookieSameSite::LAX_MODE:
Expand Down
5 changes: 2 additions & 3 deletions net/cookies/canonical_cookie.h
View file
Expand Up @@ -430,9 +430,8 @@ class NET_EXPORT CanonicalCookie {
const std::string& path);

// Returns the effective SameSite mode to apply to this cookie. Depends on the
// value of the given SameSite attribute and whether the
// SameSiteByDefaultCookies feature is enabled, as well as the access
// semantics of the cookie.
// value of the given SameSite attribute and the access semantics of the
// cookie.
// Note: If you are converting to a different representation of a cookie, you
// probably want to use SameSite() instead of this method. Otherwise, if you
// are considering using this method, consider whether you should use
Expand Down

0 comments on commit f98033a

Please sign in to comment.