Skip to content
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 Solution] Fixes exception item comment validation on newline chars \n #202063

Merged
merged 19 commits into from
Dec 10, 2024

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Nov 27, 2024

Summary

Fixes: #201820

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Unit or functional tests were updated or added to match the most common scenarios
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@dhurley14 dhurley14 self-assigned this Dec 4, 2024
@dhurley14 dhurley14 added v9.0.0 v8.17.0 v8.18.0 v8.16.2 v8.17.1 release_note:fix Team:Detections and Resp Security Detection Response Team bug Fixes for quality problems that affect the customer experience review Feature:Rule Exceptions Security Solution Detection Rule Exceptions area and removed v8.17.0 labels Dec 4, 2024
@nicpenning
Copy link

👀

@dhurley14 dhurley14 added the backport:version Backport to applied version labels label Dec 4, 2024
@dhurley14 dhurley14 marked this pull request as ready for review December 4, 2024 12:46
@dhurley14 dhurley14 requested review from a team as code owners December 4, 2024 12:46
@elasticmachine
Copy link
Contributor

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

@dhurley14 dhurley14 requested review from maximpn and xcrzx December 9, 2024 17:35
Copy link
Contributor

@janmonschke janmonschke left a comment

Choose a reason for hiding this comment

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

Threat Hunting changes lgtm

@dhurley14
Copy link
Contributor Author

/ci

@dhurley14
Copy link
Contributor Author

CI failing due to this test: #203477

Copy link
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@dhurley14 Thanks for addressing the comments 🙏

It looks good to go. @xcrzx do you have any objections?

Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
integrationAssistant 472 473 +1
inventory 256 257 +1
securitySolution 6348 6349 +1
total +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/zod-helpers 10 13 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 14.7MB 14.7MB +1.1KB
Unknown metric groups

API count

id before after diff
@kbn/zod-helpers 20 23 +3

History

cc @dhurley14

@pmuellr
Copy link
Member

pmuellr commented Dec 10, 2024

This appears to be a change to some SIEM rule type parameters, where the schema is now a little more lenient? Basically, the schema bits "pattern": "^(?! *$).+$" have been removed from some parameter properties.

In terms of serverless BWC/ZDT, this seems "safe" from an upgrade POV (making this less lenient can be a problem). But roll back? What happens if this updated is deployed for some amount of time, and then we determine we need to roll back to the previous code. I'm guessing they would see the same errors they saw before, which is obviously fine.

Thought I'd double-check those assumptions before approving.

@dhurley14
Copy link
Contributor Author

@pmuellr great questions!

This appears to be a change to some SIEM rule type parameters, where the schema is now a little more lenient?

Yes we have a better system in place for ensuring strings passed to certain parameters are not empty strings and do not fail validation when newline chars are included. This same regex was used in places where free-text fields were not used, however the team felt it better to replace all of the instances of the regex with this new trim function. Which I suppose could be considered more lenient!

What happens if this updated is deployed for some amount of time, and then we determine we need to roll back to the previous code

Great question - If these changes are deployed and then subsequently rolled back, we would enter into a similar situation as an upgrade from 8.15 to 8.16 where customers who had previously added newline chars would have errors in the UI, specifically in the exceptions UI.

@pmuellr
Copy link
Member

pmuellr commented Dec 10, 2024

What happens if this updated is deployed for some amount of time, and then we determine we need to roll back to the previous code

If these changes are deployed and then subsequently rolled back, we would enter into a similar situation as an upgrade from 8.15 to 8.16 where customers who had previously added newline chars would have errors in the UI, specifically in the exceptions UI.

Ya, so if you go back to the buggy release, you'll see the bugs. Hard to imagine that's fixable, really, even with an "intermediate" release - or it would not be easy (probably involving some backward-compatible migration). Seems fine to me.

thx!

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM, based on comments posted.

@dhurley14 dhurley14 merged commit 35aeac1 into elastic:main Dec 10, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12265712482

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.16 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.16:
- [Security Assistant] Fix animation border color of security AI assistant (#202319)
- [Infra] Fix call to service api (#203451)
8.17 Backport failed because of merge conflicts
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- remove redux tools from presentation_utils plugin (#197891)
- [Security Solution][Expandable flyout] Introducing Flyout history in document flyout (#184970)
- Support incoming Preconfigured Endpoints (#203473)
- [dashboard] remove legacy embeddable client migrations (#203669)
- Upgrade express 4.21.1→ 4.21.2 (#203504)
- [Infra][ObsUX] Hosts & Container Logs only overview (#202992)
- [SecuritySolution] Fix serviceEntityStoreEnabled experimental flag initial value (#203573)
- [Security Assistant] Fix animation border color of security AI assistant (#202319)

Manual backport

To create the backport manually run:

node scripts/backport --pr 202063

Questions ?

Please refer to the Backport tool documentation

dhurley14 added a commit to dhurley14/kibana that referenced this pull request Dec 10, 2024
…e chars `\n` (elastic#202063)

## Summary

Fixes: elastic#201820

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 35aeac1)

# Conflicts:
#	oas_docs/output/kibana.serverless.yaml
dhurley14 added a commit to dhurley14/kibana that referenced this pull request Dec 10, 2024
…e chars `\n` (elastic#202063)

## Summary

Fixes: elastic#201820

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 35aeac1)

# Conflicts:
#	oas_docs/output/kibana.serverless.yaml
#	x-pack/plugins/security_solution/common/siem_migrations/model/common.schema.yaml
@dhurley14
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x
8.17
8.16

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

Questions ?

Please refer to the Backport tool documentation

dhurley14 added a commit to dhurley14/kibana that referenced this pull request Dec 10, 2024
…e chars `\n` (elastic#202063)

## Summary

Fixes: elastic#201820

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 35aeac1)

# Conflicts:
#	oas_docs/output/kibana.serverless.yaml
#	x-pack/plugins/security_solution/common/siem_migrations/model/common.gen.ts
#	x-pack/plugins/security_solution/common/siem_migrations/model/common.schema.yaml
dhurley14 added a commit that referenced this pull request Dec 11, 2024
… newline chars `\n` (#202063) (#203709)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Fixes exception item comment validation on
newline chars `\n`
(#202063)](#202063)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Devin W.
Hurley","email":"devin.hurley@elastic.co"},"sourceCommit":{"committedDate":"2024-12-10T22:19:32Z","message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","review","release_note:fix","v9.0.0","Team:Detections
and Resp","Feature:Rule
Exceptions","backport:version","v8.18.0","v8.16.2","v8.17.1"],"number":202063,"url":"https://github.com/elastic/kibana/pull/202063","mergeCommit":{"message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202063","number":202063,"mergeCommit":{"message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
dhurley14 added a commit that referenced this pull request Dec 11, 2024
…newline chars `\n` (#202063) (#203707)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Fixes exception item comment validation on
newline chars `\n`
(#202063)](#202063)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Devin W.
Hurley","email":"devin.hurley@elastic.co"},"sourceCommit":{"committedDate":"2024-12-10T22:19:32Z","message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","review","release_note:fix","v9.0.0","Team:Detections
and Resp","Feature:Rule
Exceptions","backport:version","v8.18.0","v8.16.2","v8.17.1"],"number":202063,"url":"https://github.com/elastic/kibana/pull/202063","mergeCommit":{"message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.16","8.17"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202063","number":202063,"mergeCommit":{"message":"[Security
Solution] Fixes exception item comment validation on newline chars `\\n`
(#202063)\n\n## Summary\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/201820\r\n\r\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"35aeac104359eae81a233d0b8a9acaa97119d006"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.16","label":"v8.16.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.17","label":"v8.17.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
…e chars `\n` (elastic#202063)

## Summary

Fixes: elastic#201820

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience Feature:Rule Exceptions Security Solution Detection Rule Exceptions area release_note:fix review Team:Detections and Resp Security Detection Response Team v8.16.2 v8.17.1 v8.18.0 v9.0.0
Projects
None yet