Move origin canonicalization to FirstPartySetParser.

This ensures that we perform the same validation and canonicalization,
no matter how the First-Party Set was provided.

Change-Id: Ifdd76cb2d29a2a28215051a2b5b2ad6f312f95aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2510233Reviewed-by: Eric Orth <ericorth@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Chris Fredrickson <cfredric@google.com>
Auto-Submit: Chris Fredrickson <cfredric@google.com>
Cr-Commit-Position: refs/heads/master@{#823617}

services/network/first_party_sets/first_party_set_parser.cc

@@ -6,15 +6,56 @@
 
 #include <iterator>
 #include <memory>
+#include <string>
+#include <utility>
 
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/json/json_reader.h"
+#include "base/logging.h"
+#include "base/optional.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "url/gurl.h"
+#include "url/origin.h"
 
 namespace network {
 
 namespace {
 
+// Ensures that the string represents an origin that is non-opaque and HTTPS.
+// Returns the registered domain.
+base::Optional<std::string> Canonicalize(const base::StringPiece origin_string,
+                                         bool emit_errors) {
+  const url::Origin origin(url::Origin::Create(GURL(origin_string)));
+  if (origin.opaque()) {
+    if (emit_errors) {
+      LOG(ERROR) << "First-Party Set origin " << origin_string
+                 << " is not valid; ignoring.";
+    }
+    return base::nullopt;
+  }
+  if (origin.scheme() != "https") {
+    if (emit_errors) {
+      LOG(ERROR) << "First-Party Set origin " << origin_string
+                 << " is not HTTPS; ignoring.";
+    }
+    return base::nullopt;
+  }
+  const std::string domain_and_registry =
+      net::registry_controlled_domains::GetDomainAndRegistry(
+          origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+
+  if (domain_and_registry.empty()) {
+    if (emit_errors) {
+      LOG(ERROR) << "First-Party Set origin" << origin_string
+                 << " does not have a valid registered domain; ignoring.";
+    }
+    return base::nullopt;
+  }
+
+  return domain_and_registry;
+}
+
 const char kFirstPartySetOwnerField[] = "owner";
 const char kFirstPartySetMembersField[] = "members";
 
@@ -37,11 +78,16 @@ void ParsePreloadedSet(
   if (!maybe_owner)
     return;
 
+  base::Optional<std::string> canonical_owner =
+      Canonicalize(std::move(*maybe_owner), false /* emit_errors */);
+  if (!canonical_owner.has_value())
+    return;
+
   // An owner may only be listed once, and may not be a member of another set.
-  if (members.contains(*maybe_owner) || owners.contains(*maybe_owner))
+  if (members.contains(*canonical_owner) || owners.contains(*canonical_owner))
     return;
 
-  owners.insert(*maybe_owner);
+  owners.insert(*canonical_owner);
 
   // Confirm that the members field is present, and is an array of strings.
   const base::Value* maybe_members_list =
@@ -54,17 +100,26 @@ void ParsePreloadedSet(
     // Members may not be a member of another set, and may not be an owner of
     // another set.
     if (item.is_string()) {
-      std::string member = item.GetString();
-      if (!members.contains(member) && !owners.contains(member)) {
-        map_storage.emplace_back(member, *maybe_owner);
-        members.insert(member);
+      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));
     }
   }
 }
 
 }  // namespace
 
+base::Optional<std::string> FirstPartySetParser::CanonicalizeRegisteredDomain(
+    const base::StringPiece origin_string,
+    bool emit_errors) {
+  return Canonicalize(origin_string, emit_errors);
+}
+
 std::unique_ptr<base::flat_map<std::string, std::string>>
 FirstPartySetParser::ParsePreloadedSets(base::StringPiece raw_sets) {
   base::Optional<base::Value> maybe_value = base::JSONReader::Read(

services/network/first_party_sets/first_party_set_parser.h

@@ -7,9 +7,12 @@
 
 #include <map>
 #include <memory>
+#include <string>
 
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
+#include "base/optional.h"
+#include "base/strings/string_piece_forward.h"
 #include "base/values.h"
 
 namespace network {
@@ -29,6 +32,13 @@ class FirstPartySetParser {
   // only for *preloaded* sets.
   static std::unique_ptr<base::flat_map<std::string, std::string>>
   ParsePreloadedSets(base::StringPiece raw_sets);
+
+  // Canonicalizes the passed in origin to a registered domain. In particular,
+  // this ensures that the origin is non-opaque, is HTTPS, and has a registered
+  // domain. Returns base::nullopt in case of any error.
+  static base::Optional<std::string> CanonicalizeRegisteredDomain(
+      const base::StringPiece origin_string,
+      bool emit_errors);
 };
 
 }  // namespace network

services/network/first_party_sets/preloaded_first_party_sets.cc

@@ -6,45 +6,15 @@
 
 #include <memory>
 
-#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/optional.h"
 #include "base/ranges/algorithm.h"
 #include "base/strings/string_split.h"
-#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "services/network/first_party_sets/first_party_set_parser.h"
-#include "url/gurl.h"
-#include "url/origin.h"
 
-namespace {
-
-// Ensures that the string represents an origin that is non-opaque and HTTPS.
-// Returns the registered domain.
-base::Optional<std::string> CanonicalizeRegisteredDomain(
-    const std::string& origin_string) {
-  const url::Origin origin(url::Origin::Create(GURL(origin_string)));
-  if (origin.opaque()) {
-    LOG(ERROR) << "First-Party Set origin " << origin_string
-               << " is not valid; ignoring.";
-    return base::nullopt;
-  }
-  if (origin.scheme() != "https") {
-    LOG(ERROR) << "First-Party Set origin " << origin_string
-               << " is not HTTPS; ignoring.";
-    return base::nullopt;
-  }
-  const std::string domain_and_registry =
-      net::registry_controlled_domains::GetDomainAndRegistry(
-          origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
-
-  if (domain_and_registry.empty()) {
-    LOG(ERROR) << "First-Party Set origin" << origin_string
-               << " does not have a valid registered domain; ignoring.";
-    return base::nullopt;
-  }
+namespace network {
 
-  return domain_and_registry;
-}
+namespace {
 
 base::Optional<std::pair<std::string, base::flat_set<std::string>>>
 CanonicalizeSet(const std::vector<std::string>& origins) {
@@ -52,7 +22,8 @@ CanonicalizeSet(const std::vector<std::string>& origins) {
     return base::nullopt;
 
   const base::Optional<std::string> maybe_owner =
-      CanonicalizeRegisteredDomain(origins[0]);
+      FirstPartySetParser::CanonicalizeRegisteredDomain(origins[0],
+                                                        true /* emit_errors */);
   if (!maybe_owner.has_value()) {
     LOG(ERROR) << "First-Party Set owner is not valid; aborting.";
     return base::nullopt;
@@ -62,7 +33,8 @@ CanonicalizeSet(const std::vector<std::string>& origins) {
   base::flat_set<std::string> members;
   for (auto it = origins.begin() + 1; it != origins.end(); ++it) {
     const base::Optional<std::string> maybe_member =
-        CanonicalizeRegisteredDomain(*it);
+        FirstPartySetParser::CanonicalizeRegisteredDomain(
+            *it, true /* emit_errors */);
     if (maybe_member.has_value() && maybe_member != owner)
       members.emplace(std::move(*maybe_member));
   }
@@ -78,8 +50,6 @@ CanonicalizeSet(const std::vector<std::string>& origins) {
 
 }  // namespace
 
-namespace network {
-
 PreloadedFirstPartySets::PreloadedFirstPartySets() = default;
 
 PreloadedFirstPartySets::~PreloadedFirstPartySets() = default;

services/network/first_party_sets/preloaded_first_party_sets_unittest.cc

@@ -25,23 +25,26 @@ TEST(PreloadedFirstPartySets, ParsesJSON) {
 
 TEST(PreloadedFirstPartySets, AcceptsMinimal) {
   const std::string input =
-      R"( [ { "owner": "example.test", "members": ["aaaa"] } ] )";
+      R"([{
+        "owner": "https://example.test",
+        "members": ["https://aaaa.test"]
+        }])";
   ASSERT_TRUE(base::JSONReader::Read(input));
 
   EXPECT_THAT(PreloadedFirstPartySets().ParseAndSet(input),
-              Pointee(UnorderedElementsAre(Pair("aaaa", "example.test"))));
+              Pointee(UnorderedElementsAre(Pair("aaaa.test", "example.test"))));
 }
 
 TEST(PreloadedFirstPartySets, AcceptsMultipleSets) {
   const std::string input = R"(
   [
     {
-      "owner": "example.test",
-      "members": ["member1.test"]
+      "owner": "https://example.test",
+      "members": ["https://member1.test"]
     },
     {
-      "owner": "foo.test",
-      "members": ["member2.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member2.test"]
     }
   ]
   )";
@@ -56,12 +59,12 @@ TEST(PreloadedFirstPartySets, OwnerIsOnlyMember) {
   const std::string input = R"(
   [
     {
-      "owner": "example.test",
-      "members": ["example.test"]
+      "owner": "https://example.test",
+      "members": ["https://example.test"]
     },
     {
-      "owner": "foo.test",
-      "members": ["member2.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member2.test"]
     }
   ]
   )";
@@ -75,12 +78,12 @@ TEST(PreloadedFirstPartySets, OwnerIsMember) {
   const std::string input = R"(
   [
     {
-      "owner": "example.test",
-      "members": ["example.test", "member1.test"]
+      "owner": "https://example.test",
+      "members": ["https://example.test", "https://member1.test"]
     },
     {
-      "owner": "foo.test",
-      "members": ["member2.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member2.test"]
     }
   ]
   )";
@@ -95,12 +98,16 @@ TEST(PreloadedFirstPartySets, RepeatedMember) {
   const std::string input = R"(
   [
     {
-      "owner": "example.test",
-      "members": ["member1.test", "member2.test", "member1.test"]
+      "owner": "https://example.test",
+      "members": [
+        "https://member1.test",
+        "https://member2.test",
+        "https://member1.test"
+        ]
     },
     {
-      "owner": "foo.test",
-      "members": ["member3.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member3.test"]
     }
   ]
   )";
@@ -206,12 +213,12 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesOwnerOwner) {
   const std::string input = R"(
   [
     {
-      "owner": "example.test",
-      "members": ["member2.test", "member3.test"]
+      "owner": "https://example.test",
+      "members": ["https://member2.test", "https://member3.test"]
     },
     {
-      "owner": "bar.test",
-      "members": ["member4.test"]
+      "owner": "https://bar.test",
+      "members": ["https://member4.test"]
     }
   ]
   )";
@@ -230,12 +237,12 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesOwnerMember) {
   const std::string input = R"(
   [
     {
-      "owner": "foo.test",
-      "members": ["member1.test", "example.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member1.test", "https://example.test"]
     },
     {
-      "owner": "bar.test",
-      "members": ["member2.test"]
+      "owner": "https://bar.test",
+      "members": ["https://member2.test"]
     }
   ]
   )";
@@ -255,8 +262,8 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesMemberOwner) {
   const std::string input = R"(
   [
     {
-      "owner": "foo.test",
-      "members": ["member1.test", "member2.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member1.test", "https://member2.test"]
     },
     {
       "owner": "member3.test",
@@ -279,12 +286,12 @@ TEST(PreloadedFirstPartySets, SetsManuallySpecified_DeduplicatesMemberMember) {
   const std::string input = R"(
   [
     {
-      "owner": "foo.test",
-      "members": ["member2.test", "member3.test"]
+      "owner": "https://foo.test",
+      "members": ["https://member2.test", "https://member3.test"]
     },
     {
-      "owner": "bar.test",
-      "members": ["member4.test"]
+      "owner": "https://bar.test",
+      "members": ["https://member4.test"]
     }
   ]
   )";