Skip to content

Commit

Permalink
Shared Storage: Add iframe attribute for response header writes
Browse files Browse the repository at this point in the history
We include the previously added mixin interface
`HTMLSharedStorageWritableElementUtils` with boolean attribute
`sharedStorageWritable` in `HTMLIFrameElement`.

The value of the IDL attribute is plumbed to the new field
`blink::mojom::IframeAttributes::shared_storage_writable`, and then
`blink::mojom::LocalFrameHost::DidChangeIframeAttributes()` notifies `FrameTreeNode` the attribute value is parsed or changes.

We add infra to `NavigationRequest` and `NavigationRequestInfo` to
check PermissionsPolicy and plumb the writable value to
`network::ResourceRequest`.

Since the `URLLoaderNetworkContext::Type` for iframe navigations is
`URLLoaderNetworkContext::Type::kServiceWorkerContext` (https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:content/browser/storage_partition_impl.cc;drc=b27071598a6b291a7319d99ce7aa4af81ebc060f;l=3329), we have to use a workaround to send a
`RenderFrameHost` to `SharedStorageHeaderObserver::HeaderReceived()`.

Hence we add
`network::ResourceRequest::TrustedParams::shared_storage_writable_id`, which in the case of (eligible) iframe navigations will be the
existing `NavigationRequest::navigation_id_`, and otherwise -1. This
long integer is sent over mojom and can thus be sent back again via
`URLLoaderNetworkServiceObserver::OnSharedStorageHeaderReceived()` in
order to identify and defer a header's operations, which will then be
looked up by id and invoked on iframe commit.

This is part of a project to add support to writing to shared storage
from response headers. See
WICG/shared-storage#70 for more information
and https://crrev.com/c/4691573 for a prototype for adding image and
iframe attributes.

Browser tests are added in https://crrev.com/c/4718688.

Bug: 1434529,1218540
Change-Id: I290a8e06cf4824845e12421bbd34906f9399b25f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4717613
Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Commit-Queue: Cammie Smith Barnes <cammie@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187498}
  • Loading branch information
pythagoraskitty authored and Chromium LUCI CQ committed Aug 23, 2023
1 parent 65a4334 commit 6d2966c
Show file tree
Hide file tree
Showing 33 changed files with 502 additions and 51 deletions.
1 change: 0 additions & 1 deletion content/browser/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ specific_include_rules = {
],
"navigation_url_loader_impl_unittest\.cc": [
"+services/network/resource_scheduler/resource_scheduler_client.h",
"+services/network/shared_storage/shared_storage_request_helper.h",
"+services/network/url_loader.h",
"+services/network/url_request_context_owner.h",
],
Expand Down
1 change: 1 addition & 0 deletions content/browser/bad_message.h
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ enum BadMessageReason {
RFH_CAN_ACCESS_FILES_OF_PAGE_STATE_AT_COMMIT = 303,
PERMISSION_SERVICE_REQUEST_EMBEDDED_PERMISSION_WITHOUT_FEATURE = 304,
RFH_FOCUS_ACROSS_FENCED_BOUNDARY = 305,
RFH_RECEIVED_INVALID_SHARED_STORAGE_WRITABLE_ATTRIBUTE = 306,

// Please add new elements here. The naming convention is abbreviated class
// name (e.g. RenderFrameHost becomes RFH) plus a unique description of the
Expand Down
2 changes: 2 additions & 0 deletions content/browser/loader/navigation_url_loader_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +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;

return new_request;
}

Expand Down
8 changes: 3 additions & 5 deletions content/browser/loader/navigation_url_loader_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
#include "services/network/public/cpp/url_loader_completion_status.h"
#include "services/network/public/mojom/fetch_api.mojom.h"
#include "services/network/resource_scheduler/resource_scheduler_client.h"
#include "services/network/shared_storage/shared_storage_request_helper.h"
#include "services/network/test/url_loader_context_for_tests.h"
#include "services/network/url_loader.h"
#include "services/network/url_request_context_owner.h"
Expand Down Expand Up @@ -129,9 +128,7 @@ class TestNavigationLoaderInterceptor : public NavigationLoaderInterceptor {
/*accept_ch_frame_observer=*/mojo::NullRemote(),
net::CookieSettingOverrides(),
/*attribution_request_helper=*/nullptr,
std::make_unique<network::SharedStorageRequestHelper>(
/*shared_storage_writable=*/false,
/*observer=*/nullptr));
/*shared_storage_writable=*/false);
}

bool MaybeCreateLoaderForResponse(
Expand Down Expand Up @@ -283,7 +280,8 @@ class NavigationURLLoaderImplTest : public testing::Test {
false /* is_pdf */,
content::WeakDocumentPtr() /* initiator_document */,
GlobalRenderFrameHostId() /* previous_render_frame_host_id */,
false /* allow_cookies_from_browser */, 0 /* navigation_id */));
false /* allow_cookies_from_browser */, 0 /* navigation_id */,
false /* shared_storage_writable */));
std::vector<std::unique_ptr<NavigationLoaderInterceptor>> interceptors;
most_recent_resource_request_ = absl::nullopt;
interceptors.push_back(std::make_unique<TestNavigationLoaderInterceptor>(
Expand Down
3 changes: 2 additions & 1 deletion content/browser/loader/navigation_url_loader_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ class NavigationURLLoaderTest : public testing::Test {
false /* is_pdf */,
content::WeakDocumentPtr() /* initiator_document */,
GlobalRenderFrameHostId() /* previous_render_frame_host_id */,
false /* allow_cookies_from_browser */, 0 /* navigation_id */));
false /* allow_cookies_from_browser */, 0 /* navigation_id */,
false /* shared_storage_writable */));
return NavigationURLLoader::Create(
browser_context_.get(), storage_partition, std::move(request_info),
nullptr, nullptr, nullptr, delegate,
Expand Down
8 changes: 8 additions & 0 deletions content/browser/renderer_host/frame_tree_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,14 @@ class CONTENT_EXPORT FrameTreeNode : public RenderFrameHostOwner {
// navigation requests on this frame should calculate and send the
// `Sec-Browsing-Topics` header.
bool browsing_topics() const { return attributes_->browsing_topics; }

// Tracks iframe's 'sharedstoragewritable' attribute, indicating what value
// the the corresponding `network::ResourceRequest::shared_storage_writable`
// should take for the navigation(s) on this frame. If true, the network
// service will send the `Shared-Storage-Write` request header.
bool shared_storage_writable() const {
return attributes_->shared_storage_writable;
}
const absl::optional<std::string> html_id() const { return attributes_->id; }
// This tracks iframe's 'name' attribute instead of window.name, which is
// tracked in FrameReplicationState. See the comment for frame_name() for
Expand Down
61 changes: 60 additions & 1 deletion content/browser/renderer_host/navigation_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
#include "content/browser/scoped_active_url.h"
#include "content/browser/service_worker/service_worker_context_wrapper.h"
#include "content/browser/service_worker/service_worker_main_resource_handle.h"
#include "content/browser/shared_storage/shared_storage_header_observer.h"
#include "content/browser/site_info.h"
#include "content/browser/site_instance_impl.h"
#include "content/browser/storage_partition_impl.h"
Expand Down Expand Up @@ -239,6 +240,8 @@ const char kIsolatedAppCSP[] =
"style-src 'self' 'unsafe-inline';"
"require-trusted-types-for 'script';";

const char kSharedStorageWritableRequestHeaderKey[] = "Shared-Storage-Writable";

// Denotes the type of user agent string value sent in the User-Agent request
// header.
//
Expand Down Expand Up @@ -1051,6 +1054,46 @@ network::mojom::WebSandboxFlags GetSandboxFlagsInitiator(
return policy_container_host->policies().sandbox_flags;
}

bool IsSharedStorageWritableForNavigationRequest(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()) {
return false;
}

// Only child frames should have the `sharedstoragewritable` attribute set to
// true.
CHECK(!frame_tree_node->IsMainFrame());

// Apart from fenced frames' frame trees, skip non-primary pages (e.g. portal,
// prerendered pages).
if (frame_tree_node->fenced_frame_status() !=
RenderFrameHostImpl::FencedFrameStatus::
kIframeNestedWithinFencedFrame &&
(frame_tree_node->frame_tree().type() != FrameTree::Type::kPrimary ||
!frame_tree_node->frame_tree().root()->IsOutermostMainFrame())) {
return false;
}

url::Origin origin = url::Origin::Create(url);
if (origin.opaque()) {
return false;
}

if (!network::IsOriginPotentiallyTrustworthy(origin)) {
return false;
}

CHECK(frame_tree_node->parent());
const blink::PermissionsPolicy* parent_policy =
frame_tree_node->parent()->permissions_policy();

DCHECK(parent_policy);
return parent_policy->IsFeatureEnabledForOrigin(
blink::mojom::PermissionsPolicyFeature::kSharedStorage, origin);
}

} // namespace

NavigationRequest::PrerenderActivationNavigationState::
Expand Down Expand Up @@ -1667,6 +1710,12 @@ NavigationRequest::NavigationRequest(
blink::kFencedFrameForcedSandboxFlags;
}

if (base::FeatureList::IsEnabled(blink::features::kSharedStorageAPI) &&
base::FeatureList::IsEnabled(blink::features::kSharedStorageAPIM118)) {
shared_storage_writable_ = IsSharedStorageWritableForNavigationRequest(
frame_tree_node_, common_params_->url);
}

if (from_begin_navigation_) {
// This is needed to have data URLs commit in the same SiteInstance as the
// initiating renderer.
Expand Down Expand Up @@ -4897,7 +4946,7 @@ void NavigationRequest::OnStartChecksComplete(
BuildClientSecurityStateForNavigationFetch(),
devtools_accepted_stream_types, is_pdf_, initiator_document_,
GetPreviousRenderFrameHostId(), allow_cookies_from_browser_,
navigation_id_),
navigation_id_, shared_storage_writable_),
std::move(navigation_ui_data), service_worker_handle_.get(),
std::move(prefetched_signed_exchange_cache_), this, loader_type,
CreateCookieAccessObserver(), CreateTrustTokenAccessObserver(),
Expand Down Expand Up @@ -5059,6 +5108,16 @@ void NavigationRequest::OnRedirectChecksComplete(
*topics_header_value);
}

if (shared_storage_writable_) {
// 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(kSharedStorageWritableRequestHeaderKey);
}
}

// Removes all Client Hints from the request, that were passed on from the
// previous one.
for (const auto& elem : network::GetClientHintToNameMap()) {
Expand Down
7 changes: 7 additions & 0 deletions content/browser/renderer_host/navigation_request.h
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,8 @@ class CONTENT_EXPORT NavigationRequest
return std::move(web_ui_);
}

bool shared_storage_writable() const { return shared_storage_writable_; }

enum ErrorPageProcess {
kNotErrorPage,
kPostCommitErrorPage,
Expand Down Expand Up @@ -2558,6 +2560,11 @@ 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
// response headers. See
// https://github.com/WICG/shared-storage#from-response-headers
bool shared_storage_writable_ = false;

// A WeakPtr for the BindContext associated with the browser routing loader
// factory for the committing document. This will be set in
// `CommitNavigation()`, and can become null if the corresponding factory is
Expand Down
6 changes: 4 additions & 2 deletions content/browser/renderer_host/navigation_request_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ NavigationRequestInfo::NavigationRequestInfo(
WeakDocumentPtr initiator_document,
const GlobalRenderFrameHostId& previous_render_frame_host_id,
bool allow_cookies_from_browser,
int64_t navigation_id)
int64_t navigation_id,
bool shared_storage_writable)
: common_params(std::move(common_params)),
begin_params(std::move(begin_params)),
sandbox_flags(sandbox_flags),
Expand All @@ -55,7 +56,8 @@ NavigationRequestInfo::NavigationRequestInfo(
initiator_document(std::move(initiator_document)),
previous_render_frame_host_id(previous_render_frame_host_id),
allow_cookies_from_browser(allow_cookies_from_browser),
navigation_id(navigation_id) {}
navigation_id(navigation_id),
shared_storage_writable(shared_storage_writable) {}

NavigationRequestInfo::~NavigationRequestInfo() {}

Expand Down
8 changes: 7 additions & 1 deletion content/browser/renderer_host/navigation_request_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ struct CONTENT_EXPORT NavigationRequestInfo {
WeakDocumentPtr initiator_document,
const GlobalRenderFrameHostId& previous_render_frame_host_id,
bool allow_cookies_from_browser,
int64_t navigation_id);
int64_t navigation_id,
bool shared_storage_writable);
NavigationRequestInfo(const NavigationRequestInfo& other) = delete;
~NavigationRequestInfo();

Expand Down Expand Up @@ -145,6 +146,11 @@ struct CONTENT_EXPORT NavigationRequestInfo {

// Unique id that identifies the navigation.
const int64_t navigation_id;

// 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;
};

} // namespace content
Expand Down
87 changes: 87 additions & 0 deletions content/browser/renderer_host/navigation_request_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "services/network/public/cpp/content_security_policy/content_security_policy.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/common/features.h"
#include "third_party/blink/public/common/navigation/navigation_params.h"
#include "third_party/blink/public/common/origin_trials/origin_trial_feature.h"
#include "third_party/blink/public/common/origin_trials/scoped_test_origin_trial_policy.h"
Expand Down Expand Up @@ -479,6 +480,92 @@ TEST_F(NavigationRequestTest, WillFailRequestSetsSSLInfo) {
navigation->GetNavigationHandle()->GetSSLInfo()->connection_status);
}

TEST_F(NavigationRequestTest, SharedStorageWritable) {
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeatures(
/*enabled_features=*/{blink::features::kSharedStorageAPI,
blink::features::kSharedStorageAPIM118,
blink::features::kFencedFrames},
/*disabled_features=*/{});

// Create and start a simulated `NavigationRequest` for the main frame.
GURL main_url = GURL("https://main.com");
auto main_navigation =
NavigationSimulatorImpl::CreateBrowserInitiated(main_url, contents());
main_navigation->Start();
main_navigation->ReadyToCommit();

// Verify that the main frame's `NavigationRequest` will not be
// SharedStorageWritable.
ASSERT_TRUE(main_navigation->GetNavigationHandle());
EXPECT_FALSE(
main_navigation->GetNavigationHandle()->shared_storage_writable());

// Commit the navigation.
main_navigation->Commit();

// Append a child frame and set its `shared_storage_writable` attribute to
// true.
TestRenderFrameHost* child_frame = static_cast<TestRenderFrameHost*>(
content::RenderFrameHostTester::For(main_rfh())->AppendChild("child"));
blink::mojom::IframeAttributesPtr child_attributes =
blink::mojom::IframeAttributes::New();
child_attributes->shared_storage_writable = true;
child_frame->frame_tree_node()->SetAttributes(std::move(child_attributes));

// Create and start a simulated `NavigationRequest` for the child frame.
GURL a_url = GURL("https://a.com");
auto child_navigation =
NavigationSimulatorImpl::CreateRendererInitiated(a_url, child_frame);
child_navigation->Start();

// Verify that the `NavigationRequest` will be SharedStorageWritable.
ASSERT_TRUE(child_navigation->GetNavigationHandle());
EXPECT_TRUE(
child_navigation->GetNavigationHandle()->shared_storage_writable());

// Commit the navigation.
child_navigation->Commit();

// Append a fenced frame and give it permission to access Shared Storage.
TestRenderFrameHost* fenced_frame_root = static_cast<TestRenderFrameHost*>(
content::RenderFrameHostTester::For(main_rfh())->AppendFencedFrame());
FrameTreeNode* fenced_frame_node =
static_cast<RenderFrameHostImpl*>(fenced_frame_root)->frame_tree_node();
absl::optional<FencedFrameProperties> new_props =
fenced_frame_node->GetFencedFrameProperties();
new_props->effective_enabled_permissions.push_back(
blink::mojom::PermissionsPolicyFeature::kSharedStorage);
fenced_frame_node->set_fenced_frame_properties(new_props);
fenced_frame_root->ResetPermissionsPolicy();

// Append a child frame to the fenced frame root and set its
// `shared_storage_writable` attribute to true.
TestRenderFrameHost* child_of_fenced_frame =
static_cast<TestRenderFrameHost*>(
fenced_frame_root->AppendChild("child_of_fenced"));
blink::mojom::IframeAttributesPtr child_of_fenced_frame_attributes =
blink::mojom::IframeAttributes::New();
child_of_fenced_frame_attributes->shared_storage_writable = true;
child_of_fenced_frame->frame_tree_node()->SetAttributes(
std::move(child_of_fenced_frame_attributes));

// Create and start a simulated `NavigationRequest` for the child frame.
GURL b_url = GURL("https://b.com");
auto child_of_fenced_frame_navigation =
NavigationSimulatorImpl::CreateRendererInitiated(b_url,
child_of_fenced_frame);
child_of_fenced_frame_navigation->Start();

// 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());

// Commit the navigation.
child_of_fenced_frame_navigation->Commit();
}

namespace {

// Helper throttle which checks that it can access NavigationHandle's
Expand Down
8 changes: 8 additions & 0 deletions content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7813,6 +7813,14 @@ void RenderFrameHostImpl::DidChangeIframeAttributes(
return;
}

if (attributes->shared_storage_writable &&
(!base::FeatureList::IsEnabled(blink::features::kSharedStorageAPIM118))) {
bad_message::ReceivedBadMessage(
GetProcess(),
bad_message::RFH_RECEIVED_INVALID_SHARED_STORAGE_WRITABLE_ATTRIBUTE);
return;
}

auto* child = FindAndVerifyChild(
child_frame_token, bad_message::RFH_DID_CHANGE_IFRAME_ATTRIBUTE);
if (!child)
Expand Down
1 change: 1 addition & 0 deletions content/browser/renderer_host/render_frame_host_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3227,6 +3227,7 @@ class CONTENT_EXPORT RenderFrameHostImpl
FRIEND_TEST_ALL_PREFIXES(
NavigationBrowserTest,
NavigationSuddenTerminationDisablerTypeRecordUmaActivation);
FRIEND_TEST_ALL_PREFIXES(NavigationRequestTest, SharedStorageWritable);

class SubresourceLoaderFactoriesConfig;

Expand Down
1 change: 0 additions & 1 deletion content/test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2951,7 +2951,6 @@ test("content_unittests") {
"//services/network:test_support",
"//services/network/public/cpp",
"//services/network/public/mojom",
"//services/network/shared_storage:shared_storage",
"//services/service_manager/public/cpp/test:test_support",
"//services/video_capture/public/cpp:mocks",
"//services/video_capture/public/mojom",
Expand Down

0 comments on commit 6d2966c

Please sign in to comment.