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

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

Index: chrome/browser/extensions/api/web_request/web_request_api.cc
diff --git a/chrome/browser/extensions/api/web_request/web_request_api.cc b/chrome/browser/extensions/api/web_request/web_request_api.cc
index bff5aea226f38494b67568fe844e87eda55c0c8f..3165bd01b3526aa2279823de3b6331570e086c16 100644
--- a/chrome/browser/extensions/api/web_request/web_request_api.cc
+++ b/chrome/browser/extensions/api/web_request/web_request_api.cc
@@ -44,6 +44,7 @@
 #include "grit/generated_resources.h"
 #include "net/base/auth.h"
 #include "net/base/net_errors.h"
+#include "net/base/upload_data.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -169,6 +170,341 @@ void ExtractRequestInfo(net::URLRequest* request, DictionaryValue* out) {
   out->SetDouble(keys::kTimeStampKey, base::Time::Now().ToDoubleT() * 1000);
 }
 
+// Interface for parsers for the POST data.
+class PostDataParser {
+ public:
+  struct Result {
+    const char* key;
+    size_t key_length;

[battre, 2012/07/06 07:36:24]

This looks like a usecase of StringPiece

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+    const char* val;
+    size_t val_length;
+  };
+  // Attempts to set the |length| bytes at |source| as the data to be parsed.

[battre, 2012/07/06 07:36:24]

s/Attempts to set/Sets/

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  // Returns true on success.
+  virtual bool SetSource(const char* source, size_t length) = 0;
+  // Returns the next key-value pair as |result|. After SetSource has succeeded,
+  // this allows to iterate over all pairs in the source.
+  // Returns true as long as a new pair was successfully found.
+  virtual bool GetNextPair(Result* result) = 0;
+  // Returns true if there was some data, it was well formed and all was read.
+  virtual bool AllDataReadOK() = 0;

[battre, 2012/07/06 07:36:24]

new line before protected: and private:

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+ protected:
+  PostDataParser() {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PostDataParser);
+};
+
+class PostDataParserUrlEncoded : public PostDataParser {
+ public:
+  PostDataParserUrlEncoded() : source_(NULL) {}

[battre, 2012/07/06 07:36:24]

length_ and offset_ should be initialized

[vabr (Chromium), 2012/07/06 14:09:01]

I did not initialize them on purpose. Their value does not make sense until the
source is set, and I found it confusing to assign anything to them. While
setting the source, length_ and offset_ get initialized properly, and before
that these two values are never used.
Do you insist on initializing them?

+  ~PostDataParserUrlEncoded() {}
+  virtual bool SetSource(const char* source, size_t length) OVERRIDE;

[battre, 2012/07/06 07:36:24]

Please add a new line

// Implementation of PostDataParser between 200 and 201

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool AllDataReadOK() OVERRIDE;

[battre, 2012/07/06 07:36:24]

new line before private:

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+ private:
+  // We parse the first |length_| bytes from |source_|.
+  const char* source_;
+  size_t length_;
+  size_t offset_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserUrlEncoded);
+};
+
+bool PostDataParserUrlEncoded::AllDataReadOK() {
+  return source_ != NULL && offset_ >= length_;
+}
+
+bool PostDataParserUrlEncoded::SetSource(const char* source, size_t length) {
+  if (source_ != NULL) return false;
+  source_ = source;
+  length_ = length;
+  offset_ = 0;
+  return true;
+}
+
+bool PostDataParserUrlEncoded::GetNextPair(Result* result) {
+  if (source_ == NULL) return false;
+  if (offset_ >= length_) return false;
+  size_t seek = offset_;
+  // (*) Now we have |seek| >= |offset_| until the end of this function:
+  while (seek < length_ && *(source_ + seek) != '=') ++seek;

[battre, 2012/07/06 07:36:24]

I think this is legal but unconventional style. Typically you would move the
++seek; to the next line.

[vabr (Chromium), 2012/07/06 14:09:01]

Done, also at the similar while-cycle below.

+  if (seek >= length_) {
+    // This means the data is malformed.
+    offset_ = seek;
+    return false;
+  }
+  result->key = source_ + offset_;
+  result->key_length = seek - offset_; // Safe, see (*).
+  offset_ = ++seek;
+  while (seek < length_ && *(source_ + seek) != '&') ++seek;
+  result->val = source_ + offset_;
+  result->val_length = seek - offset_; // Safe, see (*).
+  offset_ = ++seek;
+  return true;
+}
+
+class PostDataParserMultipart : public PostDataParser {
+ public:
+  explicit PostDataParserMultipart(const std::string& boundary)
+      : source_(NULL),
+        boundary_(boundary),
+        state_(kInit) {}

[battre, 2012/07/06 07:36:24]

initialize all atomic variables

[vabr (Chromium), 2012/07/06 14:09:01]

Similarly to PostDataParserUrlEncoded, the variables left out from
initialization have no meaningful value to be initialized with at the time of
the object's construction. They get initialized before the first use, line_type_
in GetLineType() via DoStep(), and the rest in SetSource().
In my opinion it would hurt the logic of the code to assign random values to
them here, please let me know any counterarguments you have.

+  ~PostDataParserMultipart() {}
+  virtual bool SetSource(const char* source, size_t length) OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool AllDataReadOK() OVERRIDE;
+ private:

[battre, 2012/07/06 07:36:24]

newline before private:

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  // Note on implementation:
+  // This parser reads the source line by line. There are four types of lines:
+  // (BOUND) "Boundary" line, separating entries.
+  // (FBOUND) Final "boundary" line, ends the data.
+  // (DISP) "Content-Disposition" line, containing the "key" for |result|.
+  // (EMPTY) empty lines
+  // (OTHER) other non-empty lines
+  enum Lines {kBound, kFBound, kDisp, kEmpty, kOther};
+  // The parser uses the following 7-state automaton to check for structure:
+  // kInit --BOUND--> kFirst, kInit --not(BOUND)--> kError
+  // kFirst --DISP--> kSkip, kFirst --not(DISP)--> kHead
+  // kHead --DISP--> kSkip, kHead --not(DISP)-->kHead
+  // kSkip --EMPTY-->kBody, kSkip --not(EMPTY)--> kSkip
+  // kBody --BOUND--> kFirst, kBody --FBOUND--> kFinal,
+  //      kBody --not(BOUND)--> kBody
+  enum States {kInit, kFirst, kHead, kSkip, kBody, kFinal, kError};
+  // Read one more line from |source_|, update line pointers.
+  bool GetNextLine();
+  // Determine the |line_type_| of the current line_.
+  void GetLineType();
+  // One-step of the automaton, based on |state_| and |line_type_|.
+  // This calls GetNextLine() and returns it return value.
+  bool DoStep();
+  // Extracts "key" and possibly "val" from a DISP line. Returns success.
+  bool ParseHead(Result* result);
+  // We parse the first |length_| bytes from |source_|.
+  const char* source_;
+  size_t length_;
+  // Offset of the current and next line from |source_|:
+  // [line_]...line... [line_end_]EOL [next_line_]...line...
+  size_t line_;
+  size_t line_end_;
+  size_t next_line_;
+  const std::string boundary_;
+  States state_;
+  Lines line_type_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserMultipart);
+};
+
+bool PostDataParserMultipart::AllDataReadOK() {
+  return source_ != NULL && next_line_ >= length_ && state_ == kFinal;
+}
+
+bool PostDataParserMultipart::SetSource(const char* source, size_t length) {
+  if (state_ == kError) return false;
+  if (source_ != NULL && next_line_ < length_) return false;
+  source_ = source;
+  length_ = length;
+  next_line_ = 0;
+  return true;
+}
+
+bool PostDataParserMultipart::GetNextPair(Result* result) {
+  if (state_ == kError) return false;
+  while (state_ != kSkip) if (!DoStep()) return false;

[battre, 2012/07/06 07:36:24]

This should go into multiple lines

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  bool name_parsed = ParseHead(result);
+  while (state_ != kBody) if (!DoStep()) return false;
+  size_t val_start;
+  size_t val_end;
+  // There may not be more to read from |source_| if the current result comes
+  // from a "file" input element. But then |result->val| != NULL already.
+  if (!DoStep()) return result->val != NULL;
+  val_start = line_;
+  while (state_ != kFirst && state_ != kFinal) {
+    val_end = line_end_;
+    if (!DoStep()) break;
+  }
+  if (name_parsed && result->val == NULL) {
+    // This pair is from non-file form item, |val| is on subsequent lines.
+    result->val = source_ + val_start;
+    result->val_length = val_end - val_start;
+  }
+  return name_parsed;
+}
+
+// Contract: only to be called from GetNextLine().
+void PostDataParserMultipart::GetLineType() {
+  const char* line = source_ + line_;
+  const size_t line_length = line_end_ - line_;
+  const bool boundary_test = line[0] == '-' && line[1] == '-' &&
+      strncmp(boundary_.c_str(), line+2, boundary_.size()) == 0;
+  if (boundary_test && line_length == boundary_.size() + 2)
+    line_type_ = kBound;
+  else if (boundary_test && line_length == boundary_.size() + 4 &&
+           line[line_length-2] == '-' && line[line_length-1] == '-')
+    line_type_ = kFBound;
+  else if (strncmp(line, "Content-Disposition:", 20) == 0)

[battre, 2012/07/06 07:36:24]

static const char kContentDisposition[] = "Content-Disposition:";
... strncmp(line, kContentDisposition, arraysize(kContentDisposition) - 1)

(please check whether this should be -1 or not, not sure whether this appends a
\0 by default)

[vabr (Chromium), 2012/07/06 14:09:01]

Done via strlen(), as it appears to be evaluated at compile time on string
constants under GCC. Unlike sizeof or arraysize, strlen does not include the
trailing zero.

+    line_type_ = kDisp;
+  else if (line_ == line_end_)
+    line_type_ = kEmpty;
+  else
+    line_type_ = kOther;
+}
+
+// Contract: only to be called from DoStep().
+bool PostDataParserMultipart::GetNextLine() {
+  if (source_ == NULL || state_ == kError) return false;
+  if (next_line_ >= length_) return false;
+  size_t seek = line_ = next_line_;
+  while (seek < length_ && *(source_ + seek) != '\r') ++seek;
+  line_end_ = seek;
+  GetLineType();
+  if (seek < length_ && *(source_ + seek + 1) != '\n') return false;
+  next_line_ = seek + 2;
+  return true;
+}
+
+bool PostDataParserMultipart::DoStep() {
+  if (!GetNextLine()) return false;
+  switch (state_) {
+    case kInit:
+      if (line_type_ == kBound) state_ = kFirst;

[battre, 2012/07/06 07:36:24]

\n after )

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+      else
+        state_ = kError;
+      break;
+    case kFirst:
+      if (line_type_ == kDisp) state_ = kSkip;
+      else
+        state_ = kHead;
+      break;
+    case kHead:
+      if (line_type_ == kDisp) state_ = kSkip;
+      break;
+    case kSkip:
+      if (line_type_ == kEmpty) state_ = kBody;
+      break;
+    case kBody:
+      if (line_type_ == kBound) state_ = kFirst;
+      else if (line_type_ == kFBound) state_ = kFinal;
+      break;
+    case kFinal:
+      if (line_type_ != kEmpty) state_ = kError;
+    case kError:
+      break;
+  }
+  return true;
+}
+
+// Contract: line_type_ == kDisp.
+bool PostDataParserMultipart::ParseHead(Result* result) {
+  std::string line(source_ + line_, line_end_-line_);
+  size_t key_offset = line.find(" name=\"") + 7;
+  if (key_offset == std::string::npos) return false;
+  result->key = source_ + line_ + key_offset;
+  result->key_length = line.find('"', key_offset) - key_offset;
+  size_t val_offset = line.find(" filename=\"");
+  if (val_offset == std::string::npos) {
+    result->val = NULL;
+  } else {
+    val_offset += 11;
+    result->val = source_ + line_ + val_offset;
+    result->val_length = line.find('"', val_offset) - val_offset;
+  }
+  return true;
+}
+
+// Helps to choose the right POST data parser and takes care of its lifetime.
+class PostDataParserProxy : public PostDataParser {
+ public:
+  PostDataParserProxy() : parser_(NULL) {}
+  ~PostDataParserProxy() {
+    if (parser_ != NULL) delete parser_;
+  }
+  // Chooses the parser based on content-type of the request. True == success.
+  bool Init(net::URLRequest* request);
+  virtual bool SetSource(const char* source, size_t length) OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool AllDataReadOK() OVERRIDE;
+ private:
+  PostDataParser* parser_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserProxy);
+};
+
+bool PostDataParserProxy::AllDataReadOK() {
+  return parser_ != NULL ? parser_->AllDataReadOK() : false;
+}
+
+bool PostDataParserProxy::Init(net::URLRequest* request) {
+  std::string value;
+  const bool found = request->extra_request_headers().GetHeader(
+      "Content-Type", &value);
+  std::string content_type = value.substr(0, value.find(';'));
+  if (!found || content_type == "application/x-www-form-urlencoded") {
+    parser_ = new PostDataParserUrlEncoded();
+  } else if (content_type == "text/plain") {
+    // Unable to parse, may be ambiguous.
+  } else if (content_type == "multipart/form-data") {
+    size_t offset = value.find("boundary=");
+    offset += 9; // 9 == length of "boundary="
+    std::string boundary = value.substr(offset, value.find(';', offset));
+    parser_ = new PostDataParserMultipart(boundary);
+  } else {
+    return false;
+  }
+  return parser_ != NULL;
+}
+
+bool PostDataParserProxy::SetSource(const char* source, size_t length) {
+  return parser_ != NULL ? parser_->SetSource(source, length) : false;
+}
+
+bool PostDataParserProxy::GetNextPair(Result* result) {
+  if (parser_ != NULL) return parser_->GetNextPair(result);

[battre, 2012/07/06 07:36:24]

newline before return.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  else
+    return false;
+}
+
+// Takes |dictionary| of <string, list of strings> pairs, and appends
+// |value| to the list for |key|, creating it if necessary.
+// Contract: |dictionary| only contains ListValue objects.
+void UpdateDictionaryWithString(DictionaryValue* dictionary,
+                                const std::string& key,
+                                const std::string& value) {
+  ListValue* list = NULL;
+  Value* receive = NULL;
+  if (!dictionary->Get(key, &receive)) {
+    list = new ListValue();
+    dictionary->Set(key, list);
+  } else {
+    list = reinterpret_cast<ListValue*>(receive); // Safe due to contract.

[battre, 2012/07/06 07:36:24]

you can get rid of the else clause if you use
if (!dictionary->GetList(key, &list)) {
above. Then you don't need receive either.

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  }
+  StringValue* string_value = new StringValue(value);
+  list->Append(string_value);

[battre, 2012/07/06 07:36:24]

I would inline string_value.

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+}
+
+// Extracts the POST data from |request| and writes the data into |out|.
+// This can be expensive, so it's separated from ExtractRequestInfo().
+// Contract: request->method() == "POST"
+void ExtractRequestInfoPost(net::URLRequest* request, DictionaryValue* out) {
+  const std::vector<net::UploadData::Element>* elements =
+      request->get_upload()->elements();
+  PostDataParserProxy parser;
+  parser.Init(request);
+  DictionaryValue* post_data = new DictionaryValue();

[battre, 2012/07/06 07:36:24]

please use a scoped_ptr<DictionaryValue>, this way the pointer cannot get lost.

[vabr (Chromium), 2012/07/06 14:09:01]

Done, including using get()/release() where appropriate and removing the delete
clause.

+  std::vector<net::UploadData::Element>::const_iterator element;
+  for (element = elements->begin(); element != elements->end(); ++element) {
+    if (element->type() != net::UploadData::TYPE_BYTES) continue;
+    const char* bytes = &(element->bytes()[0]);
+    const size_t size = element->bytes().size();
+    if (!parser.SetSource(bytes, size)) continue;

[battre, 2012/07/06 07:36:24]

Any reason why you don't pass a const std::vector<char>& here?

[vabr (Chromium), 2012/07/06 14:09:01]

No, good point. Done.

+    PostDataParser::Result result;
+    while (parser.GetNextPair(&result)) {
+      std::string key(result.key, result.key_length);
+      std::string val(result.val, result.val_length);
+      UpdateDictionaryWithString(post_data, key, val);

[battre, 2012/07/06 07:36:24]

How about
GetOrCreateList(post_data, key)->Append(new StringValue(val));
?
"Update" is kind of abstract.

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+    }
+  }
+  if (parser.AllDataReadOK()) out->Set(keys::kPostDataKey, post_data);

[battre, 2012/07/06 07:36:24]

newline before out->Set
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

[vabr (Chromium), 2012/07/06 14:09:01]

Done.

+  else
+    delete post_data;
+}
+
 // Converts a HttpHeaders dictionary to a |name|, |value| pair. Returns
 // true if successful.
 bool FromHeaderDictionary(const DictionaryValue* header_value,
@@ -416,6 +752,8 @@ bool ExtensionWebRequestEventRouter::ExtraInfoSpec::InitFromValue(
       *extra_info_spec |= BLOCKING;
     else if (str == "asyncBlocking")
       *extra_info_spec |= ASYNC_BLOCKING;
+    else if (str == "requestPostData")
+      *extra_info_spec |= REQUEST_POST_DATA;
     else
       return false;
 
@@ -500,6 +838,9 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest(
     ListValue args;
     DictionaryValue* dict = new DictionaryValue();
     ExtractRequestInfo(request, dict);
+    if (request->method() == "POST" &&
+        extra_info_spec | ExtraInfoSpec::REQUEST_POST_DATA)

[battre, 2012/07/06 07:36:24]

this should probably be extra_info_spec & ...

I would reverse the order of the check.

[vabr (Chromium), 2012/07/06 14:09:01]

Done. I agree that reversing the order could be a nice speed-up, thanks for
pointing that out.

+      ExtractRequestInfoPost(request, dict);
     args.Append(dict);
 
     initialize_blocked_requests |=