[Security Solution] Invalid KQL Query Bug

[dplumlee]

Summary

Addresses #98283

Currently, our method of converting KQL to Elasticsearch queries silently suppresses errors bubbled up by ES and returns an empty query string. This makes it so the entire query, including filters, etc. gets wiped out and potentially incorrect data is displayed.

This PR addresses that by bubbling up the errors and putting them in a toast component as well as cancelling any request that was made with the invalid query so that incorrect data is never fetched.

Checklist

Delete any items that are not applicable to this PR.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[XavierM]

You are not lucky because only spellings that I can fix are the french ones ;)

Suggested change

	* Deprecated in leiu of `convertToBuildEsQueryOrError`
	* Deprecated in lieu of `convertToBuildEsQueryOrError`

[XavierM]

just thinking out loud here, instead of creating convertToBuildEsQueryOrError, I am wondering if convertToBuildEsQuery can return a type like that [string | undefined, Error | undefined] or we can even be stricter [string, undefined] | [undefined, Error]
Since we are changing the type, we can do a little more to avoid the deprecation and this will also avoid to call esQuery.buildEsQuery twice to get the error and it will simplify your new hook useInvalidFilterQuery to display the error. I am always scared of function that can return the valid answer or an error.

[dplumlee]

We could do that, i think that's a good idea to condense the two and update inline instead of creating the new function. @spong @andrew-goldstein thoughts?

[elasticmachine]

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

[dplumlee]

Is this the right approach here? Our plan was to skip the tab filter queries so this just continues the logic that was in place before, just further down the line

[XavierM]

I think you can go for it and add addError as a dependency, because you won't have kqlError != null

[andrew-goldstein]

Thanks for this thoughtful, surgical fix @dplumlee!

I'm seeing an unexpected result that doesn't appear to be introduced by this PR, but the fix makes the unexpected result more obvious:

1. To ensure there's no existing URL state, open a new tab and visit http://localhost:5601

2. Navigate to Security > Detections

3. Enter the following KQL, which intentionally has an extraneous trailing double quote, in the search bar and press Enter:

not file.path : C:\elk\mimikatz.exe"

Expected result:

• The new KQLSyntaxError toast appears

4. Click the X button on the toast to dismiss it

5. Remove the extra trailing quote from the search bar, such that the query is valid as shown in the example below and press Enter:

not file.path : C:\elk\mimikatz.exe

Expected result:

• The now-valid query executes, and detections appear in the table

Actual result:

• The new query appears to execute (the Refresh button changes state, and loading indicators are displayed)
• The table doesn't update

Additional observations:

• The unexpected behavior is still reproducible if, in step 5, the Refresh button is clicked (as opposed to pressing Enter) after fixing the query to make it valid
• When the page is in the unexpected state described above, clicking the Refresh button a 2nd time does appear to re-execute the query, and the results are then displayed (as expected)
• To determine whether or not this is a new or existing behavior, I applied the technique above (transitioning the query state from invalid to valid) on a rule details page (using [Security Solution]Incorrect detection alert list apear on searching event in search bar under individual Rule page #98283 as guidance), and I'm seeing similar behavior. Specifically, after transitioning the query from invalid to valid and then inspecting the query, it doesn't appear that the newly-valid KQL is being applied. Based on the above, I don't think a change in this PR introduced this unexpected behavior, but the fix makes it more obvious.

[XavierM]

@andrew-goldstein good catch 🎣 as always. @dplumlee I think to fix this behavior without you going crazy, will be to override the refetch function to show the toaster again.

[andrew-goldstein]

@andrew-goldstein good catch 🎣 as always. @dplumlee I think to fix this behavior without you going crazy, will be to override the refetch function to show the toaster again.

There appears to be a similar statefulness issue with the Inspect action:

1. To ensure there's no existing URL state, open a new tab and visit http://localhost:5601

2. Navigate to Security > Detections

3. Enter the following KQL, which intentionally has an extraneous trailing double quote, in the search bar and press Enter:

not file.path : C:\elk\mimikatz.exe"

Expected result:

• The new KQLSyntaxError toast appears

4. Click the X button on the toast to dismiss it

5. Hover over the Inspect button icon and click it

Expected result:

• The Inspect dialog appears

Actual result:

• The Inspect dialog does NOT appear

6. Completely clear the KQL bar and press Enter

Expected results:

• The table refreshes to display data
• Nothing else happens

Actual results:

• The table refreshes to display data
• The Inspect dialog unexpectedly appears after the table displays the data

[andrew-goldstein]

In the (specific) case of a KQL syntax error, the internal Kibana stack trace of the error that was thrown will not be useful to the user. All the relevant feedback is contained in the message property of the JSON error shown in the screenshot below:

Consider, if this suggestion doesn't introduce significant complexity, eliminating the stack trace in the screenshot above, by displaying only the message property of the error when, specifically, a KQL syntax error is thrown.

[MadameSheema]

Hi team! Thanks for working on this :)

BC5 has been already shipped, last BC (BC6) is planned for next week. If you think that this fix is risky, we should probably wait for 7.13.1.

[andrew-goldstein]

BC5 has been already shipped, last BC (BC6) is planned for next week. If you think that this fix is risky, we should probably wait for 7.13.1.

As a team, we've collaborated and iterated on the fix to minimize risk, and we are all still in agreement it should be included in the final BC. I'm also thoroughly desk testing it as part of the review.

@MadameSheema, would you be willing to coordinate with the team such that the fix is tested as soon as it's merged to master (ahead of the next BC)?

[MadameSheema]

Thanks for the heads up @andrew-goldstein :)

I'll coordinate with the team to make sure this issue is tested on master as soon as is merged.

[andrew-goldstein]

Ideally we might avoid mutating the kqlError arg, because consumers of the hook are probably not expecting this side effect.

As an alternative to using delete to remove the stack, I would typically recommend using a utility function like omit from lodash/fp, because it's guaranteed not to mutate the original, and it's less verbose than the native TS alternative. Unfortunately when I tried that, the error toaster didn't behave as expected, because the implementation of addError uses IEsError in a TS type guard.

I experimented with a few different options for safely cloning and/or constructing a new error (that omits the stack), but each path I explored had some unexpected "gotchas".

To keep this PR moving forward, consider documenting the side effect of mutating kqlError if we can't reasonably eliminate it.

[andrew-goldstein]

It looks like the KPIs in Timeline are showing unexpected results when a KQLSyntaxError is thrown. To reproduce:

1. To ensure there's no existing URL state, open a new tab and visit http://localhost:5601

2. Navigate to the Security Solution

3. Click Untitled timeline to open Timeline

4. Enter the following KQL, which intentionally contains a file.path with a username, pr_99442, that won't match any documents:

file.path: "C:\Users\pr_99442\Downloads\mimikatz_trunk (1)\x64\mimikatz.exe"

Expected result:

• The KPIs at the top of Timeline show all zeros, as shown in the screenshot below:

5. Next, remove both the leading and trailing quotes in the KQL, as shown in the query below, and press Enter:

file.path: C:\Users\pr_99442\Downloads\mimikatz_trunk (1)\x64\mimikatz.exe

Expected results:

• The error toaster appears
• The KPIs revert to their initial "all dashes" - state, shown in the screenshot below, because nothing matches an invalid query:

Above: The KPIs are displayed with -s when Timeline is first opened, because there are no matching documents

Actual result:

• The error toaster appears
• The KPIs that previously displayed all zeros, (because the first query was valid, but didn't match any results) now display (unexpected) non-zero results, per the screenshot below:

[andrew-goldstein]

I'm seeing two unexpected behaviors in Timeline:

1. Clicking the Refresh button while the query is invalid sometimes (other times it hangs, per #2, below) refreshes the grid with invalid results:

2. Clicking the Refresh button when the query is invalid sometimes causes timeline to hang forever in the "Loading Events..." state:

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c77026a
• Storybooks Preview

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	2186	2187	+1

Async chunks

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

id	before	after	diff
securitySolution	6.3MB	6.3MB	+7.8KB

History

• 💔 Build #135369 failed d3805a6
• 💚 Build #133892 succeeded e27399549870105825bbe88dd2cc636423283f1d
• 💔 Build #133874 failed 9ce26b97bae62e192ec000d2002b3e5ae73bbc35
• 💔 Build #133851 failed 1fbb66e2016b55b1dabe4147b9d49036ab267f4f
• 💔 Build #133845 failed c3a9fd34539dc34c794b738f6160f404fb930e87

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

cc @dplumlee

[andrew-goldstein]

Thanks for all your effort on this fix @dplumlee!
Per a discussion with @peluja1012, let's merge this immediately ahead of the BC that's about to be built, and fix the last known issue in a follow-up PR
LGTM

[kibanamachine]

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 99442