Re-enable DownloadsApiTest.Downloads

chrome/browser/download/download_extension_api.cc

Index: chrome/browser/download/download_extension_api.cc
diff --git a/chrome/browser/download/download_extension_api.cc b/chrome/browser/download/download_extension_api.cc
index f551c94df974b34c647111137df3c2f4d3735fcb..0fdfb6139ceaf49c90384f34a37076e8c3f09e5f 100644
--- a/chrome/browser/download/download_extension_api.cc
+++ b/chrome/browser/download/download_extension_api.cc
@@ -114,6 +114,17 @@ const char* StateString(DownloadItem::DownloadState state) {
   }
 }
 
+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(
@@ -186,30 +197,39 @@ bool DownloadsDownloadFunction::ParseArgs() {
     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;