Only disallow top-level navigations in platform apps

r118246 added a mode where all navigations were sent to the browser process, so
that for platform apps we could ignore them. However, we still want to allow
platform apps to have iframes that point to app pages or data: URLs, so the
check needs to be made a bit more fine-grained.

The fine-grained check will be implemented in a follow-up CL (via CSP), for now all
frame navigations are allowed.

R=miket@chromium.org,abarth@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138304

[Mihai Parparita -not on Chrome, 2012-05-11 00:30:59]

http://codereview.chromium.org/10387074/diff/3001/content/public/common/rende...
File content/public/common/renderer_preferences.h (right):

http://codereview.chromium.org/10387074/diff/3001/content/public/common/rende...
content/public/common/renderer_preferences.h:76: bool
browser_handles_all_top_level_or_non_local_requests;
This name is kind of a mouthful. An alternative is to add a content client
method BorwserHandlesRequest and put this logic (which is only needed for
platform apps) on the chrome side. John, do you have a preference?

[miket_OOO, 2012-05-11 18:29:07]

LGTM. Just some silly nits.

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/iframes/main.html (right):

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/iframes/main.html:2: * Copyright (c)
2012 The Chromium Authors. All rights reserved.  Use of this
Is this boilerplate? There's one space after the first period, then two after
the second.

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/iframes/remote-iframe.html
(right):

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/iframes/remote-iframe.html:2: *
Copyright (c) 2012 The Chromium Authors. All rights reserved.  Use of this
Same.

http://codereview.chromium.org/10387074/diff/3001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/3001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5223: // data: URLs don't involve remote
content either.
Tiny nit: "don't involve remote" is a different kind of double negative from
"NonLocal," and it makes the code slightly harder to review. Can you rephrase
the comment using the same terminology as the method name?

[Mihai Parparita -not on Chrome, 2012-05-11 19:40:49]

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/iframes/main.html (right):

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/iframes/main.html:2: * Copyright (c)
2012 The Chromium Authors. All rights reserved.  Use of this
On 2012/05/11 18:29:08, miket wrote:
> Is this boilerplate? There's one space after the first period, then two after
> the second.

I copied and pasted this from another file. Fixed.

P.S. This is what happens when you enforce this header via regular expressions:
http://code.google.com/searchframe#OAMlx_jo-ck/tools/depot_tools/presubmit_ca...

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/iframes/remote-iframe.html
(right):

http://codereview.chromium.org/10387074/diff/3001/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/iframes/remote-iframe.html:2: *
Copyright (c) 2012 The Chromium Authors. All rights reserved.  Use of this
On 2012/05/11 18:29:08, miket wrote:
> Same.

Done.

http://codereview.chromium.org/10387074/diff/3001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/3001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5223: // data: URLs don't involve remote
content either.
On 2012/05/11 18:29:08, miket wrote:
> Tiny nit: "don't involve remote" is a different kind of double negative from
> "NonLocal," and it makes the code slightly harder to review. Can you rephrase
> the comment using the same terminology as the method name?

Rephrased.

[Mihai Parparita -not on Chrome, 2012-05-11 19:42:11]

John's OOO this week. Darin, do you have time to look at these
navigation-related content/ changes?

[darin (slow to review), 2012-05-12 00:00:50]

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5218: bool
RenderViewImpl::IsNonLocalOrTopLevelNavigation(
it seems like you are using "NonLocal" to mean !same-origin.  maybe it would be
good to use the word "origin" in the function name?  it is a little bit longer,
but IsDifferentOriginOrTopLevelNavigation seems a bit clearer to me.

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5227: return url.GetOrigin() !=
GURL(frame->top()->document().url()).GetOrigin();
why do you check the origin of frame->top() as opposed to just the origin of the
frame?  should you be comparing WebSecurityOrigin objects instead?

[Mihai Parparita -not on Chrome, 2012-05-15 20:51:46]

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5218: bool
RenderViewImpl::IsNonLocalOrTopLevelNavigation(
On 2012/05/12 00:00:51, darin wrote:
> it seems like you are using "NonLocal" to mean !same-origin.  maybe it would
be
> good to use the word "origin" in the function name?  it is a little bit
longer,
> but IsDifferentOriginOrTopLevelNavigation seems a bit clearer to me.

The problem is that data: URLs are allowed too, so it's not just about strict
origin checks.

How about IsRemoteOrTopLevelNavigation?

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5227: return url.GetOrigin() !=
GURL(frame->top()->document().url()).GetOrigin();
On 2012/05/12 00:00:51, darin wrote:
> why do you check the origin of frame->top() as opposed to just the origin of
the
> frame?  should you be comparing WebSecurityOrigin objects instead?

The frame hasn't navigated yet, so it's at about:blank, so its origin isn't
relevant. If I use WebSecurityOrigin (presumably you meant the canRequest
method), then I get some undesired side-effects:
- canRequest returns false for data: URLs
- canRequest returns true for origins that the app is whitelisted to load data
from (via its permissions). However, we still don't want to allow loading of
those remote URLs.

[darin (slow to review), 2012-05-15 23:58:43]

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5218: bool
RenderViewImpl::IsNonLocalOrTopLevelNavigation(
On 2012/05/15 20:51:46, Mihai Parparita wrote:
> On 2012/05/12 00:00:51, darin wrote:
> > it seems like you are using "NonLocal" to mean !same-origin.  maybe it would
> be
> > good to use the word "origin" in the function name?  it is a little bit
> longer,
> > but IsDifferentOriginOrTopLevelNavigation seems a bit clearer to me.
> 
> The problem is that data: URLs are allowed too, so it's not just about strict
> origin checks.
> 
> How about IsRemoteOrTopLevelNavigation?

What about blob URLs or filesystem URLs?  I think I'm pretty confused about the
desired policy.  Blob/Filesystem URLs have an embedded origin, and yet they are
local resources.

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5227: return url.GetOrigin() !=
GURL(frame->top()->document().url()).GetOrigin();
On 2012/05/15 20:51:46, Mihai Parparita wrote:
> On 2012/05/12 00:00:51, darin wrote:
> > why do you check the origin of frame->top() as opposed to just the origin of
> the
> > frame?  should you be comparing WebSecurityOrigin objects instead?
> 
> The frame hasn't navigated yet, so it's at about:blank, so its origin isn't
> relevant. If I use WebSecurityOrigin (presumably you meant the canRequest
> method), then I get some undesired side-effects:
> - canRequest returns false for data: URLs
> - canRequest returns true for origins that the app is whitelisted to load data
> from (via its permissions). However, we still don't want to allow loading of
> those remote URLs.

But what if the top-most frame was created dynamically via window.open(), and
then the opener just scribbled some HTML into it?  The URL of the top-most frame
would be about:blank in this case, yet its security origin would be that of the
opener.  What if that window contains an IFRAME that is in the same origin as
the top-most window and opener?  The URL-based checks fail to capture subtle
cases like this, right?

[Mihai Parparita -not on Chrome, 2012-05-16 00:36:45]

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5218: bool
RenderViewImpl::IsNonLocalOrTopLevelNavigation(
On 2012/05/15 23:58:44, darin wrote:
> What about blob URLs or filesystem URLs?  I think I'm pretty confused about
the
> desired policy.  Blob/Filesystem URLs have an embedded origin, and yet they
are
> local resources.

Both of those should be allowed (added them to the test).

filesystem: URLs are handled by GURL::GetOrigin (it returns the inner URL's
origin, see
http://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exac...).
blob: ones aren't, I've added them to the special-case list along with data:
URLs.

http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5227: return url.GetOrigin() !=
GURL(frame->top()->document().url()).GetOrigin();
On 2012/05/15 23:58:44, darin wrote:
> But what if the top-most frame was created dynamically via window.open(), and
> then the opener just scribbled some HTML into it?  The URL of the top-most
frame
> would be about:blank in this case, yet its security origin would be that of
the
> opener.  What if that window contains an IFRAME that is in the same origin as
> the top-most window and opener?  The URL-based checks fail to capture subtle
> cases like this, right?

I've switched to using the document's security origin. It feels a bit weird to
be turning a WebSecurityOrigin into a GURL, but I guess that toString() docs say
it is a valid URL, and other places to it too:

http://code.google.com/p/chromium/source/search?q=GURL.*securityOrigin.*toStr...

BTW, I was following the pattern of IsNonLocalTopLevelNavigation (see below) for
comparing origins. I think that needs to be fixed too, but I think it's for
ChromeFrame usage, so I'm scared to touch it.

[darin (slow to review), 2012-05-16 06:37:12]

I think you should get Adam Barth to review this code.  I get nervous
reviewing anything having to do with origin checks.

-Darin




On Tue, May 15, 2012 at 5:36 PM, <mihaip@chromium.org> wrote:

>
> http://codereview.chromium.**org/10387074/diff/6001/**
>
content/renderer/render_view_**impl.cc<http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> http://codereview.chromium.**org/10387074/diff/6001/**
>
content/renderer/render_view_**impl.cc#newcode5218<http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_view_impl.cc#newcode5218>
> content/renderer/render_view_**impl.cc:5218: bool
> RenderViewImpl::**IsNonLocalOrTopLevelNavigation**(
> On 2012/05/15 23:58:44, darin wrote:
>
>> What about blob URLs or filesystem URLs?  I think I'm pretty confused
>>
> about the
>
>> desired policy.  Blob/Filesystem URLs have an embedded origin, and yet
>>
> they are
>
>> local resources.
>>
>
> Both of those should be allowed (added them to the test).
>
> filesystem: URLs are handled by GURL::GetOrigin (it returns the inner
> URL's origin, see
> http://code.google.com/**searchframe#OAMlx_jo-ck/src/**
> googleurl/src/gurl.cc&exact_**package=chromium&q=gurl%**
>
20GetOrigin&type=cs&l=333<http://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exact_package=chromium&q=gurl%20GetOrigin&type=cs&l=333>
> ).
> blob: ones aren't, I've added them to the special-case list along with
> data: URLs.
>
>
> http://codereview.chromium.**org/10387074/diff/6001/**
>
content/renderer/render_view_**impl.cc#newcode5227<http://codereview.chromium.org/10387074/diff/6001/content/renderer/render_view_impl.cc#newcode5227>
> content/renderer/render_view_**impl.cc:5227: return url.GetOrigin() !=
> GURL(frame->top()->document().**url()).GetOrigin();
> On 2012/05/15 23:58:44, darin wrote:
>
>> But what if the top-most frame was created dynamically via
>>
> window.open(), and
>
>> then the opener just scribbled some HTML into it?  The URL of the
>>
> top-most frame
>
>> would be about:blank in this case, yet its security origin would be
>>
> that of the
>
>> opener.  What if that window contains an IFRAME that is in the same
>>
> origin as
>
>> the top-most window and opener?  The URL-based checks fail to capture
>>
> subtle
>
>> cases like this, right?
>>
>
> I've switched to using the document's security origin. It feels a bit
> weird to be turning a WebSecurityOrigin into a GURL, but I guess that
> toString() docs say it is a valid URL, and other places to it too:
>
> http://code.google.com/p/**chromium/source/search?q=GURL.**
>
*securityOrigin.*toString&**origq=GURL.*securityOrigin.***toString<http://code.google.com/p/chromium/source/search?q=GURL.*securityOrigin.*toString&origq=GURL.*securityOrigin.*toString>
>
> BTW, I was following the pattern of IsNonLocalTopLevelNavigation (see
> below) for comparing origins. I think that needs to be fixed too, but I
> think it's for ChromeFrame usage, so I'm scared to touch it.
>
>
http://codereview.chromium.**org/10387074/<http://codereview.chromium.org/103...
>

[Mihai Parparita -not on Chrome, 2012-05-16 17:34:55]

Adam, can you take a look at this?

The intent is to implement the platform app navigation policy (allow the top
frame to never be navigated, and child frames to never load remote URLs)

Thanks,
Mihai

[abarth-chromium, 2012-05-18 18:28:12]

We can't enforce this with CSP?

[abarth-chromium, 2012-05-18 18:34:07]

I talked with Mihai, and he's going to investigate a CSP-based approach.  Below
are some random notes.

http://codereview.chromium.org/10387074/diff/13001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5311: bool
RenderViewImpl::IsRemoteOrTopLevelNavigation(
"remote" is sort of a funny term to use here.  For example, "file" URLs aren't
remote, but this function will return true.

http://codereview.chromium.org/10387074/diff/13001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5318: if (url.SchemeIs(chrome::kBlobScheme)
|| url.SchemeIs(chrome::kDataScheme))
Presumably filesystem is in this category too.

[Mihai Parparita -not on Chrome, 2012-05-18 22:43:05]

On 2012/05/18 18:34:07, abarth wrote:
> I talked with Mihai, and he's going to investigate a CSP-based approach.

Actually, I'd like to do that in a follow up CL, so that I can get this one
landed and unblock Mike and others that need to use (local) iframes in their
app.

All this CL does now is to change browser_handles_all_requests to
browser_handles_all_top_level_requests (confusingly, there was already a
renderer preference with that name, but it actually meant
browser_handles_non_local_top_level_requests, so I've renamed it to that).

[abarth-chromium, 2012-05-21 20:44:19]

It's hard for me to review this patch because most of this code is wrong.

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:410: if (!url.SchemeIs(chrome::kHttpScheme)
&& !url.SchemeIs(chrome::kHttpsScheme))
What about FTP?  Whenever you find yourself listing schemes like this, you've
most likely got a bug.

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:426: if (url.GetOrigin() !=
GURL(opener->document().url()).GetOrigin())
This isn't the right way to do an origin comparison.  You need to use the
WebSecurityOrigin class to do origin comparisons.

[abarth-chromium, 2012-05-21 20:45:00]

Also, the function name doesn't really match what the function does because the
function cares about form submission but the name of the function doesn't give
any hint that that's the case.

[abarth-chromium, 2012-05-21 20:45:01]

Also, the function name doesn't really match what the function does because the
function cares about form submission but the name of the function doesn't give
any hint that that's the case.

[abarth-chromium, 2012-05-21 20:57:44]

Sorry, I didn't mean to sound negative.  I see that you're just moving this code
around.  The best thing to do here is probably to move forward with this CL if
you're happy with it and then write another CL to make this code correct.

[Mihai Parparita -not on Chrome, 2012-05-21 21:17:28]

On Mon, May 21, 2012 at 1:57 PM, <abarth@chromium.org> wrote:

> Sorry, I didn't mean to sound negative.  I see that you're just moving
> this code
> around.  The best thing to do here is probably to move forward with this
> CL if
> you're happy with it and then write another CL to make this code correct.
>

I didn't write IsNonLocalTopLevelNavigation, I was just moving it around,
since I didn't see any reason for it to be a function method. I'm not 100%
sure of its intended use (based on http://crrev.com/43276 and
http://crrev.com/46721, it looks like ChromeFrame?).

I can leave it alone if you think that's best.

Mihai

[abarth-chromium, 2012-05-21 21:27:07]

> I can leave it alone if you think that's best.

I don't have strong opinions.  It's probably better for someone else to review
your CL.  It's worrisome to me that there's a bunch of wrong code in this file,
but that's not the fault of your CL.

[Mihai Parparita -not on Chrome, 2012-05-21 21:30:18]

Darin: back to you for general contents/ review. Scary origin checks are gone.

[darin (slow to review), 2012-05-21 22:09:53]

I realize you're just moving code, but here's some nits for the existing code...

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:399: WebKit::WebFrame* frame,
nit: should not need the WebKit:: prefixes due to 'using WebKit::WebFoo" at the
top of the file.

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:425: } else {
nit: no need for "else" after "return"

[Mihai Parparita -not on Chrome, 2012-05-21 22:13:00]

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:399: WebKit::WebFrame* frame,
On 2012/05/21 22:09:54, darin wrote:
> nit: should not need the WebKit:: prefixes due to 'using WebKit::WebFoo" at
the
> top of the file.

Done.

http://codereview.chromium.org/10387074/diff/9002/content/renderer/render_vie...
content/renderer/render_view_impl.cc:425: } else {
On 2012/05/21 22:09:54, darin wrote:
> nit: no need for "else" after "return"

Done.

[darin (slow to review), 2012-05-21 22:25:47]

LGTM

[commit-bot: I haz the power, 2012-05-21 22:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10387074/23001

[commit-bot: I haz the power, 2012-05-22 00:26:57]

Try job failure for 10387074-23001 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 00:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10387074/23001

[commit-bot: I haz the power, 2012-05-22 08:20:15]

Try job failure for 10387074-23001 (retry) (retry) on win_rel for steps
"base_unittests, sync_unit_tests" (clobber build).
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 15:26:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10387074/23001

[commit-bot: I haz the power, 2012-05-22 15:55:46]

Try job failure for 10387074-23001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-22 17:01:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10387074/23001

[commit-bot: I haz the power, 2012-05-22 18:18:24]

Try job failure for 10387074-23001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...