[Security Solution] is_customized Flag is Set to True When Reverting Changes to Prebuilt Rules

[pborgonovi]

Describe the bug:

If a user adds a new value to a previously empty field, saves the rule, and then removes the value to return it to its original state, the rule correctly reverts to is_customized: false. However, if a user modifies an existing value and then reverts the change, the rule remains marked as “Customized.”

Kibana/Elasticsearch Stack version:

8.x

Functional Area (e.g. Endpoint management, timelines, resolver, etc.):

Prebuilt Rules

Pre requisites:

1. prebuiltRulesCustomizationEnabled Feature Flag is enabled
2. Prebuilt rules are available

Steps to reproduce:

1. Navigate to the Rules Management page and locate a prebuilt rule.
2. Edit the rule and modify an existing value (e.g., query).
3. Save the changes.

• Observe that the rule is marked as Customized.

4. Edit the rule again and undo the change, returning the field to its original value.
5. Repeat the process, but this time:

• Add a new value to a previously empty field (e.g., add a new tag, new integration).
• Save the rule (it should be marked as Customized).
• Remove the value, returning the rule to its original state.
• Save the rule again.

Current behavior:

• Scenario 1: When modifying and reverting an existing value:
  The rule remains marked as Customized, even though the change is undone, and the rule matches its original state.

• Scenario 2: When adding and removing a new value:
  The rule is correctly reverted to not Customized (is_customized: false) after the value is removed.

Expected behavior:

In both scenarios, when a user undoes all changes to a prebuilt rule, the rule should return to its original state and be marked as not Customized (is_customized: false).

Screenshots (if relevant):

Modifying an existing value:

Screen.Recording.2024-12-05.at.9.05.26.AM.mov

Adding new value:

Screen.Recording.2024-12-05.at.9.07.53.AM.mov

[elasticmachine]

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

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[elasticmachine]

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

[banderror]

@pborgonovi Very interesting! Thank you for catching this one 👍

@dplumlee Could you please look into this? I'm not sure if it's the same as #201631 and #202575 or a different one.

[dplumlee]

This is happening because the original query field for the "Container Workload Protection" rule has a newline character at the end of the string and when we paste the original value back in it doesn't have it. (You can even see the bit of new line in the video above when the query bar is clicked on originally)

event.kind:alert and event.module:cloud_defend\n

vs

event.kind:alert and event.module:cloud_defend

Technically this means the diff algorithm is correctly marking it as modified but I do agree this is weird from the UI point of view, especially since you can't just add back a new line character from the rule edit form.

I think there's an easy way to solve this that wouldn't be too invasive with the existing diff algorithm where we "trim" the strings before comparison. So any whitespace before or after non-whitespace characters wouldn't be included in the comparison but any whitespace characters in the query itself would still be diffed upon.

For instance:

\n\nevent.kind:alert and event.module:cloud_defend\n\n

and

event.kind:alert and event.module:cloud_defend\n

would show as equal to one another.

But

event.kind:alert and\n event.module:cloud_defend

and

event.kind:alert and event.module:cloud_defend

would be unequal. Just so formatting is still addressed in the algorithms.

To address what I think would be the obvious follow-up question of why not remove all whitespace characters, we could probably do that as well, I'm just not sure if that would start to interfere with the queries themselves (do we have whitespace characters in the query for substantive reasons) and what it would take to write a regex against those cases. Plus, if we do end up adding the logic for multi-line query diffs, it might conflict with that.

@banderror @xcrzx @jpdjere any thoughts on this approach?

[xcrzx]

Trimming strings before comparing them seems fine to me, but removing newlines within a multiline string is more problematic. We'd need to ensure that the newlines don't carry any semantic meaning, or handle escaped newlines, etc. There could be edge cases we're not aware of. So, I lean towards the simpler approach of trimming only the start and end of a string.

Also, a multiline merge wouldn’t work if we were to remove newlines.

[banderror]

I lean towards the simpler approach of trimming only the start and end of a string.

Yep, this seems to be the correct solution 👍

@dplumlee @xcrzx Where do we do this?

[dplumlee]

@banderror this would be done either in the diff algorithm itself or the normalization we do for fields prior to the diff algorithm

[banderror]

the normalization we do for fields prior to the diff algorithm

@dplumlee I think this is the right option. I think diff algorithms should have a single responsibility.

Also, probably out of the scope of this issue, but something we could consider in the future. Maybe some normalization like trimming strings etc should be done even before that - every time we create or update a rule in Elasticsearch.

[xcrzx]

Maybe some normalization like trimming strings etc should be done even before that - every time we create or update a rule in Elasticsearch.

++ It's a common practice embedding some simple data normalization in request schemaa, like using z.string().trim()

[pborgonovi]

Hey @banderror @dplumlee @xcrzx

I just tested changing data source and related integrations fields values and observed that:

Reverting data source value causes the rule to return to its non-customized state:

Screen.Recording.2024-12-06.at.9.30.00.AM.mov

Reverting related integrations causes the rule to be kept as customized:

Screen.Recording.2024-12-06.at.9.31.41.AM.mov

Would it also have same newline character root cause?

Interesting thing is that, as 203011 and 202613, this issue happens ONLY if the rule has an available update. Check this:

There is no update available: reverting query value sets is_customized=false

Screen.Recording.2024-12-06.at.9.44.05.AM.mp4

There is available update: reverting query value keeps is_customized=true

Screen.Recording.2024-12-06.at.9.53.40.AM.mov

[dplumlee]

@pborgonovi it looks like the version field is different (^8.17.0 vs ^8.2.0) when you revert to the original related integration, I suspect that's the reason it's still modified

[pborgonovi]

@dplumlee thanks for noticing! That's the situation with related integrations. When reverted version to 8.2.0 the rule is set back to non-customized correctly.

So let's just consider my last observation from previous comment:

Interesting thing is that, as #203011 (comment) and #202613 (comment), this issue happens ONLY if the rule has an available update. Check this:

There is no update available: reverting query value sets is_customized=false

There is available update: reverting query value keeps is_customized=true

[dplumlee]

@pborgonovi the original issue for this bug should be fixed by #203482, the missing base version I would consider it's own error at this point

[pborgonovi]

Retested with latest changes and fix looks good.

Screen.Recording.2024-12-13.at.9.03.22.AM.mov