Add JAS violations #788
Add JAS violations #788
Conversation
attiasas
added
the
safe to test
github-actions
bot
removed
the
safe to test
attiasas
added
the
safe to test
github-actions
bot
removed
the
safe to test
attiasas
added
the
safe to test
github-actions
bot
removed
the
safe to test
| if shouldSendExposedSecretsEmail { | ||
| secretsEmailDetails := utils.NewSecretsEmailDetails(client, repo, issues.Secrets) | ||
| secretsEmailDetails := utils.NewSecretsEmailDetails(client, repo, append(issues.SecretsVulnerabilities, issues.SecretsViolations...)) |
There was a problem hiding this comment.
would we need the secrets violations and vulnerabilities seperated at later stage?
Just for the sake we will we might want to combine them in a different var and not in the original resutls struct
| SetFailOnInstallationErrors(*repository.FailOnSecurityIssues). | ||
| SetFixableOnly(repository.FixableOnly). | ||
| SetConfigProfile(repository.ConfigProfile). | ||
| SetSkipAutoInstall(repository.SkipAutoInstall). | ||
| SetAllowPartialResults(repository.AllowPartialResults). | ||
| SetDisableJas(repository.DisableJas) | ||
|
|
||
| cfp.scanDetails.XrayVersion = cfp.XrayVersion | ||
| cfp.scanDetails.XscVersion = cfp.XscVersion | ||
| repositoryInfo, err := client.GetRepositoryInfo(context.Background(), cfp.scanDetails.RepoOwner, cfp.scanDetails.RepoName) |
There was a problem hiding this comment.
remove this call since you have the CloneInfo.HTTP fetched few lines later (either from repository.Git.RepositoryCloneUrl or from a new call I made that we must have at this point)
| if err != nil { | ||
| return | ||
| } | ||
| cfp.scanDetails.SetResultsContext(repositoryInfo.CloneInfo.HTTP, repository.Watches, repository.JFrogProjectKey, repository.IncludeVulnerabilities, len(repository.AllowedLicenses) > 0) |
There was a problem hiding this comment.
call this below the if repository.Git.RepositoryCloneUrl != "" lines
| } | ||
|
|
||
| // Downloads Pull Requests branches code and audits them | ||
| func auditPullRequest(repoConfig *utils.Repository, client vcsclient.VcsClient) (issuesCollection *utils.IssuesCollection, err error) { | ||
| func auditPullRequest(repoConfig *utils.Repository, client vcsclient.VcsClient) (issuesCollection *issues.ScansIssuesCollection, err error) { | ||
| repositoryInfo, err := client.GetRepositoryInfo(context.Background(), repoConfig.RepoOwner, repoConfig.RepoName) |
There was a problem hiding this comment.
extract this logic to a function, while adding a check if the cloneUrl already exists in Repository.Params.Git.RepositoryCloneUrl.
Notice that GetRepositoryInfo might be called few times earlier in the flow (if we fetched config profile by url, if we are using GitHub and called verifyGitHubFrogbotEnvironment).
Please implement the following in all those places: whenever this function is called - please insert CloneInfo.HTTP to Repository.Params.Git.RepositoryCloneUrl. whenever we need to use this value - check there first and only if doesnt exists call GetRepositoryInfo (it can be in a case where we are not using github and bot using config profile with url).
Notice: in verifyGitHubFrogbotEnvironment we call GetRepositoryInfo for other perpouse but we can take advantage of this so set the cloneUrl.
Notice: the latest call to GetRepositoryInfo is in tryCheckoutToMostCommonAncestor. with your new call in earlier stage we will always have the cloneUrl at this point in Repository.Params.Git.RepositoryCloneUrl so we can delete the 'if' there and just take the value
| return len(ic.SastVulnerabilities) > 0 || len(ic.SastViolations) > 0 | ||
| } | ||
|
|
||
| func (ic *ScansIssuesCollection) GetTotalIssues(includeSecrets bool) int { |
There was a problem hiding this comment.
rename to GetTotalIssuesCount
| @@ -49,6 +50,16 @@ const ( | |||
| JfrogHomeDirEnv = "JFROG_CLI_HOME_DIR" | |||
| ) | |||
|
|
|||
| // ViolationContext is a type for violation context (None,Project,GitRepo) | |||
There was a problem hiding this comment.
or delete if not needed
| @@ -3,28 +3,31 @@ package utils | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "os" | |||
| // "os" | |||
| "path/filepath" | ||
| "time" | ||
|
|
||
| clientservices "github.com/jfrog/jfrog-client-go/xsc/services" | ||
|
|
||
| "github.com/jfrog/froggit-go/vcsclient" | ||
| "github.com/jfrog/jfrog-cli-core/v2/utils/config" | ||
| "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" | ||
| // "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" |
There was a problem hiding this comment.
Don't forget to uncomment / delete the line
| "github.com/jfrog/jfrog-cli-security/commands/audit" | ||
| "github.com/jfrog/jfrog-cli-security/utils" | ||
| "github.com/jfrog/jfrog-cli-security/utils/results" | ||
| "github.com/jfrog/jfrog-cli-security/utils/severityutils" | ||
| "github.com/jfrog/jfrog-cli-security/utils/xray/scangraph" | ||
| // "github.com/jfrog/jfrog-cli-security/utils/xray/scangraph" |
There was a problem hiding this comment.
Don't forget to uncomment / delete the line
| ) | ||
|
|
||
| type ScanDetails struct { | ||
| *Project | ||
| *Git | ||
| *services.XrayGraphScanParams | ||
|
|
||
| // *services.XrayGraphScanParams |
There was a problem hiding this comment.
Don't forget to uncomment / delete the line
| @@ -31,7 +32,23 @@ const ( | |||
| commentRemovalErrorMsg = "An error occurred while attempting to remove older Frogbot pull request comments:" | |||
| ) | |||
|
|
|||
| func HandlePullRequestCommentsAfterScan(issues *IssuesCollection, repo *Repository, client vcsclient.VcsClient, pullRequestID int) (err error) { | |||
| // // In Scan PR, if there is an error, a comment will be added to the PR with the error message. | |||
There was a problem hiding this comment.
Don't forget to uncomment / delete
|
|
||
|
|
||
| --- | ||
| ### Full description |
There was a problem hiding this comment.
are the 2 empty lines below 'full description' intended?
|
|
||
|
|
||
| --- | ||
| ### 🔖 Details |
There was a problem hiding this comment.
Is the empty section under 'Details' intended? or we just have 2 empty lines there?
This also happens in the other md files
| | :---------------------: | :-----------------------------------: | :-----------------------------------: | :-----------------------------------: | :-----------------------------------: | | ||
| | High | License1 | Comp1:1.0 | Dep1:2.0 | watch | | ||
| | High | License2 | root:1.0.0 | Dep2:3.0 | watch2 | | ||
| | | | minimatch:1.2.3 | | | |
There was a problem hiding this comment.
should this partially full line exist?
| return | ||
| } | ||
| // Result context should be the same for all collections | ||
| ic.ResultContext = issues.ResultContext |
There was a problem hiding this comment.
shouldn't we implement an 'append' here like in the rest of the fields instead of just replacing the entire ResultsContext?
In the single usage we have for this func, if we iterate over several projects we will get the resultsContext of the last of them only (see auditPullRequest func)
|
|
||
| type ApplicableEvidences struct { | ||
| Evidence formats.Evidence | ||
| Severity, ScannerDescription, IssueId, CveSummary, ImpactedDependency, Remediation string |
There was a problem hiding this comment.
I'd rather putting each in a different line
| @@ -10,6 +10,7 @@ type IconName string | |||
|
|
|||
| const ( | |||
| baseResourceUrl = "https://raw.githubusercontent.com/jfrog/frogbot/master/resources/" | |||
| // baseResourceUrl = "https://raw.githubusercontent.com/attiasas/frogbot/jas_violations/resources/" | |||
| // Calculate Hidden columns | ||
| for c := range t.columns { | ||
| // Reset shouldHideColumn flag | ||
| t.columns[c].shouldHideColumn = t.columns[c].OmitEmpty |
There was a problem hiding this comment.
Please explain why you are doing this and how the OmitE,pty value is related to the shouldHideColumn
| for c, column := range t.columns { | ||
| if c == 0 { | ||
| tableBuilder.WriteString(fmt.Sprintf(cellFirstCellPlaceholder, column.Name)) | ||
| actualColumnCount := 0 |
There was a problem hiding this comment.
not critical, but you can simple have a bool var 'isFirstCol = true' and check it in the condition. when you enter the first column action just change it to false. I cannot see a reason to keep counting after the first col
| func VulnerabilitiesContent(vulnerabilities []formats.VulnerabilityOrViolationRow, writer OutputWriter) (content []string) { | ||
| if len(vulnerabilities) == 0 { | ||
| return []string{} | ||
| // func GetFrogbotErrorCommentContent(contentForComments []string, err error, writer OutputWriter) (comments []string) { |
| // } | ||
|
|
||
| // Adding markdown suffix to show the error details and next steps | ||
| // func getFrogbotErrorSuffixDecorator(writer OutputWriter, err error) CommentDecorator { |
| issuesDetailsSubTitle = "🔖 Details" | ||
| jfrogResearchDetailsSubTitle = "🔬 JFrog Research Details" | ||
|
|
||
| jobErrorTitle = "⚠️ Error Details" |
There was a problem hiding this comment.
not in use. only usage in the commented test
|
|
||
| jobErrorTitle = "⚠️ Error Details" | ||
|
|
||
| // Violation Contexts text |
There was a problem hiding this comment.
all 4 values not in use
| if len(issues) == 1 { | ||
| content = append(content, getScaSecurityIssueDetails(issue, violations, writer)) | ||
| } else { | ||
| content = append(content, "\n"+writer.MarkAsDetails( |
There was a problem hiding this comment.
please extract the inner calls to variables and only then place them in the func. It is extreamly hard to read what is going on here. Id rather it to be longer but so we can understand it next time we come back to it
| vulnerabilitiesWithDetails[i].dependencyName, | ||
| vulnerabilitiesWithDetails[i].dependencyVersion), | ||
| 4, vulnerabilitiesWithDetails[i].details, | ||
| content = append(content, "\n"+writer.MarkAsDetails( |
There was a problem hiding this comment.
please extract the inner calls to variables and only then place them in the func. It is extreamly hard to read what is going on here. Id rather it to be longer but so we can understand it next time we come back to it
| if v, ok := codeFlows[info.RuleId]; ok { | ||
| scannerCodeFlows = v | ||
| } | ||
| if len(rulesInfo) == 1 { |
There was a problem hiding this comment.
why do we create a one table if len(rulesInfo)==1 and a different one when not?
| }, | ||
| }, | ||
| { | ||
| name: "Multiple issues - no similar issues", |
There was a problem hiding this comment.
how do you define "similar issues"? similar in what?
in the func description (and the way it works) it seems like you define similar issues as 'same location, same id but different rule'.
from this test case you have 2 issues with everything similar BUT the ruldId and you considered it as "no similar issues".
Please define in the groupSimilarJasIssues description what is exactly "similar issues" since the test cases names are not making sense to me
| PullRequestTitleTemplate string `yaml:"pullRequestTitleTemplate,omitempty"` | ||
| PullRequestCommentTitle string `yaml:"pullRequestCommentTitle,omitempty"` | ||
| PullRequestSecretComments bool `yaml:"pullRequestSecretComments,omitempty"` | ||
| PullRequestDisableErrorComment bool `yaml:"pullRequestDisableErrorComment,omitempty"` |
There was a problem hiding this comment.
what is this? I cannot see any reference or usage to this new field
| commonParams.ProjectKey = os.Getenv(coreutils.Project) | ||
| } else { | ||
| commonParams.ProjectKey = sc.ProjectKey | ||
| func createXrayScanParams(httpCloneUrl string, watches []string, project string, includeVulnerabilities, includeLicenses bool) (params *services.XrayGraphScanParams) { |
There was a problem hiding this comment.
I cannot find any usage for this function.
You also deleted its test so I assume this is not required anymore and can be deleted
Depends on:
Adding JAS Violation Support
JF_INCLUDE_VULNERABILITIESto include vulnerabilities ifJF_PROJECT/JF_WATCHESare defined or if there areGit Repositorywatches defined at the platform.Adding JAS violation results and improving Issue Details
Improve the Issue display and add a bit more information in the comment
Adding Scan Summary
Will also now show the error if it exists