Last Comment Bug 327812 - No server name validation (checking) for POP3/IMAP/News/LDAP server, whereas enabled for SMTP
: No server name validation (checking) for POP3/IMAP/News/LDAP server, whereas ...
Status: RESOLVED FIXED
: addon-compat
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
Mentors:
: 604393 (view as bug list)
Depends on: 80855
Blocks: 811272 810680 810763
  Show dependency treegraph
 
Reported: 2006-02-19 08:08 PST by Andrés G. Aragoneses
Modified: 2014-03-13 06:08 PDT (History)
10 users (show)
acelists: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.13 KB, patch)
2012-11-11 11:41 PST, :aceman
mkmelin+mozilla: review+
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v2 (16.29 KB, patch)
2012-11-21 11:09 PST, :aceman
iann_bugzilla: review-
Details | Diff | Splinter Review
patch v3 (25.21 KB, patch)
2012-11-28 12:57 PST, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Andrés G. Aragoneses 2006-02-19 08:08:49 PST
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.
Comment 1 Dan Johnson 2008-06-12 17:47:47 PDT
This behavior is still present in Thunderbird 2.0.0.14 (Windows XP Pro SP3).
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2008-06-12 17:53:39 PDT
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?
Comment 3 :aceman 2012-01-26 01:47:38 PST
Still valid, only SMTP is checked.
I'll take this. But we must first decide what names are valid, in bug 80855.
Comment 4 :aceman 2012-02-13 13:23:38 PST
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?
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-02-13 13:26:06 PST
(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.
Comment 6 :aceman 2012-02-13 13:35:48 PST
Thanks, maybe Mark and David are, per https://wiki.mozilla.org/Modules/MailNews_Core.
Comment 7 Mark Banner (:standard8) (afk until 26th July) 2012-03-12 11:44:44 PDT
(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.
Comment 8 :aceman 2012-03-12 12:27:45 PDT
Thanks, will do that too.
Comment 9 Magnus Melin 2012-03-14 00:26:26 PDT
I think for the whitespace issue originally reported, we should just trim() the value. And actually, with trim hasOnlyWhitespaces isn't needed
Comment 10 :aceman 2012-03-14 01:32:27 PDT
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 11 :aceman 2012-11-11 06:52:17 PST
*** Bug 604393 has been marked as a duplicate of this bug. ***
Comment 12 :aceman 2012-11-11 11:41:10 PST
Created attachment 680456 [details] [diff] [review]
patch
Comment 13 Ian Neal 2012-11-12 15:54:49 PST
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.
Comment 14 Ian Neal 2012-11-14 13:54:14 PST
(In reply to :aceman from comment #12)
> Created attachment 680456 [details] [diff] [review]
> patch

You're bitrotted in Hunk #2 of AccountManager.js
Comment 15 :aceman 2012-11-14 22:40:12 PST
(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 16 Magnus Melin 2012-11-15 12:07:48 PST
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 17 Ian Neal 2012-11-18 09:33:16 PST
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.
Comment 18 :aceman 2012-11-21 11:00:29 PST
(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" :)
Comment 19 :aceman 2012-11-21 11:07:57 PST
(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).
Comment 20 :aceman 2012-11-21 11:09:00 PST
Created attachment 684105 [details] [diff] [review]
patch v2

There were some string changes and more code changes per the reviews so I'd better ask for another final review.
Comment 21 Ian Neal 2012-11-27 13:47:59 PST
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
Comment 22 :aceman 2012-11-27 23:40:01 PST
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.
Comment 23 Ian Neal 2012-11-28 09:07:34 PST
(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
Comment 24 :aceman 2012-11-28 09:12:35 PST
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).
Comment 25 :aceman 2012-11-28 12:57:39 PST
Created attachment 686237 [details] [diff] [review]
patch v3
Comment 26 :aceman 2012-12-02 10:57:58 PST
Thanks.
I'll do some tests in bug 810763.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-12-02 12:29:16 PST
https://hg.mozilla.org/comm-central/rev/4db7af212d13
Comment 28 :aceman 2013-03-11 10:01:47 PDT
Marking this addon-compat in case addons allow some special hostnames that may now be rejected with the new checker.
Comment 29 :aceman 2013-03-26 12:53:19 PDT
Test in bug 810763.
Comment 30 :aceman 2013-05-03 00:02:33 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.