From 2c5ae3609050feca233f9c4dc64ffb4993c45bce Mon Sep 17 00:00:00 2001 From: Ievgen Sorokopud Date: Thu, 15 Aug 2024 13:44:48 +0200 Subject: [PATCH] [Detections] EQL query validation: Separate syntax errors into a own error type (#10181) (#190149) ## Summary Addresses https://github.com/elastic/security-team/issues/10181 This PR is a refactoring of EQL query validator: * to separate different validation errors passed from ES. Before we marked `parsing_exception`, `verification_exception` and `mapping_exception` as the same error of type `ERR_INVALID_EQL`. After these changes we will split these errors into separate ones: syntax (`parsing_exception`), invalid EQL (`verification_exception` and `mapping_exception`; can be split in future if needed) * to handle missing data source as a new EQL error of type `MISSING_DATA_SOURCE`. Before `data.search.search()` call would throw an exception in case data source does not exist and we would handle it as a failed request and show an error toast (see relevant ticket https://github.com/elastic/kibana/issues/178611). After these changes we would not show a toast and handle missing data source error as other EQL validation errors - showing an error message in the EQL query bar. This will allow us to distinguish between different types of EQL validation errors and will help to decide on whether certain errors are blocking during the rule creation/editing flow (https://github.com/elastic/kibana/issues/180407). ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [Integration: Rule execution](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6754) (100 ESS & 100 Serverless) - [Cypress: Detection Engine](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6766) (100 ESS & 100 Serverless) --------- Co-authored-by: Elastic Machine --- .../eql/validation/helpers.mock.ts | 26 +++ .../search_strategy/eql/validation/helpers.ts | 15 ++ .../public/common/hooks/eql/api.test.ts | 162 ++++++++++++++++++ .../public/common/hooks/eql/api.ts | 88 +++++++--- .../eql_query_bar/validators.mock.ts | 4 +- .../components/eql_query_bar/validators.ts | 22 +-- .../rule_creation/eql_rule.cy.ts | 62 +++++++ .../cypress/screens/create_new_rule.ts | 3 + 8 files changed, 342 insertions(+), 40 deletions(-) create mode 100644 x-pack/plugins/security_solution/public/common/hooks/eql/api.test.ts diff --git a/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.mock.ts b/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.mock.ts index ca44885d164d6..86b5d246ff378 100644 --- a/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.mock.ts +++ b/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.mock.ts @@ -56,6 +56,32 @@ export const getEqlResponseWithValidationErrors = (): ErrorResponse => ({ }, }); +export const getEqlResponseWithMappingError = (): ErrorResponse => ({ + error: { + root_cause: [ + { + type: 'mapping_exception', + reason: "Inaccessible index 'restricted-*'", + }, + ], + type: 'mapping_exception', + reason: "Inaccessible index 'restricted-*'", + }, +}); + +export const getEqlResponseWithParsingError = (): ErrorResponse => ({ + error: { + root_cause: [ + { + type: 'parsing_exception', + reason: "line 1:5: missing 'where' at 'demo'", + }, + ], + type: 'parsing_exception', + reason: "line 1:5: missing 'where' at 'demo'", + }, +}); + export const getEqlResponseWithNonValidationError = (): TransportResult['body'] => ({ error: { root_cause: [ diff --git a/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.ts b/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.ts index 63a812cad759a..c183cd7ceb605 100644 --- a/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.ts +++ b/x-pack/plugins/security_solution/common/search_strategy/eql/validation/helpers.ts @@ -23,12 +23,27 @@ export interface ErrorResponse { const isValidationErrorType = (type: unknown): boolean => type === PARSING_ERROR_TYPE || type === VERIFICATION_ERROR_TYPE || type === MAPPING_ERROR_TYPE; +const isParsingErrorType = (type: unknown): boolean => type === PARSING_ERROR_TYPE; + +const isVerificationErrorType = (type: unknown): boolean => type === VERIFICATION_ERROR_TYPE; + +const isMappingErrorType = (type: unknown): boolean => type === MAPPING_ERROR_TYPE; + export const isErrorResponse = (response: unknown): response is ErrorResponse => has(response, 'error.type'); export const isValidationErrorResponse = (response: unknown): response is ErrorResponse => isErrorResponse(response) && isValidationErrorType(get(response, 'error.type')); +export const isParsingErrorResponse = (response: unknown): response is ErrorResponse => + isErrorResponse(response) && isParsingErrorType(get(response, 'error.type')); + +export const isVerificationErrorResponse = (response: unknown): response is ErrorResponse => + isErrorResponse(response) && isVerificationErrorType(get(response, 'error.type')); + +export const isMappingErrorResponse = (response: unknown): response is ErrorResponse => + isErrorResponse(response) && isMappingErrorType(get(response, 'error.type')); + export const getValidationErrors = (response: ErrorResponse): string[] => response.error.root_cause .filter((cause) => isValidationErrorType(cause.type)) diff --git a/x-pack/plugins/security_solution/public/common/hooks/eql/api.test.ts b/x-pack/plugins/security_solution/public/common/hooks/eql/api.test.ts new file mode 100644 index 0000000000000..ee7c9a1515f9e --- /dev/null +++ b/x-pack/plugins/security_solution/public/common/hooks/eql/api.test.ts @@ -0,0 +1,162 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { of } from 'rxjs'; + +import { dataPluginMock } from '@kbn/data-plugin/public/mocks'; +import { searchServiceMock } from '@kbn/data-plugin/public/search/mocks'; + +import { validateEql } from './api'; +import { + getEqlResponseWithMappingError, + getEqlResponseWithNonValidationError, + getEqlResponseWithParsingError, + getEqlResponseWithValidationError, + getEqlResponseWithValidationErrors, +} from '../../../../common/search_strategy/eql/validation/helpers.mock'; + +const searchMock = searchServiceMock.createStartContract(); + +const mockDataService = { + ...dataPluginMock.createStartContract(), + search: searchMock, +}; + +const triggerValidateEql = () => { + const signal = new AbortController().signal; + return validateEql({ + data: mockDataService, + dataViewTitle: 'logs-*', + query: 'any where true', + signal, + runtimeMappings: undefined, + options: undefined, + }); +}; + +describe('validateEql', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('handle EqlSearchStrategyResponse', () => { + it('should return valid set to true if validation response is not an error', async () => { + searchMock.search.mockImplementation(() => of({ rawResponse: 'Successful validation!' })); + const response = await triggerValidateEql(); + + expect(response).toEqual({ valid: true }); + }); + + it('should return EQL_ERR_INVALID_EQL error in case of `verification_exception` error', async () => { + searchMock.search.mockImplementation(() => + of({ rawResponse: getEqlResponseWithValidationError() }) + ); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { + code: 'EQL_ERR_INVALID_EQL', + messages: [ + 'Found 2 problems\nline 1:1: Unknown column [event.category]\nline 1:13: Unknown column [event.name]', + ], + }, + }); + }); + + it('should return EQL_ERR_INVALID_EQL error in case of multiple `verification_exception` errors', async () => { + searchMock.search.mockImplementation(() => + of({ rawResponse: getEqlResponseWithValidationErrors() }) + ); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { + code: 'EQL_ERR_INVALID_EQL', + messages: [ + 'Found 2 problems\nline 1:1: Unknown column [event.category]\nline 1:13: Unknown column [event.name]', + "line 1:4: mismatched input '' expecting 'where'", + ], + }, + }); + }); + + it('should return EQL_ERR_INVALID_EQL error in case of `mapping_exception` error', async () => { + searchMock.search.mockImplementation(() => + of({ rawResponse: getEqlResponseWithMappingError() }) + ); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { code: 'EQL_ERR_INVALID_EQL', messages: ["Inaccessible index 'restricted-*'"] }, + }); + }); + + it('should return EQL_ERR_INVALID_SYNTAX error in case of `parsing_exception` error', async () => { + searchMock.search.mockImplementation(() => + of({ rawResponse: getEqlResponseWithParsingError() }) + ); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { + code: 'EQL_ERR_INVALID_SYNTAX', + messages: ["line 1:5: missing 'where' at 'demo'"], + }, + }); + }); + + it('should return EQL_ERR_FAILED_REQUEST error in case of non-validation error', async () => { + searchMock.search.mockImplementation(() => + of({ rawResponse: getEqlResponseWithNonValidationError() }) + ); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { + code: 'EQL_ERR_FAILED_REQUEST', + error: expect.objectContaining( + new Error(JSON.stringify(getEqlResponseWithNonValidationError())) + ), + }, + }); + }); + + it('should return EQL_ERR_MISSING_DATA_SOURCE error in case `data.search` throws an error which starts with `index_not_found_exception`', async () => { + const message = 'index_not_found_exception Found 1 problem line -1:-1: Unknown index [*,-*]'; + searchMock.search.mockImplementation(() => { + throw new Error(message); + }); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { code: 'EQL_ERR_MISSING_DATA_SOURCE', messages: [message] }, + }); + }); + + it('should return EQL_ERR_FAILED_REQUEST error in case `data.search` throws an error', async () => { + const message = 'Internal exception'; + searchMock.search.mockImplementation(() => { + throw new Error(message); + }); + const response = await triggerValidateEql(); + + expect(response).toEqual({ + valid: false, + error: { + code: 'EQL_ERR_FAILED_REQUEST', + error: expect.objectContaining(new Error(message)), + }, + }); + }); + }); +}); diff --git a/x-pack/plugins/security_solution/public/common/hooks/eql/api.ts b/x-pack/plugins/security_solution/public/common/hooks/eql/api.ts index 46d01bf15d577..921d4fde4bfd0 100644 --- a/x-pack/plugins/security_solution/public/common/hooks/eql/api.ts +++ b/x-pack/plugins/security_solution/public/common/hooks/eql/api.ts @@ -15,9 +15,18 @@ import type { EqlOptionsSelected } from '../../../../common/search_strategy'; import { getValidationErrors, isErrorResponse, - isValidationErrorResponse, + isMappingErrorResponse, + isParsingErrorResponse, + isVerificationErrorResponse, } from '../../../../common/search_strategy/eql'; +export enum EQL_ERROR_CODES { + FAILED_REQUEST = 'EQL_ERR_FAILED_REQUEST', + INVALID_EQL = 'EQL_ERR_INVALID_EQL', + INVALID_SYNTAX = 'EQL_ERR_INVALID_SYNTAX', + MISSING_DATA_SOURCE = 'EQL_ERR_MISSING_DATA_SOURCE', +} + interface Params { dataViewTitle: string; query: string; @@ -34,31 +43,60 @@ export const validateEql = async ({ signal, runtimeMappings, options, -}: Params): Promise<{ valid: boolean; errors: string[] }> => { - const { rawResponse: response } = await firstValueFrom( - data.search.search( - { - params: { - index: dataViewTitle, - body: { query, runtime_mappings: runtimeMappings, size: 0 }, - timestamp_field: options?.timestampField, - tiebreaker_field: options?.tiebreakerField || undefined, - event_category_field: options?.eventCategoryField, +}: Params): Promise<{ + valid: boolean; + error?: { code: EQL_ERROR_CODES; messages?: string[]; error?: Error }; +}> => { + try { + const { rawResponse: response } = await firstValueFrom( + data.search.search( + { + params: { + index: dataViewTitle, + body: { query, runtime_mappings: runtimeMappings, size: 0 }, + timestamp_field: options?.timestampField, + tiebreaker_field: options?.tiebreakerField || undefined, + event_category_field: options?.eventCategoryField, + }, + options: { ignore: [400] }, }, - options: { ignore: [400] }, + { + strategy: EQL_SEARCH_STRATEGY, + abortSignal: signal, + } + ) + ); + if (isParsingErrorResponse(response)) { + return { + valid: false, + error: { code: EQL_ERROR_CODES.INVALID_SYNTAX, messages: getValidationErrors(response) }, + }; + } else if (isVerificationErrorResponse(response) || isMappingErrorResponse(response)) { + return { + valid: false, + error: { code: EQL_ERROR_CODES.INVALID_EQL, messages: getValidationErrors(response) }, + }; + } else if (isErrorResponse(response)) { + return { + valid: false, + error: { code: EQL_ERROR_CODES.FAILED_REQUEST, error: new Error(JSON.stringify(response)) }, + }; + } else { + return { valid: true }; + } + } catch (error) { + if (error instanceof Error && error.message.startsWith('index_not_found_exception')) { + return { + valid: false, + error: { code: EQL_ERROR_CODES.MISSING_DATA_SOURCE, messages: [error.message] }, + }; + } + return { + valid: false, + error: { + code: EQL_ERROR_CODES.FAILED_REQUEST, + error, }, - { - strategy: EQL_SEARCH_STRATEGY, - abortSignal: signal, - } - ) - ); - - if (isValidationErrorResponse(response)) { - return { valid: false, errors: getValidationErrors(response) }; - } else if (isErrorResponse(response)) { - throw new Error(JSON.stringify(response)); - } else { - return { valid: true, errors: [] }; + }; } }; diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.mock.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.mock.ts index 83d24114e4f86..b6c9d4d71f270 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.mock.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.mock.ts @@ -5,11 +5,11 @@ * 2.0. */ +import { EQL_ERROR_CODES } from '../../../../common/hooks/eql/api'; import type { ValidationError } from '../../../../shared_imports'; -import { ERROR_CODES } from './validators'; export const getEqlValidationError = (): ValidationError => ({ - code: ERROR_CODES.INVALID_EQL, + code: EQL_ERROR_CODES.INVALID_EQL, messages: ['line 1: WRONG\nline 2: ALSO WRONG'], message: '', }); diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.ts index b3d6abc62d0b1..04ea9bbc43356 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components/eql_query_bar/validators.ts @@ -12,15 +12,10 @@ import { isEqlRule } from '../../../../../common/detection_engine/utils'; import { KibanaServices } from '../../../../common/lib/kibana'; import type { DefineStepRule } from '../../../../detections/pages/detection_engine/rules/types'; import { DataSourceType } from '../../../../detections/pages/detection_engine/rules/types'; -import { validateEql } from '../../../../common/hooks/eql/api'; +import { validateEql, EQL_ERROR_CODES } from '../../../../common/hooks/eql/api'; import type { FieldValueQueryBar } from '../query_bar'; import * as i18n from './translations'; -export enum ERROR_CODES { - FAILED_REQUEST = 'ERR_FAILED_REQUEST', - INVALID_EQL = 'ERR_INVALID_EQL', -} - /** * Unlike lodash's debounce, which resolves intermediate calls with the most * recent value, this implementation waits to resolve intermediate calls until @@ -54,7 +49,7 @@ export const debounceAsync = -): Promise | void | undefined> => { +): Promise | void | undefined> => { const [{ value, formData }] = args; const { query: queryValue } = value as FieldValueQueryBar; const query = queryValue.query as string; @@ -93,14 +88,15 @@ export const eqlValidator = async ( if (response?.valid === false) { return { - code: ERROR_CODES.INVALID_EQL, + code: response.error?.code, message: '', - messages: response.errors, + messages: response.error?.messages, + error: response.error?.error, }; } } catch (error) { return { - code: ERROR_CODES.FAILED_REQUEST, + code: EQL_ERROR_CODES.FAILED_REQUEST, message: i18n.EQL_VALIDATION_REQUEST_ERROR, error, }; @@ -117,10 +113,10 @@ export const getValidationResults = ( const [error] = field.errors; const message = error.message; - if (error.code === ERROR_CODES.INVALID_EQL) { - return { isValid, message, messages: error.messages }; - } else { + if (error.code === EQL_ERROR_CODES.FAILED_REQUEST) { return { isValid, message, error: error.error }; + } else { + return { isValid, message, messages: error.messages }; } } else { return { isValid, message: '' }; diff --git a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/eql_rule.cy.ts b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/eql_rule.cy.ts index 6f804f9385e63..2b0b9f3fdab61 100644 --- a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/eql_rule.cy.ts +++ b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/eql_rule.cy.ts @@ -64,6 +64,7 @@ import { EQL_OPTIONS_TIMESTAMP_INPUT, EQL_QUERY_INPUT, EQL_QUERY_VALIDATION_ERROR, + EQL_QUERY_VALIDATION_ERROR_CONTENT, RULES_CREATION_FORM, } from '../../../../screens/create_new_rule'; @@ -222,4 +223,65 @@ describe('EQL rules', { tags: ['@ess', '@serverless'] }, () => { cy.get(EQL_QUERY_VALIDATION_ERROR).should('not.exist'); }); }); + + describe('EQL query validation', () => { + it('validates missing data source', () => { + login(); + visit(CREATE_RULE_URL); + selectEqlRuleType(); + getIndexPatternClearButton().click(); + getRuleIndexInput().type('endgame-*{enter}'); + + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('exist'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('be.visible'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).type('any where true'); + + cy.get(EQL_QUERY_VALIDATION_ERROR).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR).should('have.text', '1'); + cy.get(EQL_QUERY_VALIDATION_ERROR).click(); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should( + 'have.text', + `EQL Validation Errorsindex_not_found_exception\n\tCaused by:\n\t\tverification_exception: Found 1 problem\nline -1:-1: Unknown index [*,-*]\n\tRoot causes:\n\t\tverification_exception: Found 1 problem\nline -1:-1: Unknown index [*,-*]` + ); + }); + + it('validates missing data fields', () => { + login(); + visit(CREATE_RULE_URL); + selectEqlRuleType(); + + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('exist'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('be.visible'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).type('any where field1'); + + cy.get(EQL_QUERY_VALIDATION_ERROR).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR).should('have.text', '1'); + cy.get(EQL_QUERY_VALIDATION_ERROR).click(); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should( + 'have.text', + 'EQL Validation ErrorsFound 1 problem\nline 1:11: Unknown column [field1]' + ); + }); + + it('validates syntax errors', () => { + login(); + visit(CREATE_RULE_URL); + selectEqlRuleType(); + + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('exist'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).should('be.visible'); + cy.get(RULES_CREATION_FORM).find(EQL_QUERY_INPUT).type('test any where true'); + + cy.get(EQL_QUERY_VALIDATION_ERROR).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR).should('have.text', '1'); + cy.get(EQL_QUERY_VALIDATION_ERROR).click(); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should('be.visible'); + cy.get(EQL_QUERY_VALIDATION_ERROR_CONTENT).should( + 'have.text', + `EQL Validation Errorsline 1:6: extraneous input 'any' expecting 'where'` + ); + }); + }); }); diff --git a/x-pack/test/security_solution_cypress/cypress/screens/create_new_rule.ts b/x-pack/test/security_solution_cypress/cypress/screens/create_new_rule.ts index 46b0b3a68734a..4e6df1ed3a750 100644 --- a/x-pack/test/security_solution_cypress/cypress/screens/create_new_rule.ts +++ b/x-pack/test/security_solution_cypress/cypress/screens/create_new_rule.ts @@ -119,6 +119,9 @@ export const EQL_QUERY_VALIDATION_LABEL = '.euiFormLabel-isInvalid'; export const EQL_QUERY_VALIDATION_ERROR = '[data-test-subj="eql-validation-errors-popover-button"]'; +export const EQL_QUERY_VALIDATION_ERROR_CONTENT = + '[data-test-subj="eql-validation-errors-popover-content"]'; + export const EQL_OPTIONS_POPOVER_TRIGGER = '[data-test-subj="eql-settings-trigger"]'; export const EQL_OPTIONS_TIMESTAMP_INPUT =