Re-enable DownloadsApiTest.Downloads

chrome/browser/download/download_extension_api.cc

 // Copyright (c) 2011 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/download/download_extension_api.h"
 
 #include <algorithm>
 #include <cctype>
 #include <iterator>
 #include <set>
@@ skipping 96 matching lines @@
     case DownloadItem::COMPLETE: return kStateComplete;
     case DownloadItem::INTERRUPTED:  // fall through
     case DownloadItem::CANCELLED: return kStateInterrupted;
     case DownloadItem::REMOVING:  // fall through
     default:
       NOTREACHED();
       return "";
   }
 }
 
+bool ValidateFilename(const string16& filename) {

[benjhayden, 2012/01/09 15:22:49]

It strikes me that a more robust, perhaps equally simple method would be to
concat the DownloadsFolder pref with this filename, evaluate '..'s but not
symlinks, and check to see that the resulting filename begins with
DownloadsFolder. You can either try implementing that here, or add it to my
todo, or don't worry about it.

+  // TODO(benjhayden): More robust validation of filename.
+  if (filename.size() >= 2u && filename[0] == L'.' && filename[1] == L'.')
+    return false;
+
+  if (filename.size() >= 1u && filename[0] == L'/')
+    return false;
+
+  return true;
+}
+
 }  // namespace
 
 bool DownloadsFunctionInterface::RunImplImpl(
     DownloadsFunctionInterface* pimpl) {
   CHECK(pimpl);
   if (!pimpl->ParseArgs()) return false;
   UMA_HISTOGRAM_ENUMERATION(
       "Download.ApiFunctions", pimpl->function(), DOWNLOADS_FUNCTION_LAST);
   return pimpl->RunInternal();
 }
@@ skipping 52 matching lines @@
   base::DictionaryValue* options = NULL;
   std::string url;
   iodata_.reset(new IOData());
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &options));
   EXTENSION_FUNCTION_VALIDATE(options->GetString(kUrlKey, &url));
   iodata_->url = GURL(url);
   if (!iodata_->url.is_valid()) {
     error_ = download_extension_errors::kInvalidURLError;
     return false;
   }
-  if (options->HasKey(kFilenameKey))
+
+  if (options->HasKey(kFilenameKey)) {
     EXTENSION_FUNCTION_VALIDATE(options->GetString(
         kFilenameKey, &iodata_->filename));
-  // TODO(benjhayden): More robust validation of filename.
-  if (((iodata_->filename[0] == L'.') && (iodata_->filename[1] == L'.')) ||
-      (iodata_->filename[0] == L'/')) {
-    error_ = download_extension_errors::kGenericError;
-    return false;
+    if (!ValidateFilename(iodata_->filename)) {
+      error_ = download_extension_errors::kGenericError;
+      return false;
+    }
   }
-  if (options->HasKey(kSaveAsKey))
+
+  if (options->HasKey(kSaveAsKey)) {

[benjhayden, 2012/01/09 15:22:49]

I thought that the style guide forbade braces around single-statement blocks.

[cbentzel, 2012/01/09 19:16:41]

Not if the body spans multiple lines. 

     EXTENSION_FUNCTION_VALIDATE(options->GetBoolean(
         kSaveAsKey, &iodata_->save_as));
-  if (options->HasKey(kMethodKey))
+  }
+
+  if (options->HasKey(kMethodKey)) {
     EXTENSION_FUNCTION_VALIDATE(options->GetString(
         kMethodKey, &iodata_->method));
+  }
+
   // It's ok to use a pointer to extra_headers without DeepCopy()ing because
   // |args_| (which owns *extra_headers) is guaranteed to live as long as
   // |this|.
-  if (options->HasKey(kHeadersKey))
+  if (options->HasKey(kHeadersKey)) {
     EXTENSION_FUNCTION_VALIDATE(options->GetList(
         kHeadersKey, &iodata_->extra_headers));
-  if (options->HasKey(kBodyKey))
+  }
+
+  if (options->HasKey(kBodyKey)) {
     EXTENSION_FUNCTION_VALIDATE(options->GetString(
         kBodyKey, &iodata_->post_body));
+  }
+
   if (iodata_->extra_headers != NULL) {
     for (size_t index = 0; index < iodata_->extra_headers->GetSize(); ++index) {
       base::DictionaryValue* header = NULL;
       std::string name, value;
       EXTENSION_FUNCTION_VALIDATE(iodata_->extra_headers->GetDictionary(
             index, &header));
       EXTENSION_FUNCTION_VALIDATE(header->GetString(
             kHeaderNameKey, &name));
       EXTENSION_FUNCTION_VALIDATE(header->GetString(
             kHeaderValueKey, &value));
@@ skipping 378 matching lines @@
   ListValue args;
   args.Append(arg);
   std::string json_args;
   base::JSONWriter::Write(&args, false, &json_args);
   profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
       event_name,
       json_args,
       profile_,
       GURL());
 }