419116 - Sending mail through SMTP server that doesn't require user:pass fails

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Phil Ringnalda (:philor)]

Starting with the 2008-02-21-03 Thunderbird trunk nightly, sending email through an SMTP server that doesn't use a username/password fails, claiming that it was "unable to log in to SMTP server smtp.example.com" (though it's clearly something else, since it doesn't require any network activity to be unable to log in). In the same builds, sending through a server that *does* support SMTP-AUTH works just fine.

Unfortunately, I'm going to be gone all weekend so I won't be able to experiment with backing things out, but by far the most likely looking thing between 2008-02-20-03 and 2008-02-21-03 is bug 415034, making it look like somewhere (maybe http://mxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsSmtpDelegateFactory.cpp#79, and why are we stomping on the SetSpec rv rather than checking it?), we are creating a URI with an empty user:pass, rather than with none.

Comment 1

[Phil Ringnalda (:philor)]

Stole enough time to back out bug 415034, which does indeed make things work again.

Comment 2

[Phil Ringnalda (:philor)]

And since bug 415034 already has approval1.8.1.13, it would be super-extra-nice if someone who's home this weekend could build branch Tbird with that patch applied, and see whether we're going to break there, too.

Comment 3

[Mark Banner (:standard8)]

I've just tried this on trunk seamonkey (pulled earlier today), I get this on the console

WARNING: NS_ENSURE_TRUE(port >= 0 && port <= 0xFFFF) failed: file /opt/mozillaextra/master/mozilla/netwerk/base/src/nsSocketTransportService2.cpp, line 465

Here nsMsgProtocol::OpenNetworkSocket calls GetPort() on a URI and gets -1 (i.e. default port), but doesn't do anything with it, e.g. translate it to be non-default.

I'm fairly sure this first part is due to bug 403480.

I tried manually hacking that to set it to 25 (for smtp), but still got the error message as described, but with the following warning this time:

WARNING: NS_ENSURE_TRUE(host && *host) failed: file /opt/mozillaextra/master/mozilla/netwerk/dns/src/nsHostResolver.cpp, line 413

Turns out in nsMsgProtocol::OpenNetworkSocket, GetHost also returns empty for the hostname.

This is due to incorrect mailnews code (I believe), but is shown up by bug 415034. See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/compose/src/nsSmtpService.cpp&rev=1.148&mark=161-163,210-216#159 - when aSmtpUserName is empty, .get() returns "" (the first marked blocked), the second marked block then does !aSmtpUserName (this time its a char*) so in the empty username case we get a url of the form:

smtp://@localhost:25/...

hence this triggers the protection in bug 415034.

I've a patch for this second part, but not for the first yet. SMTP isn't currently registered as a protocol (though it will need to be for password manager), fixing it that way might be the best way to go, but I haven't assessed how long that will take. The other question, is would the nsMsgProtocol::OpenNetworkSocket function affect any of our other mailnews protocols with default ports?

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix (diff -w)

This fixes the second part of the bug that I just described in comment 3. To test this, add "if (port == -1) port = 25;" in nsMsgProtocol::OpenNetworkSocket.

It seemed best to just pass aSmtpHostName and aSmtpUserName as nsCString, thus we could use !IsEmpty() in the check. Additionally I dropped the horrible abuse of getter_Copies for nsEscape and replaced it with the (frozen API compatible) mailnews version.

The urlSpec.get() check was also just useless (urlSpec would never have been empty, and if it was, it'd returned "" which will always return true), so that meant re-indenting half the function, so I decided to tidy up the whole function at the same time.

Attaching diff -u, normal patch in a mo.

Comment 5

[Mark Banner (:standard8)]

Attachment: [checked in] The fix (normal patch)

Comment 6

[[e-mail]]

Comment on attachment 305379 [details] [diff] [review]
The fix (diff -w)

I'm a little worried about the port issue - what happens with SSL?

>+  return smtpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl);
return CallQueryInterface(smtpUrl, aUrl);

Comment 8

[Norm Rowe]

So I wen to edit the smtp and told it to use a user name and smtp over ssl and it will not send. What now?

Comment 9

[Phil Ringnalda (:philor)]

Attachment: The additional hack, v.1

I'm pretty sure Standard8's dead right that the correct answer is to make smtp URLs real, but while we wait for that, I think it would be nice to be able to send mail, and this plus attachment 305379 [details] [diff] [review] seems to give us that.

Comment 10

[Phil Ringnalda (:philor)]

Comment on attachment 305379 [details] [diff] [review]
The fix (diff -w)

r=philringnalda in a "for what little that's worth, since I'm not actually a mailnews peer" sort of way. dmose, though, is eagerly awaiting an r? mail :)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 305664 [details] [diff] [review]
The additional hack, v.1

Looks good; r=dmose.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 305379 [details] [diff] [review]
The fix (diff -w)

r=dmose

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #9)
> Created an attachment (id=305664) [details]
> The additional hack, v.1
> 
> I'm pretty sure Standard8's dead right that the correct answer is to make smtp
> URLs real, but while we wait for that, I think it would be nice to be able to
> send mail, and this plus attachment 305379 [details] [diff] [review] seems to give us that.

I agree this is the right hack for now. I've raised bug 419591 and a series of others to cover the work we need to do for removing this hack once it is committed.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 305380 [details] [diff] [review]
[checked in] The fix (normal patch)

I've checked this in now. It'll change the symptoms a little, but hopefully we can follow it up soon with the second patch.

Comment 15

[[e-mail]]

OK, so with attachment 305380 [details] [diff] [review] I can't actually reproduce comment #3...

Comment 16

[[e-mail]]

Comment on attachment 305664 [details] [diff] [review]
The additional hack, v.1

This is wrong. Attachment 305380 [details] [diff] would work were it not for a typo.

Comment 17

[[e-mail]]

Comment on attachment 305380 [details] [diff] [review]
[checked in] The fix (normal patch)

>-        if (!PL_strchr(aSmtpHostName, ':'))
>-        {
>-            urlSpec.Append(':');
>-            urlSpec.AppendInt(aSmtpPort);
>-        }
>+  if (aSmtpHostName.FindChar(':') != -1)
>+  {
>+    urlSpec.Append(':');
>+    urlSpec.AppendInt(aSmtpPort);
>+  }
So, this is the bit everyone overlooked:
PL_strchr(aSmtpHostName, ':') is null when there is no colon,
so the condition needs to be aSmtpHostName.FindChar(':') == -1

The reason I didn't notice before is that all I did for my sr was to use the debugger to change aSmtpUserName to null and verify that mail was sent.

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> So, this is the bit everyone overlooked:
> PL_strchr(aSmtpHostName, ':') is null when there is no colon,
> so the condition needs to be aSmtpHostName.FindChar(':') == -1

I've now checked in a bustage fix for this and sending SMTP now works correctly. Note that because we don't have an smtp protocol registered, the smtp url still returns a valid port number even in the default case - which is why this bug now works (I'll have to alter the follow-up bugs).

So marking as fixed. Note that TB builds have been respun to incorporate the new fix, the buggy bug fix missed the SM builds.

Comment 21

[Norm Rowe]

Just attempted to use Thunderbird 3.01pre and it added some updates but will not send. I am not a programmer. I am a tester. I have gone back to Thunderbird version 2.0.0.12 (20080213) which does send with the smtp mail box I have been using for over a year. 

Comment 22

[Magnus Melin [:mkmelin]]

Works fine over here, make sure you are using the 20080227 build (See Help | About.)

Comment 23

[Norm Rowe]

I got the update to 20080227 and it works! Thanks !!

Comment 24

[Norm Rowe]

How ever I can not delete messages.

Comment 25

[Norm Rowe]

Quit and re started and now it does work.

Comment 26

[Jean-Michel Reghem]

Hello ...
This bug is occuring also on 1.8 branch since TB 2.0.0.13pre 20080304(at least)  ... and is still there in the last 2.0.0.13pre 2008030603

See thread in forum: http://forums.mozillazine.org/viewtopic.php?t=632380&start=15&postdays=0&postorder=asc&highlight=

My smtp server is Sendmail 8.13.4/8.13.4

It is really the same bug, because, if i switch to the gmail smtp (with authentification and TSL), it is working.

This bug was for trunk
Reopen and change version to all ???

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #26)
> Hello ...
> This bug is occuring also on 1.8 branch since TB 2.0.0.13pre 20080304(at least)
>  ... and is still there in the last 2.0.0.13pre 2008030603

Thanks for bringing this up. I'll get onto it later today. 

> This bug was for trunk
> Reopen and change version to all ???

I've changed the version, but we don't reopen because the status normally refers to trunk where it affects more than one version.

Comment 28

[earlpiggot]

this has made Tb unusable in 1.8 branch, lately. Please, fix it! See here:
http://forums.mozillazine.org/viewtopic.php?t=634434

Comment 29

[Daniel Veditz [:dveditz]]

Attachment: Allow http://@example.com

We can allow empty userinfo, and there might be other extensions which always use "@" with or without userinfo. Shouldn't encounter it in web content since IE doesn't allow it, but Opera does.

Comment 30

[[e-mail]]

Comment on attachment 308037 [details] [diff] [review]
Allow http://@example.com

>-    if (userinfoLen == 0)
>-        return NS_ERROR_MALFORMED_URI;
I think it might possibly make sense to leave this special case in instead of merging it into the passwordless case below.

Comment 31

[Daniel Veditz [:dveditz]]

Attachment: Keep empty userinfo check separate

Neil suggested keeping the "no userinfo at all" case separate for clarity, since that's the case that's broken here.

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #6)
> (From update of attachment 305379 [details] [diff] [review])
> I'm a little worried about the port issue - what happens with SSL?
> 
> >+  return smtpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl);
> return CallQueryInterface(smtpUrl, aUrl);

Do you even need the QI?

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

this patch seems better

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 308037 [details] [diff] [review]
Allow http://@example.com

Not using this one, the other patch makes the logic clearer than using the more compact ?: operator

Comment 35

[Samuel Sidler (old account; do not CC)]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

Approved for 1.8.1.13. a=ss

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

Requesting approval1.9 to land this fix on the trunk as well in case other code/addons are doing the same thing as mailnews.

Comment 37

[Daniel Veditz [:dveditz]]

fix checked in. I doubt the trunk triagers will see the request in a closed bug so I'll open a new bug about landing on trunk

Comment 38

[Jean-Michel Reghem]

Verified 1.8.1.13

(was not working in TB 2.0.0.13pre 2008030903 ... and working after update to 2008031003)

Don't mark bug as verified because i don't know what is the status for trunk (Comment #36 request aproval but on bug #37 Daniel told that he have open bug 421840 to signal aproval request for 1.9 ...)

Comment 39

[Reed Loden [:reed]]

You're right, they won't see the request. Open the bug, and they will, though.

Comment 40

[Damon Sicore (:damons)]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

a1.9+=damons

Comment 41

[Daniel Veditz [:dveditz]]

Fix checked into trunk

Comment 42

[Alexander Sack]

blocking 1.8.0.15

Comment 43

[Alexander Sack]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

caillon, please sign off for 1.8.0.15. it fixes regression from bug 415034 and should apply cleanly.

Comment 44

[Mark Banner (:standard8)]

Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

Removing long-obsolete approval request as it is no longer needed any more.