[fsp] Fix incorrect handling of file system URLs when containing a %.

webkit/common/fileapi/file_system_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 "webkit/common/fileapi/file_system_util.h"
 
 #include <algorithm>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ skipping 436 matching lines @@
 }
 
 std::string GetIsolatedFileSystemRootURIString(
     const GURL& origin_url,
     const std::string& filesystem_id,
     const std::string& optional_root_name) {
   std::string root = GetFileSystemRootURI(origin_url,
                                           kFileSystemTypeIsolated).spec();
   if (base::FilePath::FromUTF8Unsafe(filesystem_id).ReferencesParent())
     return std::string();
-  root.append(filesystem_id);
+  root.append(net::EscapePath(filesystem_id));

[kinaba, 2014/06/06 05:40:11]

Maybe EscapeQueryParamValue is more appropriate?

According to the comment, net::EscapePath doesn't escape '/'.

[mtomasz, 2014/06/06 05:45:37]

Mount point names must not contain "/", per:
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/fil...

However, there is no restriction on the problematic %. So I believe, we don't
need to escape "/" here, since it is not allowed at all.

Note, that file system provider escapes "/" when generating the mount point
name:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

However, it seems that escaping "%" and "." in mount_path_util.cc is completely
redundant, and we should just escape "/" there.

Yet another solution is to remove the restriction on "/" in the mount point
name, and use EscapeQueryParamValue here, instead of EscapePath.

WDYT?

[kinaba, 2014/06/06 06:10:47]

Ah, thanks, I hope I've got your point.
So, the assumption is that the strings touched in this patch (filesystem_id,
mount_name)
are already avoiding '/' in some way (actually by escaping by '%2F'),
and what you want to handle here is the '%' characters here, right?


On 2014/06/06 05:45:37, mtomasz wrote:
If you want to treat ".." as a valid id in fileSystemProvider,
you still may want to escape dots. Otherwise the ReferencesParent() in
the line above will reject it.

[mtomasz, 2014/06/06 06:14:43]

You're right. In mount_path_util.cc, "." need to be escaped, as well as "%",
since otherwise we would have a collision:
%25-Wine and .-Wine both would resolve to %25-Wine.

   root.append("/");
   if (!optional_root_name.empty()) {
     if (base::FilePath::FromUTF8Unsafe(optional_root_name).ReferencesParent())
       return std::string();
-    root.append(optional_root_name);
+    root.append(net::EscapePath(optional_root_name));
     root.append("/");
   }
   return root;
 }
 
 std::string GetExternalFileSystemRootURIString(
     const GURL& origin_url,
     const std::string& mount_name) {
   std::string root = GetFileSystemRootURI(origin_url,
                                           kFileSystemTypeExternal).spec();
   if (base::FilePath::FromUTF8Unsafe(mount_name).ReferencesParent())
     return std::string();
-  root.append(mount_name);
+  root.append(net::EscapePath(mount_name));
   root.append("/");
   return root;
 }
 
 base::File::Error NetErrorToFileError(int error) {
   switch (error) {
     case net::OK:
       return base::File::FILE_OK;
     case net::ERR_ADDRESS_IN_USE:
       return base::File::FILE_ERROR_IN_USE;
@@ skipping 17 matching lines @@
       return base::File::FILE_ERROR_ABORT;
     case net::ERR_ADDRESS_INVALID:
     case net::ERR_INVALID_URL:
       return base::File::FILE_ERROR_INVALID_URL;
     default:
       return base::File::FILE_ERROR_FAILED;
   }
 }
 
 }  // namespace fileapi