Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Normalizes filters field before rule diff comparison #206344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,32 @@
* 2.0.
*/

import type { Filter } from '@kbn/es-query';
import { KqlQueryType } from '../../../api/detection_engine';
import {
extractRuleEqlQuery,
extractRuleEsqlQuery,
extractRuleKqlQuery,
} from './extract_rule_data_query';

const mockFilter: Filter = {
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test',
params: {
query: 'value',
},
},
query: {
term: {
field: 'value',
},
},
};

describe('extract rule data queries', () => {
describe('extractRuleKqlQuery', () => {
it('extracts a trimmed version of the query field for inline query types', () => {
Expand All @@ -24,6 +43,39 @@ describe('extract rule data queries', () => {
filters: [],
});
});

it('normalizes filters', () => {
const extractedKqlQuery = extractRuleKqlQuery(
'event.kind:alert',
'kuery',
[mockFilter],
undefined
);

expect(extractedKqlQuery).toEqual({
type: KqlQueryType.inline_query,
query: 'event.kind:alert',
language: 'kuery',
filters: [
{
meta: {
negate: false,
disabled: false,
type: 'phrase',
key: 'test',
params: {
query: 'value',
},
},
query: {
term: {
field: 'value',
},
},
},
],
});
});
});

describe('extractRuleEqlQuery', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import type { Filter } from '@kbn/es-query';
import type {
EqlQueryLanguage,
EsqlQueryLanguage,
Expand Down Expand Up @@ -48,7 +49,7 @@ export const extractInlineKqlQuery = (
type: KqlQueryType.inline_query,
query: query?.trim() ?? '',
language: language ?? 'kuery',
filters: filters ?? [],
filters: normalizeFilterArray(filters),
};
};

Expand All @@ -65,7 +66,7 @@ export const extractRuleEqlQuery = (params: ExtractRuleEqlQueryParams): RuleEqlQ
return {
query: params.query.trim(),
language: params.language,
filters: params.filters ?? [],
filters: normalizeFilterArray(params.filters),
event_category_override: params.eventCategoryOverride,
timestamp_field: params.timestampField,
tiebreaker_field: params.tiebreakerField,
Expand All @@ -81,3 +82,20 @@ export const extractRuleEsqlQuery = (
language,
};
};

/**
* Removes the null `alias` field that gets appended from the internal kibana filter util for comparison
* Relevant issue: https://github.com/elastic/kibana/issues/202966
*/
const normalizeFilterArray = (filters: RuleFilterArray | undefined): RuleFilterArray => {
if (filters && filters.length > 0) {
return (filters as Filter[]).map((filter) => {
if (filter?.meta.alias == null) {
return { ...filter, meta: { ...filter.meta, alias: undefined } };
}
return filter;
});
} else {
return [];
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ export const QueryBarField = ({
// if saved query fetched, reset values in queryBar input and filters to saved query's values
useEffect(() => {
if (resetToSavedQuery && savedQuery) {
const newFiledValue = savedQueryToFieldValue(savedQuery);
setFieldValue(newFiledValue);
const newFieldValue = savedQueryToFieldValue(savedQuery);
setFieldValue(newFieldValue);
}
}, [resetToSavedQuery, savedQuery, setFieldValue]);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,110 @@ export default ({ getService }: FtrProviderContext): void => {
expect(reviewResponse.stats.num_rules_with_non_solvable_conflicts).toBe(0);
});
});

describe('when all query versions have filters with alias fields set to null', () => {
it('should not show in the upgrade/_review API response', async () => {
// Install base prebuilt detection rule
await createHistoricalPrebuiltRuleAssetSavedObjects(es, [
createRuleAssetSavedObject({
rule_id: 'rule-1',
version: 1,
type: 'query',
query: 'query string = true',
language: 'kuery',
filters: [
{
meta: {
negate: false,
disabled: false,
type: 'phrase',
key: 'test',
params: {
query: 'value',
},
},
query: {
term: {
field: 'value',
},
},
},
],
}),
]);
await installPrebuiltRules(es, supertest);

// Customize a kql_query field on the installed rule
await updateRule(supertest, {
...getPrebuiltRuleMock(),
rule_id: 'rule-1',
type: 'query',
query: 'query string = true',
language: 'kuery',
filters: [
{
meta: {
alias: null,
negate: false,
disabled: false,
type: 'phrase',
key: 'test',
params: {
query: 'value',
},
},
query: {
term: {
field: 'value',
},
},
},
],
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',
language: 'kuery',
filters: [
{
meta: {
negate: false,
disabled: false,
type: 'phrase',
key: 'test',
params: {
query: 'value',
},
},
query: {
term: {
field: 'value',
},
},
},
],
}),
];
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", () => {
Expand Down