Inside preferences, if we insert a POP server name like "pop.example.com " (note the space at the end of the string), Thunderbird won't complain. However, if we do the same for SMTP, Thunderbird will tell us to insert a valid name for the host. I think we must unify the behaviour here.

This behavior is still present in Thunderbird 2.0.0.14 (Windows XP Pro SP3).

Confirming on Mac Leopard 10.5.3 build 3.0a2pre from yesterday's checkout. Should we do validation of the server name at the stage of quitting Thunderbird's Account Settings dialog?

Still valid, only SMTP is checked. I'll take this. But we must first decide what names are valid, in bug 80855.

Mconley, should the same check be applied in the addressbook on the LDAP server hostname? The current check in pref-directory-add.js is: if ((!hostname) || hasOnlyWhitespaces(hostname)) errorValue = "invalidHostname"; Is a LDAP server a normal Internet host going by RFC 1123?

(In reply to :aceman from comment #4) > Mconley, should the same check be applied in the addressbook on the LDAP > server hostname? I don't think it could hurt. > Is a LDAP server a normal Internet host going by RFC 1123? As far as I know, yes - though I'll admit, I'm not really an LDAP expert.

Thanks, maybe Mark and David are, per https://wiki.mozilla.org/Modules/MailNews_Core.

(In reply to Mike Conley (:mconley) from comment #5) > (In reply to :aceman from comment #4) > > Mconley, should the same check be applied in the addressbook on the LDAP > > server hostname? > > I don't think it could hurt. > > > Is a LDAP server a normal Internet host going by RFC 1123? > > As far as I know, yes - though I'll admit, I'm not really an LDAP expert. Yes, it should be.

Thanks, will do that too.

I think for the whitespace issue originally reported, we should just trim() the value. And actually, with trim hasOnlyWhitespaces isn't needed

Yes, trim is covered, see patch in bug 80855 (hostnameIsLegal function). We aim to make a universal validity checking function in bug 80855. Also a universal trimming/sanitizing function for AM in amUtils.js (the same bug). And this bug here then just hooks those functions to the input fields where it is missing.

Comment on attachment 680456 [details] [diff] [review] patch >+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js > Components.utils.import("resource://gre/modules/Services.jsm"); >+Components.utils.import("resource://gre/modules/hostnameUtils.jsm"); Does this file really need to import this jsm? >+++ b/mailnews/base/prefs/content/accountcreation/sanitizeDatatypes.js > /** > * DNS hostnames like foo.bar.example.com > * Allow only letters, numbers, "-" and "." > * Empty strings not allowed. > * Currently does not support IDN (international domain names). > */ > hostname : function(unchecked) > { >+ let str = cleanUpHostName(this.nonemptystring(unchecked)); > > // Allow placeholders. TODO move to a new hostnameOrPlaceholder() > // The regex is "anything, followed by one or more (placeholders than > // anything)". This doesn't catch the non-placeholder case, but that's > // handled down below. This comment might need fixing. Is what format a placeholder takes mentioned elsewhere? Otherwise looks good to me, not tested as yet.

(In reply to :aceman from comment #12) > Created attachment 680456 [details] [diff] [review] > patch You're bitrotted in Hunk #2 of AccountManager.js

(In reply to Ian Neal from comment #13) > >+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js > > Components.utils.import("resource://gre/modules/Services.jsm"); > >+Components.utils.import("resource://gre/modules/hostnameUtils.jsm"); > Does this file really need to import this jsm? Not in the current patch, I'll remove it. > >+++ b/mailnews/base/prefs/content/accountcreation/sanitizeDatatypes.js > > // Allow placeholders. TODO move to a new hostnameOrPlaceholder() > > // The regex is "anything, followed by one or more (placeholders than > > // anything)". This doesn't catch the non-placeholder case, but that's > > // handled down below. > This comment might need fixing. Is what format a placeholder takes mentioned > elsewhere? I do not understand this functionality much. I just replaced the spot mentioned in the comment that should not allow placeholders. I can look around for the definition. (In reply to Ian Neal from comment #14) > (In reply to :aceman from comment #12) > > Created attachment 680456 [details] [diff] [review] > > patch > > You're bitrotted in Hunk #2 of AccountManager.js Yeah. You can test without that hunk applied, it is not needed for the functionality.

Comment on attachment 680456 [details] [diff] [review] patch Review of attachment 680456 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work fine, r=mkmelin ::: mailnews/addrbook/prefs/content/pref-directory-add.js @@ +305,5 @@ > var errorValue = null; > var saslMechanism = ""; > if ((!description) || hasOnlyWhitespaces(description)) > errorValue = "invalidName"; > + else if (!isLegalHostNameOrIP(hostname)) Just a note that isLegalHostNameOrIP should probably be made to bail instantly for !hostname... ::: mailnews/base/prefs/content/AccountManager.js @@ +384,5 @@ > alertText = prefBundle.getString("serverNameEmpty"); > } else { > + if (!isLegalHostNameOrIP(newHost)) { > + alertText = prefBundle.getString("enterValidServerName"); > + } else { elses on new line please

Comment on attachment 680456 [details] [diff] [review] patch >+++ b/mailnews/base/prefs/content/AccountManager.js > alertText = null; > // If something is changed then check if the new user/host already exists. > if ((oldUser != newUser) || (oldHost != newHost)) { >+ newUser = newUser.trim(); >+ newHost = cleanUpHostName(newHost); > if ((checkUser && (newUser == "")) || (newHost == "")) { > alertText = prefBundle.getString("serverNameEmpty"); If isLegalHostNameOrIP() bails early on newHost being empty/null then perhaps this check should be just for newUser == "" and change the string to reflect that. > } else { >+ if (!isLegalHostNameOrIP(newHost)) { Instead of this, you could do: } else if (!isLegalHostNameOrIP(newHost)) { r=me with those addressed.

(In reply to Ian Neal from comment #17) > > if ((checkUser && (newUser == "")) || (newHost == "")) { > > alertText = prefBundle.getString("serverNameEmpty"); > If isLegalHostNameOrIP() bails early on newHost being empty/null then > perhaps this check should be just for newUser == "" and change the string to > reflect that. OK, with the added value that when putting empty hostname into a News server (where user name is hidden) the message now does not say "server name or user name must not be empty" :)

(In reply to :aceman from comment #15) > > >+++ b/mailnews/base/prefs/content/accountcreation/sanitizeDatatypes.js > > > // Allow placeholders. TODO move to a new hostnameOrPlaceholder() > > > // The regex is "anything, followed by one or more (placeholders than > > > // anything)". This doesn't catch the non-placeholder case, but that's > > > // handled down below. > > This comment might need fixing. Is what format a placeholder takes mentioned > > elsewhere? > > I do not understand this functionality much. I just replaced the spot > mentioned in the comment that should not allow placeholders. I can look > around for the definition. There is some explanation of placeholders in emailWizard.js. So the function is more forgiving while there are placeholders (with %). I did not touch that. If there are not, we apply our new strict function. I think this does not change the meaning of the code and the comment is still valid. The callers should probably indicate when placeholders are still allowed and when not - the domain is final. But that redesign is for another bug. This may cause that even after this patch we will allow to create something that looks like a placeholder (with 2 %'s).

There were some string changes and more code changes per the reviews so I'd better ask for another final review.

Comment on attachment 684105 [details] [diff] [review] patch v2 >+++ b/mailnews/base/prefs/content/amUtils.js >-function cleanUpHostname(aHostName) >+++ b/mailnews/base/util/hostnameUtils.jsm >+ "cleanUpHostName" ]; >+function cleanUpHostName(aHostName) Spot the difference? This should have been picked up in testing. r- for the moment

I do see the difference but do not see a problem. Do you still see any callers using the "Hostname" (small n) version? I assume I converted them all.

(In reply to :aceman from comment #22) > I do see the difference but do not see a problem. Do you still see any > callers using the "Hostname" (small n) version? I assume I converted them > all. http://mxr.mozilla.org/comm-central/ident?i=cleanUpHostname

Well, testing probably tested only the spots changed in this patch and those passed. The older callers got broken but that was not part of the testing. Sorry about that. I'll see which version is better (in line with the other functions in the hostnameutils file).

Thanks. I'll do some tests in bug 810763.

Marking this addon-compat in case addons allow some special hostnames that may now be rejected with the new checker.

Test in bug 810763.

I propose to mention this change in TB 24 release notes. Some existing hosts in account manager may suddenly be reported as invalid. Can happen e.g. with hostnames using underscore '_', which is invalid, but some systems allow it. We should instruct users what to do in that case.