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

content/browser/tab_contents/navigation_controller.cc

Index: content/browser/tab_contents/navigation_controller.cc
diff --git a/content/browser/tab_contents/navigation_controller.cc b/content/browser/tab_contents/navigation_controller.cc
index 4483d669dafe5ed787cf32021101ebab46529135..bc29a359472050ae9abb0814350138716fc873b3 100644
--- a/content/browser/tab_contents/navigation_controller.cc
+++ b/content/browser/tab_contents/navigation_controller.cc
@@ -290,8 +290,14 @@ NavigationEntry* NavigationController::GetActiveEntry() const {
 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();
 }