[per-frame rSA] Apply Storage Access Override enum to eligible requests.

Bug: 1401089 Change-Id: I9d62ae83021061a238c1688f13f5774ca6a552da Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4277242 Reviewed-by: Maks Orlovich <morlovich@chromium.org> Commit-Queue: Shuran Huang <shuuran@chromium.org> Reviewed-by: Chris Fredrickson <cfredric@chromium.org> Cr-Commit-Position: refs/heads/main@{#1109111}
chromium · Feb 23, 2023 · fe994a2 · fe994a2
1 parent 2a45103
commit fe994a2
Show file tree

Hide file tree

Showing 10 changed files with 168 additions and 102 deletions.
diff --git a/chrome/browser/storage_access_api/api_browsertest.cc b/chrome/browser/storage_access_api/api_browsertest.cc
@@ -246,12 +246,13 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
 
   // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
-  // the cookie is sent:
+  // the cookie is not sent:
   NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
   // Only when the initiator is the frame that's been navigated can inherit
   // per-frame storage access.
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
@@ -263,15 +264,18 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
   // Navigate iframe to a cross-site frame with a frame, and navigate _that_
-  // frame to a cross-site page that echos the cookie header, and verify that
-  // the cookie is sent:
+  // frame to the same cross-site page that echos the cookie header, and verify
+  // that allowing storage access for the iframe does not enable cookie access
+  // from the nested iframe.
   NavigateFrameTo(kHostB, "/iframe.html");
   NavigateNestedFrameTo(kHostB, "/echoheader?cookie");
-  // TODO(crbug.com/1401089): Should not send cookie unless requesting storage
-  // access again.
-  EXPECT_EQ(GetNestedFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "cross-site=b.test");
+  EXPECT_EQ(GetNestedFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "");
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame()));
+  EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
+  EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "");
 
   // Navigate nested iframe to c.test and verify that the cookie is not
   // sent.
@@ -282,18 +286,9 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
 
   // Navigate iframe to a cross-site frame with a frame, and navigate _that_
   // frame to a distinct cross-site page that echos the cookie header, and
-  // verify that the cookie is sent:
+  // verify that the cookie is not sent:
   NavigateFrameTo(kHostC, "/iframe.html");
   NavigateNestedFrameTo(kHostB, "/echoheader?cookie");
-  // TODO(crbug.com/1401089): Should not send cookie unless requesting storage
-  // access again.
-  EXPECT_EQ(GetNestedFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "cross-site=b.test");
-  EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame()));
-
-  // Navigate nested iframe to c.test and verify that the cookie is not
-  // sent.
-  NavigateNestedFrameTo(kHostC, "/echoheader?cookie");
   EXPECT_EQ(GetNestedFrameContent(), "None");
   EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "");
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame()));
@@ -409,24 +404,20 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
 
   NavigateToPageWithFrame(kHostA);
   NavigateFrameTo(kHostB, "/echoheader?cookie");
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
 
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
-
-  // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
-  // the cookie is sent:
-  NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(), "thirdparty=1");
   EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "thirdparty=1");
-  // Only when the initiator is the frame that's been navigated can inherit
-  // per-frame storage access.
-  EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
   // Manually delete all our grants.
   HostContentSettingsMap* settings_map =
       HostContentSettingsMapFactory::GetForProfile(browser()->profile());
   settings_map->ClearSettingsForOneType(ContentSettingsType::STORAGE_ACCESS);
+  // Verify cookie cannot be accessed.
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
 
   NavigateFrameTo(kHostB, "/echoheader?cookie");
   EXPECT_EQ(GetFrameContent(), "None");
@@ -526,6 +517,7 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest, ThirdPartyGrantsExpiry) {
   // The nested iframe reuses the existing grant without requesting.
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetNestedFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetNestedFrame()));
+  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "thirdparty=c");
   // We don't get to record a sample for the "reuse" case, so that histogram
   // still only has 1 sample in total.
   histogram_tester.ExpectTotalCount(kRequestOutcomeHistogram, 1);
@@ -536,10 +528,8 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest, ThirdPartyGrantsExpiry) {
   // per-frame storage access.
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetNestedFrame()));
-  // TODO(crbug.com/1401089): Should not send cookie unless requesting storage
-  // access again.
-  EXPECT_EQ(GetNestedFrameContent(), "thirdparty=c");
-  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "thirdparty=c");
+  EXPECT_EQ(GetNestedFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetNestedFrame()), "");
 }
 
 IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
@@ -559,22 +549,14 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
-
-  // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
-  // the cookie is sent:
-  NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
   EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
-  // Only when the initiator is the frame that's been navigated can inherit
-  // per-frame storage access.
-  EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
+  // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
+  // the cookie is not sent due to per-frame storage access:
   NavigateToPageWithFrame(kHostASubdomain);
   NavigateFrameTo(kHostB, "/echoheader?cookie");
-  // Similar to the rsaFor equivalent, scoping may or may not allow access for
-  // the subdomain, depending on the setting.
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 }
 
@@ -595,22 +577,22 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
 
   // Navigate iframe to a cross-site, cookie-reading endpoint, and verify that
-  // the cookie is sent:
+  // the cookie is not sent due to per-frame storage access:
   NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
   // Only when the initiator is the frame that's been navigated can inherit
   // per-frame storage access.
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
   NavigateToPageWithFrame(kHostA);
   NavigateFrameTo(kHostB, "/echoheader?cookie");
-  // Similar to the rsaFor equivalent, scoping may or may not allow access for
-  // the subdomain, depending on the setting.
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  // Verify that the cookie is not sent due to per-frame storage access:
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 }
 
@@ -629,6 +611,7 @@ IN_PROC_BROWSER_TEST_P(StorageAccessAPIBrowserTest,
 
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=a.test");
 
   // Regardless of the top-level site or origin scoping, the embedded origin
   // should be used.
@@ -826,12 +809,12 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIWithFirstPartySetsBrowserTest,
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
-  // When the frame subsequently navigates to an endpoint on kHostB,
-  // kHostB's cookies are sent, and the iframe retains storage
-  // access.
+  // When the frame subsequently navigates to an endpoint on kHostB, the frame
+  // obtained storage access is not carried over since this navigation is not
+  // made by the frame itself, so that kHostB's cookies are not sent:
   NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(), "cross-site=b.test");
-  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test");
+  EXPECT_EQ(GetFrameContent(), "None");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "");
   // Only when the initiator is the frame that's been navigated can inherit
   // per-frame storage access.
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
@@ -998,18 +981,18 @@ IN_PROC_BROWSER_TEST_F(StorageAccessAPIWithCHIPSBrowserTest,
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));
 
   // kHostB can request storage access, and it is granted (by an implicit
-  // grant):
+  // grant), kHostB's unpartitioned and partitioned cookies are sent:
   EXPECT_TRUE(storage::test::RequestStorageAccessForFrame(GetFrame()));
   EXPECT_TRUE(storage::test::HasStorageAccessForFrame(GetFrame()));
-
-  // When the frame subsequently navigates to an endpoint on kHostB, kHostB's
-  // unpartitioned and partitioned cookies are sent, and the iframe retains
-  // storage access.
-  NavigateFrameTo(kHostB, "/echoheader?cookie");
-  EXPECT_EQ(GetFrameContent(),
-            "cross-site=b.test; cross-site=b.test(partitioned)");
   EXPECT_EQ(ReadCookiesViaJS(GetFrame()),
             "cross-site=b.test; cross-site=b.test(partitioned)");
+
+  // When the frame subsequently navigates to an endpoint on kHostB, the frame
+  // obtained storage access is not carried over since this navigation is not
+  // made by the frame itself, only kHostB's partitioned cookies are sent:
+  NavigateFrameTo(kHostB, "/echoheader?cookie");
+  EXPECT_EQ(GetFrameContent(), "cross-site=b.test(partitioned)");
+  EXPECT_EQ(ReadCookiesViaJS(GetFrame()), "cross-site=b.test(partitioned)");
   // Only when the initiator is the frame that's been navigated can inherit
   // per-frame storage access.
   EXPECT_FALSE(storage::test::HasStorageAccessForFrame(GetFrame()));

diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
@@ -846,6 +846,13 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
     expected_response_checksum_ = std::string(checksum);
   }
 
+  void set_has_storage_access(bool has_storage_access) {
+    DCHECK(!is_pending_);
+    DCHECK(!has_notified_completion_);
+    has_storage_access_ = has_storage_access;
+  }
+  bool has_storage_access() const { return has_storage_access_; }
+
   static bool DefaultCanUseCookies();
 
   base::WeakPtr<URLRequest> GetWeakPtr();
@@ -974,10 +981,7 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
   bool force_ignore_site_for_cookies_ = false;
   bool force_ignore_top_frame_party_for_cookies_ = false;
   bool force_main_frame_for_same_site_cookies_ = false;
-  // TODO(https://crbug.com/1401089): this request ought to be ineligible for
-  // Storage Access API grants unless the requestor has opted in.
-  CookieSettingOverrides cookie_setting_overrides_ = CookieSettingOverrides(
-      CookieSettingOverride::kStorageAccessGrantEligible);
+  CookieSettingOverrides cookie_setting_overrides_;
 
   absl::optional<url::Origin> initiator_;
   GURL delegate_redirect_url_;
@@ -994,6 +998,10 @@ class NET_EXPORT URLRequest : public base::SupportsUserData {
   // Whether the request is allowed to send credentials in general. Set by
   // caller.
   bool allow_credentials_ = true;
+  // Whether the request is eligible for using storage access permission grant
+  // if one exists. Only set by caller when constructed and will not change
+  // during redirects.
+  bool has_storage_access_ = false;
   SecureDnsPolicy secure_dns_policy_ = SecureDnsPolicy::kAllow;
 
   CookieAccessResultList maybe_sent_cookies_;

diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
@@ -278,6 +278,15 @@ void URLRequestHttpJob::Start() {
   request_info_.reporting_upload_depth = request_->reporting_upload_depth();
 #endif
 
+  // Add/remove the Storage Access override enum based on whether the request's
+  // url and initiator are same-site, to prevent cross-site sibling iframes
+  // benefit from each other's storage access API grants.
+  request()->cookie_setting_overrides().PutOrRemove(
+      net::CookieSettingOverride::kStorageAccessGrantEligible,
+      request()->has_storage_access() && request_initiator_site().has_value() &&
+          request_initiator_site().value() ==
+              net::SchemefulSite(request()->url()));
+
   bool should_add_cookie_header = ShouldAddCookieHeader();
   UMA_HISTOGRAM_BOOLEAN("Net.HttpJob.CanIncludeCookies",
                         should_add_cookie_header);

diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
@@ -23,6 +23,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/network_delegate.h"
 #include "net/base/proxy_server.h"
+#include "net/base/schemeful_site.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cookies/cookie_setting_override.h"
 #include "net/log/net_log.h"
@@ -84,7 +85,12 @@ class URLRequestJob::URLRequestJobSourceStream : public SourceStream {
   const raw_ptr<URLRequestJob> job_;
 };
 
-URLRequestJob::URLRequestJob(URLRequest* request) : request_(request) {}
+URLRequestJob::URLRequestJob(URLRequest* request)
+    : request_(request),
+      request_initiator_site_(request->initiator().has_value()
+                                  ? absl::make_optional(net::SchemefulSite(
+                                        request->initiator().value()))
+                                  : absl::nullopt) {}
 
 URLRequestJob::~URLRequestJob() = default;
 

diff --git a/net/url_request/url_request_job.h b/net/url_request/url_request_job.h
@@ -353,6 +353,10 @@ class NET_EXPORT URLRequestJob {
   // On return, |this| may be deleted.
   void ReadRawDataComplete(int bytes_read);
 
+  const absl::optional<net::SchemefulSite>& request_initiator_site() const {
+    return request_initiator_site_;
+  }
+
   // The request that initiated this job. This value will never be nullptr.
   const raw_ptr<URLRequest> request_;
 
@@ -439,6 +443,11 @@ class NET_EXPORT URLRequestJob {
   // checks are performed, so this field must not be modified.
   absl::optional<RedirectInfo> deferred_redirect_info_;
 
+  // The request's initiator never changes, so we store it in format of
+  // SchemefulSite so that we don't recompute (including looking up the
+  // registrable domain) it during every redirect.
+  absl::optional<net::SchemefulSite> request_initiator_site_;
+
   // Non-null if ReadRawData() returned ERR_IO_PENDING, and the read has not
   // completed.
   CompletionOnceCallback read_raw_callback_;

diff --git a/services/network/restricted_cookie_manager.cc b/services/network/restricted_cookie_manager.cc
@@ -877,10 +877,11 @@ bool RestrictedCookieManager::ValidateAccessToCookiesAt(
 
 net::CookieSettingOverrides RestrictedCookieManager::GetCookieSettingOverrides(
     bool has_storage_access) const {
-  // TODO(https://crbug.com/1401089): the overrides ought to exclude Storage
-  // Access API grants unless the frame has opted in.
-  return net::CookieSettingOverrides(
-      net::CookieSettingOverride::kStorageAccessGrantEligible);
+  net::CookieSettingOverrides overrides;
+  if (has_storage_access) {
+    overrides.Put(net::CookieSettingOverride::kStorageAccessGrantEligible);
+  }
+  return overrides;
 }
 
 }  // namespace network
diff --git a/services/network/restricted_cookie_manager_unittest.cc b/services/network/restricted_cookie_manager_unittest.cc
@@ -664,13 +664,10 @@ TEST_P(RestrictedCookieManagerTest, GetAllForUrlEqualsMatch_WithStorageAccess) {
   auto options = mojom::CookieManagerGetOptions::New();
   options->name = "cookie-name";
   options->match_type = mojom::CookieMatchType::EQUALS;
-  // TODO(https://crbug.com/1401089): third-party cookies should be inaccessible
-  // when `has_storage_access` is false. Fix this to expect an empty vector.
-  EXPECT_THAT(
-      sync_service_->GetAllForUrl(kDefaultUrlWithPath, net::SiteForCookies(),
-                                  kOtherOrigin, /*has_storage_access=*/false,
-                                  options.Clone()),
-      ElementsAre(net::MatchesCookieNameValue("cookie-name", "cookie-value")));
+  EXPECT_THAT(sync_service_->GetAllForUrl(
+                  kDefaultUrlWithPath, net::SiteForCookies(), kOtherOrigin,
+                  /*has_storage_access=*/false, options.Clone()),
+              IsEmpty());
 
   // When `has_storage_access` is true, Storage Access grants may be used to
   // access cookies.
@@ -1214,9 +1211,7 @@ TEST_P(RestrictedCookieManagerTest, SetCanonicalCookie_WithStorageAccess) {
       base::Value(ContentSetting::CONTENT_SETTING_ALLOW), /*source=*/"",
       /*incognito=*/false)});
 
-  // TODO(https://crbug.com/1401089): this write should fail, since
-  // `has_storage_access` is false.
-  EXPECT_TRUE(sync_service_->SetCanonicalCookie(
+  EXPECT_FALSE(sync_service_->SetCanonicalCookie(
       *net::CanonicalCookie::CreateUnsafeCookieForTesting(
           "new-name", "new-value", "example.com", "/", base::Time(),
           base::Time(), base::Time(), base::Time(), /*secure=*/true,
@@ -1610,12 +1605,10 @@ TEST_P(RestrictedCookieManagerTest, CookiesEnabledFor_WithStorageAccess) {
       /*incognito=*/false)});
 
   bool result;
-  // TODO(https://crbug.com/1401089): access should not be allowed when
-  // `has_storage_access` is false.
   EXPECT_TRUE(backend()->CookiesEnabledFor(
       kDefaultUrl, net::SiteForCookies(), kOtherOrigin,
       /*has_storage_access=*/false, &result));
-  EXPECT_TRUE(result);
+  EXPECT_FALSE(result);
 
   // When `has_storage_access` is true, access is allowed since there's a
   // matching permission grant.

diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc
@@ -732,6 +732,8 @@ URLLoader::URLLoader(
         request.net_log_reference_info.value());
   }
 
+  url_request_->set_has_storage_access(request.has_storage_access);
+
   url_request_->cookie_setting_overrides().PutAll(cookie_setting_overrides);
   if (request.is_outermost_main_frame &&
       network::cors::IsCorsEnabledRequestMode(request_mode_)) {