Don't show URL for pending new navigations initiated by the renderer.

content/browser/tab_contents/navigation_controller.cc

 // Copyright (c) 2011 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/tab_contents/navigation_controller.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ skipping 272 matching lines @@
   if (transient_entry_index_ != -1)
     return entries_[transient_entry_index_].get();
   if (pending_entry_)
     return pending_entry_;
   return GetLastCommittedEntry();
 }
 
 NavigationEntry* NavigationController::GetVisibleEntry() const {
   if (transient_entry_index_ != -1)
     return entries_[transient_entry_index_].get();
-  // Only return pending_entry for new navigations.
-  if (pending_entry_ && pending_entry_->page_id() == -1)
+  // Only return pending_entry for new browser-initiated navigations, and not
+  // during link clicks (which might allow URL spoof attacks).
+  // Ideally we would also show the pending entry's URL if there is no last
+  // committed entry (e.g., a link opening in a new tab), but the opener window
+  // can insert content into that about:blank page while the pending URL loads.
+  if (pending_entry_ &&
+      pending_entry_->page_id() == -1 &&
+      pending_entry_->transition_type() != PageTransition::LINK)

[brettw, 2011/10/11 18:11:29]

I'm kind of freaked out by this condition. Is this complicated code really the
way that we detect browser-initiated navigations? Is there a way to make this
more explicit? If this is the best way, can we add another function
IsPendingEntryBrowserInitiated or something?

[Charlie Reis, 2011/10/11 19:40:42]

There's two conditions here:

1) page_id == -1 means it's a new navigation, not a history navigation.  That's
to prevent URL spoofs using back/forward, as in http://crbug.com/89564.

2) transition_type != LINK means it's browser-initiated.  I think it's a good
idea to add that as a helper function to make it more explicit, but it won't
simplify this clause much.  (The helper should also probably check for
FORM_SUBMIT, but I can't think of a way to have a pending_entry with
FORM_SUBMIT.)

It's a pain to add state to NavigationEntry (given session restore), so I'm not
sure it's worth having an explicit field if we can infer it from PageTransition.

Code and comments updated-- let me know if you think of a better approach.

[brettw, 2011/10/11 19:51:09]

I don't think we ever expected page transition types to be used to make security
decisions. It's one of those things where we did as good a job as we could for
making history / new tab page work well, but as far as making the guarantee that
link == renderer initiated makes me nervous.

[Charlie Reis, 2011/10/11 20:14:29]

Well, if we wanted to approach this another way, we could stamp an extra bit of
state on the NavigationEntry if it arrives via RenderViewHost::OnMsgOpenURL. 
That would avoid relying on any mistaken uses of PageTransition::LINK.

If I took that approach, do you see any reason it needs to be persisted in
session history?  I imagine not, and that it's even safe to clear the flag once
the navigation commits, since future navigations to the entry might take a
different path (e.g., browser-initiated back/forward).

     return pending_entry_;
   return GetLastCommittedEntry();
 }
 
 int NavigationController::GetCurrentEntryIndex() const {
   if (transient_entry_index_ != -1)
     return transient_entry_index_;
   if (pending_entry_index_ != -1)
     return pending_entry_index_;
   return last_committed_entry_index_;
@@ skipping 889 matching lines @@
   size_t insert_index = 0;
   for (int i = 0; i < max_index; i++) {
     // When cloning a tab, copy all entries except interstitial pages
     if (source.entries_[i].get()->page_type() != INTERSTITIAL_PAGE) {
       entries_.insert(entries_.begin() + insert_index++,
                       linked_ptr<NavigationEntry>(
                           new NavigationEntry(*source.entries_[i])));
     }
   }
 }