Don't change the extension for downloads that already have benign extensions.

We still append extensions to extensionless downloads, and do some extension rewriting around dangerous extensions, but don't try to be smart about renaming, e.g., .txt to .zip

BUG=57080
TEST=download_util_unittest

Review URL: http://codereview.chromium.org/3453027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61070 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 13 insertions, 71 deletions

diff --git a/chrome/browser/download/download_exe.cc b/chrome/browser/download/download_exe.cc
index 69ce1c5..2ceab73 100644
--- a/chrome/browser/download/download_exe.cc
+++ b/chrome/browser/download/download_exe.cc
@@ -198,6 +198,7 @@ static const char* kExecutableWhiteList[] = {
   // JavaScript is just as powerful as EXE.
   "text/javascript",
   "text/javascript;version=*",
+  "text/html",
   // Registry files can cause critical changes to the MS OS behavior.
   // Addition of this mimetype also addresses bug 7337.
   "text/x-registry",
diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc
index 5747e51..8ced2b4 100644
--- a/chrome/browser/download/download_util.cc
+++ b/chrome/browser/download/download_util.cc
@@ -156,15 +156,6 @@ void GenerateExtension(const FilePath& file_name,
     extension.assign(default_extension);
 #endif
 
-  std::string mime_type_from_extension;
-  net::GetMimeTypeFromFile(file_name,
-                           &mime_type_from_extension);
-  if (mime_type == mime_type_from_extension) {
-    // The hinted extension matches the mime type.  It looks like a winner.
-    generated_extension->swap(extension);
-    return;
-  }
-
   if (IsExecutableExtension(extension) && !IsExecutableMimeType(mime_type)) {
     // We want to be careful about executable extensions.  The worry here is
     // that a trusted web site could be tricked into dropping an executable file
@@ -176,34 +167,8 @@ void GenerateExtension(const FilePath& file_name,
     }
   }
 
-  if (extension.empty()) {
+  if (extension.empty())
     net::GetPreferredExtensionForMimeType(mime_type, &extension);
-  } else {
-    // Append extension generated from the mime type if:
-    // 1. New extension is not ".txt"
-    // 2. New extension is not the same as the already existing extension.
-    // 3. New extension is not executable. This action mitigates the case when
-    //    an executable is hidden in a benign file extension;
-    //    E.g. my-cat.jpg becomes my-cat.jpg.js if content type is
-    //         application/x-javascript.
-    // 4. New extension is not ".tar" for .tar.gz/tgz files. For misconfigured
-    //    web servers, i.e. bug 5772.
-    // 5. The original extension is not ".tgz" & the new extension is not "gz".
-    FilePath::StringType append_extension;
-    if (net::GetPreferredExtensionForMimeType(mime_type, &append_extension)) {
-      if (append_extension != FILE_PATH_LITERAL("txt") &&
-          append_extension != extension &&
-          !IsExecutableExtension(append_extension) &&
-          !(append_extension == FILE_PATH_LITERAL("gz") &&
-            extension == FILE_PATH_LITERAL("tgz")) &&
-          !(append_extension == FILE_PATH_LITERAL("tar") &&
-           (extension == FILE_PATH_LITERAL("tar.gz") ||
-            extension == FILE_PATH_LITERAL("tgz")))) {
-        extension += FILE_PATH_LITERAL(".");
-        extension += append_extension;
-      }
-    }
-  }
 
   generated_extension->swap(extension);
 }
diff --git a/chrome/browser/download/download_util_unittest.cc b/chrome/browser/download/download_util_unittest.cc
index 158256b..d257174 100644
--- a/chrome/browser/download/download_util_unittest.cc
+++ b/chrome/browser/download/download_util_unittest.cc
@@ -68,10 +68,6 @@ const struct {
    "application/octet-stream",
    L"My Downloaded File.exe"},
 
-  // This block tests whether we append extensions based on MIME types;
-  // we don't do this on Linux, so we skip the tests rather than #ifdef
-  // them up.
-#if !defined(OS_POSIX) || defined(OS_MACOSX)
   {"filename=my-cat",
    "http://www.example.com/my-cat",
    "image/jpeg",
@@ -93,7 +89,6 @@ const struct {
    "http://www.example.com/my-cat",
    "dance/party",
    L"my-cat"},
-#endif  // !defined(OS_POSIX) || defined(OS_MACOSX)
 
   {"filename=my-cat.jpg",
    "http://www.example.com/my-cat.jpg",
@@ -356,22 +351,10 @@ const struct {
 #endif
   },
 
-  {"filename=source.srf",
-   "http://www.hotmail.com",
-   "image/jpeg",
-   L"source.srf" JPEG_EXT
-  },
-
   {"filename=source.jpg",
    "http://www.hotmail.com",
    "application/x-javascript",
-#if defined(OS_WIN)
-   L"source.jpg"
-#elif defined(OS_MACOSX)
-   L"source.jpg.js"
-#else
    L"source.jpg"
-#endif
   },
 
   // NetUtilTest.{GetSuggestedFilename, GetFileNameFromCD} test these
@@ -437,7 +420,7 @@ const struct {
   {"",
    "http://www.example.com/bar.bogus",
    "application/x-tar",
-   L"bar.bogus" TAR_EXT
+   L"bar.bogus"
   },
 
   // http://code.google.com/p/chromium/issues/detail?id=20337
@@ -451,12 +434,12 @@ const struct {
 // (content-disposition, URL name, etc) don't cause security holes.
 TEST(DownloadUtilTest, GenerateFileName) {
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
-  // This test doesn't run when the locale is not UTF-8 becuase some of the
+  // This test doesn't run when the locale is not UTF-8 because some of the
   // string conversions fail. This is OK (we have the default value) but they
   // don't match our expectations.
   std::string locale = setlocale(LC_CTYPE, NULL);
   StringToLowerASCII(&locale);
-  ASSERT_NE(std::string::npos, locale.find("utf-8"))
+  EXPECT_NE(std::string::npos, locale.find("utf-8"))
       << "Your locale must be set to UTF-8 for this test to pass!";
 #endif
 
@@ -468,7 +451,7 @@ TEST(DownloadUtilTest, GenerateFileName) {
                                     kGenerateFileNameTestCases[i].mime_type,
                                     &generated_name);
     EXPECT_EQ(kGenerateFileNameTestCases[i].expected_name,
-              generated_name.ToWStringHack());
+              generated_name.ToWStringHack()) << i;
   }
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kGenerateFileNameTestCases); ++i) {
@@ -479,7 +462,7 @@ TEST(DownloadUtilTest, GenerateFileName) {
                                     kGenerateFileNameTestCases[i].mime_type,
                                     &generated_name);
     EXPECT_EQ(kGenerateFileNameTestCases[i].expected_name,
-              generated_name.ToWStringHack());
+              generated_name.ToWStringHack()) << i;
   }
 
   // A couple of cases with raw 8bit characters in C-D.
@@ -529,14 +512,14 @@ const struct {
 
   { FILE_PATH_LITERAL("C:\\foo\\bar.exe"),
     "text/html",
-    FILE_PATH_LITERAL("C:\\foo\\bar.htm") },
+    FILE_PATH_LITERAL("C:\\foo\\bar.exe") },
   { FILE_PATH_LITERAL("C:\\foo\\bar.exe"),
     "image/gif",
     FILE_PATH_LITERAL("C:\\foo\\bar.gif") },
 
   { FILE_PATH_LITERAL("C:\\foo\\google.com"),
     "text/html",
-    FILE_PATH_LITERAL("C:\\foo\\google.htm") },
+    FILE_PATH_LITERAL("C:\\foo\\google.com") },
 
   { FILE_PATH_LITERAL("C:\\foo\\con.htm"),
     "text/html",
@@ -557,29 +540,22 @@ const struct {
 
   { FILE_PATH_LITERAL("/bar.html"),
     "image/png",
-    FILE_PATH_LITERAL("/bar.html.png") },
+    FILE_PATH_LITERAL("/bar.html") },
   { FILE_PATH_LITERAL("/bar"),
     "image/png",
     FILE_PATH_LITERAL("/bar.png") },
 
   { FILE_PATH_LITERAL("/foo/bar.exe"),
-    "text/html",
-#if defined(OS_MACOSX)
-    FILE_PATH_LITERAL("/foo/bar.exe.html") },
-#else
-    FILE_PATH_LITERAL("/foo/bar.html") },
-#endif
-  { FILE_PATH_LITERAL("/foo/bar.exe"),
     "image/gif",
 #if defined(OS_MACOSX)
-    FILE_PATH_LITERAL("/foo/bar.exe.gif") },
+    FILE_PATH_LITERAL("/foo/bar.exe") },
 #else
     FILE_PATH_LITERAL("/foo/bar.gif") },
 #endif
 
   { FILE_PATH_LITERAL("/foo/google.com"),
     "text/html",
-    FILE_PATH_LITERAL("/foo/google.com.html") },
+    FILE_PATH_LITERAL("/foo/google.com") },
 
   { FILE_PATH_LITERAL("/foo/con.htm"),
     "text/html",
@@ -594,7 +570,7 @@ TEST(DownloadUtilTest, GenerateSafeFileName) {
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kSafeFilenameCases); ++i) {
     FilePath path(kSafeFilenameCases[i].path);
     download_util::GenerateSafeFileName(kSafeFilenameCases[i].mime_type, &path);
-    EXPECT_EQ(kSafeFilenameCases[i].expected_path, path.value());
+    EXPECT_EQ(kSafeFilenameCases[i].expected_path, path.value()) << i;
   }
 }