HTTPS deployment is starting to get some momentum, having recently crossed 50% [1]. We should start preparing for a shift toward marking non-secure sites as insecure (as opposed to marking secure sites as secure). As a first step, let's add a negative indicator for all non-secure sites, gated by a pref that's off by default. [1] https://mzl.la/2e3RQ8I

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites Nice quick work Richard! This looks good, but we need a browser peer to review as well. So r? to paolo. Also, we should add a mochitest for this (or potentially modify an existing one), to make sure we get the right UI on an HTTP page when the pref is enabled. You may want to turn the pref on and push to try to see what other tests need an update (to pushprefenv the pref to false) - i.e. tests that check the identity block and expect to see no icon for HTTP pages. You don't have to do this second step now; we could do it when we decide to enable it on Nightly.

(In reply to Tanvi Vyas [:tanvi] from comment #2) > Comment on attachment 8801549 [details] > Bug 1310447 - Add a pref to display a negative indicator in the URL bar for > non-secure sites > > Nice quick work Richard! This looks good, but we need a browser peer to > review as well. So r? to paolo. > > Also, we should add a mochitest for this (or potentially modify an existing > one), to make sure we get the right UI on an HTTP page when the pref is > enabled. Good point. Updated the patch to add mochitest coverage for this. Test code was bigger than the code itself :) > You may want to turn the pref on and push to try to see what other > tests need an update (to pushprefenv the pref to false) - i.e. tests that > check the identity block and expect to see no icon for HTTP pages. You > don't have to do this second step now; we could do it when we decide to > enable it on Nightly. Yeah, let's do this if/when we make that change.

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/86250/#review85562 It's good to see this short patch as a statement of intent, but to me there's no point in landing a patch that, if enabled, might cause multiple regressions with the other connection states, so it's not useful or is misleading for people who want to test this feature. Existing regression tests are useful to verify that we don't break anything, and also that the code does what it says. ::: browser/app/profile/firefox.js:1232 (Diff revision 2) > +// Show degraded UI for http pages; disabled for now > +pref("security.non-secure-warning.ui.enabled", false); Looks like nearby preferences all use underscores, also in code we have "connection-not-secure" or the "insecure" concept for logins, but we don't use the term "non-secure". Suggested names: security.insecure_connection_icon.enabled security.not_secure_connection_icon.enabled ::: browser/base/content/browser.js:7021 (Diff revision 2) > + let warnOnNonSecure = Services.prefs.getBoolPref("security.non-secure-warning.ui.enabled") > + if (this._uriHasHost && warnOnNonSecure) { > + this._identityBox.className = "notSecure"; > + } I'd expect this to match the logic in refreshIdentityPopup(), even if obtained in a different way. The insecure login forms flag already does some checks so that's why it works here, might not be the same for the general insecure connection case. So, does this code match the cases where we show the insecure connection text in red in the Control Center? Actually, there are specific tests that check the text and at the same time the styling to determine which actual icons are visible. I think this is what Tanvi already mentioned. We should definitely update those tests to check that the new styling is applied, conditionally on the preference. Running them with the preference set would ensure that this change does not regress other cases. Also, probably adding a class rather than replacing unknownIdentity might be better for compatibility and consistency with cases like insecureLoginForms, not totally sure though. It will probably become clearer when updating the tests. Would unknownIdentity apply to local sites only? Will it make sense to rename that class to something clearer? ::: browser/base/content/test/general/browser_bug590206.js:85 (Diff revision 2) > is(getIdentityMode(), "verifiedDomain", "Identity should be verified"); > > gBrowser.removeTab(newTab); > }); > > +add_task(function* test_https() { The test name should be changed. ::: browser/base/content/test/general/browser_bug590206.js:88 (Diff revision 2) > + let prefName = "security.non-secure-warning.ui.enabled" > + let prefValue = Services.prefs.getBoolPref(prefName); > + Services.prefs.setBoolPref(prefName, true); This should use pushPrefEnv/popPrefEnv. ::: browser/base/content/test/general/browser_bug590206.js:93 (Diff revision 2) > + let prefName = "security.non-secure-warning.ui.enabled" > + let prefValue = Services.prefs.getBoolPref(prefName); > + Services.prefs.setBoolPref(prefName, true); > + > + let newTab = yield loadNewTab("http://example.com/" + DUMMY); > + is(getIdentityMode(), "notSecure", "Identity should be verified"); "should be not secure" ::: browser/base/content/test/general/browser_bug590206.js:96 (Diff revision 2) > + > + let newTab = yield loadNewTab("http://example.com/" + DUMMY); > + is(getIdentityMode(), "notSecure", "Identity should be verified"); > + > + gBrowser.selectedTab = oldTab; > + is(getIdentityMode(), "unknownIdentity", "Identity should be verified"); "should be unknown"

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/86250/#review85628

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/86250/#review85562 > I'd expect this to match the logic in refreshIdentityPopup(), even if obtained in a different way. The insecure login forms flag already does some checks so that's why it works here, might not be the same for the general insecure connection case. > > So, does this code match the cases where we show the insecure connection text in red in the Control Center? > > Actually, there are specific tests that check the text and at the same time the styling to determine which actual icons are visible. I think this is what Tanvi already mentioned. We should definitely update those tests to check that the new styling is applied, conditionally on the preference. Running them with the preference set would ensure that this change does not regress other cases. > > Also, probably adding a class rather than replacing unknownIdentity might be better for compatibility and consistency with cases like insecureLoginForms, not totally sure though. It will probably become clearer when updating the tests. Would unknownIdentity apply to local sites only? Will it make sense to rename that class to something clearer? > So, does this code match the cases where we show the insecure connection text in red in the Control Center? It should, yes. I will trigger a try run with the pref set and see if what changes. > Also, probably adding a class rather than replacing unknownIdentity might be better for compatibility and consistency with cases like insecureLoginForms, not totally sure though. It will probably become clearer when updating the tests. Would unknownIdentity apply to local sites only? Will it make sense to rename that class to something clearer? I actually think replacing the base class is a better option here. The "unknownIdentity" class should be reserved for things where there's actually not a host, vs. where there is a host, but it's unverified. In the long run, we should use the notSecure class as the base class for all states that have a host and are not secure; we just have to switch for now. I would propose that we split out the no-hostname case as a separate case in the if/else tree, then just use the pref to switch between the base classes for the (host && !secure) case.

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/86250/#review86014 OK, waiting for the tryserver build to determine what tests break. We should already add conditionals to make these tests pass when the preference is set, unless it turns out the changes would be too many. ::: browser/base/content/browser.js:7018 (Diff revision 3) > - } else { > + } else if (!this._uriHasHost) { > this._identityBox.className = "unknownIdentity"; I wonder if this would break the insecure login forms indication with URIs with no host that have an "http:" codebase principal. Tanvi, what do you think? ::: browser/base/content/browser.js:7021 (Diff revision 3) > [this.getIdentityData().caOrg]); > } > - } else { > + } else if (!this._uriHasHost) { > this._identityBox.className = "unknownIdentity"; > + } else { > + let warnOnNonSecure = Services.prefs.getBoolPref("security.not_secure_connection_icon.enabled") Typo, semicolon ::: browser/base/content/test/general/browser_bug590206.js:89 (Diff revision 3) > + yield new Promise(r => SpecialPowers.pushPrefEnv({set: > + [[prefName, true]]}, r)); pushPrefEnv/popPrefEnv now return a Promise so this can be simplified.

Tanvi, see the question in comment 9. In general I'd appreciate if you can share any thoughts on the patch or the logic we should use here.

And there is the question of what we want to show for "http://localhost/". We don't currently show the insecure login forms indication, but if we show a negative indication for all pages over "http:" we might also want to do that for "localhost". We can refine this case in a follow-up anyways, as long as this patch does not regress anything when the preference is disabled.

(In reply to :Paolo Amadini from comment #11) > And there is the question of what we want to show for "http://localhost/". > We don't currently show the insecure login forms indication, but if we show > a negative indication for all pages over "http:" we might also want to do > that for "localhost". We can refine this case in a follow-up anyways, as > long as this patch does not regress anything when the preference is disabled. The way we treat localhost is still inconsistent in the codebase. I'm okay going either way here... marking http://localhost (with or without a login form) as insecure with a lock with a strike through, or as unknownidentity with no icon. We shouldn't treat it as secure with a green lock.

(In reply to :Paolo Amadini from comment #9) > ::: browser/base/content/browser.js:7018 > (Diff revision 3) > > - } else { > > + } else if (!this._uriHasHost) { > > this._identityBox.className = "unknownIdentity"; > > I wonder if this would break the insecure login forms indication with URIs > with no host that have an "http:" codebase principal. > > Tanvi, what do you think? > Yes, this would, since the insecure login form check doesn't happen until line 7035 which is now in a separate else block. How would you get into a state where a URI has no host but an http: codebase principal? Are you imagining a window.open that inserts a login form and inherits the previous page's codebase principal? Besides the issues with providing a password over http://, the user would be providing a password to an unknown host! If the codebase principal is http://, we should use the negative indicator in the url bar. Arguably, we should use the negative indicator in the url bar even if the codebase principal is https:// and the page contains a password field.

Comment on attachment 8801549 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/86248/#review94346 ::: browser/base/content/browser.js:7035 (Diff revision 3) > this._identityBox.classList.add("mixedDisplayContent"); > } else { > this._identityBox.classList.add("weakCipher"); > } > } > if (this._hasInsecureLoginForms) { As Paolo mentioned, we have to handle this case in the !this._uriHasHost case as well.

Philipp, just flagging you to ensure you are good with the UI for this.

Could you maybe post a screen shot of what this will look like? Thanks!

Could we consider making it at least like insecure password: http://http-password.badssl.com/ Or ideally more like Chromes with the text 'not secure' too.

To start with we should just leverage the assets that we have - lock with a strikethrough. This will be off by default. We may just enable it in Nightly or use it for user testing studies (shield, heartbeat, etc). Before it rides the trains, we can figure out the right UI with new mocks for UX and perhaps a round of testing. This can be the first step bug, and we can followup in other bugs on potentially changing the lock with a strikethrough to something else. Richard, are you still working on this? Or would you like to hand it off?

Will work on fixing this patch and adding the style agreed in #18

This is a WIP patch due to there being an intermittent issue on one of the tests on my machine. The chrome test for extensions URL return 'not-secure' on the following line: is(getConnectionState(), "file", "Connection should be file"); However it seems if you copy the test_chrome twice it has the same issue, so it seems unrelated. The browser seems to still think it the page is still about:blank rather than being the mentioned URL. It looks like it might be a race condition with BrowserTestUtils.openNewForegroundTab(gBrowser,url). There appears to be one test failing: TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug435035.js | identity box has class name for mixed content - Got notSecure mixedDisplayContent, expected unknownIdentity mixedDisplayContent The eslint issue is fixed in the review patch mentioned in try.

Everything in the context of bug 1351684: I would like to ask you to let both true, but with a "#ifdef NIGHTLY_BUILD" before them, as it is also requested for bug 1353705 and bug 1353710 for the yellow triangle, so website owners can see in Nightly what they would have to expect in the future. (And the text of bug 1335970 as opt-in.)

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review138376 I haven't looked at the tests too closely, however I feel this is enough to clear the review for now. Please make all const names SCREAMING_SNAKE_CASE and end comments with a dot. ::: browser/app/profile/firefox.js:1278 (Diff revision 3) > > // Show in-content login form warning UI for insecure login fields > pref("security.insecure_field_warning.contextual.enabled", true); > > +// Show degraded UI for http pages; disabled for now > +pref("security.insecure_connection_icon.enabled", true); Shouldn't these prefs be false? ::: browser/app/profile/firefox.js:1281 (Diff revision 3) > > +// Show degraded UI for http pages; disabled for now > +pref("security.insecure_connection_icon.enabled", true); > + > +// Show degraded UI for mixed content pages and weak cipher pages; disabled for now > +pref("security.insecure_connection_icon_on_broken.enabled", true); What does on_broken mean here? I'm not sure if I would understand that pref name without context. Is there a reason for the naming? ::: browser/base/content/browser.js:7239 (Diff revision 3) > [this.getIdentityData().caOrg]); > } > - } else { > + } else if (!this._uriHasHost) { > this._identityBox.className = "unknownIdentity"; > + } else { > + let warnOnInsecure = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled"); I wonder what impact this has on performance and whether we should load this pref lazily using defineLazyPreferenceGetter. In any case, we'll soon do a rundown of a perf profile of this function and there are likely worse offenders, so I'm fine with keeping this around for now. ::: browser/base/content/browser.js:7245 (Diff revision 3) > + let className = (warnOnInsecure) ? "notSecure" : "unknownIdentity"; > + > + // For net errors we should not show notSecure as it's likely confusing > + // isNullPrincipal is set when on about:netError pages > + // We should always consider local files to be unknownIdentity rather than notSecure > + if (gBrowser.contentPrincipal.isNullPrincipal I don't think this is correct (it doesn't work for me when I test it). Also, this would whitelist HTTP pages with CSP: sandbox headers because these also get a null principal. Use gBrowser.selectedBrowser.documentURI instead. This should also not be in the "else" clause and rather in its own "else if". ::: browser/base/content/browser.js:7246 (Diff revision 3) > + > + // For net errors we should not show notSecure as it's likely confusing > + // isNullPrincipal is set when on about:netError pages > + // We should always consider local files to be unknownIdentity rather than notSecure > + if (gBrowser.contentPrincipal.isNullPrincipal > + || this._isSecureInternalUI Pretty sure this condition is impossible to hit given that we already check for this in line 7197 ::: browser/base/content/browser.js:7247 (Diff revision 3) > + // For net errors we should not show notSecure as it's likely confusing > + // isNullPrincipal is set when on about:netError pages > + // We should always consider local files to be unknownIdentity rather than notSecure > + if (gBrowser.contentPrincipal.isNullPrincipal > + || this._isSecureInternalUI > + || this._isURILoadedFromFile) { This should not be necessary because this._uriHasHost should really be false in this case. Unfortunately this._uri.host is "" in case of file:// uris. I wonder if we should change https://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/browser/base/content/browser.js#7508 to properly check for empty hosts, too. We might have to be careful about windows remote file sharing hosts. In any case please move this check to line 7236. ::: browser/base/content/browser.js:7250 (Diff revision 3) > + if (gBrowser.contentPrincipal.isNullPrincipal > + || this._isSecureInternalUI > + || this._isURILoadedFromFile) { > + className = "unknownIdentity"; > + } > + this._identityBox.className = className; This should be in an else clause of the if condition below. ::: browser/base/content/browser.js:7254 (Diff revision 3) > + } > + this._identityBox.className = className; > + > if (this._isBroken) { > + let warnOnBroken = Services.prefs.getBoolPref("security.insecure_connection_icon_on_broken.enabled"); > + this._identityBox.className = (warnOnBroken) ? "notSecure" : "unknownIdentity"; Are the parentheses necessary here? ::: browser/base/content/test/general/browser_bug590206.js:2 (Diff revision 3) > /* > * Test the identity mode UI for a variety of page types This would be a good opportunity to move this file to test/siteIdentity/ and give it a proper name :) ::: browser/base/content/test/general/browser_bug590206.js:8 (Diff revision 3) > */ > > "use strict"; > > const DUMMY = "browser/browser/base/content/test/general/dummy_page.html"; > +const insecurePref = "security.insecure_connection_icon.enabled"; INSECURE_PREF ::: browser/base/content/test/siteIdentity/head.js:164 (Diff revision 3) > is(activeBlocked, !!stateActiveBlocked, "Expected state for activeBlocked matches UI state"); > is(activeLoaded, !!stateActiveLoaded, "Expected state for activeLoaded matches UI state"); > is(passiveLoaded, !!statePassiveLoaded, "Expected state for passiveLoaded matches UI state"); > > if (stateInsecure) { > + const insecureConnectionIcon = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled"); INSECURE_CONNECTION_ICON or let, please :) ::: browser/base/content/test/siteIdentity/head.js:171 (Diff revision 3) > - // HTTP request, there should be no MCB classes for the identity box and the non secure icon > + // HTTP request, there should be no MCB classes for the identity box and the non secure icon > - // should always be visible regardless of MCB state. > + // should always be visible regardless of MCB state. > - ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > + ok(classList.contains("unknownIdentity"), "unknownIdentity on HTTP page"); > - ok(is_hidden(connectionIcon), "connection icon should be hidden"); > + ok(is_hidden(connectionIcon), "connection icon should be hidden"); > + } else { > + // HTTP request, there should be a broken padlock shown always Nit: end comments with a dot. ::: browser/base/content/test/siteIdentity/head.js:191 (Diff revision 3) > is(classList.contains("mixedDisplayContent"), passiveLoaded && !(activeLoaded || activeBlocked), > "identityBox has expected class for passiveLoaded && !(activeLoaded || activeBlocked)"); > is(classList.contains("mixedDisplayContentLoadedActiveBlocked"), passiveLoaded && activeBlocked, > "identityBox has expected class for passiveLoaded && activeBlocked"); > > + const insecureConnectionIconOnMixed = Services.prefs.getBoolPref("security.insecure_connection_icon_on_broken.enabled"); INSECURE_CONNECTION_ICON_MIXED or let

Jonathan, do you want to keep this bug or release it? This pref may help Luke with some experimentation.

Thanks the HTTP password work this is already possible with an extension: https://github.com/jonathanKingston/snap-http-padlocks (AMO rejected it for some reason that I am trying to establish, I think it is because I mention experiment). I can finish this now we have more time from finishing containers, is it a priority to Lukes work? If so we should probably talk about what he might need and if we can expose something better though an extension and what the security implications of that might be. I would like to finish it certainly, let me know if we need it for something and if there is a priority with it etc.

When creating this pref, we should also consider private browsing mode. So we either create two prefs, or make the pref an int instead of a boolean: security.not_secure_connection_icon = 0 - current situation 1 - not secure pages get the lock with the strikethrough treatment in Private Browisng Mode 2 - not secure pages get the lock with the strikethrough treatment in Regular and Private Browsing Mode OR security.not_secure_connection_icon.regularMode.enabled security.not_secure_connection_icon.privateMode.enabled (You could even take it a step further and have a mode for containers. That's probably going overboard in this bug, but we could consider future proofing when choosing the method we end up using.)

(In reply to Tanvi Vyas[:tanvi] from comment #39) > security.not_secure_connection_icon.regularMode.enabled > security.not_secure_connection_icon.privateMode.enabled We use a slightly different pattern for tracking protection: privacy.trackingprotection.enabled privacy.trackingprotection.pbmode.enabled The first one is global (i.e. includes both normal mode and private browsing), the second only controls private browsing.

+1 for: security.not_secure_connection_icon.enabled security.not_secure_connection_icon.pbmode.enabled

(In reply to Luke Crouch [:groovecoder] from comment #41) > +1 for: > > security.not_secure_connection_icon.enabled > security.not_secure_connection_icon.pbmode.enabled Sounds good!

I plan to work on this in the coming days, making the pbmode pref isn't much work at all and totally useful given our other work here. Regarding containers, I think we should instead look into allowing extensions to mark any page as insecure, this would allow interesting experiments like per containers settings or using the observatory to check sub resources for cert reliability or whatever. These experiments perhaps would never make it into the browser due to perf but they might be interesting for a post HTTPS future. The only alternative would be "all containers" I guess? I don't mind that either as using containers is likely to be a similar signal to pbmode too.

I would love to see WebExtensions be given the ability to downgrade (and not upgrade) the security indicator. There are lots of interesting extensions that could develop around that.

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review138376 > Shouldn't these prefs be false? Yes using it right now to test what is breaking with them turned on. > What does on_broken mean here? I'm not sure if I would understand that pref name without context. Is there a reason for the naming? Dropping this part of the code anyway :). > I wonder what impact this has on performance and whether we should load this pref lazily using defineLazyPreferenceGetter. > > In any case, we'll soon do a rundown of a perf profile of this function and there are likely worse offenders, so I'm fine with keeping this around for now. Keeping as it is, can change easily. > I don't think this is correct (it doesn't work for me when I test it). Also, this would whitelist HTTP pages with CSP: sandbox headers because these also get a null principal. > > Use gBrowser.selectedBrowser.documentURI instead. > > This should also not be in the "else" clause and rather in its own "else if". NetErrors don't appear to have the correct url on session restore. I have a feeling I was trying to fix that here. Leaving this issue open so I don't forget.

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review138376 > NetErrors don't appear to have the correct url on session restore. I have a feeling I was trying to fix that here. Leaving this issue open so I don't forget. adding the following seemed to fix this and also a null error that came from documentURI in tests: ``` let uriToCheck = gBrowser.selectedBrowser.documentURI || gBrowser.selectedBrowser.currentURI; ```

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review205938 ::: browser/base/content/browser.js:7117 (Diff revision 5) > _uri: null, > > /** > * We only know the connection type if this._uri has a defined "host" part. > * > - * These URIs, like "about:" and "data:" URIs, will usually be treated as a > + * These URIs, like "about:", "file:" and "data:" URIs, will usually be treated as a If I read the current implementation correctly, those three are not treated as non-secure and rather as, uh, unknown? :) ::: browser/base/content/browser.js:7119 (Diff revision 5) > /** > * We only know the connection type if this._uri has a defined "host" part. > * > - * These URIs, like "about:" and "data:" URIs, will usually be treated as a > + * These URIs, like "about:", "file:" and "data:" URIs, will usually be treated as a > * non-secure connection, unless they refer to an internally implemented > * browser page or resolve to "file:" URIs. This sentence just doesn't make sense at all anymore, I'd say we rewrite it entirely. ::: browser/base/content/browser.js:7525 (Diff revision 5) > let tooltip = ""; > let icon_country_label = ""; > let icon_labels_dir = "ltr"; > > + let uriToCheck = gBrowser.selectedBrowser.documentURI > + || gBrowser.selectedBrowser.currentURI; These are different things, we can't just use them interchangeably. For about: URIs, currentURI is self-signed.badssl.com and documentURI is about:certerror. (In addition, you should probably always use this.\_uri instead of gBrowser.selectedBrowser.currentURI) ::: browser/base/content/browser.js:7575 (Diff revision 5) > + this._identityBox.className = "unknownIdentity"; > + } else if (uriToCheck.spec.startsWith("about:")) { > - this._identityBox.className = "unknownIdentity"; > + this._identityBox.className = "unknownIdentity"; > + } else { > if (this._isBroken) { > + this._identityBox.className = "unknownIdentity"; Is this necessary with "weakCipher" getting added below? ::: browser/base/content/browser.js:7594 (Diff revision 5) > + PrivateBrowsingUtils.isWindowPrivate(window)); > + let className = warnOnInsecure ? "notSecure" : "unknownIdentity"; > + > + // For net errors we should not show notSecure as it's likely confusing > + // isNullPrincipal is set when on about:netError pages > + if (gBrowser.contentPrincipal.isNullPrincipal) { I'm 100% against checking null principal to find out if we're on an error page. As I mentioned, there are other cases where top level documents can have null principals, and while some of them may be filtered out already at this point, I don't think this is a good way to do it. about:neterror and about:certerror pages should be caught by the startsWith("about:") check on documentURI above. If there's any reason documentURI isn't set, we need to figure out how to solve that or work around it differently. We should probably sync up on that over IRC. Unless, of course, you explicitly want to not show the notSecure warning on null principal documents. Please add a different comment then. ::: browser/base/content/browser.js:7849 (Diff revision 5) > } catch (ex) { > // NetUtil's methods will throw for malformed URIs and the like > } > + > + // We should always consider local files to be as if it has no host > + if (this._isURILoadedFromFile) { I wonder if this can be solved by writing this._uriHasHost = !!this._uri.host; above, instead. That would, like, catch any other case where host just returns "". Maybe that's not what we want, though, what do you think?

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review206034 ::: browser/base/content/browser.js:7525 (Diff revision 5) > let tooltip = ""; > let icon_country_label = ""; > let icon_labels_dir = "ltr"; > > + let uriToCheck = gBrowser.selectedBrowser.documentURI > + || gBrowser.selectedBrowser.currentURI; Ok thanks for the explanation, I possibly just need to presence check documentUri.spec instead as it was failing some tests when it was null. ::: browser/base/content/browser.js:7594 (Diff revision 5) > + PrivateBrowsingUtils.isWindowPrivate(window)); > + let className = warnOnInsecure ? "notSecure" : "unknownIdentity"; > + > + // For net errors we should not show notSecure as it's likely confusing > + // isNullPrincipal is set when on about:netError pages > + if (gBrowser.contentPrincipal.isNullPrincipal) { Sorry, I meant to copy the comment from this and place it above where I did this check. I will remove this and it should behave the same.

Comment on attachment 8832591 [details] Bug 1310447 - Add a pref to display a negative indicator in the URL bar for non-secure sites https://reviewboard.mozilla.org/r/108796/#review206854 This looks good to me now, thank you for working on this! Once this lands, we should try to solve bug 1419007 quickly, trying it out it's really a bit irritating. ::: browser/base/content/browser.js:7570 (Diff revision 6) > [this.getIdentityData().caOrg]); > } > - } else { > + } else if (!this._uriHasHost) { > + this._identityBox.className = "unknownIdentity"; > + } else if (gBrowser.selectedBrowser.documentURI && > + gBrowser.selectedBrowser.documentURI.spec.startsWith("about:")) { Can you do gBrowser.selectedBrowser.documentURI.scheme == "about"? ::: browser/base/content/browser.js:7814 (Diff revision 6) > > setURI(uri) { > this._uri = uri; > > try { > this._uri.host; You can remove this. ::: browser/base/content/test/siteIdentity/browser_check_identity_state.js:7 (Diff revision 6) > * Test the identity mode UI for a variety of page types > */ > > "use strict"; > > const DUMMY = "browser/browser/base/content/test/general/dummy_page.html"; Your tests are hitting a 404 page because you haven't added this file to the support-files section in browser.ini of b/b/c/test/siteIdentity. ::: browser/base/content/test/siteIdentity/browser_check_identity_state.js:230 (Diff revision 6) > +add_task(async function test_resource_uri() { > + await resourceUriTest(true); > + await resourceUriTest(false); > }); > > -add_task(async function test_data_uri() { > +async function aboutNetErrorTest(secureCheck) { I don't really see the value in writing a test for accessing about:neterror directly, but I don't mind it either. Isn't nocert.example.com or expired.example.com enough for our case? ::: browser/base/content/test/siteIdentity/browser_check_identity_state.js:333 (Diff revision 6) > +add_task(async function test_data_uri() { > + dataUriTest(true); > + dataUriTest(false); > +}); > + > +function promiseOpenAndLoadWindow(aOptions, aWaitForDelayedStartup = false) { It looks like you can just use BTU.openNewBrowserWindow instead. ::: browser/base/content/test/siteIdentity/head.js:160 (Diff revision 6) > is(activeBlocked, !!stateActiveBlocked, "Expected state for activeBlocked matches UI state"); > is(activeLoaded, !!stateActiveLoaded, "Expected state for activeLoaded matches UI state"); > is(passiveLoaded, !!statePassiveLoaded, "Expected state for passiveLoaded matches UI state"); > > if (stateInsecure) { > + const INSECURE_CONNECTION_ICON = Services.prefs.getBoolPref("security.insecure_connection_icon.enabled"); We generally only use consts as globals, you can make this let insecureConnectionIcon = ...

Pushed by aiakab@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb8da591a522 Add a pref to display a negative indicator in the URL bar for non-secure sites r=johannh

This isn't enabled anywhere by default currently, but I still think it's interesting to mention on MDN. for now, I've just included an entry on our Experimental features page: https://developer.mozilla.org/en-US/Firefox/Experimental_features#Security Does this sound OK to you?

This sounds great Chris, thanks again for this!

On a related note, Chrome will be showing "Not Secure" for HTTP websites, starting in Chrome 68 (July 2018): https://security.googleblog.com/2018/02/a-secure-web-is-here-to-stay.html