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.

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

Apply allow-partial-results to JAS scans #223

25 changes: 15 additions & 10 deletions commands/audit/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,30 +181,30 @@
}
jfrogAppsConfig, err := jas.CreateJFrogAppsConfig(cmdResults.GetTargetsPaths())
if err != nil {
return cmdResults.AddGeneralError(fmt.Errorf("failed to create JFrogAppsConfig: %s", err.Error()))
return cmdResults.AddGeneralError(fmt.Errorf("failed to create JFrogAppsConfig: %s", err.Error()), false)

Check failure on line 184 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 184 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 184 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 184 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 184 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
// Initialize the parallel runner
auditParallelRunner := utils.CreateSecurityParallelRunner(auditParams.threads)
// Add the JAS scans to the parallel runner
var jasScanner *jas.JasScanner
var generalJasScanErr error
if jasScanner, generalJasScanErr = RunJasScans(auditParallelRunner, auditParams, cmdResults, jfrogAppsConfig); generalJasScanErr != nil {
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during JAS scan process. JAS scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalJasScanErr.Error()))
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during JAS scan process. JAS scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalJasScanErr.Error()), auditParams.AllowPartialResults())

Check failure on line 192 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 192 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 192 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 192 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 192 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
if auditParams.Progress() != nil {
auditParams.Progress().SetHeadlineMsg("Scanning for issues")
}
// The sca scan doesn't require the analyzer manager, so it can run separately from the analyzer manager download routine.
if generalScaScanError := buildDepTreeAndRunScaScan(auditParallelRunner, auditParams, cmdResults); generalScaScanError != nil {
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalScaScanError.Error()))
cmdResults.AddGeneralError(fmt.Errorf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s\n%s", strings.Join(cmdResults.GetTargetsPaths(), ","), generalScaScanError.Error()), auditParams.AllowPartialResults())

Check failure on line 199 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 199 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 199 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 199 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 199 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
go func() {
auditParallelRunner.ScaScansWg.Wait()
auditParallelRunner.JasWg.Wait()
// Wait for all jas scanners to complete before cleaning up scanners temp dir
auditParallelRunner.JasScannersWg.Wait()
if jasScanner != nil && jasScanner.ScannerDirCleanupFunc != nil {
cmdResults.AddGeneralError(jasScanner.ScannerDirCleanupFunc())
cmdResults.AddGeneralError(jasScanner.ScannerDirCleanupFunc(), false)

Check failure on line 207 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 207 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 207 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 207 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 207 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
auditParallelRunner.Runner.Done()
}()
Expand Down Expand Up @@ -242,7 +242,8 @@
}
auditParallelRunner.JasWg.Add(1)
if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(createJasScansTasks(auditParallelRunner, scanResults, serverDetails, auditParams, jasScanner, jfrogAppsConfig), func(taskErr error) {
generalError = errors.Join(generalError, fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()))
// TODO this change was for capturing a missed error that is coming from the threads
scanResults.AddGeneralError(fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()), auditParams.AllowPartialResults())

Check failure on line 246 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to scanResults.AddGeneralError

Check failure on line 246 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to scanResults.AddGeneralError

Check failure on line 246 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to scanResults.AddGeneralError

Check failure on line 246 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to scanResults.AddGeneralError

Check failure on line 246 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to scanResults.AddGeneralError
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is a bug fix.
We missed capturing errors coming from the threads and tried to propagate them up the stack, where they "got lost" in the process or got override. This fix ensure we add the to the GeneralError so it will be examined later on in the process

}); jasErr != nil {
generalError = fmt.Errorf("failed to create JAS task: %s", jasErr.Error())
}
Expand Down Expand Up @@ -281,9 +282,13 @@
SignedDescriptions: auditParams.OutputFormat() == format.Sarif,
ScanResults: targetResult,
TargetOutputDir: auditParams.scanResultsOutputDir,
AllowPartialResults: auditParams.AllowPartialResults(),

Check failure on line 285 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

unknown field AllowPartialResults in struct literal of type runner.JasRunnerParams

Check failure on line 285 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

unknown field AllowPartialResults in struct literal of type runner.JasRunnerParams

Check failure on line 285 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

unknown field AllowPartialResults in struct literal of type runner.JasRunnerParams

Check failure on line 285 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

unknown field AllowPartialResults in struct literal of type runner.JasRunnerParams

Check failure on line 285 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

unknown field AllowPartialResults in struct literal of type runner.JasRunnerParams
}
if generalError := runner.AddJasScannersTasks(params); generalError != nil {
if generalError = runner.AddJasScannersTasks(params); generalError != nil {
// TODO this fix was in order to avoid capturing the error twice when using partial-results. if this is disables the error is collected twice - once from the target error and once from general error
_ = targetResult.AddTargetError(fmt.Errorf("%s failed to add JAS scan tasks: %s", logPrefix, generalError.Error()), auditParams.AllowPartialResults())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug fix.
After adding the error to TargetError (or logging it if partial results are allowed) we left generalError as it was and it propagated up the stack and caught again later in the process. this caused the error to be logged twice.
After handling it, due to the new way we handle errors, there is no need to propagate the error by returning it after we handle it once

// We assign nil to 'generalError' after handling it to prevent it to propagate further, so it will not be captured twice - once here, and once in the error handling function of createJasScansTasks
generalError = nil
}
}
return
Expand All @@ -295,20 +300,20 @@
// Initialize general information
serverDetails, err := params.ServerDetails()
if err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)

Check failure on line 303 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 303 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 303 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 303 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 303 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
var xrayManager *xray.XrayServicesManager
if xrayManager, params.xrayVersion, err = xrayutils.CreateXrayServiceManagerAndGetVersion(serverDetails); err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)

Check failure on line 307 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 307 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 307 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 307 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 307 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
} else {
cmdResults.SetXrayVersion(params.xrayVersion)
}
if err = clientutils.ValidateMinimumVersion(clientutils.Xray, params.xrayVersion, scangraph.GraphScanMinXrayVersion); err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)

Check failure on line 312 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 312 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 312 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 312 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 312 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
}
entitledForJas, err := isEntitledForJas(xrayManager, params)
if err != nil {
return cmdResults.AddGeneralError(err)
return cmdResults.AddGeneralError(err, false)

Check failure on line 316 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint macos

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 316 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint ubuntu

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 316 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Static-Check

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 316 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Go-Sec

too many arguments in call to cmdResults.AddGeneralError

Check failure on line 316 in commands/audit/audit.go

View workflow job for this annotation

GitHub Actions / Lint windows

too many arguments in call to cmdResults.AddGeneralError
} else {
cmdResults.SetEntitledForJas(entitledForJas)
}
Expand Down
Loading