Stop returning the true root path of each filesystem from openFileSystem....

webkit/fileapi/file_system_path_manager.cc

Index: webkit/fileapi/file_system_path_manager.cc
===================================================================
--- webkit/fileapi/file_system_path_manager.cc	(revision 77587)
+++ webkit/fileapi/file_system_path_manager.cc	(working copy)
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -17,6 +17,8 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebCString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFileSystem.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"

[kinuko, 2011/03/14 11:03:57]

line 17,19 not needed

[ericu, 2011/03/15 02:43:11]

Done.

+#include "webkit/fileapi/file_system_util.h"
+#include "webkit/fileapi/sandbox_mount_point_provider.h"
 #include "webkit/glue/webkit_glue.h"
 
 // We use some of WebKit types for conversions between origin identifiers
@@ -27,158 +29,22 @@
 
 using base::PlatformFileError;
 
-namespace fileapi {
-
-const FilePath::CharType FileSystemPathManager::kFileSystemDirectory[] =
-    FILE_PATH_LITERAL("FileSystem");
-
-const char FileSystemPathManager::kPersistentName[] = "Persistent";
-const char FileSystemPathManager::kTemporaryName[] = "Temporary";
-
-static const FilePath::CharType kFileSystemUniqueNamePrefix[] =
-    FILE_PATH_LITERAL("chrome-");
-static const int kFileSystemUniqueLength = 16;
-static const unsigned kFileSystemUniqueDirectoryNameLength =
-    kFileSystemUniqueLength + arraysize(kFileSystemUniqueNamePrefix) - 1;
-
-namespace {
-
-// Restricted names.
-// http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#naming-restrictions
-static const char* const kRestrictedNames[] = {
-  "con", "prn", "aux", "nul",
-  "com1", "com2", "com3", "com4", "com5", "com6", "com7", "com8", "com9",
-  "lpt1", "lpt2", "lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9",
-};
-
-// Restricted chars.
-static const FilePath::CharType kRestrictedChars[] = {
-  '/', '\\', '<', '>', ':', '?', '*', '"', '|',
-};
-
-inline std::string FilePathStringToASCII(
-    const FilePath::StringType& path_string) {
-#if defined(OS_WIN)
-  return WideToASCII(path_string);
-#elif defined(OS_POSIX)
-  return path_string;
-#endif
-}
-
-FilePath::StringType CreateUniqueDirectoryName(const GURL& origin_url) {
-  // This can be anything but need to be unpredictable.
-  static const FilePath::CharType letters[] = FILE_PATH_LITERAL(
-    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789");
-  FilePath::StringType unique(kFileSystemUniqueNamePrefix);
-  for (int i = 0; i < kFileSystemUniqueLength; ++i)
-    unique += letters[base::RandInt(0, arraysize(letters) - 2)];
-  return unique;
-}
-
 static const char kExtensionScheme[] = "chrome-extension";
 
-}  // anonymous namespace
+namespace fileapi {
 
-class FileSystemPathManager::GetFileSystemRootPathTask
-    : public base::RefCountedThreadSafe<
-        FileSystemPathManager::GetFileSystemRootPathTask> {
- public:
-  GetFileSystemRootPathTask(
-      scoped_refptr<base::MessageLoopProxy> file_message_loop,
-      const std::string& name,
-      FileSystemPathManager::GetRootPathCallback* callback)
-      : file_message_loop_(file_message_loop),
-        origin_message_loop_proxy_(
-            base::MessageLoopProxy::CreateForCurrentThread()),
-        name_(name),
-        callback_(callback) {
-  }
-
-  void Start(const GURL& origin_url,
-             const FilePath& origin_base_path,
-             bool create) {
-    file_message_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-        &GetFileSystemRootPathTask::GetFileSystemRootPathOnFileThread,
-        origin_url, origin_base_path, create));
-  }
-
- private:
-  void GetFileSystemRootPathOnFileThread(
-      const GURL& origin_url,
-      const FilePath& base_path,
-      bool create) {
-    FilePath root;
-    if (ReadOriginDirectory(base_path, origin_url, &root)) {
-      DispatchCallbackOnCallerThread(root);
-      return;
-    }
-
-    if (!create) {
-      DispatchCallbackOnCallerThread(FilePath());
-      return;
-    }
-
-    // Creates the root directory.
-    root = base_path.Append(CreateUniqueDirectoryName(origin_url));
-    if (!file_util::CreateDirectory(root)) {
-      DispatchCallbackOnCallerThread(FilePath());
-      return;
-    }
-    DispatchCallbackOnCallerThread(root);
-  }
-
-  bool ReadOriginDirectory(const FilePath& base_path,
-                           const GURL& origin_url,
-                           FilePath* unique) {
-    file_util::FileEnumerator file_enum(
-        base_path, false /* recursive */,
-        file_util::FileEnumerator::DIRECTORIES,
-        FilePath::StringType(kFileSystemUniqueNamePrefix) +
-            FILE_PATH_LITERAL("*"));
-    FilePath current;
-    bool found = false;
-    while (!(current = file_enum.Next()).empty()) {
-      if (current.BaseName().value().length() !=
-          kFileSystemUniqueDirectoryNameLength)
-        continue;
-      if (found) {
-        // TODO(kinuko): Should notify the user to ask for some action.
-        LOG(WARNING) << "Unexpectedly found more than one FileSystem "
-                     << "directories for " << origin_url;
-        return false;
-      }
-      found = true;
-      *unique = current;
-    }
-    return !unique->empty();
-  }
-
-  void DispatchCallbackOnCallerThread(const FilePath& root_path) {
-    origin_message_loop_proxy_->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &GetFileSystemRootPathTask::DispatchCallback,
-                          root_path));
-  }
-
-  void DispatchCallback(const FilePath& root_path) {
-    callback_->Run(!root_path.empty(), root_path, name_);
-    callback_.reset();
-  }
-
-  scoped_refptr<base::MessageLoopProxy> file_message_loop_;
-  scoped_refptr<base::MessageLoopProxy> origin_message_loop_proxy_;
-  std::string name_;
-  scoped_ptr<FileSystemPathManager::GetRootPathCallback> callback_;
-};
-
 FileSystemPathManager::FileSystemPathManager(
     scoped_refptr<base::MessageLoopProxy> file_message_loop,
     const FilePath& profile_path,
     bool is_incognito,
     bool allow_file_access_from_files)
-    : file_message_loop_(file_message_loop),
-      base_path_(profile_path.Append(kFileSystemDirectory)),
-      is_incognito_(is_incognito),
-      allow_file_access_from_files_(allow_file_access_from_files) {
+    : is_incognito_(is_incognito),
+      allow_file_access_from_files_(allow_file_access_from_files),
+      sandbox_provider_(
+          new SandboxMountPointProvider(
+              ALLOW_THIS_IN_INITIALIZER_LIST(this),
+              file_message_loop,
+              profile_path)) {
 }
 
 FileSystemPathManager::~FileSystemPathManager() {}
@@ -186,95 +52,86 @@
 void FileSystemPathManager::GetFileSystemRootPath(
     const GURL& origin_url, fileapi::FileSystemType type,
     bool create, GetRootPathCallback* callback_ptr) {
-  scoped_ptr<GetRootPathCallback> callback(callback_ptr);
-  if (is_incognito_) {
-    // TODO(kinuko): return an isolated temporary directory.
-    callback->Run(false, FilePath(), std::string());
-    return;
-  }
 
-  if (!IsAllowedScheme(origin_url)) {
-    callback->Run(false, FilePath(), std::string());
-    return;
+  switch (type) {
+  case kFileSystemTypeTemporary:
+  case kFileSystemTypePersistent:
+    sandbox_provider_->GetFileSystemRootPath(
+        origin_url, type, create, callback_ptr);
+    break;
+  case kFileSystemTypeUnknown:
+  default:
+    NOTREACHED();
+    callback_ptr->Run(false, FilePath(), std::string());
   }
+}
 
-  std::string origin_identifier = GetOriginIdentifierFromURL(origin_url);
-  FilePath origin_base_path = GetFileSystemBaseDirectoryForOriginAndType(
-      base_path(), origin_identifier, type);
-  if (origin_base_path.empty()) {
-    callback->Run(false, FilePath(), std::string());
-    return;
+FilePath FileSystemPathManager::GetFileSystemRootPathOnFileThread(
+    const GURL& origin_url, FileSystemType type, bool create) {
+  switch (type) {
+  case kFileSystemTypeTemporary:
+  case kFileSystemTypePersistent:
+    return sandbox_provider_->GetFileSystemRootPathOnFileThread(
+        origin_url, type, create);
+    break;
+  case kFileSystemTypeUnknown:
+  default:
+    NOTREACHED();
+    return FilePath();
   }
-
-  std::string type_string = GetFileSystemTypeString(type);
-  DCHECK(!type_string.empty());
-
-  scoped_refptr<GetFileSystemRootPathTask> task(
-      new GetFileSystemRootPathTask(file_message_loop_,
-                                    origin_identifier + ":" + type_string,
-                                    callback.release()));
-  task->Start(origin_url, origin_base_path, create);
 }
 
 bool FileSystemPathManager::CrackFileSystemPath(
     const FilePath& path, GURL* origin_url, FileSystemType* type,
     FilePath* virtual_path) const {
-  // Any paths that includes parent references are considered invalid.
-  if (path.ReferencesParent())
-    return false;
+  // TODO(ericu):
+  // Paths come in here [for now] as a URL, followed by a virtual path in
+  // platform format.  For example, on Windows, this will look like
+  // filesystem:http://www.example.com/temporary/\path\to\file.txt.
+  // A potentially dangerous malicious path on Windows might look like:
+  // filesystem:http://www.example.com/temporary/foo/../../\path\to\file.txt.
+  // This code is ugly, but will get cleaned up as we fix the calling side.
+  // We should be passing these to WebKit as string, not FilePath, for ease of
+  // manipulation, or possibly as GURL/KURL.
 
-  // The path should be a child of the profile FileSystem path.
-  FilePath relative;
-  if (!base_path_.AppendRelativePath(path, &relative))
-    return false;
+  std::string path_as_string;
+#ifdef OS_WIN
+  path_as_string = WideToUTF8(path.value());
+#else
+  path_as_string = path.value();
+#endif
+  GURL path_as_url(path_as_string);
 
-  // The relative path from the profile FileSystem path should contain
-  // at least three components, one for storage identifier, one for type
-  // and one for the 'unique' part.
-  std::vector<FilePath::StringType> components;
-  relative.GetComponents(&components);
-  if (components.size() < 3)
+  FilePath local_path;
+  GURL local_url;
+  FileSystemType local_type;
+  if (!CrackFileSystemURL(path_as_url, &local_url, &local_type, &local_path))
     return false;
 
-  // The second component of the relative path to the root directory
-  // must be kPersistent or kTemporary.
-  if (!IsStringASCII(components[1]))
-    return false;
+#if defined(FILE_PATH_USES_WIN_SEPARATORS)
+  // TODO(ericu): This puts the separators back to windows-standard; they come
+  // out of the above code as '/' no matter the platform.  Long-term, we'll
+  // want to let the underlying FileSystemFileUtil implementation do this part,
+  // since they won't all need it.
+  local_path = local_path.NormalizeWindowsPathSeparators();
+#endif
 
-  std::string ascii_type_component = FilePathStringToASCII(components[1]);
-  FileSystemType cracked_type = kFileSystemTypeUnknown;
-  if (ascii_type_component == kPersistentName)
-    cracked_type = kFileSystemTypePersistent;
-  else if (ascii_type_component == kTemporaryName)
-    cracked_type = kFileSystemTypeTemporary;
-  else
-    return false;
+  // Any paths that include parent references are considered invalid.
+  // These should have been taken care of in CrackFileSystemURL.
+  DCHECK(!local_path.ReferencesParent());

[kinuko, 2011/03/14 11:03:57]

Can't we get rid of these checks (line 111 to 121) as we seem to be doing the
same check in CrackFileSystemURL?

[ericu, 2011/03/15 02:43:11]

111->117 fixes up separators that are the wrong way [for Windows] when they come
out of CrackFSURL.

119->121 just makes sure that the work /was/ done.  I'd like to leave that in
for now, since I'm going to go in and clean up the underlying call [as
documented in the TODOs], and I want to have lots of checks in to make sure I
didn't break anything.

[kinuko, 2011/03/16 19:33:38]

Ok... can we add some comments about that?  (A short one is enough, just to
avoid confusion why we have similar code in multiple places) 

[ericu, 2011/03/16 23:47:31]

Added a small addendum to the upper comment in this function.  All the todos in
both functions say that the code is going away anyway; I hope that's clear
enough for now.

 
-  DCHECK(cracked_type != kFileSystemTypeUnknown);
-
-  // The given |path| seems valid. Populates the |origin_url|, |type|
+  // The given |local_path| seems valid. Populates the |origin_url|, |type|
   // and |virtual_path| if they are given.
 
   if (origin_url) {
-    WebSecurityOrigin web_security_origin =
-        WebSecurityOrigin::createFromDatabaseIdentifier(
-            webkit_glue::FilePathStringToWebString(components[0]));
-    *origin_url = GURL(web_security_origin.toString());
-
-    // We need this work-around for file:/// URIs as
-    // createFromDatabaseIdentifier returns empty origin_url for them.
-    if (allow_file_access_from_files_ && origin_url->spec().empty() &&
-        components[0].find(FILE_PATH_LITERAL("file")) == 0)
-      *origin_url = GURL("file:///");
+    *origin_url = local_url;
   }
 
   if (type)
-    *type = cracked_type;
+    *type = local_type;
 
   if (virtual_path) {
-    virtual_path->clear();
-    for (size_t i = 3; i < components.size(); ++i)
-      *virtual_path = virtual_path->Append(components[i]);
+    *virtual_path = local_path;
   }
 
   return true;
@@ -289,89 +146,29 @@
 }
 
 // static
-bool FileSystemPathManager::IsRestrictedFileName(const FilePath& filename) {
-  if (filename.value().empty())
-    return false;
-
-  if (IsWhitespace(filename.value()[filename.value().size() - 1]) ||
-      filename.value()[filename.value().size() - 1] == '.')
-    return true;
-
-  std::string filename_lower = StringToLowerASCII(
-      FilePathStringToASCII(filename.value()));
-
-  for (size_t i = 0; i < arraysize(kRestrictedNames); ++i) {
-    // Exact match.
-    if (filename_lower == kRestrictedNames[i])
-      return true;
-    // Starts with "RESTRICTED_NAME.".
-    if (filename_lower.find(std::string(kRestrictedNames[i]) + ".") == 0)
-      return true;
-  }
-
-  for (size_t i = 0; i < arraysize(kRestrictedChars); ++i) {
-    if (filename.value().find(kRestrictedChars[i]) !=
-        FilePath::StringType::npos)
-      return true;
-  }
-
-  return false;
-}
-
-// static
 std::string FileSystemPathManager::GetFileSystemTypeString(
     fileapi::FileSystemType type) {
   if (type == fileapi::kFileSystemTypeTemporary)
-    return fileapi::FileSystemPathManager::kTemporaryName;
+    return fileapi::SandboxMountPointProvider::kTemporaryName;
   else if (type == fileapi::kFileSystemTypePersistent)
-    return fileapi::FileSystemPathManager::kPersistentName;
+    return fileapi::SandboxMountPointProvider::kPersistentName;
   return std::string();
 }
 
-// static
-std::string FileSystemPathManager::GetOriginIdentifierFromURL(
-    const GURL& url) {
-  WebKit::WebSecurityOrigin web_security_origin =
-      WebKit::WebSecurityOrigin::createFromString(UTF8ToUTF16(url.spec()));
-  return web_security_origin.databaseIdentifier().utf8();
-}
-
-// static
-FilePath FileSystemPathManager::GetFileSystemBaseDirectoryForOriginAndType(
-    const FilePath& base_path, const std::string& origin_identifier,
-    fileapi::FileSystemType type) {
-  if (origin_identifier.empty())
-    return FilePath();
-  std::string type_string = GetFileSystemTypeString(type);
-  if (type_string.empty()) {
-    LOG(WARNING) << "Unknown filesystem type is requested:" << type;
-    return FilePath();
+// Checks if a given |name| contains any restricted names/chars in it.
+bool FileSystemPathManager::IsRestrictedFileName(
+    FileSystemType type, const FilePath& filename) {
+  switch (type) {
+  case kFileSystemTypeTemporary:
+  case kFileSystemTypePersistent:
+    return sandbox_provider_->IsRestrictedFileName(filename);
+  case kFileSystemTypeUnknown:
+  default:
+    NOTREACHED();
+    return true;
   }
-  return base_path.AppendASCII(origin_identifier)
-                  .AppendASCII(type_string);
 }
 
-FileSystemPathManager::OriginEnumerator::OriginEnumerator(
-    const FilePath& base_path)
-    : enumerator_(base_path, false /* recursive */,
-                  file_util::FileEnumerator::DIRECTORIES) {
-}
-
-std::string FileSystemPathManager::OriginEnumerator::Next() {
-  current_ = enumerator_.Next();
-  return FilePathStringToASCII(current_.BaseName().value());
-}
-
-bool FileSystemPathManager::OriginEnumerator::HasTemporary() {
-  return !current_.empty() && file_util::DirectoryExists(current_.AppendASCII(
-      FileSystemPathManager::kTemporaryName));
-}
-
-bool FileSystemPathManager::OriginEnumerator::HasPersistent() {
-  return !current_.empty() && file_util::DirectoryExists(current_.AppendASCII(
-      FileSystemPathManager::kPersistentName));
-}
-
 }  // namespace fileapi
 
 COMPILE_ASSERT(int(WebFileSystem::TypeTemporary) == \