Removes silent defaults from source and trigger registration fields

Silent defaults are maintained if the keys do not exist in the registration. An exception is made for `debug_key`. See WICG/attribution-reporting-api#825 OBSOLETE_HISTOGRAM[Conversions.SourceRegistrationError2]=Replaced with Conversions.SourceRegistrationError3 due to enum value change. OBSOLETE_HISTOGRAM[Conversions.TriggerRegistrationError5]=Replaced with Conversions.TriggerRegistrationError6 due to enum value change. (cherry picked from commit 5af0b58) Bug: 1449248 Change-Id: I7489727e68389877b0c1983a5c7dd0bb9a172bf7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4566956 Commit-Queue: Thomas Quintanilla <tquintanilla@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1151369} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4591753 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: John Delaney <johnidel@chromium.org> Auto-Submit: Thomas Quintanilla <tquintanilla@chromium.org> Reviewed-by: John Delaney <johnidel@chromium.org> Cr-Commit-Position: refs/branch-heads/5790@{#409} Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
chromium · Jun 6, 2023 · 5ba9fa6 · 5ba9fa6
1 parent d116360
commit 5ba9fa6
Show file tree

Hide file tree

Showing 16 changed files with 270 additions and 107 deletions.
diff --git a/components/attribution_reporting/aggregatable_dedup_key.cc b/components/attribution_reporting/aggregatable_dedup_key.cc
@@ -36,7 +36,11 @@ AggregatableDedupKey::FromJSON(base::Value& value) {
   if (!filters.has_value()) {
     return base::unexpected(filters.error());
   }
-  absl::optional<uint64_t> dedup_key = ParseDeduplicationKey(*dict);
+  absl::optional<uint64_t> dedup_key;
+  if (!ParseDeduplicationKey(*dict, dedup_key)) {
+    return base::unexpected(
+        TriggerRegistrationError::kAggregatableDedupKeyValueInvalid);
+  }
   return AggregatableDedupKey(dedup_key, std::move(*filters));
 }
 

diff --git a/components/attribution_reporting/aggregatable_dedup_key_unittest.cc b/components/attribution_reporting/aggregatable_dedup_key_unittest.cc
@@ -45,12 +45,14 @@ TEST(AggregatableDedupKeyTest, FromJSON) {
       {
           "dedup_key_wrong_type",
           R"json({"deduplication_key":123})json",
-          AggregatableDedupKey(),
+          base::unexpected(
+              TriggerRegistrationError::kAggregatableDedupKeyValueInvalid),
       },
       {
           "dedup_key_invalid",
           R"json({"deduplication_key":"abc"})json",
-          AggregatableDedupKey(),
+          base::unexpected(
+              TriggerRegistrationError::kAggregatableDedupKeyValueInvalid),
       },
       {
           "filters_valid",

diff --git a/components/attribution_reporting/event_trigger_data.cc b/components/attribution_reporting/event_trigger_data.cc
@@ -37,11 +37,26 @@ EventTriggerData::FromJSON(base::Value& value) {
   if (!filters.has_value())
     return base::unexpected(filters.error());
 
-  uint64_t data = ParseUint64(*dict, kTriggerData).value_or(0);
-  int64_t priority = ParsePriority(*dict);
-  absl::optional<uint64_t> dedup_key = ParseDeduplicationKey(*dict);
+  absl::optional<uint64_t> data;
+  if (!ParseUint64(*dict, kTriggerData, data)) {
+    return base::unexpected(
+        TriggerRegistrationError::kEventTriggerDataValueInvalid);
+  }
+
+  absl::optional<int64_t> priority;
+  if (!ParsePriority(*dict, priority)) {
+    return base::unexpected(
+        TriggerRegistrationError::kEventPriorityValueInvalid);
+  }
+
+  absl::optional<uint64_t> dedup_key;
+  if (!ParseDeduplicationKey(*dict, dedup_key)) {
+    return base::unexpected(
+        TriggerRegistrationError::kEventDedupKeyValueInvalid);
+  }
 
-  return EventTriggerData(data, priority, dedup_key, std::move(*filters));
+  return EventTriggerData(data.value_or(0), priority.value_or(0), dedup_key,
+                          std::move(*filters));
 }
 
 EventTriggerData::EventTriggerData() = default;

diff --git a/components/attribution_reporting/event_trigger_data_unittest.cc b/components/attribution_reporting/event_trigger_data_unittest.cc
@@ -44,12 +44,14 @@ TEST(EventTriggerDataTest, FromJSON) {
       {
           "trigger_data_wrong_type",
           R"json({"trigger_data":123})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventTriggerDataValueInvalid),
       },
       {
           "trigger_data_invalid",
           R"json({"trigger_data":"-5"})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventTriggerDataValueInvalid),
       },
       {
           "priority_valid",
@@ -60,12 +62,14 @@ TEST(EventTriggerDataTest, FromJSON) {
       {
           "priority_wrong_type",
           R"json({"priority":123})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventPriorityValueInvalid),
       },
       {
           "priority_invalid",
           R"json({"priority":"abc"})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventPriorityValueInvalid),
       },
       {
           "dedup_key_valid",
@@ -76,12 +80,14 @@ TEST(EventTriggerDataTest, FromJSON) {
       {
           "dedup_key_wrong_type",
           R"json({"deduplication_key":123})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventDedupKeyValueInvalid),
       },
       {
           "dedup_key_invalid",
           R"json({"deduplication_key":"abc"})json",
-          EventTriggerData(),
+          base::unexpected(
+              TriggerRegistrationError::kEventDedupKeyValueInvalid),
       },
       {
           "filters_valid",

diff --git a/components/attribution_reporting/parsing_utils.cc b/components/attribution_reporting/parsing_utils.cc
@@ -51,38 +51,63 @@ std::string HexEncodeAggregationKey(absl::uint128 value) {
   return out.str();
 }
 
-absl::optional<uint64_t> ParseUint64(const base::Value::Dict& dict,
-                                     base::StringPiece key) {
-  const std::string* s = dict.FindString(key);
-  if (!s)
-    return absl::nullopt;
+bool ParseUint64(const base::Value::Dict& dict,
+                 base::StringPiece key,
+                 absl::optional<uint64_t>& out) {
+  const base::Value* value = dict.Find(key);
+  if (!value) {
+    out = absl::nullopt;
+    return true;
+  }
+
+  const std::string* str = value->GetIfString();
+  if (!str) {
+    out = absl::nullopt;
+    return false;
+  }
 
-  uint64_t value;
-  return base::StringToUint64(*s, &value) ? absl::make_optional(value)
-                                          : absl::nullopt;
+  uint64_t parsed_val;
+  out = base::StringToUint64(*str, &parsed_val)
+            ? absl::make_optional(parsed_val)
+            : absl::nullopt;
+  return out.has_value();
 }
 
-absl::optional<int64_t> ParseInt64(const base::Value::Dict& dict,
-                                   base::StringPiece key) {
-  const std::string* s = dict.FindString(key);
-  if (!s)
-    return absl::nullopt;
+bool ParseInt64(const base::Value::Dict& dict,
+                base::StringPiece key,
+                absl::optional<int64_t>& out) {
+  const base::Value* value = dict.Find(key);
+  if (!value) {
+    out = absl::nullopt;
+    return true;
+  }
+
+  const std::string* str = value->GetIfString();
+  if (!str) {
+    out = absl::nullopt;
+    return false;
+  }
 
-  int64_t value;
-  return base::StringToInt64(*s, &value) ? absl::make_optional(value)
-                                         : absl::nullopt;
+  int64_t parsed_val;
+  out = base::StringToInt64(*str, &parsed_val) ? absl::make_optional(parsed_val)
+                                               : absl::nullopt;
+  return out.has_value();
 }
 
-int64_t ParsePriority(const base::Value::Dict& dict) {
-  return ParseInt64(dict, kPriority).value_or(0);
+bool ParsePriority(const base::Value::Dict& dict,
+                   absl::optional<int64_t>& out) {
+  return ParseInt64(dict, kPriority, out);
 }
 
 absl::optional<uint64_t> ParseDebugKey(const base::Value::Dict& dict) {
-  return ParseUint64(dict, kDebugKey);
+  absl::optional<uint64_t> debug_key;
+  std::ignore = ParseUint64(dict, kDebugKey, debug_key);
+  return debug_key;
 }
 
-absl::optional<uint64_t> ParseDeduplicationKey(const base::Value::Dict& dict) {
-  return ParseUint64(dict, kDeduplicationKey);
+bool ParseDeduplicationKey(const base::Value::Dict& dict,
+                           absl::optional<uint64_t>& out) {
+  return ParseUint64(dict, kDeduplicationKey, out);
 }
 
 bool ParseDebugReporting(const base::Value::Dict& dict) {

diff --git a/components/attribution_reporting/parsing_utils.h b/components/attribution_reporting/parsing_utils.h
@@ -29,21 +29,38 @@ std::string HexEncodeAggregationKey(absl::uint128);
 COMPONENT_EXPORT(ATTRIBUTION_REPORTING)
 bool AggregationKeyIdHasValidLength(const std::string& key);
 
-COMPONENT_EXPORT(ATTRIBUTION_REPORTING)
-absl::optional<uint64_t> ParseUint64(const base::Value::Dict& dict,
-                                     base::StringPiece key);
-
-COMPONENT_EXPORT(ATTRIBUTION_REPORTING)
-absl::optional<int64_t> ParseInt64(const base::Value::Dict& dict,
-                                   base::StringPiece key);
-
-int64_t ParsePriority(const base::Value::Dict& dict);
-
+// Returns false if `dict` contains `key` but the value is invalid (e.g. not a
+// string, negative), returns true otherwise.
+[[nodiscard]] COMPONENT_EXPORT(ATTRIBUTION_REPORTING) bool ParseUint64(
+    const base::Value::Dict& dict,
+    base::StringPiece key,
+    absl::optional<uint64_t>& out);
+
+// Returns false if `dict` contains `key` but the value is invalid (e.g. not a
+// string, int64 overflow), returns true otherwise.
+[[nodiscard]] COMPONENT_EXPORT(ATTRIBUTION_REPORTING) bool ParseInt64(
+    const base::Value::Dict& dict,
+    base::StringPiece key,
+    absl::optional<int64_t>& out);
+
+// Returns false if `dict` contains `priority` key but the value is invalid,
+// returns true otherwise.
+[[nodiscard]] bool ParsePriority(const base::Value::Dict& dict,
+                                 absl::optional<int64_t>& out);
+
+// Returns `debug_key` value as we do not need to fail the source registration
+// if the value is invalid, see
+// https://github.com/WICG/attribution-reporting-api/issues/793 for context.
 absl::optional<uint64_t> ParseDebugKey(const base::Value::Dict& dict);
 
-bool ParseDebugReporting(const base::Value::Dict& dict);
+// Returns false if `dict` contains `debug_reporting` key but the value is
+// invalid, returns true otherwise.
+[[nodiscard]] bool ParseDebugReporting(const base::Value::Dict& dict);
 
-absl::optional<uint64_t> ParseDeduplicationKey(const base::Value::Dict& dict);
+// Returns false if `dict` contains `deduplication_key` key but the value is
+// invalid, returns true otherwise.
+[[nodiscard]] bool ParseDeduplicationKey(const base::Value::Dict& dict,
+                                         absl::optional<uint64_t>& out);
 
 void SerializeUint64(base::Value::Dict&, base::StringPiece key, uint64_t value);
 

diff --git a/components/attribution_reporting/parsing_utils_unittest.cc b/components/attribution_reporting/parsing_utils_unittest.cc
@@ -47,89 +47,109 @@ TEST(AttributionReportingParsingUtilsTest, ParseUint64) {
   const struct {
     const char* description;
     const char* json;
-    absl::optional<uint64_t> expected;
+    absl::optional<uint64_t> expected_out;
+    bool expected_return;
   } kTestCases[] = {
       {
           "missing_key",
           R"json({})json",
           absl::nullopt,
+          true,
       },
       {
           "not_string",
           R"json({"key":123})json",
           absl::nullopt,
+          false,
       },
       {
           "negative",
           R"json({"key":"-1"})json",
           absl::nullopt,
+          false,
       },
       {
           "zero",
           R"json({"key":"0"})json",
           0,
+          true,
       },
       {
           "max",
           R"json({"key":"18446744073709551615"})json",
           std::numeric_limits<uint64_t>::max(),
+          true,
       },
       {
           "out_of_range",
           R"json({"key":"18446744073709551616"})json",
           absl::nullopt,
+          false,
       },
   };
 
   for (const auto& test_case : kTestCases) {
     base::Value value = base::test::ParseJson(test_case.json);
-    EXPECT_EQ(ParseUint64(value.GetDict(), "key"), test_case.expected)
+    absl::optional<uint64_t> out;
+    EXPECT_EQ(ParseUint64(value.GetDict(), "key", out),
+              test_case.expected_return)
         << test_case.description;
+    EXPECT_EQ(out, test_case.expected_out) << test_case.description;
   }
 }
 
 TEST(AttributionReportingParsingUtilsTest, ParseInt64) {
   const struct {
     const char* description;
     const char* json;
-    absl::optional<int64_t> expected;
+    absl::optional<int64_t> expected_out;
+    bool expected_return;
   } kTestCases[] = {
       {
           "missing_key",
           R"json({})json",
           absl::nullopt,
+          true,
       },
       {
           "not_string",
           R"json({"key":123})json",
           absl::nullopt,
+          false,
       },
       {
           "zero",
           R"json({"key":"0"})json",
           0,
+          true,
       },
       {
           "min",
           R"json({"key":"-9223372036854775808"})json",
           std::numeric_limits<int64_t>::min(),
+          true,
       },
       {
           "max",
           R"json({"key":"9223372036854775807"})json",
           std::numeric_limits<int64_t>::max(),
+          true,
       },
       {
           "out_of_range",
           R"json({"key":"9223372036854775808"})json",
           absl::nullopt,
+          false,
       },
   };
 
   for (const auto& test_case : kTestCases) {
     base::Value value = base::test::ParseJson(test_case.json);
-    EXPECT_EQ(ParseInt64(value.GetDict(), "key"), test_case.expected)
+    absl::optional<int64_t> out;
+    EXPECT_EQ(ParseInt64(value.GetDict(), "key", out),
+              test_case.expected_return)
         << test_case.description;
+    EXPECT_EQ(out, test_case.expected_out) << test_case.description;
   }
 }