Conversation
…config in new flow
| return []string{target.Target} | ||
| } | ||
|
|
||
| func GetWorkingDirsFromTarget(target results.ScanTarget) []string { |
|
|
||
| func (a *JasScanner) Run(scannerCmd ScannerCmd, module jfrogappsconfig.Module) (vulnerabilitiesSarifRuns []*sarif.Run, violationsSarifRuns []*sarif.Run, err error) { | ||
| func (a *JasScanner) DeprecatedRun(scannerCmd ScannerCmd, module jfrogappsconfig.Module, centralConfigExclusions []string) (vulnerabilitiesSarifRuns []*sarif.Run, violationsSarifRuns []*sarif.Run, err error) { | ||
| func() { |
There was a problem hiding this comment.
we can remove the func() {...}() wrapper - it adds no value it wraps it and calls the func inside DeprecatedRun.
| if len(st.Include) > 0 { | ||
| relativePaths := []string{} | ||
| for _, path := range st.Include { | ||
| relativePaths = append(relativePaths, utils.GetRelativePath(path, st.Target)) |
There was a problem hiding this comment.
aren't they already relative paths here?
| return false | ||
| } | ||
|
|
||
| func (st ScanTarget) String() (str string) { |
There was a problem hiding this comment.
- in line 267 do str += st.Name or you'll erase the previous step of the Target/Target + Include dirs
- The log can contain Target + Include dirs + Name + techs and it will look like that:
{} [].
maybe we should make it a bit more readable, like:
: {} [technologies: ]
| relative := utils.GetRelativePath(potential.Target, sourceBasePath) | ||
| log.Debug(fmt.Sprintf("Comparing target %s, relative: '%s'", potential.String(), relative)) | ||
| if technology != techutils.NoTech && potential.Technology != technology { | ||
| if len(technologies) > 0 && !utils.ElementsEqual[techutils.Technology](potential.Technologies, technologies) { |
There was a problem hiding this comment.
maybe not something we want to address but maybe worth noting- if a new tech is added in a PR (new module or something) we will find no match. this is a edge case but maybe worth a comment
| return filepath.Join(curationFolder, "nuget"), nil | ||
| } | ||
|
|
||
| func GetFullPathsWorkingDirs(workingDirs []string) ([]string, error) { |
There was a problem hiding this comment.
can maybe be replaced with coreutils.GetFullPathsWorkingDirs
| }) | ||
| } | ||
| // Load deprecated apps config information for all targets | ||
| if params.DeprecatedAppsConfig() == nil && !isNewFlow(params.bomGenerator) { |
There was a problem hiding this comment.
shouldn't it be params.DeprecatedAppsConfig != nil ?
| return | ||
| } | ||
|
|
||
| func runApplicabilityScan(applicabilityScanManager *ApplicabilityScanManager, params ContextualAnalysisScanParams) (vulnerabilitiesSarifRuns []*sarif.Run, err error) { |
There was a problem hiding this comment.
why manager is passed as a param and it is not a method?
| return | ||
| } | ||
|
|
||
| func runIacScan(iacScanManager *IacScanManager, params IacScanParams) (vulnerabilitiesResults []*sarif.Run, violationsResults []*sarif.Run, err error) { |
There was a problem hiding this comment.
why manager is passed as a param and it is not a method?
| return | ||
| } | ||
|
|
||
| func runSastScan(sastScanManager *SastScanManager, params SastScanParams) (vulnerabilitiesResults []*sarif.Run, violationsResults []*sarif.Run, err error) { |
There was a problem hiding this comment.
why manager is passed as a param and it is not a method?
| return | ||
| } | ||
|
|
||
| func runSecretsScan(secretScanManager *SecretScanManager, params SecretsScanParams) (vulnerabilitiesResults []*sarif.Run, violationsResults []*sarif.Run, err error) { |
There was a problem hiding this comment.
why manager is passed as a param and it is not a method?
| log.Debug(fmt.Sprintf("Skipping %s scan as requested by '%s' config profile...", jasType, configProfile.ProfileName)) | ||
| return true | ||
| } | ||
| // validate target is excluded by config profile exclude patterns |
There was a problem hiding this comment.
// validate IF target is excluded by config profile exclude patterns
| require.Equal(t, "dir/file2", sarifutils.GetLocationFileName(result.Locations[0])) | ||
| } | ||
|
|
||
| func TestShouldSkipScannerByConfigProfile(t *testing.T) { |
There was a problem hiding this comment.
what about test for ShouldSkipScannerByModule
|
|
||
| func GenerateSbomForTarget(generator SbomGenerator, params SbomGeneratorParams) { | ||
| startLog := "Generating SBOM" | ||
| if params.TotalTargets > 1 { |
There was a problem hiding this comment.
just making sure - it TotalTarget > 1, do we have them all in params.Target.Target?.
Also change the log to " for targets: %s"
| } | ||
| outLog := "SBOM generated" | ||
| if totalTargets > 1 { | ||
| outLog += fmt.Sprintf(" for target '%s'", target) |
There was a problem hiding this comment.
also here, it totalTargets > 1 we need to change the log to " for targets ..."
| log.Debug(fmt.Sprintf("%sSkipping SCA for %s as requested by input...", logPrefix, params.ScanResults.Target)) | ||
| return false, nil | ||
| } | ||
| if params.ScanResults == nil { |
There was a problem hiding this comment.
I see this check existed before, but I dont think it is really a possible usecase. we can leave it for safety though
| } | ||
|
|
||
| func TestPrepareSimpleJsonVulnerabilities_Technology(t *testing.T) { | ||
| testCases := []struct { |
There was a problem hiding this comment.
maybe add another testcase with multi-rows and different technologies?
| return | ||
| } | ||
| // Applicability scan does not produce violations. | ||
| vulnerabilitiesSarifRuns, _, err = applicabilityScanManager.scanner.DeprecatedRun(applicabilityScanManager, *params.Target.DeprecatedAppsConfigModule, params.Target.GetCentralConfigExclusions(utils.ContextualAnalysisScan)) |
There was a problem hiding this comment.
please check that calling GetCentralConfigExclusions is the correct call here. Here we are at the Deprecated case but we are callign the new GetExclusions func (which doesnt include AppsConfig)
| if params.Target.DeprecatedAppsConfigModule == nil { | ||
| return iacScanManager.scanner.Run(iacScanManager, params.Target) | ||
| } | ||
| return iacScanManager.scanner.DeprecatedRun(iacScanManager, *params.Target.DeprecatedAppsConfigModule, params.Target.GetCentralConfigExclusions(utils.IacScan)) |
There was a problem hiding this comment.
please check that calling GetCentralConfigExclusions is the correct call here. Here we are at the Deprecated case but we are callign the new GetExclusions func (which doesnt include AppsConfig)
| if params.Target.DeprecatedAppsConfigModule == nil { | ||
| return sastScanManager.scanner.Run(sastScanManager, params.Target) | ||
| } | ||
| return sastScanManager.scanner.DeprecatedRun(sastScanManager, *params.Target.DeprecatedAppsConfigModule, params.Target.GetCentralConfigExclusions(utils.SastScan)) |
There was a problem hiding this comment.
please check that calling GetCentralConfigExclusions is the correct call here. Here we are at the Deprecated case but we are callign the new GetExclusions func (which doesnt include AppsConfig)
| slice1 []string | ||
| slice2 []string | ||
| expected bool | ||
| }{ |
There was a problem hiding this comment.
please add testcases of:
slice1: {a, b, c}
slice2: (a, b, b)
slice1: {a,b,b}
slice2: {a,b,c}
| freq[v]++ | ||
| } | ||
| for _, v := range slice2 { | ||
| freq[v]-- |
There was a problem hiding this comment.
if v doesnt exist here wont we get a panic?
for example slice1 = {a,b,b}, slice2={a,c,b}
when we try doing freq["c"]-- I think we get a panic
eranturgeman
left a comment
There was a problem hiding this comment.
LGTM! see my comments

Support multiple working dirs with single-target scan flow & deprecate jfrog-apps-config in new flow
Summary
Refactors the audit scan pipeline to support multiple working directories via a single scan target (
--static-sca/ Xray lib flow), and deprecates thejfrog-apps-config.ymlmodule system for JAS scans in the new flow. Scan configuration (include/exclude patterns, central config modules) is now carried directly onScanTargetinstead of being resolved throughjfrog-apps-configmodules.Depends on:
Analyzer-Manager minimum version:
1.33.0Changes
ScanTargetextended – addedInclude,Exclude,DeprecatedAppsConfigModule, andCentralConfigModulesfields; added methodsIsScanRequestedByCentralConfig,GetCentralConfigExclusions,GetDeprecatedAppsConfigModuleExclusions.createSingleScanTarget) – when the new flow (Xray lib BOM generator) is active, a singleScanTargetis created with the working directories as include paths, instead of detecting one target per technology directory.Run(target ScanTarget)(new flow, target-based) andDeprecatedRun(module, centralConfigExclusions)(old flow, module-based). Config file generation, exclude-pattern resolution, and SARIF result reading follow the same split.jfrog-apps-configdeprecated in new flow –AppsConfigModulereplaced byDeprecatedAppsConfigModule; config loading only happens in the old (graph-based) flow. A deprecation warning is emitted when the file is detected.ScanTarget.Excludeis set during target detection and passed through to BOM generation (Xray libIgnorePatterns+IncludeDirs) and all JAS scanner config files.matchCentralConfigModulesassigns profile modules to targets;ShouldSkipScannerByConfigProfilechecks enablement per scan type on the target.fillMissingRequiredInvocationInformationnow aggregates execution success and creates a single canonical invocation withincludeproperty bag for multi-root targets.bomgenerator– logging of component counts and duration is now in the sharedGenerateSbomForTargetinstead of duplicated inXrayLibBomGenerator.GetFullPathsWorkingDirs,IsPathExcluded,ElementsEqual,CreateNewInvocation.commands/audit/audit_test.go,jas/common_test.go,jas/secrets/secretsscanner_test.go,jas/iac/iacscanner_test.go,jas/applicability/applicabilitymanager_test.go,utils/results/results_test.go,utils/paths_test.go,utils/utils_test.go.Testing
Notes
jfrog-apps-config.ymlis deprecated – flags, env vars, or central JFrog Platform config should be used instead.jfrog-apps-configas before.