Skip to content

Commit 8daba7e

Browse files
omerziomerzi
authored
Add newlines in pull request comment table (#237)
1 parent 1d208f7 commit 8daba7e

12 files changed

+391
-104
lines changed

‎commands/scanpullrequest.go

+39-58
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,10 @@ package commands
33
import (
44
"context"
55
"errors"
6-
"fmt"
6+
coreconfig "github.com/jfrog/jfrog-cli-core/v2/utils/config"
77
"os"
88
"os/exec"
99
"path/filepath"
10-
"strings"
11-
12-
coreconfig "github.com/jfrog/jfrog-cli-core/v2/utils/config"
1310

1411
"github.com/jfrog/frogbot/commands/utils"
1512
"github.com/jfrog/froggit-go/vcsclient"
@@ -28,8 +25,7 @@ const (
2825
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"
2926
)
3027

31-
type ScanPullRequestCmd struct {
32-
}
28+
type ScanPullRequestCmd struct{}
3329

3430
// Run ScanPullRequest method only works for single repository scan.
3531
// Therefore, the first repository config represents the repository on which Frogbot runs, and it is the only one that matters.
@@ -55,49 +51,58 @@ func scanPullRequest(repoConfig *utils.FrogbotRepoConfig, client vcsclient.VcsCl
5551
if len(repoConfig.Branches) == 0 {
5652
return &utils.ErrMissingEnv{VariableName: utils.GitBaseBranchEnv}
5753
}
54+
5855
// Audit PR code
56+
vulnerabilitiesRows, err := auditPullRequest(repoConfig, client)
57+
if err != nil {
58+
return err
59+
}
60+
61+
// Create pull request message
62+
message := createPullRequestMessage(vulnerabilitiesRows, repoConfig.OutputWriter)
63+
64+
// Add comment to the pull request
65+
if err = client.AddPullRequestComment(context.Background(), repoConfig.RepoOwner, repoConfig.RepoName, message, repoConfig.PullRequestID); err != nil {
66+
return errors.New("couldn't add pull request comment: " + err.Error())
67+
}
68+
69+
// Fail the Frogbot task, if a security issue is found and Frogbot isn't configured to avoid the failure.
70+
if repoConfig.FailOnSecurityIssues != nil && *repoConfig.FailOnSecurityIssues && len(vulnerabilitiesRows) > 0 {
71+
err = errors.New(securityIssueFoundErr)
72+
}
73+
return err
74+
}
75+
76+
func auditPullRequest(repoConfig *utils.FrogbotRepoConfig, client vcsclient.VcsClient) ([]formats.VulnerabilityOrViolationRow, error) {
5977
xrayScanParams := createXrayScanParams(repoConfig.Watches, repoConfig.JFrogProjectKey)
6078
var vulnerabilitiesRows []formats.VulnerabilityOrViolationRow
6179
for _, project := range repoConfig.Projects {
6280
currentScan, isMultipleRoot, err := auditSource(xrayScanParams, project, &repoConfig.Server)
6381
if err != nil {
64-
return err
82+
return nil, err
6583
}
6684
if repoConfig.IncludeAllVulnerabilities {
6785
log.Info("Frogbot is configured to show all vulnerabilities")
6886
allIssuesRows, err := createAllIssuesRows(currentScan, isMultipleRoot)
6987
if err != nil {
70-
return err
88+
return nil, err
7189
}
7290
vulnerabilitiesRows = append(vulnerabilitiesRows, allIssuesRows...)
7391
continue
7492
}
7593
// Audit target code
7694
previousScan, isMultipleRoot, err := auditTarget(client, xrayScanParams, project, repoConfig.Branches[0], &repoConfig.Git, &repoConfig.Server)
7795
if err != nil {
78-
return err
96+
return nil, err
7997
}
8098
newIssuesRows, err := createNewIssuesRows(previousScan, currentScan, isMultipleRoot)
8199
if err != nil {
82-
return err
100+
return nil, err
83101
}
84102
vulnerabilitiesRows = append(vulnerabilitiesRows, newIssuesRows...)
85103
}
86-
87104
log.Info("Xray scan completed")
88-
89-
// Frogbot adds a comment on the PR.
90-
getTitleFunc, getSeverityTagFunc := getCommentFunctions(repoConfig.SimplifiedOutput)
91-
message := createPullRequestMessage(vulnerabilitiesRows, getTitleFunc, getSeverityTagFunc)
92-
err := client.AddPullRequestComment(context.Background(), repoConfig.RepoOwner, repoConfig.RepoName, message, repoConfig.PullRequestID)
93-
if err != nil {
94-
return errors.New("couldn't add pull request comment: " + err.Error())
95-
}
96-
// Fail the Frogbot task, if a security issue is found and Frogbot isn't configured to avoid the failure.
97-
if repoConfig.FailOnSecurityIssues != nil && *repoConfig.FailOnSecurityIssues && len(vulnerabilitiesRows) > 0 {
98-
err = errors.New(securityIssueFoundErr)
99-
}
100-
return err
105+
return vulnerabilitiesRows, nil
101106
}
102107

103108
// Verify that the 'frogbot' GitHub environment was properly configured on the repository
@@ -132,15 +137,6 @@ func verifyGitHubFrogbotEnvironment(client vcsclient.VcsClient, repoConfig *util
132137
return nil
133138
}
134139

135-
func getCommentFunctions(simplifiedOutput bool) (utils.GetTitleFunc, utils.GetSeverityTagFunc) {
136-
if simplifiedOutput {
137-
return utils.GetSimplifiedTitle, func(name utils.IconName) string {
138-
return ""
139-
}
140-
}
141-
return utils.GetBanner, utils.GetSeverityTag
142-
}
143-
144140
// Create vulnerabilities rows. The rows should contain only the new issues added by this PR
145141
func createNewIssuesRows(previousScan, currentScan []services.ScanResponse, isMultipleRoot bool) (vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, err error) {
146142
previousScanAggregatedResults := aggregateScanResults(previousScan)
@@ -337,33 +333,18 @@ func getUniqueID(vulnerability formats.VulnerabilityOrViolationRow) string {
337333
return vulnerability.ImpactedDependencyName + vulnerability.ImpactedDependencyVersion + vulnerability.IssueId
338334
}
339335

340-
func createPullRequestMessage(vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, getBanner utils.GetTitleFunc, getSeverityTag utils.GetSeverityTagFunc) string {
336+
func createPullRequestMessage(vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, writer utils.OutputWriter) string {
341337
if len(vulnerabilitiesRows) == 0 {
342-
return getBanner(utils.NoVulnerabilityBannerSource) + utils.WhatIsFrogbotMd
338+
return writer.NoVulnerabilitiesTitle()
343339
}
340+
tableContent := getTableContent(vulnerabilitiesRows, writer)
341+
return writer.VulnerabiltiesTitle() + writer.TableHeader() + tableContent
342+
}
343+
344+
func getTableContent(vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, writer utils.OutputWriter) string {
344345
var tableContent string
345346
for _, vulnerability := range vulnerabilitiesRows {
346-
var cve string
347-
var directDependencies, directDependenciesVersions strings.Builder
348-
if len(vulnerability.Components) > 0 {
349-
for _, dependency := range vulnerability.Components {
350-
directDependencies.WriteString(fmt.Sprintf("%s; ", dependency.Name))
351-
directDependenciesVersions.WriteString(fmt.Sprintf("%s; ", dependency.Version))
352-
}
353-
}
354-
if len(vulnerability.Cves) > 0 {
355-
cve = vulnerability.Cves[0].Id
356-
}
357-
fixedVersionString := strings.Join(vulnerability.FixedVersions, " ")
358-
tableContent += fmt.Sprintf("\n| %s%8s | %s | %s | %s | %s | %s | %s ",
359-
getSeverityTag(utils.IconName(vulnerability.Severity)),
360-
vulnerability.Severity,
361-
strings.TrimSuffix(directDependencies.String(), "; "),
362-
strings.TrimSuffix(directDependenciesVersions.String(), "; "),
363-
vulnerability.ImpactedDependencyName,
364-
vulnerability.ImpactedDependencyVersion,
365-
fixedVersionString,
366-
cve)
367-
}
368-
return getBanner(utils.VulnerabilitiesBannerSource) + utils.WhatIsFrogbotMd + utils.TableHeader + tableContent
347+
tableContent += writer.TableRow(vulnerability)
348+
}
349+
return tableContent
369350
}

‎commands/scanpullrequest_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ func TestGetNewVulnerabilitiesCaseNoNewVulnerabilities(t *testing.T) {
353353

354354
func TestCreatePullRequestMessageNoVulnerabilities(t *testing.T) {
355355
vulnerabilities := []formats.VulnerabilityOrViolationRow{}
356-
message := createPullRequestMessage(vulnerabilities, utils.GetBanner, utils.GetSeverityTag)
356+
message := createPullRequestMessage(vulnerabilities, &utils.StandardOutput{})
357357

358358
expectedMessageByte, err := os.ReadFile(filepath.Join("testdata", "messages", "novulnerabilities.md"))
359359
assert.NoError(t, err)
@@ -402,7 +402,7 @@ func TestCreatePullRequestMessage(t *testing.T) {
402402
Cves: []formats.CveRow{{Id: "CVE-2022-26652"}},
403403
},
404404
}
405-
message := createPullRequestMessage(vulnerabilities, utils.GetBanner, utils.GetSeverityTag)
405+
message := createPullRequestMessage(vulnerabilities, &utils.StandardOutput{})
406406

407407
expectedMessage := "[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/vulnerabilitiesBanner.png)](https://github.com/jfrog/frogbot#readme)\n\n[What is Frogbot?](https://github.com/jfrog/frogbot#readme)\n\n| SEVERITY | DIRECT DEPENDENCIES | DIRECT DEPENDENCIES VERSIONS | IMPACTED DEPENDENCY NAME | IMPACTED DEPENDENCY VERSION | FIXED VERSIONS | CVE\n:--: | -- | -- | -- | -- | :--: | --\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/highSeverity.png)<br> High | github.com/nats-io/nats-streaming-server | v0.21.0 | github.com/nats-io/nats-streaming-server | v0.21.0 | [0.24.1] | CVE-2022-24450 \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/highSeverity.png)<br> High | github.com/mholt/archiver/v3 | v3.5.1 | github.com/mholt/archiver/v3 | v3.5.1 | | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/mediumSeverity.png)<br> Medium | github.com/nats-io/nats-streaming-server | v0.21.0 | github.com/nats-io/nats-streaming-server | v0.21.0 | [0.24.3] | CVE-2022-26652 "
408408
assert.Equal(t, expectedMessage, message)

‎commands/scanpullrequests.go

+5-17
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
"github.com/jfrog/froggit-go/vcsutils"
87
"sort"
98
"strings"
109

@@ -75,7 +74,7 @@ func shouldScanPullRequest(repo utils.FrogbotRepoConfig, client vcsclient.VcsCli
7574
return true, nil
7675
}
7776
// if this is a Frogbot 'scan results' comment and not 're-scan' request comment, do not scan this pull request.
78-
if isFrogbotResultComment(comment.Content, repo.SimplifiedOutput) {
77+
if repo.OutputWriter.IsFrogbotResultComment(comment.Content) {
7978
return false, nil
8079
}
8180
}
@@ -87,13 +86,6 @@ func isFrogbotRescanComment(comment string) bool {
8786
return strings.Contains(strings.ToLower(strings.TrimSpace(comment)), utils.RescanRequestComment)
8887
}
8988

90-
func isFrogbotResultComment(comment string, simplifiedOutput bool) bool {
91-
if simplifiedOutput {
92-
return strings.HasPrefix(comment, utils.GetSimplifiedTitle(utils.NoVulnerabilityBannerSource)) || strings.HasPrefix(comment, utils.GetSimplifiedTitle(utils.VulnerabilitiesBannerSource))
93-
}
94-
return strings.Contains(comment, utils.GetIconTag(utils.NoVulnerabilityBannerSource)) || strings.Contains(comment, utils.GetIconTag(utils.VulnerabilitiesBannerSource))
95-
}
96-
9789
func downloadAndScanPullRequest(pr vcsclient.PullRequestInfo, repo utils.FrogbotRepoConfig, client vcsclient.VcsClient) error {
9890
// Download the pull request source ("from") branch
9991
params := utils.Params{Git: utils.Git{
@@ -150,15 +142,11 @@ func downloadAndScanPullRequest(pr vcsclient.PullRequestInfo, repo utils.Frogbot
150142
JFrogProjectKey: repo.JFrogProjectKey,
151143
},
152144
}
153-
var simplifiedOutput bool
154-
// Bitbucket server requires a simple output without emojis + images
155-
if repo.GitProvider.String() == vcsutils.BitbucketServer.String() {
156-
simplifiedOutput = true
157-
}
145+
158146
frogbotParams = &utils.FrogbotRepoConfig{
159-
SimplifiedOutput: simplifiedOutput,
160-
Server: repo.Server,
161-
Params: params,
147+
OutputWriter: utils.GetCompatibleOutputWriter(repo.GitProvider),
148+
Server: repo.Server,
149+
Params: params,
162150
}
163151
return scanPullRequest(frogbotParams, client)
164152
}

‎commands/scanpullrequests_test.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import (
1616
)
1717

1818
var gitParams = &utils.FrogbotRepoConfig{
19-
SimplifiedOutput: true,
19+
OutputWriter: &utils.SimplifiedOutput{},
2020
Params: utils.Params{
2121
Git: utils.Git{
2222
RepoOwner: "repo-owner",
@@ -119,14 +119,14 @@ func TestScanAllPullRequestsMultiRepo(t *testing.T) {
119119

120120
configAggregator := utils.FrogbotConfigAggregator{
121121
{
122-
SimplifiedOutput: true,
123-
Server: server,
124-
Params: firstRepoParams,
122+
OutputWriter: &utils.SimplifiedOutput{},
123+
Server: server,
124+
Params: firstRepoParams,
125125
},
126126
{
127-
SimplifiedOutput: true,
128-
Server: server,
129-
Params: secondRepoParams,
127+
OutputWriter: &utils.SimplifiedOutput{},
128+
Server: server,
129+
Params: secondRepoParams,
130130
},
131131
}
132132
mockParams := []MockParams{
@@ -166,9 +166,9 @@ func TestScanAllPullRequests(t *testing.T) {
166166
Git: gitParams.Git,
167167
}
168168
repoParams := &utils.FrogbotRepoConfig{
169-
SimplifiedOutput: true,
170-
Server: server,
171-
Params: params,
169+
OutputWriter: &utils.SimplifiedOutput{},
170+
Server: server,
171+
Params: params,
172172
}
173173
paramsAggregator := utils.FrogbotConfigAggregator{}
174174
paramsAggregator = append(paramsAggregator, *repoParams)

‎commands/utils/consts.go

+3-8
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,6 @@ type IconName string
44
type ImageSource string
55
type vcsProvider string
66

7-
// GetGetTitleFunc, a func to determine the title of Frogbot comment
8-
type GetTitleFunc func(ImageSource) string
9-
10-
// GetGetTitleFunc, a func to determine the table's severity tag in the Frogbot comment
11-
type GetSeverityTagFunc func(IconName) string
12-
137
const (
148
baseResourceUrl = "https://raw.githubusercontent.com/jfrog/frogbot/master/resources/"
159

@@ -66,9 +60,10 @@ const (
6660
GitApiEndpointEnv = "JF_GIT_API_ENDPOINT"
6761

6862
// Comment
69-
TableHeader = "\n| SEVERITY | DIRECT DEPENDENCIES | DIRECT DEPENDENCIES VERSIONS | IMPACTED DEPENDENCY NAME | IMPACTED DEPENDENCY VERSION | FIXED VERSIONS | CVE\n" +
63+
tableHeader = "\n| SEVERITY | DIRECT DEPENDENCIES | DIRECT DEPENDENCIES VERSIONS | IMPACTED DEPENDENCY NAME | IMPACTED DEPENDENCY VERSION | FIXED VERSIONS | CVE\n" +
7064
":--: | -- | -- | -- | -- | :--: | --"
71-
WhatIsFrogbotMd = "\n\n[What is Frogbot?](https://github.com/jfrog/frogbot#readme)\n"
65+
simplifiedTableHeader = "\n| SEVERITY | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY NAME | IMPACTED DEPENDENCY VERSION | FIXED VERSIONS | CVE\n" + ":--: | -- | -- | -- | :--: | --"
66+
WhatIsFrogbotMd = "\n\n[What is Frogbot?](https://github.com/jfrog/frogbot#readme)\n"
7267

7368
// Product ID for usage reporting
7469
productId = "frogbot"

‎commands/utils/git.go

+1
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ func (gm *GitManager) createBranchAndCheckout(branchName string, create bool) er
9999
checkoutConfig := &git.CheckoutOptions{
100100
Create: create,
101101
Branch: getFullBranchName(branchName),
102+
Force: true,
102103
}
103104
worktree, err := gm.repository.Worktree()
104105
if err != nil {

‎commands/utils/params.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ var frogbotConfigPath = filepath.Join(".frogbot", FrogbotConfigFile)
2929
type FrogbotConfigAggregator []FrogbotRepoConfig
3030

3131
type FrogbotRepoConfig struct {
32-
Server coreconfig.ServerDetails
33-
SimplifiedOutput bool
34-
Params `yaml:"params,omitempty"`
32+
Params `yaml:"params,omitempty"`
33+
OutputWriter
34+
Server coreconfig.ServerDetails
3535
}
3636

3737
type Params struct {
@@ -170,9 +170,9 @@ func NewConfigAggregator(configData *FrogbotConfigAggregator, gitParams Git, ser
170170
}
171171
config.Git = gitParams
172172
newConfigAggregator = append(newConfigAggregator, FrogbotRepoConfig{
173-
SimplifiedOutput: isSimplifiedOutput(gitParams.GitProvider),
174-
Server: *server,
175-
Params: config.Params,
173+
OutputWriter: GetCompatibleOutputWriter(gitParams.GitProvider),
174+
Server: *server,
175+
Params: config.Params,
176176
})
177177
}
178178
return newConfigAggregator, nil
@@ -393,7 +393,7 @@ func generateConfigAggregatorFromEnv(gitParams *Git, server *coreconfig.ServerDe
393393
return nil, err
394394
}
395395
repo.Projects = append(repo.Projects, project)
396-
repo.SimplifiedOutput = isSimplifiedOutput(gitParams.GitProvider)
396+
repo.OutputWriter = GetCompatibleOutputWriter(gitParams.GitProvider)
397397
return &FrogbotConfigAggregator{repo}, nil
398398
}
399399

‎commands/utils/simplifiedoutput.go

+45
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package utils
2+
3+
import (
4+
"fmt"
5+
"github.com/jfrog/jfrog-cli-core/v2/xray/formats"
6+
"strings"
7+
)
8+
9+
type SimplifiedOutput struct{}
10+
11+
func (smo *SimplifiedOutput) TableRow(vulnerability formats.VulnerabilityOrViolationRow) string {
12+
var cveId string
13+
if len(vulnerability.Cves) > 0 {
14+
cveId = vulnerability.Cves[0].Id
15+
}
16+
var directDependencies strings.Builder
17+
if len(vulnerability.Components) > 0 {
18+
for _, dependency := range vulnerability.Components {
19+
directDependencies.WriteString(fmt.Sprintf("%s:%s, ", dependency.Name, dependency.Version))
20+
}
21+
}
22+
return fmt.Sprintf("\n| %s | %s | %s | %s | %s | %s |",
23+
vulnerability.Severity,
24+
strings.TrimSuffix(directDependencies.String(), ", "),
25+
vulnerability.ImpactedDependencyName,
26+
vulnerability.ImpactedDependencyVersion,
27+
strings.Join(vulnerability.FixedVersions, " "),
28+
cveId)
29+
}
30+
31+
func (smo *SimplifiedOutput) NoVulnerabilitiesTitle() string {
32+
return GetSimplifiedTitle(NoVulnerabilityBannerSource) + WhatIsFrogbotMd
33+
}
34+
35+
func (smo *SimplifiedOutput) VulnerabiltiesTitle() string {
36+
return GetSimplifiedTitle(VulnerabilitiesBannerSource) + WhatIsFrogbotMd
37+
}
38+
39+
func (smo *SimplifiedOutput) TableHeader() string {
40+
return simplifiedTableHeader
41+
}
42+
43+
func (smo *SimplifiedOutput) IsFrogbotResultComment(comment string) bool {
44+
return strings.HasPrefix(comment, GetSimplifiedTitle(NoVulnerabilityBannerSource)) || strings.HasPrefix(comment, GetSimplifiedTitle(VulnerabilitiesBannerSource))
45+
}

0 commit comments

Comments
 (0)