Fix userVisibility for files in ChromeOS filesystems.

Fix userVisibility for files in ChromeOS filesystems.

In Web applications, the Filesystem API is used to create sandboxed
filesystems. Files in these filesystems should never be shown in a
filepicker. ChromeOS supports a few special filesystems (Downloads,
Gallery) where the files should be shown in filepickers.

This change fixes the userVisibility property for File instances created
from such filesystems.

BUG=398366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181530

[pwnall-personal, 2014-08-28 04:38:06]

Patchset #1 (id:1) has been deleted

[pwnall-personal, 2014-08-28 08:33:09]

costan@gmail.com changed reviewers:
+ mtomasz@chromium.org, tkent@chromium.org

[pwnall-personal, 2014-08-28 08:40:18]

After some digging around, I think this is the right missing piece neeeded to
fix https://crbug.com/398366

Fortunately, the API used by Files.app (chrome.fileBrowserPrivate) vends
Filesystem instances of the type External, whereas Web apps only use Temporary /
Persistent filesystems. I tried this out in a Chromium build with "chromeos=1",
and drag-and-drop works from Files.app to both the extensions page (triggering
an install for .crx files) and a normal <input type="file"> (the file picker
works as it should when invoked). 

Unfortunately, I'm not sure how to test this automatically. Do you have any
thoughts?
 
Thank you very much!

[mtomasz, 2014-08-29 00:43:55]

mtomasz@chromium.org changed reviewers:
- mtomasz@chromium.org

[mtomasz, 2014-08-29 00:44:53]

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

[mtomasz, 2014-08-29 00:44:53]

@pwnall: I'm not familiar with Blink code base. Please find a better reviewer.

[mtomasz, 2014-08-29 01:07:13]

mtomasz@chromium.org changed reviewers:
+ sigbjornf@opera.com

[mtomasz, 2014-08-29 01:08:28]

Adding sigbjornf@ per git blame.

[pwnall-personal, 2014-08-29 03:43:05]

On 2014/08/29 00:44:53, mtomasz wrote:
> @pwnall: I'm not familiar with Blink code base. Please find a better reviewer.

I saw your name in Files.app and in the chrome.fileBrowserPrivate code. I was
hoping can help me understand if the FileSystemTypeExternal check is correct.
I'm relying on the fact that Web apps filesystems are Persistent / Temporary,
and it seems like only ChromeOS code uses External. I'm pretty sure
FileSystemTypeExternal maps to kFileSystemTypeExternal in
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/file...

The userVisibility flag is a concept that I introduced when I broke Files.app.
It's supposed to distinguish between files that the user would actually browse
to using their file picker, and files that they wouldn't. Right now, the
distinction is:
* files sourced using the <input type="file"> file picker are userVisible, 
because the user had to browse to them to select them
* files created using the Filesystem API are not userVisible, because they're
created in some place hidden inside Chrome's internals
* files created using the constructor in the File API are not visible, they do
not exist on any filesystem

Based on what I learned, it seems like Files.app uses the Filesystem API. It
receives filesystem instances from the chrome.fileBrowserPrivate API. Since
Files.app is the file browser on ChromeOS, I think that Files handled by it
should be userVisible. Fortunately, the instance used by Downloads is of type
FileSystemTypeExternal, so I can use that to decide if File instances coming
from the Filesystem API should be userVisible or not. Does this seem reasonable?

I hope that Kent-san can review the Blink aspects of the code.

[tkent, 2014-09-01 02:45:39]

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

[tkent, 2014-09-01 02:45:39]

+tzik

I don't understand behavior of neither File System API nor Files.app, and can't
evaluate the correctness of this patch.
Anyway, this CL needs a test.  Maybe C++ unit test.

[tzik, 2014-09-01 05:14:05]

On 2014/08/29 03:43:05, pwnall wrote:
> On 2014/08/29 00:44:53, mtomasz wrote:
> > @pwnall: I'm not familiar with Blink code base. Please find a better
reviewer.
> 
> I saw your name in Files.app and in the chrome.fileBrowserPrivate code. I was
> hoping can help me understand if the FileSystemTypeExternal check is correct.
> I'm relying on the fact that Web apps filesystems are Persistent / Temporary,
> and it seems like only ChromeOS code uses External. I'm pretty sure
> FileSystemTypeExternal maps to kFileSystemTypeExternal in
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/file...
> 
> The userVisibility flag is a concept that I introduced when I broke Files.app.
> It's supposed to distinguish between files that the user would actually browse
> to using their file picker, and files that they wouldn't. Right now, the
> distinction is:
> * files sourced using the <input type="file"> file picker are userVisible, 
> because the user had to browse to them to select them
> * files created using the Filesystem API are not userVisible, because they're
> created in some place hidden inside Chrome's internals
> * files created using the constructor in the File API are not visible, they do
> not exist on any filesystem
> 
> Based on what I learned, it seems like Files.app uses the Filesystem API. It
> receives filesystem instances from the chrome.fileBrowserPrivate API. Since
> Files.app is the file browser on ChromeOS, I think that Files handled by it
> should be userVisible. Fortunately, the instance used by Downloads is of type
> FileSystemTypeExternal, so I can use that to decide if File instances coming
> from the Filesystem API should be userVisible or not. Does this seem
reasonable?
> 
> I hope that Kent-san can review the Blink aspects of the code.

There is some other filesystems that are mounted as external but doesn't have
unsandboxed backing file: Drive, Syncable and others.
This CL seems not to work for them.
Also, I think files in sandboxed filesystem are not necessarily hidden in the
file picker, though it's not supported at this moment.

So my recommendation is adding |selectedFileSystemFile| to FileChooserSettings
to hold filesystem scheme URLs of selected files.
And let the filechooser implementation handle them.
in_reply_to: 5653164804014080
send_mail: 1
subject: Fix userVisibility for files in ChromeOS filesystems.

[pwnall-personal, 2014-09-02 23:58:59]

I added the userVisibility flag to handle situations like
https://crbug.com/360308

In this case, the <input type="file">'s value is set by JavaScript. The bug I
mentioned is a particularly contrived way of obtaining a FileList, but in the
future, it is likely that there will be a more direct way of setting the
<input>'s value.

The problem here is that if the user clicks the "Choose File" button, we have to
decide if the file picker will show to the path of the file that is the
<input>'s value, or if it should show some default path, like the user's desktop
/ home directory.

Before userVisibility was added, we always had the file picker show the path.
This was confusing for files produced via the Filesystem API, which are written
in sandbox directories inside the user's profile, with names like "00000000". I
thought that the file picker should not navigate to these files.

Also, given that drag-and-drop usually results in a copy operation that exposes
the initial filename, I thought that Filesystem API files should be represented
as URLs, not as paths, in a drag-and-drop operation. Otherwise, dragging a
filesystem file out of the renderer will create files named "00000000" /
"00000001" / etc.

On 2014/09/01 05:14:05, tzik wrote:
> There is some other filesystems that are mounted as external but doesn't have
> unsandboxed backing file: Drive, Syncable and others.
> This CL seems not to work for them.

Thank you very much for pointing this out! This is exactly the kind of
information that I was looking for!

> Also, I think files in sandboxed filesystem are not necessarily hidden in the
> file picker, though it's not supported at this moment.

I'm pretty sure that the user is able to browse to their profile in the File
picker (at least on Windows / OSX / Linux) and therefore select files that
belong to sandboxed filesystems. I am not concerned about this case, because the
user is deliberately navigating there.

I am concerned about a case like https://crbug.com/360308, where the user would
click "Choose File" and find themselves in the sandbox, without knowing why that
happened.

> So my recommendation is adding |selectedFileSystemFile| to FileChooserSettings
> to hold filesystem scheme URLs of selected files.
> And let the filechooser implementation handle them.

Based on the information about Drive / Syncable / etc., it seems like the right
solution is to have a separate flag that indicates whether a filesystem's files
are sandboxed or not. I think that'll require a bit of effort and a 2-sided
patch.

In the mean time, I would advocate for getting this patch in, to get M38
unblocked.

It seems like the patch will erroneously mark File instances coming from Drive
and Syncable as UserVisible. However, before userVisibility was added, all File
instances were in effect UserVisible. So, after this patch, we wouldn't be in a
worst state than in M37. At the same time, we would behave correctly in Web apps
that only use the Filesystem API in the W3C draft, and we would unblock
Files.app on ChromeOS.

Does this sound reasonable?

[tzik, 2014-09-03 07:57:09]

On 2014/09/02 23:58:59, pwnall wrote:
> I added the userVisibility flag to handle situations like
> https://crbug.com/360308
> 
> In this case, the <input type="file">'s value is set by JavaScript. The bug I
> mentioned is a particularly contrived way of obtaining a FileList, but in the
> future, it is likely that there will be a more direct way of setting the
> <input>'s value.
> 
> The problem here is that if the user clicks the "Choose File" button, we have
to
> decide if the file picker will show to the path of the file that is the
> <input>'s value, or if it should show some default path, like the user's
desktop
> / home directory.
> 
> Before userVisibility was added, we always had the file picker show the path.
> This was confusing for files produced via the Filesystem API, which are
written
> in sandbox directories inside the user's profile, with names like "00000000".
I
> thought that the file picker should not navigate to these files.
> 
> Also, given that drag-and-drop usually results in a copy operation that
exposes
> the initial filename, I thought that Filesystem API files should be
represented
> as URLs, not as paths, in a drag-and-drop operation. Otherwise, dragging a
> filesystem file out of the renderer will create files named "00000000" /
> "00000001" / etc.
> 
> On 2014/09/01 05:14:05, tzik wrote:
> > There is some other filesystems that are mounted as external but doesn't
have
> > unsandboxed backing file: Drive, Syncable and others.
> > This CL seems not to work for them.
> 
> Thank you very much for pointing this out! This is exactly the kind of
> information that I was looking for!
> 
> > Also, I think files in sandboxed filesystem are not necessarily hidden in
the
> > file picker, though it's not supported at this moment.
> 
> I'm pretty sure that the user is able to browse to their profile in the File
> picker (at least on Windows / OSX / Linux) and therefore select files that
> belong to sandboxed filesystems. I am not concerned about this case, because
the
> user is deliberately navigating there.
> 
> I am concerned about a case like https://crbug.com/360308, where the user
would
> click "Choose File" and find themselves in the sandbox, without knowing why
that
> happened.

Ah, I agree that the FilePicker should not dig into the profile directory, and
should not show an obfuscated path like "001/000001".
I meant that I don't oppose FilePicker to show the directory structure of a
sandboxed filesystem on it, just like you may see in
filesystem:http://localhost/temporary/.
That is, rather than determining UserVisibility by the FileSystemType, we should
pass FileSystemURL to the implementation of the FilePicker and let it determine
the visibility. 

> 
> > So my recommendation is adding |selectedFileSystemFile| to
FileChooserSettings
> > to hold filesystem scheme URLs of selected files.
> > And let the filechooser implementation handle them.
> 
> Based on the information about Drive / Syncable / etc., it seems like the
right
> solution is to have a separate flag that indicates whether a filesystem's
files
> are sandboxed or not. I think that'll require a bit of effort and a 2-sided
> patch.
> 
> In the mean time, I would advocate for getting this patch in, to get M38
> unblocked.
> 
> It seems like the patch will erroneously mark File instances coming from Drive
> and Syncable as UserVisible. However, before userVisibility was added, all
File
> instances were in effect UserVisible. So, after this patch, we wouldn't be in
a
> worst state than in M37. At the same time, we would behave correctly in Web
apps
> that only use the Filesystem API in the W3C draft, and we would unblock
> Files.app on ChromeOS.
> 
> Does this sound reasonable?

Well OK, that will be a lower hanging improvement.

LGTM.

[pwnall-personal, 2014-09-03 18:22:15]

On 2014/09/03 07:57:09, tzik wrote:
> Well OK, that will be a lower hanging improvement.
> 
> LGTM.

Thank you! I will try to understand more of the filesystem code and make some
progress on this soon.

Kent-san, can I get a LGTM for the File API change, please?

[tkent, 2014-09-04 04:46:29]

> Kent-san, can I get a LGTM for the File API change, please?

Please add a test.

[pwnall-personal, 2014-09-04 22:11:12]

On 2014/09/04 04:46:29, tkent wrote:
> > Kent-san, can I get a LGTM for the File API change, please?
> 
> Please add a test.

I started with FileSystemSync, because I figured it'd be easier to test the
synchronous case first. Unfortunately, I can't add a C++ test without heavy
mocking for blink::Platform.

The FileSystemSync callback (CreateFileHelper::didCreateSnapshotFile) that I
modified is in an anonymous namespace, so I can't reach it directly. I tried
creating a DOMFileSystemSync, then using DirectoryEntrySync::getFile() on its
root, then FileEntrySync::file() to get a File instance. Unfortunately, getFile
(which delegates to DOMFileSystemBase::getFile) uses
blink::Platform::fileSystem().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The blink::Platform used in tests does not have a WebFileSystem implementation.
I made a crude attempt to use the existing WebFileSystem implementation, but
that didn't work. I got a null FileEntrySync* from getFile().
https://codereview.chromium.org/545553003/

I think this happened because WebFileSystemImpl relies on being able to IPC with
other threads in Chrome.
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/file...
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/file...

Even if the approach above worked, it'd cause this patch to rely on a CL similar
to https://codereview.chromium.org/545553003/, which would make the patch
undesirable for cherry-picking into M38.

I'll look into adding a test for the FileSystemCallbacks change now, but I
expect to run into similar issues.

[pwnall-personal, 2014-09-05 09:12:45]

On 2014/09/04 22:11:12, pwnall wrote:
> I'll look into adding a test for the FileSystemCallbacks change now, but I
> expect to run into similar issues.

I was able to add a test for the FileSystemCallbacks diff. It is quite big (100
lines of test for a 2-line change), and only tests
SnapshotFileCallback::didCreateSnapshotFile (the method I changed). Testing
DOMFileSystem::getFile() would require blink::Platform::fileSystem(), just like
in the DOMFileSystemSync case.

What do you think of this approach, Kent-san?
If you like it, I can try to put together a similar test for DOMFileSystemSync,
which would require exporting CreateFileHelper in DOMFileSystemSync.cpp. I can
nest it under DOMFileSystemSync, so that it wouldn't pollute the blink
namespace.

[tkent, 2014-09-05 09:21:26]

On 2014/09/05 09:12:45, pwnall wrote:
> On 2014/09/04 22:11:12, pwnall wrote:
> > I'll look into adding a test for the FileSystemCallbacks change now, but I
> > expect to run into similar issues.
> 
> I was able to add a test for the FileSystemCallbacks diff. It is quite big
(100
> lines of test for a 2-line change), and only tests
> SnapshotFileCallback::didCreateSnapshotFile (the method I changed). Testing
> DOMFileSystem::getFile() would require blink::Platform::fileSystem(), just
like
> in the DOMFileSystemSync case.
> 
> What do you think of this approach, Kent-san?

This is ok.
Also, you may create a static function like the following and use it to improve
testability.

PassRefPtr<File> createFileWithPlatformPath(const FileMetadata&, FileSystemType
type, const String name);

[pwnall-personal, 2014-09-05 09:35:40]

Thank you for the quick response, Kent-san!

On 2014/09/05 09:21:26, tkent wrote:
> Also, you may create a static function like the following and use it to
improve
> testability.
> 
> PassRefPtr<File> createFileWithPlatformPath(const FileMetadata&,
FileSystemType
> type, const String name);

This would go in DOMFileSystemBase, right?

I'm assuming it shouldn't go in File, because that would introduce a logical
dependency from core/ on modules/

[tkent, 2014-09-05 09:37:55]

On 2014/09/05 09:35:40, pwnall wrote:
> This would go in DOMFileSystemBase, right?

Yeah, I think so.

[pwnall-personal, 2014-09-05 10:20:16]

Thank you for your advice, Kent-san! The code looks much cleaner, and I was able
to add a reasonably small DOMFileSystemBaseTest.cpp that covers the logic.

At this point, I don't think that testing at the didCreateSnapshotFile() level
is valuable anymore, because the code there is straight-line, for both
DOMFileSystem and DOMFileSystemSync. I think that the type system and the layout
tests can catch any bugs there might be there. If you agree, I'd like to remove
FileSystemCallbacksTest.cpp from this patch, and not pursue adding a C++ test
for DOMFileSystemSyncTest.

What do you think?

[pwnall-personal, 2014-09-05 11:02:46]

On 2014/09/05 10:20:16, pwnall wrote:
> At this point, I don't think that testing at the didCreateSnapshotFile() level
> is valuable anymore, because the code there is straight-line, for both
> DOMFileSystem and DOMFileSystemSync. I think that the type system and the
layout
> tests can catch any bugs there might be there. If you agree, I'd like to
remove
> FileSystemCallbacksTest.cpp from this patch, and not pursue adding a C++ test
> for DOMFileSystemSyncTest.

FWIW, the error in Patch Set 3 was not intentional. Fortunately, it was caught
by a layout test, which supports the 2nd half of my claim above.

[tkent, 2014-09-05 11:19:22]

On 2014/09/05 10:20:16, pwnall wrote:
> Thank you for your advice, Kent-san! The code looks much cleaner, and I was
able
> to add a reasonably small DOMFileSystemBaseTest.cpp that covers the logic.
> 
> At this point, I don't think that testing at the didCreateSnapshotFile() level
> is valuable anymore, because the code there is straight-line, for both
> DOMFileSystem and DOMFileSystemSync. I think that the type system and the
layout
> tests can catch any bugs there might be there. If you agree, I'd like to
remove
> FileSystemCallbacksTest.cpp from this patch, and not pursue adding a C++ test
> for DOMFileSystemSyncTest.
> 
> What do you think?

Yes, this CL doesn't need FileSystemCallbacksTest.cpp.

[pwnall-personal, 2014-09-05 14:06:14]

Thank you very much for your patience and guidance, Kent-san!

Can you please take another look?

[tkent, 2014-09-07 21:30:35]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-07 21:30:37]

lgtm

[commit-bot: I haz the power, 2014-09-07 21:31:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/516763002/100001

[commit-bot: I haz the power, 2014-09-07 21:37:19]

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

[commit-bot: I haz the power, 2014-09-07 21:37:20]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[pwnall-personal, 2014-09-08 01:12:39]

The CQ bit was checked by costan@gmail.com

[commit-bot: I haz the power, 2014-09-08 01:13:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/costan@gmail.com/516763002/120001

[commit-bot: I haz the power, 2014-09-08 01:13:40]

Committed patchset #6 (id:120001) as 181530