Rewrite the Pale Moon padlock code #1717

The padlock code we're using is relying on very old nsIWebProgressListener code that makes assumptions about old IDL specifics and not using additional pieces of info it provides (e.g. weak cipher info). The platform has also done away with always providing a security level along with "is_secure" so the padlock code needs to get a once-over to modernize it based on our current API.

Specifically, anytime STATE_IS_SECURE it also returns STATE_SECURE_HIGH in most cases (STATE_SECURE_MED and STATE_SECURE_LOW were removed at some point along the way).

This makes STATE_SECURE_HIGH basically worthless and the other two still being bitwise values in the IDL seems to have been kept around only to satisfy consumers of nsIWebProgressListener in a rare case of not breaking extensions but most likely so internal consumers didn't have to be rewritten at the time.

What needs to happen in my view is either STATE_SECURE_HIGH and the idl bitwise definitions that aren't valid anymore need to be removed or they all need restored to working order. Otherwise we need to make notes and keep in mind that if STATE_IS_SECURE we should also set STATE_SECURE_HIGH to maintain consistancy.

As for the padlock component in browser base this simply needs aligned to what is actually being used in cpp and a condition should be added to switch logic for the possibility of being STATE_IS_SECURE but not having STATE_SECURE_HIGH. Which very well might be desirable for the case of local injection on a secure site in the uBO data: uri case.

The padlock code we're using is relying on very old nsIWebProgressListener code that makes assumptions about old IDL specifics and not using additional pieces of info it provides (e.g. weak cipher info). The platform has also done away with always providing a security level along with "is_secure" so the padlock code needs to get a once-over to modernize it based on our current API. Specifically, anytime STATE_IS_SECURE it also returns STATE_SECURE_HIGH in most cases (STATE_SECURE_MED and STATE_SECURE_LOW were removed at some point along the way). This makes STATE_SECURE_HIGH basically worthless and the other two still being bitwise values in the IDL seems to have been kept around only to satisfy consumers of nsIWebProgressListener in a rare case of not breaking extensions but most likely so internal consumers didn't have to be rewritten at the time. What needs to happen in my view is either STATE_SECURE_HIGH and the idl bitwise definitions that aren't valid anymore need to be removed or they all need restored to working order. Otherwise we need to make notes and keep in mind that if STATE_IS_SECURE we should also set STATE_SECURE_HIGH to maintain consistancy. As for the padlock component in browser base this simply needs aligned to what is actually being used in cpp and a condition should be added to switch logic for the possibility of being STATE_IS_SECURE but not having STATE_SECURE_HIGH. Which very well might be desirable for the case of local injection on a secure site in the uBO data: uri case.

I have a... suggestion about this case. I don't really want to make two drafts to explain it, so I just referenced this issue with the commits and am making this comment. If you could, please take a look at the changes I'm suggesting.

I haven't actually compiled to see if this brings back the identity-box "Mixed Content" tooltip message and urlbar "security_level" value, but it seems like a valid methodology regardless. Note that I've kept the values defined to allow graceful deprecation of UXP applications, but that STATE_SECURE_HIGH will no longer be sent. A new STATE_SECURE_MIXED value is added with the same value as STATE_SECURE_LOW for compatibility with the old Mixed Content methodology. I also added it to the aState value in MapInternalToExternalState (which I think will bring back the Mixed Content functionality I mentioned above).

Also, I'm not sure what is_insecure was for, but I got rid of it because it wasn't being used. And I made sure highlight_urlbar was false for "about:" "chrome:" and "file:" protocols, just in case.

I have a... suggestion about this case. I don't really want to make two drafts to explain it, so I just referenced this issue with the commits and am making this comment. If you could, please take a look at the changes I'm suggesting. I haven't actually compiled to see if this brings back the `identity-box` "Mixed Content" tooltip message and `urlbar` "security_level" value, but it seems like a valid methodology regardless. Note that I've kept the values defined to allow graceful deprecation of UXP applications, but that STATE_SECURE_HIGH will no longer be sent. A new STATE_SECURE_MIXED value is added with the same value as STATE_SECURE_LOW for compatibility with the old Mixed Content methodology. I also added it to the aState value in MapInternalToExternalState (which I think will bring back the Mixed Content functionality I mentioned above). Also, I'm not sure what `is_insecure` was for, but I got rid of it because it wasn't being used. And I made sure `highlight_urlbar` was false for "about:" "chrome:" and "file:" protocols, just in case.

No. nonononono. We do NOT want to change the way the platform works because it will break everything (extensions, etc.). The padlock code (meaning the specific padlock code used in Pale Moon) uses old calls/interfaces and needs to be rewritten. That is front-end code in palemoon/base/content/padlock.js
This issue was created on the Pale Moon repo and not on the UXP repo for a reason.

No. nonononono. We do NOT want to change the way the platform works because it will break everything (extensions, etc.). The padlock code (meaning the specific padlock code used in Pale Moon) uses old calls/interfaces and needs to be rewritten. That is front-end code in `palemoon/base/content/padlock.js` This issue was created on the Pale Moon repo and not on the UXP repo for a reason.

In addition, part of the whole point of the rewrite would be to actually make use of "weak security" again as a visual indicator (_low) based on different states (in particular weak ciphers or protocols).

Okay, Adding STATE_SECURE_HIGH back in is easy enough. But what about the return of the Mixed Content state? Should I scrap that, revert it to using LOW since we're keeping HIGH, or bring it back to life?

And if any security is weak, should it really be allowed? I guess if people re-enable it through PMC...

Edit: Went back to using LOW, but kept the changes to bring Mixed Content back. Low now exists as a state if HIGH is not set, but STATE_IS_SECURE | STATE_SECURE_LOW doesn't exist. Is this more what you're aiming for?

Oh, and of course, this comparison makes it much easier to look at.

Okay, Adding STATE_SECURE_HIGH back in is easy enough. But what about the return of the Mixed Content state? Should I scrap that, revert it to using LOW since we're keeping HIGH, or bring it back to life? And if any security is weak, should it really be allowed? I guess if people re-enable it through PMC... Edit: Went back to using LOW, but kept the changes to bring Mixed Content back. Low now exists as a state if HIGH is not set, but STATE_IS_SECURE | STATE_SECURE_LOW doesn't exist. Is this more what you're aiming for? Oh, and of course, [this comparison](https://github.com/MoonchildProductions/Pale-Moon/compare/master...RealityRipple:security-1717) makes it much easier to look at.

I'm gonna need some time to think about this. LOW is not the same as MIXED. Mixed content is a separate state, and not necessarily indicating the security level of the parts that are secured. Although mixed content does indicate of course that something isn't quite right, the unsecured content can be from trusted sources. Maybe your original approach was good after all, but I'm not sure if all of that pans out. The idea I had was to use the nsISSLStatus interface and currentBrowser.SecurityUI instead of the WebProgressListener (or maybe in combination) to get the various states of the page instead.

I'm gonna need some time to think about this. `LOW` is not the same as `MIXED`. Mixed content is a separate state, and not necessarily indicating the security level of the parts that _are_ secured. Although mixed content does indicate of course that something isn't quite right, the unsecured content can be from trusted sources. Maybe your original approach was good after all, but I'm not sure if all of that pans out. The idea I had was to use the `nsISSLStatus` interface and `currentBrowser.SecurityUI` instead of the WebProgressListener (or maybe in combination) to get the various states of the page instead.

I was using BROKEN | LOW for MIXED because that's how it was previously written. Backward compatibility and all that.

And, I see. That might be a little bit more intensive of a changeset, but it would undoubtedly allow for other information to be used in the process.

I was using `BROKEN | LOW` for `MIXED` because that's how it was previously written. Backward compatibility and all that. And, I see. That might be a little bit more intensive of a changeset, but it would undoubtedly allow for other information to be used in the process.

I think your original proposed changes are good, after all, but it won't solve this issue; it would make the rewrite later simpler, most likely?

What I'd ultimately want to end up with is a system where we have 4 distinct states:
EV, green, always high security. If mixed content or weak ciphers are used with EV it should always be considered broken.
DV, blue, secure ciphers used. High security. If mixed content is used (only passive) it should be considered OK. Mixed active content should make it low security.
Low security: weak ciphers are used. RC4_SHA, 3DES, protocol SSL3 (potentially also TLS 1.0/TLS 1.1), weak signature algorithms.
Broken: Unacceptable ciphers used (EXPORT, RC2, RC4_MD5), unacceptable signature algorithms.

That is something we'd need nsISSLStatus for to get the cipher suites and protocol version from.

I think your original proposed changes are good, after all, but it won't solve _this_ issue; it would make the rewrite later simpler, most likely? What I'd ultimately want to end up with is a system where we have 4 distinct states: **EV**, green, always high security. If mixed content or weak ciphers are used with EV it should **always** be considered **broken**. **DV**, blue, secure ciphers used. High security. If mixed content is used (only _passive_) it should be considered OK. Mixed _active_ content should make it low security. **Low security**: weak ciphers are used. RC4_SHA, 3DES, protocol SSL3 (potentially also TLS 1.0/TLS 1.1), weak signature algorithms. **Broken**: Unacceptable ciphers used (EXPORT, RC2, RC4_MD5), unacceptable signature algorithms. That is something we'd need nsISSLStatus for to get the cipher suites and protocol version from.

I was thinking SSL3 and TLS 1.0 should be BROKEN, TLS 1.1 LOW, since it's only vulnerable to POODLE downgrades, but yeah. I was thinking along the same lines looking at the nsISSLStatus.idl file.

And, actually, if we're scrapping Mixed as a state and divvying it up like that, maybe we should start over.

Do we want to keep HIGH, MED, and LOW for compatibility, or drop them in favor of detecting everything in PM code rather than UXP code? If we drop them, do we delete them from UXP as a breaking change or do we just ignore them in padlock.js and phase them out of UXP at a later date?

I was thinking SSL3 and TLS 1.0 should be `BROKEN`, TLS 1.1 `LOW`, since it's only vulnerable to POODLE downgrades, but yeah. I was thinking along the same lines looking at the `nsISSLStatus.idl` file. And, actually, if we're scrapping Mixed as a state and divvying it up like that, maybe we should start over. Do we want to keep HIGH, MED, and LOW for compatibility, or drop them in favor of detecting everything in PM code rather than UXP code? If we drop them, do we delete them from UXP as a breaking change or do we just ignore them in `padlock.js` and phase them out of UXP at a later date?

I disagree with broken for TLS 1.0 because it's perfectly possible to have TLS 1.0 in a secure manner! Indicating low should already draw attention to it not having a lot of trust. SSL3... I guess by now Broken is fine, yeah. People really really shouldn't be using that anymore.

Alright, I dropped the UXP changes entirely. STATE_SECURE_* can be marked deprecated and eventually removed, but for now, we'll just ignore it completely in padlock.js. STATE_LOADED_MIXED_ACTIVE_CONTENT and STATE_LOADED_MIXED_DISPLAY_CONTENT are directly checked for now. And if the state isn't null or "broken" after checking against aState, then nsISSLStatus for gBrowser is loaded up. The protocolVersion is checked, then the cipherSuite. If any specific signatureSchemeName values should be added in, they should probably go between the protocol and the cipher in check order.

I have not opened this in an IDE or attempted a compile, so I'm sure I dropped at least one semicolon somewhere, but it should at least be a better starting point.

Also, I'm kind of on the fence about TLS 1.1 being "low" now, too... It really did get pushed aside for no rational reason.

Alright, I dropped the UXP changes entirely. STATE_SECURE_* can be marked deprecated and eventually removed, but for now, we'll just ignore it completely in `padlock.js`. `STATE_LOADED_MIXED_ACTIVE_CONTENT` and `STATE_LOADED_MIXED_DISPLAY_CONTENT` are directly checked for now. And if the state isn't `null` or `"broken"` after checking against `aState`, then nsISSLStatus for gBrowser is loaded up. The `protocolVersion` is checked, then the `cipherSuite`. If any specific `signatureSchemeName` values should be added in, they should probably go between the protocol and the cipher in check order. I have not opened this in an IDE or attempted a compile, so I'm sure I dropped at least one semicolon somewhere, but it should at least be a better starting point. Also, I'm kind of on the fence about TLS 1.1 being "low" now, too... It really did get pushed aside for no rational reason.

Following the back-and-forth in the PR draft:

Hmm I'd have to look up what NSS's internal IDs are, then. I may misremember.
Also there is no point in taking suites into account that we don't enable at all in out PSM module even if NSS supports them.

I don't agree with 3DES ever being marked as medium anymore. Not only do we have the SWEET32 (and the same class of attacks) against it, there are also attacks against the fact that it uses 3 individually small keys and the fact that the common usage in browsers is only 112 bits. We haven't enabled 3DES for a few years so certainly not going to mark it "could be OK". No. it's considered a weak cipher.

TLS_RSA_WITH_AES_128_GCM_SHA256 specifically is marked bad because it makes no sense to use plain RSA key exchange with AES-GCM. It indicates a misconfiguration more than anything else.

Localizing the padlock tooltips is a good idea. I just never got around to it, myself.

More details isn't needed and undesirable. It needs to hit the balance between general usability and tech goop so needs to provide clear concise descriptions of the status in general terms. There is the security info window with the cipher details, and if you want even more details there is cipherfox which targets Pale Moon.

Following the back-and-forth in the PR draft: Hmm I'd have to look up what NSS's internal IDs are, then. I may misremember. Also there is no point in taking suites into account that we don't enable at all in out PSM module even if NSS supports them. I don't agree with 3DES ever being marked as medium anymore. Not only do we have the SWEET32 (and the same class of attacks) against it, there are also attacks against the fact that it uses 3 individually small keys and the fact that the common usage in browsers is only 112 bits. We haven't enabled 3DES for a few years so certainly not going to mark it "could be OK". No. it's considered a weak cipher. `TLS_RSA_WITH_AES_128_GCM_SHA256` specifically is marked bad because it makes no sense to use plain RSA key exchange with AES-GCM. It indicates a misconfiguration more than anything else. Localizing the padlock tooltips is a good idea. I just never got around to it, myself. More details isn't needed and undesirable. It needs to hit the balance between general usability and tech goop so needs to provide clear concise descriptions of the status in general terms. There is the security info window with the cipher details, and if you want even more details there is cipherfox which targets Pale Moon.

Let me know what you find in the ID list - far as I can tell, the best option would be what I originally had. _EDE3_ only seems to be used by those two old and hopefully unused methods, and _3DES_ will catch all the others.

Should we throw in TLS_RSA_WITH_AES_128_GCM_SHA256 as "broken" just in case it somehow comes up, or will a more solid error page take care of that condition?

Let me know what you find in the ID list - far as I can tell, the best option would be what I originally had. `_EDE3_` only seems to be used by those two old and hopefully unused methods, and `_3DES_` will catch all the others. Should we throw in `TLS_RSA_WITH_AES_128_GCM_SHA256` as "broken" just in case it somehow comes up, or will a more solid error page take care of that condition?

I was checking for STATE_IS_SECURE with mixed content. UXP still says those are STATE_IS_BROKEN. Sorry about that.

Unfortunately, I was partially right. Mixed content gets rid of the STATE_IDENTITY_EV_TOPLEVEL flag, so I've rewritten the code to use nsISSLStatus's isExtendedValidation property to tell the difference between mixed content methodologies.

The current setup ignores potential issues with mixed display content pages on DV pages, since any other errors are overwritten with the mixed content's broken state value. Checking for errors might end up being a job for nsISSLStatus as well, leading us to completely drop use of aState.

I was checking for `STATE_IS_SECURE` with mixed content. UXP still says those are `STATE_IS_BROKEN`. Sorry about that. Unfortunately, I was partially right. Mixed content gets rid of the `STATE_IDENTITY_EV_TOPLEVEL` flag, so I've rewritten the code to use `nsISSLStatus`'s `isExtendedValidation` property to tell the difference between mixed content methodologies. The current setup ignores potential issues with mixed display content pages on DV pages, since any other errors are overwritten with the mixed content's broken state value. Checking for errors might end up being a job for `nsISSLStatus` as well, leading us to completely drop use of `aState`.

Let me know if the localization is named poorly, in the wrong place, or doesn't correctly handle missing strings in other locales.

Alright, I've tried to document exactly what we're doing this time around, but I think I got things in the right paradigm now:

Check isExtendedValidation
- Check for Mixed Content
If not "broken", Check protocolVersion
If not "broken", Check cipherSuite
If not "broken", Check isUntrusted, isDomainMismatch, and isNotValidAtThisTime

Note that at present, all three booleans in step 4 result in "broken", even if the problem is bypassed by the user. That keeps the warning there so users have a reminder to be careful. If any of those values should be "low" instead, (like the time, maybe?) let me know. Same if you want one or more to go with null, hiding the padlock entirely.
I've looked through nsIWebProgressListener.idl to see if there are any failure types that aren't included in nsISSLStatus, but nothing stood out.

Also, the check for "about:" "chrome:" and "file:" protocols that used to set the unused is_insecure variable was removed. If there's some special case for these protocols that lets any of them have an SSLStatus, we can add some special behavior for them if you want, but... I don't see how that would happen.

Oh, and lastly, I made a quick extension to overwrite padlock.js so a full compile isn't necessary (except to test localization) if you want to check the new function.

Alright, I've tried to document exactly what we're doing this time around, but I think I got things in the right paradigm now: 1. Check `isExtendedValidation` - Check for Mixed Content 2. If not "broken", Check `protocolVersion` 3. If not "broken", Check `cipherSuite` 4. If not "broken", Check `isUntrusted`, `isDomainMismatch`, and `isNotValidAtThisTime` Note that at present, all three booleans in step 4 result in "broken", even if the problem is bypassed by the user. That keeps the warning there so users have a reminder to be careful. If any of those values should be "low" instead, (like the time, maybe?) let me know. Same if you want one or more to go with `null`, hiding the padlock entirely. I've looked through `nsIWebProgressListener.idl` to see if there are any failure types that aren't included in `nsISSLStatus`, but nothing stood out. Also, the check for "about:" "chrome:" and "file:" protocols that used to set the unused `is_insecure` variable was removed. If there's some special case for these protocols that lets any of them have an `SSLStatus`, we can add some special behavior for them if you want, but... I don't see how that would happen. Oh, and lastly, I made a [quick extension](https://realityripple.com/Software/Mozilla-Extensions/1717/) to overwrite padlock.js so a full compile isn't necessary (except to test localization) if you want to check the new function.

Sorry for not being responsive a few days.
The logic flow described looks good. And no, step 4's checks should all be broken. We don't want to mark untrusted, mismatched or expired certificates as anything but broken.

As for the "does not supply identity information" when the status is mixed, that is correct if you look at what content is presented to the user. You don't want to indicate that the content is supplied by the secure party when it isn't true (because part of the content is not and you cannot say it is). It probably needs a better message in that situation though.

Sorry for not being responsive a few days. The logic flow described looks good. And no, step 4's checks should all be broken. We don't want to mark untrusted, mismatched or expired certificates as anything but broken. As for the "does not supply identity information" when the status is mixed, that is correct if you look at what content is presented to the user. You don't want to indicate that the content is supplied by the secure party when it isn't true (because part of the content is not and you cannot say it is). It probably needs a better message in that situation though.

Alright. That's probably work for a different PR tho.
I do think a few more glances over the code would be a good idea, but I guess it's time take the PR out of Draft. If you notice any faulty states or weird error types I failed to take into account, please let me know.

Alright. That's probably work for a different PR tho. I do think a few more glances over the code would be a good idea, but I guess it's time take the PR out of Draft. If you notice any faulty states or weird error types I failed to take into account, please let me know.

@wolfbeast, сould you please give a hint for translation, can these messages be interpreted as follows:

Extended Validated [connection]
Secure [connection]
Weak security [connection]
Not secure [connection]

And can we use the full phrase as a source for translation if the short form does not give a clear enough result?

@wolfbeast, сould you please give a hint for translation, can [these messages](https://github.com/RealityRipple/Pale-Moon/blob/1f488302c18e9d1ef14c333bb81bbdbab2485634/palemoon/locales/en-US/chrome/browser/browser.properties#L283-L286) be interpreted as follows: ``` Extended Validated [connection] Secure [connection] Weak security [connection] Not secure [connection] ``` And can we use the full phrase as a source for translation if the short form does not give a clear enough result?

It is the connection status, so yes, you should interpret it as such.
Please do note that EV/Extended Validated is a fixed term and should likely either not be localized, or at least have a mention of "EV" if it is localized, in brackets.
You should be able to use the full phrase since these are tooltips and have some leeway in terms of space available, but don't make it too long.

It is the connection status, so yes, you should interpret it as such. Please do note that EV/Extended Validated is a fixed term and should likely either not be localized, or at least have a mention of "EV" if it is localized, in brackets. You should be able to use the full phrase since these are tooltips and have some leeway in terms of space available, but don't make it too long.

FTR, the current unstables should have these changes in them.

Thanks, strings changes were pushed to CrowdIn.

OK, so there's an issue now with mixed passive content, as it no longer triggers the doorhanger/shield while it should, and kills the domain name otherwise in the identity panel, and does not mark it weak but rather secure.
https://mixed.badssl.com/

@RealityRipple can you have a look at this particular situation?

OK, so there's an issue now with mixed passive content, as it no longer triggers the doorhanger/shield while it should, and kills the domain name otherwise in the identity panel, and does not mark it weak but rather secure. https://mixed.badssl.com/ ![image](https://user-images.githubusercontent.com/3359132/92554509-c8a38600-f265-11ea-9522-e2259045cf47.png) @RealityRipple can you have a look at this particular situation?

The changes I made only have an effect on the padlock, not the rest of the identity panel. That's handled elsewhere. And your instructions were:

DV, blue, secure ciphers used. High security. If mixed content is used (only passive) it should be considered OK. Mixed active content should make it low security.

So that's exactly the way the padlock code's written.

Here's the exact logic in question: padlock.js. EV checks for any mixed content, DV checks for active mixed content only. Whatever changes to the padlock you want can be made there.

I considered the rest of the panel outside of this specific issue's purview when I wrote Pull 1828, but it looks like the state can be manipulated in browser.js's checkIdentity function and the setIdentityMessages function takes care of the domain name, I think. Also, it looks like some code to show the mixed content doorhanger for passive content if passive content is blocked by preference should be added to the checkIdentity function, but again, that should probably be a separate issue.

What is the exact state you want for mixed passive and active content? Adding an IDENTITY_MODE_MIXED_CONTENT case to browser.js's setIdentityMessages() should show the domain name in passive content situations. Adding || (aState & wpl.STATE_LOADED_MIXED_DISPLAY_CONTENT) to padlock.js's onSecurityChange will make passive content "low" for DV. The coloring of the panel is a matter of the unknownIdentity+mixedContent±mixedActiveContent class names for the identity-box object in the default theme's browser.css.

The changes I made only have an effect on the padlock, not the rest of the identity panel. That's handled elsewhere. And your instructions were: > **DV**, blue, secure ciphers used. High security. If mixed content is used (only _passive_) it should be considered OK. Mixed _active_ content should make it low security. So that's exactly the way the padlock code's written. Here's the exact logic in question: [padlock.js](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/padlock.js#L30-L47). EV checks for any mixed content, DV checks for active mixed content only. Whatever changes to the padlock you want can be made there. I considered the rest of the panel outside of this specific issue's purview when I wrote Pull 1828, but it looks like the state can be manipulated in [browser.js's checkIdentity function](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/browser.js#L6712-L6741) and the [setIdentityMessages function](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/browser.js#L6822) takes care of the domain name, I think. Also, it looks like some code to show the mixed content doorhanger for passive content if passive content is blocked by preference should be added to the checkIdentity function, but again, that should probably be a separate issue. What is the exact state you want for mixed passive and active content? Adding an `IDENTITY_MODE_MIXED_CONTENT` case to [browser.js's setIdentityMessages()](https://github.com/MoonchildProductions/Pale-Moon/blob/24a06c4742629163629e50604918ac31a427d352/palemoon/base/content/browser.js#L6835) should show the domain name in passive content situations. Adding `|| (aState & wpl.STATE_LOADED_MIXED_DISPLAY_CONTENT)` to [padlock.js's onSecurityChange](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/padlock.js#L43) will make passive content "low" for DV. The coloring of the panel is a matter of the `unknownIdentity`+`mixedContent`±`mixedActiveContent` class names for the `identity-box` object in the default theme's browser.css.

I apologize, you are correct in what I asked. I'll have to think about what to do with mixed content some more. I'd prefer to block it outright but that would cause too much breakage so we need some other way to indicate this. Perhaps a unique icon for it instead of the padlock when not showing the domain name in mixed content.

Probably better is this for DV:

Everything secure: High (subject to other checks like protocol etc.)
Mixed display content: keep displaying domain name but make level "low" (possibly make this a separate state in the future?)
Mixed active content: low, don't display domain name (possibly make this a separate state in the future?)

Probably better is this for DV: - Everything secure: High (subject to other checks like protocol etc.) - Mixed display content: **keep displaying** domain name but make level "low" (possibly make this a separate state in the future?) - Mixed active content: low, **don't** display domain name (possibly make this a separate state in the future?)

Just about any arrangement you want is possible. As is checking pref states, so we could code an alternate behavior if the security.mixed_content.block_display_content pref is true, and turn the pref on at a later time, possibly with the mixed content doorhanger to let users fix any breakage caused by that change.

Just about any arrangement you want is possible. As is checking pref states, so we could code an alternate behavior if the `security.mixed_content.block_display_content` pref is true, and turn the pref on at a later time, possibly with the mixed content [doorhanger](https://github.com/MoonchildProductions/Pale-Moon/blob/24a06c4742629163629e50604918ac31a427d352/palemoon/base/content/browser.js#L6738) to let users fix any breakage caused by that change.

The way the code in browser.js is currently written, if security.mixed_content.block_active_content is false, setMode() gets called with IDENTITY_MODE_MIXED_CONTENT instead of IDENTITY_MODE_MIXED_ACTIVE_CONTENT, which means that if we display the identity label for mixed display content, it will also show up for mixed active content when mixed active content is allowed by pref. If you don't want that behavior, please let me know; it can be prevented, but it'd require a few extra changes.

Also, sorry for the double-commit. Somehow, 24a06c4742 wasn't checked out when I cloned the repo.

The way the code in browser.js is currently written, if `security.mixed_content.block_active_content` is false, `setMode()` gets called with `IDENTITY_MODE_MIXED_CONTENT` instead of `IDENTITY_MODE_MIXED_ACTIVE_CONTENT`, which means that if we display the identity label for mixed display content, it will also show up for mixed active content when mixed active content is allowed by pref. If you don't want that behavior, please let me know; it can be prevented, but it'd require a few extra changes. Also, sorry for the double-commit. Somehow, 24a06c4742629163629e50604918ac31a427d352 wasn't checked out when I cloned the repo.

it will also show up for mixed active content when mixed active content is allowed by pref.

If people enable this pref themselves they take responsibility for their browser use if so. There's no reason to go against the user's wishes (even if they are foolhardy), so displaying the domain in that case is fine.

> it will also show up for mixed active content when mixed active content is allowed by pref. If people enable this pref themselves _they_ take responsibility for their browser use if so. There's no reason to go against the user's wishes (even if they are foolhardy), so displaying the domain in that case is fine.

Actually, we already have a level=mixed state in our themes, so we should leverage that.

Actually, we already have a `level=mixed` state in our themes, so we should leverage that.

security_level=mixed will provide a different urlbar shadow coloring, but the padlock's mixed styling is all low styling at present, and the identity panel is only styled for verifiedIdentity and verifiedDomain, so there'd be no visual difference to the padlock or the identity panel unless we make changes to padlock.css and/or browser.css. If you do want to use it, do you want it for both active and passive mixed content or just one of them?

`security_level=mixed` will provide a different urlbar shadow coloring, but the padlock's `mixed` styling is all `low` styling at present, and the identity panel is only styled for `verifiedIdentity` and `verifiedDomain`, so there'd be no visual difference to the padlock or the identity panel unless we make changes to `padlock.css` and/or `browser.css`. If you do want to use it, do you want it for both active and passive mixed content or just one of them?

You're missing the point that not everyone might be on the default theme ;) other themes may have unique styling already.

We might want to split this up into a few different issues going forward: Padlock, Panel, and Label. It might make deciding on courses of action for each element a little cleaner, and future research a little simpler. Changing the security_level on the urlbar as a whole will also change the padlock level, so those should probably remain together. And while the identity label is planned to be physically changed, the panel's CSS is the only thing that would potentially be changed for this, not any of the underlying JS.

Oh, also, it should be noted that at present, active mixed content pages are detected as secure when the mixed content is blocked. It only shows up as low security and removes the identity label if the user allows the mixed content. If you want any changes to that behavior, that's gonna be real messy.

We might want to split this up into a few different issues going forward: Padlock, Panel, and Label. It might make deciding on courses of action for each element a little cleaner, and future research a little simpler. Changing the `security_level` on the urlbar as a whole will also change the padlock `level`, so those should probably remain together. And while the identity label is planned to be physically changed, the panel's CSS is the only thing that would potentially be changed for this, not any of the underlying JS. Oh, also, it should be noted that at present, active mixed content pages are detected as secure when the mixed content is blocked. It only shows up as low security and removes the identity label if the user allows the mixed content. If you want any changes to that behavior, that's gonna be real messy.

Oh, and if we are bringing mixed back, do we want to give it a unique (localized) tooltip or just use low's "Weak security" or broken's "Not secure"?

Woah dude, slow down

Pages are secure if the mixed content is blocked. Think about it. We do NOT want to change that.
also, I'm saying that "level" should be changed, not "security_level"

But you know what, just leave it, I'll have a look at it myself because I'm thinking at this point you're trying to change way too much.

Woah dude, slow down Pages are secure if the mixed content is blocked. Think about it. We do NOT want to change that. also, I'm saying that "level" should be changed, not "security_level" But you know what, just leave it, I'll have a look at it myself because I'm thinking at this point you're trying to change way too much.

And regarding adding the identity label to passive mixed content for domain-validated pages, do we want to color the panel or leave it transparent? I'm not any good with aesthetics, so while I can say exactly where to add the right CSS changes, I would be completely the wrong person to make any decisions on what those changes would be. Coloring and shading and all that. But leaving it transparent definitely looks... weird.

And regarding adding the identity label to passive mixed content for domain-validated pages, do we want to color the panel or leave it transparent? I'm not any good with aesthetics, so while I can say exactly _where_ to add the right CSS changes, I would be completely the wrong person to make any decisions on _what_ those changes would be. Coloring and shading and all that. But leaving it transparent definitely looks... ![mixed_nopanelbg](https://user-images.githubusercontent.com/25967473/92573698-02ab6280-f23b-11ea-86dc-08d411a644d4.png) weird.

also, I'm saying that "level" should be changed, not "security_level"

level and security_level are set in the same place with the same variable. We'd need to add a second variable if you want them separated.

Here's a few extra notes:

The default theme uses [security_level="mixed"] css to stylize the urlbar inner-shadow, not [level="mixed"].
The level value in padlock.js is used to set the urlbar's security_level, the padlock's level, and the padlock's tooltip. Throwing in mixed will either result in security_level=mixed happening and adding a case "mixed": to the select statement for the tooltip, either with a new tooltip value or as a fallthrough alongside case "low":, or use of a new variable that is equal to level unless level="mixed" in which case the new variable is set to "low", to pass along to setAttribute("security_level" and to use in the tooltip select statement.
The identity panel can be colored for passive mixed content using #urlbar[pageproxystate="valid"] > #identity-box.mixedContent:not(.mixedActiveContent) without interfering with the lack of panel coloring for mixed active content (should be placed around here on the Windows theme). Whether you want a new color or just to go with the DV blue's all up to you of course.
There's commented-out code for the default favicon here that you may want to add passive mixed content to just in case that feature is ever brought back.

> also, I'm saying that "level" should be changed, not "security_level" `level` and `security_level` are set in the same place with the same variable. We'd need to add a second variable if you want them separated. --- Here's a few extra notes: - The default theme uses `[security_level="mixed"]` css to stylize the [urlbar inner-shadow](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/themes/windows/browser.css#L1518), not `[level="mixed"]`. - The `level` value in `padlock.js` is used to set the urlbar's `security_level`, the padlock's `level`, and the padlock's tooltip. Throwing in `mixed` will either result in `security_level=mixed` [happening](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/padlock.js#L98) and adding a `case "mixed":` to the [select statement](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/base/content/padlock.js#L145) for the tooltip, either with a new tooltip value or as a fallthrough alongside `case "low":`, or use of a new variable that is equal to `level` unless `level`="mixed" in which case the new variable is set to "low", to pass along to `setAttribute("security_level"` and to use in the tooltip select statement. - The identity panel can be colored for passive mixed content using `#urlbar[pageproxystate="valid"] > #identity-box.mixedContent:not(.mixedActiveContent)` without interfering with the lack of panel coloring for mixed active content (should be placed around [here](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/themes/windows/browser.css#L1465) on the Windows theme). Whether you want a new color or just to go with the DV blue's all up to you of course. - There's commented-out code for the default favicon [here](https://github.com/MoonchildProductions/Pale-Moon/blob/master/palemoon/themes/windows/browser.css#L1597) that you may want to add passive mixed content to just in case that feature is ever brought back.

Right, so i've taken all the info and come up with restyling for it all.

What I have now:
Mixed active content, blocked:

Mixed active content, when protection is disabled from the doorhanger:

Mixed active content, when protection is disabled from the pref:

Mixed passive content:

and classic style:

Mixed passive and active content on the same page (passive allowed, active blocked, per default settings):

Weak protocol or cipher:

(this disables the URL bar shading, too, which is good)

Feedback?

Right, so i've taken all the info and come up with restyling for it all. What I have now: Mixed active content, blocked: ![mixed-act-blocked](https://user-images.githubusercontent.com/3359132/92731627-9f721b00-f375-11ea-9e54-daa89b489690.png) Mixed active content, when protection is disabled from the doorhanger: ![mixed-act-protdis](https://user-images.githubusercontent.com/3359132/92731686-b153be00-f375-11ea-900c-6f3606320fbd.png) Mixed active content, when protection is disabled from the pref: ![mixed-act-protdis-pref](https://user-images.githubusercontent.com/3359132/92731966-114a6480-f376-11ea-8a65-68fa96f4b228.png) Mixed passive content: ![mixed-passive](https://user-images.githubusercontent.com/3359132/92732294-8a49bc00-f376-11ea-812b-c8e029467e5d.png) and classic style: ![mixed-passive-classic](https://user-images.githubusercontent.com/3359132/92732320-93d32400-f376-11ea-80f4-565571df4fd7.png) Mixed passive _and_ active content on the same page (passive allowed, active blocked, per default settings): ![mixed-passive-and-active](https://user-images.githubusercontent.com/3359132/92732439-c11fd200-f376-11ea-8067-50daffda95a6.png) Weak protocol or cipher: ![weak](https://user-images.githubusercontent.com/3359132/92732379-aa797b00-f376-11ea-9b78-300d559d825b.png) (this disables the URL bar shading, too, which is good) Feedback?

Looks good and straightforward. ~~If you want to add a tiny bit more of a "this is not a great state to be in" vibe, you could add a "⚠️" to the padlock, but it's already pretty obviously "not perfect" as-is.~~ Actually, the more I think about it, the more abrasive the warning triangle feels.

Is adding the "mixed passive" padlock gonna be a breaking change for themes, tho? Or will there be a "low" or "broken" state fallback? (Hopefully most themes still combine level=low and level=mixed at the very least, but it's not guaranteed. Cleanliness should probably come before backward compatibility, though, I suppose.) And I'm guessing it'll get its own "Low security (HTTP Files)"-esque tooltip message, too?

It's also possible to give the "broken" padlock a "Warning - Contains HTTP scripts" tooltip when dealing with mixed content specifically, if you want.

Looks good and straightforward. ~~If you want to add a tiny bit more of a "this is not a great state to be in" vibe, you could add a "⚠️" to the padlock, but it's already pretty obviously "not perfect" as-is.~~ Actually, the more I think about it, the more abrasive the warning triangle feels. Is adding the "mixed passive" padlock gonna be a breaking change for themes, tho? Or will there be a "low" or "broken" state fallback? (Hopefully most themes still combine `level=low` and `level=mixed` at the very least, but it's not guaranteed. Cleanliness should probably come before backward compatibility, though, I suppose.) And I'm guessing it'll get its own "Low security (HTTP Files)"-esque tooltip message, too? It's also possible to give the "broken" padlock a "Warning - Contains HTTP scripts" tooltip when dealing with mixed content specifically, if you want.

Sorry, but do you really think that most users know the difference between all the listed cases, and even more so that someone is able to remember what each icon and color mean? Perhaps it should be done in less detail, or somehow have a link to a page with a detailed explanation?

When you click on the identity panel, the identity popup already has a line that says "your connection to this site is only partially encrypted".

Sorry, but do you really think that most users know the difference between all the listed cases

They don't have to - there are tooltips and details if clicked. Not sure why you are criticizing this since it's not all that different from what we've always had, the only difference being the more clearly indicated "mixed content" instead of just defaulting to "broken" in that case.

I listed all the cases because I want to be thorough to cover all possible cases there can be as part of my testing.

> Sorry, but do you really think that most users know the difference between all the listed cases They don't have to - there are tooltips and details if clicked. Not sure why you are criticizing this since it's not all that different from what we've always had, the only difference being the more clearly indicated "mixed content" instead of just defaulting to "broken" in that case. *I* listed all the cases because I want to be thorough to cover all possible cases there *can* be as part of my testing.

@Lootyhoof There are some changes here that might affect full themes (primarily the identity panel styling, since the padlock code is theme-independent), although I don't know if "mixed" and/or "low" was already adopted because we did have that before.

The test extension I made for the last round of changes has been updated to match 2771b99 (minus the theme changes). While it'll seem broken when using the default theme, it should allow testing of other themes with the currently proposed changes. Pretty sure it should only be used on Windows at present 'cause of the way browser.js is assembled.

The [test extension](https://realityripple.com/Software/Mozilla-Extensions/1717/) I made for the last round of changes has been updated to match 2771b99 (minus the theme changes). While it'll seem broken when using the default theme, it should allow testing of other themes with the currently proposed changes. Pretty sure it should only be used on Windows at present 'cause of the way browser.js is assembled.

The theme changes aren't breaking (worst case scenario is that the urlbar isn't styled for particular new security levels) but obviously do need updating.

@FranklinDM FYI for you too.

The theme changes aren't breaking (worst case scenario is that the urlbar isn't styled for particular new security levels) but obviously do need updating. @FranklinDM FYI for you too.

While I was doing some extension work last night, I noticed something about browser.js (which was only minimally modified here) that kind of concerns me. There are quite a few If statements of the style if (aValue !== undefined) {. Correct me if I'm wrong, but isn't that a bad way to check for undefined? I'm pretty sure strict equality comparisons for undefined don't work that way in JS and need to be written as if (typeof aValue !== 'undefined') { instead, right?

While I was doing some extension work last night, I noticed something about browser.js (which was only minimally modified here) that kind of concerns me. There are quite a few If statements of the style `if (aValue !== undefined) {`. Correct me if I'm wrong, but isn't that a bad way to check for **undefined**? I'm pretty sure strict equality comparisons for **undefined** don't work that way in JS and need to be written as `if (typeof aValue !== 'undefined') {` instead, right?

Please don't tack unrelated questions/observations onto issues.
And no, while it would be "more correct" to use typeof, JS's syntax parsing is anything but strict because that's how people like it. == undefined is fine.

Please don't tack unrelated questions/observations onto issues. And no, while it would be "more correct" to use typeof, JS's syntax parsing is anything but strict because that's how people like it. `== undefined` is fine.

Yeah, sorry about being off-topic. And I know the abstract equality check works with undefined, but I think the strict equality checks are... less functional.

No it isn't.
You can't redefine undefined to anything else (which would be possible before ES5, like... a decade ago) so it's not possible to misinterpret this in any way, least of all in static internal chrome scripting.

No it isn't. You can't redefine `undefined` to anything else (which would be possible before ES5, like... a decade ago) so it's not possible to misinterpret this in any way, least of all in static internal chrome scripting.

Alright. And I guess if any variables are actually not defined, a reference error in the log would be a good thing. Sorry for the false alarm.

Since this is live with no notable reports of regressions, I'm closing this.

Labels Milestones

Rewrite the Pale Moon padlock code #1717