Fix incorrect SSL state being shown for client redirects.

content/browser/frame_host/navigation_controller_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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 1160 matching lines @@
 void NavigationControllerImpl::RendererDidNavigateToExistingPage(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
   // We should only get here for main frame navigations.
   DCHECK(!rfh->GetParent());
 
   // TODO(creis): Classify location.replace as NEW_PAGE instead of EXISTING_PAGE
   // in https://crbug.com/596707.
 
   NavigationEntryImpl* entry;
+  NavigationHandleImpl* handle = rfh->navigation_handle();
   if (params.intended_as_new_entry) {
     // This was intended as a new entry but the pending entry was lost in the
     // meanwhile and no new page was created. We are stuck at the last committed
     // entry.
     entry = GetLastCommittedEntry();
   } else if (params.nav_entry_id) {
     // This is a browser-initiated navigation (back/forward/reload).
     entry = GetEntryWithUniqueID(params.nav_entry_id);
 
     // Needed for the restore case, where the serialized NavigationEntry doesn't
     // have the SSL state.
-    NavigationHandleImpl* handle = rfh->navigation_handle();
     entry->GetSSL() = handle->ssl_status();
   } else {
     // This is renderer-initiated. The only kinds of renderer-initated
     // navigations that are EXISTING_PAGE are reloads and location.replace,
     // which land us at the last committed entry.
     entry = GetLastCommittedEntry();
+
+    // Until https://crbug.com/596707 is fixed (see comment at top of method),
+    // we will get here for location.replace for both same origin and different
+    // origin. For the former, there won't be a network request that gives the
+    // SSLStatus so we don't want to change this. For the latter, there will be
+    // a request and so we need to update the SSLStatus.
+    if (params.url.GetOrigin() != entry->GetURL().GetOrigin())

[felt, 2016/09/06 20:09:55]

Oof. This fixes the obvious common case but I am concerned it does not really
fix the whole issue (corner cases).

The tricky thing here is that navigating within the same origin could
theoretically still change the state, right? For example, if the new page has
mixed content or if there are malicious subresources on the new page.

[jam, 2016/09/06 20:20:37]

If these events occur (mixed content or malicious subresources), the relevant
notifications will still be fired from the renderer and the SSLStatus will be
updated. They'll fire after the did commit provisional load IPC which is what
calls this method.

[jam, 2016/09/06 20:28:54]

ok thinking some more about this, one issue which I think you bring up is:
-user visits https://foo.com/mixedcontent
-the SSLStatus is later updated because of mixed content from that page
-page uses window.location.replace to go to https://foo.com/goodpage
-we don't create a new SSLStatus and use the old bad one

so it seems we would end up in a situation where a bad icon is used when it
should be good. the other way around shouldn't be possible (because of my
earlier reply). i'll look some more into differentiating this case.

+      entry->GetSSL() = handle->ssl_status();
   }
   DCHECK(entry);
 
   // The URL may have changed due to redirects.
   entry->set_page_type(params.url_is_unreachable ? PAGE_TYPE_ERROR
                                                  : PAGE_TYPE_NORMAL);
   entry->SetURL(params.url);
   entry->SetReferrer(params.referrer);
   if (entry->update_virtual_url_with_url())
     UpdateVirtualURLToURL(entry, params.url);
@@ skipping 902 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content