From 43ca62c9087aba7874cba31a3f4981fc7592044b Mon Sep 17 00:00:00 2001 From: Davis Plumlee <56367316+dplumlee@users.noreply.github.com> Date: Thu, 12 Dec 2024 22:58:50 -0500 Subject: [PATCH 1/3] [Security Solution] Adds normalization for `query` fields before diff algorithm comparison (#203482) ## Summary Fixes https://github.com/elastic/kibana/issues/203151 Adds a normalization for the `kql_query`, `eql_query`, and `esql_query` fields that trims the whitespace from the beginning and end of query strings for a more robust comparison in the diff algorithms. Since whitespace before or after the query string is purely a formatting choice and doesn't impact the query itself, we discard the excess whitespace characters before the direct string comparison. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [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 (cherry picked from commit 0294838a95975ac6b5ee37a94ecacfe2e9a19955) # Conflicts: # x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts --- .../diff/convert_rule_to_diffable.ts | 2 +- .../diff/extract_rule_data_query.test.ts | 61 +++++++++++++++++++ .../diff/extract_rule_data_query.ts | 6 +- ..._review_prebuilt_rules.eql_query_fields.ts | 40 ++++++++++++ ...review_prebuilt_rules.esql_query_fields.ts | 38 ++++++++++++ ..._review_prebuilt_rules.kql_query_fields.ts | 46 ++++++++++++++ ...rebuilt_rules.single_line_string_fields.ts | 35 +++++++++++ 7 files changed, 224 insertions(+), 4 deletions(-) create mode 100644 x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts diff --git a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/convert_rule_to_diffable.ts b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/convert_rule_to_diffable.ts index 882dcae3e36aa..9085ad4001a92 100644 --- a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/convert_rule_to_diffable.ts +++ b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/convert_rule_to_diffable.ts @@ -118,7 +118,7 @@ const extractDiffableCommonFields = ( version: rule.version, // Main domain fields - name: rule.name, + name: rule.name.trim(), tags: rule.tags ?? [], description: rule.description, severity: rule.severity, diff --git a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts new file mode 100644 index 0000000000000..03dc7ecbe5331 --- /dev/null +++ b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts @@ -0,0 +1,61 @@ +/* + * 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 { KqlQueryType } from '../../../api/detection_engine'; +import { + extractRuleEqlQuery, + extractRuleEsqlQuery, + extractRuleKqlQuery, +} from './extract_rule_data_query'; + +describe('extract rule data queries', () => { + describe('extractRuleKqlQuery', () => { + it('extracts a trimmed version of the query field for inline query types', () => { + const extractedKqlQuery = extractRuleKqlQuery('\nevent.kind:alert\n', 'kuery', [], undefined); + + expect(extractedKqlQuery).toEqual({ + type: KqlQueryType.inline_query, + query: 'event.kind:alert', + language: 'kuery', + filters: [], + }); + }); + }); + + describe('extractRuleEqlQuery', () => { + it('extracts a trimmed version of the query field', () => { + const extractedEqlQuery = extractRuleEqlQuery({ + query: '\n\nquery where true\n\n', + language: 'eql', + filters: [], + eventCategoryOverride: undefined, + timestampField: undefined, + tiebreakerField: undefined, + }); + + expect(extractedEqlQuery).toEqual({ + query: 'query where true', + language: 'eql', + filters: [], + event_category_override: undefined, + timestamp_field: undefined, + tiebreaker_field: undefined, + }); + }); + }); + + describe('extractRuleEsqlQuery', () => { + it('extracts a trimmed version of the query field', () => { + const extractedEsqlQuery = extractRuleEsqlQuery('\nFROM * where true\t\n', 'esql'); + + expect(extractedEsqlQuery).toEqual({ + query: 'FROM * where true', + language: 'esql', + }); + }); + }); +}); diff --git a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts index 4f4164c6a0086..1738d5c5d4f78 100644 --- a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts +++ b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts @@ -43,7 +43,7 @@ export const extractInlineKqlQuery = ( ): InlineKqlQuery => { return { type: KqlQueryType.inline_query, - query: query ?? '', + query: query?.trim() ?? '', language: language ?? 'kuery', filters: filters ?? [], }; @@ -55,7 +55,7 @@ export const extractRuleEqlQuery = ( filters: RuleFilterArray | undefined ): RuleEqlQuery => { return { - query, + query: query.trim(),, language, filters: filters ?? [], }; @@ -66,7 +66,7 @@ export const extractRuleEsqlQuery = ( language: EsqlQueryLanguage ): RuleEsqlQuery => { return { - query, + query: query.trim(), language, }; }; diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts index eef3e4b6b7ce4..6c49f8722abd4 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.eql_query_fields.ts @@ -80,6 +80,46 @@ export default ({ getService }: FtrProviderContext): void => { expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); }); + + it('should trim all whitespace before version comparison', async () => { + // Install base prebuilt detection rule + await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects()); + await installPrebuiltRules(es, supertest); + + // Customize an eql_query field on the installed rule + await updateRule(supertest, { + ...getPrebuiltRuleMock(), + rule_id: 'rule-1', + type: 'eql', + query: '\nquery where true\n', + language: 'eql', + filters: [], + } as RuleUpdateProps); + + // Add a v2 rule asset to make the upgrade possible, do NOT update the related eql_query field, and create the new rule assets + const updatedRuleAssetSavedObjects = [ + createRuleAssetSavedObject({ + rule_id: 'rule-1', + version: 2, + type: 'eql', + query: '\nquery where true', + language: 'eql', + filters: [], + }), + ]; + await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects); + + // Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but eql_query field is NOT returned + const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest); + const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff; + expect(fieldDiffObject.eql_query).toBeUndefined(); + + expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field + expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0); + expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); + }); }); describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => { diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.esql_query_fields.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.esql_query_fields.ts index 9561393e84549..d8329ce023ea6 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.esql_query_fields.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.esql_query_fields.ts @@ -78,6 +78,44 @@ export default ({ getService }: FtrProviderContext): void => { expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); }); + + it('should trim all whitespace before version comparison', async () => { + // Install base prebuilt detection rule + await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects()); + await installPrebuiltRules(es, supertest); + + // Customize an esql_query field on the installed rule + await updateRule(supertest, { + ...getPrebuiltRuleMock(), + rule_id: 'rule-1', + type: 'esql', + query: '\tFROM query WHERE true\t', + language: 'esql', + } as RuleUpdateProps); + + // Add a v2 rule asset to make the upgrade possible, do NOT update the related esql_query field, and create the new rule assets + const updatedRuleAssetSavedObjects = [ + createRuleAssetSavedObject({ + rule_id: 'rule-1', + version: 2, + type: 'esql', + query: '\n\nFROM query WHERE true\n\n', + language: 'esql', + }), + ]; + await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects); + + // Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but esql_query field is NOT returned + const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest); + const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff; + expect(fieldDiffObject.esql_query).toBeUndefined(); + + expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field + expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0); + expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); + }); }); describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => { diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts index e8f9d2f48b9e0..d00d2d842f2ba 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.kql_query_fields.ts @@ -133,6 +133,52 @@ export default ({ getService }: FtrProviderContext): void => { expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); }); }); + + describe('when all query versions have different surrounding whitespace', () => { + it('should not show in the upgrade/_review API response', async () => { + // Install base prebuilt detection rule + await createHistoricalPrebuiltRuleAssetSavedObjects( + es, + getQueryRuleAssetSavedObjects() + ); + await installPrebuiltRules(es, supertest); + + // Customize a kql_query field on the installed rule + await updateRule(supertest, { + ...getPrebuiltRuleMock(), + rule_id: 'rule-1', + type: 'query', + query: '\nquery string = true', + language: 'kuery', + filters: [], + saved_id: undefined, + } as RuleUpdateProps); + + // Add a v2 rule asset to make the upgrade possible, do NOT update the related kql_query field, and create the new rule assets + const updatedRuleAssetSavedObjects = [ + createRuleAssetSavedObject({ + rule_id: 'rule-1', + version: 2, + type: 'query', + query: 'query string = true\n', + language: 'kuery', + filters: [], + }), + ]; + await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects); + + // Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update but kql_query field is NOT returned + const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest); + const fieldDiffObject = reviewResponse.rules[0].diff.fields as AllFieldsDiff; + expect(fieldDiffObject.kql_query).toBeUndefined(); + + expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); // `version` is considered an updated field + expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0); + expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); + }); + }); }); describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => { diff --git a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.single_line_string_fields.ts b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.single_line_string_fields.ts index 6fe10b9fa6012..6d32d8df7bc72 100644 --- a/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.single_line_string_fields.ts +++ b/x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/prebuilt_rules/management/trial_license_complete_tier/upgrade_review_prebuilt_rules.single_line_string_fields.ts @@ -69,6 +69,41 @@ export default ({ getService }: FtrProviderContext): void => { expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); }); + + it('should trim all whitespace before version comparison', async () => { + // Install base prebuilt detection rule + await createHistoricalPrebuiltRuleAssetSavedObjects(es, getRuleAssetSavedObjects()); + await installPrebuiltRules(es, supertest); + + // Customize a single line string field on the installed rule + await patchRule(supertest, log, { + rule_id: 'rule-1', + name: 'A\n', + }); + + // Increment the version of the installed rule, do NOT update the related single line string field, and create the new rule assets + const updatedRuleAssetSavedObjects = [ + createRuleAssetSavedObject({ + rule_id: 'rule-1', + name: '\nA', + version: 2, + }), + ]; + await createHistoricalPrebuiltRuleAssetSavedObjects(es, updatedRuleAssetSavedObjects); + + // Call the upgrade review prebuilt rules endpoint and check that there is 1 rule eligible for update + // but single line string field (name) is NOT returned + const reviewResponse = await reviewPrebuiltRulesToUpgrade(supertest); + expect(reviewResponse.rules[0].diff.fields.name).toBeUndefined(); + + expect(reviewResponse.rules[0].diff.num_fields_with_updates).toBe(1); + expect(reviewResponse.rules[0].diff.num_fields_with_conflicts).toBe(0); + expect(reviewResponse.rules[0].diff.num_fields_with_non_solvable_conflicts).toBe(0); + + expect(reviewResponse.stats.num_rules_to_upgrade_total).toBe(1); + expect(reviewResponse.stats.num_rules_with_conflicts).toBe(0); + expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0); + }); }); describe("when rule field doesn't have an update but has a custom value - scenario ABA", () => { From 24eaa37e397b7cb8d0b714025a3f93055d0c035b Mon Sep 17 00:00:00 2001 From: Davis Plumlee Date: Fri, 13 Dec 2024 00:08:49 -0500 Subject: [PATCH 2/3] fix merge --- .../prebuilt_rules/diff/extract_rule_data_query.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts index 1738d5c5d4f78..fc34827dc4e80 100644 --- a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts +++ b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.ts @@ -55,7 +55,7 @@ export const extractRuleEqlQuery = ( filters: RuleFilterArray | undefined ): RuleEqlQuery => { return { - query: query.trim(),, + query: query.trim(), language, filters: filters ?? [], }; From 1ea7dfd654342c84575be884081edcb3583cb9ec Mon Sep 17 00:00:00 2001 From: Davis Plumlee Date: Fri, 13 Dec 2024 02:11:02 -0500 Subject: [PATCH 3/3] fix merge --- .../diff/extract_rule_data_query.test.ts | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts index 03dc7ecbe5331..3805a00464b77 100644 --- a/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts +++ b/x-pack/plugins/security_solution/common/detection_engine/prebuilt_rules/diff/extract_rule_data_query.test.ts @@ -28,22 +28,12 @@ describe('extract rule data queries', () => { describe('extractRuleEqlQuery', () => { it('extracts a trimmed version of the query field', () => { - const extractedEqlQuery = extractRuleEqlQuery({ - query: '\n\nquery where true\n\n', - language: 'eql', - filters: [], - eventCategoryOverride: undefined, - timestampField: undefined, - tiebreakerField: undefined, - }); + const extractedEqlQuery = extractRuleEqlQuery('\n\nquery where true\n\n', 'eql', []); expect(extractedEqlQuery).toEqual({ query: 'query where true', language: 'eql', filters: [], - event_category_override: undefined, - timestamp_field: undefined, - tiebreaker_field: undefined, }); }); });