carlosil@ -- do you know who should own this?

Might be one for Chris, but I'll take a look since he's out, so I'll keep it for now.

FWIW, this only happens if you go to the exact same URL, and only from a Bookmark. I can't imagine how this is exploitable.

Agreed, probably not much to exploit in this one, but we can keep it as a security low, since it does cause the warning triangle to turn into the circled i if reproduced on a site with broken HTTPS (say https://expired.badssl.com/#anyhash). 

From a first look seems like is_cryptographic_with_certificate is being set to false here (https://cs.chromium.org/chromium/src/components/security_state/core/security_state.cc?rcl=6eeb35a312825278c902bcd0b3a4e9485f5621bd&l=132) on the second bookmark loads for that URL (whereas it gets set to true on the first load, or for sites without a fragment), still digging as to why that is happening.

It looks like the issue is that NavigationHandleImpl::WillProcessResponse is never called for the broken case (since it is handled as a same page link with a fragment, so no request happens), I think the best way to fix this would be to treat it just as any other fragment link and keep the previous security state.

Seems the issue is here https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?rcl=b8febb05dba8d94b3b1bc6b63b7f03d26a6e3c53&l=1456, when RendererDidNavigateToSamePage gets called for this type of navigation, it overwrites the current SSLStatus with a new one created from the new NavigationHandle. A fix would be to add a check for !is_same_document before overwriting the SSLInfo (a similar check is done in RendererDidNavigateToExistingPage).

However it is not very clear to me why the replacement is needed at all for cases that call RendererDidNavigateToSamePage, and it seems the test that was added to check for this case (SameDocumentHasSSLState) now passes even with the line removed, in fact, the case the test checks for is now handled as a RendererDidNavigateToExistingPage. Adding jam@ and nasko@ for thoughts since they were on the CL where the SSLStatus update was added to RendererDidNavigateToSamePage (crrev.com/2299843002), depending on whether there are still cases I'm missing that rely on that update or not, the fix would be either to remove that line, or to add a check for is_same_document.

Pinging jam@ and nasko@ again for their thoughts.

Since, as carlosil@ notes,if a user presses enter in the omnibox and the server redirects, this is now handled as a new page navigation (not same page), it seems safe to remove the SSLStatus overwrite, uless there are other cases that jam@ or nasko@ know of.

Some debugging confirms that we would need to check |handle->IsSameDocument()| and only skip the SSLStatus overwrite for navigations that are same document (SAME_PAGE includes reloads via re-entering the same URL in the omnibox, for example, which _should_ trigger the overwrite).

It's interesting that bookmarks like this hit this navigation path though, while in-page fragment navigations do not. But handling this as a special case here seems okay to me, and should fix the test failure issues.

The SAME_PAGE classification is actually very confusing and we have been wanting to remove it for a while (issue 536102). What it means in reality is "user hit enter in the omnibox", which does *not* mean that the navigation will result in the same URL being navigated to. A simple example of that is hitting enter on a page that requires authentication, for which in the meantime the auth credential has expired. The resulting navigation will be redirected by the server to the authentication landing page, which is very much likely not the same URL as the page that the user hit enter on.

This behavior is why SAME_PAGE and EXISTING_PAGE are different and one cannot assume state will be preserved in the case of SAME_PAGE.

That said, what comment 12 claims is different and I'm not certain that it is the case. Do we have a repro that we have confirmed that a server redirect on pressing enter will indeed result in a NEW_PAGE classification type?

Thanks nasko@ for the additional info on the SAME_PAGE classification.

> Do we have a repro that we have confirmed that a server redirect on pressing enter will indeed result in a NEW_PAGE classification type?

Nope I don't think we do (or, I don't think it's relevant for this bug). For this bug, I think we're fine just handling the special case where it is classified as a SAME_PAGE navigation *and* it is also a same-document navigation.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/531e3d905a2d6335f67d17cf816a8b4eebcf3df0

commit 531e3d905a2d6335f67d17cf816a8b4eebcf3df0
Author: Joe DeBlasio <jdeblasio@chromium.org>
Date: Tue Nov 06 06:26:12 2018

Prevent overwriting SSLStatus on non-network navigations.

This CL removes a now-erroneous overwrite of a valid SSLStatus when
a user performs a same-document navigation that does not result in a
network request.

Bug:  877618 
Change-Id: I495b9e43fe11d2dc6650e7f2a4d776d5cf804c5d
Reviewed-on: https://chromium-review.googlesource.com/c/1311126
Commit-Queue: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605613}
[modify] https://crrev.com/531e3d905a2d6335f67d17cf816a8b4eebcf3df0/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/531e3d905a2d6335f67d17cf816a8b4eebcf3df0/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/531e3d905a2d6335f67d17cf816a8b4eebcf3df0/content/browser/frame_host/navigation_controller_impl.h

Hi - the Chrome VRP panel determined this isn't a security bug, so declined to reward. But many thanks for the report all the same :-)