Make AttributionReporting EnabledForAll by default

This CL also adds logic for sending a warning issue to devtools if the API was used in a context where it might break in the future when this change is reverted. See WICG/attribution-reporting-api#551 Bug: 1360563 Change-Id: I9a04b3ce9dbab5ab432c963419cb173e232aa881 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3877268 Reviewed-by: Ian Clelland <iclelland@chromium.org> Reviewed-by: John Delaney <johnidel@chromium.org> Reviewed-by: Andrey Kosyakov <caseq@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Nate Chapin <japhet@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Commit-Position: refs/heads/main@{#1045172} NOKEYCHECK=True GitOrigin-RevId: e959519cfe885d8387f53a50ba8271465dc06347
mjfroman · Sep 9, 2022 · 17758de · 17758de
1 parent 5c4a024
commit 17758de
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 0 deletions.
diff --git a/blink/public/devtools_protocol/browser_protocol.pdl b/blink/public/devtools_protocol/browser_protocol.pdl
@@ -701,6 +701,7 @@ experimental domain Audits
   type AttributionReportingIssueType extends string
     enum
       PermissionPolicyDisabled
+      PermissionPolicyNotDelegated
       UntrustworthyReportingOrigin
       InsecureContext
       # TODO(apaseltiner): Rename this to InvalidRegisterSourceHeader

diff --git a/blink/renderer/core/frame/attribution_src_loader.cc b/blink/renderer/core/frame/attribution_src_loader.cc
@@ -16,6 +16,7 @@
 #include "net/http/structured_headers.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
+#include "third_party/blink/public/common/frame/frame_policy.h"
 #include "third_party/blink/public/common/navigation/impression.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
 #include "third_party/blink/public/mojom/conversions/attribution_data_host.mojom-blink.h"
@@ -24,6 +25,7 @@
 #include "third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom-blink.h"
 #include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/core/frame/attribution_response_parsing.h"
+#include "third_party/blink/renderer/core/frame/frame_owner.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/local_frame_client.h"
@@ -110,6 +112,25 @@ bool IsValidReportingOrigin(const SecurityOrigin* origin) {
           origin->Protocol() == WTF::g_http_atom);
 }
 
+bool SubframeHasAllowedContainerPolicy(LocalFrame* frame) {
+  DCHECK(frame->Parent());
+  const FramePolicy& frame_policy = frame->Owner()->GetFramePolicy();
+  const SecurityOrigin* origin =
+      frame->GetSecurityContext()->GetSecurityOrigin();
+  for (const auto& decl : frame_policy.container_policy) {
+    if (decl.feature ==
+        mojom::blink::PermissionsPolicyFeature::kAttributionReporting) {
+      // TODO(csharrison): This logic duplicates existing code in
+      // PermissionsPolicy::Allowlist. Clean this up by enhancing the logic
+      // exposed by PermissionsPolicy code and re-using that.
+      return decl.matches_all_origins ||
+             (decl.matches_opaque_src && origin->IsOpaque()) ||
+             base::Contains(decl.allowed_origins, origin->ToUrlOrigin());
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 class AttributionSrcLoader::ResourceClient
@@ -353,6 +374,12 @@ AttributionSrcLoader::ReportingOriginForUrlIfValid(
     return nullptr;
   }
 
+  if (local_frame_->Parent() &&
+      !SubframeHasAllowedContainerPolicy(local_frame_)) {
+    maybe_log_audit_issue(
+        AttributionReportingIssueType::kPermissionPolicyNotDelegated);
+  }
+
   if (!window->IsSecureContext()) {
     maybe_log_audit_issue(AttributionReportingIssueType::kInsecureContext,
                           window->GetSecurityContext().GetSecurityOrigin());

diff --git a/blink/renderer/core/inspector/inspector_audits_issue.cc b/blink/renderer/core/inspector/inspector_audits_issue.cc
@@ -143,6 +143,9 @@ BuildAttributionReportingIssueType(AttributionReportingIssueType type) {
     case AttributionReportingIssueType::kPermissionPolicyDisabled:
       return protocol::Audits::AttributionReportingIssueTypeEnum::
           PermissionPolicyDisabled;
+    case AttributionReportingIssueType::kPermissionPolicyNotDelegated:
+      return protocol::Audits::AttributionReportingIssueTypeEnum::
+          PermissionPolicyNotDelegated;
     case AttributionReportingIssueType::kUntrustworthyReportingOrigin:
       return protocol::Audits::AttributionReportingIssueTypeEnum::
           UntrustworthyReportingOrigin;

diff --git a/blink/renderer/core/inspector/inspector_audits_issue.h b/blink/renderer/core/inspector/inspector_audits_issue.h
@@ -105,6 +105,7 @@ enum class RendererCorsIssueCode {
 
 enum class AttributionReportingIssueType {
   kPermissionPolicyDisabled,
+  kPermissionPolicyNotDelegated,
   kUntrustworthyReportingOrigin,
   kInsecureContext,
   kInvalidRegisterSourceHeader,

diff --git a/blink/renderer/core/permissions_policy/permissions_policy_features.json5 b/blink/renderer/core/permissions_policy/permissions_policy_features.json5
@@ -65,6 +65,7 @@
     {
       name: "AttributionReporting",
       permissions_policy_name: "attribution-reporting",
+      feature_default: "EnableForAll",
       depends_on: ["AttributionReporting"],
     },
     {

diff --git a/...http/tests/inspector-protocol/attribution-reporting/non-delegated-permission-expected.txt b/...http/tests/inspector-protocol/attribution-reporting/non-delegated-permission-expected.txt
@@ -0,0 +1,11 @@
+Test that an iframe without the attribution-reporting permission gets a devtools warning
+Issue reported: {
+    code : AttributionReportingIssue
+    details : {
+        attributionReportingIssueDetails : {
+            violatingNodeId : <number>
+            violationType : PermissionPolicyNotDelegated
+        }
+    }
+}
+
diff --git a/...web_tests/http/tests/inspector-protocol/attribution-reporting/non-delegated-permission.js b/...web_tests/http/tests/inspector-protocol/attribution-reporting/non-delegated-permission.js
@@ -0,0 +1,19 @@
+// Copyright 2022 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+(async function(testRunner) {
+  const {dp} = await testRunner.startURL(
+      'https://devtools.test:8443/inspector-protocol/empty.html',
+      'Test that an iframe without the attribution-reporting permission gets a devtools warning');
+
+  await dp.Audits.enable();
+  const issue = dp.Audits.onceIssueAdded();
+
+  await dp.Runtime.evaluate({expression: `
+    document.body.innerHTML = '<iframe src="https://a.devtools.test:8443/inspector-protocol/attribution-reporting/resources/iframe-register-source.html">';
+  `});
+
+  testRunner.log((await issue).params.issue, 'Issue reported: ', ['violatingNodeId']);
+  testRunner.completeTest();
+})
diff --git a/...http/tests/inspector-protocol/attribution-reporting/resources/iframe-register-source.html b/...http/tests/inspector-protocol/attribution-reporting/resources/iframe-register-source.html
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<body>
+  <img attributionsrc src="/inspector-protocol/resources/image.png">
+</body>
+