Add read-only access to POST data for webRequest's onBeforeRequest

chrome/browser/extensions/api/web_request/post_data_parser.cc

+// Copyright (c) 2012 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 "chrome/browser/extensions/api/web_request/post_data_parser.h"
+
+#include "base/values.h"
+#include "net/base/escape.h"
+#include "net/base/upload_data.h"
+#include "net/url_request/url_request.h"
+
+namespace {
+const char kContentDisposition[] = "Content-Disposition:";
+
+// Takes |dictionary| of <string, list of strings> pairs, and gets the list
+// for |key|, creating it if necessary.
+ListValue* GetOrCreateList(DictionaryValue* dictionary,
+                           const std::string& key) {
+  ListValue* list = NULL;
+  if (!dictionary->GetList(key, &list)) {
+    list = new ListValue();
+    dictionary->Set(key, list);
+  }
+  return list;
+}
+}

[wtc, 2012/08/03 00:57:19]

Nit: add a //namespace comment, like this:

  }  // namespace

Nit: add a blank line after
  namespace {
and before
  }  // namespace

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+
+namespace extensions {
+
+// Implementation of PostDataParser and PostDataParser::Result .
+
+PostDataParser::Result::Result() {}
+PostDataParser::Result::~Result() {}
+
+void PostDataParser::Result::Reset() {
+  key_.erase();
+  val_.erase();
+}
+
+void PostDataParser::Result::SetKey(const base::StringPiece& str) {
+  key_.replace(0, std::string::npos, str.data(), str.size());
+}
+
+void PostDataParser::Result::SetVal(const base::StringPiece& str) {
+  val_.replace(0, std::string::npos, str.data(), str.size());

[wtc, 2012/08/03 00:57:19]

I think you can use base::CopyToString (declared in
"base/string_piece.h") to implement these two methods.

[vabr (Chromium), 2012/08/05 18:54:46]

Thanks, that's much more readable!

+}
+
+void PostDataParser::Result::SetKey(const std::string& str) {
+  key_ = str;
+}
+
+void PostDataParser::Result::SetVal(const std::string& str) {
+  val_ = str;
+}

[wtc, 2012/08/03 00:57:19]

I think these four methods should be named set_key()
and set_value(), to match the naming convention of the
getter methods key() and value().

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

(For the record, if somebody asks me later: the Google style guide prescribes
unix_hacker style for fast inlined functions OR accessors/mutators. In
particular, the accessors/mutators should be unix_hacker style even if not
inlined.)

[Matt Perry, 2012/08/06 21:06:45]

Interesting, I didn't know that! Thanks!

+
+PostDataParser::~PostDataParser() {}
+
+// static
+scoped_ptr<PostDataParser> PostDataParser::CreatePostDataParser(
+    const net::URLRequest* request) {
+  std::string value;
+  const bool found = request->extra_request_headers().GetHeader(
+      net::HttpRequestHeaders::kContentType, &value);
+  return CreatePostDataParser(found ? &value : NULL);
+}
+
+// static
+scoped_ptr<PostDataParser> PostDataParser::CreatePostDataParser(
+    const std::string* content_type_header) {

[wtc, 2012/08/03 00:57:19]

Nit: this argument is named |content_type| in the header file.

[vabr (Chromium), 2012/08/05 18:54:46]

Done (corrected the header).

+  enum ParserChoice {kUrlEncoded, kMultipart, kError};
+  ParserChoice choice = kError;
+  std::string boundary;
+
+  if (content_type_header == NULL) {
+    choice = kUrlEncoded;
+  } else {
+    const std::string content_type(
+        content_type_header->substr(0, content_type_header->find(';')));
+    if (content_type == "application/x-www-form-urlencoded") {
+      choice = kUrlEncoded;
+    } else if (content_type == "multipart/form-data") {

[wtc, 2012/08/03 00:57:19]

Should we do case-insensitive string comparison in this
function?

[vabr (Chromium), 2012/08/05 18:54:46]

That's a good point. RFC 2388 does not specify case sensitivity of
multipart/form-data (it uses lower-case, but initially mentions
Multipart/Form-Data). RFC 2046 about MIME media types also does not specify case
sensitivity. I believe it is safer to do case-insensitive comparison, as you
suggest.

+      const char kBoundaryString[] = "boundary=";

[wtc, 2012/08/03 00:57:19]

Nit: add 'static'

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+      size_t offset = content_type_header->find(kBoundaryString);
+      if (offset == std::string::npos) {
+        // Malformed header.
+        return scoped_ptr<PostDataParser>();
+      }
+      offset += strlen(kBoundaryString);
+      boundary = content_type_header->substr(
+          offset, content_type_header->find(';', offset));

[wtc, 2012/08/03 00:57:19]

I think we should also return scoped_ptr<PostDataParser>()
if boundary.empty() is true.

[vabr (Chromium), 2012/08/05 18:54:46]

Indeed, thanks for catching this.

+      choice = kMultipart;
+    }
+  }
+  // Other cases are unparseable, including when |content_type| is "text/plain".
+
+  switch (choice) {
+    case kUrlEncoded:
+      return scoped_ptr<PostDataParser>(new PostDataParserUrlEncoded());
+    case kMultipart:
+      return scoped_ptr<PostDataParser>(new PostDataParserMultipart(boundary));
+    default:  // In other words, case kError:
+      return scoped_ptr<PostDataParser>();
+  }
+}
+
+// static
+scoped_ptr<base::DictionaryValue> PostDataParser::ParseURLRequestData(
+    const net::URLRequest* request) {
+  if (request->method() != "POST")
+    return scoped_ptr<base::DictionaryValue>();
+  const std::vector<net::UploadData::Element>* elements =
+      request->get_upload()->elements();
+  scoped_ptr<PostDataParser> parser = CreatePostDataParser(request);
+  if (parser.get() == NULL) {

[wtc, 2012/08/03 00:57:19]

Nit: just do
  if (!parser) {

[vabr (Chromium), 2012/08/05 18:54:46]

Function deleted in the meantime.

+    // No parser means most probably unsupported form encoding.
+    return scoped_ptr<base::DictionaryValue>();
+  }
+  scoped_ptr<base::DictionaryValue> form_data(new base::DictionaryValue);
+  std::vector<net::UploadData::Element>::const_iterator element;
+  bool data_valid = true;
+  for (element = elements->begin();
+       data_valid && element != elements->end(); ++element) {
+    if (element->type() != net::UploadData::TYPE_BYTES) {
+      // We do not handle data including blobs or chunks.
+      if (element->type() != net::UploadData::TYPE_FILE)
+        data_valid = false;

[wtc, 2012/08/03 00:57:19]

If we simply return scoped_ptr<base::DictionaryValue>() here,
we don't need the data_valid variable.

We ignore all the POST data in a file.  Is that intentional?
Is it because form data is unlikely to be in a file?

Why do you consider blobs as invalid data?

[vabr (Chromium), 2012/08/05 18:54:46]

Function, and |data_valid| deleted in the meantime.
Yes, this is intentional. For files we only pass their name, to avoid copying
large amounts of data.
This is due to comment in upload_data.h, above the definition of SetToBlobUrl:
"UploadData should not contain any blob reference".

+      continue;
+    }
+    if (!parser->SetSource(&(element->bytes())))
+      continue;
+    Result result;
+    while (parser->GetNextPair(&result)) {
+      GetOrCreateList(form_data.get(), result.key())->Append(
+          new StringValue(result.val()));
+    }
+  }
+  if (data_valid && parser->AllDataReadOK())
+    return form_data.Pass();
+  else
+    return scoped_ptr<base::DictionaryValue>();
+}
+
+// Implementation of PostDataParserUrlEncoded.
+
+PostDataParserUrlEncoded::PostDataParserUrlEncoded() : source_(NULL) {}
+
+PostDataParserUrlEncoded::~PostDataParserUrlEncoded() {}
+
+bool PostDataParserUrlEncoded::AllDataReadOK() {
+  return source_ != NULL && offset_ == source_->end();
+}
+
+bool PostDataParserUrlEncoded::GetNextPair(Result* result) {
+  result->Reset();
+  if (source_ == NULL)
+    return false;
+  if (offset_ == source_->end())
+    return false;
+  std::vector<char>::const_iterator seek = offset_;
+  // (*) Now we have |seek| >= |offset_| until the end of this function:
+  while (seek != source_->end() && *seek != '=')
+    ++seek;
+  if (seek == source_->end()) {
+    // This means the data is malformed.
+    offset_ = seek;

[wtc, 2012/08/03 00:57:19]

BUG: if you set offset_ to seek here, it'll cause
AllDataReadOK() to return true, which means the data
was well formed, but the data is actually malformed.

[vabr (Chromium), 2012/08/05 18:54:46]

Indeed, very nicely spotted, thanks!

However, this class has been rewritten slightly in the meantime, to also include
check for the number of occurrences of '=' and '&'. In this case there would be
one '&' too many, and thus AllDataReadOK() would return false. Also, for clarity
I added Abort(), which I needed to introduce in GetNextChar anyway.

+    return false;
+  }
+  std::string encoded_key(&(*offset_), seek - offset_);  // Safe, see (*).

[wtc, 2012/08/03 00:57:19]

Is it necessary to do &(*offset_) ?  I guess offset_ isn't
of the right type?

[vabr (Chromium), 2012/08/05 18:54:46]

I'm afraid it is necessary. offset_ is a iterator, *offset_ is a char, and
&(*offset_) is a pointer to char. There seems to be no implicit conversion from
the iterator to char*.

+  const net::UnescapeRule::Type unescape_rules =
+      net::UnescapeRule::URL_SPECIAL_CHARS | net::UnescapeRule::CONTROL_CHARS |
+      net::UnescapeRule::SPACES | net::UnescapeRule::REPLACE_PLUS_WITH_SPACE;
+  result->SetKey(net::UnescapeURLComponent(encoded_key, unescape_rules));
+  offset_ = ++seek;
+  while (seek != source_->end() && *seek != '&')
+    ++seek;
+  std::string encoded_val(&(*offset_), seek - offset_);  // Safe, see (*).
+  result->SetVal(net::UnescapeURLComponent(encoded_val, unescape_rules));
+  offset_ = (seek == source_->end()) ? seek : seek + 1;
+  return true;
+}
+
+bool PostDataParserUrlEncoded::SetSource(const std::vector<char>* source) {
+  if (source_ != NULL)
+    return false;

[wtc, 2012/08/03 00:57:19]

This allows SetSource() to be called only once.  Why?

[vabr (Chromium), 2012/08/05 18:54:46]

I don't think Chrome/WebKit splits URLEncoded forms into more blocks, and it is
not clear how would it split them if it did. For example, which of the following
splits would be allowed?
"name1=value1", "&name2=value2"
"name1=value1&", "name2=value2"
"name1=value1&nam", "e2=value2"
Writing the parser to accept any of the above examples would mean an increased
complexity of it. Taking care of the first two cases feels a bit random. So I
decided to assume one block only, and fix it when it proves to be a problem.

Please let me know if you disagree with my decision on this.

[wtc, 2012/08/09 22:02:50]

Thank you for the explanation.

I was wondering if we need to allow a PostDataParser to be
reused after having parsed one source.  If each PostDataParser
object is intended to be used only once, then I agree with
your decision.

[vabr (Chromium), 2012/08/10 17:12:55]

I can confirm that the each instance of PostDataParser implementations is only
meant for parsing one form.
(The Multipart implementation can be fed multiple parts of the encoded form,
thus multiple calls to SetSource; this is not the case uf the URLencoded
implementation.)

+  source_ = source;
+  offset_ = source_->begin();
+  return true;
+}
+
+// Implementation of PostDataParserMultipart.
+
+PostDataParserMultipart::PostDataParserMultipart(
+    const std::string& boundary_separator)
+    : source_(NULL),
+      length_(0),  // Dummy value.
+      line_start_(0),  // Dummy value.
+      line_end_(0),  // Dummy value.
+      next_line_(0),  // Dummy value.
+      boundary_("--" + boundary_separator),
+      final_boundary_(boundary_ + "--"),
+      state_(kInit),
+      line_type_(kEmpty)  // Dummy value.
+{}

[wtc, 2012/08/03 00:57:19]

Nit: the opening curly brace '{' probably should be on the
previous line.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+
+PostDataParserMultipart::~PostDataParserMultipart() {}
+
+bool PostDataParserMultipart::AllDataReadOK() {
+  return source_ != NULL && next_line_ >= length_ && state_ == kFinal;
+}
+
+// This function reads one block of the data, between two boundaries.
+// First it reads the header to learn the key, and possibly also the
+// value, if this block is for a file input element.
+// Otherwise it then reads the value from the body.
+bool PostDataParserMultipart::GetNextPair(Result* result) {
+  result->Reset();
+  if (state_ == kError)
+    return false;
+  while (state_ != kHeadRead) {
+    if (!DoStep())
+      return false;
+  }
+  bool val_extracted = false;
+  bool name_parsed = ParseHead(result, &val_extracted);
+  while (state_ != kBody) {
+    if (!DoStep())
+      return false;
+  }
+  size_t val_start;
+  size_t val_end = 0;  // Dummy value, replaced below, see (*).
+  // There may not be more to read from |source_| if the current result comes
+  // from a "file" input element. But then |result| is complete already.
+  if (!DoStep())
+    return val_extracted;
+  val_start = line_start_;
+  // (*) Now state_ == kBody, so val_end gets updated below.
+  while (state_ != kHeadStart && state_ != kFinal) {
+    val_end = line_end_;
+    if (!DoStep()) break;
+  }
+  if (name_parsed && !val_extracted) {
+    result->SetVal(base::StringPiece(source_ + val_start, val_end - val_start));
+  }
+  return name_parsed;
+}
+
+bool PostDataParserMultipart::SetSource(const std::vector<char>* source) {
+  if (state_ == kError)
+    return false;
+  if (source_ != NULL && next_line_ < length_)
+    return false;
+  source_ = &(source->front());
+  length_ = source->size();
+  next_line_ = 0;

[wtc, 2012/08/03 00:57:19]

Should we also set line_start_ and line_end_ to 0?

[vabr (Chromium), 2012/08/05 18:54:46]

We do not need to, they will be set accordingly as soon as the first line is
requested via SeekNextLine.
My preference here was not to set them to 0 here, because it would indicate to
the reader that the first line is being considered to be of length 0.

I had to step aside and set them to 0 in the constructor, as non-class members
should always be initialized. There I marked those values explicitly as dummy. I
can do the same here, but in my opinion adding unnecessary lines does not help
the readability.

Please let me know if you still want me to reset them here.

[wtc, 2012/08/09 22:02:50]

No, I don't.  My previous suggestion was based on the
incorrect assumption that we need to allow a PostDataParser
to be reused, so it would seem that SetSource should
initialize the data members to the same values as the
constructor does.

[vabr (Chromium), 2012/08/10 17:12:55]

Just to be clear, PostDataParserMultipart::SetSource can be called multiple
times during the parser's lifetime. My arguments were actually against
consistency with the constructor, to omit unnecessary code and improve
readability.

+  return true;
+}
+
+bool PostDataParserMultipart::DoStep() {
+  if (!SeekNextLine())
+    return false;
+  switch (state_) {
+    case kInit:
+      if (line_type_ == kBoundary)
+        state_ = kHeadStart;
+      else
+        state_ = kError;
+      break;
+    case kHeadStart:
+      if (line_type_ == kDisposition)
+        state_ = kHeadRead;
+      else
+        state_ = kHead;
+      break;
+    case kHead:
+      if (line_type_ == kDisposition)
+        state_ = kHeadRead;
+      break;
+    case kHeadRead:
+      if (line_type_ == kEmpty)
+        state_ = kBody;
+      break;
+    case kBody:
+      if (line_type_ == kBoundary)
+        state_ = kHeadStart;
+      else if (line_type_ == kEndBoundary)
+        state_ = kFinal;
+      break;
+    case kFinal:
+      if (line_type_ != kEmpty)
+        state_ = kError;

[wtc, 2012/08/03 00:57:19]

Add a break statement.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+    case kError:
+      break;
+  }
+  return true;
+}
+
+PostDataParserMultipart::LineType PostDataParserMultipart::GetLineType() {
+  const size_t line_length = line_end_ - line_start_;
+  const base::StringPiece line(source_ + line_start_, line_length);
+  if (line == boundary_)
+    return kBoundary;
+  else if (line == final_boundary_)
+    return kEndBoundary;

[wtc, 2012/08/03 00:57:19]

Nit: I suggest making "final boundary" and "end boundary"
consistent.

[vabr (Chromium), 2012/08/05 18:54:46]

Done.

+  else if (line.starts_with(kContentDisposition))
+    return kDisposition;
+  else if (line_start_ == line_end_)
+    return kEmpty;
+  else
+    return kOther;
+}
+
+// Contract: only to be called from DoStep().
+bool PostDataParserMultipart::SeekNextLine() {
+  if (source_ == NULL || state_ == kError)
+    return false;
+  if (next_line_ >= length_)
+    return false;
+  line_start_ = next_line_;
+  size_t seek = line_start_;
+  while (seek < length_ && *(source_ + seek) != '\r')
+    ++seek;
+  line_end_ = seek;
+  line_type_ = GetLineType();
+  if ((seek+1) < length_ && strncmp(source_ + seek, "\r\n", 2) != 0)
+    return false;
+  next_line_ = seek + 2;

[wtc, 2012/08/03 00:57:19]

If the data does not end with a final "\r\n", this will
move next_line_ beyond length_ more than necessary.  It
won't break anything, just not necessary.

[vabr (Chromium), 2012/08/05 18:54:46]

Due to adding a check for garbage at the end, this has been taken care of as
well.

+  return true;
+}
+
+// Contract: line_type_ == kDisposition.
+bool PostDataParserMultipart::ParseHead(Result* result, bool* val_extracted) {
+  DCHECK_EQ(kDisposition, line_type_);
+  base::StringPiece line(source_ + line_start_, line_end_ - line_start_);
+  const char kNameEquals[] = " name=\"";
+  const char kFilenameEquals[] = " filename=\"";
+  size_t key_offset = line.find(kNameEquals);
+  if (key_offset == base::StringPiece::npos)
+    return false;
+  key_offset += strlen(kNameEquals);
+  result->SetKey(base::StringPiece(source_ + line_start_ + key_offset,
+                                   line.find('"', key_offset) - key_offset));
+  size_t val_offset = line.find(kFilenameEquals);
+  if (val_offset == std::string::npos) {
+    *val_extracted = false;
+  } else {
+    *val_extracted = true;
+    val_offset += strlen(kFilenameEquals);
+    result->SetVal(base::StringPiece(source_ + line_start_ + val_offset,
+                                     line.find('"', val_offset) - val_offset));
+  }
+  return true;
+}
+
+}  // namespace extensions