[Security Solution] Fixes assert unreachable to be within the common section and the type to never (#75798)

## Summary

Assert unreachable was created through advice given by both the Typescript community and through the techniques that TyepScript is trying to achieve type safety with switch statements.

This fixes recent bugs by:
* Re-adding the never type
* Reduces the two different types by putting the helper within the common section so there's not duplication
* Fixes on type that looks like it was a regular string rather than a one of the enum types

The reasoning for exhaustive checks within switch statements and techniques can be seen in numerous areas such as here:
https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript

You can do it either way with TypeScript as long as you ensure you have a explicit return type and you do early return statements you can actually avoid having to call into the assertUnreachable.

If introduced and used correctly it is there to help out like this error it is telling us that this string type is not exhaustive:
<img width="921" alt="Screen Shot 2020-08-24 at 10 39 42 AM" src="https://user-images.githubusercontent.com/1151048/91075618-9b1ad380-e5fb-11ea-9200-1c355faf5dca.png">

You can notice that for this pull request I actually remove the assertion like so if someone accidentally removes one of the switch statements:
<img width="1014" alt="Screen Shot 2020-08-24 at 10 42 08 AM" src="https://user-images.githubusercontent.com/1151048/91075662-a968ef80-e5fb-11ea-8d74-a92eedd63892.png">

And since the function has an explicit return type it is not needed. You will see that TypeScript improved its never types behind the scenes where it actually will tell you that it will never reach the `assertUnreachable` and want to remove it as an auto-refactor. That is ok as long as we have explicit return types and what I did with one line of code here.

<img width="536" alt="Screen Shot 2020-08-24 at 11 21 05 AM" src="https://user-images.githubusercontent.com/1151048/91075861-efbe4e80-e5fb-11ea-9991-dda111a04f1d.png">

Without this fix, and having the never type become an unknown it introduces less safety where any code that is utilizing the assertUnknown without explicit return types will be prone to having run time errors being thrown when something new is added to their switch enum types.
This commit is contained in:
Frank Hassanabad 2020-08-25 09:22:13 -06:00 committed by GitHub
parent 1dc48b3fdd
commit 8f85593910
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 39 additions and 38 deletions

View file

@ -40,7 +40,7 @@ export enum Direction {
}
export interface SortField {
field: string;
field: 'lastSeen' | 'hostName';
direction: Direction;
}

View file

@ -26,3 +26,21 @@ export const stringEnum = <T>(enumObj: T, enumName = 'enum') =>
: runtimeTypes.failure(u, c),
(a) => (a as unknown) as string
);
/**
* Unreachable Assertion helper for scenarios like exhaustive switches.
* For references see: https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript
* This "x" should _always_ be a type of "never" and not change to "unknown" or any other type. See above link or the generic
* concept of exhaustive checks in switch blocks.
*
* Optionally you can avoid the use of this by using early returns and TypeScript will clear your type checking without complaints
* but there are situations and times where this function might still be needed.
* @param x Unreachable field
* @param message Message of error thrown
*/
export const assertUnreachable = (
x: never, // This should always be a type of "never"
message = 'Unknown Field in switch statement'
): never => {
throw new Error(`${message}: ${x}`);
};

View file

@ -24,19 +24,6 @@ export const asArrayIfExists: WrapArrayIfExitts = (value) =>
*/
export type ValueOf<T> = T[keyof T];
/**
* Unreachable Assertion helper for scenarios like exhaustive switches
*
* @param x Unreachable field
* @param message Message of error thrown
*/
export const assertUnreachable = (
x: never,
message = 'Unknown Field in switch statement'
): never => {
throw new Error(`${message}: ${x}`);
};
/**
* Global variables
*/

View file

@ -21,6 +21,7 @@ import { isEmpty } from 'lodash/fp';
import React from 'react';
import styled from 'styled-components';
import { assertUnreachable } from '../../../../../common/utility_types';
import * as i18nSeverity from '../severity_mapping/translations';
import * as i18nRiskScore from '../risk_score_mapping/translations';
import { Threshold } from '../../../../../common/detection_engine/schemas/common/schemas';
@ -33,7 +34,6 @@ import * as i18n from './translations';
import { BuildQueryBarDescription, BuildThreatDescription, ListItems } from './types';
import { SeverityBadge } from '../severity_badge';
import ListTreeIcon from './assets/list_tree_icon.svg';
import { assertUnreachable } from '../../../../common/lib/helpers';
import { AboutStepRiskScore, AboutStepSeverity } from '../../../pages/detection_engine/rules/types';
import { defaultToEmptyTag } from '../../../../common/components/empty_value';

View file

@ -8,6 +8,7 @@ import React, { useMemo, useCallback } from 'react';
import { connect, ConnectedProps } from 'react-redux';
import { IIndexPattern } from 'src/plugins/data/public';
import { assertUnreachable } from '../../../../common/utility_types';
import {
Direction,
HostFields,
@ -17,7 +18,6 @@ import {
HostsSortField,
OsFields,
} from '../../../graphql/types';
import { assertUnreachable } from '../../../common/lib/helpers';
import { State } from '../../../common/store';
import {
Columns,

View file

@ -8,6 +8,7 @@ import React, { useCallback, useMemo } from 'react';
import { connect, ConnectedProps } from 'react-redux';
import deepEqual from 'fast-deep-equal';
import { assertUnreachable } from '../../../../common/utility_types';
import { networkActions, networkModel, networkSelectors } from '../../store';
import {
Direction,
@ -26,7 +27,6 @@ import {
import { getUsersColumns } from './columns';
import * as i18n from './translations';
import { assertUnreachable } from '../../../common/lib/helpers';
const tableType = networkModel.IpDetailsTableType.users;
interface OwnProps {

View file

@ -4,8 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { assertUnreachable } from '../../../../../../../common/utility_types';
import { Direction } from '../../../../../../graphql/types';
import { assertUnreachable } from '../../../../../../common/lib/helpers';
import { ColumnHeaderOptions } from '../../../../../../timelines/store/timeline/model';
import { Sort, SortDirection } from '../../sort';

View file

@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { assertUnreachable } from '../../../../common/utility_types';
import { getQueryFilter } from '../../../../common/detection_engine/get_query_filter';
import {
LanguageOrUndefined,
@ -15,7 +16,6 @@ import {
} from '../../../../common/detection_engine/schemas/common/schemas';
import { ExceptionListItemSchema } from '../../../../../lists/common/schemas';
import { AlertServices } from '../../../../../alerts/server';
import { assertUnreachable } from '../../../utils/build_query';
import { PartialFilter } from '../types';
import { BadRequestError } from '../errors/bad_request_error';

View file

@ -4,8 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { assertUnreachable } from '../../../../common/utility_types';
import { JobStatus } from '../../../../common/detection_engine/schemas/common/schemas';
import { assertUnreachable } from '../../../utils/build_query';
import { IRuleStatusAttributes } from '../rules/types';
import { getOrCreateRuleStatuses } from './get_or_create_rule_statuses';
import { RuleStatusSavedObjectsClient } from './rule_status_saved_objects_client';

View file

@ -6,9 +6,9 @@
import { isEmpty } from 'lodash/fp';
import { assertUnreachable } from '../../../common/utility_types';
import { LastEventTimeRequestOptions } from './types';
import { LastEventIndexKey } from '../../graphql/types';
import { assertUnreachable } from '../../utils/build_query';
interface EventIndices {
[key: string]: string[];

View file

@ -6,8 +6,9 @@
import { isEmpty } from 'lodash/fp';
import { assertUnreachable } from '../../../common/utility_types';
import { Direction, HostsFields, HostsSortField } from '../../graphql/types';
import { assertUnreachable, createQueryFilterClauses } from '../../utils/build_query';
import { createQueryFilterClauses } from '../../utils/build_query';
import { HostsRequestOptions } from '.';

View file

@ -4,8 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { assertUnreachable } from '../../../common/utility_types';
import { Direction, UsersFields, UsersSortField } from '../../graphql/types';
import { assertUnreachable, createQueryFilterClauses } from '../../utils/build_query';
import { createQueryFilterClauses } from '../../utils/build_query';
import { UsersRequestOptions } from './index';

View file

@ -6,8 +6,9 @@
import { isEmpty } from 'lodash/fp';
import { assertUnreachable } from '../../../common/utility_types';
import { Direction, NetworkDnsFields, NetworkDnsSortField } from '../../graphql/types';
import { assertUnreachable, createQueryFilterClauses } from '../../utils/build_query';
import { createQueryFilterClauses } from '../../utils/build_query';
import { NetworkDnsRequestOptions } from './index';

View file

@ -10,8 +10,8 @@ import {
NetworkTopTablesSortField,
NetworkTopTablesFields,
} from '../../graphql/types';
import { assertUnreachable, createQueryFilterClauses } from '../../utils/build_query';
import { createQueryFilterClauses } from '../../utils/build_query';
import { assertUnreachable } from '../../../common/utility_types';
import { NetworkTopCountriesRequestOptions } from './index';
const getCountAgg = (flowTarget: FlowTargetSourceDest) => ({

View file

@ -4,13 +4,14 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { assertUnreachable } from '../../../common/utility_types';
import {
Direction,
FlowTargetSourceDest,
NetworkTopTablesSortField,
NetworkTopTablesFields,
} from '../../graphql/types';
import { assertUnreachable, createQueryFilterClauses } from '../../utils/build_query';
import { createQueryFilterClauses } from '../../utils/build_query';
import { NetworkTopNFlowRequestOptions } from './index';

View file

@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { createQueryFilterClauses, assertUnreachable } from '../../utils/build_query';
import { assertUnreachable } from '../../../common/utility_types';
import { createQueryFilterClauses } from '../../utils/build_query';
import { TlsRequestOptions } from './index';
import { TlsSortField, Direction, TlsFields } from '../../graphql/types';

View file

@ -11,7 +11,7 @@ import {
HostsRequestOptions,
SortField,
} from '../../../../../../common/search_strategy/security_solution';
import { assertUnreachable, createQueryFilterClauses } from '../../../../../utils/build_query';
import { createQueryFilterClauses } from '../../../../../utils/build_query';
export const buildHostsQuery = ({
defaultIndex,
@ -83,7 +83,5 @@ const getQueryOrder = (sort: SortField): QueryOrder => {
return { lastSeen: sort.direction };
case 'hostName':
return { _key: sort.direction };
default:
return assertUnreachable(sort.field);
}
};

View file

@ -9,13 +9,6 @@ export * from './filters';
export * from './merge_fields_with_hits';
export * from './calculate_timeseries_interval';
export const assertUnreachable = (
x: unknown,
message: string = 'Unknown Field in switch statement'
): never => {
throw new Error(`${message} ${x}`);
};
export const inspectStringifyObject = (obj: unknown) => {
try {
return JSON.stringify(obj, null, 2);