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] Fix incorrect changes highlighting in diff view #205138

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nikitaindik
Copy link
Contributor

@nikitaindik nikitaindik commented Dec 24, 2024

Resolves: #202016

Summary

This PR resolves an issue where the diff view incorrectly marked certain characters as changed (using bold font) in some cases.

Root Cause

The issue arises from a bug in the diff library (v5). The library is used to compute two-way diffs between strings (old field value and new field value), producing an array of change objects that is then used for rendering.

Conditions for the bug:

  • diff v5 library is in use (fixed in v6 and above) and DiffMethod.WORDS is passed as a parameter to it.
  • The old field value contains a line with an addition separated by a space (see example below).
  • The next line contains some changes (additions, deletions, or updates).

For example, for these input strings:

foo bar
spring
foo
sprint
You would expect to see this diff But you actually see this
expected actual

A more real-life example
more_real_life

Solution

Switching to DiffMethod.WORDS_WITH_SPACE avoids this issue.
Screenshot showing the difference between DiffMethod.WORDS and DiffMethod.WORDS_WITH_SPACE:
words_vs_words_with_space

Other changes

  • Removed DiffMethod.TRIMMED_LINES since it's now deprecated in the diff library and we are not using it anyways.
  • Stopped using the "zip" option since I believe it produces a less readable diff, especially for cases when there's a different number of lines in the original value vs updated value.
Screenshots: with and without "zip" (click to expand) With the "zip" option (how it was before) zip

No "zip" (this branch)
no_zip

Testing

I thoroughly tested with DiffMethod.WORDS_WITH_SPACE across various inputs and scenarios, including:

  • Single-line and multi-line strings.
  • Numbers, arrays, and objects.
  • Additions, deletions, and updates at different positions (start, middle, and end) within and across lines.

I also validated diffs against real prebuilt rules by installing an older Fleet package version and observed no issues.

You can test by trying different input strings and settings in Storybook.
Run Storybook: yarn storybook security_solution.

storybook.mov

You can notice that ComparisonSide stories are broken, but that's unrelated to these changes and needs to be handled separately.

Compatibility with future upgrades

There's an open PR that will upgrade the diff library from v5 to v7. I verified the behavior of DiffMethod.WORDS_WITH_SPACE on v7 and found no differences compared to v5, so it should be safe to upgrade to v7 without any changes on our end.

Work started on 23-Dec-2024.

@nikitaindik nikitaindik added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area backport:version Backport to applied version labels v8.18.0 labels Dec 24, 2024
@nikitaindik nikitaindik self-assigned this Dec 24, 2024
@nikitaindik nikitaindik marked this pull request as ready for review December 24, 2024 13:24
@nikitaindik nikitaindik requested a review from a team as a code owner December 24, 2024 13:24
@nikitaindik nikitaindik requested a review from xcrzx December 24, 2024 13:24
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @nikitaindik

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 Feature:Prebuilt Detection Rules Security Solution Prebuilt Detection Rules area Feature:Rule Management Security Solution Detection Rule Management area release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Diff view incorrectly highlights changes in multiline strings
2 participants