Fix web accessible resource checks in ShouldAllowOpenURL

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 719 matching lines @@
   }
 
   SiteInstance* current_site_instance = render_frame_host->GetSiteInstance();
 
   // TODO(creis): Pass the redirect_chain into this method to support client
   // redirects.  http://crbug.com/311721.
   std::vector<GURL> redirect_chain;
 
   GURL dest_url(url);
   if (!GetContentClient()->browser()->ShouldAllowOpenURL(
           current_site_instance, url)) {

[Charlie Reis, 2016/11/01 18:12:19]

Sanity check: Are we ok with sending the current_site_instance here?  Maybe we
should add a comment about why it differs from RequestTransferURL if so.

[alexmos, 2016/11/01 22:04:22]

I added a comment - I think |current_site_instance| is still ok here.  We'd get
here if one of the frames in the current frame's SiteInstance navigates the
current frame to a new URL, right?  It seems reasonable to do the WAR checks
against the RFH's current SiteInstance, as that's where the navigation was
initiated.

It's confusing to have both |source_site_instance| and |current_site_instance|
though.  This is called by only two places: RFHI::OnOpenURL, which always
computes |source_site_instance| as the current RFH's SiteInstance (i.e., equal
to |current_site_instance|), and in a test, which passes nullptr for
|source_site_instance|.  So really there's no difference between the two
SiteInstances, and seems like we could get rid of the |source_site_instance|
that's passed here altogether and just use |current_site_instance| here and for
OpenURLParams.source_site_instance.  I'm happy to do that here while I'm at it,
or in a followup CL.

[Charlie Reis, 2016/11/01 23:16:27]

Interesting-- yes.  While it could have been a different frame initiating the
navigation, it necessarily was in the same SiteInstance if it came through
RequestOpenURL rather than RequestTransferURL.
Are we not considering changing RenderFrameProxyHost back to using
RequestOpenURL anymore, now that we found an alternate fix for
https://crbug.com/647772?  (That may be one reason source_site_instance was
useful there.)

Anyway, I'm fine with removing it (either here or in a followup) if nothing
needs it at the moment.

[alexmos, 2016/11/01 23:36:59]

Yes, after also checking with lukasza@, I don't think we have any reasons to
convert RFPH to use RequestOpenURL at the moment.  So I think it should be fine
to remove source_site_instance; I'll put together a small followup for this.

     dest_url = GURL(url::kAboutBlankURL);

[Charlie Reis, 2016/11/01 18:12:19]

Should we be returning here, as below?

[alexmos, 2016/11/01 22:04:22]

Good question.  I think this is ok as-is, since a request hasn't been made yet
when we're here, and when it's made below, it will always use |dest_url|.  So
there's no danger that we'll hit the |is_transfer_to_same| path and ignore the
rewritten |dest_url| like we do for the other case.  That said, it might be
better to also return here for consistency.  That may need some more test
updates though (I haven't checked), so I'd vote for trying this in a follow-up
CL.

[Charlie Reis, 2016/11/01 23:16:27]

Sure, cleaning it up separately sounds fine.

   }
 
   int frame_tree_node_id = -1;
 
   // Send the navigation to the current FrameTreeNode if it's destined for a
   // subframe in the current tab.  We'll assume it's for the main frame
   // (possibly of a new or different WebContents) otherwise.
   if (SiteIsolationPolicy::UseSubframeNavigationEntries() &&
       disposition == WindowOpenDisposition::CURRENT_TAB &&
       render_frame_host->GetParent()) {
@@ skipping 63 matching lines @@
 
   // Allow the delegate to cancel the transfer.
   if (!delegate_->ShouldTransferNavigation(
           render_frame_host->frame_tree_node()->IsMainFrame()))
     return;
 
   GURL dest_url(url);
   Referrer referrer_to_use(referrer);
   FrameTreeNode* node = render_frame_host->frame_tree_node();
   SiteInstance* current_site_instance = render_frame_host->GetSiteInstance();
-  if (!GetContentClient()->browser()->ShouldAllowOpenURL(current_site_instance,
-                                                         url)) {
-    dest_url = GURL(url::kAboutBlankURL);
+  // It is important to pass in the source_site_instance if it is available
+  // (such as when navigating a proxy).  See https://crbug.com/656752.
+  if (!GetContentClient()->browser()->ShouldAllowOpenURL(
+          source_site_instance ? source_site_instance : current_site_instance,
+          url)) {
+    // It is important to return here, rather than rewrite the dest_url to
+    // about:blank.  The latter won't actually have any effect when
+    // transferring, as NavigateToEntry will think that the transfer is to the
+    // same RFH that started the navigation and let the existing navigation
+    // (for the disallowed URL) proceed.
+    return;
   }
 
   // TODO(creis): Determine if this transfer started as a browser-initiated
   // navigation.  See https://crbug.com/495161.
   bool is_renderer_initiated = true;
   if (render_frame_host->web_ui()) {
     // Web UI pages sometimes want to override the page transition type for
     // link clicks (e.g., so the new tab page can specify AUTO_BOOKMARK for
     // automatically generated suggestions).  We don't override other types
     // like TYPED because they have different implications (e.g., autocomplete).
@@ skipping 423 matching lines @@
     if (navigation_handle)
       navigation_handle->update_entry_id_for_transfer(entry->GetUniqueID());
 
     controller_->SetPendingEntry(std::move(entry));
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content