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

Fix build scan command table printing with --vuln=true #157

Closed
wants to merge 2 commits into from

Conversation

dortam888
Copy link
Contributor

  • The pull request is targeting the dev branch.
  • The code has been validated to compile successfully by running go vet ./....
  • The code has been formatted properly using go fmt ./....
  • All static analysis checks passed.
  • All tests have passed. If this feature is not already covered by the tests, new tests have been added.
  • All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

Fixing issues when using --vuln=true flag and --format=table (default) in build scan.
Issue 1: build scan printed vulnerability table twice instead of printing the violation table and the vulnerabilities table.
Issue 2: The vulnerability table is messed up, not sorted, and shows the same vulnerability multiple times.

…ties table twice when --vuln is on instead of printing violations table and vulnerabilities table
@dortam888 dortam888 added the bug Something isn't working label Aug 25, 2024
@@ -155,7 +155,6 @@ func (bsc *BuildScanCommand) runBuildScanAndPrintResults(xrayManager *xray.XrayS

resultsPrinter := utils.NewResultsWriter(scanResults).
SetOutputFormat(bsc.outputFormat).
SetIncludeVulnerabilities(bsc.includeVulnerabilities).
SetIncludeLicenses(false).
Copy link
Contributor

Choose a reason for hiding this comment

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

after removing this line - did you test the CLI behaviour with and without watches?

Copy link
Contributor Author

@dortam888 dortam888 Aug 25, 2024

Choose a reason for hiding this comment

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

@hadarshjfrog I did the regular tests there is no watch test in jf build-scan (must have a watch with fail build policy).
This line doesn't affect watches as it relates to vulnerabilities. (and watches are affecting violations).
We call SetIncludeVulnerabilities in line 179 if vulnerabilities should be added.
https://github.com/jfrog/jfrog-cli-security/blob/main/commands/scan/buildscan.go#L180

@attiasas attiasas added the safe to test Approve running integration tests on a pull request label Aug 26, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Aug 26, 2024
Copy link

👍 Frogbot scanned this pull request and did not find any new security issues.


Copy link
Contributor

@attiasas attiasas left a comment

Choose a reason for hiding this comment

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

This change is not how to fix the issue.
There are 2 issues here:

  1. we can see that in the parsing of the flags, we don't take into account the project flag only in build-scan. the fix should add it, instead of removing and always printing violations...
    image
    image

  2. In the table printing code, we do not expect to print both violations and vulnerabilities tables so we need to choose one (preferable vuln if requested)
    image

@dortam888
Copy link
Contributor Author

@attiasas @hadarshjfrog We have a couple of issues with jf bs.
Some parameters are missing, like projects and JAS enablement so we don't get results for builds from projects and we don't get JAS results at all (we don't have secrets but we can have CA).
Regarding this fix, See the comment in the code:
https://github.com/jfrog/jfrog-cli-security/blob/main/commands/scan/buildscan.go#L171
The behavior when the format is json for example is appending vulnerabilities to violations and that is what the code meant to do for tables as well based on the comment.
Jira is about having violations and no vulnerabilities try to get vulnerabilites.
The build is failing because we have violations but we're not showing it in tables printing.

@attiasas
Copy link
Contributor

attiasas commented Sep 4, 2024

@attiasas attiasas closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants