[Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items #72748

yctercero · 2020-07-21T21:29:25Z

Summary

This PR updates the exception list entries schemas.

Prior: entries could be undefined or empty array on ExceptionListItemSchema
- Now: entries is a required field that cannot be empty - there's really no use for an item without entries
Prior: field and value could be empty string in EntryMatch
- Now: field and value can no longer be empty strings
Prior: field could be empty string and value could be empty array in EntryMatchAny
- Now: field and value can no longer be empty string and array respectively
Prior: field and list.id could be empty string in EntryList
- Now: field and list.id can no longer be empty strings
Prior: field could be empty string in EntryExists
- Now: field can no longer be empty string
Prior: field could be empty string in EntryNested
- Now: field can no longer be empty string
Prior: entries could be empty array in EntryNested
- Now: entries can no longer be empty array

...also...

breaks up the entries.ts file so now each entry type is in its own file. @FrankHassanabad helped me find that having everything in one file was starting to cause some circular dependency issues 🎪
updates build_exception_query.ts to allow for more than just match in nested types as KQL supports it

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

madirey · 2020-07-21T23:47:45Z

x-pack/plugins/lists/common/schemas/types/entries.mock.ts

-  field: FIELD,
-  type: NESTED,
-});
+import { EntriesArray } from './entries';


x-pack/plugins/lists/common/schemas/types/entries.mock.ts

rylnd

I had a few nits and a few questions about strange error assertions, but I don't think that should block this from being merged!

rylnd · 2020-07-21T23:30:25Z

x-pack/plugins/lists/common/schemas/request/create_exception_list_item_schema.ts

  DefaultCreateCommentsArray,
-  DefaultEntryArray,
  NamespaceType,
+  nonEmptyEntriesArray,


Nit: these seem to normally be exported as uppercase constants. I know you're exporting TypeOf with that name, but maybe we need a convention for that if there isn't one.

Yea, spoke with @FrankHassanabad about that earlier today. He'd suggested going lowercase here to differentiate as throughout we tend to use the lowercase for io-ts. Def something we should discuss to settle on one way or the other.

x-pack/plugins/lists/common/schemas/types/entries.mock.ts

rylnd · 2020-07-21T23:36:16Z

x-pack/plugins/lists/common/schemas/types/entry_list.ts

+    type: t.keyof({ list: null }),
+  })
+);
+export type EntryList = t.TypeOf<typeof entriesList>;


nit: Should this just be the capitalized version, i.e. EntriesList?

x-pack/plugins/lists/common/schemas/types/entry_nested.mock.ts

rylnd · 2020-07-21T23:41:23Z

x-pack/plugins/lists/common/schemas/types/non_empty_entries_array.test.ts

+    const decoded = nonEmptyEntriesArray.decode(payload);
+    const message = pipe(decoded, foldLeftRight);
+
+    expect(getPaths(left(message.errors))).toEqual([


Maybe I'm missing something, but why are we expecting five (5) error messages here?

It's because it's a union, and on unions and checks each one. Def could clean up in our error formatter.

x-pack/plugins/lists/common/schemas/types/non_empty_nested_entries_array.test.ts

x-pack/plugins/lists/common/schemas/types/entry_exists.test.ts

x-pack/plugins/security_solution/public/common/components/autocomplete/helpers.ts

x-pack/plugins/security_solution/public/common/components/exceptions/helpers.tsx

x-pack/plugins/lists/common/schemas/types/entries.test.ts

madirey · 2020-07-22T00:15:31Z

x-pack/plugins/lists/common/schemas/types/non_empty_nested_entries_array.ts

+  unknown
+>(
+  'NonEmptyNestedEntriesArray',
+  nestedEntriesArray.is,


I'm not 100% sure about this, but I think this is supposed to align with the validation below... in this case, an empty array will still test true here, but will fail to validate and thus fail to decode to a NestedEntriesArray below...

madirey · 2020-07-22T00:21:32Z

x-pack/plugins/lists/common/schemas/types/non_empty_or_nullable_string_array.ts

+ */
+export const nonEmptyOrNullableStringArray = new t.Type<string[], string[], unknown>(
+  'NonEmptyOrNullableStringArray',
+  t.array(t.string).is,


Same thing here.

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.test.ts

madirey

May be some things we want to address, but we can follow up with that later!

marshallmain

buildNested logic change looks good!

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.ts

kibanamachine · 2020-07-22T00:58:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6a73892

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lists	101	+7	94

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	+3.2KB	7.3MB

page load bundle size

id	value	diff	baseline
lists	267.7KB	+11.0KB	256.7KB

History

💔 Build #63444 failed 4558f2c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…mpty string values in exception list items (elastic#72748) ## Summary This PR updates the exception list entries schemas. - **Prior:** `entries` could be `undefined` or empty array on `ExceptionListItemSchema` - **Now:** `entries` is a required field that cannot be empty - there's really no use for an item without `entries` - **Prior:** `field` and `value` could be empty string in `EntryMatch` - **Now:** `field` and `value` can no longer be empty strings - **Prior:** `field` could be empty string and `value` could be empty array in `EntryMatchAny` - **Now:** `field` and `value` can no longer be empty string and array respectively - **Prior:** `field` and `list.id` could be empty string in `EntryList` - **Now:** `field` and `list.id` can no longer be empty strings - **Prior:** `field` could be empty string in `EntryExists` - **Now:** `field` can no longer be empty string - **Prior:** `field` could be empty string in `EntryNested` - **Now:** `field` can no longer be empty string - **Prior:** `entries` could be empty array in `EntryNested` - **Now:** `entries` can no longer be empty array

…mpty string values in exception list items (#72748) (#72779) ## Summary This PR updates the exception list entries schemas. - **Prior:** `entries` could be `undefined` or empty array on `ExceptionListItemSchema` - **Now:** `entries` is a required field that cannot be empty - there's really no use for an item without `entries` - **Prior:** `field` and `value` could be empty string in `EntryMatch` - **Now:** `field` and `value` can no longer be empty strings - **Prior:** `field` could be empty string and `value` could be empty array in `EntryMatchAny` - **Now:** `field` and `value` can no longer be empty string and array respectively - **Prior:** `field` and `list.id` could be empty string in `EntryList` - **Now:** `field` and `list.id` can no longer be empty strings - **Prior:** `field` could be empty string in `EntryExists` - **Now:** `field` can no longer be empty string - **Prior:** `field` could be empty string in `EntryNested` - **Now:** `field` can no longer be empty string - **Prior:** `entries` could be empty array in `EntryNested` - **Now:** `entries` can no longer be empty array

…mpty string values in exception list items (#72748) (#72780) ## Summary This PR updates the exception list entries schemas. - **Prior:** `entries` could be `undefined` or empty array on `ExceptionListItemSchema` - **Now:** `entries` is a required field that cannot be empty - there's really no use for an item without `entries` - **Prior:** `field` and `value` could be empty string in `EntryMatch` - **Now:** `field` and `value` can no longer be empty strings - **Prior:** `field` could be empty string and `value` could be empty array in `EntryMatchAny` - **Now:** `field` and `value` can no longer be empty string and array respectively - **Prior:** `field` and `list.id` could be empty string in `EntryList` - **Now:** `field` and `list.id` can no longer be empty strings - **Prior:** `field` could be empty string in `EntryExists` - **Now:** `field` can no longer be empty string - **Prior:** `field` could be empty string in `EntryNested` - **Now:** `field` can no longer be empty string - **Prior:** `entries` could be empty array in `EntryNested` - **Now:** `entries` can no longer be empty array

* master: (23 commits) Stabilize closing toast (elastic#72097) stabilize failing test (elastic#72086) Stabilize filter bar test (elastic#72032) Unskip vislib tests (elastic#71452) [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689) fix preAuth/preRouting mocks (elastic#72663) [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788) [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584) [Security Solution] Fixes exception modal not loading content (elastic#72770) [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748) [Detections] Add validation for Threshold value field (elastic#72611) [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730) [Security Solution][Detections] Validate file type of value lists (elastic#72746) [pre-req] New Component Layout proposal (elastic#72385) [ML] do not throw an error when agg is not supported by UI (elastic#72685) [Resolver] Origin process (elastic#72382) [Ingest Manager] Allow to force unenroll from the UI (elastic#72386) skip 6.8 branch when triggering baseline-capture builds (elastic#72706) [CI] In-progress PR comments (elastic#72211) Fix sorting of scripted string fields (elastic#72681) ...

* master: (34 commits) Adds Role Based Access-Control to the Alerting & Action plugins based on Kibana Feature Controls (elastic#67157) [Monitoring] Revert direct shipping code (elastic#72505) Use server basepath when creating reporting jobs (elastic#72722) Adding api test for transaction_groups /breakdown and /avg_duration_by_browser (elastic#72623) [Task Manager] Addresses flaky test introduced by buffered store (elastic#72815) [Observability] filter "hasData" api by processor event (elastic#72810) do not pass title as part of tsvb request (elastic#72619) [Lens] Legend config (elastic#70619) Stabilize closing toast (elastic#72097) stabilize failing test (elastic#72086) Stabilize filter bar test (elastic#72032) Unskip vislib tests (elastic#71452) [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689) fix preAuth/preRouting mocks (elastic#72663) [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788) [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584) [Security Solution] Fixes exception modal not loading content (elastic#72770) [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748) [Detections] Add validation for Threshold value field (elastic#72611) [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730) ...

…ort (#73865) ## Summary Addresses feedback from #72748 - Updates `plugins/lists` tests text from `should not validate` to `should FAIL validation` after feedback that previous text is a bit confusing and can be interpreted to mean that validation is not conducted - Remove unnecessary spreads from one of my late night PRs - Removes `siem_common_deps` in favor of `shared_imports` in `plugins/lists` - Updates `build_exceptions_query.test.ts` to use existing mocks

…ort (elastic#73865) ## Summary Addresses feedback from elastic#72748 - Updates `plugins/lists` tests text from `should not validate` to `should FAIL validation` after feedback that previous text is a bit confusing and can be interpreted to mean that validation is not conducted - Remove unnecessary spreads from one of my late night PRs - Removes `siem_common_deps` in favor of `shared_imports` in `plugins/lists` - Updates `build_exceptions_query.test.ts` to use existing mocks

…ort (#73865) (#73905) ## Summary Addresses feedback from #72748 - Updates `plugins/lists` tests text from `should not validate` to `should FAIL validation` after feedback that previous text is a bit confusing and can be interpreted to mean that validation is not conducted - Remove unnecessary spreads from one of my late night PRs - Removes `siem_common_deps` in favor of `shared_imports` in `plugins/lists` - Updates `build_exceptions_query.test.ts` to use existing mocks

elasticmachine · 2021-09-23T14:29:11Z

Pinging @elastic/security-solution (Team: SecuritySolution)

yctercero added 5 commits July 21, 2020 08:42

updated lists plugin exception item nested entry type and tests

b20c066

updated backend buildNested function to include other operators

eabf1ac

updating tests and types

4558f2c

tidy up types

d0a43de

update tests

6a73892

yctercero marked this pull request as ready for review July 21, 2020 23:22

yctercero requested review from a team as code owners July 21, 2020 23:22

yctercero self-assigned this Jul 21, 2020

yctercero added release_note:enhancement Team:SIEM v7.10.0 v7.9.0 v8.0.0 labels Jul 21, 2020

madirey reviewed Jul 21, 2020

View reviewed changes

x-pack/plugins/lists/common/schemas/types/entries.mock.ts Show resolved Hide resolved

rylnd approved these changes Jul 21, 2020

View reviewed changes

madirey reviewed Jul 21, 2020

View reviewed changes

x-pack/plugins/lists/common/schemas/types/entries.test.ts Show resolved Hide resolved

madirey reviewed Jul 22, 2020

View reviewed changes

x-pack/plugins/lists/common/schemas/types/entries.test.ts Show resolved Hide resolved

madirey reviewed Jul 22, 2020

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.test.ts Show resolved Hide resolved

madirey reviewed Jul 22, 2020

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.test.ts Show resolved Hide resolved

madirey approved these changes Jul 22, 2020

View reviewed changes

marshallmain approved these changes Jul 22, 2020

View reviewed changes

x-pack/plugins/security_solution/common/detection_engine/build_exceptions_query.ts Show resolved Hide resolved

yctercero merged commit 9c7d65c into elastic:master Jul 22, 2020

yctercero mentioned this pull request Jul 22, 2020

[7.x] [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (#72748) #72779

Merged

yctercero mentioned this pull request Jul 22, 2020

[7.9] [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (#72748) #72780

Merged

yctercero mentioned this pull request Jul 30, 2020

[Security Solution][Lists] - Tests cleanup and remove unnecessary import #73865

Merged

1 task

yctercero deleted the de_nested branch October 14, 2020 12:01

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

[Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items #72748

[Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items #72748

Uh oh!

Conversation

yctercero commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rylnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madirey left a comment

Choose a reason for hiding this comment

Uh oh!

marshallmain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Jul 22, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

Uh oh!

elasticmachine commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yctercero commented Jul 21, 2020 •

edited

Loading