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..33e758b9116fdbb4ce04f288f32b3d263d47538c 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"
@@ -73,6 +74,8 @@ static const char* const kWebRequestEvents[] = {
 
 #define ARRAYEND(array) (array + arraysize(array))
 
+const char kContentDisposition[] = "Content-Disposition:";
+
 // Returns the frame ID as it will be passed to the extension:
 // 0 if the navigation happens in the main frame, or the frame ID
 // modulo 32 bits otherwise.
@@ -169,6 +172,361 @@ 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 {

[battre, 2012/07/11 10:59:50]

could you move all of this new code to
c/b/e/api/web_request/post_data_parser.{h,cc}?

[vabr (Chromium), 2012/07/12 15:13:11]

Done, except for ExtractRequestInfoPost which I feel belongs here.

+ public:
+  struct Result {
+    base::StringPiece key;
+    base::StringPiece val;
+  };
+  // Sets the |length| bytes at |source| as the data to be parsed.
+  // Returns true on success.

[battre, 2012/07/11 10:59:50]

There is no |length| anymore.

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  virtual bool SetSource(const std::vector<char>& source) = 0;

[battre, 2012/07/11 10:59:50]

I think you should pass a const std::vector<char>* to demonstrate that the class
may keep a pointer to |source|. My assumption of this function definition would
be that |source| needs to be valid for the life time of the PostDataParser. You
should also document that life time requirements of |source|.

[battre, 2012/07/11 10:59:50]

What do you think of making SetSource part of the constructor rather than a
separate function?

[vabr (Chromium), 2012/07/12 15:13:11]

That would not work, as SetSource is called multiple times (see
PostDataParsetMultipart).

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  // 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;
+
+ protected:
+  PostDataParser() {}

[battre, 2012/07/11 10:59:50]

You need to declare a virtual destructor.

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

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

[battre, 2012/07/11 10:59:50]

Can you add a reference to the RFC according to which this data is parsed? What
do you think of de-uuencoding the data returns by GetNextPair?

[vabr (Chromium), 2012/07/12 15:13:11]

This is not covered in a RFC, but it is described in the HTML standard:
http://www.w3.org/TR/REC-html40-971218/interact/forms.html#h-17.13.4.1.
Decoding sounds good to me, once adding the write-support, we will have to
encode back anyway. I had to modify the PostDataParser::Result to make this
possible (up to now Result was only holding a pointer to original data, making
it impossible to alter that).

+ public:
+  PostDataParserUrlEncoded() : source_(NULL) {}
+  ~PostDataParserUrlEncoded() {}

[battre, 2012/07/11 10:59:50]

virtual

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  // Implementation of PostDataParser.
+  virtual bool SetSource(const std::vector<char>& source) OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool AllDataReadOK() OVERRIDE;
+
+ private:
+  // We parse the first |length_| bytes from |source_|.

[battre, 2012/07/11 10:59:50]

There is no |length_| anymore.

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  const std::vector<char>* source_;
+  std::vector<char>::const_iterator offset_;
+  DISALLOW_COPY_AND_ASSIGN(PostDataParserUrlEncoded);
+};
+
+bool PostDataParserUrlEncoded::AllDataReadOK() {

[battre, 2012/07/11 10:59:50]

nit: the function order should match the function order in the class definition.

[vabr (Chromium), 2012/07/12 15:13:11]

Done, converted all to alphabetical order.

+  return source_ != NULL && offset_ == source_->end();
+}
+
+bool PostDataParserUrlEncoded::SetSource(const std::vector<char>& source) {
+  if (source_ != NULL)
+    return false;
+  source_ = &source;
+  offset_ = source_->begin();
+  return true;
+}
+
+bool PostDataParserUrlEncoded::GetNextPair(Result* result) {
+  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;
+    return false;
+  }
+  result->key.set(&(*offset_), seek - offset_); // Safe, see (*).
+  offset_ = ++seek;
+  while (seek != source_->end() && *seek != '&')
+    ++seek;
+  result->val.set(&(*offset_), seek - offset_); // Safe, see (*).
+  offset_ = seek == source_->end() ? seek : seek+1;

[battre, 2012/07/11 10:59:50]

nit: spaces around +
opt: I think I would put parentheses around the (seek == ... + 1)

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  return true;
+}
+
+class PostDataParserMultipart : public PostDataParser {

[battre, 2012/07/11 10:59:50]

Can you mention the RFC according to which this parses? Is this RFC2388?

[vabr (Chromium), 2012/07/12 15:13:11]

Done. Yes.

+ public:
+  explicit PostDataParserMultipart(const std::string& boundary)
+      : source_(NULL),
+        boundary_(boundary),
+        state_(kInit) {}
+  ~PostDataParserMultipart() {}

[battre, 2012/07/11 10:59:50]

virtual

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  // Implementation of PostDataParser.
+  virtual bool SetSource(const std::vector<char>& source) OVERRIDE;
+  virtual bool GetNextPair(Result* result) OVERRIDE;
+  virtual bool AllDataReadOK() OVERRIDE;
+
+ private:
+  // 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};

[battre, 2012/07/11 10:59:50]

Can you expand these names? We prefer to have no abbreviations in Chrome. ->
kBoundary, kFinalBoundary, kContentDisposition, kEmpty, kOther

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  // 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 std::vector<char>& source) {
+  if (state_ == kError)
+    return false;
+  if (source_ != NULL && next_line_ < length_)
+    return false;
+  source_ = &(source[0]);
+  length_ = source.size();
+  next_line_ = 0;
+  return true;
+}
+
+bool PostDataParserMultipart::GetNextPair(Result* result) {
+  if (state_ == kError)
+    return false;
+  while (state_ != kSkip) {
+    if (!DoStep())
+      return false;
+  }
+  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.data()| != NULL already.
+  if (!DoStep())
+    return result->val.data() != NULL;
+  val_start = line_;
+  while (state_ != kFirst && state_ != kFinal) {
+    val_end = line_end_;
+    if (!DoStep()) break;
+  }
+  if (name_parsed && result->val.data() == NULL) {
+    // This pair is from non-file form item, |val| is on subsequent lines.
+    result->val.set(source_ + val_start, 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_;

[battre, 2012/07/11 10:59:50]

How about this?

StringPiece line = (source_ + line_, line_end_ - line_); // maybe line_ becomes
line_start_.
bool boundary_test = line.starts_with("--" + boundary_);

alternatively:

if (line == "--" + bounary_)
  line_type = kBound;
else if (line == "--" + boundary_ + "--")
  line_type_ = kFBound;

instead of calculating "--" + bounadry_ and "--" + boundary_ + "--" each time
you may store them in the class.

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  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, kContentDisposition, strlen(kContentDisposition)) == 0)
+    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_;

[battre, 2012/07/11 10:59:50]

could please you split variable declarations and assignment into separate lines
here?

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  while (seek < length_ && *(source_ + seek) != '\r')
+    ++seek;
+  line_end_ = seek;
+  GetLineType();
+  if (seek < length_ && *(source_ + seek + 1) != '\n')

[battre, 2012/07/11 10:59:50]

should this be if (seek + 1 < length_ ...) ?

[vabr (Chromium), 2012/07/12 15:13:11]

No, it should equal to the first part of the condition in the while-cycle above.

+    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;
+      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) {

[battre, 2012/07/11 10:59:50]

You can verify the contract via
DCHECK_EQ(kDisp, line_type_)

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  std::string line(source_ + line_, line_end_-line_);

[battre, 2012/07/11 10:59:50]

nit: space around -

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+  size_t key_offset = line.find(" name=\"") + 7;

[battre, 2012/07/11 10:59:50]

what happens if find returns std::string::npos?

[vabr (Chromium), 2012/07/12 15:13:11]

Corrected to:
1. Check against npos.
2. Increment if offset != npos.

+  if (key_offset == std::string::npos)

[battre, 2012/07/11 10:59:50]

won't be the case because of the +7

[vabr (Chromium), 2012/07/12 15:13:11]

Solved, see above.

+    return false;
+  result->key.set(source_ + line_ + key_offset,
+                  line.find('"', key_offset) - key_offset);
+  size_t val_offset = line.find(" filename=\"");
+  if (val_offset == std::string::npos) {
+    result->val.set(NULL);
+  } else {
+    val_offset += 11;
+    result->val.set(source_ + line_ + val_offset,
+                    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 {

[battre, 2012/07/11 10:59:50]

Would it not be simpler to write a scoped_ptr<PostDataParser>
CreatePostDataParser(net::URLRequest* request) function?

[vabr (Chromium), 2012/07/12 15:13:11]

Done.

+ public:
+  PostDataParserProxy() : parser_(NULL) {}
+  ~PostDataParserProxy() {
+    if (parser_ != NULL)
+      delete parser_;

[battre, 2012/07/11 10:59:50]

The better way to do this is using a scoped_ptr

[vabr (Chromium), 2012/07/12 15:13:11]

Done via replacing the whole parser-proxy thing with the scoped_ptr.

+  }
+  // Chooses the parser based on content-type of the request. True == success.
+  bool Init(net::URLRequest* request);
+  virtual bool SetSource(const std::vector<char>& source) 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 std::vector<char>& source) {
+  return parser_ != NULL ? parser_->SetSource(source) : false;
+}
+
+bool PostDataParserProxy::GetNextPair(Result* result) {
+  if (parser_ != NULL)
+    return parser_->GetNextPair(result);
+  else
+    return false;
+}
+
+// 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;
+}
+
+// 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);
+  scoped_ptr<DictionaryValue> post_data(new DictionaryValue());
+  std::vector<net::UploadData::Element>::const_iterator element;
+  for (element = elements->begin(); element != elements->end(); ++element) {
+    if (element->type() != net::UploadData::TYPE_BYTES) continue;
+    if (!parser.SetSource(element->bytes())) continue;
+    PostDataParser::Result result;
+    while (parser.GetNextPair(&result)) {
+      GetOrCreateList(post_data.get(), result.key.as_string())->Append(
+          new StringValue(result.val.as_string()));
+    }
+  }
+  if (parser.AllDataReadOK())
+    out->Set(keys::kPostDataKey, post_data.release());
+}
+
 // Converts a HttpHeaders dictionary to a |name|, |value| pair. Returns
 // true if successful.
 bool FromHeaderDictionary(const DictionaryValue* header_value,
@@ -416,6 +774,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 +860,9 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest(
     ListValue args;
     DictionaryValue* dict = new DictionaryValue();
     ExtractRequestInfo(request, dict);
+    if (extra_info_spec & ExtraInfoSpec::REQUEST_POST_DATA &&
+        request->method() == "POST")
+      ExtractRequestInfoPost(request, dict);
     args.Append(dict);
 
     initialize_blocked_requests |=