[Security Solution] JSON diff view PoC #171750
Conversation
nikitaindik
force-pushed
the
json-diffs-prebuilt-rules
branch
from
50bcfde
to
878fce0
Compare
nikitaindik
force-pushed
the
json-diffs-prebuilt-rules
branch
from
90a09e3
to
e4761fb
Compare
nikitaindik
force-pushed
the
json-diffs-prebuilt-rules
branch
from
95ccaf4
to
be5cad2
Compare
| import type { RuleFieldsDiff } from '../../../../../common/api/detection_engine/prebuilt_rules/model/diff/rule_diff/rule_diff'; | ||
| import type { RuleResponse } from '../../../../../common/api/detection_engine/model/rule_schema/rule_schemas.gen'; | ||
|
|
||
| const HIDDEN_FIELDS = ['meta', 'rule_schedule', 'version']; |
There was a problem hiding this comment.
I know it's not relevant for the POC, but curious why rule_schedule is hidden here? The rule schedule can be updated by the TRADE team (and later on customzied by the user)
| const mergedVersion: string = get(fields, [fieldName, 'merged_version'], ''); | ||
|
|
||
| const oldSource = | ||
| compareMethod === DiffMethod.JSON && typeof currentVersion === 'object' |
There was a problem hiding this comment.
currentVersion will always be a string so this always evaulates to JSON.stringify(currentVersion, null, 2). Same in the calculation of newSource
| import { isDelete, isInsert, isNormal, pickRanges } from 'react-diff-view'; | ||
| import type { ChangeData, HunkData, RangeTokenNode, TokenizeEnhancer } from 'react-diff-view'; | ||
|
|
||
| interface JsDiff { |
There was a problem hiding this comment.
Could be refactored like:
type StringDiffFn = (oldStr: string, newStr: string) => Change[];
type JsonDiffFn = (
oldObject: Record<string, unknown>,
newObject: Record<string, unknown>
) => Change[];
interface JsDiff {
diffChars: StringDiffFn;
diffWords: StringDiffFn;
diffWordsWithSpace: StringDiffFn;
diffLines: StringDiffFn;
diffTrimmedLines: StringDiffFn;
diffSentences: StringDiffFn;
diffCss: StringDiffFn;
diffJson: JsonDiffFn;
}
| isInsert: true, | ||
| lineNumber: 14, | ||
| type: "insert" |
There was a problem hiding this comment.
I think these 3 properties live within the changes object, which is a sibling of content?
| <Unfold | ||
| direction="down" | ||
| start={collapsedStart} | ||
| end={collapsedStart + 10} | ||
| onExpand={onExpand} | ||
| /> | ||
| <Unfold direction="none" start={collapsedStart} end={collapsedEnd} onExpand={onExpand} /> | ||
| <Unfold direction="up" start={collapsedEnd - 10} end={collapsedEnd} onExpand={onExpand} /> | ||
| </> |
There was a problem hiding this comment.
Not sure here if this is the best UX. While I think it's helpful to be able to expand 10 lines down, 10 lines up or all collapsed lines, my first reaction when I saw how it looks was a little confused:
Wondering if displaying only the "Expand hidden 38 lines" is clearer when there are no other changes in the hidden lines. But would like to know what the team thinks.
There was a problem hiding this comment.
I agree. I also found it a bit confusing, but I decided to leave it there for the PoC to show that a possibility to show a bit more exists in case someone might find it useful. But yeah, I also vote for having only "Expand 38 lines".
There was a problem hiding this comment.
@nikitaindik Thanks for this great effort! The diffing component looks great and covers everything we need for this phase and pretty sure for all requirements we will have on Phase 3 as well.
From my review of the alternatives, and the discussion we had synchronously, my conclusions are:
- app-experience-team and diff2html: discarded since they don't satisfy all our requirements.
- react-diff-viewer-continued: since with react-diff-view we can achieve all the same results (albeit with more manual work, less out of the box features) I'd discard this option in favour of react-diff-view.
So for me, it boils down to react-diff-view vs monaco:
- one of the main reasons we were doubting to use monaco was its potential bundle size, but the PR's description shows that for react-diff-view it's 22kb zipped, while the combined size for the two bundles needed for monaco is 19kb, zipped.
- monaco is used already in a number of parts of the Kibana application, so it's a proved solution (although not used for diffing functionality, yet)
- react-diff-view indeed seems to be more flexible and we can achieve results that monaco can't. The most notable for me:
- styling: monaco seems very unflexible with this (not sure if changing styles is possible though) and the POC shows that react-diff-view allows basically any styling choices that we want. If we decide to go with monaco I think we should communicate this to Product/Design and get them to compromise - I think the styles monaco uses are acceptable.
- tokenizing: in the POC we couldn't get monaco to tokenize by word, and some diffs might look a little off this way. For example, comparing tags, there's no point in knowing that "Defense Evasion" shares the "D" with "Data Source: Elastic Endgame".
react-diff-view even allows us to select the diffing algorithm and switch between them. Having said that, I consider the choosing of the diff algorithm a nice-to-have and don't consider the way that monaco displays diffs a deal-breaker at all. Especially considering that the react-diff-view's diffing algorithms had to be coded manually. - unchanged lines collapse/expansion: here both libs seems to follow quite a distinct philosophy. With react-diff-view we collapse sections of the rule with no changes and have the "Expand N lines" clickable buttons to expand it. monaco shows all content by default, which could not be the best UX for particularly long files. I think, however, that this "painpoint" is greatly reduced through the filemap at the right (beside the scrollbar) which allows the user to see where the changes are, and to quickly scroll to them to review them (in way a dev can be used to do when checking for changes/reviewing PRs in VSCode)
- alignment of properties: I feel Monaco does a better job here keeping the properties aligned side by side, and prefer the way it fills empty space with the crossed-gray lines. For example, compare how the diff for description and false_positives are handled in monaco vs react-diff-view:
Also, Monaco keeps the name and note props better aligned when meta is removed:
- use as editable field: thinking ahead here, but for Phase 3 we have to think about giving the users the ability to edit rule fields. Unsure here how much effort it would take to adapt react-diff-view if we wanted to make it editable (we can always fallback to a normal editable EUI component), but monaco will do that out of the box, and could potentially be less design breaking to have all columns in the three way diff component done with the same library.
All in all, I'm favouring the use of monaco here, especially considering the effort required to use react-diff-view and the pretty tight deadline we have for FF 8.12 (I think if we went for the react-diff-view option, it should be created as a package that we import).
Looking forward to the teams opinion though.
|
Thanks for checking this out @jpdjere! Here's what I know about Monaco styling and word diffing. Monaco diffs styling is limited. You can only change background and border colors for added / removed sections and chars. Here's an example. And a full list of stylable properties. So no way to add the Also Monaco doesn't support having different color stylings for different instances of monaco-editor. Once you apply it to one instance, it also applies to others. I wonder if it may cause unwanted styling of other monacos in Kibana. Also it seems that in many scenarios monaco doesn't handle multiple instances correctly. There are many related issues on GitHub, like 1, 2, 3. Theoretically there is a possibility to add word-diffing to Monaco. There's a way to write a diffing plugin, but there are no docs for this and you'll have to work with monaco / VSCode specific objects, not plain strings. I couldn't find any ready npm package that does this or any example of someone actually doing this successfully. |
There was a problem hiding this comment.
Visualizing changes in the MITRE ATT&CK (threat) rule field
Hey @nikitaindik, I did some local testing and would like to share something.
I installed the Linux Restricted Shell Breakout via Linux Binary(s) prebuilt rule. Its rule_id is 52376a86-ee86-4967-97ae-1a05f55816f0.
I called the rule patch endpoint against it with the following request body:
{
"id": "716c682f-287a-45af-b6b7-2556655847f4",
"name": "Linux Restricted Shell Breakout via Linux Binary(s) (UPDATED)",
"version": 109,
"tags": [
"Domain: Endpoint",
"OS: Linux",
"Use Case: Threat Detection",
"Tactic: Execution",
"Data Source: Elastic Defend (RENAMED)",
"Some: New Tag"
],
"severity": "high",
"risk_score": 77,
"query": "process where host.os.type == \"linux\" and event.type == \"start\"",
"threat": [
{
"framework": "MITRE ATT&CK",
"tactic": {
"id": "TA0002",
"name": "Execution",
"reference": "https://attack.mitre.org/tactics/TA0002/"
},
"technique": [
{
"id": "T1059",
"name": "Command and Scripting Interpreter",
"reference": "https://attack.mitre.org/techniques/T1059/"
}
]
}
]
}This request removes a MITRE subtechnique from the threat structure.
The only implementation that could visualize the diff properly was the react-diff-viewer-continued with the JSON diff algorithm:
All the other implementations show the threat field as an array having a single empty object inside. @nikitaindik do you have an idea why it works like that? Can other implementations show nested arrays and objects correctly?
react-diff-viewer-continued:
react-diff-view:
monaco:
|
As soon as I open the 
|
nikitaindik
force-pushed
the
json-diffs-prebuilt-rules
branch
from
5e9b29e
to
a450270
Compare
@banderror Good catch! Apparently my simplistic approach to JSON sorting didn't handle nested arrays properly. That's what I used JSON.stringify(jsObject, Object.keys(jsObject).sort(), 2);Should be fixed now. JSON sorting algorithm from "react-diff-viewer-continued" worked because unlike other algorithms it accepts objects (not strings) and sorts them under the hood. |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @nikitaindik |
| import type { Change } from 'diff'; | ||
| import { diffLines } from 'diff'; |
There was a problem hiding this comment.
Note that the diff library is specified in dev dependencies in package.json. We should only use it in code if we move it to normal dependencies.
| original={sortAndStringifyJson(oldRule)} | ||
| value={sortAndStringifyJson(newRule)} | ||
| fullWidth={true} | ||
| height="500px" |
There was a problem hiding this comment.
Can it grow to the full available height in the flyout? Or consume as much height as needed?
Setting height="100%" doesn't work, and fixed 500px height doesn't look like something we could ship in production:
| export const sortObject = (jsObject) => { | ||
| if (typeof jsObject !== 'object' || jsObject === null) { | ||
| return jsObject; | ||
| } | ||
|
|
||
| if (Array.isArray(jsObject)) { | ||
| return jsObject.map((item) => sortObject(item)); | ||
| } | ||
|
|
||
| return Object.keys(jsObject) | ||
| .sort() | ||
| .reduce((sorted, key) => { | ||
| sorted[key] = sortObject(jsObject[key]); | ||
| return sorted; | ||
| }, {}); | ||
| }; | ||
|
|
||
| export const sortAndStringifyJson = (jsObject: Record<string, unknown>): string => | ||
| JSON.stringify(sortObject(jsObject), null, 2); |
There was a problem hiding this comment.
@nikitaindik Thanks for fixing #171750 (comment) using this code.
I think we can remove this complexity by using https://www.npmjs.com/package/json-stable-stringify. It's a popular library and it's already used in Kibana.
This code works (just tested it with all the libraries in your branch):
import stringify from 'json-stable-stringify';
export const sortAndStringifyJson = (jsObject: Record<string, unknown>): string =>
stringify(jsObject, { space: 2 });
|
I created individual PRs - one PR per each library - to be able to compare:
Here are the PRs: |
Some pros and consreact-diff-viewer-continuedPros:
Cons:
react-diff-viewPros:
Cons:
Monaco editorPros:
Cons:
|
I feel like choosing the diff algorithm might be more than just a nice-to-have. The char tokenizing for things that are just a string or regular text is pretty straight forward to see and deal with but when we have super long query strings, it definitely doesn't help the readability especially compared to the word tokenizers in Here's some basic examples with a KQL query: monaco react-diff-viewer I'll admit neither works perfectly in some of these scenarios (the dashes and slashes seem to confuse all the tokenizers) but in my opinion the words all being together helps with some cohesion in an already hard to read, long query string diff. How the That being said, I agree with most of the pros for I also do like For me the slowest library was Overall, I probably agree with the decision being between Also thanks for putting this together @nikitaindik, this is an awesome hands on visualization! |
++ Agree with Davis, being able to show meaningful diffs for individual text-based fields will be very important from the UX perspective. Thank you for providing these screenshot examples @dplumlee! |
|
If this is for the pre-built rule upgrade workflow, wouldn’t it be easier to let users enter a list of fields which don’t get overwritten when they upgrade the enabled pre-built rules? All customers need is a way to exclude fields like timestamp override, index pattern, data view and rule exceptions. Or when you do the upgrade you only update fields like the query (alerting logic), description, investigation guide etc. If customers need to add filtering logic to the rule to reduce the number of alerts before the exception stage, add a new filtering section which can be copied between json objects. Then it’s a simple case of copying the field from the previous json object to the new json object, or elastic only updating fields required to update alerting logic. We just want elastic to manage the pre-built rules with as little work on our side as possible. In my view json diff is too complicated and something which could be works on as a long term objective. Due to the complexity in the current method I could see it being hit by bugs when it’s released. |
|
I don't think you need a diff library for this. Another option is take the key from the current rule and key from new rule, if key1 != key2 value ask the user if they want to update the value. It should be possible to iterate object keys in nodejs, even if they are child keys. Cache the keys from the current rule, and add any new keys from the new rule to the current rule json object. |
|
Hey @mbudge, thanks for your suggestions. The JSON diff we're working on is going to be readonly, and it will show what updates from Elastic will the user receive if they click "upgrade this prebuilt rule". This is a short-term feature to slightly improve the current rule upgrade UX. Long-term, when we allow users to customize prebuilt rules, the UX for rule upgrades will be developed further and will not be based on interactive JSON diffs, so no complexity should be introduced for our users in that regard.
This is an interesting suggestion, thank you for it. We will discuss it with our team members. |
Ticket: #169160
Summary
Comparing different libraries for showing text diffs.
About the libs
react-diff-viewer-continued
GitHub stars: 65
Weekly downloads: 55,000
Bundle size: 46kB (min), 16kb (min + gzip)
It's fork of a popular lib that was abandoned by its maintainer. The original version doesn't work in our project because it relies on old versions of emotion packages. The fork has more up-to-date dependencies and works with our version of emotion.
Pros:
Cons: The original project is abandoned. Right now the fork doesn't seem to have a lot of dev activity going on and I couldn't find any info about future plans.
react-diff-view
GitHub stars: 743
Weekly downloads: 18,000
Bundle size: 71kB (min), 22kb (min + gzip)
Very customisable, but requires DIY work to be useful. You can't pass your diff as two strings - you have to do a number of conversions first. You can check my comments in code to understand what's going on more quickly. Development is not very active, but the author seems to be preparing a new major version.
Pros:
Cons:
@kbn/monaco
GitHub stars: 36,400
Weekly downloads: 842,794
Bundle size (main package): 110kB (min), 18kb (min + gzip)
Bundle size (react-monaco-editor): 5kB (min), 1kb (min + gzip)
Code editor developed by Microsoft that powers VSCode. It has a diff mode, the same as you see in VSCode. You can even merge sections of diff using arrow buttons (not our use case, of course).
Pros:
Cons:
diff2html
Not customisable. Not very reacty - generates an HTML string that you insert via
dangerouslySetInnerHTML:Cons:
Application Experience Team PoC
Davis from the Application Experience Team made a very nice-looking PoC for a feature that didn't end up on their roadmap. No plans to prioritise it for now, so the PR is still in draft. You can check this video to see it in action.
demo_json.mp4
It's simple and good-looking but very barebones for our use case: no split view and expandable / hidden code sections, for example.