Allow navigations to non-web-accessible resources from chrome schemes.

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=662602
Committed: https://crrev.com/cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4
Cr-Commit-Position: refs/heads/master@{#431074}

[alexmos, 2016-11-09 06:58:30]

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

[commit-bot: I haz the power, 2016-11-09 06:58:45]

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-11-09 07:44:18]

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

[commit-bot: I haz the power, 2016-11-09 07:44:19]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-11-09 18:32:31]

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

[alexmos, 2016-11-09 18:32:31]

Devlin, Nick - can you please take a look?  I checked that the NTP extension in
the bug now works for three cases that were previously broken:
- navigating to the extension's NTP URL from chrome://history
- clicking on the tile in old (chrome-search://) NTP for the extension's NTP URL
- redirecting to extension's NTP URL by a content script running on
chrome-search:// site URL.

[ncarter (slow), 2016-11-09 18:44:21]

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

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:539:
site_url.SchemeIs(chrome::kChromeSearchScheme)) {
Are we sure we want to allow this for extension blob/filesystem targets too? Or
can we limit this to "&& to_url.SchemeIs(kExtensionScheme)" ?

I don't have a particular case in mind, other than minimizing privileges.

[ncarter (slow), 2016-11-09 19:10:15]

Given that filesystem URLs can appear in history, I think a blanket relaxation
is probably okay.

lgtm

[Devlin, 2016-11-09 21:54:43]

lgtm

Something else I thought of: we allow extensions to override ntp, bookmarks,
etc.  It's conceivable they may want to open extension pages from that page to
offer similar functionality, but that'll break here.  However, I think that a)
trying to untangle the case of it being valid or invalid for those purposes is
probably impossible and
b) this should have always failed, even before your original patch, since
from_extension != to_extension and the resource isn't web accessible.

So I think we're okay - just making a non-mental note that it's something to
think about...

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/window_open_apitest.cc (right):

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/window_open_apitest.cc:343:
last_loaded_extension_id()),
This might be cleaner as:
const Extension* extension = LoadExtension(...);
ASSERT_TRUE(extension);
GURL extension_url = extension->GetResourceURL("test.html");

Sorry I didn't notice this in the earlier one; for some reason I didn't see that
we were loading the extension right above it.

[alexmos, 2016-11-09 21:59:50]

On 2016/11/09 19:10:15, ncarter wrote:
> Given that filesystem URLs can appear in history, I think a blanket relaxation
> is probably okay.

Yes, and blob URLs appear in history too, actually.  (They just resolve to an
error if you click on them in history after the creator document is closed.)

But interestingly, I just tried this, and with a plain/middle/ctrl click on
blob/filesystem URLs from chrome://history, the navigation is already blocked by
Blink's "Not allowed to load local resource" logic.  So given that, we could
actually tighten up the check for nested URLs, I think.  (FWIW, right-clicking
them from history and opening in new tab/window works, with and without this
CL.)

[alexmos, 2016-11-09 22:15:20]

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

[commit-bot: I haz the power, 2016-11-09 22:16:04]

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

[alexmos, 2016-11-09 22:19:20]

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

[commit-bot: I haz the power, 2016-11-09 22:19:45]

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

[alexmos, 2016-11-09 22:20:31]

Thanks for reviewing!

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

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:539:
site_url.SchemeIs(chrome::kChromeSearchScheme)) {
On 2016/11/09 18:44:21, ncarter wrote:
> Are we sure we want to allow this for extension blob/filesystem targets too?
Or
> can we limit this to "&& to_url.SchemeIs(kExtensionScheme)" ?
> 
> I don't have a particular case in mind, other than minimizing privileges.

Done, given the observation that Blink already blocks this.  Instead of adding
the scheme check, I just moved this down to happen after the nested URL check
(and added a comment).  Can you check if that makes sense?

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/window_open_apitest.cc (right):

https://codereview.chromium.org/2486843003/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/window_open_apitest.cc:343:
last_loaded_extension_id()),
On 2016/11/09 21:54:43, Devlin (slow) wrote:
> This might be cleaner as:
> const Extension* extension = LoadExtension(...);
> ASSERT_TRUE(extension);
> GURL extension_url = extension->GetResourceURL("test.html");
> 
> Sorry I didn't notice this in the earlier one; for some reason I didn't see
that
> we were loading the extension right above it.

Done for both tests.  Much cleaner, thanks for the suggestion!

[ncarter (slow), 2016-11-09 22:24:07]

lgtm

[alexmos, 2016-11-09 22:42:09]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2016-11-09 22:42:10]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-11-09 22:42:10]

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

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

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-11-09 23:20:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-09 23:27:48]

Description was changed from

==========
Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=662602
==========

to

==========
Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=662602

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

[commit-bot: I haz the power, 2016-11-09 23:27:49]

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