Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

AllowCrossRendererResourceLoadHelper is supposed to check whether a
resource that a webview guest tries to load is webview-accessible [1],
and denies the load if it isn't.  However, it currently has an
exception for all web-triggerable page transitions, blindly allowing
them through.  This allows a webview to happily navigate itself to
non-webview-accessible resources.

The page transition exception seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.  The fix was
originally proposed by Rob in https://crbug.com/640072.

[1] https://developer.chrome.com/apps/tags/webview#local_resources

BUG=640072, 691941
Review-Url: https://codereview.chromium.org/2768863003
Cr-Commit-Position: refs/heads/master@{#459326}
Committed: https://chromium.googlesource.com/chromium/src/+/a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075

[alexmos, 2017-03-23 01:32:24]

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

[commit-bot: I haz the power, 2017-03-23 01:33:19]

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-03-23 01:42:37]

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

[commit-bot: I haz the power, 2017-03-23 01:42:38]

Dry run: 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...)

[alexmos, 2017-03-23 17:24:10]

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

[commit-bot: I haz the power, 2017-03-23 17:24:27]

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

[alexmos, 2017-03-23 17:46:40]

Description was changed from

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

BUG=640072
==========

to

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

The code in AllowCrossRendererResourceLoadHelper is supposed to check
whether a resource that a webview guest tries to load is
webview-accessible, and denies the load if it isn't.  However, it
currently has a hole for all web-triggerable page transitions, blindly
allowing them through.  This allows a webview to navigate itself to
non-webview-accessible resources.

The page transition check seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.

BUG=640072,691941
==========

[alexmos, 2017-03-23 18:01:48]

Description was changed from

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

The code in AllowCrossRendererResourceLoadHelper is supposed to check
whether a resource that a webview guest tries to load is
webview-accessible, and denies the load if it isn't.  However, it
currently has a hole for all web-triggerable page transitions, blindly
allowing them through.  This allows a webview to navigate itself to
non-webview-accessible resources.

The page transition check seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.

BUG=640072,691941
==========

to

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

AllowCrossRendererResourceLoadHelper is supposed to check whether a
resource that a webview guest tries to load is webview-accessible [1],
and denies the load if it isn't.  However, it currently has an
exception for all web-triggerable page transitions, blindly allowing
them through.  This allows a webview to happily navigate itself to
non-webview-accessible resources.

The page transition exception seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.  The fix was
originally proposed by Rob in https://crbug.com/640072.

[1] https://developer.chrome.com/apps/tags/webview#local_resources

BUG=640072,691941
==========

[alexmos, 2017-03-23 18:23:53]

alexmos@chromium.org changed reviewers:
+ lfg@chromium.org

[alexmos, 2017-03-23 18:23:54]

Lucas, can you please take a look?  We discussed this a while back, and I
finally got around to it.

CC'ing Rob Wu, who proposed this in 640072.  I agree with Rob's analysis there.

https://codereview.chromium.org/2768863003/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (right):

https://codereview.chromium.org/2768863003/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:158: if (owner_extension != extension) {
Seems like the old check also allowed web-triggerable loads to any non-owner
extension.  Is there any case where a webview should be loading resources from
an app/extension that doesn't own it?  If we wanted to be conservative here, we
could also return false without setting *allowed for that case.  But this
stricter check doesn't break any tests either. :)

[lfg, 2017-03-23 18:49:40]

Thanks for fixing this, lgtm.

https://codereview.chromium.org/2768863003/diff/20001/extensions/browser/url_...
File extensions/browser/url_request_util.cc (right):

https://codereview.chromium.org/2768863003/diff/20001/extensions/browser/url_...
extensions/browser/url_request_util.cc:158: if (owner_extension != extension) {
On 2017/03/23 18:23:53, alexmos wrote:
> Seems like the old check also allowed web-triggerable loads to any non-owner
> extension.  Is there any case where a webview should be loading resources from
> an app/extension that doesn't own it?  If we wanted to be conservative here,
we
> could also return false without setting *allowed for that case.  But this
> stricter check doesn't break any tests either. :)

This is a bug I fixed in the past, but Paul reintroduced it here
https://codereview.chromium.org/1312653003 (PS5->PS6; shame on me for not
looking at the CL after l-g-t-m'ing it).

<webview> should only load resources specified in the manifest of the extension
that owns it.

+cc paulmeyer

[commit-bot: I haz the power, 2017-03-23 19:29:28]

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

[commit-bot: I haz the power, 2017-03-23 19:29:29]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-03-23 21:20:43]

alexmos@chromium.org changed reviewers:
+ lazyboy@chromium.org, rdevlin.cronin@chromium.org

[alexmos, 2017-03-23 21:20:44]

Thanks, adding OWNERS:
- Devlin for extensions/browser/url_request_util.cc
- lazyboy@ for web_view_browsertest.cc

[Devlin, 2017-03-23 21:33:40]

Yay!  lgtm

[lazyboy, 2017-03-24 01:08:26]

tests lgtm.

[alexmos, 2017-03-24 01:13:39]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:13:57]

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-03-24 01:23:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490318019454770,
"parent_rev": "ffb0ffaccfcded76b0165c24c6ba0c8c4712089c", "commit_rev":
"a7d08ae5f1e081b95ae6f4e1f163cbd0be6dc075"}

[commit-bot: I haz the power, 2017-03-24 01:24:02]

Description was changed from

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

AllowCrossRendererResourceLoadHelper is supposed to check whether a
resource that a webview guest tries to load is webview-accessible [1],
and denies the load if it isn't.  However, it currently has an
exception for all web-triggerable page transitions, blindly allowing
them through.  This allows a webview to happily navigate itself to
non-webview-accessible resources.

The page transition exception seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.  The fix was
originally proposed by Rob in https://crbug.com/640072.

[1] https://developer.chrome.com/apps/tags/webview#local_resources

BUG=640072,691941
==========

to

==========
Fix webview-accessible resource checks in AllowCrossRendererResourceLoadHelper

AllowCrossRendererResourceLoadHelper is supposed to check whether a
resource that a webview guest tries to load is webview-accessible [1],
and denies the load if it isn't.  However, it currently has an
exception for all web-triggerable page transitions, blindly allowing
them through.  This allows a webview to happily navigate itself to
non-webview-accessible resources.

The page transition exception seems wrong, and rob@robwu.nl made a
detailed analysis of how we ended up with it in
https://crbug.com/633963#c14.  This CL removes it.  The fix was
originally proposed by Rob in https://crbug.com/640072.

[1] https://developer.chrome.com/apps/tags/webview#local_resources

BUG=640072,691941

Review-Url: https://codereview.chromium.org/2768863003
Cr-Commit-Position: refs/heads/master@{#459326}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7d08ae5f1e081b95ae6f4e1f163...
==========

[commit-bot: I haz the power, 2017-03-24 01:24:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a7d08ae5f1e081b95ae6f4e1f163...