Only disallow top-level navigations in platform apps

content/renderer/render_view_impl.cc

Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index e85257490bd2194fbdaea628150207a0205cba87..a937429e2fc489f7034710be5d6e451376117796 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -2362,14 +2362,18 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
     }
   }
 
-  // If the browser is interested, then give it a chance to look at top level
-  // navigations.
+  // If the browser is interested, then give it a chance to look at the request.
   if (is_content_initiated) {
-    bool browser_handles_top_level_requests =
+    bool browser_handles_request =
         renderer_preferences_.browser_handles_top_level_requests &&
         IsNonLocalTopLevelNavigation(url, frame, type);
-    if (browser_handles_top_level_requests ||
-        renderer_preferences_.browser_handles_all_requests) {
+    if (!browser_handles_request) {
+      browser_handles_request = renderer_preferences_.
+              browser_handles_all_top_level_or_non_local_requests &&
+          IsNonLocalOrTopLevelNavigation(url, frame);
+    }
+
+    if (browser_handles_request) {
       // Reset these counters as the RenderView could be reused for the next
       // navigation.
       page_id_ = -1;
@@ -5211,8 +5215,21 @@ WebKit::WebUserMediaClient* RenderViewImpl::userMediaClient() {
   return media_stream_impl_;
 }
 
+bool RenderViewImpl::IsNonLocalOrTopLevelNavigation(

[darin (slow to review), 2012/05/12 00:00:51]

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.

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

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

How about IsRemoteOrTopLevelNavigation?

[darin (slow to review), 2012/05/15 23:58:44]

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.

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

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.

+    const GURL& url, WebKit::WebFrame* frame) const {
+  if (frame->parent() == NULL)
+    return true;
+
+  // data: URLs don't involve remote content either, thus are considered local.
+  if (url.SchemeIs(chrome::kDataScheme))
+    return false;
+
+  return url.GetOrigin() != GURL(frame->top()->document().url()).GetOrigin();

[darin (slow to review), 2012/05/12 00:00:51]

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]

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:44]

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]

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.

+}
+
 bool RenderViewImpl::IsNonLocalTopLevelNavigation(
-    const GURL& url, WebKit::WebFrame* frame, WebKit::WebNavigationType type) {
+    const GURL& url, WebKit::WebFrame* frame, WebKit::WebNavigationType type)
+    const {
   // Must be a top level frame.
   if (frame->parent() != NULL)
     return false;