Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scan PR - Add option to scan source and most common ancestor target c… #763

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ require (
github.com/jfrog/build-info-go v1.10.1
github.com/jfrog/froggit-go v1.16.1
github.com/jfrog/gofrog v1.7.6
github.com/jfrog/jfrog-cli-core/v2 v2.56.1
github.com/jfrog/jfrog-cli-core/v2 v2.56.0
github.com/jfrog/jfrog-cli-security v1.10.1
github.com/jfrog/jfrog-client-go v1.47.1
github.com/jfrog/jfrog-client-go v1.47.0
github.com/jordan-wright/email v4.0.1-0.20210109023952-943e75fe5223+incompatible
github.com/owenrumney/go-sarif/v2 v2.3.1
github.com/stretchr/testify v1.9.0
Expand Down Expand Up @@ -119,7 +119,8 @@ require (
gopkg.in/warnings.v0 v0.1.2 // indirect
)

// replace github.com/jfrog/jfrog-cli-security => github.com/jfrog/jfrog-cli-security dev
// hadarshjfrog:am_ver_1_9_4
replace github.com/jfrog/jfrog-cli-security => github.com/hadarshjfrog/jfrog-cli-security v0.0.0-20241010072437-f2240586d479

// replace github.com/jfrog/jfrog-cli-core/v2 => github.com/jfrog/jfrog-cli-core/v2 dev

Expand Down
12 changes: 6 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,8 @@ github.com/grokify/mogo v0.62.6/go.mod h1:gK6Qf761S7iOxI3LrILjoTOJWdQPgs8LxSPdDm
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0/go.mod h1:hgWBS7lorOAVIJEQMi4ZsPv9hVvWI6+ch50m39Pf2Ks=
github.com/grpc-ecosystem/grpc-gateway/v2 v2.11.3/go.mod h1:o//XUCC/F+yRGJoPO/VU0GSB0f8Nhgmxx0VIRUvaC0w=
github.com/hadarshjfrog/jfrog-cli-security v0.0.0-20241010072437-f2240586d479 h1:0wV0K4gaJ0nzdNhZ98B1RmsRUpa6kcHRNEPqkTK9pxE=
github.com/hadarshjfrog/jfrog-cli-security v0.0.0-20241010072437-f2240586d479/go.mod h1:0vBYBP1jztDf5e25Ww3CkQAA1C609CAccz9NJLoSoRk=
github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ=
Expand Down Expand Up @@ -899,12 +901,10 @@ github.com/jfrog/gofrog v1.7.6 h1:QmfAiRzVyaI7JYGsB7cxfAJePAZTzFz0gRWZSE27c6s=
github.com/jfrog/gofrog v1.7.6/go.mod h1:ntr1txqNOZtHplmaNd7rS4f8jpA5Apx8em70oYEe7+4=
github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYLipdsOFMY=
github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w=
github.com/jfrog/jfrog-cli-core/v2 v2.56.1 h1:+Me+RQx8BYKib+RZLFtGWFftLjEd3NrjVVxJbSYElKU=
github.com/jfrog/jfrog-cli-core/v2 v2.56.1/go.mod h1:+a9VRDizwc+SK2Io6e4Yp8j7hkTeQstQTmNVwrxdh6Q=
github.com/jfrog/jfrog-cli-security v1.10.1 h1:0YfDosXXazUJVQRBPmeoUwvrmEotMSGyE+3ICELmFJE=
github.com/jfrog/jfrog-cli-security v1.10.1/go.mod h1:Z4hS3Ge6LDqOF2vXeO6duuNZyPCEaKjoyoeJ7vGoy54=
github.com/jfrog/jfrog-client-go v1.47.1 h1:VT2v28/usTSP56+i3MC3fgRvZoh6vjRgQgs8xTk+sYU=
github.com/jfrog/jfrog-client-go v1.47.1/go.mod h1:7M/vgei7VGcLjUxwQ/3r9pH3lvDHlt6Q+Gw+YMis/mc=
github.com/jfrog/jfrog-cli-core/v2 v2.56.0 h1:rCNKhfESgsq0o6//gU1mNCvuCboE5BMfycj/RM/gq8k=
github.com/jfrog/jfrog-cli-core/v2 v2.56.0/go.mod h1:D8m0L8GCZiYCY9MjhnWY4egCqyVlU2iZsVA0yysBsVw=
github.com/jfrog/jfrog-client-go v1.47.0 h1:OBMB6TxqziBByjuk6hm0BM30pQwOb3XzjZKf/cmwCeM=
github.com/jfrog/jfrog-client-go v1.47.0/go.mod h1:UxzL9Q4pDoM+HQjSuQiGNakyoJNuxqPSs35/amBJvdY=
github.com/jordan-wright/email v4.0.1-0.20210109023952-943e75fe5223+incompatible h1:jdpOPRN1zP63Td1hDQbZW73xKmzDvZHzVdNYxhnTMDA=
github.com/jordan-wright/email v4.0.1-0.20210109023952-943e75fe5223+incompatible/go.mod h1:1c7szIrayyPPB/987hsnvNzLushdWf4o/79s3P08L8A=
github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1:6v2b51hI/fHJwM22ozAgKL4VKDeJcHhJFhtBdhmNjmU=
Expand Down
59 changes: 57 additions & 2 deletions scanpullrequest/scanpullrequest.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,7 @@
// Download target branch (if needed)
cleanupTarget := func() error { return nil }
if !repoConfig.IncludeAllVulnerabilities {
targetBranchInfo := repoConfig.PullRequestDetails.Target
if targetBranchWd, cleanupTarget, err = utils.DownloadRepoToTempDir(scanDetails.Client(), targetBranchInfo.Owner, targetBranchInfo.Repository, targetBranchInfo.Name); err != nil {
if targetBranchWd, cleanupTarget, err = prepareTargetForScan(repoConfig.PullRequestDetails, scanDetails); err != nil {
return
}
}
Expand All @@ -227,6 +226,62 @@
return
}

func prepareTargetForScan(pullRequestDetails vcsclient.PullRequestInfo, scanDetails *utils.ScanDetails) (targetBranchWd string, cleanupTarget func() error, err error) {
target := pullRequestDetails.Target
// Download target branch
if targetBranchWd, cleanupTarget, err = utils.DownloadRepoToTempDir(scanDetails.Client(), target.Owner, target.Repository, target.Name); err != nil {
return
}
if !scanDetails.Git.UseMostCommonAncestorAsTarget {
return
}
log.Debug("Using most common ancestor commit as target branch commit")
// Get common parent commit between source and target and use it (checkout) to the target branch commit
if e := tryCheckoutToMostCommonAncestor(scanDetails, pullRequestDetails.Source.Name, target.Name, targetBranchWd); e != nil {
log.Warn(fmt.Sprintf("Failed to get best common ancestor commit between source branch: %s and target branch: %s, defaulting to target branch commit. Error: %s", pullRequestDetails.Source.Name, target.Name, e.Error()))
}
return
}

func tryCheckoutToMostCommonAncestor(scanDetails *utils.ScanDetails, baseBranch, headBranch, targetBranchWd string) (err error) {
repositoryInfo, err := scanDetails.Client().GetRepositoryInfo(context.Background(), scanDetails.RepoOwner, scanDetails.RepoName)
if err != nil {
return
}
scanDetails.Git.RepositoryCloneUrl = repositoryInfo.CloneInfo.HTTP
bestAncestorHash, err := getMostCommonAncestorCommitHash(scanDetails, baseBranch, headBranch)
if err != nil {
return
}
return checkoutToCommitAtTempWorkingDir(scanDetails, bestAncestorHash, targetBranchWd)
}

func getMostCommonAncestorCommitHash(scanDetails *utils.ScanDetails, baseBranch, headBranch string) (hash string, err error) {
gitManager, err := utils.NewGitManager().SetAuth(scanDetails.Username, scanDetails.Token).SetRemoteGitUrl(scanDetails.Git.RepositoryCloneUrl)
if err != nil {
return
}
return gitManager.GetMostCommonAncestorHash(baseBranch, headBranch)
}

func checkoutToCommitAtTempWorkingDir(scanDetails *utils.ScanDetails, commitHash, wd string) (err error) {
// Change working directory to the temp target branch directory
cwd, err := os.Getwd()
if err != nil {
return
}
if err = os.Chdir(wd); err != nil {
return
}
defer os.Chdir(cwd)

Check failure on line 276 in scanpullrequest/scanpullrequest.go

View workflow job for this annotation

GitHub Actions / Static-Check

Error return value of `os.Chdir` is not checked (errcheck)
// Load .git info in directory and Checkout to the commit hash
gitManager, err := utils.NewGitManager().SetAuth(scanDetails.Username, scanDetails.Token).SetRemoteGitUrl(scanDetails.Git.RepositoryCloneUrl)
if err != nil {
return
}
return gitManager.CheckoutToHash(commitHash, wd)
}

func getAllIssues(results *securityutils.Results, allowedLicenses []string) (*utils.IssuesCollection, error) {
log.Info("Frogbot is configured to show all vulnerabilities")
scanResults := results.ExtendedScanResults
Expand Down
9 changes: 5 additions & 4 deletions utils/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ const (
GitUsernameEnv = "JF_GIT_USERNAME"

// Git naming template environment variables
BranchNameTemplateEnv = "JF_BRANCH_NAME_TEMPLATE"
CommitMessageTemplateEnv = "JF_COMMIT_MESSAGE_TEMPLATE"
PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE"
PullRequestCommentTitleEnv = "JF_PR_COMMENT_TITLE"
BranchNameTemplateEnv = "JF_BRANCH_NAME_TEMPLATE"
CommitMessageTemplateEnv = "JF_COMMIT_MESSAGE_TEMPLATE"
PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE"
PullRequestCommentTitleEnv = "JF_PR_COMMENT_TITLE"
UseMostCommonAncestorAsTargetEnv = "JF_USE_MOST_COMMON_ANCESTOR_AS_TARGET"

// Repository environment variables - Ignored if the frogbot-config.yml file is used
InstallCommandEnv = "JF_INSTALL_DEPS_CMD"
Expand Down
79 changes: 79 additions & 0 deletions utils/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"net/http"
// "os/exec"
"regexp"
"strings"
"time"
Expand Down Expand Up @@ -154,6 +155,64 @@ func (gm *GitManager) Checkout(branchName string) error {
return nil
}

func (gm *GitManager) CheckoutToHash(hash, targetBranchWd string) error {
log.Debug("Running git fetch...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate debug log, as similar one exists in Fetch itself

if err := gm.Fetch(); err != nil {
return err
}
log.Debug("Running git checkout to hash:", hash)
if err := gm.createBranchAndCheckoutToHash(hash, false); err != nil {
return fmt.Errorf("'git checkout %s' failed with error: %s", hash, err.Error())
}
return nil
}

func (gm *GitManager) Fetch() error {
log.Debug("Running git fetch...")
err := gm.localGitRepository.Fetch(&git.FetchOptions{
RemoteName: gm.remoteName,
RemoteURL: gm.remoteGitUrl,
Auth: gm.auth,
})
if err != nil && err != git.NoErrAlreadyUpToDate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when the error is NoErrAlreadyUpToDate? will the fetch is still performed when we got this error?

return fmt.Errorf("git fetch failed with error: %s", err.Error())
}
return nil
}

func (gm *GitManager) GetMostCommonAncestorHash(baseBranch, headBranch string) (string, error) {
// Get the commit of the base branch
baseCommitHash, err := gm.localGitRepository.ResolveRevision(plumbing.Revision(baseBranch))
if err != nil {
return "", err
}
baseCommit, err := gm.localGitRepository.CommitObject(*baseCommitHash)
if err != nil {
return "", err
}
// Get the commit of the head branch
Copy link
Contributor

@eranturgeman eranturgeman Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Get the commit of the head branch
// Get the HEAD commit of the target branch

Its just a bit more clear term than "head branch"

headCommitHash, err := gm.localGitRepository.ResolveRevision(plumbing.Revision(headBranch))
if err != nil {
return "", err
}
headCommit, err := gm.localGitRepository.CommitObject(*headCommitHash)
if err != nil {
return "", err
}
// Get the most common ancestor
log.Debug(fmt.Sprintf("Finding common ancestor between %s and %s...", baseBranch, headBranch))
ancestorCommit, err := baseCommit.MergeBase(headCommit)
if err != nil {
return "", err
}
if len(ancestorCommit) == 0 {
return "", fmt.Errorf("no common ancestor found for %s and %s", baseBranch, headBranch)
} else if len(ancestorCommit) > 1 {
return "", fmt.Errorf("more than one common ancestor found for %s and %s", baseBranch, headBranch)
}
return ancestorCommit[0].Hash.String(), nil
}

func (gm *GitManager) Clone(destinationPath, branchName string) error {
if gm.dryRun {
// "Clone" the repository from the testdata folder
Expand Down Expand Up @@ -201,6 +260,26 @@ func (gm *GitManager) CreateBranchAndCheckout(branchName string, keepLocalChange
return err
}

func (gm *GitManager) createBranchAndCheckoutToHash(hash string, keepLocalChanges bool) error {
var checkoutConfig *git.CheckoutOptions
if keepLocalChanges {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the single usage of this new function you pass keepLocalChanges=false, so why do we need to even refer to that in this function?
Is this intended for future usage or plans? If not we can consider deleting it

checkoutConfig = &git.CheckoutOptions{
Hash: plumbing.NewHash(hash),
Keep: true,
}
} else {
checkoutConfig = &git.CheckoutOptions{
Hash: plumbing.NewHash(hash),
Force: true,
}
}
worktree, err := gm.localGitRepository.Worktree()
if err != nil {
return err
}
return worktree.Checkout(checkoutConfig)
}

func (gm *GitManager) createBranchAndCheckout(branchName string, create bool, keepLocalChanges bool) error {
var checkoutConfig *git.CheckoutOptions
if keepLocalChanges {
Expand Down
30 changes: 18 additions & 12 deletions utils/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,18 +278,19 @@ func (jp *JFrogPlatform) setDefaultsIfNeeded() (err error) {
type Git struct {
GitProvider vcsutils.VcsProvider
vcsclient.VcsInfo
RepoOwner string
RepoName string `yaml:"repoName,omitempty"`
Branches []string `yaml:"branches,omitempty"`
BranchNameTemplate string `yaml:"branchNameTemplate,omitempty"`
CommitMessageTemplate string `yaml:"commitMessageTemplate,omitempty"`
PullRequestTitleTemplate string `yaml:"pullRequestTitleTemplate,omitempty"`
PullRequestCommentTitle string `yaml:"pullRequestCommentTitle,omitempty"`
AvoidExtraMessages bool `yaml:"avoidExtraMessages,omitempty"`
EmailAuthor string `yaml:"emailAuthor,omitempty"`
AggregateFixes bool `yaml:"aggregateFixes,omitempty"`
PullRequestDetails vcsclient.PullRequestInfo
RepositoryCloneUrl string
UseMostCommonAncestorAsTarget bool `yaml:"useMostCommonAncestorAsTarget,omitempty"`
RepoOwner string
RepoName string `yaml:"repoName,omitempty"`
Branches []string `yaml:"branches,omitempty"`
BranchNameTemplate string `yaml:"branchNameTemplate,omitempty"`
CommitMessageTemplate string `yaml:"commitMessageTemplate,omitempty"`
PullRequestTitleTemplate string `yaml:"pullRequestTitleTemplate,omitempty"`
PullRequestCommentTitle string `yaml:"pullRequestCommentTitle,omitempty"`
AvoidExtraMessages bool `yaml:"avoidExtraMessages,omitempty"`
EmailAuthor string `yaml:"emailAuthor,omitempty"`
AggregateFixes bool `yaml:"aggregateFixes,omitempty"`
PullRequestDetails vcsclient.PullRequestInfo
RepositoryCloneUrl string
}

func (g *Git) setDefaultsIfNeeded(gitParamsFromEnv *Git, commandName string) (err error) {
Expand Down Expand Up @@ -329,6 +330,11 @@ func (g *Git) extractScanPullRequestEnvParams(gitParamsFromEnv *Git) (err error)
if g.PullRequestCommentTitle == "" {
g.PullRequestCommentTitle = getTrimmedEnv(PullRequestCommentTitleEnv)
}
if !g.UseMostCommonAncestorAsTarget {
if g.UseMostCommonAncestorAsTarget, err = getBoolEnv(UseMostCommonAncestorAsTargetEnv, true); err != nil {
return
}
}
g.AvoidExtraMessages, err = getBoolEnv(AvoidExtraMessages, false)
return
}
Expand Down
Loading