Files.app: Stop to use file system URLs having externalfile:// scheme origin internally.

Files.app: Stop to use file system URLs having externalfile:// scheme origin internally.

Priviously in the external file protocol handler, for accessing files via file
system API, we uses file system URLs having externalfile:// scheme origin
internally.  And we added special check to pass the origin in IsAccessAllowed
check.

This CL removes the special check. Instead the CL generated isolated file system
URL to access files.

BUG=367027
TEST=unit_tests --gtest_filter=*ExternalFileURL*

Committed: https://crrev.com/1a815bc356ca4e4e387870b2cd953030ae6647ba
Cr-Commit-Position: refs/heads/master@{#297361}

[hirono, 2014-09-29 12:29:26]

hirono@chromium.org changed reviewers:
+ mtomasz@chromium.org

[hirono, 2014-09-29 12:29:26]

I found a way to remove externalfile origin check from IsAccessAllowed.
PTAL the CL? Thank you!

[mtomasz, 2014-09-29 12:30:02]

On 2014/09/29 12:29:26, hirono wrote:
> I found a way to remove externalfile origin check from IsAccessAllowed.
> PTAL the CL? Thank you!

nit: intarnally -> internally

[mtomasz, 2014-09-29 12:47:40]

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:66: GURL(""),
nit: Maybe just GURL()?

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:314:
file_system_url_, offset, size, base::Time());
Could you briefly explain how it works? We're passing an isolated url, right?
How does it get get a file stream reader for drive then? I'm not very sure how
isolated file systems work under the hood. How do we skip the permission check?

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:43:
file_system_url.virtual_path().AsUTF8Unsafe().c_str()));
Sorry for not catching earlier, but does this properly escape characters such as
? & and so on?

[hirono, 2014-09-29 13:24:02]

Thank you!

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:66: GURL(""),
On 2014/09/29 12:47:40, mtomasz wrote:
> nit: Maybe just GURL()?

Done.

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:314:
file_system_url_, offset, size, base::Time());
On 2014/09/29 12:47:40, mtomasz wrote:
> Could you briefly explain how it works? We're passing an isolated url, right?
> How does it get get a file stream reader for drive then? I'm not very sure how
> isolated file systems work under the hood. How do we skip the permission
check?

CreateFileStreamReader basically refers two members. file_system_url.path() and
file_system_url.mount_type();

path() refers the location of file.
It is set correctly when the file system URL is cracked (#65) by referring
isolated context's mount point.

mount_type() is always kFileSystemTypeIsolated. It is checked by IsAccessAllowed
and IsAccessAllowed always returns true for kFileSystemTypeIsolated.

So basically the CL just replaces the magic word to pass the check
(kExternalFileScheme) with kFileSystemTypeIsolated. But kFileSystemTypeIsolated
is harder to abuse than kExternalFileScheme, because isolated URLs (mount point
name part) is unpredictable.

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:43:
file_system_url.virtual_path().AsUTF8Unsafe().c_str()));
On 2014/09/29 12:47:40, mtomasz wrote:
> Sorry for not catching earlier, but does this properly escape characters such
as
> ? & and so on?

Yes we needed to this. It seems an issue from the drive scheme URL.
Actually we can open files having such name, because url.GetContent at #50
returns the raw string of URL.

One problem I found is URL is not shown correctly in the location bar after
clicking <a href="#hash">Link</a> if the file name includes '#'.

Let me handle the issue in the separated CL.

[mtomasz, 2014-09-29 13:35:45]

lgtm from me, but please ask @tzik for the final approval, since I may not be
aware of all security implications by using the isolated context.

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/615523002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:314:
file_system_url_, offset, size, base::Time());
On 2014/09/29 13:24:02, hirono wrote:
> On 2014/09/29 12:47:40, mtomasz wrote:
> > Could you briefly explain how it works? We're passing an isolated url,
right?
> > How does it get get a file stream reader for drive then? I'm not very sure
how
> > isolated file systems work under the hood. How do we skip the permission
> check?
> 
> CreateFileStreamReader basically refers two members. file_system_url.path()
and
> file_system_url.mount_type();
> 
> path() refers the location of file.
> It is set correctly when the file system URL is cracked (#65) by referring
> isolated context's mount point.
> 
> mount_type() is always kFileSystemTypeIsolated. It is checked by
IsAccessAllowed
> and IsAccessAllowed always returns true for kFileSystemTypeIsolated.
> 
> So basically the CL just replaces the magic word to pass the check
> (kExternalFileScheme) with kFileSystemTypeIsolated. But
kFileSystemTypeIsolated
> is harder to abuse than kExternalFileScheme, because isolated URLs (mount
point
> name part) is unpredictable.

Got it. Thanks for the explanation.

[hirono, 2014-09-29 14:44:53]

hirono@chromium.org changed reviewers:
+ tzik@chromium.org

[hirono, 2014-09-29 14:44:54]

PTAL the CL? Thank you!

[tzik, 2014-09-30 04:00:27]

lgtm

[hirono, 2014-09-30 04:43:26]

The CQ bit was checked by hirono@chromium.org

[hirono, 2014-09-30 04:43:48]

On 2014/09/30 04:00:27, tzik wrote:
> lgtm

Thank you!

[commit-bot: I haz the power, 2014-09-30 04:46:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615523002/20001

[commit-bot: I haz the power, 2014-09-30 04:57:25]

Committed patchset #2 (id:20001) as bbc77b2b60532dea4457a76f05fcba210d745f9a

[commit-bot: I haz the power, 2014-09-30 04:58:17]

Patchset 2 (id:??) landed as
https://crrev.com/1a815bc356ca4e4e387870b2cd953030ae6647ba
Cr-Commit-Position: refs/heads/master@{#297361}