SameSite cookies: Hide cross-site redirect chain check behind a Feature
This adds a base::Feature (disabled by default) to control the redirect
chain checking for SameSite cookies.
When the feature is enabled, any cross-site redirect hop makes a request
cross-site. This is consistent with the behavior specified by RFC
6265bis as of https://github.com/httpwg/http-extensions/pull/1348.
When the feature is disabled, only the site-for-cookies and
initiator are considered, relative to the request URL. This is
consistent with the behavior specified by previous versions of the spec.
The redirect chain checking behavior is being hidden behind a Feature
due to site breakage. We intend to re-enable it later.
The new base::Feature is added to the set of features enabled by
--enable-experimental-web-platform-features, so that web tests can run
with the feature enabled (so as to match Firefox's behavior).
The new base::Feature is also added to the set of features enabled by
--enable-experimental-cookie-features.
Bug: 1215167
Change-Id: Ie9a637f8ab0921f13559254caba93772ba8990fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2930367
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#888696}
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index b54c252..2025b14 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -67,6 +67,7 @@
#include "content/test/content_browser_test_utils_internal.h"
#include "content/test/fake_network_url_loader_factory.h"
#include "content/test/test_content_browser_client.h"
+#include "net/base/features.h"
#include "net/base/filename_util.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/controllable_http_response.h"
@@ -3989,7 +3990,25 @@
download->GetTargetFilePath().BaseName().value().c_str());
}
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadAttributeSameSiteCookie) {
+class DownloadContentSameSiteCookieTest
+ : public DownloadContentTest,
+ public ::testing::WithParamInterface<bool> {
+ public:
+ DownloadContentSameSiteCookieTest() {
+ if (DoesCookieSameSiteConsiderRedirectChain()) {
+ inner_feature_list_.InitAndEnableFeature(
+ net::features::kCookieSameSiteConsidersRedirectChain);
+ }
+ }
+
+ bool DoesCookieSameSiteConsiderRedirectChain() { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList inner_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(DownloadContentSameSiteCookieTest,
+ DownloadAttributeSameSiteCookie) {
base::ScopedAllowBlockingForTesting allow_blocking;
net::EmbeddedTestServer test_server;
ASSERT_TRUE(test_server.InitializeAndListen());
@@ -4048,7 +4067,8 @@
EXPECT_STREQ("B=C", file_contents.c_str());
// OriginOne redirects through OriginTwo. Because the redirect chain contains
- // a cross-site redirect, SameSite=Strict cookies are not sent.
+ // a cross-site redirect, SameSite=Strict cookies are not sent (if redirect
+ // chains are considered).
//
// Initiator origin: kOriginOne
// Redirect chain contains: kOriginTwo
@@ -4063,9 +4083,17 @@
ASSERT_TRUE(
base::ReadFileToString(download->GetTargetFilePath(), &file_contents));
- EXPECT_STREQ("B=C", file_contents.c_str());
+ if (DoesCookieSameSiteConsiderRedirectChain()) {
+ EXPECT_STREQ("B=C", file_contents.c_str());
+ } else {
+ EXPECT_STREQ("A=B; B=C", file_contents.c_str());
+ }
}
+INSTANTIATE_TEST_SUITE_P(/* no label */,
+ DownloadContentSameSiteCookieTest,
+ ::testing::Bool());
+
// The file empty.bin is served with a MIME type of application/octet-stream.
// The content body is empty. Make sure this case is handled properly and we
// don't regress on http://crbug.com/320394.
diff --git a/content/browser/net/http_cookie_browsertest.cc b/content/browser/net/http_cookie_browsertest.cc
index ee7dd8e..f2dc5a6 100644
--- a/content/browser/net/http_cookie_browsertest.cc
+++ b/content/browser/net/http_cookie_browsertest.cc
@@ -5,6 +5,7 @@
#include "base/strings/strcat.h"
#include "base/strings/string_split.h"
#include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/web_contents.h"
@@ -34,10 +35,15 @@
// See also (tests for cookie access via JavaScript):
// //content/browser/renderer_host/cookie_browsertest.cc
-class HttpCookieBrowserTest : public ContentBrowserTest {
+class HttpCookieBrowserTest : public ContentBrowserTest,
+ public ::testing::WithParamInterface<bool> {
public:
- HttpCookieBrowserTest()
- : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {}
+ HttpCookieBrowserTest() : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
+ if (DoesSameSiteConsiderRedirectChain()) {
+ feature_list_.InitAndEnableFeature(
+ net::features::kCookieSameSiteConsidersRedirectChain);
+ }
+ }
~HttpCookieBrowserTest() override = default;
@@ -57,6 +63,8 @@
kHostC));
}
+ bool DoesSameSiteConsiderRedirectChain() { return GetParam(); }
+
const char* kHostA = "a.test";
const char* kHostB = "b.test";
const char* kHostC = "c.test";
@@ -217,9 +225,10 @@
private:
net::test_server::EmbeddedTestServer https_server_;
+ base::test::ScopedFeatureList feature_list_;
};
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SendSameSiteCookies) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SendSameSiteCookies) {
SetSameSiteCookies(kHostA);
SetSameSiteCookies(kHostB);
@@ -267,7 +276,7 @@
net::CookieStringIs(UnorderedElementsAre(Key(kSameSiteNoneCookieName))));
}
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SendSameSiteCookies_Redirect) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SendSameSiteCookies_Redirect) {
SetSameSiteCookies(kHostA);
WebContentsImpl* web_contents =
@@ -285,29 +294,56 @@
Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
Key(kSameSiteNoneCookieName), Key(kSameSiteUnspecifiedCookieName))));
- // Main frame cross-site redirect (B->A) sends Lax but not Strict SameSite
- // cookies...
- ASSERT_TRUE(NavigateToURL(
- web_contents,
- RedirectUrl(https_server(), kHostB,
- EchoCookiesUrl(https_server(), kHostA)),
- /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
- EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
- net::CookieStringIs(UnorderedElementsAre(
- Key(kSameSiteLaxCookieName), Key(kSameSiteNoneCookieName),
- Key(kSameSiteUnspecifiedCookieName))));
+ if (DoesSameSiteConsiderRedirectChain()) {
+ // Main frame cross-site redirect (B->A) sends Lax but not Strict SameSite
+ // cookies...
+ ASSERT_TRUE(NavigateToURL(
+ web_contents,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA)),
+ /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
+ EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteLaxCookieName), Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
- // ... even if the first URL is same-site. (A->B->A)
- ASSERT_TRUE(NavigateToURL(
- web_contents,
- RedirectUrl(https_server(), kHostA,
- RedirectUrl(https_server(), kHostB,
- EchoCookiesUrl(https_server(), kHostA))),
- /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
- EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
- net::CookieStringIs(UnorderedElementsAre(
- Key(kSameSiteLaxCookieName), Key(kSameSiteNoneCookieName),
- Key(kSameSiteUnspecifiedCookieName))));
+ // ... even if the first URL is same-site. (A->B->A)
+ ASSERT_TRUE(NavigateToURL(
+ web_contents,
+ RedirectUrl(https_server(), kHostA,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA))),
+ /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
+ EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteLaxCookieName), Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
+ } else {
+ // If redirect chains are not considered, then cross-site redirects do not
+ // make the request cross-site.
+ ASSERT_TRUE(NavigateToURL(
+ web_contents,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA)),
+ /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
+ EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
+ Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
+
+ ASSERT_TRUE(NavigateToURL(
+ web_contents,
+ RedirectUrl(https_server(), kHostA,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA))),
+ /*expected_commit_url=*/EchoCookiesUrl(https_server(), kHostA)));
+ EXPECT_THAT(ExtractFrameContent(web_contents->GetMainFrame()),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
+ Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
+ }
// A same-site redirected iframe (A->A embedded in A) sends all SameSite
// cookies.
@@ -320,26 +356,51 @@
Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
Key(kSameSiteNoneCookieName), Key(kSameSiteUnspecifiedCookieName))));
- // A cross-site redirected iframe in a same-site context (B->A embedded in A)
- // does not send SameSite cookies...
- EXPECT_THAT(
- ArrangeFramesAndGetContentFromLeaf(
- "a.test(%s)", {0},
- RedirectUrl(https_server(), kHostB,
- EchoCookiesUrl(https_server(), kHostA))),
- net::CookieStringIs(UnorderedElementsAre(Key(kSameSiteNoneCookieName))));
+ if (DoesSameSiteConsiderRedirectChain()) {
+ // A cross-site redirected iframe in a same-site context (B->A embedded in
+ // A) does not send SameSite cookies...
+ EXPECT_THAT(ArrangeFramesAndGetContentFromLeaf(
+ "a.test(%s)", {0},
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA))),
+ net::CookieStringIs(
+ UnorderedElementsAre(Key(kSameSiteNoneCookieName))));
- // ... even if the first URL is same-site. (A->B->A embedded in A)
- EXPECT_THAT(
- ArrangeFramesAndGetContentFromLeaf(
- "a.test(%s)", {0},
- RedirectUrl(https_server(), kHostA,
- RedirectUrl(https_server(), kHostB,
- EchoCookiesUrl(https_server(), kHostA)))),
- net::CookieStringIs(UnorderedElementsAre(Key(kSameSiteNoneCookieName))));
+ // ... even if the first URL is same-site. (A->B->A embedded in A)
+ EXPECT_THAT(
+ ArrangeFramesAndGetContentFromLeaf(
+ "a.test(%s)", {0},
+ RedirectUrl(https_server(), kHostA,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA)))),
+ net::CookieStringIs(
+ UnorderedElementsAre(Key(kSameSiteNoneCookieName))));
+ } else {
+ // If redirect chains are not considered, then cross-site redirects do not
+ // make the request cross-site.
+ EXPECT_THAT(ArrangeFramesAndGetContentFromLeaf(
+ "a.test(%s)", {0},
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA))),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
+ Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
+
+ EXPECT_THAT(
+ ArrangeFramesAndGetContentFromLeaf(
+ "a.test(%s)", {0},
+ RedirectUrl(https_server(), kHostA,
+ RedirectUrl(https_server(), kHostB,
+ EchoCookiesUrl(https_server(), kHostA)))),
+ net::CookieStringIs(UnorderedElementsAre(
+ Key(kSameSiteStrictCookieName), Key(kSameSiteLaxCookieName),
+ Key(kSameSiteNoneCookieName),
+ Key(kSameSiteUnspecifiedCookieName))));
+ }
}
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SetSameSiteCookies) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SetSameSiteCookies) {
WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(shell()->web_contents());
@@ -373,7 +434,7 @@
ASSERT_EQ(1U, ClearCookies());
}
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SetSameSiteCookies_Redirect) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SetSameSiteCookies_Redirect) {
WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(shell()->web_contents());
@@ -420,18 +481,33 @@
net::MatchesCookieWithName(kSameSiteUnspecifiedCookieName)));
ASSERT_EQ(4U, ClearCookies());
- // A cross-site redirected iframe only sets SameSite=None cookies.
- EXPECT_THAT(ArrangeFramesAndGetCanonicalCookiesForLeaf(
- "a.test(%s)",
- RedirectUrl(https_server(), kHostB,
- SetSameSiteCookiesUrl(https_server(), kHostA)),
- https_server()->GetURL(kHostA, "/")),
- UnorderedElementsAre(
- net::MatchesCookieWithName(kSameSiteNoneCookieName)));
- ASSERT_EQ(1U, ClearCookies());
+ if (DoesSameSiteConsiderRedirectChain()) {
+ // A cross-site redirected iframe only sets SameSite=None cookies.
+ EXPECT_THAT(ArrangeFramesAndGetCanonicalCookiesForLeaf(
+ "a.test(%s)",
+ RedirectUrl(https_server(), kHostB,
+ SetSameSiteCookiesUrl(https_server(), kHostA)),
+ https_server()->GetURL(kHostA, "/")),
+ UnorderedElementsAre(
+ net::MatchesCookieWithName(kSameSiteNoneCookieName)));
+ ASSERT_EQ(1U, ClearCookies());
+ } else {
+ EXPECT_THAT(
+ ArrangeFramesAndGetCanonicalCookiesForLeaf(
+ "a.test(%s)",
+ RedirectUrl(https_server(), kHostB,
+ SetSameSiteCookiesUrl(https_server(), kHostA)),
+ https_server()->GetURL(kHostA, "/")),
+ UnorderedElementsAre(
+ net::MatchesCookieWithName(kSameSiteStrictCookieName),
+ net::MatchesCookieWithName(kSameSiteLaxCookieName),
+ net::MatchesCookieWithName(kSameSiteNoneCookieName),
+ net::MatchesCookieWithName(kSameSiteUnspecifiedCookieName)));
+ ASSERT_EQ(4U, ClearCookies());
+ }
}
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SendSamePartyCookies) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SendSamePartyCookies) {
SetSamePartyCookies(kHostA);
SetSamePartyCookies(kHostD);
@@ -515,7 +591,7 @@
Key(kSamePartyUnspecifiedCookieName))));
}
-IN_PROC_BROWSER_TEST_F(HttpCookieBrowserTest, SetSamePartyCookies) {
+IN_PROC_BROWSER_TEST_P(HttpCookieBrowserTest, SetSamePartyCookies) {
WebContentsImpl* web_contents =
static_cast<WebContentsImpl*>(shell()->web_contents());
@@ -606,5 +682,9 @@
ASSERT_EQ(3U, ClearCookies());
}
+INSTANTIATE_TEST_SUITE_P(/* no label */,
+ HttpCookieBrowserTest,
+ ::testing::Bool());
+
} // namespace
} // namespace content
diff --git a/content/public/common/content_switch_dependent_feature_overrides.cc b/content/public/common/content_switch_dependent_feature_overrides.cc
index 763db06..47fb1cc 100644
--- a/content/public/common/content_switch_dependent_feature_overrides.cc
+++ b/content/public/common/content_switch_dependent_feature_overrides.cc
@@ -29,8 +29,12 @@
{switches::kAppCacheForceEnabled,
std::cref(blink::features::kAppCacheRequireOriginTrial),
base::FeatureList::OVERRIDE_DISABLE_FEATURE},
+
// Overrides for --enable-experimental-web-platform-features.
{switches::kEnableExperimentalWebPlatformFeatures,
+ std::cref(net::features::kCookieSameSiteConsidersRedirectChain),
+ base::FeatureList::OVERRIDE_ENABLE_FEATURE},
+ {switches::kEnableExperimentalWebPlatformFeatures,
std::cref(network::features::kCrossOriginEmbedderPolicyCredentialless),
base::FeatureList::OVERRIDE_ENABLE_FEATURE},
{switches::kEnableExperimentalWebPlatformFeatures,
@@ -84,6 +88,9 @@
// Overrides for --enable-experimental-cookie-features.
{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,
diff --git a/net/base/features.cc b/net/base/features.cc
index 54f8b3e..630d372d 100644
--- a/net/base/features.cc
+++ b/net/base/features.cc
@@ -239,5 +239,8 @@
base::FEATURE_DISABLED_BY_DEFAULT};
#endif // defined(OS_POSIX) || defined(OS_FUCHSIA)
+const base::Feature kCookieSameSiteConsidersRedirectChain{
+ "CookieSameSiteConsidersRedirectChain", base::FEATURE_DISABLED_BY_DEFAULT};
+
} // namespace features
} // namespace net
diff --git a/net/base/features.h b/net/base/features.h
index b91cc925..b97d7dd 100644
--- a/net/base/features.h
+++ b/net/base/features.h
@@ -348,6 +348,14 @@
NET_EXPORT extern const base::Feature kUdpSocketPosixAlwaysUpdateBytesReceived;
#endif // defined(OS_POSIX) || defined(OS_FUCHSIA)
+// When this feature is enabled, redirected requests will be considered
+// cross-site for the purpose of SameSite cookies if any redirect hop was
+// cross-site to the target URL, even if the original initiator of the
+// redirected request was same-site with the target URL (and the
+// site-for-cookies).
+// See spec changes in https://github.com/httpwg/http-extensions/pull/1348
+NET_EXPORT extern const base::Feature kCookieSameSiteConsidersRedirectChain;
+
} // namespace features
} // namespace net
diff --git a/net/cookies/cookie_util.cc b/net/cookies/cookie_util.cc
index e1ecfcbd5..5300a57 100644
--- a/net/cookies/cookie_util.cc
+++ b/net/cookies/cookie_util.cc
@@ -146,8 +146,12 @@
url_chain.size() == 1u ||
base::ranges::all_of(url_chain, is_same_site_with_site_for_cookies);
- if (same_site_initiator && same_site_redirect_chain)
+ if (same_site_initiator &&
+ (!base::FeatureList::IsEnabled(
+ features::kCookieSameSiteConsidersRedirectChain) ||
+ same_site_redirect_chain)) {
return {ContextType::SAME_SITE_STRICT, false};
+ }
if (is_http) {
base::UmaHistogramBoolean("Cookie.SameSiteContextAffectedByBugfix1166211",
diff --git a/net/cookies/cookie_util_unittest.cc b/net/cookies/cookie_util_unittest.cc
index 166b397..e41c755 100644
--- a/net/cookies/cookie_util_unittest.cc
+++ b/net/cookies/cookie_util_unittest.cc
@@ -363,17 +363,25 @@
// Tests for the various ComputeSameSiteContextFor*() functions. The first
// boolean test param is whether the results of the computations are evaluated
// schemefully. The second boolean test param is whether the fix for
-// crbug.com/1166211 is enabled.
+// crbug.com/1166211 is enabled. The third boolean param is whether SameSite
+// considers redirect chains.
class CookieUtilComputeSameSiteContextTest
- : public ::testing::TestWithParam<std::tuple<bool, bool>> {
+ : public ::testing::TestWithParam<std::tuple<bool, bool, bool>> {
public:
CookieUtilComputeSameSiteContextTest() {
- // No need to explicitly enable the feature because it is enabled by
- // default.
- if (!IsBugfix1166211Enabled()) {
- feature_list_.InitAndDisableFeature(
- features::kSameSiteCookiesBugfix1166211);
+ std::vector<base::Feature> enabled_features;
+ std::vector<base::Feature> disabled_features;
+ // No need to explicitly enable the Bugfix1166211 feature because it
+ // is enabled by default.
+ if (!IsBugfix1166211Enabled())
+ disabled_features.push_back(features::kSameSiteCookiesBugfix1166211);
+ // No need to explicitly disable the redirect chain feature because it
+ // is disabled by default.
+ if (DoesSameSiteConsiderRedirectChain()) {
+ enabled_features.push_back(
+ features::kCookieSameSiteConsidersRedirectChain);
}
+ feature_list_.InitWithFeatures(enabled_features, disabled_features);
}
~CookieUtilComputeSameSiteContextTest() override = default;
@@ -384,6 +392,10 @@
bool IsBugfix1166211Enabled() const { return std::get<1>(GetParam()); }
+ bool DoesSameSiteConsiderRedirectChain() const {
+ return std::get<2>(GetParam());
+ }
+
// Returns the proper gtest matcher to use for the schemeless/schemeful mode.
auto ContextTypeIs(ContextType context_type) const {
return ContextTypeIsWithSchemefulMode(context_type, IsSchemeful());
@@ -967,38 +979,66 @@
bool url_chain_is_same_site;
bool site_for_cookies_is_same_site;
bool initiator_is_same_site;
+ // These are the expected context types considering redirect chains:
ContextType expected_context_type; // for non-main-frame-nav requests.
ContextType expected_context_type_for_main_frame_navigation;
+ // These are the expected context types not considering redirect chains:
+ ContextType expected_context_type_without_chain;
+ ContextType expected_context_type_for_main_frame_navigation_without_chain;
} kTestCases[] = {
+ // If the url chain is same-site, then the result is the same with or
+ // without considering the redirect chain.
{"GET", true, true, true, ContextType::SAME_SITE_STRICT,
+ ContextType::SAME_SITE_STRICT, ContextType::SAME_SITE_STRICT,
ContextType::SAME_SITE_STRICT},
- {"GET", true, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX},
+ {"GET", true, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ incorrectly_lax, ContextType::SAME_SITE_LAX},
{"GET", true, false, true, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
{"GET", true, false, false, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
- {"GET", false, true, true, incorrectly_lax, ContextType::SAME_SITE_LAX},
- {"GET", false, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX},
+ // If the url chain is cross-site, then the result will differ depending
+ // on whether the redirect chain is considered, when the site-for-cookies
+ // and initiator are both same-site.
+ {"GET", false, true, true, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ ContextType::SAME_SITE_STRICT, ContextType::SAME_SITE_STRICT},
+ {"GET", false, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ incorrectly_lax, ContextType::SAME_SITE_LAX},
{"GET", false, false, true, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
{"GET", false, false, false, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
+ // If the url chain is same-site, then the result is the same with or
+ // without considering the redirect chain.
{"POST", true, true, true, ContextType::SAME_SITE_STRICT,
+ ContextType::SAME_SITE_STRICT, ContextType::SAME_SITE_STRICT,
ContextType::SAME_SITE_STRICT},
{"POST", true, true, false, incorrectly_lax_unsafe,
+ ContextType::SAME_SITE_LAX_METHOD_UNSAFE, incorrectly_lax_unsafe,
ContextType::SAME_SITE_LAX_METHOD_UNSAFE},
{"POST", true, false, true, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
{"POST", true, false, false, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE,
ContextType::CROSS_SITE},
+ // If the url chain is cross-site, then the result will differ depending
+ // on whether the redirect chain is considered, when the site-for-cookies
+ // and initiator are both same-site.
{"POST", false, true, true, incorrectly_lax_unsafe,
- ContextType::SAME_SITE_LAX_METHOD_UNSAFE},
+ ContextType::SAME_SITE_LAX_METHOD_UNSAFE, ContextType::SAME_SITE_STRICT,
+ ContextType::SAME_SITE_STRICT},
{"POST", false, true, false, incorrectly_lax_unsafe,
+ ContextType::SAME_SITE_LAX_METHOD_UNSAFE, incorrectly_lax_unsafe,
ContextType::SAME_SITE_LAX_METHOD_UNSAFE},
{"POST", false, false, true, ContextType::CROSS_SITE,
- ContextType::CROSS_SITE},
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
{"POST", false, false, false, ContextType::CROSS_SITE,
- ContextType::CROSS_SITE},
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
};
for (const auto& test_case : kTestCases) {
@@ -1011,6 +1051,15 @@
std::vector<absl::optional<url::Origin>> initiators =
test_case.initiator_is_same_site ? GetSameSiteInitiators()
: GetCrossSiteInitiators();
+ ContextType expected_context_type =
+ DoesSameSiteConsiderRedirectChain()
+ ? test_case.expected_context_type
+ : test_case.expected_context_type_without_chain;
+ ContextType expected_context_type_for_main_frame_navigation =
+ DoesSameSiteConsiderRedirectChain()
+ ? test_case.expected_context_type_for_main_frame_navigation
+ : test_case
+ .expected_context_type_for_main_frame_navigation_without_chain;
for (const std::vector<GURL>& url_chain : url_chains) {
for (const SiteForCookies& site_for_cookies : sites_for_cookies) {
for (const absl::optional<url::Origin>& initiator : initiators) {
@@ -1018,7 +1067,7 @@
test_case.method, url_chain, site_for_cookies,
initiator, false /* is_main_frame_navigation */,
false /* force_ignore_site_for_cookies */),
- ContextTypeIs(test_case.expected_context_type))
+ ContextTypeIs(expected_context_type))
<< UrlChainToString(url_chain) << " "
<< site_for_cookies.ToDebugString() << " "
<< (initiator ? initiator->Serialize() : "nullopt");
@@ -1029,8 +1078,7 @@
test_case.method, url_chain, site_for_cookies, initiator,
true /* is_main_frame_navigation */,
false /* force_ignore_site_for_cookies */),
- ContextTypeIs(
- test_case.expected_context_type_for_main_frame_navigation))
+ ContextTypeIs(expected_context_type_for_main_frame_navigation))
<< UrlChainToString(url_chain) << " "
<< site_for_cookies.ToDebugString() << " "
<< (initiator ? initiator->Serialize() : "nullopt");
@@ -1220,18 +1268,34 @@
bool url_chain_is_same_site;
bool site_for_cookies_is_same_site;
bool initiator_is_same_site;
+ // These are the expected context types considering redirect chains:
ContextType expected_context_type; // for non-main-frame-nav requests.
ContextType expected_context_type_for_main_frame_navigation;
+ // These are the expected context types not considering redirect chains:
+ ContextType expected_context_type_without_chain;
+ ContextType expected_context_type_for_main_frame_navigation_without_chain;
} kTestCases[] = {
- {true, true, true, ContextType::SAME_SITE_LAX,
- ContextType::SAME_SITE_LAX},
- {true, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX},
- {true, false, true, ContextType::CROSS_SITE, ContextType::CROSS_SITE},
- {true, false, false, ContextType::CROSS_SITE, ContextType::CROSS_SITE},
- {false, true, true, incorrectly_lax, ContextType::SAME_SITE_LAX},
- {false, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX},
- {false, false, true, ContextType::CROSS_SITE, ContextType::CROSS_SITE},
- {false, false, false, ContextType::CROSS_SITE, ContextType::CROSS_SITE},
+ // If the url chain is same-site, then the result is the same with or
+ // without considering the redirect chain.
+ {true, true, true, ContextType::SAME_SITE_LAX, ContextType::SAME_SITE_LAX,
+ ContextType::SAME_SITE_LAX, ContextType::SAME_SITE_LAX},
+ {true, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ incorrectly_lax, ContextType::SAME_SITE_LAX},
+ {true, false, true, ContextType::CROSS_SITE, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
+ {true, false, false, ContextType::CROSS_SITE, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
+ // If the url chain is cross-site, then the result will differ depending
+ // on whether the redirect chain is considered, when the site-for-cookies
+ // and initiator are both same-site.
+ {false, true, true, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ ContextType::SAME_SITE_LAX, ContextType::SAME_SITE_LAX},
+ {false, true, false, incorrectly_lax, ContextType::SAME_SITE_LAX,
+ incorrectly_lax, ContextType::SAME_SITE_LAX},
+ {false, false, true, ContextType::CROSS_SITE, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
+ {false, false, false, ContextType::CROSS_SITE, ContextType::CROSS_SITE,
+ ContextType::CROSS_SITE, ContextType::CROSS_SITE},
};
for (const auto& test_case : kTestCases) {
std::vector<std::vector<GURL>> url_chains =
@@ -1243,6 +1307,15 @@
std::vector<absl::optional<url::Origin>> initiators =
test_case.initiator_is_same_site ? GetSameSiteInitiators()
: GetCrossSiteInitiators();
+ ContextType expected_context_type =
+ DoesSameSiteConsiderRedirectChain()
+ ? test_case.expected_context_type
+ : test_case.expected_context_type_without_chain;
+ ContextType expected_context_type_for_main_frame_navigation =
+ DoesSameSiteConsiderRedirectChain()
+ ? test_case.expected_context_type_for_main_frame_navigation
+ : test_case
+ .expected_context_type_for_main_frame_navigation_without_chain;
for (const std::vector<GURL>& url_chain : url_chains) {
for (const SiteForCookies& site_for_cookies : sites_for_cookies) {
for (const absl::optional<url::Origin>& initiator : initiators) {
@@ -1250,7 +1323,7 @@
url_chain, site_for_cookies, initiator,
false /* is_main_frame_navigation */,
false /* force_ignore_site_for_cookies */),
- ContextTypeIs(test_case.expected_context_type))
+ ContextTypeIs(expected_context_type))
<< UrlChainToString(url_chain) << " "
<< site_for_cookies.ToDebugString() << " "
<< (initiator ? initiator->Serialize() : "nullopt");
@@ -1261,8 +1334,7 @@
url_chain, site_for_cookies, initiator,
true /* is_main_frame_navigation */,
false /* force_ignore_site_for_cookies */),
- ContextTypeIs(
- test_case.expected_context_type_for_main_frame_navigation))
+ ContextTypeIs(expected_context_type_for_main_frame_navigation))
<< UrlChainToString(url_chain) << " "
<< site_for_cookies.ToDebugString() << " "
<< (initiator ? initiator->Serialize() : "nullopt");
@@ -1367,6 +1439,7 @@
INSTANTIATE_TEST_SUITE_P(/* no label */,
CookieUtilComputeSameSiteContextTest,
::testing::Combine(::testing::Bool(),
+ ::testing::Bool(),
::testing::Bool()));
TEST(CookieUtilTest, AdaptCookieAccessResultToBool) {
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index cced331..4a41d3a3 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -2185,7 +2185,26 @@
}
}
-TEST_F(URLRequestTest, SameSiteCookies) {
+// Tests for SameSite cookies. The test param indicates whether the same-site
+// calculation considers redirect chains.
+class URLRequestSameSiteCookiesTest
+ : public URLRequestTest,
+ public ::testing::WithParamInterface<bool> {
+ public:
+ URLRequestSameSiteCookiesTest() {
+ if (DoesCookieSameSiteConsiderRedirectChain()) {
+ feature_list_.InitAndEnableFeature(
+ features::kCookieSameSiteConsidersRedirectChain);
+ }
+ }
+
+ bool DoesCookieSameSiteConsiderRedirectChain() { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_P(URLRequestSameSiteCookiesTest, SameSiteCookies) {
HttpTestServer test_server;
ASSERT_TRUE(test_server.Start());
@@ -2390,7 +2409,7 @@
}
}
-TEST_F(URLRequestTest, SameSiteCookies_Redirect) {
+TEST_P(URLRequestSameSiteCookiesTest, SameSiteCookies_Redirect) {
EmbeddedTestServer http_server;
RegisterDefaultHandlers(&http_server);
EmbeddedTestServer https_server(EmbeddedTestServer::TYPE_HTTPS);
@@ -2505,10 +2524,14 @@
EXPECT_NE(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
}
+ // If redirect chains are considered:
// Verify that the Strict cookie may or may not be sent for a cross-scheme
// (same-registrable-domain) redirected top level navigation, depending on the
// status of Schemeful Same-Site. The Lax cookie is sent regardless, because
// this is a top-level navigation.
+ //
+ // If redirect chains are not considered:
+ // Verify that both cookies are sent, because this is a top-level navigation.
{
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(features::kSchemefulSameSite);
@@ -2524,7 +2547,7 @@
req->set_first_party_url_policy(
RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT);
req->set_site_for_cookies(kHttpSiteForCookies);
- req->set_initiator(kHttpOrigin);
+ req->set_initiator(kOrigin);
req->Start();
d.RunUntilComplete();
@@ -2548,18 +2571,20 @@
req->set_first_party_url_policy(
RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT);
req->set_site_for_cookies(kHttpSiteForCookies);
- req->set_initiator(kHttpOrigin);
+ req->set_initiator(kOrigin);
req->Start();
d.RunUntilComplete();
EXPECT_EQ(2u, req->url_chain().size());
- EXPECT_EQ(std::string::npos,
- d.data_received().find("StrictSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("StrictSameSiteCookie=1"));
EXPECT_NE(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
}
- // Verify that the Strict cookie is not sent for a cross-site redirected top
- // level navigation...
+ // Verify that (depending on whether redirect chains are considered), the
+ // Strict cookie is (not) sent for a cross-site redirected top level
+ // navigation...
{
TestDelegate d;
GURL url = https_server.GetURL(
@@ -2579,8 +2604,9 @@
d.RunUntilComplete();
EXPECT_EQ(2u, req->url_chain().size());
- EXPECT_EQ(std::string::npos,
- d.data_received().find("StrictSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("StrictSameSiteCookie=1"));
EXPECT_NE(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
}
// ... even if the initial URL is same-site.
@@ -2605,13 +2631,15 @@
d.RunUntilComplete();
EXPECT_EQ(3u, req->url_chain().size());
- EXPECT_EQ(std::string::npos,
- d.data_received().find("StrictSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("StrictSameSiteCookie=1"));
EXPECT_NE(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
}
- // Verify that neither SameSite cookie is sent for a cross-site redirected
- // subresource request...
+ // Verify that (depending on whether redirect chains are considered), neither
+ // (or both) SameSite cookie is sent for a cross-site redirected subresource
+ // request...
{
TestDelegate d;
GURL url = https_server.GetURL(
@@ -2629,9 +2657,12 @@
d.RunUntilComplete();
EXPECT_EQ(2u, req->url_chain().size());
- EXPECT_EQ(std::string::npos,
- d.data_received().find("StrictSameSiteCookie=1"));
- EXPECT_EQ(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("StrictSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("LaxSameSiteCookie=1"));
}
// ... even if the initial URL is same-site.
{
@@ -2653,13 +2684,16 @@
d.RunUntilComplete();
EXPECT_EQ(3u, req->url_chain().size());
- EXPECT_EQ(std::string::npos,
- d.data_received().find("StrictSameSiteCookie=1"));
- EXPECT_EQ(std::string::npos, d.data_received().find("LaxSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("StrictSameSiteCookie=1"));
+ EXPECT_EQ(
+ DoesCookieSameSiteConsiderRedirectChain(),
+ std::string::npos == d.data_received().find("LaxSameSiteCookie=1"));
}
}
-TEST_F(URLRequestTest, SettingSameSiteCookies) {
+TEST_P(URLRequestSameSiteCookiesTest, SettingSameSiteCookies) {
HttpTestServer test_server;
ASSERT_TRUE(test_server.Start());
@@ -2855,7 +2889,7 @@
// Tests special chrome:// scheme that is supposed to always attach SameSite
// cookies if the requested site is secure.
-TEST_F(URLRequestTest, SameSiteCookiesSpecialScheme) {
+TEST_P(URLRequestSameSiteCookiesTest, SameSiteCookiesSpecialScheme) {
url::ScopedSchemeRegistryForTests scoped_registry;
url::AddStandardScheme("chrome", url::SchemeType::SCHEME_WITH_HOST);
@@ -2947,7 +2981,7 @@
}
}
-TEST_F(URLRequestTest, SettingSameSiteCookies_Redirect) {
+TEST_P(URLRequestSameSiteCookiesTest, SettingSameSiteCookies_Redirect) {
EmbeddedTestServer http_server;
RegisterDefaultHandlers(&http_server);
EmbeddedTestServer https_server(EmbeddedTestServer::TYPE_HTTPS);
@@ -3115,9 +3149,9 @@
EXPECT_EQ(expected_set_cookie_count, network_delegate.set_cookie_count());
}
- // Verify that SameSite cookies *cannot* be set for a cross-site redirected
- // subresource request, even if the site-for-cookies and initiator are
- // same-site, ...
+ // Verify that (depending on whether redirect chains are considered) SameSite
+ // cookies can/cannot be set for a cross-site redirected subresource request,
+ // even if the site-for-cookies and initiator are same-site, ...
{
TestDelegate d;
GURL set_cookie_url = https_server.GetURL(
@@ -3132,7 +3166,7 @@
req->set_site_for_cookies(kSiteForCookies);
req->set_initiator(kOrigin);
- expected_cookies += 0;
+ expected_cookies += DoesCookieSameSiteConsiderRedirectChain() ? 0 : 2;
expected_set_cookie_count += 2;
req->Start();
@@ -3158,7 +3192,7 @@
req->set_site_for_cookies(kSiteForCookies);
req->set_initiator(kOrigin);
- expected_cookies += 0;
+ expected_cookies += DoesCookieSameSiteConsiderRedirectChain() ? 0 : 2;
expected_set_cookie_count += 2;
req->Start();
@@ -3170,7 +3204,7 @@
// Verify that SameSite cookies may or may not be set for a cross-scheme
// (same-registrable-domain) redirected subresource request, depending on the
- // status of Schemeful Same-Site.
+ // status of Schemeful Same-Site and whether redirect chains are considered.
{
base::test::ScopedFeatureList feature_list;
feature_list.InitAndDisableFeature(features::kSchemefulSameSite);
@@ -3184,8 +3218,10 @@
req->set_isolation_info(IsolationInfo::Create(
IsolationInfo::RequestType::kOther, kHttpOrigin, kHttpOrigin,
kHttpSiteForCookies, {} /* party_context */));
+ req->set_first_party_url_policy(
+ RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT);
req->set_site_for_cookies(kHttpSiteForCookies);
- req->set_initiator(kHttpOrigin);
+ req->set_initiator(kOrigin);
expected_cookies += 2;
expected_set_cookie_count += 2;
@@ -3209,10 +3245,12 @@
req->set_isolation_info(IsolationInfo::Create(
IsolationInfo::RequestType::kOther, kHttpOrigin, kHttpOrigin,
kHttpSiteForCookies, {} /* party_context */));
+ req->set_first_party_url_policy(
+ RedirectInfo::FirstPartyURLPolicy::UPDATE_URL_ON_REDIRECT);
req->set_site_for_cookies(kHttpSiteForCookies);
- req->set_initiator(kHttpOrigin);
+ req->set_initiator(kOrigin);
- expected_cookies += 0;
+ expected_cookies += DoesCookieSameSiteConsiderRedirectChain() ? 0 : 2;
expected_set_cookie_count += 2;
req->Start();
@@ -3223,6 +3261,10 @@
}
}
+INSTANTIATE_TEST_SUITE_P(/* no label */,
+ URLRequestSameSiteCookiesTest,
+ ::testing::Bool());
+
// Tests that __Secure- cookies can't be set on non-secure origins.
TEST_F(URLRequestTest, SecureCookiePrefixOnNonsecureOrigin) {
EmbeddedTestServer http_server;
