[Security Solution] Validate exception list size when adding new items #73399
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| import { ExceptionListClient } from '../services/exception_lists/exception_list_client'; | ||
| import { MAX_EXCEPTION_LIST_SIZE } from '../../common/constants'; | ||
| import { foundExceptionListItemSchema } from '../../common/schemas'; | ||
| import { NamespaceType } from '../../common/schemas/types'; | ||
| import { validate } from '../../common/siem_common_deps'; | ||
|
|
||
| export const validateExceptionListSize = async ( | ||
| exceptionLists: ExceptionListClient, | ||
| listId: string, | ||
| namespaceType: NamespaceType | ||
| ): Promise<{ body: string; statusCode: number } | null> => { | ||
| const exceptionListItems = await exceptionLists.findExceptionListItem({ | ||
| filter: undefined, | ||
| listId, | ||
| namespaceType, | ||
| page: undefined, | ||
| perPage: undefined, | ||
| sortField: undefined, | ||
| sortOrder: undefined, | ||
| }); | ||
| if (exceptionListItems == null) { | ||
| // If exceptionListItems is null then we couldn't find the list so it may have been deleted | ||
| return { | ||
| body: `Unable to find list id: ${listId} to verify max exception list size`, | ||
| statusCode: 500, | ||
| }; | ||
| } | ||
| const [validatedItems, err] = validate(exceptionListItems, foundExceptionListItemSchema); | ||
| if (err != null) { | ||
| return { | ||
| body: err, | ||
| statusCode: 500, | ||
| }; | ||
| } | ||
| // Unnecessary since validatedItems comes from exceptionListItems which is already | ||
| // checked for null, but typescript fails to detect that | ||
| if (validatedItems == null) { | ||
| return { | ||
| body: `Unable to find list id: ${listId} to verify max exception list size`, | ||
| statusCode: 500, | ||
| }; | ||
| } | ||
| if (validatedItems.total > MAX_EXCEPTION_LIST_SIZE) { | ||
| return { | ||
| body: `Failed to add exception item, exception list would exceed max size of ${MAX_EXCEPTION_LIST_SIZE}`, | ||
| statusCode: 400, | ||
| }; | ||
| } | ||
| return null; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import { SavedObjectsClientContract } from 'kibana/server'; | |
|
|
||
| import { | ||
| ExceptionListItemSchema, | ||
| Id, | ||
| IdOrUndefined, | ||
| ItemIdOrUndefined, | ||
| NamespaceType, | ||
|
|
@@ -23,6 +24,12 @@ interface DeleteExceptionListItemOptions { | |
| savedObjectsClient: SavedObjectsClientContract; | ||
| } | ||
|
|
||
| interface DeleteExceptionListItemByIdOptions { | ||
| id: Id; | ||
| namespaceType: NamespaceType; | ||
| savedObjectsClient: SavedObjectsClientContract; | ||
| } | ||
|
|
||
| export const deleteExceptionListItem = async ({ | ||
| itemId, | ||
| id, | ||
|
|
@@ -43,3 +50,12 @@ export const deleteExceptionListItem = async ({ | |
| return exceptionListItem; | ||
| } | ||
| }; | ||
|
|
||
| export const deleteExceptionListItemById = async ({ | ||
| id, | ||
| namespaceType, | ||
| savedObjectsClient, | ||
| }: DeleteExceptionListItemByIdOptions): Promise<void> => { | ||
| const savedObjectType = getSavedObjectType({ namespaceType }); | ||
| await savedObjectsClient.delete(savedObjectType, id); | ||
| }; | ||
|
There was a problem hiding this comment. The function above this works where you can call: deleteExceptionListItem({ itemId: undefined, id, namespaceType, savedObjectsClient });Are you just trying to avoid the extra call to get the item if it does not exist? There was a problem hiding this comment. That, and also it avoids having to check for a There was a problem hiding this comment. Yeah, I hear ya'. I think there's been a bit of back and forth throughout the API's throughout the code with regards to throws and return statements that can contain errors. Sometimes people will want to do a throw and unwind the stack and then other times I see people wrapping those in a catch everywhere and then returning null and preferring the non-throw states (or wrapping it in an object/array that can contain an error and a response and optionally typing both). In some cases people have been using more of a disjoint union such as the Either and io-ts does use this for their return types: (Similar to RustLang but Rust has more first class citizen treatment of it) So I have seen people kind of go back and forth depending on who I program with. You don't have to change anything, I'm fine with it, as it works for your use case. I think in newer contemporary programming languages (golang, rustlang) most errors are now returned and not many throws are used unless they are something like a panic. Regardless of if they're using a disjointed union type, array type, etc... TypeScript makes it a pain and close to impossible to strongly type throws or advertise what something can throw and when it can throw, so some circles of people tend to reduce their usage unless it's a panic type thing or they have nothing left to do way down the stack and kind of just say, "Ugh...Guess I should just unwind with a throw or I have to re-do all the return types throughout this piece of code". I kind of do all of the above ;-) depending mostly on where I am in the code base and sometimes where my head is at, but for the validation type things which are right now returning arrays I know that we have been considering embracing more io-ts Either instead of flattening it out into an array and returning that as an Either at some point. Again, no changes this is fine as is, just some thoughts of what I have noticed. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use statusCode camel case for internals but when we return the actual status code we transform it at the last moment to be
status_codeto keep it as snake case. Will this return a camel case to the rest response or will it return snake_case? Looks like camel case.For historical perspective, not all teams return
status_codein even newer code. Some returnstatusCodeand the reason that some utilities and teams still usestatusCodeis that it is from Hapi Boom era and there's just a lot of code still using that.However, for detection rules and this repo we are trying to use the newer
status_codeand stay one step ahead if possible and on the front end a lot of times our utilities can inspect either thestatusCodeor thestatus_code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the historical reasoning is interesting. The result of this function is passed in to
siemResponse.errorwhich does the conversion so the REST response will eventually return it asstatus_code.