Disallow navigation to documents not explicitly listed as web accessible.

Disallow navigation to documents not explicitly listed as web accessible.

The existing check for web accessible resources is inadequate and allows
navigations to non-whitelisted pages to succeed. This patch ensures that
the document is listed explicitly in the manifest when a navigation is
performed to it.

BUG=576867
Committed: https://crrev.com/5cf9d45c437b7b2d899e46f2f324c147a2743eb7
Cr-Commit-Position: refs/heads/master@{#397066}

[nasko, 2016-05-27 23:05:13]

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

[nasko, 2016-05-27 23:05:14]

Hey Antony,
Can you review this CL for me?
Thanks in advance!
Nasko

[asargent_no_longer_on_chrome, 2016-05-31 18:17:38]

lgtm

https://codereview.chromium.org/2007133004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:349: // All MAIN_FRAME
and SUB_FRAME requests should succeed.
nit: mind updating this comment? Something like "All MAIN_FRAME requests should
succeed but SUB_FRAME requests that aren't in web_accesible_resources or a
same-origin parent should not"

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/private.html
(right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/private.html:5:
<script src="private.js"></script>
looks like private.js is missing from CL - I guess the test in
extension_resource_request_policy_apitest.cc verifies that we don't even try to
load private.html, so you could just drop the script tag here.

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/public.html
(right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/public.html:5:
<script src="public.js"></script>
public.js is missing - should we just remove the script tag here as well?

[nasko, 2016-05-31 23:16:17]

https://codereview.chromium.org/2007133004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_protocols_unittest.cc:349: // All MAIN_FRAME
and SUB_FRAME requests should succeed.
On 2016/05/31 18:17:38, Antony Sargent wrote:
> nit: mind updating this comment? Something like "All MAIN_FRAME requests
should
> succeed but SUB_FRAME requests that aren't in web_accesible_resources or a
> same-origin parent should not"

Done.

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/private.html
(right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/private.html:5:
<script src="private.js"></script>
On 2016/05/31 18:17:38, Antony Sargent wrote:
> looks like private.js is missing from CL - I guess the test in
> extension_resource_request_policy_apitest.cc verifies that we don't even try
to
> load private.html, so you could just drop the script tag here.
> 
> 

Done.

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/public.html
(right):

https://codereview.chromium.org/2007133004/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/extension_resource_request_policy/some_accessible/public.html:5:
<script src="public.js"></script>
On 2016/05/31 18:17:38, Antony Sargent wrote:
> public.js is missing - should we just remove the script tag here as well?

Done.

[nasko, 2016-05-31 23:16:25]

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

[commit-bot: I haz the power, 2016-05-31 23:17:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007133004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007133004/140001

[commit-bot: I haz the power, 2016-06-01 01:26:26]

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

[commit-bot: I haz the power, 2016-06-01 01:26:27]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-06-01 05:30:39]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-06-01 05:30:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2007133004/#ps140001
(title: "Fix issues from code review.")

[commit-bot: I haz the power, 2016-06-01 05:30:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007133004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007133004/140001

[commit-bot: I haz the power, 2016-06-01 05:35:07]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-01 05:36:18]

Description was changed from

==========
Disallow navigation to documents not explicitly listed as web accessible.

The existing check for web accessible resources is inadequate and allows
navigations to non-whitelisted pages to succeed. This patch ensures that
the document is listed explicitly in the manifest when a navigation is
performed to it.

BUG=576867
==========

to

==========
Disallow navigation to documents not explicitly listed as web accessible.

The existing check for web accessible resources is inadequate and allows
navigations to non-whitelisted pages to succeed. This patch ensures that
the document is listed explicitly in the manifest when a navigation is
performed to it.

BUG=576867

Committed: https://crrev.com/5cf9d45c437b7b2d899e46f2f324c147a2743eb7
Cr-Commit-Position: refs/heads/master@{#397066}
==========

[commit-bot: I haz the power, 2016-06-01 05:36:19]

Patchset 8 (id:??) landed as
https://crrev.com/5cf9d45c437b7b2d899e46f2f324c147a2743eb7
Cr-Commit-Position: refs/heads/master@{#397066}