From 140c2e0ecf9f8a0277699052f9ba472066a0e96d Mon Sep 17 00:00:00 2001 From: Nikita Indik Date: Mon, 30 Dec 2024 13:38:42 +0100 Subject: [PATCH] [Security Solution] Fix incorrect changes highlighting in diff view (#205138) **Resolves: https://github.com/elastic/kibana/issues/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](https://github.com/kpdecker/jsdiff/pull/486) 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`. https://github.com/user-attachments/assets/0440b73c-a4d7-40cf-9cee-e632146d292e 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](https://github.com/elastic/kibana/pull/202622) 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. --- .../json_diff/diff_view.stories.tsx | 60 +++++++++++++++++++ .../rule_details/json_diff/diff_view.tsx | 23 ++++--- .../rule_details/json_diff/mark_edits.tsx | 3 +- 3 files changed, 77 insertions(+), 9 deletions(-) create mode 100644 x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.stories.tsx diff --git a/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.stories.tsx b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.stories.tsx new file mode 100644 index 0000000000000..2e2e2dbb0895b --- /dev/null +++ b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.stories.tsx @@ -0,0 +1,60 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import React from 'react'; +import type { Story } from '@storybook/react'; +import type { DiffViewProps } from './diff_view'; +import { DiffView } from './diff_view'; +import { DiffMethod } from './mark_edits'; + +export default { + component: DiffView, + title: 'Rule Management/Prebuilt Rules/Upgrade Flyout/ThreeWayDiff/DiffView', + argTypes: { + oldSource: { + control: 'text', + }, + newSource: { + control: 'text', + }, + diffMethod: { + control: 'select', + options: [ + DiffMethod.WORDS_WITH_SPACE, + DiffMethod.WORDS, + DiffMethod.CHARS, + DiffMethod.LINES, + DiffMethod.SENTENCES, + ], + defaultValue: DiffMethod.WORDS_WITH_SPACE, + }, + zip: { + control: 'boolean', + defaultValue: false, + }, + }, +}; + +const Template: Story = ({ oldSource, newSource, diffMethod, zip }) => { + return ( + + ); +}; + +export const Default = Template.bind({}); +Default.args = { + oldSource: + 'from logs-endpoint.alerts-*\n| where event.code in ("malicious_file", "memory_signature", "shellcode_thread") and rule.name is not null\n| keep host.id, rule.name, event.code\n| stats hosts = count_distinct(host.id) by rule.name, event.code\n| where hosts >= 3', + newSource: + 'from logs-endpoint.alerts-*\n| where event.code in ("malicious_file", "memory_signature", "shellcode_thread")\n| stats hosts = count_distinct(host.id)\n| where hosts >= 3', +}; diff --git a/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx index ba3195edab73a..9b10fb00b12eb 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/diff_view.tsx @@ -25,7 +25,6 @@ import type { HunkTokens, } from 'react-diff-view'; import unidiff from 'unidiff'; -import type { Change } from 'diff'; import { useEuiTheme, COLOR_MODES_STANDARD } from '@elastic/eui'; import { Hunks } from './hunks'; import { markEdits, DiffMethod } from './mark_edits'; @@ -89,7 +88,7 @@ const useTokens = ( try { /* - Synchroniously apply all the enhancers to the hunks and return an array of tokens. + Synchronously apply all the enhancers to the hunks and return an array of tokens. */ return tokenize(hunks, options); } catch (ex) { @@ -128,7 +127,7 @@ const convertChangesToUnifiedDiffString = (changes: Change[]): string => { return unifiedDiff; }; -const convertToDiffFile = (oldSource: string, newSource: string) => { +const convertToDiffFile = (oldSource: string, newSource: string, zip?: boolean) => { /* "diffLines" call converts two strings of text into an array of Change objects. */ @@ -156,7 +155,7 @@ const convertToDiffFile = (oldSource: string, newSource: string) => { Hunks represent changed lines of code plus a few unchanged lines above and below for context. */ const [diffFile] = parseDiff(unifiedDiff, { - nearbySequences: 'zip', + nearbySequences: zip ? 'zip' : undefined, }); return diffFile; @@ -255,18 +254,25 @@ const CustomStyles: FC> = ({ children }) => { ); }; -interface DiffViewProps extends Partial { +export interface DiffViewProps extends Partial { oldSource: string; newSource: string; diffMethod?: DiffMethod; viewType?: 'split' | 'unified'; + /* + When "zip" is set to true, the change lines will be rendered in an interlaced style. + For an example, refer to: + https://github.com/otakustay/react-diff-view/blob/8a2dbdf97af0890aff6e563ed435e7da13c5e7b1/README.md#parse-diff-text + */ + zip?: boolean; } export const DiffView = ({ oldSource, newSource, - diffMethod = DiffMethod.WORDS, + diffMethod = DiffMethod.WORDS_WITH_SPACE, viewType = 'split', + zip = false, }: DiffViewProps) => { /* "react-diff-view" components consume diffs not as a strings, but as something they call "hunks". @@ -277,7 +283,10 @@ export const DiffView = ({ /* "diffFile" is essentially an object containing an array of hunks plus some metadata. */ - const diffFile = useMemo(() => convertToDiffFile(oldSource, newSource), [oldSource, newSource]); + const diffFile = useMemo( + () => convertToDiffFile(oldSource, newSource, zip), + [oldSource, newSource, zip] + ); /* Sections of diff without changes are hidden by default, because they are not present in the "hunks" array. diff --git a/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/mark_edits.tsx b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/mark_edits.tsx index 95eaefdc37f15..57cb34d28c338 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/mark_edits.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/json_diff/mark_edits.tsx @@ -38,7 +38,6 @@ export enum DiffMethod { WORDS = 'diffWords', WORDS_WITH_SPACE = 'diffWordsWithSpace', LINES = 'diffLines', - TRIMMED_LINES = 'diffTrimmedLines', SENTENCES = 'diffSentences', CSS = 'diffCss', } @@ -262,7 +261,7 @@ function diffChangeBlock( * The format of the strings is as follows: */ export function markEdits(hunks: HunkData[], diffMethod: DiffMethod): TokenizeEnhancer { - /* + /* changeBlocks is an array that contains information about the lines that have changes (additions or deletions). Unchanged lines are not included. */