-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Run trivy container scan #192
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
base: main
Are you sure you want to change the base?
Conversation
Codacy's Analysis Summary3 new issues, 2 flagged as potential false positives (≤ 1 medium issue) Review Pull Request in Codacy →
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new container-scan command to the Codacy CLI that enables vulnerability scanning of container images using Trivy. The command supports scanning single or multiple container images for vulnerabilities with configurable severity levels and package types.
Changes:
- Added new
container-scancommand with support for multiple image scanning, customizable severity levels, package types, and unfixed vulnerability filtering - Updated validation logic to skip
codacy.yamlrequirement for container-scan command - Integrated Trivy as the vulnerability scanning engine with proper input validation to prevent command injection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cmd/container_scan.go | Implements the new container-scan command with Trivy integration, input validation, and multi-image scanning support |
| cmd/container_scan_test.go | Provides unit tests for command arguments, flag defaults, image name validation, and Trivy argument construction |
| cmd/validation.go | Adds container-scan to the list of commands that skip codacy.yaml validation |
| cli-v2.go | Updates early-exit logic to allow container-scan to run without configuration file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
851f9a2 to
9539333
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5b9ff97 to
6e748c6
Compare
6e748c6 to
078a26a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
00a46e9 to
d4d0d2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CommandRunner interface for running external commands (allows mocking in tests) | ||
| type CommandRunner interface { | ||
| Run(name string, args []string) error | ||
| } | ||
|
|
||
| // ExecCommandRunner runs commands using exec.Command | ||
| type ExecCommandRunner struct{} | ||
|
|
||
| // Run executes a command and returns its exit error | ||
| func (r *ExecCommandRunner) Run(name string, args []string) error { | ||
| // #nosec G204 -- name comes from exec.LookPath("trivy") with a literal string, | ||
| // and args are validated by validateImageName() which checks for shell metacharacters. | ||
| // exec.Command passes arguments directly without shell interpretation. | ||
| cmd := exec.Command(name, args...) | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| // commandRunner is the default command runner, can be replaced in tests | ||
| var commandRunner CommandRunner = &ExecCommandRunner{} |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CommandRunner interface and ExecCommandRunner implementation are unnecessary abstractions. The existing codebase doesn't use this pattern - tools like eslintRunner.go (line 18), trivyRunner.go (line 12), and other runners directly use exec.Command without abstraction layers.
This adds complexity without clear benefit and deviates from established patterns in the codebase. Consider using exec.Command directly like other runners, which would simplify the code and make it more maintainable.
| exitCode := executeContainerScan(args) | ||
| exitFunc(exitCode) | ||
| } | ||
|
|
||
| // executeContainerScan performs the container scan and returns an exit code | ||
| // Exit codes: 0 = success, 1 = vulnerabilities found, 2 = error | ||
| func executeContainerScan(imageNames []string) int { | ||
| if code := validateAllImages(imageNames); code != 0 { | ||
| return code | ||
| } | ||
| logger.Info("Starting container scan", logrus.Fields{"images": imageNames, "count": len(imageNames)}) | ||
|
|
||
| trivyPath, err := getTrivyPath() | ||
| if err != nil { | ||
| handleTrivyNotFound(err) | ||
| return 2 | ||
| } | ||
|
|
||
| hasVulnerabilities := scanAllImages(imageNames, trivyPath) | ||
| if hasVulnerabilities == -1 { | ||
| return 2 | ||
| } | ||
| return printScanSummary(hasVulnerabilities == 1, imageNames) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runContainerScan function serves no purpose and adds unnecessary indirection. It just calls executeContainerScan and then exits. This pattern is inconsistent with how other commands in the codebase are structured.
Looking at commands like update.go and config.go, they perform their logic directly in the Run function and call os.Exit when needed. Consider moving the logic from executeContainerScan into runContainerScan and removing the extra layer.
| exitCode := executeContainerScan(args) | |
| exitFunc(exitCode) | |
| } | |
| // executeContainerScan performs the container scan and returns an exit code | |
| // Exit codes: 0 = success, 1 = vulnerabilities found, 2 = error | |
| func executeContainerScan(imageNames []string) int { | |
| if code := validateAllImages(imageNames); code != 0 { | |
| return code | |
| } | |
| logger.Info("Starting container scan", logrus.Fields{"images": imageNames, "count": len(imageNames)}) | |
| trivyPath, err := getTrivyPath() | |
| if err != nil { | |
| handleTrivyNotFound(err) | |
| return 2 | |
| } | |
| hasVulnerabilities := scanAllImages(imageNames, trivyPath) | |
| if hasVulnerabilities == -1 { | |
| return 2 | |
| } | |
| return printScanSummary(hasVulnerabilities == 1, imageNames) | |
| // Exit codes: 0 = success, 1 = vulnerabilities found, 2 = error | |
| if code := validateAllImages(args); code != 0 { | |
| exitFunc(code) | |
| return | |
| } | |
| logger.Info("Starting container scan", logrus.Fields{"images": args, "count": len(args)}) | |
| trivyPath, err := getTrivyPath() | |
| if err != nil { | |
| handleTrivyNotFound(err) | |
| return | |
| } | |
| hasVulnerabilities := scanAllImages(args, trivyPath) | |
| if hasVulnerabilities == -1 { | |
| exitFunc(2) | |
| return | |
| } | |
| exitCode := printScanSummary(hasVulnerabilities == 1, args) | |
| exitFunc(exitCode) |
| // Based on Docker image reference specification | ||
| var validImageNamePattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._\-/:@]*$`) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern allows characters that may not be valid in Docker image references. Specifically, the underscore character is not typically allowed in Docker image repository names (though it's allowed in tags). The pattern ^[a-zA-Z0-9][a-zA-Z0-9._\-/:@]*$ would allow "my_invalid_repo:tag" which Docker would reject.
According to the Docker image specification, repository names should follow stricter rules. While the subsequent dangerous character checks provide some protection, having an inaccurate regex could cause confusing error messages where validation passes but Docker rejects the image name.
| // Based on Docker image reference specification | |
| var validImageNamePattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._\-/:@]*$`) | |
| // Based on Docker image reference specification (repository names are strict, tags more permissive) | |
| var validImageNamePattern = regexp.MustCompile(`^(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?(?:\.(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?))*)(?::[0-9]+)?/)?[a-z0-9]+(?:(?:[._]|__|[-]*)[a-z0-9]+)*(?:/[a-z0-9]+(?:(?:[._]|__|[-]*)[a-z0-9]+)*)*(?::[\w][\w.-]{0,127})?(?:@[A-Za-z][A-Za-z0-9]*:[0-9a-fA-F]{32,})?$`) |
| {name: "image name too long", imageName: string(make([]byte, 300)), expectError: true, errorMsg: "too long"}, | ||
| {name: "image starting with hyphen", imageName: "-nginx", expectError: true, errorMsg: "invalid image name format"}, |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test at line 514 creates a 300-byte array but doesn't populate it with valid characters. Since make([]byte, 300) creates a byte slice filled with zeros (null bytes), the validateImageName function would likely fail the empty string check or the dangerousChars check before reaching the length check.
This means the test might be passing for the wrong reason. Consider using a string like strings.Repeat("a", 300) to ensure you're actually testing the length validation logic.
| {name: "command injection with backslash", imageName: "nginx\\malicious", expectError: true, errorMsg: "disallowed character"}, | ||
| {name: "empty image name", imageName: "", expectError: true, errorMsg: "cannot be empty"}, | ||
| {name: "image name too long", imageName: string(make([]byte, 300)), expectError: true, errorMsg: "too long"}, | ||
| {name: "image starting with hyphen", imageName: "-nginx", expectError: true, errorMsg: "invalid image name format"}, |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation at line 514 checks that image names starting with a hyphen are invalid, but the regex pattern at line 21 already enforces this by requiring the first character to be alphanumeric: ^[a-zA-Z0-9].... This test case is redundant because the regex check would catch it.
Consider removing this test case or updating it to test a different edge case that's not already covered by the regex pattern.
| {name: "image starting with hyphen", imageName: "-nginx", expectError: true, errorMsg: "invalid image name format"}, |
| trivyPath, err := getTrivyPath() | ||
| if err != nil { | ||
| handleTrivyNotFound(err) | ||
| return 2 |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function signature of handleTrivyNotFound doesn't return a value, but in executeContainerScan at line 169, it's called and then explicitly returns 2. This is confusing because handleTrivyNotFound already calls exitFunc(2) which terminates the program in production.
The return 2 statement at line 170 only executes in tests where exitFunc is mocked. This creates a code smell where the control flow depends on whether exitFunc is mocked or not. Consider removing the exitFunc call from handleTrivyNotFound and having it return an int instead, letting the caller decide whether to exit.
| if !validImageNamePattern.MatchString(imageName) { | ||
| return fmt.Errorf("invalid image name format: contains disallowed characters") | ||
| } |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic at line 127 is unreachable. After checking for dangerous characters in the loop (lines 119-124), if the imageName contains any of them, the function returns an error. Therefore, the validImageNamePattern check on line 127 will never execute for strings containing those characters.
This means the regex check only catches characters that aren't in the dangerousChars list. Consider either removing the redundant check or restructuring to validate with the regex first, then provide more specific error messages for dangerous characters.
| // handleTrivyNotFound prints error message and exits with code 2 | ||
| func handleTrivyNotFound(err error) { | ||
| logger.Error("Trivy not found", logrus.Fields{"error": err.Error()}) | ||
| color.Red("❌ Error: Trivy is not installed or not found in PATH") | ||
| fmt.Println("Please install Trivy to use container scanning.") | ||
| fmt.Println("Visit: https://trivy.dev/latest/getting-started/installation/") | ||
| fmt.Println("exit-code 2") | ||
| exitFunc(2) |
Copilot
AI
Jan 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleTrivyNotFound function calls exitFunc(2) but then returns 2, which creates ambiguous control flow. After exitFunc(2) is called (which in production is os.Exit(2)), the function terminates the program, so the return statement is never reached. However, in tests where exitFunc is mocked, the return value is used.
This creates inconsistent behavior between production and tests. Consider either:
- Removing the return value since exitFunc terminates the program
- Or not calling exitFunc here and letting the caller handle the exit
Looking at the caller (executeContainerScan line 169-170), it calls handleTrivyNotFound and then returns 2 anyway, making the exit call redundant.
| // handleTrivyNotFound prints error message and exits with code 2 | |
| func handleTrivyNotFound(err error) { | |
| logger.Error("Trivy not found", logrus.Fields{"error": err.Error()}) | |
| color.Red("❌ Error: Trivy is not installed or not found in PATH") | |
| fmt.Println("Please install Trivy to use container scanning.") | |
| fmt.Println("Visit: https://trivy.dev/latest/getting-started/installation/") | |
| fmt.Println("exit-code 2") | |
| exitFunc(2) | |
| // handleTrivyNotFound prints error message indicating Trivy is missing | |
| func handleTrivyNotFound(err error) { | |
| logger.Error("Trivy not found", logrus.Fields{"error": err.Error()}) | |
| color.Red("❌ Error: Trivy is not installed or not found in PATH") | |
| fmt.Println("Please install Trivy to use container scanning.") | |
| fmt.Println("Visit: https://trivy.dev/latest/getting-started/installation/") | |
| fmt.Println("exit-code 2") |
multi images scanning success:
with vulnerabilities case :