[Security Solution][Detections] Fixes 'Detection Rule "reference url" links don't work if they are created with missing "http://'

[MadameSheema]

Summary

In this PR we are fixing the following issue: #99742

In order to fix it, what we are doing is to don't accept URL's that does not contain the http:// https:// prefix, updating the regex used for the URL validation.

[MadameSheema]

@elasticmachine merge upstream

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[ecezalp]

LGTM

[banderror]

LGTM 👍

Just wondering, could we make the validation message more descriptive? E.g. something like

The URLs entered must be valid and start with http:// or https://

[MadameSheema]

Just wondering, could we make the validation message more descriptive? E.g. something like

The URLs entered must be valid and start with http:// or https://

@banderror I think is a great idea, @peluja1012 @rylnd what do you think?

[FrankHassanabad]

The existing regex you are modifying is not very robust and misses a lot of common use cases.

Examples of tests that it will fail when it should pass would be:

    test('should verify valid wwww such as 4 of them.', () => {
      expect(isUrlInvalid('https://wwww.example.com')).toBeFalsy();
    });

    test('should validate characters such as %22 being part of a correct URL.', () => {
      expect(isUrlInvalid('https://www.exam%22ple.com')).toBeFalsy();
    });

    test('should validate characters incorrectly such as ;', () => {
      expect(isUrlInvalid('https://www.example.com[')).toBeTruthy();
    });

I think also this should include at least one test case for http such as:

    test('verifies valid http url', () => {
      expect(isUrlInvalid('http://www.example.com/')).toBeFalsy();
    });

I always look for what is missing first before moving forward.

If you look online you will see a lot of ambiguous language around the RFC for what are valid and not valid URI characters.
https://datatracker.ietf.org/doc/html/rfc3986
https://stackoverflow.com/questions/1547899/which-characters-make-a-url-invalid

But those I think are common use cases. The encoded characters such as %22 are very common in URL copying and pasting. I think a better way would be to swap the code to use new URL such as this something like this below:

const allowedSchemes = ['http:', 'https:'];

export const isUrlInvalid = (url: string | null | undefined) => {
  try {
    if (url != null) {
      const urlParsed = new URL(url);
      if (allowedSchemes.includes(urlParsed.protocol)) {
        return false;
      }
    }
  } catch (error) {
    // intentionally left empty
  }
  return true;
};

Although this name is isUrlInvalid most developers prefer the positive case such as isUrlValid rather than the negative case fwiw as well if you want to refactor towards that.

[FrankHassanabad]

One more test case that would be helpful reading the code below is to check to ensure empty strings return false since there was an original isEmpty(url) and we want to ensure that the wanted behaviors are the same before any refactoring. So this test would be helpful to add to preserve the original behavior first before the refactoring.

    test('verifies as valid when given an empty string', () => {
      expect(isUrlInvalid('')).toBeFalsy();
    });

Now if you look at my re-factored code above I am introducing an accidental bug and behavioral change because I am not checking this empty string condition so it is a bug. So in reality my code should be more something like this:

const allowedSchemes = ['http:', 'https:'];

export const isUrlInvalid = (url: string | null | undefined) => {
  try {
    if (url != null) {
      if (url === '') {
        return false;
      } else {
        const urlParsed = new URL(url);
        if (allowedSchemes.includes(urlParsed.protocol)) {
          return false;
        }
      }
    }
  } catch (error) {
    // intentionally left empty
  }
  return true;
};

It's a bit more noisy but it adheres to the original behavior which looks to involve allowing a completely empty string to work which I think the original author wanted.

One more test that would be helpful is to double check our understanding of "what about empty spaces"? If the original code did not allow empty spaces, then we should add that test case as well for ourselves and for other programmers refactoring later on such as:

    test('empty spaces should valid as not valid ', () => {
      expect(isUrlInvalid(' ')).toBeTruthy();
    });

A lot of times as I change and refactor code I keep the "old code" commented out and then write the tests to ensure each behavior until I remove the older code for the newer refactored version. I like to swap between them as I encounter more of these types of things to broaden my understanding.

As noted below and to the left it's no mistake I made most of my tests use example.com as that is the recommended URL for testing:
https://datatracker.ietf.org/doc/html/rfc2606#page-2

Lastly, I think JSDocs and looking for examples of them within kibana-core would be helpful to add to the function, including what is looking like a "gotcha" around how empty strings validate as valid URL's.

[FrankHassanabad]

👍

[FrankHassanabad]

nit: You could maybe combine these as if (url != null || url === '') { if you want. Most people do, but this is still fine as is to me as this overall PR improves things.

[FrankHassanabad]

I left a few more comments, but I would resolve things you have taken as feedback if you could. Anything left not addressed people typically see it as the nits weren't resolved but we all fine with leaving nits.

PR's shouldn't go on too long but not too short either in a way.

[MadameSheema]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c429425
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	4.3MB	4.3MB	-143.0B

History

• 💔 Build #156566 failed 9932e66
• 💔 Build #156560 failed d7d8d45
• 💛 Build #155593 was flaky b170b5c
• 💚 Build #154217 succeeded 395d7dd
• 💔 Build #154175 failed ba7a17c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @MadameSheema

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.