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] Invalid KQL Query Bug #99442

Merged
merged 20 commits into from
Jun 30, 2021
Merged
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 @@ -50,6 +50,7 @@ jest.mock('../../../common/hooks/use_selector', () => ({
useShallowEqualSelector: jest.fn(),
useDeepEqualSelector: jest.fn(),
}));
jest.mock('../../../common/hooks/use_invalid_filter_query.tsx');

const mockUiSettingsForFilterManager = coreMock.createStart().uiSettings;
const timelineId = TimelineId.active;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ import {
import { GraphOverlay } from '../../../timelines/components/graph_overlay';
import { CellValueElementProps } from '../../../timelines/components/timeline/cell_rendering';
import { SELECTOR_TIMELINE_GLOBAL_CONTAINER } from '../../../timelines/components/timeline/styles';
import { defaultControlColumn } from '../../../timelines/components/timeline/body/control_columns';
import { timelineSelectors, timelineActions } from '../../../timelines/store/timeline';
import { useDeepEqualSelector } from '../../hooks/use_selector';
import { defaultControlColumn } from '../../../timelines/components/timeline/body/control_columns';

export const EVENTS_VIEWER_HEADER_HEIGHT = 90; // px
const UTILITY_BAR_HEIGHT = 19; // px
Expand Down Expand Up @@ -243,7 +243,7 @@ const EventsViewerComponent: React.FC<Props> = ({
sort: sortField,
startDate: start,
endDate: end,
skip: !canQueryTimeline,
skip: !canQueryTimeline || combinedQueries?.filterQuery === undefined, // When the filterQuery comes back as undefined, it means an error has been thrown and the request should be skipped
});

const totalCountMinusDeleted = useMemo(
Expand Down Expand Up @@ -296,6 +296,7 @@ const EventsViewerComponent: React.FC<Props> = ({
height={headerFilterGroup ? COMPACT_HEADER_HEIGHT : EVENTS_VIEWER_HEADER_HEIGHT}
subtitle={utilityBar ? undefined : subtitle}
title={globalFullScreen ? titleWithExitFullScreen : justTitle}
isInspectDisabled={combinedQueries!.filterQuery === undefined}
>
{HeaderSectionContent}
</HeaderSection>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export interface HeaderSectionProps extends HeaderProps {
children?: React.ReactNode;
height?: number;
id?: string;
isInspectDisabled?: boolean;
split?: boolean;
subtitle?: string | React.ReactNode;
title: string | React.ReactNode;
Expand All @@ -55,6 +56,7 @@ const HeaderSectionComponent: React.FC<HeaderSectionProps> = ({
children,
height,
id,
isInspectDisabled,
split,
subtitle,
title,
Expand Down Expand Up @@ -85,7 +87,12 @@ const HeaderSectionComponent: React.FC<HeaderSectionProps> = ({

{id && (
<EuiFlexItem grow={false}>
<InspectButton queryId={id} multiple={inspectMultiple} title={title} />
<InspectButton
isDisabled={isInspectDisabled}
queryId={id}
multiple={inspectMultiple}
title={title}
/>
</EuiFlexItem>
)}
</EuiFlexGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export const MatrixHistogramComponent: React.FC<MatrixHistogramComponentProps> =
title,
titleSize,
yTickFormatter,
skip,
}) => {
const dispatch = useDispatch();
const handleBrushEnd = useCallback(
Expand Down Expand Up @@ -146,6 +147,7 @@ export const MatrixHistogramComponent: React.FC<MatrixHistogramComponentProps> =
stackByField: selectedStackByOption.value,
isPtrIncluded,
docValueFields,
skip,
};

const [loading, { data, inspect, totalCount, refetch }] = useMatrixHistogramCombined(
Expand Down Expand Up @@ -216,6 +218,7 @@ export const MatrixHistogramComponent: React.FC<MatrixHistogramComponentProps> =
titleSize={titleSize}
subtitle={subtitleWithCounts}
inspectMultiple
isInspectDisabled={filterQuery === undefined}
>
<EuiFlexGroup alignItems="center" gutterSize="none">
<EuiFlexItem grow={false}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ const AnomaliesHostTableComponent: React.FC<AnomaliesHostTableProps> = ({
)}`}
title={i18n.ANOMALIES}
tooltip={i18n.TOOLTIP}
isInspectDisabled={skip}
/>

<BasicTable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const AnomaliesNetworkTableComponent: React.FC<AnomaliesNetworkTableProps> = ({
)}`}
title={i18n.ANOMALIES}
tooltip={i18n.TOOLTIP}
isInspectDisabled={skip}
/>

<BasicTable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ export const useMatrixHistogramCombined = (
const [missingDataLoading, missingDataResponse] = useMatrixHistogram({
...matrixHistogramQueryProps,
includeMissingData: false,
skip: skipMissingData,
skip: skipMissingData || matrixHistogramQueryProps.filterQuery === undefined,
});

const combinedLoading = useMemo<boolean>(() => mainLoading || missingDataLoading, [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* 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 { useEffect, useMemo } from 'react';
import { useDispatch } from 'react-redux';
import { Query } from 'src/plugins/data/public';
import { appSelectors } from '../store';
import { appActions } from '../store/app';
import { useAppToasts } from './use_app_toasts';
import { useDeepEqualSelector } from './use_selector';

/**
* Adds a toast error message whenever invalid KQL is submitted through the search bar
*/
export const useInvalidFilterQuery = ({
id,
filterQuery,
kqlError,
query,
startDate,
endDate,
}: {
id: string;
filterQuery?: string;
kqlError?: Error;
query: Query;
startDate: string;
endDate: string;
}) => {
const { addError } = useAppToasts();
const dispatch = useDispatch();
const getErrorsSelector = useMemo(() => appSelectors.errorsSelector(), []);
const errors = useDeepEqualSelector(getErrorsSelector);

useEffect(() => {
if (filterQuery === undefined && kqlError != null) {
// Local util for creating an replicatable error hash
const hashCode = kqlError.message
.split('')
// eslint-disable-next-line no-bitwise
.reduce((a, b) => ((a << 5) - a + b.charCodeAt(0)) | 0, 0)
.toString();
dispatch(
appActions.addErrorHash({
id,
hash: hashCode,
title: kqlError.name,
message: [kqlError.message],
})
);
}
// This disable is required to only trigger the toast once per render
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [id, filterQuery, addError, query, startDate, endDate]);

useEffect(() => {
const myError = errors.find((e) => e.id === id);
if (myError != null && myError.displayError && kqlError != null) {
// Removes error stack from user view
delete kqlError.stack; // Mutates the error object and can possibly lead to side effects, only going this route for type issues. Change when we add a stackless toast error
addError(kqlError, { title: kqlError.name });
}
}, [addError, errors, id, kqlError]);
};
31 changes: 17 additions & 14 deletions x-pack/plugins/security_solution/public/common/lib/keury/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,23 @@ export const convertToBuildEsQuery = ({
indexPattern: IIndexPattern;
queries: Query[];
filters: Filter[];
}) => {
}): [string, undefined] | [undefined, Error] => {
try {
return JSON.stringify(
esQuery.buildEsQuery(
indexPattern,
queries,
filters.filter((f) => f.meta.disabled === false),
{
...config,
dateFormatTZ: undefined,
}
)
);
} catch (exp) {
return '';
return [
JSON.stringify(
esQuery.buildEsQuery(
indexPattern,
queries,
filters.filter((f) => f.meta.disabled === false),
{
...config,
dateFormatTZ: undefined,
}
)
),
undefined,
];
} catch (error) {
return [undefined, error];
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,10 @@ export const addError = actionCreator<{ id: string; title: string; message: stri
);

export const removeError = actionCreator<{ id: string }>('REMOVE_ERRORS');

export const addErrorHash = actionCreator<{
id: string;
hash: string;
title: string;
message: string[];
}>('ADD_ERROR_HASH');
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export interface Error {
id: string;
title: string;
message: string[];
hash?: string;
displayError?: boolean;
}

export type ErrorModel = Error[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { reducerWithInitialState } from 'typescript-fsa-reducers';

import { Note } from '../../lib/note';

import { addError, addNotes, removeError, updateNote } from './actions';
import { addError, addErrorHash, addNotes, removeError, updateNote } from './actions';
import { AppModel, NotesById } from './model';

export type AppState = AppModel;
Expand Down Expand Up @@ -46,4 +46,26 @@ export const appReducer = reducerWithInitialState(initialAppState)
...state,
errors: state.errors.filter((error) => error.id !== id),
}))
.case(addErrorHash, (state, { id, hash, title, message }) => {
const errorIdx = state.errors.findIndex((e) => e.id === id);
const errorObj = state.errors.find((e) => e.id === id) || { id, title, message };
if (errorIdx === -1) {
return {
...state,
errors: state.errors.concat({
...errorObj,
hash,
displayError: !state.errors.some((e) => e.hash === hash),
}),
};
}
return {
...state,
errors: [
...state.errors.slice(0, errorIdx),
{ ...errorObj, hash, displayError: !state.errors.some((e) => e.hash === hash) },
...state.errors.slice(errorIdx + 1),
],
};
})
.build();
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
// create a unique, but stable (across re-renders) query id
const uniqueQueryId = useMemo(() => `${DETECTIONS_HISTOGRAM_ID}-${uuid.v4()}`, []);
const [isInitialLoading, setIsInitialLoading] = useState(true);
const [isInspectDisabled, setIsInspectDisabled] = useState(false);
const [defaultNumberFormat] = useUiSetting$<string>(DEFAULT_NUMBER_FORMAT);
const [totalAlertsObj, setTotalAlertsObj] = useState<AlertsTotal>(defaultTotalAlertsObj);
const [selectedStackByOption, setSelectedStackByOption] = useState<AlertsHistogramOption>(
Expand Down Expand Up @@ -261,7 +262,7 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
}
);
}

setIsInspectDisabled(false);
setAlertsQuery(
getAlertsHistogramQuery(
selectedStackByOption.value,
Expand All @@ -271,6 +272,7 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
)
);
} catch (e) {
setIsInspectDisabled(true);
setAlertsQuery(getAlertsHistogramQuery(selectedStackByOption.value, from, to, []));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -305,6 +307,7 @@ export const AlertsHistogramPanel = memo<AlertsHistogramPanelProps>(
title={titleText}
titleSize={onlyField == null ? 'm' : 's'}
subtitle={!isInitialLoading && showTotalAlertsCount && totalAlerts}
isInspectDisabled={isInspectDisabled}
>
<EuiFlexGroup alignItems="center" gutterSize="none">
<EuiFlexItem grow={false}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import { useSourcererScope } from '../../../common/containers/sourcerer';
import { buildTimeRangeFilter } from './helpers';
import { defaultRowRenderers } from '../../../timelines/components/timeline/body/renderers';
import { columns, RenderCellValue } from '../../configurations/security_solution_detections';
import { useInvalidFilterQuery } from '../../../common/hooks/use_invalid_filter_query';

interface OwnProps {
defaultFilters?: Filter[];
Expand Down Expand Up @@ -132,6 +133,15 @@ export const AlertsTableComponent: React.FC<AlertsTableComponentProps> = ({
[browserFields, defaultFilters, globalFilters, globalQuery, indexPatterns, kibana, to, from]
);

useInvalidFilterQuery({
id: timelineId,
filterQuery: getGlobalQuery([])?.filterQuery,
kqlError: getGlobalQuery([])?.kqlError,
query: globalQuery,
startDate: from,
endDate: to,
});

const setEventsLoadingCallback = useCallback(
({ eventIds, isLoading }: SetEventsLoadingProps) => {
setEventsLoading!({ id: timelineId, eventIds, isLoading });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { UpdateDateRange } from '../../../common/components/charts/common';
import { GlobalTimeArgs } from '../../../common/containers/use_global_time';

export interface HostsKpiProps {
filterQuery: string;
filterQuery?: string;
from: string;
to: string;
indexNames: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { InspectResponse } from '../../../types';
import { useTransforms } from '../../../transforms/containers/use_transforms';
import { useAppToasts } from '../../../common/hooks/use_app_toasts';

const ID = 'hostsAllQuery';
export const ID = 'hostsAllQuery';

type LoadPage = (newActivePage: number) => void;
export interface HostsArgs {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export const HostDetailsTabs = React.memo<HostDetailsTabsProps>(
deleteQuery,
endDate: to,
filterQuery,
skip: isInitializing,
skip: isInitializing || filterQuery === undefined,
setQuery,
startDate: from,
type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ import { TimelineId } from '../../../../common/types/timeline';
import { timelineDefaults } from '../../../timelines/store/timeline/defaults';
import { useSourcererScope } from '../../../common/containers/sourcerer';
import { useDeepEqualSelector, useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { useHostDetails } from '../../containers/hosts/details';
import { ID, useHostDetails } from '../../containers/hosts/details';
import { manageQuery } from '../../../common/components/page/manage_query';
import { useInvalidFilterQuery } from '../../../common/hooks/use_invalid_filter_query';

const HostOverviewManage = manageQuery(HostOverview);

Expand Down Expand Up @@ -103,13 +104,15 @@ const HostDetailsComponent: React.FC<HostDetailsProps> = ({ detailName, hostDeta
indexNames: selectedPatterns,
skip: selectedPatterns.length === 0,
});
const filterQuery = convertToBuildEsQuery({
const [filterQuery, kqlError] = convertToBuildEsQuery({
config: esQuery.getEsQueryConfig(kibana.services.uiSettings),
indexPattern,
queries: [query],
filters: getFilters(),
});

useInvalidFilterQuery({ id: ID, filterQuery, kqlError, query, startDate: from, endDate: to });

useEffect(() => {
dispatch(setHostDetailsTablesActivePageToZero());
}, [dispatch, detailName]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export type HostDetailsTabsProps = HostBodyComponentDispatchProps &
docValueFields?: DocValueFields[];
indexNames: string[];
pageFilters?: Filter[];
filterQuery: string;
filterQuery?: string;
indexPattern: IIndexPattern;
type: hostsModel.HostsType;
};
Expand Down
Loading