Share link header parsing code between blink and content.

net/http/http_util.cc

Index: net/http/http_util.cc
diff --git a/net/http/http_util.cc b/net/http/http_util.cc
index 932f8386dadc401914a559f20805b951f66876cb..b04571ca51e4f7c726013a9d260b87d122bf262b 100644
--- a/net/http/http_util.cc
+++ b/net/http/http_util.cc
@@ -501,6 +501,55 @@ std::string HttpUtil::Unquote(const std::string& str) {
 }
 
 // static
+bool HttpUtil::StrictUnquote(std::string::const_iterator begin,

[mmenke, 2016/04/26 18:05:57]

Hrm...Should this and Unquote share an implementation?  The only differences are
the c == quote check, and the terminal escape characted check, right?

I'd suggest a shared implementation in an anonymous namespace in this file, and
just have these two methods call it.  It makes the differences between them
clearer, and gets us a bit better test coverage of the code.  Also makes
accidental future divergence of the two methods less likely.

[Marijn Kruisselbrink, 2016/04/27 01:49:49]

Good idea. Done.

+                             std::string::const_iterator end,
+                             std::string* out) {
+  // Empty string
+  if (begin == end)
+    return false;
+
+  // Nothing to unquote.
+  if (!IsQuote(*begin))
+    return false;
+
+  // No terminal quote mark.
+  if (end - begin < 2 || *begin != *(end - 1))
+    return false;
+
+  char quote = *begin;
+
+  // Strip quotemarks
+  ++begin;
+  --end;
+
+  // Terminal quote is escaped.
+  if (begin != end && *(end - 1) == '\\')

[mmenke, 2016/04/26 18:05:57]

BUG:  "\\" is valid.  Instead, should have a "prev_unescaped == false" check on
the end.  Add a test for this.

[Marijn Kruisselbrink, 2016/04/27 01:49:49]

Yeah, just caught that myself too. Added a test and fixed the code.

+    return false;
+
+  // Unescape quoted-pair (defined in RFC 2616 section 2.2)
+  bool prev_escape = false;
+  std::string unescaped;
+  for (; begin != end; ++begin) {
+    char c = *begin;
+    if (c == '\\' && !prev_escape) {
+      prev_escape = true;
+      continue;
+    }
+    if (!prev_escape && c == quote)
+      return false;
+    prev_escape = false;
+    unescaped.push_back(c);
+  }
+  *out = std::move(unescaped);
+  return true;
+}
+
+// static
+bool HttpUtil::StrictUnquote(const std::string& str, std::string* out) {
+  return StrictUnquote(str.begin(), str.end(), out);
+}
+
+// static
 std::string HttpUtil::Quote(const std::string& str) {
   std::string escaped;
   escaped.reserve(2 + str.size());
@@ -910,7 +959,8 @@ HttpUtil::NameValuePairsIterator::NameValuePairsIterator(
     std::string::const_iterator begin,
     std::string::const_iterator end,
     char delimiter,
-    OptionalValues optional_values)
+    Values optional_values,
+    Quotes strict_quotes)
     : props_(begin, end, delimiter),
       valid_(true),
       name_begin_(end),
@@ -918,13 +968,18 @@ HttpUtil::NameValuePairsIterator::NameValuePairsIterator(
       value_begin_(end),
       value_end_(end),
       value_is_quoted_(false),
-      values_optional_(optional_values == VALUES_OPTIONAL) {}
+      values_optional_(optional_values == Values::OPTIONAL),
+      strict_quotes_(strict_quotes == Quotes::STRICT) {}
 
 HttpUtil::NameValuePairsIterator::NameValuePairsIterator(
     std::string::const_iterator begin,
     std::string::const_iterator end,
     char delimiter)
-    : NameValuePairsIterator(begin, end, delimiter, VALUES_NOT_OPTIONAL) {}
+    : NameValuePairsIterator(begin,
+                             end,
+                             delimiter,
+                             Values::NOT_OPTIONAL,
+                             Quotes::NOT_STRICT) {}
 
 HttpUtil::NameValuePairsIterator::NameValuePairsIterator(
     const NameValuePairsIterator& other) = default;
@@ -980,6 +1035,14 @@ bool HttpUtil::NameValuePairsIterator::GetNext() {
   }
 
   if (value_begin_ != value_end_ && HttpUtil::IsQuote(*value_begin_)) {

[mmenke, 2016/04/26 18:05:57]

So strict mode fails on foo='bar but succeeds on foo=bar'?  IsToken even returns
true on the latter (Though if it were ", it would return false).  Is this
expected behavior?

[Marijn Kruisselbrink, 2016/04/27 01:49:49]

Argh, excellent question... And not really something I have an answer to. Maybe
it makes most sense for strict-mode to disallow single quotes entirely. Also
because treating single quotes as quoted strings is actually possibly
spec-incompliant (at least in the Link header spec foo='bar is perfectly legal,
assigning "'bar" to the foo extended parameter. Similarly foo='a\b' should
result in "'a\b'" as value, without any unescaping/unquoting).

Yoav: any particular reason you wrote LinkHeader.cpp to (incorrectly) unquote
strings in single quotes?

Of course as you point out the situation is also weird for foo=bar". At least
code consuming NameValuePairsIterator could detect that via
(!it.value_is_quoted() && !HttpUtil::IsToken(it.value_begin(), it.value_end())).
I don't think it makes sense to make NameValuePairsIterator even more strict by
implying this IsToken check as part of strict quotes (for example the Link
header spec actually uses a |ptoken| rule as valid unquoted values, which allows
for more characters than a plain |token|...)

+    value_is_quoted_ = true;
+
+    if (strict_quotes_) {
+      if (!HttpUtil::StrictUnquote(value_begin_, value_end_, &unquoted_value_))
+        return valid_ = false;
+      return true;
+    }
+
     // Trim surrounding quotemarks off the value
     if (*value_begin_ != *(value_end_ - 1) || value_begin_ + 1 == value_end_) {
       // NOTE: This is not as graceful as it sounds:
@@ -987,9 +1050,9 @@ bool HttpUtil::NameValuePairsIterator::GetNext() {
       //   (["\"hello] should give ["hello]).
       // * Does not detect when the final quote is escaped
       //   (["value\"] should give [value"])
+      value_is_quoted_ = false;
       ++value_begin_;  // Gracefully recover from mismatching quotes.
     } else {
-      value_is_quoted_ = true;
       // Do not store iterators into this. See declaration of unquoted_value_.
       unquoted_value_ = HttpUtil::Unquote(value_begin_, value_end_);
     }