Deprecate Frogbot env vars

integrationutils.go

@@ -3,6 +3,12 @@ package main
 import (
 	"context"
 	"fmt"
+	"os"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
 	"github.com/go-git/go-git/v5"
 	githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
 	"github.com/jfrog/frogbot/v2/scanpullrequest"
@@ -13,11 +19,6 @@ import (
 	"github.com/jfrog/froggit-go/vcsutils"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"os"
-	"strconv"
-	"strings"
-	"testing"
-	"time"
 )
 
 const (
@@ -171,8 +172,8 @@ func runScanPullRequestCmd(t *testing.T, client vcsclient.VcsClient, testDetails
 	defer unsetEnvs()
 
 	err = Exec(&scanpullrequest.ScanPullRequestCmd{}, utils.ScanPullRequest)
-	// Validate that issues were found and the relevant error returned
-	require.Errorf(t, err, scanpullrequest.SecurityIssueFoundErr)
+	// Validate that no error is returned
+	require.NoError(t, err)
 
 	validateResults(t, ctx, client, testDetails, prId)
 }

scanpullrequest/scanpullrequest.go

@@ -24,8 +24,7 @@ import (
 
 const (
 	SecurityIssueFoundErr = "issues were detected by Frogbot\n" +
-		"You can avoid marking the Frogbot scan as failed by setting the " + utils.FailOnSecurityIssuesEnv + " environment variable to false\n" +
-		"Note that even if " + utils.FailOnSecurityIssuesEnv + " is set to false, but a security violation with 'fail-pull-request' rule is found, Frogbot scan will fail as well"
+		"Security violation with 'fail-pull-request' rule is found"
 	noGitHubEnvErr                       = "frogbot did not scan this PR, because a GitHub Environment named 'frogbot' does not exist. Please refer to the Frogbot documentation for instructions on how to create the Environment"
 	noGitHubEnvReviewersErr              = "frogbot did not scan this PR, because the existing GitHub Environment named 'frogbot' doesn't have reviewers selected. Please refer to the Frogbot documentation for instructions on how to create the Environment"
 	analyticsScanPrScanType              = "PR"
@@ -81,7 +80,7 @@ func verifyGitHubFrogbotEnvironment(client vcsclient.VcsClient, repoConfig *util
 	return nil
 }
 
-// By default, includeAllVulnerabilities is set to false and the scan goes as follows:
+// By default, the scan goes as follows:
 // a. Audit the dependencies of the source and the target branches.
 // b. Compare the vulnerabilities found in source and target branches, and show only the new vulnerabilities added by the pull request.
 // Otherwise, only the source branch is scanned and all found vulnerabilities are being displayed.
@@ -104,25 +103,14 @@ func scanPullRequest(repo *utils.Repository, client vcsclient.VcsClient) (err er
 		return
 	}
 
-	// Fail the Frogbot task if a security issue is found and Frogbot isn't configured to avoid the failure.
-	if toFailTaskStatus(repo, issues) {
+	// Fail the Frogbot task if a security violation is found and fail pr rule applied.
+	if issues.IsFailPrRuleApplied() {
 		err = errors.New(SecurityIssueFoundErr)
 		return
 	}
 	return
 }
 
-func toFailTaskStatus(repo *utils.Repository, issues *issues.ScansIssuesCollection) bool {
-	failFlagSet := repo.Params.Scan.FailOnSecurityIssues != nil && *repo.Params.Scan.FailOnSecurityIssues
-	if failFlagSet {
-		// If the fail flag is set to true (JF_FAIL), we check if any security ISSUE exists (not just violations), and if so, we fail the build.
-		return issues.IssuesExists(repo.Params.Git.PullRequestSecretComments)
-	} else {
-		// When fail flag is set to false, we check for fail-pr rule in existing VIOLATIONS. If one exists, we fail the build as well.
-		return issues.IsFailPrRuleApplied()
-	}
-}
-
 func auditPullRequestAndReport(repoConfig *utils.Repository, client vcsclient.VcsClient) (issuesCollection *issues.ScansIssuesCollection, resultContext results.ResultContext, err error) {
 	// Prepare
 	scanDetails, err := createBaseScanDetails(repoConfig, client)
@@ -171,7 +159,6 @@ func createBaseScanDetails(repoConfig *utils.Repository, client vcsclient.VcsCli
 		SetFixableOnly(repoConfig.Params.Scan.FixableOnly).
 		SetConfigProfile(repoConfig.Params.Scan.ConfigProfile).
 		SetSkipAutoInstall(repoConfig.Params.Scan.SkipAutoInstall).
-		SetDisableJas(repoConfig.Params.Scan.DisableJas).
 		SetXscPRGitInfoContext(repoConfig.Params.Git.Project, client, repoConfig.Params.Git.PullRequestDetails).
 		SetDiffScan(!repoConfig.Params.JFrogPlatform.IncludeVulnerabilities).
 		SetAllowPartialResults(repoConfig.Params.Scan.AllowPartialResults)
@@ -212,15 +199,13 @@ func auditPullRequestCode(repoConfig *utils.Repository, scanDetails *utils.ScanD
 		// Reset scan details for each project
 		scanDetails.SetProject(&repoConfig.Params.Scan.Projects[i]).SetResultsToCompare(nil)
 		// Scan target branch of the project
-		if !repoConfig.Params.JFrogPlatform.IncludeVulnerabilities {
-			log.Debug("Scanning target branch code...")
-			if targetScanResults, e := auditPullRequestTargetCode(scanDetails, targetBranchWd); e != nil {
-				issuesCollection.AppendStatus(getResultScanStatues(targetScanResults))
-				err = errors.Join(err, fmt.Errorf("failed to audit target branch code for %v project. Error: %s", repoConfig.Params.Scan.Projects[i].WorkingDirs, e.Error()))
-				continue
-			} else {
-				scanDetails.SetResultsToCompare(targetScanResults)
-			}
+		log.Debug("Scanning target branch code...")
+		if targetScanResults, e := auditPullRequestTargetCode(scanDetails, targetBranchWd); e != nil {
+			issuesCollection.AppendStatus(getResultScanStatues(targetScanResults))
+			err = errors.Join(err, fmt.Errorf("failed to audit target branch code for %v project. Error: %s", repoConfig.Params.Scan.Projects[i].WorkingDirs, e.Error()))
+			continue
+		} else {
+			scanDetails.SetResultsToCompare(targetScanResults)
 		}
 		// Scan source branch of the project
 		log.Debug("Scanning source branch code...")
@@ -282,7 +267,6 @@ func filterOutFailedScansIfAllowPartialResultsEnabled(targetResults, sourceResul
 		return nil
 	}
 	if targetResults == nil {
-		// If IncludeAllVulnerabilities is applied, only sourceResults exists and we don't need to filter anything - we present results we have
 		return nil
 	}
 
@@ -403,7 +387,6 @@ func filterOutScaResultsIfScanFailed(targetResult, sourceResult *results.TargetR
 // Sorts the Targets slice in both targetResults and sourceResults
 // by the physical location (Target field) of each scan target in ascending order.
 func sortTargetsByPhysicalLocation(targetResults, sourceResults *results.SecurityCommandResults) error {
-	// If !IncludeAllVulnerabilities we expect targetResults and sourceResults to be non-empty and to have the same amount of targets.
 	if len(targetResults.Targets) != len(sourceResults.Targets) {
 		return fmt.Errorf("amount of targets in target results is different than source results: %d vs %d", len(targetResults.Targets), len(sourceResults.Targets))
 	}

scanpullrequest/scanpullrequest_test.go

@@ -220,60 +220,49 @@ func TestScanResultsToIssuesCollection(t *testing.T) {
 
 func TestScanPullRequest(t *testing.T) {
 	tests := []struct {
-		testName             string
-		projectName          string
-		failOnSecurityIssues bool
+		testName    string
+		projectName string
 	}{
 		{
-			testName:             "ScanPullRequest",
-			projectName:          "test-proj",
-			failOnSecurityIssues: true,
+			testName:    "ScanPullRequest",
+			projectName: "test-proj",
 		},
 		{
-			testName:             "ScanPullRequestNoFail",
-			projectName:          "test-proj",
-			failOnSecurityIssues: false,
+			testName:    "ScanPullRequestNoFail",
+			projectName: "test-proj",
 		},
 		{
-			testName:             "ScanPullRequestSubdir",
-			projectName:          "test-proj-subdir",
-			failOnSecurityIssues: true,
+			testName:    "ScanPullRequestSubdir",
+			projectName: "test-proj-subdir",
 		},
 		{
-			testName:             "ScanPullRequestNoIssues",
-			projectName:          "clean-test-proj",
-			failOnSecurityIssues: false,
+			testName:    "ScanPullRequestNoIssues",
+			projectName: "clean-test-proj",
 		},
 		{
-			testName:             "ScanPullRequestMultiWorkDir",
-			projectName:          "multi-dir-test-proj",
-			failOnSecurityIssues: false,
+			testName:    "ScanPullRequestMultiWorkDir",
+			projectName: "multi-dir-test-proj",
 		},
 		{
-			testName:             "ScanPullRequestMultiWorkDirNoFail",
-			projectName:          "multi-dir-test-proj",
-			failOnSecurityIssues: true,
+			testName:    "ScanPullRequestMultiWorkDirNoFail",
+			projectName: "multi-dir-test-proj",
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testScanPullRequest(t, test.projectName, test.failOnSecurityIssues)
+			testScanPullRequest(t, test.projectName)
 		})
 	}
 }
 
-func testScanPullRequest(t *testing.T, projectName string, failOnSecurityIssues bool) {
-	config, client, cleanUp := preparePullRequestTest(t, projectName, failOnSecurityIssues)
+func testScanPullRequest(t *testing.T, projectName string) {
+	config, client, cleanUp := preparePullRequestTest(t, projectName)
 	defer cleanUp()
 
 	// Run "frogbot scan pull request"
 	var scanPullRequest ScanPullRequestCmd
 	err := scanPullRequest.Run(config, client, utils.MockHasConnection())
-	if failOnSecurityIssues {
-		assert.EqualErrorf(t, err, SecurityIssueFoundErr, "Error should be: %v, got: %v", SecurityIssueFoundErr, err)
-	} else {
-		assert.NoError(t, err)
-	}
+	assert.NoError(t, err)
 
 	// Check env sanitize
 	err = utils.SanitizeEnv()
@@ -492,7 +481,7 @@ func TestAuditDiffInPullRequest(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			repoConfig, client, cleanUpTest := preparePullRequestTest(t, test.projectName, false)
+			repoConfig, client, cleanUpTest := preparePullRequestTest(t, test.projectName)
 			defer cleanUpTest()
 
 			issuesCollection, _, err := auditPullRequestAndReport(&repoConfig, client)
@@ -514,7 +503,7 @@ func TestToFailTaskStatus(t *testing.T) {
 		failureExpected  bool
 	}{
 		{
-			name:        "fail flag set to false and no violations with fail_pr",
+			name:        "no violations with fail_pr",
 			setFailFlag: false,
 			issuesCollection: issues.ScansIssuesCollection{
 				LicensesViolations: []formats.LicenseViolationRow{{
@@ -541,74 +530,7 @@ func TestToFailTaskStatus(t *testing.T) {
 			failureExpected: false,
 		},
 		{
-			name:        "fail flag set to true, sca vulnerability",
-			setFailFlag: true,
-			issuesCollection: issues.ScansIssuesCollection{
-				ScaVulnerabilities: []formats.VulnerabilityOrViolationRow{
-					{
-						ImpactedDependencyDetails: formats.ImpactedDependencyDetails{
-							ImpactedDependencyName:    "impacted-name",
-							ImpactedDependencyVersion: "1.0.0",
-							SeverityDetails:           formats.SeverityDetails{Severity: "High"},
-							Components: []formats.ComponentRow{
-								{
-									Name:    "vuln-pack-name1",
-									Version: "1.0.0",
-								},
-								{
-									Name:    "vuln-pack-name1",
-									Version: "1.2.3",
-								},
-								{
-									Name:    "vuln-pack-name2",
-									Version: "1.2.3",
-								},
-							},
-						},
-						Cves: []formats.CveRow{{
-							Id: "CVE-2021-1234",
-							Applicability: &formats.Applicability{
-								Status:             "Applicable",
-								ScannerDescription: "scanner",
-								Evidence: []formats.Evidence{
-									{Reason: "reason", Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 2, EndLine: 3, EndColumn: 4, Snippet: "snippet1"}},
-									{Reason: "other reason", Location: formats.Location{File: "file2", StartLine: 5, StartColumn: 6, EndLine: 7, EndColumn: 8, Snippet: "snippet2"}},
-								},
-							},
-						}},
-						JfrogResearchInformation: &formats.JfrogResearchInformation{
-							Remediation: "remediation",
-						},
-						Summary:    "summary",
-						Applicable: "Applicable",
-						IssueId:    "Xray-Id",
-					},
-					{
-						ImpactedDependencyDetails: formats.ImpactedDependencyDetails{
-							ImpactedDependencyName:    "impacted-name2",
-							ImpactedDependencyVersion: "1.0.0",
-							SeverityDetails:           formats.SeverityDetails{Severity: "Low"},
-							Components: []formats.ComponentRow{
-								{
-									Name:    "vuln-pack-name3",
-									Version: "1.0.0",
-								},
-							},
-						},
-						Cves: []formats.CveRow{{
-							Id:            "CVE-1111-2222",
-							Applicability: &formats.Applicability{Status: "Not Applicable"},
-						}},
-						Summary:    "other summary",
-						Applicable: "Not Applicable",
-						IssueId:    "Xray-Id2",
-					},
-				},
-			},
-			failureExpected: true,
-		},
-		{
-			name:        "fail flag is set to false, fail_pr in licenses violation",
+			name:        "fail_pr in licenses violation",
 			setFailFlag: false,
 			issuesCollection: issues.ScansIssuesCollection{
 				LicensesViolations: []formats.LicenseViolationRow{{
@@ -638,19 +560,7 @@ func TestToFailTaskStatus(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			failFlag := test.setFailFlag
-			repo := &utils.Repository{
-				Params: utils.Params{
-					Scan: utils.Scan{
-						FailOnSecurityIssues: &failFlag,
-					},
-					Git: utils.Git{
-						PullRequestSecretComments: false,
-					},
-				},
-			}
-
-			assert.Equal(t, test.failureExpected, toFailTaskStatus(repo, &test.issuesCollection))
+			assert.Equal(t, test.failureExpected, test.issuesCollection.IsFailPrRuleApplied())
 		})
 	}
 }
@@ -1138,14 +1048,11 @@ func TestFilterOutFailedScansIfAllowPartialResultsEnabled(t *testing.T) {
 	}
 }
 
-func preparePullRequestTest(t *testing.T, projectName string, failOnSecurityIssues bool) (utils.Repository, vcsclient.VcsClient, func()) {
+func preparePullRequestTest(t *testing.T, projectName string) (utils.Repository, vcsclient.VcsClient, func()) {
 	params, restoreEnv := utils.VerifyEnv(t)
 
 	// Set test-specific environment variables
 	envVars := map[string]string{}
-	if !failOnSecurityIssues {
-		envVars[utils.FailOnSecurityIssuesEnv] = "false"
-	}
 
 	// Set working directories for multi-dir tests
 	if projectName == "multi-dir-test-proj" {

scanrepository/scanrepository.go

@@ -132,8 +132,7 @@ func (cfp *ScanRepositoryCmd) setCommandPrerequisites(repository *utils.Reposito
 		SetFixableOnly(repository.Params.Scan.FixableOnly).
 		SetConfigProfile(repository.Params.Scan.ConfigProfile).
 		SetSkipAutoInstall(repository.Params.Scan.SkipAutoInstall).
-		SetAllowPartialResults(repository.Params.Scan.AllowPartialResults).
-		SetDisableJas(repository.Params.Scan.DisableJas)
+		SetAllowPartialResults(repository.Params.Scan.AllowPartialResults)
 
 	if cfp.scanDetails, err = cfp.scanDetails.SetMinSeverity(repository.Params.Scan.MinSeverity); err != nil {
 		return

testdata/messages/summarycomment/structure/fix_mr_entitled.md

@@ -13,7 +13,6 @@
 some content
 ```
 
-
 ---
 <div align='center'>
 

testdata/messages/summarycomment/structure/fix_pr_entitled.md

@@ -13,7 +13,6 @@
 some content
 ```
 
-
 ---
 <div align='center'>
 

testdata/messages/summarycomment/structure/fix_simplified_entitled.md

@@ -8,6 +8,5 @@
 some content
 ```
 
-
 ---
 [🐸 JFrog Frogbot](https://jfrog.com/help/r/jfrog-security-user-guide/shift-left-on-security/frogbot)

testdata/messages/summarycomment/structure/summary_comment_issues_mr_entitled.md

@@ -13,7 +13,6 @@
 some content
 ```
 
-
 ---
 <div align='center'>
 

testdata/messages/summarycomment/structure/summary_comment_issues_mr_entitled_with_title.md

@@ -14,7 +14,6 @@
 some content
 ```
 
-
 ---
 <div align='center'>
 

testdata/messages/summarycomment/structure/summary_comment_issues_pr_entitled.md

@@ -13,7 +13,6 @@
 some content
 ```
 
-
 ---
 <div align='center'>