blink: Make SecurityOrigin's tuple constructor directly populate members

Right now, blink::SecurityOrigin's
  Create(scheme, host, port)
method smushes (that's the technical term, right?) the arguments
together with some URL glue substrings like "://", feeds the result into
KURL, and feeds the resulting KURL into the SecurityOrigin(KURL)
constructor.

Even though this mostly works, it's kind of complex, and it's different
than how the corresponding url::Origin(url::SchemeHostPort) constructor
works. The difference is that url::Origin(url::SchemeHostPort) imposes a
precondition that its input be valid, so that the resulting origin is
never opaque. This lets url::Origin(url::SchemeHostPort) just take the
given scheme, host, and port and store them in Origin's fields.

The immediate motivation for this change is that I'm working on a
concurrent CL that will fix up SecurityOrigin's handling of URLs with
port 0 (crbug.com/1136678). There are a bunch of parameterized
tests that take input structs containing a scheme, a host, and a port,
and they need to support arguments like
  {"file", "", 0}
which should yield the non-opaque tuple origin ("file", "", 0) with
serialization "file://", as well as arguments like
  {"http", "davids.website", 0}
which should yield the non-opaque tuple origin ("http",
"davids.website", 0).

In the latter case, the current implementation omits the 0 from the
string it passes to the KURL constructor, and gives back
  SecurityOrigin(KURL("https://davids.website")),
which means there's no way to construct these test cases for the URL
"https://davids.website:0", in a manner yielding an origin
distinguishable from one constructed with the default port.

Instead of the implemented fix, I considered just making the method
unconditionally add the port to the string it passes to the KURL
constructor. But this doesn't work for the {file, "", 0} case, because
"file://:0" is an invalid URL.

Bug: 1136678
Change-Id: Ifa5deadfd3a5a79fc6e00a2af1291c2063434203
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2464363Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819941}

third_party/blink/renderer/core/frame/csp/source_list_directive_test.cc

@@ -108,7 +108,7 @@ TEST_F(SourceListDirectiveTest, StarallowsSelf) {
 
   // With a protocol of 'file', '*' allows 'file:':
   scoped_refptr<const SecurityOrigin> origin =
-      SecurityOrigin::Create("file", "", 0);
+      SecurityOrigin::CreateFromValidTuple("file", "", 0);
   csp->SetupSelf(*origin);
   EXPECT_TRUE(source_list.Allows(KURL(base, "file:///etc/hosts")));
 

third_party/blink/renderer/platform/mojo/kurl_security_origin_test.cc

@@ -76,7 +76,7 @@ TEST(KURLSecurityOriginStructTraitsTest, Basic) {
 
   // Test basic Origin serialization.
   scoped_refptr<const SecurityOrigin> non_unique =
-      SecurityOrigin::Create("http", "www.google.com", 80);
+      SecurityOrigin::CreateFromValidTuple("http", "www.google.com", 80);
   scoped_refptr<const SecurityOrigin> output;
   EXPECT_TRUE(proxy->BounceOrigin(non_unique, &output));
   EXPECT_TRUE(non_unique->IsSameOriginWith(output.get()));

third_party/blink/renderer/platform/mojo/security_origin_mojom_traits.h

@@ -25,13 +25,9 @@ struct UrlOriginAdapter {
       const base::Optional<base::UnguessableToken>& nonce_if_opaque) {
     scoped_refptr<blink::SecurityOrigin> tuple_origin;
     if (tuple.IsValid()) {
-      // url::SchemeHostPort is percent encoded and SecurityOrigin is percent
-      // decoded.
-      String host = blink::DecodeURLEscapeSequences(
-          String::FromUTF8(tuple.host()),
-          url::DecodeURLMode::kUTF8OrIsomorphic);
-      tuple_origin = blink::SecurityOrigin::Create(
-          String::FromUTF8(tuple.scheme()), host, tuple.port());
+      tuple_origin = blink::SecurityOrigin::CreateFromValidTuple(
+          String::FromUTF8(tuple.scheme()), String::FromUTF8(tuple.host()),
+          tuple.port());
     }
 
     if (nonce_if_opaque) {

third_party/blink/renderer/platform/weborigin/security_origin.cc

@@ -144,6 +144,21 @@ SecurityOrigin::SecurityOrigin(const KURL& url)
   can_load_local_resources_ = IsLocal();
 }
 
+SecurityOrigin::SecurityOrigin(const String& protocol,
+                               const String& host,
+                               uint16_t port)
+    : protocol_(protocol),
+      host_(host),
+      domain_(host_),
+      port_(IsDefaultPortForProtocol(port, protocol_) ? kInvalidPort : port),
+      effective_port_(port_ ? port_ : DefaultPortForProtocol(protocol_)) {
+  DCHECK(url::SchemeHostPort(protocol.Utf8(), host.Utf8(), port,
+                             url::SchemeHostPort::CHECK_CANONICALIZATION)
+             .IsValid());
+  DCHECK(!IsOpaque());
+  can_load_local_resources_ = IsLocal();
+}
+
 SecurityOrigin::SecurityOrigin(const url::Origin::Nonce& nonce,
                                const SecurityOrigin* precursor)
     : nonce_if_opaque_(nonce), precursor_origin_(precursor) {}
@@ -238,14 +253,9 @@ scoped_refptr<SecurityOrigin> SecurityOrigin::CreateFromUrlOrigin(
 
   scoped_refptr<SecurityOrigin> tuple_origin;
   if (tuple.IsValid()) {
-    String scheme = String::FromUTF8(tuple.scheme());
-    String host = String::FromUTF8(tuple.host());
-    uint16_t port = tuple.port();
-
-    // url::Origin is percent encoded and SecurityOrigin is percent decoded.
-    host = DecodeURLEscapeSequences(host, DecodeURLMode::kUTF8OrIsomorphic);
-
-    tuple_origin = Create(scheme, host, port);
+    tuple_origin =
+        CreateFromValidTuple(String::FromUTF8(tuple.scheme()),
+                             String::FromUTF8(tuple.host()), tuple.port());
   }
   base::Optional<base::UnguessableToken> nonce_if_opaque =
       origin.GetNonceForSerialization();
@@ -536,14 +546,11 @@ scoped_refptr<SecurityOrigin> SecurityOrigin::CreateFromString(
   return SecurityOrigin::Create(KURL(NullURL(), origin_string));
 }
 
-scoped_refptr<SecurityOrigin> SecurityOrigin::Create(const String& protocol,
-                                                     const String& host,
-                                                     uint16_t port) {
-  DCHECK_EQ(host,
-            DecodeURLEscapeSequences(host, DecodeURLMode::kUTF8OrIsomorphic));
-
-  String port_part = port ? ":" + String::Number(port) : String();
-  return Create(KURL(NullURL(), protocol + "://" + host + port_part + "/"));
+scoped_refptr<SecurityOrigin> SecurityOrigin::CreateFromValidTuple(
+    const String& protocol,
+    const String& host,
+    uint16_t port) {
+  return base::AdoptRef(new SecurityOrigin(protocol, host, port));
 }
 
 bool SecurityOrigin::IsSameOriginWith(const SecurityOrigin* other) const {

third_party/blink/renderer/platform/weborigin/security_origin.h

@@ -88,9 +88,17 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   static scoped_refptr<SecurityOrigin> CreateUniqueOpaque();
 
   static scoped_refptr<SecurityOrigin> CreateFromString(const String&);
-  static scoped_refptr<SecurityOrigin> Create(const String& protocol,
-                                              const String& host,
-                                              uint16_t port);
+
+  // Constructs a non-opaque tuple origin, analogously to
+  // url::Origin::Origin(url::SchemeHostPort).
+  //
+  // REQUIRES: The tuple be valid: |protocol| must contain a standard scheme and
+  // |host| must be canonicalized and (except for "file" URLs) nonempty.
+  static scoped_refptr<SecurityOrigin> CreateFromValidTuple(
+      const String& protocol,
+      const String& host,
+      uint16_t port);
+
   static scoped_refptr<SecurityOrigin> CreateFromUrlOrigin(const url::Origin&);
   url::Origin ToUrlOrigin() const;
 
@@ -385,6 +393,10 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   // Create a tuple SecurityOrigin, with parameters via KURL
   explicit SecurityOrigin(const KURL& url);
 
+  // Constructs a non-opaque tuple origin, analogously to
+  // url::Origin::Origin(url::SchemeHostPort).
+  SecurityOrigin(const String& protocol, const String& host, uint16_t port);
+
   enum class ConstructIsolatedCopy { kConstructIsolatedCopyBit };
   // Clone a SecurityOrigin which is safe to use on other threads.
   SecurityOrigin(const SecurityOrigin* other, ConstructIsolatedCopy);

third_party/blink/renderer/platform/weborigin/security_origin_test.cc

@@ -63,7 +63,7 @@ TEST_F(SecurityOriginTest, ValidPortsCreateTupleOrigins) {
 
   for (size_t i = 0; i < base::size(ports); ++i) {
     scoped_refptr<const SecurityOrigin> origin =
-        SecurityOrigin::Create("http", "example.com", ports[i]);
+        SecurityOrigin::CreateFromValidTuple("http", "example.com", ports[i]);
     EXPECT_FALSE(origin->IsOpaque())
         << "Port " << ports[i] << " should have generated a tuple origin.";
   }
@@ -584,7 +584,7 @@ TEST_F(SecurityOriginTest, CreateFromTuple) {
 
   for (const auto& test : cases) {
     scoped_refptr<const SecurityOrigin> origin =
-        SecurityOrigin::Create(test.scheme, test.host, test.port);
+        SecurityOrigin::CreateFromValidTuple(test.scheme, test.host, test.port);
     EXPECT_EQ(test.origin, origin->ToString()) << test.origin;
   }
 }
@@ -752,8 +752,8 @@ TEST_F(SecurityOriginTest, UrlOriginConversions) {
 
     if (!test_case.opaque) {
       scoped_refptr<const SecurityOrigin> security_origin =
-          SecurityOrigin::Create(test_case.scheme, test_case.host,
-                                 test_case.port);
+          SecurityOrigin::CreateFromValidTuple(test_case.scheme, test_case.host,
+                                               test_case.port);
       EXPECT_TRUE(
           security_origin->IsSameOriginWith(security_origin_via_gurl.get()));
       EXPECT_TRUE(
@@ -1169,4 +1169,16 @@ TEST_F(SecurityOriginTest, IsSameOriginDomainWithWithLocalScheme) {
   EXPECT_FALSE(b->IsSameOriginDomainWith(a.get()));
 }
 
+// Non-canonical hosts provided to the string constructor should end up
+// canonicalized:
+TEST_F(SecurityOriginTest, PercentEncodesHost) {
+  EXPECT_EQ(
+      SecurityOrigin::CreateFromString("http://foo,.example.test/")->Host(),
+      "foo%2C.example.test");
+
+  EXPECT_EQ(
+      SecurityOrigin::CreateFromString("http://foo%2C.example.test/")->Host(),
+      "foo%2C.example.test");
+}
+
 }  // namespace blink