Add HTTPMultipartBuilder and its test.

util/net/http_multipart_builder.cc

+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/net/http_multipart_builder.h"
+
+#include <vector>
+
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+#include "util/net/http_body.h"
+
+namespace crashpad {
+
+namespace {
+
+// Generates a random string suitable for use as a multipart boundary.
+std::string GenerateBoundaryString() {
+  // RFC 2046 §5.1.1 says that the boundary string may be 1 to 70 characters
+  // long, choosing from the set of alphanumeric characters along with
+  // characters from the set “'()+_,-./:=? ”, and not ending in a space.
+  // However, some servers have been observed as dealing poorly with certain
+  // nonalphanumeric characters. See
+  // blink/Source/platform/network/FormDataBuilder.cpp
+  // blink::FormDataBuilder::generateUniqueBoundaryString().
+  //
+  // This implementation produces a 56-character string with over 190 bits of
+  // randomness (62^32 > 2^190).
+  std::string boundary_string = "---MultipartBoundary-";
+  for (int index = 0; index < 32; ++index) {
+    const char kCharacters[] =
+        "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
+    int random_value = base::RandGenerator(strlen(kCharacters));
+    boundary_string += kCharacters[random_value];
+  }
+  boundary_string += "---";
+  return boundary_string;
+}
+
+// Escapes the specified name to be suitable for the name field of a
+// form-data part.
+std::string EncodeFieldName(const std::string& name) {
+  // RFC 2388 §3 says to encode non-ASCII field names according to RFC 2047, but
+  // no browsers implement that behavior. Instead, they send field names in the
+  // page hosting the form’s encoding. However, some form of escaping is needed.
+  // This URL-escapes the quote character and newline characters, per Blink. See
+  // blink/Source/platform/network/FormDataBuilder.cpp
+  // blink::appendQuotedString().
+  //
+  // TODO(mark): This encoding is not necessarily correct, and the same code in
+  // Blink is marked with a FIXME. Blink does not escape the '%' character,
+  // that’s a local addition, but it seems appropriate to be able to decode the
+  // string properly.
+  std::string encoded;
+  for (char character : name) {
+    switch (character) {
+      case '\r':
+      case '\n':
+      case '"':
+      case '%':
+        encoded += base::StringPrintf("%%%02x", character);
+        break;
+      default:
+        encoded += character;
+        break;
+    }
+  }
+
+  return encoded;
+}
+
+const char kBoundaryCRLF[] = "\r\n\r\n";

[Mark Mentovai, 2014/10/29 21:29:13]

Can you move this above or below the function definitions so that it stands out
on its own a little bit more?

Actually, it’s only used in GetBodyStream(), so maybe you can just move it to
the top of that method.

[Robert Sesek, 2014/10/29 21:47:53]

Done.

+
+// Returns a string, formatted with a multipart boundary and a field name,
+// after which the contents of the part at |name| can be appended.
+std::string GetFormDataBoundary(const std::string& boundary,
+                                const std::string& name) {
+  return base::StringPrintf(
+      "--%s\r\nContent-Disposition: form-data; name=\"%s\"",
+      boundary.c_str(),
+      EncodeFieldName(name).c_str());
+}
+
+}  // namespace
+
+HTTPMultipartBuilder::HTTPMultipartBuilder()
+    : boundary_(GenerateBoundaryString()), form_data_(), file_attachments_() {
+}
+
+HTTPMultipartBuilder::~HTTPMultipartBuilder() {
+}
+
+void HTTPMultipartBuilder::SetFormData(const std::string& key,
+                                       const std::string& value) {
+  EraseKey(key);
+  form_data_[key] = value;
+}
+
+void HTTPMultipartBuilder::SetFileAttachment(
+    const std::string& key,
+    const std::string& upload_file_name,
+    const base::FilePath& path,
+    const std::string& content_type) {
+  EraseKey(upload_file_name);
+
+  FileAttachment attachment;
+  attachment.filename = upload_file_name;
+  attachment.content_type = content_type;
+  if (attachment.content_type.empty())

[Mark Mentovai, 2014/10/29 21:29:14]

Since you need the if anyway, I’d write this as

  if (content_type.empty())
    attachment.content_type = "application/octet-stream";
  else
    attachment.content_type = content_type;

or perhaps with a ternary, which is the most space-savingest option of all

  attachment.content_type =
      content_type.empty() ? "application/octet-stream" : content_type;

or consider giving FileAttachment a constructor so you can just write

  FileAttachment attachment(upload_file_name, content_type, path);

[Robert Sesek, 2014/10/29 21:47:53]

Done.

[Robert Sesek, 2014/10/29 21:47:53]

Done.

+    attachment.content_type = "application/octet-stream";
+  attachment.path = path;
+
+  file_attachments_[key] = attachment;
+}
+
+scoped_ptr<HTTPBodyStream> HTTPMultipartBuilder::GetBodyStream() {
+  // The objects inserted into this vector will be owned by the returned
+  // CompositeHTTPBodyStream. Take care to not early-return without deleting
+  // this memory.
+  std::vector<HTTPBodyStream*> streams;
+
+  for (const auto& pair : form_data_) {
+    std::string field = GetFormDataBoundary(boundary(), pair.first);
+    field += kBoundaryCRLF;
+    field += pair.second;
+    field += "\r\n";

[Mark Mentovai, 2014/10/29 21:29:14]

Since you have kBoundaryCRLF for a pair of CRLFs, maybe you want a const char[]
for a single CRLF too. You use it three times in this function.

[Robert Sesek, 2014/10/29 21:47:53]

Done.

+    streams.push_back(new StringHTTPBodyStream(field));
+  }
+
+  for (const auto& pair : file_attachments_) {
+    const FileAttachment& attachment = pair.second;
+    std::string header = GetFormDataBoundary(boundary(), pair.first);
+    header += base::StringPrintf("; filename=\"%s\"\r\n",
+        attachment.filename.c_str());

[Mark Mentovai, 2014/10/29 21:29:13]

Encode me with EncodeFieldName(). Blink does this too. Maybe rename
EncodeFieldName() to reflect that it’s more generic than just field names.

[Robert Sesek, 2014/10/29 21:47:53]

Done.

+    header += base::StringPrintf("Content-Type: %s%s",
+        attachment.content_type.c_str(), kBoundaryCRLF);

[Mark Mentovai, 2014/10/29 21:29:14]

Not checking Content-Type for bad characters gives me the heebie-jeebies.

Could you add a CHECK that makes sure that the Content-Type isn’t fishy? I guess
I’d base fishiness on the definition of “token” from RFC 2045 §5.1, except that
we’d also include “/” in the set of accepted characters for obvious reasons. You
could use find_first_not_of() to look for bad characters.

I’d place the CHECK here, in SetFileAttachment(), or in the FileAttachment
constructor (if you add one).

[Robert Sesek, 2014/10/29 21:47:53]

I don't see the point of checking for "fishyness". The field is either valid or
invalid, and just checking for invalid characters seems like a half-baked
solution. For example, if you used any one of the "tspecials", then this
validation would also need to ensure that it's properly quoted. You could also
specify allays/ibf-x which would be totally invalid but not "fishy". Fully
validating Content-Type seems pointless, since it's really up to the server to
decide what to accept.

[Mark Mentovai, 2014/10/29 22:46:55]

Robert Sesek wrote:
My concern is something setting Content-Type to something like

"application/pdf\r\n\r\n%PDFBlahBlahBlahExploitCommonPDFParserVulnerabilityBlahHereIsSomeTrailingGarbage"

which, if anyone ever uses this code along with some OS-level extension-to-MIME
type mapper, would be totally plausible and outside of our control.

I think we should detect this with a strict CHECK.

allays/ibf-x would be valid if we checked this as I proposed—we’d want all of
the legal characters for “token” plus “/”. I agree that it’s the server’s
responsibility to decide what to do with this. But I don’t agree that we should
leave ourselves open to the possibility of being hijacked and taking us out of
control of what the server actually sees as the intended “part.”

[Robert Sesek, 2014/10/29 23:12:04]

Per offline discussion, limiting to [a-zA-Z0-9-_./]

+
+    streams.push_back(new StringHTTPBodyStream(header));
+    streams.push_back(new FileHTTPBodyStream(attachment.path));
+    streams.push_back(new StringHTTPBodyStream("\r\n"));
+  }
+
+  streams.push_back(new StringHTTPBodyStream(boundary() + "--"));

[Mark Mentovai, 2014/10/29 21:29:13]

I would also end this with a CRLF even if no standard strictly requires it.
blink::FormDataBuilder::addBoundaryToMultiPartHeader() does this too.

[Mark Mentovai, 2014/10/29 21:29:14]

No leading “--” here too? I think it needs it.

RFC 2046 §5.1.1 (“The boundary delimiter line following the last body part…”)
http://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2

[Robert Sesek, 2014/10/29 21:47:53]

Done.

[Robert Sesek, 2014/10/29 21:47:53]

Done.

+
+  return scoped_ptr<HTTPBodyStream>(new CompositeHTTPBodyStream(streams));
+}
+
+void HTTPMultipartBuilder::EraseKey(const std::string& key) {
+  auto data_it = form_data_.find(key);
+  if (data_it != form_data_.end())
+    form_data_.erase(data_it);
+
+  auto file_it = file_attachments_.find(key);
+  if (file_it != file_attachments_.end())
+    file_attachments_.erase(file_it);
+}
+
+}  // namespace crashpad