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

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

Mount point names can (and in case of File System Provider API often do) the %
character, in order to create a safe mount point name from an arbitrary file
system id, which can be any string, and often is a file name path. Such path
may contain /, which have to be escaped, since the mount point name must not
contain that character.

However, GetExternalFileSystemRootURIString() wasn't properly escaping the %
character, what caused treating it later as an encoding sequence of an url.

TEST=Tested manually with a file systems containing % in the mount point name.
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276002

[mtomasz, 2014-06-05 09:32:42]

@ericu: PTAL as OWNER.
@kinaba: PTAL.

[kinaba, 2014-06-06 05:40:12]

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
webkit/common/fileapi/file_system_util.cc:457:
root.append(net::EscapePath(filesystem_id));
Maybe EscapeQueryParamValue is more appropriate?

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

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

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
webkit/common/fileapi/file_system_util.cc:457:
root.append(net::EscapePath(filesystem_id));
On 2014/06/06 05:40:11, kinaba wrote:
> Maybe EscapeQueryParamValue is more appropriate?
> 
> According to the comment, net::EscapePath doesn't escape '/'.

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]

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
webkit/common/fileapi/file_system_util.cc:457:
root.append(net::EscapePath(filesystem_id));
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:
> However, it seems that escaping "%" and "." in mount_path_util.cc is
completely
> redundant, and we should just escape "/" there.

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]

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
File webkit/common/fileapi/file_system_util.cc (right):

https://codereview.chromium.org/312283002/diff/1/webkit/common/fileapi/file_s...
webkit/common/fileapi/file_system_util.cc:457:
root.append(net::EscapePath(filesystem_id));
On 2014/06/06 06:10:47, kinaba wrote:
> 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:
> > However, it seems that escaping "%" and "." in mount_path_util.cc is
> completely
> > redundant, and we should just escape "/" there.
> 
> 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.

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.

[kinaba, 2014-06-06 06:51:03]

lgtm (I forgot to write this magic word)

[mtomasz, 2014-06-06 06:54:32]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-06-06 06:56:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/312283002/1

[commit-bot: I haz the power, 2014-06-06 07:17:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-06 07:21:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 07:21:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[ericu, 2014-06-06 21:36:43]

This change needs a regression test.  We've had far too many escaping bugs in
the various paths through filesystem URL code; we need to make sure none of them
come back.

[mtomasz, 2014-06-09 06:23:24]

On 2014/06/06 21:36:43, ericu wrote:
> This change needs a regression test.  We've had far too many escaping bugs in
> the various paths through filesystem URL code; we need to make sure none of
them
> come back.

Added regression tests. PTAL. Thanks.

[ericu, 2014-06-09 16:40:03]

lgtm

[mtomasz, 2014-06-09 23:01:50]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-06-09 23:04:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/312283002/20001

[mtomasz, 2014-06-10 04:28:11]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-06-10 04:28:16]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-06-10 04:28:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/312283002/20001

[commit-bot: I haz the power, 2014-06-10 12:52:00]

Change committed as 276002