Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Mail Account Setup doesn't detect service on smtps (465)

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Account Manager
P1
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoshiaki Kasahara, Assigned: Irving)

Tracking

({regression})

12 Branch
Thunderbird 17.0
regression

Thunderbird Tracking Flags

(thunderbird15-, thunderbird16+)

Details

(Whiteboard: Probably caused by bug 721369)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120515 Firefox/14.0a2
Build ID: 20120515042006

Steps to reproduce:

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120516 Thunderbird/14.0a2

I noticed that "Mail Account Setup" wizard cannot detect service running on port 465 with SSL/TLS.  I found the problem with version 12.0.1 (for Windows and Mac).  I tried Earlybird (14.0a2) for Windows but it was still broken.

I did the following steps:
(Assume an (internal) SMTP server running services on standard port 25/465/587)

* Select Account Settings -> Account Actions -> Add Mail Account
* Enter a name and Email address, and click "Continue"
* Thunderbird found configuration by trying common server names
* Click "Manual config" and change outgoing SSL setting from "STARTTLS" to "SSL/TLS"
  (The port number was automatically changed from 587 to 465)
* Click "Re-test"


Actual results:

Thunderbird reported "Thunderbird failed to find the settings for your email account".

If I reverted outgoing SSL setting to "STARTTLS" and clicked "Re-test", Thunderbird reported "The following settings were found by probing the given server".

I'm sure that the server is configured correctly, because I could manually edit "Outgoing Server (SMTP)" in Account Settings to use "SSL/TLS" for "Connection security" and send messages without problems.


Expected results:

Thunderbird should be able to find service on port 465 with SSL/TLS by probing the given server. Actually, version 9.0.1 worked as expected, so it is a regression.

I captured packets between the client and server using wireshark, and noticed that Thunderbird didn't send SSL client hello to port 465 after 3-way handshake, and just closed the connection.  imaps (993) and pop3s (995) are detected properly. I guess something is wrong with the service detection logic.
Keywords: regression

Comment 1

5 years ago
We definitely do try to find servers on port 465. (This doesn't exclude the possibility of a bug.)

Does the server send the same certificate on both ports? We may ignore any servers with wrong certs, and servers can have different configurations on the 2 ports, so that is a possibility.

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

5 years ago
CONFIRMED. I tried 3-4 ISPs that, according to our database, have SMTP servers on port 465, but when I changed pref mailnews.auto_config_url and ran guess config, it never found port 465, only STARTLS on port 587 or 25. Even manually setting the server to port 465 and SSL and clicking "Re-Test" failed to find the server.

bsmith, could this be a regression in SSL?

Comment 3

5 years ago
This is definitely a regression. I tried a TB trunk from 2012-03-31, and it worked. The TB trunk from 2012-04-25 failed. So, it's a regression in that timeframe.

Bug 745950 falls in that time-frame, and changes code relevant to this, so could well be the culprit.

Reproduction:
1. Edit | Preferences... | Advanced | General | Config Editor ...
   Set pref "mailnews.auto_config_url" to "http://domain.invalid/" to force guess config
1. File | New | Existing Mail Account...
2. Realname: nonsense, Email address: nonsense@laposte.net , password: nonsense
   (Be sure to use @laposte.net as domain, because they have an SSL SMTP server at port 465,
   but no STARTTLS at SMTP.)
3. Continue and wait
4. Manual config

Expected result:
SMTP: smtp.laposte.net, port 465, SSL/TLS

Actual result:
SMTP: smtp.laposte.net, port 465, None (No encryption
Blocks: 745950
Keywords: regressionwindow-wanted

Updated

5 years ago
Keywords: regressionwindow-wanted

Comment 4

5 years ago
Sorry, I meant bug 721369
Blocks: 721369
No longer blocks: 745950

Comment 5

5 years ago
> SMTP: smtp.laposte.net, port 465, None (No encryption)

And here I meant "..., port 587, None", of course. Sorry.

Updated

5 years ago
Severity: normal → major
Priority: -- → P1

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 14 → 12

Updated

5 years ago
Duplicate of this bug: 781253

Comment 7

5 years ago
As said, this is likely caused by bug 721369.
This bug is causing quite some grief. If somebody could investigate and fix this, this would be appreciated.
Whiteboard: Probably caused by bug 721369
tracking-thunderbird15: --- → ?
Assignee: nobody → irving
Created attachment 653920 [details] [diff] [review]
Handle both number and string values for encryption type code

It won't be easy to stop swearing at JavaScript for long enough to write this up...

The problem is a combination of two things. First, https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe for SSL support when we were dealing with a socket where the encryption type is UNKNOWN (-1).

Second, the switch statement at https://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/guessConfig.js#867 uses (under the hood) JavaScript's === comparator, which does NOT coerce strings to numbers. That means if you pass the number 2 in, you get back SSL, but if you pass in the string "2" you get back UNKNOWN

Third, when we read from our ISP database we get the number for the encryption type, and we initialize the manual settings UI with those numbers, and as long as the user doesn't change anything they stay numbers. BUT, if the user make a selection on the encryption type UI element, the value gets changed to a string.

So, if the user explicitly selects an encryption type of SSL in the manual settings UI, it gets converted to UNKNOWN by the JS switch() statement, and the UNKNOWN encryption type no longer probes for SSL support, so our config test fails.


Patch changes the switch() to a series of if statements so that string/integer coercion works and we correctly try SSL when the user explicitly chooses that in the UI.
Attachment #653920 - Flags: review?(mbanner)
Attachment #653920 - Flags: review?(ben.bucksch)

Comment 9

5 years ago
Comment on attachment 653920 [details] [diff] [review]
Handle both number and string values for encryption type code

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

Thanks, irving, for looking into this!

> https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> for SSL support when we were dealing with a socket where the encryption type is
> UNKNOWN (-1).

That looks wrong to me, even though I wrote the patch.
Yet, I am not in the habit of throwing in useless changes. I wonder why I did that. Could you check in the comments of bug 721369 why we did that?

Speaking of unneeded changes: Please make the patch minimal, removing all changes that are not strictly necessary to fix this bug. If you feel that other changes are useful, please attach them separately, one per purose (e.g. one only logging statements, but I don't think we need all these).

> BUT, if the user make a selection on the encryption type UI element,
> the value gets changed to a string.

Yeah, that's a bug. Please fix this part as well, though: Both 1) the ConvertSocketTypeToSSL() (switch vs. if|, I agree in preferring |if|), as well as 2) what the UI returns: We should never have "2" in that enum variable in the first place, that's already the bug, not just ConvertSocketTypeToSSL().

Thanks a lot again for fixing this! I can see how horrible that was to debug. It is a hideous bug.
Attachment #653920 - Flags: review?(ben.bucksch) → review-

Comment 10

5 years ago
The real problem is having any of this Wizard code.

Every mail setup can be customized in many ways, making Wizard design complex
and as we've seen... many times broken so badly Thunderbird has to be rolled back
to 2.0.0.24 just to configure new accounts.

Please add a Manual Config button to the first panel of the Wizard which allows bypassing
the Wizard and going directly to the "Settings" for a given account ala the 2.0.0.24 way of
doing setups.
Created attachment 654253 [details] [diff] [review]
Additional logging to help diagnose the issue
Attachment #654253 - Flags: review?(ben.bucksch)
Created attachment 654254 [details] [diff] [review]
Fix JavaScript strict errors in affected files

Always like to leave things cleaner than I found them...
Attachment #654254 - Flags: review?(ben.bucksch)
Created attachment 654271 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI

The code that copied config out of UI elements converted to integer for the incoming parameters but not for the outgoing. This patch adds parseInt to the outgoing parameters
Attachment #653920 - Attachment is obsolete: true
Attachment #653920 - Flags: review?(mbanner)
Attachment #654271 - Flags: review?(ben.bucksch)
(In reply to Ben Bucksch (:BenB) from comment #9)
> Comment on attachment 653920 [details] [diff] [review]
> Handle both number and string values for encryption type code
> 
> Review of attachment 653920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, irving, for looking into this!
> 
> > https://hg.mozilla.org/comm-central/rev/69177e1f55f4 commented out the probe
> > for SSL support when we were dealing with a socket where the encryption type is
> > UNKNOWN (-1).
> 
> That looks wrong to me, even though I wrote the patch.
> Yet, I am not in the habit of throwing in useless changes. I wonder why I
> did that. Could you check in the comments of bug 721369 why we did that?

The code checked at https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c53 is not the patch attached to the bug; you'll need to look at it yourself to figure out what's up. There are some remarks in https://bugzilla.mozilla.org/show_bug.cgi?id=721369#c51 about trying different combinations of probes, perhaps that is where the commented out lines came from?

Comment 15

5 years ago
Comment on attachment 654271 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI

+      config.outgoing.socketType = parseInt(e("outgoing_ssl").value);
+      config.outgoing.auth = parseInt(e("outgoing_authMethod").value);

Please use
... = santize.integer(e("outgoing_ssl").value);
like the other code.
Attachment #654271 - Flags: review?(ben.bucksch) → review-

Updated

5 years ago
Attachment #654254 - Flags: review?(ben.bucksch) → review+

Comment 16

5 years ago
Comment on attachment 654253 [details] [diff] [review]
Additional logging to help diagnose the issue

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

I can see that this was useful for you during debugging, but I am not sure we need this in the code that we ship.
Particularly the guessConfig changes: I think the cancel/abort is being called for all other in-flight probes when we found a good host. Ditto when the user cancels the probe, you'll get 50 cancel messages, because we probe 50 hosts at the same time.
Attachment #654253 - Flags: review?(ben.bucksch) → review-

Comment 17

5 years ago
(In reply to Ben Bucksch (:BenB) from comment #15)
> Comment on attachment 654271 [details] [diff] [review]
...
> Please use
> ... = santize.integer(e("outgoing_ssl").value);
> like the other code.

With this change: r+

Thanks, irving!
Created attachment 654646 [details] [diff] [review]
Fix JS Strict warnings, updated to not depend on debug patch

Carrying forward r=benb
Attachment #654254 - Attachment is obsolete: true
Attachment #654646 - Flags: review+
Created attachment 654648 [details] [diff] [review]
Handle string/integer differences in the manual configuration UI, using sanitize.integer()

r=benb from https://bugzilla.mozilla.org/show_bug.cgi?id=755988#c17
Attachment #654271 - Attachment is obsolete: true
Attachment #654648 - Flags: review+
Pushed to comm-central:

https://hg.mozilla.org/comm-central/rev/82882c866072
https://hg.mozilla.org/comm-central/rev/115d3d4fa725
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Attachment #654646 - Flags: review+

Updated

5 years ago
Attachment #654648 - Flags: review+

Comment 21

5 years ago
Thanks a lot, irving, for fixing this! Much appreciated.
tracking-thunderbird15: ? → -
tracking-thunderbird16: --- → ?
Target Milestone: --- → Thunderbird 17.0
tracking-thunderbird16: ? → +
You need to log in before you can comment on or make changes to this bug.