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 <[email protected]> Commit-Queue: Garrett Tanzer <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Cr-Commit-Position: refs/heads/main@{#1197342}
chromium · Sep 15, 2023 · 98c1fd8 · 98c1fd8
1 parent 6bcb11e
commit 98c1fd8
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 2 deletions.
diff --git a/content/services/auction_worklet/bidder_worklet_unittest.cc b/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(

diff --git a/content/services/auction_worklet/register_ad_macro_bindings.cc b/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;
 }
 

diff --git a/third_party/blink/common/features.cc b/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);

diff --git a/url/url_util.cc b/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) {

diff --git a/url/url_util.h b/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_