Be explicit about target type in platform_util::OpenItem()

chrome/browser/chromeos/file_manager/open_util.cc

 // Copyright (c) 2012 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/chromeos/file_manager/open_util.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 109 matching lines @@
     }
   }
 
   ExecuteFileTaskForUrl(profile, chosen_task->task_descriptor(), url);
   callback.Run(true);
 }
 
 // Opens the file specified by |url| by finding and executing a file task for
 // the file. In case of success, calls |callback| with true. Otherwise the
 // returned value is false.
-void OpenFile(Profile* profile,
-              const base::FilePath& path,
-              const GURL& url,
-              const base::Callback<void(bool)>& callback) {
+void OpenFileInternal(Profile* profile,
+                      const base::FilePath& path,
+                      const GURL& url,
+                      const base::Callback<void(bool)>& callback) {
   extensions::app_file_handler_util::GetMimeTypeForLocalPath(
       profile,
       path,
       base::Bind(&OpenFileWithMimeType, profile, path, url, callback));
 }
 
 // Called when execution of ContinueOpenItem() is completed.
-void OnContinueOpenItemCompleted(Profile* profile,
+void OnContinueOpenFileCompleted(Profile* profile,
                                  const base::FilePath& file_path,
                                  bool result) {
   if (!result) {
     int message;
     if (file_path.Extension() == FILE_PATH_LITERAL(".dmg"))
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE_FOR_DMG;
     else if (file_path.Extension() == FILE_PATH_LITERAL(".exe") ||
              file_path.Extension() == FILE_PATH_LITERAL(".msi"))
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE_FOR_EXECUTABLE;
     else
       message = IDS_FILE_BROWSER_ERROR_VIEWING_FILE;
     ShowWarningMessageBox(profile, file_path, message);
   }
 }
 
-// Used to implement OpenItem().
-void ContinueOpenItem(Profile* profile,
+void ContinueOpenFile(Profile* profile,
                       const base::FilePath& file_path,
                       const GURL& url,
-                      base::File::Error error) {
+                      base::File::Error folder_check_result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (error == base::File::FILE_OK) {
-    // A directory exists at |url|. Open it with the file manager.
-    OpenFileManagerWithInternalActionId(profile, url, "open");
-  } else {
-    // |url| should be a file. Open it.
-    OpenFile(profile,
-             file_path,
-             url,
-             base::Bind(&OnContinueOpenItemCompleted, profile, file_path));
+  if (folder_check_result == base::File::FILE_OK) {
+    OnContinueOpenFileCompleted(profile, file_path, false);
+    return;
   }
+  OpenFileInternal(
+      profile,
+      file_path,
+      url,
+      base::Bind(&OnContinueOpenFileCompleted, profile, file_path));
+}
+
+void ContinueOpenFolder(Profile* profile,
+                        const GURL& url,
+                        base::File::Error folder_check_result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (folder_check_result != base::File::FILE_OK)
+    return;
+  OpenFileManagerWithInternalActionId(profile, url, "open");

[hashimoto, 2014/09/26 06:12:43]

Sorry, I should have noticed this before.
Does this really prevent inappropriate files from being opened?

I think the directory at the path can be replaced with a file with the same name
at a timing between CheckIfDirectoryExists() and the path gets actually opened.
Maybe we should leave Chrome OS code unchanged as it doesn't allow executable
files running after all.
Though, it's still possible to open an arbitrary file to launch the associated
app. (should we also be worried about this problem?)

[hashimoto, 2014/09/26 06:37:20]

hirono@ said OpenFileManagerWithInternalActionId() never launches apps other
than Files.app itself. (thanks hirono@!)
So it seems what we should do is to drop CheckIfDirectoryExists() and just call
OpenFileManagerWithInternalActionId() directly from OpenFolder().
Also, OpenFile() can be changed to skip directory existence check too.

[asanka, 2014/09/26 19:35:41]

Thanks! Done.

 }
 
 // Converts the |given_path| passed from external callers to the form that the
 // file manager can correctly handle. It first migrates old Drive/Download
 // folder path to the new formats, and then converts path to filesystem URL.
 //
 // When conversion fails, it shows a warning dialog UI and returns false.
 bool ConvertPath(Profile* profile,
                  const base::FilePath& given_path,
                  base::FilePath* path,
@@ skipping 19 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   OpenFileManagerWithInternalActionId(profile, url, "auto-open");
 }
 
-void OpenItem(Profile* profile, const base::FilePath& file_path) {
+void OpenFile(Profile* profile, const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
+  GURL url;
+  if (!ConvertPath(profile, file_path, &converted_path, &url))
+    return;
+
+  CheckIfDirectoryExists(
+      GetFileSystemContextForExtensionId(profile, kFileManagerAppId),
+      url,
+      base::Bind(&ContinueOpenFile, profile, converted_path, url));
+}
+
+void OpenFolder(Profile* profile, const base::FilePath& file_path) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   CheckIfDirectoryExists(
       GetFileSystemContextForExtensionId(profile, kFileManagerAppId),
       url,
-      base::Bind(&ContinueOpenItem, profile, converted_path, url));
+      base::Bind(&ContinueOpenFolder, profile, url));
 }
 
 void ShowItemInFolder(Profile* profile, const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   base::FilePath converted_path;
   GURL url;
   if (!ConvertPath(profile, file_path, &converted_path, &url))
     return;
 
   // This action changes the selection so we do not reuse existing tabs.
   OpenFileManagerWithInternalActionId(profile, url, "select");
 }
 
 }  // namespace util
 }  // namespace file_manager