From d502603ed6d3dacb5678bbc48224419ed08ab162 Mon Sep 17 00:00:00 2001 From: attiasas Date: Thu, 30 Jan 2025 16:37:00 +0200 Subject: [PATCH 01/14] Git Audit - Diff mode --- cli/docs/git/audit/help.go | 9 +++++++++ cli/gitcommands.go | 6 ++++++ commands/git/audit/gitauditparams.go | 8 +++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/cli/docs/git/audit/help.go b/cli/docs/git/audit/help.go index ff04d8e4..d96caf9a 100644 --- a/cli/docs/git/audit/help.go +++ b/cli/docs/git/audit/help.go @@ -1,5 +1,14 @@ package audit +import ( + "github.com/jfrog/jfrog-cli-core/v2/plugins/components" +) + func GetDescription() string { return "Audit your local git repository project for security issues." } + +func GetArguments() []components.Argument { + return []components.Argument{{Name: "target", Description: `Diff mode, run git diff between the cwd commit to the given target commit and audit the differences. + You can specify a commit hash, a branch name a tag name or a reference like HEAD~1.`}} +} diff --git a/cli/gitcommands.go b/cli/gitcommands.go index 3fba3456..4ada37ea 100644 --- a/cli/gitcommands.go +++ b/cli/gitcommands.go @@ -42,6 +42,12 @@ func getGitNameSpaceCommands() []components.Command { func GitAuditCmd(c *components.Context) error { gitAuditCmd := audit.NewGitAuditCommand() + + if len(c.Arguments) > 0 { + // Diff mode + gitAuditCmd.SetDiffTarget(c.Arguments[0]) + } + // Set connection params serverDetails, err := createServerDetailsWithConfigOffer(c) if err != nil { diff --git a/commands/git/audit/gitauditparams.go b/commands/git/audit/gitauditparams.go index 34030426..02b472d4 100644 --- a/commands/git/audit/gitauditparams.go +++ b/commands/git/audit/gitauditparams.go @@ -12,7 +12,8 @@ import ( type GitAuditParams struct { // Git Params - source services.XscGitInfoContext + source services.XscGitInfoContext + diffTarget string // Connection params serverDetails *config.ServerDetails // Violations params @@ -37,6 +38,11 @@ func NewGitAuditParams() *GitAuditParams { return &GitAuditParams{} } +func (gap *GitAuditParams) SetDiffTarget(diffTarget string) *GitAuditParams { + gap.diffTarget = diffTarget + return gap +} + func (gap *GitAuditParams) SetServerDetails(serverDetails *config.ServerDetails) *GitAuditParams { gap.serverDetails = serverDetails return gap From 915e9ecec7aab6e751692af24c251cac120ad274 Mon Sep 17 00:00:00 2001 From: attiasas Date: Sun, 2 Feb 2025 09:34:12 +0200 Subject: [PATCH 02/14] Start impl git diff --- utils/gitutils/gitmanager.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/utils/gitutils/gitmanager.go b/utils/gitutils/gitmanager.go index 7c9d45a9..f5094c62 100644 --- a/utils/gitutils/gitmanager.go +++ b/utils/gitutils/gitmanager.go @@ -122,6 +122,33 @@ func (gm *GitManager) IsClean() (bool, error) { return status.IsClean(), nil } + +func (gm *GitManager) Diff(reference string) (err error) { + // Get the current branch + currentBranch, err := gm.localGitRepository.Head() + if err != nil { + return + } + // Get the commit object of the current branch + currentCommit, err := gm.localGitRepository.CommitObject(currentBranch.Hash()) + if err != nil { + return + } + // Get the commit object of the reference + referenceCommit, err := gm.localGitRepository.CommitObject(currentCommit.Hash) + if err != nil { + return + } + // Get the diff between the current branch and the reference + diff, err := currentCommit.Patch(referenceCommit) + if err != nil { + return + } + // Print the diff + fmt.Println(diff) + return +} + // Detect git information func (gm *GitManager) GetGitContext() (gitInfo *services.XscGitInfoContext, err error) { remoteUrl, err := getRemoteUrl(gm.remote) From 825ae056420264c2d5266f3ce49d49ebf64a8eea Mon Sep 17 00:00:00 2001 From: attiasas Date: Sun, 2 Feb 2025 09:35:43 +0200 Subject: [PATCH 03/14] remove ignore to fail frogbot --- utils/gitutils/gitmanager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/gitutils/gitmanager.go b/utils/gitutils/gitmanager.go index f5094c62..cf114a68 100644 --- a/utils/gitutils/gitmanager.go +++ b/utils/gitutils/gitmanager.go @@ -203,7 +203,6 @@ func getRemoteUrl(remote *goGit.Remote) (remoteUrl string, err error) { // Normalize the URL by removing protocol prefix and any trailing ".git" func normalizeGitUrl(url string) string { - // jfrog-ignore - false positive, not used for communication url = strings.TrimPrefix(url, "http://") url = strings.TrimPrefix(url, "https://") url = strings.TrimPrefix(url, "ssh://") From 815ef399d6cd26774e6bb9c461d2076ec3486dcb Mon Sep 17 00:00:00 2001 From: attiasas Date: Sun, 2 Feb 2025 10:03:42 +0200 Subject: [PATCH 04/14] revert --- utils/gitutils/gitmanager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/gitutils/gitmanager.go b/utils/gitutils/gitmanager.go index cf114a68..10d3d4fd 100644 --- a/utils/gitutils/gitmanager.go +++ b/utils/gitutils/gitmanager.go @@ -122,7 +122,6 @@ func (gm *GitManager) IsClean() (bool, error) { return status.IsClean(), nil } - func (gm *GitManager) Diff(reference string) (err error) { // Get the current branch currentBranch, err := gm.localGitRepository.Head() @@ -203,6 +202,7 @@ func getRemoteUrl(remote *goGit.Remote) (remoteUrl string, err error) { // Normalize the URL by removing protocol prefix and any trailing ".git" func normalizeGitUrl(url string) string { + // jfrog-ignore - false positive, not used for communication url = strings.TrimPrefix(url, "http://") url = strings.TrimPrefix(url, "https://") url = strings.TrimPrefix(url, "ssh://") From 2516eb992ff3e63ef131eabed6d926c7022cafe3 Mon Sep 17 00:00:00 2001 From: attiasas Date: Mon, 3 Feb 2025 18:03:05 +0200 Subject: [PATCH 05/14] start diff --- commands/git/audit/diffmode.go | 50 +++++++ commands/git/audit/gitaudit.go | 33 ++++- utils/gitutils/filediff.go | 135 ++++++++++++++++++ utils/gitutils/filediff_test.go | 4 + utils/gitutils/gitmanager.go | 62 ++++---- utils/gitutils/gitmanager_test.go | 49 +++++-- .../results/output/securityJobSummary_test.go | 2 +- utils/results/results.go | 19 +-- utils/xsc/analyticsmetrics_test.go | 4 +- 9 files changed, 309 insertions(+), 49 deletions(-) create mode 100644 commands/git/audit/diffmode.go create mode 100644 utils/gitutils/filediff.go create mode 100644 utils/gitutils/filediff_test.go diff --git a/commands/git/audit/diffmode.go b/commands/git/audit/diffmode.go new file mode 100644 index 00000000..845f1fae --- /dev/null +++ b/commands/git/audit/diffmode.go @@ -0,0 +1,50 @@ +package audit + +import ( + "github.com/jfrog/jfrog-cli-security/utils/gitutils" + "github.com/jfrog/jfrog-cli-security/utils/results" + "github.com/jfrog/jfrog-client-go/utils/log" +) + +func filterResultsNotInDiff(scanResults *results.SecurityCommandResults, changes *gitutils.ChangesRelevantToScan) (onlyResultsInDiff *results.SecurityCommandResults) { + if changes == nil || !changes.HasFileChanges() { + log.Debug("No diff targets to filter results") + return scanResults + } + diffDescriptors := getDescriptorsFromDiff(changes.GetChangedFilesPaths()) + // Create a new results object with the same metadata + onlyResultsInDiff = results.NewCommandResults(scanResults.CmdType) + onlyResultsInDiff.CommandMetaData = scanResults.CommandMetaData + // Loop over the scan targets and filter out the results that are not in the diff + for _, target := range scanResults.Targets { + // Add scan target to the new results object with the same metadata and no results + filterTarget := onlyResultsInDiff.NewScanResults(target.ScanTarget) + filterTarget.Errors = target.Errors + // Go over the results and filter out the ones that are not in the diff + filterTarget.ScaResults = filterScaResultsNotInDiff(target.ScaResults, diffDescriptors) + filterTarget.JasResults = filterJasResultsNotInDiff(target.JasResults, changes) + } + return +} + +func getDescriptorsFromDiff(diffTargets []string) (descriptors []string) { + for _, target := range diffTargets { + descriptors = append(descriptors, target) + } + return +} + +// Filter SCA results that are not in the diff, if at least one SCA descriptor is in the diff, the target is in the diff +// TODO: when we can discover and match SCA issue to location at file level, we can improve filter capabilities +func filterScaResultsNotInDiff(scaResults *results.ScaScanResults, changedDescriptors []string) (filterResults *results.ScaScanResults) { + if len(changedDescriptors) == 0 { + log.Debug("No diff targets to filter SCA results") + return scaResults + } + log.Warn("Filtering SCA results based on diff is not fully supported yet, all SCA results at the file level are included if changed") + return scaResults +} + +func filterJasResultsNotInDiff(jasResults *results.JasScansResults, changes *gitutils.ChangesRelevantToScan) (filterResults *results.JasScansResults) { + return jasResults +} diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 81db8f8b..005996ba 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -72,7 +72,7 @@ func DetectGitInfo(wd string) (gitInfo *services.XscGitInfoContext, err error) { return gitManager.GetGitContext() } -func toAuditParams(params GitAuditParams) *sourceAudit.AuditParams { +func toAuditParams(params GitAuditParams, changes *gitutils.ChangesRelevantToScan) *sourceAudit.AuditParams { auditParams := sourceAudit.NewAuditParams() // Connection params auditParams.SetServerDetails(params.serverDetails).SetInsecureTls(params.serverDetails.InsecureTls).SetXrayVersion(params.xrayVersion).SetXscVersion(params.xscVersion) @@ -90,6 +90,9 @@ func toAuditParams(params GitAuditParams) *sourceAudit.AuditParams { log.Debug(fmt.Sprintf("Results context: %+v", resultContext)) // Scan params auditParams.SetThreads(params.threads).SetWorkingDirs([]string{params.repositoryLocalPath}).SetExclusions(params.exclusions).SetScansToPerform(params.scansToPerform) + if changedPaths := changes.GetChangedFilesPaths(); len(changedPaths) > 0 { + log.Debug(fmt.Sprintf("Diff targets: %v", changedPaths)) + } // Output params auditParams.SetOutputFormat(params.outputFormat) // Cmd information @@ -100,6 +103,15 @@ func toAuditParams(params GitAuditParams) *sourceAudit.AuditParams { } func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandResults) { + // Get diff targets to scan if needed + diffTargets, err := getDiffTargets(params) + if err != nil { + return results.NewCommandResults(utils.SourceCode).AddGeneralError(err, false) + } + if diffTargets != nil && !diffTargets.HasChanges() { + log.Warn("No changes detected in the diff, skipping the scan") + return results.NewCommandResults(utils.SourceCode) + } // Send scan started event event := xsc.CreateAnalyticsEvent(services.CliProduct, services.CliEventType, params.serverDetails) event.GitInfo = ¶ms.source @@ -112,13 +124,28 @@ func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandRes ) params.multiScanId = multiScanId params.startTime = startTime - // Run the scan - scanResults = sourceAudit.RunAudit(toAuditParams(params)) + // Run the scan and filter results not in diff + scanResults = filterResultsNotInDiff(sourceAudit.RunAudit(toAuditParams(params, diffTargets)), diffTargets) // Send scan ended event xsc.SendScanEndedWithResults(params.serverDetails, scanResults) return scanResults } +func getDiffTargets(params GitAuditParams) (changes *gitutils.ChangesRelevantToScan, err error) { + if params.diffTarget == "" { + return + } + log.Info(fmt.Sprintf("Diff mode: comparing against target '%s'", params.diffTarget)) + gitManager, err := gitutils.NewGitManager(params.repositoryLocalPath) + if err != nil { + return + } + if relevantChanges, err := gitManager.ScanRelevantDiff(params.diffTarget); err == nil { + changes = &relevantChanges + } + return +} + func (gaCmd *GitAuditCommand) getResultWriter(cmdResults *results.SecurityCommandResults) *output.ResultsWriter { var messages []string if !cmdResults.EntitledForJas { diff --git a/utils/gitutils/filediff.go b/utils/gitutils/filediff.go new file mode 100644 index 00000000..b09d15a9 --- /dev/null +++ b/utils/gitutils/filediff.go @@ -0,0 +1,135 @@ +package gitutils + +import ( + "fmt" + + goDiff "github.com/go-git/go-git/v5/plumbing/format/diff" + + "github.com/jfrog/gofrog/datastructures" + "github.com/jfrog/jfrog-client-go/utils/log" +) + +type ChangesRelevantToScan struct { + ChangedFiles []FileChanges + ChangedBinaries []string +} + +func (c ChangesRelevantToScan) HasChanges() bool { + return c.HasFileChanges() || c.HasBinaryChanges() +} + +func (c ChangesRelevantToScan) HasFileChanges() bool { + return len(c.ChangedFiles) > 0 +} + +func (c ChangesRelevantToScan) HasBinaryChanges() bool { + return len(c.ChangedBinaries) > 0 +} + +func (c ChangesRelevantToScan) GetChangedFilesPaths() (paths []string) { + for _, file := range c.ChangedFiles { + paths = append(paths, file.Path) + } + return +} + +func (c ChangesRelevantToScan) GetFileChanges(path string) (changes *FileChanges) { + for _, file := range c.ChangedFiles { + if file.Path == path { + return &file + } + } + return +} + +// FileChangeRanges represents the changes in the +type FileChanges struct { + Path string + Changes []Range +} + +func (f FileChanges) String() string { + return fmt.Sprintf("%s: %v", f.Path, f.Changes) +} + +func (f FileChanges) Contains(startRow, startCol, endRow, endCol int) bool { + for _, change := range f.Changes { + if change.Contains(startRow, startCol, endRow, endCol) { + return true + } + } + return false +} + +func (f FileChanges) Overlaps(startRow, startCol, endRow, endCol int) bool { + for _, change := range f.Changes { + if change.Overlaps(startRow, startCol, endRow, endCol) { + return true + } + } + return false +} + +type Range struct { + StartRow int + StartCol int + EndRow int + EndCol int +} + +func (r Range) String() string { + return fmt.Sprintf("%d:%d-%d:%d", r.StartRow, r.StartCol, r.EndRow, r.EndCol) +} + +// Contains checks if the range contains (fully) the given range +func (r Range) Contains(startRow, startCol, endRow, endCol int) bool { + return r.StartRow <= startRow && r.StartCol <= startCol && r.EndRow >= endRow && r.EndCol >= endCol +} + +// Overlaps checks if the range overlaps (partially or fully) the given range +func (r Range) Overlaps(startRow, startCol, endRow, endCol int) bool { + return r.StartRow < endRow && r.EndRow > startRow && r.StartCol < endCol && r.EndCol > startCol +} + +func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelevantToScan, err error) { + binariesChanged := datastructures.MakeSet[string]() + // Go over the file patches and detect the relevant changes + for _, filePatch := range filePatches { + from, to := filePatch.Files() + fromPath := "" + if from != nil { + fromPath = from.Path() + } + toPath := "" + if to != nil { + toPath = to.Path() + } + log.Debug(fmt.Sprintf("Checking Diff between: %s (from) and %s (to)", fromPath, toPath)) + // Get the relevant changes for the file + if filePatch.IsBinary() { + // Binary file, only file path is relevant + binariesChanged.Add(to.Path()) + continue + } + if to == nil { + // Deleted file, not relevant to scan + continue + } + // Get the relevant changes in the file, if new file (from is nil) all the content is relevant + if fileChanges := processFileChunksForRelevantChanges(filePatch.Chunks(), from == nil); /*len(fileChanges) > 0*/ true { + changes.ChangedFiles = append(changes.ChangedFiles, FileChanges{Path: to.Path(), Changes: fileChanges}) + } + } + changes.ChangedBinaries = binariesChanged.ToSlice() + return +} + +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk, isNewFile bool) (changes []Range) { + // SARIF locations start at 1 + // row, col := 1, 1 + for _, diffChunk := range fileChunks { + chunkContent := diffChunk.Content() + log.Debug(fmt.Sprintf("Chunk (type = %d): \"%s\"", diffChunk.Type(), chunkContent)) + } + return +} diff --git a/utils/gitutils/filediff_test.go b/utils/gitutils/filediff_test.go new file mode 100644 index 00000000..41d6bc57 --- /dev/null +++ b/utils/gitutils/filediff_test.go @@ -0,0 +1,4 @@ +package gitutils + +import ( +) diff --git a/utils/gitutils/gitmanager.go b/utils/gitutils/gitmanager.go index 10d3d4fd..a1db4f01 100644 --- a/utils/gitutils/gitmanager.go +++ b/utils/gitutils/gitmanager.go @@ -5,7 +5,8 @@ import ( "strings" goGit "github.com/go-git/go-git/v5" - + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/format/diff" "github.com/jfrog/jfrog-client-go/utils/log" "github.com/jfrog/jfrog-client-go/xsc/services" ) @@ -122,32 +123,6 @@ func (gm *GitManager) IsClean() (bool, error) { return status.IsClean(), nil } -func (gm *GitManager) Diff(reference string) (err error) { - // Get the current branch - currentBranch, err := gm.localGitRepository.Head() - if err != nil { - return - } - // Get the commit object of the current branch - currentCommit, err := gm.localGitRepository.CommitObject(currentBranch.Hash()) - if err != nil { - return - } - // Get the commit object of the reference - referenceCommit, err := gm.localGitRepository.CommitObject(currentCommit.Hash) - if err != nil { - return - } - // Get the diff between the current branch and the reference - diff, err := currentCommit.Patch(referenceCommit) - if err != nil { - return - } - // Print the diff - fmt.Println(diff) - return -} - // Detect git information func (gm *GitManager) GetGitContext() (gitInfo *services.XscGitInfoContext, err error) { remoteUrl, err := getRemoteUrl(gm.remote) @@ -200,6 +175,39 @@ func getRemoteUrl(remote *goGit.Remote) (remoteUrl string, err error) { return remote.Config().URLs[0], nil } +func (gm *GitManager) Diff(reference string) (changes []diff.FilePatch, err error) { + // Get the current branch + currentBranch, err := gm.localGitRepository.Head() + if err != nil { + return + } + // Get the commit object of the current branch + currentCommit, err := gm.localGitRepository.CommitObject(currentBranch.Hash()) + if err != nil { + return + } + // Get the commit object of the reference + referenceCommit, err := gm.localGitRepository.CommitObject(plumbing.NewHash(reference)) + if err != nil { + return + } + // Get the diff between the current branch and the reference + diff, err := currentCommit.Patch(referenceCommit) + if err != nil { + return + } + changes = diff.FilePatches() + return +} + +func (gm *GitManager) ScanRelevantDiff(reference string) (changes ChangesRelevantToScan, err error) { + diff, err := gm.Diff(reference) + if err != nil { + return + } + return detectRelevantChanges(diff) +} + // Normalize the URL by removing protocol prefix and any trailing ".git" func normalizeGitUrl(url string) string { // jfrog-ignore - false positive, not used for communication diff --git a/utils/gitutils/gitmanager_test.go b/utils/gitutils/gitmanager_test.go index f0b5f4b7..a3038ce6 100644 --- a/utils/gitutils/gitmanager_test.go +++ b/utils/gitutils/gitmanager_test.go @@ -14,9 +14,9 @@ import ( "github.com/jfrog/jfrog-client-go/xsc/services" ) -func TestGetGitContext(t *testing.T) { - basePath := filepath.Join("..", "..", "tests", "testdata", "git", "projects") +var testDir = filepath.Join("..", "..", "tests", "testdata", "git", "projects") +func TestGetGitContext(t *testing.T) { testCases := []struct { name string testProjectZipDirPath string @@ -26,11 +26,11 @@ func TestGetGitContext(t *testing.T) { { name: "No Git Info", NoDotGitFolder: true, - testProjectZipDirPath: filepath.Join(basePath, "nogit"), + testProjectZipDirPath: filepath.Join(testDir, "nogit"), }, { name: "Clean Project (after clone)", - testProjectZipDirPath: filepath.Join(basePath, "clean"), + testProjectZipDirPath: filepath.Join(testDir, "clean"), gitInfo: &services.XscGitInfoContext{ GitRepoHttpsCloneUrl: "https://github.com/attiasas/test-security-git.git", GitRepoName: "test-security-git", @@ -44,7 +44,7 @@ func TestGetGitContext(t *testing.T) { }, { name: "Self-Hosted Git Project (and SSO credentials)", - testProjectZipDirPath: filepath.Join(basePath, "selfhosted"), + testProjectZipDirPath: filepath.Join(testDir, "selfhosted"), gitInfo: &services.XscGitInfoContext{ GitRepoHttpsCloneUrl: "ssh://git@git.jfrog.info/~assafa/test-security-git.git", GitRepoName: "test-security-git", @@ -58,7 +58,7 @@ func TestGetGitContext(t *testing.T) { }, { name: "Gitlab Project (group tree structure)", - testProjectZipDirPath: filepath.Join(basePath, "gitlab"), + testProjectZipDirPath: filepath.Join(testDir, "gitlab"), gitInfo: &services.XscGitInfoContext{ GitRepoHttpsCloneUrl: "https://gitlab.com/attiasas/test-group/test-security-git.git", GitRepoName: "test-security-git", @@ -72,7 +72,7 @@ func TestGetGitContext(t *testing.T) { }, { name: "Forked Project (multiple remotes)", - testProjectZipDirPath: filepath.Join(basePath, "forked"), + testProjectZipDirPath: filepath.Join(testDir, "forked"), gitInfo: &services.XscGitInfoContext{ GitRepoHttpsCloneUrl: "https://github.com/attiasas/test-security-git.git", GitRepoName: "test-security-git", @@ -87,7 +87,7 @@ func TestGetGitContext(t *testing.T) { // Not supported yet { name: "Dirty Project (with uncommitted changes)", - testProjectZipDirPath: filepath.Join(basePath, "dirty"), + testProjectZipDirPath: filepath.Join(testDir, "dirty"), // gitInfo: &services.XscGitInfoContext{ // GitRepoHttpsCloneUrl: "https://github.com/attiasas/test-security-git.git", // GitRepoName: "test-security-git", @@ -210,3 +210,36 @@ func TestGetGitProject(t *testing.T) { }) } } + + +func TestDetectRelevantChanges(t *testing.T) { + testCases := []struct { + name string + testZipDir string + targetReference string + expectedChanges ChangesRelevantToScan + }{ + { + name: "No relevant changes", + testZipDir: "clean", + targetReference: "861b7aff93eeb9be4806f1d9cc668e3d702d90b6", + expectedChanges: ChangesRelevantToScan{}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + // Create the project from the zip + projectPath, cleanUpProject := securityTestUtils.CreateTestProjectFromZip(t, filepath.Join(testDir, "clean")) + defer cleanUpProject() + // Prepare the git manager at the project path + gitManager, err := NewGitManager(projectPath) + require.NoError(t, err) + // Get the relevant changes + changes, err := gitManager.ScanRelevantDiff(testCase.targetReference) + require.NoError(t, err) + // Assert the expected changes + assert.Equal(t, testCase.expectedChanges, changes) + }) + } +} \ No newline at end of file diff --git a/utils/results/output/securityJobSummary_test.go b/utils/results/output/securityJobSummary_test.go index 4ec435b5..0928da6f 100644 --- a/utils/results/output/securityJobSummary_test.go +++ b/utils/results/output/securityJobSummary_test.go @@ -73,7 +73,7 @@ func TestSaveSarifOutputOnlyForJasEntitled(t *testing.T) { } func createDummyJasResult(entitled bool) *results.SecurityCommandResults { - return &results.SecurityCommandResults{EntitledForJas: entitled} + return &results.SecurityCommandResults{CommandMetaData: results.CommandMetaData{EntitledForJas: entitled}} } func hasFilesInDir(t *testing.T, dir string) bool { diff --git a/utils/results/results.go b/utils/results/results.go index 0c3f2a6f..6ac4e2db 100644 --- a/utils/results/results.go +++ b/utils/results/results.go @@ -19,7 +19,15 @@ import ( // SecurityCommandResults is a struct that holds the results of a security scan/audit command. type SecurityCommandResults struct { - // General fields describing the command metadata + CommandMetaData + // Results for each target in the command + Targets []*TargetResults `json:"targets"` + targetsMutex sync.Mutex `json:"-"` + errorsMutex sync.Mutex `json:"-"` +} + +// General fields describing the command metadata +type CommandMetaData struct { XrayVersion string `json:"xray_version"` XscVersion string `json:"xsc_version,omitempty"` EntitledForJas bool `json:"jas_entitled"` @@ -29,12 +37,8 @@ type SecurityCommandResults struct { StartTime time.Time `json:"start_time"` // MultiScanId is a unique identifier that is used to group multiple scans together. MultiScanId string `json:"multi_scan_id,omitempty"` - // Results for each target in the command - Targets []*TargetResults `json:"targets"` - targetsMutex sync.Mutex `json:"-"` // GeneralError that occurred during the command execution - GeneralError error `json:"general_error,omitempty"` - errorsMutex sync.Mutex `json:"-"` + GeneralError error `json:"general_error,omitempty"` } // We have three types of results: vulnerabilities, violations and licenses. @@ -130,7 +134,7 @@ func (st ScanTarget) String() (str string) { } func NewCommandResults(cmdType utils.CommandType) *SecurityCommandResults { - return &SecurityCommandResults{CmdType: cmdType, targetsMutex: sync.Mutex{}, errorsMutex: sync.Mutex{}} + return &SecurityCommandResults{CommandMetaData: CommandMetaData{CmdType: cmdType}, targetsMutex: sync.Mutex{}, errorsMutex: sync.Mutex{}} } func (r *SecurityCommandResults) SetStartTime(startTime time.Time) *SecurityCommandResults { @@ -288,7 +292,6 @@ func (r *SecurityCommandResults) NewScanResults(target ScanTarget) *TargetResult if r.EntitledForJas { targetResults.JasResults = &JasScansResults{JasVulnerabilities: JasScanResults{}, JasViolations: JasScanResults{}} } - r.targetsMutex.Lock() r.Targets = append(r.Targets, targetResults) r.targetsMutex.Unlock() diff --git a/utils/xsc/analyticsmetrics_test.go b/utils/xsc/analyticsmetrics_test.go index 0e1bf10c..63e6958a 100644 --- a/utils/xsc/analyticsmetrics_test.go +++ b/utils/xsc/analyticsmetrics_test.go @@ -149,7 +149,7 @@ func TestCreateFinalizedEvent(t *testing.T) { }{ { name: "No audit results", - auditResults: &results.SecurityCommandResults{MultiScanId: "msi", StartTime: time}, + auditResults: &results.SecurityCommandResults{CommandMetaData: results.CommandMetaData{MultiScanId: "msi", StartTime: time}}, expected: xscservices.XscAnalyticsGeneralEventFinalize{ XscAnalyticsBasicGeneralEvent: xscservices.XscAnalyticsBasicGeneralEvent{EventStatus: xscservices.Completed}, }, @@ -178,7 +178,7 @@ func TestCreateFinalizedEvent(t *testing.T) { }, { name: "Scan failed no findings.", - auditResults: &results.SecurityCommandResults{MultiScanId: "msi", StartTime: time, Targets: []*results.TargetResults{{Errors: []error{errors.New("an error")}}}}, + auditResults: &results.SecurityCommandResults{CommandMetaData: results.CommandMetaData{MultiScanId: "msi", StartTime: time}, Targets: []*results.TargetResults{{Errors: []error{errors.New("an error")}}}}, expected: xscservices.XscAnalyticsGeneralEventFinalize{ XscAnalyticsBasicGeneralEvent: xscservices.XscAnalyticsBasicGeneralEvent{TotalFindings: 0, EventStatus: xscservices.Failed}, }, From 69882ca26d4f93490d2b18aac4a05a5663e3ce3a Mon Sep 17 00:00:00 2001 From: attiasas Date: Tue, 4 Feb 2025 11:55:25 +0200 Subject: [PATCH 06/14] fix static, format --- commands/git/audit/diffmode.go | 5 +---- commands/git/audit/gitaudit.go | 2 +- utils/gitutils/filediff.go | 2 +- utils/gitutils/filediff_test.go | 3 +-- utils/gitutils/gitmanager_test.go | 11 +++++------ 5 files changed, 9 insertions(+), 14 deletions(-) diff --git a/commands/git/audit/diffmode.go b/commands/git/audit/diffmode.go index 845f1fae..41ea53a3 100644 --- a/commands/git/audit/diffmode.go +++ b/commands/git/audit/diffmode.go @@ -28,10 +28,7 @@ func filterResultsNotInDiff(scanResults *results.SecurityCommandResults, changes } func getDescriptorsFromDiff(diffTargets []string) (descriptors []string) { - for _, target := range diffTargets { - descriptors = append(descriptors, target) - } - return + return append(descriptors, diffTargets...) } // Filter SCA results that are not in the diff, if at least one SCA descriptor is in the diff, the target is in the diff diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 005996ba..8d0692f4 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -143,7 +143,7 @@ func getDiffTargets(params GitAuditParams) (changes *gitutils.ChangesRelevantToS if relevantChanges, err := gitManager.ScanRelevantDiff(params.diffTarget); err == nil { changes = &relevantChanges } - return + return } func (gaCmd *GitAuditCommand) getResultWriter(cmdResults *results.SecurityCommandResults) *output.ResultsWriter { diff --git a/utils/gitutils/filediff.go b/utils/gitutils/filediff.go index b09d15a9..49bd02f5 100644 --- a/utils/gitutils/filediff.go +++ b/utils/gitutils/filediff.go @@ -124,7 +124,7 @@ func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelev return } -func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk, isNewFile bool) (changes []Range) { +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (changes []Range) { // SARIF locations start at 1 // row, col := 1, 1 for _, diffChunk := range fileChunks { diff --git a/utils/gitutils/filediff_test.go b/utils/gitutils/filediff_test.go index 41d6bc57..17ac91ab 100644 --- a/utils/gitutils/filediff_test.go +++ b/utils/gitutils/filediff_test.go @@ -1,4 +1,3 @@ package gitutils -import ( -) +import () diff --git a/utils/gitutils/gitmanager_test.go b/utils/gitutils/gitmanager_test.go index a3038ce6..a7351d91 100644 --- a/utils/gitutils/gitmanager_test.go +++ b/utils/gitutils/gitmanager_test.go @@ -211,17 +211,16 @@ func TestGetGitProject(t *testing.T) { } } - func TestDetectRelevantChanges(t *testing.T) { testCases := []struct { - name string - testZipDir string + name string + testZipDir string targetReference string expectedChanges ChangesRelevantToScan }{ { - name: "No relevant changes", - testZipDir: "clean", + name: "No relevant changes", + testZipDir: "clean", targetReference: "861b7aff93eeb9be4806f1d9cc668e3d702d90b6", expectedChanges: ChangesRelevantToScan{}, }, @@ -242,4 +241,4 @@ func TestDetectRelevantChanges(t *testing.T) { assert.Equal(t, testCase.expectedChanges, changes) }) } -} \ No newline at end of file +} From 427c651d2a1f5c05b78d99b35c98d219c36521e7 Mon Sep 17 00:00:00 2001 From: attiasas Date: Thu, 6 Feb 2025 09:35:44 +0200 Subject: [PATCH 07/14] Add option for CommonAncestor scan --- cli/docs/flags.go | 6 +++++ cli/gitcommands.go | 1 + commands/git/audit/gitaudit.go | 8 +++++- commands/git/audit/gitauditparams.go | 10 ++++++-- utils/gitutils/filediff.go | 38 ++++++++++++++++++++++++++-- utils/gitutils/gitmanager.go | 31 +++++++++++++++++++++++ 6 files changed, 89 insertions(+), 5 deletions(-) diff --git a/cli/docs/flags.go b/cli/docs/flags.go index 08a4830c..ec428ef4 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -136,6 +136,9 @@ const ( RepoName = "repo-name" Months = "months" DetailedSummary = "detailed-summary" + + // Unique git audit flags + CommonAncestor = "common-ancestor" ) // Mapping between security commands (key) and their flags (key). @@ -167,6 +170,7 @@ var commandFlags = map[string][]string{ // Violations params Project, Watches, ScanVuln, Fail, // Scan params + CommonAncestor, Threads, ExclusionsAudit, Sca, Iac, Sast, Secrets, WithoutCA, SecretValidation, // Output params @@ -281,6 +285,8 @@ var flagsMap = map[string]components.Flag{ WithoutCA: components.NewBoolFlag(WithoutCA, fmt.Sprintf("Selective scanners mode: Disable Contextual Analysis scanner after SCA. Relevant only with --%s flag.", Sca)), SecretValidation: components.NewBoolFlag(SecretValidation, fmt.Sprintf("Selective scanners mode: Triggers token validation on found secrets. Relevant only with --%s flag.", Secrets)), + CommonAncestor: components.NewBoolFlag(CommonAncestor, "Set to true to compare against the common ancestor of the current and the target commit.", components.WithBoolDefaultValue(true)), + // Git flags InputFile: components.NewStringFlag(InputFile, "Path to an input file in YAML format contains multiple git providers. With this option, all other scm flags will be ignored and only git servers mentioned in the file will be examined.."), ScmType: components.NewStringFlag(ScmType, fmt.Sprintf("SCM type. Possible values are: %s.", contributors.NewScmType().GetValidScmTypeString()), components.SetMandatory()), diff --git a/cli/gitcommands.go b/cli/gitcommands.go index 4ada37ea..0b903e06 100644 --- a/cli/gitcommands.go +++ b/cli/gitcommands.go @@ -46,6 +46,7 @@ func GitAuditCmd(c *components.Context) error { if len(c.Arguments) > 0 { // Diff mode gitAuditCmd.SetDiffTarget(c.Arguments[0]) + gitAuditCmd.SetUseCommonAncestorAsTarget(c.GetBoolFlagValue(flags.CommonAncestor)) } // Set connection params diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 8d0692f4..8575cb59 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -135,8 +135,14 @@ func getDiffTargets(params GitAuditParams) (changes *gitutils.ChangesRelevantToS if params.diffTarget == "" { return } - log.Info(fmt.Sprintf("Diff mode: comparing against target '%s'", params.diffTarget)) gitManager, err := gitutils.NewGitManager(params.repositoryLocalPath) + if params.calculateCommonAncestorAsTarget { + log.Info(fmt.Sprintf("Diff mode: check for common ancestor with target '%s'", params.diffTarget)) + if params.diffTarget, err = gitManager.GetCommonAncestor(params.diffTarget); err != nil { + return + } + } + log.Info(fmt.Sprintf("Diff mode: comparing against target '%s'", params.diffTarget)) if err != nil { return } diff --git a/commands/git/audit/gitauditparams.go b/commands/git/audit/gitauditparams.go index 02b472d4..d94e8a38 100644 --- a/commands/git/audit/gitauditparams.go +++ b/commands/git/audit/gitauditparams.go @@ -12,8 +12,9 @@ import ( type GitAuditParams struct { // Git Params - source services.XscGitInfoContext - diffTarget string + source services.XscGitInfoContext + diffTarget string + calculateCommonAncestorAsTarget bool // Connection params serverDetails *config.ServerDetails // Violations params @@ -43,6 +44,11 @@ func (gap *GitAuditParams) SetDiffTarget(diffTarget string) *GitAuditParams { return gap } +func (gap *GitAuditParams) SetUseCommonAncestorAsTarget(commonAncesto bool) *GitAuditParams { + gap.calculateCommonAncestorAsTarget = commonAncesto + return gap +} + func (gap *GitAuditParams) SetServerDetails(serverDetails *config.ServerDetails) *GitAuditParams { gap.serverDetails = serverDetails return gap diff --git a/utils/gitutils/filediff.go b/utils/gitutils/filediff.go index 49bd02f5..b8b6c166 100644 --- a/utils/gitutils/filediff.go +++ b/utils/gitutils/filediff.go @@ -2,6 +2,7 @@ package gitutils import ( "fmt" + // "strings" goDiff "github.com/go-git/go-git/v5/plumbing/format/diff" @@ -124,12 +125,45 @@ func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelev return } -func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (changes []Range) { +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (relevantChanges []Range) { // SARIF locations start at 1 - // row, col := 1, 1 + row, col := 1, 1 for _, diffChunk := range fileChunks { chunkContent := diffChunk.Content() log.Debug(fmt.Sprintf("Chunk (type = %d): \"%s\"", diffChunk.Type(), chunkContent)) + switch diffChunk.Type() { + case goDiff.Add: + // Added content + // Add the range of the added content + relevantChanges = append(relevantChanges, Range{StartRow: row, StartCol: col, EndRow: row, EndCol: col + len(chunkContent)}) + // Move the cursor to the end of the added content + + case goDiff.Delete: + // Deleted content + // Move the cursor to the end of the deleted content + + case goDiff.Equal: + // Unchanged content + // Move the cursor to the end of the unchanged content + + } + } + return +} + +// func createRangeAtChunk(cursorRow, cursorCol int, chunk string) Range { +// return Range{StartRow: row, StartCol: col, EndRow: row, EndCol: col + len(chunk)} +// } + +func getCursorNewPosition(cursorRow, cursorCol int, chunk string) (newRow, newCol int) { + newRow, newCol = cursorRow, cursorCol + for _, char := range chunk { + if char == '\n' { + newRow++ + newCol = 1 + } else { + newCol++ + } } return } diff --git a/utils/gitutils/gitmanager.go b/utils/gitutils/gitmanager.go index a1db4f01..0711550e 100644 --- a/utils/gitutils/gitmanager.go +++ b/utils/gitutils/gitmanager.go @@ -175,6 +175,37 @@ func getRemoteUrl(remote *goGit.Remote) (remoteUrl string, err error) { return remote.Config().URLs[0], nil } +func (gm *GitManager) GetCommonAncestor(reference string) (ancestorCommit string, err error) { + // Get the current branch + currentBranch, err := gm.localGitRepository.Head() + if err != nil { + return + } + // Get the commit object of the current branch + currentCommit, err := gm.localGitRepository.CommitObject(currentBranch.Hash()) + if err != nil { + return + } + // Get the commit object of the reference + referenceCommit, err := gm.localGitRepository.CommitObject(plumbing.NewHash(reference)) + if err != nil { + return + } + // Get the common ancestor commit + log.Debug(fmt.Sprintf("Finding common ancestor between %s and %s", currentBranch.Name().Short(), reference)) + ancestorCommitHash, err := currentCommit.MergeBase(referenceCommit) + if err != nil { + return + } + if len(ancestorCommitHash) == 0 { + err = fmt.Errorf("no common ancestor found between %s and %s", currentBranch.Name().Short(), reference) + } else if len(ancestorCommitHash) > 1 { + err = fmt.Errorf("multiple common ancestors found between %s and %s", currentBranch.Name().Short(), reference) + } + ancestorCommit = ancestorCommitHash[0].Hash.String() + return +} + func (gm *GitManager) Diff(reference string) (changes []diff.FilePatch, err error) { // Get the current branch currentBranch, err := gm.localGitRepository.Head() From d90936883bb3cb5f7403708be3939eb0a2321622 Mon Sep 17 00:00:00 2001 From: attiasas Date: Thu, 6 Feb 2025 11:34:08 +0200 Subject: [PATCH 08/14] add test --- utils/gitutils/gitmanager_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/utils/gitutils/gitmanager_test.go b/utils/gitutils/gitmanager_test.go index a7351d91..5579d8a4 100644 --- a/utils/gitutils/gitmanager_test.go +++ b/utils/gitutils/gitmanager_test.go @@ -219,11 +219,17 @@ func TestDetectRelevantChanges(t *testing.T) { expectedChanges ChangesRelevantToScan }{ { - name: "No relevant changes", + name: "No relevant changes (commit reference)", testZipDir: "clean", targetReference: "861b7aff93eeb9be4806f1d9cc668e3d702d90b6", expectedChanges: ChangesRelevantToScan{}, }, + { + name: "No relevant changes (branch reference)", + testZipDir: "clean", + targetReference: "main", + expectedChanges: ChangesRelevantToScan{}, + }, } for _, testCase := range testCases { From 038f7770784d31a1a7a99bd5c3070c91d20eb300 Mon Sep 17 00:00:00 2001 From: attiasas Date: Thu, 20 Feb 2025 12:52:37 +0200 Subject: [PATCH 09/14] merge fix --- commands/git/audit/diffmode.go | 6 ++-- commands/git/audit/gitaudit.go | 6 ++-- utils/scm/filediff.go | 2 +- utils/scm/filediff_test.go | 2 +- utils/scm/gitmanager.go | 59 ---------------------------------- utils/scm/gitmanager_test.go | 6 ++-- 6 files changed, 11 insertions(+), 70 deletions(-) diff --git a/commands/git/audit/diffmode.go b/commands/git/audit/diffmode.go index 41ea53a3..1e1d4671 100644 --- a/commands/git/audit/diffmode.go +++ b/commands/git/audit/diffmode.go @@ -1,12 +1,12 @@ package audit import ( - "github.com/jfrog/jfrog-cli-security/utils/gitutils" "github.com/jfrog/jfrog-cli-security/utils/results" + "github.com/jfrog/jfrog-cli-security/utils/scm" "github.com/jfrog/jfrog-client-go/utils/log" ) -func filterResultsNotInDiff(scanResults *results.SecurityCommandResults, changes *gitutils.ChangesRelevantToScan) (onlyResultsInDiff *results.SecurityCommandResults) { +func filterResultsNotInDiff(scanResults *results.SecurityCommandResults, changes *scm.ChangesRelevantToScan) (onlyResultsInDiff *results.SecurityCommandResults) { if changes == nil || !changes.HasFileChanges() { log.Debug("No diff targets to filter results") return scanResults @@ -42,6 +42,6 @@ func filterScaResultsNotInDiff(scaResults *results.ScaScanResults, changedDescri return scaResults } -func filterJasResultsNotInDiff(jasResults *results.JasScansResults, changes *gitutils.ChangesRelevantToScan) (filterResults *results.JasScansResults) { +func filterJasResultsNotInDiff(jasResults *results.JasScansResults, changes *scm.ChangesRelevantToScan) (filterResults *results.JasScansResults) { return jasResults } diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 6870deb2..0d91e8eb 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -72,7 +72,7 @@ func DetectGitInfo(wd string) (gitInfo *services.XscGitInfoContext, err error) { return scmManager.GetSourceControlContext() } -func toAuditParams(params GitAuditParams, changes *gitutils.ChangesRelevantToScan) *sourceAudit.AuditParams { +func toAuditParams(params GitAuditParams, changes *scm.ChangesRelevantToScan) *sourceAudit.AuditParams { auditParams := sourceAudit.NewAuditParams() // Connection params auditParams.SetServerDetails(params.serverDetails).SetInsecureTls(params.serverDetails.InsecureTls).SetXrayVersion(params.xrayVersion).SetXscVersion(params.xscVersion) @@ -131,11 +131,11 @@ func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandRes return scanResults } -func getDiffTargets(params GitAuditParams) (changes *gitutils.ChangesRelevantToScan, err error) { +func getDiffTargets(params GitAuditParams) (changes *scm.ChangesRelevantToScan, err error) { if params.diffTarget == "" { return } - gitManager, err := gitutils.NewGitManager(params.repositoryLocalPath) + gitManager, err := scm.NewGitManager(params.repositoryLocalPath) if params.calculateCommonAncestorAsTarget { log.Info(fmt.Sprintf("Diff mode: check for common ancestor with target '%s'", params.diffTarget)) if params.diffTarget, err = gitManager.GetCommonAncestor(params.diffTarget); err != nil { diff --git a/utils/scm/filediff.go b/utils/scm/filediff.go index b8b6c166..28a0d694 100644 --- a/utils/scm/filediff.go +++ b/utils/scm/filediff.go @@ -1,4 +1,4 @@ -package gitutils +package scm import ( "fmt" diff --git a/utils/scm/filediff_test.go b/utils/scm/filediff_test.go index 17ac91ab..4a507ccb 100644 --- a/utils/scm/filediff_test.go +++ b/utils/scm/filediff_test.go @@ -1,3 +1,3 @@ -package gitutils +package scm import () diff --git a/utils/scm/gitmanager.go b/utils/scm/gitmanager.go index cae56bd0..ef6439a2 100644 --- a/utils/scm/gitmanager.go +++ b/utils/scm/gitmanager.go @@ -151,7 +151,6 @@ func getRemoteUrl(remote *goGit.Remote) (remoteUrl string, err error) { } return remote.Config().URLs[0], nil } -<<<<<<< HEAD:utils/gitutils/gitmanager.go func (gm *GitManager) GetCommonAncestor(reference string) (ancestorCommit string, err error) { // Get the current branch @@ -216,61 +215,3 @@ func (gm *GitManager) ScanRelevantDiff(reference string) (changes ChangesRelevan } return detectRelevantChanges(diff) } - -// Normalize the URL by removing protocol prefix and any trailing ".git" -func normalizeGitUrl(url string) string { - // jfrog-ignore - false positive, not used for communication - url = strings.TrimPrefix(url, "http://") - url = strings.TrimPrefix(url, "https://") - url = strings.TrimPrefix(url, "ssh://") - return strings.TrimSuffix(url, ".git") -} - -func getGitRepoName(url string) string { - urlParts := strings.Split(normalizeGitUrl(url), "/") - return urlParts[len(urlParts)-1] -} - -func getGitProject(url string) string { - // First part after base Url is the owner or the organization name. - urlParts := strings.Split(normalizeGitUrl(url), "/") - if len(urlParts) < 2 { - log.Debug(fmt.Sprintf("Failed to get project name from URL: %s", url)) - return "" - } - if len(urlParts) > 2 && urlParts[1] == "scm" { - // In BB https clone url looks like this: https://git.id.info/scm/repo-name/repo-name.git --> ['git.id.info', 'scm', 'repo-name', 'repo-name'] - return urlParts[2] - } - return urlParts[1] -} - -func getGitProvider(url string) GitProvider { - if strings.Contains(url, Github.String()) { - return Github - } - if strings.Contains(url, Gitlab.String()) { - return Gitlab - } - if isBitbucketProvider(url) { - return Bitbucket - } - if strings.Contains(url, Azure.String()) { - return Azure - } - // Unknown for self-hosted git providers - log.Debug(fmt.Sprintf("Unknown git provider for URL: %s", url)) - return Unknown -} - -func isBitbucketProvider(url string) bool { - if urlParts := strings.Split(normalizeGitUrl(url), "/"); len(urlParts) > 2 && urlParts[1] == "scm" { - return true - } - if projectName := getGitProject(url); strings.Contains(projectName, "~") { - return true - } - return strings.Contains(url, Bitbucket.String()) -} -======= ->>>>>>> upstream/dev:utils/scm/gitmanager.go diff --git a/utils/scm/gitmanager_test.go b/utils/scm/gitmanager_test.go index da025f21..08f456a2 100644 --- a/utils/scm/gitmanager_test.go +++ b/utils/scm/gitmanager_test.go @@ -72,7 +72,7 @@ func TestGetGitContext(t *testing.T) { }, { name: "Gerrit Project (no owner)", - testProjectZipDirPath: filepath.Join(basePath, "gerrit"), + testProjectZipDirPath: filepath.Join(testDir, "gerrit"), gitInfo: &services.XscGitInfoContext{ GitRepoHttpsCloneUrl: "https://gerrit.googlesource.com/git-repo", GitRepoName: "git-repo", @@ -269,8 +269,8 @@ func TestDetectRelevantChanges(t *testing.T) { expectedChanges: ChangesRelevantToScan{}, }, { - name: "No relevant changes (branch reference)", - testZipDir: "clean", + name: "No relevant changes (branch reference)", + testZipDir: "clean", targetReference: "main", expectedChanges: ChangesRelevantToScan{}, }, From e3b5bf1ab695859873d465e0d10b67f670b729ea Mon Sep 17 00:00:00 2001 From: attiasas Date: Thu, 20 Feb 2025 13:04:06 +0200 Subject: [PATCH 10/14] start to connect audit to diff --- commands/audit/auditparams.go | 11 +++++++++++ commands/git/audit/gitaudit.go | 1 + utils/scm/filediff.go | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/commands/audit/auditparams.go b/commands/audit/auditparams.go index 90dc3678..23c2276b 100644 --- a/commands/audit/auditparams.go +++ b/commands/audit/auditparams.go @@ -25,6 +25,8 @@ type AuditParams struct { configProfile *xscservices.ConfigProfile scanResultsOutputDir string startTime time.Time + // Diff mode, scan only the files affected by the diff. + filesToScan []string } func NewAuditParams() *AuditParams { @@ -133,3 +135,12 @@ func (params *AuditParams) createXrayGraphScanParams() *services.XrayGraphScanPa ScanType: services.Dependency, } } + +func (params *AuditParams) SetFilesToScan(filesToScan []string) *AuditParams { + params.filesToScan = filesToScan + return params +} + +func (params *AuditParams) FilesToScan() []string { + return params.filesToScan +} \ No newline at end of file diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 0d91e8eb..16a686d3 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -92,6 +92,7 @@ func toAuditParams(params GitAuditParams, changes *scm.ChangesRelevantToScan) *s auditParams.SetThreads(params.threads).SetWorkingDirs([]string{params.repositoryLocalPath}).SetExclusions(params.exclusions).SetScansToPerform(params.scansToPerform) if changedPaths := changes.GetChangedFilesPaths(); len(changedPaths) > 0 { log.Debug(fmt.Sprintf("Diff targets: %v", changedPaths)) + auditParams.SetFilesToScan(changedPaths) } // Output params auditParams.SetOutputFormat(params.outputFormat) diff --git a/utils/scm/filediff.go b/utils/scm/filediff.go index 28a0d694..1013490c 100644 --- a/utils/scm/filediff.go +++ b/utils/scm/filediff.go @@ -125,7 +125,7 @@ func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelev return } -func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (relevantChanges []Range) { +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk, /*isNewFile*/_ bool) (relevantChanges []Range) { // SARIF locations start at 1 row, col := 1, 1 for _, diffChunk := range fileChunks { From aed1d146ea39d4b072ec39db378907497f6568f5 Mon Sep 17 00:00:00 2001 From: attiasas Date: Mon, 24 Feb 2025 10:47:44 +0200 Subject: [PATCH 11/14] add partial scan for sca --- commands/audit/scarunner.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 33a6e185..58325adc 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -92,6 +92,14 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner log.Warn(fmt.Sprintf("Couldn't determine a package manager or build tool used by this project. Skipping the SCA scan in '%s'...", targetResult.Target)) continue } + if partialScan, filterDescriptors := getTargetDescriptorsToScan(targetResult.ScaResults, auditParams.filesToScan); partialScan { + if len(filterDescriptors) == 0 { + log.Info(fmt.Sprintf("No changed descriptors found for %s. Skipping SCA scan...", targetResult.Target)) + continue + } else { + log.Info(fmt.Sprintf("'%s' SCA scan will be performed on the following descriptors: %v", targetResult.Target, filterDescriptors)) + } + } // Get the dependency tree for the technology in the working directory. treeResult, bdtErr := buildDependencyTree(targetResult, auditParams) if bdtErr != nil { @@ -103,6 +111,10 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner _ = targetResult.AddTargetError(fmt.Errorf("failed to build dependency tree: %s", bdtErr.Error()), auditParams.AllowPartialResults()) continue } + // TODO: get dependency from the changes and filter tree only to contain the changed dependencies + // Filter the dependency tree to contain only the changed dependencies. + + // Create sca scan task auditParallelRunner.ScaScansWg.Add(1) // defer auditParallelRunner.ScaScansWg.Done() @@ -117,6 +129,20 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner return } +func getTargetDescriptorsToScan(scaResults *results.ScaScanResults, filesToScan []string) (bool, []string) { + if len(filesToScan) == 0 { + return false, scaResults.Descriptors + } + var filteredDescriptors []string + for _, descriptor := range scaResults.Descriptors { + if slices.Contains(filesToScan, descriptor) { + filteredDescriptors = append(filteredDescriptors, descriptor) + } + } + return true, filteredDescriptors + +} + func getRequestedDescriptors(params *AuditParams) map[techutils.Technology][]string { requestedDescriptors := map[techutils.Technology][]string{} if params.PipRequirementsFile() != "" { From 936aea8817a801141ebd6147e9607d7447626bef Mon Sep 17 00:00:00 2001 From: attiasas Date: Mon, 24 Feb 2025 11:09:39 +0200 Subject: [PATCH 12/14] done partial sca scan - all or nothing --- commands/audit/auditparams.go | 2 +- commands/audit/scarunner.go | 21 ++++++++++++--------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/commands/audit/auditparams.go b/commands/audit/auditparams.go index 23c2276b..247fbd6f 100644 --- a/commands/audit/auditparams.go +++ b/commands/audit/auditparams.go @@ -143,4 +143,4 @@ func (params *AuditParams) SetFilesToScan(filesToScan []string) *AuditParams { func (params *AuditParams) FilesToScan() []string { return params.filesToScan -} \ No newline at end of file +} diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 58325adc..8febcb73 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -4,6 +4,8 @@ import ( "encoding/json" "errors" "fmt" + "strings" + "github.com/jfrog/jfrog-cli-security/commands/audit/sca/swift" biutils "github.com/jfrog/build-info-go/utils" @@ -97,7 +99,7 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner log.Info(fmt.Sprintf("No changed descriptors found for %s. Skipping SCA scan...", targetResult.Target)) continue } else { - log.Info(fmt.Sprintf("'%s' SCA scan will be performed on the following descriptors: %v", targetResult.Target, filterDescriptors)) + log.Info(fmt.Sprintf("Partial SCA scan will be performed on the following descriptors: %v", filterDescriptors)) } } // Get the dependency tree for the technology in the working directory. @@ -114,15 +116,14 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner // TODO: get dependency from the changes and filter tree only to contain the changed dependencies // Filter the dependency tree to contain only the changed dependencies. - // Create sca scan task auditParallelRunner.ScaScansWg.Add(1) // defer auditParallelRunner.ScaScansWg.Done() _, taskErr := auditParallelRunner.Runner.AddTaskWithError(executeScaScanTask(auditParallelRunner, serverDetails, auditParams, targetResult, treeResult), func(err error) { - _ = targetResult.AddTargetError(fmt.Errorf("Failed to execute SCA scan: %s", err.Error()), auditParams.AllowPartialResults()) + _ = targetResult.AddTargetError(fmt.Errorf("failed to execute SCA scan: %s", err.Error()), auditParams.AllowPartialResults()) }) if taskErr != nil { - _ = targetResult.AddTargetError(fmt.Errorf("Failed to create SCA scan task: %s", taskErr.Error()), auditParams.AllowPartialResults()) + _ = targetResult.AddTargetError(fmt.Errorf("failed to create SCA scan task: %s", taskErr.Error()), auditParams.AllowPartialResults()) auditParallelRunner.ScaScansWg.Done() } } @@ -133,13 +134,15 @@ func getTargetDescriptorsToScan(scaResults *results.ScaScanResults, filesToScan if len(filesToScan) == 0 { return false, scaResults.Descriptors } - var filteredDescriptors []string - for _, descriptor := range scaResults.Descriptors { - if slices.Contains(filesToScan, descriptor) { - filteredDescriptors = append(filteredDescriptors, descriptor) + filteredDescriptors := datastructures.MakeSet[string]() + for _, fileToScan := range filesToScan { + for _, descriptor := range scaResults.Descriptors { + if strings.HasSuffix(descriptor, fileToScan) { + filteredDescriptors.Add(descriptor) + } } } - return true, filteredDescriptors + return true, filteredDescriptors.ToSlice() } From 4c32553480de34b4e8d5629692092445335e9598 Mon Sep 17 00:00:00 2001 From: attiasas Date: Mon, 24 Feb 2025 14:22:15 +0200 Subject: [PATCH 13/14] start to pass partial scan to jas --- commands/audit/audit.go | 2 ++ commands/audit/scarunner.go | 1 + commands/scan/scan.go | 1 + jas/common.go | 11 ++++++++--- jas/runner/jasrunner.go | 1 + jas/runner/jasrunner_test.go | 6 +++--- jas/secrets/secretsscanner.go | 14 ++++++++------ jas/secrets/secretsscanner_test.go | 2 +- utils/scm/filediff.go | 2 +- 9 files changed, 26 insertions(+), 14 deletions(-) diff --git a/commands/audit/audit.go b/commands/audit/audit.go index 19f2788f..b1ae632a 100644 --- a/commands/audit/audit.go +++ b/commands/audit/audit.go @@ -299,6 +299,7 @@ func RunJasScans(auditParallelRunner *utils.SecurityParallelRunner, auditParams auditParams.resultsContext.Watches, scanResults.GetTechnologies()..., ), + auditParams.filesToScan, auditParams.Exclusions()..., ) auditParallelRunner.ResultsMu.Unlock() @@ -343,6 +344,7 @@ func createJasScansTasks(auditParallelRunner *utils.SecurityParallelRunner, scan Module: *module, ConfigProfile: auditParams.configProfile, ScansToPerform: auditParams.ScansToPerform(), + FilesToScan: auditParams.filesToScan, SecretsScanType: secrets.SecretsScannerType, DirectDependencies: auditParams.DirectDependencies(), ThirdPartyApplicabilityScan: auditParams.thirdPartyApplicabilityScan, diff --git a/commands/audit/scarunner.go b/commands/audit/scarunner.go index 8febcb73..67c7b6f5 100644 --- a/commands/audit/scarunner.go +++ b/commands/audit/scarunner.go @@ -95,6 +95,7 @@ func buildDepTreeAndRunScaScan(auditParallelRunner *utils.SecurityParallelRunner continue } if partialScan, filterDescriptors := getTargetDescriptorsToScan(targetResult.ScaResults, auditParams.filesToScan); partialScan { + // Diff mode, scan only the files affected by the diff. if len(filterDescriptors) == 0 { log.Info(fmt.Sprintf("No changed descriptors found for %s. Skipping SCA scan...", targetResult.Target)) continue diff --git a/commands/scan/scan.go b/commands/scan/scan.go index c7ea0227..9d004daf 100644 --- a/commands/scan/scan.go +++ b/commands/scan/scan.go @@ -481,6 +481,7 @@ func (scanCmd *ScanCommand) createIndexerHandlerFunc(file *spec.File, cmdResults scanCmd.resultsContext.Watches, targetResults.GetTechnologies()..., ), + []string{}, ) if err != nil { return targetResults.AddTargetError(fmt.Errorf("failed to create jas scanner: %s", err.Error()), false) diff --git a/jas/common.go b/jas/common.go index 37d31a99..a7f56b8d 100644 --- a/jas/common.go +++ b/jas/common.go @@ -42,11 +42,12 @@ type JasScanner struct { ServerDetails *config.ServerDetails ScannerDirCleanupFunc func() error EnvVars map[string]string + FilesToScan []string Exclusions []string MinSeverity severityutils.Severity } -func CreateJasScanner(serverDetails *config.ServerDetails, validateSecrets bool, minSeverity severityutils.Severity, envVars map[string]string, exclusions ...string) (scanner *JasScanner, err error) { +func CreateJasScanner(serverDetails *config.ServerDetails, validateSecrets bool, minSeverity severityutils.Severity, envVars map[string]string, filesToScan []string, exclusions ...string) (scanner *JasScanner, err error) { if serverDetails == nil { err = errors.New(NoServerDetailsError) return @@ -75,6 +76,7 @@ func CreateJasScanner(serverDetails *config.ServerDetails, validateSecrets bool, return fileutils.RemoveTempDir(tempDir) } scanner.ServerDetails = serverDetails + scanner.FilesToScan = filesToScan scanner.Exclusions = exclusions scanner.MinSeverity = minSeverity return @@ -289,7 +291,7 @@ var FakeBasicXrayResults = []services.ScanResponse{ func InitJasTest(t *testing.T) (*JasScanner, func()) { assert.NoError(t, DownloadAnalyzerManagerIfNeeded(0)) - scanner, err := CreateJasScanner(&FakeServerDetails, false, "", GetAnalyzerManagerXscEnvVars("", "", "", []string{})) + scanner, err := CreateJasScanner(&FakeServerDetails, false, "", GetAnalyzerManagerXscEnvVars("", "", "", []string{}), []string{}) assert.NoError(t, err) return scanner, func() { assert.NoError(t, scanner.ScannerDirCleanupFunc()) @@ -318,7 +320,10 @@ func ShouldSkipScanner(module jfrogappsconfig.Module, scanType jasutils.JasScanT return false } -func GetSourceRoots(module jfrogappsconfig.Module, scanner *jfrogappsconfig.Scanner) ([]string, error) { +func GetSourceRoots(module jfrogappsconfig.Module, scanner *jfrogappsconfig.Scanner, filesToScan ...string) ([]string, error) { + if len(filesToScan) > 0 { + return filesToScan, nil + } root, err := filepath.Abs(module.SourceRoot) if err != nil { return []string{}, errorutils.CheckError(err) diff --git a/jas/runner/jasrunner.go b/jas/runner/jasrunner.go index 534a2053..d8b2dd52 100644 --- a/jas/runner/jasrunner.go +++ b/jas/runner/jasrunner.go @@ -32,6 +32,7 @@ type JasRunnerParams struct { AllowPartialResults bool ScansToPerform []utils.SubScanType + FilesToScan []string // Secret scan flags SecretsScanType secrets.SecretsScanType diff --git a/jas/runner/jasrunner_test.go b/jas/runner/jasrunner_test.go index 42a7f699..1d4d58e8 100644 --- a/jas/runner/jasrunner_test.go +++ b/jas/runner/jasrunner_test.go @@ -28,7 +28,7 @@ func TestJasRunner_AnalyzerManagerNotExist(t *testing.T) { defer func() { assert.NoError(t, os.Unsetenv(coreutils.HomeDir)) }() - scanner, err := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{})) + scanner, err := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{}), []string{}) assert.NoError(t, err) if scanner.AnalyzerManager.AnalyzerManagerFullPath, err = jas.GetAnalyzerManagerExecutable(); err != nil { return @@ -43,7 +43,7 @@ func TestJasRunner(t *testing.T) { securityParallelRunnerForTest := utils.CreateSecurityParallelRunner(cliutils.Threads) targetResults := results.NewCommandResults(utils.SourceCode).SetEntitledForJas(true).SetSecretValidation(true).NewScanResults(results.ScanTarget{Target: "target", Technology: techutils.Pip}) - jasScanner, err := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{}, targetResults.GetTechnologies()...)) + jasScanner, err := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{}, targetResults.GetTechnologies()...), []string{}) assert.NoError(t, err) targetResults.NewScaScanResults(0, jas.FakeBasicXrayResults[0]) @@ -63,7 +63,7 @@ func TestJasRunner_AnalyzerManagerReturnsError(t *testing.T) { assert.NoError(t, jas.DownloadAnalyzerManagerIfNeeded(0)) jfrogAppsConfigForTest, _ := jas.CreateJFrogAppsConfig(nil) - scanner, _ := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{})) + scanner, _ := jas.CreateJasScanner(&jas.FakeServerDetails, false, "", jas.GetAnalyzerManagerXscEnvVars("", "", "", []string{}), []string{}) _, err := applicability.RunApplicabilityScan(jas.FakeBasicXrayResults, []string{"issueId_2_direct_dependency", "issueId_1_direct_dependency"}, scanner, false, applicability.ApplicabilityScannerType, jfrogAppsConfigForTest.Modules[0], 0) // Expect error: assert.ErrorContains(t, jas.ParseAnalyzerManagerError(jasutils.Applicability, err), "failed to run Applicability scan") diff --git a/jas/secrets/secretsscanner.go b/jas/secrets/secretsscanner.go index e589db78..fff2fe33 100644 --- a/jas/secrets/secretsscanner.go +++ b/jas/secrets/secretsscanner.go @@ -60,7 +60,7 @@ func newSecretsScanManager(scanner *jas.JasScanner, scanType SecretsScanType, sc } func (ssm *SecretScanManager) Run(module jfrogappsconfig.Module) (vulnerabilitiesSarifRuns []*sarif.Run, violationsSarifRuns []*sarif.Run, err error) { - if err = ssm.createConfigFile(module, ssm.scanner.Exclusions...); err != nil { + if err = ssm.createConfigFile(module, ssm.scanner.FilesToScan, ssm.scanner.Exclusions...); err != nil { return } if err = ssm.runAnalyzerManager(); err != nil { @@ -80,15 +80,17 @@ type secretsScanConfiguration struct { SkippedDirs []string `yaml:"skipped-folders"` } -func (s *SecretScanManager) createConfigFile(module jfrogappsconfig.Module, exclusions ...string) error { - roots, err := jas.GetSourceRoots(module, module.Scanners.Secrets) - if err != nil { - return err +func (s *SecretScanManager) createConfigFile(module jfrogappsconfig.Module, filesToScan []string, exclusions ...string) (err error) { + scanTarget := filesToScan + if len(scanTarget) == 0 { + if scanTarget, err = jas.GetSourceRoots(module, module.Scanners.Secrets); err != nil { + return + } } configFileContent := secretsScanConfig{ Scans: []secretsScanConfiguration{ { - Roots: roots, + Roots: scanTarget, Output: s.resultsFileName, Type: string(s.scanType), SkippedDirs: jas.GetExcludePatterns(module, module.Scanners.Secrets, exclusions...), diff --git a/jas/secrets/secretsscanner_test.go b/jas/secrets/secretsscanner_test.go index 99b68cb8..c2390ce2 100644 --- a/jas/secrets/secretsscanner_test.go +++ b/jas/secrets/secretsscanner_test.go @@ -37,7 +37,7 @@ func TestSecretsScan_CreateConfigFile_VerifyFileWasCreated(t *testing.T) { currWd, err := coreutils.GetWorkingDirectory() assert.NoError(t, err) - err = secretScanManager.createConfigFile(jfrogappsconfig.Module{SourceRoot: currWd}) + err = secretScanManager.createConfigFile(jfrogappsconfig.Module{SourceRoot: currWd}, []string{}) assert.NoError(t, err) defer func() { diff --git a/utils/scm/filediff.go b/utils/scm/filediff.go index 1013490c..28a0d694 100644 --- a/utils/scm/filediff.go +++ b/utils/scm/filediff.go @@ -125,7 +125,7 @@ func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelev return } -func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk, /*isNewFile*/_ bool) (relevantChanges []Range) { +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (relevantChanges []Range) { // SARIF locations start at 1 row, col := 1, 1 for _, diffChunk := range fileChunks { From 1a0a8772846aa3fe68265af8268e596a39d3f7a5 Mon Sep 17 00:00:00 2001 From: attiasas Date: Tue, 25 Feb 2025 09:42:37 +0200 Subject: [PATCH 14/14] continue parse diff --- cli/docs/flags.go | 6 - cli/gitcommands.go | 1 - commands/git/audit/gitaudit.go | 42 ++++-- commands/git/audit/gitauditparams.go | 12 +- utils/scm/filediff.go | 184 +++++++++++++++++++++------ utils/scm/filediff_test.go | 18 ++- utils/scm/gitmanager.go | 34 ++++- utils/scm/gitmanager_test.go | 2 +- 8 files changed, 232 insertions(+), 67 deletions(-) diff --git a/cli/docs/flags.go b/cli/docs/flags.go index ec428ef4..08a4830c 100644 --- a/cli/docs/flags.go +++ b/cli/docs/flags.go @@ -136,9 +136,6 @@ const ( RepoName = "repo-name" Months = "months" DetailedSummary = "detailed-summary" - - // Unique git audit flags - CommonAncestor = "common-ancestor" ) // Mapping between security commands (key) and their flags (key). @@ -170,7 +167,6 @@ var commandFlags = map[string][]string{ // Violations params Project, Watches, ScanVuln, Fail, // Scan params - CommonAncestor, Threads, ExclusionsAudit, Sca, Iac, Sast, Secrets, WithoutCA, SecretValidation, // Output params @@ -285,8 +281,6 @@ var flagsMap = map[string]components.Flag{ WithoutCA: components.NewBoolFlag(WithoutCA, fmt.Sprintf("Selective scanners mode: Disable Contextual Analysis scanner after SCA. Relevant only with --%s flag.", Sca)), SecretValidation: components.NewBoolFlag(SecretValidation, fmt.Sprintf("Selective scanners mode: Triggers token validation on found secrets. Relevant only with --%s flag.", Secrets)), - CommonAncestor: components.NewBoolFlag(CommonAncestor, "Set to true to compare against the common ancestor of the current and the target commit.", components.WithBoolDefaultValue(true)), - // Git flags InputFile: components.NewStringFlag(InputFile, "Path to an input file in YAML format contains multiple git providers. With this option, all other scm flags will be ignored and only git servers mentioned in the file will be examined.."), ScmType: components.NewStringFlag(ScmType, fmt.Sprintf("SCM type. Possible values are: %s.", contributors.NewScmType().GetValidScmTypeString()), components.SetMandatory()), diff --git a/cli/gitcommands.go b/cli/gitcommands.go index 0b903e06..4ada37ea 100644 --- a/cli/gitcommands.go +++ b/cli/gitcommands.go @@ -46,7 +46,6 @@ func GitAuditCmd(c *components.Context) error { if len(c.Arguments) > 0 { // Diff mode gitAuditCmd.SetDiffTarget(c.Arguments[0]) - gitAuditCmd.SetUseCommonAncestorAsTarget(c.GetBoolFlagValue(flags.CommonAncestor)) } // Set connection params diff --git a/commands/git/audit/gitaudit.go b/commands/git/audit/gitaudit.go index 16a686d3..e8936546 100644 --- a/commands/git/audit/gitaudit.go +++ b/commands/git/audit/gitaudit.go @@ -105,6 +105,9 @@ func toAuditParams(params GitAuditParams, changes *scm.ChangesRelevantToScan) *s func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandResults) { // Get diff targets to scan if needed + getTargetIfDiffMode(params) + return results.NewCommandResults(utils.SourceCode) + diffTargets, err := getDiffTargets(params) if err != nil { return results.NewCommandResults(utils.SourceCode).AddGeneralError(err, false) @@ -113,6 +116,9 @@ func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandRes log.Warn("No changes detected in the diff, skipping the scan") return results.NewCommandResults(utils.SourceCode) } + log.Debug(fmt.Sprintf("Diff targets: %v", diffTargets)) + // TODO: delete this line + return results.NewCommandResults(utils.SourceCode) // Send scan started event event := xsc.CreateAnalyticsEvent(services.CliProduct, services.CliEventType, params.serverDetails) event.GitInfo = ¶ms.source @@ -132,22 +138,42 @@ func RunGitAudit(params GitAuditParams) (scanResults *results.SecurityCommandRes return scanResults } -func getDiffTargets(params GitAuditParams) (changes *scm.ChangesRelevantToScan, err error) { +func getTargetIfDiffMode(params GitAuditParams) (changes []scm.FileDiffGenerator, err error) { if params.diffTarget == "" { return } gitManager, err := scm.NewGitManager(params.repositoryLocalPath) - if params.calculateCommonAncestorAsTarget { - log.Info(fmt.Sprintf("Diff mode: check for common ancestor with target '%s'", params.diffTarget)) - if params.diffTarget, err = gitManager.GetCommonAncestor(params.diffTarget); err != nil { - return - } + if err != nil { + return } - log.Info(fmt.Sprintf("Diff mode: comparing against target '%s'", params.diffTarget)) + if params.commonAncestor, err = gitManager.GetCommonAncestor(params.diffTarget); err != nil { + return + } + log.Info(fmt.Sprintf("Diff mode: comparing '%s' against target '%s' (common ancestor '%s')", params.source.LastCommitHash, params.diffTarget, params.commonAncestor)) + diff, err := gitManager.Diff(params.diffTarget) if err != nil { return } - if relevantChanges, err := gitManager.ScanRelevantDiff(params.diffTarget); err == nil { + changes = scm.FilePatchToFileDiffGenerator(diff...) + for _, analyzedFile := range changes { + log.Info(fmt.Sprintf("Analyzing file: %s", analyzedFile.String())) + } + return +} + +func getDiffTargets(params GitAuditParams) (changes *scm.ChangesRelevantToScan, err error) { + if params.diffTarget == "" { + return + } + gitManager, err := scm.NewGitManager(params.repositoryLocalPath) + if err != nil { + return + } + if params.commonAncestor, err = gitManager.GetCommonAncestor(params.diffTarget); err != nil { + return + } + log.Info(fmt.Sprintf("Diff mode: comparing '%s' against target '%s' (common ancestor '%s')", params.source.LastCommitHash, params.diffTarget, params.commonAncestor)) + if relevantChanges, err := gitManager.ScanRelevantDiff(params.diffTarget, params.commonAncestor); err == nil { changes = &relevantChanges } return diff --git a/commands/git/audit/gitauditparams.go b/commands/git/audit/gitauditparams.go index d94e8a38..9408edf7 100644 --- a/commands/git/audit/gitauditparams.go +++ b/commands/git/audit/gitauditparams.go @@ -12,9 +12,8 @@ import ( type GitAuditParams struct { // Git Params - source services.XscGitInfoContext - diffTarget string - calculateCommonAncestorAsTarget bool + source services.XscGitInfoContext + diffTarget string // Connection params serverDetails *config.ServerDetails // Violations params @@ -33,6 +32,8 @@ type GitAuditParams struct { repositoryLocalPath string multiScanId string startTime time.Time + + commonAncestor string } func NewGitAuditParams() *GitAuditParams { @@ -44,11 +45,6 @@ func (gap *GitAuditParams) SetDiffTarget(diffTarget string) *GitAuditParams { return gap } -func (gap *GitAuditParams) SetUseCommonAncestorAsTarget(commonAncesto bool) *GitAuditParams { - gap.calculateCommonAncestorAsTarget = commonAncesto - return gap -} - func (gap *GitAuditParams) SetServerDetails(serverDetails *config.ServerDetails) *GitAuditParams { gap.serverDetails = serverDetails return gap diff --git a/utils/scm/filediff.go b/utils/scm/filediff.go index 28a0d694..12ef1a81 100644 --- a/utils/scm/filediff.go +++ b/utils/scm/filediff.go @@ -10,6 +10,107 @@ import ( "github.com/jfrog/jfrog-client-go/utils/log" ) +type Range struct { + StartRow int + StartCol int + EndRow int + EndCol int +} + +func (r Range) String() string { + return fmt.Sprintf("%d:%d-%d:%d", r.StartRow, r.StartCol, r.EndRow, r.EndCol) +} + +// Contains checks if the range contains (fully) the given range +func (r Range) Contains(startRow, startCol, endRow, endCol int) bool { + return r.StartRow <= startRow && r.StartCol <= startCol && r.EndRow >= endRow && r.EndCol >= endCol +} + +// Overlaps checks if the range overlaps (partially or fully) the given range +func (r Range) Overlaps(startRow, startCol, endRow, endCol int) bool { + return r.StartRow < endRow && r.EndRow > startRow && r.StartCol < endCol && r.EndCol > startCol +} + +type MarkedRange struct { + goDiff.Operation + Range +} + +func (mr MarkedRange) String() string { + return fmt.Sprintf("(%s) %s", opToStr(mr.Operation), mr.Range) +} + +func opToStr(op goDiff.Operation) string { + switch op { + case goDiff.Add: + return "Add" + case goDiff.Delete: + return "Delete" + case goDiff.Equal: + return "Equal" + default: + return "Unknown" + } +} + +type FileDiffGenerator struct { + Path string + OrderedContent []MarkedRange +} + +func (fdg FileDiffGenerator) String() string { + return fmt.Sprintf("%s: %v", fdg.Path, fdg.OrderedContent) +} + +func FilePatchToFileDiffGenerator(filePatches ...goDiff.FilePatch) (fileDiffs []FileDiffGenerator) { + for _, filePatch := range filePatches { + fileDiff := FileDiffGenerator{Path: getFilePathFromFiles(filePatch)} + startRow, startCol := 1, 1 + for _, chunk := range filePatch.Chunks() { + // Create the content for the chunk + content := MarkedRange{Operation: chunk.Type(), Range: Range{StartRow: startRow, StartCol: startCol}} + + log.Debug(fmt.Sprintf("Chunk (type = %s): \"%s\"", opToStr(content.Operation), chunk.Content())) + // Parse cursor based on the operation + // switch content.Operation { + // case goDiff.Equal: + + // } + + // Update the cursor position and content end position + startRow, startCol = getCursorNewPosition(startRow, startCol, chunk.Content()) + content.EndRow, content.EndCol = startRow, startCol + // Add the content to the ordered content + log.Debug(fmt.Sprintf("Adding ordered content: %s", content)) + fileDiff.OrderedContent = append(fileDiff.OrderedContent, content) + } + fileDiffs = append(fileDiffs, fileDiff) + } + return +} + +func getFilePathFromFiles(filePatch goDiff.FilePatch) string { + from, to := filePatch.Files() + fromPath := "" + if from != nil { + fromPath = from.Path() + } + toPath := "" + if to != nil { + toPath = to.Path() + } + log.Debug(fmt.Sprintf("Checking Diff between: %s (from) and %s (to)", fromPath, toPath)) + if fromPath == "" { + return toPath + } + if toPath == "" { + return fromPath + } + return fromPath +} + +// TODO: DELETE + type ChangesRelevantToScan struct { ChangedFiles []FileChanges ChangedBinaries []string @@ -71,61 +172,58 @@ func (f FileChanges) Overlaps(startRow, startCol, endRow, endCol int) bool { return false } -type Range struct { - StartRow int - StartCol int - EndRow int - EndCol int -} - -func (r Range) String() string { - return fmt.Sprintf("%d:%d-%d:%d", r.StartRow, r.StartCol, r.EndRow, r.EndCol) -} - -// Contains checks if the range contains (fully) the given range -func (r Range) Contains(startRow, startCol, endRow, endCol int) bool { - return r.StartRow <= startRow && r.StartCol <= startCol && r.EndRow >= endRow && r.EndCol >= endCol -} - -// Overlaps checks if the range overlaps (partially or fully) the given range -func (r Range) Overlaps(startRow, startCol, endRow, endCol int) bool { - return r.StartRow < endRow && r.EndRow > startRow && r.StartCol < endCol && r.EndCol > startCol -} - -func detectRelevantChanges(filePatches []goDiff.FilePatch) (changes ChangesRelevantToScan, err error) { +// The Source code is the code at the cwd, the target +// if removedContent is true, the content of the removed files is relevant. else only the added content is relevant +func detectRelevantChanges(filePatches []goDiff.FilePatch, removedContent bool) (changes ChangesRelevantToScan, err error) { binariesChanged := datastructures.MakeSet[string]() // Go over the file patches and detect the relevant changes for _, filePatch := range filePatches { - from, to := filePatch.Files() - fromPath := "" - if from != nil { - fromPath = from.Path() - } - toPath := "" - if to != nil { - toPath = to.Path() + relevantFileName := getRelevantFileName(filePatch, removedContent) + if relevantFileName == "" { + // Not relevant file + continue } - log.Debug(fmt.Sprintf("Checking Diff between: %s (from) and %s (to)", fromPath, toPath)) // Get the relevant changes for the file if filePatch.IsBinary() { // Binary file, only file path is relevant - binariesChanged.Add(to.Path()) - continue - } - if to == nil { - // Deleted file, not relevant to scan + binariesChanged.Add(relevantFileName) continue } // Get the relevant changes in the file, if new file (from is nil) all the content is relevant - if fileChanges := processFileChunksForRelevantChanges(filePatch.Chunks(), from == nil); /*len(fileChanges) > 0*/ true { - changes.ChangedFiles = append(changes.ChangedFiles, FileChanges{Path: to.Path(), Changes: fileChanges}) + if fileChanges := processFileChunksForRelevantChanges(filePatch.Chunks(), removedContent); len(fileChanges) > 0 { + changes.ChangedFiles = append(changes.ChangedFiles, FileChanges{Path: relevantFileName, Changes: fileChanges}) } } changes.ChangedBinaries = binariesChanged.ToSlice() return } -func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/, _ bool) (relevantChanges []Range) { +func getRelevantFileName(filePatch goDiff.FilePatch, removedContent bool) string { + from, to := filePatch.Files() + fromPath := "" + if from != nil { + fromPath = from.Path() + } + toPath := "" + if to != nil { + toPath = to.Path() + } + log.Debug(fmt.Sprintf("Checking Diff between: %s (from) and %s (to)", fromPath, toPath)) + if removedContent && from != nil { + // removedContent = source is after target, if content is removed from 'from' it is relevant + log.Debug(fmt.Sprintf("Removed content: %s", from.Path())) + return from.Path() + } else if !removedContent && to != nil { + // addedContent = source is before target, if content is added to 'to' it is relevant + log.Debug(fmt.Sprintf("Added content: %s", to.Path())) + return to.Path() + } + // No relevant file name + log.Debug("No relevant file name") + return "" +} + +func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk, removedContent bool) (relevantChanges []Range) { // SARIF locations start at 1 row, col := 1, 1 for _, diffChunk := range fileChunks { @@ -135,11 +233,17 @@ func processFileChunksForRelevantChanges(fileChunks []goDiff.Chunk /*isNewFile*/ case goDiff.Add: // Added content // Add the range of the added content - relevantChanges = append(relevantChanges, Range{StartRow: row, StartCol: col, EndRow: row, EndCol: col + len(chunkContent)}) + if !removedContent { + relevantChanges = append(relevantChanges, Range{StartRow: row, StartCol: col, EndRow: row, EndCol: col + len(chunkContent)}) + } // Move the cursor to the end of the added content case goDiff.Delete: // Deleted content + if removedContent { + relevantChanges = append(relevantChanges, Range{StartRow: row, StartCol: col, EndRow: row, EndCol: col + len(chunkContent)}) + } + // Move the cursor to the end of the deleted content case goDiff.Equal: diff --git a/utils/scm/filediff_test.go b/utils/scm/filediff_test.go index 4a507ccb..fdabf918 100644 --- a/utils/scm/filediff_test.go +++ b/utils/scm/filediff_test.go @@ -1,3 +1,19 @@ package scm -import () +import ( +// "path/filepath" +// "testing" + +// goGit "github.com/go-git/go-git/v5" + +// "github.com/stretchr/testify/assert" +// "github.com/stretchr/testify/require" + +// securityTestUtils "github.com/jfrog/jfrog-cli-security/tests/utils" + +// "github.com/jfrog/jfrog-client-go/xsc/services" +) + +func Test() { + +} diff --git a/utils/scm/gitmanager.go b/utils/scm/gitmanager.go index ef6439a2..52644fad 100644 --- a/utils/scm/gitmanager.go +++ b/utils/scm/gitmanager.go @@ -204,14 +204,44 @@ func (gm *GitManager) Diff(reference string) (changes []diff.FilePatch, err erro if err != nil { return } + log.Debug(fmt.Sprintf("Diff stats between %s and %s:\n%v", currentBranch.Name().Short(), reference, diff.Stats())) changes = diff.FilePatches() return } -func (gm *GitManager) ScanRelevantDiff(reference string) (changes ChangesRelevantToScan, err error) { +func (gm *GitManager) DiffGetAddedContent(reference string) (changes ChangesRelevantToScan, err error) { diff, err := gm.Diff(reference) if err != nil { return } - return detectRelevantChanges(diff) + log.Debug(fmt.Sprintf("Checking for added content in the diff between the cwd and %s", reference)) + return detectRelevantChanges(diff, false) +} + +func (gm *GitManager) DiffGetRemovedContent(reference string) (changes ChangesRelevantToScan, err error) { + diff, err := gm.Diff(reference) + if err != nil { + return + } + log.Debug(fmt.Sprintf("Checking for removed content in the diff between the cwd and %s", reference)) + return detectRelevantChanges(diff, true) +} + +func (gm *GitManager) ScanRelevantDiff(reference, commonAncestor string) (changes ChangesRelevantToScan, err error) { + // Get the current branch + currentBranch, err := gm.localGitRepository.Head() + if err != nil { + return + } + // Get the commit object of the current branch + currentCommit, err := gm.localGitRepository.CommitObject(currentBranch.Hash()) + if err != nil { + return + } + if currentCommit.Hash.String() == commonAncestor { + // Source code is the code at the cwd and the common ancestor, target is after it. relevant changes are added content up to reference + return gm.DiffGetAddedContent(reference) + } + // Source code and target have a common ancestor + return gm.DiffGetRemovedContent(commonAncestor) } diff --git a/utils/scm/gitmanager_test.go b/utils/scm/gitmanager_test.go index 08f456a2..2a9fe1a3 100644 --- a/utils/scm/gitmanager_test.go +++ b/utils/scm/gitmanager_test.go @@ -285,7 +285,7 @@ func TestDetectRelevantChanges(t *testing.T) { gitManager, err := NewGitManager(projectPath) require.NoError(t, err) // Get the relevant changes - changes, err := gitManager.ScanRelevantDiff(testCase.targetReference) + changes, err := gitManager.ScanRelevantDiff(testCase.targetReference, "") require.NoError(t, err) // Assert the expected changes assert.Equal(t, testCase.expectedChanges, changes)