Change FirstPartySetParser and PreloadedFirstPartySets failure modes to

be as obvious and early as possible.

This also makes some simplifications in the implementation, namely:
* Using a single set to track all FPS elements, rather than separate
sets for owners and members;
* Building the mapping directly, rather than building a vector to be
transformed into the map.

Change-Id: Ia5d45e8d728e85f7831e7930021fa9bfaec45240
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2517702Reviewed-by: Eric Orth <ericorth@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824143}

services/network/first_party_sets/first_party_set_parser.cc

@@ -62,54 +62,57 @@ const char kFirstPartySetMembersField[] = "members";
 // Parses a single First-Party Set into a map from member to owner (not
 // including the owner). Note that this is intended for use *only* on sets that
 // were preloaded via the component updater, so this does not check assertions
-// or versions. It does not handle non-disjoint sets (i.e. sets which have
-// non-empty intersections of owners and/or members)..
-void ParsePreloadedSet(
-    const base::Value& value,
-    std::vector<std::pair<std::string, std::string>>& map_storage,
-    base::flat_set<std::string>& owners,
-    base::flat_set<std::string>& members) {
+// or versions. It rejects sets which are non-disjoint with
+// previously-encountered sets (i.e. sets which have non-empty intersections
+// with `elements`).
+//
+// Uses `elements` to check disjointness of sets; builds the mapping in `map`;
+// and augments `elements` to include the elements of the set that was parsed.
+//
+// Returns true if parsing and validation were successful, false otherwise.
+bool ParsePreloadedSet(const base::Value& value,
+                       base::flat_map<std::string, std::string>& map,
+                       base::flat_set<std::string>& elements) {
   if (!value.is_dict())
-    return;
+    return false;
 
   // Confirm that the set has an owner, and the owner is a string.
   const std::string* maybe_owner =
       value.FindStringKey(kFirstPartySetOwnerField);
   if (!maybe_owner)
-    return;
+    return false;
 
   base::Optional<std::string> canonical_owner =
       Canonicalize(std::move(*maybe_owner), false /* emit_errors */);
   if (!canonical_owner.has_value())
-    return;
+    return false;
 
-  // An owner may only be listed once, and may not be a member of another set.
-  if (members.contains(*canonical_owner) || owners.contains(*canonical_owner))
-    return;
+  // An owner may not be a member of another set.
+  if (elements.contains(*canonical_owner))
+    return false;
 
-  owners.insert(*canonical_owner);
+  elements.insert(*canonical_owner);
 
   // Confirm that the members field is present, and is an array of strings.
   const base::Value* maybe_members_list =
       value.FindListKey(kFirstPartySetMembersField);
   if (!maybe_members_list)
-    return;
+    return false;
 
   // Add each member to our mapping (assuming the member is a string).
   for (const auto& item : maybe_members_list->GetList()) {
     // Members may not be a member of another set, and may not be an owner of
     // another set.
-    if (item.is_string()) {
-      base::Optional<std::string> member =
-          Canonicalize(item.GetString(), false /* emit_errors */);
-      if (!member.has_value() || members.contains(*member) ||
-          owners.contains(*member)) {
-        continue;
-      }
-      map_storage.emplace_back(*member, *canonical_owner);
-      members.insert(std::move(*member));
-    }
+    if (!item.is_string())
+      return false;
+    base::Optional<std::string> member =
+        Canonicalize(item.GetString(), false /* emit_errors */);
+    if (!member.has_value() || elements.contains(*member))
+      return false;
+    map.emplace(*member, *canonical_owner);
+    elements.insert(std::move(*member));
   }
+  return true;
 }
 
 }  // namespace
@@ -129,15 +132,14 @@ FirstPartySetParser::ParsePreloadedSets(base::StringPiece raw_sets) {
   if (!maybe_value->is_list())
     return nullptr;
 
-  std::vector<std::pair<std::string, std::string>> map_storage;
-  base::flat_set<std::string> owners;
-  base::flat_set<std::string> members;
+  auto map = std::make_unique<base::flat_map<std::string, std::string>>();
+  base::flat_set<std::string> elements;
   for (const auto& value : maybe_value->GetList()) {
-    ParsePreloadedSet(value, map_storage, owners, members);
+    if (!ParsePreloadedSet(value, *map, elements))
+      return nullptr;
   }
 
-  return std::make_unique<base::flat_map<std::string, std::string>>(
-      std::move(map_storage));
+  return map;
 }
 
 }  // namespace network

services/network/first_party_sets/first_party_set_parser.h

@@ -30,6 +30,8 @@ class FirstPartySetParser {
   // specified in this document: https://github.com/privacycg/first-party-sets.
   // This function does not check versions or assertions, since it is intended
   // only for *preloaded* sets.
+  //
+  // Returns nullptr if parsing or validation of any set failed.
   static std::unique_ptr<base::flat_map<std::string, std::string>>
   ParsePreloadedSets(base::StringPiece raw_sets);
 

services/network/first_party_sets/first_party_set_parser_unittest.cc

@@ -9,6 +9,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::IsEmpty;
+using ::testing::IsNull;
 using ::testing::Pair;
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
@@ -72,8 +73,7 @@ TEST(FirstPartySetParser, RejectsMissingOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsTypeUnsafeOwner) {
@@ -83,8 +83,7 @@ TEST(FirstPartySetParser, RejectsTypeUnsafeOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsNonHTTPSOwner) {
@@ -97,8 +96,7 @@ TEST(FirstPartySetParser, RejectsNonHTTPSOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsNonOriginOwner) {
@@ -111,8 +109,7 @@ TEST(FirstPartySetParser, RejectsNonOriginOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsOwnerWithoutRegisteredDomain) {
@@ -125,8 +122,7 @@ TEST(FirstPartySetParser, RejectsOwnerWithoutRegisteredDomain) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsMissingMembers) {
@@ -135,8 +131,7 @@ TEST(FirstPartySetParser, RejectsMissingMembers) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsTypeUnsafeMembers) {
@@ -149,8 +144,7 @@ TEST(FirstPartySetParser, RejectsTypeUnsafeMembers) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(UnorderedElementsAre(Pair("aaaa.test", "example.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsNonHTTPSMember) {
@@ -163,8 +157,7 @@ TEST(FirstPartySetParser, RejectsNonHTTPSMember) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsNonOriginMember) {
@@ -177,8 +170,7 @@ TEST(FirstPartySetParser, RejectsNonOriginMember) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, RejectsMemberWithoutRegisteredDomain) {
@@ -191,8 +183,7 @@ TEST(FirstPartySetParser, RejectsMemberWithoutRegisteredDomain) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(IsEmpty()));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, TruncatesSubdomain_Owner) {
@@ -245,7 +236,7 @@ TEST(FirstPartySetParser, AcceptsMultipleSets) {
                                            Pair("member2.test", "foo.test"))));
 }
 
-TEST(FirstPartySetParser, IgnoresInvalidSets_InvalidOwner) {
+TEST(FirstPartySetParser, RejectsInvalidSets_InvalidOwner) {
   const std::string input = R"(
   [
     {
@@ -262,11 +253,10 @@ TEST(FirstPartySetParser, IgnoresInvalidSets_InvalidOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(UnorderedElementsAre(Pair("member2.test", "foo.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
-TEST(FirstPartySetParser, IgnoresInvalidSets_InvalidMember) {
+TEST(FirstPartySetParser, RejectsInvalidSets_InvalidMember) {
   const std::string input = R"(
   [
     {
@@ -283,8 +273,7 @@ TEST(FirstPartySetParser, IgnoresInvalidSets_InvalidMember) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(UnorderedElementsAre(Pair("member2.test", "foo.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 TEST(FirstPartySetParser, AllowsTrailingCommas) {
@@ -306,7 +295,7 @@ TEST(FirstPartySetParser, AllowsTrailingCommas) {
       Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"))));
 }
 
-TEST(FirstPartySetParser, IgnoresSubsequent_SameOwner) {
+TEST(FirstPartySetParser, Rejects_SameOwner) {
   const std::string input = R"(
   [
     {
@@ -323,12 +312,10 @@ TEST(FirstPartySetParser, IgnoresSubsequent_SameOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(
-      FirstPartySetParser::ParsePreloadedSets(input),
-      Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
-TEST(FirstPartySetParser, IgnoresSubsequent_MemberAsOwner) {
+TEST(FirstPartySetParser, Rejects_MemberAsOwner) {
   const std::string input = R"(
   [
     {
@@ -345,12 +332,10 @@ TEST(FirstPartySetParser, IgnoresSubsequent_MemberAsOwner) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(
-      FirstPartySetParser::ParsePreloadedSets(input),
-      Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
-TEST(FirstPartySetParser, IgnoresSubsequent_SameMember) {
+TEST(FirstPartySetParser, Rejects_SameMember) {
   const std::string input = R"(
   [
     {
@@ -367,12 +352,10 @@ TEST(FirstPartySetParser, IgnoresSubsequent_SameMember) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input),
-              Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
-                                           Pair("member2.test", "foo.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
-TEST(FirstPartySetParser, IgnoresSubsequent_OwnerAsMember) {
+TEST(FirstPartySetParser, Rejects_OwnerAsMember) {
   const std::string input = R"(
   [
     {
@@ -389,10 +372,7 @@ TEST(FirstPartySetParser, IgnoresSubsequent_OwnerAsMember) {
   // Sanity check that the input is actually valid JSON.
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(
-      FirstPartySetParser::ParsePreloadedSets(input),
-      Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
-                                   Pair("member2.test", "example2.test"))));
+  EXPECT_THAT(FirstPartySetParser::ParsePreloadedSets(input), IsNull());
 }
 
 }  // namespace network

services/network/first_party_sets/preloaded_first_party_sets.cc

@@ -59,7 +59,7 @@ void PreloadedFirstPartySets::SetManuallySpecifiedSet(
   manually_specified_set_ = CanonicalizeSet(base::SplitString(
       flag_value, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY));
 
-  ApplyManuallySpecifiedSet(sets_);
+  ApplyManuallySpecifiedSet();
 }
 
 base::flat_map<std::string, std::string>* PreloadedFirstPartySets::ParseAndSet(
@@ -67,14 +67,17 @@ base::flat_map<std::string, std::string>* PreloadedFirstPartySets::ParseAndSet(
   std::unique_ptr<base::flat_map<std::string, std::string>> parsed =
       FirstPartySetParser::ParsePreloadedSets(raw_sets);
   if (parsed) {
-    ApplyManuallySpecifiedSet(*parsed);
     sets_.swap(*parsed);
+  } else {
+    // On any error, we clear the sets, to avoid using the old data and to make
+    // the failure as obvious as possible.
+    sets_.clear();
   }
+  ApplyManuallySpecifiedSet();
   return &sets_;
 }
 
-void PreloadedFirstPartySets::ApplyManuallySpecifiedSet(
-    base::flat_map<std::string, std::string>& sets) const {
+void PreloadedFirstPartySets::ApplyManuallySpecifiedSet() {
   if (!manually_specified_set_)
     return;
 
@@ -82,18 +85,19 @@ void PreloadedFirstPartySets::ApplyManuallySpecifiedSet(
   const base::flat_set<std::string>& manual_members =
       manually_specified_set_->second;
 
-  sets.erase(base::ranges::remove_if(
-                 sets,
-                 [&manual_members, &manual_owner](const auto& p) {
-                   return p.first == manual_owner || p.second == manual_owner ||
-                          manual_members.contains(p.first) ||
-                          manual_members.contains(p.second);
-                 }),
-             sets.end());
+  sets_.erase(
+      base::ranges::remove_if(sets_,
+                              [&manual_members, &manual_owner](const auto& p) {
+                                return p.first == manual_owner ||
+                                       p.second == manual_owner ||
+                                       manual_members.contains(p.first) ||
+                                       manual_members.contains(p.second);
+                              }),
+      sets_.end());
 
   // Next, we must add the manually-added set to the parsed value.
   for (const std::string& member : manual_members) {
-    sets.emplace(member, manual_owner);
+    sets_.emplace(member, manual_owner);
   }
 }
 

services/network/first_party_sets/preloaded_first_party_sets.h

@@ -27,11 +27,14 @@ class PreloadedFirstPartySets {
 
   void SetManuallySpecifiedSet(const std::string& flag_value);
 
-  // Overwrites the current owners-to-sets map with the values in |raw_sets|,
+  // Overwrites the current members-to-owners map with the values in |raw_sets|,
   // which should be the JSON-encoded string representation of a collection of
   // set declarations according to the format specified in this document:
   // https://github.com/privacycg/first-party-sets. Returns a pointer to the
-  // set, for testing.
+  // mapping, for testing.
+  //
+  // In case of invalid input, clears the current members-to-owners map, but
+  // keeps any manually-specified set (i.e. a set provided on the command line).
   base::flat_map<std::string, std::string>* ParseAndSet(
       base::StringPiece raw_sets);
 
@@ -40,11 +43,10 @@ class PreloadedFirstPartySets {
  private:
   // We must ensure there's no intersection between the manually-specified set
   // and the sets that came from Component Updater. (When reconciling the
-  // manually-specified set and `sets`, entries in the manually-specified set
-  // always win.) We must also ensure that `sets` includes the set described by
+  // manually-specified set and `sets_`, entries in the manually-specified set
+  // always win.) We must also ensure that `sets_` includes the set described by
   // `manually_specified_set_`.
-  void ApplyManuallySpecifiedSet(
-      base::flat_map<std::string, std::string>& sets) const;
+  void ApplyManuallySpecifiedSet();
 
   base::flat_map<std::string, std::string> sets_;
   base::Optional<std::pair<std::string, base::flat_set<std::string>>>

services/network/first_party_sets/preloaded_first_party_sets_unittest.cc

@@ -55,6 +55,29 @@ TEST(PreloadedFirstPartySets, AcceptsMultipleSets) {
                                            Pair("member2.test", "foo.test"))));
 }
 
+TEST(PreloadedFirstPartySets, ClearsPreloadedOnError) {
+  const std::string input = R"(
+  [
+    {
+      "owner": "https://example.test",
+      "members": ["https://member1.test"]
+    },
+    {
+      "owner": "https://foo.test",
+      "members": ["https://member2.test"]
+    }
+  ]
+  )";
+  ASSERT_TRUE(base::JSONReader::Read(input));
+
+  PreloadedFirstPartySets sets;
+  EXPECT_THAT(sets.ParseAndSet(input),
+              Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
+                                           Pair("member2.test", "foo.test"))));
+
+  EXPECT_THAT(sets.ParseAndSet("{}"), Pointee(IsEmpty()));
+}
+
 TEST(PreloadedFirstPartySets, OwnerIsOnlyMember) {
   const std::string input = R"(
   [
@@ -70,8 +93,7 @@ TEST(PreloadedFirstPartySets, OwnerIsOnlyMember) {
   )";
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input),
-              Pointee(UnorderedElementsAre(Pair("member2.test", "foo.test"))));
+  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input), Pointee(IsEmpty()));
 }
 
 TEST(PreloadedFirstPartySets, OwnerIsMember) {
@@ -89,9 +111,7 @@ TEST(PreloadedFirstPartySets, OwnerIsMember) {
   )";
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input),
-              Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
-                                           Pair("member2.test", "foo.test"))));
+  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input), Pointee(IsEmpty()));
 }
 
 TEST(PreloadedFirstPartySets, RepeatedMember) {
@@ -113,10 +133,7 @@ TEST(PreloadedFirstPartySets, RepeatedMember) {
   )";
   ASSERT_TRUE(base::JSONReader::Read(input));
 
-  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input),
-              Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
-                                           Pair("member2.test", "example.test"),
-                                           Pair("member3.test", "foo.test"))));
+  EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input), Pointee(IsEmpty()));
 }
 
 TEST(PreloadedFirstPartySets, SetsManuallySpecified_Invalid_TooSmall) {
@@ -266,8 +283,8 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesMemberOwner) {
       "members": ["https://member1.test", "https://member2.test"]
     },
     {
-      "owner": "member3.test",
-      "members": ["member4.test"]
+      "owner": "https://member3.test",
+      "members": ["https://member4.test"]
     }
   ]
   )";
@@ -307,4 +324,29 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesMemberMember) {
                                            Pair("member4.test", "bar.test"))));
 }
 
+TEST(PreloadedFirstPartySets, SetsManuallySpecified_ClearsPreloadedOnError) {
+  const std::string input = R"(
+  [
+    {
+      "owner": "https://bar.test",
+      "members": ["https://member3.test"]
+    }
+  ]
+  )";
+  ASSERT_TRUE(base::JSONReader::Read(input));
+
+  PreloadedFirstPartySets sets;
+  sets.SetManuallySpecifiedSet(
+      "https://example.test,https://member1.test,https://member2.test");
+  EXPECT_THAT(sets.ParseAndSet(input),
+              Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
+                                           Pair("member2.test", "example.test"),
+                                           Pair("member3.test", "bar.test"))));
+
+  EXPECT_THAT(
+      sets.ParseAndSet("{}"),
+      Pointee(UnorderedElementsAre(Pair("member1.test", "example.test"),
+                                   Pair("member2.test", "example.test"))));
+}
+
 }  // namespace network