Allow webview guests to skip WAR checks in ShouldAllowOpenURL.

Allow webview guests to skip WAR checks in ShouldAllowOpenURL.

Prior to r429532, the web-accessible resource checks in
ShouldAllowOpenURL were skipped for all schemes other than
HTTP/HTTPS/chrome-extension.  After r429532, we've started enforcing
them for most other schemes, but this breaks navigations from webview
guests to webview-accessible app resources whenever those go through
the OpenURL path, since the webview-accessible resource check is done
separately later, in AllowCrossRendererResourceLoadHelper, and
ShouldAllowOpenURL was killing the navigation before it ever got
there.

This CL will return webview guests to the previous behavior.

BUG=691941
Review-Url: https://codereview.chromium.org/2766313002
Cr-Commit-Position: refs/heads/master@{#458959}
Committed: https://chromium.googlesource.com/chromium/src/+/f89e04046aaac44ecad27056fb856c3d6e37370f

[alexmos, 2017-03-22 17:38:30]

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

[commit-bot: I haz the power, 2017-03-22 17:38:57]

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-22 19:06:12]

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

[commit-bot: I haz the power, 2017-03-22 19:06:13]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-03-22 21:16:41]

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

[alexmos, 2017-03-22 21:16:42]

Nick, can you please take a look?  I kept this simple so it's easier to merge to
M58.  There are remaining issues with the actual webview-accessible resource
check in AllowCrossRendererResourceLoadHelper (see https://crbug.com/691941#c5),
which I (or rob@robwu.nl, or someone else) will follow up on separately as part
of issue 640072.

[ncarter (slow), 2017-03-22 21:19:26]

lgtm

[alexmos, 2017-03-22 21:24:25]

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

[alexmos, 2017-03-22 21:24:25]

Thanks!  Adding OWNERS:
- Devlin for chrome_content_browser_client_extensions_part.cc
- lazyboy@ for the webview test

[lazyboy, 2017-03-22 21:54:03]

webview tests lgtm.

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js
(right):

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js:42:
if (didReload) {
nit: I think we've always done 2 space indentation for these in JS, e.g. see
line 63 below.

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js:54:
webview.executeScript({code: 'location.reload();'});
I'm assuming this causes ShouldAllowOpenURL()?

[Devlin, 2017-03-22 21:58:57]

You also wrote in the bug:
> it seems like we were allowing all app resources to load in <webview>, whether
or not they were webview-accessible, for web-triggerable transitions

Do we have a test for trying to load non-accessible resources?

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/apps/guest_v...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3309:
replace_path.SetPathStr("/assets/foo.html");
GURL::Replacements always drive me crazy... there's some bizarre ownership of
strings involved.  My recollection is that they don't actually own the strings
passed to them, and thus using them like this can result in UAF.  But maybe I'm
misremembering, or things have changed?

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:618:
// AllowCrossRendererResourceLoadHelper, so allow <webview> guests to proceed
This makes me a bit sad... what would need to happen to combine these checks? 
(I don't trust my knowledge of when each is called.)

[alexmos, 2017-03-22 21:59:14]

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

[commit-bot: I haz the power, 2017-03-22 21:59:41]

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-22 22:23:50]

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

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

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-22 23:56:39]

Thanks!  Responses below.

On 2017/03/22 21:58:57, Devlin wrote:
> You also wrote in the bug:
> > it seems like we were allowing all app resources to load in <webview>,
whether
> or not they were webview-accessible, for web-triggerable transitions
> 
> Do we have a test for trying to load non-accessible resources?

I didn't find one, but I might've missed some test files - the webview folks
would know this better.  We definitely don't have any tests for the webview
navigating itself to a non-webview-accessible page, since that doesn't work. :) 
I'll add a test for this (along with an actual fix) in a followup CL, since I
want to keep this one simple to merge to M58.

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/apps/guest_v...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/apps/guest_v...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3309:
replace_path.SetPathStr("/assets/foo.html");
On 2017/03/22 21:58:56, Devlin wrote:
> GURL::Replacements always drive me crazy... there's some bizarre ownership of
> strings involved.  My recollection is that they don't actually own the strings
> passed to them, and thus using them like this can result in UAF.  But maybe
I'm
> misremembering, or things have changed?

I'm not a big fan of replacements either, so I just changed this to avoid the
replacements altogether, which also saved a couple of lines. :)  Thanks for
mentioning this, I wasn't aware of it - looks like you're right about SetPathStr
not owning the string passed to it, though we might be ok here since string
literals should have static storage duration.  Certainly lots of other tests
seem to use this. :)

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2766313002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:618:
// AllowCrossRendererResourceLoadHelper, so allow <webview> guests to proceed
On 2017/03/22 21:58:56, Devlin wrote:
> This makes me a bit sad... what would need to happen to combine these checks? 
> (I don't trust my knowledge of when each is called.)

I think this one is called on the UI thread only for navigations, before the
request is kicked off, but only on the OpenURL/transfer path. 
AllowCrossRendererResourceLoad happens on the IO thread for all network
requests, including subresources.  So they're not exactly the same, and I think
we had issues doing some of those checks on the IO thread due to not having some
necessary info there.  One thing that might help is PlzNavigate, as (1) it will
remove the transfer path, so we won't need to worry about this for transfers
anymore, and (2) it already reuses AllowCrossRendererResourceLoadHelper for main
frame requests, which it calls from the UI thread in
ExtensionNavigationThrottle.  I think we could at least combine the checks here
with those in ExtensionNavigationThrottle.  I definitely agree we have too many
places enforcing these things, so this is something we need to think about for
sure.

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js
(right):

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js:42:
if (didReload) {
On 2017/03/22 21:54:03, lazyboy wrote:
> nit: I think we've always done 2 space indentation for these in JS, e.g. see
> line 63 below.

Done.

https://codereview.chromium.org/2766313002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view/load_webview_accessible_resource/embedder.js:54:
webview.executeScript({code: 'location.reload();'});
On 2017/03/22 21:54:02, lazyboy wrote:
> I'm assuming this causes ShouldAllowOpenURL()?

Yes, exactly.  ShouldAllowOpenURL is called from OpenURL, which is triggered
from ChromeExtensionsRendererClient::ShouldFork, as part of
decidePolicyForNavigation in the renderer, which this reload will hit (more
detail in https://crbug.com/691941#c7)

[Devlin, 2017-03-23 00:00:09]

lgtm

[alexmos, 2017-03-23 00:12:31]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2017-03-23 00:12:33]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2017-03-23 00:12:36]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2766313002/#ps40001
(title: "Devlin's comments")

[alexmos, 2017-03-23 00:12:39]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:12: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-23 00:46:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490227959084890,
"parent_rev": "38727920b34836a7a31c36c831c59fdcb1ed4dda", "commit_rev":
"f89e04046aaac44ecad27056fb856c3d6e37370f"}

[commit-bot: I haz the power, 2017-03-23 00:47:32]

Description was changed from

==========
Allow webview guests to skip WAR checks in ShouldAllowOpenURL.

Prior to r429532, the web-accessible resource checks in
ShouldAllowOpenURL were skipped for all schemes other than
HTTP/HTTPS/chrome-extension.  After r429532, we've started enforcing
them for most other schemes, but this breaks navigations from webview
guests to webview-accessible app resources whenever those go through
the OpenURL path, since the webview-accessible resource check is done
separately later, in AllowCrossRendererResourceLoadHelper, and
ShouldAllowOpenURL was killing the navigation before it ever got
there.

This CL will return webview guests to the previous behavior.

BUG=691941
==========

to

==========
Allow webview guests to skip WAR checks in ShouldAllowOpenURL.

Prior to r429532, the web-accessible resource checks in
ShouldAllowOpenURL were skipped for all schemes other than
HTTP/HTTPS/chrome-extension.  After r429532, we've started enforcing
them for most other schemes, but this breaks navigations from webview
guests to webview-accessible app resources whenever those go through
the OpenURL path, since the webview-accessible resource check is done
separately later, in AllowCrossRendererResourceLoadHelper, and
ShouldAllowOpenURL was killing the navigation before it ever got
there.

This CL will return webview guests to the previous behavior.

BUG=691941

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

[commit-bot: I haz the power, 2017-03-23 00:47:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f89e04046aaac44ecad27056fb85...