Improving hostname validation

[douglasgabriel]

PR based on this discussion:
ManageIQ/manageiq#16220

This PR is able to:

• validate hostname format in client;
• avoid user to put protocol on hostname field;
• avoid user to put path on hostname field;
• avoid user to put port on hostname field
• show error message to user;
  

[douglasgabriel]

@agrare @rodneyhbrown7

[agrare]

@miq-bot assign @dclarizio

[dclarizio]

@douglasgabriel definitely need to take a look at the codeclimate issues.

[douglasgabriel]

@AndreyMenezes

[agrare]

Hey @AparnaKarve @douglasgabriel fixed the code climate issues, can you take a look?

[rodneyhbrown7]

miq-bot add_label gaprindashvili/yes

[AparnaKarve]

@douglasgabriel In the screenshot above, why is the hostname format incorrect?
Is it because of http?

If that's the case, it's probably not applicable across the board for all hostnames.

[douglasgabriel]

@AparnaKarve this is based on discussion in this PR, only extend the validation to front-end. Can you mention some case that this validation is not applicable?

[AparnaKarve]

@douglasgabriel I don't know off-hand.

What I do know is that my Lenovo XClarity EMS was created using the https prefix.

Although I just verified that it does work even without https specified.

@h-kataria Would you have an idea if we allow using the protocol (http/https) in the hostname in other areas of the app?

[douglasgabriel]

In this validation occurs what I believe that is an inconsistency: the user provides some data (with the protocol, for example) and the system validates only a part of this input (without protocol), after this the system store all data provided by user (with protocol). So, if the user be allowed to provide the protocol, so I believe that the validation strategy must be revised, don't you think?

[saulotoledo]

I believe that the validation in this PR should be the same considered here in the ruby code. Also I think that the behavior considered for the hostname should be the same considered for the IP.

Imagine that the hostname mysample.hostname.com points to the (fake) IP 100.100.100.100. Making an HTTP(S) call to mysample.hostname.com or 100.100.100.100 should return the same results unless there is an explicit requirement not to do so.

See examples of the current PR:

1. mysample.hostname.com <- VALID
2. mysample.hostname.com/path <- VALID
3. 100.100.100.100 <- VALID
4. 100.100.100.100/path <- INVALID

According to the previous explanation, 1 and 4 are equivalent. Thus, 4 should not be invalid (or both 2 and 4 should be invalid).

The first question then: what option should we choose?
For any answer, both PRs should be updated.

In order to contribute with the discussion, I think we should accept the path in the URLs. Let me prove my point with an example: imagine that, for some reason (security, for example) I just have one public IP, but I have several hosts I want to put in my MiQ instance. I can install something in that public IP (Zuul, for example, or any other technology I want) that transparently redirect subdomains to my internal hosts. Something like:

my.public.host.com/myhostname1 -> redirect to my.local.hostname1.com
my.public.host.com/myhostname2 -> redirect to my.local.hostname2.com
my.public.host.com/myhostname3 -> redirect to my.local.hostname3.com

Considering that my.local.* is not visible for the MiQ, this should be a possible situation.

@agrare, @rodneyhbrown7 could you confirm the information and decide about what should be done in these PRs?

[saulotoledo]

I think the following cases should be invalid: 100.100.100.100invalid, invalid100.100.100.100invalid. Could you confirm that?

[saulotoledo]

In addition, this does not support IPV6 format, pointed out in the UI. The following expression can handle ipv4 and ipv6:

return /((^\s*((([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))\s*$)|(^\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\s*$))/.test(hostname);

[agrare]

I think we should accept the path in the URLs.

This won't work across the board because most on-premise providers add a path after the hostname/ipaddr for the API endpoint so you wouldn't be able to tell what component of the path is meant to point to another host and what part is for the API endpoint.

[saulotoledo]

This won't work across the board because most on-premise providers add a path after the hostname/ipaddr for the API endpoint so you wouldn't be able to tell what component of the path is meant to point to another host and what part is for the API endpoint.

Ok. In addition, strictly speaking, host and path are different concepts, and the port has a specific field for it. So, after thinking a little about it, I agree with you. So, can we continue these PRs by removing the path from the host field?

@agrare @rodneyhbrown7 @douglasgabriel

[saulotoledo]

If we consider removing the path (see current discussion), here is my suggestion for this code (needs testing):

(function() {
  angular.module('ManageIQ').directive('hostnameValidation', directive);

  function directive() {
    return {
      require: 'ngModel',
      restrict: 'A',
      link: link,
    };
    function link(_scope, _elem, _attrs, ctrl) {
      ctrl.$validators.hostnameValidation = function(_modelValue, viewValue) {
        return isHostname(viewValue) || isIpAddress(viewValue);
      };

      /**
       * Verifies if the informed value is a valid hostname.
       *
       * @param {string} value - Value to be validated
       * @returns {boolean} True if the value is a valid hostname
       */
      var isHostname = function(value) {
        return /^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z]|[A-Za-z][A-Za-z0-9\-]*[A-Za-z0-9])$/.test(value);
      };

      /**
       * Verifies if the informed value is a valid IP (IPV4 or IPV6).
       *
       * @param {string} value - Value to be validated
       * @returns {boolean} True if the value is a valid IP address (IPV4 or IPV6)
       */
      var isIpAddress = function(value) {
        return /((^\s*((([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))\s*$)|(^\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\s*$))/.test(value);
      };
    }
  }
})();

[agrare]

My lack of regex skill precludes me from reviewing the code changes :)
From a functional standpoint I think we should enforce that only and ip address or dns name was added to the hostname field. No protocol, no paths, no ports.

@douglasgabriel IDK the ui tests very well but I highly recommend adding some tests for this validation.

[douglasgabriel]

Well, it's done guys. I've add the Saulo's solution on the validation and add some tests to it. Now it avoid the user to put protocol, path and port on the hostname field. What do you think @agrare @saulotoledo @AparnaKarve ?

[agrare]

Nice tests @douglasgabriel !

[saulotoledo]

LGTM @douglasgabriel

[AparnaKarve]

From a functional standpoint I think we should enforce that only and ip address or dns name was added to the hostname field. No protocol, no paths, no ports.

@douglasgabriel Based on the benchmark that @agrare mentioned above, your hostname validation directive works.

I did find something in my testing, though.
I tried something that matches this format 100.100.100.100 but interspersed with letters 100xx.100yy.100zz.100aa, and the directive did not flag it as invalid.
From a practical, real-life aspect, users will not enter invalid hostnames like the above example, especially if they want the validation to work -- so I think we can consider this test to be a nit-pick.
But I guess my point is that the regex does not seem to be 100% foolproof.

The regex does satisfy the "No protocol, no paths, no ports" condition, hence I would say -
LGTM.

[miq-bot]

Checked commit douglasgabriel@af4cd88 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 6e39914c763887da8876418751a363ee4fe440cb
Author: Dan Clarizio <[email protected]>
Date:   Mon Dec 18 12:36:57 2017 -0800

    Merge pull request #2455 from douglasgabriel/fix-XCS-404
    
    Improving hostname validation
    (cherry picked from commit a26d12424fe0ab9d5b91155ad6083a8b278b296c)