Block plugins for platform apps

Block plugins for platform apps

To block plugins a new content settings has been
added, with the highest priority (i.e. at the front of
the list). This could be used down the track to hang
off more platform app specific stuff.

The provider knows which platform apps have been run
by watching a new notification.

BUG=None
TEST=Tested manually on Windows


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120541

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120742

[benwells, 2012-01-25 08:07:12]

Bernhard - you're the only reviewer at the moment mainly for any feedback you
have over this approach. If this approach is OK I'll add more reviewers.

[Bernhard Bauer, 2012-01-25 13:59:04]

Looks pretty good in general. Some comments:

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_platform_app_provider.cc
(right):

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_platform_app_provider.cc:48:
ResourceIdentifier("adobe-flash-player"),
Can't we just disallow all plug-ins already? ;-)

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_platform_app_provider.h
(right):

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_platform_app_provider.h:40:
Value* value) OVERRIDE { return false; }
Inlining virtual methods doesn't make much sense, see
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

[Mihai Parparita -not on Chrome, 2012-01-25 19:52:48]

http://codereview.chromium.org/9169042/diff/1/chrome/browser/ui/extensions/sh...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/9169042/diff/1/chrome/browser/ui/extensions/sh...
chrome/browser/ui/extensions/shell_window.cc:69: // Notify anyone interested
that the app has started.
We might have non-ShellWindow UIs for platform apps (e.g. for panels). It seems
like this should be moved to ExtensionProcessManager::CreateShellHost.

Also, you might be able to use the existing NOTIFICATION_EXTENSION_HOST_CREATED
notification instead, and check if the extension is a platform app.

[benwells, 2012-01-27 04:03:52]

Feedback addressed and some other cleanup done.

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/content_settings_platform_app_provider.h
(right):

http://codereview.chromium.org/9169042/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/content_settings_platform_app_provider.h:40:
Value* value) OVERRIDE { return false; }
On 2012/01/25 13:59:04, Bernhard Bauer wrote:
> Inlining virtual methods doesn't make much sense, see
>
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

Done, and thanks for the link its got some good stuff in it.

http://codereview.chromium.org/9169042/diff/1/chrome/browser/ui/extensions/sh...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/9169042/diff/1/chrome/browser/ui/extensions/sh...
chrome/browser/ui/extensions/shell_window.cc:69: // Notify anyone interested
that the app has started.
On 2012/01/25 19:52:49, Mihai Parparita wrote:
> We might have non-ShellWindow UIs for platform apps (e.g. for panels). It
seems
> like this should be moved to ExtensionProcessManager::CreateShellHost.
> 
> Also, you might be able to use the existing
NOTIFICATION_EXTENSION_HOST_CREATED
> notification instead, and check if the extension is a platform app.

Done: just used NOTIFICATION_EXTENSION_HOST_CREATED.

http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:416: return
content_type != CONTENT_SETTINGS_TYPE_PLUGINS &&
Mihai: do you know if its OK to stop whitelisting plugins for extension pages?

[Mihai Parparita -not on Chrome, 2012-01-28 00:17:02]

After thinking about this more, and talking with Rahul and some security guys,
can we try disabling all plug-ins for platform apps? 

Would that make this CL simpler? We'd still like to allow Nacl though.

http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.cc:416: return
content_type != CONTENT_SETTINGS_TYPE_PLUGINS &&
On 2012/01/27 04:03:52, benwells wrote:
> Mihai: do you know if its OK to stop whitelisting plugins for extension pages?

I don't know the full story for this restriction (looks like
http://crrev.com/38536 added it, but the NTP doesn't need plugins).

Based on http://crrbug.com/110900, it looks like the current state is a bit
broken already (extensions still trigger the plugin infobar).

[benwells, 2012-01-29 23:42:33]

On 2012/01/28 00:17:02, Mihai Parparita wrote:
> After thinking about this more, and talking with Rahul and some security guys,
> can we try disabling all plug-ins for platform apps? 
> 
> Would that make this CL simpler? We'd still like to allow Nacl though.
> 
>
http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
> File chrome/browser/content_settings/host_content_settings_map.cc (right):
> 
>
http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
> chrome/browser/content_settings/host_content_settings_map.cc:416: return
> content_type != CONTENT_SETTINGS_TYPE_PLUGINS &&
> On 2012/01/27 04:03:52, benwells wrote:
> > Mihai: do you know if its OK to stop whitelisting plugins for extension
pages?
> 
> I don't know the full story for this restriction (looks like
> http://crrev.com/38536 added it, but the NTP doesn't need plugins).
> 
> Based on http://crrbug.com/110900, it looks like the current state is a bit
> broken already (extensions still trigger the plugin infobar).

OK. There might be a few ways to block all plugins for platform extensions:

- remove the previous patch to block NPAPI plugins through the filter, and make
this patch block all plugins.

- remove this patch and update the previous NPAPI plugins patch disable all
plugins. Note, they appear in the UI as disabled and hence a broken link appears
to change your settings.

- have some sort of hard coded thing in the host content settings map, like the
white list is done now. This might be a bit messy and difficult to do thread
safe.

From my limited understanding I prefer the first option. It feels most in
harmony with the content settings system we have, and gets the UI right (there
is no broken link). After reading crbug.com/110900 it makes more sense as the
'platform app provider' could be extended to fix that as well (if the content
settings approach is used). Maybe there is something simpler we can do though.

Bernhard - what's your opinion?

[Bernhard Bauer, 2012-01-30 10:25:56]

On 2012/01/29 23:42:33, benwells wrote:
> On 2012/01/28 00:17:02, Mihai Parparita wrote:
> > After thinking about this more, and talking with Rahul and some security
guys,
> > can we try disabling all plug-ins for platform apps? 
> > 
> > Would that make this CL simpler? We'd still like to allow Nacl though.
> > 
> >
>
http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
> > File chrome/browser/content_settings/host_content_settings_map.cc (right):
> > 
> >
>
http://codereview.chromium.org/9169042/diff/6001/chrome/browser/content_setti...
> > chrome/browser/content_settings/host_content_settings_map.cc:416: return
> > content_type != CONTENT_SETTINGS_TYPE_PLUGINS &&
> > On 2012/01/27 04:03:52, benwells wrote:
> > > Mihai: do you know if its OK to stop whitelisting plugins for extension
> pages?
> > 
> > I don't know the full story for this restriction (looks like
> > http://crrev.com/38536 added it, but the NTP doesn't need plugins).
> > 
> > Based on http://crrbug.com/110900, it looks like the current state is a bit
> > broken already (extensions still trigger the plugin infobar).
> 
> OK. There might be a few ways to block all plugins for platform extensions:
> 
> - remove the previous patch to block NPAPI plugins through the filter, and
make
> this patch block all plugins.
> 
> - remove this patch and update the previous NPAPI plugins patch disable all
> plugins. Note, they appear in the UI as disabled and hence a broken link
appears
> to change your settings.
> 
> - have some sort of hard coded thing in the host content settings map, like
the
> white list is done now. This might be a bit messy and difficult to do thread
> safe.
> 
> From my limited understanding I prefer the first option. It feels most in
> harmony with the content settings system we have, and gets the UI right (there
> is no broken link). After reading crbug.com/110900 it makes more sense as the
> 'platform app provider' could be extended to fix that as well (if the content
> settings approach is used). Maybe there is something simpler we can do though.
> 
> Bernhard - what's your opinion?

Yeah, option 1 sounds good. Note though that blocking a plug-in via content
settings still allows it to run if the user wants to, whereas disabling it makes
it completely unavailable to the web page.

[benwells, 2012-01-30 10:37:13]

> Yeah, option 1 sounds good. Note though that blocking a plug-in via content
> settings still allows it to run if the user wants to, whereas disabling it
makes
> it completely unavailable to the web page.

OK, I think we want to make it completely unavailable. Platform apps have their
own container window (not Browser) and I haven't seen any bubbles / prompts when
testing this, so maybe this is already solved by not using Browser.

[Bernhard Bauer, 2012-01-31 11:59:03]

On 2012/01/30 10:37:13, benwells wrote:
> > Yeah, option 1 sounds good. Note though that blocking a plug-in via content
> > settings still allows it to run if the user wants to, whereas disabling it
> makes
> > it completely unavailable to the web page.
> 
> OK, I think we want to make it completely unavailable. Platform apps have
their
> own container window (not Browser) and I haven't seen any bubbles / prompts
when
> testing this, so maybe this is already solved by not using Browser.

Hm, you could still right-click on a blocked plug-in to run it, but honestly,
how we choose to implement not supporting plug-ins in platform apps is an
implementation detail, and it doesn't necessarily need to be completely
water-tight. It will still keep a non-attentive app developer from relying on
plug-ins, and if they still use one *and* the user chooses to run it, it's not a
Very Bad Thing.

http://codereview.chromium.org/9169042/diff/13001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/9169042/diff/13001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:44: POLICY_PROVIDER
= 1,
Nit: can you remove the = 1 here?

[benwells, 2012-02-01 02:30:34]

Nit fixed.

I checked the right click thing, we've already killed the right click menu so
the plugins can't be run that way.

http://codereview.chromium.org/9169042/diff/13001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/9169042/diff/13001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:44: POLICY_PROVIDER
= 1,
On 2012/01/31 11:59:03, Bernhard Bauer wrote:
> Nit: can you remove the = 1 here?

Done.

[Bernhard Bauer, 2012-02-02 11:33:43]

Sorry for the delay, this always slips under my radar :(

LGTM!

[benwells, 2012-02-02 22:44:59]

+sky as owner of chrome/browser/ui

[sky, 2012-02-03 17:58:37]

LGTM

[commit-bot: I haz the power, 2012-02-06 02:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9169042/20001

[commit-bot: I haz the power, 2012-02-06 04:20:12]

Change committed as 120541

[commit-bot: I haz the power, 2012-02-07 06:35:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/9169042/28002

[commit-bot: I haz the power, 2012-02-07 07:57:13]

Change committed as 120742