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][Endpoint Exceptions] Wildcard warning with IS operator for endpoint exceptions creation/editing #182903

Merged
merged 43 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
0ee80c0
working callout warning
parkiino Apr 16, 2024
8982ba0
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Apr 18, 2024
3c7644c
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Apr 29, 2024
7769015
working basic fixed callout
parkiino May 7, 2024
2e30f0c
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 8, 2024
e017929
revert code and use common function, wip for edit
parkiino May 9, 2024
67a18cc
working modal
parkiino May 14, 2024
a067bc7
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 14, 2024
4ebc021
use array.some
parkiino May 15, 2024
0a70be9
get has wrong operator unit test
parkiino May 16, 2024
9eff267
Merge branch 'main' into task/endpoint-exceptions-is-wildcard
vitaliidm May 17, 2024
2e64791
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 20, 2024
24ac26d
remove random file, remove unnecessary arg
parkiino May 20, 2024
35ac720
Merge branch 'task/endpoint-exceptions-is-wildcard' of github.com:par…
parkiino May 20, 2024
6023385
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 21, 2024
8e67f21
fix type errors
parkiino May 22, 2024
273975b
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 22, 2024
dba86b8
spacing
parkiino May 22, 2024
dcb1345
adjust naming for edit exception flyout reducer
parkiino May 23, 2024
ec6eefd
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 23, 2024
1c90baa
add initial value for edit reducer
parkiino May 23, 2024
87b1dcb
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 24, 2024
897f171
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 28, 2024
17b9665
unit test wip
parkiino May 31, 2024
ef2ad5d
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino May 31, 2024
6d4adf0
working callout unit test
parkiino Jun 4, 2024
b2ad473
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 4, 2024
a448c9e
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jun 4, 2024
fddd40d
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 6, 2024
a4962c8
adjust tests, and text
parkiino Jun 6, 2024
1772390
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 6, 2024
fd43dbb
fix test function name
parkiino Jun 7, 2024
4704ac4
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 7, 2024
deb623c
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 10, 2024
315b43a
rename getHasWrongOperator"
parkiino Jun 10, 2024
885046d
lint
parkiino Jun 10, 2024
351ee80
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 10, 2024
e685ab4
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 10, 2024
22d932a
fix function and variable names
parkiino Jun 10, 2024
f90749d
Merge remote-tracking branch 'upstream/main' into task/endpoint-excep…
parkiino Jun 10, 2024
d86d499
Merge branch 'main' into task/endpoint-exceptions-is-wildcard
kibanamachine Jun 11, 2024
8c11c8b
Merge branch 'main' into task/endpoint-exceptions-is-wildcard
kibanamachine Jun 11, 2024
5fa9e41
Merge branch 'main' into task/endpoint-exceptions-is-wildcard
kibanamachine Jun 11, 2024
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 @@ -39,6 +39,7 @@ const BOOLEAN_OPTIONS = [
];

const SINGLE_SELECTION = { asPlainText: true };

type Warning = string | React.ReactNode;

interface AutocompleteFieldMatchProps {
Expand Down Expand Up @@ -152,9 +153,10 @@ export const AutocompleteFieldMatchComponent: React.FC<AutocompleteFieldMatchPro

handleSpacesWarning(newValue);
handleError(undefined);
handleWarning(undefined);
onChange(newValue ?? '');
},
[handleError, handleSpacesWarning, labels, onChange, optionsMemo]
[handleError, handleWarning, handleSpacesWarning, labels, onChange, optionsMemo]
);

const handleSearchChange = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const WildCardWithWrongOperatorCallout = () => {
<p>
<FormattedMessage
id="exceptionList-components.wildcardWithWrongOperatorCallout.body"
defaultMessage="Using a '*' or a '?' in the value with the 'IS' operator can make the entry ineffective. {operator} to ''{matches}'' to ensure wildcards run properly."
defaultMessage='Using a "*" or a "?" in the value with the "is" operator can make the entry ineffective. {operator} to "{matches}" to ensure wildcards run properly.'
values={{
operator: (
<strong>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { getMappingConflictsInfo, fieldSupportsMatches } from '.';
import { getMappingConflictsInfo, fieldSupportsMatches, hasWrongOperatorWithWildcard } from '.';

describe('Helpers', () => {
describe('getMappingConflictsInfo', () => {
Expand Down Expand Up @@ -181,4 +181,43 @@ describe('Helpers', () => {
).toBeFalsy();
});
});
describe('hasWrongOperatorWithWildcard', () => {
test('it returns true if there is at least one exception entry with a wildcard and the wrong operator', () => {
expect(
hasWrongOperatorWithWildcard([
{
description: '',
name: '',
type: 'simple',
entries: [{ type: 'match', value: 'withwildcard*', field: '', operator: 'included' }],
},
])
).toBeTruthy();
expect(
hasWrongOperatorWithWildcard([
{
description: '',
name: '',
type: 'simple',
entries: [{ type: 'match', value: 'withwildcard?', field: '', operator: 'included' }],
},
])
).toBeTruthy();
});
test('it returns false if there are no exception entries with a wildcard and the wrong operator', () => {
expect(
hasWrongOperatorWithWildcard([
{
description: '',
name: '',
type: 'simple',
entries: [
{ type: 'match', value: 'nowildcard', field: '', operator: 'excluded' },
{ type: 'wildcard', value: 'withwildcard*?', field: '', operator: 'included' },
],
},
])
).toBeFalsy();
});
});
});
22 changes: 21 additions & 1 deletion packages/kbn-securitysolution-list-utils/src/helpers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
*/

import { v4 as uuidv4 } from 'uuid';
import { addIdToItem, removeIdFromItem } from '@kbn/securitysolution-utils';
import {
addIdToItem,
removeIdFromItem,
validateHasWildcardWithWrongOperator,
} from '@kbn/securitysolution-utils';
import { validate } from '@kbn/securitysolution-io-ts-utils';
import {
CreateExceptionListItemSchema,
Expand Down Expand Up @@ -1021,3 +1025,19 @@ export const getMappingConflictsInfo = (field: DataViewField): FieldConflictsInf
}
return conflicts;
};

/**
* Given an exceptions list, determine if any entries have an "IS" operator with a wildcard value
*/
export const hasWrongOperatorWithWildcard = (
items: ExceptionsBuilderReturnExceptionItem[]
): boolean => {
return items[0]?.entries.some((e) => {
if (e.type !== 'list' && 'value' in e) {
return validateHasWildcardWithWrongOperator({
operator: e.type,
value: e.value,
});
}
});
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import {
hasSimpleExecutableName,
OperatingSystem,
ConditionEntryField,
validateWildcardInput,
validateHasWildcardWithWrongOperator,
validatePotentialWildcardInput,
validateFilePathInput,
validateWildcardInput,
WILDCARD_WARNING,
FILEPATH_WARNING,
} from '.';
Expand Down
16 changes: 12 additions & 4 deletions packages/kbn-securitysolution-utils/src/path_validations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,17 @@ export const validateFilePathInput = ({
}
};

export const validateWildcardInput = (value?: string): string | undefined => {
if (/[*?]/.test(value ?? '')) {
return WILDCARD_WARNING;
export const validateWildcardInput = (value: string | string[]): string | undefined => {
const wildcardRegex = /[*?]/;
if (Array.isArray(value)) {
const doesAnyValueContainWildcardInput = value.some((v) => wildcardRegex.test(v));
if (doesAnyValueContainWildcardInput) {
return WILDCARD_WARNING;
}
} else {
if (wildcardRegex.test(value)) {
return WILDCARD_WARNING;
}
}
};

Expand All @@ -112,7 +120,7 @@ export const validateHasWildcardWithWrongOperator = ({
value,
}: {
operator: TrustedAppEntryTypes | EventFiltersTypes;
value: string;
value: string | string[];
}): boolean => {
if (operator !== 'wildcard' && validateWildcardInput(value)) {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,10 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
([newOperator]: OperatorOption[]): void => {
const { updatedEntry, index } = getEntryOnOperatorChange(entry, newOperator);
handleError(false);
handleWarning(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if you could think of a different way to manage these errors/warning? Just updating multiple states at once seems odd, and also having wrappers just for calling prop function seems redundant.
Would you have some time to refactor this component a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think refactoring would probably have to be at a later stage if that's OK

onChange(updatedEntry, index);
},
[onChange, entry, handleError]
[onChange, entry, handleError, handleWarning]
);

const handleFieldMatchValueChange = useCallback(
Expand Down Expand Up @@ -420,7 +421,7 @@ export const BuilderEntryItem: React.FC<EntryItemProps> = ({
const value = typeof entry.value === 'string' ? entry.value : undefined;
const fieldMatchWarning = /[*?]/.test(value ?? '')
? getWildcardWithIsOperatorWarning()
: '';
: undefined;
return (
<AutocompleteFieldMatchComponent
autocompleteService={autocompleteService}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,32 @@ describe('When the add exception modal is opened', () => {
it('should NOT display the eql sequence callout', () => {
expect(wrapper.find('[data-test-subj="eqlSequenceCallout"]').exists()).not.toBeTruthy();
});

it('should show a warning callout if wildcard is used', async () => {
const callProps = mockGetExceptionBuilderComponentLazy.mock.calls[0][0];
await waitFor(() =>
callProps.onChange({
exceptionItems: [
{
...getExceptionListItemSchemaMock(),
entries: [
{
field: 'event.category',
operator: 'included',
type: 'match',
value: 'wildcardvalue*?',
},
],
},
],
})
);

wrapper.update();
expect(
wrapper.find('[data-test-subj="wildcardWithWrongOperatorCallout"]').exists()
).toBeTruthy();
});
});

describe('alert data is passed in', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import React, { memo, useEffect, useCallback, useMemo, useReducer } from 'react';
import React, { memo, useEffect, useCallback, useMemo, useReducer, useState } from 'react';
import styled, { css } from 'styled-components';
import { isEmpty } from 'lodash/fp';

Expand All @@ -30,11 +30,13 @@ import {
import { ENDPOINT_LIST_ID } from '@kbn/securitysolution-list-constants';
import { ExceptionListTypeEnum } from '@kbn/securitysolution-io-ts-list-types';
import type { OsTypeArray, ExceptionListSchema } from '@kbn/securitysolution-io-ts-list-types';
import { hasWrongOperatorWithWildcard } from '@kbn/securitysolution-list-utils';
import type {
ExceptionsBuilderExceptionItem,
ExceptionsBuilderReturnExceptionItem,
} from '@kbn/securitysolution-list-utils';

import { WildCardWithWrongOperatorCallout } from '@kbn/securitysolution-exception-list-components';
import type { Moment } from 'moment';
import type { Status } from '../../../../../common/api/detection_engine';
import * as i18n from './translations';
Expand All @@ -44,6 +46,7 @@ import {
retrieveAlertOsTypes,
getPrepopulatedRuleExceptionWithHighlightFields,
} from '../../utils/helpers';
import { RULE_EXCEPTION, ENDPOINT_EXCEPTION } from '../../utils/translations';
import type { AlertData } from '../../utils/types';
import { initialState, createExceptionItemsReducer } from './reducer';
import { ExceptionsFlyoutMeta } from '../flyout_components/item_meta_form';
Expand All @@ -58,6 +61,8 @@ import { useCloseAlertsFromExceptions } from '../../logic/use_close_alerts';
import { ruleTypesThatAllowLargeValueLists } from '../../utils/constants';
import { useInvalidateFetchRuleByIdQuery } from '../../../rule_management/api/hooks/use_fetch_rule_by_id_query';
import { ExceptionsExpireTime } from '../flyout_components/expire_time';
import { CONFIRM_WARNING_MODAL_LABELS } from '../../../../management/common/translations';
import { ArtifactConfirmModal } from '../../../../management/components/artifact_list_page/components/artifact_confirm_modal';

const SectionHeader = styled(EuiTitle)`
${() => css`
Expand Down Expand Up @@ -117,6 +122,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
onConfirm,
}: AddExceptionFlyoutProps) {
const { euiTheme } = useEuiTheme();
const [showConfirmModal, setShowConfirmModal] = useState<boolean>(false);
const { isLoading, indexPatterns, getExtendedFields } = useFetchIndexPatterns(rules);
const [isSubmitting, submitNewExceptionItems] = useAddNewExceptionItems();
const [isClosingAlerts, closeAlerts] = useCloseAlertsFromExceptions();
Expand Down Expand Up @@ -165,6 +171,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
errorSubmitting,
expireTime,
expireErrorExists,
wildcardWarningExists,
},
dispatch,
] = useReducer(createExceptionItemsReducer(), {
Expand Down Expand Up @@ -193,6 +200,10 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({

const setExceptionItemsToAdd = useCallback(
(items: ExceptionsBuilderReturnExceptionItem[]): void => {
dispatch({
type: 'setWildcardWithWrongOperator',
warningExists: hasWrongOperatorWithWildcard(items),
});
dispatch({
type: 'setExceptionItems',
items,
Expand Down Expand Up @@ -380,7 +391,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
return hasAlertData ? retrieveAlertOsTypes(alertData) : selectedOs ? [...selectedOs] : [];
}, [hasAlertData, alertData, selectedOs]);

const handleOnSubmit = useCallback(async (): Promise<void> => {
const submitException = useCallback(async (): Promise<void> => {
if (submitNewExceptionItems == null) return;

try {
Expand Down Expand Up @@ -451,6 +462,14 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
expireTime,
]);

const handleOnSubmit = useCallback(() => {
if (wildcardWarningExists) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that adding this if statement to the original handleOnSubmit makes sense? We could avoid having another layer of abstraction then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would make sense in this case since I needed to separate the original submit handler for the confirmation modal. We allows the user to add the exception without addressing the warning in the confirmation modal so I wanted to have that logic on its own for the confirmation modal submit handler.

setShowConfirmModal(true);
} else {
return submitException();
}
}, [wildcardWarningExists, submitException]);

const isSubmitButtonDisabled = useMemo(
(): boolean =>
isSubmitting ||
Expand Down Expand Up @@ -499,6 +518,24 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
prefix: 'exceptionFlyoutTitle',
});

const confirmModal = useMemo(() => {
const { title, body, confirmButton, cancelButton } = CONFIRM_WARNING_MODAL_LABELS(
listType === ExceptionListTypeEnum.ENDPOINT ? ENDPOINT_EXCEPTION : RULE_EXCEPTION
);

return (
<ArtifactConfirmModal
title={title}
body={body}
confirmButton={confirmButton}
cancelButton={cancelButton}
onSuccess={submitException}
onCancel={() => setShowConfirmModal(false)}
data-test-subj="artifactConfirmModal"
/>
);
}, [listType, submitException]);

return (
<EuiFlyout
size="l"
Expand Down Expand Up @@ -567,7 +604,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
onSetErrorExists={setConditionsValidationError}
getExtendedFields={getExtendedFields}
/>

{wildcardWarningExists && <WildCardWithWrongOperatorCallout />}
{listType !== ExceptionListTypeEnum.ENDPOINT && !sharedListToAddTo?.length && (
<>
<EuiHorizontalRule />
Expand Down Expand Up @@ -639,6 +676,7 @@ export const AddExceptionFlyout = memo(function AddExceptionFlyout({
</EuiButton>
</FlyoutFooterGroup>
</EuiFlyoutFooter>
{showConfirmModal && confirmModal}
</EuiFlyout>
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export interface State {
errorSubmitting: Error | null;
expireTime: Moment | undefined;
expireErrorExists: boolean;
wildcardWarningExists: boolean;
}

export const initialState: State = {
Expand All @@ -55,6 +56,7 @@ export const initialState: State = {
errorSubmitting: null,
expireTime: undefined,
expireErrorExists: false,
wildcardWarningExists: false,
};

export type Action =
Expand Down Expand Up @@ -129,11 +131,15 @@ export type Action =
| {
type: 'setExpireError';
errorExists: boolean;
}
| {
type: 'setWildcardWithWrongOperator';
warningExists: boolean;
};

export const createExceptionItemsReducer =
() =>
/* eslint complexity: ["error", 21]*/
/* eslint complexity: ["error", 22]*/
(state: State, action: Action): State => {
switch (action.type) {
case 'setExceptionItemMeta': {
Expand Down Expand Up @@ -171,6 +177,13 @@ export const createExceptionItemsReducer =
itemConditionValidationErrorExists: errorExists,
};
}
case 'setWildcardWithWrongOperator': {
const { warningExists } = action;
return {
...state,
wildcardWarningExists: warningExists,
};
}
case 'setComment': {
const { comment } = action;

Expand Down Expand Up @@ -250,7 +263,6 @@ export const createExceptionItemsReducer =
}
case 'setListType': {
const { listType } = action;

return {
...state,
listType,
Expand Down
Loading