283521 - Add security icon/button to the Chatzilla status bar.

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, enhancement)

Description

[Justin Turner Arthur]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050219
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050219

With bug 174087 (IRC over SSL support) fixed, it would be nice to have the
security lock icon from the browser available in the status bar of chatzilla to
denote the security status of the current Chatzilla view. It should look exactly
like the browser's security icon if the user is running Chatzilla on the Suite
or Firefox. It would also be nice to give it tooltip text showing the signer and
have it clickable to invoke a certificate info dialog box.

Reproducible: Always

Steps to Reproduce:

Comment 1

[:Gijs (out for now; he/him)]

Taking this bug. Thanks a bundle (in advance) to Justin Arthur, Christian
Biesinger and Darin Fisher for helping me out when looking for the proper
interface on the transport variable. I'll be attaching a patch as soon as I have
enough time to properly fix all of this.

Comment 2

[:Gijs (out for now; he/him)]

Small explanation as to why this is taking a bit longer than I expected:
Seamonkey and Firefox both have a different CSS used for styling this icon
(navigator.css vs. browser.css) and viewing the certificate requires just a left
click in Seamonkey while it needs a double-click in Firefox. 

Since we want to mimic this behaviour to the best possible extent, I'm currently
trying to figure out if and how I can set what css to use during runtime (on a
XUL window!). Which is a pain, but I'll keep trying ;-).

Comment 3

[:Gijs (out for now; he/him)]

Attachment: Patch v1

Prototype patch. I think we need XULRunner fixed (at the moment, it's bound not
to work), does anyone know what css I could use for that? Should we add our
own?

Asking review to see if the general approach I'm taking is okay :-).

Comment 4

[James Ross]

Comment on attachment 178577 [details] [diff] [review]
Patch v1

I know this has been marked obsolete, but I want to comment on some things
anyway.

>Index: mozilla/extensions/irc/js/lib/irc.js

>     this.parent.users = this.users;
>     e.destObject = this.parent;
>     e.set = "network";
>+

The new code should go before the destObject/set lines - they are traditionally
at the end.

>+    const wpl = Components.interfaces.nsIWebProgressListener;
>+    if (!this.isSecure)
>+    {
>+        this.securityStatus = wpl.STATE_IS_INSECURE;
>+        this.securityTooltip = MSG_SECURITY_INFO;
>+        this.connection.cert = null;

We should not be using WPL constants in the library; heck, Components may not
even exist.

Also, the IRC library should never have UI-related code (tooltip).

>+        updateSecurityIcon();
>+        return;
>+    }
>+    /* We're on a secure server, store Certificate for later use. We can only
>+     * do this when fully connected, so we do it here. Every IRC server is
>+     * supposed to send a 001, so we should always be able to set it :-)
>+     */

If we don't get a 001, NOTHING will work. ;)

>+    try
>+    {
>+        var sslSp = Components.interfaces.nsISSLStatusProvider;
>+        var sslStatus = Components.interfaces.nsISSLStatus;
>+
>+        // Get the actual SSL Status
>+        sslSp = this.connection._transport.securityInfo.QueryInterface(sslSp);
>+        sslStatus = sslSp.SSLStatus.QueryInterface(sslStatus);

Ick. You should never need to use an _ property unless you are part of the
implementation (in this case, you must be in connection*.js to use _transport).

My suggestion is to add a method to CBSConnection to get the different things
you want below, and probably make connection-rhino.js have impls. that return
null/0/whatever instead. So, we'd probably want something like...

  getSecurityState(), which can return:
    0	unknown
    1	insecure
    2	broken
    3	secure

  getSecurityStrenth()
    0  unknown or no security
    1  low security
    2  med security
    3  high security

  getSecurityCertificate()
    returns the actual cert.

>+
>+        // Store certificate and appropriate tooltip
>+        this.connection.cert = sslStatus.serverCert;
>+        this.securityTooltip = getMsg(MSG_SECURE_CONNECTION,
>+                                      this.connection.cert.issuerOrganization);
>+
>+        // Store appropriate status
>+        if (!("keyLength" in sslStatus) || !sslStatus.keyLength)
>+            this.securityStatus = wpl.STATE_IS_BROKEN;
>+        else if (sslStatus.keyLength >= 90)
>+            this.securityStatus = wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH;
>+        else
>+            this.securityStatus = wpl.STATE_IS_SECURE | wpl.STATE_SECURE_LOW;
>+    }
>+    catch (ex)
>+    {
>+        // Break means broken, set correct values:
>+        dd("Exception getting certificate stuff: " + ex.message);
>+        this.securityStatus = wpl.STATE_IS_BROKEN;
>+        this.securityTooltip = MSG_SECURITY_INFO;
>+        this.connection.cert = null;
>+    }
>+    updateSecurityIcon();


>Index: mozilla/extensions/irc/js/lib/utils.js

>+function viewCert(cert, parent)
>+{
>+    const nsICertificateDialogs = Components.interfaces.nsICertificateDialogs;
>+    const nsCertificateDialogs = "@mozilla.org/nsCertificateDialogs;1"
>+    var cd = getService(nsCertificateDialogs, nsICertificateDialogs);

At least use the shorter form!

    var cd = getService("@mozilla.org/nsCertificateDialogs;1",
"nsICertificateDialogs");

>+
>+    if (!parent)
>+        parent = window;
>+    cd.viewCert(parent, cert);
>+}
>+


>Index: mozilla/extensions/irc/xul/content/handlers.js

>+    // Correct security Values
>+    const wpl = Components.interfaces.nsIWebProgressListener;
>+    e.server.securityStatus = wpl.STATE_IS_INSECURE;
>+    e.server.securityTooltip = MSG_SECURITY_INFO;
>+

It would probably be better to do all the security state changes in the IRC
library - remeber that can be used without the CZ UI. Then, you can send a new
event (e.g. onSecurityChange) to the network so that the FE code can update the
statusbar/tooltip.

>     dispatch("sync-header");
>     updateTitle();
>     updateProgress();
>+    updateSecurityIcon();
>+


>Index: mozilla/extensions/irc/xul/content/static.js

>+    // Get the appropriate css in to style the security icon
>+    var attribs, processingInstruction, windowElem
>+    attribs = " type=\"text/css\"";
>+    if (client.host == "Mozilla")
>+        attribs = "href=\"chrome://navigator/skin/navigator.css\"" + attribs;
>+    else if (client.host == "Firefox")
>+        attribs = "href=\"chrome://browser/skin/browser.css\"" + attribs;
>+    else if (client.host == "XULRunner")
>+        return; // Unless anyone knows what we can use for XULRunner?

Returning early for XULRunner is a bad idea; nest the code below in a if
(secCSSUrl), and use that above for simplicity. THEN construct the PI inside
the if.

>+    processingInstruction = document.createProcessingInstruction(
>+                            "xml-stylesheet", attribs);
>+    windowElem = document.getElementById("chatzilla-window");
>+    document.insertBefore(processingInstruction, windowElem);
>+
>+    // Copy behaviour of security-button from browser
>+    var eventhandler = "if (event.button == 0) displayCertificateInfo();";
>+    var securityButton = document.getElementById("security-button");
>+    if (client.host == "Firefox")
>+    {
>+        securityButton.setAttribute("ondblclick", eventhandler);
>+        securityButton.setAttribute("class", "statusbarpanel-iconic-text");
>+    }
>+    else if (client.host == "Mozilla")
>+    {
>+        securityButton.setAttribute("class", "statusbarpanel-iconic");
>+        securityButton.setAttribute("oncommand", "displayCertificateInfo();");
>+    }

I wonder if we can crash an old Mozilla with this stuff... :)

>+function updateSecurityIcon()
>+{
>+    var o = getObjectDetails (client.currentObject);
>+    const wpl = Components.interfaces.nsIWebProgressListener;
>+    var securityButton = window.document.getElementById("security-button");
>+
>+    if (!o.server || !o.server.securityStatus)
>+    {
>+        // No server or security Status
>+        securityButton.removeAttribute("level");
>+        securityButton.setAttribute("tooltiptext", MSG_SECURITY_INFO);
>+        if (client.host == "Firefox")
>+            securityButton.removeAttribute("label");
>+        return;
>+    }
>+

This next switch would be better if you used a security interface on the
server; you could actually use the interface I wrote out for the connection,
and make the server not do anything but pass through the calls...

>+    switch (o.server.securityStatus) 
>+    {
>+        case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH:
>+            securityButton.setAttribute("level", "high");
>+            break;
>+        case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_LOW:
>+            securityButton.setAttribute("level", "low");
>+            break;
>+        case wpl.STATE_IS_BROKEN:
>+            securityButton.setAttribute("level", "broken");
>+            break;
>+        case wpl.STATE_IS_INSECURE:
>+        default:
>+            securityButton.removeAttribute("level");
>+            break;
>+    }
>+    securityButton.setAttribute("tooltiptext", o.server.securityTooltip);

Clearly this is v.bad. :)  Need to pick the tooltip *here*.

>+
>+    // Set click / dblclick events, and label if appropriate
>+    var serverSecure = o.server.securityStatus & wpl.STATE_IS_SECURE
>+    if (client.host == "Firefox" && serverSecure)
>+        securityButton.setAttribute("label", o.server.hostname);

Comment seems to be out-of-date... and I think it would be better to use some
of FF's behaviour globally - like showing the hostname.

>+function displayCertificateInfo()
>+{
>+    var o = getObjectDetails (client.currentObject);
>+    if (!o.server)
>+        return;
>+
>+    if (!o.server.isSecure)
>+    {
>+        display(getMsg(MSG_INSECURE_SERVER, o.server.hostname), MT_INFO);
>+        return;
>+    }
>+
>+    viewCert(o.server.connection.cert);
>+}

It somehow seems wrong to use display for failing when success opens a
dialog... oh well.

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Patch v2

I think this fixes everything mentioned by Silver, but I'm feeling drowsy and
I've already been spending way too much time on this thing, from stupid XUL
bugs (tooltips suck, let me tell you that much) to incompatibility (XBL is well
hidden and I still don't get why people use it, but anyway) to Fx trunk not
taking in extensions. Yay.

Anyhow, this should have separated UI vs. underlying mechanisms. No XPCom
constants used anymore. Works on Moz 1.0, Moz trunk, XULRunner trunk, Fx 1.0.3,
and it ought to work on Fx trunk, but that failed to install it so I stopped
bothering.

Comment 6

[:Gijs (out for now; he/him)]

For reference: there are icons added to mozilla/extensions/irc/xul/skin/images
called 'secure.png' and 'security-broken.png' (both full-lowercase, like the
rest of that directory), they are copied from:

http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/Secure.png
http://lxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/Security-broken.png

Comment 7

[James Ross]

Comment on attachment 183187 [details] [diff] [review]
Patch v2

>? mozilla/extensions/irc/xpi/log.tmp
>? mozilla/extensions/irc/xul/skin/images/secure.png
>? mozilla/extensions/irc/xul/skin/images/security-broken.png
>Index: mozilla/extensions/irc/jar.mn

>+	skin/modern/chatzilla/images/secure.png         (xul/skin/images/secure.png)
>+	skin/modern/chatzilla/images/security-broken.png         (xul/skin/images/security-broken.png)

The naming is inconsistent here. Could you maybe fix it?



>Index: mozilla/extensions/irc/js/lib/connection-xpcom.js

>+// Security Constants. 
>+const STATE_IS_INSECURE = 4
>+const STATE_IS_BROKEN = 1
>+const STATE_IS_SECURE = 2
>+
>+const STATE_SECURE_HIGH = 262144
>+const STATE_SECURE_LOW = 131072

Hmm. Could you use hex notation (0x...) so that these make some kind of sense?
Preferably do it for all the constants. Actually, alternatively, make both be
single digits (1, 2, 3 only) - see later for why.

>+CBSConnection.prototype.getSecurityState =
>+function bc_getsecuritystate()
>+{
>+    if (!this.isConnected || !this._transport.securityInfo)
>+        return STATE_IS_INSECURE;
>+
>+    try
>+    {
>+        var sslSp = Components.interfaces.nsISSLStatusProvider;
>+        var sslStatus = Components.interfaces.nsISSLStatus;
>+
>+        // Get the actual SSL Status
>+        sslSp = this._transport.securityInfo.QueryInterface(sslSp);
>+        sslStatus = sslSp.SSLStatus.QueryInterface(sslStatus);
>+        // Store appropriate status
>+        if (!("keyLength" in sslStatus) || !sslStatus.keyLength)
>+            return STATE_IS_BROKEN;
>+        else if (sslStatus.keyLength >= 90)
>+            return (STATE_IS_SECURE | STATE_SECURE_HIGH);
>+        else
>+            return (STATE_IS_SECURE | STATE_SECURE_LOW);

Would it make sense to return an array?
  [STATE_IS_BROKEN]
  [STATE_IS_SECURE, STATE_SECURE_LOW]

Also, shouldn't we have a medium too?

>+    }
>+    catch (ex)
>+    {
>+        // Something goes wrong -> broken security icon
>+        dd("Exception getting certificate for connection: " + ex.message);
>+        return STATE_IS_BROKEN;
>+    }
>+}



>Index: mozilla/extensions/irc/xul/content/menus.xul

>+      <statusbarpanel id="security-button" ondblclick="if (event.button == 0) displayCertificateInfo();">
>+        <label id="security-button-label"/>
>+        <image id="security-button-image"/>
>+      </statusbarpanel>

I feel the even hook should be in handlers.js, but it's not like all the code
is that good. :)



>Index: mozilla/extensions/irc/xul/content/static.js

>+function updateSecurityIcon()
>+{
>+    var o = getObjectDetails (client.currentObject);

Nit: no space before parmeter list.

>+    var securityButton = window.document.getElementById("security-button");
>+    securityButton.removeAttribute("level");
>+    securityButton.removeAttribute("tooltiptext");
>+    if (!o.server || !o.server.isConnected) // No server or connection?
>+    {
>+        securityButton.setAttribute("tooltiptext", MSG_SECURITY_INFO);
>+        return;
>+    }
>+
>+    var securityState = o.server.connection.getSecurityState()

Now, here we go... I really dislike this switch. The main problem is this: if
we are secure, but at a new level, what happens? We get the insecure one. If
you want to keep this bitmask thing (rather than switching to an array/object),
please do it like this:

  if (securityState & STATE_IS_SECURE)
  {
      ...

So that we always have an else for secure, but unknown level, and what-not.

If you go with an array/object, this will also be nessessary.

>+    switch (securityState) 
>+    {
>+        case STATE_IS_SECURE | STATE_SECURE_HIGH:
>+            securityButton.setAttribute("level", "high");
>+            securityButton.firstChild.value = o.server.hostname;
>+            break;
>+        case STATE_IS_SECURE | STATE_SECURE_LOW:
>+            securityButton.setAttribute("level", "low");
>+            securityButton.firstChild.value = o.server.hostname;
>+            break;
>+        case STATE_IS_BROKEN:
>+            securityButton.setAttribute("level", "broken");
>+            // No break to make sure we get the correct tooltip
>+        case STATE_IS_INSECURE:
>+        default:
>+            securityButton.setAttribute("tooltiptext", MSG_SECURITY_INFO);
>+    }
>+
>+    if ((securityState & STATE_IS_SECURE) == STATE_IS_SECURE)

The first use of the constant should be a mask constant, if you compare to a
single bit flag.

>+    {
>+        var issuer = o.server.connection.getCertificate().issuerOrganization;
>+        var tooltiptext = getMsg(MSG_SECURE_CONNECTION, issuer);
>+        securityButton.setAttribute("tooltiptext", tooltiptext);
>+        securityButton.firstChild.setAttribute("tooltiptext", tooltiptext);
>+        securityButton.lastChild.setAttribute("tooltiptext", tooltiptext);
>+    }
>+}

Also, let me get this right... this code will /always/ display the security
box? That seems wrong...


>+function displayCertificateInfo()
>+{
>+    var o = getObjectDetails (client.currentObject);

Nit: again. :)

>+    if (!o.server)
>+        return;
>+
>+    if (!o.server.isSecure)
>+    {
>+        alert(getMsg(MSG_INSECURE_SERVER, o.server.hostname));
>+        return;
>+    }
>+
>+    viewCert(o.server.connection.getCertificate());
>+}



>Index: mozilla/extensions/irc/xul/skin/chatzilla.css

>+/* Hack; Stop the status-bar from distorting without a security icon */
>+#status-text {
>+    min-height: 17px;
>+}
>+
>+#security-button {
>+    min-width: 20px;
>+    display: none;
>+}
>+
>+#security-button[level="high"], #security-button[level="low"], 
>+#security-button[level="broken"] {
>+    display: -moz-box;
>+}

*cough*

Can we do this the "nice" way, rather than thinking we know what display is
'right'?

#security-button:not([level="high"]):not([level="low"]):not([level="broken"]) {
    display: none;
}


>+
>+#security-button[level="high"] > image {
>+    list-style-image: url("chrome://chatzilla/skin/images/secure.png");
>+}
>+
>+#security-button[level="low"] > image {
>+    list-style-image: url("chrome://chatzilla/skin/images/secure.png");
>+}
>+
>+#security-button[level="broken"] > image {
>+    list-style-image: url("chrome://chatzilla/skin/images/security-broken.png");
>+}
>+
>+/* prevent margins of a value-less label from shifting the image */
>+#security-button > label:not([value]) {
>+    display: none;
>+}

See! You used :not() here! ;)

Comment 8

[:Gijs (out for now; he/him)]

Attachment: Patch with James' criticism fixed (should be)

Fixes the nits, uses hex values for the constants, still no medium constant
because Mozilla doesn't use it either (check lxr for it, or in Silver's case,
your logs, as we discussed it previously), ondblclick handler now in
handlers.js, passing arrays to updateSecurityIcon instead of added-up
constants, which simplifies, clarifies and robustifies (if that's a word) the
security checking's logic. I've also fixed (as far as I can see) the criticism
on the css used.

Setting r? again.

Comment 9

[James Ross]

Comment on attachment 185301 [details] [diff] [review]
Patch with James' criticism fixed (should be)

r=silver with the stuff below addressed (don't need to bother with another
patch, they're all trivial changes).


>Index: mozilla/extensions/irc/js/lib/connection-xpcom.js

>+// Security Constants. 
>+const STATE_IS_BROKEN = 0x00000001;
>+const STATE_IS_SECURE = 0x00000002;
>+const STATE_IS_INSECURE = 0x00000004;
>+
>+const STATE_SECURE_LOW = 0x00020000;
>+const STATE_SECURE_HIGH = 0x00040000;

Since these are separate now (returned as separate items in the array) they
don't need to be in different bit positions. Do they even need to be bit flags?
Not from what I can see.... just give the items the values 1, 2, 3, 1, 2
respectively.

>+CBSConnection.prototype.getSecurityState =
>+function bc_getsecuritystate()

We could do with a comment above this explaining what it returns (array with at
least one item, and if the first item is STATE_IS_SECURE you get the security
level in a 2nd item).


>Index: mozilla/extensions/irc/xul/locale/en-US/chatzilla.properties

>+msg.insecure.server   = Your connection to the server ``%S'' is not secure.
>+msg.secure.connection = Signed by %S

This needs a " on the end (sorry I missed this before).

>+msg.security.info     = Displays security information about the current connection

Comment 10

[:Gijs (out for now; he/him)]

Comment on attachment 185301 [details] [diff] [review]
Patch with James' criticism fixed (should be)

Requesting approval for yet another ChatZilla patch.

Comment 11

[James Ross]

Checked in --> FIXED.

Comment 12

[Simo Melenius]

Attachment: Nevermind, my apologies. I was sending a patch to #289851, I don't know where I got to the patch submission page for #283521 (wasn't even viewing this bug).