Prefer opening PDF downloads in the browser.

Prefer opening PDF downloads in the browser.

PDFs in particular are safer to open in the browser. This patch changes
the handling of downloads to open such files in the browser by default
instead of the system handler for the file type. A "Open with system
handler" menu item will be available so that users can still use the
system application if needed.

The determination that a file is safer to handle in the browser is done
as follows:

a) DownloadTargetDeterminer determines whether the MIME type
   corresponding to the target filename of the download is one which is
   handled by the renderer or one that is handled by a sandboxed pepper
   plugin. If so, then the file is considered safely handled by the
   browser.

b) ChromeDownloadManagerDelegate determines whether opening in the
   browser is preferred for the file type assuming the browser is able
   to handle it safely. Currently this is true for .pdf files.

Opening behavior for a download will default to opening in the browser
if both results from a) and b) are true.

BUG=148492
TEST=(1) Make sure Chrome PDF Viewer is enabled in chrome://plugins.
     (2) Download a .pdf file.
     (3) Download shelf context menu should show "Open" and "Open with
         system handler" options.
     (4) "Open" as well as clicking on the shelf icon and clicking on
         the download in chrome://downloads should all result in opening
         the file within the browser.
     (5) "Open with system handler" should open the .pdf with the
         application that's set up as the default handler for .pdf files
         on the users' machine.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233460
Reverted : https://src.chromium.org/viewvc/chrome?view=rev&revision=233497

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234031

[asanka, 2013-10-31 17:27:39]

FYI. Working on tests.

[cbentzel, 2013-10-31 20:19:54]

On 2013/10/31 17:27:39, asanka wrote:
> FYI. Working on tests.

What happens if there is no PDF plugin, or it is disabled, or the plugin is
using NPAPI rather than PPAPI?

[palmer, 2013-10-31 21:09:14]

https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrom...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrom...
chrome/browser/download/chrome_download_manager_delegate.cc:173: if
(path.MatchesExtension(FILE_PATH_LITERAL(".pdf")))
Ideally, we should check by the website-declared MIME type, or by sniffed MIME
type.

Or, we should at least handle as many of the PDF-related file suffixes as our
plugin can handle. See https://codereview.chromium.org/49353005/

[asanka, 2013-11-01 14:23:55]

On 2013/10/31 20:19:54, cbentzel wrote:
> On 2013/10/31 17:27:39, asanka wrote:
> > FYI. Working on tests.
> 
> What happens if there is no PDF plugin, or it is disabled, or the plugin is
> using NPAPI rather than PPAPI?

The code already checks for the existence of a plug-in and also whether it is
enabled.

Good point about NPAPI vs. PPAPI. I believe this change presupposes the security
advantage of PPAPI vs. NPAPI. So I'll add a check to DownloadTargetDeterminer to
only check for sandboxed PPAPI plugins.

The types of plugins are:
    PLUGIN_TYPE_NPAPI,
    PLUGIN_TYPE_PEPPER_IN_PROCESS,
    PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS,
    PLUGIN_TYPE_PEPPER_UNSANDBOXED

I'm proposing that we only look for PEPPER_{IN,OUT_OF}_PROCESS plugins.

[asanka, 2013-11-01 14:29:36]

https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrom...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrom...
chrome/browser/download/chrome_download_manager_delegate.cc:173: if
(path.MatchesExtension(FILE_PATH_LITERAL(".pdf")))
On 2013/10/31 21:09:15, Chromium Palmer wrote:
> Ideally, we should check by the website-declared MIME type, or by sniffed MIME
> type.
> 
> Or, we should at least handle as many of the PDF-related file suffixes as our
> plugin can handle. See https://codereview.chromium.org/49353005/

I'm ignoring the website-declared MIME type and the sniffed MIME type here
because the only MIME type that affects plug-in selection for a file:// URL is
the one determined by net::GetMimeTypeFromFile(). Of course, we can change that
logic as well, but I think such a change would have to precede this CL.

Good point about other file types. I'm starting to wonder whether we should
whitelist specific plug-ins so that we would open any file type that is handled
by that plug-in. That way this code would be safe from bitrotting against
changes to handled filetypes for that plug-in.

[palmer, 2013-11-01 19:04:26]

> I'm proposing that we only look for PEPPER_{IN,OUT_OF}_PROCESS plugins.

SGTM.

[palmer, 2013-11-01 19:06:28]

> I'm ignoring the website-declared MIME type and the sniffed MIME type here
> because the only MIME type that affects plug-in selection for a file:// URL is
> the one determined by net::GetMimeTypeFromFile(). Of course, we can change
that
> logic as well, but I think such a change would have to precede this CL.

I see. Makes sense, thanks.

> Good point about other file types. I'm starting to wonder whether we should
> whitelist specific plug-ins so that we would open any file type that is
handled
> by that plug-in. That way this code would be safe from bitrotting against
> changes to handled filetypes for that plug-in.

In a future CL, yeah. For the immediate future and our immediate goals, what we
have is on the right track I think.

We need to get the strings in today before the string freeze, so if you don't
mind I'll commit your version of the strings (which make more sense than my
version) today, in a separate quickie CL.

[palmer, 2013-11-01 19:11:30]

LGTM, FWIW, BTW. :)

[asanka, 2013-11-02 00:15:18]

Randy: Could you have a look?

[cbentzel, 2013-11-04 15:14:38]

On 2013/11/01 19:06:28, Chromium Palmer wrote:
> > I'm ignoring the website-declared MIME type and the sniffed MIME type here
> > because the only MIME type that affects plug-in selection for a file:// URL
is
> > the one determined by net::GetMimeTypeFromFile(). Of course, we can change
> that
> > logic as well, but I think such a change would have to precede this CL.
> 
> I see. Makes sense, thanks.
> 
> > Good point about other file types. I'm starting to wonder whether we should
> > whitelist specific plug-ins so that we would open any file type that is
> handled
> > by that plug-in. That way this code would be safe from bitrotting against
> > changes to handled filetypes for that plug-in.
> 
> In a future CL, yeah. For the immediate future and our immediate goals, what
we
> have is on the right track I think.
> 
> We need to get the strings in today before the string freeze, so if you don't
> mind I'll commit your version of the strings (which make more sense than my
> version) today, in a separate quickie CL.

Did the string commit happen?

[asanka, 2013-11-04 17:28:30]

On 2013/11/04 15:14:38, cbentzel wrote:
> On 2013/11/01 19:06:28, Chromium Palmer wrote:
> > > I'm ignoring the website-declared MIME type and the sniffed MIME type here
> > > because the only MIME type that affects plug-in selection for a file://
URL
> is
> > > the one determined by net::GetMimeTypeFromFile(). Of course, we can change
> > that
> > > logic as well, but I think such a change would have to precede this CL.
> > 
> > I see. Makes sense, thanks.
> > 
> > > Good point about other file types. I'm starting to wonder whether we
should
> > > whitelist specific plug-ins so that we would open any file type that is
> > handled
> > > by that plug-in. That way this code would be safe from bitrotting against
> > > changes to handled filetypes for that plug-in.
> > 
> > In a future CL, yeah. For the immediate future and our immediate goals, what
> we
> > have is on the right track I think.
> > 
> > We need to get the strings in today before the string freeze, so if you
don't
> > mind I'll commit your version of the strings (which make more sense than my
> > version) today, in a separate quickie CL.
> 
> Did the string commit happen?

Yup. The strings landed in http://crrev.com/232508

[Chris Palmer, 2013-11-04 17:53:26]

Yes, it did.
On Nov 4, 2013 7:15 AM, <cbentzel@chromium.org> wrote:

> On 2013/11/01 19:06:28, Chromium Palmer wrote:
>
>> > I'm ignoring the website-declared MIME type and the sniffed MIME type
>> here
>> > because the only MIME type that affects plug-in selection for a file://
>> URL
>>
> is
>
>> > the one determined by net::GetMimeTypeFromFile(). Of course, we can
>> change
>> that
>> > logic as well, but I think such a change would have to precede this CL.
>>
>
>  I see. Makes sense, thanks.
>>
>
>  > Good point about other file types. I'm starting to wonder whether we
>> should
>> > whitelist specific plug-ins so that we would open any file type that is
>> handled
>> > by that plug-in. That way this code would be safe from bitrotting
>> against
>> > changes to handled filetypes for that plug-in.
>>
>
>  In a future CL, yeah. For the immediate future and our immediate goals,
>> what
>>
> we
>
>> have is on the right track I think.
>>
>
>  We need to get the strings in today before the string freeze, so if you
>> don't
>> mind I'll commit your version of the strings (which make more sense than
>> my
>> version) today, in a separate quickie CL.
>>
>
> Did the string commit happen?
>
> https://codereview.chromium.org/55063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Randy Smith (Not in Mondays), 2013-11-04 19:53:25]

So my basic reaction I think I communicated verbally when we talked, but I'll
include it here for the record: This is a pretty invasive change, for something
that just changes the behavior of an "Open".   We're changing two conceptual
pieces around opening behavior at once here: a) From "just leave it up to the
system" to "open some files ourselves, ask the system to open others", and b)
giving the user a (very narrow) choice in the browser about how to open a file
type.  The latter is what's driving the complexity, and I think it also makes
the user interface more complex than is ideal for the user, so long term I'd
like to consider killing it.  If we don't kill it, I'd suggest that we think
about how to refactor the "ways to open a file" into its own section of code,
and change the other code to call in there, possibly with more information
(target file mime type) stored in the download item.   But all of that's
long-term; for the moment, I'll accept the user interface choices we've made,
and I've reviewed the CL based on those UI choices.

I haven't reviewed the tests, and I'll want one more round on the main code
after these comments as well.  What's our deadline for this CL?

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:174: const
base::Callback<void(const std::string&)>& callback) {
Worth an assertion that we're on the IO thread?

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:187: if
(path.MatchesExtension(FILE_PATH_LITERAL(".pdf")))
Doesn't this duplicate the plugin information as to what plugins handle what
mime-types/file types?  Also, won't this break if the user has specifically
disabled the PDF plugin?

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:419: #if
!defined(OS_ANDROID)
Is there a reason not to hide the ifdef in the ShouldPreferOpeningInBrowser()
logic?

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:420: if
(DownloadItemModel(download).ShouldPreferOpeningInBrowser()) {
Suggestion: I'm tempted to suggest reversing the text and code inclusion (i.e.
making the OpenDownloadUsingPlatformHandler() the if dependency).  That goes
(somewhat) against the "common path should be outside of if" guideline, but it
will result in a lot less indented code, which might make the code a bit easier
to read.  Not sure what's best here, so just raising the idea.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/download_item_model.h (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_item_model.h:134: void
SetShouldPreferOpeningInBrowser(bool preference);
As gestured at in the high level comment, an alternative to this implementation
would be to store the target path mime type in the download item, and make the
determination of whether or not we should be opening using a platform handler a
pure functional check at all points at which we need the information.  In some
sense that'd be equivalent to this implementation, as DownloadItemModel just
uses user data to store information associated with the DownloadItem.  So it's
an aesthetics preference.  But I don't really think of
"ShouldPreferOpeningInBrowser()" as a pure UI preference; it's derivative of
other non-UI issues, and how "Open" is implemented isn't really about the UI. 
And target path mime type is indeed information associated with the download
(destination).  But I'm not requesting the change; it's something of an
aesthetic preference, and if I saw the other code maybe I wouldn't like that
either :-}.  But I figured I'd raise the possibility for your consideration.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:95: #endif  //
ENABLE_PLUGINS
I'm a bit torn about this code organization.  On the one hand, it keeps the
plugin logic together.  OTOH, you need to jump around in the file to figure out
what the code's doing.  My belief is that the code would be a bit clearer if you
moved the reference to IsSafePluginVaialbleForProfile() down to
DetermineIfHandledByBrowserDone().  But it's not a strong feeling, and if you
feel differently, feel free to keep this layout.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:616:
target_info->is_filetype_handled_securely = is_filetype_handled_securely_;
Long-term thought: One of the goals I had for the refactor that we never got to
was to have sub-structures of DownloadItem "DownloadSourceInfo" (net/url info),
"DownloadTargetInfo" (for file info), + a few others.  If you/we ever do that
refactor, it's possible that the target info struct could be used here.

[asanka, 2013-11-04 21:20:10]

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:174: const
base::Callback<void(const std::string&)>& callback) {
On 2013/11/04 19:53:26, rdsmith wrote:
> Worth an assertion that we're on the IO thread?

This is actually called on the blocking pool. The only requirement is that
GetMimeTypeFromFile be allowed to block on IO if the system MIME associations
have to be looked up. This is asserted with an 
base::ThreadRestrictions::AssertIOAllowed() deep in the guts of
GetMimeTypeFromFile().

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:187: if
(path.MatchesExtension(FILE_PATH_LITERAL(".pdf")))
On 2013/11/04 19:53:26, rdsmith wrote:
> Doesn't this duplicate the plugin information as to what plugins handle what
> mime-types/file types?  Also, won't this break if the user has specifically
> disabled the PDF plugin?

I'll fiddle with the names to make this clearer, but there are several inputs
into the decision:

- Whether the browser is capable of handling the file type securely: This is
determined in DownloadTargetDeterminer and takes into consideration whether any
sandboxed plugins are available and enabled to handle the file type.

- Whether we want to handle this file in the browser if it is possible to do so:
Not every file that can be handled in the browser should be handled. At least
for now. This decision is made here in IsOpenInBrowserPreferredForFile().

A file will default to being opened in the browser if both of the above are
true.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:419: #if
!defined(OS_ANDROID)
On 2013/11/04 19:53:26, rdsmith wrote:
> Is there a reason not to hide the ifdef in the ShouldPreferOpeningInBrowser()
> logic?

Moved to IsOpenInBrowserPreferredForFile.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:420: if
(DownloadItemModel(download).ShouldPreferOpeningInBrowser()) {
On 2013/11/04 19:53:26, rdsmith wrote:
> Suggestion: I'm tempted to suggest reversing the text and code inclusion (i.e.
> making the OpenDownloadUsingPlatformHandler() the if dependency).  That goes
> (somewhat) against the "common path should be outside of if" guideline, but it
> will result in a lot less indented code, which might make the code a bit
easier
> to read.  Not sure what's best here, so just raising the idea.

Done.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/download_item_model.h (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_item_model.h:134: void
SetShouldPreferOpeningInBrowser(bool preference);
On 2013/11/04 19:53:26, rdsmith wrote:
> As gestured at in the high level comment, an alternative to this
implementation
> would be to store the target path mime type in the download item, and make the
> determination of whether or not we should be opening using a platform handler
a
> pure functional check at all points at which we need the information.  In some
> sense that'd be equivalent to this implementation, as DownloadItemModel just
> uses user data to store information associated with the DownloadItem.  So it's
> an aesthetics preference.  But I don't really think of
> "ShouldPreferOpeningInBrowser()" as a pure UI preference; it's derivative of
> other non-UI issues, and how "Open" is implemented isn't really about the UI. 
> And target path mime type is indeed information associated with the download
> (destination).  But I'm not requesting the change; it's something of an
> aesthetic preference, and if I saw the other code maybe I wouldn't like that
> either :-}.  But I figured I'd raise the possibility for your consideration.

It's a valid point.

However, the deciding whether a something should be opened in the browser based
on the MIME type isn't trivial and requires consulting data that's not available
on the UI thread. That's why I opted to make the decision early and store it in
the DownloadItemModel. And since the decision itself didn't belong in //content
(responsibility for opening a download is delegated to DownloadManagerDelegate
and hence to the embedder), I kept it in DownloadItemModel instead of adding it
to DownloadItem.

How the layering works for handling of 'open' is something that we'd need to
revisit when we are considering the componentization of downloads. But for now,
let me know if this addresses the issue you raise.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:95: #endif  //
ENABLE_PLUGINS
On 2013/11/04 19:53:26, rdsmith wrote:
> I'm a bit torn about this code organization.  On the one hand, it keeps the
> plugin logic together.  OTOH, you need to jump around in the file to figure
out
> what the code's doing.  My belief is that the code would be a bit clearer if
you
> moved the reference to IsSafePluginVaialbleForProfile() down to
> DetermineIfHandledByBrowserDone().  But it's not a strong feeling, and if you
> feel differently, feel free to keep this layout.

I tried that, but it still seemed a bit confusing. I moved this block down next
to DetermineIfHandledByBrowserDone so that it would at least be less effort to
figure out what the code is doing.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:616:
target_info->is_filetype_handled_securely = is_filetype_handled_securely_;
On 2013/11/04 19:53:26, rdsmith wrote:
> Long-term thought: One of the goals I had for the refactor that we never got
to
> was to have sub-structures of DownloadItem "DownloadSourceInfo" (net/url
info),
> "DownloadTargetInfo" (for file info), + a few others.  If you/we ever do that
> refactor, it's possible that the target info struct could be used here.

Yup. We can look at it when we revisit the handling of 'Open' and how download
completion is blocked. Currently much of DownloadTargetInfo is concerned with
these.

[asanka, 2013-11-04 21:30:29]

On 2013/11/04 19:53:25, rdsmith wrote:
> So my basic reaction I think I communicated verbally when we talked, but I'll
> include it here for the record: This is a pretty invasive change, for
something
> that just changes the behavior of an "Open".   We're changing two conceptual
> pieces around opening behavior at once here: a) From "just leave it up to the
> system" to "open some files ourselves, ask the system to open others", and b)
> giving the user a (very narrow) choice in the browser about how to open a file
> type.  The latter is what's driving the complexity, and I think it also makes
> the user interface more complex than is ideal for the user, so long term I'd
> like to consider killing it.  If we don't kill it, I'd suggest that we think
> about how to refactor the "ways to open a file" into its own section of code,
> and change the other code to call in there, possibly with more information
> (target file mime type) stored in the download item.   But all of that's
> long-term; for the moment, I'll accept the user interface choices we've made,
> and I've reviewed the CL based on those UI choices.
> 
> I haven't reviewed the tests, and I'll want one more round on the main code
> after these comments as well.  What's our deadline for this CL?

Point taken about the UI complexity, but I'm also willing to go with it for the
short term.

The code complexity derives from how we have to determine whether a file can be
opened safely within the browser (the MIME type lookup may block and the plugin
list lives on the IO thread). Also the thread hopping could race with a
DownloadItem lifetime. To address both, I opted to do the determination early in
DownloadTargetDeterminer and store it in DownloadItemModel.

We could conceivably do this work when we are opening the download. That would
relieve DownloadItemModel / DownloadShelfContextMenu from needing to know how
Chrome intends to open the file. I agree that would be cleaner but would come at
the cost of not giving the user an escape hatch. If UI/security folks are okay
with that, I can make this change. Otherwise we can go with the current plan
(allow the user to choose whether to use the system handler) for now and then
decide whether to remove the option in the next release.

[asanka, 2013-11-04 21:33:48]

On 2013/11/04 21:30:29, asanka wrote:
> On 2013/11/04 19:53:25, rdsmith wrote:
> > So my basic reaction I think I communicated verbally when we talked, but
I'll
> > include it here for the record: This is a pretty invasive change, for
> something
> > that just changes the behavior of an "Open".   We're changing two conceptual
> > pieces around opening behavior at once here: a) From "just leave it up to
the
> > system" to "open some files ourselves, ask the system to open others", and
b)
> > giving the user a (very narrow) choice in the browser about how to open a
file
> > type.  The latter is what's driving the complexity, and I think it also
makes
> > the user interface more complex than is ideal for the user, so long term I'd
> > like to consider killing it.  If we don't kill it, I'd suggest that we think
> > about how to refactor the "ways to open a file" into its own section of
code,
> > and change the other code to call in there, possibly with more information
> > (target file mime type) stored in the download item.   But all of that's
> > long-term; for the moment, I'll accept the user interface choices we've
made,
> > and I've reviewed the CL based on those UI choices.
> > 
> > I haven't reviewed the tests, and I'll want one more round on the main code
> > after these comments as well.  What's our deadline for this CL?

Technically the deadline was last Friday :). Practically it's the branch cut
tonight. Realistically, we can land this as soon as it's ready and ask for a
merge.

> Point taken about the UI complexity, but I'm also willing to go with it for
the
> short term.
> 
> The code complexity derives from how we have to determine whether a file can
be
> opened safely within the browser (the MIME type lookup may block and the
plugin
> list lives on the IO thread). Also the thread hopping could race with a
> DownloadItem lifetime. To address both, I opted to do the determination early
in
> DownloadTargetDeterminer and store it in DownloadItemModel.
> 
> We could conceivably do this work when we are opening the download. That would
> relieve DownloadItemModel / DownloadShelfContextMenu from needing to know how
> Chrome intends to open the file. I agree that would be cleaner but would come
at
> the cost of not giving the user an escape hatch. If UI/security folks are okay
> with that, I can make this change. Otherwise we can go with the current plan
> (allow the user to choose whether to use the system handler) for now and then
> decide whether to remove the option in the next release.

[Randy Smith (Not in Mondays), 2013-11-04 22:29:41]

On 2013/11/04 21:30:29, asanka wrote:
> On 2013/11/04 19:53:25, rdsmith wrote:
> > So my basic reaction I think I communicated verbally when we talked, but
I'll
> > include it here for the record: This is a pretty invasive change, for
> something
> > that just changes the behavior of an "Open".   We're changing two conceptual
> > pieces around opening behavior at once here: a) From "just leave it up to
the
> > system" to "open some files ourselves, ask the system to open others", and
b)
> > giving the user a (very narrow) choice in the browser about how to open a
file
> > type.  The latter is what's driving the complexity, and I think it also
makes
> > the user interface more complex than is ideal for the user, so long term I'd
> > like to consider killing it.  If we don't kill it, I'd suggest that we think
> > about how to refactor the "ways to open a file" into its own section of
code,
> > and change the other code to call in there, possibly with more information
> > (target file mime type) stored in the download item.   But all of that's
> > long-term; for the moment, I'll accept the user interface choices we've
made,
> > and I've reviewed the CL based on those UI choices.
> > 
> > I haven't reviewed the tests, and I'll want one more round on the main code
> > after these comments as well.  What's our deadline for this CL?
> 
> Point taken about the UI complexity, but I'm also willing to go with it for
the
> short term.
> 
> The code complexity derives from how we have to determine whether a file can
be
> opened safely within the browser (the MIME type lookup may block and the
plugin
> list lives on the IO thread). Also the thread hopping could race with a
> DownloadItem lifetime. To address both, I opted to do the determination early
in
> DownloadTargetDeterminer and store it in DownloadItemModel.
> 
> We could conceivably do this work when we are opening the download. That would
> relieve DownloadItemModel / DownloadShelfContextMenu from needing to know how
> Chrome intends to open the file. I agree that would be cleaner but would come
at
> the cost of not giving the user an escape hatch. If UI/security folks are okay
> with that, I can make this change. Otherwise we can go with the current plan
> (allow the user to choose whether to use the system handler) for now and then
> decide whether to remove the option in the next release.

That last is what I'm assuming, and I'm comfortable with that.  But to be clear,
my long-term argument is: I think giving the user the escape hatch is a mistake,
because a) it makes more complex the user interaction in a way that isn't
particularly clean (we're not doing any general options about how to open files,
we're just allowing this one choice in a corner case when we want to open the
file in the browser) and b) the user already has an escape hatch, called the
system shell :-}.  We should have the user behavior discussion separately from
the code implementation discussion, but whichever way we decide to go long-term
around the behavior discussion, there are changes I'd like to make to the code
implementation (either bundle this all up in "open", or bundle it somewhere else
and have the code mechanism for "choosing options for open" be more general,
even if we don't expose that to the user).

[palmer, 2013-11-04 22:34:56]

> The code complexity derives from how we have to determine whether a file can
be
> opened safely within the browser (the MIME type lookup may block and the
plugin
> list lives on the IO thread). Also the thread hopping could race with a
> DownloadItem lifetime. To address both, I opted to do the determination early
in
> DownloadTargetDeterminer and store it in DownloadItemModel.
> 
> We could conceivably do this work when we are opening the download. That would
> relieve DownloadItemModel / DownloadShelfContextMenu from needing to know how
> Chrome intends to open the file. I agree that would be cleaner but would come
at
> the cost of not giving the user an escape hatch. If UI/security folks are okay
> with that, I can make this change. Otherwise we can go with the current plan
> (allow the user to choose whether to use the system handler) for now and then
> decide whether to remove the option in the next release.

I think it is best to go with the current plan. We should not take away the
user's escape hatch; making the affordance stronger for opening in Chrome when
possible is enough, from a security perspective.

[Randy Smith (Not in Mondays), 2013-11-04 22:57:46]

My apologies--I didn't manage to get to reviewing the test, and I have to take
off now.

I'd like to request a comment that summarizes the entire change (that we're
figuring out both a) whether we want to open it in the browser and b) whether we
can do so securely, where we're doing that and why it's done so early, and what
might allow us to simplify that in the future).  I'm torn about location; the
two options that have occurred to me are a) in download_target_determiner.cc (as
that's where the primary complex code is), or b) In the description for this CL
(so that if anyone goes looking for "Why is X code here?" they'll find the
commit, look at the log message and go "Oh!").  I think of the two I'd prefer
the commit message/CL description, but if you have a preference the other way
I'm good with that--it feels weird to put a code description comment somewhere
other than the code :-}.

I'll do the test review first thing tomorrow.

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
File chrome/browser/download/download_item_model.h (right):

https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/...
chrome/browser/download/download_item_model.h:134: void
SetShouldPreferOpeningInBrowser(bool preference);
On 2013/11/04 21:20:11, asanka wrote:
> On 2013/11/04 19:53:26, rdsmith wrote:
> > As gestured at in the high level comment, an alternative to this
> implementation
> > would be to store the target path mime type in the download item, and make
the
> > determination of whether or not we should be opening using a platform
handler
> a
> > pure functional check at all points at which we need the information.  In
some
> > sense that'd be equivalent to this implementation, as DownloadItemModel just
> > uses user data to store information associated with the DownloadItem.  So
it's
> > an aesthetics preference.  But I don't really think of
> > "ShouldPreferOpeningInBrowser()" as a pure UI preference; it's derivative of
> > other non-UI issues, and how "Open" is implemented isn't really about the
UI. 
> > And target path mime type is indeed information associated with the download
> > (destination).  But I'm not requesting the change; it's something of an
> > aesthetic preference, and if I saw the other code maybe I wouldn't like that
> > either :-}.  But I figured I'd raise the possibility for your consideration.
> 
> It's a valid point.
> 
> However, the deciding whether a something should be opened in the browser
based
> on the MIME type isn't trivial and requires consulting data that's not
available
> on the UI thread. That's why I opted to make the decision early and store it
in
> the DownloadItemModel. And since the decision itself didn't belong in
//content
> (responsibility for opening a download is delegated to DownloadManagerDelegate
> and hence to the embedder), I kept it in DownloadItemModel instead of adding
it
> to DownloadItem.
> 
> How the layering works for handling of 'open' is something that we'd need to
> revisit when we are considering the componentization of downloads. But for
now,
> let me know if this addresses the issue you raise.

Yes, it does; I hadn't fully groked that we were combining two pieces of
information (whether we wanted to open things in the browser == net supported
image type or .pdf, and whether we could open them securely == net supported
mime type or have a plugin loaded for them).

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:408: // handle |mime_type|
for a specific profile. Must be called on the IO thread.
nit: The second sentence can be ambiguously read as applying either to this
function or the callback.  Disambiguation could be done by saying where the
callback must be called as well?  Or interchanging the two sentences?

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:448:
base::Bind(&DownloadTargetDeterminer::DetermineIfHandledByBrowserDone,
Just confirming: I read this code as using the mime_type to determine the
is_filetype_handled_securely_ question, based (in the PDF case) on whether or
not we have a PDF plugin that securely handles the PDF mime type.  For this to
match the actual open code, the way that an open that uses the PDF plugin would
have to work is that visiting a file:://<path>.pdf file  would need to go
through the net logic to figure out the mime type, and then go through the
plugin list to find a plugin that matches it.  But I have a memory that the
plugin list also includes file suffixes.  Have you confirmed that the actual URL
opening code matches the probe that's being done here (i.e. suffix -> MIME type
-> plugin list, as opposed to suffix -> plugin list directly)?

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
File chrome/browser/download/download_target_info.h (right):

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_info.h:19: base::FilePath
intermediate_path;
nit: There should probably be a sentence comment explaining each of the above
variables for consistency.

[Randy Smith (Not in Mondays), 2013-11-04 23:02:13]

On 2013/11/04 22:34:56, Chromium Palmer wrote:
> > The code complexity derives from how we have to determine whether a file can
> be
> > opened safely within the browser (the MIME type lookup may block and the
> plugin
> > list lives on the IO thread). Also the thread hopping could race with a
> > DownloadItem lifetime. To address both, I opted to do the determination
early
> in
> > DownloadTargetDeterminer and store it in DownloadItemModel.
> > 
> > We could conceivably do this work when we are opening the download. That
would
> > relieve DownloadItemModel / DownloadShelfContextMenu from needing to know
how
> > Chrome intends to open the file. I agree that would be cleaner but would
come
> at
> > the cost of not giving the user an escape hatch. If UI/security folks are
okay
> > with that, I can make this change. Otherwise we can go with the current plan
> > (allow the user to choose whether to use the system handler) for now and
then
> > decide whether to remove the option in the next release.
> 
> I think it is best to go with the current plan. We should not take away the
> user's escape hatch; making the affordance stronger for opening in Chrome when
> possible is enough, from a security perspective.

I'd like a chance to dispute this opinion for reasons of best UI (not security)
at some point in the future :-}.

[palmer, 2013-11-04 23:48:09]

> > I think it is best to go with the current plan. We should not take away the
> > user's escape hatch; making the affordance stronger for opening in Chrome
when
> > possible is enough, from a security perspective.
> 
> I'd like a chance to dispute this opinion for reasons of best UI (not
security)
> at some point in the future :-}.

Sure. But how is it good UI to force users into one choice that may not fit
them? Our PDF reader definitely does not support some features that some users
need, and we'll hear about it if we try to shoe-horn them into our preferred
choice.

[palmer, 2013-11-04 23:48:09]

> > I think it is best to go with the current plan. We should not take away the
> > user's escape hatch; making the affordance stronger for opening in Chrome
when
> > possible is enough, from a security perspective.
> 
> I'd like a chance to dispute this opinion for reasons of best UI (not
security)
> at some point in the future :-}.

Sure. But how is it good UI to force users into one choice that may not fit
them? Our PDF reader definitely does not support some features that some users
need, and we'll hear about it if we try to shoe-horn them into our preferred
choice.

[palmer, 2013-11-05 00:19:03]

Hmm, when I build and run Chrome with this patch, it doesn't open PDFs in
Chrome. Is it not ready? What should I expect at this point?

[asanka, 2013-11-05 00:28:23]

On 2013/11/05 00:19:03, Chromium Palmer wrote:
> Hmm, when I build and run Chrome with this patch, it doesn't open PDFs in
> Chrome. Is it not ready? What should I expect at this point?

Do you see the Chrome PDF Viewer plugin in chrome://plugins?

If the plug-in is available, after you download a .pdf, you should see an "Open"
option and a "Open with system viewer" option in the context menu. The "Open"
option or clicking on the download item should open the PDF in the browser.

This should also happen for images.

[palmer, 2013-11-05 00:48:01]

> Do you see the Chrome PDF Viewer plugin in chrome://plugins?

Ahh, my bad; I had a profile setting to block all plugins. Hence it did not
appear as available to your code. When I made the PDF plugin always enabled, it
works as you say. Sorry about that.

[Randy Smith (Not in Mondays), 2013-11-05 15:31:18]

On 2013/11/04 23:48:09, Chromium Palmer wrote:
> > > I think it is best to go with the current plan. We should not take away
the
> > > user's escape hatch; making the affordance stronger for opening in Chrome
> when
> > > possible is enough, from a security perspective.
> > 
> > I'd like a chance to dispute this opinion for reasons of best UI (not
> security)
> > at some point in the future :-}.
> 
> Sure. But how is it good UI to force users into one choice that may not fit
> them? Our PDF reader definitely does not support some features that some users
> need, and we'll hear about it if we try to shoe-horn them into our preferred
> choice.

On downloads we generally have a background dull roar of user complaints (*);
we're continually having to make choices that upset some people and satisfy
others.  One thing people have asked for for years is the ability to specify
what program is used to open the download at download time ("Open With").  We've
historically said that we're not in the business of opening files; we hand them
off to the OS, and the OS opens them.  This is on the basis of simplicity;
there's a lot of mechanism in the OS for opening files, and we don't want to
reproduce that.

But this change moves a bit in that direction--it gives the user a very small
choice about how they open one specific category of file.  It complicates the UI
(both simply by adding an entry to the context menu, which we've also
historically had a lot of resistance to, and by having the concept of different
ways to open files).  And it gives users a bit of what they've wanted, but not
the whole thing.

Even without this change, we aren't forcing the users to open files with Chrome.
 If they want to open them some other way, they can click "Show in Finder" (or
whatever is it--I can't check right now), then double click or right click the
file icon.  It's a few more clicks, but forcing a few more clicks for the
minority of users for a simpler UI overall strikes me as a no-brainer. 

Of course, we don't really know that it's a minority of users, which is why I'm
not pushing this argument hard right now (well, that, and M32).  But I don't
think that adding an "Open with platform opener" entry to the context menu is
consistent with the UI decisions we've made to date on downloads.

(*) Mind you, one big thread has been around PDFs being a dangerous file, so
this change may actually make a perceptible change for the better in user
complaints.

[Randy Smith (Not in Mondays), 2013-11-05 15:32:07]

Asanka: In the context of this argument, could you make sure in this CL that
there's UMA to check how many times the user choose the "Platform open" open in
the context of how often they open the download?

On 2013/11/05 15:31:18, rdsmith wrote:
> On 2013/11/04 23:48:09, Chromium Palmer wrote:
> > > > I think it is best to go with the current plan. We should not take away
> the
> > > > user's escape hatch; making the affordance stronger for opening in
Chrome
> > when
> > > > possible is enough, from a security perspective.
> > > 
> > > I'd like a chance to dispute this opinion for reasons of best UI (not
> > security)
> > > at some point in the future :-}.
> > 
> > Sure. But how is it good UI to force users into one choice that may not fit
> > them? Our PDF reader definitely does not support some features that some
users
> > need, and we'll hear about it if we try to shoe-horn them into our preferred
> > choice.
> 
> On downloads we generally have a background dull roar of user complaints (*);
> we're continually having to make choices that upset some people and satisfy
> others.  One thing people have asked for for years is the ability to specify
> what program is used to open the download at download time ("Open With"). 
We've
> historically said that we're not in the business of opening files; we hand
them
> off to the OS, and the OS opens them.  This is on the basis of simplicity;
> there's a lot of mechanism in the OS for opening files, and we don't want to
> reproduce that.
> 
> But this change moves a bit in that direction--it gives the user a very small
> choice about how they open one specific category of file.  It complicates the
UI
> (both simply by adding an entry to the context menu, which we've also
> historically had a lot of resistance to, and by having the concept of
different
> ways to open files).  And it gives users a bit of what they've wanted, but not
> the whole thing.
> 
> Even without this change, we aren't forcing the users to open files with
Chrome.
>  If they want to open them some other way, they can click "Show in Finder" (or
> whatever is it--I can't check right now), then double click or right click the
> file icon.  It's a few more clicks, but forcing a few more clicks for the
> minority of users for a simpler UI overall strikes me as a no-brainer. 
> 
> Of course, we don't really know that it's a minority of users, which is why
I'm
> not pushing this argument hard right now (well, that, and M32).  But I don't
> think that adding an "Open with platform opener" entry to the context menu is
> consistent with the UI decisions we've made to date on downloads.
> 
> (*) Mind you, one big thread has been around PDFs being a dangerous file, so
> this change may actually make a perceptible change for the better in user
> complaints.

[Randy Smith (Not in Mondays), 2013-11-05 16:52:39]

Oh, I've done the test review, and this looks basically good.  Let me know what
you think of the requests I've made so far (key ones are high level comment and
validating that the secure determination pathway for figuring if there's a
plugin actually matches the pathway used when we navigate to a file), and I'll
stamp.

https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:2000:
EXPECT_EQ(test_case.expected_mime_type, target_info->mime_type);
nit: Why doesn't VerifyDownloadTarget do this/isn't the expected_mime_type part
of download test case?

[asanka, 2013-11-05 21:31:38]

I'll update the CL description.

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:408: // handle |mime_type|
for a specific profile. Must be called on the IO thread.
On 2013/11/04 22:57:47, rdsmith wrote:
> nit: The second sentence can be ambiguously read as applying either to this
> function or the callback.  Disambiguation could be done by saying where the
> callback must be called as well?  Or interchanging the two sentences?

Done. Reworded to explicitly state thread restrictions for the callback and the
function.

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:448:
base::Bind(&DownloadTargetDeterminer::DetermineIfHandledByBrowserDone,
On 2013/11/04 22:57:47, rdsmith wrote:
> Just confirming: I read this code as using the mime_type to determine the
> is_filetype_handled_securely_ question, based (in the PDF case) on whether or
> not we have a PDF plugin that securely handles the PDF mime type.  For this to
> match the actual open code, the way that an open that uses the PDF plugin
would
> have to work is that visiting a file:://<path>.pdf file  would need to go
> through the net logic to figure out the mime type, and then go through the
> plugin list to find a plugin that matches it.  But I have a memory that the
> plugin list also includes file suffixes.  Have you confirmed that the actual
URL
> opening code matches the probe that's being done here (i.e. suffix -> MIME
type
> -> plugin list, as opposed to suffix -> plugin list directly)?

The flow for file:// URLs is:
//net:
  - MIME type for the UrlRequest is determined using net::GetMimeTypeFromFile()
by URLRequestFileJob

//content:
  - MIME type overridden by BufferedResourceHandler if the MIME type is empty or
one of kSniffableTypes.
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...).

Renderer:
  - Renderer creates WebCore::PluginDocument to render content, which
instantiates the plugin via a WebCore::HTMLEmbedElement with src=<file://url>
type=<MIME type from above>.
  - HTMLEmbedElement instantiates the plug-in which is eventually handled by
RenderFrameImpl::createPlugin().
  - ViewHostMsg_GetPluginInfo is sent back to browser.

Browser:
  - PluginInfoMessageFilter::OnGetPluginInfo() is called with file://url and
MIME type.
  - PluginService::GetPluginInfoArray is called which adds plugins based on MIME
type first and then based on URL.
  - PluginInfoMessageFilter::Context::FindEnabledPlugin() returns the first
plugin that is enabled from previous list.

...

I've updated the logic to use the file:// URL as well as the MIME type to more
closely resemble the underlying logic. Thanks for reminding me!

The difference in logic between the DownloadTargetDeterminer and the browsing
flow is in BufferedResourceHandler. Our decision that the file type is handled
securely might be wrong if something like the following (very contrived
scenario) happens:

- Some application associates .pdf with text/plain. text/plain is handled by the
renderer, so we would consider this to be handled securely. However text/plain
is also a sniffable MIME type.
- BufferedResourceHandler sniffs the contents and determines it's
application/pdf.
- Opening file://.../foo.pdf therefore results in instantiating the plug-in
associated with application/pdf which may not be our sandboxed plug-in.

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
File chrome/browser/download/download_target_info.h (right):

https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
chrome/browser/download/download_target_info.h:19: base::FilePath
intermediate_path;
On 2013/11/04 22:57:47, rdsmith wrote:
> nit: There should probably be a sentence comment explaining each of the above
> variables for consistency.

Done.

https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
chrome/browser/download/download_target_determiner_unittest.cc:2000:
EXPECT_EQ(test_case.expected_mime_type, target_info->mime_type);
On 2013/11/05 16:52:40, rdsmith wrote:
> nit: Why doesn't VerifyDownloadTarget do this/isn't the expected_mime_type
part
> of download test case? 

DownloadTestCase is currently overspecified. Most of the tests don't use all the
fields, but must supply them anyway and in turn become sensitive to unrelated
changes. So I was a bit reluctant to add another field that's only used in one
test. I'll clean up this test suite, but that seemed overkill for this CL.

[Randy Smith (Not in Mondays), 2013-11-05 21:43:12]

On 2013/11/05 21:31:38, asanka wrote:
> The flow for file:// URLs is:
> //net:
>   - MIME type for the UrlRequest is determined using
net::GetMimeTypeFromFile()
> by URLRequestFileJob
> 
> //content:
>   - MIME type overridden by BufferedResourceHandler if the MIME type is empty
or
> one of kSniffableTypes.
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...).
> 
> Renderer:
>   - Renderer creates WebCore::PluginDocument to render content, which
> instantiates the plugin via a WebCore::HTMLEmbedElement with src=<file://url>
> type=<MIME type from above>.
>   - HTMLEmbedElement instantiates the plug-in which is eventually handled by
> RenderFrameImpl::createPlugin().
>   - ViewHostMsg_GetPluginInfo is sent back to browser.
> 
> Browser:
>   - PluginInfoMessageFilter::OnGetPluginInfo() is called with file://url and
> MIME type.
>   - PluginService::GetPluginInfoArray is called which adds plugins based on
MIME
> type first and then based on URL.
>   - PluginInfoMessageFilter::Context::FindEnabledPlugin() returns the first
> plugin that is enabled from previous list.
> 
> ...
> 
> I've updated the logic to use the file:// URL as well as the MIME type to more
> closely resemble the underlying logic. Thanks for reminding me!
> 
> The difference in logic between the DownloadTargetDeterminer and the browsing
> flow is in BufferedResourceHandler. Our decision that the file type is handled
> securely might be wrong if something like the following (very contrived
> scenario) happens:
> 
> - Some application associates .pdf with text/plain. text/plain is handled by
the
> renderer, so we would consider this to be handled securely. However text/plain
> is also a sniffable MIME type.
> - BufferedResourceHandler sniffs the contents and determines it's
> application/pdf.
> - Opening file://.../foo.pdf therefore results in instantiating the plug-in
> associated with application/pdf which may not be our sandboxed plug-in.

My major concern in this space is if there's a contrived example that someone on
the web might be able to construct to get past our security.  Given that our
security (for PDFs, at least) is relying on having the PDF plugin installed and
enabled, and we're just going to use the system service if it's not, I think
we're ok--from your description even if the web site serves it as text/plain,
it'll be sniffed as PDF, and that'll get it to the PDF plugin (if installed and
enabled).  

I guess I'll just do one more probe: If we make the assumption that a website
spoofs the mime-type and makes a PDF not sniffable as PDF by
BufferredResourceHandler, but still downloads with a .pdf suffix, will it be
routed to the PDF plugin?  Or will it be passed to the system to open?  If the
latter, I think that's a security hole that should get security review before
this goes in.

I'll review the last change now; please send another email (or ping me via IM)
when you've updated the CL description.
> 
>
https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
> File chrome/browser/download/download_target_info.h (right):
> 
>
https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
> chrome/browser/download/download_target_info.h:19: base::FilePath
> intermediate_path;
> On 2013/11/04 22:57:47, rdsmith wrote:
> > nit: There should probably be a sentence comment explaining each of the
above
> > variables for consistency.
> 
> Done.
> 
>
https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
> File chrome/browser/download/download_target_determiner_unittest.cc (right):
> 
>
https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
> chrome/browser/download/download_target_determiner_unittest.cc:2000:
> EXPECT_EQ(test_case.expected_mime_type, target_info->mime_type);
> On 2013/11/05 16:52:40, rdsmith wrote:
> > nit: Why doesn't VerifyDownloadTarget do this/isn't the expected_mime_type
> part
> > of download test case? 
> 
> DownloadTestCase is currently overspecified. Most of the tests don't use all
the
> fields, but must supply them anyway and in turn become sensitive to unrelated
> changes. So I was a bit reluctant to add another field that's only used in one
> test. I'll clean up this test suite, but that seemed overkill for this CL.

[asanka, 2013-11-05 21:56:59]

On 2013/11/05 21:43:12, rdsmith wrote:
> On 2013/11/05 21:31:38, asanka wrote:
> > The flow for file:// URLs is:
> > //net:
> >   - MIME type for the UrlRequest is determined using
> net::GetMimeTypeFromFile()
> > by URLRequestFileJob
> > 
> > //content:
> >   - MIME type overridden by BufferedResourceHandler if the MIME type is
empty
> or
> > one of kSniffableTypes.
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...).
> > 
> > Renderer:
> >   - Renderer creates WebCore::PluginDocument to render content, which
> > instantiates the plugin via a WebCore::HTMLEmbedElement with
src=<file://url>
> > type=<MIME type from above>.
> >   - HTMLEmbedElement instantiates the plug-in which is eventually handled by
> > RenderFrameImpl::createPlugin().
> >   - ViewHostMsg_GetPluginInfo is sent back to browser.
> > 
> > Browser:
> >   - PluginInfoMessageFilter::OnGetPluginInfo() is called with file://url and
> > MIME type.
> >   - PluginService::GetPluginInfoArray is called which adds plugins based on
> MIME
> > type first and then based on URL.
> >   - PluginInfoMessageFilter::Context::FindEnabledPlugin() returns the first
> > plugin that is enabled from previous list.
> > 
> > ...
> > 
> > I've updated the logic to use the file:// URL as well as the MIME type to
more
> > closely resemble the underlying logic. Thanks for reminding me!
> > 
> > The difference in logic between the DownloadTargetDeterminer and the
browsing
> > flow is in BufferedResourceHandler. Our decision that the file type is
handled
> > securely might be wrong if something like the following (very contrived
> > scenario) happens:
> > 
> > - Some application associates .pdf with text/plain. text/plain is handled by
> the
> > renderer, so we would consider this to be handled securely. However
text/plain
> > is also a sniffable MIME type.
> > - BufferedResourceHandler sniffs the contents and determines it's
> > application/pdf.
> > - Opening file://.../foo.pdf therefore results in instantiating the plug-in
> > associated with application/pdf which may not be our sandboxed plug-in.
> 
> My major concern in this space is if there's a contrived example that someone
on
> the web might be able to construct to get past our security.  Given that our
> security (for PDFs, at least) is relying on having the PDF plugin installed
and
> enabled, and we're just going to use the system service if it's not, I think
> we're ok--from your description even if the web site serves it as text/plain,
> it'll be sniffed as PDF, and that'll get it to the PDF plugin (if installed
and
> enabled).  
> 
> I guess I'll just do one more probe: If we make the assumption that a website
> spoofs the mime-type and makes a PDF not sniffable as PDF by
> BufferredResourceHandler, but still downloads with a .pdf suffix, will it be
> routed to the PDF plugin?  Or will it be passed to the system to open?  If the
> latter, I think that's a security hole that should get security review before
> this goes in.

It will be routed to the PDF plugin.

Once the file is downloaded, the only things that affect the handling are:
- the filename (which in this case would <example>.pdf).
- the MIME mapping for the file type (which is application/pdf, assuming the
local machine isn't compromised. [1])
- browser's plugin configuration for the profile (presumably Chrome PDF Viewer
is available and enabled).

Of these the only thing that can be controlled by a web server would be the
filename.

[1]: It is possible that there's no system handler for .pdf in which case there
would be no mapping for that file type. However //net has a hardcoded secondary
mapping for .pdf files that will be used if there's no system mapping.

> I'll review the last change now; please send another email (or ping me via IM)
> when you've updated the CL description.
> > 
> >
>
https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
> > File chrome/browser/download/download_target_info.h (right):
> > 
> >
>
https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/...
> > chrome/browser/download/download_target_info.h:19: base::FilePath
> > intermediate_path;
> > On 2013/11/04 22:57:47, rdsmith wrote:
> > > nit: There should probably be a sentence comment explaining each of the
> above
> > > variables for consistency.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
> > File chrome/browser/download/download_target_determiner_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/55063002/diff/410001/chrome/browser/download/...
> > chrome/browser/download/download_target_determiner_unittest.cc:2000:
> > EXPECT_EQ(test_case.expected_mime_type, target_info->mime_type);
> > On 2013/11/05 16:52:40, rdsmith wrote:
> > > nit: Why doesn't VerifyDownloadTarget do this/isn't the expected_mime_type
> > part
> > > of download test case? 
> > 
> > DownloadTestCase is currently overspecified. Most of the tests don't use all
> the
> > fields, but must supply them anyway and in turn become sensitive to
unrelated
> > changes. So I was a bit reluctant to add another field that's only used in
one
> > test. I'll clean up this test suite, but that seemed overkill for this CL.

[Chris Palmer, 2013-11-05 22:14:58]

AFAICT, this CL only increases the chance that the Chrome PDF plugin will
be used. That, in effect, hardens the platform somewhat against malicious
files. I don't see that this CL increases the risk of host attack from evil
data (PDF, fake PDF, or other).

In case it isn't clear, I'm from the Chrome security team and I'm pushing
for this feature, so if it's not safe it's all my fault, not Asanka's. :)
On Nov 5, 2013 1:43 PM, <rdsmith@chromium.org> wrote:

> On 2013/11/05 21:31:38, asanka wrote:
>
>> The flow for file:// URLs is:
>> //net:
>>    - MIME type for the UrlRequest is determined using
>>
> net::GetMimeTypeFromFile()
>
>> by URLRequestFileJob
>>
>
>  //content:
>>    - MIME type overridden by BufferedResourceHandler if the MIME type is
>> empty
>>
> or
>
>> one of kSniffableTypes.
>>
>
> (https://code.google.com/p/chromium/codesearch#chromium/
> src/net/base/mime_sniffer.cc&rcl=1383642566&l=819).
>
>  Renderer:
>>    - Renderer creates WebCore::PluginDocument to render content, which
>> instantiates the plugin via a WebCore::HTMLEmbedElement with
>> src=<file://url>
>> type=<MIME type from above>.
>>    - HTMLEmbedElement instantiates the plug-in which is eventually
>> handled by
>> RenderFrameImpl::createPlugin().
>>    - ViewHostMsg_GetPluginInfo is sent back to browser.
>>
>
>  Browser:
>>    - PluginInfoMessageFilter::OnGetPluginInfo() is called with
>> file://url and
>> MIME type.
>>    - PluginService::GetPluginInfoArray is called which adds plugins
>> based on
>>
> MIME
>
>> type first and then based on URL.
>>    - PluginInfoMessageFilter::Context::FindEnabledPlugin() returns the
>> first
>> plugin that is enabled from previous list.
>>
>
>  ...
>>
>
>  I've updated the logic to use the file:// URL as well as the MIME type to
>> more
>> closely resemble the underlying logic. Thanks for reminding me!
>>
>
>  The difference in logic between the DownloadTargetDeterminer and the
>> browsing
>> flow is in BufferedResourceHandler. Our decision that the file type is
>> handled
>> securely might be wrong if something like the following (very contrived
>> scenario) happens:
>>
>
>  - Some application associates .pdf with text/plain. text/plain is handled
>> by
>>
> the
>
>> renderer, so we would consider this to be handled securely. However
>> text/plain
>> is also a sniffable MIME type.
>> - BufferedResourceHandler sniffs the contents and determines it's
>> application/pdf.
>> - Opening file://.../foo.pdf therefore results in instantiating the
>> plug-in
>> associated with application/pdf which may not be our sandboxed plug-in.
>>
>
> My major concern in this space is if there's a contrived example that
> someone on
> the web might be able to construct to get past our security.  Given that
> our
> security (for PDFs, at least) is relying on having the PDF plugin
> installed and
> enabled, and we're just going to use the system service if it's not, I
> think
> we're ok--from your description even if the web site serves it as
> text/plain,
> it'll be sniffed as PDF, and that'll get it to the PDF plugin (if
> installed and
> enabled).
>
> I guess I'll just do one more probe: If we make the assumption that a
> website
> spoofs the mime-type and makes a PDF not sniffable as PDF by
> BufferredResourceHandler, but still downloads with a .pdf suffix, will it
> be
> routed to the PDF plugin?  Or will it be passed to the system to open?  If
> the
> latter, I think that's a security hole that should get security review
> before
> this goes in.
>
> I'll review the last change now; please send another email (or ping me via
> IM)
> when you've updated the CL description.
>
>
> https://codereview.chromium.org/55063002/diff/310001/
> chrome/browser/download/download_target_info.h
>
>> File chrome/browser/download/download_target_info.h (right):
>>
>
>
> https://codereview.chromium.org/55063002/diff/310001/
> chrome/browser/download/download_target_info.h#newcode19
>
>> chrome/browser/download/download_target_info.h:19: base::FilePath
>> intermediate_path;
>> On 2013/11/04 22:57:47, rdsmith wrote:
>> > nit: There should probably be a sentence comment explaining each of the
>>
> above
>
>> > variables for consistency.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/55063002/diff/410001/
> chrome/browser/download/download_target_determiner_unittest.cc
>
>> File chrome/browser/download/download_target_determiner_unittest.cc
>> (right):
>>
>
>
> https://codereview.chromium.org/55063002/diff/410001/
> chrome/browser/download/download_target_determiner_unittest.cc#newcode2000
>
>> chrome/browser/download/download_target_determiner_unittest.cc:2000:
>> EXPECT_EQ(test_case.expected_mime_type, target_info->mime_type);
>> On 2013/11/05 16:52:40, rdsmith wrote:
>> > nit: Why doesn't VerifyDownloadTarget do this/isn't the
>> expected_mime_type
>> part
>> > of download test case?
>>
>
>  DownloadTestCase is currently overspecified. Most of the tests don't use
>> all
>>
> the
>
>> fields, but must supply them anyway and in turn become sensitive to
>> unrelated
>> changes. So I was a bit reluctant to add another field that's only used
>> in one
>> test. I'll clean up this test suite, but that seemed overkill for this CL.
>>
>
>
>
> https://codereview.chromium.org/55063002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[asanka, 2013-11-05 22:40:38]

UMA added. I also limited this change to PDF files for now.

[asanka, 2013-11-05 22:43:55]

+isherman: Could you take a look at histograms.xml?

[Randy Smith (Not in Mondays), 2013-11-05 22:59:37]

LGTM.

[Ilya Sherman, 2013-11-05 23:14:02]

histograms LGTM with a question:

https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21158: +  <int value="2" label="Opened
with plaform handler by user choice"/>
Should there also be an entry for "Opened in the browser by user choice"?

[asanka, 2013-11-05 23:24:38]

Thanks everyone! I'll land once the tests are fixed.

https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:21158: +  <int value="2" label="Opened
with plaform handler by user choice"/>
On 2013/11/05 23:14:03, Ilya Sherman wrote:
> Should there also be an entry for "Opened in the browser by user choice"?

That would have been the "Open in Chrome" option that was removed in favor of
making that the default for PDF and allowing the user to manually choose the
platform handler.

I didn't add that value to the enum because now there's no direct way to
manually choose to open a download in Chrome. They could conceivably select
"Open" and select a file that they downloaded, but we can't easily tell at this
point whether it was the same file that the browser downloaded some time earlier
or some other file.

[commit-bot: I haz the power, 2013-11-06 17:41:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1010001

[commit-bot: I haz the power, 2013-11-06 18:50:21]

Retried try job too often on linux_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-06 19:46:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1010001

[commit-bot: I haz the power, 2013-11-07 01:27:53]

Change committed as 233460

[commit-bot: I haz the power, 2013-11-08 20:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1410001

[commit-bot: I haz the power, 2013-11-08 23:24:56]

Change committed as 234031