Closed Bug 1057029 Opened 7 years ago Closed 7 years ago

update certificate exception handling in chat to deal with bug 940506

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: clokep)

References

Details

(Whiteboard: [1.6-blocking])

Attachments

(2 files, 1 obsolete file)

+++ 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.
Blocks: cc-backlog
Whiteboard: [1.6-blocking]
Assignee: nobody → clokep
Attached patch c-c part v1 (obsolete) — Splinter Review
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)
Attached patch purple part v1Splinter Review
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+
Attached patch c-c part v2Splinter Review
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+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/fa4b68528277
http://hg.mozilla.org/users/florian_queze.net/purple/rev/29ed2e68dde1
Status: NEW → RESOLVED
Closed: 7 years ago
Component: Instant Messaging → General
Keywords: checkin-needed
Product: Thunderbird → Chat Core
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Version: 33 → trunk
No longer blocks: cc-backlog
You need to log in before you can comment on or make changes to this bug.