Enable allow-partial-results for Audit SCA scan and dep tree construction (#200)

Author: eranturgeman

cli/docs/flags.go

@@ -119,6 +119,7 @@ const (
 	WorkingDirs                  = "working-dirs"
 	OutputDir                    = "output-dir"
 	SkipAutoInstall              = "skip-auto-install"
+	AllowPartialResults          = "allow-partial-results"
 
 	// Unique curation flags
 	CurationOutput = "curation-format"
@@ -155,7 +156,7 @@ var commandFlags = map[string][]string{
 		url, user, password, accessToken, ServerId, InsecureTls, Project, Watches, RepoPath, Licenses, OutputFormat, ExcludeTestDeps,
 		useWrapperAudit, DepType, RequirementsFile, Fail, ExtendedTable, WorkingDirs, ExclusionsAudit, Mvn, Gradle, Npm,
 		Pnpm, Yarn, Go, Nuget, Pip, Pipenv, Poetry, MinSeverity, FixableOnly, ThirdPartyContextualAnalysis, Threads,
-		Sca, Iac, Sast, Secrets, WithoutCA, ScanVuln, SecretValidation, OutputDir, SkipAutoInstall,
+		Sca, Iac, Sast, Secrets, WithoutCA, ScanVuln, SecretValidation, OutputDir, SkipAutoInstall, AllowPartialResults,
 	},
 	CurationAudit: {
 		CurationOutput, WorkingDirs, Threads, RequirementsFile,
@@ -230,9 +231,10 @@ var flagsMap = map[string]components.Flag{
 		"Set to false if you wish to not use the gradle or maven wrapper.",
 		components.WithBoolDefaultValue(true),
 	),
-	WorkingDirs:     components.NewStringFlag(WorkingDirs, "A comma-separated list of relative working directories, to determine audit targets locations."),
-	OutputDir:       components.NewStringFlag(OutputDir, "Target directory to save partial results to.", components.SetHiddenStrFlag()),
-	SkipAutoInstall: components.NewBoolFlag(SkipAutoInstall, "Set to true to skip auto-install of dependencies in un-built modules. Currently supported for Yarn and NPM only.", components.SetHiddenBoolFlag()),
+	WorkingDirs:         components.NewStringFlag(WorkingDirs, "A comma-separated list of relative working directories, to determine audit targets locations."),
+	OutputDir:           components.NewStringFlag(OutputDir, "Target directory to save partial results to.", components.SetHiddenStrFlag()),
+	SkipAutoInstall:     components.NewBoolFlag(SkipAutoInstall, "Set to true to skip auto-install of dependencies in un-built modules. Currently supported for Yarn and NPM only.", components.SetHiddenBoolFlag()),
+	AllowPartialResults: components.NewBoolFlag(AllowPartialResults, "Set to true to allow partial results and continuance of the scan in case of certain errors.", components.SetHiddenBoolFlag()),
 	ExclusionsAudit: components.NewStringFlag(
 		Exclusions,
 		"List of exclusions separated by semicolons, utilized to skip sub-projects from undergoing an audit. These exclusions may incorporate the * and ? wildcards.",

cli/scancommands.go

@@ -478,7 +478,8 @@ func CreateAuditCmd(c *components.Context) (*audit.AuditCommand, error) {
 		SetFixableOnly(c.GetBoolFlagValue(flags.FixableOnly)).
 		SetThirdPartyApplicabilityScan(c.GetBoolFlagValue(flags.ThirdPartyContextualAnalysis)).
 		SetScansResultsOutputDir(scansOutputDir).
-		SetSkipAutoInstall(c.GetBoolFlagValue(flags.SkipAutoInstall))
+		SetSkipAutoInstall(c.GetBoolFlagValue(flags.SkipAutoInstall)).
+		SetAllowPartialResults(c.GetBoolFlagValue(flags.AllowPartialResults))
 
 	if c.GetStringFlagValue(flags.Watches) != "" {
 		auditCmd.SetWatches(splitByCommaAndTrim(c.GetStringFlagValue(flags.Watches)))

commands/audit/audit.go

@@ -202,7 +202,8 @@ func RunAudit(auditParams *AuditParams) (results *utils.Results, err error) {
 	}
 	// The sca scan doesn't require the analyzer manager, so it can run separately from the analyzer manager download routine.
 	if scaScanErr := buildDepTreeAndRunScaScan(auditParallelRunner, auditParams, results); scaScanErr != nil {
-		auditParallelRunner.AddErrorToChan(scaScanErr)
+		// If error to be caught, we add it to the auditParallelRunner error queue and continue. The error need not be returned
+		_ = createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, fmt.Sprintf("An error has occurred during SCA scan process. SCA scan is skipped for the following directories: %s.", auditParams.workingDirs), scaScanErr)
 	}
 	go func() {
 		auditParallelRunner.ScaScansWg.Wait()
@@ -277,3 +278,27 @@ func downloadAnalyzerManagerAndRunScanners(auditParallelRunner *utils.SecurityPa
 	}
 	return
 }
+
+// This function checks if partial results are allowed. If so we log the error and continue.
+// If partial results are not allowed and a SecurityParallelRunner is provided we add the error to its error queue and return without an error, since the errors will be later collected from the queue.
+// If partial results are not allowed and a SecurityParallelRunner is not provided we return the error.
+func createErrorIfPartialResultsDisabled(auditParams *AuditParams, auditParallelRunner *utils.SecurityParallelRunner, extraMassageForLog string, err error) error {
+	if err == nil {
+		return nil
+	}
+
+	if auditParams.AllowPartialResults() {
+		if extraMassageForLog == "" {
+			extraMassageForLog = "An error has occurred during the audit scans"
+		}
+		log.Warn(fmt.Sprintf("%s\nSince partial results are allowed, the error is skipped: %s", extraMassageForLog, err.Error()))
+		return nil
+	}
+
+	// When SecurityParallelRunner is provided we add the error to the queue, otherwise we return the error
+	if auditParallelRunner != nil {
+		auditParallelRunner.AddErrorToChan(err)
+		return nil
+	}
+	return err
+}

commands/audit/audit_test.go

@@ -1,6 +1,9 @@
 package audit
 
 import (
+	"errors"
+	"fmt"
+	"net/http"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -197,3 +200,144 @@ func searchForStrWithSubString(t *testing.T, filesList []string, subString strin
 	}
 	assert.Fail(t, "File %s not found in the list", subString)
 }
+
+func TestAuditWithPartialResults(t *testing.T) {
+	testcases := []struct {
+		name                string
+		allowPartialResults bool
+		useJas              bool
+		testDirPath         string
+	}{
+		{
+			name:                "Failure in SCA during dependency tree construction",
+			allowPartialResults: false,
+			useJas:              false,
+			testDirPath:         filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-un-installable"),
+		},
+		{
+			name:                "Failure in SCA during scan itself",
+			allowPartialResults: false,
+			useJas:              false,
+			testDirPath:         filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-project"),
+		},
+		{
+			name:                "Skip failure in SCA during dependency tree construction",
+			allowPartialResults: true,
+			useJas:              false,
+			testDirPath:         filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-un-installable"),
+		},
+		{
+			name:                "Skip failure in SCA during scan itself",
+			allowPartialResults: true,
+			useJas:              false,
+			testDirPath:         filepath.Join("..", "..", "tests", "testdata", "projects", "package-managers", "npm", "npm-project"),
+		},
+		// TODO when applying allow-partial-results to JAS make sure to add a test case that checks failures in JAS scans + add  some JAS api call to the mock server
+	}
+
+	serverMock, serverDetails := utils.CreateXrayRestsMockServer(func(w http.ResponseWriter, r *http.Request) {
+		if r.RequestURI == "/xray/api/v1/system/version" {
+			_, err := w.Write([]byte(fmt.Sprintf(`{"xray_version": "%s", "xray_revision": "xxx"}`, scangraph.GraphScanMinXrayVersion)))
+			if !assert.NoError(t, err) {
+				return
+			}
+		}
+		if strings.HasPrefix(r.RequestURI, "/xray/api/v1/scan/graph") && r.Method == http.MethodPost {
+			// We set SCA scan graph API to fail
+			w.WriteHeader(http.StatusBadRequest)
+		}
+	})
+	defer serverMock.Close()
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			tempDirPath, createTempDirCallback := coreTests.CreateTempDirWithCallbackAndAssert(t)
+			defer createTempDirCallback()
+
+			assert.NoError(t, biutils.CopyDir(testcase.testDirPath, tempDirPath, false, nil))
+
+			auditBasicParams := (&utils.AuditBasicParams{}).
+				SetServerDetails(serverDetails).
+				SetOutputFormat(format.Table).
+				SetUseJas(testcase.useJas).
+				SetAllowPartialResults(testcase.allowPartialResults)
+
+			auditParams := NewAuditParams().
+				SetWorkingDirs([]string{tempDirPath}).
+				SetGraphBasicParams(auditBasicParams).
+				SetCommonGraphScanParams(&scangraph.CommonGraphScanParams{
+					ScanType:               scanservices.Dependency,
+					IncludeVulnerabilities: true,
+					MultiScanId:            utils.TestScaScanId,
+				})
+			auditParams.SetIsRecursiveScan(true)
+
+			scanResults, err := RunAudit(auditParams)
+			if testcase.allowPartialResults {
+				assert.NoError(t, scanResults.ScansErr)
+				assert.NoError(t, err)
+			} else {
+				assert.Error(t, scanResults.ScansErr)
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestCreateErrorIfPartialResultsDisabled(t *testing.T) {
+	testcases := []struct {
+		name                string
+		allowPartialResults bool
+		auditParallelRunner bool
+	}{
+		{
+			name:                "Allow partial results - no error expected",
+			allowPartialResults: true,
+			auditParallelRunner: true,
+		},
+		{
+			name:                "Partial results disabled with SecurityParallelRunner",
+			allowPartialResults: false,
+			auditParallelRunner: true,
+		},
+		{
+			name:                "Partial results disabled without SecurityParallelRunner",
+			allowPartialResults: false,
+			auditParallelRunner: false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(testcase.name, func(t *testing.T) {
+			auditBasicParams := (&utils.AuditBasicParams{}).SetAllowPartialResults(testcase.allowPartialResults)
+			auditParams := NewAuditParams().SetGraphBasicParams(auditBasicParams)
+
+			var auditParallelRunner *utils.SecurityParallelRunner
+			if testcase.auditParallelRunner {
+				auditParallelRunner = utils.CreateSecurityParallelRunner(1)
+			}
+
+			err := createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, "", errors.New("error"))
+			if testcase.allowPartialResults {
+				assert.NoError(t, err)
+			} else {
+				if testcase.auditParallelRunner {
+					assert.False(t, isErrorsQueueEmpty(auditParallelRunner))
+				} else {
+					assert.Error(t, err)
+				}
+			}
+		})
+	}
+}
+
+func isErrorsQueueEmpty(spr *utils.SecurityParallelRunner) bool {
+	select {
+	case <-spr.ErrorsQueue:
+		// Channel is not empty
+		return false
+	default:
+		// Channel is empty
+		return true
+	}
+}

commands/audit/scarunner.go

@@ -82,13 +82,15 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner
 				log.Warn(bdtErr.Error())
 				continue
 			}
-			err = errors.Join(err, fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, bdtErr.Error()))
+			err = errors.Join(err, createErrorIfPartialResultsDisabled(auditParams, nil, fmt.Sprintf("Dependencies tree construction ha failed for the following target: %s", scan.Target), fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, bdtErr.Error())))
 			continue
 		}
 		// Create sca scan task
 		auditParallelRunner.ScaScansWg.Add(1)
 		_, taskErr := auditParallelRunner.Runner.AddTaskWithError(executeScaScanTask(auditParallelRunner, serverDetails, auditParams, scan, treeResult), func(err error) {
-			auditParallelRunner.AddErrorToChan(fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, err.Error()))
+			// If error to be caught, we add it to the auditParallelRunner error queue and continue. The error need not be returned
+			_ = createErrorIfPartialResultsDisabled(auditParams, auditParallelRunner, fmt.Sprintf("Failed to execute SCA scan for the following target: %s", scan.Target), fmt.Errorf("audit command in '%s' failed:\n%s", scan.Target, err.Error()))
+			auditParallelRunner.ScaScansWg.Done()
 		})
 		if taskErr != nil {
 			return fmt.Errorf("failed to create sca scan task for '%s': %s", scan.Target, taskErr.Error())
@@ -147,8 +149,12 @@ func executeScaScanTask(auditParallelRunner *utils.SecurityParallelRunner, serve
 	scan *xrayutils.ScaScanResult, treeResult *DependencyTreeResult) parallel.TaskFunc {
 	return func(threadId int) (err error) {
 		log.Info(clientutils.GetLogMsgPrefix(threadId, false)+"Running SCA scan for", scan.Target, "vulnerable dependencies in", scan.Target, "directory...")
+		var xrayErr error
 		defer func() {
-			auditParallelRunner.ScaScansWg.Done()
+			if xrayErr == nil {
+				// We Sca waitGroup as done only when we have no errors. If we have errors we mark it done in the error's handler function
+				auditParallelRunner.ScaScansWg.Done()
+			}
 		}()
 		// Scan the dependency tree.
 		scanResults, xrayErr := runScaWithTech(scan.Technology, auditParams, serverDetails, *treeResult.FlatTree, treeResult.FullDepTrees)

tests/testdata/projects/package-managers/npm/npm-un-installable/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "jfrog-cli-tests",
+  "version": "v1.0.0",
+  "description": "test package",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "ISC",
+  "dependencies": {
+    "non-existing-dependency": "1.0.1"
+  }
+}

utils/auditbasicparams.go

@@ -65,6 +65,7 @@ type AuditBasicParams struct {
 	exclusions                       []string
 	isRecursiveScan                  bool
 	skipAutoInstall                  bool
+	allowPartialResults              bool
 }
 
 func (abp *AuditBasicParams) DirectDependencies() *[]string {
@@ -105,6 +106,11 @@ func (abp *AuditBasicParams) SetSkipAutoInstall(skipAutoInstall bool) *AuditBasi
 	return abp
 }
 
+func (abp *AuditBasicParams) SetAllowPartialResults(allowPartialResults bool) *AuditBasicParams {
+	abp.allowPartialResults = allowPartialResults
+	return abp
+}
+
 func (abp *AuditBasicParams) UseJas() bool {
 	return abp.useJas
 }
@@ -264,3 +270,7 @@ func (abp *AuditBasicParams) IsRecursiveScan() bool {
 func (abp *AuditBasicParams) SkipAutoInstall() bool {
 	return abp.skipAutoInstall
 }
+
+func (abp *AuditBasicParams) AllowPartialResults() bool {
+	return abp.allowPartialResults
+}

utils/results.go

@@ -31,8 +31,8 @@ func (r *Results) GetScaScansXrayResults() (results []services.ScanResponse) {
 	return
 }
 
-func (r *Results) GetScaScannedTechnologies() []techutils.Technology {
-	technologies := datastructures.MakeSet[techutils.Technology]()
+func (r *Results) GetScaScannedTechnologies(otherTech ...techutils.Technology) []techutils.Technology {
+	technologies := datastructures.MakeSetFromElements(otherTech...)
 	for _, scaResult := range r.ScaResults {
 		technologies.Add(scaResult.Technology)
 	}