Skip to content

[Security Solution][Alerts] fixes merge fields with source in Detection Engine on Alerts creation#151004

Merged
vitaliidm merged 10 commits intoelastic:mainfrom
vitaliidm:alerts/fix-source-fields-merge
Mar 1, 2023
Merged

[Security Solution][Alerts] fixes merge fields with source in Detection Engine on Alerts creation#151004
vitaliidm merged 10 commits intoelastic:mainfrom
vitaliidm:alerts/fix-source-fields-merge

Conversation

@vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Feb 13, 2023

Summary

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@vitaliidm vitaliidm self-assigned this Feb 13, 2023
@vitaliidm vitaliidm added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Alerts Security Detection Alerts Area Team backport:prev-minor v8.7.0 v8.8.0 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. bug Fixes for quality problems that affect the customer experience labels Feb 13, 2023
@vitaliidm vitaliidm changed the title [Security Solution][Alerts] fixes merge missing fields with source in Detection Engine on Alerts creation [Security Solution][Alerts] fixes merge fields with source in Detection Engine on Alerts creation Feb 13, 2023
@vitaliidm vitaliidm marked this pull request as ready for review February 14, 2023 11:08
@vitaliidm vitaliidm requested review from a team as code owners February 14, 2023 11:08
@elasticmachine
Copy link
Contributor

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

# Conflicts:
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/source_fields_merging/utils/is_path_valid.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/utils/source_fields_merging/utils/is_path_valid.ts
Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another edge case that we may want to fix in the future, but it should be rare so we don't have to worry about fixing it for 8.7 at this point.

test.only('does the right stuff', () => {
        const _source: SignalSourceHit['_source'] = {
          '@timestamp': '2023-02-10T10:15:50Z',
          'process.command_line': { text: 'a' },
        };

        const fields: SignalSourceHit['fields'] = {
          'process.command_line.text': ['string longer than 10 characters'],
          '@timestamp': ['2023-02-10T10:15:50.000Z'],
        };
        const doc: SignalSourceHit = { ...emptyEsResult(), _source, fields };
        const merged = mergeMissingFieldsWithSource({ doc, ignoreFields: [] })._source;
        expect(merged).toEqual<ReturnTypeMergeFieldsWithSource>(_source);
      });

If, for some reason, source documents mix the dot and nested notations, then mergeMissingFields logic can't find the original field in the _source and still merges the value from fields into it.

…ule_types/utils/source_fields_merging/utils/is_path_valid.ts

Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @vitaliidm

@vitaliidm vitaliidm merged commit 88be889 into elastic:main Mar 1, 2023
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.7 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.7:
- [Security Solution] Re-enable a skipped url state e2e test (#152075)

Manual backport

To create the backport manually run:

node scripts/backport --pr 151004

Questions ?

Please refer to the Backport tool documentation

@vitaliidm
Copy link
Contributor Author

vitaliidm commented Mar 1, 2023

There's another edge case that we may want to fix in the future, but it should be rare so we don't have to worry about fixing it for 8.7 at this point.

Created an issue for this one #152446

@vitaliidm
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

vitaliidm added a commit to vitaliidm/kibana that referenced this pull request Mar 1, 2023
…on Engine on Alerts creation (elastic#151004)

## Summary

- fixes elastic#147389
- `mergeMissingFieldsWithSource` and `mergeAllFieldsWithSource` method
will not be merging anymore multi field values into source.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
(cherry picked from commit 88be889)

# Conflicts:
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/source_fields_merging/utils/is_path_valid.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/source_fields_merging/utils/is_path_valid.ts
vitaliidm added a commit that referenced this pull request Mar 1, 2023
…etection Engine on Alerts creation (#151004) (#152449)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution][Alerts] fixes merge fields with source in
Detection Engine on Alerts creation
(#151004)](#151004)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Vitalii
Dmyterko","email":"92328789+vitaliidm@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-03-01T11:29:53Z","message":"[Security
Solution][Alerts] fixes merge fields with source in Detection Engine on
Alerts creation (#151004)\n\n## Summary\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/147389\r\n-
`mergeMissingFieldsWithSource` and `mergeAllFieldsWithSource`
method\r\nwill not be merging anymore multi field values into
source.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Marshall Main
<55718608+marshallmain@users.noreply.github.com>","sha":"88be889e1c5bfd97943eaf5eef6c7002433e2169","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","Team:
SecuritySolution","Team:Detection
Alerts","backport:prev-minor","v8.7.0","v8.8.0"],"number":151004,"url":"https://github.com/elastic/kibana/pull/151004","mergeCommit":{"message":"[Security
Solution][Alerts] fixes merge fields with source in Detection Engine on
Alerts creation (#151004)\n\n## Summary\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/147389\r\n-
`mergeMissingFieldsWithSource` and `mergeAllFieldsWithSource`
method\r\nwill not be merging anymore multi field values into
source.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Marshall Main
<55718608+marshallmain@users.noreply.github.com>","sha":"88be889e1c5bfd97943eaf5eef6c7002433e2169"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/151004","number":151004,"mergeCommit":{"message":"[Security
Solution][Alerts] fixes merge fields with source in Detection Engine on
Alerts creation (#151004)\n\n## Summary\r\n\r\n- fixes
https://github.com/elastic/kibana/issues/147389\r\n-
`mergeMissingFieldsWithSource` and `mergeAllFieldsWithSource`
method\r\nwill not be merging anymore multi field values into
source.\r\n\r\n### Checklist\r\n\r\nDelete any items that are not
applicable to this PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Marshall Main
<55718608+marshallmain@users.noreply.github.com>","sha":"88be889e1c5bfd97943eaf5eef6c7002433e2169"}}]}]
BACKPORT-->
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
…on Engine on Alerts creation (elastic#151004)

## Summary

- fixes elastic#147389
- `mergeMissingFieldsWithSource` and `mergeAllFieldsWithSource` method
will not be merging anymore multi field values into source.

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
@vitaliidm vitaliidm deleted the alerts/fix-source-fields-merge branch March 4, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Fixes for quality problems that affect the customer experience impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.7.0 v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security Solution][Alerts] Merge _source and fields correctly in alert creation logic

5 participants