Last Comment Bug 80855 - fix hostnameIsIllegal() to properly check validity of hostnames and be more robust
: fix hostnameIsIllegal() to properly check validity of hostnames and be more r...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 19.0
Assigned To: :aceman
:
Mentors:
Depends on: 563172 733210 491843 738810 756821
Blocks: 235312 89945 327812 604393 809609 810680 810763 913785
  Show dependency treegraph
 
Reported: 2001-05-14 20:03 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2013-09-13 02:58 PDT (History)
11 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
new hostnameIsIllegal() (866 bytes, patch)
2001-12-13 08:55 PST, Denis Antrushin
no flags Details | Diff | Review
more strict version (4.68 KB, patch)
2012-01-26 13:26 PST, :aceman
no flags Details | Diff | Review
patch, current state of things (32.02 KB, patch)
2012-02-13 13:39 PST, :aceman
iann_bugzilla: feedback-
Details | Diff | Review
patch v4 (39.06 KB, patch)
2012-05-20 08:11 PDT, :aceman
iann_bugzilla: feedback-
Details | Diff | Review
patch v5 (39.25 KB, patch)
2012-05-20 13:46 PDT, :aceman
neil: feedback-
Details | Diff | Review
patch v6 (38.35 KB, patch)
2012-05-21 13:40 PDT, :aceman
no flags Details | Diff | Review
patch v7 (38.26 KB, patch)
2012-05-21 15:01 PDT, :aceman
neil: feedback+
iann_bugzilla: feedback-
Details | Diff | Review
patch v8 (38.36 KB, patch)
2012-11-07 13:45 PST, :aceman
neil: review+
Details | Diff | Review
patch v9 (40.15 KB, patch)
2012-11-09 12:23 PST, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v10 (40.15 KB, patch)
2012-11-11 05:14 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Review
patch v11 (40.17 KB, patch)
2012-11-11 11:26 PST, :aceman
acelists: review+
Details | Diff | Review
fixes for SM part [Checked in: Comment 98] (2.63 KB, patch)
2012-11-17 12:23 PST, Jens Hatlak (:InvisibleSmiley)
iann_bugzilla: review+
acelists: feedback+
Details | Diff | Review

Description (not reading, please use seth@sspitzer.org instead) 2001-05-14 20:03:50 PDT
fix hostnameIsIllegal()in aw-server to be more robust

right now, hostnames like "...", "_" and ".foo.bar" will get through.
Comment 1 (not reading, please use seth@sspitzer.org instead) 2001-05-15 12:01:50 PDT
whoops, I messed up.  - is legal, not _

from rfc 952:

   1. A "name" (Net, Host, Gateway, or Domain name) is a text string up
   to 24 characters drawn from the alphabet (A-Z), digits (0-9), minus
   sign (-), and period (.).  Note that periods are only allowed when
   they serve to delimit components of "domain style names". (See
   RFC-921, "Domain Name System Implementation Schedule", for
   background).  No blank or space characters are permitted as part of a
   name. No distinction is made between upper and lower case.  The first
   character must be an alpha character.  The last character must not be
   a minus sign or period.
Comment 2 Denis Antrushin 2001-12-13 08:55:53 PST
Created attachment 61590 [details] [diff] [review]
new hostnameIsIllegal()

What about this?
Comment 3 R.K.Aa. 2002-05-16 12:40:59 PDT
according to bug 145069, an error message *does* appear when user tries to add a
hostname with a _
Comment 4 varada 2002-09-26 14:33:10 PDT
hostnameIsIllegal() has now been moved to amUtils.js (utility functions).
Comment 5 (not reading, please use seth@sspitzer.org instead) 2003-05-08 09:43:38 PDT
mass re-assign.
Comment 6 benc 2008-09-13 00:49:56 PDT
This appears in two places, it should be consolidated.
Comment 7 Ian Neal 2012-01-23 14:18:41 PST
As this is used by both SeaMonkey and Thunderbird, move it to MailNews Core. As comment 6 suggests it should also be consolidated - there are versions currently in both aw-server.js and amUtils.js
Comment 8 :aceman 2012-01-24 14:23:53 PST
What about the underscore _ ? Should I implement Bug 89945 comment 16?
Comment 9 :aceman 2012-01-25 13:27:07 PST
See also text on underscores on http://en.wikipedia.org/wiki/Hostname .
Comment 10 Ian Neal 2012-01-25 13:35:12 PST
(In reply to :aceman from comment #8)
> What about the underscore _ ? Should I implement Bug 89945 comment 16?

I think we should stick to the relevant standards.

(In reply to :aceman from comment #9)
> See also text on underscores on http://en.wikipedia.org/wiki/Hostname .

A useful reference to the relevant standards.
Comment 11 :aceman 2012-01-25 14:25:35 PST
OK. I came up with this regex. It allows valid IPs too and also many invalid ones. Is it a problem?

/^(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9]))(\.(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9])))*$/i

Also, it seems this function is only used to chcek SMTP server and incoming server in account wizard. In the normal account settings, no check is done (for POP3 servers etc.) Should I add this checking in this bug?
Comment 12 Ian Neal 2012-01-25 15:26:02 PST
(In reply to :aceman from comment #11)
> OK. I came up with this regex. It allows valid IPs too and also many invalid
> ones. Is it a problem?
> 
> /^(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9]))(\.(([a-
> z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9])))*$/i
> 
I think that trying to do it with a regex on its own is not correct.
phishingDetector.js has a helper function hostNameIsIPAddress which potentially could be moved (sharedglue.js perhaps), perhaps tweaked, and made use of and then have any resulting hostname checked for validity (with a suitable regex maybe).

> Also, it seems this function is only used to chcek SMTP server and incoming
> server in account wizard. In the normal account settings, no check is done
> (for POP3 servers etc.) Should I add this checking in this bug?

That would be a follow-up bug once this helper function is perfected/made less bad.
Comment 13 :aceman 2012-01-26 01:37:00 PST
OK. Is the proposed regex suitable for matching just hostnames?

I propose this (pseudo code) validity check:
if (/^([0-9])*(\.([0-9])*)*$/.test(givenString)) then
   return hostNameIsIPAddress(givenString);
else
   return /^(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9]))(\.(([a-
> z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9])))*$/i.test(givenString);

This would eliminate invalid IPs (like 1.1 or 67890.2323.2343.11) too.
If it contains at least one alpha character, then it is a valid hostname and not an IP.
Comment 14 :aceman 2012-01-26 12:41:02 PST
It seems hostNameIsIPAddress is in suite only. And it seems it allows many more formats (like hex?). I am not sure we want this in the account manager. I'll try to make some new function. If you wish, you can then merge it in some way.
Comment 15 :aceman 2012-01-26 13:26:18 PST
Created attachment 591919 [details] [diff] [review]
more strict version
Comment 16 Ian Neal 2012-01-26 14:57:30 PST
(In reply to :aceman from comment #14)
> It seems hostNameIsIPAddress is in suite only. And it seems it allows many
> more formats (like hex?). I am not sure we want this in the account manager.
> I'll try to make some new function. If you wish, you can then merge it in
> some way.

http://mxr.mozilla.org/comm-central/search?string=hostNameIsIPAddress
So in both /mail/base/content/phishingDetector.js and /suite/mailnews/phishingDetector.js
My ideal solution would be a single shared version that both account manager and phishingDetector can use across both suite and TB. Obviously isIPv4HostName and isIPv6HostName would have to be moved too.
Comment 17 Ian Neal 2012-01-26 15:02:38 PST
Comment on attachment 591919 [details] [diff] [review]
more strict version

>+++ b/mailnews/base/prefs/content/aw-server.js

>+    if ((!gHideIncoming && hostnameIsIllegal(trim(incomingServerName))) ||
>+        (!usingDefaultSMTP && hostnameIsIllegal(trim(smtpserver))))

>+    if (hostnameIsIllegal(trim(newsServerName)))
I rather you trim within the hostnameIsIllegal function than in each caller.

Cancelling rather r- now that have hopefully pointed you in a slightly different direction, so you can generate a new patch.
Comment 18 :aceman 2012-01-27 00:10:25 PST
Notice the trim not in each caller, only in the Account WIZARD, where after it determines the trimmed value is legal, it trims also the proper field content. Trimming in hostnameIsIllegal would require it to return the trimmed value too and for the Account MANAGER to trim the fields too. I didn't want to do that as it would be a change in behaviour and more complicated. But if you wish that I can try it.

If you really want to use hostNameIsIPAddress (which is only a part of the solution)  then you have not answered my issue with it. Whether you want to allow all its exotic formats to be input in the account manager. Will the backend handle them?
Comment 19 Ian Neal 2012-01-27 17:53:39 PST
(In reply to :aceman from comment #18)
> Notice the trim not in each caller, only in the Account WIZARD, where after
> it determines the trimmed value is legal, it trims also the proper field
> content. Trimming in hostnameIsIllegal would require it to return the
> trimmed value too and for the Account MANAGER to trim the fields too. I
> didn't want to do that as it would be a change in behaviour and more
> complicated. But if you wish that I can try it.
Where we are taking user input, it is probably best to trim that, so, yes, I should have been more explicit in saying that is what I wanted.
> 
> If you really want to use hostNameIsIPAddress (which is only a part of the
> solution)  then you have not answered my issue with it. Whether you want to
> allow all its exotic formats to be input in the account manager. Will the
> backend handle them?
It doesn't need to, as this frontend will convert an IP address a user inputs as hex/octal into decimal.
Comment 20 :aceman 2012-01-28 05:35:56 PST
(In reply to Ian Neal from comment #19)
> It doesn't need to, as this frontend will convert an IP address a user
> inputs as hex/octal into decimal.
How will it convert it?
Comment 21 Ian Neal 2012-01-28 05:51:41 PST
(In reply to :aceman from comment #20)
> (In reply to Ian Neal from comment #19)
> > It doesn't need to, as this frontend will convert an IP address a user
> > inputs as hex/octal into decimal.
> How will it convert it?

http://mxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#300 onwards gives a clue. It takes each component and runs it through parseInt which, without a radix, will attempt to convert each component into a decimal.
Comment 22 :aceman 2012-01-28 13:39:58 PST
Yes, but I still do not see how it will feed the converted (sanitized) IP (hostname) back to the caller so that it can redisplay in the input field. I don't think that functionality is there. Do you mean I should add it? Add an OUT argument that returns the converted value?
Comment 23 Ian Neal 2012-01-28 13:50:27 PST
(In reply to :aceman from comment #22)
> Yes, but I still do not see how it will feed the converted (sanitized) IP
> (hostname) back to the caller so that it can redisplay in the input field. I
> don't think that functionality is there. Do you mean I should add it? Add an
> OUT argument that returns the converted value?
It returns either false or the converted value.
http://mxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#330 onwards
and
http://mxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#412 onwards
and
http://mxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#279
Comment 24 :aceman 2012-01-28 14:06:54 PST
Ah yes.
But I still do not see how to reject invalid numeric addresses. Now if we want to allow some hex formats I do not know what is allowed there and how to update the logic from comment 13. I can't use the numeric regex now. Then even if hostNameIsIPAddress returns false on something, my hostname regex will still return true on many invalid numeric adresses.
Comment 25 Ian Neal 2012-01-28 15:46:35 PST
(In reply to :aceman from comment #24)
> Ah yes.
> But I still do not see how to reject invalid numeric addresses. Now if we
> want to allow some hex formats I do not know what is allowed there and how
> to update the logic from comment 13. I can't use the numeric regex now. Then
> even if hostNameIsIPAddress returns false on something, my hostname regex
> will still return true on many invalid numeric adresses.

hostNameIsIPAddress will return false if it is not a valid IP address, but it could still be a valid hostname, so the next stage of the process (if false has been returned) is to test for a valid hostname.
The valid hostname test probably needs to deal with IDN too, so a regex might not be suitable.
Comment 26 :aceman 2012-01-28 16:10:51 PST
I just say that if 1.2.3.4.5 is not a valid IP, it seems to be a valid hostname per the spec. I am not sure that is correct.

I think IDN hostnames encoded in punycode (per RFC 3490) would pass the regex (as they seem to be valid per RFC 1123).
Comment 27 Ian Neal 2012-01-28 16:19:57 PST
(In reply to :aceman from comment #26)
> I just say that if 1.2.3.4.5 is not a valid IP, it seems to be a valid
> hostname per the spec. I am not sure that is correct.
Good question, there are probably additional rules around the FQDN parts.
> 
> I think IDN hostnames encoded in punycode (per RFC 3490) would pass the
> regex (as they seem to be valid per RFC 1123).
Yes, we should store in punycode, but should we accept localised input?
Comment 28 :aceman 2012-01-28 16:28:57 PST
Maybe if Firefox does accept it (in the location bar) we could use its code. But I it may be big and hard.
Comment 29 Ian Neal 2012-01-28 16:34:04 PST
This is also overlapping into Bug 235312
Comment 30 Ian Neal 2012-01-28 16:53:10 PST
You would need to pass it through idn-service's convertUTF8toACE to ensure it is ascii before testing against the relevant rules.
Comment 31 :aceman 2012-01-28 17:38:15 PST
Maybe bug 235312 should block this one. The backend must be prepared to handle IDN before we allow it in the account manager. Or does it work fine if we pass it punycode? Bug 235312 comment 4 does not look like that. Maybe we should intentionally make this function very strict (no IDN/punycode) until the backed is realy.
Comment 32 :aceman 2012-02-12 14:51:42 PST
Well, I am playing with this, I have the value sanitization in place, the functions are moved out of phishingDetector into a common module, etc.

But the isLegalIPv4HostName is doing weird stuff. E.g. it accepts "1.2" and converts it to "0.0.0.1".
What is that? Is it that Dword stuff? Should this be improved/forbidden?
Comment 33 :aceman 2012-02-13 13:39:52 PST
Created attachment 596778 [details] [diff] [review]
patch, current state of things

This is where I am today. Is it the direction you wanted?
What it does so far:
- moves the implementation of isLegalIPv4HostName, isLegalIPv6HostName, isLegalLocalIPAddress from mail/phishingDetector.js (because it had a more complete/extensive version) into a new shared module hostnameUtils.js in /mailnews/base/util
- updates mail/phishingDetector.js and suite/phishingDetector.js to call these shared functions, but otherwise their functionality should be unchanged. (I don't know how to test this, when these phishing features are called).
- updates existing places that check the hostname (aw-server.js and Accountmanager.js) to use these new functions. If the function returns a new sanitized hostname, the form fields should pick it up. The values are trimmed and lowercased automatically in all spots. The trim was requested by Ian, but lowercase is a suggestion. If it is not wished, I must solve bug 238583 comment 28 in the places that check hostname collisions.

Allowing of IPv6 addresses or UTF8 hostnames is dependent upon other bugs so is not yet enabled.

Any new ideas?
Comment 34 :aceman 2012-03-02 14:31:00 PST
If patch from bug 491843 sticks, we can enable IPv6 here.
Comment 35 :aceman 2012-04-14 08:24:02 PDT
Can we move here in any way?
Comment 36 :aceman 2012-04-25 09:53:42 PDT
We have some problems interpreting the relevant RFCs about what is an acceptable hostname.

Boris, it there some code in Firefox already checking validity of hostnames? Or can you help us finding a person that could know this topic good?
Comment 37 Ian Neal 2012-05-19 14:24:23 PDT
Comment on attachment 596778 [details] [diff] [review]
patch, current state of things

>+++ b/mail/base/content/phishingDetector.js
>@@ -34,16 +34,18 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ****** */
> 
> // Dependencies:
> // gPrefBranch should already be defined
> // gatherTextUnder from utilityOverlay.js
> 
>+Components.utils.import("resource:///modules/hostnameUtils.js");
I think the preference is for these to be .jsm rather than .js

>   isIPv4HostName: function(aHostName)
>   {
>+    return isLegalIPv4HostName(aHostName);
>   },
Can't we just call this directly?
> 
>   /**
>    * Check if the given host name is an IPv6 address.
>    * @return the full IPv6 address if aHostName is an IPv6 address.
>    */
>+  isIPv6HostName: function(aHostName)
>+  {
>+    return isIPv6HostName(aHostName);
>   },
Can't we just call this directly? And isn't it return isLegalIPv6HostName(aHostName);
> 
>   /**
>    * Check if the given host name is a local IP address.
>    * @return true if unobscuredHostName is a local IP address.
>    */
>   isLocalIPAddress: function(unobscuredHostNameValue)
>   {
>+    return isLegalLocalIPAddress(unobscuredHostNameValue);
>   },
Can't we just call this directly?

>+++ b/mailnews/base/prefs/content/SmtpServerEdit.js
>+++ b/mailnews/base/prefs/content/amUtils.js

> function hostnameIsIllegal(hostname)
Nit: use aHostname
> {
>+  // Just a temporary wrapper function. It will be removed after bug 224831 lands
>+  // and AccountManager.js / checkUserServerChanges() can be converted to hostnameIsLegal()
>+  return (hostnameIsLegal(hostname) == false);
> }
> 
>+function hostnameIsLegal(hostname)
Nit: use aHostname
> {
>+  //TODO: Bug 235312: if UTF8 string was input, convert to punycode using convertUTF8toACE()
>+  //but bug 563172 needs resolving first.
>+  return isLegalHostNameOrIP(hostname.trim().toLowerCase());
Why the .toLowerCase(), dealt with by the i in the hostPattern?

>+++ b/mailnews/base/util/hostnameUtils.js
>@@ -0,0 +1,269 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
Use the new boilerplate.

>+function isLegalHostName(aHostName)
>+{
>+  const hostPattern = /^(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9]))(\.(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9])))*$/i;

([a-z0-9][a-z0-9]) is the same as |([a-z0-9][-a-z0-9]{0,61}[a-z0-9]) when the {0,61} is 0, so this becomes:
/^(([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])$/i
Note I also have a preference for using \- rather than - at the beginning of a-z is it is more obvious.

>+  if ((aHostName.length <= 255) && hostPattern.test(aHostName))
>+    return aHostName;
>+
>+  return false;
>+}
You could write this as:
return ((aHostName.length <= 255) && hostPattern.test(aHostName)) ? aHostName : false;

>+function isLegalLocalIPAddress(unobscuredHostNameValue)
Nit: argument should start with an "a" e.g. aHostName

>+
>+    // fe80::/10 - link local addresses
>+    if (ipComponents[0] == "fe80")
>+      return true;
>+
>+    // TODO: also detect fc00::/7 - unique local addresses
Would be good to get this fixed in this bug.

>+++ b/suite/mailnews/phishingDetector.js

> function hostNameIsIPAddress(aHostName, aUnobscuredHostName)
> {
>+  let cleanedHostName = isLegalIPv4Hostname(aHostName) || isLegalIPv6Hostname(aHostName);
>+  if (cleanedHostName != false) {
>+    aUnobscuredHostName.value = cleanedHostName;
>     return true;
>   }
>   return false;
> }
If the caller was fixed up to call the same way as in TB then could call directly rather than via this function (out of scope of this bug).
f- for the moment
Comment 38 :aceman 2012-05-19 14:37:13 PDT
(In reply to Ian Neal from comment #37)
> >   isIPv4HostName: function(aHostName)
> >   {
> >+    return isLegalIPv4HostName(aHostName);
> >   },
> Can't we just call this directly?
I do not understand these strange declarations so I can't answer this.
How would a direct call look like?

> >+function hostnameIsLegal(hostname)
> Nit: use aHostname
> > {
> >+  //TODO: Bug 235312: if UTF8 string was input, convert to punycode using convertUTF8toACE()
> >+  //but bug 563172 needs resolving first.
> >+  return isLegalHostNameOrIP(hostname.trim().toLowerCase());
> Why the .toLowerCase(), dealt with by the i in the hostPattern?
No, the pattern just lowercases the argument while checking it.
But this one here lowercases it permanently and returns back.
But I think I should drop this. People may want to have their names capitalized in various ways.

> >+function isLegalHostName(aHostName)
> >+{
> >+  const hostPattern = /^(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9]))(\.(([a-z0-9])|([a-z0-9][a-z0-9])|([a-z0-9][-a-z0-9]{0,61}[a-z0-9])))*$/i;
> 
> ([a-z0-9][a-z0-9]) is the same as |([a-z0-9][-a-z0-9]{0,61}[a-z0-9]) when
> the {0,61} is 0, so this becomes:
> /^(([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9\-
> ]{0,61}[a-z0-9])$/i
> Note I also have a preference for using \- rather than - at the beginning of
> a-z is it is more obvious.
OK. But we do not know if this regex is good.

> >+    // fe80::/10 - link local addresses
> >+    if (ipComponents[0] == "fe80")
> >+      return true;
> >+
> >+    // TODO: also detect fc00::/7 - unique local addresses
> Would be good to get this fixed in this bug.
But I do not know IPv6 to implement this.

The rest is nice, thanks.
Comment 39 Ian Neal 2012-05-19 15:29:32 PDT
(In reply to :aceman from comment #38)
> (In reply to Ian Neal from comment #37)
> > >   isIPv4HostName: function(aHostName)
> > >   {
> > >+    return isLegalIPv4HostName(aHostName);
> > >   },
> > Can't we just call this directly?
> I do not understand these strange declarations so I can't answer this.
> How would a direct call look like?
At the moment you have something like:
http://mxr.mozilla.org/comm-central/source/mail/base/content/phishingDetector.js#279
return this.isIPv4HostName(aHostName) || this.isIPv6HostName(aHostName);
In theory, you should be able to change this to:
return isLegalIPv4HostName(aHostName) || isLegalIPv6HostName(aHostName);

> > >+  return isLegalHostNameOrIP(hostname.trim().toLowerCase());
> > Why the .toLowerCase(), dealt with by the i in the hostPattern?
> No, the pattern just lowercases the argument while checking it.
> But this one here lowercases it permanently and returns back.
> But I think I should drop this. People may want to have their names
> capitalized in various ways.
Agreed.

> > /^(([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9\-
> > ]{0,61}[a-z0-9])$/i
> > Note I also have a preference for using \- rather than - at the beginning of
> > a-z is it is more obvious.
> OK. But we do not know if this regex is good.
Well it matches what you have put in the comments (as far as I can tell), see what Mark (and maybe Neil) think of it.

> > >+    // fe80::/10 - link local addresses
> > >+    if (ipComponents[0] == "fe80")
> > >+      return true;
> > >+
> > >+    // TODO: also detect fc00::/7 - unique local addresses
> > Would be good to get this fixed in this bug.
> But I do not know IPv6 to implement this.
I had a look at http://en.wikipedia.org/wiki/Unique_local_address which seems to have a good explanation. So need to check that the first component is of the form fdxx and the same for fcxx or add a comment that fcxx doesn't have acceptance yet.
Comment 40 :aceman 2012-05-19 15:35:16 PDT
(In reply to Ian Neal from comment #39)
> (In reply to :aceman from comment #38)
> > (In reply to Ian Neal from comment #37)
> > > >   isIPv4HostName: function(aHostName)
> > > >   {
> > > >+    return isLegalIPv4HostName(aHostName);
> > > >   },
> > > Can't we just call this directly?
> > I do not understand these strange declarations so I can't answer this.
> > How would a direct call look like?
> At the moment you have something like:
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> phishingDetector.js#279
> return this.isIPv4HostName(aHostName) || this.isIPv6HostName(aHostName);
> In theory, you should be able to change this to:
> return isLegalIPv4HostName(aHostName) || isLegalIPv6HostName(aHostName);

Ok, I understand. But there must not be any other callers (in other files) that would call detector.isIPv4HostName() directly.
Comment 41 Ian Neal 2012-05-19 15:46:19 PDT
(In reply to :aceman from comment #40)
> Ok, I understand. But there must not be any other callers (in other files)
> that would call detector.isIPv4HostName() directly.
As far as I can see it, and the others, are only used within phishingDetector.js
Comment 42 :aceman 2012-05-20 08:11:03 PDT
Created attachment 625491 [details] [diff] [review]
patch v4
Comment 43 Ian Neal 2012-05-20 09:19:41 PDT
Comment on attachment 625491 [details] [diff] [review]
patch v4

>+++ b/mailnews/base/util/hostnameUtils.jsm
>@@ -0,0 +1,305 @@
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
>+ * ***** BEGIN LICENSE BLOCK *****
You're still not using the new license header, see http://www.mozilla.org/MPL/headers/

>+/**
>+ * Check if aUnobscuredHostNameValue is a local IP address.
>+ *
>+ * @return  true if it is a local IPv4 or IPv6 address, otherwise false.
>+ */
>+function isLegalLocalIPAddress(aUnobscuredHostNameValue)
>+{
>+  // TODO: maybe this function should be changed to:
>+  // - check if the passed in value really is valid (isLegalIPv*Address())
>+  // - return the unobscured value in the same way as all the other functions.
As far as I can see we do not make use of the unobscured value, so no point returning it.
What the callers want is to find out if the host name originally provided is not a legal local ip address, so just negate the logic of this function, returning true if it is not a legal local IP address and false if it is. This means if isLegalIPAddress returns false this function should return early with true, otherwise use the value returned to check to see if it is not local.

>+++ b/suite/mailnews/phishingDetector.js
Might we worth re-basing on top of my patch for bug 756821 if you want to do the above changes.
Comment 44 :aceman 2012-05-20 10:41:42 PDT
Are you planning to do any changes to phishingDetector.js in bug 756821? I understood it those changes are being done here, that's why I reversed the dependence.

I don't like reversing the return value of isLegalLocalIPAddress. If you don't like my proposal then I'd at least keep the return values as they are now to keep it consistent with the other functions. So that false from all of them means failure.
Comment 45 :aceman 2012-05-20 11:29:36 PDT
Comment on attachment 625491 [details] [diff] [review]
patch v4

In the current form of the patch the user inputed values are run through the checking functions (isLegal*) and those cleanup and canonicalize the values (mostly in IP address case). Those canonicalized values are fed back and displayed in the input fields. It came up on IRC that that may not be wanted as the user inputed IP address may be valid but shortened form that the RFCs allow. And the canonicalized form may be quite longer and unexpected for the user.

Bwinton, we'd need UI decision if we want to touch the user's input or just leave it as is if it is found valid by the checking functions.
Comment 46 :aceman 2012-05-20 13:46:18 PDT
Created attachment 625515 [details] [diff] [review]
patch v5

So this is the version that does not touch the user input if it is valid.

Also I found the existing isLegalIPv6Address() allowed (accepted as valid) some strange strings like "::ffffty:192.0.2.128 yy". It seems the usage of parseInt() allowed that, because it just consumed the chars that were valid and skipped the rest (like "128 yy" = 128 => OK). So I added some regexes to check there are only valid chars and in the proper amount, to both isLegalIPv*Address() functions. I hope it doesn't break the phishingDetector...

Other tweaks like fixed comments and .toLowerCase() in the canonicalization of IPv6.
Comment 47 neil@parkwaycc.co.uk 2012-05-20 14:14:35 PDT
Comment on attachment 625515 [details] [diff] [review]
patch v5

>+Components.utils.import("resource:///modules/hostnameUtils.jsm");
You're importing this on every wizard page, it would be better to import it once, in AccountWizard.js presumably.

>+        incomingServer.value = incomingServerClean;
You can't do this in an input event handler, it will really confuse the user. (In fact I'd prefer that you only trim when you actually save the value.)

>+   Host software MUST handle host names of up to 63 characters and
>+   SHOULD handle host names of up to 255 characters.
>+  */
>+
>+  const hostPattern = /^(([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])$/i;
>+  return ((aHostName.length <= 255) && hostPattern.test(aHostName)) ? aHostName : false;
So, when it says host names, does it mean each part between dots or the whole string? Also, we have better ways of limiting text entry to 255 characters.

>+      // By leaving the radix parameter blank, we can handle IP addresses
>+      // where one component is hex, another is octal, etc.
>+      if (/^[0-9a-f]{1,3}$/.test(ipComponents[i]))
>+        ipComponents[i] = parseInt(ipComponents[i]);
Presumably hex will have a 0x prefix? You might want to use Number(ipComponents[i]) instead, which checks for trailing garbage, but also accepts a leading minus sign.

>+  // Take care if the last part is written in decimal using dots as separators.
Don't we have a function that can check that for you already?

>+  if (cleanedHostName !== false) {
Why are you specifically comparing this to false, surely cleanedHostName cannot successfully be any of the other false-like values? (If, as in this case, the true value is normally a string, then I would suggest using the empty string or null as good false-like values instead of false itself.)
Comment 48 :aceman 2012-05-21 00:40:14 PDT
(In reply to neil@parkwaycc.co.uk from comment #47)
> >+Components.utils.import("resource:///modules/hostnameUtils.jsm");
> You're importing this on every wizard page, it would be better to import it
> once, in AccountWizard.js presumably.
I was told it is so cheap it does not matter. So I always include there where it is used.

> >+        incomingServer.value = incomingServerClean;
> You can't do this in an input event handler, it will really confuse the
> user. (In fact I'd prefer that you only trim when you actually save the
> value.)
I do not understand. Why would trimming confuse the user? The canonicalization could, but I removed that.

> >+   Host software MUST handle host names of up to 63 characters and
> >+   SHOULD handle host names of up to 255 characters.
> >+  */
> >+
> >+  const hostPattern = /^(([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9\-]{0,61}[a-z0-9])$/i;
> >+  return ((aHostName.length <= 255) && hostPattern.test(aHostName)) ? aHostName : false;
> So, when it says host names, does it mean each part between dots or the
> whole string?
That is one of the open questions from comment 36. During my investigation via various web forums this seems to be the interpretation people came up. But it is not sure.

> Also, we have better ways of limiting text entry to 255
> characters.
But this would also reject invalid values written directly into prefs.js. (Once user opens the editor the value is checked again. At least once I implement this in the Account manager too.)

> >+      // By leaving the radix parameter blank, we can handle IP addresses
> >+      // where one component is hex, another is octal, etc.
> >+      if (/^[0-9a-f]{1,3}$/.test(ipComponents[i]))
> >+        ipComponents[i] = parseInt(ipComponents[i]);
> Presumably hex will have a 0x prefix? You might want to use
> Number(ipComponents[i]) instead, which checks for trailing garbage, but also
> accepts a leading minus sign.
I have no idea what those obfuscations look like. I can try Number and rejecting minus sign. Depends on if we want to fix the function here or just leave it as it was.

> >+  // Take care if the last part is written in decimal using dots as separators.
> Don't we have a function that can check that for you already?
isLegalIPv4Address :) But I do not think we want to allow the hex/octal garbage in IPv6 so it would need to take an argument. Would that work for you?

> 
> >+  if (cleanedHostName !== false) {
> Why are you specifically comparing this to false, surely cleanedHostName
> cannot successfully be any of the other false-like values?
Just safety. Why think about if something like "0" is a valid IPv6 address or hostname? Just compare to false.
Comment 49 neil@parkwaycc.co.uk 2012-05-21 01:29:08 PDT
(In reply to aceman from comment #48)
> (In reply to comment #47)
> > >+Components.utils.import("resource:///modules/hostnameUtils.jsm");
> > You're importing this on every wizard page, it would be better to import it
> > once, in AccountWizard.js presumably.
> I was told it is so cheap it does not matter. So I always include there
> where it is used.
OK, that makes sense I suppose.

> > >+        incomingServer.value = incomingServerClean;
> > You can't do this in an input event handler, it will really confuse the
> > user. (In fact I'd prefer that you only trim when you actually save the
> > value.)
> I do not understand. Why would trimming confuse the user? The
> canonicalization could, but I removed that.
It's the setting the value that is a bad idea in an input event handler. (In some of your other code you do it on some other sort of event, but I would still prefer you not to.)

> > So, when it says host names, does it mean each part between dots or the
> > whole string?
> That is one of the open questions from comment 36.
Fair enough.

> > Also, we have better ways of limiting text entry to 255
> > characters.
> But this would also reject invalid values written directly into prefs.js.
There's a big warning in about:config and a fairly big one in prefs.js, any user using those deserves what they get. (Some users even override the crashy video driver preference!)

> > >+      // By leaving the radix parameter blank, we can handle IP addresses
> > >+      // where one component is hex, another is octal, etc.
> > >+      if (/^[0-9a-f]{1,3}$/.test(ipComponents[i]))
> > >+        ipComponents[i] = parseInt(ipComponents[i]);
> > Presumably hex will have a 0x prefix? You might want to use
> > Number(ipComponents[i]) instead, which checks for trailing garbage, but also
> > accepts a leading minus sign.
> I have no idea what those obfuscations look like. I can try Number and
> rejecting minus sign. Depends on if we want to fix the function here or just
> leave it as it was.
Sorry, it's hard to tell what was copy/paste.

> > >+  // Take care if the last part is written in decimal using dots as separators.
> > Don't we have a function that can check that for you already?
> isLegalIPv4Address :) But I do not think we want to allow the hex/octal
> garbage in IPv6
Ah, that explains it. (I really should have noticed the word "decimal"!)

> > >+  if (cleanedHostName !== false) {
> > Why are you specifically comparing this to false, surely cleanedHostName
> > cannot successfully be any of the other false-like values?
> Just safety. Why think about if something like "0" is a valid IPv6 address
> or hostname? Just compare to false.
Unlike Perl, "0" is not a false-like value in JavaScript; only "", 0, null and undefined can convert to false.
Comment 50 :aceman 2012-05-21 04:36:38 PDT
(In reply to neil@parkwaycc.co.uk from comment #49)
> > > >+        incomingServer.value = incomingServerClean;
> > > You can't do this in an input event handler, it will really confuse the
> > > user. (In fact I'd prefer that you only trim when you actually save the
> > > value.)
> > I do not understand. Why would trimming confuse the user? The
> > canonicalization could, but I removed that.
> It's the setting the value that is a bad idea in an input event handler. (In
> some of your other code you do it on some other sort of event, but I would
> still prefer you not to.)
Ok, I can put the trim back to the setPageData().

> > > So, when it says host names, does it mean each part between dots or the
> > > whole string?
> > That is one of the open questions from comment 36.
> Fair enough.
> 
> > > Also, we have better ways of limiting text entry to 255
> > > characters.
> > But this would also reject invalid values written directly into prefs.js.
> There's a big warning in about:config and a fairly big one in prefs.js, any
> user using those deserves what they get. (Some users even override the
> crashy video driver preference!)
OK. You can limit the entry field to 255 chars. BUT this checking is used also in the phishingDetector and there you can't know what the received message will contain.
Also I'd like the functions to be complete and if they say the value is OK it should be trustable without depending on checks somewhere else (which may differ or be missing, like AW fields vs. prefs.js).

> > > >+      // By leaving the radix parameter blank, we can handle IP addresses
> > > >+      // where one component is hex, another is octal, etc.
> > > >+      if (/^[0-9a-f]{1,3}$/.test(ipComponents[i]))
> > > >+        ipComponents[i] = parseInt(ipComponents[i]);
> > > Presumably hex will have a 0x prefix? You might want to use
> > > Number(ipComponents[i]) instead, which checks for trailing garbage, but also
> > > accepts a leading minus sign.
> > I have no idea what those obfuscations look like. I can try Number and
> > rejecting minus sign. Depends on if we want to fix the function here or just
> > leave it as it was.
> Sorry, it's hard to tell what was copy/paste.
All the code for isLegalIPv*Address is directly copied from phishingDetector.js as said in previous comments. The only change I did was what I wrote in comment 46 (lowercase and the regexes). And also the local Ipv6 addressses as Ian wanted. If the changes are problematic, the only way for me is to just remove them and it will be as it was. I can't argue about the correct format of addresses, I do not know them enough.

> > > >+  // Take care if the last part is written in decimal using dots as separators.
> > > Don't we have a function that can check that for you already?
> > isLegalIPv4Address :) But I do not think we want to allow the hex/octal
> > garbage in IPv6
> Ah, that explains it. (I really should have noticed the word "decimal"!)
But I'd like to use that function on that IPv4 part. Would an optional argument to it be bad? Like isLegalIPv4Address(aHostName, aOnlyAllowDecimal).

> 
> > > >+  if (cleanedHostName !== false) {
> > > Why are you specifically comparing this to false, surely cleanedHostName
> > > cannot successfully be any of the other false-like values?
> > Just safety. Why think about if something like "0" is a valid IPv6 address
> > or hostname? Just compare to false.
> Unlike Perl, "0" is not a false-like value in JavaScript; only "", 0, null
> and undefined can convert to false.
Too much thinking:) Why is strict comparing to false bad? What do we want to optimize? Do you want to reduce it to !isLegal*() ?
Comment 51 neil@parkwaycc.co.uk 2012-05-21 04:51:22 PDT
(In reply to aceman from comment #50)
> this checking is used also in the phishingDetector
Fair enough.

> All the code for isLegalIPv*Address is directly copied from
> phishingDetector.js as said in previous comments.
Sorry, I've come late to this bug and don't know which comments to read.

> But I'd like to use that function on that IPv4 part. Would an optional
> argument to it be bad? Like isLegalIPv4Address(aHostName, aOnlyAllowDecimal).
It seems reasonable to have a radix argument and pass that to parseInt, that would then allow you to pass 10 when you wanted to force decimal parsing.

> Too much thinking:) Why is strict comparing to false bad? What do we want to
> optimize? Do you want to reduce it to !isLegal*() ?
I want to optimise for readability and maintainability ;-)
Comment 52 :aceman 2012-05-21 06:57:30 PDT
IanN, any comments?
Comment 53 :aceman 2012-05-21 11:34:32 PDT
Ian, are your changes to phishingDetector done? Can I update the patch?
Comment 54 :aceman 2012-05-21 13:40:46 PDT
Created attachment 625747 [details] [diff] [review]
patch v6

What about this?
Comment 55 Ian Neal 2012-05-21 14:45:56 PDT
I don't know if you spotted from the SM phishingDetector.js changes, Neil prefers that null rather than false is returned for when it is not legal.
Comment 56 :aceman 2012-05-21 14:51:52 PDT
Thanks, I missed that.
Comment 57 :aceman 2012-05-21 15:01:53 PDT
Created attachment 625777 [details] [diff] [review]
patch v7
Comment 58 Ian Neal 2012-05-21 16:19:57 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

Just looking at the new file for the moment...
>+++ b/mailnews/base/util/hostnameUtils.jsm
>+function isLegalHostNameOrIP(aHostName, aAllowExtendedIPFormats)
>+{
>+  return (isLegalIPAddress(aHostName, aAllowExtendedIPFormats) ||
>+          isLegalHostName(aHostName));
Nit: I don't think the outer brackets/braces are needed here.

>+function isLegalIPv4Address(aHostName, aAllowExtendedIPFormats)
>+{

>+      // is the component decimal?
>+      if (/^(0|([1-9][0-9]{0,2}))$/.test(ipComponents[i])) {
>+        ipComponents[i] = parseInt(ipComponents[i], 10);
>+      } else if (aAllowExtendedIPFormats) {
>+        // is the component octal?
>+        if (/^(0[0-7]{1,3})$/.test(ipComponents[i]))
>+          ipComponents[i] = parseInt(ipComponents[i], 8);
>+        // is the component octal?
>+        else if (/^(0x[0-9a-f]{1,2})$/.test(ipComponents[i]))
>+          ipComponents[i] = parseInt(ipComponents[i], 16);
>+        else
>+          return null;
>+      } else {
>+        return null;
>+      }
I think you are over-complicating it here. Surely something like the following would do:
ipComponents[i] = aAllowExtendedIPFormats ? parseInt(ipComponents[i]) :
                                            parseInt(ipComponents[i], 10);

>+function isLegalIPv6Address(aHostName)
>+{

>+  // Take care if the last part is written in decimal using dots as separators.
>+  let lastPart = isLegalIPv4Address(ipComponents[ipComponents.length - 1], true);
Do you really mean true here?
Would it help to set a variable length = ipComponents.length - 1
>+  if (lastPart)
>+  {
>+    let lastPartComponents = lastPart.split(".");
>+    // Convert it into standard IPv6 components.
>+    ipComponents[ipComponents.length - 1] =
>+      ((lastPartComponents[0] << 8) | lastPartComponents[1]).toString(16);
>+    ipComponents[ipComponents.length] =
>+      ((lastPartComponents[2] << 8) | lastPartComponents[3]).toString(16);
>+  }

>+function isLegalIPAddress(aHostName, aAllowExtendedIPFormats)
>+{
>+  return isLegalIPv4Address(aHostName, aAllowExtendedIPFormats) || isLegalIPv6Address(aHostName, aAllowExtendedIPFormats);
Nit: split over two lines.
Comment 59 :aceman 2012-05-21 23:43:04 PDT
(In reply to Ian Neal from comment #58)
> >+function isLegalIPv4Address(aHostName, aAllowExtendedIPFormats)
> >+{
> 
> >+      // is the component decimal?
> >+      if (/^(0|([1-9][0-9]{0,2}))$/.test(ipComponents[i])) {
> >+        ipComponents[i] = parseInt(ipComponents[i], 10);
> >+      } else if (aAllowExtendedIPFormats) {
> >+        // is the component octal?
> >+        if (/^(0[0-7]{1,3})$/.test(ipComponents[i]))
> >+          ipComponents[i] = parseInt(ipComponents[i], 8);
> >+        // is the component octal?
> >+        else if (/^(0x[0-9a-f]{1,2})$/.test(ipComponents[i]))
> >+          ipComponents[i] = parseInt(ipComponents[i], 16);
> >+        else
> >+          return null;
> >+      } else {
> >+        return null;
> >+      }
> I think you are over-complicating it here. Surely something like the
> following would do:
> ipComponents[i] = aAllowExtendedIPFormats ? parseInt(ipComponents[i]) :parseInt(ipComponents[i], 10);
As said, parseInt() accepts trailing garbage which we don't want...

> >+function isLegalIPv6Address(aHostName)
> >+{
> 
> >+  // Take care if the last part is written in decimal using dots as separators.
> >+  let lastPart = isLegalIPv4Address(ipComponents[ipComponents.length - 1], true);
> Do you really mean true here?
Good catch! No, it is a remnant of inverting the argument meaning :)
Comment 60 Ian Neal 2012-05-22 07:55:08 PDT
(In reply to :aceman from comment #59)
> (In reply to Ian Neal from comment #58)
> > >+function isLegalIPv4Address(aHostName, aAllowExtendedIPFormats)
> > >+{
> > 
> > >+      // is the component decimal?
> > >+      if (/^(0|([1-9][0-9]{0,2}))$/.test(ipComponents[i])) {
> > >+        ipComponents[i] = parseInt(ipComponents[i], 10);
> > >+      } else if (aAllowExtendedIPFormats) {
> > >+        // is the component octal?
> > >+        if (/^(0[0-7]{1,3})$/.test(ipComponents[i]))
> > >+          ipComponents[i] = parseInt(ipComponents[i], 8);
> > >+        // is the component octal?
> > >+        else if (/^(0x[0-9a-f]{1,2})$/.test(ipComponents[i]))
> > >+          ipComponents[i] = parseInt(ipComponents[i], 16);
> > >+        else
> > >+          return null;
> > >+      } else {
> > >+        return null;
> > >+      }
> > I think you are over-complicating it here. Surely something like the
> > following would do:
> > ipComponents[i] = aAllowExtendedIPFormats ? parseInt(ipComponents[i]) :parseInt(ipComponents[i], 10);
> As said, parseInt() accepts trailing garbage which we don't want...
What sort of trailing garbage are you talking about? 0x and 0 or something else?
Comment 61 :aceman 2012-05-22 09:08:07 PDT
See comment 46: E.g. "::ffffty:192.0.2.128 yy" is accepted as a valid address, because parseInt processes ffffty as ffff and skips the rest of the component. The same with "128 yy".
Comment 62 Ian Neal 2012-05-27 09:43:09 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

>+++ b/mailnews/base/prefs/content/AccountManager.js
>@@ -265,20 +265,25 @@ function onAccept() {
> function checkUserServerChanges(showAlert) {
>   const prefBundle = document.getElementById("bundle_prefs");
>   const alertTitle = prefBundle.getString("prefPanel-server");
>   var alertText = null;
>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"]
>                             .document.getElementById("smtp.hostname");
Is this ever defined? As far as I can see "smtp.hostname" is only in smtpEditOverlay.xul and that never seems to call the function checkUserServerChanges
Comment 63 :aceman 2012-05-27 10:29:56 PDT
I think this is called when exiting Account manager.
smtpEditOverlay.xul does not need to call checkUserServerChanges(). The question is if checkUserServerChanges() can find the "smtp.hostname" element in the smtpEditOverlay.xul pane, if that can be found in top.frames["contentFrame"]. I don't know, I just update existing callers of hostnameIsIllegal().
Comment 64 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-31 21:41:51 PDT
> Boris, it there some code in Firefox already checking validity of hostnames?

Sorry, I must have missed this question....  I don't actually know offhand.  biesi might know.
Comment 65 Ian Neal 2012-06-02 15:34:04 PDT
(In reply to :aceman from comment #63)
> I think this is called when exiting Account manager.
Yes, it is, or switching between panes.
> smtpEditOverlay.xul does not need to call checkUserServerChanges(). The
> question is if checkUserServerChanges() can find the "smtp.hostname" element
> in the smtpEditOverlay.xul pane, if that can be found in
> top.frames["contentFrame"].
No, it cannot, this happened after the patch on Bug 525238 landed.
I'm not even sure the code is still needed as it is not currently called and doesn't seem to be causing any problems. There is already a check in the onAccept function in SmtpServerEdit.js
Comment 66 :aceman 2012-06-02 15:49:52 PDT
Should I remove it?
Comment 67 Ian Neal 2012-06-02 16:01:55 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

>+++ b/mailnews/base/util/hostnameUtils.jsm

>+function isLegalIPAddress(aHostName, aAllowExtendedIPFormats)
>+{
>+  return isLegalIPv4Address(aHostName, aAllowExtendedIPFormats) || isLegalIPv6Address(aHostName, aAllowExtendedIPFormats);
>+}
Nit: split this over two lines.

>+function isLegalLocalIPAddress(aIPAddress)
>+{

>+  // IPv6 address?
>+  ipComponents = aIPAddress.split(":");
>+  if (ipComponents.length == 8)
Is it worth returning false at this point if it is not 8? Saying that we should only be passed legal IP addresses, so if it is not IPv4 then it must be IPv6.
Comment 68 Ian Neal 2012-06-02 16:10:39 PDT
(In reply to :aceman from comment #66)
> Should I remove it?

I would say so, Neil might have a view as he was involved in the review of Bug 525238
Comment 69 neil@parkwaycc.co.uk 2012-06-17 09:37:27 PDT
(In reply to Ian Neal from comment #65)
> (In reply to aceman from comment #63)
> > I think this is called when exiting Account manager.
> Yes, it is, or switching between panes.
> > smtpEditOverlay.xul does not need to call checkUserServerChanges(). The
> > question is if checkUserServerChanges() can find the "smtp.hostname" element
> > in the smtpEditOverlay.xul pane, if that can be found in
> > top.frames["contentFrame"].
> No, it cannot, this happened after the patch on Bug 525238 landed.
Earlier than that, I think you'll find: bug 202468.
Comment 70 neil@parkwaycc.co.uk 2012-06-18 09:25:30 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

I'm not terribly keen on your drive-by var/let changes.

>+        // is the component octal?
>+        if (/^(0[0-7]{1,3})$/.test(ipComponents[i]))
>+          ipComponents[i] = parseInt(ipComponents[i], 8);
>+        // is the component octal?
Comment oops ;-)

>+    // Convert to a binary to test for possible DWORD.
>+    let binaryDword = parseInt(aHostName).toString(2);
>+    if (isNaN(binaryDword))
What is this trying to do?

Last time I looked at this I had a vague memory of there being something else that I didn't think looked right, but I can't find it now.
Comment 71 :aceman 2012-06-18 11:43:40 PDT
(In reply to neil@parkwaycc.co.uk from comment #70)
> >+    // Convert to a binary to test for possible DWORD.
> >+    let binaryDword = parseInt(aHostName).toString(2);
> >+    if (isNaN(binaryDword))
> What is this trying to do?

No idea, it is copied code.
Comment 72 Ian Neal 2012-07-07 10:04:31 PDT
(In reply to neil@parkwaycc.co.uk from comment #70)
> Comment on attachment 625777 [details] [diff] [review]
> patch v7
> 
> >+    // Convert to a binary to test for possible DWORD.
> >+    let binaryDword = parseInt(aHostName).toString(2);
> >+    if (isNaN(binaryDword))
> What is this trying to do?
Presumably this is checking this is a valid DWORD, so as well as checking that it is a number you also to need to check the value is within the number range that is a DWORD (as far as I am aware that means it must be > 0 and the length of the binary string cannot be more than 32).
Comment 73 Ian Neal 2012-07-07 10:07:33 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

f- as need to deal with the smtp.hostname and DWORD checking issues.
Comment 74 neil@parkwaycc.co.uk 2012-07-08 08:48:58 PDT
Comment on attachment 625777 [details] [diff] [review]
patch v7

Well, just to be awkward, I will grant f+ because the DWORD issue is just moving bad code*, so you can be forgiven for not fixing it, and the smtp.hostname issue is dead code (checkUserServerChanges should die), and so it won't matter whether you fix that either.

*Rather like IPv6 not requiring all components (::0:: assumed), IPv4 does not require all components either. In this case, the last component can be a larger integer to make up the missing bytes. Decimal, octal and hex are all legal. Some examples in hex: 0xC0.0xA8.0x90.0x78 can be written 0xC0.0xA8.0x9078 or 0xC0.0xA89078 or 0xC0A89078 and they are all the same address.
Comment 75 :aceman 2012-07-20 08:27:57 PDT
OK, I'll remove the smtp block.

Neil, the existing code does not seem to allow the IPv4 version with less than 3 dots. It seems firefox accepts even numbers (decimal an hex) without dots and converts them to IPv4 addresses.
I am not sure we want to allow this all.

checkUserServerChanges() is a useful function (except the smtp check) and I am still adding new checks to it. Maybe you wanted to say it should be split or reworked in some way?
Comment 76 neil@parkwaycc.co.uk 2012-11-07 12:26:27 PST
(In reply to aceman from comment #75)
> checkUserServerChanges() is a useful function (except the smtp check) and I
> am still adding new checks to it. Maybe you wanted to say it should be split
> or reworked in some way?
Sorry, I had overlooked that there were other checks in that function.
Comment 77 :aceman 2012-11-07 12:36:21 PST
Yeah, the function name should have been extended :)

What about the other points in comment 75?
Comment 78 neil@parkwaycc.co.uk 2012-11-07 12:50:46 PST
(In reply to :aceman from comment #75)
> Neil, the existing code does not seem to allow the IPv4 version with less
> than 3 dots. It seems firefox accepts even numbers (decimal an hex) without
> dots and converts them to IPv4 addresses.
> I am not sure we want to allow this all.
Well, seeing as IPv6 addresses get canonicalised, it would be a valid enhancement.
Comment 79 :aceman 2012-11-07 13:03:44 PST
They do not get canonicalized in the textbox. The intention is to preserve what the user enters. We just canonicalize it internally to see if it is valid.
Yes, the new functions return a canonicalized version, but we do not return it back into the account manager textboxes (the actual usage of the new functions is bug 327812, but I'll implement it the way I say).

I'll try to refresh the patch and spin off the special formats from comment 74 into a new bug.
Comment 80 :aceman 2012-11-07 13:45:28 PST
Created attachment 679355 [details] [diff] [review]
patch v8
Comment 81 neil@parkwaycc.co.uk 2012-11-08 05:15:01 PST
Comment on attachment 679355 [details] [diff] [review]
patch v8

>-        gSmtpHostNameIsIllegal = true;
[Now this variable becomes entirely unnecessary...]

>+Components.utils.import("resource:///modules/mailServices.js");
> Components.utils.import("resource://gre/modules/Services.jsm");
>-Components.utils.import("resource:///modules/mailServices.js");
Oops. Mismerge? This switch happens twice. r=me with them fixed.

>+++ b/mailnews/base/util/hostnameUtils.jsm
I'm going to assume that we've covered this well enough already.
Comment 82 :aceman 2012-11-08 05:46:52 PST
(In reply to neil@parkwaycc.co.uk from comment #81)
> >+++ b/mailnews/base/util/hostnameUtils.jsm
> I'm going to assume that we've covered this well enough already.
Yes, I have not done any functional changes there since last reviews, only Ian's nits and function descriptions.
Comment 83 :aceman 2012-11-09 12:23:33 PST
Created attachment 680189 [details] [diff] [review]
patch v9

>>-        gSmtpHostNameIsIllegal = true;
>[Now this variable becomes entirely unnecessary...]
This also allowed to remove some more code attached to the variable.
Comment 84 Ian Neal 2012-11-10 16:45:12 PST
Comment on attachment 680189 [details] [diff] [review]
patch v9

Great work, thanks.
Comment 85 :aceman 2012-11-11 05:14:27 PST
Created attachment 680435 [details] [diff] [review]
patch v10

hostnameIsLegal should have been isLegalHostnameOrIp in one spot.
Comment 86 Magnus Melin 2012-11-11 11:18:28 PST
Comment on attachment 680435 [details] [diff] [review]
patch v10

Review of attachment 680435 [details] [diff] [review]:
-----------------------------------------------------------------

Thx aceman! r=mkmelin

::: mailnews/base/util/hostnameUtils.jsm
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*

/** style comment please (and i'd start that sentence from "generic ..."

This allows also e.g. lxr directory listings to say what files do

@@ +7,5 @@
> + * The code here is a generic shared utility code for checking of IP and hostname
> + * validity.
> + */
> +
> +var EXPORTED_SYMBOLS = [ "isLegalHostNameOrIP",

should really be const

@@ +124,5 @@
> +    ipComponents[0] = (aHostName >> 24) & 255;
> +    ipComponents[1] = (aHostName >> 16) & 255;
> +    ipComponents[2] = (aHostName >>  8) & 255;
> +    ipComponents[3] = (aHostName & 255);
> +  } else {

else on new row

@@ +212,5 @@
> +    if (/^[0-9a-f]{1,4}$/.test(ipComponents[i])) {
> +      ipComponents[i] = parseInt(ipComponents[i], 16);
> +      if (isNaN(ipComponents[i]) || ipComponents[i] > 0xffff)
> +        return null;
> +    } else {

new row
Comment 87 :aceman 2012-11-11 11:26:44 PST
Created attachment 680455 [details] [diff] [review]
patch v11

Thanks!
Comment 88 Ryan VanderMeulen [:RyanVM] 2012-11-11 14:13:55 PST
https://hg.mozilla.org/comm-central/rev/a7b5def15d8c

Should this have tests?
Comment 89 :aceman 2012-11-11 23:42:18 PST
Yes, I'll do some in bug 810763 after bug 327812 lands.
Comment 90 Jens Hatlak (:InvisibleSmiley) 2012-11-16 07:37:58 PST
Error: SyntaxError: missing ) after condition
Source File: chrome://messenger/content/aw-outgoing.js
Line: 16, Column: 60
Source Code:
ultSMTP && !isLegalHostNameOrIP(cleanUpHostname(smtpServer));

Please fix ASAP (simple s/;/)/ AFAICS), this prevented me from creating a new news account in SM (didn't check TB).
Comment 91 Jens Hatlak (:InvisibleSmiley) 2012-11-16 07:56:25 PST
Even with the error from comment 90 fixed locally I could not create a news account. I got:

Error: ReferenceError: newsServerName is not defined
Source File: chrome://messenger/content/aw-incoming.js
Line: 35

It seems this is because "newsServerName" is only defined using "let" inside an "if" above; declaring variables for "incomingServerName" and "newsServerName" using "var" at the beginning of the function fixed it for me locally. Please fix this, too.

I wonder whether nothing of the above showed up when using TB. If it did, maybe the rest of the patch needs closer inspection, too.
Comment 92 :aceman 2012-11-16 08:54:23 PST
I see the problems, thanks.
I think the aw-* files are not used in TB. TB uses the mailnews/base/prefs/content/accountcreation/* files.
Comment 93 Ian Neal 2012-11-16 16:13:27 PST
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #91)

> It seems this is because "newsServerName" is only defined using "let" inside
> an "if" above; declaring variables for "incomingServerName" and
> "newsServerName" using "var" at the beginning of the function fixed it for
> me locally. Please fix this, too.
Going back to using var within each if statement works also.
Comment 94 :aceman 2012-11-17 09:14:28 PST
I'd better like to move the variables out of the if blocks as Jens suggests. I don't like that 'var' leaks outside of the containing block feature (and I am not used to it from other languages). So when converting to let I didn't notice (and didn't appear to me to look if) the variables are used later on.

But I can't produce a patch until 19th Nov. I am not sure when the tree is branched. So if it is branched before that date I'd ask Ian to produce the quick fix for the 3 lines please. Thanks.
Comment 95 :aceman 2012-11-17 09:14:29 PST
I'd better like to move the variables out of the if blocks as Jens suggests. I don't like that 'var' leaks outside of the containing block feature (and I am not used to it from other languages). So when converting to let I didn't notice (and didn't appear to me to look if) the variables are used later on.

But I can't produce a patch until 19th Nov. I am not sure when the tree is branched. So if it is branched before that date I'd ask Ian to produce the quick fix for the 3 lines please. Thanks.
Comment 96 Jens Hatlak (:InvisibleSmiley) 2012-11-17 12:23:09 PST
Created attachment 682788 [details] [diff] [review]
fixes for SM part [Checked in: Comment 98]

19th is a bit too close for my taste. I'd say let's fix this properly while we're here, and sooner than later so we don't need to care about the upcoming uplift.

[Ian: Feel free to land right away with any possible review nits addressed; I'll be AFK for most of tomorrow.]
Comment 97 :aceman 2012-11-17 12:32:24 PST
Comment on attachment 682788 [details] [diff] [review]
fixes for SM part [Checked in: Comment 98]

Very nice, thanks.
Comment 98 Ian Neal 2012-11-17 14:10:47 PST
Comment on attachment 682788 [details] [diff] [review]
fixes for SM part [Checked in: Comment 98]

http://hg.mozilla.org/comm-central/rev/1824e8daac72

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