Add severity filtering to profile loading and CLI configuration #15
Conversation
|
hey @joanlopez , can you please check my approac? |
* Feature: added last check updates and check for updates once per day * removed logs calls and included review suggestions * fixed lint issue * Refine the update process * Apply suggestions from code review --------- Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com>
| @@ -127,6 +127,8 @@ func Parse(args []string) (Config, error) { | |||
| fs.Alias("ste", "stream-errors") | |||
| fs.BoolVar(output, &config.StreamMatches, "stream-matches", true, "If specified, those requests that caused a match are printed to stdout during the scan (live)\n\tEnabled by default, can be disabled with --stream-matches=false or -stm=false") | |||
| fs.Alias("stm", "stream-matches") | |||
| fs.StringVar(output, &config.Severity, "severity-filter", "", "If specified, only results with the given severity will be included in the output\n\t") | |||
There was a problem hiding this comment.
I don't think we need that
| fs.StringVar(output, &config.Severity, "severity-filter", "", "If specified, only results with the given severity will be included in the output\n\t") | |
| fs.StringVar(output, &config.Severity, "severity-filter", "", "If specified, only results with the given severity will be included in the output") |
| // Profile represents the behavior expected from a scan profile. | ||
| // It can be a passive or active profile (e.g. Active). | ||
| type Profile interface { | ||
| GetName() string | ||
| GetType() Type | ||
| IsEnabled() bool | ||
| GetTags() []string | ||
| GetSteps() []Step |
There was a problem hiding this comment.
I don't understand why we need this 🤔 It doesn't look like an outstanding design, cause it's only implemented by Active.
If you definitely need to access .Steps, and you have a Profile, then I'd suggest to try to type convert it with:
active, isActive := p.(*profile.Active)However, at time of filtering profiles, you already have typed profiles, so you probably don't need type conversation, but just rethink the SeverityGetter interface approach, which by the way doesn't work as expected (see the other comment).
| func (a *Active) GetSeverity() string { | ||
| if len(a.GetSteps()) > 0 { | ||
| return a.GetSteps()[0].IssueSeverity | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
I don't think this approach is a good idea, cause you want to consider all the steps for filtering.
For instance, the first step may have a Info severity, even not have an issue/severity, but a later step may have a Critical severity. So, in such case, if you're filtering to see only Critical, you'll want to keep this profile, and not filter it.
| @@ -511,6 +516,21 @@ func loadProfiles( | |||
| return actives, passiveReqs, passiveRes | |||
| } | |||
|
|
|||
| func filterBySeverity[T profile.SeverityGetter](profiles []T, severity string) []T { | |||
There was a problem hiding this comment.
I don't know if this is implementing the correct behavior.
I feel we want to treat "severities" as "levels". So, if I filter by "High" severity, I'd expect to see only those issues with "High" severity, but if I filter by "Medium", I'd expect to see both, those issues with "Medium", but also those with "High" severity.
Can you confirm @wagiro?
cc/ @iambouali cause you created the original issue.
Anyhow, @shubhamku044 could you please move this function (or all the filtering functions if you cannot apply the profile.SeverityGetter strategy pattern) to the profile package, and write at least a unit test that proofs the desired behavior?
There was a problem hiding this comment.
Hi! If the filter is High, only the profiles that contains any step with severity high, will be launched. If the filter is Medium, only the profiles with any step medium will be launched, and so on.
There was a problem hiding this comment.
Okay, then the behavior is correct I guess, but @shubhamku044 you still need to check the SeverityGetter approach to satisfy what @wagiro said: "with any step", not only the first one as you have now.
What do you @shubhamku044 think about defining the interface in a slightly different way, like: HasIssueWithSeverity(s string) bool, that for Request and Response checks if the severity is equals, and for Active it goes over all steps.
There was a problem hiding this comment.
Hey @shubhamku044! Thanks for your contribution! I left you some initial feedback, once we confirm the desired behavior with @wagiro, and you fix it, we'll be able to merge it.
However, don't forget that this is only half of the work. You'll also need to modify the piece of code responsible for reporting issues (matches), so it doesn't report when the severity is different/lower than the filtered one (this only applies to active profiles, which are the ones with multiple steps, and thus the ones where each step can have a different severity).
joanlopez
force-pushed
the
main
branch
3 times, most recently
from
0039716 to
1b2b37a
Compare
joanlopez
force-pushed
the
main
branch
2 times, most recently
from
96aa723 to
1e43205
Compare
No description provided.