[Downloads] Prevent dangerous files from being opened automatically.

[Downloads] Prevent executable files from being opened automatically.

Users can accidentally mark executable or other dangerous file types for
opening automatically upon download. This change prevents such file
types from being considered for auto open.

If the user already has any dangerous file types in the auto-open list,
those entries will be ignored on new browser sessions. Any action that
causes the persisted auto-open list to be updated will, as a
side-effect, premanently remove any dangerous file types that were
there.

Currently the list of file types that are excluded from auto open is
maintained by chrome/browser/download/download_extension.cc.

BUG=461858
TEST=(1) Download an .exe (on Windows) or .swf file.
     (2) Verify that the "Always open files of this type" option is
         disabled.

Committed: https://crrev.com/39841e54180e2583dffa16fbbb9b99fd293821d0
Cr-Commit-Position: refs/heads/master@{#334677}

[asanka, 2015-06-03 22:21:56]

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

[Randy Smith (Not in Mondays), 2015-06-03 22:29:42]

On 2015/06/03 22:23:53, asanka wrote:

*chuckle*  CC me on the bug so I can look at it?

[Randy Smith (Not in Mondays), 2015-06-03 23:44:08]

First round; high level behavior and nits.

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_extensions.cc:235: return !extension.empty() &&
GetFileDangerLevel(path) == NOT_DANGEROUS;
The !extension.empty() test is redundant with the same check in
GetFileDangerLevel(), I think.

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_extensions.cc:235: return !extension.empty() &&
GetFileDangerLevel(path) == NOT_DANGEROUS;
So we no longer allow ALLOW_ON_USER_GESTURE files to be opened automatically? 
My gut is that that's going to annoy people, though the example I was going to
give was .pdf, which doesn't appear to be a dangerous file type anymore.  Would
it make sense to allow ALLOW_ON_USER_GESTURE types to open automatically iff
they were downloaded associated with a user gesture?  (I know we can't rely on
that, but we've already made the judgement that we're ok with downloading them
with the imperfect signal of a user gesture ...)

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_prefs.h:70: // fail if the |file_name| is
allowed to open automatically.
"is allowed" -> "isn't allowed"?

[asanka, 2015-06-04 15:30:48]

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_extensions.cc:235: return !extension.empty() &&
GetFileDangerLevel(path) == NOT_DANGEROUS;
On 2015/06/03 at 23:44:07, rdsmith wrote:
> The !extension.empty() test is redundant with the same check in
GetFileDangerLevel(), I think.

The check is different. GetFileDangerLevel() returns NOT_DANGEROUS if the file
has no extension. We want files without extensions to return false from this
function since they can't be opened automatically.

Also, regarding the ALLOW_ON_USER_GESTURE behavior: Downloads that had a user
gesture and those that didn't are both equivalent by the time the download
completes. Remember that if there wasn't a user gesture, we would've prompted
the user. In either case, by the end there would be a user gesture.

The only exception to this rule are those files that are now being handled by
SafeBrowsing. Those may not have a user gesture by the end of the download.

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_prefs.h:70: // fail if the |file_name| is
allowed to open automatically.
On 2015/06/03 at 23:44:08, rdsmith wrote:
> "is allowed" -> "isn't allowed"?

Done.

[Randy Smith (Not in Mondays), 2015-06-04 19:06:23]

Quick note: Asanka and I spoke offline, and he's agreed to explore an explicit
blacklist of types to inhibit auto-opening for, as opposed to doing it for all
user_gesture dangerous level types.

[asanka, 2015-06-05 16:15:39]

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

[asanka, 2015-06-05 19:01:48]

On 2015/06/05 at 16:17:34, asanka wrote:
> 

Right. Forgot to add a comment.

The latest patchset implements a sort-of blacklist by annotating the list of
dangerous file types in download_extension.cc with whether the file type is
disallowed from automatically opening.

rdsmith: PTAL again?

palmer: Could you do a quick pass through the annotated file types and see if
they make sense?

[palmer, 2015-06-08 19:05:08]

LGTM modulo nits and a potentially off-base question about alternative data
streams.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:230: {"dex",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
I'm actually not sure if Android even would directly run a dex, or what would
happen. Obviously it's maximally safe to specify EXCLUDE_FROM_AUTO_OPEN here.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:178: // IsAllowedToOpenAutomatically()
needs a filename.
Nit: "...since |IsAllowedToOpenAutomatically| needs..." or "...since the
|IsAllowedToOpenAutomatically| function needs..."

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_prefs_unittest.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:23: TEST(DownloadPrefsTest,
NoAutoOpenForExecutables) {
Do we need to do anything special about alternative data streams on systems
using NTFS? (Perhaps not, because a web download can't cause any ADS to be
created? But could Chrome somehow get invoked to run a file that does have an
ADS?)

[asanka, 2015-06-09 00:38:10]

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:230: {"dex",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
On 2015/06/08 at 19:05:07, palmer wrote:
> I'm actually not sure if Android even would directly run a dex, or what would
happen. Obviously it's maximally safe to specify EXCLUDE_FROM_AUTO_OPEN here.

Yeah. I added the flag for a couple of file types that don't make sense to open
directly. This should make no difference if the files can't be opened directly.
If they can (by, say, some custom editor), then users won't be able to auto open
them.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:178: // IsAllowedToOpenAutomatically()
needs a filename.
On 2015/06/08 at 19:05:07, palmer wrote:
> Nit: "...since |IsAllowedToOpenAutomatically| needs..." or "...since the
|IsAllowedToOpenAutomatically| function needs..."

Done

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_prefs_unittest.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:23: TEST(DownloadPrefsTest,
NoAutoOpenForExecutables) {
On 2015/06/08 at 19:05:07, palmer wrote:
> Do we need to do anything special about alternative data streams on systems
using NTFS? (Perhaps not, because a web download can't cause any ADS to be
created? But could Chrome somehow get invoked to run a file that does have an
ADS?)

Hm. We are making the following assumptions:

 1. Chrome can only auto-open file it downloaded itself.

 2. The files Chrome downloaded are have names corresponding to what
DownloadItem::GetTargetFilePath() returns.

 3. Said files consist of a primary data stream corresponding to the resource
transferred from DownloadItem::GetURL().

 4. Opening behavior of a downloaded file is predicted by the filename as
mentioned in #2, and not on the contents of the primary data stream nor
alternate streams.

If these assumptions are not correct, then we have a security issue. Of course,
they can be violated by some nefarious program running on the users' machine
with user level privileges, but that's outside our threat model.

[palmer, 2015-06-09 01:06:20]

> Hm. We are making the following assumptions:
> 
>  1. Chrome can only auto-open file it downloaded itself.
> 
>  2. The files Chrome downloaded are have names corresponding to what
> DownloadItem::GetTargetFilePath() returns.
> 
>  3. Said files consist of a primary data stream corresponding to the resource
> transferred from DownloadItem::GetURL().
> 
>  4. Opening behavior of a downloaded file is predicted by the filename as
> mentioned in #2, and not on the contents of the primary data stream nor
> alternate streams.
> 
> If these assumptions are not correct, then we have a security issue. Of
course,
> they can be violated by some nefarious program running on the users' machine
> with user level privileges, but that's outside our threat model.

That is how I hope it is (thank you for clarifying!), and I have no reason to
think these assumptions do not hold.

[Randy Smith (Not in Mondays), 2015-06-09 19:40:04]

I apologize for my slowness of response on this review.  This is a partial
review that I'm posting since it's pushing back a bit on some of the choices
made around allowing auto-open, and I could imagine that discussion will take
longer than any technical nits I may find in the rest of the review.  I'll keep
reviewing in the background in parallel with this conversation.

Chris, it might be useful for you to skim my comments, as I'm taking the
anti-security perspective and direct evaluation of that by the pro-security
perspective might be useful.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:62: enum DownloadDangerFlags {
Jamming these together with DownloadDangerLevel feels very weird to me.  Is
there any reason, given that g_executables is a static, that we couldn't just
add another entry to the Executable structure?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:105: {"application",
ALLOW_ON_USER_GESTURE},
Naively, I would have thought that something named .application would be an
executable that shouldn't be auto-opened; do you know something I don't?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:122: {"hlp",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
This is something I could easily imagine users wanting to auto-open, and I don't
see the danger; why block it?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:152: {"mhtml",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
Does opening a web page on the disk give it some special privileges?  This is
another case in which I could imagine users wanting auto-open.  (Applies to
other archive formats too.)

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:176: {"url", DANGEROUS |
EXCLUDE_FROM_AUTO_OPEN},
Why?  I'm not sure why people would want to download a URL, but I'd think that
if a web page had nefarious intent, it could just do a "document.location =". 
What cases am I missing?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:177: {"vb",
ALLOW_ON_USER_GESTURE},
Why isn't open a .vb file dangerous?  Does it always open in the IDE?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:181: {"vsmacros",
ALLOW_ON_USER_GESTURE},
You know Windows (much) better than I, but I'd think there's executable
nefariousness to be had in almost any macro language.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:223: {"pkg",
ALLOW_ON_USER_GESTURE},
Hmmm.  This is right at the boundary for me--I can definitely see the user
wanting to auto-execute, but it can do almost arbitrary bad things, right?

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:227: {"rpm",
ALLOW_ON_USER_GESTURE},
I think that "opening" a package on Linux installs it, which I believe includes
running a script, so I think these are executable equivalents.

[Randy Smith (Not in Mondays), 2015-06-09 21:29:25]

Remaining comments.

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
File chrome/browser/download/download_prefs_unittest.cc (right):

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:25: const base::FilePath
kFileWithNoExtension(FILE_PATH_LITERAL("abcd"));
This doesn't look like it's used in this test (I'm a bit confused the compiler
didn't warn you about that).

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:32:
EXPECT_FALSE(prefs.IsAutoOpenEnabledBasedOnExtension(kDangerousFilePath));
This looks like it's just testing .swf files, but that doesn't seem to match the
comment before the test; am I confused?

[asanka, 2015-06-12 19:27:58]

Patchset #6 (id:100001) has been deleted

[asanka, 2015-06-12 20:12:53]

Have another look?

Rather than explain in the CR, I've attempted to summarize the criteria we are
using in the comment at download_extensions.cc. I figured that's where folks
would be looking when they want to change the dangerous file types list.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:62: enum DownloadDangerFlags {
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Jamming these together with DownloadDangerLevel feels very weird to me.  Is
there any reason, given that g_executables is a static, that we couldn't just
add another entry to the Executable structure?

Yeah. I see what you mean. I split it out into a separate field.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:105: {"application",
ALLOW_ON_USER_GESTURE},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Naively, I would have thought that something named .application would be an
executable that shouldn't be auto-opened; do you know something I don't?

I added some details in the form of comments. Let me know if it helps.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:122: {"hlp",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> This is something I could easily imagine users wanting to auto-open, and I
don't see the danger; why block it?

This was due to the known default file handler having multiple vulnerabilities
in the past. However, I did remove the flag for this round.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:152: {"mhtml",
ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Does opening a web page on the disk give it some special privileges?  This is
another case in which I could imagine users wanting auto-open.  (Applies to
other archive formats too.)

Removed the flag.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:176: {"url", DANGEROUS |
EXCLUDE_FROM_AUTO_OPEN},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Why?  I'm not sure why people would want to download a URL, but I'd think that
if a web page had nefarious intent, it could just do a "document.location =". 
What cases am I missing?

A .url file is an OLE persisted container that can point at a lot more things
than what 'document.location'.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:177: {"vb",
ALLOW_ON_USER_GESTURE},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Why isn't open a .vb file dangerous?  Does it always open in the IDE?

.vb (not to be confused with .vbs) isn't associated with the Windows Script
Host. It's usually associated with an Visual Studio.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:181: {"vsmacros",
ALLOW_ON_USER_GESTURE},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> You know Windows (much) better than I, but I'd think there's executable
nefariousness to be had in almost any macro language.

As far as I could discover, opening this type of file doesn't automatically
execute code until the macro is invoked.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:223: {"pkg",
ALLOW_ON_USER_GESTURE},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> Hmmm.  This is right at the boundary for me--I can definitely see the user
wanting to auto-execute, but it can do almost arbitrary bad things, right?

Added.

https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download...
chrome/browser/download/download_extensions.cc:227: {"rpm",
ALLOW_ON_USER_GESTURE},
On 2015/06/09 at 19:40:04, rdsmith wrote:
> I think that "opening" a package on Linux installs it, which I believe
includes running a script, so I think these are executable equivalents.

Added.

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
File chrome/browser/download/download_prefs_unittest.cc (right):

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:25: const base::FilePath
kFileWithNoExtension(FILE_PATH_LITERAL("abcd"));
On 2015/06/09 at 21:29:25, rdsmith wrote:
> This doesn't look like it's used in this test (I'm a bit confused the compiler
didn't warn you about that).

Removed.

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:32:
EXPECT_FALSE(prefs.IsAutoOpenEnabledBasedOnExtension(kDangerousFilePath));
On 2015/06/09 at 21:29:25, rdsmith wrote:
> This looks like it's just testing .swf files, but that doesn't seem to match
the comment before the test; am I confused?

I don't quite follow. Which comment?

[Randy Smith (Not in Mondays), 2015-06-15 19:19:17]

LGTM; all comments below are suggestions that you can do with as you wish.

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
File chrome/browser/download/download_prefs_unittest.cc (right):

https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download...
chrome/browser/download/download_prefs_unittest.cc:32:
EXPECT_FALSE(prefs.IsAutoOpenEnabledBasedOnExtension(kDangerousFilePath));
On 2015/06/12 20:12:52, asanka wrote:
> On 2015/06/09 at 21:29:25, rdsmith wrote:
> > This looks like it's just testing .swf files, but that doesn't seem to match
> the comment before the test; am I confused?
> 
> I don't quite follow. Which comment?

Whoops, sorry, I meant the test name, not the comment. 
"NoAutoOpenForExecutables" implies to me that the test is either for all
executable files or in some way for the class of files that is executables. 
This is testing either just .swf files or the set of dangerous files.  I think
you've changed the name in the latest PS, so feel free to ignore this comment.

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:78: // ALLOW_ON_USER_GESTURE.
Even if the user explicitly consents to the
I'd rewrite this a bit to explicitly acknowledge that ALLOW_ON_USER_GESTURE
isn't a reliable security guarantee, more of a speed bump.

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:79: // download, they should be
able to choose when or if the file should be
The issue is whether they should be forced to rather than whether they be able
to choose to, right?

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:105: // Some files are dangerous
on all platforms.
I really like the addition of detailed comments to this list.  Of course, the
addition of those comments makes sharper the lack of comments for some file
types.  Not as part of this CL, but maybe long-term we should have a goal of
commenting all lines in this list?  WDYT?

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:70: // fail if the |file_name| isn't
allowed to open automatically.
I find this on second reading a little confusing.  I read these two sentences as
"Returns true on success.  One way to fail is if the file_name isn't allowed to
open automatically." which from an API contract point of view seems like either
too much or too little documentation.  If we trust the code, the only reason the
function will return false is if the file name isn't allowed to open
automatically; is that worth making clearer?  Or do we want to fuzz the API
contract a bit?

[asanka, 2015-06-16 19:23:42]

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/download_extensions.cc (right):

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:78: // ALLOW_ON_USER_GESTURE.
Even if the user explicitly consents to the
On 2015/06/15 at 19:19:17, rdsmith wrote:
> I'd rewrite this a bit to explicitly acknowledge that ALLOW_ON_USER_GESTURE
isn't a reliable security guarantee, more of a speed bump.

Yup. I moved the general overview over to download_extensions.h since that, IMO,
should be part of the public interface. I left the comment about specific
criteria here.

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:79: // download, they should be
able to choose when or if the file should be
On 2015/06/15 at 19:19:17, rdsmith wrote:
> The issue is whether they should be forced to rather than whether they be able
to choose to, right?

Yeah. Clarified.

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_extensions.cc:105: // Some files are dangerous
on all platforms.
On 2015/06/15 at 19:19:17, rdsmith wrote:
> I really like the addition of detailed comments to this list.  Of course, the
addition of those comments makes sharper the lack of comments for some file
types.  Not as part of this CL, but maybe long-term we should have a goal of
commenting all lines in this list?  WDYT?

Agree completely. I'll fill in the blanks that I'm comfortable with and add a
comment that all future additions should include a justification.

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:70: // fail if the |file_name| isn't
allowed to open automatically.
On 2015/06/15 at 19:19:17, rdsmith wrote:
> I find this on second reading a little confusing.  I read these two sentences
as "Returns true on success.  One way to fail is if the file_name isn't allowed
to open automatically." which from an API contract point of view seems like
either too much or too little documentation.  If we trust the code, the only
reason the function will return false is if the file name isn't allowed to open
automatically; is that worth making clearer?  Or do we want to fuzz the API
contract a bit?

Reworded.

[asanka, 2015-06-16 19:25:04]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-06-16 19:25:04]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1165893004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 19:25:30]

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

[commit-bot: I haz the power, 2015-06-16 20:23:27]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-06-16 20:24:25]

Patchset 10 (id:??) landed as
https://crrev.com/39841e54180e2583dffa16fbbb9b99fd293821d0
Cr-Commit-Position: refs/heads/master@{#334677}