Shared Storage: Allow fetch/img 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 `fetch()` and `HTMLImageElement` 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/4935951 will do the same for `HTMLIframeElement`.

Bug: 1218540,1489536
Change-Id: I3110a237fb7f960b91f5940a3309abc623e81dbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4911530
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217122}

content/browser/loader/navigation_url_loader_impl.cc

@@ -337,7 +337,8 @@ std::unique_ptr<network::ResourceRequest> CreateResourceRequest(
         request_info.begin_params->impression->runtime_features;
   }
 
-  new_request->shared_storage_writable = request_info.shared_storage_writable;
+  new_request->shared_storage_writable_eligible =
+      request_info.shared_storage_writable;
 
   return new_request;
 }

services/network/cors/cors_url_loader.cc

@@ -463,8 +463,10 @@ void CorsURLLoader::FollowRedirect(
   }
   request_.headers.MergeFrom(modified_headers);
 
-  if (base::Contains(removed_headers, kSecSharedStorageWritableHeader)) {
-    request_.shared_storage_writable = false;
+  if (GetSecSharedStorageWritableHeader(modified_headers)) {
+    request_.shared_storage_writable_eligible = true;
+  } else if (base::Contains(removed_headers, kSecSharedStorageWritableHeader)) {
+    request_.shared_storage_writable_eligible = false;
   }
 
   if (!allow_any_cors_exempt_header_ &&

services/network/public/cpp/resource_request.cc

@@ -273,7 +273,8 @@ bool ResourceRequest::EqualsForTesting(const ResourceRequest& request) const {
          destination == request.destination &&
          request_body == request.request_body &&
          keepalive == request.keepalive &&
-         shared_storage_writable == request.shared_storage_writable &&
+         shared_storage_writable_eligible ==
+             request.shared_storage_writable_eligible &&
          has_user_gesture == request.has_user_gesture &&
          enable_load_timing == request.enable_load_timing &&
          enable_upload_progress == request.enable_upload_progress &&

services/network/public/cpp/resource_request.h

@@ -175,7 +175,7 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
   bool keepalive = false;
   bool browsing_topics = false;
   bool ad_auction_headers = false;
-  bool shared_storage_writable = false;
+  bool shared_storage_writable_eligible = false;
   bool has_user_gesture = false;
   bool enable_load_timing = false;
   bool enable_upload_progress = false;

services/network/public/cpp/url_request_mojom_traits.cc

@@ -203,7 +203,8 @@ bool StructTraits<
   out->keepalive = data.keepalive();
   out->browsing_topics = data.browsing_topics();
   out->ad_auction_headers = data.ad_auction_headers();
-  out->shared_storage_writable = data.shared_storage_writable();
+  out->shared_storage_writable_eligible =
+      data.shared_storage_writable_eligible();
   out->has_user_gesture = data.has_user_gesture();
   out->enable_load_timing = data.enable_load_timing();
   out->enable_upload_progress = data.enable_upload_progress();

services/network/public/cpp/url_request_mojom_traits.h

@@ -276,8 +276,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE)
   static bool ad_auction_headers(const network::ResourceRequest& request) {
     return request.ad_auction_headers;
   }
-  static bool shared_storage_writable(const network::ResourceRequest& request) {
-    return request.shared_storage_writable;
+  static bool shared_storage_writable_eligible(
+      const network::ResourceRequest& request) {
+    return request.shared_storage_writable_eligible;
   }
   static bool has_user_gesture(const network::ResourceRequest& request) {
     return request.has_user_gesture;

services/network/public/cpp/url_request_mojom_traits_unittest.cc

@@ -85,7 +85,7 @@ TEST(URLRequestMojomTraitsTest, Roundtrips_ResourceRequest) {
   original.keepalive = true;
   original.browsing_topics = true;
   original.ad_auction_headers = true;
-  original.shared_storage_writable = true;
+  original.shared_storage_writable_eligible = true;
   original.has_user_gesture = false;
   original.enable_load_timing = true;
   original.enable_upload_progress = false;

services/network/public/mojom/url_request.mojom

@@ -332,7 +332,7 @@ struct URLRequest {
 
   // True if the request should send the `Sec-Shared-Storage-Writable` request
   // header.
-  bool shared_storage_writable;
+  bool shared_storage_writable_eligible;
 
   // True if the request was user initiated.
   bool has_user_gesture;

services/network/shared_storage/shared_storage_header_utils.cc

@@ -6,6 +6,8 @@
 
 #include "base/containers/fixed_flat_map.h"
 #include "base/strings/string_piece.h"
+#include "net/http/http_request_headers.h"
+#include "net/http/structured_headers.h"
 #include "services/network/public/mojom/url_loader_network_service_observer.mojom.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
@@ -52,4 +54,21 @@ StringToSharedStorageHeaderParamType(base::StringPiece param_str) {
   return param_it->second;
 }
 
+bool GetSecSharedStorageWritableHeader(const net::HttpRequestHeaders& headers) {
+  std::string value;
+  if (!headers.GetHeader(kSecSharedStorageWritableHeader, &value)) {
+    return false;
+  }
+  absl::optional<net::structured_headers::Item> item =
+      net::structured_headers::ParseBareItem(value);
+  if (!item || !item->is_boolean() || !item->GetBoolean()) {
+    // We only expect the value "?1", which parses to boolean true.
+    // TODO(cammie): Log a histogram to see if this ever happens.
+    LOG(ERROR) << "Unexpected value '" << value << "' found for '"
+               << kSecSharedStorageWritableHeader << "' header.";
+    return false;
+  }
+  return true;
+}
+
 }  // namespace network

services/network/shared_storage/shared_storage_header_utils.h

@@ -9,6 +9,10 @@
 #include "services/network/public/mojom/url_loader_network_service_observer.mojom.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
+namespace net {
+class HttpRequestHeaders;
+}  // namespace net
+
 namespace network {
 
 inline constexpr base::StringPiece kSecSharedStorageWritableHeader =
@@ -29,6 +33,8 @@ StringToSharedStorageOperationType(base::StringPiece operation_str);
 absl::optional<SharedStorageHeaderParamType>
 StringToSharedStorageHeaderParamType(base::StringPiece param_str);
 
+bool GetSecSharedStorageWritableHeader(const net::HttpRequestHeaders& headers);
+
 }  // namespace network
 
 #endif  // SERVICES_NETWORK_SHARED_STORAGE_SHARED_STORAGE_HEADER_UTILS_H_

services/network/shared_storage/shared_storage_request_helper.cc

@@ -120,15 +120,16 @@ mojom::SharedStorageOperationPtr MakeSharedStorageOperation(
 }  // namespace
 
 SharedStorageRequestHelper::SharedStorageRequestHelper(
-    bool shared_storage_writable,
+    bool shared_storage_writable_eligible,
     mojom::URLLoaderNetworkServiceObserver* observer)
-    : shared_storage_writable_(shared_storage_writable), observer_(observer) {}
+    : shared_storage_writable_eligible_(shared_storage_writable_eligible),
+      observer_(observer) {}
 
 SharedStorageRequestHelper::~SharedStorageRequestHelper() = default;
 
 void SharedStorageRequestHelper::ProcessOutgoingRequest(
     net::URLRequest& request) {
-  if (!shared_storage_writable_ || !observer_) {
+  if (!shared_storage_writable_eligible_ || !observer_) {
     // This `request` isn't eligible for shared storage writes and/or `this` has
     // no `observer_` to forward any processed shared storage response header
     // to.
@@ -151,7 +152,7 @@ bool SharedStorageRequestHelper::ProcessIncomingResponse(
     return false;
   }
 
-  if (!shared_storage_writable_ || !observer_) {
+  if (!shared_storage_writable_eligible_ || !observer_) {
     // This response has a shared storage header but isn't eligible to write to
     // shared storage (or there is no `observer_` to forward the processed
     // response header to); remove the header(s) to prevent a cross-site data
@@ -164,11 +165,17 @@ bool SharedStorageRequestHelper::ProcessIncomingResponse(
   return ProcessResponse(request, header_value.value(), std::move(done));
 }
 
-void SharedStorageRequestHelper::
-    RemoveEligibilityIfSharedStorageWritableRemoved(
-        const std::vector<std::string>& removed_headers) {
-  if (base::Contains(removed_headers, kSecSharedStorageWritableHeader)) {
-    shared_storage_writable_ = false;
+void SharedStorageRequestHelper::UpdateSharedStorageWritableEligible(
+    const std::vector<std::string>& removed_headers,
+    const net::HttpRequestHeaders& modified_headers) {
+  // Note that in `net::RedirectUtil::UpdateHttpRequest()`, `modified_headers`
+  // are set after `removed_headers` are removed, so if the
+  // `kSecSharedStorageWritableHeader` is in both of these, ``modified_headers`
+  // takes precedence.
+  if (GetSecSharedStorageWritableHeader(modified_headers)) {
+    shared_storage_writable_eligible_ = true;
+  } else if (base::Contains(removed_headers, kSecSharedStorageWritableHeader)) {
+    shared_storage_writable_eligible_ = false;
   }
 }
 

services/network/shared_storage/shared_storage_request_helper.h

@@ -40,15 +40,17 @@ namespace network {
 // `mojom::OnSharedStorageHeaderReceived()`.
 class SharedStorageRequestHelper {
  public:
-  SharedStorageRequestHelper(bool shared_storage_writable,
+  SharedStorageRequestHelper(bool shared_storage_writable_eligible,
                              mojom::URLLoaderNetworkServiceObserver* observer);
 
   ~SharedStorageRequestHelper();
 
-  bool shared_storage_writable() const { return shared_storage_writable_; }
+  bool shared_storage_writable_eligible() const {
+    return shared_storage_writable_eligible_;
+  }
 
-  // If `shared_storage_writable_` is false or there is no `observer_`, then
-  // this is a no-op. Otherwise, this method adds the
+  // If `shared_storage_writable_eligible_` is false or there is no `observer_`,
+  // then this is a no-op. Otherwise, this method adds the
   // `kSecSharedStorageWritableHeader` request header.
   void ProcessOutgoingRequest(net::URLRequest& request);
 
@@ -59,11 +61,13 @@ class SharedStorageRequestHelper {
   bool ProcessIncomingResponse(net::URLRequest& request,
                                base::OnceClosure done);
 
-  // Determines whether, on redirect, a request has lost its eligibility for
-  // shared storage writing. If so, resets `shared_storage_writable_` to false.
-  // Called in `URLLoader::FollowRedirect()`.
-  void RemoveEligibilityIfSharedStorageWritableRemoved(
-      const std::vector<std::string>& removed_headers);
+  // Determines whether, on redirect, a request has lost or regained its
+  // eligibility for shared storage writing. If so, updates
+  // `shared_storage_writable_eligible_` accordingly. Called in
+  // `URLLoader::FollowRedirect()`.
+  void UpdateSharedStorageWritableEligible(
+      const std::vector<std::string>& removed_headers,
+      const net::HttpRequestHeaders& modified_headers);
 
  private:
   bool ProcessResponse(net::URLRequest& request,
@@ -72,7 +76,11 @@ class SharedStorageRequestHelper {
 
   void OnOperationsQueued(base::OnceClosure done);
 
-  bool shared_storage_writable_;
+  // True if the current request should have the
+  // `kSharedStorageWritable` header attached and is eligible to
+  // write to shared storage from response headers.
+  bool shared_storage_writable_eligible_;
+
   raw_ptr<mojom::URLLoaderNetworkServiceObserver> observer_;
   base::WeakPtrFactory<SharedStorageRequestHelper> weak_ptr_factory_{this};
 };

services/network/shared_storage/shared_storage_request_helper_unittest.cc

@@ -123,9 +123,9 @@ class SharedStorageRequestHelperTest : public net::TestWithTaskEnvironment {
 
   net::URLRequestContext& context() { return *context_; }
 
-  void CreateSharedStorageRequestHelper(bool shared_storage_writable) {
+  void CreateSharedStorageRequestHelper(bool shared_storage_writable_eligible) {
     helper_ = std::make_unique<SharedStorageRequestHelper>(
-        shared_storage_writable, observer_.get());
+        shared_storage_writable_eligible, observer_.get());
   }
 
   [[nodiscard]] mojom::URLLoaderNetworkServiceObserver* MaybeGetHeaderObserver(
@@ -222,31 +222,82 @@ class SharedStorageRequestHelperTest : public net::TestWithTaskEnvironment {
 TEST_F(SharedStorageRequestHelperTest,
        SharedStorageWritableHeaderRemoved_EligibityRemoved) {
   GURL request_url = GetHttpsRequestURL();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/true);
-  EXPECT_TRUE(helper_->shared_storage_writable());
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/true);
+  EXPECT_TRUE(helper_->shared_storage_writable_eligible());
 
   std::vector<std::string> removed_headers(
       {std::string(kSecSharedStorageWritableHeader.data(),
                    kSecSharedStorageWritableHeader.size())});
-  helper_->RemoveEligibilityIfSharedStorageWritableRemoved(removed_headers);
-  EXPECT_FALSE(helper_->shared_storage_writable());
+  helper_->UpdateSharedStorageWritableEligible(removed_headers,
+                                               /*modified_headers=*/{});
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
 }
 
 TEST_F(SharedStorageRequestHelperTest,
        SharedStorageWritableHeaderNotRemoved_EligibityNotRemoved) {
   GURL request_url = GetHttpsRequestURL();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/true);
-  EXPECT_TRUE(helper_->shared_storage_writable());
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/true);
+  EXPECT_TRUE(helper_->shared_storage_writable_eligible());
 
-  std::vector<std::string> removed_headers;
-  helper_->RemoveEligibilityIfSharedStorageWritableRemoved(removed_headers);
-  EXPECT_TRUE(helper_->shared_storage_writable());
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               /*modified_headers=*/{});
+  EXPECT_TRUE(helper_->shared_storage_writable_eligible());
+}
+
+TEST_F(SharedStorageRequestHelperTest,
+       SharedStorageWritableHeaderAdded_EligibityRestored) {
+  GURL request_url = GetHttpsRequestURL();
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+
+  net::HttpRequestHeaders modified_headers;
+  modified_headers.SetHeader(kSecSharedStorageWritableHeader, "?1");
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               modified_headers);
+  EXPECT_TRUE(helper_->shared_storage_writable_eligible());
+}
+
+TEST_F(SharedStorageRequestHelperTest,
+       SharedStorageWritableHeaderNotAdded_EligibityNotRestored) {
+  GURL request_url = GetHttpsRequestURL();
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               /*modified_headers=*/{});
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+}
+
+TEST_F(SharedStorageRequestHelperTest,
+       IncorrectSharedStorageWritableHeaderAdded_EligibityNotRestored) {
+  GURL request_url = GetHttpsRequestURL();
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+
+  net::HttpRequestHeaders modified_headers;
+  // Unparsable.
+  modified_headers.SetHeader(kSecSharedStorageWritableHeader, "can=not=parse");
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               modified_headers);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+
+  // Parses to a token item instead of a boolean item.
+  modified_headers.SetHeader(kSecSharedStorageWritableHeader, "hello");
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               modified_headers);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
+
+  // Wrong boolean value.
+  modified_headers.SetHeader(kSecSharedStorageWritableHeader, "?0");
+  helper_->UpdateSharedStorageWritableEligible(/*removed_headers=*/{},
+                                               modified_headers);
+  EXPECT_FALSE(helper_->shared_storage_writable_eligible());
 }
 
 TEST_F(SharedStorageRequestHelperTest,
        ProcessOutgoingRequest_Eligible_RequestHeaderAdded) {
   GURL request_url = GetHttpsRequestURL();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/true);
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/true);
   std::unique_ptr<net::URLRequest> request = CreateTestUrlRequest(request_url);
   RunProcessOutgoingRequest(request.get());
 
@@ -259,7 +310,7 @@ TEST_F(SharedStorageRequestHelperTest,
 TEST_F(SharedStorageRequestHelperTest,
        ProcessOutgoingRequest_NotEligible_RequestHeaderNotAdded) {
   GURL request_url = GetHttpsRequestURL();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/false);
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
   std::unique_ptr<net::URLRequest> request = CreateTestUrlRequest(request_url);
   RunProcessOutgoingRequest(request.get());
 
@@ -275,7 +326,7 @@ TEST_F(SharedStorageRequestHelperTest,
   RegisterSharedStorageHandlerAndStartServer(kHeader);
 
   const GURL kUrl = GetRequestURLFromServer();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/false);
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
   auto r = CreateTestUrlRequest(kUrl);
   r->Start();
   EXPECT_TRUE(r->is_pending());
@@ -298,7 +349,7 @@ TEST_F(SharedStorageRequestHelperTest,
   RegisterSharedStorageHandlerAndStartServer(kHeader);
 
   const GURL kUrl = GetRequestURLFromServer();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/true);
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/true);
   auto r = CreateTestUrlRequest(kUrl);
   r->Start();
   EXPECT_TRUE(r->is_pending());
@@ -323,7 +374,7 @@ TEST_F(SharedStorageRequestHelperTest,
   // Get a request whose response will have the Shared-Storage-Write header even
   // though we don't have a helper.
   const GURL kUrl = GetSharedStorageBypassRequestURLFromServer();
-  CreateSharedStorageRequestHelper(/*shared_storage_writable=*/false);
+  CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/false);
   auto r = CreateTestUrlRequest(kUrl);
   r->Start();
   EXPECT_TRUE(r->is_pending());
@@ -349,7 +400,7 @@ class SharedStorageRequestHelperProcessHeaderTest
     : public SharedStorageRequestHelperTest {
  public:
   [[nodiscard]] std::unique_ptr<net::URLRequest> CreateSharedStorageRequest() {
-    CreateSharedStorageRequestHelper(/*shared_storage_writable=*/true);
+    CreateSharedStorageRequestHelper(/*shared_storage_writable_eligible=*/true);
     GURL request_url = GetRequestURLFromServer();
     request_origin_ = url::Origin::Create(request_url);
     auto request = CreateTestUrlRequest(request_url);