From d3d5a7c7fe4325b64c98d8d57bb17d553a261fe8 Mon Sep 17 00:00:00 2001 From: Ievgen Sorokopud Date: Wed, 7 Aug 2024 20:27:57 +0200 Subject: [PATCH] [Security Solution] Use AST utils from @kbn/esql-ast for ES|QL rule type query parsing (#9282) (#189780) ## Summary With these changes we utilise AST based utils to do ES|QL query validation. This allows us to recognise and display syntax errors. Syntax errors have higher priority than the rest of the validation errors. Validation errors priorities from top to bottom: 1. Syntax error 2. Missing metadata for non-aggregating queries 3. Missing data source and/or data fields 4. Missing `_id` column requested for non-aggregating queries via metadata operator These priorities define the sequence in which we display errors to the user. If there are several errors detected, that the one with higher priority will be shown. https://github.com/user-attachments/assets/cef88c60-b0a4-413e-885a-b619773fd853 ### 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 tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6681) (100 ESS, 100 Serverless) * [Cypress DE tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6712) (100 ESS, 100 Serverless) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../esql/compute_if_esql_query_aggregating.ts | 9 +- .../kbn-securitysolution-utils/tsconfig.json | 3 +- .../logic/esql_validator.test.ts | 85 +++++++++++++------ .../rule_creation/logic/esql_validator.ts | 65 ++++++++++++-- .../hooks/use_all_esql_rule_fields.test.ts | 16 ++-- .../hooks/use_all_esql_rule_fields.ts | 6 +- .../plugins/security_solution/tsconfig.json | 2 + .../rule_creation/esql_rule.cy.ts | 11 +++ 8 files changed, 152 insertions(+), 45 deletions(-) diff --git a/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts b/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts index 44deada7cd155..251c00d4a75b4 100644 --- a/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts +++ b/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts @@ -6,11 +6,18 @@ * Side Public License, v 1. */ +import { ESQLAst, getAstAndSyntaxErrors } from '@kbn/esql-ast'; + +export const isAggregatingQuery = (ast: ESQLAst): boolean => { + return ast.some((astItem) => astItem.type === 'command' && astItem.name === 'stats'); +}; + /** * compute if esqlQuery is aggregating/grouping, i.e. using STATS...BY command * @param esqlQuery * @returns boolean */ export const computeIsESQLQueryAggregating = (esqlQuery: string): boolean => { - return /\|\s+stats\s/i.test(esqlQuery); + const { ast } = getAstAndSyntaxErrors(esqlQuery); + return isAggregatingQuery(ast); }; diff --git a/packages/kbn-securitysolution-utils/tsconfig.json b/packages/kbn-securitysolution-utils/tsconfig.json index c734b5c153fb0..5b9520c487e31 100644 --- a/packages/kbn-securitysolution-utils/tsconfig.json +++ b/packages/kbn-securitysolution-utils/tsconfig.json @@ -12,7 +12,8 @@ ], "kbn_references": [ "@kbn/i18n", - "@kbn/esql-utils" + "@kbn/esql-utils", + "@kbn/esql-ast" ], "exclude": [ "target/**/*", diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts index 07f14830d6a71..2fdd8cf8120d7 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts @@ -5,82 +5,117 @@ * 2.0. */ +import { getAstAndSyntaxErrors } from '@kbn/esql-ast'; import { parseEsqlQuery, computeHasMetadataOperator } from './esql_validator'; -import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils'; +import { isAggregatingQuery } from '@kbn/securitysolution-utils'; -jest.mock('@kbn/securitysolution-utils', () => ({ computeIsESQLQueryAggregating: jest.fn() })); +jest.mock('@kbn/securitysolution-utils', () => ({ isAggregatingQuery: jest.fn() })); -const computeIsESQLQueryAggregatingMock = computeIsESQLQueryAggregating as jest.Mock; +const isAggregatingQueryMock = isAggregatingQuery as jest.Mock; + +const getQeryAst = (query: string) => { + const { ast } = getAstAndSyntaxErrors(query); + return ast; +}; describe('computeHasMetadataOperator', () => { it('should be false if query does not have operator', () => { - expect(computeHasMetadataOperator('from test*')).toBe(false); - expect(computeHasMetadataOperator('from test* [metadata]')).toBe(false); - expect(computeHasMetadataOperator('from test* [metadata id]')).toBe(false); - expect(computeHasMetadataOperator('from metadata*')).toBe(false); - expect(computeHasMetadataOperator('from test* | keep metadata')).toBe(false); - expect(computeHasMetadataOperator('from test* | eval x="[metadata _id]"')).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from test*'))).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from test* [metadata]'))).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from test* [metadata id]'))).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from metadata*'))).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from test* | keep metadata'))).toBe(false); + expect(computeHasMetadataOperator(getQeryAst('from test* | eval x="[metadata _id]"'))).toBe( + false + ); }); it('should be true if query has operator', () => { - expect(computeHasMetadataOperator('from test* metadata _id')).toBe(true); - expect(computeHasMetadataOperator('from test* metadata _id, _index')).toBe(true); - expect(computeHasMetadataOperator('from test* metadata _index, _id')).toBe(true); - expect(computeHasMetadataOperator('from test* metadata _id ')).toBe(true); - expect(computeHasMetadataOperator('from test* metadata _id | limit 10')).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id, _index'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* metadata _index, _id'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id '))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id | limit 10'))).toBe( + true + ); expect( - computeHasMetadataOperator(`from packetbeat* metadata + computeHasMetadataOperator( + getQeryAst(`from packetbeat* metadata _id | limit 100`) + ) ).toBe(true); // still validates deprecated square bracket syntax - expect(computeHasMetadataOperator('from test* [metadata _id]')).toBe(true); - expect(computeHasMetadataOperator('from test* [metadata _id, _index]')).toBe(true); - expect(computeHasMetadataOperator('from test* [metadata _index, _id]')).toBe(true); - expect(computeHasMetadataOperator('from test* [ metadata _id ]')).toBe(true); - expect(computeHasMetadataOperator('from test* [ metadata _id] ')).toBe(true); - expect(computeHasMetadataOperator('from test* [ metadata _id] | limit 10')).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _id]'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _id, _index]'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _index, _id]'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [ metadata _id ]'))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [ metadata _id] '))).toBe(true); + expect(computeHasMetadataOperator(getQeryAst('from test* [ metadata _id] | limit 10'))).toBe( + true + ); expect( - computeHasMetadataOperator(`from packetbeat* [metadata + computeHasMetadataOperator( + getQeryAst(`from packetbeat* [metadata _id ] | limit 100`) + ) ).toBe(true); }); }); describe('parseEsqlQuery', () => { it('returns isMissingMetadataOperator true when query is not aggregating and does not have metadata operator', () => { - computeIsESQLQueryAggregatingMock.mockReturnValueOnce(false); + isAggregatingQueryMock.mockReturnValueOnce(false); expect(parseEsqlQuery('from test*')).toEqual({ + errors: [], isEsqlQueryAggregating: false, isMissingMetadataOperator: true, }); }); it('returns isMissingMetadataOperator false when query is not aggregating and has metadata operator', () => { - computeIsESQLQueryAggregatingMock.mockReturnValueOnce(false); + isAggregatingQueryMock.mockReturnValueOnce(false); expect(parseEsqlQuery('from test* metadata _id')).toEqual({ + errors: [], isEsqlQueryAggregating: false, isMissingMetadataOperator: false, }); }); it('returns isMissingMetadataOperator false when query is aggregating', () => { - computeIsESQLQueryAggregatingMock.mockReturnValue(true); + isAggregatingQueryMock.mockReturnValue(true); expect(parseEsqlQuery('from test*')).toEqual({ + errors: [], isEsqlQueryAggregating: true, isMissingMetadataOperator: false, }); expect(parseEsqlQuery('from test* metadata _id')).toEqual({ + errors: [], isEsqlQueryAggregating: true, isMissingMetadataOperator: false, }); }); + + it('returns error when query is syntactically invalid', () => { + isAggregatingQueryMock.mockReturnValueOnce(false); + + expect(parseEsqlQuery('aaa bbbb ssdasd')).toEqual({ + errors: expect.arrayContaining([ + expect.objectContaining({ + message: + "SyntaxError: mismatched input 'aaa' expecting {'explain', 'from', 'meta', 'metrics', 'row', 'show'}", + }), + ]), + isEsqlQueryAggregating: false, + isMissingMetadataOperator: true, + }); + }); }); diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts index 484f78c53f0e0..869e379c21aed 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts @@ -7,8 +7,11 @@ import { isEmpty } from 'lodash'; import type { QueryClient } from '@tanstack/react-query'; -import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils'; +import { isAggregatingQuery } from '@kbn/securitysolution-utils'; +import type { ESQLAst } from '@kbn/esql-ast'; +import { getAstAndSyntaxErrors } from '@kbn/esql-ast'; +import { isColumnItem, isOptionItem } from '@kbn/esql-validation-autocomplete'; import { KibanaServices } from '../../../common/lib/kibana'; import type { ValidationError, ValidationFunc } from '../../../shared_imports'; @@ -21,6 +24,7 @@ export type FieldType = 'string'; export enum ERROR_CODES { INVALID_ESQL = 'ERR_INVALID_ESQL', + INVALID_SYNTAX = 'ERR_INVALID_SYNTAX', ERR_MISSING_ID_FIELD_FROM_RESULT = 'ERR_MISSING_ID_FIELD_FROM_RESULT', } @@ -34,11 +38,52 @@ const constructValidationError = (error: Error) => { }; }; +const constructSyntaxError = (error: Error) => { + return { + code: ERROR_CODES.INVALID_SYNTAX, + message: error?.message + ? i18n.esqlValidationErrorMessage(error.message) + : i18n.ESQL_VALIDATION_UNKNOWN_ERROR, + error, + }; +}; + +const getMetadataOption = (ast: ESQLAst) => { + const fromCommand = ast.find((astItem) => astItem.type === 'command' && astItem.name === 'from'); + + if (!fromCommand?.args) { + return undefined; + } + + // Check whether the `from` command has `metadata` operator + for (const fromArg of fromCommand.args) { + if (isOptionItem(fromArg) && fromArg.name === 'metadata') { + return fromArg; + } + } + + return undefined; +}; + /** * checks whether query has metadata _id operator */ -export const computeHasMetadataOperator = (esqlQuery: string) => { - return /(? { + // Check whether the `from` command has `metadata` operator + const metadataOption = getMetadataOption(ast); + if (!metadataOption) { + return false; + } + + // Check whether the `metadata` operator has `_id` argument + const idColumnItem = metadataOption.args.find( + (fromArg) => isColumnItem(fromArg) && fromArg.name === '_id' + ); + if (!idColumnItem) { + return false; + } + + return true; }; /** @@ -61,7 +106,12 @@ export const esqlValidator = async ( const queryClient = (customData.value as { queryClient: QueryClient | undefined })?.queryClient; const services = KibanaServices.get(); - const { isEsqlQueryAggregating, isMissingMetadataOperator } = parseEsqlQuery(query); + const { isEsqlQueryAggregating, isMissingMetadataOperator, errors } = parseEsqlQuery(query); + + // Check if there are any syntax errors + if (errors.length) { + return constructSyntaxError(new Error(errors[0].message)); + } if (isMissingMetadataOperator) { return { @@ -97,11 +147,14 @@ export const esqlValidator = async ( * - if it's non aggregation query it must have metadata operator */ export const parseEsqlQuery = (query: string) => { - const isEsqlQueryAggregating = computeIsESQLQueryAggregating(query); + const { ast, errors } = getAstAndSyntaxErrors(query); + + const isEsqlQueryAggregating = isAggregatingQuery(ast); return { + errors, isEsqlQueryAggregating, // non-aggregating query which does not have [metadata], is not a valid one - isMissingMetadataOperator: !isEsqlQueryAggregating && !computeHasMetadataOperator(query), + isMissingMetadataOperator: !isEsqlQueryAggregating && !computeHasMetadataOperator(ast), }; }; diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts index 377a1772a5ea0..1a13f0ff8e3a2 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts @@ -12,11 +12,9 @@ import { getESQLQueryColumns } from '@kbn/esql-utils'; import { useAllEsqlRuleFields } from './use_all_esql_rule_fields'; import { createQueryWrapperMock } from '../../../common/__mocks__/query_wrapper'; -import { parseEsqlQuery } from '../../rule_creation/logic/esql_validator'; +import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils'; -jest.mock('../../rule_creation/logic/esql_validator', () => ({ - parseEsqlQuery: jest.fn(), -})); +jest.mock('@kbn/securitysolution-utils', () => ({ computeIsESQLQueryAggregating: jest.fn() })); jest.mock('@kbn/esql-utils', () => { return { @@ -25,7 +23,7 @@ jest.mock('@kbn/esql-utils', () => { }; }); -const parseEsqlQueryMock = parseEsqlQuery as jest.Mock; +const computeIsESQLQueryAggregatingMock = computeIsESQLQueryAggregating as jest.Mock; const getESQLQueryColumnsMock = getESQLQueryColumns as jest.Mock; const { wrapper } = createQueryWrapperMock(); @@ -61,7 +59,7 @@ describe.skip('useAllEsqlRuleFields', () => { : mockEsqlDatatable.columns ) ); - parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false }); + computeIsESQLQueryAggregatingMock.mockReturnValue(false); }); it('should return loading true when esql fields still loading', () => { @@ -104,7 +102,7 @@ describe.skip('useAllEsqlRuleFields', () => { }); it('should return index pattern fields concatenated with ES|QL fields when ES|QL query is non-aggregating', async () => { - parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false }); + computeIsESQLQueryAggregatingMock.mockReturnValue(false); const { result, waitFor } = renderHook( () => @@ -127,7 +125,7 @@ describe.skip('useAllEsqlRuleFields', () => { }); it('should return only ES|QL fields when ES|QL query is aggregating', async () => { - parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: true }); + computeIsESQLQueryAggregatingMock.mockReturnValue(true); const { result, waitFor } = renderHook( () => @@ -149,7 +147,7 @@ describe.skip('useAllEsqlRuleFields', () => { it('should deduplicate index pattern fields and ES|QL fields when fields have same name', async () => { // getESQLQueryColumnsMock.mockClear(); - parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false }); + computeIsESQLQueryAggregatingMock.mockReturnValue(false); const { result, waitFor } = renderHook( () => diff --git a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts index a67b990c88b80..80bfc364e5b51 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts +++ b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts @@ -13,7 +13,7 @@ import useDebounce from 'react-use/lib/useDebounce'; import { useQuery } from '@tanstack/react-query'; import { useKibana } from '@kbn/kibana-react-plugin/public'; -import { parseEsqlQuery } from '../../rule_creation/logic/esql_validator'; +import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils'; import { getEsqlQueryConfig } from '../../rule_creation/logic/get_esql_query_config'; @@ -89,8 +89,8 @@ export const useAllEsqlRuleFields: UseAllEsqlRuleFields = ({ esqlQuery, indexPat const [debouncedEsqlQuery, setDebouncedEsqlQuery] = useState(undefined); const { fields: esqlFields, isLoading } = useEsqlFields(debouncedEsqlQuery); - const { isEsqlQueryAggregating } = useMemo( - () => parseEsqlQuery(debouncedEsqlQuery ?? ''), + const isEsqlQueryAggregating = useMemo( + () => computeIsESQLQueryAggregating(debouncedEsqlQuery ?? ''), [debouncedEsqlQuery] ); diff --git a/x-pack/plugins/security_solution/tsconfig.json b/x-pack/plugins/security_solution/tsconfig.json index 3258167eb50b7..bdaf656b9c986 100644 --- a/x-pack/plugins/security_solution/tsconfig.json +++ b/x-pack/plugins/security_solution/tsconfig.json @@ -208,6 +208,8 @@ "@kbn/core-theme-browser", "@kbn/integration-assistant-plugin", "@kbn/avc-banner", + "@kbn/esql-ast", + "@kbn/esql-validation-autocomplete", "@kbn/config", ] } diff --git a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts index 1dad7edc63ab6..348133b1d2802 100644 --- a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts +++ b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts @@ -180,6 +180,17 @@ describe( cy.get(ESQL_QUERY_BAR).contains('Error validating ES|QL'); }); + + it('shows syntax error when query is syntactically invalid - prioritizing it over missing metadata operator error', function () { + const invalidNonAggregatingQuery = 'from auditbeat* | limit 5 test'; + selectEsqlRuleType(); + fillEsqlQueryBar(invalidNonAggregatingQuery); + getDefineContinueButton().click(); + + cy.get(ESQL_QUERY_BAR).contains( + `Error validating ES|QL: "SyntaxError: extraneous input 'test' expecting "` + ); + }); }); describe('ES|QL investigation fields', () => {