diff --git a/content/browser/devtools/devtools_url_loader_interceptor.cc b/content/browser/devtools/devtools_url_loader_interceptor.cc index 68ef9ab1d85501..89a3864cf309f7 100644 --- a/content/browser/devtools/devtools_url_loader_interceptor.cc +++ b/content/browser/devtools/devtools_url_loader_interceptor.cc @@ -1117,6 +1117,7 @@ void InterceptionJob::ProcessSetCookies(const net::HttpResponseHeaders& headers, create_loader_params_->request.url, create_loader_params_->request.site_for_cookies, create_loader_params_->request.request_initiator, + create_loader_params_->request.is_main_frame, should_treat_as_first_party)); // |this| might be deleted here if |cookies| is empty! @@ -1265,7 +1266,8 @@ void InterceptionJob::FetchCookies( options.set_same_site_cookie_context( net::cookie_util::ComputeSameSiteContextForRequest( request.method, request.url, request.site_for_cookies, - request.request_initiator, should_treat_as_first_party)); + request.request_initiator, request.is_main_frame, + should_treat_as_first_party)); cookie_manager_->GetCookieList(request.url, options, std::move(callback)); } diff --git a/net/base/features.cc b/net/base/features.cc index a34161cd41f6d1..6007e28aa59fa6 100644 --- a/net/base/features.cc +++ b/net/base/features.cc @@ -199,5 +199,8 @@ constexpr base::Feature kFirstPartySets{"FirstPartySets", const base::FeatureParam kFirstPartySetsIsDogfooder{ &kFirstPartySets, "FirstPartySetsIsDogfooder", false}; +const base::Feature kSameSiteCookiesBugfix1166211{ + "SameSiteCookiesBugfix1166211", base::FEATURE_ENABLED_BY_DEFAULT}; + } // namespace features } // namespace net diff --git a/net/base/features.h b/net/base/features.h index a202be49d3b940..4cdccdc17398a2 100644 --- a/net/base/features.h +++ b/net/base/features.h @@ -294,6 +294,15 @@ NET_EXPORT extern const base::Feature kFirstPartySets; // feature. NET_EXPORT extern const base::FeatureParam kFirstPartySetsIsDogfooder; +// Controls whether the fix for crbug.com/1166211 is enabled. When this is +// enabled, SameSite=Lax cookies may only be accessed for cross-site requests if +// they are top-level navigations. When it is disabled, the (incorrect) previous +// behavior that allows SameSite=Lax cookies on cross-site, non-top-level +// requests if all frame ancestors are same-site with the request URL is used +// instead. This fix is implemented behind a flag (kill switch) due to potential +// compatibility risk. +NET_EXPORT extern const base::Feature kSameSiteCookiesBugfix1166211; + } // namespace features } // namespace net diff --git a/net/cookies/cookie_util.cc b/net/cookies/cookie_util.cc index 19cb4aa49299bb..313c520f732d84 100644 --- a/net/cookies/cookie_util.cc +++ b/net/cookies/cookie_util.cc @@ -83,21 +83,49 @@ bool SaturatedTimeFromUTCExploded(const base::Time::Exploded& exploded, return false; } +// This function consolidates the common logic for computing SameSite cookie +// access context in various situations (HTTP vs JS; get vs set). +// +// `is_http` is whether the current cookie access request is associated with a +// network request (as opposed to a non-HTTP API, i.e., JavaScript). +// +// `compute_schemefully` is whether the current computation is for a +// schemeful_context, i.e. whether scheme should be considered when comparing +// two sites. +// +// See documentation of `ComputeSameSiteContextForRequest` for explanations of +// other parameters. ContextType ComputeSameSiteContext(const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, + bool is_http, + bool is_main_frame_navigation, bool compute_schemefully) { - if (site_for_cookies.IsFirstPartyWithSchemefulMode(url, - compute_schemefully)) { + bool site_for_cookies_is_same_site = + site_for_cookies.IsFirstPartyWithSchemefulMode(url, compute_schemefully); + + // If the request is a main frame navigation, site_for_cookies must either be + // null (for opaque origins, e.g., data: origins) or same-site with the + // request URL (both schemefully and schemelessly), and the URL cannot be + // ws/wss (these schemes are not navigable). + DCHECK(!is_main_frame_navigation || site_for_cookies_is_same_site || + site_for_cookies.IsNull()); + DCHECK(!is_main_frame_navigation || !url.SchemeIsWSOrWSS()); + + if (site_for_cookies_is_same_site) { // Create a SiteForCookies object from the initiator so that we can reuse // IsFirstPartyWithSchemefulMode(). if (!initiator || SiteForCookies::FromOrigin(initiator.value()) .IsFirstPartyWithSchemefulMode(url, compute_schemefully)) { return ContextType::SAME_SITE_STRICT; - } else { - return ContextType::SAME_SITE_LAX; } + // Preserve old behavior if the bugfix is disabled. + if (!base::FeatureList::IsEnabled(features::kSameSiteCookiesBugfix1166211)) + return ContextType::SAME_SITE_LAX; + + if (!is_http || is_main_frame_navigation) + return ContextType::SAME_SITE_LAX; } return ContextType::CROSS_SITE; } @@ -105,23 +133,26 @@ ContextType ComputeSameSiteContext(const GURL& url, CookieOptions::SameSiteCookieContext ComputeSameSiteContextForSet( const GURL& url, const SiteForCookies& site_for_cookies, - bool force_ignore_site_for_cookies) { - if (force_ignore_site_for_cookies) - return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet(); + const base::Optional& initiator, + bool is_http, + bool is_main_frame_navigation) { + CookieOptions::SameSiteCookieContext same_site_context; - // Schemeless check - if (!site_for_cookies.IsFirstPartyWithSchemefulMode(url, false)) { - return CookieOptions::SameSiteCookieContext(ContextType::CROSS_SITE, - ContextType::CROSS_SITE); - } + same_site_context.set_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, is_http, is_main_frame_navigation, + false /* compute_schemefully */)); + same_site_context.set_schemeful_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, is_http, is_main_frame_navigation, + true /* compute_schemefully */)); - // Schemeful check - if (!site_for_cookies.IsFirstPartyWithSchemefulMode(url, true)) { - return CookieOptions::SameSiteCookieContext(ContextType::SAME_SITE_LAX, - ContextType::CROSS_SITE); - } + // Setting any SameSite={Strict,Lax} cookie only requires a LAX context, so + // normalize any strictly same-site contexts to Lax for cookie writes. + if (same_site_context.context() == ContextType::SAME_SITE_STRICT) + same_site_context.set_context(ContextType::SAME_SITE_LAX); + if (same_site_context.schemeful_context() == ContextType::SAME_SITE_STRICT) + same_site_context.set_schemeful_context(ContextType::SAME_SITE_LAX); - return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet(); + return same_site_context; } } // namespace @@ -470,22 +501,24 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForRequest( const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, + bool is_main_frame_navigation, bool force_ignore_site_for_cookies) { - // Set SameSiteCookieMode according to the rules laid out in - // https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02: + // Set SameSiteCookieContext according to the rules laid out in + // https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis: // // * Include both "strict" and "lax" same-site cookies if the request's // |url|, |initiator|, and |site_for_cookies| all have the same // registrable domain. Note: this also covers the case of a request // without an initiator (only happens for browser-initiated main frame - // navigations). + // navigations). If computing schemefully, the schemes must also match. // // * Include only "lax" same-site cookies if the request's |URL| and // |site_for_cookies| have the same registrable domain, _and_ the - // request's |http_method| is "safe" ("GET" or "HEAD"). + // request's |http_method| is "safe" ("GET" or "HEAD"), and the request + // is a main frame navigation. // - // This case should generally occur only for cross-site requests which - // target a top-level browsing context. + // This case should occur only for cross-site requests which + // target a top-level browsing context, with a "safe" method. // // * Include both "strict" and "lax" same-site cookies if the request is // tagged with a flag allowing it. @@ -501,10 +534,12 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForRequest( CookieOptions::SameSiteCookieContext same_site_context; - same_site_context.set_context( - ComputeSameSiteContext(url, site_for_cookies, initiator, false)); - same_site_context.set_schemeful_context( - ComputeSameSiteContext(url, site_for_cookies, initiator, true)); + same_site_context.set_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, true /* is_http */, + is_main_frame_navigation, false /* compute_schemefully */)); + same_site_context.set_schemeful_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, true /* is_http */, + is_main_frame_navigation, true /* compute_schemefully */)); // If the method is safe, the context is Lax. Otherwise, make a note that // the method is unsafe. @@ -531,10 +566,12 @@ ComputeSameSiteContextForScriptGet(const GURL& url, CookieOptions::SameSiteCookieContext same_site_context; - same_site_context.set_context( - ComputeSameSiteContext(url, site_for_cookies, initiator, false)); - same_site_context.set_schemeful_context( - ComputeSameSiteContext(url, site_for_cookies, initiator, true)); + same_site_context.set_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, false /* is_http */, + false /* is_main_frame_navigation */, false /* compute_schemefully */)); + same_site_context.set_schemeful_context(ComputeSameSiteContext( + url, site_for_cookies, initiator, false /* is_http */, + false /* is_main_frame_navigation */, true /* compute_schemefully */)); return same_site_context; } @@ -543,20 +580,39 @@ CookieOptions::SameSiteCookieContext ComputeSameSiteContextForResponse( const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, + bool is_main_frame_navigation, bool force_ignore_site_for_cookies) { - // |initiator| is here in case it'll be decided to ignore |site_for_cookies| - // for entirely browser-side requests (see https://crbug.com/958335). + if (force_ignore_site_for_cookies) + return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet(); + + if (is_main_frame_navigation && !site_for_cookies.IsNull()) { + // If the request is a main frame navigation, site_for_cookies must either + // be null (for opaque origins, e.g., data: origins) or same-site with the + // request URL (both schemefully and schemelessly), and the URL cannot be + // ws/wss (these schemes are not navigable). + DCHECK(site_for_cookies.IsFirstPartyWithSchemefulMode(url, true)); + DCHECK(!url.SchemeIsWSOrWSS()); + return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet(); + } - return ComputeSameSiteContextForSet(url, site_for_cookies, - force_ignore_site_for_cookies); + return ComputeSameSiteContextForSet(url, site_for_cookies, initiator, + true /* is_http */, + is_main_frame_navigation); } CookieOptions::SameSiteCookieContext ComputeSameSiteContextForScriptSet( const GURL& url, const SiteForCookies& site_for_cookies, bool force_ignore_site_for_cookies) { - return ComputeSameSiteContextForSet(url, site_for_cookies, - force_ignore_site_for_cookies); + if (force_ignore_site_for_cookies) + return CookieOptions::SameSiteCookieContext::MakeInclusiveForSet(); + + // It doesn't matter what initiator origin we pass here. Either way, the + // context will be considered same-site iff the site_for_cookies is same-site + // with the url. + return ComputeSameSiteContextForSet( + url, site_for_cookies, base::nullopt /* initiator */, false /* is_http */, + false /* is_main_frame_navigation */); } CookieOptions::SameSiteCookieContext ComputeSameSiteContextForSubresource( diff --git a/net/cookies/cookie_util.h b/net/cookies/cookie_util.h index 74771d0e055c95..4ffa877c4ff0ec 100644 --- a/net/cookies/cookie_util.h +++ b/net/cookies/cookie_util.h @@ -140,25 +140,29 @@ NET_EXPORT void ParseRequestCookieLine(const std::string& header_value, NET_EXPORT std::string SerializeRequestCookieLine( const ParsedRequestCookies& parsed_cookies); -// Determines which of the cookies for |url| can be accessed, with respect to -// the SameSite attribute. This applies to looking up existing cookies; for -// setting new ones, see ComputeSameSiteContextForResponse and -// ComputeSameSiteContextForScriptSet. +// Determines which of the cookies for `url` can be accessed, with respect to +// the SameSite attribute. This applies to looking up existing cookies for HTTP +// requests. For looking up cookies for non-HTTP APIs (i.e., JavaScript), see +// ComputeSameSiteContextForScriptGet. For setting new cookies, see +// ComputeSameSiteContextForResponse and ComputeSameSiteContextForScriptSet. // -// |site_for_cookies| is the currently navigated to site that should be +// `site_for_cookies` is the currently navigated to site that should be // considered "first-party" for cookies. // -// |initiator| is the origin ultimately responsible for getting the request -// issued; it may be different from |site_for_cookies| in that it may be some -// other website that caused the navigation to |site_for_cookies| to occur. +// `initiator` is the origin ultimately responsible for getting the request +// issued. It may be different from `site_for_cookies`. // -// base::nullopt for |initiator| denotes that the navigation was initiated by +// base::nullopt for `initiator` denotes that the navigation was initiated by // the user directly interacting with the browser UI, e.g. entering a URL // or selecting a bookmark. // -// If |force_ignore_site_for_cookies| is specified, all SameSite cookies will be +// `is_main_frame_navigation` is whether the request is for a navigation that +// targets the main frame or top-level browsing context. These requests may +// sometimes send SameSite=Lax cookies but not SameSite=Strict cookies. +// +// If `force_ignore_site_for_cookies` is specified, all SameSite cookies will be // attached, i.e. this will return SAME_SITE_STRICT. This flag is set to true -// when the |site_for_cookies| is a chrome:// URL embedding a secure origin, +// when the `site_for_cookies` is a chrome:// URL embedding a secure origin, // among other scenarios. // This is *not* set when the *initiator* is chrome-extension://, // which is intentional, since it would be bad to let an extension arbitrarily @@ -166,7 +170,7 @@ NET_EXPORT std::string SerializeRequestCookieLine( // // See also documentation for corresponding methods on net::URLRequest. // -// |http_method| is used to enforce the requirement that, in a context that's +// `http_method` is used to enforce the requirement that, in a context that's // lax same-site but not strict same-site, SameSite=lax cookies be only sent // when the method is "safe" in the RFC7231 section 4.2.1 sense. NET_EXPORT CookieOptions::SameSiteCookieContext @@ -174,33 +178,38 @@ ComputeSameSiteContextForRequest(const std::string& http_method, const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, + bool is_main_frame_navigation, bool force_ignore_site_for_cookies); -// As above, but applying for scripts. |initiator| here should be the initiator +// As above, but applying for scripts. `initiator` here should be the initiator // used when fetching the document. -// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_STRICT. +// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_STRICT. NET_EXPORT CookieOptions::SameSiteCookieContext ComputeSameSiteContextForScriptGet(const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, bool force_ignore_site_for_cookies); -// Determines which of the cookies for |url| can be set from a network response, +// Determines which of the cookies for `url` can be set from a network response, // with respect to the SameSite attribute. This will only return CROSS_SITE or // SAME_SITE_LAX (cookie sets of SameSite=strict cookies are permitted in same // contexts that sets of SameSite=lax cookies are). -// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_LAX. +// `is_main_frame_navigation` is whether the request was for a navigation that +// targets the main frame or top-level browsing context. Both SameSite=Lax and +// SameSite=Strict cookies may be set by any main frame navigation. +// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_LAX. NET_EXPORT CookieOptions::SameSiteCookieContext ComputeSameSiteContextForResponse(const GURL& url, const SiteForCookies& site_for_cookies, const base::Optional& initiator, + bool is_main_frame_navigation, bool force_ignore_site_for_cookies); -// Determines which of the cookies for |url| can be set from a script context, +// Determines which of the cookies for `url` can be set from a script context, // with respect to the SameSite attribute. This will only return CROSS_SITE or // SAME_SITE_LAX (cookie sets of SameSite=strict cookies are permitted in same // contexts that sets of SameSite=lax cookies are). -// If |force_ignore_site_for_cookies| is true, this returns SAME_SITE_LAX. +// If `force_ignore_site_for_cookies` is true, this returns SAME_SITE_LAX. NET_EXPORT CookieOptions::SameSiteCookieContext ComputeSameSiteContextForScriptSet(const GURL& url, const SiteForCookies& site_for_cookies, diff --git a/net/cookies/cookie_util_unittest.cc b/net/cookies/cookie_util_unittest.cc index fad6f302b0c9ef..6ab7ddfbc7c2a6 100644 --- a/net/cookies/cookie_util_unittest.cc +++ b/net/cookies/cookie_util_unittest.cc @@ -351,18 +351,29 @@ MATCHER_P2(ContextTypeIsWithSchemefulMode, context_type, schemeful, "") { return context_type == (schemeful ? arg.schemeful_context() : arg.context()); } -// Tests for the various ComputeSameSiteContextFor*() functions. The test param -// is whether the results of the computations are evaluated schemefully. +// 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. class CookieUtilComputeSameSiteContextTest - : public ::testing::TestWithParam { + : public ::testing::TestWithParam> { public: - CookieUtilComputeSameSiteContextTest() = default; + CookieUtilComputeSameSiteContextTest() { + // No need to explicitly enable the feature because it is enabled by + // default. + if (!IsBugfix1166211Enabled()) { + feature_list_.InitAndDisableFeature( + features::kSameSiteCookiesBugfix1166211); + } + } ~CookieUtilComputeSameSiteContextTest() override = default; using SameSiteCookieContext = CookieOptions::SameSiteCookieContext; using ContextType = CookieOptions::SameSiteCookieContext::ContextType; - bool IsSchemeful() const { return GetParam(); } + bool IsSchemeful() const { return std::get<0>(GetParam()); } + + bool IsBugfix1166211Enabled() const { return std::get<1>(GetParam()); } // Returns the proper gtest matcher to use for the schemeless/schemeful mode. auto ContextTypeIs(ContextType context_type) const { @@ -456,6 +467,23 @@ class CookieUtilComputeSameSiteContextTest return cross_site_initiators; } + // Computes possible values of is_main_frame_navigation that are consistent + // with the DCHECKs. + bool CanBeMainFrameNavigation(const GURL& url, + const SiteForCookies& site_for_cookies) const { + return (site_for_cookies.IsNull() || + site_for_cookies.IsFirstPartyWithSchemefulMode(url, true)) && + !url.SchemeIsWSOrWSS(); + } + + std::vector IsMainFrameNavigationPossibleValues( + const GURL& url, + const SiteForCookies& site_for_cookies) const { + return CanBeMainFrameNavigation(url, site_for_cookies) + ? std::vector{false, true} + : std::vector{false}; + } + // Request URL. const GURL kSiteUrl{"http://example.test/"}; const GURL kSiteUrlWithPath{"http://example.test/path"}; @@ -493,6 +521,9 @@ class CookieUtilComputeSameSiteContextTest base::make_optional(url::Origin::Create(kSecureSubdomainUrl)); const base::Optional kUnrelatedInitiator = base::make_optional(url::Origin::Create(GURL("https://unrelated.test/"))); + + protected: + base::test::ScopedFeatureList feature_list_; }; TEST_P(CookieUtilComputeSameSiteContextTest, UrlAndSiteForCookiesCrossSite) { @@ -511,14 +542,19 @@ TEST_P(CookieUtilComputeSameSiteContextTest, UrlAndSiteForCookiesCrossSite) { url, site_for_cookies, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::CROSS_SITE)); - EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( - method, url, site_for_cookies, initiator, - false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::CROSS_SITE)); - EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( - url, site_for_cookies, initiator, - false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::CROSS_SITE)); + for (bool is_main_frame_navigation : + IsMainFrameNavigationPossibleValues(url, site_for_cookies)) { + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::CROSS_SITE)); + EXPECT_THAT( + cookie_util::ComputeSameSiteContextForResponse( + url, site_for_cookies, initiator, is_main_frame_navigation, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::CROSS_SITE)); + } EXPECT_THAT(cookie_util::ComputeSameSiteContextForSubresource( url, site_for_cookies, false /* force_ignore_site_for_cookies */), @@ -552,14 +588,20 @@ TEST_P(CookieUtilComputeSameSiteContextTest, SiteForCookiesNotSchemefullySame) { url, site_for_cookies, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::CROSS_SITE)); + + // If the site-for-cookies isn't schemefully_same, this cannot be a + // main frame navigation. EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( method, url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::CROSS_SITE)); EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::CROSS_SITE)); + EXPECT_THAT(cookie_util::ComputeSameSiteContextForSubresource( url, site_for_cookies, false /* force_ignore_site_for_cookies */), @@ -653,30 +695,68 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForRequest) { for (const base::Optional& initiator : GetSameSiteInitiators()) { for (const std::string& method : {"GET", "POST", "PUT", "HEAD"}) { - EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( - method, url, site_for_cookies, initiator, - false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_STRICT)); + for (bool is_main_frame_navigation : + IsMainFrameNavigationPossibleValues(url, site_for_cookies)) { + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_STRICT)); + } } } // Cross-Site initiator -> it's same-site lax iff the method is safe. - // TODO(crbug.com/1166211): This should take into account whether the - // request is a main frame navigation. for (const base::Optional& initiator : GetCrossSiteInitiators()) { + // For main frame navigations, the context is Lax (or Lax-unsafe). for (const std::string& method : {"GET", "HEAD"}) { + if (!CanBeMainFrameNavigation(url, site_for_cookies)) + break; EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( method, url, site_for_cookies, initiator, + true /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_LAX)); } for (const std::string& method : {"POST", "PUT"}) { + if (!CanBeMainFrameNavigation(url, site_for_cookies)) + break; EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( method, url, site_for_cookies, initiator, + true /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_LAX_METHOD_UNSAFE)); } + + // For non-main-frame-navigation requests, the context should be + // cross-site, or (if the old incorrect behavior is in effect) lax or + // lax-unsafe. + if (IsBugfix1166211Enabled()) { + for (const std::string& method : {"GET", "POST", "PUT", "HEAD"}) { + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::CROSS_SITE)); + } + } else { + for (const std::string& method : {"GET", "HEAD"}) { + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_LAX)); + } + for (const std::string& method : {"POST", "PUT"}) { + EXPECT_THAT( + cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_LAX_METHOD_UNSAFE)); + } + } } } } @@ -686,85 +766,125 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForRequest_SchemefulDowngrade) { // Some test cases where the context is downgraded when computed schemefully. // (Should already be covered above, but just to be explicit.) - // Strict -> cross downgrade. + // Cross-scheme URL and site-for-cookies with (schemelessly) same-site + // initiator. + // (The request cannot be a main frame navigation if the site-for-cookies is + // not schemefully same-site). for (const std::string& method : {"GET", "POST"}) { EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, ContextType::CROSS_SITE), cookie_util::ComputeSameSiteContextForRequest( method, kSecureSiteUrl, kSiteForCookies, kSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */)); EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, ContextType::CROSS_SITE), cookie_util::ComputeSameSiteContextForRequest( method, kSiteUrl, kSecureSiteForCookies, kSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */)); } - // Strict -> lax downgrade. - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, - ContextType::SAME_SITE_LAX), - cookie_util::ComputeSameSiteContextForRequest( - "GET", kSecureSiteUrl, kSecureSiteForCookies, kSiteInitiator, - false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, - ContextType::SAME_SITE_LAX), - cookie_util::ComputeSameSiteContextForRequest( - "GET", kSiteUrl, kSiteForCookies, kSecureSiteInitiator, - false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, - ContextType::SAME_SITE_LAX_METHOD_UNSAFE), - cookie_util::ComputeSameSiteContextForRequest( - "POST", kSecureSiteUrl, kSecureSiteForCookies, kSiteInitiator, - false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, - ContextType::SAME_SITE_LAX_METHOD_UNSAFE), - cookie_util::ComputeSameSiteContextForRequest( - "POST", kSiteUrl, kSiteForCookies, kSecureSiteInitiator, - false /* force_ignore_site_for_cookies */)); + // Schemefully same-site URL and site-for-cookies with cross-scheme + // initiator. + for (bool is_main_frame_navigation : {false, true}) { + ContextType lax_if_main_frame = + is_main_frame_navigation || !IsBugfix1166211Enabled() + ? ContextType::SAME_SITE_LAX + : ContextType::CROSS_SITE; + ContextType lax_unsafe_if_main_frame = + is_main_frame_navigation || !IsBugfix1166211Enabled() + ? ContextType::SAME_SITE_LAX_METHOD_UNSAFE + : ContextType::CROSS_SITE; + + EXPECT_EQ( + SameSiteCookieContext(ContextType::SAME_SITE_STRICT, lax_if_main_frame), + cookie_util::ComputeSameSiteContextForRequest( + "GET", kSecureSiteUrl, kSecureSiteForCookies, kSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ( + SameSiteCookieContext(ContextType::SAME_SITE_STRICT, lax_if_main_frame), + cookie_util::ComputeSameSiteContextForRequest( + "GET", kSiteUrl, kSiteForCookies, kSecureSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, + lax_unsafe_if_main_frame), + cookie_util::ComputeSameSiteContextForRequest( + "POST", kSecureSiteUrl, kSecureSiteForCookies, kSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_STRICT, + lax_unsafe_if_main_frame), + cookie_util::ComputeSameSiteContextForRequest( + "POST", kSiteUrl, kSiteForCookies, kSecureSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); + } - // Lax -> cross downgrade. - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, - ContextType::CROSS_SITE), + // Cross-scheme URL and site-for-cookies with cross-site initiator. + // (The request cannot be a main frame navigation if the site-for-cookies is + // not schemefully same-site). + ContextType incorrectly_lax = IsBugfix1166211Enabled() + ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX; + ContextType incorrectly_lax_unsafe = + IsBugfix1166211Enabled() ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX_METHOD_UNSAFE; + EXPECT_EQ(SameSiteCookieContext(incorrectly_lax, ContextType::CROSS_SITE), cookie_util::ComputeSameSiteContextForRequest( "GET", kSiteUrl, kSecureSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, - ContextType::CROSS_SITE), + EXPECT_EQ(SameSiteCookieContext(incorrectly_lax, ContextType::CROSS_SITE), cookie_util::ComputeSameSiteContextForRequest( "GET", kSecureSiteUrl, kSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX_METHOD_UNSAFE, - ContextType::CROSS_SITE), - cookie_util::ComputeSameSiteContextForRequest( - "POST", kSiteUrl, kSecureSiteForCookies, kCrossSiteInitiator, - false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX_METHOD_UNSAFE, - ContextType::CROSS_SITE), - cookie_util::ComputeSameSiteContextForRequest( - "POST", kSecureSiteUrl, kSiteForCookies, kCrossSiteInitiator, - false /* force_ignore_site_for_cookies */)); + EXPECT_EQ( + SameSiteCookieContext(incorrectly_lax_unsafe, ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForRequest( + "POST", kSiteUrl, kSecureSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ( + SameSiteCookieContext(incorrectly_lax_unsafe, ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForRequest( + "POST", kSecureSiteUrl, kSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); } TEST_P(CookieUtilComputeSameSiteContextTest, ForRequest_WebSocketSchemes) { // wss/https and http/ws are considered the same for schemeful purposes. + // (ws/wss requests cannot be main frame navigations.) + ContextType incorrectly_lax = IsBugfix1166211Enabled() + ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX; + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( "GET", kWssUrl, kSecureSiteForCookies, kSecureSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_STRICT)); EXPECT_THAT( cookie_util::ComputeSameSiteContextForRequest( "GET", kWssUrl, kSecureSiteForCookies, kSecureCrossSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_LAX)); + ContextTypeIs(incorrectly_lax)); EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( "GET", kWsUrl, kSiteForCookies, kSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_STRICT)); EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( "GET", kWsUrl, kSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_LAX)); + ContextTypeIs(incorrectly_lax)); } TEST_P(CookieUtilComputeSameSiteContextTest, ForScriptSet) { @@ -814,16 +934,40 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForResponse) { // (Cross-site cases covered above in UrlAndSiteForCookiesCrosSite test.) for (const SiteForCookies& site_for_cookies : GetSameSiteSitesForCookies()) { - // The initiator doesn't matter. If the Site for cookies and URL are - // same-site, then the context is Lax. - // TODO(crbug.com/1166211): This should take the initiator into account - // when the request is not a main frame navigation. + // For main frame navigations, setting all SameSite cookies is allowed + // regardless of initiator. for (const base::Optional& initiator : GetAllInitiators()) { + if (!CanBeMainFrameNavigation(url, site_for_cookies)) + break; + EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( + url, site_for_cookies, initiator, + true /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_LAX)); + } + + // For non-main-frame-navigation requests, the context should be lax iff + // the initiator is same-site, and cross-site otherwise. If the old + // (incorrect) behavior is in effect, it is always lax. + for (const base::Optional& initiator : + GetSameSiteInitiators()) { EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_LAX)); } + for (const base::Optional& initiator : + GetCrossSiteInitiators()) { + ContextType incorrectly_lax = IsBugfix1166211Enabled() + ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX; + EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( + url, site_for_cookies, initiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */), + ContextTypeIs(incorrectly_lax)); + } } } } @@ -831,33 +975,83 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForResponse) { TEST_P(CookieUtilComputeSameSiteContextTest, ForResponse_SchemefulDowngrade) { // Some test cases where the context is downgraded when computed schemefully. // (Should already be covered above, but just to be explicit.) - // TODO(crbug.com/1166211): This should take the initiator into account when - // the request is not a main frame navigation. - for (const base::Optional& initiator : GetAllInitiators()) { - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, - ContextType::CROSS_SITE), - cookie_util::ComputeSameSiteContextForResponse( - kSiteUrl, kSecureSiteForCookies, initiator, - false /* force_ignore_site_for_cookies */)); - EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, - ContextType::CROSS_SITE), - cookie_util::ComputeSameSiteContextForResponse( - kSecureSiteUrl, kSiteForCookies, initiator, - false /* force_ignore_site_for_cookies */)); + + // URL and site-for-cookies are cross-scheme. + // (If the URL and site-for-cookies are not schemefully same-site, this cannot + // be a main frame navigation.) + // With same-site initiator: + EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, + ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForResponse( + kSiteUrl, kSecureSiteForCookies, kSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ(SameSiteCookieContext(ContextType::SAME_SITE_LAX, + ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForResponse( + kSecureSiteUrl, kSiteForCookies, kSecureSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); + // With cross-site initiator: + ContextType incorrectly_lax = IsBugfix1166211Enabled() + ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX; + EXPECT_EQ(SameSiteCookieContext(incorrectly_lax, ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForResponse( + kSiteUrl, kSecureSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ(SameSiteCookieContext(incorrectly_lax, ContextType::CROSS_SITE), + cookie_util::ComputeSameSiteContextForResponse( + kSecureSiteUrl, kSiteForCookies, kCrossSiteInitiator, + false /* is_main_frame_navigation */, + false /* force_ignore_site_for_cookies */)); + + // Schemefully same-site URL and site-for-cookies with cross-scheme + // initiator. + for (bool is_main_frame_navigation : {false, true}) { + ContextType lax_if_main_frame = + is_main_frame_navigation || !IsBugfix1166211Enabled() + ? ContextType::SAME_SITE_LAX + : ContextType::CROSS_SITE; + EXPECT_EQ( + SameSiteCookieContext(ContextType::SAME_SITE_LAX, lax_if_main_frame), + cookie_util::ComputeSameSiteContextForResponse( + kSiteUrl, kSiteForCookies, kSecureSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); + EXPECT_EQ( + SameSiteCookieContext(ContextType::SAME_SITE_LAX, lax_if_main_frame), + cookie_util::ComputeSameSiteContextForResponse( + kSecureSiteUrl, kSecureSiteForCookies, kSiteInitiator, + is_main_frame_navigation, + false /* force_ignore_site_for_cookies */)); } } TEST_P(CookieUtilComputeSameSiteContextTest, ForResponse_WebSocketSchemes) { // wss/https and http/ws are considered the same for schemeful purposes. - for (const base::Optional& initiator : GetAllInitiators()) { + // (ws/wss requests cannot be main frame navigations.) + + // Same-site initiators. + for (const base::Optional& initiator : GetSameSiteInitiators()) { EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( - kWssUrl, kSecureSiteForCookies, initiator, + kWsUrl, kSiteForCookies, initiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_LAX)); + } + // Cross-site initiators. + for (const base::Optional& initiator : + GetCrossSiteInitiators()) { + ContextType incorrectly_lax = IsBugfix1166211Enabled() + ? ContextType::CROSS_SITE + : ContextType::SAME_SITE_LAX; EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( kWsUrl, kSiteForCookies, initiator, + false /* is_main_frame_navigation */, false /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_LAX)); + ContextTypeIs(incorrectly_lax)); } } @@ -918,14 +1112,19 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForceIgnoreSiteForCookies) { url, site_for_cookies, true /* force_ignore_site_for_cookies */), ContextTypeIs(ContextType::SAME_SITE_LAX)); - EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( - method, url, site_for_cookies, initiator, - true /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_STRICT)); - EXPECT_THAT(cookie_util::ComputeSameSiteContextForResponse( - url, site_for_cookies, initiator, - true /* force_ignore_site_for_cookies */), - ContextTypeIs(ContextType::SAME_SITE_LAX)); + for (bool is_main_frame_navigation : + IsMainFrameNavigationPossibleValues(url, site_for_cookies)) { + EXPECT_THAT(cookie_util::ComputeSameSiteContextForRequest( + method, url, site_for_cookies, initiator, + is_main_frame_navigation, + true /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_STRICT)); + EXPECT_THAT( + cookie_util::ComputeSameSiteContextForResponse( + url, site_for_cookies, initiator, is_main_frame_navigation, + true /* force_ignore_site_for_cookies */), + ContextTypeIs(ContextType::SAME_SITE_LAX)); + } EXPECT_THAT(cookie_util::ComputeSameSiteContextForSubresource( url, site_for_cookies, true /* force_ignore_site_for_cookies */), @@ -938,7 +1137,8 @@ TEST_P(CookieUtilComputeSameSiteContextTest, ForceIgnoreSiteForCookies) { INSTANTIATE_TEST_SUITE_P(/* no label */, CookieUtilComputeSameSiteContextTest, - ::testing::Bool()); + ::testing::Combine(::testing::Bool(), + ::testing::Bool())); TEST(CookieUtilTest, AdaptCookieAccessResultToBool) { bool result_out = true; diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc index 9d993cce5e0e01..053612f3191b5b 100644 --- a/net/url_request/url_request_http_job.cc +++ b/net/url_request/url_request_http_job.cc @@ -548,10 +548,13 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() { request_->site_for_cookies())) { force_ignore_site_for_cookies = true; } + bool is_main_frame_navigation = IsolationInfo::RequestType::kMainFrame == + request_->isolation_info().request_type(); CookieOptions::SameSiteCookieContext same_site_context = net::cookie_util::ComputeSameSiteContextForRequest( request_->method(), request_->url(), request_->site_for_cookies(), - request_->initiator(), force_ignore_site_for_cookies); + request_->initiator(), is_main_frame_navigation, + force_ignore_site_for_cookies); net::SchemefulSite request_site(request_->url()); @@ -699,10 +702,12 @@ void URLRequestHttpJob::SaveCookiesAndNotifyHeadersComplete(int result) { request_->url(), request_->site_for_cookies())) { force_ignore_site_for_cookies = true; } + bool is_main_frame_navigation = IsolationInfo::RequestType::kMainFrame == + request_->isolation_info().request_type(); CookieOptions::SameSiteCookieContext same_site_context = net::cookie_util::ComputeSameSiteContextForResponse( request_->url(), request_->site_for_cookies(), request_->initiator(), - force_ignore_site_for_cookies); + is_main_frame_navigation, force_ignore_site_for_cookies); const CookieAccessDelegate* delegate = cookie_store->cookie_access_delegate(); net::SchemefulSite request_site(request_->url()); diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index a45042e543c6d7..1d26119266a2b6 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -2194,6 +2194,15 @@ TEST_F(URLRequestTest, SameSiteCookies) { const std::string kHost = "example.test"; const std::string kSubHost = "subdomain.example.test"; const std::string kCrossHost = "cross-origin.test"; + const url::Origin kOrigin = + url::Origin::Create(test_server.GetURL(kHost, "/")); + const url::Origin kSubOrigin = + url::Origin::Create(test_server.GetURL(kSubHost, "/")); + const url::Origin kCrossOrigin = + url::Origin::Create(test_server.GetURL(kCrossHost, "/")); + const SiteForCookies kSiteForCookies = SiteForCookies::FromOrigin(kOrigin); + const SiteForCookies kCrossSiteForCookies = + SiteForCookies::FromOrigin(kCrossOrigin); // Set up two 'SameSite' cookies on 'example.test' { @@ -2203,9 +2212,8 @@ TEST_F(URLRequestTest, SameSiteCookies) { "/set-cookie?StrictSameSiteCookie=1;SameSite=Strict&" "LaxSameSiteCookie=1;SameSite=Lax"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator(url::Origin::Create(test_server.GetURL(kHost, "/"))); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kOrigin); req->Start(); d.RunUntilComplete(); EXPECT_EQ(0, network_delegate.blocked_get_cookies_count()); @@ -2213,15 +2221,21 @@ TEST_F(URLRequestTest, SameSiteCookies) { EXPECT_EQ(2, network_delegate.set_cookie_count()); } - // Verify that both cookies are sent for same-site requests. - { + // Verify that both cookies are sent for same-site requests, whether they are + // subresource requests, subframe navigations, or main frame navigations. + for (IsolationInfo::RequestType request_type : + {IsolationInfo::RequestType::kMainFrame, + IsolationInfo::RequestType::kSubFrame, + IsolationInfo::RequestType::kOther}) { TestDelegate d; std::unique_ptr req(default_context().CreateRequest( test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator(url::Origin::Create(test_server.GetURL(kHost, "/"))); + req->set_isolation_info(IsolationInfo::Create(request_type, kOrigin, + kOrigin, kSiteForCookies, + {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kOrigin); req->Start(); d.RunUntilComplete(); @@ -2239,8 +2253,7 @@ TEST_F(URLRequestTest, SameSiteCookies) { std::unique_ptr req(default_context().CreateRequest( test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); + req->set_site_for_cookies(kSiteForCookies); req->Start(); d.RunUntilComplete(); @@ -2259,7 +2272,7 @@ TEST_F(URLRequestTest, SameSiteCookies) { TRAFFIC_ANNOTATION_FOR_TESTS)); req->set_site_for_cookies( SiteForCookies::FromUrl(test_server.GetURL(kSubHost, "/"))); - req->set_initiator(url::Origin::Create(test_server.GetURL(kSubHost, "/"))); + req->set_initiator(kSubOrigin); req->Start(); d.RunUntilComplete(); @@ -2276,10 +2289,8 @@ TEST_F(URLRequestTest, SameSiteCookies) { std::unique_ptr req(default_context().CreateRequest( test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kCrossHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_site_for_cookies(kCrossSiteForCookies); + req->set_initiator(kCrossOrigin); req->Start(); d.RunUntilComplete(); @@ -2291,16 +2302,17 @@ TEST_F(URLRequestTest, SameSiteCookies) { } // Verify that the lax cookie is sent for cross-site initiators when the - // method is "safe". + // method is "safe" and the request is a main frame navigation. { TestDelegate d; std::unique_ptr req(default_context().CreateRequest( test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kMainFrame, kOrigin, kOrigin, + kSiteForCookies, {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kCrossOrigin); req->set_method("GET"); req->Start(); d.RunUntilComplete(); @@ -2313,16 +2325,18 @@ TEST_F(URLRequestTest, SameSiteCookies) { } // Verify that neither cookie is sent for cross-site initiators when the - // method is unsafe (e.g. POST). + // method is unsafe (e.g. POST), even if the request is a main frame + // navigation. { TestDelegate d; std::unique_ptr req(default_context().CreateRequest( test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kMainFrame, kOrigin, kOrigin, + kSiteForCookies, {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kCrossOrigin); req->set_method("POST"); req->Start(); d.RunUntilComplete(); @@ -2333,6 +2347,30 @@ TEST_F(URLRequestTest, SameSiteCookies) { EXPECT_EQ(0, network_delegate.blocked_get_cookies_count()); EXPECT_EQ(0, network_delegate.blocked_set_cookie_count()); } + + // Verify that neither cookie is sent for cross-site initiators when the + // method is safe and the site-for-cookies is same-site, but the request is + // not a main frame navigation. + { + TestDelegate d; + std::unique_ptr req(default_context().CreateRequest( + test_server.GetURL(kHost, "/echoheader?Cookie"), DEFAULT_PRIORITY, &d, + TRAFFIC_ANNOTATION_FOR_TESTS)); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kSubFrame, kOrigin, kOrigin, + kSiteForCookies, {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kCrossOrigin); + req->set_method("GET"); + req->Start(); + d.RunUntilComplete(); + + EXPECT_EQ(std::string::npos, + d.data_received().find("StrictSameSiteCookie=1")); + EXPECT_EQ(std::string::npos, d.data_received().find("LaxSameSiteCookie=1")); + EXPECT_EQ(0, network_delegate.blocked_get_cookies_count()); + EXPECT_EQ(0, network_delegate.blocked_set_cookie_count()); + } } TEST_F(URLRequestTest, SettingSameSiteCookies) { @@ -2345,6 +2383,15 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { const std::string kHost = "example.test"; const std::string kSubHost = "subdomain.example.test"; const std::string kCrossHost = "cross-origin.test"; + const url::Origin kOrigin = + url::Origin::Create(test_server.GetURL(kHost, "/")); + const url::Origin kSubOrigin = + url::Origin::Create(test_server.GetURL(kSubHost, "/")); + const url::Origin kCrossOrigin = + url::Origin::Create(test_server.GetURL(kCrossHost, "/")); + const SiteForCookies kSiteForCookies = SiteForCookies::FromOrigin(kOrigin); + const SiteForCookies kCrossSiteForCookies = + SiteForCookies::FromOrigin(kCrossOrigin); int expected_cookies = 0; @@ -2355,9 +2402,8 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { "/set-cookie?Strict1=1;SameSite=Strict&" "Lax1=1;SameSite=Lax"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator(url::Origin::Create(test_server.GetURL(kHost, "/"))); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kOrigin); // 'SameSite' cookies are settable from strict same-site contexts // (same-origin site_for_cookies, same-origin initiator), so this request @@ -2378,14 +2424,15 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { "/set-cookie?Strict2=1;SameSite=Strict&" "Lax2=1;SameSite=Lax"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kMainFrame, kOrigin, kOrigin, + kSiteForCookies, {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kCrossOrigin); // 'SameSite' cookies are settable from lax same-site contexts (same-origin - // site_for_cookies, cross-site initiator), so this request should result in - // two cookies being set. + // site_for_cookies, cross-site initiator, main frame navigation), so this + // request should result in two cookies being set. expected_cookies += 2; req->Start(); @@ -2402,14 +2449,16 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { "/set-cookie?Strict3=1;SameSite=Strict&" "Lax3=1;SameSite=Lax"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kMainFrame, kSubOrigin, kSubOrigin, + kSiteForCookies, {} /* party_context */)); req->set_site_for_cookies( SiteForCookies::FromUrl(test_server.GetURL(kSubHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_initiator(kCrossOrigin); // 'SameSite' cookies are settable from lax same-site contexts (same-site - // site_for_cookies, cross-site initiator), so this request should result in - // two cookies being set. + // site_for_cookies, cross-site initiator, main frame navigation), so this + // request should result in two cookies being set. expected_cookies += 2; req->Start(); @@ -2441,6 +2490,7 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { EXPECT_EQ(expected_cookies, network_delegate.set_cookie_count()); } + int expected_network_delegate_set_cookie_count; { TestDelegate d; std::unique_ptr req(default_context().CreateRequest( @@ -2448,25 +2498,56 @@ TEST_F(URLRequestTest, SettingSameSiteCookies) { "/set-cookie?Strict5=1;SameSite=Strict&" "Lax5=1;SameSite=Lax"), DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); - req->set_site_for_cookies( - SiteForCookies::FromUrl(test_server.GetURL(kCrossHost, "/"))); - req->set_initiator( - url::Origin::Create(test_server.GetURL(kCrossHost, "/"))); + req->set_site_for_cookies(kCrossSiteForCookies); + req->set_initiator(kCrossOrigin); // 'SameSite' cookies are not settable from cross-site contexts, so this // should not result in any new cookies being set. expected_cookies += 0; + // This counts the number of successful calls to CanSetCookie() when + // attempting to set a cookie. The two cookies above were created and + // attempted to be set, and were not rejected by the NetworkDelegate, so the + // count here is 2 more than the number of cookies actually set. + expected_network_delegate_set_cookie_count = expected_cookies + 2; req->Start(); d.RunUntilComplete(); // This counts the number of cookies actually set. EXPECT_EQ(expected_cookies, static_cast(GetAllCookies(&default_context()).size())); + EXPECT_EQ(expected_network_delegate_set_cookie_count, + network_delegate.set_cookie_count()); + } + + { + TestDelegate d; + std::unique_ptr req(default_context().CreateRequest( + test_server.GetURL(kHost, + "/set-cookie?Strict6=1;SameSite=Strict&" + "Lax6=1;SameSite=Lax"), + DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS)); + req->set_isolation_info(IsolationInfo::Create( + IsolationInfo::RequestType::kSubFrame, kOrigin, kOrigin, + kSiteForCookies, {} /* party_context */)); + req->set_site_for_cookies(kSiteForCookies); + req->set_initiator(kCrossOrigin); + + // Same-site site-for-cookies, cross-site initiator, non main frame + // navigation -> context is considered cross-site so no SameSite cookies are + // set. + expected_cookies += 0; // This counts the number of successful calls to CanSetCookie() when // attempting to set a cookie. The two cookies above were created and // attempted to be set, and were not rejected by the NetworkDelegate, so the // count here is 2 more than the number of cookies actually set. - EXPECT_EQ(expected_cookies + 2, network_delegate.set_cookie_count()); + expected_network_delegate_set_cookie_count += 2; + + req->Start(); + d.RunUntilComplete(); + EXPECT_EQ(expected_cookies, + static_cast(GetAllCookies(&default_context()).size())); + EXPECT_EQ(expected_network_delegate_set_cookie_count, + network_delegate.set_cookie_count()); } } diff --git a/services/network/network_context_unittest.cc b/services/network/network_context_unittest.cc index cbd9ada75a2083..67ea74bccafdfa 100644 --- a/services/network/network_context_unittest.cc +++ b/services/network/network_context_unittest.cc @@ -6474,6 +6474,14 @@ class NetworkContextSplitCacheTest : public NetworkContextTest { request.trusted_params = ResourceRequest::TrustedParams(); request.trusted_params->isolation_info = isolation_info; params->is_trusted = true; + // These params must be individually set, to be consistent with the + // IsolationInfo if its request type is a main frame navigation. + // TODO(crbug.com/1172314): Unify these to avoid inconsistencies. + if (isolation_info.request_type() == + net::IsolationInfo::RequestType::kMainFrame) { + request.is_main_frame = true; + request.update_first_party_url_on_redirect = true; + } } params->automatically_assign_isolation_info = diff --git a/third_party/blink/web_tests/external/wpt/cookies/resources/cookie-helper.sub.js b/third_party/blink/web_tests/external/wpt/cookies/resources/cookie-helper.sub.js index a0ded48f2a8249..789d38d1eb4062 100644 --- a/third_party/blink/web_tests/external/wpt/cookies/resources/cookie-helper.sub.js +++ b/third_party/blink/web_tests/external/wpt/cookies/resources/cookie-helper.sub.js @@ -35,6 +35,12 @@ function redirectTo(origin, url) { return origin + "/cookies/resources/redirectWithCORSHeaders.py?status=307&location=" + encodeURIComponent(url); } +// Returns a URL on |origin| which navigates the window to the given URL (by +// setting window.location). +function navigateTo(origin, url) { + return origin + "/cookies/resources/navigate.html?location=" + encodeURIComponent(url); +} + // Asserts that `document.cookie` contains or does not contain (according to // the value of |present|) a cookie named |name| with a value of |value|. function assert_dom_cookie(name, value, present) { diff --git a/third_party/blink/web_tests/external/wpt/cookies/resources/navigate.html b/third_party/blink/web_tests/external/wpt/cookies/resources/navigate.html new file mode 100644 index 00000000000000..077efba5699315 --- /dev/null +++ b/third_party/blink/web_tests/external/wpt/cookies/resources/navigate.html @@ -0,0 +1,8 @@ + + + diff --git a/third_party/blink/web_tests/external/wpt/cookies/samesite/iframe.https.html b/third_party/blink/web_tests/external/wpt/cookies/samesite/iframe.https.html index 04ebb95836d7b4..3c7b638810e2f7 100644 --- a/third_party/blink/web_tests/external/wpt/cookies/samesite/iframe.https.html +++ b/third_party/blink/web_tests/external/wpt/cookies/samesite/iframe.https.html @@ -50,7 +50,7 @@ create_test(SECURE_ORIGIN, redirectTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Subdomain redirecting to same-host fetches are strictly same-site"); create_test(SECURE_ORIGIN, redirectTo(SECURE_CROSS_SITE_ORIGIN, SECURE_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Cross-site redirecting to same-host fetches are strictly same-site"); - // Redirect from {same-host,subdomain,cross-site} to same-host: + // Redirect from {same-host,subdomain,cross-site} to subdomain: create_test(SECURE_SUBDOMAIN_ORIGIN, redirectTo(SECURE_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Same-host redirecting to subdomain fetches are strictly same-site"); create_test(SECURE_SUBDOMAIN_ORIGIN, redirectTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Subdomain redirecting to subdomain fetches are strictly same-site"); create_test(SECURE_SUBDOMAIN_ORIGIN, redirectTo(SECURE_CROSS_SITE_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Cross-site redirecting to subdomain fetches are strictly same-site"); @@ -59,4 +59,19 @@ create_test(SECURE_CROSS_SITE_ORIGIN, redirectTo(SECURE_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Same-host redirecting to cross-site fetches are cross-site"); create_test(SECURE_CROSS_SITE_ORIGIN, redirectTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Subdomain redirecting to cross-site fetches are cross-site"); create_test(SECURE_CROSS_SITE_ORIGIN, redirectTo(SECURE_CROSS_SITE_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Cross-site redirecting to cross-site fetches are cross-site"); + + // Navigate from {same-host,subdomain,cross-site} to same-host: + create_test(SECURE_ORIGIN, navigateTo(SECURE_ORIGIN, SECURE_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Same-host navigating to same-host fetches are strictly same-site"); + create_test(SECURE_ORIGIN, navigateTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Subdomain navigating to same-host fetches are strictly same-site"); + create_test(SECURE_ORIGIN, navigateTo(SECURE_CROSS_SITE_ORIGIN, SECURE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.SAME_SITE, "Cross-site navigating to same-host fetches are cross-site"); + + // Navigate from {same-host,subdomain,cross-site} to subdomain: + create_test(SECURE_SUBDOMAIN_ORIGIN, navigateTo(SECURE_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Same-host navigating to subdomain fetches are strictly same-site"); + create_test(SECURE_SUBDOMAIN_ORIGIN, navigateTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.STRICT, DomSameSiteStatus.SAME_SITE, "Subdomain navigating to subdomain fetches are strictly same-site"); + create_test(SECURE_SUBDOMAIN_ORIGIN, navigateTo(SECURE_CROSS_SITE_ORIGIN, SECURE_SUBDOMAIN_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.SAME_SITE, "Cross-site navigating to subdomain fetches are cross-site-site"); + + // Navigate from {same-host,subdomain,cross-site} to cross-site: + create_test(SECURE_CROSS_SITE_ORIGIN, navigateTo(SECURE_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Same-host navigating to cross-site fetches are cross-site"); + create_test(SECURE_CROSS_SITE_ORIGIN, navigateTo(SECURE_SUBDOMAIN_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Subdomain navigating to cross-site fetches are cross-site"); + create_test(SECURE_CROSS_SITE_ORIGIN, navigateTo(SECURE_CROSS_SITE_ORIGIN, SECURE_CROSS_SITE_ORIGIN), SameSiteStatus.CROSS_SITE, DomSameSiteStatus.CROSS_SITE, "Cross-site navigating to cross-site fetches are cross-site"); diff --git a/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/echo-cookies.html b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/echo-cookies.html index 9b8b286015f2ad..a1b29b9b03c198 100644 --- a/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/echo-cookies.html +++ b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/echo-cookies.html @@ -1,5 +1,8 @@ diff --git a/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate-iframe.html b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate-iframe.html new file mode 100644 index 00000000000000..98ad6264fabb7a --- /dev/null +++ b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate-iframe.html @@ -0,0 +1,26 @@ + + + + + diff --git a/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate.html b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate.html index 7d0f87d4943908..88de6dff92e5fb 100644 --- a/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate.html +++ b/third_party/blink/web_tests/external/wpt/cookies/samesite/resources/navigate.html @@ -3,15 +3,13 @@