Be explicit about target type in platform_util::OpenItem()

Be explicit about target type in platform_util::OpenItem()

OpenItem() now takes an OpenItemType parameter that should specify
expected type of the object to be opened. It verifies the type of the
object before invoking platform specific logic for opening the item.

Code that assumed that the target of OpenItem() was always a folder
should now no longer unintentionally open or execute the file at the
target location when this assumption was found to not be correct.

In addition to the checks performed by OpenItem, the platform specific
logic used to open folders fail if the target type is not a directory.

BUG=387037
Committed: https://crrev.com/655d1118025f1b7d2b0cff9fb465da3a50ad15e9
Cr-Commit-Position: refs/heads/master@{#319555}

[asanka, 2014-09-24 22:07:22]

Patchset #1 (id:60001) has been deleted

[asanka, 2014-09-24 22:07:33]

Patchset #1 (id:80001) has been deleted

[asanka, 2014-09-24 22:12:16]

asanka@chromium.org changed reviewers:
+ benjhayden@chromium.org, gbillock@chromium.org, hashimoto@chromium.org,
jhawkins@chromium.org

[asanka, 2014-09-24 22:16:18]

hashimoto:
  chrome/browser/chromeos/

benjhayden:
  chrome/browser/extensions/api/downloads/

gbillock:
  chrome/browser/media_galleries/

jhawkins:
  chrome/browser/
  chrome/browser/ui/webui/

[hashimoto, 2014-09-25 02:34:05]

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:163: void
ContinueOpenItem(Profile* profile,
I think it's better to split this function into two.
One for opening files (this can be merged with OpenFileInternal) and another for
opening directories.

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:36: void OpenFolder(Profile* profile, const
base::FilePath& full_path);
Please use one of "directory" and "folder", not both of them.
I think "directory" is a better word because it seems used more widely.

[James Hawkins, 2014-09-25 17:47:26]

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:28: // Open the given file in the desktop's
default manner. |full_path| must point
Unfortunately it's not consistent with the rest of this file, but these should
be "Opens".

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:34: // is not a directory, the nno action will be
taken. Must be called from the UI
"then no"

[asanka, 2014-09-25 22:24:20]

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:163: void
ContinueOpenItem(Profile* profile,
On 2014/09/25 02:34:05, hashimoto wrote:
> I think it's better to split this function into two.
> One for opening files (this can be merged with OpenFileInternal) and another
for
> opening directories.

Done. Good call.

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:28: // Open the given file in the desktop's
default manner. |full_path| must point
On 2014/09/25 17:47:26, James Hawkins wrote:
> Unfortunately it's not consistent with the rest of this file, but these should
> be "Opens".

Done. Changed to "Opens". Also reworded a bit.

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:34: // is not a directory, the nno action will be
taken. Must be called from the UI
On 2014/09/25 17:47:26, James Hawkins wrote:
> "then no"

Done. Need spell checker for comments.

https://codereview.chromium.org/352393002/diff/100001/chrome/browser/platform...
chrome/browser/platform_util.h:36: void OpenFolder(Profile* profile, const
base::FilePath& full_path);
On 2014/09/25 02:34:05, hashimoto wrote:
> Please use one of "directory" and "folder", not both of them.
> I think "directory" is a better word because it seems used more widely.

'Folder' is consistent with the ShowItemInFolder function above and the use of
folder vs. directory in general parlance. See for example:
http://superuser.com/questions/187900/what-is-the-difference-between-a-direct...
. While the target is represented by a FilePath, the actual object that will be
opened is not required to be the file system directory named by |full_path|.

You are right that I refer to a 'directory' occasionally in the comments. I've
fixed the instances that I found.

[hashimoto, 2014-09-26 06:12:44]

cc: hirono

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:180:
OpenFileManagerWithInternalActionId(profile, url, "open");
Sorry, I should have noticed this before.
Does this really prevent inappropriate files from being opened?

I think the directory at the path can be replaced with a file with the same name
at a timing between CheckIfDirectoryExists() and the path gets actually opened.
Maybe we should leave Chrome OS code unchanged as it doesn't allow executable
files running after all.
Though, it's still possible to open an arbitrary file to launch the associated
app. (should we also be worried about this problem?)

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.h (right):

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.h:30: // Open a directory in the
file manager.
nit: "Opens a folder"

[hashimoto, 2014-09-26 06:37:20]

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:180:
OpenFileManagerWithInternalActionId(profile, url, "open");
On 2014/09/26 06:12:43, hashimoto wrote:
> Sorry, I should have noticed this before.
> Does this really prevent inappropriate files from being opened?
> 
> I think the directory at the path can be replaced with a file with the same
name
> at a timing between CheckIfDirectoryExists() and the path gets actually
opened.
> Maybe we should leave Chrome OS code unchanged as it doesn't allow executable
> files running after all.
> Though, it's still possible to open an arbitrary file to launch the associated
> app. (should we also be worried about this problem?)

hirono@ said OpenFileManagerWithInternalActionId() never launches apps other
than Files.app itself. (thanks hirono@!)
So it seems what we should do is to drop CheckIfDirectoryExists() and just call
OpenFileManagerWithInternalActionId() directly from OpenFolder().
Also, OpenFile() can be changed to skip directory existence check too.

[asanka, 2014-09-26 19:35:41]

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:180:
OpenFileManagerWithInternalActionId(profile, url, "open");
On 2014/09/26 06:37:20, hashimoto wrote:
> On 2014/09/26 06:12:43, hashimoto wrote:
> > Sorry, I should have noticed this before.
> > Does this really prevent inappropriate files from being opened?
> > 
> > I think the directory at the path can be replaced with a file with the same
> name
> > at a timing between CheckIfDirectoryExists() and the path gets actually
> opened.
> > Maybe we should leave Chrome OS code unchanged as it doesn't allow
executable
> > files running after all.
> > Though, it's still possible to open an arbitrary file to launch the
associated
> > app. (should we also be worried about this problem?)
> 
> hirono@ said OpenFileManagerWithInternalActionId() never launches apps other
> than Files.app itself. (thanks hirono@!)
> So it seems what we should do is to drop CheckIfDirectoryExists() and just
call
> OpenFileManagerWithInternalActionId() directly from OpenFolder().
> Also, OpenFile() can be changed to skip directory existence check too.

Thanks! Done.

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.h (right):

https://codereview.chromium.org/352393002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.h:30: // Open a directory in the
file manager.
On 2014/09/26 06:12:44, hashimoto wrote:
> nit: "Opens a folder"

Done.

[hashimoto, 2014-09-29 05:34:13]

chrome/browser/chromeos/file_manager/ lgtm.
Thanks, the new code looks much simpler!

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:91: const base::FilePath&
file_path) {
nit: indent.

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:92: int message;
nit: While you are here, could you fix this line not to leave this variable
uninitialized?

[asanka, 2014-09-29 16:17:53]

Thanks hashimoto!

benjhayden, jhawkins, gbillock: Over to you!

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:91: const base::FilePath&
file_path) {
On 2014/09/29 05:34:13, hashimoto wrote:
> nit: indent.

Done.

https://codereview.chromium.org/352393002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:92: int message;
On 2014/09/29 05:34:13, hashimoto wrote:
> nit: While you are here, could you fix this line not to leave this variable
> uninitialized?

Done.

[James Hawkins, 2014-09-29 21:09:14]

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:29: // to an existing file. If |full_path|
doesn't refer to an existing file, then
This secondary condition "If ... doesn't refer to an existing file" is confusing
because the statement preceding it says it shouldn't be possible.  When the
documentation says 'must', we must assert that the condition is true.  Any other
behavior must be undocumented.

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:33: // Opens the given folder in the file
manager. No action will be taken if
What is the 'file manager'?  Perhaps we should be less-specific, as is the case
for OpenFile where we refer to the desktop's default manner.

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:33: // Opens the given folder in the file
manager. No action will be taken if
Why do we not say anything about |full_path| pointing to an existing folder, as
we do for OpenFile?

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:28: void OpenFile(Profile* profile,
const base::FilePath& full_path) {
The documentation says |full_path| must poing to an existing file; is that
asserted in file_manager::util::OpenFile?

[asanka, 2014-09-30 23:07:46]

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:29: // to an existing file. If |full_path|
doesn't refer to an existing file, then
On 2014/09/29 21:09:14, James Hawkins wrote:
> This secondary condition "If ... doesn't refer to an existing file" is
confusing
> because the statement preceding it says it shouldn't be possible.  When the
> documentation says 'must', we must assert that the condition is true.  Any
other
> behavior must be undocumented.

Yup. I updated the comments for ShowItemInFolder(), OpenFile(), and
OpenFolder(). I relaxed the requirement that |full_path| must exist and point to
files/folders since that's often not practical for the call site to determine
prior to calling these functions.

They are also currently fire-and-forget with no feedback of whether the
operation succeeded. This behavior reflects the nature of the platform level
APIs that are used to implement them. So I only documented the restrictions that
are enforced within Chrome before the request is passed down to the underlying
platform.

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:33: // Opens the given folder in the file
manager. No action will be taken if
On 2014/09/29 21:09:14, James Hawkins wrote:
> Why do we not say anything about |full_path| pointing to an existing folder,
as
> we do for OpenFile?

Comment updated.

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util.h:33: // Opens the given folder in the file
manager. No action will be taken if
On 2014/09/29 21:09:14, James Hawkins wrote:
> What is the 'file manager'?  Perhaps we should be less-specific, as is the
case
> for OpenFile where we refer to the desktop's default manner.

Comment updated.

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/352393002/diff/160001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:28: void OpenFile(Profile* profile,
const base::FilePath& full_path) {
On 2014/09/29 21:09:14, James Hawkins wrote:
> The documentation says |full_path| must poing to an existing file; is that
> asserted in file_manager::util::OpenFile?

I updated the documentation to not require callers to ensure the existence of
|full_path|.

[James Hawkins, 2014-10-01 16:54:16]

Where are the tests?

[rickyz (no longer on Chrome), 2015-02-04 23:19:30]

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
File chrome/browser/platform_util_linux.cc (right):

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_linux.cc:61: XDGOpen(path.value());
Now that https://codereview.chromium.org/885423003/ has landed, you can
specialize this for directories by setting current_directory in LaunchOptions
and running xdg-open .

This avoids any possibility of TOCTOU issues where a somebody can race the
checks by replacing a directory with a file.

[asanka, 2015-02-04 23:21:49]

On 2014/10/01 at 16:54:16, jhawkins wrote:
> Where are the tests?

Here you go :) It's been a while and this fell through the cracks a bit.

The ChromeOS tests are a bit tricky because the file operations are performed on
a virtual file system and require more setup. Let me know if you'd like these
tests to be split up. I wanted the tests to be in one place so that the common
behavior can be tested in one place.

(As refresher, you were looking at /chrome/browser/platform_*)

[Robert Sesek, 2015-02-05 00:52:28]

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

[Robert Sesek, 2015-02-05 00:52:30]

Some drive-by Mac-y comments.

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:34: // This function opens a file.  This
doesn't use LaunchServices or NSWorkspace
This comment isn't accurate anymore (c.f. lines 47-50), so you can remove it. (I
think it was a holdover from before I made the change below).

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:163: [[NSWorkspace sharedWorkspace]
openFile:path_string
Does:

  NSURL* url = [NSURL fileURLWithPath:path_string];
  [[NSWorkspace sharedWorkspace] activateFileViewerSelectingURLs:
      @[ url ]];

work? Users can technically have Finder alternatives installed.

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:168: }
nit: blank line before, and "  // namespace internal"

[asanka, 2015-02-05 18:07:25]

Thanks for the reviews!

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
File chrome/browser/platform_util_linux.cc (right):

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_linux.cc:61: XDGOpen(path.value());
On 2015/02/04 at 23:19:30, rickyz wrote:
> Now that https://codereview.chromium.org/885423003/ has landed, you can
specialize this for directories by setting current_directory in LaunchOptions
and running xdg-open .
> 
> This avoids any possibility of TOCTOU issues where a somebody can race the
checks by replacing a directory with a file.

Did so. PTAL?

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:34: // This function opens a file.  This
doesn't use LaunchServices or NSWorkspace
On 2015/02/05 at 00:52:29, Robert Sesek wrote:
> This comment isn't accurate anymore (c.f. lines 47-50), so you can remove it.
(I think it was a holdover from before I made the change below).

Comment removed.

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:163: [[NSWorkspace sharedWorkspace]
openFile:path_string
On 2015/02/05 at 00:52:29, Robert Sesek wrote:
> Does:
> 
>   NSURL* url = [NSURL fileURLWithPath:path_string];
>   [[NSWorkspace sharedWorkspace] activateFileViewerSelectingURLs:
>       @[ url ]];
> 
> work? Users can technically have Finder alternatives installed.

I tried this, but it doesn't have the intended effect. This causes the
containing folder to the opened in Finder with the target folder item selected.
Using "/target/path/." also doesn't help.

By the way is this method considered canonical for opening a finder and
selecting an item? I can update ShowItemInFolder() above while I'm here since
the behavior matches what's expected there.

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:168: }
On 2015/02/05 at 00:52:29, Robert Sesek wrote:
> nit: blank line before, and "  // namespace internal"

Done.

[benjhayden, 2015-02-05 18:31:56]

downloads LGTM

[Robert Sesek, 2015-02-05 23:37:55]

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:163: [[NSWorkspace sharedWorkspace]
openFile:path_string
On 2015/02/05 18:07:25, asanka wrote:
> On 2015/02/05 at 00:52:29, Robert Sesek wrote:
> > Does:
> > 
> >   NSURL* url = [NSURL fileURLWithPath:path_string];
> >   [[NSWorkspace sharedWorkspace] activateFileViewerSelectingURLs:
> >       @[ url ]];
> > 
> > work? Users can technically have Finder alternatives installed.
> 
> I tried this, but it doesn't have the intended effect. This causes the
> containing folder to the opened in Finder with the target folder item
selected.
> Using "/target/path/." also doesn't help.
> 
> By the way is this method considered canonical for opening a finder and
> selecting an item? I can update ShowItemInFolder() above while I'm here since
> the behavior matches what's expected there.

I think the only difference between this method and the current implementation
is selecting a single vs. multiple files, and based on URL vs file path.
Otherwise, I think they're equivalent.

For here, though, if you remove the withApplication:@"Finder" does this still
work as expected? Specifically, has |path| been validated to be a directory, or
is Finder explicitly being called out here to handle that?

[rickyz (no longer on Chrome), 2015-02-06 01:09:58]

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

[rickyz (no longer on Chrome), 2015-02-06 01:10:00]

https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
File chrome/browser/platform_util_linux.cc (right):

https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
path.DirName() : path, path.value());
In the case of OPEN_DIR, we can avoid passing the path as an argument with
something like:

if (type == OPEN_FILE) {
  XDGOpen(base::FilePath(), path.value());
} else if (type == OPEN_DIR) {
  XDGOpen(path, ".");
}

[asanka, 2015-02-06 23:10:10]

New patchsets have been uploaded after l-g-t-m from benjhayden@chromium.org

[asanka, 2015-02-06 23:15:47]

On 2015/02/05 at 23:37:55, rsesek wrote:
>
https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
> File chrome/browser/platform_util_mac.mm (right):
> 
>
https://codereview.chromium.org/352393002/diff/220001/chrome/browser/platform...
> chrome/browser/platform_util_mac.mm:163: [[NSWorkspace sharedWorkspace]
openFile:path_string
> On 2015/02/05 18:07:25, asanka wrote:
> > On 2015/02/05 at 00:52:29, Robert Sesek wrote:
> > > Does:
> > > 
> > >   NSURL* url = [NSURL fileURLWithPath:path_string];
> > >   [[NSWorkspace sharedWorkspace] activateFileViewerSelectingURLs:
> > >       @[ url ]];
> > > 
> > > work? Users can technically have Finder alternatives installed.
> > 
> > I tried this, but it doesn't have the intended effect. This causes the
> > containing folder to the opened in Finder with the target folder item
selected.
> > Using "/target/path/." also doesn't help.
> > 
> > By the way is this method considered canonical for opening a finder and
> > selecting an item? I can update ShowItemInFolder() above while I'm here
since
> > the behavior matches what's expected there.
> 
> I think the only difference between this method and the current implementation
is selecting a single vs. multiple files, and based on URL vs file path.
Otherwise, I think they're equivalent.
> 
> For here, though, if you remove the withApplication:@"Finder" does this still
work as expected? Specifically, has |path| been validated to be a directory, or
is Finder explicitly being called out here to handle that?

That works! Thanks for the suggestion. See new patchset.

[asanka, 2015-02-06 23:16:11]

https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
File chrome/browser/platform_util_linux.cc (right):

https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
path.DirName() : path, path.value());
On 2015/02/06 at 01:09:59, rickyz wrote:
> In the case of OPEN_DIR, we can avoid passing the path as an argument with
something like:
> 
> if (type == OPEN_FILE) {
>   XDGOpen(base::FilePath(), path.value());
> } else if (type == OPEN_DIR) {
>   XDGOpen(path, ".");
> }

Thanks. The OPEN_FOLDER case is now handled via XDGOpen(path, "."). I opted to
use a switch() since that causes a compile error if someone were to add another
enum value.

I also kept the logic for using the containing directory as the current_folder
when handling the OPEN_FILE case. The latter seems to yield a better user
experience if, for example, the user invokes a file chooser from the launched
application. If the current_directory is not explicitly set, the folder that
would be opened by default would be the one containing google-chrome or
chromium, rather than the folder containing the opened file.

[rickyz (no longer on Chrome), 2015-02-06 23:45:37]

rickyz@chromium.org changed reviewers:
+ wfh@chromium.org

[rickyz (no longer on Chrome), 2015-02-06 23:45:38]

Thanks, platform_util_linux.cc lgtm

I think there may be a similar TOCTOU issue on windows, where someone might be
able to execute a command if they manage to replace the directory with a program
in between the directory check and the ShellExecute. Could we look into doing a
similar trick on windows? I'm not super familiar with windows, but it looks like
we end up calling ShellExecuteExW.

ShellExecuteExW takes a SHELLEXECUTEINFO, and it looks like we can set the
lpVerb member to L"explore" to make it expect a directory. Adding wfh@ who might
be more familiar the windows side of things.

[rickyz (no longer on Chrome), 2015-02-06 23:58:45]

Adding jschuh@ as another windows person - whoever gets to it first I guess

[asanka, 2015-02-09 16:51:15]

On 2015/02/06 at 23:16:11, asanka wrote:
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> File chrome/browser/platform_util_linux.cc (right):
> 
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
path.DirName() : path, path.value());
> On 2015/02/06 at 01:09:59, rickyz wrote:
> > In the case of OPEN_DIR, we can avoid passing the path as an argument with
something like:
> > 
> > if (type == OPEN_FILE) {
> >   XDGOpen(base::FilePath(), path.value());
> > } else if (type == OPEN_DIR) {
> >   XDGOpen(path, ".");
> > }
> 
> Thanks. The OPEN_FOLDER case is now handled via XDGOpen(path, "."). I opted to
use a switch() since that causes a compile error if someone were to add another
enum value.
> 
> I also kept the logic for using the containing directory as the current_folder
when handling the OPEN_FILE case. The latter seems to yield a better user
experience if, for example, the user invokes a file chooser from the launched
application. If the current_directory is not explicitly set, the folder that
would be opened by default would be the one containing google-chrome or
chromium, rather than the folder containing the opened file.

I'll see if holding an open handle for the directory across the shell invocation
would work in the case of Windows. Since we have the option of invoking the
shell in a utility process I'll carry the file/folder separation deeper into
ui/base since shell.cc needs to handle the two cases separately in the utility
process.

I'm a bit skeptical about how bullet-proof this is since the shell invocation or
xdg-open could still handle the open action asynchronously in a manner that
would defeat this mitigation. But I think it's still better than not having any
mitigation.

rsesek: This also affects Mac OS X. I'll see what I can do there.

[rickyz (no longer on Chrome), 2015-02-09 19:19:29]

On 2015/02/09 16:51:15, asanka wrote:
> On 2015/02/06 at 23:16:11, asanka wrote:
> >
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > File chrome/browser/platform_util_linux.cc (right):
> > 
> >
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
> path.DirName() : path, path.value());
> > On 2015/02/06 at 01:09:59, rickyz wrote:
> > > In the case of OPEN_DIR, we can avoid passing the path as an argument with
> something like:
> > > 
> > > if (type == OPEN_FILE) {
> > >   XDGOpen(base::FilePath(), path.value());
> > > } else if (type == OPEN_DIR) {
> > >   XDGOpen(path, ".");
> > > }
> > 
> > Thanks. The OPEN_FOLDER case is now handled via XDGOpen(path, "."). I opted
to
> use a switch() since that causes a compile error if someone were to add
another
> enum value.
> > 
> > I also kept the logic for using the containing directory as the
current_folder
> when handling the OPEN_FILE case. The latter seems to yield a better user
> experience if, for example, the user invokes a file chooser from the launched
> application. If the current_directory is not explicitly set, the folder that
> would be opened by default would be the one containing google-chrome or
> chromium, rather than the folder containing the opened file.
> 
> I'll see if holding an open handle for the directory across the shell
invocation
> would work in the case of Windows. Since we have the option of invoking the
> shell in a utility process I'll carry the file/folder separation deeper into
> ui/base since shell.cc needs to handle the two cases separately in the utility
> process.
> 
> I'm a bit skeptical about how bullet-proof this is since the shell invocation
or
> xdg-open could still handle the open action asynchronously in a manner that
> would defeat this mitigation. But I think it's still better than not having
any
> mitigation.
> 
> rsesek: This also affects Mac OS X. I'll see what I can do there.

For what it's worth, I tried ShellExecuteEx with lpVerb = L"explore" on a
windows machine, and it does pop up an error if you attempt to give it say,
calc.exe instead of a directory. I didn't confirm whether the directory check in
the library was done in a non-racy way though (will definitely defer to anyone
more familiar with windows than me :-)).

What kind of dangerous situation were you thinking about with xdg-open .? Were
you worried about an incorrect directory being shown? My main goal is to ensure
that someone who controls the directory being opened cannot trick the function
into running a command instead.

[asanka, 2015-02-10 01:13:00]

New patchsets have been uploaded after l-g-t-m from rickyz@chromium.org

[asanka, 2015-02-10 01:47:48]

Patchset #10 (id:280001) has been deleted

[asanka, 2015-02-10 01:57:51]

Updated Windows logic to handle TOCTOU race.

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:156: // Note that there exists a TOCTOU race
between the time that |path| was
rsesek: FYI. This unfortunately also happens even when using [[NSWorkspace ...]
openFile:... withApplication:@"Finder"]. Finder will open the associated
application if the target path is not a folder.

[asanka, 2015-02-10 02:14:44]

On 2015/02/09 at 19:19:29, rickyz wrote:
> On 2015/02/09 16:51:15, asanka wrote:
> > On 2015/02/06 at 23:16:11, asanka wrote:
> > >
> >
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > > File chrome/browser/platform_util_linux.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > > chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
> > path.DirName() : path, path.value());
> > > On 2015/02/06 at 01:09:59, rickyz wrote:
> > > > In the case of OPEN_DIR, we can avoid passing the path as an argument
with
> > something like:
> > > > 
> > > > if (type == OPEN_FILE) {
> > > >   XDGOpen(base::FilePath(), path.value());
> > > > } else if (type == OPEN_DIR) {
> > > >   XDGOpen(path, ".");
> > > > }
> > > 
> > > Thanks. The OPEN_FOLDER case is now handled via XDGOpen(path, "."). I
opted to
> > use a switch() since that causes a compile error if someone were to add
another
> > enum value.
> > > 
> > > I also kept the logic for using the containing directory as the
current_folder
> > when handling the OPEN_FILE case. The latter seems to yield a better user
> > experience if, for example, the user invokes a file chooser from the
launched
> > application. If the current_directory is not explicitly set, the folder that
> > would be opened by default would be the one containing google-chrome or
> > chromium, rather than the folder containing the opened file.
> > 
> > I'll see if holding an open handle for the directory across the shell
invocation
> > would work in the case of Windows. Since we have the option of invoking the
> > shell in a utility process I'll carry the file/folder separation deeper into
> > ui/base since shell.cc needs to handle the two cases separately in the
utility
> > process.
> > 
> > I'm a bit skeptical about how bullet-proof this is since the shell
invocation or
> > xdg-open could still handle the open action asynchronously in a manner that
> > would defeat this mitigation. But I think it's still better than not having
any
> > mitigation.
> > 
> > rsesek: This also affects Mac OS X. I'll see what I can do there.
> 
> For what it's worth, I tried ShellExecuteEx with lpVerb = L"explore" on a
windows machine, and it does pop up an error if you attempt to give it say,
calc.exe instead of a directory. I didn't confirm whether the directory check in
the library was done in a non-racy way though (will definitely defer to anyone
more familiar with windows than me :-)).
> 
> What kind of dangerous situation were you thinking about with xdg-open .? Were
you worried about an incorrect directory being shown? My main goal is to ensure
that someone who controls the directory being opened cannot trick the function
into running a command instead.

What if xdg-open or an application launched by it tries to expand the relative
path "." to a full path? xdg-open itself uses "$(pwd)/$file". Even if the
current directory currently has no named links, pwd still reports the original
path which at this point could point to a file instead of a directory.

Granted, the window of opportunity for such a race is small.

[rickyz (no longer on Chrome), 2015-02-10 05:53:40]

On 2015/02/10 02:14:44, asanka wrote:
> On 2015/02/09 at 19:19:29, rickyz wrote:
> > On 2015/02/09 16:51:15, asanka wrote:
> > > On 2015/02/06 at 23:16:11, asanka wrote:
> > > >
> > >
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > > > File chrome/browser/platform_util_linux.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/352393002/diff/240001/chrome/browser/platform...
> > > > chrome/browser/platform_util_linux.cc:64: XDGOpen(type == OPEN_FILE ?
> > > path.DirName() : path, path.value());
> > > > On 2015/02/06 at 01:09:59, rickyz wrote:
> > > > > In the case of OPEN_DIR, we can avoid passing the path as an argument
> with
> > > something like:
> > > > > 
> > > > > if (type == OPEN_FILE) {
> > > > >   XDGOpen(base::FilePath(), path.value());
> > > > > } else if (type == OPEN_DIR) {
> > > > >   XDGOpen(path, ".");
> > > > > }
> > > > 
> > > > Thanks. The OPEN_FOLDER case is now handled via XDGOpen(path, "."). I
> opted to
> > > use a switch() since that causes a compile error if someone were to add
> another
> > > enum value.
> > > > 
> > > > I also kept the logic for using the containing directory as the
> current_folder
> > > when handling the OPEN_FILE case. The latter seems to yield a better user
> > > experience if, for example, the user invokes a file chooser from the
> launched
> > > application. If the current_directory is not explicitly set, the folder
that
> > > would be opened by default would be the one containing google-chrome or
> > > chromium, rather than the folder containing the opened file.
> > > 
> > > I'll see if holding an open handle for the directory across the shell
> invocation
> > > would work in the case of Windows. Since we have the option of invoking
the
> > > shell in a utility process I'll carry the file/folder separation deeper
into
> > > ui/base since shell.cc needs to handle the two cases separately in the
> utility
> > > process.
> > > 
> > > I'm a bit skeptical about how bullet-proof this is since the shell
> invocation or
> > > xdg-open could still handle the open action asynchronously in a manner
that
> > > would defeat this mitigation. But I think it's still better than not
having
> any
> > > mitigation.
> > > 
> > > rsesek: This also affects Mac OS X. I'll see what I can do there.
> > 
> > For what it's worth, I tried ShellExecuteEx with lpVerb = L"explore" on a
> windows machine, and it does pop up an error if you attempt to give it say,
> calc.exe instead of a directory. I didn't confirm whether the directory check
in
> the library was done in a non-racy way though (will definitely defer to anyone
> more familiar with windows than me :-)).
> > 
> > What kind of dangerous situation were you thinking about with xdg-open .?
Were
> you worried about an incorrect directory being shown? My main goal is to
ensure
> that someone who controls the directory being opened cannot trick the function
> into running a command instead.
> 
> What if xdg-open or an application launched by it tries to expand the relative
> path "." to a full path? xdg-open itself uses "$(pwd)/$file". Even if the
> current directory currently has no named links, pwd still reports the original
> path which at this point could point to a file instead of a directory.
I think $(pwd)/$file is OK, but I took a quick glance at gvfs-open, and it does
exactly what you said - it converts "." to an absolute path, which is another
potential race, sigh :-( jln@ came up with some hacks involving /proc/PID/fd/X
to try to work around this behavior, but I definitely don't want to block your
CL on dealing with this :-)

> Granted, the window of opportunity for such a race is small.
Yeah, though this isn't super reassuring - since losing the race is pretty
inconsequential, an attacker could probably try as much as they need to until it
works.

[Robert Sesek, 2015-02-10 23:30:26]

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:156: // Note that there exists a TOCTOU race
between the time that |path| was
On 2015/02/10 01:57:51, asanka wrote:
> rsesek: FYI. This unfortunately also happens even when using [[NSWorkspace
...]
> openFile:... withApplication:@"Finder"]. Finder will open the associated
> application if the target path is not a folder.

Yeah, I was concerned about that. Using any of the open variants could be
problematic. One thing that could work but would be hacky is to use
-[NSWorkspace selectFile:inFileViewerRootedAtPath:] and then appending the file
name ".DS_Store" to the directory path. That file contains information generated
by the Finder and generally ends up in every directory that Finder has viewed,
but relying on its existence is a bit of a hack. I can't think of any other way
to just show the folder using NSWorkspace. An AppleEvent could do it though,
maybe.

[asanka, 2015-02-10 23:46:29]

asanka@chromium.org changed reviewers:
+ sky@chromium.org

[asanka, 2015-02-10 23:46:29]

asanka@chromium.org changed required reviewers:
+ hashimoto@chromium.org, jhawkins@chromium.org, rickyz@chromium.org,
rsesek@chromium.org, sky@chromium.org

[asanka, 2015-02-10 23:59:34]

+sky for ui/base/win which looks like it'll be somewhat stable.

rickyz, jschuh: Could you comment on the threat model regarding the TOCTOU
issue? It seems that once the initial check passes, the TOCTOU would only be an
issue if an attacker can replace a directory with a file with the same name.

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
File chrome/browser/platform_util_mac.mm (right):

https://codereview.chromium.org/352393002/diff/300001/chrome/browser/platform...
chrome/browser/platform_util_mac.mm:156: // Note that there exists a TOCTOU race
between the time that |path| was
On 2015/02/10 at 23:30:26, Robert Sesek wrote:
> On 2015/02/10 01:57:51, asanka wrote:
> > rsesek: FYI. This unfortunately also happens even when using [[NSWorkspace
...]
> > openFile:... withApplication:@"Finder"]. Finder will open the associated
> > application if the target path is not a folder.
> 
> Yeah, I was concerned about that. Using any of the open variants could be
problematic. One thing that could work but would be hacky is to use
-[NSWorkspace selectFile:inFileViewerRootedAtPath:] and then appending the file
name ".DS_Store" to the directory path. That file contains information generated
by the Finder and generally ends up in every directory that Finder has viewed,
but relying on its existence is a bit of a hack. I can't think of any other way
to just show the folder using NSWorkspace. An AppleEvent could do it though,
maybe.

I tried fiddling with Finder.app using ScriptingBridge but that involves adding
a dependency on ScriptingBridge to build something that only works for Finder.

Let me know if you would like me to look into using an AppleEvent. Though I
might have to pluck your brain about that.

[rickyz (no longer on Chrome), 2015-02-11 12:05:24]

On 2015/02/10 23:59:34, asanka wrote:
> +sky for ui/base/win which looks like it'll be somewhat stable.
> 
> rickyz, jschuh: Could you comment on the threat model regarding the TOCTOU
> issue? It seems that once the initial check passes, the TOCTOU would only be
an
> issue if an attacker can replace a directory with a file with the same name.
Yeah, with this change, we've raised the bar for abusing this to execute a
command from placing a file on the system to being able to race
xdg-open/ShellExecuteEx, etc., which requires the ability to create and move or
delete files and directories (although is it possible to get this by setting the
download directory to a samba share or web dav server, or will windows refuse to
run stuff programs from such sketchy places?)

(I did some reversing/testing on Windows, and even with lpVerb = L"explore", the
filename is passed around as a string, so I was able to successfully race it
with renaming files/directories too.)

Also, on Linux, I think gvfs-open and similar could be fooled by creating
$HOME/file:/etc/passwd (an attacker can create directories by setting the
download dir to the desired directory and triggering a download) and then
setting the download directory to file:///path/to/downloaded/evil/file. Blech,
this stuff is a horrible mess :-(

[rickyz (no longer on Chrome), 2015-02-11 12:16:01]

On 2015/02/11 12:05:24, rickyz wrote:
> On 2015/02/10 23:59:34, asanka wrote:
> > +sky for ui/base/win which looks like it'll be somewhat stable.
> > 
> > rickyz, jschuh: Could you comment on the threat model regarding the TOCTOU
> > issue? It seems that once the initial check passes, the TOCTOU would only be
> an
> > issue if an attacker can replace a directory with a file with the same name.
> Yeah, with this change, we've raised the bar for abusing this to execute a
> command from placing a file on the system to being able to race
> xdg-open/ShellExecuteEx, etc., which requires the ability to create and move
or
> delete files and directories (although is it possible to get this by setting
the
> download directory to a samba share or web dav server, or will windows refuse
to
> run stuff programs from such sketchy places?)
> 
> (I did some reversing/testing on Windows, and even with lpVerb = L"explore",
the
> filename is passed around as a string, so I was able to successfully race it
> with renaming files/directories too.)
> 
> Also, on Linux, I think gvfs-open and similar could be fooled by creating
> $HOME/file:/etc/passwd (an attacker can create directories by setting the
> download dir to the desired directory and triggering a download) and then
> setting the download directory to file:///path/to/downloaded/evil/file. Blech,
> this stuff is a horrible mess :-(

Correction: I don't think what I said about Linux is true - looks like the
default download path gets normalized via conversion to a FilePath. Maybe it's
not worth putting so much effort on this opint - if someone has the ability to
modify preferences, it seems that they can already write files by setting the
download path, disabling prompting, and downloading some files. Pretty sure
there are tons of ways to get code running from that point.

[sky, 2015-02-11 18:28:22]

sky@chromium.org changed reviewers:
+ ananta@chromium.org
- sky@chromium.org

[sky, 2015-02-11 18:28:23]

sky@chromium.org changed required reviewers:
+ ananta@chromium.org
- sky@chromium.org

[sky, 2015-02-11 18:28:24]

sky->ananta

[asanka, 2015-02-13 00:21:22]

On 2015/02/11 at 12:16:01, rickyz wrote:
> On 2015/02/11 12:05:24, rickyz wrote:
> > On 2015/02/10 23:59:34, asanka wrote:
> > > +sky for ui/base/win which looks like it'll be somewhat stable.
> > > 
> > > rickyz, jschuh: Could you comment on the threat model regarding the TOCTOU
> > > issue? It seems that once the initial check passes, the TOCTOU would only
be
> > an
> > > issue if an attacker can replace a directory with a file with the same
name.
> > Yeah, with this change, we've raised the bar for abusing this to execute a
> > command from placing a file on the system to being able to race
> > xdg-open/ShellExecuteEx, etc., which requires the ability to create and move
or
> > delete files and directories (although is it possible to get this by setting
the
> > download directory to a samba share or web dav server, or will windows
refuse to
> > run stuff programs from such sketchy places?)
> > 
> > (I did some reversing/testing on Windows, and even with lpVerb = L"explore",
the
> > filename is passed around as a string, so I was able to successfully race it
> > with renaming files/directories too.)
> > 
> > Also, on Linux, I think gvfs-open and similar could be fooled by creating
> > $HOME/file:/etc/passwd (an attacker can create directories by setting the
> > download dir to the desired directory and triggering a download) and then
> > setting the download directory to file:///path/to/downloaded/evil/file.
Blech,
> > this stuff is a horrible mess :-(
> 
> Correction: I don't think what I said about Linux is true - looks like the
default download path gets normalized via conversion to a FilePath. Maybe it's
not worth putting so much effort on this opint - if someone has the ability to
modify preferences, it seems that they can already write files by setting the
download path, disabling prompting, and downloading some files. Pretty sure
there are tons of ways to get code running from that point.

Yeah. Things are pretty grim once an attacker commandeers the download path. My
impression when I set out with this CL is that the pre-launch check would thwart
an attacker from running a downloaded executable without user interaction. I'm
assuming you still believe this to be the case?

[asanka, 2015-02-17 19:29:15]

Ping:

ananta: ui/base/win

hashimoto: /chromeos/ You gave an LGTM, but the code has changed quite a bit
since then.

rsesek: platform_util_mac.mm

jhawkins: chrome/browser/

[James Hawkins, 2015-02-17 19:42:58]

I'm OOO until next Monday.

[ananta, 2015-02-17 19:49:30]

lgtm

[Robert Sesek, 2015-02-17 20:59:47]

platform_util_mac.mm LGTM

I can't think of a good way to defeat that race condition, but I also cannot
think of how you'd exploit it with just downloads.

[timwillis, 2015-03-06 04:18:49]

hashimoto / jhawkins: Based on #48, I understand we're still waiting for LGTMs
from you both.

[Will Harris, 2015-03-06 06:13:38]

wfh@chromium.org changed reviewers:
+ sky@chromium.org

[Will Harris, 2015-03-06 06:13:57]

also added +sky who is a reviewer of ui/

[hashimoto, 2015-03-06 06:14:41]

hashimoto@chromium.org changed reviewers:
+ mtomasz@chromium.org
- sky@chromium.org

[hashimoto, 2015-03-06 06:14:42]

Oops, sorry for dismissing your ping.
Deferring chrome/browser/chromeos/file_manager/ to mtomasz@ who has been more
active on this code and should be more familiar with it.

[mtomasz, 2015-03-06 07:05:49]

lgtm with minor nits.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:238: storage::FileSystemURL
file_system_url = file_system_context->CrackURL(url);
nit: Can be const.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:581:
storage::ExternalFileSystemBackend* backend =
nit: Can be const.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:144: OpenFile(profile,
file_path, url, callback);
nit: TOCTOU race here as well. Can we add a comment?

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.cc:52: }
nit: // namespace
nit: Add \n in #51 or remove in #48.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:75: }
nit: // namespace internal

[Will Harris, 2015-03-06 07:35:20]

wfh@chromium.org changed reviewers:
+ sadrul@chromium.org

[Will Harris, 2015-03-06 07:35:41]

Windows shell stuff lgtm

Still need a ui/ reviewer to get past presubmit.  sky was removed for some
reason, so add +sadrul

[hashimoto, 2015-03-06 07:46:04]

On 2015/03/06 07:35:41, Will Harris wrote:
> Windows shell stuff lgtm
> 
> Still need a ui/ reviewer to get past presubmit.  sky was removed for some
> reason, so add +sadrul

Sorry, it seems I overwrote the reviewer list after you updated it so that sky@
got removed.

[sadrul, 2015-03-06 14:35:51]

sadrul@chromium.org changed reviewers:
+ cpu@chromium.org, sky@chromium.org
- sadrul@chromium.org

[sadrul, 2015-03-06 14:35:52]

+cpu@, sky@

I don't really know Windows well, so can't review the change in //ui/base/win.
Replacing me with sky@/cpu@

[James Hawkins, 2015-03-06 15:37:34]

LGTM with nits.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.h:49: // specifeid in |item_type|. This error will
be reported to |callback|.
s/specifeid/specified/

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.h:63: // Chrome OS only. Not used in other
platforms. Must be called on the UI
nit: The sentence "Not used in other platforms" is ambiguous as to whether it's
referring to the method or the profile parameter.  I suggest clarifying like,
"|profile| is not used on other platforms." etc.

[sky, 2015-03-06 18:21:33]

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.cc
File ui/base/win/shell.cc (right):

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.cc#ne...
ui/base/win/shell.cc:57: sei.lpVerb = (verb.empty() ? NULL : verb.c_str());
nullptr evey where?

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.h
File ui/base/win/shell.h (right):

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.h#new...
ui/base/win/shell.h:42: // Note: Must be called on a thread that allows
blocking.
The synchronous/blocking nature of this code is new. Are all existing call sites
able to deal with the new behavior?

[asanka, 2015-03-06 20:58:27]

Patchset #12 (id:340001) has been deleted

[asanka, 2015-03-06 21:09:40]

Thanks everyone for the reviews! And also to jhawkins for not letting me get
away without sufficient testing.

However, much of the platform specific functionality (opening external
applications and invoking the shell) require desktop isolation in a manner
that's not possible with our testing infrastructure. I'll pull in QA to make
sure manual testing covers the behavior affected by this CL that's not covered
by automated testing.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:238: storage::FileSystemURL
file_system_url = file_system_context->CrackURL(url);
On 2015/03/06 07:05:49, mtomasz wrote:
> nit: Can be const.

Done.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:581:
storage::ExternalFileSystemBackend* backend =
On 2015/03/06 07:05:49, mtomasz wrote:
> nit: Can be const.

The GrantFullAccessToExtension() call requires |backend| not be const.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_util.cc:144: OpenFile(profile,
file_path, url, callback);
On 2015/03/06 07:05:49, mtomasz wrote:
> nit: TOCTOU race here as well. Can we add a comment?

Right. Added a comment above.

Although the "open" action ID for the file manager app should always cause the
file manager to open, correct (or fail without doing anything if the target is
not a folder)? The problem with other platforms was that if there's a TOCTOU
race, the platform 'open folder' logic would turn into 'open file', thus
potentially causing code execution during an attempt to open a folder.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.cc:52: }
On 2015/03/06 07:05:49, mtomasz wrote:
> nit: // namespace
> nit: Add \n in #51 or remove in #48.

Done.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util.h (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.h:49: // specifeid in |item_type|. This error will
be reported to |callback|.
On 2015/03/06 15:37:34, James Hawkins wrote:
> s/specifeid/specified/

Done.

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util.h:63: // Chrome OS only. Not used in other
platforms. Must be called on the UI
On 2015/03/06 15:37:33, James Hawkins wrote:
> nit: The sentence "Not used in other platforms" is ambiguous as to whether
it's
> referring to the method or the profile parameter.  I suggest clarifying like,
> "|profile| is not used on other platforms." etc.

Clarified. Thanks!

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
File chrome/browser/platform_util_chromeos.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/platform...
chrome/browser/platform_util_chromeos.cc:75: }
On 2015/03/06 07:05:49, mtomasz wrote:
> nit: // namespace internal

Done.

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.cc
File ui/base/win/shell.cc (right):

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.cc#ne...
ui/base/win/shell.cc:57: sei.lpVerb = (verb.empty() ? NULL : verb.c_str());
On 2015/03/06 18:21:33, sky wrote:
> nullptr evey where?

Done.

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.h
File ui/base/win/shell.h (right):

https://codereview.chromium.org/352393002/diff/320001/ui/base/win/shell.h#new...
ui/base/win/shell.h:42: // Note: Must be called on a thread that allows
blocking.
On 2015/03/06 18:21:33, sky wrote:
> The synchronous/blocking nature of this code is new. Are all existing call
sites
> able to deal with the new behavior?

Yeah. They are able to deal. The callers are platform_util (which calls
Open*ViaShell on the blocking pool), shell_handler_win (runs in main thread of
utility process), and first_run_internal_win (runs prior to setting up browser
process threads).

The blocking nature isn't completely new. The SEE_MASK_NOASYNC flag to
ShellExecuteEx() is identical to the SEE_MASK_FLAG_DDEWAIT flag that was used
with the "openas" verb (same numerical value). The behavior is explicitly
documented now along with enforcement via a ThreadRestrictions check. In
addition, the NOASYNC/DDEWAIT flag is now used deterministically instead of only
being used when a suitable handler was not found for the file.

[sky, 2015-03-06 23:30:03]

Ok, LGTM

[mtomasz, 2015-03-07 00:01:12]

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:581:
storage::ExternalFileSystemBackend* backend =
On 2015/03/06 21:09:40, asanka wrote:
> On 2015/03/06 07:05:49, mtomasz wrote:
> > nit: Can be const.
> 
> The GrantFullAccessToExtension() call requires |backend| not be const.

I meant:
storage::ExternalFileSystemBackend* const backend = ...

[asanka, 2015-03-07 00:37:01]

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/fileapi_util.cc (right):

https://codereview.chromium.org/352393002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/fileapi_util.cc:581:
storage::ExternalFileSystemBackend* backend =
On 2015/03/07 00:01:12, mtomasz wrote:
> On 2015/03/06 21:09:40, asanka wrote:
> > On 2015/03/06 07:05:49, mtomasz wrote:
> > > nit: Can be const.
> > 
> > The GrantFullAccessToExtension() call requires |backend| not be const.
> 
> I meant:
> storage::ExternalFileSystemBackend* const backend = ...

Ah. Got it and done.

[asanka, 2015-03-07 03:37:18]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-03-07 03:37:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org, hashimoto@chromium.org, jhawkins@chromium.org,
mtomasz@chromium.org, rickyz@chromium.org, rsesek@chromium.org,
ananta@chromium.org, wfh@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/352393002/#ps380001
(title: "Catch up with changes to JSONStringValueSerializer and address CrOS
comment")

[commit-bot: I haz the power, 2015-03-07 03:38:24]

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

[commit-bot: I haz the power, 2015-03-07 05:33:58]

Committed patchset #13 (id:380001)

[commit-bot: I haz the power, 2015-03-07 05:34:42]

Patchset 13 (id:??) landed as
https://crrev.com/655d1118025f1b7d2b0cff9fb465da3a50ad15e9
Cr-Commit-Position: refs/heads/master@{#319555}