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.

This should fix it for /mail. chat looks to require more work, so I'll leave that for the chat people. I used the alexdpsg.net case to test a mail account. For account config I had to move the clearValidityOverride for the temporary cert to after success, otherwise we get into an infinite loop, probably because the cert remembering is temporary and the next try gets a new connection which that temporariness doesn't cover. I don't know if connection closing changed, or if this never worked properly for that case. So, enter foo@alexdpsg.net, nothing is found, then manually set imap.alexdpsg.net ssl smtp.gmail.com ssl And yes, that we don't set the found incoming server because the outgoing server can't be reached is another bug...

Comment on attachment 8470464 [details] [diff] [review] bug1046328_cert_ex_update__for_tb.patch Review of attachment 8470464 [details] [diff] [review]: ----------------------------------------------------------------- This looks great - thanks for working on this!

Comment on attachment 8470464 [details] [diff] [review] bug1046328_cert_ex_update__for_tb.patch Review of attachment 8470464 [details] [diff] [review]: ----------------------------------------------------------------- It all looks fine to me. You _might_ want to run the changes by BenB before checking them in, since he's more familiar with the guessConfig/verifyConfig code. Thanks, Blake.

Please keep changes minimal. Most of your changes are general code cleanup, which are good changes, but have nothing to do with this bug. 1. The guts of this patch seem to be move that you move the clearVailidyOverride() to a different place. You're explaining this in comment 2, but please also make a source code comment. The next person trying to understand or change this code won't read the bugzilla comment. You don't seem to be too sure why that failed or why it works now. Please find out, as this might change what the correct solution is. 2. Why do you remove |if (!status) return;| (2 places)? 3. _gotCertError: If this matters to you, please define in a comment (where the variable is define) what this variable contains. Also, you've been missing at least one place where it's still assigned |false|. 4. FIXME: exceptionDialog.xul should get passed an sslStatus (nsISSLStatus) You're fixing it in another place. Please fix it here, too, if it's necessary. If it isn't, remove the "FIXME". Generally good changes, but I'd prefer the code cleanup and the bugfix to be in separate commits, if possible. It seems from the patch that this wouldn't be too difficult. You could do that after the review, directly before commit, to avoid re-doing the split-up for every review round.

Comment on attachment 8470464 [details] [diff] [review] bug1046328_cert_ex_update__for_tb.patch r-. Good patch, but stuff to fix.

(In reply to Ben Bucksch (:BenB) from comment #5) > Please keep changes minimal. Most of your changes are general code cleanup, > which are good changes, but have nothing to do with this bug. > > 2. Why do you remove |if (!status) return;| (2 places)? I don't think it actually happens, but if it does the exception dialog deals with that situation. On a second though I reverted one of the removals. > 3. _gotCertError: If this matters to you, please define in a comment (where > the variable is define) what this variable contains. Also, you've been > missing at least one place where it's still assigned |false|. > 4. FIXME: exceptionDialog.xul should get passed an sslStatus (nsISSLStatus) > You're fixing it in another place. Please fix it here, too, if it's > necessary. If it isn't, remove the "FIXME". I filed bug 1057029, it's a more complex fix for chat I'm afraid, so I'll leave it for people who knows that code better.

Comment on attachment 8476915 [details] [diff] [review] bug1046328_cert_ex_update__for_tb.patch Review of attachment 8476915 [details] [diff] [review]: ----------------------------------------------------------------- I just noticed this nit while looking over these changes. ::: mailnews/base/prefs/content/accountcreation/guessConfig.js @@ +529,5 @@ > thisTry.status = kSuccess; > + > + if (thisTry.selfSignedCert) { // eh, ERROR_UNTRUSTED or ERROR_TIME > + // We clear the temporary override now after success. If we clear it > + // earlier we get into an infinite loop, probably because the cert Nit: Trailing white space.

https://hg.mozilla.org/comm-central/rev/350ababdb84f -> FIXED

This is fixed for Thunderbird 34 only but status-thunderbird33:affected is set. Should this be ported back to comm-beta?

I don't know if we care enough for betas to uplift it. It has plenty of time to ride the trains until tb38. Feel free to nominate if you feel it's important.