Reland "Make PUSH_PROMISE headers URL-parsing more robust and conform…

… to HTTP/2 spec"

Reland of https://chromium-review.googlesource.com/c/chromium/src/+/887970.

MSAN use-of-uninitialized-value should be fixed.

Change-Id: I392ad1a83a269b47ff0c8583f6c72e18a4a0e5b9
Reviewed-on: https://chromium-review.googlesource.com/891302
Commit-Queue: Yixin Wang <wangyix@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532669}

net/BUILD.gn

@@ -5163,6 +5163,7 @@ test("net_unittests") {
     "quic/platform/api/quic_test_random.h",
     "quic/platform/api/quic_text_utils_test.cc",
     "quic/platform/api/quic_url_test.cc",
+    "quic/platform/api/quic_url_utils_test.cc",
     "quic/platform/impl/quic_chromium_clock_test.cc",
     "quic/platform/impl/quic_test_random_impl.cc",
     "quic/platform/impl/quic_test_random_impl.h",

net/quic/chromium/quic_http_stream_test.cc

@@ -339,7 +339,7 @@ class QuicHttpStreamTest
     promised_response_[":version"] = "HTTP/1.1";
     promised_response_["content-type"] = "text/plain";
 
-    promise_url_ = SpdyUtils::GetUrlFromHeaderBlock(push_promise_);
+    promise_url_ = SpdyUtils::GetPromisedUrlFromHeaderBlock(push_promise_);
   }
 
   void SetRequest(const string& method,
@@ -1873,7 +1873,7 @@ TEST_P(QuicHttpStreamTest, ServerPushCrossOriginOK) {
   // packet, but does it matter?
 
   push_promise_[":authority"] = "mail.example.org";
-  promise_url_ = SpdyUtils::GetUrlFromHeaderBlock(push_promise_);
+  promise_url_ = SpdyUtils::GetPromisedUrlFromHeaderBlock(push_promise_);
 
   ReceivePromise(promise_id_);
   EXPECT_NE(session_->GetPromisedByUrl(promise_url_), nullptr);
@@ -1942,7 +1942,7 @@ TEST_P(QuicHttpStreamTest, ServerPushCrossOriginFail) {
   // TODO(ckrasic) - could do this via constructing a PUSH_PROMISE
   // packet, but does it matter?
   push_promise_[":authority"] = "www.notexample.org";
-  promise_url_ = SpdyUtils::GetUrlFromHeaderBlock(push_promise_);
+  promise_url_ = SpdyUtils::GetPromisedUrlFromHeaderBlock(push_promise_);
 
   ReceivePromise(promise_id_);
   // The promise will have been rejected because the cert doesn't

net/quic/core/quic_client_promised_info_test.cc

@@ -84,7 +84,7 @@ class QuicClientPromisedInfoTest : public QuicTest {
     push_promise_[":method"] = "GET";
     push_promise_[":scheme"] = "https";
 
-    promise_url_ = SpdyUtils::GetUrlFromHeaderBlock(push_promise_);
+    promise_url_ = SpdyUtils::GetPromisedUrlFromHeaderBlock(push_promise_);
 
     client_request_ = push_promise_.Clone();
     promise_id_ =

net/quic/core/quic_client_push_promise_index.cc

@@ -33,7 +33,7 @@ QuicAsyncStatus QuicClientPushPromiseIndex::Try(
     const SpdyHeaderBlock& request,
     QuicClientPushPromiseIndex::Delegate* delegate,
     TryHandle** handle) {
-  string url(SpdyUtils::GetUrlFromHeaderBlock(request));
+  string url(SpdyUtils::GetPromisedUrlFromHeaderBlock(request));
   QuicPromisedByUrlMap::iterator it = promised_by_url_.find(url);
   if (it != promised_by_url_.end()) {
     QuicClientPromisedInfo* promised = it->second;

net/quic/core/quic_client_push_promise_index_test.cc

@@ -63,7 +63,7 @@ class QuicClientPushPromiseIndexTest : public QuicTest {
     request_[":version"] = "HTTP/1.1";
     request_[":method"] = "GET";
     request_[":scheme"] = "https";
-    url_ = SpdyUtils::GetUrlFromHeaderBlock(request_);
+    url_ = SpdyUtils::GetPromisedUrlFromHeaderBlock(request_);
   }
 
   MockQuicConnectionHelper helper_;

net/quic/core/quic_spdy_client_session_base.cc

@@ -99,7 +99,7 @@ bool QuicSpdyClientSessionBase::HandlePromised(QuicStreamId /* associated_id */,
     return false;
   }
 
-  const string url = SpdyUtils::GetUrlFromHeaderBlock(headers);
+  const string url = SpdyUtils::GetPromisedUrlFromHeaderBlock(headers);
   QuicClientPromisedInfo* old_promised = GetPromisedByUrl(url);
   if (old_promised) {
     QUIC_DVLOG(1) << "Promise for stream " << promised_id

net/quic/core/spdy_utils.cc

@@ -128,39 +128,69 @@ bool SpdyUtils::CopyAndValidateTrailers(const QuicHeaderList& header_list,
 }
 
 // static
-string SpdyUtils::GetUrlFromHeaderBlock(const SpdyHeaderBlock& headers) {
-  SpdyHeaderBlock::const_iterator it = headers.find(":scheme");
-  if (it == headers.end()) {
-    return "";
+string SpdyUtils::GetPromisedUrlFromHeaderBlock(
+    const SpdyHeaderBlock& headers) {
+  // RFC 7540, Section 8.1.2.3: All HTTP/2 requests MUST include exactly
+  // one valid value for the ":method", ":scheme", and ":path" pseudo-header
+  // fields, unless it is a CONNECT request.
+
+  // RFC 7540, Section  8.2.1:  The header fields in PUSH_PROMISE and any
+  // subsequent CONTINUATION frames MUST be a valid and complete set of request
+  // header fields (Section 8.1.2.3).  The server MUST include a method in the
+  // ":method" pseudo-header field that is safe and cacheable.
+  //
+  // RFC 7231, Section  4.2.1: Of the request methods defined by this
+  // specification, the GET, HEAD, OPTIONS, and TRACE methods are defined to be
+  // safe.
+  //
+  // RFC 7231, Section  4.2.1: ... this specification defines GET, HEAD, and
+  // POST as cacheable, ...
+  //
+  // So the only methods allowed in a PUSH_PROMISE are GET and HEAD.
+  SpdyHeaderBlock::const_iterator it = headers.find(":method");
+  if (it == headers.end() || (it->second != "GET" && it->second != "HEAD")) {
+    return string();
   }
-  std::string url = it->second.as_string();
 
-  url.append("://");
+  it = headers.find(":scheme");
+  if (it == headers.end() || it->second.empty()) {
+    return string();
+  }
+  QuicStringPiece scheme = it->second;
 
+  // RFC 7540, Section 8.2: The server MUST include a value in the
+  // ":authority" pseudo-header field for which the server is authoritative
+  // (see Section 10.1).
   it = headers.find(":authority");
-  if (it == headers.end()) {
-    return "";
+  if (it == headers.end() || it->second.empty()) {
+    return string();
   }
-  url.append(it->second.as_string());
+  QuicStringPiece authority = it->second;
 
+  // RFC 7540, Section 8.1.2.3 requires that the ":path" pseudo-header MUST
+  // NOT be empty for "http" or "https" URIs;
+  //
+  // However, to ensure the scheme is consistently canonicalized, that check
+  // is deferred to implementations in QuicUrlUtils::GetPushPromiseUrl().
   it = headers.find(":path");
   if (it == headers.end()) {
-    return "";
+    return string();
   }
-  url.append(it->second.as_string());
-  return url;
+  QuicStringPiece path = it->second;
+
+  return QuicUrlUtils::GetPushPromiseUrl(scheme, authority, path);
 }
 
 // static
 string SpdyUtils::GetHostNameFromHeaderBlock(const SpdyHeaderBlock& headers) {
   // TODO(fayang): Consider just checking out the value of the ":authority" key
   // in headers.
-  return QuicUrlUtils::HostName(GetUrlFromHeaderBlock(headers));
+  return QuicUrlUtils::HostName(GetPromisedUrlFromHeaderBlock(headers));
 }
 
 // static
 bool SpdyUtils::UrlIsValid(const SpdyHeaderBlock& headers) {
-  string url(GetUrlFromHeaderBlock(headers));
+  string url(GetPromisedUrlFromHeaderBlock(headers));
   return !url.empty() && QuicUrlUtils::IsValidUrl(url);
 }
 

net/quic/core/spdy_utils.h

@@ -35,14 +35,17 @@ class QUIC_EXPORT_PRIVATE SpdyUtils {
                                       size_t* final_byte_offset,
                                       SpdyHeaderBlock* trailers);
 
-  // Returns URL composed from scheme, authority, and path header
-  // values, or empty string if any of those fields are missing.
-  static std::string GetUrlFromHeaderBlock(const SpdyHeaderBlock& headers);
+  // Returns a canonicalized URL composed from the :scheme, :authority, and
+  // :path headers of a PUSH_PROMISE. Returns empty string if the headers do not
+  // conform to HTTP/2 spec or if the ":method" header contains a forbidden
+  // method for PUSH_PROMISE.
+  static std::string GetPromisedUrlFromHeaderBlock(
+      const SpdyHeaderBlock& headers);
 
   // Returns hostname, or empty string if missing.
   static std::string GetHostNameFromHeaderBlock(const SpdyHeaderBlock& headers);
 
-  // Returns true if result of |GetUrlFromHeaderBlock()| is non-empty
+  // Returns true if result of |GetPromisedUrlFromHeaderBlock()| is non-empty
   // and is a well-formed URL.
   static bool UrlIsValid(const SpdyHeaderBlock& headers);
 

net/quic/core/spdy_utils_test.cc

@@ -300,28 +300,136 @@ TEST_F(CopyAndValidateTrailers, DuplicateCookies) {
           Pair("key", "value")));
 }
 
-using GetUrlFromHeaderBlock = QuicTest;
+using GetPromisedUrlFromHeaderBlock = QuicTest;
 
-TEST_F(GetUrlFromHeaderBlock, Basic) {
+TEST_F(GetPromisedUrlFromHeaderBlock, Basic) {
   SpdyHeaderBlock headers;
-  EXPECT_EQ(SpdyUtils::GetUrlFromHeaderBlock(headers), "");
+  headers[":method"] = "GET";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
   headers[":scheme"] = "https";
-  EXPECT_EQ(SpdyUtils::GetUrlFromHeaderBlock(headers), "");
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
   headers[":authority"] = "www.google.com";
-  EXPECT_EQ(SpdyUtils::GetUrlFromHeaderBlock(headers), "");
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
   headers[":path"] = "/index.html";
-  EXPECT_EQ(SpdyUtils::GetUrlFromHeaderBlock(headers),
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers),
             "https://www.google.com/index.html");
   headers["key1"] = "value1";
   headers["key2"] = "value2";
-  EXPECT_EQ(SpdyUtils::GetUrlFromHeaderBlock(headers),
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers),
             "https://www.google.com/index.html");
 }
 
+TEST_F(GetPromisedUrlFromHeaderBlock, Connect) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "CONNECT";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+  headers[":authority"] = "www.google.com";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+  headers[":scheme"] = "https";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+  headers[":path"] = "https";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, UnsupportedFileScheme) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "file";
+  headers[":authority"] = "localhost";
+  headers[":path"] = "/etc/password";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+  headers[":authority"] = "";
+  headers[":path"] = "/C:/Windows/System32/Config/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, EmptySchemeAndInvalidAuthority) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "";
+  headers[":authority"] = "https://www.google.com";
+  headers[":path"] = "/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, SchemeWithAuthority) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https://www.google.com";
+  headers[":authority"] = "www.google.com";
+  headers[":path"] = "/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, InvalidScheme) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https://";
+  headers[":authority"] = "www.google.com";
+  headers[":path"] = "/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, EmptyAuthority) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https";
+  headers[":authority"] = "";
+  headers[":path"] = "/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, EmptyAuthorityAndInvalidPath) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https";
+  headers[":authority"] = "";
+  headers[":path"] = "www.google.com/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, AuthorityWithPath) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https";
+  headers[":authority"] = "www.google.com/";
+  headers[":path"] = "/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, EmptyPath) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https";
+  headers[":authority"] = "www.google.com";
+  headers[":path"] = "";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, InvalidPath) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "https";
+  headers[":authority"] = "www.google";
+  headers[":path"] = ".com/";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers), "");
+}
+
+TEST_F(GetPromisedUrlFromHeaderBlock, Canonicalize) {
+  SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
+  headers[":scheme"] = "hTtPs";
+  headers[":authority"] = "Www.gOo-Gle.Com:000003278";
+  headers[":path"] = "/pAth/To/reSOurce";
+  EXPECT_EQ(SpdyUtils::GetPromisedUrlFromHeaderBlock(headers),
+            "https://www.goo-gle.com:3278/pAth/To/reSOurce");
+}
+
 using GetHostNameFromHeaderBlock = QuicTest;
 
 TEST_F(GetHostNameFromHeaderBlock, NormalUsage) {
   SpdyHeaderBlock headers;
+  headers[":method"] = "GET";
   EXPECT_EQ(SpdyUtils::GetHostNameFromHeaderBlock(headers), "");
   headers[":scheme"] = "https";
   EXPECT_EQ(SpdyUtils::GetHostNameFromHeaderBlock(headers), "");

net/quic/platform/api/quic_url_utils.cc

@@ -18,4 +18,11 @@ bool QuicUrlUtils::IsValidUrl(QuicStringPiece url) {
   return QuicUrlUtilsImpl::IsValidUrl(url);
 }
 
+// static
+string QuicUrlUtils::GetPushPromiseUrl(QuicStringPiece scheme,
+                                       QuicStringPiece authority,
+                                       QuicStringPiece path) {
+  return QuicUrlUtilsImpl::GetPushPromiseUrl(scheme, authority, path);
+}
+
 }  // namespace net

net/quic/platform/api/quic_url_utils.h

@@ -22,6 +22,13 @@ class QUIC_EXPORT_PRIVATE QuicUrlUtils {
   // (e.g. greater than 65535).
   static bool IsValidUrl(QuicStringPiece url);
 
+  // Returns a canonical, valid URL for a PUSH_PROMISE with the specified
+  // ":scheme", ":authority", and ":path" header fields, or an empty
+  // string if the resulting URL is not valid or supported.
+  static std::string GetPushPromiseUrl(QuicStringPiece scheme,
+                                       QuicStringPiece authority,
+                                       QuicStringPiece path);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(QuicUrlUtils);
 };