Allow transitions to WebUI pages which are extension urls (new tab page is the relevant example).

Base web accessibility decisions on page transition. We only need to perform the check if the transition is web triggerable.

BUG=137435
TEST=ExtensionResourceRequestPolicyTest.WebAccessibleResources
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150606

[Mihai Parparita -not on Chrome, 2012-07-31 23:19:33]

http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:69: // - new tab
page transitions will be in a seperate process and have the
I'm still confused why we end up with a top page URL that's chrome://swappedout.
In cases where an extension overrides the NTP, the top-level URL should be the
chrome-extension:// URL that is overriding the NTP.

What is frame->dataSource()->request().url() set to at this point?

[Charlie Reis, 2012-07-31 23:23:37]

On 2012/07/31 23:19:33, Mihai Parparita wrote:
>
http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
> File chrome/renderer/extensions/extension_resource_request_policy.cc (right):
> 
>
http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
> chrome/renderer/extensions/extension_resource_request_policy.cc:69: // - new
tab
> page transitions will be in a seperate process and have the
> I'm still confused why we end up with a top page URL that's
chrome://swappedout.
> In cases where an extension overrides the NTP, the top-level URL should be the
> chrome-extension:// URL that is overriding the NTP.
> 
> What is frame->dataSource()->request().url() set to at this point?

Yeah, I'm still wondering about this as well.  I just loaded it in a debugger
and found that it's swappedout://, which is strange.

Higher in the call stack, RenderViewImpl::OnNavigate has params.url =
chrome-extension://mjijecphoghgnciakogfniaiafllgpjp/layout.html.  Something
appears to be going wrong between there and the actual load.

It appears that we're loading the swappedout:// URL as a history item by pulling
it from params.state (in RenderViewImpl::OnNavigate).  That sounds like a major
bug to me.

[Charlie Reis, 2012-07-31 23:40:15]

On 2012/07/31 23:23:37, creis wrote:
> On 2012/07/31 23:19:33, Mihai Parparita wrote:
> >
>
http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
> > File chrome/renderer/extensions/extension_resource_request_policy.cc
(right):
> > 
> >
>
http://codereview.chromium.org/10827107/diff/1/chrome/renderer/extensions/ext...
> > chrome/renderer/extensions/extension_resource_request_policy.cc:69: // - new
> tab
> > page transitions will be in a seperate process and have the
> > I'm still confused why we end up with a top page URL that's
> chrome://swappedout.
> > In cases where an extension overrides the NTP, the top-level URL should be
the
> > chrome-extension:// URL that is overriding the NTP.
> > 
> > What is frame->dataSource()->request().url() set to at this point?
> 
> Yeah, I'm still wondering about this as well.  I just loaded it in a debugger
> and found that it's swappedout://, which is strange.
> 
> Higher in the call stack, RenderViewImpl::OnNavigate has params.url =
> chrome-extension://mjijecphoghgnciakogfniaiafllgpjp/layout.html.  Something
> appears to be going wrong between there and the actual load.
> 
> It appears that we're loading the swappedout:// URL as a history item by
pulling
> it from params.state (in RenderViewImpl::OnNavigate).  That sounds like a
major
> bug to me.

No, upon closer inspection, OnNavigate is doing the right thing.  We're using
the correct chrome-extension:// URL for the load, and it's present as
"resource_url" in this CanRequestResource function.

Maybe I'm missing the the point of this function?  The problem in this case is
IsResourceWebAccessible is returning false for
chrome-extension://mjijecphoghgnciakogfniaiafllgpjp/layout.html.  We shouldn't
make an exception for it just because the current page is swappedout://.

Is it supposed to be web accessible?  Is there a reason we're not catching this
the first time the page is loaded?  (Maybe the is_empty_origin exception is
actually a problem, and we should have blocked it the first time.)

[Cris Neckar, 2012-07-31 23:46:08]

I think that the right fix might be to find a way to determine that we are in
this condition inside CanRequestResource() and never actually call
IsResourceWebAccessible(). Mihai, if it is a top level navigation do we ever
care to block non web accessible resources?

If there a way to determine if it is a top level navigation inside of
CanRequestResource()?

[Cris Neckar, 2012-07-31 23:49:34]

Actually I take that back. We don't want pages to be able to redirect into an
extension which would work if we did that.

[Mihai Parparita -not on Chrome, 2012-08-01 00:16:49]

On Tue, Jul 31, 2012 at 4:40 PM, <creis@chromium.org> wrote:

> Maybe I'm missing the the point of this function?  The problem in this
> case is
> IsResourceWebAccessible is returning false for
> chrome-extension://**mjijecphoghgnciakogfniaiafllgp**jp/layout.html.  We
> shouldn't
> make an exception for it just because the current page is swappedout://.
>

I'm still not understanding how we send up with the swappedout
URL. chrome-extension://**mjijecphoghgnciakogfniaiafllgp**jp/layout.html is
being loaded into a top-level tab, I would expect the page_url to be equal
to that, and therefore the IsResourceWebAccessible check to pass.

Mihai

[Charlie Reis, 2012-08-01 00:22:56]

On 2012/08/01 00:16:49, Mihai Parparita wrote:
> I'm still not understanding how we send up with the swappedout
> URL. chrome-extension://**mjijecphoghgnciakogfniaiafllgp**jp/layout.html is
> being loaded into a top-level tab, I would expect the page_url to be equal
> to that, and therefore the IsResourceWebAccessible check to pass.
> 
> Mihai

That part is clear: when you navigate away from the NTP to a RenderView in a
different process, we load swappedout:// as the top-level page in the old
RenderView.  When you click Back, we're about to go from swappedout:// (the last
committed page) to the chrome-extension:// URL (the destination page) when the
check happens.

Did you expect page_url to be equal to the destination page, or the last
committed page?

[Mihai Parparita -not on Chrome, 2012-08-01 00:30:43]

On Tue, Jul 31, 2012 at 5:22 PM, <creis@chromium.org> wrote:

> That part is clear: when you navigate away from the NTP to a RenderView in
> a
> different process, we load swappedout:// as the top-level page in the old
> RenderView.  When you click Back, we're about to go from swappedout://
> (the last
> committed page) to the chrome-extension:// URL (the destination page) when
> the
> check happens.
>
> Did you expect page_url to be equal to the destination page, or the last
> committed page?
>

The last committed page, so the current behavior makes sense.

From talking to Cris, I now understand that IsResourceWebAccessible is
meant to do two things:
1. Prevent web content from loading extension URLs as sub-resources (i.e.
as images, iframes, etc.)
2. Prevent web content from linking/redirecting/navigating to extension URLs

I was only thinking of 1 until now. I think doing 2 is more nuanced than
just URL checks (history traversals are OK, but links are not), so perhaps
it should be done separately, instead of
complicating IsResourceWebAccessible further.

Mihai

[Cris Neckar, 2012-08-01 17:54:07]

Bah, apparently this isn't actually sufficient. I am just going to pipe through
the navigation type today.

[Cris Neckar, 2012-08-01 23:17:41]

Ok this seems like the correct way to do this. We'll let the trybots tell me how
wrong I am.

[Charlie Reis, 2012-08-02 18:39:25]

Just a few questions, apart from the try job failures.

http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:52: // Disallow
loading of extension resources which are not explicitely listed
nit: explicitly (as long as we're cleaning stuff up)

http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:68: // In all
other cases we forbid certain types of transitions.
I don't quite follow this, with all the exceptions and negatives.  What's the
high level intention?  That certain types of page transitions should be allowed?
 Which ones?

http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
content/public/common/content_client.h:84: virtual void CustomStreamAddURL(const
GURL& url, bool top_level_resource) {}
What are these for?

[Cris Neckar, 2012-08-02 18:55:24]

On 2012/08/02 18:39:25, creis wrote:
> Just a few questions, apart from the try job failures.
> 
>
http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
> File chrome/renderer/extensions/extension_resource_request_policy.cc (right):
> 
>
http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
> chrome/renderer/extensions/extension_resource_request_policy.cc:52: //
Disallow
> loading of extension resources which are not explicitely listed
> nit: explicitly (as long as we're cleaning stuff up)
> 
>
http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
> chrome/renderer/extensions/extension_resource_request_policy.cc:68: // In all
> other cases we forbid certain types of transitions.
> I don't quite follow this, with all the exceptions and negatives.  What's the
> high level intention?  That certain types of page transitions should be
allowed?
>  Which ones?
> 
>
http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
> File content/public/common/content_client.h (right):
> 
>
http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
> content/public/common/content_client.h:84: virtual void
CustomStreamAddURL(const
> GURL& url, bool top_level_resource) {}
> What are these for?

Aaron just made a huge change to this function so please ignore this CL for
now.. gonna need to figure out what state this is currently in.

[Cris Neckar, 2012-08-02 19:08:26]

http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10827107/diff/5007/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:68: // In all
other cases we forbid certain types of transitions.
On 2012/08/02 18:39:26, creis wrote:
> I don't quite follow this, with all the exceptions and negatives.  What's the
> high level intention?  That certain types of page transitions should be
allowed?
>  Which ones?

Yes the intent was to ensure that certain types of page transitions are always
allowed. Namely anything that is not web triggerable (UI interaction etc).

Perhaps a helpers function in page_transition_types TransitionIsWebTriggerable()
or similar would be less prone to future breakage?

http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/10827107/diff/5007/content/public/common/conte...
content/public/common/content_client.h:84: virtual void CustomStreamAddURL(const
GURL& url, bool top_level_resource) {}
On 2012/08/02 18:39:26, creis wrote:
> What are these for?

A totally different change... oops

[Cris Neckar, 2012-08-02 23:54:51]

Ok I think this is good now. creis, should i move this into a helper function or
is it good like this?

[Charlie Reis, 2012-08-02 23:58:55]

On 2012/08/02 23:54:51, Cris Neckar wrote:
> Ok I think this is good now. creis, should i move this into a helper function
or
> is it good like this?

I think it'd be much clearer what condition you're trying to detect if you move
it into a helper function.  It'll also be easier to spot if we need to add
something to it.

[Charlie Reis, 2012-08-03 21:36:10]

LGTM, but an improved comment would be nice (noted below), and you'll need a
separate owner's review for the content/public changes.

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:8: #include <vector>
Don't think you need this, right?

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/extensions/ex...
File chrome/renderer/extensions/extension_resource_request_policy.h (right):

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/extensions/ex...
chrome/renderer/extensions/extension_resource_request_policy.h:21: // from
|frame_url|.
Can we say something in this comment about why the PageTransition is relevant? 
It's not immediately obvious.

[Mihai Parparita -not on Chrome, 2012-08-03 22:27:22]

Can you add some tests?

[Cris Neckar, 2012-08-06 19:25:49]

On 2012/08/03 22:27:22, Mihai Parparita wrote:
> Can you add some tests?

Mihai, added a test for the condition in the bug.

[Cris Neckar, 2012-08-06 19:25:58]

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:8: #include <vector>
On 2012/08/03 21:36:11, creis wrote:
> Don't think you need this, right?

It's not for me. It is a lint error that I was resolving along with this.

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/extensions/ex...
File chrome/renderer/extensions/extension_resource_request_policy.h (right):

http://codereview.chromium.org/10827107/diff/28/chrome/renderer/extensions/ex...
chrome/renderer/extensions/extension_resource_request_policy.h:21: // from
|frame_url|.
On 2012/08/03 21:36:11, creis wrote:
> Can we say something in this comment about why the PageTransition is relevant?

> It's not immediately obvious.

Done.

[Cris Neckar, 2012-08-06 19:32:33]

+kalman for owners review

[not at google - send to devlin, 2012-08-06 22:14:11]

mihai is an owner?

[Cris Neckar, 2012-08-06 22:16:12]

On 2012/08/06 22:14:11, kalman wrote:
> mihai is an owner?

Oh strange.. it suggested you for some reason.

[Cris Neckar, 2012-08-06 22:22:05]

+jam for chrome/ owners review

[Mihai Parparita -not on Chrome, 2012-08-07 01:13:12]

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:214:
"web_accessible/accessible_newtab_override.html"));
Can you name this file something else? It doesn't actually override the NTP, so
the name was confusing.

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:222:
EXPECT_EQ("New Tab Page Loaded Successfully", result);
This tests that reported bug was fixed, but I'm not seeing a test that
navigations with the PAGE_TRANSITION_LINK transition aren't allowed.

[jam, 2012-08-07 16:17:01]

lgtm

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
File content/public/renderer/content_renderer_client.cc (right):

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
content/public/renderer/content_renderer_client.cc:68: content::PageTransition
transition_type,
ditto

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:142: content::PageTransition
transition_type,
nit: content:: is redundant

[Mihai Parparita -not on Chrome, 2012-08-07 18:33:37]

LGTM

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:222:
EXPECT_EQ("New Tab Page Loaded Successfully", result);
On 2012/08/07 01:13:12, Mihai Parparita wrote:
> This tests that reported bug was fixed, but I'm not seeing a test that
> navigations with the PAGE_TRANSITION_LINK transition aren't allowed.

Cris explained that this CL doesn't actually disallow those navigations.

[Cris Neckar, 2012-08-07 18:45:53]

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_resource_request_policy_apitest.cc
(right):

http://codereview.chromium.org/10827107/diff/7019/chrome/browser/extensions/e...
chrome/browser/extensions/extension_resource_request_policy_apitest.cc:214:
"web_accessible/accessible_newtab_override.html"));
On 2012/08/07 01:13:12, Mihai Parparita wrote:
> Can you name this file something else? It doesn't actually override the NTP,
so
> the name was confusing.

Done.

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
File content/public/renderer/content_renderer_client.cc (right):

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
content/public/renderer/content_renderer_client.cc:68: content::PageTransition
transition_type,
On 2012/08/07 16:17:01, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/10827107/diff/7019/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:142: content::PageTransition
transition_type,
On 2012/08/07 16:17:01, John Abd-El-Malek wrote:
> nit: content:: is redundant

Done.

[commit-bot: I haz the power, 2012-08-07 19:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/10051

[commit-bot: I haz the power, 2012-08-07 19:02:09]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-07 19:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/10051

[commit-bot: I haz the power, 2012-08-07 19:02:27]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-07 19:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/10051

[commit-bot: I haz the power, 2012-08-07 19:05:27]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-07 19:11:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/11024

[commit-bot: I haz the power, 2012-08-07 19:11:40]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-07 23:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/8029

[commit-bot: I haz the power, 2012-08-07 23:26:07]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-08 19:51:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cdn@chromium.org/10827107/10069

[commit-bot: I haz the power, 2012-08-08 19:51:45]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p0 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej