Block navigations to hosted apps non-icon resources with PlzNavigate.

Block navigations to hosted apps non-icon resources with PlzNavigate.

BUG=717626
Review-Url: https://codereview.chromium.org/2875493002
Cr-Commit-Position: refs/heads/master@{#474469}
Committed: https://chromium.googlesource.com/chromium/src/+/68e5e6de4f09b9bc50ec0460fcad02d5923304d9

[nasko, 2017-05-09 23:56:43]

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

[commit-bot: I haz the power, 2017-05-09 23:58:01]

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, 2017-05-10 01:47:41]

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

[commit-bot: I haz the power, 2017-05-10 01:47:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nasko, 2017-05-23 22:41:49]

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

[commit-bot: I haz the power, 2017-05-23 22:42:29]

Dry run: CQ is trying da patch.

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

[nasko, 2017-05-23 22:43:13]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2017-05-23 22:43:14]

Hey Nick,
Can you review this CL for me? It fixes the issue with hosted apps manifest
being given to renderer process to render, while displaying chrome://invalid in
the omnibox.
Thanks in advance!
Nasko

[commit-bot: I haz the power, 2017-05-23 23:46:09]

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

[commit-bot: I haz the power, 2017-05-23 23:46:10]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-05-24 17:10:54]

lgtm

[nasko, 2017-05-24 17:15:45]

nasko@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[nasko, 2017-05-24 17:15:46]

Hey Devlin,
Can you do an owners review on this CL?
Thanks in advance!
Nasko

[Devlin, 2017-05-24 17:27:19]

lgtm!

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:1251:
content::PAGE_TYPE_ERROR);
Can we expand this to include a check for icons that passes?

https://codereview.chromium.org/2875493002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2875493002/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:71: if
(target_extension->is_hosted_app()) {
Will the old check in chrome/renderer/extensions/resource_request_policy.cc
still be necessary once PlzNavigate ships?  If not, can we add a TODO to
coalesce these?

[nasko, 2017-05-24 17:37:16]

https://codereview.chromium.org/2875493002/diff/20001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2875493002/diff/20001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:71: if
(target_extension->is_hosted_app()) {
On 2017/05/24 17:27:19, Devlin (catching up) wrote:
> Will the old check in chrome/renderer/extensions/resource_request_policy.cc
> still be necessary once PlzNavigate ships?  If not, can we add a TODO to
> coalesce these?

Yes, it can be removed, as it will be redundant and arguably a bit of a worse
behavior, as "chrome-extension://invalid/" is a strange URL to navigate to and
it has no user feedback. I've added a TODO.

[nasko, 2017-05-24 17:37:23]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-24 17:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2875493002/#ps40001
(title: "Add a TODO.")

[commit-bot: I haz the power, 2017-05-24 17:38:06]

CQ is trying da patch.

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

[Devlin, 2017-05-24 17:39:17]

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:1251:
content::PAGE_TYPE_ERROR);
On 2017/05/24 17:27:19, Devlin (catching up) wrote:
> Can we expand this to include a check for icons that passes?

Missed this one?

[nasko, 2017-05-24 17:40:23]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2017-05-24 18:46:38]

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2875493002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:1251:
content::PAGE_TYPE_ERROR);
On 2017/05/24 17:39:17, Devlin (catching up) wrote:
> On 2017/05/24 17:27:19, Devlin (catching up) wrote:
> > Can we expand this to include a check for icons that passes?
> 
> Missed this one?

Oops, sorry, indeed I missed this one. Added.

[nasko, 2017-05-24 18:47:15]

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

[commit-bot: I haz the power, 2017-05-24 18:48:21]

Dry run: CQ is trying da patch.

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

[Devlin, 2017-05-24 18:48:34]

s lgtm as long as the bots are happy.  It's possible there's some test out there
that relies on hosted_app not having icons; if there is we can just copy it to
make hosted_app_with_icons. :) (And if not, then this is fine)

[nasko, 2017-05-24 19:03:39]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2017-05-24 19:54:32]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-24 19:54:33]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2875493002/#ps60001
(title: "Add a test for allowed navigation to an icon file.")

[commit-bot: I haz the power, 2017-05-24 19:55:08]

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, 2017-05-24 20:04:25]

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

[commit-bot: I haz the power, 2017-05-24 20:04:27]

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...)

[nasko, 2017-05-24 20:56:57]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-05-24 20:57:42]

CQ is trying da patch.

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

[ncarter (slow), 2017-05-24 21:13:16]

https://codereview.chromium.org/2875493002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2875493002/diff/60001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:73: // once PlzNavigate is
the default.
Can we though? If you remove the enforcement in ResourceRequestPolicy, doesn't
that mean you'll be able to xhr manifest.json?

[nasko, 2017-05-24 21:15:43]

https://codereview.chromium.org/2875493002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_navigation_throttle.cc (right):

https://codereview.chromium.org/2875493002/diff/60001/extensions/browser/exte...
extensions/browser/extension_navigation_throttle.cc:73: // once PlzNavigate is
the default.
On 2017/05/24 21:13:16, ncarter (slow) wrote:
> Can we though? If you remove the enforcement in ResourceRequestPolicy, doesn't
> that mean you'll be able to xhr manifest.json?

This is a good point. It might still be possible, but I think we are better off
blocking this on the browser side. However, you are correct that PlzNavigate
will only help with navigations.

[nasko, 2017-05-24 21:22:33]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-05-24 21:22:34]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2875493002/#ps80001
(title: "Remove incorrect TODO.")

[commit-bot: I haz the power, 2017-05-24 21:23:29]

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, 2017-05-24 23:19:41]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495660953729180,
"parent_rev": "59c3060ac761de2221eea6febe7be5d223f0266a", "commit_rev":
"68e5e6de4f09b9bc50ec0460fcad02d5923304d9"}

[commit-bot: I haz the power, 2017-05-24 23:19:53]

Description was changed from

==========
Block navigations to hosted apps non-icon resources with PlzNavigate.

BUG=717626
==========

to

==========
Block navigations to hosted apps non-icon resources with PlzNavigate.

BUG=717626

Review-Url: https://codereview.chromium.org/2875493002
Cr-Commit-Position: refs/heads/master@{#474469}
Committed:
https://chromium.googlesource.com/chromium/src/+/68e5e6de4f09b9bc50ec0460fcad...
==========

[commit-bot: I haz the power, 2017-05-24 23:19:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/68e5e6de4f09b9bc50ec0460fcad...