Fenced frames: Add reporting to custom destination URLs [6/N]

Add input sanitization for the macro keys/values to prevent them from
breaking out of their url query parameters.

WICG/turtledove#477

Change-Id: I5e822378064d32330ba652e11b4edde5e8acb260
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4842629
Reviewed-by: Qingxin Wu <qingxinwu@google.com>
Commit-Queue: Garrett Tanzer <gtanzer@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1197342}

content/services/auction_worklet/bidder_worklet_unittest.cc

@@ -9098,8 +9098,10 @@ TEST_F(BidderWorkletTest,
 class BidderWorkletAdMacroReportingEnabledTest : public BidderWorkletTest {
  public:
   BidderWorkletAdMacroReportingEnabledTest() {
-    scoped_feature_list_.InitAndEnableFeature(
-        blink::features::kAdAuctionReportingWithMacroApi);
+    scoped_feature_list_.InitWithFeatures(
+        /*enabled_features=*/{blink::features::kAdAuctionReportingWithMacroApi,
+                              blink::features::kFencedFramesM119Features},
+        /*disabled_features=*/{});
   }
 
  private:
@@ -9150,6 +9152,18 @@ TEST_F(BidderWorkletAdMacroReportingEnabledTest, ReportWinRegisterAdMacro) {
       /*expected_ad_beacon_map=*/{},
       {{"uppercase", "ABC"}, {"lowercase", "abc"}});
 
+  // URL-encoded strings should be accepted.
+  RunReportWinWithFunctionBodyExpectingResult(
+      R"(registerAdMacro('URL_ENC_KEY', 'http%3A%2F%2Fpub%2Eexample%2Fpage');
+        registerAdMacro('http%3A%2F%2Fpub%2Eexample%2Fpage', 'URL_ENC_VAL');
+        registerAdMacro('URL_ENC_KEY_http%3A%2F', 'URL_ENC_VAL_http%3A%2F');
+      )",
+      /*expected_report_url=*/absl::nullopt,
+      /*expected_ad_beacon_map=*/{},
+      {{"URL_ENC_KEY", "http%3A%2F%2Fpub%2Eexample%2Fpage"},
+       {"http%3A%2F%2Fpub%2Eexample%2Fpage", "URL_ENC_VAL"},
+       {"URL_ENC_KEY_http%3A%2F", "URL_ENC_VAL_http%3A%2F"}});
+
   // When called multiple times for a macro name, use the last valid call's
   // value.
   RunReportWinWithFunctionBodyExpectingResult(
@@ -9193,6 +9207,14 @@ TEST_F(BidderWorkletAdMacroReportingEnabledTest,
       {R"(registerAdMacro('123', {toString:{}});)",
        "https://url.test/:11 Uncaught TypeError: Cannot convert object to "
        "primitive value."},
+      // Invalid characters in macro key.
+      {R"(registerAdMacro('}${FOO', 'foo');)",
+       "https://url.test/:11 Uncaught TypeError: registerAdMacro macro key and "
+       "value must be URL-encoded."},
+      // Invalid characters in macro value.
+      {R"(registerAdMacro('MACRO_KEY', 'baz&foo=bar');)",
+       "https://url.test/:11 Uncaught TypeError: registerAdMacro macro key and "
+       "value must be URL-encoded."},
   };
   for (const auto& test_case : kTestCases) {
     RunReportWinWithFunctionBodyExpectingResult(

content/services/auction_worklet/register_ad_macro_bindings.cc

@@ -14,6 +14,7 @@
 #include "content/services/auction_worklet/public/mojom/bidder_worklet.mojom.h"
 #include "content/services/auction_worklet/webidl_compat.h"
 #include "third_party/blink/public/common/features.h"
+#include "url/url_util.h"
 #include "v8/include/v8-exception.h"
 #include "v8/include/v8-external.h"
 #include "v8/include/v8-function-callback.h"
@@ -67,6 +68,21 @@ void RegisterAdMacroBindings::RegisterAdMacro(
     return;
   }
 
+  auto ContainsDisallowedCharacters = [](const std::string& str) -> bool {
+    return base::ranges::any_of(
+        str, [](char c) { return !url::IsURIComponentChar(c) && c != '%'; });
+  };
+
+  if (base::FeatureList::IsEnabled(
+          blink::features::kFencedFramesM119Features)) {
+    if (ContainsDisallowedCharacters(macro_name) ||
+        ContainsDisallowedCharacters(macro_value)) {
+      args.GetIsolate()->ThrowException(
+          v8::Exception::TypeError(v8_helper->CreateStringFromLiteral(
+              "registerAdMacro macro key and value must be URL-encoded")));
+    }
+  }
+
   bindings->ad_macro_map_[macro_name] = macro_value;
 }
 

third_party/blink/common/features.cc

@@ -760,6 +760,8 @@ BASE_FEATURE(kFencedFrames, "FencedFrames", base::FEATURE_DISABLED_BY_DEFAULT);
 // * Extra format for ad size macro substitution:
 //   ${AD_WIDTH} and ${AD_HEIGHT}, on top of the previous
 //   {%AD_WIDTH%} and {%AD_HEIGHT%}.
+// * Input validation (no disallowed URI component characters) in
+//   registerAdMacro keys and values.
 BASE_FEATURE(kFencedFramesM119Features,
              "FencedFramesM119Features",
              base::FEATURE_DISABLED_BY_DEFAULT);

url/url_util.cc

@@ -918,6 +918,10 @@ void EncodeURIComponent(const char* input, int length, CanonOutput* output) {
   }
 }
 
+bool IsURIComponentChar(char c) {
+  return IsComponentChar(c);
+}
+
 bool CompareSchemeComponent(const char* spec,
                             const Component& component,
                             const char* compare_to) {

url/url_util.h

@@ -309,6 +309,13 @@ void DecodeURLEscapeSequences(const char* input,
 COMPONENT_EXPORT(URL)
 void EncodeURIComponent(const char* input, int length, CanonOutput* output);
 
+// Returns true if `c` is a character that does not require escaping in
+// encodeURIComponent.
+// TODO(crbug.com/1481056): Remove this when event-level reportEvent is removed
+// (if it is still this function's only consumer).
+COMPONENT_EXPORT(URL)
+bool IsURIComponentChar(char c);
+
 }  // namespace url
 
 #endif  // URL_URL_UTIL_H_