Security Reports regression - 0 Results shown, shows completed, but pipeline run has findings.
Regression introduced by !1326 (merged)
The UI statuses were updated recently to incorporate the report status to be more accurate, that caused us to refactor how the reports are merged causing the regression in some edge cases.
Context
We do a separate network request for security findings for each scanner type. Sometimes we may only have on scanner, sometimes we have multiple.
Cause of Bug
We need to merge multiple scanner results. If only one report exists, or if one or more reports error out but we have some returned results the wrong status is returned or the results are not merged in correctly.
The fix
Instead of returning partial results, which may give the user a false sense of security because we may be omitting failed reports without their knowledge, we should fail early and report a 'Error' if any reports fail to parse or a network error occurs, and No Results
only if all scanners have no report, and Complete
if at least one report exists AND NO parse or network errors exist for other reports.
Challenge
We don't currently differentiate in vs-code workflow extension if ERROR
is due to the report not being generated because no pipeline job exists or because the file format is invalid.
Update
We can add to our query to include the status_reason
to differentiate between reports that don't exist, vs failed to parsed.
Plan
Gitlab Rails side review
-
Make use of graqph field
status_reason
https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/graphql/types/security/finding_reports_comparer_type.rb?ref_type=heads#L16-19 -
Resolver is here https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/graphql/resolvers/security_report/finding_reports_comparer_resolver.rb?ref_type=heads#L21
-
Compare call https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/models/ee/merge_request.rb?ref_type=heads#L299
-
Return
status_reason
https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/models/merge_request.rb?ref_type=heads#L2240
Vs code changes
-
If any of the report network requests fail, or any contain a
error
status for any other reason than a empty report, return early and show error status in the UI. -
Re-work filter in https://gitlab.com/gitlab-org/gitlab-vscode-extension/-/blob/main/src/desktop/gitlab/security_findings/get_all_security_reports.ts?ref_type=heads#L57 to filter to only allow for successful reports
-
Remove the possibility we call mergeReports with null params.
Add functional level tests to test the following scenarios
- When some reports are still parsing
- When some reports have a network fetch failure
- When all all reports parsed, and some reports report no report exists.
Code comments
document the expected behavior for folks since right now it looks like we always use the first finding's status
regardless of the source finding having a different status.
Additional test coverage
mergeReports
tested with integration level tests. We test getAllSecurityReports
Introducing a couple of specs for getAllSecurityReports
to test these edge cases
- All reports in the array being valid.
- The first item in the findings array being
null
. - A later item in the findings array being
null
.
These inputs can be mocked:
gitlabService: GitLabService,
project: GitLabProject,
mr: RestMr,
Also worth confirming that these branches are covered:
// Confirm stub validateVersion to throw an execption and assert it returned undefined.
await gitlabService.validateVersion('Security Findings', REQUIRED_VERSIONS.SECURITY_FINDINGS);
// Mock gitlabService.fetchFromApi to return the expected reports.
findings.reduce((acc, report) => mergeReports(acc, report));
// assert a UserFriendlyError is thrown if the last catch should have hit.
throw new UserFriendlyError(