[Topics] fix permissions policy behavior for fetch()

The permissions policy should follow the algorithm in
https://www.w3.org/TR/permissions-policy/#algo-is-feature-enabled plus
the proposed update in
w3c/webappsec-permissions-policy#499.

This CL:
- Removes the renderer side check that checks the policy against
request initiator context's origin. (I misunderstood the spec before). This also means that on permissions error, we won’t throw an exception
in any case, but will just skip adding the topics header.
- Calls IsFeatureEnabledForSubresourceRequest() to align with the
proposed update. This has no behavior difference for now as the
default feature state is `EnableForAll`, but this would fix the
behavior had the default state been switched to `EnableForSelf`.
The distinction was already tested in
PermissionsPolicyTest.ProposedTestIsBrowsingTopicsFeatureEnabledForSubresourceRequest,
thus no test for this change / we cannot easily override the default
feature state outside blink.
- Enforce resource_request.browsing_topics at mojom boundary at
BrowsingTopicsURLLoaderService::CreateLoaderAndStart().

Bug: 1401483
Change-Id: Ibea86eb1d63997955d9ce9de8a81762c2c047318
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111963
Reviewed-by: Josh Karlin <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1087639}

Author: yaoxiachromium

chrome/browser/browsing_topics/browsing_topics_service_browsertest.cc

@@ -1133,24 +1133,4 @@ IN_PROC_BROWSER_TEST_F(BrowsingTopicsBrowserTest, Fetch_Success) {
       content::JsReplace("fetch($1, {browsingTopics: true})", fetch_url)));
 }
 
-IN_PROC_BROWSER_TEST_F(BrowsingTopicsBrowserTest,
-                       Fetch_PermissionsPolicyDisabledInInitiatorContext) {
-  GURL main_frame_url = https_server_.GetURL(
-      "a.test", "/browsing_topics/empty_page_browsing_topics_none.html");
-
-  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), main_frame_url));
-
-  GURL fetch_url =
-      https_server_.GetURL("a.test", "/browsing_topics/empty_page.html");
-
-  content::EvalJsResult result = EvalJs(
-      web_contents()->GetPrimaryMainFrame(),
-      content::JsReplace("fetch($1, {browsingTopics: true})", fetch_url));
-
-  EXPECT_THAT(
-      result.error,
-      testing::HasSubstr("The \"browsing-topics\" Permissions Policy denied "
-                         "the use of fetch(<url>, {browsingTopics: true})."));
-}
-
 }  // namespace browsing_topics

content/browser/browsing_topics/browsing_topics_url_loader.cc

@@ -26,6 +26,7 @@ namespace {
 bool GetTopicsHeaderValueForSubresourceRequest(
     WeakDocumentPtr request_initiator_document,
     const GURL& url,
+    const network::ResourceRequest& request,
     std::string& header_value) {
   DCHECK(header_value.empty());
 
@@ -71,12 +72,13 @@ bool GetTopicsHeaderValueForSubresourceRequest(
       static_cast<RenderFrameHostImpl*>(request_initiator_frame)
           ->permissions_policy();
 
-  if (!permissions_policy->IsFeatureEnabledForOrigin(
-          blink::mojom::PermissionsPolicyFeature::kBrowsingTopics, origin) ||
-      !permissions_policy->IsFeatureEnabledForOrigin(
+  if (!permissions_policy->IsFeatureEnabledForSubresourceRequest(
+          blink::mojom::PermissionsPolicyFeature::kBrowsingTopics, origin,
+          request) ||
+      !permissions_policy->IsFeatureEnabledForSubresourceRequest(
           blink::mojom::PermissionsPolicyFeature::
               kBrowsingTopicsBackwardCompatible,
-          origin)) {
+          origin, request)) {
     return false;
   }
 
@@ -119,24 +121,21 @@ BrowsingTopicsURLLoader::BrowsingTopicsURLLoader(
     scoped_refptr<network::SharedURLLoaderFactory> network_loader_factory)
     : document_(std::move(document)),
       url_(resource_request.url),
+      request_(resource_request),
       forwarding_client_(std::move(client)) {
   DCHECK(network_loader_factory);
 
-  network::ResourceRequest new_resource_request = resource_request;
-
   std::string header_value;
   topics_eligible_ = GetTopicsHeaderValueForSubresourceRequest(
-      document_, new_resource_request.url, header_value);
+      document_, request_.url, request_, header_value);
 
   if (topics_eligible_) {
-    new_resource_request.headers.SetHeader(kBrowsingTopicsRequestHeaderKey,
-                                           header_value);
+    request_.headers.SetHeader(kBrowsingTopicsRequestHeaderKey, header_value);
   }
 
   network_loader_factory->CreateLoaderAndStart(
-      loader_.BindNewPipeAndPassReceiver(), request_id, options,
-      new_resource_request, client_receiver_.BindNewPipeAndPassRemote(),
-      traffic_annotation);
+      loader_.BindNewPipeAndPassReceiver(), request_id, options, request_,
+      client_receiver_.BindNewPipeAndPassRemote(), traffic_annotation);
 
   client_receiver_.set_disconnect_handler(
       base::BindOnce(&BrowsingTopicsURLLoader::OnNetworkConnectionError,
@@ -159,8 +158,8 @@ void BrowsingTopicsURLLoader::FollowRedirect(
   new_removed_headers.push_back(kBrowsingTopicsRequestHeaderKey);
 
   std::string header_value;
-  topics_eligible_ =
-      GetTopicsHeaderValueForSubresourceRequest(document_, url_, header_value);
+  topics_eligible_ = GetTopicsHeaderValueForSubresourceRequest(
+      document_, url_, request_, header_value);
 
   if (topics_eligible_) {
     new_modified_headers.SetHeader(kBrowsingTopicsRequestHeaderKey,

content/browser/browsing_topics/browsing_topics_url_loader.h

@@ -100,6 +100,10 @@ class CONTENT_EXPORT BrowsingTopicsURLLoader
   // The current request or redirect URL.
   GURL url_;
 
+  // The initial request state. This will be used to derive the opt-in
+  // permissions policy features for each request/redirect.
+  network::ResourceRequest request_;
+
   // Whether the ongoing request or redirect is eligible for topics. Set to the
   // desired state when a request/redirect is made. Reset to false when the
   // corresponding response is received.

content/browser/browsing_topics/browsing_topics_url_loader_service.cc

@@ -62,6 +62,14 @@ void BrowsingTopicsURLLoaderService::CreateLoaderAndStart(
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  if (!resource_request.browsing_topics) {
+    loader_factory_receivers_.ReportBadMessage(
+        "Unexpected `resource_request` in "
+        "BrowsingTopicsURLLoaderService::CreateLoaderAndStart(): no "
+        "resource_request.browsing_topics");
+    return;
+  }
+
   const std::unique_ptr<BindContext>& current_context =
       loader_factory_receivers_.current_context();
 

content/browser/browsing_topics/browsing_topics_url_loader_service_unittest.cc

@@ -4,13 +4,16 @@
 
 #include "content/browser/browsing_topics/browsing_topics_url_loader_service.h"
 
+#include "base/test/bind.h"
+#include "base/test/scoped_feature_list.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_utils.h"
 #include "content/public/test/web_contents_tester.h"
 #include "content/test/test_render_frame_host.h"
 #include "content/test/test_render_view_host.h"
+#include "mojo/public/cpp/system/functions.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "services/network/public/cpp/wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -88,6 +91,10 @@ class TopicsInterceptingContentBrowserClient : public ContentBrowserClient {
 
 class BrowsingTopicsURLLoaderServiceTest : public RenderViewHostTestHarness {
  public:
+  BrowsingTopicsURLLoaderServiceTest() {
+    scoped_feature_list_.InitAndEnableFeature(blink::features::kBrowsingTopics);
+  }
+
   void SetUp() override {
     content::RenderViewHostTestHarness::SetUp();
 
@@ -137,9 +144,11 @@ class BrowsingTopicsURLLoaderServiceTest : public RenderViewHostTestHarness {
     return head;
   }
 
-  network::ResourceRequest CreateResourceRequest(const GURL& url) {
+  network::ResourceRequest CreateResourceRequest(const GURL& url,
+                                                 bool browsing_topics = true) {
     network::ResourceRequest request;
     request.url = url;
+    request.browsing_topics = browsing_topics;
     return request;
   }
 
@@ -172,6 +181,8 @@ class BrowsingTopicsURLLoaderServiceTest : public RenderViewHostTestHarness {
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   TopicsInterceptingContentBrowserClient browser_client_;
   raw_ptr<ContentBrowserClient> original_client_ = nullptr;
 
@@ -979,4 +990,43 @@ TEST_F(BrowsingTopicsURLLoaderServiceTest, BindContextClearedDueToDisconnect) {
   EXPECT_FALSE(bind_context);
 }
 
+TEST_F(BrowsingTopicsURLLoaderServiceTest, ReportBadMessageOnInvalidRequest) {
+  NavigatePage(GURL("https://google.com"));
+
+  mojo::Remote<network::mojom::URLLoaderFactory> remote_url_loader_factory;
+  network::TestURLLoaderFactory proxied_url_loader_factory;
+  mojo::Remote<network::mojom::URLLoader> remote_loader;
+  mojo::PendingReceiver<network::mojom::URLLoaderClient> client;
+
+  base::WeakPtr<BrowsingTopicsURLLoaderService::BindContext> bind_context =
+      CreateFactory(proxied_url_loader_factory, remote_url_loader_factory);
+  bind_context->OnDidCommitNavigation(
+      web_contents()->GetPrimaryMainFrame()->GetWeakDocumentPtr());
+
+  std::string received_error;
+  mojo::SetDefaultProcessErrorHandler(base::BindLambdaForTesting(
+      [&](const std::string& error) { received_error = error; }));
+
+  // Invoke CreateLoaderAndStart() with a ResourceRequest invalid for this
+  // factory. This will trigger a ReportBadMessage.
+  remote_url_loader_factory->CreateLoaderAndStart(
+      remote_loader.BindNewPipeAndPassReceiver(),
+      /*request_id=*/0, /*options=*/0,
+      CreateResourceRequest(GURL("https://foo1.com"),
+                            /*browsing_topics=*/false),
+      client.InitWithNewPipeAndPassRemote(),
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS));
+  remote_url_loader_factory.FlushForTesting();
+
+  EXPECT_FALSE(remote_url_loader_factory.is_connected());
+  EXPECT_EQ(0, proxied_url_loader_factory.NumPending());
+  EXPECT_EQ(
+      "Unexpected `resource_request` in "
+      "BrowsingTopicsURLLoaderService::CreateLoaderAndStart(): no "
+      "resource_request.browsing_topics",
+      received_error);
+
+  mojo::SetDefaultProcessErrorHandler(base::NullCallback());
+}
+
 }  // namespace content

third_party/blink/renderer/core/fetch/request.cc

@@ -541,28 +541,6 @@ Request* Request::CreateRequestWithRequestOrString(
       return nullptr;
     }
 
-    // Check the permissions policy on `execution_context` as part of the
-    // "Should request be allowed to use feature?" algorithm
-    // (https://www.w3.org/TR/permissions-policy/#algo-should-request-be-allowed-to-use-feature).
-    // The check against request’s origin is done in `BrowsingTopicsURLLoader`
-    // that is able to cover redirects.
-    if (!execution_context->IsFeatureEnabled(
-            mojom::blink::PermissionsPolicyFeature::kBrowsingTopics)) {
-      exception_state.ThrowTypeError(
-          "The \"browsing-topics\" Permissions Policy denied the use of "
-          "fetch(<url>, {browsingTopics: true}).");
-      return nullptr;
-    }
-
-    if (!execution_context->IsFeatureEnabled(
-            mojom::blink::PermissionsPolicyFeature::
-                kBrowsingTopicsBackwardCompatible)) {
-      exception_state.ThrowTypeError(
-          "The \"interest-cohort\" Permissions Policy denied the use of "
-          "fetch(<url>, {browsingTopics: true}).");
-      return nullptr;
-    }
-
     request->SetBrowsingTopics(init->browsingTopics());
   }