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

RESOLVED FIXED in Thunderbird 20.0

Status

MailNews Core
Account Manager
--
minor
RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: Andrés G. Aragoneses, Assigned: aceman)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
Thunderbird 20.0
addon-compat
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

25.21 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
OS: Linux → All

Comment 1

9 years ago
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
(Assignee)

Comment 3

6 years ago
Still valid, only SMTP is checked.
I'll take this. But we must first decide what names are valid, in bug 80855.
Component: Account Manager → Account Manager
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)

Updated

5 years ago
Assignee: nobody → acelists
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 8

5 years ago
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

Comment 9

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

Comment 10

5 years ago
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.
(Assignee)

Updated

5 years ago
No longer depends on: 80855
(Assignee)

Updated

5 years ago
Depends on: 80855
(Assignee)

Updated

5 years ago
Blocks: 604393
(Assignee)

Updated

5 years ago
Blocks: 810680
(Assignee)

Updated

5 years ago
Duplicate of this bug: 604393
(Assignee)

Updated

5 years ago
No longer blocks: 604393
(Assignee)

Comment 12

5 years ago
Created attachment 680456 [details] [diff] [review]
patch
Attachment #680456 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #680456 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Blocks: 810763

Comment 13

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 811272

Comment 14

5 years ago
(In reply to :aceman from comment #12)
> Created attachment 680456 [details] [diff] [review]
> patch

You're bitrotted in Hunk #2 of AccountManager.js
(Assignee)

Comment 15

5 years ago
(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

5 years ago
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 17

5 years ago
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+
(Assignee)

Comment 18

5 years ago
(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" :)
(Assignee)

Comment 19

5 years ago
(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).
(Assignee)

Comment 20

5 years ago
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.
Attachment #680456 - Attachment is obsolete: true
Attachment #684105 - Flags: review?(iann_bugzilla)

Comment 21

5 years ago
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-
(Assignee)

Comment 22

5 years ago
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

5 years ago
(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
(Assignee)

Comment 24

5 years ago
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).
(Assignee)

Comment 25

5 years ago
Created attachment 686237 [details] [diff] [review]
patch v3
Attachment #684105 - Attachment is obsolete: true
Attachment #686237 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #686237 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 26

5 years ago
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
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
(Assignee)

Comment 28

4 years ago
Marking this addon-compat in case addons allow some special hostnames that may now be rejected with the new checker.
Keywords: addon-compat
(Assignee)

Comment 29

4 years ago
Test in bug 810763.
Flags: in-testsuite? → in-testsuite+
(Assignee)

Comment 30

4 years ago
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.