Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Adds pre-packaged rule updates through the "Prebuilt Security Detection Rules" Fleet integration #96698
[Security] Adds pre-packaged rule updates through the "Prebuilt Security Detection Rules" Fleet integration #96698
Changes from 5 commits
850856d
38ba797
93e3159
4a855ee
a1febb9
dc48e1a
1e56114
3df98ad
84248b3
41536f0
df517f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think you're shoe horning a bit here and this isn't the
validateAllPrepackagedRules
as it is written is really what you want maybe.The
ts-expect-error
kind of gives it away as a bit as maybe something doesn't seem right.If
validateAllPrepackagedRules
throws if the rule is not validated and was written when rules on a file system are a thing. Is that what you want? You want a throw here if it is not valid? Also it will say in the error message:I don't know if that's what you want either. The best bet would be to do something like this where you add a new function that is specific to checking and validating and throwing a specific error:
Then you can remove the
ts-expect-error
but also look at that bit of weirdness where I have two arrays?I don't think even that is 100% but it's closer to your intent of validating and then casting the data from a SO key/value generic data structure into a more non-generic data structure which has guaranteed fields such as the io-ts ones. I think maybe there just needs to be a loop over the inner array or something else is possibly off, but it does show at least progress here to where restructuring with a new function would be better.
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.
yeah, fair enough to make a separate function to validate here, so we don't assume any more than absolutely necessary about the incoming dynamically typed data.
I definitely need to do this check, I just mistakenly assumed that the
validateAllPrepackagedRules
would also be appropriate here.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.
41536f0
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.
fwiw,
@ts-expect-error
count outside of test files is increasing per release when it should be be decreasing or staying steady or rising only slightly.Maintainers, operators, and general outsiders from security_solutions who are cutting across our solution pepper them in and then later I think that creates the situation where it feels like the norm to whip them out and use them as if it is a normal typescript thing when in reality they're considered a code smell and something that should be fixed soon if possible.
Granted in a few situations where typescript is very wrong or we have to do an import of JS it is something we cannot avoid. However, we should avoid them altogether even if typescript is slightly wrong but we can work around typescript.
Can we can this code for example to be this:
Then that removes the
ts-expect-error
.For example, a maintainer can now change that code to have accidental character addition like this below and if it passes the unit tests it will end up shipping broken.
Even in tests I try to avoid them if I can. My scale of good to bad goes like this:
?
or another technique but failed.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 for the explanation, that's good to understand more of your thought process and approach!
i am definitely not a fan of
ts-expect-error
style suppression for a whole line either. especially since a future error could be introduced and it's already suppressed. that's not good.in this case, it seems the typesystem was unable to infer how
has
andget
relate to each other, so even though what i did looks safe it guessed wrong.storing
.get()
in a variable seems like just the right solution. you can explicitly check that againstnull
, and then the other check it knows is non-null. i'm trying that change out locally now and running through the type checkingThere 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.
Is
== null
or=== undefined
more idiomatic in this case?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.
updated in 3df98ad (for
== null
)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.
bummer the
.find
doesn't take a template input and forces you to do thatso as IRuleAssetSavedObject
below.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.
If this total is zero, won't this exit the loop automatically? Just curious, I have used this newer API before. I couldn't tell from the docs.
In other parts of the code base for example I see:
Something like that where they're not looking for totals and just looping until done and then doing a close.
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.
good question. i couldn't tell either. any guess who we should ping to double check?