[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the plugin list retrieval. This didn't matter before HBD because the plugin list wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f2a1e9f94f9ac74896d5fe4c0e691befdfa40bc2
Cr-Commit-Position: refs/heads/master@{#423598}

[tommycli, 2016-09-29 18:38:27]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP and non-HTTPs URLs for Flash.

BUG=649223
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP and non-HTTPs URLs for Flash.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tommycli, 2016-09-29 19:23:52]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 19:24:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-09-29 19:39:18]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP and non-HTTPs URLs for Flash.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-09-29 19:46:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 19:46:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tommycli, 2016-09-29 22:48:01]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:48:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-09-29 23:04:59]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-29 23:19:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 23:19:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tommycli, 2016-09-29 23:32:46]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc.

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost, since
WebSecurityOrigin serializes FILE origins to "null". This patch fixes it by
passing url::Origin throughout the Plugins code rather than GURL for the page
origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tommycli, 2016-09-29 23:33:21]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 23:33:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 00:09:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 00:09:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-09-30 00:42:07]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 00:42:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-09-30 00:42:36]

tommycli@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org, raymes@chromium.org

[tommycli, 2016-09-30 00:42:37]

raymes: PTAL plugins stuff
jochen: PTAL chrome/ and content/
nasko: PTAL messages changes

Thanks!

https://codereview.chromium.org/2378573005/diff/40001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (left):

https://codereview.chromium.org/2378573005/diff/40001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:566: WebString top_origin =
frame->top()->getSecurityOrigin().toString();
Here was where the issue was: WebSecurityOrigin.toString() makes file:// URLs
null.

My solution was just to pass everything through as url::Origin so we have more
control over it.

[commit-bot: I haz the power, 2016-09-30 01:30:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 01:30:29]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-30 10:50:35]

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:42: !policy_url.SchemeIsHTTPOrHTTPS() &&
!policy_url.SchemeIsFile()) {
aren't file urls unique as well?

[tommycli, 2016-09-30 17:44:43]

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:42: !policy_url.SchemeIsHTTPOrHTTPS() &&
!policy_url.SchemeIsFile()) {
On 2016/09/30 10:50:34, jochen (slow) wrote:
> aren't file urls unique as well?

Surprisingly... no. I tested this, And I got:

WebSecurityOrigin isunique: 0 string: null
url::Origin isunique: 0 string: file://

I thought they were unique as well, but they don't seem to be.

[tommycli, 2016-09-30 17:46:21]

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:42: !policy_url.SchemeIsHTTPOrHTTPS() &&
!policy_url.SchemeIsFile()) {
On 2016/09/30 17:44:43, tommycli wrote:
> On 2016/09/30 10:50:34, jochen (slow) wrote:
> > aren't file urls unique as well?
> 
> Surprisingly... no. I tested this, And I got:
> 
> WebSecurityOrigin isunique: 0 string: null
> url::Origin isunique: 0 string: file://
> 
> I thought they were unique as well, but they don't seem to be.

+cc dcheng/alexmos - I thought file:// urls were unique, but they don't seem to
be. - in either WebSecurityOrigin or url::Origin.

[jochen (gone - plz use gerrit), 2016-09-30 17:57:16]

On 2016/09/30 at 17:46:21, tommycli wrote:
>
https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
> File chrome/browser/plugins/plugin_utils.cc (right):
> 
>
https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
> chrome/browser/plugins/plugin_utils.cc:42: !policy_url.SchemeIsHTTPOrHTTPS()
&& !policy_url.SchemeIsFile()) {
> On 2016/09/30 17:44:43, tommycli wrote:
> > On 2016/09/30 10:50:34, jochen (slow) wrote:
> > > aren't file urls unique as well?
> > 
> > Surprisingly... no. I tested this, And I got:
> > 
> > WebSecurityOrigin isunique: 0 string: null
> > url::Origin isunique: 0 string: file://
> > 
> > I thought they were unique as well, but they don't seem to be.
> 
> +cc dcheng/alexmos - I thought file:// urls were unique, but they don't seem
to be. - in either WebSecurityOrigin or url::Origin.

Maybe you tested with --allow-file-access-from-files or an option that implies
that?

[tommycli, 2016-09-30 18:04:42]

On 2016/09/30 17:57:16, jochen (slow) wrote:
> On 2016/09/30 at 17:46:21, tommycli wrote:
> >
>
https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
> > File chrome/browser/plugins/plugin_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/2378573005/diff/120001/chrome/browser/plugins...
> > chrome/browser/plugins/plugin_utils.cc:42: !policy_url.SchemeIsHTTPOrHTTPS()
> && !policy_url.SchemeIsFile()) {
> > On 2016/09/30 17:44:43, tommycli wrote:
> > > On 2016/09/30 10:50:34, jochen (slow) wrote:
> > > > aren't file urls unique as well?
> > > 
> > > Surprisingly... no. I tested this, And I got:
> > > 
> > > WebSecurityOrigin isunique: 0 string: null
> > > url::Origin isunique: 0 string: file://
> > > 
> > > I thought they were unique as well, but they don't seem to be.
> > 
> > +cc dcheng/alexmos - I thought file:// urls were unique, but they don't seem
> to be. - in either WebSecurityOrigin or url::Origin.
> 
> Maybe you tested with --allow-file-access-from-files or an option that implies
> that?

I dug a little deeper.

--allow-file-access-from-files is off. If it were on, I would have gotten this
result:
WebSecurityOrigin isunique: 0 string: file://
url::Origin isunique: 0 string: file://

FILE origins aren't unique, but still aren't allowed to access each other if
that flag is off per this code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...

You might call them ... pseudo-unique, since I'm guessing they aren't unique in
order to support that flag. If they were just truly unique, there would be no
way to implement that flag, since you couldn't distinguish pseudo-unique FILE
urls from other unique urls.

[nasko, 2016-09-30 20:38:49]

Please ensure that URL is no longer present in the parameter/variable names when
they are now origins.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/flash_download_interception.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/flash_download_interception.cc:68:
host_content_settings_map, url::Origin(source_url), source_url, nullptr);
Just FYI, this is a lossy conversion. For about:blank for example, you will not
get a valid origin back.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:213: url::Origin
top_origin_url;
This variable is no longer an URL.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:219: const url::Origin&
top_origin_url,
Not an URL

https://codereview.chromium.org/2378573005/diff/140001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/common/render_m...
chrome/common/render_messages.h:360: url::Origin /* top origin url */,
nit: Drop the "url" part of the comment, as it is only the origin part of it.

https://codereview.chromium.org/2378573005/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:509: const
url::Origin& page_url,
Not on URL.

https://codereview.chromium.org/2378573005/diff/140001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2378573005/diff/140001/content/common/frame_m...
content/common/frame_messages.h:1241: url::Origin /* page_url */,
Please update the comment.

[tommycli, 2016-09-30 21:19:36]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 21:20:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-09-30 21:20:42]

nasko: Thanks! I changed everything to main_frame_origin.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/flash_download_interception.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/flash_download_interception.cc:68:
host_content_settings_map, url::Origin(source_url), source_url, nullptr);
On 2016/09/30 20:38:48, nasko (slow) wrote:
> Just FYI, this is a lossy conversion. For about:blank for example, you will
not
> get a valid origin back.

Yes, that's perfect! :)

We want to BLOCK flash on everything but HTTP and FILE. If it becomes an empty
origin, it gets blocked.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:213: url::Origin
top_origin_url;
On 2016/09/30 20:38:48, nasko (slow) wrote:
> This variable is no longer an URL.

Done.

https://codereview.chromium.org/2378573005/diff/140001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:219: const url::Origin&
top_origin_url,
On 2016/09/30 20:38:48, nasko (slow) wrote:
> Not an URL

Done.

https://codereview.chromium.org/2378573005/diff/140001/chrome/common/render_m...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/2378573005/diff/140001/chrome/common/render_m...
chrome/common/render_messages.h:360: url::Origin /* top origin url */,
On 2016/09/30 20:38:48, nasko (slow) wrote:
> nit: Drop the "url" part of the comment, as it is only the origin part of it.

Done.

https://codereview.chromium.org/2378573005/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:509: const
url::Origin& page_url,
On 2016/09/30 20:38:48, nasko (slow) wrote:
> Not on URL.

Done.

https://codereview.chromium.org/2378573005/diff/140001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2378573005/diff/140001/content/common/frame_m...
content/common/frame_messages.h:1241: url::Origin /* page_url */,
On 2016/09/30 20:38:49, nasko (slow) wrote:
> Please update the comment.

Done.

[commit-bot: I haz the power, 2016-09-30 22:11:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 22:11:57]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-10-02 05:12:56]

Thanks Tommy!

> Fixes FILE plugin loads.

Can you explain what exactly wasn't working properly? Were plugins just not
loading at all on file: URLs? Was it only because of the changes you're making?
What was the GURL that was being passed into GetPluginContentSetting for file:
URLs? 

Orthogonally to this, we still have to decide the specific behavior for file:
URLs because currently a prompt will be displayed that doesn't work properly.
Should we always allow content without a prompt?

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:40: // top level origin is any scheme
other HTTP, HTTPS, or FILE.
nit: other than HTTP, ...

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:46: *uses_default_content_setting = true;
I think this should be false - it's not necessarily the default for that
setting.

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:48: *is_managed = true;
This should only be true if the setting is being overridden by policy, so this
should be false too, although the specific semantics are unclear (this is true
in many cases though...)

[raymes, 2016-10-02 12:00:05]

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:40: // top level origin is any scheme
other HTTP, HTTPS, or FILE.
On 2016/10/02 05:12:56, raymes wrote:
> nit: other than HTTP, ...

After thinking about this a bit, I feel like we should wait until after we've
queried a content setting for these URLs before we block. Specifically I'm
thinking about the case where the user has allowed plugins everywhere. In that
case, it's probably fine to let them run on data: URLs, etc.

[tommycli, 2016-10-03 18:27:44]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc.

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost, since
WebSecurityOrigin serializes FILE origins to "null". This patch fixes it by
passing url::Origin throughout the Plugins code rather than GURL for the page
origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc.

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tommycli, 2016-10-03 18:30:14]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc.

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless
the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[tommycli, 2016-10-03 18:49:28]

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:40: // top level origin is any scheme
other HTTP, HTTPS, or FILE.
On 2016/10/02 05:12:56, raymes wrote:
> nit: other than HTTP, ...

Done.

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:40: // top level origin is any scheme
other HTTP, HTTPS, or FILE.
On 2016/10/02 12:00:05, raymes wrote:
> On 2016/10/02 05:12:56, raymes wrote:
> > nit: other than HTTP, ...
> 
> After thinking about this a bit, I feel like we should wait until after we've
> queried a content setting for these URLs before we block. Specifically I'm
> thinking about the case where the user has allowed plugins everywhere. In that
> case, it's probably fine to let them run on data: URLs, etc.

Done.

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:46: *uses_default_content_setting = true;
On 2016/10/02 05:12:56, raymes wrote:
> I think this should be false - it's not necessarily the default for that
> setting.

I preserved the actual is_default and is_managed values.

https://codereview.chromium.org/2378573005/diff/200001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:48: *is_managed = true;
On 2016/10/02 05:12:56, raymes wrote:
> This should only be true if the setting is being overridden by policy, so this
> should be false too, although the specific semantics are unclear (this is true
> in many cases though...)

Done.

[tommycli, 2016-10-03 18:51:27]

Previously FILE origins weren't being passed correctly in the GetPluginList
path.

This never bothered us before, since the origin has never been used in the
GetPluginList path before in any serious way.

Since HBD, we're now using the origin to check against content settings and SEI,
so this patch fixes the FILE origin case.


I think for the FILE case, we should fix the permission prompt to add a
file:///* exception into the content settings.

In fact, the permission prompt already says: "Allow Flash for file://"?

[tommycli, 2016-10-03 21:51:37]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 21:52:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 22:51:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 22:51:42]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-10-04 19:27:34]

On 2016/10/03 18:51:27, tommycli wrote:
> Previously FILE origins weren't being passed correctly in the GetPluginList
> path.
> 
> This never bothered us before, since the origin has never been used in the
> GetPluginList path before in any serious way.
> 
> Since HBD, we're now using the origin to check against content settings and
SEI,
> so this patch fixes the FILE origin case.
> 
> 
> I think for the FILE case, we should fix the permission prompt to add a
> file:///* exception into the content settings.
> 
> In fact, the permission prompt already says: "Allow Flash for file://"?

raymes: ping
others: maybe also ping ?

[raymes, 2016-10-05 03:16:27]

chrome/browser/plugins/plugin_utils.cc lgtm

[tommycli, 2016-10-05 16:16:34]

nasko: PTAL
jochen: PTAL

thanks!

[nasko, 2016-10-05 21:14:31]

LGTM with a few nits.

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:227: settings_map,
GURL(main_frame_origin.Serialize())) <
main_frame_origin.GetURL()

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:109:
GURL(main_frame_origin.Serialize()),
main_frame_origin.GetURL()

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:29: main_frame_origin.unique() ? GURL() :
GURL(main_frame_origin.Serialize());
main_frame_origin.GetURL()

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:495: GURL
plugin_content_url =
This is still an origin, even if it is stored as a GURL.

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:496:
main_frame_origin.unique() ? GURL() : GURL(main_frame_origin.Serialize());
Let's change that to be main_frame_origin.GetURL(), since this new method avoids
serializing and parsing it again.

[tommycli, 2016-10-05 21:41:10]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2016-10-05 21:41:23]

nasko: thanks!

jochen: PTAL, thanks

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/chrome_plugin_service_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/chrome_plugin_service_filter.cc:227: settings_map,
GURL(main_frame_origin.Serialize())) <
On 2016/10/05 21:14:31, nasko (slow) wrote:
> main_frame_origin.GetURL()

Done. I was wondering how I missed that method, but checking git blame, it only
landed two days ago!

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_info_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/plugin_info_message_filter.cc:109:
GURL(main_frame_origin.Serialize()),
On 2016/10/05 21:14:31, nasko (slow) wrote:
> main_frame_origin.GetURL()

Done.

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_utils.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/chrome/browser/plugins...
chrome/browser/plugins/plugin_utils.cc:29: main_frame_origin.unique() ? GURL() :
GURL(main_frame_origin.Serialize());
On 2016/10/05 21:14:31, nasko (slow) wrote:
> main_frame_origin.GetURL()

Done.

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:495: GURL
plugin_content_url =
On 2016/10/05 21:14:31, nasko (slow) wrote:
> This is still an origin, even if it is stored as a GURL.

Done.

https://codereview.chromium.org/2378573005/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:496:
main_frame_origin.unique() ? GURL() : GURL(main_frame_origin.Serialize());
On 2016/10/05 21:14:31, nasko (slow) wrote:
> Let's change that to be main_frame_origin.GetURL(), since this new method
avoids
> serializing and parsing it again.

Done.

[commit-bot: I haz the power, 2016-10-05 21:42:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 21:45:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 21:45:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tommycli, 2016-10-05 21:50:21]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 21:50:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-10-05 22:03:36]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:04:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 22:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 22:20:05]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[tommycli, 2016-10-05 22:24:31]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:25:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 23:26:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 23:26:11]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-10-06 12:17:09]

lgtm

[tommycli, 2016-10-06 16:02:08]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-10-06 16:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2378573005/#ps340001
(title: "fix non-mechanical merge issues")

[commit-bot: I haz the power, 2016-10-06 16:02:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 16:07:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 16:07:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tommycli, 2016-10-06 17:35:54]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-10-06 17:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
raymes@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2378573005/#ps360001
(title: "fix dat merge")

[commit-bot: I haz the power, 2016-10-06 17:36:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 18:34:56]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless
the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless
the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-06 18:34:57]

Committed patchset #18 (id:360001)

[commit-bot: I haz the power, 2016-10-06 18:37:26]

Description was changed from

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless
the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[HBD] Blanket BLOCK on all non-HTTP(s) and non-FILE URLs for Flash.

This patch does two things:

1) Blocks all non-HTTP and non-FILE plugin loads within plugin_utils.cc (unless
the user has chosen ALLOW)

2) Fixes FILE plugin loads. Previously, FILE origins were getting lost in the
GetPlugins, since WebSecurityOrigin serializes FILE origins to "null".

This meant that content settings exceptions did not work correctly for the
plugin list retrieval. This didn't matter before HBD because the plugin list
wasn't affected by content settings until HBD.

This patch fixes it by passing url::Origin throughout the Plugins code rather
than GURL for the page origin.

BUG=649223
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f2a1e9f94f9ac74896d5fe4c0e691befdfa40bc2
Cr-Commit-Position: refs/heads/master@{#423598}
==========

[commit-bot: I haz the power, 2016-10-06 18:37:28]

Patchset 18 (id:??) landed as
https://crrev.com/f2a1e9f94f9ac74896d5fe4c0e691befdfa40bc2
Cr-Commit-Position: refs/heads/master@{#423598}