But (ignoring  Issue 712482 ) isn't it accurate?

I think it would be concerning not to see "Secure" if that's the standard treatment for all secure URLs.

Ignoring 712482 is a pretty big if. According to  bug 712482 , the contents of a view-source URL cannot be trusted, so I don't see a reason to show the secure badge.

Alright, now that I understand how bad Issue 247151 is, I agree that an appropriate short-term fix is to avoid showing security indicators for view-source.

Besides the security problem, the omnibox just looks weird in this state. It would be great if someone could pick this up. I think the steps to fix this are:

1.) Add an |is_internal_page| boolean to VisibleSecurityState in components/security_state/core/security_state.h.*

2.) Make GetSecurityLevelForRequest() in security_state.cc return NONE when |is_internal_page| is true (and skip over setting the rest of the connection security stuff).

3.) In components/security_state/content/content_utils.cc, set |is_internal_page| to true in GetVisibleSecurityState() when entry->GetVirtualURL() matches the checks in [1]. For bonus points, we could carry |is_internal_page| over to SecurityInfo in security_state::GetSecurityInfo() and use that to do the check in [1] instead of duplicating the URL checks.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/page_info/page_info_bubble_view.cc?type=cs&l=355

Note: this will make view-source pages fall into the weird DevTools area of issue 543864, where we will show "This page is not secure" for various internal pages, which is misleading. However, I think it leaves us better off than where we are now; I'd rather have weirdness in DevTools than in the omnibox, plus we can fix view-source DevTools weirdness as part of fixing issue 543864.

Instead of excluding internal pages, can we display secure only if the scheme is https? (blacklisting vs whitelisting) Do we show the badge for any other scheme?

Re #6: that might work, but we don't currently look at the virtual URL for any security state calculations. If we were looking at the virtual URL, I think we would already be doing the right thing [1]. I'm a little afraid of defaulting to look at GetVirtualURL() instead of GetURL() -- what if there are cases where the URL has some important security properties that get "hidden" by the virtual URL?

So if we don't default to using GetVirtualURL(), then it amounts to the same thing; we'd be listing a few exceptional cases where we should use the virtual URL. Or we can default to using GetVirtualURL() which might be safe but I can't convince myself. WDYT?

[1] https://cs.chromium.org/chromium/src/components/security_state/core/security_state.cc?q=GetSecurityLevelForRequest+package:%5Echromium$&l=130

I don't know all the important differences between virtual and normal URLs so this isn't a fully informed comment, but I think virtual URL is used for display purposes. Since the badge should accompany the displayed URL and not the underlying implementation, it sounds to me that using the virtual URL is more appropriate, no? Probably a question for navigation team though.

Yeah, I'm not sure. I see what you're saying about the security UI matching the displayed URL, but I also can imagine cases where we want to reflect the actual URL that was loaded. Like, if a page is loaded with certificate errors but overridden with a non-https virtual URL, I think we'd end up not showing the Not Secure state, which could be bad. (I think this is okay in the view-source case, but not sure that it's going to always be true for any virtual URL.)

Basically, I could imagine both approaches going terribly wrong.

> if a page is loaded with certificate errors but overridden with a non-https virtual URL, I think we'd end up not showing the Not Secure state, which could be bad.

Is this something we do today? (I'm not familiar with this stuff)

As a contrived solution: what about using both, and displaying the most conservative result? E.g. if GetURL() result says "Secure" and GetVirtualURL() results says "Not secure", display "Not Secure" (or None in the view-source case).

> Is this something we do today? (I'm not familiar with this stuff)

Today we don't take the virtual URL into account at all. But if we were to simply change the URL that we look at from entry->GetURL() to entry->GetVirtualURL(), then we would end up in the situation I described (because we only take cert errors into account if the scheme of the URL is cryptographic [1]).

I think your idea of using both URLs could work. It would mean that the view-source omnibox would look a little funny on a page with cert errors (it would have the Dangerous chip and the https in view-source:https://... would be red and cross out), but that's probably enough of an edge case that it doesn't matter.

[1] https://cs.chromium.org/chromium/src/components/security_state/core/security_state.cc?l=107

+creis: do you know if there is any security-relevant information that would be hidden or elided by GetVirtualURL()? And could you comment on estark's comment c#9?

The context is that the "internal" URLs like the view-source ones get a "Secure" marking, when they probably shouldn't. They get this marking because the URL passed to the security state code is the raw URL, without the view-source prefix that was actually navigated to.

> It would mean that the view-source omnibox would look a little funny on 
> a page with cert errors (it would have the Dangerous chip and the https
> in view-source:https://... would be red and cross out)

That's what already happens today, right? We always show the security state of the page as if View-Source weren't involved (screenshot attached). 

In this issue, I think we're concerned about showing any "positive" states because of Issue 247151 (which I don't really understand). 

So we could approach this by simply treating the security level of View-Source:// as NONE, but that's tricky because 

   1) We don't have that virtual URL inside GetSecurityLevelForRequest, and 
   2) Even if we did have that information, we might still want to show "negative" states if the underlying connection had a cert error or whatever.

We could do something like store VirtualURLScheme on the VisibleSecurityState, and have code in the middle of GetSecurityLevelForRequest (after the "badness" detection checks) that says 

  if (visible_security_state.VirtualURLScheme == kViewSourceScheme)
    return NONE;

So that we never show a "positive" state, but otherwise all negative states flow through. We'd have to decide what to do about cases where the user has MarkNonSecureAs-Dangerous set, which would be a corner-case-of-this-corner-case.

(I'm not really sure this is a P2 though, insofar as the user navigating to view-source is rare, and in most cases what we display isn't unreasonable).

Deleted: ViewSource.png 105 KB

	ViewSource.png 105 KB View Download

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

commit b905976462296f40ab1732ff41428f7f6530d056
Author: Carlos IL <carlosil@chromium.org>
Date: Mon Nov 20 19:23:39 2017

Omnibox no longer shows secure for view-source urls

Added is_view_source boolean to VisibleSecurityState.
GetSecurityLevelForRequest now checks that flag and returns 'NONE' for
pages that would return SECURE or EV_SECURE without the flag.
Added tests that validate the new behavior

Bug:  712482 
Change-Id: Ic23c20ffdb92262987dae2119148932cb9d187a6
Reviewed-on: https://chromium-review.googlesource.com/773218
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517886}
[modify] https://crrev.com/b905976462296f40ab1732ff41428f7f6530d056/components/security_state/content/content_utils.cc
[modify] https://crrev.com/b905976462296f40ab1732ff41428f7f6530d056/components/security_state/core/security_state.cc
[modify] https://crrev.com/b905976462296f40ab1732ff41428f7f6530d056/components/security_state/core/security_state.h
[modify] https://crrev.com/b905976462296f40ab1732ff41428f7f6530d056/components/security_state/core/security_state_unittest.cc