feat: store multiple lines in one alert #426
Conversation
| @@ -309,42 +310,6 @@ export namespace Detection { | |||
| setSourceFocus: SetSourceFocus; | |||
| }; | |||
|
|
|||
| export function ShortExplanation(props: { | |||
There was a problem hiding this comment.
Note: this appears to removed for dead-code reasons.
There was a problem hiding this comment.
yes, it is not related to this feature, so I've discarded this change
| export function countAlertLocations(es: Alert[]): number { | ||
| return new Set(es.flatMap(e => e.lines.map(line => `${e.file}:${line}`))).size; | ||
| } |
There was a problem hiding this comment.
I think this is a huge semantic change.
Imagine an analysis that correctly identifies the following branch condition as being true in all cases, and that this relates to a concrete CVE:
if (
alllowSkipSecurity
// debug 1979: remember to remove this one later
|| !allowSkipSecurity
) {
skipSecurity()
}The analysis would rightfully flag up all three lines, and as a result, countAlertLocations would return 3.
IMO, this way of counting is wrong, and it changes what the framework is doing at the conceptual level.
Furthermore, we have the following above:
(flagging {countAlertLocations(props.hits)} out of{" "}
{countWeaknessLocations(props.weaknesses)} weakness locations in{" "}Which then would change its output to something that seems impossible:
- flagging 1 out of 1 weakness locations
+ flagging 3 out of 1 weakness locationsThere was a problem hiding this comment.
Good point, thank you. I've updated the Alert counting part. I've also moved from lines field to lineStart and lineEnd fields, it is not neccessary to store the alerted lines line by line in case of a multi-line alert. Also, it's less heavy to check like this wheter a weakness line is within the range of alerted lines or not.
| @@ -4,8 +4,7 @@ import * as ReactCodeMirror from "react-codemirror2"; | |||
| import * as ReactDOM from "react-dom"; | |||
| import * as Conclusions from "./Conclusions"; | |||
| import { Alert, Weakness } from "../shared/shared-types"; | |||
| import { getToolIcon, getToolOrderID } from "./util"; | |||
| import { rangeToArray } from "../../../../../src/util"; | |||
| import { getToolIcon, getToolOrderID, rangeToArray } from "./util"; | |||
There was a problem hiding this comment.
also, I've made an import mistake here which was preventing webpack from running, it's fixed now
I'm using this framework to compare AI driven tools vs static, or other AI tools. Usually, these AI tools generate a lot more alert, than static tools, which the current architecture could not handle (I've managed to exceed the maximum string lenght limit, where the reporting server tries to send the results data to the client in JSON format). I've overcame this problem by slightly changing the alert storing format from one line per Alert (here Alert with big A is the so-called data type in the framework), to multiple lines in one Alert.
This change does not modify anything in the behavior of the framework, only makes it possible to be used on a vast amount of data (also can be useful later in case a lot more static tools will be added to the contributions folder).