Refactor output writer (#514)

scanpullrequest/scanallpullrequests.go

@@ -4,9 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
+
 	"github.com/jfrog/frogbot/utils"
+	"github.com/jfrog/frogbot/utils/outputwriter"
 	"github.com/jfrog/jfrog-client-go/utils/log"
-	"strings"
 
 	"github.com/jfrog/froggit-go/vcsclient"
 )
@@ -64,18 +65,14 @@ func shouldScanPullRequest(repo utils.Repository, client vcsclient.VcsClient, pr
 
 	for _, comment := range pullRequestsComments {
 		// If this a 're-scan' request comment
-		if isFrogbotRescanComment(comment.Content) {
+		if utils.IsFrogbotRescanComment(comment.Content) {
 			return true, nil
 		}
 		// if this is a Frogbot 'scan results' comment and not 're-scan' request comment, do not scan this pull request.
-		if repo.OutputWriter.IsFrogbotResultComment(comment.Content) {
+		if outputwriter.IsFrogbotSummaryComment(repo.OutputWriter, comment.Content) {
 			return false, nil
 		}
 	}
 	// This is a new pull request, and it therefore should be scanned.
 	return true, nil
 }
-
-func isFrogbotRescanComment(comment string) bool {
-	return strings.Contains(strings.ToLower(strings.TrimSpace(comment)), utils.RescanRequestComment)
-}

scanpullrequest/scanallpullrequests_test.go

@@ -16,16 +16,19 @@ import (
 	"time"
 )
 
-var gitParams = &utils.Repository{
-	OutputWriter: &outputwriter.SimplifiedOutput{},
-	Params: utils.Params{
-		Git: utils.Git{
-			RepoOwner: "repo-owner",
-			Branches:  []string{"master"},
-			RepoName:  "repo-name",
+var (
+	gitParams = &utils.Repository{
+		OutputWriter: &outputwriter.SimplifiedOutput{},
+		Params: utils.Params{
+			Git: utils.Git{
+				RepoOwner: "repo-owner",
+				Branches:  []string{"master"},
+				RepoName:  "repo-name",
+			},
 		},
-	},
-}
+	}
+	allPrIntegrationPath = filepath.Join(outputwriter.TestMessagesDir, "integration")
+)
 
 type MockParams struct {
 	repoName         string
@@ -140,13 +143,13 @@ func TestScanAllPullRequestsMultiRepo(t *testing.T) {
 	err := scanAllPullRequestsCmd.Run(configAggregator, client)
 	if assert.NoError(t, err) {
 		assert.Len(t, frogbotMessages, 4)
-		expectedMessage := "<div align='center'>\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n</div>\n\n\n## 📦 Vulnerable Dependencies\n\n### ✍️ Summary\n\n<div align=\"center\">\n\n| SEVERITY                | CONTEXTUAL ANALYSIS                  | DIRECT DEPENDENCIES                  | IMPACTED DEPENDENCY                   | FIXED VERSIONS                       | CVES                       |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/notApplicableCritical.png)<br>Critical | Not Applicable | minimist:1.2.5 | minimist:1.2.5 | [0.2.4]<br>[1.2.6] | CVE-2021-44906 |\n\n</div>\n\n## 🔬 Research Details\n\n\n**Description:**\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y`  is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n**Remediation:**\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n---\n<div align=\"center\">\n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>"
+		expectedMessage := outputwriter.GetOutputFromFile(t, filepath.Join(allPrIntegrationPath, "test_proj_with_vulnerability_standard.md"))
 		assert.Equal(t, expectedMessage, frogbotMessages[0])
-		expectedMessage = "<div align='center'>\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/noVulnerabilityBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n</div>\n\n\n---\n<div align=\"center\">\n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>"
+		expectedMessage = outputwriter.GetPRSummaryContentNoIssues(t, outputwriter.TestSummaryCommentDir, true, false)
 		assert.Equal(t, expectedMessage, frogbotMessages[1])
-		expectedMessage = "<div align='center'>\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n</div>\n\n\n## 📦 Vulnerable Dependencies\n\n### ✍️ Summary\n\n<div align=\"center\">\n\n| SEVERITY                | CONTEXTUAL ANALYSIS                  | DIRECT DEPENDENCIES                  | IMPACTED DEPENDENCY                   | FIXED VERSIONS                       | CVES                       |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)<br>    High | Undetermined | pip-example:1.2.3 | pyjwt:1.7.1 | [2.4.0] | CVE-2022-29217 |\n\n</div>\n\n## 🔬 Research Details\n\n\n**Description:**\n[PyJWT](https://pypi.org/project/PyJWT) is a Python implementation of the RFC 7519 standard (JSON Web Tokens). [JSON Web Tokens](https://jwt.io/) are an open, industry standard method for representing claims securely between two parties. A JWT comes with an inline signature that is meant to be verified by the receiving application. JWT supports multiple standard algorithms, and the algorithm itself is **specified in the JWT token itself**.\n\nThe PyJWT library uses the signature-verification algorithm that is specified in the JWT token (that is completely attacker-controlled), however - it requires the validating application to pass an `algorithms` kwarg that specifies the expected algorithms in order to avoid key confusion. Unfortunately -  a non-default value `algorithms=jwt.algorithms.get_default_algorithms()` exists that allows all algorithms.\nThe PyJWT library also tries to mitigate key confusions in this case, by making sure that public keys are not used as an HMAC secret. For example, HMAC secrets that begin with `-----BEGIN PUBLIC KEY-----` are rejected when encoding a JWT.\n\nIt has been discovered that due to missing key-type checks, in cases where -\n1. The vulnerable application expects to receive a JWT signed with an Elliptic-Curve key (one of the algorithms `ES256`, `ES384`, `ES512`, `EdDSA`)\n2. The vulnerable application decodes the JWT token using the non-default kwarg `algorithms=jwt.algorithms.get_default_algorithms()` (or alternatively, `algorithms` contain both an HMAC-based algorithm and an EC-based algorithm)\n\nAn attacker can create an HMAC-signed (ex. `HS256`) JWT token, using the (well-known!) EC public key as the HMAC key. The validating application will accept this JWT token as a valid token.\n\nFor example, an application might have planned to validate an `EdDSA`-signed token that was generated as follows -\n```python\n# Making a good jwt token that should work by signing it with the private key\nencoded_good = jwt.encode({\"test\": 1234}, priv_key_bytes, algorithm=\"EdDSA\")\n```\nAn attacker in posession of the public key can generate an `HMAC`-signed token to confuse PyJWT - \n```python\n# Using HMAC with the public key to trick the receiver to think that the public key is a HMAC secret\nencoded_bad = jwt.encode({\"test\": 1234}, pub_key_bytes, algorithm=\"HS256\")\n```\n\nThe following vulnerable `decode` call will accept BOTH of the above tokens as valid - \n```\ndecoded = jwt.decode(encoded_good, pub_key_bytes, \nalgorithms=jwt.algorithms.get_default_algorithms())\n```\n**Remediation:**\n##### Development mitigations\n\nUse a specific algorithm instead of `jwt.algorithms.get_default_algorithms`.\nFor example, replace the following call - \n`jwt.decode(encoded_jwt, pub_key_bytes, algorithms=jwt.algorithms.get_default_algorithms())`\nWith -\n`jwt.decode(encoded_jwt, pub_key_bytes, algorithms=[\"ES256\"])`\n\n\n---\n<div align=\"center\">\n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>"
+		expectedMessage = outputwriter.GetOutputFromFile(t, filepath.Join(allPrIntegrationPath, "test_proj_pip_with_vulnerability.md"))
 		assert.Equal(t, expectedMessage, frogbotMessages[2])
-		expectedMessage = "<div align='center'>\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/noVulnerabilityBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n</div>\n\n\n---\n<div align=\"center\">\n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n</div>"
+		expectedMessage = outputwriter.GetPRSummaryContentNoIssues(t, outputwriter.TestSummaryCommentDir, true, false)
 		assert.Equal(t, expectedMessage, frogbotMessages[3])
 	}
 }
@@ -182,9 +185,9 @@ func TestScanAllPullRequests(t *testing.T) {
 	err := scanAllPullRequestsCmd.Run(paramsAggregator, client)
 	assert.NoError(t, err)
 	assert.Len(t, frogbotMessages, 2)
-	expectedMessage := "**🚨 Frogbot scanned this pull request and found the below:**\n\n---\n## 📦 Vulnerable Dependencies\n---\n\n### ✍️ Summary\n\n| SEVERITY                | CONTEXTUAL ANALYSIS                  | DIRECT DEPENDENCIES                  | IMPACTED DEPENDENCY                   | FIXED VERSIONS                       | CVES                       |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| Critical | Not Applicable | minimist:1.2.5 | minimist:1.2.5 | [0.2.4], [1.2.6] | CVE-2021-44906 |\n\n---\n## 🔬 Research Details\n---\n\n\n#### [ CVE-2021-44906 ] minimist 1.2.5\n\n\n**Description:**\n[Minimist](https://github.com/substack/minimist) is a simple and very popular argument parser. It is used by more than 14 million by Mar 2022. This package developers stopped developing it since April 2020 and its community released a [newer version](https://github.com/meszaros-lajos-gyorgy/minimist-lite) supported by the community.\n\n\nAn incomplete fix for [CVE-2020-7598](https://nvd.nist.gov/vuln/detail/CVE-2020-7598) partially blocked prototype pollution attacks. Researchers discovered that it does not check for constructor functions which means they can be overridden. This behavior can be triggered easily when using it insecurely (which is the common usage). For example:\n```\nvar argv = parse(['--_.concat.constructor.prototype.y', '123']);\nt.equal((function(){}).foo, undefined);\nt.equal(argv.y, undefined);\n```\nIn this example, `prototype.y`  is assigned with `123` which will be derived to every newly created object. \n\nThis vulnerability can be triggered when the attacker-controlled input is parsed using Minimist without any validation. As always with prototype pollution, the impact depends on the code that follows the attack, but denial of service is almost always guaranteed.\n**Remediation:**\n##### Development mitigations\n\nAdd the `Object.freeze(Object.prototype);` directive once at the beginning of your main JS source code file (ex. `index.js`), preferably after all your `require` directives. This will prevent any changes to the prototype object, thus completely negating prototype pollution attacks.\n\n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)"
+	expectedMessage := outputwriter.GetOutputFromFile(t, filepath.Join(allPrIntegrationPath, "test_proj_with_vulnerability_simplified.md"))
 	assert.Equal(t, expectedMessage, frogbotMessages[0])
-	expectedMessage = "**👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.** \n\n[🐸 JFrog Frogbot](https://github.com/jfrog/frogbot#readme)"
+	expectedMessage = outputwriter.GetPRSummaryContentNoIssues(t, outputwriter.TestSummaryCommentDir, true, true)
 	assert.Equal(t, expectedMessage, frogbotMessages[1])
 }
 

scanpullrequest/scanpullrequest.go

@@ -4,9 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"golang.org/x/exp/slices"
 	"os"
 
+	"golang.org/x/exp/slices"
+
 	"github.com/jfrog/frogbot/utils"
 	"github.com/jfrog/froggit-go/vcsclient"
 	"github.com/jfrog/froggit-go/vcsutils"
@@ -104,6 +105,8 @@ func scanPullRequest(repo *utils.Repository, client vcsclient.VcsClient) (err er
 			return
 		}
 	}
+
+	// Handle PR comments for scan output
 	if err = utils.HandlePullRequestCommentsAfterScan(issues, repo, client, int(pullRequestDetails.ID)); err != nil {
 		return
 	}

scanpullrequest/scanpullrequest_test.go

@@ -502,7 +502,6 @@ func TestGetAllIssues(t *testing.T) {
 			EntitledForJas: true,
 		},
 	}
-
 	expectedOutput := &utils.IssuesCollection{
 		Vulnerabilities: []formats.VulnerabilityOrViolationRow{
 			{
@@ -809,13 +808,10 @@ func createGitLabHandler(t *testing.T, projectName string) http.HandlerFunc {
 			assert.NotEmpty(t, buf.String())
 
 			var expectedResponse []byte
-			switch {
-			case strings.Contains(projectName, "multi-dir"):
-				expectedResponse, err = os.ReadFile(filepath.Join("..", "expectedResponseMultiDir.json"))
-			case strings.Contains(projectName, "pip"):
-				expectedResponse, err = os.ReadFile(filepath.Join("..", "expectedResponsePip.json"))
-			default:
-				expectedResponse, err = os.ReadFile(filepath.Join("..", "expectedResponse.json"))
+			if strings.Contains(projectName, "multi-dir") {
+				expectedResponse = outputwriter.GetJsonBodyOutputFromFile(t, filepath.Join("..", "expected_response_multi_dir.md"))
+			} else {
+				expectedResponse = outputwriter.GetJsonBodyOutputFromFile(t, filepath.Join("..", "expected_response.md"))
 			}
 			assert.NoError(t, err)
 			assert.JSONEq(t, string(expectedResponse), buf.String())

scanrepository/scanrepository.go

@@ -363,7 +363,7 @@ func (cfp *ScanRepositoryCmd) preparePullRequestDetails(vulnerabilitiesDetails .
 		return cfp.gitManager.GenerateAggregatedPullRequestTitle(cfp.projectTech), "", nil
 	}
 	vulnerabilitiesRows := utils.ExtractVulnerabilitiesDetailsToRows(vulnerabilitiesDetails)
-	prBody = cfp.OutputWriter.VulnerabilitiesTitle(false) + "\n" + cfp.OutputWriter.VulnerabilitiesContent(vulnerabilitiesRows) + cfp.OutputWriter.UntitledForJasMsg() + cfp.OutputWriter.Footer()
+	prBody = utils.GenerateFixPullRequestDetails(vulnerabilitiesRows, cfp.OutputWriter)
 	if cfp.aggregateFixes {
 		var scanHash string
 		if scanHash, err = utils.VulnerabilityDetailsToMD5Hash(vulnerabilitiesRows...); err != nil {