The default bug view has changed. See this FAQ.

fix hostnameIsIllegal() to properly check validity of hostnames and be more robust

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: aceman)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

Trunk
Thunderbird 19.0
Dependency tree / graph
Bug Flags:
in-testsuite ?

SeaMonkey Tracking Flags

(seamonkey2.16 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

fix hostnameIsIllegal()in aw-server to be more robust

right now, hostnames like "...", "_" and ".foo.bar" will get through.
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.
QA Contact: esther → stephend

Comment 2

16 years ago
Created attachment 61590 [details] [diff] [review]
new hostnameIsIllegal()

What about this?

Comment 3

15 years ago
according to bug 145069, an error message *does* appear when user tries to add a
hostname with a _

Comment 4

15 years ago
hostnameIsIllegal() has now been moved to amUtils.js (utility functions).
mass re-assign.
Assignee: racham → sspitzer
Product: Browser → Seamonkey

Updated

12 years ago
Assignee: sspitzer → mail
QA Contact: stephend

Comment 6

9 years ago
This appears in two places, it should be consolidated.

Comment 7

5 years ago
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
Assignee: mail → nobody
Component: MailNews: Account Configuration → Account Manager
OS: Other → All
Product: SeaMonkey → MailNews Core
QA Contact: account-manager
Hardware: x86 → All
Summary: fix hostnameIsIllegal() in aw-server.js to be more robust → fix hostnameIsIllegal() to be more robust
(Assignee)

Updated

5 years ago
Assignee: nobody → acelists
(Assignee)

Updated

5 years ago
Blocks: 89945
(Assignee)

Comment 8

5 years ago
What about the underscore _ ? Should I implement Bug 89945 comment 16?
(Assignee)

Comment 9

5 years ago
See also text on underscores on http://en.wikipedia.org/wiki/Hostname .

Comment 10

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

Comment 11

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

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

Comment 13

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

Updated

5 years ago
Blocks: 327812
(Assignee)

Comment 14

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

Comment 15

5 years ago
Created attachment 591919 [details] [diff] [review]
more strict version
Attachment #61590 - Attachment is obsolete: true
Attachment #591919 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED

Comment 16

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

5 years ago
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.
Attachment #591919 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 18

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

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

Comment 20

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

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

Comment 22

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

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

Comment 24

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

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

Comment 26

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

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

Comment 28

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

5 years ago
This is also overlapping into Bug 235312
Blocks: 235312

Comment 30

5 years ago
You would need to pass it through idn-service's convertUTF8toACE to ensure it is ascii before testing against the relevant rules.
(Assignee)

Comment 31

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

Updated

5 years ago
Blocks: 604393
(Assignee)

Updated

5 years ago
Depends on: 563172
(Assignee)

Updated

5 years ago
Depends on: 491843
(Assignee)

Comment 32

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

Comment 33

5 years ago
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?
Attachment #591919 - Attachment is obsolete: true
Attachment #596778 - Flags: feedback?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Attachment #596778 - Flags: feedback?(mbanner)
(Assignee)

Comment 34

5 years ago
If patch from bug 491843 sticks, we can enable IPv6 here.
(Assignee)

Updated

5 years ago
Depends on: 733210
(Assignee)

Comment 35

5 years ago
Can we move here in any way?
(Assignee)

Comment 36

5 years ago
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?
Summary: fix hostnameIsIllegal() to be more robust → fix hostnameIsIllegal() to properly check validity of hostnames and be more robust

Comment 37

5 years ago
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
Attachment #596778 - Flags: feedback?(iann_bugzilla) → feedback-
(Assignee)

Comment 38

5 years ago
(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.
No longer blocks: 235312, 327812, 604393, 89945
No longer depends on: 563172, 733210, 491843
(Assignee)

Updated

5 years ago
Depends on: 563172, 733210, 491843

Updated

5 years ago
Depends on: 756821

Comment 39

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

Comment 40

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

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

Updated

5 years ago
Depends on: 738810
(Assignee)

Comment 42

5 years ago
Created attachment 625491 [details] [diff] [review]
patch v4
Attachment #596778 - Attachment is obsolete: true
Attachment #596778 - Flags: feedback?(mbanner)
Attachment #625491 - Flags: review?(neil)
Attachment #625491 - Flags: feedback?(iann_bugzilla)
(Assignee)

Updated

5 years ago
Blocks: 756821
No longer depends on: 756821

Comment 43

5 years ago
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.
Attachment #625491 - Flags: feedback?(iann_bugzilla) → feedback-
(Assignee)

Comment 44

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

Updated

5 years ago
Keywords: uiwanted
(Assignee)

Comment 45

5 years ago
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.
Attachment #625491 - Flags: ui-review?(bwinton)
(Assignee)

Comment 46

5 years ago
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.
Attachment #625491 - Attachment is obsolete: true
Attachment #625491 - Flags: ui-review?(bwinton)
Attachment #625491 - Flags: review?(neil)
Attachment #625515 - Flags: ui-review?(bwinton)
Attachment #625515 - Flags: feedback?(neil)
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.)
Attachment #625515 - Flags: feedback?(neil) → feedback-
(Assignee)

Comment 48

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

Comment 50

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

Comment 52

5 years ago
IanN, any comments?
(Assignee)

Updated

5 years ago
No longer blocks: 756821
Depends on: 756821
(Assignee)

Comment 53

5 years ago
Ian, are your changes to phishingDetector done? Can I update the patch?
(Assignee)

Comment 54

5 years ago
Created attachment 625747 [details] [diff] [review]
patch v6

What about this?
Attachment #625515 - Attachment is obsolete: true
Attachment #625515 - Flags: ui-review?(bwinton)
Attachment #625747 - Flags: feedback?(neil)
(Assignee)

Updated

5 years ago
Attachment #625747 - Flags: feedback?(iann_bugzilla)

Comment 55

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

Comment 56

5 years ago
Thanks, I missed that.
(Assignee)

Comment 57

5 years ago
Created attachment 625777 [details] [diff] [review]
patch v7
Attachment #625747 - Attachment is obsolete: true
Attachment #625747 - Flags: feedback?(neil)
Attachment #625747 - Flags: feedback?(iann_bugzilla)
Attachment #625777 - Flags: feedback?(neil)
(Assignee)

Updated

5 years ago
Attachment #625777 - Flags: feedback?(iann_bugzilla)

Comment 58

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

Comment 59

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

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

Comment 61

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

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

Comment 63

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

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

Comment 66

5 years ago
Should I remove it?

Comment 67

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

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

Comment 71

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

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

5 years ago
Comment on attachment 625777 [details] [diff] [review]
patch v7

f- as need to deal with the smtp.hostname and DWORD checking issues.
Attachment #625777 - Flags: feedback?(iann_bugzilla) → feedback-
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.
Attachment #625777 - Flags: feedback?(neil) → feedback+
(Assignee)

Comment 75

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

Updated

4 years ago
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 77

4 years ago
Yeah, the function name should have been extended :)

What about the other points in comment 75?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(Assignee)

Comment 79

4 years ago
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.
Keywords: uiwanted
(Assignee)

Updated

4 years ago
Blocks: 809609
(Assignee)

Comment 80

4 years ago
Created attachment 679355 [details] [diff] [review]
patch v8
Attachment #625777 - Attachment is obsolete: true
Attachment #679355 - Flags: review?(neil)
(Assignee)

Updated

4 years ago
Attachment #679355 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

4 years ago
Attachment #679355 - Flags: review?(mkmelin+mozilla)
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.
Attachment #679355 - Flags: review?(neil) → review+
(Assignee)

Comment 82

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

Comment 83

4 years ago
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.
Attachment #679355 - Attachment is obsolete: true
Attachment #679355 - Flags: review?(mkmelin+mozilla)
Attachment #679355 - Flags: review?(iann_bugzilla)
Attachment #680189 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

4 years ago
Attachment #680189 - Flags: review?(mkmelin+mozilla)

Comment 84

4 years ago
Comment on attachment 680189 [details] [diff] [review]
patch v9

Great work, thanks.
Attachment #680189 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 85

4 years ago
Created attachment 680435 [details] [diff] [review]
patch v10

hostnameIsLegal should have been isLegalHostnameOrIp in one spot.
Attachment #680189 - Attachment is obsolete: true
Attachment #680189 - Flags: review?(mkmelin+mozilla)
Attachment #680435 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

4 years ago
Blocks: 810680

Comment 86

4 years ago
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
Attachment #680435 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 87

4 years ago
Created attachment 680455 [details] [diff] [review]
patch v11

Thanks!
Attachment #680435 - Attachment is obsolete: true
Attachment #680455 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a7b5def15d8c

Should this have tests?
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
(Assignee)

Updated

4 years ago
Blocks: 810763
(Assignee)

Comment 89

4 years ago
Yes, I'll do some in bug 810763 after bug 327812 lands.
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).
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.
(Assignee)

Comment 92

4 years ago
I see the problems, thanks.
I think the aw-* files are not used in TB. TB uses the mailnews/base/prefs/content/accountcreation/* files.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 93

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

Comment 94

4 years ago
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.
Flags: needinfo?(iann_bugzilla)
(Assignee)

Comment 95

4 years ago
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.
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.]
Attachment #682788 - Flags: superreview?
Attachment #682788 - Flags: review?(iann_bugzilla)
Flags: needinfo?(iann_bugzilla)
(Assignee)

Comment 97

4 years ago
Comment on attachment 682788 [details] [diff] [review]
fixes for SM part [Checked in: Comment 98]

Very nice, thanks.
Attachment #682788 - Flags: superreview? → feedback+

Comment 98

4 years ago
Comment on attachment 682788 [details] [diff] [review]
fixes for SM part [Checked in: Comment 98]

http://hg.mozilla.org/comm-central/rev/1824e8daac72
Attachment #682788 - Attachment description: fixes for SM part → fixes for SM part [Checked in: Comment 98]
Attachment #682788 - Flags: review?(iann_bugzilla) → review+

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-seamonkey2.16: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 913785
You need to log in before you can comment on or make changes to this bug.