diff --git a/internal/commands/agenthooks/cx/hooks.go b/internal/commands/agenthooks/cx/hooks.go index c333685a..12b043c2 100644 --- a/internal/commands/agenthooks/cx/hooks.go +++ b/internal/commands/agenthooks/cx/hooks.go @@ -116,11 +116,15 @@ func cxBeforeFileEdit(ev agenthooks.FileEditEvent) agenthooks.FileEditVerdict { logRemediationTelemetry(agent, "Asca", severity, ev.SessionID) return agenthooks.RejectEditWithContext(reason, context) } + var kicsNote string if kicsScanner != nil { - if blocked, reason, context := kics.ScanFileEdit(ev, kicsScanner); blocked { + blocked, reason, context, note, severity := kics.ScanFileEdit(&ev, kicsScanner, telemetryWrapper, agent) + if blocked { sessiontally.Add(ev.SessionID, engineKics, 1, 1) + logRemediationTelemetry(agent, "IaC", severity, ev.SessionID) return agenthooks.RejectEditWithContext(reason, context) } + kicsNote = note } if scaScanner != nil { for _, diff := range ev.Changes { @@ -131,6 +135,11 @@ func cxBeforeFileEdit(ev agenthooks.FileEditEvent) agenthooks.FileEditVerdict { } } } + // A note on an ALLOW, deliberately: blocking every IaC edit because a + // container engine is down would be worse than an unscanned edit. + if kicsNote != "" { + return agenthooks.AllowWithNote(kicsNote) + } return agenthooks.AcceptEdit() } diff --git a/internal/commands/agenthooks/cx/hooks_test.go b/internal/commands/agenthooks/cx/hooks_test.go index 13898de1..36763db7 100644 --- a/internal/commands/agenthooks/cx/hooks_test.go +++ b/internal/commands/agenthooks/cx/hooks_test.go @@ -36,6 +36,9 @@ const ( "eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ." + "SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c" osWindows = "windows" + + telemetryEngineIaC = "IaC" + telemetryTypeHooksRemediate = "hooks-remediate" ) type recordingTelemetry struct { @@ -332,6 +335,8 @@ func TestCxBeforeFileEdit_TotalFileSize_Rejects(t *testing.T) { func TestCxBeforeFileEdit_KICSFinding_RejectsWithContext(t *testing.T) { resetHookGlobals(t) + tel := &recordingTelemetry{} + telemetryWrapper = tel kicsScanner = kics.NewScannerWithFunc(func(string, string) ([]iacrealtime.IacRealtimeResult, error) { return []iacrealtime.IacRealtimeResult{{ Title: "Privileged Container", @@ -357,6 +362,18 @@ func TestCxBeforeFileEdit_KICSFinding_RejectsWithContext(t *testing.T) { if !strings.Contains(v.Message, "KICS") { t.Errorf("expected KICS in reason, got %q", v.Message) } + if len(tel.calls) != 2 { + t.Fatalf("expected 2 telemetry calls (detect + remediate), got %d", len(tel.calls)) + } + if tel.calls[0].Type != "hooks-detect" || tel.calls[0].Engine != telemetryEngineIaC { + t.Errorf("detect telemetry = Type %q Engine %q", tel.calls[0].Type, tel.calls[0].Engine) + } + if tel.calls[1].Type != telemetryTypeHooksRemediate || tel.calls[1].Engine != telemetryEngineIaC { + t.Errorf("remediate telemetry = Type %q Engine %q", tel.calls[1].Type, tel.calls[1].Engine) + } + if tel.calls[1].ProblemSeverity != "HIGH" { + t.Errorf("ProblemSeverity = %q, want HIGH", tel.calls[1].ProblemSeverity) + } } func TestCxBeforeFileEdit_SCAManifest_RejectsWithContext(t *testing.T) { @@ -573,7 +590,7 @@ func TestLogRemediationTelemetry(t *testing.T) { if got.Engine != "Asca" || got.ScanType != "asca" { t.Errorf("Engine/ScanType = %q/%q", got.Engine, got.ScanType) } - if got.Type != "hooks-remediate" || got.SubType != "fixWithAIAssist" { + if got.Type != telemetryTypeHooksRemediate || got.SubType != "fixWithAIAssist" { t.Errorf("Type/SubType = %q/%q", got.Type, got.SubType) } if got.ProblemSeverity != "Critical" || got.AiAgentSessionId != "sess-9" { diff --git a/internal/commands/agenthooks/guardrails/kics/delta.go b/internal/commands/agenthooks/guardrails/kics/delta.go index 0387b8fc..cd3bb64e 100644 --- a/internal/commands/agenthooks/guardrails/kics/delta.go +++ b/internal/commands/agenthooks/guardrails/kics/delta.go @@ -1,13 +1,17 @@ package kics import ( + "encoding/json" "fmt" + "os" "path/filepath" "strings" agenthooks "github.com/Checkmarx/ast-cx-hooks" + "github.com/checkmarx/ast-cli/internal/commands/agenthooks/agentprofile" "github.com/checkmarx/ast-cli/internal/commands/agenthooks/cursorplugin" "github.com/checkmarx/ast-cli/internal/services/realtimeengine/iacrealtime" + "github.com/checkmarx/ast-cli/internal/services/realtimeengine/ignore" ) // findingKey is the deduplication tuple used for delta detection. @@ -55,8 +59,8 @@ func findingsSummary(filePath string, findings []iacrealtime.IacRealtimeResult) if description == "" { description = "No description provided" } - fmt.Fprintf(&sb, " - %s line %d [%s] %s — %s\n", - filePath, line, f.Severity, f.Title, description) + fmt.Fprintf(&sb, " - %s line %d [%s] %s (similarity_id %s) — %s\n", + filePath, line, f.Severity, f.Title, f.SimilarityID, description) } return sb.String() } @@ -64,111 +68,142 @@ func findingsSummary(filePath string, findings []iacrealtime.IacRealtimeResult) // formatFindings builds the two verdict fields delivered to the agent. // Cursor receives cursorAdditionalContext (folded into agent_message); other agents // (including Gemini) receive additionalContext, with MCP tool names adjusted per agent. -func formatFindings(filePath string, findings []iacrealtime.IacRealtimeResult, agent agenthooks.AgentID) (reason, context string) { +func formatFindings(filePath string, findings []iacrealtime.IacRealtimeResult, agent agenthooks.AgentID, workDir, sessionID string) (reason, context string) { summary := findingsSummary(filePath, findings) + cxBinary := cxExecutable() reason = permissionDecisionReason(filePath, summary) switch agent { case agenthooks.AgentCursor: - context = cursorAdditionalContext(filePath, findings) + context = cursorAdditionalContext(filePath, cxBinary, findings, workDir, sessionID) default: - context = additionalContext(filePath, findings, agent) + context = additionalContext(filePath, cxBinary, findings, workDir, agent, sessionID) } return reason, context } -// permissionDecisionReason is the human-readable deny message shown to the user. -func permissionDecisionReason(filePath, summary string) string { - return fmt.Sprintf( - "KICS security scan detected IaC vulnerabilities in %s.\nFindings:\n%s", - filePath, summary, - ) +func cxExecutable() string { + cxExe, err := os.Executable() + if err != nil { + return "cx" + } + return cxExe } -// dockerImagePlatforms are the KICS "platform" values (result.Platform, sourced from -// KICS query metadata) whose findings concern container images rather than generic -// IaC misconfigurations. These line up with the fileType enum accepted by the -// imageRemediation MCP tool (Dockerfile, DockerCompose). -var dockerImagePlatforms = map[string]bool{ - "dockerfile": true, - "dockercompose": true, - "docker compose": true, +// ignoredFilePathFlag returns the " --ignored-file-path ''" fragment that pins the +// suppression command to the workspace ignore file anchored at workDir. +func ignoredFilePathFlag(workDir string) string { + if workDir == "" { + return "" + } + return fmt.Sprintf(" --ignored-file-path '%s'", ignore.PathFor(workDir)) } -// isDockerImageFinding reports whether a finding's KICS platform identifies it as a -// container image issue (Dockerfile/docker-compose) rather than generic IaC. Falls -// back to filename heuristics only when platform is unavailable (e.g. older cached -// results), since platform is scanner-reported ground truth and filenames can vary. -func isDockerImageFinding(filePath string, findings []iacrealtime.IacRealtimeResult) bool { - for i := range findings { - if findings[i].Platform != "" { - return dockerImagePlatforms[strings.ToLower(findings[i].Platform)] - } +// cursorIgnoredFilePathFlag is the Cursor-specific variant of ignoredFilePathFlag. +func cursorIgnoredFilePathFlag(workDir string) string { + if workDir == "" { + return "" } - return isDockerImageFileByName(filePath) + p := filepath.ToSlash(ignore.PathFor(workDir)) + return fmt.Sprintf(" --ignored-file-path %q", p) } -// isDockerImageFileByName is a filename-based fallback for when KICS platform metadata -// isn't available. Mirrors the basename conventions in params.KicsBaseFilters plus the -// docker-compose/compose naming convention (not in KicsBaseFilters since compose files -// match on the generic .yml/.yaml extensions). -func isDockerImageFileByName(filePath string) bool { - base := strings.ToLower(filepath.Base(filePath)) - if base == "dockerfile" || strings.HasSuffix(base, ".dockerfile") { - return true +func optionalFlagsFragment(agent agenthooks.AgentID, sessionID string) string { + label := agentLabel(agent) + if label == "" { + return "" } - name := strings.TrimSuffix(strings.TrimSuffix(base, ".yaml"), ".yml") - return name == "docker-compose" || strings.HasPrefix(name, "docker-compose.") || - name == "compose" || strings.HasPrefix(name, "compose.") + pairs := "aiProvider=" + label + ";agent=" + label + "-cli" + if sessionID != "" { + pairs += ";aiAgentSessionId=" + sessionID + } + return fmt.Sprintf(" --optional-flags %q", pairs) } -// additionalContext is injected into the agent's context window to drive remediation. -// KICS is a deterministic IaC rule engine: unlike ASCA, its findings are not caused by -// missing cross-file context, so the agent is NOT given discretion to treat findings as -// false positives. Every new finding must be fixed. -// Used for Claude, Gemini, Copilot, and other non-Cursor agents. -func additionalContext(filePath string, findings []iacrealtime.IacRealtimeResult, agent agenthooks.AgentID) string { - var findingList strings.Builder - for _, f := range findings { - line := 0 - if len(f.Locations) > 0 { - line = f.Locations[0].Line +func agentLabel(agent agenthooks.AgentID) string { + switch agent { + case agenthooks.AgentClaude: + return "Claude" + case agenthooks.AgentCopilot, agenthooks.AgentCopilotCLI: + return "Copilot" + case agenthooks.AgentCursor: + return "Cursor" + case agenthooks.AgentGemini: + return "Gemini" + case agenthooks.AgentCodex: + return "Codex" + default: + return "" + } +} + +func kicsSuppressCommands(cxBinary string, findings []iacrealtime.IacRealtimeResult, workDir string, agent agenthooks.AgentID, sessionID string) string { + provenance := optionalFlagsFragment(agent, sessionID) + var suppressCmds strings.Builder + for i := range findings { + f := &findings[i] + data, _ := json.Marshal(iacrealtime.IgnoredIacFinding{ + Title: f.Title, + SimilarityID: f.SimilarityID, + }) + if agent == agenthooks.AgentCursor { + ignoreFlag := cursorIgnoredFilePathFlag(workDir) + suppressCmds.WriteString(cursorplugin.IgnoreVulnerabilityCommand(cxBinary, "iac", data, ignoreFlag, provenance)) + suppressCmds.WriteString("\n") + continue + } + ignoreFlag := ignoredFilePathFlag(workDir) + if agent == agenthooks.AgentGemini { + fmt.Fprintf(&suppressCmds, " %s ignore-vulnerability --scan-type iac --data %s%s%s\n", cxBinary, ignore.QuoteDataFlag(data), ignoreFlag, provenance) + } else { + fmt.Fprintf(&suppressCmds, " %s ignore-vulnerability --scan-type iac --data '%s'%s%s\n", cxBinary, string(data), ignoreFlag, provenance) } - fmt.Fprintf(&findingList, " - line %d [%s] %s: %s\n", - line, f.Severity, f.Title, f.Description) } - imageTool, codeTool := "mcp__Checkmarx__imageRemediation", "mcp__Checkmarx__codeRemediation" + return suppressCmds.String() +} + +// permissionDecisionReason is the human-readable deny message shown to the user. +func permissionDecisionReason(filePath, summary string) string { + return fmt.Sprintf( + "KICS security scan detected IaC vulnerabilities in %s.\nFindings:\n%s", + filePath, summary, + ) +} + +// additionalContext is injected into the agent's context window to drive remediation. +// Contains all action instructions — not shown directly to the user on Claude; on Gemini +// BeforeTool it is folded into the hook deny reason by the ast-cx-hooks gemini adapter. +// Used for Claude, Copilot, Gemini, and other non-Cursor agents. Gemini suppress commands +// use ignore.QuoteDataFlag (PowerShell-safe quoting on Windows); other agents keep the +// original single-quoted JSON payload. +func additionalContext(filePath, cxBinary string, findings []iacrealtime.IacRealtimeResult, workDir string, agent agenthooks.AgentID, sessionID string) string { + codeTool := "mcp__Checkmarx__codeRemediation" if agent == agenthooks.AgentGemini { - imageTool, codeTool = "mcp_Checkmarx_imageRemediation", "mcp_Checkmarx_codeRemediation" + codeTool = "mcp_Checkmarx_codeRemediation" } + skill := kicsSkillName(agent) + suppressCmds := kicsSuppressCommands(cxBinary, findings, workDir, agent, sessionID) return fmt.Sprintf( - "KICS detected IaC misconfigurations in %s. These are deterministic rule matches "+ - "against the configuration itself — they are NOT false positives caused by code "+ - "the scanner cannot see. Do not skip, suppress, or dismiss any finding as a false "+ - "positive, and do not bypass the scan by writing the same content through another "+ - "tool or shell command.\n"+ - "Fix every finding below, then retry the write:\n"+ - "%s"+ - "%s", - filePath, findingList.String(), remediationInstructions(filePath, findings, imageTool, codeTool), + "KICS detected IaC misconfigurations in %s. "+ + "Do not bypass the scan by writing the same content through another tool or shell command.\n"+ + "Action required:\n"+ + " 1. For each finding, invoke the %s skill — "+ + "the findings are already in context so it will skip the scan and go directly to "+ + "MCP-driven remediation; the skill also handles MCP unavailability and self-recovery.\n"+ + " 2. If that skill is not available in this session, %s\n"+ + " This is the only supported remediation path — do not attempt manual configuration changes.\n"+ + " 3. If %s is unavailable, tell the user to reconnect the\n"+ + " Checkmarx MCP (%s), then retry. Do not proceed until the MCP is available.\n"+ + " 4. If the user accepts the risk for a finding, suppress it by running the corresponding command\n"+ + " below, then retry the write:\n%s", + filePath, skill, remediationInstructions(codeTool), + codeTool, agentprofile.McpReconnect(agentLabel(agent)), suppressCmds, ) } -// remediationInstructions returns the tool-call guidance for the finding's file type. -// Dockerfile/docker-compose findings are about container images, so they must go -// through imageRemediation (base image CVEs, safer tags, hardening). All other -// KICS-supported files (Terraform, Kubernetes manifests, CloudFormation, etc.) are -// generic IaC misconfigurations and go through codeRemediation. -func remediationInstructions(filePath string, findings []iacrealtime.IacRealtimeResult, imageTool, codeTool string) string { - if isDockerImageFinding(filePath, findings) { - return fmt.Sprintf("For each finding, call the %s tool with:\n"+ - " {\n"+ - " \"imageName\": \"[image name from the finding/file, without the tag]\",\n"+ - " \"imageTag\": \"[image tag from the finding/file, e.g. latest]\",\n"+ - " \"fileType\": \"[Dockerfile or DockerCompose, matching this file]\"\n"+ - " }\n"+ - "Apply the remediation guidance the tool returns (safer base image, pinned digest, "+ - "hardening steps), then retry the write.", imageTool) - } +// remediationInstructions returns MCP tool-call guidance for KICS IaC findings. +// All findings from this guardrail come from RunIacRealtimeScan (KICS), so they +// always use codeRemediation with type "iac" — including Dockerfile findings. +func remediationInstructions(codeTool string) string { return fmt.Sprintf("For each finding, call the %s tool with:\n"+ " {\n"+ " \"type\": \"iac\",\n"+ @@ -184,54 +219,41 @@ func remediationInstructions(filePath string, findings []iacrealtime.IacRealtime "the finding.", codeTool) } -func cursorRemediationInstructions(filePath string, findings []iacrealtime.IacRealtimeResult) string { - if isDockerImageFinding(filePath, findings) { - return fmt.Sprintf("For each finding, call the %s tool with:\n"+ - " {\n"+ - " \"imageName\": \"[image name from the finding/file, without the tag]\",\n"+ - " \"imageTag\": \"[image tag from the finding/file, e.g. latest]\",\n"+ - " \"fileType\": \"[Dockerfile or DockerCompose, matching this file]\"\n"+ - " }\n"+ - "Apply the remediation guidance the tool returns (safer base image, pinned digest, "+ - "hardening steps), then retry the write.", cursorplugin.MCPTool("imageRemediation")) - } - return fmt.Sprintf("For each finding, call the %s tool with:\n"+ - " {\n"+ - " \"type\": \"iac\",\n"+ - " \"metadata\": {\n"+ - " \"title\": \"[Title from finding]\",\n"+ - " \"description\": \"[Description from finding]\",\n"+ - " \"remediationAdvice\": \"[how to harden this configuration]\"\n"+ - " }\n"+ - " }\n"+ - "Apply the remediation guidance the tool returns, then retry the write. If a fix "+ - "genuinely requires resources outside this file (for example a separate KMS key or "+ - "a centrally-managed policy), add them as part of your change rather than skipping "+ - "the finding.", cursorplugin.MCPTool("codeRemediation")) +// cursorAdditionalContext is remediation guidance for Cursor only. Uses the plugin-prefixed MCP +// tool name and PowerShell --% stop-parsing for suppress commands on Windows. +func cursorAdditionalContext(filePath, cxBinary string, findings []iacrealtime.IacRealtimeResult, workDir, sessionID string) string { + suppressCmds := kicsSuppressCommands(cxBinary, findings, workDir, agenthooks.AgentCursor, sessionID) + skill := kicsSkillName(agenthooks.AgentCursor) + return fmt.Sprintf( + "KICS detected IaC misconfigurations in %s. "+ + "Do not bypass the scan by writing the same content through another tool or shell command. "+ + "ANALYZE each finding to determine if it is a real misconfiguration or a false positive "+ + "(for example an acceptable deviation for this environment or platform). "+ + "Follow the cx-hook-deny.mdc rule for this deny. "+ + "ASK THE USER FIRST, for every real finding, before taking any action: \"An IaC misconfiguration "+ + "was detected. Would you like to remediate it (harden the configuration via MCP) "+ + "or suppress it (mark as a confirmed false positive and unblock the write)?\" and wait for "+ + "their answer. Do not decide this yourself — an intentionally-inserted misconfiguration (e.g. "+ + "in a lab/demo/training file the user asked for on purpose) is NOT the same as a confirmed "+ + "false positive: suppress only on the user's explicit instruction, never because the "+ + "request seems intentional. "+ + "Apply the cx-devassist-kics.mdc rule: for each finding the user asks you to remediate, "+ + "invoke the %s skill exactly as written — do not skip, abbreviate, or reimplement its steps "+ + "inline. The findings are already in context so it will skip the scan and go directly to "+ + "MCP-driven remediation; the skill also handles MCP unavailability and self-recovery. "+ + "Always show its Step 4 IaC Remediation Summary to the user verbatim when done. "+ + "Do not retry the blocked Write/StrReplace, paste code in chat, or bypass the scan with shell workarounds. "+ + "If that skill is not available in this session, %s\n"+ + "If the user chooses to suppress a finding, run the corresponding command below, then retry the write:\n%s", + filePath, skill, remediationInstructions(cursorplugin.MCPTool("codeRemediation")), + suppressCmds, + ) } -// cursorAdditionalContext is remediation guidance for Cursor only. Cursor has no -// additionalContext field on preToolUse — ast-cx-hooks folds this into agent_message. -func cursorAdditionalContext(filePath string, findings []iacrealtime.IacRealtimeResult) string { - var findingList strings.Builder - for i := range findings { - f := &findings[i] - line := 0 - if len(f.Locations) > 0 { - line = f.Locations[0].Line - } - fmt.Fprintf(&findingList, " - line %d [%s] %s: %s\n", - line, f.Severity, f.Title, f.Description) +// kicsSkillName returns the agent-specific skill invocation string for KICS remediation. +func kicsSkillName(agent agenthooks.AgentID) string { + if agent == agenthooks.AgentGemini { + return "/cx-devassist-kics" } - return fmt.Sprintf( - "KICS IaC findings in %s — apply the cx-hook-deny.mdc rule for this deny, and the "+ - "cx-devassist-kics.mdc rule exactly as written: do not "+ - "skip, abbreviate, or reorder its steps, and always show its Step 5 IaC Remediation Summary "+ - "to the user verbatim when done. "+ - "Do not retry the blocked Write/StrReplace, paste code in chat, or bypass the scan with shell workarounds.\n\n"+ - "Fix every finding below (deterministic IaC rule matches — not false positives). "+ - "%s\n"+ - "%s", - filePath, cursorRemediationInstructions(filePath, findings), findingList.String(), - ) + return "cx-devassist:cx-devassist-kics" } diff --git a/internal/commands/agenthooks/guardrails/kics/delta_test.go b/internal/commands/agenthooks/guardrails/kics/delta_test.go index a8f1f7e5..bcf8efc6 100644 --- a/internal/commands/agenthooks/guardrails/kics/delta_test.go +++ b/internal/commands/agenthooks/guardrails/kics/delta_test.go @@ -90,7 +90,7 @@ func TestNewFindings_DeltaDedup_SameKeyNotDoubled(t *testing.T) { func TestFormatFindings_ReasonContainsKICS(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) + reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") if !strings.Contains(reason, "KICS") { t.Errorf("reason should contain KICS, got: %q", reason) } @@ -98,7 +98,7 @@ func TestFormatFindings_ReasonContainsKICS(t *testing.T) { func TestFormatFindings_ReasonContainsFilePath(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) + reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") if !strings.Contains(reason, "/project/Dockerfile") { t.Errorf("reason should contain file path, got: %q", reason) } @@ -106,7 +106,7 @@ func TestFormatFindings_ReasonContainsFilePath(t *testing.T) { func TestFormatFindings_ReasonContainsSeverityAndTitle(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) + reason, _ := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") if !strings.Contains(reason, "HIGH") { t.Errorf("reason should contain severity, got: %q", reason) } @@ -117,87 +117,45 @@ func TestFormatFindings_ReasonContainsSeverityAndTitle(t *testing.T) { func TestFormatFindings_ContextContainsFixInstruction(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) - if !strings.Contains(ctx, "fix") && !strings.Contains(ctx, "Fix") { - t.Errorf("context should contain fix instruction, got: %q", ctx) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") + if !strings.Contains(ctx, "fix") && !strings.Contains(ctx, "Fix") && !strings.Contains(ctx, "remediation") { + t.Errorf("context should contain fix/remediation instruction, got: %q", ctx) } } func TestFormatFindings_ContextContainsDoNotBypass(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") if !strings.Contains(ctx, "bypass") { t.Errorf("context should warn against bypass, got: %q", ctx) } } -// ── isDockerImageFinding / remediation tool routing ──────────────────────────── +// ── remediation tool routing ───────────────────────────────────────────────── -func TestIsDockerImageFinding_ByPlatform(t *testing.T) { - cases := []struct { - platform string - want bool - }{ - {"Dockerfile", true}, - {"DockerCompose", true}, - {"Docker Compose", true}, - {"dockerfile", true}, - {"Terraform", false}, - {"Kubernetes", false}, - {"CloudFormation", false}, - {"Ansible", false}, - } - for _, c := range cases { - findings := []iacrealtime.IacRealtimeResult{ - iacResultWithPlatform("SomeFinding", c.platform), - } - // Filename deliberately contradicts platform to prove platform wins. - if got := isDockerImageFinding("/project/values.yaml", findings); got != c.want { - t.Errorf("isDockerImageFinding with platform %q = %v, want %v", c.platform, got, c.want) - } - } -} - -func TestIsDockerImageFinding_FallsBackToFilenameWhenPlatformEmpty(t *testing.T) { - cases := map[string]bool{ - "/project/Dockerfile": true, - "/project/api.dockerfile": true, - "/project/docker-compose.yml": true, - "/project/docker-compose.yaml": true, - "/project/docker-compose.prod.yml": true, - "/project/compose.yaml": true, - "/project/main.tf": false, - "/project/deployment.yaml": false, - "/project/values.yaml": false, - } - for path, want := range cases { - findings := []iacrealtime.IacRealtimeResult{iacResult("SomeFinding", "sim1", "HIGH", 1)} - if got := isDockerImageFinding(path, findings); got != want { - t.Errorf("isDockerImageFinding(%q) with no platform = %v, want %v", path, got, want) - } - } -} - -func TestFormatFindings_DockerfilePlatformUsesImageRemediation(t *testing.T) { +func TestFormatFindings_DockerfilePlatformUsesCodeRemediation(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{ iacResultWithPlatform("VulnerableBaseImage", "Dockerfile"), } - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) - if !strings.Contains(ctx, "mcp__Checkmarx__imageRemediation") { - t.Errorf("Dockerfile context should call imageRemediation, got: %q", ctx) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") + if !strings.Contains(ctx, "mcp__Checkmarx__codeRemediation") { + t.Errorf("Dockerfile KICS context should call codeRemediation, got: %q", ctx) } - if strings.Contains(ctx, "mcp__Checkmarx__codeRemediation") { - t.Errorf("Dockerfile context should not call codeRemediation, got: %q", ctx) + if strings.Contains(ctx, "mcp__Checkmarx__imageRemediation") { + t.Errorf("Dockerfile KICS context should not call imageRemediation, got: %q", ctx) } } -func TestFormatFindings_DockerComposePlatformUsesImageRemediation(t *testing.T) { +func TestFormatFindings_DockerComposePlatformUsesCodeRemediation(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{ iacResultWithPlatform("VulnerableBaseImage", "DockerCompose"), } - _, ctx := formatFindings("/project/stack.yml", findings, agenthooks.AgentClaude) - if !strings.Contains(ctx, "mcp__Checkmarx__imageRemediation") { - t.Errorf("docker-compose context should call imageRemediation, got: %q", ctx) + _, ctx := formatFindings("/project/stack.yml", findings, agenthooks.AgentClaude, "/project", "sess1") + if !strings.Contains(ctx, "mcp__Checkmarx__codeRemediation") { + t.Errorf("docker-compose KICS context should call codeRemediation, got: %q", ctx) + } + if strings.Contains(ctx, "mcp__Checkmarx__imageRemediation") { + t.Errorf("docker-compose KICS context should not call imageRemediation, got: %q", ctx) } } @@ -205,7 +163,7 @@ func TestFormatFindings_TerraformUsesCodeRemediation(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{ iacResultWithPlatform("OpenSecurityGroup", "Terraform"), } - _, ctx := formatFindings("/project/main.tf", findings, agenthooks.AgentClaude) + _, ctx := formatFindings("/project/main.tf", findings, agenthooks.AgentClaude, "/project", "sess1") if !strings.Contains(ctx, "mcp__Checkmarx__codeRemediation") { t.Errorf("Terraform context should call codeRemediation, got: %q", ctx) } @@ -214,36 +172,36 @@ func TestFormatFindings_TerraformUsesCodeRemediation(t *testing.T) { } } -func TestCursorAdditionalContext_UsesImageRemediation(t *testing.T) { +func TestCursorAdditionalContext_UsesCodeRemediation(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - ctx := cursorAdditionalContext("/project/Dockerfile", findings) - if !strings.Contains(ctx, "mcp__plugin-cx-devassist-Checkmarx__imageRemediation") { - t.Errorf("cursor KICS context should use imageRemediation, got: %q", ctx) + ctx := cursorAdditionalContext("/project/Dockerfile", "cx", findings, "/project", "sess1") + if !strings.Contains(ctx, "mcp__plugin-cx-devassist-Checkmarx__codeRemediation") { + t.Errorf("cursor KICS context should use codeRemediation, got: %q", ctx) } - if strings.Contains(ctx, "codeRemediation") { - t.Errorf("cursor KICS context should not use codeRemediation, got: %q", ctx) + if strings.Contains(ctx, "imageRemediation") { + t.Errorf("cursor KICS context should not use imageRemediation, got: %q", ctx) } if !strings.Contains(ctx, "cx-devassist-kics.mdc") { t.Errorf("cursor KICS context should reference cx-devassist-kics.mdc rule, got: %q", ctx) } + if !strings.Contains(ctx, "cx-devassist:cx-devassist-kics") { + t.Errorf("cursor KICS context should reference cx-devassist:cx-devassist-kics skill, got: %q", ctx) + } } func TestFormatFindings_RoutesCursorContext(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentCursor) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentCursor, "/project", "sess1") if !strings.Contains(ctx, "cx-devassist-kics.mdc") { t.Fatalf("cursor agent should get context with rule reference, got %q", ctx) } if strings.Contains(ctx, "MANDATORY NEXT STEPS") { t.Fatalf("cursor context should not have verbose MANDATORY NEXT STEPS block, got %q", ctx) } - if !strings.Contains(ctx, "imageRemediation") { - t.Fatalf("cursor KICS context should reference imageRemediation, got %q", ctx) + if !strings.Contains(ctx, "codeRemediation") { + t.Fatalf("cursor KICS context should reference codeRemediation, got %q", ctx) } - // Use a non-Docker path for the Claude assertion below: Dockerfile findings - // always route through imageRemediation (see isDockerImageFinding), so - // asserting codeRemediation here requires a generic IaC file instead. - _, ctx = formatFindings("/project/main.tf", findings, agenthooks.AgentClaude) + _, ctx = formatFindings("/project/main.tf", findings, agenthooks.AgentClaude, "/project", "sess1") if strings.Contains(ctx, "cx-devassist-kics.mdc") { t.Fatalf("claude agent should not get cursor-specific rule reference, got %q", ctx) } @@ -256,27 +214,52 @@ func TestAdditionalContext_GeminiUsesUnderscoreMCPNames(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{ iacResultWithPlatform("VulnerableBaseImage", "Dockerfile"), } - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentGemini) - if !strings.Contains(ctx, "mcp_Checkmarx_imageRemediation") { - t.Errorf("Gemini context should use underscore MCP name, got: %q", ctx) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentGemini, "/project", "sess1") + if !strings.Contains(ctx, "mcp_Checkmarx_codeRemediation") { + t.Errorf("Gemini context should use underscore codeRemediation MCP name, got: %q", ctx) } - if strings.Contains(ctx, "mcp__Checkmarx__imageRemediation") { + if strings.Contains(ctx, "mcp__Checkmarx__codeRemediation") { t.Errorf("Gemini context should not use double-underscore MCP name, got: %q", ctx) } + if strings.Contains(ctx, "imageRemediation") { + t.Errorf("Gemini KICS context should not use imageRemediation, got: %q", ctx) + } } -func TestAdditionalContext_ClaudeDoesNotOfferSuppress(t *testing.T) { +func TestAdditionalContext_ClaudeOffersSuppress(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude) - if strings.Contains(ctx, "ignore-vulnerability") { - t.Errorf("Claude context should not include suppress commands, got %q", ctx) + _, ctx := formatFindings("/project/Dockerfile", findings, agenthooks.AgentClaude, "/project", "sess1") + if !strings.Contains(ctx, "ignore-vulnerability") { + t.Errorf("Claude context should include suppress commands, got %q", ctx) + } + if !strings.Contains(ctx, `--scan-type iac`) { + t.Errorf("Claude context should include iac scan type, got %q", ctx) } } -func TestCursorAdditionalContext_DoesNotOfferSuppress(t *testing.T) { +func TestCursorAdditionalContext_OffersSuppress(t *testing.T) { findings := []iacrealtime.IacRealtimeResult{iacResult("PrivilegedContainer", "sim1", "HIGH", 5)} - ctx := cursorAdditionalContext("/project/Dockerfile", findings) - if strings.Contains(ctx, "ignore-vulnerability") { - t.Errorf("cursor context should not include suppress commands, got %q", ctx) + ctx := cursorAdditionalContext("/project/Dockerfile", "cx", findings, "/project", "sess1") + if !strings.Contains(ctx, "ignore-vulnerability") { + t.Errorf("cursor context should include suppress commands, got %q", ctx) + } +} + +func TestCursorAdditionalContext_MatchesAscaAskUserWording(t *testing.T) { + ctx := cursorAdditionalContext("/project/main.tf", "cx", nil, "/project", "sess1") + for _, want := range []string{ + "ANALYZE each finding", + "for every real finding", + "mark as a confirmed false positive and unblock the write", + "intentionally-inserted misconfiguration", + "never because the request seems intentional", + "If the user chooses to suppress a finding", + } { + if !strings.Contains(ctx, want) { + t.Errorf("cursor KICS context should contain %q, got: %q", want, ctx) + } + } + if strings.Contains(ctx, "accept the risk") { + t.Errorf("cursor KICS context should not use old suppress wording, got: %q", ctx) } } diff --git a/internal/commands/agenthooks/guardrails/kics/kics.go b/internal/commands/agenthooks/guardrails/kics/kics.go index d57a9d4b..0b98adbc 100644 --- a/internal/commands/agenthooks/guardrails/kics/kics.go +++ b/internal/commands/agenthooks/guardrails/kics/kics.go @@ -1,6 +1,7 @@ package kics import ( + "fmt" "os" "path/filepath" "strings" @@ -8,7 +9,9 @@ import ( agenthooks "github.com/Checkmarx/ast-cx-hooks" "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" + "github.com/checkmarx/ast-cli/internal/services/realtimeengine/iacrealtime" "github.com/checkmarx/ast-cli/internal/services/realtimeengine/ignore" + "github.com/checkmarx/ast-cli/internal/wrappers" ) // isSupportedByKICS returns true when the file matches a KICS-supported extension or basename. @@ -38,34 +41,43 @@ func isSupportedByKICS(filePath string) bool { } // ScanFileEdit runs KICS on the proposed post-edit content. -// Returns blocked=true with a formatted reason and remediation context when KICS +// Returns blocked=true with a formatted reason, remediation context, and highest severity when KICS // finds *new* vulnerabilities introduced by ev.Changes (delta-detection for edits; // any-vuln for new writes). Findings the user already suppressed via // `cx ignore-vulnerability` (the realtime ignore file) are filtered out before the -// verdict. Fail-open on infrastructure errors (Docker unavailable, image pull fail, panic). -func ScanFileEdit(ev agenthooks.FileEditEvent, svc *Scanner) (blocked bool, reason, context string) { +// verdict. Fail-open on infrastructure errors (Docker unavailable, image pull fail, panic), +// returning a skippedNote so the skipped check is visible. +func ScanFileEdit(ev *agenthooks.FileEditEvent, svc *Scanner, telemetryWrapper wrappers.TelemetryWrapper, agent string) (blocked bool, reason, context, note, severity string) { + findingCount := 0 + defer func() { if r := recover(); r != nil { logger.PrintfIfVerbose("kics guardrail: recovered from panic, failing open: %v", r) blocked = false reason = "" context = "" + note = skippedNote(ev.FilePath, fmt.Errorf("internal error: %v", r)) + severity = "" } + logKicsTelemetry(telemetryWrapper, agent, ev.SessionID, findingCount) }() if !isSupportedByKICS(ev.FilePath) { - return false, "", "" + return false, "", "", "", "" } newContent, originalContent, err := proposedContent(ev.FilePath, ev.Changes) - if err != nil || newContent == "" { - return false, "", "" + if err != nil { + return false, "", "", skippedNote(ev.FilePath, err), "" + } + if newContent == "" { + return false, "", "", "", "" } // Stage and scan the proposed (new) content stagedNew, cleanupNew, err := stageForScan(ev.FilePath, newContent, ev.SessionID) if err != nil { - return false, "", "" + return false, "", "", skippedNote(ev.FilePath, err), "" } defer cleanupNew() @@ -74,22 +86,23 @@ func ScanFileEdit(ev agenthooks.FileEditEvent, svc *Scanner) (blocked bool, reas if err != nil { // Fail open: Docker unavailable, image pull failure, feature flag disabled, etc. logger.PrintfIfVerbose("kics guardrail: scan of proposed content failed, failing open: %v", err) - return false, "", "" + return false, "", "", skippedNote(ev.FilePath, err), "" } if len(newResults) == 0 { - return false, "", "" + return false, "", "", "", "" } // For new files (no original content), every finding is new if originalContent == "" { - r, c := formatFindings(ev.FilePath, newResults, ev.Agent) - return true, r, c + r, c := formatFindings(ev.FilePath, newResults, ev.Agent, ev.WorkDir, ev.SessionID) + findingCount = len(newResults) + return true, r, c, "", highestSeverity(newResults) } // Delta: scan original content and find only newly introduced findings stagedOrig, cleanupOrig, err := stageForScan(ev.FilePath, originalContent, ev.SessionID) if err != nil { - return false, "", "" + return false, "", "", skippedNote(ev.FilePath, err), "" } defer cleanupOrig() @@ -97,16 +110,66 @@ func ScanFileEdit(ev agenthooks.FileEditEvent, svc *Scanner) (blocked bool, reas if err != nil { // Fail open on original scan error logger.PrintfIfVerbose("kics guardrail: scan of original content failed, failing open: %v", err) - return false, "", "" + return false, "", "", skippedNote(ev.FilePath, err), "" } newFindings := NewFindings(origResults, newResults) if len(newFindings) == 0 { - return false, "", "" + return false, "", "", "", "" + } + + r, c := formatFindings(ev.FilePath, newFindings, ev.Agent, ev.WorkDir, ev.SessionID) + findingCount = len(newFindings) + return true, r, c, "", highestSeverity(newFindings) +} + +// highestSeverity returns the highest severity level across the given KICS findings. +// Order: Critical > High > Medium > Low > (anything else). +func highestSeverity(findings []iacrealtime.IacRealtimeResult) string { + rank := map[string]int{"critical": 4, "high": 3, "medium": 2, "low": 1} + best := "" + bestRank := -1 + for i := range findings { + sevLower := strings.ToLower(findings[i].Severity) + if r, ok := rank[sevLower]; ok && r > bestRank { + bestRank = r + best = findings[i].Severity + } } + return best +} + +// logKicsTelemetry sends a telemetry event for KICS scan results. +// Called once after KICS scan is performed with the actual finding count. +func logKicsTelemetry(telemetryWrapper wrappers.TelemetryWrapper, agent, sessionID string, totalCount int) { + if telemetryWrapper == nil || totalCount == 0 { + return + } + + telemetryData := &wrappers.DataForAITelemetry{ + Agent: agent + "-cli", + AIProvider: agent, + Engine: "IaC", + TotalCount: totalCount, + UniqueID: wrappers.GetUniqueID(), + Type: "hooks-detect", + SubType: "scan", + ScanType: "iac", + AiAgentSessionId: sessionID, + } + + if err := telemetryWrapper.SendAIDataToLog(telemetryData); err != nil { + // fail-open: telemetry is best-effort and must never block the guardrail + logger.PrintfIfVerbose("kics guardrail: failed to send telemetry: %v", err) + } +} - r, c := formatFindings(ev.FilePath, newFindings, ev.Agent) - return true, r, c +// skippedNote is what the user sees when the guardrail fails open. Without it a +// file edited with no container engine running is indistinguishable from a file +// that scanned clean — the edit is allowed either way, and nothing says why. +func skippedNote(filePath string, err error) string { + return fmt.Sprintf("Checkmarx IaC guardrail skipped %s: %v. The edit was allowed without an IaC security check.", + filepath.Base(filePath), err) } // existingIgnoreFilePath returns the realtime ignore-file path anchored at workDir only diff --git a/internal/commands/agenthooks/guardrails/kics/kics_test.go b/internal/commands/agenthooks/guardrails/kics/kics_test.go index 4fd2ddd2..058f6f1b 100644 --- a/internal/commands/agenthooks/guardrails/kics/kics_test.go +++ b/internal/commands/agenthooks/guardrails/kics/kics_test.go @@ -3,6 +3,7 @@ package kics import ( + "errors" "fmt" "os" "path/filepath" @@ -12,6 +13,9 @@ import ( agenthooks "github.com/Checkmarx/ast-cx-hooks" "github.com/checkmarx/ast-cli/internal/services/realtimeengine" "github.com/checkmarx/ast-cli/internal/services/realtimeengine/iacrealtime" + "github.com/checkmarx/ast-cli/internal/wrappers" + "github.com/checkmarx/ast-cli/internal/wrappers/mock" + "github.com/stretchr/testify/assert" ) // ── isSupportedByKICS ──────────────────────────────────────────────────────── @@ -94,6 +98,18 @@ func makeResult(title, similarityID, severity, description string, line int) iac } } +// scanFileEditResult names ScanFileEdit's return values so tests that only care +// about a subset of them don't need a long run of blank identifiers. +type scanFileEditResult struct { + blocked bool + reason, context, note, severity string +} + +func scanFileEdit(ev *agenthooks.FileEditEvent, svc *Scanner) scanFileEditResult { + blocked, reason, context, note, severity := ScanFileEdit(ev, svc, nil, "Claude") + return scanFileEditResult{blocked, reason, context, note, severity} +} + func TestScanFileEdit_NewFileWithFinding_Blocked(t *testing.T) { finding := makeResult("Privileged Container", "sim123", "HIGH", "Container runs as privileged", 5) svc := NewScannerWithFunc(func(_, _ string) ([]iacrealtime.IacRealtimeResult, error) { @@ -106,18 +122,21 @@ func TestScanFileEdit_NewFileWithFinding_Blocked(t *testing.T) { Changes: []agenthooks.FileDiff{{Before: "", After: "FROM ubuntu\nUSER root\n"}}, } - blocked, reason, ctx := ScanFileEdit(ev, svc) - if !blocked { + res := scanFileEdit(&ev, svc) + if !res.blocked { t.Fatal("expected edit to be blocked") } - if reason == "" { + if res.reason == "" { t.Error("expected non-empty reason") } - if ctx == "" { + if res.context == "" { t.Error("expected non-empty context") } - if !strings.Contains(reason, "KICS") { - t.Errorf("reason should mention KICS, got: %q", reason) + if res.severity != "HIGH" { + t.Errorf("severity = %q, want HIGH", res.severity) + } + if !strings.Contains(res.reason, "KICS") { + t.Errorf("reason should mention KICS, got: %q", res.reason) } } @@ -140,8 +159,8 @@ func TestScanFileEdit_EditWithNoNewFindings_NotBlocked(t *testing.T) { Changes: []agenthooks.FileDiff{{Before: "FROM ubuntu", After: "FROM ubuntu:22.04"}}, } - blocked, _, _ := ScanFileEdit(ev, svc) - if blocked { + res := scanFileEdit(&ev, svc) + if res.blocked { t.Fatal("expected edit to NOT be blocked when no new findings") } } @@ -157,8 +176,8 @@ func TestScanFileEdit_ScanError_FailOpen(t *testing.T) { Changes: []agenthooks.FileDiff{{Before: "", After: "resource \"aws_s3_bucket\" \"bad\" {}"}}, } - blocked, _, _ := ScanFileEdit(ev, svc) - if blocked { + res := scanFileEdit(&ev, svc) + if res.blocked { t.Fatal("expected fail-open (not blocked) on scan error") } } @@ -175,8 +194,8 @@ func TestScanFileEdit_UnsupportedFile_NotBlocked(t *testing.T) { Changes: []agenthooks.FileDiff{{Before: "", After: "package main"}}, } - blocked, _, _ := ScanFileEdit(ev, svc) - if blocked { + res := scanFileEdit(&ev, svc) + if res.blocked { t.Fatal("expected NOT blocked for unsupported file") } } @@ -192,8 +211,65 @@ func TestScanFileEdit_EmptyNewContent_NotBlocked(t *testing.T) { Changes: []agenthooks.FileDiff{{Before: "", After: ""}}, } - blocked, _, _ := ScanFileEdit(ev, svc) - if blocked { + res := scanFileEdit(&ev, svc) + if res.blocked { t.Fatal("expected NOT blocked for empty content") } } + +// A scan that could not run must say so — a silent fail-open is +// indistinguishable from a clean scan, which is how unscanned IaC ships. +func TestScanFileEdit_EngineDownProducesNote(t *testing.T) { + svc := NewScannerWithFunc(func(path, ignoreFilePath string) ([]iacrealtime.IacRealtimeResult, error) { + return nil, errors.New("container engine 'docker' is installed but not running") + }) + ev := agenthooks.FileEditEvent{ + FilePath: filepath.Join(t.TempDir(), "main.tf"), + Changes: []agenthooks.FileDiff{{Before: "", After: "resource \"aws_s3_bucket\" \"b\" {}"}}, + } + + res := scanFileEdit(&ev, svc) + + assert.False(t, res.blocked, "must fail open, not block the edit") + assert.Contains(t, res.note, "main.tf") + assert.Contains(t, res.note, "not running") +} + +// ── logKicsTelemetry ───────────────────────────────────────────────────────── + +func TestLogKicsTelemetry_NilWrapper_NoOp(t *testing.T) { + assert.NotPanics(t, func() { + logKicsTelemetry(nil, "Claude", "", 3) + }) +} + +func TestLogKicsTelemetry_ZeroCount_DoesNotSend(t *testing.T) { + sent := false + telemetry := mock.TelemetryMockWrapper{ + CustomSendAIDataToLog: func(data *wrappers.DataForAITelemetry) error { + sent = true + return nil + }, + } + logKicsTelemetry(telemetry, "Claude", "", 0) + assert.False(t, sent) +} + +func TestLogKicsTelemetry_WithFindings_Sends(t *testing.T) { + var captured *wrappers.DataForAITelemetry + telemetry := mock.TelemetryMockWrapper{ + CustomSendAIDataToLog: func(data *wrappers.DataForAITelemetry) error { + captured = data + return nil + }, + } + logKicsTelemetry(telemetry, "Claude", "sess-1", 2) + assert.NotNil(t, captured) + assert.Equal(t, "IaC", captured.Engine) + assert.Equal(t, 2, captured.TotalCount) + assert.Equal(t, "Claude", captured.AIProvider) + assert.Equal(t, "hooks-detect", captured.Type) + assert.Equal(t, "scan", captured.SubType) + assert.Equal(t, "iac", captured.ScanType) + assert.Equal(t, "sess-1", captured.AiAgentSessionId) +} diff --git a/internal/commands/agenthooks/guardrails/kics/scanner.go b/internal/commands/agenthooks/guardrails/kics/scanner.go index aea876dd..70716e98 100644 --- a/internal/commands/agenthooks/guardrails/kics/scanner.go +++ b/internal/commands/agenthooks/guardrails/kics/scanner.go @@ -1,9 +1,12 @@ package kics import ( + "errors" "os" "os/exec" + "strings" + "github.com/checkmarx/ast-cli/internal/logger" "github.com/checkmarx/ast-cli/internal/params" "github.com/checkmarx/ast-cli/internal/services/realtimeengine/iacrealtime" "github.com/checkmarx/ast-cli/internal/wrappers" @@ -16,6 +19,14 @@ type Scanner struct { jwt wrappers.JWTWrapper ff wrappers.FeatureFlagsWrapper scan func(path, ignoreFilePath string) ([]iacrealtime.IacRealtimeResult, error) + + // engine that last completed a scan. A delta edit scans twice (proposed and + // original content), so without this the second scan repeats the whole + // selection — including a failed engine's image-inspect and pull. One hook + // invocation is one event on one goroutine, so no lock is needed. A value + // that goes stale (the engine stopped since) costs one failed scan and is + // then corrected by the fallback below. + engine string } // NewScanner returns a Scanner backed by the given wrappers. @@ -31,25 +42,35 @@ func NewScannerWithFunc(f func(path, ignoreFilePath string) ([]iacrealtime.IacRe return &Scanner{scan: f} } +// Container engine names. +const ( + engineDocker = "docker" + enginePodman = "podman" +) + // defaultContainerEngine mirrors the "docker" default of the --engine flag on // the manual `cx scan iac-realtime` command (internal/commands/scan.go), used // when neither an override nor auto-detection finds a usable engine. -const defaultContainerEngine = "docker" +const defaultContainerEngine = engineDocker -// resolveContainerEngine picks the container engine name to pass to -// RunIacRealtimeScan. The guardrail is invoked as `cx hooks ` with only -// stdin JSON (no --engine flag like the manual `cx scan iac-realtime` -// command), so it resolves the engine itself: +// resolveContainerEngine picks the container engine to try first. The guardrail +// is invoked as `cx hooks ` with only stdin JSON (no --engine flag like +// the manual `cx scan iac-realtime` command), so it resolves the engine itself: // 1. HooksContainerEngineEnv, if set — lets a Podman/Colima-only user (or the // agent plugin's own hook environment) override the choice explicitly. -// 2. Auto-detect via PATH lookup: try "docker" then "podman", first one found wins. +// 2. PATH lookup: try "docker" then "podman", first one found wins. // 3. defaultContainerEngine, if neither resolves — preserves prior behavior // and existing error messaging when no engine is installed at all. +// +// Deliberately a PATH lookup and nothing more: it runs before every IaC scan, +// and a daemon probe here would add a round-trip to edits that scan fine. +// Whether the daemon is actually up is settled by fallbackEngineFor, which is +// reached only after a scan has already failed. func resolveContainerEngine() string { - if engine := os.Getenv(params.HooksContainerEngineEnv); engine != "" { + if engine := engineOverride(); engine != "" { return engine } - for _, engine := range []string{"docker", "podman"} { + for _, engine := range []string{engineDocker, enginePodman} { if _, err := exec.LookPath(engine); err == nil { return engine } @@ -57,7 +78,92 @@ func resolveContainerEngine() string { return defaultContainerEngine } +// fallbackEngineFor returns the engine to retry with after a scan failed on +// `tried`, or "" when there is nothing worth retrying. A resolvable binary is +// not proof of a usable engine — Docker Desktop and the Podman machine can be +// installed but stopped, which is what made the guardrail fail open and let +// vulnerable IaC through. Reached only after a failure, so the daemon probe it +// costs never lands on a working scan. +func fallbackEngineFor(tried string) string { + if engineOverride() != "" { + return "" // explicit user choice — do not second-guess it + } + other := enginePodman + if tried == enginePodman { + other = engineDocker + } + if !engineReady(other) { + return "" + } + return other +} + +// engineOverride is the user's explicit engine choice, if any. Read in one place +// so resolveContainerEngine and fallbackEngineFor cannot disagree about it. +func engineOverride() string { + return os.Getenv(params.HooksContainerEngineEnv) +} + +// engineReady is iacrealtime.IsEngineRunning; replaced in tests. +var engineReady = iacrealtime.IsEngineRunning + +// engineInstalled is iacrealtime.IsEngineInstalled; replaced in tests. +var engineInstalled = iacrealtime.IsEngineInstalled + +// errAllEnginesNotRunning is returned when Docker and Podman are both installed +// but neither daemon is running — a distinct case from a single-engine failure +// so agent audit logs can record container_engine=both. +var errAllEnginesNotRunning = errors.New("container engines 'docker' and 'podman' are installed but not running; " + + "start Docker Desktop or the Podman machine and retry") + +func isEngineNotRunningError(err error) bool { + return err != nil && strings.Contains(err.Error(), "is installed but not running") +} + +func bothEnginesInstalledButStopped() bool { + if engineOverride() != "" { + return false + } + if !engineInstalled(engineDocker) || !engineInstalled(enginePodman) { + return false + } + return !engineReady(engineDocker) && !engineReady(enginePodman) +} + +// scanErrorAfterNoFallback shapes the error returned when there is no alternate +// engine to retry — upgrading to errAllEnginesNotRunning when both are down. +func scanErrorAfterNoFallback(err error) error { + if err == nil { + return nil + } + if bothEnginesInstalledButStopped() && isEngineNotRunningError(err) { + return errAllEnginesNotRunning + } + return err +} + func (s *Scanner) runRealScan(path, ignoreFilePath string) ([]iacrealtime.IacRealtimeResult, error) { svc := iacrealtime.NewIacRealtimeService(s.jwt, s.ff, iacrealtime.NewContainerManager()) - return svc.RunIacRealtimeScan(path, resolveContainerEngine(), ignoreFilePath) + + engine := s.engine + if engine == "" { + engine = resolveContainerEngine() + } + results, err := svc.RunIacRealtimeScan(path, engine, ignoreFilePath) + if err == nil { + s.engine = engine + return results, nil + } + + other := fallbackEngineFor(engine) + if other == "" { + return results, scanErrorAfterNoFallback(err) + } + + logger.PrintfIfVerbose("kics guardrail: %s scan failed (%v); retrying with %s", engine, err, other) + results, err = svc.RunIacRealtimeScan(path, other, ignoreFilePath) + if err == nil { + s.engine = other + } + return results, err } diff --git a/internal/commands/agenthooks/guardrails/kics/scanner_test.go b/internal/commands/agenthooks/guardrails/kics/scanner_test.go index 22ee1191..0168aadf 100644 --- a/internal/commands/agenthooks/guardrails/kics/scanner_test.go +++ b/internal/commands/agenthooks/guardrails/kics/scanner_test.go @@ -3,6 +3,7 @@ package kics import ( + "errors" "testing" "github.com/checkmarx/ast-cli/internal/params" @@ -11,8 +12,6 @@ import ( "github.com/stretchr/testify/assert" ) -const enginePodman = "podman" - // ── NewScanner ────────────────────────────────────────────────────────────── func TestNewScanner_ReturnsValidScanner(t *testing.T) { @@ -120,3 +119,87 @@ func TestResolveContainerEngine_EmptyEnvFallsBack(t *testing.T) { got := resolveContainerEngine() assert.Equal(t, defaultContainerEngine, got) } + +// ── fallbackEngineFor ──────────────────────────────────────────────────────── + +func stubEngineReady(t *testing.T, ready string) { + t.Helper() + orig := engineReady + t.Cleanup(func() { engineReady = orig }) + engineReady = func(engine string) bool { return engine == ready } +} + +// A stopped Docker must hand the scan to a live Podman — otherwise the scan +// errors, the guardrail fails open, and vulnerable IaC ships unflagged. +func TestFallbackEngineFor_StoppedDockerFallsBackToPodman(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineReady(t, enginePodman) + + assert.Equal(t, enginePodman, fallbackEngineFor(engineDocker)) +} + +func TestFallbackEngineFor_StoppedPodmanFallsBackToDocker(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineReady(t, engineDocker) + + assert.Equal(t, engineDocker, fallbackEngineFor(enginePodman)) +} + +func TestFallbackEngineFor_NoRetryWhenOtherEngineIsAlsoDown(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineReady(t, "") + + assert.Equal(t, "", fallbackEngineFor(engineDocker)) +} + +// An explicit override is the user's choice; silently switching engines under +// them would be worse than the error they asked for. +func TestFallbackEngineFor_NoRetryWhenEngineExplicitlyOverridden(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, engineDocker) + stubEngineReady(t, enginePodman) + + assert.Equal(t, "", fallbackEngineFor(engineDocker)) +} + +// ── scanErrorAfterNoFallback ───────────────────────────────────────────────── + +func stubEngineInstalled(t *testing.T, installed map[string]bool) { + t.Helper() + orig := engineInstalled + t.Cleanup(func() { engineInstalled = orig }) + engineInstalled = func(engine string) bool { return installed[engine] } +} + +func TestScanErrorAfterNoFallback_BothInstalledBothStopped(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineInstalled(t, map[string]bool{engineDocker: true, enginePodman: true}) + stubEngineReady(t, "") + + primary := errors.New("container engine 'docker' is installed but not running") + got := scanErrorAfterNoFallback(primary) + assert.Equal(t, errAllEnginesNotRunning, got) +} + +func TestScanErrorAfterNoFallback_SingleEngineStopped(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineInstalled(t, map[string]bool{engineDocker: true, enginePodman: false}) + stubEngineReady(t, "") + + primary := errors.New("container engine 'docker' is installed but not running") + got := scanErrorAfterNoFallback(primary) + assert.Equal(t, primary, got) +} + +func TestScanErrorAfterNoFallback_NotRunningErrorRequired(t *testing.T) { + t.Setenv(params.HooksContainerEngineEnv, "") + stubEngineInstalled(t, map[string]bool{engineDocker: true, enginePodman: true}) + stubEngineReady(t, "") + + primary := errors.New("container engine 'docker' not found") + got := scanErrorAfterNoFallback(primary) + assert.Equal(t, primary, got) +} + +func TestScanErrorAfterNoFallback_NilError(t *testing.T) { + assert.Nil(t, scanErrorAfterNoFallback(nil)) +} diff --git a/internal/services/realtimeengine/iacrealtime/container-manager.go b/internal/services/realtimeengine/iacrealtime/container-manager.go index 0a8fcae6..3025bf96 100644 --- a/internal/services/realtimeengine/iacrealtime/container-manager.go +++ b/internal/services/realtimeengine/iacrealtime/container-manager.go @@ -1,10 +1,12 @@ package iacrealtime import ( + "context" "os" "os/exec" "path/filepath" "strings" + "time" "github.com/checkmarx/ast-cli/internal/commands/util" "github.com/checkmarx/ast-cli/internal/kicsshutdown" @@ -108,6 +110,21 @@ func createCommandWithEnhancedPath(enginePath string, args ...string) *exec.Cmd return cmd } +// daemonResponds reports whether the engine at enginePath has a live daemon. +// `--version` answers from the binary alone, so it stays true while Docker +// Desktop or the Podman machine is stopped; `info` needs the daemon. Goes +// through createCommandWithEnhancedPath so a GUI-launched macOS IDE, which does +// not inherit the shell PATH, resolves credential helpers the same way every +// other engine call in this package does. +func daemonResponds(enginePath string) bool { + ctx, cancel := context.WithTimeout(context.Background(), engineVerifyTimeout*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, enginePath, "info") + cmd.Env = createCommandWithEnhancedPath(enginePath, "info").Env + return cmd.Run() == nil +} + // EnsureImageAvailable checks if the KICS image exists locally and pulls it if not available. // Returns the resolved engine path on success. func (dm *ContainerManager) EnsureImageAvailable(engine string) (string, error) { @@ -140,6 +157,14 @@ func (dm *ContainerManager) EnsureImageAvailable(engine string) (string, error) outputStr := strings.TrimSpace(string(output)) logger.PrintIfVerbose("Failed to pull KICS image. Output: " + outputStr) + // Without this the message below blames the network, sending the user to + // check connectivity when the real fix is to start the engine. Probing + // here costs nothing: the pull has already failed. + if !daemonResponds(resolvedEngine) { + return "", errors.Errorf("container engine '%s' is installed but not running. "+ + "Start Docker Desktop or the Podman machine and retry.", engine) + } + if outputStr != "" { return "", errors.Errorf("Failed to pull KICS image '%s': %s. Please check your network connectivity or pull the image manually using: %s pull %s", util.ContainerImage, outputStr, resolvedEngine, util.ContainerImage) diff --git a/internal/services/realtimeengine/iacrealtime/iac-realtime.go b/internal/services/realtimeengine/iacrealtime/iac-realtime.go index 87c63677..a0290b92 100644 --- a/internal/services/realtimeengine/iacrealtime/iac-realtime.go +++ b/internal/services/realtimeengine/iacrealtime/iac-realtime.go @@ -176,6 +176,25 @@ func engineNameResolution(engineName, fallBackDir string) (string, error) { return "", errors.Errorf("%s not found in PATH or in fallback locations: %v", engineName, checkedPaths) } +// IsEngineInstalled reports whether engineName can be resolved on this OS (PATH, +// plus the macOS GUI fallback paths engineNameResolution knows about), regardless +// of whether the daemon is running. +func IsEngineInstalled(engineName string) bool { + _, err := engineNameResolution(engineName, IacEnginePath) + return err == nil +} + +// IsEngineRunning reports whether engineName is usable right now: resolvable on +// this OS (PATH, plus the macOS GUI fallback paths engineNameResolution knows +// about) AND with a responding daemon. +func IsEngineRunning(engineName string) bool { + enginePath, err := engineNameResolution(engineName, IacEnginePath) + if err != nil { + return false + } + return daemonResponds(enginePath) +} + // getFallbackPaths returns a list of paths to check for the container engine func getFallbackPaths(engineName, fallBackDir string) []string { var paths []string diff --git a/internal/services/realtimeengine/iacrealtime/iac-realtime_test.go b/internal/services/realtimeengine/iacrealtime/iac-realtime_test.go index 88a271e5..17181e83 100644 --- a/internal/services/realtimeengine/iacrealtime/iac-realtime_test.go +++ b/internal/services/realtimeengine/iacrealtime/iac-realtime_test.go @@ -1211,3 +1211,10 @@ func TestVerifyEnginePath_ValidSystemExecutable(t *testing.T) { t.Errorf("verifyEnginePath should return true for valid executable: %s", execPath) } } + +// An engine that resolves nowhere is never "running", on every OS. +func TestIsEngineRunning_UnresolvableEngine(t *testing.T) { + if IsEngineRunning("cx-no-such-container-engine") { + t.Error("IsEngineRunning should be false for an engine that is not installed") + } +}