elastic / detection-rules

https://www.elastic.co/guide/en/security/current/detection-engine-overview.html
Other
1.92k stars 492 forks source link

[Bug] Handle formatting empty list #4086

Closed Mikaayenson closed 3 weeks ago

Mikaayenson commented 3 weeks ago

Pull Request

Issue link(s): Related to https://github.com/elastic/detection-rules/pull/4066 Related to https://elasticstack.slack.com/archives/C06TE19EP09/p1726588617531219?thread_ts=1726587438.668979&cid=C06TE19EP09

Summary - What I changed

How To Test

Running python -m detection_rules kibana --space main export-rules -d custom_rules for a custom rule to see the broken output.

Sample Rule After Export without Fix

```bash [metadata] creation_date = "2024/09/17" integration = [ ] maturity = "production" updated_date = "2024/09/17" [rule] actions = [ ] author = ["513814043"] description = "Test ESQL Suppress Rule" enabled = false exceptions_list = [ ] false_positives = [ ] from = "now-360s" interval = "5m" language = "esql" license = "" max_signals = 100 name = "Test ESQL Suppress Rule" references = [ ] related_integrations = [ ] required_fields = [ ] revision = 0 risk_score = 21 risk_score_mapping = [ ] rule_id = "058a8221-5b41-49ad-9e68-5a60fdf977e8" setup = "" severity = "low" severity_mapping = [ ] tags = [ ] threat = [ ] to = "now" type = "esql" version = 1 query = ''' from logs* metadata _id, _version, _index | WHERE process.name == "test" ''' [rule.alert_suppression] group_by = ["process.name"] missing_fields_strategy = "suppress" ```

Sample Rule After Export w/Fix ```bash [metadata] creation_date = "2024/09/17" integration = [] maturity = "production" updated_date = "2024/09/17" [rule] actions = [] author = ["513814043"] description = "Test ESQL Suppress Rule" enabled = false exceptions_list = [] false_positives = [] from = "now-360s" interval = "5m" language = "esql" license = "" max_signals = 100 name = "Test ESQL Suppress Rule" references = [] related_integrations = [] required_fields = [] revision = 0 risk_score = 21 risk_score_mapping = [] rule_id = "058a8221-5b41-49ad-9e68-5a60fdf977e8" setup = "" severity = "low" severity_mapping = [] tags = [] threat = [] to = "now" type = "esql" version = 1 query = ''' from logs* metadata _id, _version, _index | WHERE process.name == "test" ''' [rule.alert_suppression] group_by = ["process.name"] missing_fields_strategy = "suppress" ```

Checklist

protectionsmachine commented 3 weeks ago

Bug - Guidelines

These guidelines serve as a reminder set of considerations when addressing a bug in the code.

Documentation and Context

Code Standards and Practices

Testing

Additional Checks

brokensound77 commented 3 weeks ago

This should not be handled in the formatter and should be rolled back. This will now blindly strip all empty lists when dumping. Empty lists are valid toml structures and so should not be stripped at all. The proper way to handle this would have been to strip the object itself (the rule) of any unwanted empty lists, similar to:

https://github.com/elastic/detection-rules/blob/df31c002ca3a4f56b978433e9a7ca5ea30de7641/detection_rules/utils.py#L40-L45

or

https://github.com/elastic/detection-rules/blob/df31c002ca3a4f56b978433e9a7ca5ea30de7641/detection_rules/mixins.py#L31-L39