Closed Bug 327812 Opened 18 years ago Closed 11 years ago

No server name validation (checking) for POP3/IMAP/News/LDAP server, whereas enabled for SMTP

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 20.0

People

(Reporter: knocte, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(1 file, 2 obsolete files)

25.21 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
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.
OS: Linux → All
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?
Assignee: mscott → nobody
Component: Preferences → Account Manager
QA Contact: preferences → account-manager
Hardware: PC → All
Version: 1.5 → Trunk
Still valid, only SMTP is checked.
I'll take this. But we must first decide what names are valid, in bug 80855.
Depends on: 80855
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Summary: No server name validation for POP (POP3), whereas enabled for SMTP → No server name validation (checking) for POP3/IMAP/NEWS, whereas enabled for SMTP
Assignee: nobody → acelists
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.
Summary: No server name validation (checking) for POP3/IMAP/NEWS, whereas enabled for SMTP → No server name validation (checking) for POP3/IMAP/News/LDAP server, whereas enabled for SMTP
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.
No longer depends on: 80855
Depends on: 80855
Blocks: 604393
Blocks: 810680
No longer blocks: 604393
Attached patch patch (obsolete) — Splinter Review
Attachment #680456 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #680456 - Flags: review?(iann_bugzilla)
Blocks: 810763
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.
Blocks: 811272
(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
Attachment #680456 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #680456 - Flags: review?(iann_bugzilla) → review+
(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).
Attached patch patch v2 (obsolete) — Splinter Review
There were some string changes and more code changes per the reviews so I'd better ask for another final review.
Attachment #680456 - Attachment is obsolete: true
Attachment #684105 - Flags: review?(iann_bugzilla)
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
Attachment #684105 - Flags: review?(iann_bugzilla) → review-
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).
Attached patch patch v3Splinter Review
Attachment #684105 - Attachment is obsolete: true
Attachment #686237 - Flags: review?(iann_bugzilla)
Attachment #686237 - Flags: review?(iann_bugzilla) → review+
Thanks.
I'll do some tests in bug 810763.
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4db7af212d13
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
Marking this addon-compat in case addons allow some special hostnames that may now be rejected with the new checker.
Keywords: addon-compat
Test in bug 810763.
Flags: in-testsuite? → in-testsuite+
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.
Keywords: relnote
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.