[Security Solution] Validate exception list size when adding new items #73399
Conversation
marshallmain
added
release_note:skip
| }: 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 null return value as an error case - savedObjectsClient.delete should either succeed or throw
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:
https://gcanti.github.io/fp-ts/modules/Either.ts.html
(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.
| 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, |
There was a problem hiding this comment.
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_code to 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_code in even newer code. Some return statusCode and the reason that some utilities and teams still use statusCode is 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_code and stay one step ahead if possible and on the front end a lot of times our utilities can inspect either the statusCode or the status_code.
There was a problem hiding this comment.
Thanks, the historical reasoning is interesting. The result of this function is passed in to siemResponse.error which does the conversion so the REST response will eventually return it as status_code.
|
jenkins test this |
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
elastic#73399) * Validate exception list size when adding new items * Update comment * Extract list size validation and apply to endpoint route also Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
elastic#73399) * Validate exception list size when adding new items * Update comment * Extract list size validation and apply to endpoint route also Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Adds API validation preventing users from adding more than 10k exception items per exception list.
We optimistically add the exception item before checking the list size to mitigate a potential race with multiple users. If we only check the list size before adding the item, 2 simultaneous exception item creations could cause the list to exceed the limit without raising any errors. Checking the size of the list after adding the item causes simultaneous creations near the limit to fail and the list will remain under the limit (barring a service failure that prevents us from deleting items).
Checklist
Delete any items that are not applicable to this PR.
For maintainers