HTTP/2 Check header names in HeaderCoalescer

net/spdy/header_coalescer_test.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "header_coalescer.h"
 
 #include <string>
 
 #include "base/strings/string_piece.h"

[Bence, 2017/02/23 23:33:04]

In header_coalescer.h, HeaderCoalescer::OnHeader() is declared and it takes
base::StringPiece arguments by value.  Please move this include there.

[xunjieli, 2017/02/24 16:33:32]

base::StringPiece is used on line 70. 

[Bence, 2017/02/24 16:51:30]

header_coalescer.h also uses base::StringPiece, and thus should include
string_piece.h.  It does not, this is a pre-existing bug.  While you are working
with these files, you might want to move this include from
header_coalescer_test.cc to header_coalescer.h where it belongs.  Or if you'd
prefer, I can do it in a separate two-line CL.

[xunjieli, 2017/02/24 17:36:55]

Bence, I think this test file should include string_piece.h even if
header_coalescer.h includes that file, because this test file used StringPiece.

I am not able to find this in the "include what you use" rule, but I believe we
should include it. Am I missing something?

[Bence, 2017/02/24 18:48:29]

Good question.  The style guide
(https://google.github.io/styleguide/cppguide.html) says:

"...dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
the stuff in dir2/foo2.h..."

"...any includes present in the related header do not need to be included again
in the related cc (i.e., foo.cc can rely on foo.h's includes)."

Based on this, I understand header_coalescer.h to be the related header to
header_coalescer_test.cc, and I do not include files in foo_test.cc that were
included in foo.h.  But I do not feel strongly about this, so it is okay with me
if you do not change the string_piece include here.

+#include "base/strings/stringprintf.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::ElementsAre;
 using ::testing::Pair;
 
 namespace net {
 namespace test {
 
 class HeaderCoalescerTest : public ::testing::Test {
@@ skipping 49 matching lines @@
               ElementsAre(Pair("foo", base::StringPiece("bar\0quux", 8)),
                           Pair("cookie", "baz; qux")));
 }
 
 TEST_F(HeaderCoalescerTest, CRLFInHeaderValue) {
   EXPECT_FALSE(header_coalescer_.error_seen());
   header_coalescer_.OnHeader("foo", "bar\r\nbaz");
   EXPECT_TRUE(header_coalescer_.error_seen());
 }
 
+TEST_F(HeaderCoalescerTest, HeaderNameNotValid) {
+  std::string header_name("\x01\x7F\x80\xff");
+  header_coalescer_.OnHeader(header_name, "foo");
+  EXPECT_TRUE(header_coalescer_.error_seen());
+}
+
+// RFC 7230 Section 3.2. Valid header name is defined as:
+// field-name     = token
+// where token is any visible US ASCII characters.

[asanka, 2017/02/23 23:18:54]

|token| is defined in RFC 7230 Appendix B as:

tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
    "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
   token = 1*tchar

Which is what the string below is based on. Visible US ASCII characters (VCHARS)
also include delimiters like " ( ) : etc..

[Bence, 2017/02/23 23:33:04]

Section 3.2.6 might be a more specific reference for the same definition (they
are repeated in Appendix B for convenience).  There is no need to repeat the
definition in the comment, |header_name| lists all allowed characters anyway.

[xunjieli, 2017/02/24 16:33:32]

Acknowledged.

[xunjieli, 2017/02/24 16:33:32]

Done.

+TEST_F(HeaderCoalescerTest, HeaderNameValid) {
+  std::string header_name(
+      "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!#$%&'*+-."
+      "^_`|~");
+  header_coalescer_.OnHeader(header_name, "foo");
+  EXPECT_FALSE(header_coalescer_.error_seen());
+  SpdyHeaderBlock header_block = header_coalescer_.release_headers();
+  EXPECT_THAT(header_block, ElementsAre(Pair(header_name, "foo")));
+}
+
+// RFC 7230 Section 3.2. Valid header value is defined as:
+// field-value    = *( field-content / obs-fold )
+// field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
+// field-vchar    = VCHAR / obs-text
+//
+// obs-fold       = CRLF 1*( SP / HTAB )
+//                ; obsolete line folding
+//                ; see Section 3.2.4
+TEST_F(HeaderCoalescerTest, HeaderValueValid) {
+  // Add two header, one with an HTAB and one with a SP.

[Bence, 2017/02/23 23:33:04]

s/header/headers/

[xunjieli, 2017/02/24 16:33:32]

Done.

+  std::string header_values[2];
+  char prefixes[] = {'\t', ' '};
+  for (int i = 0; i < 2; ++i) {
+    std::vector<char> value;
+    value.push_back(prefixes[i]);
+    // obs-text. From 0x80 to 0xff.
+    for (int j = 0x80; j <= 0xff; j++) {
+      value.push_back(j);
+    }
+    header_values[i] = std::string(value.data());
+    header_values[i].append(
+        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789!#$%&'*+"
+        "-.^_`|~");

[asanka, 2017/02/23 23:18:54]

This should be VCHAR, which is defined in RFC 5234:

VCHAR          =  %x21-7E
                                ; visible (printing) characters

The string you used is for tchars :-).

[xunjieli, 2017/02/24 16:33:32]

Done.

+    header_coalescer_.OnHeader(base::StringPrintf("%s_%d", "foo", i),
+                               header_values[i]);
+    EXPECT_FALSE(header_coalescer_.error_seen());
+  }
+  SpdyHeaderBlock header_block = header_coalescer_.release_headers();
+  EXPECT_THAT(header_block,
+              ElementsAre(Pair("foo_0", header_values[0]),
+                          Pair("foo_1", header_values[1])));
+}
+
 }  // namespace test
+
 }  // namespace net