Show secrets detected at PR scan (#793)

testdata/messages/reviewcomment/secrets/secret_review_content_no_ca_simplified.md

@@ -0,0 +1,43 @@
+
+
+---
+## 🗝️ Secret Detected
+
+---
+| Severity                | Finding                  |
+| :---------------------: | :-----------------------------------: |
+| Medium | Secret keys were found |
+
+---
+### Full description
+
+---
+Storing hardcoded secrets in your source code or binary artifact could lead to several risks.
+
+If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.
+
+## Best practices
+
+Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -
+
+* ### Environment Variables
+
+Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
+`SECRET_VAR=MySecret ./my_application`
+This way, `MySecret` does not have to be hardcoded into `my_application`.
+
+Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.
+
+* ### Secret management services
+
+External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -
+
+* [Hashicorp Vault](https://www.vaultproject.io)
+* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
+* [Google Cloud KMS](https://cloud.google.com/security-key-management)
+
+## Least-privilege principle
+
+Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
+For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
+That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.

testdata/messages/reviewcomment/secrets/secret_review_content_no_ca_standard.md

@@ -0,0 +1,46 @@
+
+## 🗝️ Secret Detected
+<div align='center'>
+
+| Severity                | Finding                  |
+| :---------------------: | :-----------------------------------: |
+| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)<br>  Medium | Secret keys were found |
+
+</div>
+
+<details>
+<summary> <b>Full description</b> </summary>
+<br>
+
+Storing hardcoded secrets in your source code or binary artifact could lead to several risks.
+
+If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.
+
+## Best practices
+
+Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -
+
+* ### Environment Variables
+
+Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
+`SECRET_VAR=MySecret ./my_application`
+This way, `MySecret` does not have to be hardcoded into `my_application`.
+
+Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.
+
+* ### Secret management services
+
+External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -
+
+* [Hashicorp Vault](https://www.vaultproject.io)
+* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
+* [Google Cloud KMS](https://cloud.google.com/security-key-management)
+
+## Least-privilege principle
+
+Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
+For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
+That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.
+
+
+</details>

testdata/messages/reviewcomment/secrets/secret_review_content_simplified.md

@@ -0,0 +1,43 @@
+
+
+---
+## 🗝️ Secret Detected
+
+---
+| Severity                | Finding                  | Status                  |
+| :---------------------: | :-----------------------------------: | :-----------------------------------: |
+| Medium | Secret keys were found | Active |
+
+---
+### Full description
+
+---
+Storing hardcoded secrets in your source code or binary artifact could lead to several risks.
+
+If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.
+
+## Best practices
+
+Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -
+
+* ### Environment Variables
+
+Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
+`SECRET_VAR=MySecret ./my_application`
+This way, `MySecret` does not have to be hardcoded into `my_application`.
+
+Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.
+
+* ### Secret management services
+
+External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -
+
+* [Hashicorp Vault](https://www.vaultproject.io)
+* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
+* [Google Cloud KMS](https://cloud.google.com/security-key-management)
+
+## Least-privilege principle
+
+Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
+For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
+That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.

testdata/messages/reviewcomment/secrets/secret_review_content_standard.md

@@ -0,0 +1,46 @@
+
+## 🗝️ Secret Detected
+<div align='center'>
+
+| Severity                | Finding                  | Status                  |
+| :---------------------: | :-----------------------------------: | :-----------------------------------: |
+| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)<br>  Medium | Secret keys were found | Active |
+
+</div>
+
+<details>
+<summary> <b>Full description</b> </summary>
+<br>
+
+Storing hardcoded secrets in your source code or binary artifact could lead to several risks.
+
+If the secret is associated with a wide scope of privileges, attackers could extract it from the source code or binary artifact and use it maliciously to attack many targets. For example, if the hardcoded password gives high-privilege access to an AWS account, the attackers may be able to query/modify company-wide sensitive data without per-user authentication.
+
+## Best practices
+
+Use safe storage when storing high-privilege secrets such as passwords and tokens, for example -
+
+* ### Environment Variables
+
+Environment variables are set outside of the application code, and can be dynamically passed to the application only when needed, for example -
+`SECRET_VAR=MySecret ./my_application`
+This way, `MySecret` does not have to be hardcoded into `my_application`.
+
+Note that if your entire binary artifact is published (ex. a Docker container published to Docker Hub), the value for the environment variable must not be stored in the artifact itself (ex. inside the `Dockerfile` or one of the container's files) but rather must be passed dynamically, for example in the `docker run` call as an argument.
+
+* ### Secret management services
+
+External vendors offer cloud-based secret management services, that provide proper access control to each secret. The given access to each secret can be dynamically modified or even revoked. Some examples include -
+
+* [Hashicorp Vault](https://www.vaultproject.io)
+* [AWS KMS](https://aws.amazon.com/kms) (Key Management Service)
+* [Google Cloud KMS](https://cloud.google.com/security-key-management)
+
+## Least-privilege principle
+
+Storing a secret in a hardcoded manner can be made safer, by making sure the secret grants the least amount of privilege as needed by the application.
+For example - if the application needs to read a specific table from a specific database, and the secret grants access to perform this operation **only** (meaning - no access to other tables, no write access at all) then the damage from any secret leaks is mitigated.
+That being said, it is still not recommended to store secrets in a hardcoded manner, since this type of storage does not offer any way to revoke or moderate the usage of the secret.
+
+
+</details>

utils/comment.go

@@ -25,6 +25,7 @@ const (
 	ApplicableComment ReviewCommentType = "Applicable"
 	IacComment        ReviewCommentType = "Iac"
 	SastComment       ReviewCommentType = "Sast"
+	SecretComment     ReviewCommentType = "Secrets"
 
 	RescanRequestComment   = "rescan"
 	commentRemovalErrorMsg = "An error occurred while attempting to remove older Frogbot pull request comments:"
@@ -201,6 +202,12 @@ func getNewReviewComments(repo *Repository, issues *IssuesCollection) (commentsT
 	for _, sast := range issues.Sast {
 		commentsToAdd = append(commentsToAdd, generateReviewComment(SastComment, sast.Location, generateSourceCodeReviewContent(SastComment, sast, writer)))
 	}
+	if !repo.Params.PullRequestSecretComments {
+		return
+	}
+	for _, secret := range issues.Secrets {
+		commentsToAdd = append(commentsToAdd, generateReviewComment(SecretComment, secret.Location, generateSourceCodeReviewContent(SecretComment, secret, writer)))
+	}
 	return
 }
 
@@ -252,6 +259,18 @@ func generateSourceCodeReviewContent(commentType ReviewCommentType, issue format
 			issue.CodeFlow,
 			writer,
 		), writer)
+	case SecretComment:
+		applicability := ""
+		if issue.Applicability != nil {
+			applicability = issue.Applicability.Status
+		}
+		return outputwriter.GenerateReviewCommentContent(outputwriter.SecretReviewContent(
+			issue.Severity,
+			issue.Finding,
+			issue.ScannerDescription,
+			applicability,
+			writer,
+		), writer)
 	}
 	return
 }

utils/comment_test.go

@@ -50,11 +50,12 @@ func TestGetFrogbotReviewComments(t *testing.T) {
 }
 
 func TestGetNewReviewComments(t *testing.T) {
-	repo := &Repository{OutputWriter: &outputwriter.StandardOutput{}}
+	writer := &outputwriter.StandardOutput{}
 	testCases := []struct {
-		name           string
-		issues         *IssuesCollection
-		expectedOutput []ReviewComment
+		name                    string
+		generateSecretsComments bool
+		issues                  *IssuesCollection
+		expectedOutput          []ReviewComment
 	}{
 		{
 			name: "No issues for review comments",
@@ -92,6 +93,73 @@ func TestGetNewReviewComments(t *testing.T) {
 			},
 			expectedOutput: []ReviewComment{},
 		},
+		{
+			name:                    "Secret review comments",
+			generateSecretsComments: true,
+			issues: &IssuesCollection{
+				Vulnerabilities: []formats.VulnerabilityOrViolationRow{
+					{
+						Summary:    "summary-2",
+						Applicable: "Applicable",
+						IssueId:    "XRAY-2",
+						ImpactedDependencyDetails: formats.ImpactedDependencyDetails{
+							SeverityDetails:        formats.SeverityDetails{Severity: "low"},
+							ImpactedDependencyName: "component-C",
+						},
+						Cves:       []formats.CveRow{{Id: "CVE-2023-4321"}},
+						Technology: techutils.Npm,
+					},
+				},
+				Secrets: []formats.SourceCodeRow{
+					{
+						SeverityDetails: formats.SeverityDetails{
+							Severity:         "High",
+							SeverityNumValue: 13,
+						},
+						Finding:       "secret finding",
+						Applicability: &formats.Applicability{Status: "Inactive"},
+						Location: formats.Location{
+							File:        "index.js",
+							StartLine:   5,
+							StartColumn: 6,
+							EndLine:     7,
+							EndColumn:   8,
+							Snippet:     "access token exposed",
+						},
+					},
+				},
+			},
+			expectedOutput: []ReviewComment{
+				{
+					Location: formats.Location{
+						File:        "index.js",
+						StartLine:   5,
+						StartColumn: 6,
+						EndLine:     7,
+						EndColumn:   8,
+						Snippet:     "access token exposed",
+					},
+					Type: SecretComment,
+					CommentInfo: vcsclient.PullRequestComment{
+						CommentInfo: vcsclient.CommentInfo{
+							Content: outputwriter.GenerateReviewCommentContent(outputwriter.SecretReviewContent("High", "secret finding", "", "Inactive", writer), writer),
+						},
+						PullRequestDiff: vcsclient.PullRequestDiff{
+							OriginalFilePath:    "index.js",
+							OriginalStartLine:   5,
+							OriginalStartColumn: 6,
+							OriginalEndLine:     7,
+							OriginalEndColumn:   8,
+							NewFilePath:         "index.js",
+							NewStartLine:        5,
+							NewStartColumn:      6,
+							NewEndLine:          7,
+							NewEndColumn:        8,
+						},
+					},
+				},
+			},
+		},
 		{
 			name: "With issues for review comments",
 			issues: &IssuesCollection{
@@ -156,7 +224,7 @@ func TestGetNewReviewComments(t *testing.T) {
 					Type: ApplicableComment,
 					CommentInfo: vcsclient.PullRequestComment{
 						CommentInfo: vcsclient.CommentInfo{
-							Content: outputwriter.GenerateReviewCommentContent(outputwriter.ApplicableCveReviewContent("Low", "", "", "CVE-2023-4321", "summary-2", "component-C:", "", repo.OutputWriter), repo.OutputWriter),
+							Content: outputwriter.GenerateReviewCommentContent(outputwriter.ApplicableCveReviewContent("Low", "", "", "CVE-2023-4321", "summary-2", "component-C:", "", writer), writer),
 						},
 						PullRequestDiff: vcsclient.PullRequestDiff{
 							OriginalFilePath:    "file1",
@@ -184,7 +252,7 @@ func TestGetNewReviewComments(t *testing.T) {
 					Type: IacComment,
 					CommentInfo: vcsclient.PullRequestComment{
 						CommentInfo: vcsclient.CommentInfo{
-							Content: outputwriter.GenerateReviewCommentContent(outputwriter.IacReviewContent("High", "Missing auto upgrade was detected", "", repo.OutputWriter), repo.OutputWriter),
+							Content: outputwriter.GenerateReviewCommentContent(outputwriter.IacReviewContent("High", "Missing auto upgrade was detected", "", writer), writer),
 						},
 						PullRequestDiff: vcsclient.PullRequestDiff{
 							OriginalFilePath:    "file1",
@@ -212,7 +280,7 @@ func TestGetNewReviewComments(t *testing.T) {
 					Type: SastComment,
 					CommentInfo: vcsclient.PullRequestComment{
 						CommentInfo: vcsclient.CommentInfo{
-							Content: outputwriter.GenerateReviewCommentContent(outputwriter.SastReviewContent("High", "XSS Vulnerability", "", [][]formats.Location{}, repo.OutputWriter), repo.OutputWriter),
+							Content: outputwriter.GenerateReviewCommentContent(outputwriter.SastReviewContent("High", "XSS Vulnerability", "", [][]formats.Location{}, writer), writer),
 						},
 						PullRequestDiff: vcsclient.PullRequestDiff{
 							OriginalFilePath:    "file1",
@@ -233,6 +301,7 @@ func TestGetNewReviewComments(t *testing.T) {
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			repo := &Repository{OutputWriter: writer, Params: Params{Git: Git{PullRequestSecretComments: tc.generateSecretsComments}}}
 			output := getNewReviewComments(repo, tc.issues)
 			assert.ElementsMatch(t, tc.expectedOutput, output)
 		})

utils/consts.go

@@ -34,19 +34,21 @@ const (
 	JfrogConfigProfileEnv  = "JF_CONFIG_PROFILE"
 
 	// Git environment variables
-	GitProvider              = "JF_GIT_PROVIDER"
-	GitRepoOwnerEnv          = "JF_GIT_OWNER"
-	GitRepoEnv               = "JF_GIT_REPO"
-	GitProjectEnv            = "JF_GIT_PROJECT"
-	GitUsernameEnv           = "JF_GIT_USERNAME"
-	GitUseLocalRepositoryEnv = "JF_USE_LOCAL_REPOSITORY"
+	GitProvider                      = "JF_GIT_PROVIDER"
+	GitRepoOwnerEnv                  = "JF_GIT_OWNER"
+	GitRepoEnv                       = "JF_GIT_REPO"
+	GitProjectEnv                    = "JF_GIT_PROJECT"
+	GitUsernameEnv                   = "JF_GIT_USERNAME"
+	GitUseLocalRepositoryEnv         = "JF_USE_LOCAL_REPOSITORY"
+	UseMostCommonAncestorAsTargetEnv = "JF_USE_MOST_COMMON_ANCESTOR_AS_TARGET"
 
 	// 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"
-	UseMostCommonAncestorAsTargetEnv = "JF_USE_MOST_COMMON_ANCESTOR_AS_TARGET"
+	BranchNameTemplateEnv       = "JF_BRANCH_NAME_TEMPLATE"
+	CommitMessageTemplateEnv    = "JF_COMMIT_MESSAGE_TEMPLATE"
+	PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE"
+	PullRequestCommentTitleEnv  = "JF_PR_COMMENT_TITLE"
+	//#nosec G101 -- not a secret
+	PullRequestSecretCommentsEnv = "JF_PR_SHOW_SECRETS_COMMENTS"
 
 	// Repository environment variables - Ignored if the frogbot-config.yml file is used
 	InstallCommandEnv                  = "JF_INSTALL_DEPS_CMD"