Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[8.x] [Security Solution] Fix incorrect changes highlighting in diff …
…view (#205138) (#205253) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Fix incorrect changes highlighting in diff view (#205138)](#205138) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Nikita Indik","email":"nikita.indik@elastic.co"},"sourceCommit":{"committedDate":"2024-12-30T12:38:42Z","message":"[Security Solution] Fix incorrect changes highlighting in diff view (#205138)\n\n**Resolves: https://github.com/elastic/kibana/issues/202016**\n\n## Summary\n\nThis PR resolves an issue where the diff view incorrectly marked certain\ncharacters as changed (using bold font) in some cases.\n\n## Root Cause\nThe issue arises from a bug in the `diff` library (v5). The library is\nused to compute two-way diffs between strings (old field value and new\nfield value), producing an array of change objects that is then used for\nrendering.\n\nConditions for the bug:\n- `diff` v5 library is in use (fixed in v6 and above) and\n`DiffMethod.WORDS` is passed as a parameter to it.\n- The old field value contains a line with an addition separated by a\nspace (see example below).\n- The next line contains some changes (additions, deletions, or\nupdates).\n\n\nFor example, for these input strings:\n```\nfoo bar\nspring\n```\n```\nfoo\nsprint\n```\n\n| You would expect to see this diff | But you actually see this |\n|----------|----------|\n| <img width=\"119\" alt=\"expected\"\nsrc=\"https://github.com/user-attachments/assets/c41b3dec-e578-4a12-8eb8-91fbb60d7247\"\n/> | <img width=\"118\" alt=\"actual\"\nsrc=\"https://github.com/user-attachments/assets/f2a33fee-5de2-4291-876a-e7575ea07079\"\n/> |\n\n**A more real-life example**\n<img width=\"1661\" alt=\"more_real_life\"\nsrc=\"https://github.com/user-attachments/assets/91ebfe93-81ad-45c8-8f9b-e173c2cf940b\"\n/>\n\n\n## Solution\nSwitching to `DiffMethod.WORDS_WITH_SPACE` avoids this issue. \nScreenshot showing the difference between `DiffMethod.WORDS` and\n`DiffMethod.WORDS_WITH_SPACE`:\n<img width=\"675\" alt=\"words_vs_words_with_space\"\nsrc=\"https://github.com/user-attachments/assets/3c91e1d2-63fc-4fcd-a762-a905878bfc3a\"\n/>\n\n## Other changes\n- Removed `DiffMethod.TRIMMED_LINES` since it's now\n[deprecated](kpdecker/jsdiff#486) in the `diff`\nlibrary and we are not using it anyways.\n- Stopped using the \"zip\" option since I believe it produces a less\nreadable diff, especially for cases when there's a different number of\nlines in the original value vs updated value.\n\n<details>\n<summary><strong>Screenshots: with and without \"zip\" (click to\nexpand)</strong></summary>\n<strong>With the \"zip\" option (how it was before)</strong>\n<img width=\"1918\" alt=\"zip\"\nsrc=\"https://github.com/user-attachments/assets/272ed849-47d6-4fef-8acc-ab1b22c9f42e\"\n/>\n\n<strong>No \"zip\" (this branch)</strong>\n<img width=\"1919\" alt=\"no_zip\"\nsrc=\"https://github.com/user-attachments/assets/417303bf-9570-4ee1-98c5-8a78f59c7956\"\n/>\n</details>\n\n## Testing\n\nI thoroughly tested with `DiffMethod.WORDS_WITH_SPACE` across various\ninputs and scenarios, including:\n- Single-line and multi-line strings.\n- Numbers, arrays, and objects.\n- Additions, deletions, and updates at different positions (start,\nmiddle, and end) within and across lines.\n\nI also validated diffs against real prebuilt rules by installing an\nolder Fleet package version and observed no issues.\n\nYou can test by trying different input strings and settings in\nStorybook.\n**Run Storybook**: `yarn storybook security_solution`.\n\n\nhttps://github.com/user-attachments/assets/0440b73c-a4d7-40cf-9cee-e632146d292e\n\nYou can notice that `ComparisonSide` stories are broken, but that's\nunrelated to these changes and needs to be handled separately.\n\n## Compatibility with future upgrades\n\nThere's an open [PR](#202622) that\nwill upgrade the `diff` library from v5 to v7. I verified the behavior\nof `DiffMethod.WORDS_WITH_SPACE` on v7 and found no differences compared\nto v5, so it should be safe to upgrade to v7 without any changes on our\nend.\n\nWork started on 23-Dec-2024.","sha":"140c2e0ecf9f8a0277699052f9ba472066a0e96d","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections and Resp","Team: SecuritySolution","Feature:Rule Management","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","backport:version","v8.18.0"],"title":"[Security Solution] Fix incorrect changes highlighting in diff view","number":205138,"url":"https://github.com/elastic/kibana/pull/205138","mergeCommit":{"message":"[Security Solution] Fix incorrect changes highlighting in diff view (#205138)\n\n**Resolves: https://github.com/elastic/kibana/issues/202016**\n\n## Summary\n\nThis PR resolves an issue where the diff view incorrectly marked certain\ncharacters as changed (using bold font) in some cases.\n\n## Root Cause\nThe issue arises from a bug in the `diff` library (v5). The library is\nused to compute two-way diffs between strings (old field value and new\nfield value), producing an array of change objects that is then used for\nrendering.\n\nConditions for the bug:\n- `diff` v5 library is in use (fixed in v6 and above) and\n`DiffMethod.WORDS` is passed as a parameter to it.\n- The old field value contains a line with an addition separated by a\nspace (see example below).\n- The next line contains some changes (additions, deletions, or\nupdates).\n\n\nFor example, for these input strings:\n```\nfoo bar\nspring\n```\n```\nfoo\nsprint\n```\n\n| You would expect to see this diff | But you actually see this |\n|----------|----------|\n| <img width=\"119\" alt=\"expected\"\nsrc=\"https://github.com/user-attachments/assets/c41b3dec-e578-4a12-8eb8-91fbb60d7247\"\n/> | <img width=\"118\" alt=\"actual\"\nsrc=\"https://github.com/user-attachments/assets/f2a33fee-5de2-4291-876a-e7575ea07079\"\n/> |\n\n**A more real-life example**\n<img width=\"1661\" alt=\"more_real_life\"\nsrc=\"https://github.com/user-attachments/assets/91ebfe93-81ad-45c8-8f9b-e173c2cf940b\"\n/>\n\n\n## Solution\nSwitching to `DiffMethod.WORDS_WITH_SPACE` avoids this issue. \nScreenshot showing the difference between `DiffMethod.WORDS` and\n`DiffMethod.WORDS_WITH_SPACE`:\n<img width=\"675\" alt=\"words_vs_words_with_space\"\nsrc=\"https://github.com/user-attachments/assets/3c91e1d2-63fc-4fcd-a762-a905878bfc3a\"\n/>\n\n## Other changes\n- Removed `DiffMethod.TRIMMED_LINES` since it's now\n[deprecated](kpdecker/jsdiff#486) in the `diff`\nlibrary and we are not using it anyways.\n- Stopped using the \"zip\" option since I believe it produces a less\nreadable diff, especially for cases when there's a different number of\nlines in the original value vs updated value.\n\n<details>\n<summary><strong>Screenshots: with and without \"zip\" (click to\nexpand)</strong></summary>\n<strong>With the \"zip\" option (how it was before)</strong>\n<img width=\"1918\" alt=\"zip\"\nsrc=\"https://github.com/user-attachments/assets/272ed849-47d6-4fef-8acc-ab1b22c9f42e\"\n/>\n\n<strong>No \"zip\" (this branch)</strong>\n<img width=\"1919\" alt=\"no_zip\"\nsrc=\"https://github.com/user-attachments/assets/417303bf-9570-4ee1-98c5-8a78f59c7956\"\n/>\n</details>\n\n## Testing\n\nI thoroughly tested with `DiffMethod.WORDS_WITH_SPACE` across various\ninputs and scenarios, including:\n- Single-line and multi-line strings.\n- Numbers, arrays, and objects.\n- Additions, deletions, and updates at different positions (start,\nmiddle, and end) within and across lines.\n\nI also validated diffs against real prebuilt rules by installing an\nolder Fleet package version and observed no issues.\n\nYou can test by trying different input strings and settings in\nStorybook.\n**Run Storybook**: `yarn storybook security_solution`.\n\n\nhttps://github.com/user-attachments/assets/0440b73c-a4d7-40cf-9cee-e632146d292e\n\nYou can notice that `ComparisonSide` stories are broken, but that's\nunrelated to these changes and needs to be handled separately.\n\n## Compatibility with future upgrades\n\nThere's an open [PR](#202622) that\nwill upgrade the `diff` library from v5 to v7. I verified the behavior\nof `DiffMethod.WORDS_WITH_SPACE` on v7 and found no differences compared\nto v5, so it should be safe to upgrade to v7 without any changes on our\nend.\n\nWork started on 23-Dec-2024.","sha":"140c2e0ecf9f8a0277699052f9ba472066a0e96d"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/205138","number":205138,"mergeCommit":{"message":"[Security Solution] Fix incorrect changes highlighting in diff view (#205138)\n\n**Resolves: https://github.com/elastic/kibana/issues/202016**\n\n## Summary\n\nThis PR resolves an issue where the diff view incorrectly marked certain\ncharacters as changed (using bold font) in some cases.\n\n## Root Cause\nThe issue arises from a bug in the `diff` library (v5). The library is\nused to compute two-way diffs between strings (old field value and new\nfield value), producing an array of change objects that is then used for\nrendering.\n\nConditions for the bug:\n- `diff` v5 library is in use (fixed in v6 and above) and\n`DiffMethod.WORDS` is passed as a parameter to it.\n- The old field value contains a line with an addition separated by a\nspace (see example below).\n- The next line contains some changes (additions, deletions, or\nupdates).\n\n\nFor example, for these input strings:\n```\nfoo bar\nspring\n```\n```\nfoo\nsprint\n```\n\n| You would expect to see this diff | But you actually see this |\n|----------|----------|\n| <img width=\"119\" alt=\"expected\"\nsrc=\"https://github.com/user-attachments/assets/c41b3dec-e578-4a12-8eb8-91fbb60d7247\"\n/> | <img width=\"118\" alt=\"actual\"\nsrc=\"https://github.com/user-attachments/assets/f2a33fee-5de2-4291-876a-e7575ea07079\"\n/> |\n\n**A more real-life example**\n<img width=\"1661\" alt=\"more_real_life\"\nsrc=\"https://github.com/user-attachments/assets/91ebfe93-81ad-45c8-8f9b-e173c2cf940b\"\n/>\n\n\n## Solution\nSwitching to `DiffMethod.WORDS_WITH_SPACE` avoids this issue. \nScreenshot showing the difference between `DiffMethod.WORDS` and\n`DiffMethod.WORDS_WITH_SPACE`:\n<img width=\"675\" alt=\"words_vs_words_with_space\"\nsrc=\"https://github.com/user-attachments/assets/3c91e1d2-63fc-4fcd-a762-a905878bfc3a\"\n/>\n\n## Other changes\n- Removed `DiffMethod.TRIMMED_LINES` since it's now\n[deprecated](kpdecker/jsdiff#486) in the `diff`\nlibrary and we are not using it anyways.\n- Stopped using the \"zip\" option since I believe it produces a less\nreadable diff, especially for cases when there's a different number of\nlines in the original value vs updated value.\n\n<details>\n<summary><strong>Screenshots: with and without \"zip\" (click to\nexpand)</strong></summary>\n<strong>With the \"zip\" option (how it was before)</strong>\n<img width=\"1918\" alt=\"zip\"\nsrc=\"https://github.com/user-attachments/assets/272ed849-47d6-4fef-8acc-ab1b22c9f42e\"\n/>\n\n<strong>No \"zip\" (this branch)</strong>\n<img width=\"1919\" alt=\"no_zip\"\nsrc=\"https://github.com/user-attachments/assets/417303bf-9570-4ee1-98c5-8a78f59c7956\"\n/>\n</details>\n\n## Testing\n\nI thoroughly tested with `DiffMethod.WORDS_WITH_SPACE` across various\ninputs and scenarios, including:\n- Single-line and multi-line strings.\n- Numbers, arrays, and objects.\n- Additions, deletions, and updates at different positions (start,\nmiddle, and end) within and across lines.\n\nI also validated diffs against real prebuilt rules by installing an\nolder Fleet package version and observed no issues.\n\nYou can test by trying different input strings and settings in\nStorybook.\n**Run Storybook**: `yarn storybook security_solution`.\n\n\nhttps://github.com/user-attachments/assets/0440b73c-a4d7-40cf-9cee-e632146d292e\n\nYou can notice that `ComparisonSide` stories are broken, but that's\nunrelated to these changes and needs to be handled separately.\n\n## Compatibility with future upgrades\n\nThere's an open [PR](#202622) that\nwill upgrade the `diff` library from v5 to v7. I verified the behavior\nof `DiffMethod.WORDS_WITH_SPACE` on v7 and found no differences compared\nto v5, so it should be safe to upgrade to v7 without any changes on our\nend.\n\nWork started on 23-Dec-2024.","sha":"140c2e0ecf9f8a0277699052f9ba472066a0e96d"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Nikita Indik <nikita.indik@elastic.co>
- Loading branch information