Add parsers for the Encryption and Encryption-Key HTTP headers.

components/gcm_driver/crypto/encryption_header_parsers_unittest.cc

Index: components/gcm_driver/crypto/encryption_header_parsers_unittest.cc
diff --git a/components/gcm_driver/crypto/encryption_header_parsers_unittest.cc b/components/gcm_driver/crypto/encryption_header_parsers_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..601cf9e2989b9ce0f60f542a7e7c00e6723a91ff
--- /dev/null
+++ b/components/gcm_driver/crypto/encryption_header_parsers_unittest.cc
@@ -0,0 +1,190 @@
+// Copyright 2015 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 "components/gcm_driver/crypto/encryption_header_parsers.h"
+
+#include <vector>
+
+#include "base/macros.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace gcm {
+
+namespace {
+
+const int64_t kDefaultRecordSize = 4096;
+
+TEST(EncryptionHeaderParsersTest, ParseValidEncryptionHeaders) {
+  struct {
+    const char* const header;
+    const char* const parsed_keyid;
+    const char* const parsed_salt;
+    int64_t parsed_rs;
+  } expected_results[] = {
+    { "keyid=foo;salt=c2l4dGVlbmNvb2xieXRlcw;rs=1024",
+      "foo", "sixteencoolbytes", 1024 },
+    { "keyid=foo; salt=c2l4dGVlbmNvb2xieXRlcw; rs=1024",
+      "foo", "sixteencoolbytes", 1024 },
+    { "KEYID=foo;SALT=c2l4dGVlbmNvb2xieXRlcw;RS=1024",
+      "foo", "sixteencoolbytes", 1024 },
+    { " keyid = foo ; salt = c2l4dGVlbmNvb2xieXRlcw ; rs = 1024 ",
+      "foo", "sixteencoolbytes", 1024 },
+    { "keyid=foo", "foo", "", kDefaultRecordSize },

[eroman, 2015/09/25 18:18:19]

Does this file incldue a test for unrecognized roperties as in   keyid=foo; 
foopy=sup  ?

[Peter Beverloo, 2015/09/28 11:27:16]

Line 40 (Encryption) and 106 (Encryption-Key).

+    { "keyid=foo;", "foo", "", kDefaultRecordSize },
+    { "keyid=\"foo\"", "foo", "", kDefaultRecordSize },
+    { "keyid='foo'", "foo", "", kDefaultRecordSize },
+    { "salt=c2l4dGVlbmNvb2xieXRlcw",
+      "", "sixteencoolbytes", kDefaultRecordSize },
+    { "rs=2048", "", "", 2048 },
+    { "keyid=foo;someothervalue=1;rs=42", "foo", "", 42 },
+    { "keyid=foo;keyid=bar", "bar", "", kDefaultRecordSize },
+  };
+
+  for (size_t i = 0; i < arraysize(expected_results); i++) {
+    SCOPED_TRACE(i);
+
+    std::string keyid, salt;
+    int64_t rs = kDefaultRecordSize;
+
+    ASSERT_TRUE(ParseEncryptionHeader(expected_results[i].header, &keyid,
+                                      &salt, &rs));
+
+    EXPECT_EQ(expected_results[i].parsed_keyid, keyid);
+    EXPECT_EQ(expected_results[i].parsed_salt, salt);
+    EXPECT_EQ(expected_results[i].parsed_rs, rs);
+  }
+}
+
+TEST(EncryptionHeaderParsersTest, ParseInvalidEncryptionHeaders) {
+  const char* const expected_failures[] = {
+    "keyid",
+    "keyid=",
+    "keyid=foo;novaluekey",
+    "salt",
+    "salt=",
+    "salt=123$xyz",
+    "rs",
+    "rs=",
+    "rs=1",

[eroman, 2015/09/25 18:18:19]

These are all good tests!
I would additionally suggest:
  rs=0
  rs=012
  rs=0x13
  rs=+5

(Since historically these are the kind of values that non-strict http parsers
have recognized)

[Peter Beverloo, 2015/09/28 11:27:15]

Done, except for the following two which pass:

rs=012
rs=+5

(Both are allowed by the StringToUint64 implementation, and don't sound entirely
unreasonable, despite 012 == 12 rather than 10)

[eroman, 2015/09/28 16:25:29]

Regarding rs=+5:

While this behavior my be reasonable, it still strikes me as problematic.

At a minimum it is something that is underspecified by the specification, which
merely says that rs is a "positive decimal integer".

The problem with extra permissiveness in parsing is it leads to incompatible
implementations (I feel strongly about this from past experience, having to deal
with a world where other UAs implemented number parsing via sscanf(%d) to
horrifying results).

For this case in particular given that details in your target specification are
scant, I would look to RFC 2616 for precedent on how integers are represented.
For instance here is how Content-Length is specified:

       DIGIT          = <any US-ASCII digit "0".."9">
...
       Content-Length    = "Content-Length" ":" 1*DIGIT

So working off this I would expect that:
  * leading 0 is acceptable, and is to be interpreted as decimal (not octal)
  * Leading + is invalid. So too are any other prefix like 0x, or any numerical
separator.

So anyway my expectation is that rs=+5 is something to reject.

Can you follow-up with the spec on this?

[Peter Beverloo, 2015/10/01 14:56:20]

That's a great explanation, thank you! Since this is an addition on top of the
string to integer routines in //base, I will add an if-statement in
RecordSizeToInt() and re-included the test.

In general, the spec defines both headers as accepting a number of "parameter"
rules, which basically accept anything in the "key=value" syntax, and then
imposes limitations on the values based on the expected kinds of data. There
probably is a way to enforce the exact syntax for each of these rules. I'll
follow up.

+    "rs=-1",
+    "rs=99999999999999999999999999999999",
+    "rs=foobar",
+  };
+
+  for (size_t i = 0; i < arraysize(expected_failures); i++) {
+    SCOPED_TRACE(i);
+
+    std::string keyid, salt;
+    int64_t rs = kDefaultRecordSize;

[eroman, 2015/09/25 18:18:19]

Does this initialization matter? (I don't mind it, just wondering)

[Peter Beverloo, 2015/09/28 11:27:16]

No, it does not. Muscle memory reminds me of cases where not initializing a
value like this (even though it should only fail on access) causes odd compiler
errors, but that may be outdated by now.

+
+    EXPECT_FALSE(ParseEncryptionHeader(expected_failures[i], &keyid, &salt,
+                                       &rs));
+  }
+}
+
+TEST(EncryptionHeaderParsersTest, ParseValidEncryptionKeyHeaders) {
+  struct {
+    const char* const header;
+    const char* const parsed_keyid;
+    const char* const parsed_key;
+    const char* const parsed_dh;
+  } expected_results[] = {
+    { "keyid=foo;key=c2l4dGVlbmNvb2xieXRlcw;dh=dHdlbHZlY29vbGJ5dGVz",
+      "foo", "sixteencoolbytes", "twelvecoolbytes" },
+    { "keyid=foo; key=c2l4dGVlbmNvb2xieXRlcw; dh=dHdlbHZlY29vbGJ5dGVz",
+      "foo", "sixteencoolbytes", "twelvecoolbytes" },
+    { "keyid = foo ; key = c2l4dGVlbmNvb2xieXRlcw ; dh = dHdlbHZlY29vbGJ5dGVz ",
+      "foo", "sixteencoolbytes", "twelvecoolbytes" },
+    { "KEYID=foo;KEY=c2l4dGVlbmNvb2xieXRlcw;DH=dHdlbHZlY29vbGJ5dGVz",
+      "foo", "sixteencoolbytes", "twelvecoolbytes" },
+    { "keyid=foo", "foo", "", "" },
+    { "key=c2l4dGVlbmNvb2xieXRlcw", "", "sixteencoolbytes", "" },
+    { "key=\"c2l4dGVlbmNvb2xieXRlcw\"", "", "sixteencoolbytes", "" },
+    { "key='c2l4dGVlbmNvb2xieXRlcw'", "", "sixteencoolbytes", "" },
+    { "dh=dHdlbHZlY29vbGJ5dGVz", "", "", "twelvecoolbytes" },
+    { "keyid=foo;someothervalue=bar;key=dHdlbHZlY29vbGJ5dGVz",
+      "foo", "twelvecoolbytes", "" },
+    { "keyid=foo;keyid=bar", "bar", "", "" },
+  };
+
+  for (size_t i = 0; i < arraysize(expected_results); i++) {
+    SCOPED_TRACE(i);
+
+    std::string keyid, key, dh;
+
+    ASSERT_TRUE(ParseEncryptionKeyHeader(expected_results[i].header, &keyid,
+                                         &key, &dh));
+
+    EXPECT_EQ(expected_results[i].parsed_keyid, keyid);
+    EXPECT_EQ(expected_results[i].parsed_key, key);
+    EXPECT_EQ(expected_results[i].parsed_dh, dh);
+  }
+}
+
+TEST(EncryptionHeaderParsersTest, ParseInvalidEncryptionKeyHeaders) {
+  const char* const expected_failures[] = {
+    "keyid",
+    "keyid=",
+    "keyid=foo;novaluekey",

[eroman, 2015/09/25 18:18:20]

I suggest also testing:
  * characters which are not part of base64url (+ and /)
  * Incorrect base64 padding

[Peter Beverloo, 2015/09/28 11:27:16]

Done, also for the Encryption header.

+    "key",
+    "key=",
+    "key=123$xyz",
+    "dh",
+    "dh=",
+    "dh=123$xyz",
+  };
+
+  for (size_t i = 0; i < arraysize(expected_failures); i++) {

[eroman, 2015/09/25 18:18:20]

optional: Though the magic of c++11 I believe you can write this as:

 for (const auto& expected_failure : expected_failures) {
    ...
  }


But then you wouldn't be able to write SCOPED_TRACE(i) 

[Peter Beverloo, 2015/09/28 11:27:16]

Acknowledged.

+    SCOPED_TRACE(i);
+
+    std::string keyid, key, dh;
+
+    EXPECT_FALSE(ParseEncryptionKeyHeader(expected_failures[i], &keyid, &key,
+                                          &dh));
+  }
+}
+
+TEST(EncryptionHeaderParsersTest, InvalidHeadersDoNotModifyOutput) {
+  std::string keyid = "mykeyid";
+  std::string salt = "somesalt";
+  int64_t rs = 42;
+
+  ASSERT_FALSE(ParseEncryptionHeader("rs=foobar", &keyid, &salt, &rs));
+
+  EXPECT_EQ("mykeyid", keyid);
+  EXPECT_EQ("somesalt", salt);
+  EXPECT_EQ(42, rs);
+
+  std::string key = "akey";
+  std::string dh = "yourdh";
+
+  ASSERT_FALSE(ParseEncryptionKeyHeader("key=$$$", &keyid, &key, &dh));
+
+  EXPECT_EQ("mykeyid", keyid);
+  EXPECT_EQ("akey", key);
+  EXPECT_EQ("yourdh", dh);
+}
+
+TEST(EncryptionHeaderParsersTest, ParseMultipleValueHeaders) {
+  // Chrome currently does not support multiple lists of parameters as part
+  // of the Encryption headers. Make sure we reject such headers until we do.
+  std::string keyid, salt;
+  int64_t rs = kDefaultRecordSize;
+
+  EXPECT_FALSE(ParseEncryptionHeader(
+      "keyid=foo;salt=c2l4dGVlbmNvb2xieXRlcw;rs=12,"
+          "keyid=bar;salt=dHdlbHZlY29vbGJ5dGVzl;rs=24",
+      &keyid, &salt, &rs));
+
+  std::string key, dh;
+
+  EXPECT_FALSE(ParseEncryptionKeyHeader(
+      "keyid=foo;key=c2l4dGVlbmNvb2xieXRlcw;dh=dHdlbHZlY29vbGJ5dGVzl,"
+          "keyid=bar;key=dHdlbHZlY29vbGJ5dGVzl;dh=c2l4dGVlbmNvb2xieXRlcw",
+      &keyid, &key, &dh));
+}
+
+}  // namespace
+
+}  // namespace gcm