diff --git a/application/server/server_smoke_test.go b/application/server/server_smoke_test.go index 38cfd5276..311fbe4ce 100644 --- a/application/server/server_smoke_test.go +++ b/application/server/server_smoke_test.go @@ -478,11 +478,11 @@ func runSmokeTest(t *testing.T, repo string, commit string, file1 string, file2 } } -func newTestFileWithVulns(t *testing.T, cloneTargetDir string, fileName string) { +func newFileInCurrentDir(t *testing.T, cloneTargetDir string, fileName string, content string) { t.Helper() testFile := filepath.Join(cloneTargetDir, fileName) - err := os.WriteFile(testFile, []byte("var token = 'SECRET_TOKEN_f8ed84e8f41e4146403dd4a6bbcea5e418d23a9';"), 0600) + err := os.WriteFile(testFile, []byte(content), 0600) assert.NoError(t, err) } @@ -754,7 +754,7 @@ func Test_SmokeSnykCodeDelta_OneNewVuln(t *testing.T) { var cloneTargetDir, err = testutil.SetupCustomTestRepo(t, t.TempDir(), "https://github.com/snyk-labs/nodejs-goof", "0336589", c.Logger()) assert.NoError(t, err) - newTestFileWithVulns(t, cloneTargetDir, fileWithNewVulns) + newFileInCurrentDir(t, cloneTargetDir, fileWithNewVulns, "var token = 'SECRET_TOKEN_f8ed84e8f41e4146403dd4a6bbcea5e418d23a9';") initParams := prepareInitParams(t, cloneTargetDir, c) @@ -768,6 +768,53 @@ func Test_SmokeSnykCodeDelta_OneNewVuln(t *testing.T) { assert.Contains(t, snykCodeScanParams.Issues[0].FilePath, fileWithNewVulns) } +func Test_SmokeSnykCodeDelta_NoScanNecessary(t *testing.T) { + loc, jsonRPCRecorder := setupServer(t) + c := testutil.SmokeTest(t, false) + c.SetSnykCodeEnabled(true) + c.SetDeltaFindingsEnabled(true) + cleanupChannels() + di.Init() + + var cloneTargetDir, err = testutil.SetupCustomTestRepo(t, t.TempDir(), "https://github.com/snyk-labs/nodejs-goof", "0336589", c.Logger()) + assert.NoError(t, err) + + initParams := prepareInitParams(t, cloneTargetDir, c) + + ensureInitialized(t, loc, initParams) + + waitForScan(t, cloneTargetDir) + + snykCodeScanParams := checkForScanParams(t, jsonRPCRecorder, cloneTargetDir, product.ProductCode) + + assert.Equal(t, len(snykCodeScanParams.Issues), 0) +} + +func Test_SmokeSnykCodeDelta_NoNewIssuesFound(t *testing.T) { + loc, jsonRPCRecorder := setupServer(t) + c := testutil.SmokeTest(t, false) + c.SetSnykCodeEnabled(true) + c.SetDeltaFindingsEnabled(true) + cleanupChannels() + di.Init() + + fileWithNewVulns := "vulns.js" + var cloneTargetDir, err = testutil.SetupCustomTestRepo(t, t.TempDir(), "https://github.com/snyk-labs/nodejs-goof", "0336589", c.Logger()) + assert.NoError(t, err) + + newFileInCurrentDir(t, cloneTargetDir, fileWithNewVulns, "// no problems") + + initParams := prepareInitParams(t, cloneTargetDir, c) + + ensureInitialized(t, loc, initParams) + + waitForScan(t, cloneTargetDir) + + snykCodeScanParams := checkForScanParams(t, jsonRPCRecorder, cloneTargetDir, product.ProductCode) + + assert.Equal(t, len(snykCodeScanParams.Issues), 0) +} + func ensureInitialized(t *testing.T, loc server.Local, initParams types.InitializeParams) { t.Helper() diff --git a/domain/ide/workspace/folder.go b/domain/ide/workspace/folder.go index f7c0ae954..360e9c21b 100644 --- a/domain/ide/workspace/folder.go +++ b/domain/ide/workspace/folder.go @@ -19,7 +19,6 @@ package workspace import ( "context" "encoding/json" - "errors" "fmt" "strings" "sync" @@ -305,11 +304,7 @@ func (f *Folder) scan(ctx context.Context, path string) { func (f *Folder) processResults(scanData snyk.ScanData) { if scanData.Err != nil { - f.scanNotifier.SendError(scanData.Product, f.path, scanData.Err.Error()) - f.c.Logger().Err(scanData.Err). - Str("method", "processResults"). - Str("product", string(scanData.Product)). - Msg("Product returned an error") + f.sendScanError(scanData.Product, scanData.Err) return } // this also updates the severity counts in scan data, therefore we pass a pointer @@ -321,6 +316,15 @@ func (f *Folder) processResults(scanData snyk.ScanData) { f.FilterAndPublishDiagnostics(&scanData.Product) } +func (f *Folder) sendScanError(product product.Product, err error) { + f.scanNotifier.SendError(product, f.path, err.Error()) + f.c.Logger().Err(err). + Str("method", "processResults"). + Str("product", string(product)). + Msg("Product returned an error") + f.notifier.SendErrorDiagnostic(f.path, err) +} + func (f *Folder) updateGlobalCacheAndSeverityCounts(scanData *snyk.ScanData) { var newCache = snyk.IssuesByFile{} var dedupMap = map[string]bool{} @@ -471,12 +475,10 @@ func appendTestResults(sic snyk.SeverityIssueCounts, results []json_schemas.Test func (f *Folder) FilterAndPublishDiagnostics(p *product.Product) { productIssuesByFile, err := f.getDelta(f.IssuesByProduct(), p) if err != nil { - f.c.Logger().Error().Err(err).Msg("Error getting delta for product issues") - if errors.Is(err, delta.ErrNoDeltaCalculated) { - deltaErr := fmt.Errorf("Couldn't determine the difference between current and base branch for %s scan. %s"+ - "Falling back to showing full scan results. Please check the error log for more information.", p.ToProductNamesString(), err) - f.notifier.SendErrorDiagnostic(f.path, deltaErr) - } + // Error can only be returned from delta analysis. Other non delta scans are skipped with no errors. + deltaErr := fmt.Errorf("couldn't determine the difference between current and base branch for %s scan. %w", p.ToProductNamesString(), err) + f.sendScanError(*p, deltaErr) + return } if p != nil { filteredIssues := f.filterDiagnostics(productIssuesByFile[*p]) @@ -489,21 +491,25 @@ func (f *Folder) FilterAndPublishDiagnostics(p *product.Product) { } } +// Error can only be returned from delta analysis. Other non delta scans are skipped with no errors. func (f *Folder) getDelta(productIssueByFile snyk.ProductIssuesByFile, p *product.Product) (snyk.ProductIssuesByFile, error) { logger := f.c.Logger().With().Str("method", "getDelta").Logger() currentProduct := *p + // Delete product check when base scanning is implemented for other products if !f.c.IsDeltaFindingsEnabled() || currentProduct != product.ProductCode { return productIssueByFile, nil } + if len(productIssueByFile[currentProduct]) == 0 { + // If no issues found in current branch scan. We can't have deltas. + return productIssueByFile, nil + } + baseIssueList, err := f.scanPersister.GetPersistedIssueList(f.path, currentProduct) if err != nil { logger.Err(err).Msg("Error getting persisted issue list") - return productIssueByFile, delta.ErrNoDeltaCalculated - } - if len(baseIssueList) == 0 { - return productIssueByFile, delta.ErrNoDeltaCalculated + return nil, delta.ErrNoDeltaCalculated } currentFlatIssueList := getFlatIssueList(productIssueByFile, currentProduct) diff --git a/infrastructure/code/code.go b/infrastructure/code/code.go index a62559dbb..878f49cd5 100644 --- a/infrastructure/code/code.go +++ b/infrastructure/code/code.go @@ -32,8 +32,6 @@ import ( "github.com/pkg/errors" "github.com/puzpuzpuz/xsync" - gitconfig "github.com/snyk/snyk-ls/internal/git_config" - codeClient "github.com/snyk/code-client-go" codeClientObservability "github.com/snyk/code-client-go/observability" "github.com/snyk/code-client-go/scan" @@ -185,12 +183,30 @@ func (sc *Scanner) Scan(ctx context.Context, path string, folderPath string) (is filesToBeScanned := sc.getFilesToBeScanned(folderPath) sc.changedFilesMutex.Unlock() + if c.IsDeltaFindingsEnabled() { + hasChanges, gitErr := vcs.LocalRepoHasChanges(c.Logger(), folderPath) + if gitErr != nil { + logger.Error().Err(gitErr).Msg("couldn't check if working dir is clean") + return nil, gitErr + } + if !hasChanges { + // If delta is enabled but there are no changes. There can be no delta. + // else it should start scanning. + logger.Debug().Msg("skipping scanning. working dir is clean") + return []snyk.Issue{}, nil // Returning an empty slice implies that no issues were found + } + } + results, err := internalScan(ctx, sc, folderPath, logger, filesToBeScanned) + if err != nil { + return nil, err + } - if err == nil && c.IsDeltaFindingsEnabled() { - baseScanErr := scanAndPersistBaseBranch(ctx, sc, folderPath) - if baseScanErr != nil { - logger.Error().Err(baseScanErr).Msg("couldn't scan base branch for folder " + folderPath) + if c.IsDeltaFindingsEnabled() && len(results) > 0 { + err = scanAndPersistBaseBranch(ctx, sc, folderPath) + if err != nil { + logger.Error().Err(err).Msg("couldn't scan base branch for folder " + folderPath) + return nil, err } } @@ -224,16 +240,7 @@ func internalScan(ctx context.Context, sc *Scanner, folderPath string, logger ze func scanAndPersistBaseBranch(ctx context.Context, sc *Scanner, folderPath string) error { logger := sc.c.Logger().With().Str("method", "scanAndPersistBaseBranch").Logger() - baseBranchName := getBaseBranchName(folderPath) - shouldClone, err := vcs.ShouldClone(&logger, folderPath, baseBranchName) - if err != nil { - return err - } - - if !shouldClone { - return nil - } - + baseBranchName := vcs.GetBaseBranchName(folderPath) headRef, err := vcs.HeadRefHashForBranch(&logger, folderPath, baseBranchName) if err != nil { @@ -295,14 +302,6 @@ func scanAndPersistBaseBranch(ctx context.Context, sc *Scanner, folderPath strin return nil } -func getBaseBranchName(folderPath string) string { - folderConfig, err := gitconfig.GetOrCreateFolderConfig(folderPath) - if err != nil { - return "master" - } - return folderConfig.BaseBranch -} - // Populate HTML template func (sc *Scanner) enhanceIssuesDetails(issues []snyk.Issue) { logger := sc.c.Logger().With().Str("method", "issue_enhancer.enhanceIssuesDetails").Logger() diff --git a/internal/vcs/git_cloner.go b/internal/vcs/git_cloner.go index 732a0a3fa..94ffc52ea 100644 --- a/internal/vcs/git_cloner.go +++ b/internal/vcs/git_cloner.go @@ -47,6 +47,7 @@ func Clone(logger *zerolog.Logger, srcRepoPath string, destinationPath string, t } return targetRepo, nil } + return clonedRepo, nil } @@ -77,13 +78,15 @@ func cloneRepoWithFsCopy(logger *zerolog.Logger, srcRepoPath string, destination return targetRepo } -func ShouldClone(logger *zerolog.Logger, repoPath string, branchName string) (bool, error) { +func LocalRepoHasChanges(logger *zerolog.Logger, repoPath string) (bool, error) { currentRepo, err := git.PlainOpen(repoPath) if err != nil { logger.Error().Err(err).Msg("Failed to open current repo " + repoPath) return false, err } + branchName := GetBaseBranchName(repoPath) + currentRepoBranch, err := currentRepo.Head() if err != nil { logger.Error().Err(err).Msg("Failed to get HEAD for " + repoPath) diff --git a/internal/vcs/git_cloner_test.go b/internal/vcs/git_cloner_test.go index 9658c3de4..965e390cf 100644 --- a/internal/vcs/git_cloner_test.go +++ b/internal/vcs/git_cloner_test.go @@ -137,41 +137,45 @@ func TestClone_InvalidGitRepo(t *testing.T) { assert.Error(t, err) } -func TestShouldClone_SameBranchNames_NoModification_SkipClone(t *testing.T) { +func TestLocalRepoHasChanges_SameBranchNames_NoModification_SkipClone(t *testing.T) { c := testutil.UnitTest(t) repoPath := t.TempDir() initGitRepo(t, repoPath, false) - cloneTargetBranchName := "master" - shouldclone, err := ShouldClone(c.Logger(), repoPath, cloneTargetBranchName) + shouldclone, err := LocalRepoHasChanges(c.Logger(), repoPath) assert.NoError(t, err) assert.False(t, shouldclone) } -func TestShouldClone_SameBranchNames_WithModification_Clone(t *testing.T) { +func TestLocalRepoHasChanges_SameBranchNames_WithModification_Clone(t *testing.T) { c := testutil.UnitTest(t) repoPath := t.TempDir() initGitRepo(t, repoPath, true) - cloneTargetBranchName := "master" - shouldclone, err := ShouldClone(c.Logger(), repoPath, cloneTargetBranchName) + shouldclone, err := LocalRepoHasChanges(c.Logger(), repoPath) assert.NoError(t, err) assert.True(t, shouldclone) } -func TestShouldClone_DifferentBranchNames_Clone(t *testing.T) { +func TestLocalRepoHasChanges_DifferentBranchNames_Clone(t *testing.T) { c := testutil.UnitTest(t) repoPath := t.TempDir() - initGitRepo(t, repoPath, true) - cloneTargetBranchName := "feat/new" + repo, _ := initGitRepo(t, repoPath, true) + wt, err := repo.Worktree() + assert.NoError(t, err) + err = wt.Checkout(&git.CheckoutOptions{ + Branch: "feat/new", + Create: true, + }) + assert.NoError(t, err) - shouldclone, err := ShouldClone(c.Logger(), repoPath, cloneTargetBranchName) + shouldclone, err := LocalRepoHasChanges(c.Logger(), repoPath) assert.True(t, shouldclone) assert.NoError(t, err) } -func TestShouldClone_HasUncommittedChanges(t *testing.T) { +func TestLocalRepoHasChanges_HasUncommittedChanges(t *testing.T) { repo, _ := initGitRepo(t, t.TempDir(), true) hasChanges := hasUncommitedChanges(repo) @@ -179,7 +183,7 @@ func TestShouldClone_HasUncommittedChanges(t *testing.T) { assert.True(t, hasChanges) } -func TestShouldClone_HasCommittedChanges(t *testing.T) { +func TestLocalRepoHasChanges_HasCommittedChanges(t *testing.T) { repo, _ := initGitRepo(t, t.TempDir(), false) hasChanges := hasUncommitedChanges(repo) diff --git a/internal/vcs/git_utils.go b/internal/vcs/git_utils.go index b7b1e96a3..c92c0b4b7 100644 --- a/internal/vcs/git_utils.go +++ b/internal/vcs/git_utils.go @@ -22,6 +22,7 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/storage/filesystem" "github.com/rs/zerolog" + gitconfig "github.com/snyk/snyk-ls/internal/git_config" "path/filepath" "regexp" "strings" @@ -102,6 +103,14 @@ func hasUncommitedChanges(repo *git.Repository) bool { return false } +func GetBaseBranchName(folderPath string) string { + folderConfig, err := gitconfig.GetOrCreateFolderConfig(folderPath) + if err != nil { + return "master" + } + return folderConfig.BaseBranch +} + func NormalizeBranchName(branchName string) string { normalized := strings.TrimSpace(branchName) normalized = strings.ToLower(normalized)