Shared Storage: Allow iframe header writes if permission on redirect

We correct the behavior of writing to shared storage from response
headers to match the spec with regard to how `PermissionsPolicy`
checks are handled.

Currently, if permission is revoked for any request in a redirect
chain, then no subsequent request in that chain can write to shared
storage from response headers.

This CL updates the behavior for `HTMLIframeElement` so that, for
redirect chains of requests that have opted-in via
`sharedStorageWritable`, each request in the chain has its
`PermissionsPolicy` checked independently of the others in the chain.

https://crrev.com/c/4911530 will do the same for `fetch()` and
`HTMLImageElement`, and fixes the network service code common to all
three.

Bug: 1218540,1489536
Change-Id: I292c558d9ca922171a00c106a6adad142010b674
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4935951
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217159}

content/browser/loader/navigation_url_loader_impl.cc

@@ -338,7 +338,7 @@ std::unique_ptr<network::ResourceRequest> CreateResourceRequest(
   }
 
   new_request->shared_storage_writable_eligible =
-      request_info.shared_storage_writable;
+      request_info.shared_storage_writable_eligible;
 
   return new_request;
 }

content/browser/renderer_host/navigation_request.cc

@@ -977,8 +977,9 @@ network::mojom::WebSandboxFlags GetSandboxFlagsInitiator(
   return policy_container_host->policies().sandbox_flags;
 }
 
-bool IsSharedStorageWritableForNavigationRequest(FrameTreeNode* frame_tree_node,
-                                                 const GURL& url) {
+bool IsSharedStorageWritableEligibleForNavigationRequest(
+    FrameTreeNode* frame_tree_node,
+    const GURL& url) {
   // False if the <iframe> does not have the "sharedstoragewritable" opt-in
   // attribute.
   if (!frame_tree_node->shared_storage_writable()) {
@@ -1669,8 +1670,11 @@ NavigationRequest::NavigationRequest(
 
   if (base::FeatureList::IsEnabled(blink::features::kSharedStorageAPI) &&
       base::FeatureList::IsEnabled(blink::features::kSharedStorageAPIM118)) {
-    shared_storage_writable_ = IsSharedStorageWritableForNavigationRequest(
-        frame_tree_node_, common_params_->url);
+    shared_storage_writable_opted_in_ =
+        frame_tree_node_->shared_storage_writable();
+    shared_storage_writable_eligible_ =
+        IsSharedStorageWritableEligibleForNavigationRequest(
+            frame_tree_node_, common_params_->url);
   }
 
   if (from_begin_navigation_) {
@@ -4955,7 +4959,7 @@ void NavigationRequest::OnStartChecksComplete(
           devtools_accepted_stream_types, is_pdf_, GetInitiatorProcessId(),
           initiator_document_token_, GetPreviousRenderFrameHostId(),
           allow_cookies_from_browser_, navigation_id_,
-          shared_storage_writable_),
+          shared_storage_writable_eligible_),
       std::move(navigation_ui_data), service_worker_handle_.get(),
       std::move(prefetched_signed_exchange_cache_), this, loader_type,
       CreateCookieAccessObserver(), CreateTrustTokenAccessObserver(),
@@ -5131,13 +5135,23 @@ void NavigationRequest::OnRedirectChecksComplete(
                                *topics_header_value);
   }
 
-  if (shared_storage_writable_) {
+  if (shared_storage_writable_opted_in_) {
     // On a redirect, the PermissionsPolicy may change the status of this
     // request's Shared Storage eligibility, so we need to re-compute it.
-    shared_storage_writable_ = IsSharedStorageWritableForNavigationRequest(
-        frame_tree_node_, common_params_->url);
-    if (!shared_storage_writable_) {
-      removed_headers.push_back(kSecSharedStorageWritableRequestHeaderKey);
+    bool previous_shared_storage_writable_eligible =
+        shared_storage_writable_eligible_;
+    shared_storage_writable_eligible_ =
+        IsSharedStorageWritableEligibleForNavigationRequest(
+            frame_tree_node_, common_params_->url);
+
+    if (shared_storage_writable_eligible_ !=
+        previous_shared_storage_writable_eligible) {
+      if (shared_storage_writable_eligible_) {
+        modified_headers.SetHeader(kSecSharedStorageWritableRequestHeaderKey,
+                                   "?1");
+      } else {
+        removed_headers.push_back(kSecSharedStorageWritableRequestHeaderKey);
+      }
     }
   }
 

content/browser/renderer_host/navigation_request.h

@@ -1253,7 +1253,9 @@ class CONTENT_EXPORT NavigationRequest
     return std::move(web_ui_);
   }
 
-  bool shared_storage_writable() const { return shared_storage_writable_; }
+  bool shared_storage_writable_eligible() const {
+    return shared_storage_writable_eligible_;
+  }
 
   enum ErrorPageProcess {
     kNotErrorPage,
@@ -2605,10 +2607,15 @@ class CONTENT_EXPORT NavigationRequest
   // contain "Observe-Browsing-Topics: ?1", a topic observation will be stored.
   bool topics_eligible_ = false;
 
-  // Whether or not the request is eligible to write to shared storage from
+  // Whether or not the original request (without considering redirects or
+  // permissions policy) opted-in to write to shared storage from response
+  // headers. See https://github.com/WICG/shared-storage#from-response-headers
+  bool shared_storage_writable_opted_in_ = false;
+
+  // Whether or not the current request is eligible to shared storage from
   // response headers. See
   // https://github.com/WICG/shared-storage#from-response-headers
-  bool shared_storage_writable_ = false;
+  bool shared_storage_writable_eligible_ = false;
 
   // A WeakPtr for the BindContext associated with the browser routing loader
   // factory for the committing document. This will be set in

content/browser/renderer_host/navigation_request_info.cc

@@ -35,7 +35,7 @@ NavigationRequestInfo::NavigationRequestInfo(
     const GlobalRenderFrameHostId& previous_render_frame_host_id,
     bool allow_cookies_from_browser,
     int64_t navigation_id,
-    bool shared_storage_writable)
+    bool shared_storage_writable_eligible)
     : common_params(std::move(common_params)),
       begin_params(std::move(begin_params)),
       sandbox_flags(sandbox_flags),
@@ -59,7 +59,7 @@ NavigationRequestInfo::NavigationRequestInfo(
       previous_render_frame_host_id(previous_render_frame_host_id),
       allow_cookies_from_browser(allow_cookies_from_browser),
       navigation_id(navigation_id),
-      shared_storage_writable(shared_storage_writable) {}
+      shared_storage_writable_eligible(shared_storage_writable_eligible) {}
 
 NavigationRequestInfo::~NavigationRequestInfo() {}
 

content/browser/renderer_host/navigation_request_info.h

@@ -153,7 +153,7 @@ struct CONTENT_EXPORT NavigationRequestInfo {
   // Whether or not the request is eligible to write to shared storage from
   // response headers. See
   // https://github.com/WICG/shared-storage#from-response-headers.
-  bool shared_storage_writable;
+  bool shared_storage_writable_eligible;
 };
 
 }  // namespace content

content/browser/renderer_host/navigation_request_unittest.cc

@@ -497,8 +497,8 @@ TEST_F(NavigationRequestTest, SharedStorageWritable) {
   // Verify that the main frame's `NavigationRequest` will not be
   // SharedStorageWritable.
   ASSERT_TRUE(main_navigation->GetNavigationHandle());
-  EXPECT_FALSE(
-      main_navigation->GetNavigationHandle()->shared_storage_writable());
+  EXPECT_FALSE(main_navigation->GetNavigationHandle()
+                   ->shared_storage_writable_eligible());
 
   // Commit the navigation.
   main_navigation->Commit();
@@ -520,8 +520,8 @@ TEST_F(NavigationRequestTest, SharedStorageWritable) {
 
   // Verify that the `NavigationRequest` will be SharedStorageWritable.
   ASSERT_TRUE(child_navigation->GetNavigationHandle());
-  EXPECT_TRUE(
-      child_navigation->GetNavigationHandle()->shared_storage_writable());
+  EXPECT_TRUE(child_navigation->GetNavigationHandle()
+                  ->shared_storage_writable_eligible());
 
   // Commit the navigation.
   child_navigation->Commit();
@@ -559,7 +559,7 @@ TEST_F(NavigationRequestTest, SharedStorageWritable) {
   // Verify that the `NavigationRequest` will be SharedStorageWritable.
   ASSERT_TRUE(child_of_fenced_frame_navigation->GetNavigationHandle());
   EXPECT_TRUE(child_of_fenced_frame_navigation->GetNavigationHandle()
-                  ->shared_storage_writable());
+                  ->shared_storage_writable_eligible());
 
   // Commit the navigation.
   child_of_fenced_frame_navigation->Commit();

content/browser/shared_storage/shared_storage_browsertest.cc

@@ -11323,6 +11323,217 @@ IN_PROC_BROWSER_TEST_F(
                                  "denied the method on window.sharedStorage."));
 }
 
+IN_PROC_BROWSER_TEST_F(
+    SharedStorageHeaderObserverBrowserTest,
+    Iframe_CrossOrigin_Redirect_InititalAllowed_IntermediateDenied_FinalAllowed_WriteInitialAndFinal) {
+  SetUpResponsesAndNavigateMainPage(
+      /*main_hostname=*/"a.test",
+      /*subresource_or_subframe_hostname=*/"b.test",
+      /*shared_storage_permissions=*/
+      "(self \"https://b.test:{{port}}\" \"https://d.test:{{port}}\")",
+      /*is_image=*/false,
+      /*redirect_hostnames=*/std::vector<std::string>({"c.test", "d.test"}));
+
+  CreateSharedStorageWritableIframe(shell(), subresource_or_subframe_url_);
+
+  WaitForSubresourceOrSubframeRequestAndSendResponse(
+      /*expect_writable_header=*/true,
+      /*http_status=*/net::HTTP_FOUND,
+      /*extra_headers=*/
+      {base::StrCat({"Location: ", redirect_urls_.front().spec()}),
+       "Access-Control-Allow-Origin: *",
+       "Shared-Storage-Write: clear, "
+       "set;key=\"hello\";value=\"world\";ignore_if_present, "
+       "append;key=hello;value=there"});
+
+  ASSERT_TRUE(observer_);
+  observer_->WaitForOperations(3);
+
+  EXPECT_EQ(observer_->header_results().size(), 1u);
+  EXPECT_EQ(observer_->header_results().front().first,
+            subresource_or_subframe_origin_);
+  EXPECT_THAT(observer_->header_results().front().second,
+              testing::ElementsAre(true, true, true));
+  EXPECT_THAT(observer_->operations(),
+              testing::ElementsAre(
+                  ClearOperation(subresource_or_subframe_origin_,
+                                 OperationResult::kSuccess),
+                  SetOperation(subresource_or_subframe_origin_, "hello",
+                               "world", true, OperationResult::kSet),
+                  AppendOperation(subresource_or_subframe_origin_, "hello",
+                                  "there", OperationResult::kSet)));
+
+  WaitForRedirectRequestAndSendResponse(
+      /*expect_writable_header=*/false,
+      /*http_status=*/net::HTTP_TEMPORARY_REDIRECT,
+      /*extra_headers=*/
+      {base::StrCat({"Location: ", redirect_urls_.back().spec()}),
+       "Access-Control-Allow-Origin: *",
+       "Shared-Storage-Write: append;key=wont;value=set"},
+      /*redirect_index=*/0);
+
+  WaitForRedirectRequestAndSendResponse(
+      /*expect_writable_header=*/true,
+      /*http_status=*/net::HTTP_OK,
+      /*extra_headers=*/
+      {"Access-Control-Allow-Origin: *",
+       "Shared-Storage-Write: delete;key=a, set;value=will;key=set"},
+      /*redirect_index=*/1);
+
+  // There will now have been a total of 5 operations (3 previous, 2 current).
+  ASSERT_TRUE(observer_);
+  observer_->WaitForOperations(5);
+
+  EXPECT_EQ(observer_->header_results().size(), 2u);
+  EXPECT_EQ(observer_->header_results().back().first, redirect_origins_.back());
+  EXPECT_THAT(observer_->header_results().back().second,
+              testing::ElementsAre(true, true));
+  EXPECT_THAT(observer_->operations(),
+              testing::ElementsAre(
+                  ClearOperation(subresource_or_subframe_origin_,
+                                 OperationResult::kSuccess),
+                  SetOperation(subresource_or_subframe_origin_, "hello",
+                               "world", true, OperationResult::kSet),
+                  AppendOperation(subresource_or_subframe_origin_, "hello",
+                                  "there", OperationResult::kSet),
+                  DeleteOperation(redirect_origins_.back(), "a",
+                                  OperationResult::kSuccess),
+                  SetOperation(redirect_origins_.back(), "set", "will",
+                               absl::nullopt, OperationResult::kSet)));
+
+  // Create an iframe that's same-origin to the original iframe URL.
+  FrameTreeNode* iframe_node2 =
+      CreateIFrame(PrimaryFrameTreeNodeRoot(), subresource_or_subframe_url_);
+
+  WebContentsConsoleObserver console_observer(shell()->web_contents());
+
+  GURL out_script_url;
+  ExecuteScriptInWorklet(iframe_node2, R"(
+      console.log(await sharedStorage.get('hello'));
+      console.log(await sharedStorage.get('set'));
+      console.log(await sharedStorage.length());
+    )",
+                         &out_script_url);
+
+  EXPECT_EQ(3u, console_observer.messages().size());
+  EXPECT_EQ("worldthere",
+            base::UTF16ToUTF8(console_observer.messages()[0].message));
+
+  // Only one entry was set in b.test's shared storage.
+  EXPECT_EQ("undefined",
+            base::UTF16ToUTF8(console_observer.messages()[1].message));
+  EXPECT_EQ("1", base::UTF16ToUTF8(console_observer.messages()[2].message));
+
+  // Create an iframe that's same-origin to the first redirect URL.
+  FrameTreeNode* iframe_node3 =
+      CreateIFrame(PrimaryFrameTreeNodeRoot(), redirect_urls_.front());
+
+  EvalJsResult result = EvalJs(iframe_node3, R"(
+        sharedStorage.worklet.addModule('/shared_storage/simple_module.js');
+      )");
+
+  // c.test does not have permission to use shared storage.
+  EXPECT_THAT(result.error,
+              testing::HasSubstr("The \"shared-storage\" Permissions Policy "
+                                 "denied the method on window.sharedStorage."));
+
+  // Create an iframe that's same-origin to the second redirect URL.
+  FrameTreeNode* iframe_node4 =
+      CreateIFrame(PrimaryFrameTreeNodeRoot(), redirect_urls_.back());
+
+  ExecuteScriptInWorklet(iframe_node4, R"(
+      console.log(await sharedStorage.get('set'));
+      console.log(await sharedStorage.length());
+    )",
+                         &out_script_url, /*expected_total_host_count=*/2u);
+
+  // One entry was set in d.test's shared storage.
+  EXPECT_EQ(5u, console_observer.messages().size());
+  EXPECT_EQ("will", base::UTF16ToUTF8(console_observer.messages()[3].message));
+  EXPECT_EQ("1", base::UTF16ToUTF8(console_observer.messages()[4].message));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    SharedStorageHeaderObserverBrowserTest,
+    Iframe_CrossOrigin_Redirect_InitialDenied_FinalAllowed_WriteFinal) {
+  SetUpResponsesAndNavigateMainPage(
+      /*main_hostname=*/"a.test",
+      /*subresource_or_subframe_hostname=*/"b.test",
+      /*shared_storage_permissions=*/
+      "(self \"https://c.test:{{port}}\")",
+      /*is_image=*/false,
+      /*redirect_hostnames=*/std::vector<std::string>({"c.test"}));
+
+  CreateSharedStorageWritableIframe(shell(), subresource_or_subframe_url_);
+
+  WaitForSubresourceOrSubframeRequestAndSendResponse(
+      /*expect_writable_header=*/false,
+      /*http_status=*/net::HTTP_FOUND,
+      /*extra_headers=*/
+      {base::StrCat({"Location: ", redirect_urls_.back().spec()}),
+       "Access-Control-Allow-Origin: *",
+       "Shared-Storage-Write: clear, "
+       "set;key=\"hello\";value=\"world\";ignore_if_present, "
+       "append;key=hello;value=there"});
+
+  ASSERT_TRUE(observer_);
+
+  // No operations are invoked.
+  EXPECT_TRUE(observer_->header_results().empty());
+  EXPECT_TRUE(observer_->operations().empty());
+
+  WaitForRedirectRequestAndSendResponse(
+      /*expect_writable_header=*/true,
+      /*http_status=*/net::HTTP_OK,
+      /*extra_headers=*/
+      {"Access-Control-Allow-Origin: *",
+       "Shared-Storage-Write: delete;key=a, set;value=will;key=set"});
+
+  observer_->WaitForOperations(2);
+
+  EXPECT_EQ(observer_->header_results().size(), 1u);
+  EXPECT_EQ(observer_->header_results().front().first,
+            redirect_origins_.back());
+  EXPECT_THAT(observer_->header_results().front().second,
+              testing::ElementsAre(true, true));
+  EXPECT_THAT(
+      observer_->operations(),
+      testing::ElementsAre(DeleteOperation(redirect_origins_.back(), "a",
+                                           OperationResult::kSuccess),
+                           SetOperation(redirect_origins_.back(), "set", "will",
+                                        absl::nullopt, OperationResult::kSet)));
+
+  WebContentsConsoleObserver console_observer(shell()->web_contents());
+
+  // Create an iframe that's same-origin to the original iframe URL.
+  FrameTreeNode* iframe_node2 =
+      CreateIFrame(PrimaryFrameTreeNodeRoot(), subresource_or_subframe_url_);
+
+  EvalJsResult result = EvalJs(iframe_node2, R"(
+        sharedStorage.worklet.addModule('/shared_storage/simple_module.js');
+      )");
+
+  EXPECT_THAT(result.error,
+              testing::HasSubstr("The \"shared-storage\" Permissions Policy "
+                                 "denied the method on window.sharedStorage."));
+
+  // Create an iframe that's same-origin to the redirect URL.
+  FrameTreeNode* iframe_node3 =
+      CreateIFrame(PrimaryFrameTreeNodeRoot(), redirect_urls_.back());
+
+  GURL out_script_url;
+  ExecuteScriptInWorklet(iframe_node3, R"(
+      console.log(await sharedStorage.get('set'));
+      console.log(await sharedStorage.length());
+    )",
+                         &out_script_url);
+
+  // one key was set in c.test's shared storage.
+  EXPECT_EQ(2u, console_observer.messages().size());
+  EXPECT_EQ("will", base::UTF16ToUTF8(console_observer.messages()[0].message));
+  EXPECT_EQ("1", base::UTF16ToUTF8(console_observer.messages()[1].message));
+}
+
 IN_PROC_BROWSER_TEST_F(SharedStorageHeaderObserverBrowserTest,
                        Iframe_ContentAttributeIncluded_Set) {
   WebContentsConsoleObserver console_observer(shell()->web_contents());