update certificate exception handling in chat to deal with bug 940506

RESOLVED FIXED in 1.6

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mkmelin, Assigned: clokep)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1.6-blocking])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1046328 +++

Bug 940506 removed the nsIRecentBadCerts interface and implementation, which means that the old way of adding certificate exceptions do not work. I thought there was a bug on this already, but I guess not. What needs to happen is everywhere Thunderbird opens the certificate exception override dialog, it needs to pass along the nsISSLStatus from the connection that failed. See for reference the patch in bug 1025332.

---

In bug 1046328 I'm fixing it for most of thunderbird, but I'll leave just a fixme comment for chat, as the status needed isn't (at least easily?) accessible there.

Updated

4 years ago
Blocks: 1054354
Whiteboard: [1.6-blocking]
(Assignee)

Updated

4 years ago
Assignee: nobody → clokep
(Assignee)

Comment 1

4 years ago
Created attachment 8478744 [details] [diff] [review]
c-c part v1

Magnus, thanks for filing this.

Florian, I tested this using an IRC account to irc.oftc.net:6697.

I'm not really positive about the libpurple part and would appreciate feedback!
Attachment #8478744 - Flags: review?(florian)
(Assignee)

Comment 2

4 years ago
Created attachment 8478745 [details] [diff] [review]
purple part v1
Attachment #8478745 - Flags: review?(florian)
Attachment #8478745 - Flags: review?(florian) → review+
Comment on attachment 8478744 [details] [diff] [review]
c-c part v1

Review of attachment 8478744 [details] [diff] [review]:
-----------------------------------------------------------------

r- for never clearing _sslStatus, but this looks good :-).

::: chat/components/public/imIAccount.idl
@@ +147,5 @@
>    /* When a certificate error occurs, the host/port that caused a
>     * SSL/certificate error when connecting to it. This is only valid when
>     * connectionErrorReason is one of ERROR_CERT_*. */
>    readonly attribute AUTF8String connectionTarget;
> +  /* When a certificate error occurs, the nsISSLStatus error of the socket. This

nit: Looks like the other attributes have an empty line before their comment.

::: chat/modules/jsProtoHelper.jsm
@@ +60,5 @@
>  
>      if (aIsSslError)
>        return Ci.prplIAccount.ERROR_ENCRYPTION_ERROR;
>  
> +    this._sslStatus = aSocket.sslStatus;

I wonder if I wouldn't prefer:
let sslStatus = this._sslStatus = aSocket.sslStatus;

I know the .sslStatus getter does almost nothing, but we are calling it several times for no real reason :-/.

@@ +87,5 @@
>      return Ci.prplIAccount.ERROR_CERT_OTHER_ERROR;
>    },
>    _connectionTarget: "",
>    get connectionTarget() this._connectionTarget,
> +  _sslStatus: null,

Should we clear _connectionTarget and _sslStatus in the reportConnecting method?

::: im/content/accounts.js
@@ +208,5 @@
>      }
>    },
>    addException: function am_addException() {
>      let account = this.accountList.selectedItem.account;
>      if (!account.disconnected || !account.prplAccount.connectionTarget)

Should we add a |let prplAccount = account.prplAccount;| here to avoid calling this getter 3 times?
Attachment #8478744 - Flags: review?(florian)
Attachment #8478744 - Flags: review-
Attachment #8478744 - Flags: feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 8478985 [details] [diff] [review]
c-c part v2
Attachment #8478744 - Attachment is obsolete: true
Attachment #8478985 - Flags: review?(florian)
Comment on attachment 8478985 [details] [diff] [review]
c-c part v2

Thanks!
Attachment #8478985 - Flags: review?(florian) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/comm-central/rev/fa4b68528277
http://hg.mozilla.org/users/florian_queze.net/purple/rev/29ed2e68dde1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Component: Instant Messaging → General
Keywords: checkin-needed
Product: Thunderbird → Chat Core
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Version: 33 → trunk

Updated

4 years ago
No longer blocks: 1054354
You need to log in before you can comment on or make changes to this bug.