feature: container-scan command to trivy scan containers #191
Conversation
Codacy's Analysis Summary2 new issues, 2 flagged as potential false positives (≤ 1 medium issue) Review Pull Request in Codacy →
|
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.
Pull request overview
This PR adds a new container-scan command to the Codacy CLI that wraps Trivy for container vulnerability scanning. The command provides configurable scanning options while setting sensible defaults for severity levels and package types.
Changes:
- Added new
container-scancommand with Trivy integration for container image vulnerability scanning - Updated validation and configuration logic to exclude
container-scanfrom requiring codacy.yaml - Provided configurable flags for severity, package types, and unfixed vulnerability handling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/container_scan.go | New command implementation that wraps Trivy with configurable scanning options |
| cmd/validation.go | Added container-scan to commands that skip codacy.yaml validation |
| cli-v2.go | Added container-scan to commands that bypass configuration requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/container_scan.go
Outdated
| logger.Error("Failed to run Trivy", logrus.Fields{ | ||
| "error": err.Error(), | ||
| }) | ||
| color.Red("❌ Error: Failed to run Trivy: %v", err) |
There was a problem hiding this comment.
The error formatting is incorrect. color.Red does not support format specifiers like fmt.Printf. The error message will literally print "%v" instead of formatting the error value. Change this to use fmt.Sprintf to format the error first, or use a separate fmt.Printf call.
| color.Red("❌ Error: Failed to run Trivy: %v", err) | |
| color.Red(fmt.Sprintf("❌ Error: Failed to run Trivy: %v", err)) |
cmd/container_scan.go
Outdated
| if exitError, ok := err.(*exec.ExitError); ok { | ||
| exitCode := exitError.ExitCode() | ||
| logger.Warn("Container scan completed with vulnerabilities", logrus.Fields{ | ||
| "image": imageName, | ||
| "exit_code": exitCode, | ||
| }) | ||
| if exitCode == 1 { | ||
| fmt.Println() | ||
| color.Red("❌ Scanning failed: vulnerabilities found in the container image") | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling logic has a flaw. If exitError.ExitCode() returns a value other than 1 (e.g., 2 or higher), the code falls through without exiting, logging the same error twice and calling os.Exit(1) at line 111. This creates confusing duplicate error logs. The code should exit after handling any exit error, not just exit code 1, or add an else statement to prevent fall-through.
| package cmd | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
|
|
||
| "codacy/cli-v2/utils/logger" | ||
|
|
||
| "github.com/fatih/color" | ||
| "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| // Flag variables for container-scan command | ||
| var ( | ||
| severityFlag string | ||
| pkgTypesFlag string | ||
| ignoreUnfixedFlag bool | ||
| ) | ||
|
|
||
| func init() { | ||
| containerScanCmd.Flags().StringVar(&severityFlag, "severity", "", "Comma-separated list of severities to scan for (default: HIGH,CRITICAL)") | ||
| containerScanCmd.Flags().StringVar(&pkgTypesFlag, "pkg-types", "", "Comma-separated list of package types to scan (default: os)") | ||
| containerScanCmd.Flags().BoolVar(&ignoreUnfixedFlag, "ignore-unfixed", true, "Ignore unfixed vulnerabilities") | ||
| rootCmd.AddCommand(containerScanCmd) | ||
| } | ||
|
|
||
| var containerScanCmd = &cobra.Command{ | ||
| Use: "container-scan [FLAGS] <IMAGE_NAME>", | ||
| Short: "Scan container images for vulnerabilities using Trivy", | ||
| Long: `Scan container images for vulnerabilities using Trivy. | ||
| By default, scans for HIGH and CRITICAL vulnerabilities in OS packages, | ||
| ignoring unfixed issues. Use flags to override these defaults. | ||
| The --exit-code 1 flag is always applied (not user-configurable) to ensure | ||
| the command fails when vulnerabilities are found.`, | ||
| Example: ` # Default behavior (HIGH,CRITICAL severity, os packages only) | ||
| codacy-cli container-scan myapp:latest | ||
| # Scan only for CRITICAL vulnerabilities | ||
| codacy-cli container-scan --severity CRITICAL myapp:latest | ||
| # Scan all severities and package types | ||
| codacy-cli container-scan --severity LOW,MEDIUM,HIGH,CRITICAL --pkg-types os,library myapp:latest | ||
| # Include unfixed vulnerabilities | ||
| codacy-cli container-scan --ignore-unfixed=false myapp:latest`, | ||
| Args: cobra.ExactArgs(1), | ||
| Run: runContainerScan, | ||
| } | ||
|
|
||
| func runContainerScan(cmd *cobra.Command, args []string) { | ||
| imageName := args[0] | ||
|
|
||
| logger.Info("Starting container scan", logrus.Fields{ | ||
| "image": imageName, | ||
| }) | ||
|
|
||
| // Check if Trivy is installed | ||
| trivyPath, err := exec.LookPath("trivy") | ||
| if err != nil { | ||
| 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/") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| logger.Info("Found Trivy", logrus.Fields{ | ||
| "path": trivyPath, | ||
| }) | ||
|
|
||
| // Build Trivy command arguments | ||
| trivyArgs := buildTrivyArgs(imageName) | ||
|
|
||
| trivyCmd := exec.Command(trivyPath, trivyArgs...) | ||
| trivyCmd.Stdout = os.Stdout | ||
| trivyCmd.Stderr = os.Stderr | ||
|
|
||
| logger.Info("Running Trivy container scan", logrus.Fields{ | ||
| "command": trivyCmd.String(), | ||
| }) | ||
|
|
||
| fmt.Printf("🔍 Scanning container image: %s\n\n", imageName) | ||
|
|
||
| err = trivyCmd.Run() | ||
| if err != nil { | ||
| // Check if the error is due to exit code 1 (vulnerabilities found) | ||
| if exitError, ok := err.(*exec.ExitError); ok { | ||
| exitCode := exitError.ExitCode() | ||
| logger.Warn("Container scan completed with vulnerabilities", logrus.Fields{ | ||
| "image": imageName, | ||
| "exit_code": exitCode, | ||
| }) | ||
| if exitCode == 1 { | ||
| fmt.Println() | ||
| color.Red("❌ Scanning failed: vulnerabilities found in the container image") | ||
| os.Exit(1) | ||
| } | ||
| } | ||
|
|
||
| // Other errors | ||
| logger.Error("Failed to run Trivy", logrus.Fields{ | ||
| "error": err.Error(), | ||
| }) | ||
| color.Red("❌ Error: Failed to run Trivy: %v", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| logger.Info("Container scan completed successfully", logrus.Fields{ | ||
| "image": imageName, | ||
| }) | ||
|
|
||
| fmt.Println() | ||
| color.Green("✅ Success: No vulnerabilities found matching the specified criteria") | ||
| } | ||
|
|
||
| // buildTrivyArgs constructs the Trivy command arguments based on flags | ||
| func buildTrivyArgs(imageName string) []string { | ||
| args := []string{ | ||
| "image", | ||
| "--scanners", "vuln", | ||
| } | ||
|
|
||
| // Apply --ignore-unfixed if enabled (default: true) | ||
| if ignoreUnfixedFlag { | ||
| args = append(args, "--ignore-unfixed") | ||
| } | ||
|
|
||
| // Apply --severity (use default if not specified) | ||
| severity := severityFlag | ||
| if severity == "" { | ||
| severity = "HIGH,CRITICAL" | ||
| } | ||
| args = append(args, "--severity", severity) | ||
|
|
||
| // Apply --pkg-types (use default if not specified) | ||
| pkgTypes := pkgTypesFlag | ||
| if pkgTypes == "" { | ||
| pkgTypes = "os" | ||
| } | ||
| args = append(args, "--pkg-types", pkgTypes) | ||
|
|
||
| // Always apply --exit-code 1 (not user-configurable) | ||
| args = append(args, "--exit-code", "1") | ||
|
|
||
| // Add the image name as the last argument | ||
| args = append(args, imageName) | ||
|
|
||
| return args | ||
| } |
There was a problem hiding this comment.
This new command lacks test coverage. Similar commands in this repository have corresponding test files (e.g., analyze_test.go, config_test.go, init_test.go, upload_test.go). Consider adding a container_scan_test.go file to test the buildTrivyArgs function and the error handling logic in runContainerScan.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/container_scan_test.go
Outdated
| // Set the global flag variables | ||
| severityFlag = tt.severity | ||
| pkgTypesFlag = tt.pkgTypes | ||
| ignoreUnfixedFlag = tt.ignoreUnfixed | ||
|
|
||
| // Build the args | ||
| args := buildTrivyArgs(tt.imageName) |
There was a problem hiding this comment.
Multiple tests in this file modify global flag variables (severityFlag, pkgTypesFlag, ignoreUnfixedFlag) without cleanup, which can cause test pollution where one test affects another. This occurs in TestBuildTrivyArgs (lines 126-132), TestBuildTrivyArgsOrder (lines 163-166), and TestBuildTrivyArgsDefaultsApplied (lines 379-382). Consider adding t.Cleanup() in each affected test to reset the flags to their original values after the test completes.
cmd/container_scan_test.go
Outdated
| { | ||
| name: "image name too long", | ||
| imageName: string(make([]byte, 300)), | ||
| expectError: true, | ||
| errorMsg: "too long", |
There was a problem hiding this comment.
The test creates a byte slice with 300 zero bytes and converts it to a string for testing the length limit. This creates a string of 300 null characters (\x00), which are not valid Docker image name characters and will fail the regex validation before reaching the length check. This test may not actually be testing the length validation logic. Consider using a valid character pattern like string(bytes.Repeat([]byte{'a'}, 300)) instead.
cmd/container_scan_test.go
Outdated
| { | ||
| name: "image starting with hyphen", | ||
| imageName: "-nginx", | ||
| expectError: true, | ||
| errorMsg: "invalid image name format", | ||
| }, |
There was a problem hiding this comment.
The test expects an error with message "invalid image name format" for an image starting with a hyphen, but the validateImageName function doesn't have a specific check for this pattern. The regex ^[a-zA-Z0-9][a-zA-Z0-9._\-/:@]*$ requires the first character to be alphanumeric, so "-nginx" would fail the regex check and return "invalid image name format: contains disallowed characters" instead of "invalid image name format". The error message assertion may not match the actual error.
| } | ||
|
|
||
| logger.Error("Failed to run Trivy", logrus.Fields{"error": err.Error()}) | ||
| color.Red("❌ Error: Failed to run Trivy: %v", err) |
There was a problem hiding this comment.
The color.Red function is being called with a format string and argument, but color.Red doesn't support format strings like Printf. This will print the literal format string with "%v" in it, not the actual error message. Use fmt.Fprintf with color.Output or color.New(color.FgRed).Printf instead.
cmd/container_scan.go
Outdated
|
|
||
| if err := validateImageName(imageName); err != nil { | ||
| logger.Error("Invalid image name", logrus.Fields{"image": imageName, "error": err.Error()}) | ||
| color.Red("❌ Error: %v", err) |
There was a problem hiding this comment.
The color.Red function is being called with a format string and argument, but color.Red doesn't support format strings like Printf. This will print the literal format string with "%v" in it, not the actual error message. Use fmt.Fprintf with color.Output or color.New(color.FgRed).Printf instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if context != "" { | ||
| path = filepath.Join(context, path) | ||
| } | ||
| return path |
There was a problem hiding this comment.
The resolveDockerfilePath function uses filepath.Join without validating that the resulting path doesn't escape the intended directory. A malicious docker-compose.yml could specify a build context like "../../../etc" and dockerfile like "passwd" to potentially read sensitive files. Consider adding validation to ensure the resolved path stays within safe boundaries or at least doesn't use absolute paths for user-controlled input.
| return path | |
| cleaned := filepath.Clean(path) | |
| // Disallow absolute paths or paths that escape upwards from the working directory | |
| if filepath.IsAbs(cleaned) || cleaned == ".." || strings.HasPrefix(cleaned, ".."+string(os.PathSeparator)) { | |
| logger.Warn("Unsafe Dockerfile path detected, falling back to default Dockerfile", logrus.Fields{ | |
| "context": context, | |
| "dockerfile": dockerfile, | |
| }) | |
| return "Dockerfile" | |
| } | |
| return cleaned |
| // handleTrivyResult processes the Trivy command result and exits appropriately | ||
| func handleTrivyResult(err error, imageName string) { | ||
| if err == nil { | ||
| logger.Info("Container scan completed successfully", logrus.Fields{"image": imageName}) | ||
| fmt.Println() | ||
| color.Green("✅ Success: No vulnerabilities found matching the specified criteria") | ||
| return | ||
| } | ||
|
|
||
| if exitError, ok := err.(*exec.ExitError); ok && exitError.ExitCode() == 1 { | ||
| logger.Warn("Container scan completed with vulnerabilities", logrus.Fields{ | ||
| "image": imageName, "exit_code": 1, | ||
| }) | ||
| fmt.Println() | ||
| color.Red("❌ Scanning failed: vulnerabilities found in the container image") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| logger.Error("Failed to run Trivy", logrus.Fields{"error": err.Error()}) | ||
| color.Red("❌ Error: Failed to run Trivy: %v", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
There was a problem hiding this comment.
The handleTrivyResult function is defined but never used in the codebase. The actual error handling is performed by handleScanError function in the scanImages function. This dead code should be removed to improve maintainability.
| // handleTrivyResult processes the Trivy command result and exits appropriately | |
| func handleTrivyResult(err error, imageName string) { | |
| if err == nil { | |
| logger.Info("Container scan completed successfully", logrus.Fields{"image": imageName}) | |
| fmt.Println() | |
| color.Green("✅ Success: No vulnerabilities found matching the specified criteria") | |
| return | |
| } | |
| if exitError, ok := err.(*exec.ExitError); ok && exitError.ExitCode() == 1 { | |
| logger.Warn("Container scan completed with vulnerabilities", logrus.Fields{ | |
| "image": imageName, "exit_code": 1, | |
| }) | |
| fmt.Println() | |
| color.Red("❌ Scanning failed: vulnerabilities found in the container image") | |
| os.Exit(1) | |
| } | |
| logger.Error("Failed to run Trivy", logrus.Fields{"error": err.Error()}) | |
| color.Red("❌ Error: Failed to run Trivy: %v", err) | |
| os.Exit(1) | |
| } |
| {"command injection with redirect", "nginx > /tmp/output", "disallowed character"}, | ||
| {"command injection with backslash", "nginx\\malicious", "disallowed character"}, | ||
| {"empty image name", "", "cannot be empty"}, | ||
| {"image name too long", string(make([]byte, 300)), "too long"}, |
There was a problem hiding this comment.
The test case "image name too long" creates a byte slice of 300 bytes but doesn't initialize it with valid characters. This creates a string of 300 null bytes (\x00), which would fail validation at the regex check (line 96) before reaching the length check (line 91), making the test potentially verify the wrong validation logic.
| severity := severityFlag | ||
| if severity == "" { | ||
| severity = "HIGH,CRITICAL" | ||
| } | ||
| args = append(args, "--severity", severity) | ||
|
|
||
| // Apply --pkg-types (use default if not specified) | ||
| pkgTypes := pkgTypesFlag | ||
| if pkgTypes == "" { | ||
| pkgTypes = "os" | ||
| } | ||
| args = append(args, "--pkg-types", pkgTypes) |
There was a problem hiding this comment.
The severityFlag and pkgTypesFlag values are passed directly to Trivy without validation. While exec.Command properly separates arguments and prevents shell injection, malicious or malformed values could still cause unexpected Trivy behavior. Consider adding validation to ensure these flags contain only expected characters (e.g., alphanumeric, commas for severity levels).
| - [email protected] | ||
| - [email protected] | ||
| - [email protected] | ||
| - pmd@6.55.0 | ||
| - [email protected].9 | ||
| - revive@1.12.0 | ||
| - pmd@7.11.0 | ||
| - [email protected].6 | ||
| - revive@1.7.0 |
There was a problem hiding this comment.
Multiple tool version changes (eslint: 9.38.0→8.57.0, pmd: 6.55.0→7.11.0, pylint: 3.3.9→3.3.6, revive: 1.12.0→1.7.0) are included in this PR. The eslint and pylint changes appear to be downgrades, while pmd is an upgrade. These version changes seem unrelated to the container-scan feature and would benefit from being in a separate PR or being explained in the PR description.
| return detectFromDockerfile(dockerfileFlag, true) | ||
| } | ||
|
|
||
| // Priority 0: Check explicit --compose-file flag |
There was a problem hiding this comment.
Both the dockerfile and compose-file flag checks are labeled as "Priority 0" in the comments. Since these are mutually exclusive explicit flags and both have the same priority level, the second one should be labeled differently (e.g., "Priority 0b" or the logic should be adjusted to handle the case where both flags are provided).
| // Priority 0: Check explicit --compose-file flag | |
| // Priority 0b: Check explicit --compose-file flag |
| // Skip build stage aliases (e.g., FROM golang:1.21 AS builder) | ||
| // and scratch images |
There was a problem hiding this comment.
The comment "Skip build stage aliases (e.g., FROM golang:1.21 AS builder)" is somewhat misleading. The regex pattern automatically captures only the image name (the first non-whitespace sequence after FROM) and stops at the AS keyword, so aliases aren't being "skipped" through explicit logic - they're naturally excluded by the regex pattern. Consider clarifying the comment to say "Extract base image (before AS clause if present)".
| // Skip build stage aliases (e.g., FROM golang:1.21 AS builder) | |
| // and scratch images | |
| // Extract base image (before AS clause if present) and | |
| // ignore scratch images and duplicates |
| func TestBuildTrivyArgs(t *testing.T) { | ||
| for _, tc := range trivyArgsCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| severityFlag = tc.severity | ||
| pkgTypesFlag = tc.pkgTypes | ||
| ignoreUnfixedFlag = tc.ignoreUnfixed | ||
|
|
||
| args := buildTrivyArgs(tc.imageName) | ||
|
|
||
| if tc.expectedArgs != nil { | ||
| assert.Equal(t, tc.expectedArgs, args, "Args should match exactly") | ||
| } | ||
| for _, exp := range tc.expectedContains { | ||
| assert.Contains(t, args, exp, "Args should contain %s", exp) | ||
| } | ||
| for _, notExp := range tc.notContains { | ||
| assert.NotContains(t, args, notExp, "Args should not contain %s", notExp) | ||
| } | ||
| assert.Equal(t, tc.imageName, args[len(args)-1], "Image name should be last") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test modifies package-level variables (severityFlag, pkgTypesFlag, ignoreUnfixedFlag) which are shared across all tests. If tests run in parallel or don't properly reset state, this could cause test failures or flaky behavior. Consider either using test fixtures that properly isolate state, or ensuring these tests cannot run in parallel with t.Parallel() omitted.
There was a problem hiding this comment.
The change from '[email protected]' to '[email protected]' for the runtime appears unrelated to the container-scan feature being added. While this may be a valid correction (since dartanalyzer is a Dart tool, not specifically a Flutter tool), it would be clearer if configuration changes unrelated to the main feature were in a separate commit or PR, or at least mentioned in the PR description.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
| logger.Info("Starting container scan", logrus.Fields{"image": imageName}) | ||
| fmt.Printf("🔍 Scanning container image: %s\n\n", imageName) | ||
|
|
||
| trivyCmd := exec.Command(trivyPath, buildTrivyArgs(imageName)...) |
There was a problem hiding this comment.
Codacy found an issue: Detected non-static command inside Command.
| logger.Info("Starting container scan", logrus.Fields{"image": imageName}) | ||
| fmt.Printf("🔍 Scanning container image: %s\n\n", imageName) | ||
|
|
||
| trivyCmd := exec.Command(trivyPath, buildTrivyArgs(imageName)...) |
There was a problem hiding this comment.
No description provided.