Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undetermined reason for undetermined contextual analysis status #155

Merged
merged 14 commits into from
Sep 12, 2024
Merged

Undetermined reason for undetermined contextual analysis status #155

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d5c433a
Undetermined reason for undetermined contextual analysis status
barv-jfrog Aug 25, 2024
92ed481
Undetermined reason for undetermined contextual analysis status
barv-jfrog Aug 25, 2024
6563175
Undetermined reason for undetermined contextual analysis status
barv-jfrog Aug 25, 2024
7e21a31
Merge branch 'dev' into undetermined-text
barv-jfrog Aug 28, 2024
2131ae5
Merge branch 'dev' into undetermined-text
barv-jfrog Sep 3, 2024
83ecc95
sarif utils fixes
barv-jfrog Sep 5, 2024
7acda7f
undetermined text fixes
barv-jfrog Sep 5, 2024
c65d385
undetermined text fixes
barv-jfrog Sep 5, 2024
58bb739
undetermined text fixes
barv-jfrog Sep 5, 2024
a443a30
undetermined text fixes
barv-jfrog Sep 5, 2024
8001482
Merge branch 'dev' into undetermined-text
barv-jfrog Sep 5, 2024
4d3773f
Merge branch 'dev' of https://github.com/jfrog/jfrog-cli-security int…
barv-jfrog Sep 5, 2024
61962a3
undetermined text fixes
barv-jfrog Sep 5, 2024
b931f91
Merge remote-tracking branch 'origin/undetermined-text' into undeterm…
barv-jfrog Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions formats/sarifutils/sarifutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,21 @@ func GetRuleFullDescription(rule *sarif.ReportingDescriptor) string {
return ""
}

func GetRuleProperty(key string, rule *sarif.ReportingDescriptor) string {
if rule != nil && rule.Properties != nil && rule.Properties[key] != nil {
prop, ok := rule.Properties[key].(string)
if !ok {
return ""
}
return prop
}
return ""
}

func GetRuleUndeterminedReason(rule *sarif.ReportingDescriptor) string {
return GetRuleProperty("undetermined_reason", rule)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function split is positive overall - but I don't see any other usage for GetRuleProperty, so in that case, I don't see a reason for the dedicated fucntion.
Couldn't we use it for the applicability status itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use it for applicability status as it is also received in the Rule property. Do you want me to change applicability fetch to this function ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - otherwise the usage of the this generic function just one misses the point IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please do use the new GetRuleProperty when applicable. but the GetRuleUndeterminedReason does not belong here. please move it to jasutils package.
This files is only for utils to access, create or set values in sarif

}

func GetRunRules(run *sarif.Run) []*sarif.ReportingDescriptor {
if run != nil && run.Tool.Driver != nil {
return run.Tool.Driver.Rules
Expand Down
8 changes: 5 additions & 3 deletions formats/sarifutils/test_sarifutils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ func CreateRunWithDummyResults(results ...*sarif.Result) *sarif.Run {
return run
}

func CreateRunWithDummyResultAndRuleProperties(property, value string, result *sarif.Result) *sarif.Run {
func CreateRunWithDummyResultAndRuleMultipleProperties(result *sarif.Result, properties, values []string) *sarif.Run {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name indicates that we have a function for multipleProperties, and a one for single.
If it's just that one function - I think you can keep the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - why did you switch the order of the args?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was nicer to have the arrays at the end of the function, what do you think?

run := sarif.NewRunWithInformationURI("", "")
if result.RuleID != nil {
run.AddRule(*result.RuleID)
}
run.AddResult(result)
run.Tool.Driver.Rules[0].Properties = make(sarif.Properties)
run.Tool.Driver.Rules[0].Properties[property] = value
run.Tool.Driver.Rules[0].Properties = make(sarif.Properties, len(properties))
for index, _ := range properties {
barv-jfrog marked this conversation as resolved.
Show resolved Hide resolved
run.Tool.Driver.Rules[0].Properties[properties[index]] = values[index]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest makeing sure that len(values)==len(properties)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, fixed

}
return run
}

Expand Down
1 change: 1 addition & 0 deletions formats/simplejsonapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type CveRow struct {
type Applicability struct {
Status string `json:"status"`
ScannerDescription string `json:"scannerDescription,omitempty"`
UndeterminedReason string `json:"undeterminedReason,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do we use it? UndeterminedReason is being inserted into - but not called...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only in simple-json, and it is under CveRow (CveRow has Applicability struct as one of its components, it gets printed automatically)

Evidence []Evidence `json:"evidence,omitempty"`
}

Expand Down
1 change: 1 addition & 0 deletions utils/resultstable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,7 @@ func getCveApplicabilityField(cveId string, applicabilityScanResults []*sarif.Ru
if rule, _ := applicabilityRun.GetRuleById(jasutils.CveToApplicabilityRuleId(cveId)); rule != nil {
applicability.ScannerDescription = sarifutils.GetRuleFullDescription(rule)
status := getApplicabilityStatusFromRule(rule)
applicability.UndeterminedReason = sarifutils.GetRuleUndeterminedReason(rule)
if status != "" {
applicabilityStatuses = append(applicabilityStatuses, status)
}
Expand Down
27 changes: 20 additions & 7 deletions utils/resultstable_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,9 +682,9 @@ func TestGetApplicableCveValue(t *testing.T) {
name: "new scan statuses - applicable wins all statuses",
scanResults: &ExtendedScanResults{
ApplicabilityScanResults: []*sarif.Run{
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "applicable", sarifutils.CreateDummyPassingResult("applic_testCve1")),
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "not_applicable", sarifutils.CreateDummyPassingResult("applic_testCve2")),
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "not_covered", sarifutils.CreateDummyPassingResult("applic_testCve3")),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve1"), []string{"applicability"}, []string{"applicable"}),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve2"), []string{"applicability"}, []string{"not_applicable"}),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve3"), []string{"applicability"}, []string{"not_covered"}),
},
EntitledForJas: true},
cves: []services.Cve{{Id: "testCve1"}, {Id: "testCve2"}, {Id: "testCve3"}},
Expand All @@ -698,8 +698,8 @@ func TestGetApplicableCveValue(t *testing.T) {
name: "new scan statuses - not covered wins not applicable",
scanResults: &ExtendedScanResults{
ApplicabilityScanResults: []*sarif.Run{
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "not_covered", sarifutils.CreateDummyPassingResult("applic_testCve1")),
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "not_applicable", sarifutils.CreateDummyPassingResult("applic_testCve2")),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve1"), []string{"applicability"}, []string{"not_covered"}),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve2"), []string{"applicability"}, []string{"not_applicable"}),
},
EntitledForJas: true},
cves: []services.Cve{{Id: "testCve1"}, {Id: "testCve2"}},
Expand All @@ -712,8 +712,8 @@ func TestGetApplicableCveValue(t *testing.T) {
name: "new scan statuses - undetermined wins not covered",
scanResults: &ExtendedScanResults{
ApplicabilityScanResults: []*sarif.Run{
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "not_covered", sarifutils.CreateDummyPassingResult("applic_testCve1")),
sarifutils.CreateRunWithDummyResultAndRuleProperties("applicability", "undetermined", sarifutils.CreateDummyPassingResult("applic_testCve2")),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve1"), []string{"applicability"}, []string{"not_covered"}),
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve2"), []string{"applicability"}, []string{"undetermined"}),
},
EntitledForJas: true},
cves: []services.Cve{{Id: "testCve1"}, {Id: "testCve2"}},
Expand All @@ -722,6 +722,19 @@ func TestGetApplicableCveValue(t *testing.T) {
{Id: "testCve2", Applicability: &formats.Applicability{Status: jasutils.ApplicabilityUndetermined.String()}},
},
},
{
name: "undetermined with undetermined reason",
scanResults: &ExtendedScanResults{
ApplicabilityScanResults: []*sarif.Run{
sarifutils.CreateRunWithDummyResultAndRuleMultipleProperties(sarifutils.CreateDummyPassingResult("applic_testCve2"), []string{"applicability", "undetermined_reason"}, []string{"undetermined", "however"}),
},
EntitledForJas: true},
cves: []services.Cve{{Id: "testCve2"}},
expectedResult: jasutils.ApplicabilityUndetermined,
expectedCves: []formats.CveRow{
{Id: "testCve2", Applicability: &formats.Applicability{Status: jasutils.ApplicabilityUndetermined.String(), UndeterminedReason: "however"}},
},
},
}

for _, testCase := range testCases {
Expand Down
Loading