<webview>: Change how plugin load works inside guests.

<webview>: Change how plugin load works inside guests.

The changes include:
 * Always disallow loading NPAPI plugins inside guests.
 * Do not allow installing a plugin from inside guests.
 * Mark all plugins inside guests as needing authorization. This triggers a
   javascript 'permissionrequest' event on the <webview> inside the embedder.
   The embedder can chose to allow() or deny() the plugin from being loaded. If
   the embedder callback doesn't preventDefault() the event, and does not call
   either allow() or deny(), then the plugin is allowed.

BUG=156120
R=bauerb@chromium.org, fsamuel@chromium.org, jam@chromium.org

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

[sadrul, 2013-09-25 23:39:59]

jam@ Please review the enum addition in content/public/

bauerb@ Please review the change in plugin_info_message_filter

fsamuel@ Please review all changes.

I am working on a test (it's a bit tricky since there doesn't seem to be many
other tests that use ppapi plugins)

[sadrul, 2013-09-26 02:27:13]

Added a test.

[Bernhard Bauer, 2013-09-26 06:19:38]

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On Windows in Metro mode (see above) this triggers an infobar. Will you get that
here too?

[sadrul, 2013-09-26 06:43:23]

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On 2013/09/26 06:19:39, Bernhard Bauer wrote:
> On Windows in Metro mode (see above) this triggers an infobar. Will you get
that
> here too?

It won't. Infobars are not supported for guests inside <webview>. So in these
cases, the plugin is just not loaded.

It is a deliberate decision to not allow NPAPI plugins inside guests, and so
this doesn't trigger any event in the embedder either.

[Bernhard Bauer, 2013-09-26 06:56:33]

LGTM

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On 2013/09/26 06:43:23, sadrul wrote:
> On 2013/09/26 06:19:39, Bernhard Bauer wrote:
> > On Windows in Metro mode (see above) this triggers an infobar. Will you get
> that
> > here too?
> 
> It won't. Infobars are not supported for guests inside <webview>. So in these
> cases, the plugin is just not loaded.
> 
> It is a deliberate decision to not allow NPAPI plugins inside guests, and so
> this doesn't trigger any event in the embedder either.

Oh, I see, the PluginPermissionFilter filters the message out, right?

[sadrul, 2013-09-26 06:59:52]

Thanks for the quick review!

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On 2013/09/26 06:56:33, Bernhard Bauer wrote:
> On 2013/09/26 06:43:23, sadrul wrote:
> > On 2013/09/26 06:19:39, Bernhard Bauer wrote:
> > > On Windows in Metro mode (see above) this triggers an infobar. Will you
get
> > that
> > > here too?
> > 
> > It won't. Infobars are not supported for guests inside <webview>. So in
these
> > cases, the plugin is just not loaded.
> > 
> > It is a deliberate decision to not allow NPAPI plugins inside guests, and so
> > this doesn't trigger any event in the embedder either.
> 
> Oh, I see, the PluginPermissionFilter filters the message out, right?

The PluginPermissionHelper doesn't actually handle those messages at the moment.
But the messages aren't handled by anything else either (the WebContents for the
guest does not get a PluginObserver). Would you suggest the
PluginPermissionHelper explicitly adds no-op callback for these messages?

[Bernhard Bauer, 2013-09-26 08:39:09]

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On 2013/09/26 06:59:52, sadrul wrote:
> On 2013/09/26 06:56:33, Bernhard Bauer wrote:
> > On 2013/09/26 06:43:23, sadrul wrote:
> > > On 2013/09/26 06:19:39, Bernhard Bauer wrote:
> > > > On Windows in Metro mode (see above) this triggers an infobar. Will you
> get
> > > that
> > > > here too?
> > > 
> > > It won't. Infobars are not supported for guests inside <webview>. So in
> these
> > > cases, the plugin is just not loaded.
> > > 
> > > It is a deliberate decision to not allow NPAPI plugins inside guests, and
so
> > > this doesn't trigger any event in the embedder either.
> > 
> > Oh, I see, the PluginPermissionFilter filters the message out, right?
> 
> The PluginPermissionHelper doesn't actually handle those messages at the
moment.
> But the messages aren't handled by anything else either (the WebContents for
the
> guest does not get a PluginObserver). Would you suggest the
> PluginPermissionHelper explicitly adds no-op callback for these messages?

Hm, I thought we DCHECK if we can't find a handler for a message. If we don't...
hm, yeah, it's probably nice at least for documentation purposes to know that
the messages are swallowed.

[Fady Samuel, 2013-09-26 12:53:31]

This looks really good. Just a couple of small things to address then I'm happy
with it. I'm quite surprised at how simple this was.

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/plugin_permission_helper.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/guestview/...
chrome/browser/guestview/webview/plugin_permission_helper.cc:16:
DEFINE_WEB_CONTENTS_USER_DATA_KEY(PluginPermissionHelper);
Not a code issue but I actually really like the pattern of isolating things into
helpers. It makes WebViewGuest look a lot less cluttered!

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/guestview/...
chrome/browser/guestview/webview/plugin_permission_helper.cc:48: if (!webview)
I suspect we'll use this pattern more frequently in the future, because it also
serves a security check: Once a WebContents is bound to a WebView, it can't
become an AdView or *View, it's always a WebView, and so on.

Could you please move this to a small method WebViewGuest::FromWebContents,
similar to WebViewGuest::From? Thanks. This will prevent people from assuming
that using the WebView API implies that the GuestView is a WebViewGuest and
static_cast to a WebViewGuest as a compromised renderer could lie about this to
try to get elevated privileges to a guest.

https://codereview.chromium.org/24576003/diff/16001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:633: return ['media',
'geolocation', 'pointerLock', 'download', 'loadplugin'];
We usually let new API bake a bit in canary/dev and then explicitly move it to
web_view.js once we are confident it's reasonably stable (other teams have tried
it out and it works for them, and the bots have been happy for a while). Could
you please move this permission to web_view_experimental.js? You might want to
speak to Istiaque about how we do that, I'm probably not coming into work today
as I'm still fighting a cold/flu.

[sadrul, 2013-09-26 15:28:10]

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/plugin_permission_helper.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/guestview/...
chrome/browser/guestview/webview/plugin_permission_helper.cc:48: if (!webview)
On 2013/09/26 12:53:32, Fady Samuel wrote:
> I suspect we'll use this pattern more frequently in the future, because it
also
> serves a security check: Once a WebContents is bound to a WebView, it can't
> become an AdView or *View, it's always a WebView, and so on.
> 
> Could you please move this to a small method WebViewGuest::FromWebContents,
> similar to WebViewGuest::From? Thanks. This will prevent people from assuming
> that using the WebView API implies that the GuestView is a WebViewGuest and
> static_cast to a WebViewGuest as a compromised renderer could lie about this
to
> try to get elevated privileges to a guest.

Good idea. Done.

https://codereview.chromium.org/24576003/diff/16001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:633: return ['media',
'geolocation', 'pointerLock', 'download', 'loadplugin'];
On 2013/09/26 12:53:32, Fady Samuel wrote:
> We usually let new API bake a bit in canary/dev and then explicitly move it to
> web_view.js once we are confident it's reasonably stable (other teams have
tried
> it out and it works for them, and the bots have been happy for a while). Could
> you please move this permission to web_view_experimental.js? You might want to
> speak to Istiaque about how we do that, I'm probably not coming into work
today
> as I'm still fighting a cold/flu.

Done.

[sadrul, 2013-09-26 15:35:25]

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/24576003/diff/16001/chrome/browser/plugins/pl...
chrome/browser/plugins/plugin_info_message_filter.cc:210:
ChromeViewHostMsg_GetPluginInfo_Status::kNPAPINotSupported;
On 2013/09/26 08:39:09, Bernhard Bauer wrote:
> On 2013/09/26 06:59:52, sadrul wrote:
> > On 2013/09/26 06:56:33, Bernhard Bauer wrote:
> > > On 2013/09/26 06:43:23, sadrul wrote:
> > > > On 2013/09/26 06:19:39, Bernhard Bauer wrote:
> > > > > On Windows in Metro mode (see above) this triggers an infobar. Will
you
> > get
> > > > that
> > > > > here too?
> > > > 
> > > > It won't. Infobars are not supported for guests inside <webview>. So in
> > these
> > > > cases, the plugin is just not loaded.
> > > > 
> > > > It is a deliberate decision to not allow NPAPI plugins inside guests,
and
> so
> > > > this doesn't trigger any event in the embedder either.
> > > 
> > > Oh, I see, the PluginPermissionFilter filters the message out, right?
> > 
> > The PluginPermissionHelper doesn't actually handle those messages at the
> moment.
> > But the messages aren't handled by anything else either (the WebContents for
> the
> > guest does not get a PluginObserver). Would you suggest the
> > PluginPermissionHelper explicitly adds no-op callback for these messages?
> 
> Hm, I thought we DCHECK if we can't find a handler for a message. If we
don't...
> hm, yeah, it's probably nice at least for documentation purposes to know that
> the messages are swallowed.

Done.

[jam, 2013-09-26 18:20:05]

https://codereview.chromium.org/24576003/diff/2/content/public/common/browser...
File content/public/common/browser_plugin_permission_type.h (right):

https://codereview.chromium.org/24576003/diff/2/content/public/common/browser...
content/public/common/browser_plugin_permission_type.h:33:
BROWSER_PLUGIN_PERMISSION_TYPE_LOAD_PLUGIN,
if this isn't used in content, it shouldn't be here. see
http://www.chromium.org/developers/content-module/content-api

you can define BROWSER_PLUGIN_PERMISSION_MAX and have an enum in chrome, i.e.
see
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[sadrul, 2013-09-26 21:12:15]

https://codereview.chromium.org/24576003/diff/2/content/public/common/browser...
File content/public/common/browser_plugin_permission_type.h (right):

https://codereview.chromium.org/24576003/diff/2/content/public/common/browser...
content/public/common/browser_plugin_permission_type.h:33:
BROWSER_PLUGIN_PERMISSION_TYPE_LOAD_PLUGIN,
On 2013/09/26 18:20:06, jam wrote:
> if this isn't used in content, it shouldn't be here. see
> http://www.chromium.org/developers/content-module/content-api
> 
> you can define BROWSER_PLUGIN_PERMISSION_MAX and have an enum in chrome, i.e.
> see
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

Ah, cool. Done.

[Fady Samuel, 2013-09-26 21:27:11]

LGTM once my comment is addressed.

https://codereview.chromium.org/24576003/diff/60001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/24576003/diff/60001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:996: var
PERMISSIONS_DEFAULT_ALLOWED = ['loadplugin'];
For consistency, could you call to web_view_experimental here for loadplugin?

[sadrul, 2013-09-26 21:51:33]

https://codereview.chromium.org/24576003/diff/60001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/24576003/diff/60001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:996: var
PERMISSIONS_DEFAULT_ALLOWED = ['loadplugin'];
On 2013/09/26 21:27:12, Fady Samuel wrote:
> For consistency, could you call to web_view_experimental here for loadplugin?

We discussed offline. This is necessary here to make sure that plugins are
loaded correctly in non-experimentals.

[Fady Samuel, 2013-09-27 13:04:08]

https://codereview.chromium.org/24576003/diff/65001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/plugin_permission_helper.cc (right):

https://codereview.chromium.org/24576003/diff/65001/chrome/browser/guestview/...
chrome/browser/guestview/webview/plugin_permission_helper.cc:61:
base::DictionaryValue info;
It would be nice to have an UMA count here. WebView.Guest.PluginLoad? That'll
give us a sense for just how frequently we load plugins in <webview>.

[sadrul, 2013-09-27 14:17:39]

https://codereview.chromium.org/24576003/diff/65001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/plugin_permission_helper.cc (right):

https://codereview.chromium.org/24576003/diff/65001/chrome/browser/guestview/...
chrome/browser/guestview/webview/plugin_permission_helper.cc:61:
base::DictionaryValue info;
On 2013/09/27 13:04:08, Fady Samuel wrote:
> It would be nice to have an UMA count here. WebView.Guest.PluginLoad? That'll
> give us a sense for just how frequently we load plugins in <webview>.

Done.

[jam, 2013-09-27 18:18:45]

lgtm with removing max

https://codereview.chromium.org/24576003/diff/82001/content/public/common/bro...
File content/public/common/browser_plugin_permission_type.h (right):

https://codereview.chromium.org/24576003/diff/82001/content/public/common/bro...
content/public/common/browser_plugin_permission_type.h:34:
BROWSER_PLUGIN_PERMISSION_TYPE_MAX =
you don't need max, content did that for process type because they put it in an
enum. so just remove this here.

[sadrul, 2013-09-27 22:13:04]

https://codereview.chromium.org/24576003/diff/82001/content/public/common/bro...
File content/public/common/browser_plugin_permission_type.h (right):

https://codereview.chromium.org/24576003/diff/82001/content/public/common/bro...
content/public/common/browser_plugin_permission_type.h:34:
BROWSER_PLUGIN_PERMISSION_TYPE_MAX =
On 2013/09/27 18:18:45, jam wrote:
> you don't need max, content did that for process type because they put it in
an
> enum. so just remove this here.

Done.

[sadrul, 2013-09-27 22:27:44]

Committed patchset #12 manually as r225800 (presubmit successful).