diff --git a/.claude/knowledge/testing-patterns.md b/.claude/knowledge/testing-patterns.md index 7eeb663..cecc603 100644 --- a/.claude/knowledge/testing-patterns.md +++ b/.claude/knowledge/testing-patterns.md @@ -8,7 +8,7 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s - **Never commit a contiguous real-format secret as a test fixture.** GitHub push protection rejects the push (it flags GitLab/Stripe/SendGrid/Twilio/etc. tokens even in `_test.go` files), and it is poor practice in a secret-detection repo. Assemble fixtures at runtime from a prefix + body via the `cred(prefix, body)` helper in `tests/checks/test_helpers_test.go` (e.g. `cred("AKIA", "1234567890ABCDEF")`), splitting the recognizable provider prefix from the rest so no full token literal appears in source. The reconstructed value still exercises the scanner identically. `goConst(value)` wraps a value as a minimal Go source fixture. - The MCP server smoke tests (`tests/mcp/`) drive the **real binary in a subprocess**, not the in-process handler, to keep tests external. Pattern: a `Test...HelperProcess` func gated by an env var (`GO_WANT_MCP_HELPER_PROCESS` for stdio, `GO_WANT_MCP_HTTP_HELPER_PROCESS` for HTTP) re-execs `os.Args[0]` and calls `cli.Run(...)`. stdio replays NDJSON transcripts over stdin; HTTP reserves a free `127.0.0.1:0` port, launches `serve --mcp --http`, polls `/healthz` for readiness, then issues real HTTP requests. Add new MCP behavior as a transcript + assertion (stdio) or a subtest in `http_test.go` (HTTP). - **Detector precision corpus** (`tests/corpus/`): fixtures live under `testdata///{vulnerable,clean}/` with ground truth in `expectations.yaml`. `known_gaps` entries assert a documented FP/FN *still exists* — when you improve a detector the corpus test fails on purpose with a "promote it to must_fire / remove the known_gaps entry" message; update the manifest in the same change. Fixture credentials must be synthetic but pattern-shaped (see the secret-fixture rule above). -- **Every catalog rule must ship a `FixTemplate`** with a valid `Kind` (`deterministic`|`guided`) — `TestSDKRuleMetadataFixTemplatesPopulated` iterates the full catalog and fails on any rule without one. When adding a rule, add its template to the family map in `internal/codeguard/rules/catalog_fix_templates_*.go` (or inline in the catalog entry; inline wins over the registry). +- **Every catalog rule must ship a `FixTemplate`** with a valid `Kind` (`deterministic`|`guided`) — `TestSDKRuleMetadataFixTemplatesPopulated` iterates the full catalog and fails on any rule without one. When adding a rule, add its template to the family map in `internal/codeguard/rules/catalog_fix_templates_*.go` (or inline in the catalog entry; inline wins over the registry). Security rules (`security.*`) additionally need an OWASP Top 10 mapping in `internal/codeguard/rules/catalog_security_owasp.go` — `TestEverySecurityRuleHasOWASPCategory` fails otherwise (only `security.command-check` is exempt, since its category depends on the external tool). - `gofmt -l .` at repo root is polluted by `.claude/worktrees/` (live agent worktrees) and `.gomodcache/`; scope it to `gofmt -l cmd internal pkg tests changelog.go` or use `make fmt-check`. - **Bidirectional (server→client) MCP tests** live in `tests/mcp/sampling_test.go`: the test acts as the MCP client, advertises `sampling`/`roots` at `initialize`, and answers the server's server-initiated requests. stdio uses interactive `StdinPipe`/`StdoutPipe` (not the replay harness). HTTP opens the `GET /mcp` SSE stream (waits for the `: ready` comment to avoid the attach race), reads the request off the stream, and POSTs the response with the matching `Mcp-Session-Id`. propose_fix verification is expected to fail on the throwaway diff — assert the round trip fired, not a verified patch. The HTTP helper passes `-config` via `CODEGUARD_TEST_HTTP_CONFIG`. - **TS tests can be hijacked by the Node semantic engine**: on hosts with a discoverable `typescript.js` (e.g. VS Code installed), TypeScript targets route through the Node runner instead of the per-file Go path. Tests that must exercise the per-file path (tree-sitter differential tests, corpus TS groups) set `CODEGUARD_TYPESCRIPT_LIB_PATH` to an existing-but-invalid lib to force the fallback. diff --git a/docs/checks.md b/docs/checks.md index f07dbf1..a10858f 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -529,6 +529,11 @@ Config keys (under `checks.security_rules.secrets`): - `entropy` enables the high-entropy heuristic (off by default); tune `min_length` (default 20), `threshold` in bits/char (default 4.5), and `level` (default `warn`). +Invalid `allow_patterns`/`custom_patterns` entries are rejected by config validation. If +an unvalidated config reaches the scan anyway (e.g. through the SDK), the unusable +pattern is skipped and reported as a `security.secrets-config` (**fail**) finding rather +than silently reducing coverage. + Existing `exclude`, `waivers`, `baseline`, and inline `codeguard:ignore` suppressions also apply to these findings. diff --git a/internal/cli/commands.go b/internal/cli/commands.go index ef2bc1d..6cf389a 100644 --- a/internal/cli/commands.go +++ b/internal/cli/commands.go @@ -50,9 +50,8 @@ func runValidate(args []string, stdout io.Writer, stderr io.Writer) int { return code } - cfg, err := loadConfigWithProfile(*configPath, *profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) + cfg, ok := loadConfigOrFail(*configPath, *profile, stderr) + if !ok { return exitError } if err := service.ValidateConfig(cfg); err != nil { @@ -88,9 +87,8 @@ func runScan(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) return exitError } - cfg, err := loadConfigWithProfile(*inputs.configPath, *flags.profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) + cfg, ok := loadConfigOrFail(*inputs.configPath, *flags.profile, stderr) + if !ok { return exitError } if trimmedFormat := strings.TrimSpace(*format); trimmedFormat != "" { @@ -125,9 +123,8 @@ func runValidatePatch(args []string, stdin io.Reader, stdout io.Writer, stderr i return exitError } - cfg, err := loadConfigWithProfile(*configPath, *profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) + cfg, ok := loadConfigOrFail(*configPath, *profile, stderr) + if !ok { return exitError } if trimmedFormat := strings.TrimSpace(*format); trimmedFormat != "" { @@ -164,9 +161,8 @@ func runBaseline(args []string, stdout io.Writer, stderr io.Writer) int { } flags.applyTrustPolicy() - cfg, err := loadConfigWithProfile(*flags.configPath, *flags.profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) + cfg, ok := loadConfigOrFail(*flags.configPath, *flags.profile, stderr) + if !ok { return exitError } cfg.Baseline.Path = "" diff --git a/internal/cli/fix.go b/internal/cli/fix.go index 1b156a3..ee51486 100644 --- a/internal/cli/fix.go +++ b/internal/cli/fix.go @@ -22,22 +22,21 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int { line := fs.Int("line", 0, "optional 1-based line to target") format := fs.String("format", "text", "output format: text or json") if err := fs.Parse(args); err != nil { - return 1 + return exitError } flags.applyTrustPolicy() if !*enableAI { _, _ = fmt.Fprintln(stderr, "fix requires -ai so unverified AI patch generation is never implicit") - return 1 + return exitError } scanMode, err := parseScanMode(*flags.mode) if err != nil { _, _ = fmt.Fprintln(stderr, err) - return 1 + return exitError } - cfg, err := loadConfigWithProfile(*flags.configPath, *flags.profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) - return 1 + cfg, ok := loadConfigOrFail(*flags.configPath, *flags.profile, stderr) + if !ok { + return exitError } report, err := service.RunWithOptions(context.Background(), cfg, service.ScanOptions{ Mode: scanMode, @@ -46,21 +45,21 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int { }) if err != nil { _, _ = fmt.Fprintf(stderr, "scan failed: %v\n", err) - return 1 + return exitError } finding, ok := selectFixFinding(report, strings.TrimSpace(*ruleID), strings.TrimSpace(*path), *line) if !ok { _, _ = fmt.Fprintln(stderr, "no matching finding available for fix generation") - return 1 + return exitError } generator, available, err := internalfix.NewAIGenerator(cfg.AI) if err != nil { _, _ = fmt.Fprintf(stderr, "initialize ai generator: %v\n", err) - return 1 + return exitError } if !available { _, _ = fmt.Fprintln(stderr, "no AI provider is configured for fix generation") - return 1 + return exitError } result, err := service.GenerateVerifiedFix(context.Background(), service.FixGenerateRequest{ Config: cfg, @@ -74,7 +73,7 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int { }) if err != nil { _, _ = fmt.Fprintf(stderr, "generate verified fix: %v\n", err) - return 1 + return exitError } return writeFixResult(stdout, stderr, result, strings.TrimSpace(*format)) } @@ -115,18 +114,18 @@ func writeFixResult(stdout io.Writer, stderr io.Writer, result service.VerifiedF _, _ = fmt.Fprintf(stdout, "- %s (%s)\n", firstNonEmpty(step.CheckName, step.Command), step.TargetName) } } - return 0 + return exitOK case "json": data, err := json.MarshalIndent(result, "", " ") if err != nil { _, _ = fmt.Fprintf(stderr, "marshal fix result: %v\n", err) - return 1 + return exitError } _, _ = stdout.Write(append(data, '\n')) - return 0 + return exitOK default: _, _ = fmt.Fprintf(stderr, "unsupported fix output format %q\n", format) - return 1 + return exitError } } diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index de90e33..bfc0715 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -27,6 +27,18 @@ func promptString(reader *bufio.Reader, stdout io.Writer, label string, fallback return line, nil } +// loadConfigOrFail wraps loadConfigWithProfile with the shared handler +// convention: on failure it reports "load config: " on stderr and returns +// ok=false so the handler can return exitError. +func loadConfigOrFail(path string, profile string, stderr io.Writer) (service.Config, bool) { + cfg, err := loadConfigWithProfile(path, profile) + if err != nil { + _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) + return service.Config{}, false + } + return cfg, true +} + func loadConfigWithProfile(path string, profile string) (service.Config, error) { cfg, err := service.LoadConfigFile(path) if err != nil { diff --git a/internal/cli/info.go b/internal/cli/info.go index eb32b25..b299db3 100644 --- a/internal/cli/info.go +++ b/internal/cli/info.go @@ -16,15 +16,14 @@ func runRules(args []string, stdout io.Writer, stderr io.Writer) int { configPath := fs.String("config", "", "optional config path to include custom rule packs") profile := fs.String("profile", "", "optional policy profile override") if err := fs.Parse(args); err != nil { - return 1 + return exitError } rules := service.Rules() if strings.TrimSpace(*configPath) != "" { - cfg, err := loadConfigWithProfile(*configPath, *profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) - return 1 + cfg, ok := loadConfigOrFail(*configPath, *profile, stderr) + if !ok { + return exitError } rules = service.RulesForConfig(cfg) } @@ -47,7 +46,7 @@ func runRules(args []string, stdout io.Writer, stderr io.Writer) int { } _, _ = fmt.Fprintln(stdout, line.String()) } - return 0 + return exitOK } func runExplain(args []string, stdout io.Writer, stderr io.Writer) int { @@ -57,22 +56,22 @@ func runExplain(args []string, stdout io.Writer, stderr io.Writer) int { format := fs.String("format", "text", "output format: text, agent") profile := fs.String("profile", "", "optional policy profile override") if err := fs.Parse(args); err != nil { - return 1 + return exitError } if fs.NArg() == 0 { _, _ = fmt.Fprintln(stderr, "explain requires a rule id") - return 1 + return exitError } ruleID := fs.Arg(0) rule, ok, err := resolveExplainRule(*configPath, *profile, ruleID) if err != nil { _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) - return 1 + return exitError } if !ok { _, _ = fmt.Fprintf(stderr, "unknown rule %q\n", ruleID) - return 1 + return exitError } switch strings.TrimSpace(*format) { @@ -81,13 +80,13 @@ func runExplain(args []string, stdout io.Writer, stderr io.Writer) int { case "agent": if err := writeExplainAgent(stdout, rule); err != nil { _, _ = fmt.Fprintf(stderr, "write explain output: %v\n", err) - return 1 + return exitError } default: _, _ = fmt.Fprintf(stderr, "invalid explain format %q\n", *format) - return 1 + return exitError } - return 0 + return exitOK } func resolveExplainRule(configPath string, profile string, ruleID string) (service.RuleMetadata, bool, error) { @@ -175,5 +174,5 @@ func runProfiles(stdout io.Writer) int { for _, profile := range service.Profiles() { _, _ = fmt.Fprintf(stdout, "%s\t%s\n", profile.Name, profile.Description) } - return 0 + return exitOK } diff --git a/internal/cli/owasp.go b/internal/cli/owasp.go index 5689ee2..7d6ffe2 100644 --- a/internal/cli/owasp.go +++ b/internal/cli/owasp.go @@ -19,15 +19,14 @@ func runOWASP(args []string, stdout io.Writer, stderr io.Writer) int { profile := fs.String("profile", "", "optional policy profile override") format := fs.String("format", "text", "output format: text or json") if err := fs.Parse(args); err != nil { - return 1 + return exitError } coverage := service.OWASPCoverage() if strings.TrimSpace(*configPath) != "" { - cfg, err := loadConfigWithProfile(*configPath, *profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) - return 1 + cfg, ok := loadConfigOrFail(*configPath, *profile, stderr) + if !ok { + return exitError } coverage = service.OWASPCoverageForConfig(cfg) } @@ -40,13 +39,13 @@ func runOWASP(args []string, stdout io.Writer, stderr io.Writer) int { encoder.SetIndent("", " ") if err := encoder.Encode(coverage); err != nil { _, _ = fmt.Fprintf(stderr, "write owasp output: %v\n", err) - return 1 + return exitError } default: _, _ = fmt.Fprintf(stderr, "invalid owasp format %q\n", *format) - return 1 + return exitError } - return 0 + return exitOK } func writeOWASPText(stdout io.Writer, coverage []service.OWASPCoverageEntry) { diff --git a/internal/cli/report.go b/internal/cli/report.go index 12108dc..b35f71f 100644 --- a/internal/cli/report.go +++ b/internal/cli/report.go @@ -18,17 +18,16 @@ func runReport(args []string, stdout io.Writer, stderr io.Writer) int { slopHistory := fs.Bool("slop-history", false, "print the persisted slop-score trend per target") limit := fs.Int("limit", 0, "maximum history entries to print per target (0 = all)") if err := fs.Parse(args); err != nil { - return 1 + return exitError } if !*slopHistory { _, _ = fmt.Fprintln(stderr, "report requires a mode flag: -slop-history") - return 1 + return exitError } - cfg, err := loadConfigWithProfile(*configPath, *profile) - if err != nil { - _, _ = fmt.Fprintf(stderr, "load config: %v\n", err) - return 1 + cfg, ok := loadConfigOrFail(*configPath, *profile, stderr) + if !ok { + return exitError } return writeSlopHistoryReport(stdout, cfg, *limit) } @@ -38,7 +37,7 @@ func writeSlopHistoryReport(stdout io.Writer, cfg service.Config, limit int) int history := service.LoadSlopHistory(path) if len(history) == 0 { _, _ = fmt.Fprintf(stdout, "no slop-score history recorded at %s\n", path) - return 0 + return exitOK } keys := make([]string, 0, len(history)) for key := range history { diff --git a/internal/cli/scan_history.go b/internal/cli/scan_history.go index 4eb2f95..5ba6493 100644 --- a/internal/cli/scan_history.go +++ b/internal/cli/scan_history.go @@ -20,7 +20,7 @@ func runScanHistory(args []string, stdout io.Writer, stderr io.Writer) int { allRefs := fs.Bool("all", false, "scan all refs rather than just HEAD history") format := fs.String("format", "text", "output format: text or json") if err := fs.Parse(args); err != nil { - return 1 + return exitError } // Config is optional: it only supplies secret allowlist/custom-pattern/entropy @@ -37,20 +37,20 @@ func runScanHistory(args []string, stdout io.Writer, stderr io.Writer) int { }) if err != nil { _, _ = fmt.Fprintf(stderr, "scan-history failed: %v\n", err) - return 1 + return exitError } if err := writeHistoryReport(stdout, report, *format); err != nil { _, _ = fmt.Fprintf(stderr, "scan-history output: %v\n", err) - return 1 + return exitError } for _, finding := range report.Findings { if finding.Level == "fail" { - return 1 + return exitError } } - return 0 + return exitOK } func writeHistoryReport(stdout io.Writer, report service.HistoryReport, format string) error { diff --git a/internal/codeguard/checks/quality/quality_coverage_go.go b/internal/codeguard/checks/quality/quality_coverage_go.go index 11818f2..9b36798 100644 --- a/internal/codeguard/checks/quality/quality_coverage_go.go +++ b/internal/codeguard/checks/quality/quality_coverage_go.go @@ -37,8 +37,10 @@ func goCoverageProfile(ctx context.Context, env support.Context, target core.Tar Args: args, }) if err != nil { + // Keep err in the chain so callers can tell a timeout/cancellation from + // a test run that failed. if strings.TrimSpace(output) != "" { - return nil, fmt.Errorf("go test: %s", output) + return nil, fmt.Errorf("go test: %w: %s", err, output) } return nil, fmt.Errorf("go test: %w", err) } diff --git a/internal/codeguard/checks/quality/quality_coverage_lcov.go b/internal/codeguard/checks/quality/quality_coverage_lcov.go index 662fb13..c23bf62 100644 --- a/internal/codeguard/checks/quality/quality_coverage_lcov.go +++ b/internal/codeguard/checks/quality/quality_coverage_lcov.go @@ -25,10 +25,12 @@ func commandCoverageProfile(ctx context.Context, env support.Context, target cor Args: command.Args, }) if err != nil { + // Keep err in the chain so callers can tell a timeout/cancellation from + // a command that ran and reported failures. if strings.TrimSpace(output) != "" { - return nil, fmt.Errorf("%s: %s", name, output) + return nil, fmt.Errorf("%s: %w: %s", name, err, output) } - return nil, err + return nil, fmt.Errorf("%s: %w", name, err) } reportPath := command.ReportPath if !filepath.IsAbs(reportPath) { diff --git a/internal/codeguard/checks/security/security.go b/internal/codeguard/checks/security/security.go index 5e25269..0654bee 100644 --- a/internal/codeguard/checks/security/security.go +++ b/internal/codeguard/checks/security/security.go @@ -24,7 +24,15 @@ func securityTargetFindings(ctx context.Context, env support.Context, target cor // Use a distinct cache section id ("security-secrets") so this pass does not // collide with the per-file cache of the language pass below, which also // scans the "security" section for the same files. - if scanner := BuildScanner(env.Config.Checks.SecurityRules.Secrets); scanner.Enabled() { + scanner, scannerIssues := BuildScanner(env.Config.Checks.SecurityRules.Secrets) + for _, issue := range scannerIssues { + findings = append(findings, env.NewFinding(support.FindingInput{ + RuleID: "security.secrets-config", + Level: "fail", + Message: issue, + })) + } + if scanner.Enabled() { findings = append(findings, env.ScanTargetFiles(target, "security-secrets", func(string) bool { return true }, func(file string, data []byte) []core.Finding { return secretFindingsForFile(env, file, data, scanner) })...) diff --git a/internal/codeguard/checks/security/security_secrets.go b/internal/codeguard/checks/security/security_secrets.go index 25977b5..0dfb1d5 100644 --- a/internal/codeguard/checks/security/security_secrets.go +++ b/internal/codeguard/checks/security/security_secrets.go @@ -1,6 +1,7 @@ package security import ( + "fmt" "regexp" "strings" @@ -54,23 +55,36 @@ func (s Scanner) Enabled() bool { return s.enabled } // BuildScanner compiles a Scanner from config. A nil config yields the default // enabled scanner with no allowlist, no custom patterns, and entropy disabled. -func BuildScanner(cfg *core.SecretsRulesConfig) Scanner { +// Entries that cannot be used (unparseable regexes, custom patterns without an +// id) are skipped and reported as issues; config validation normally rejects +// them before this point, so the issues are the backstop for callers scanning +// with an unvalidated config. +func BuildScanner(cfg *core.SecretsRulesConfig) (Scanner, []string) { scanner := Scanner{enabled: true} if cfg == nil { - return scanner + return scanner, nil } + var issues []string if cfg.Enabled != nil { scanner.enabled = *cfg.Enabled } scanner.allowPaths = append([]string(nil), cfg.AllowPaths...) - for _, pattern := range cfg.AllowPatterns { - if re, err := regexp.Compile(pattern); err == nil { - scanner.allowRes = append(scanner.allowRes, re) + for idx, pattern := range cfg.AllowPatterns { + re, err := regexp.Compile(pattern) + if err != nil { + issues = append(issues, fmt.Sprintf("secrets allow_patterns[%d] skipped: %v", idx, err)) + continue } + scanner.allowRes = append(scanner.allowRes, re) } for _, custom := range cfg.CustomPatterns { + if strings.TrimSpace(custom.ID) == "" { + issues = append(issues, "secrets custom_patterns entry with an empty id skipped") + continue + } re, err := regexp.Compile(custom.Regex) - if err != nil || strings.TrimSpace(custom.ID) == "" { + if err != nil { + issues = append(issues, fmt.Sprintf("secrets custom_patterns[%q] skipped: %v", custom.ID, err)) continue } level := normalizeSecretLevel(custom.Level, "fail") @@ -81,7 +95,7 @@ func BuildScanner(cfg *core.SecretsRulesConfig) Scanner { scanner.customPatterns = append(scanner.customPatterns, compiledCustomPattern{id: custom.ID, re: re, level: level, msg: message}) } scanner.entropy = buildEntropySettings(cfg.Entropy) - return scanner + return scanner, issues } // SkipPath reports whether the file is covered by an allow_paths glob. diff --git a/internal/codeguard/rules/catalog_fix_templates_security.go b/internal/codeguard/rules/catalog_fix_templates_security.go index 80d0827..9211f80 100644 --- a/internal/codeguard/rules/catalog_fix_templates_security.go +++ b/internal/codeguard/rules/catalog_fix_templates_security.go @@ -13,6 +13,7 @@ var securityFixTemplates = map[string]core.FixTemplate{ "security.private-key": {Kind: guided, Text: "Delete the committed key, rotate it, and load key material from secure storage at runtime.\n\nBefore:\n-----BEGIN RSA PRIVATE KEY-----\nMIIEow...\n\nAfter:\n// key removed from the repository; provisioned by the deploy environment\nkey, err := os.ReadFile(os.Getenv(\"TLS_KEY_PATH\"))\n// rotate the exposed key and purge it from git history"}, "security.insecure-tls": {Kind: deterministic, Text: "Remove InsecureSkipVerify so certificate verification stays on.\n\nBefore:\nclient := &http.Client{Transport: &http.Transport{\n\tTLSClientConfig: &tls.Config{InsecureSkipVerify: true},\n}}\n\nAfter:\nclient := &http.Client{} // the default transport verifies certificates"}, "security.shell-execution": {Kind: guided, Text: "Run a fixed binary with an argv list instead of handing a composed string to a shell.\n\nBefore:\nexec.Command(\"sh\", \"-c\", \"convert \"+userFile)\n\nAfter:\nexec.Command(\"convert\", userFile) // fixed binary, arguments passed without a shell"}, + "security.secrets-config": {Kind: deterministic, Text: "Fix the unusable pattern in security_rules.secrets so it participates in the scan.\n\nBefore:\nsecrets:\n custom_patterns:\n - id: acme-token\n regex: \"acme_live_[0-9a-f]{16\" # unbalanced brace, pattern skipped\n\nAfter:\nsecrets:\n custom_patterns:\n - id: acme-token\n regex: \"acme_live_[0-9a-f]{16}\""}, "security.govulncheck": {Kind: guided, Text: "Upgrade the affected dependency past the fixed version and re-run govulncheck.\n\nBefore:\n$ govulncheck ./...\nGO-2023-2102: golang.org/x/net before v0.17.0 ...\n\nAfter:\n$ go get golang.org/x/net@v0.17.0 && go mod tidy\n$ govulncheck ./... # reports no findings"}, "security.command-check": {Kind: guided, Text: "Run the configured security command locally and fix each reported finding at its source.\n\nBefore:\n$ bandit -r app/\nB602 subprocess call with shell=True identified\n\nAfter:\nsubprocess.run([\"convert\", user_file], check=True) # fix at the source\n# re-run until the command exits 0; adjust the command only if it does not fit the target"}, "security.cors-wildcard": {Kind: guided, Text: "Reflect a validated allowlist of trusted origins instead of returning '*'.\n\nBefore:\nw.Header().Set(\"Access-Control-Allow-Origin\", \"*\")\n\nAfter:\nif allowedOrigins[origin] {\n\tw.Header().Set(\"Access-Control-Allow-Origin\", origin)\n\tw.Header().Set(\"Vary\", \"Origin\")\n}"}, diff --git a/internal/codeguard/rules/catalog_security.go b/internal/codeguard/rules/catalog_security.go index f8dffee..b91251e 100644 --- a/internal/codeguard/rules/catalog_security.go +++ b/internal/codeguard/rules/catalog_security.go @@ -30,6 +30,15 @@ var securityCatalog = map[string]core.RuleMetadata{ Description: "Warns on the lower-confidence name-based heuristic: a secret/token/api_key/password identifier assigned a quoted literal.", HowToFix: "Remove the secret from the repository and load it from a secret manager or environment at runtime.", }, + "security.secrets-config": { + ID: "security.secrets-config", + Section: "Security", + DefaultLevel: "fail", + ExecutionModel: core.RuleExecutionModelLanguageAgnostic, + Title: "Invalid secret-scan configuration", + Description: "Fails when a security_rules.secrets allow_pattern or custom_pattern cannot be compiled and had to be skipped, so the scan is running with less coverage than configured.", + HowToFix: "Fix the regex (or the empty custom-pattern id) in security_rules.secrets so the pattern participates in the scan.", + }, "security.private-key": { ID: "security.private-key", Section: "Security", diff --git a/internal/codeguard/rules/catalog_security_owasp.go b/internal/codeguard/rules/catalog_security_owasp.go index d559a75..193c992 100644 --- a/internal/codeguard/rules/catalog_security_owasp.go +++ b/internal/codeguard/rules/catalog_security_owasp.go @@ -12,6 +12,7 @@ var securityRuleOWASP = map[string]core.OWASPCategory{ "security.hardcoded-secret": core.OWASPA07AuthFailures, "security.high-entropy-string": core.OWASPA07AuthFailures, "security.private-key": core.OWASPA02CryptographicFailures, + "security.secrets-config": core.OWASPA05SecurityMisconfiguration, // Transport security. "security.insecure-tls": core.OWASPA02CryptographicFailures, diff --git a/pkg/codeguard/sdk_history.go b/pkg/codeguard/sdk_history.go index 4ab1398..3ca61cf 100644 --- a/pkg/codeguard/sdk_history.go +++ b/pkg/codeguard/sdk_history.go @@ -2,6 +2,8 @@ package codeguard import ( "context" + "fmt" + "strings" "github.com/devr-tools/codeguard/internal/codeguard/checks/security" "github.com/devr-tools/codeguard/internal/codeguard/history" @@ -25,7 +27,10 @@ type HistoryReport = history.Report // patterns, entropy). Findings that only exist in history still represent leaked // credentials that must be rotated. func ScanGitHistory(ctx context.Context, cfg Config, opts HistoryScanOptions) (HistoryReport, error) { - scanner := security.BuildScanner(cfg.Checks.SecurityRules.Secrets) + scanner, issues := security.BuildScanner(cfg.Checks.SecurityRules.Secrets) + if len(issues) > 0 { + return HistoryReport{}, fmt.Errorf("invalid secret scan config: %s", strings.Join(issues, "; ")) + } return history.Scan(ctx, history.Options{ RepoPath: opts.RepoPath, MaxCommits: opts.MaxCommits, diff --git a/tests/checks/security_history_test.go b/tests/checks/security_history_test.go index a65e073..b10aa2b 100644 --- a/tests/checks/security_history_test.go +++ b/tests/checks/security_history_test.go @@ -57,6 +57,21 @@ func TestScanGitHistoryFindsRemovedSecret(t *testing.T) { } } +func TestScanGitHistoryRejectsInvalidSecretPatterns(t *testing.T) { + // ScanGitHistory does not run config validation, so an unusable pattern must + // surface as an error rather than silently scanning with less coverage. + cfg := codeguard.ExampleConfig() + cfg.Checks.SecurityRules.Secrets = &codeguard.SecretsRulesConfig{ + Enabled: boolPtr(true), + AllowPatterns: []string{`(`}, + } + + _, err := codeguard.ScanGitHistory(context.Background(), cfg, codeguard.HistoryScanOptions{RepoPath: t.TempDir()}) + if err == nil { + t.Fatal("expected error for invalid allow_patterns regex") + } +} + func TestScanGitHistoryRespectsAllowPaths(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") diff --git a/tests/checks/security_secrets_scanner_test.go b/tests/checks/security_secrets_scanner_test.go index 7faf52a..cfe2534 100644 --- a/tests/checks/security_secrets_scanner_test.go +++ b/tests/checks/security_secrets_scanner_test.go @@ -1,6 +1,7 @@ package checks_test import ( + "strings" "testing" "github.com/devr-tools/codeguard/internal/codeguard/checks/security" @@ -41,7 +42,7 @@ func gateSamples() []string { } func TestSecretScannerGateCoversBuiltins(t *testing.T) { - scanner := security.BuildScanner(nil) + scanner, _ := security.BuildScanner(nil) for _, sample := range gateSamples() { if len(scanner.ScanContent(sample)) == 0 { t.Errorf("scanner missed a built-in sample (gate marker likely missing): %q", sample) @@ -49,6 +50,49 @@ func TestSecretScannerGateCoversBuiltins(t *testing.T) { } } +func TestBuildScannerReportsUnusablePatterns(t *testing.T) { + scanner, issues := security.BuildScanner(&core.SecretsRulesConfig{ + AllowPatterns: []string{`//\s*ok`, `(`}, + CustomPatterns: []core.CustomSecretPattern{ + {ID: "good", Regex: `\bacme_live_[0-9a-f]{16}\b`}, + {ID: "bad-regex", Regex: `[`}, + {ID: " ", Regex: `\btoken\b`}, + }, + }) + if len(issues) != 3 { + t.Fatalf("issues = %v, want 3 (bad allow pattern, bad custom regex, empty custom id)", issues) + } + for _, want := range []string{"allow_patterns[1]", `custom_patterns["bad-regex"]`, "empty id"} { + found := false + for _, issue := range issues { + if strings.Contains(issue, want) { + found = true + } + } + if !found { + t.Errorf("no issue mentions %q: %v", want, issues) + } + } + + // The valid entries still participate in the scan. + if matches := scanner.ScanContent(`key = "acme_live_0123456789abcdef"`); len(matches) != 1 || matches[0].RuleID != "good" { + t.Fatalf("valid custom pattern did not survive invalid siblings: %+v", matches) + } + if matches := scanner.ScanContent(`key = "` + cred("AKIA", "1234567890ABCDEF") + `" // ok`); len(matches) != 0 { + t.Fatalf("valid allow pattern did not survive invalid sibling: %+v", matches) + } +} + +func TestBuildScannerNoIssuesForValidConfig(t *testing.T) { + _, issues := security.BuildScanner(&core.SecretsRulesConfig{ + AllowPatterns: []string{`//\s*ok`}, + CustomPatterns: []core.CustomSecretPattern{{ID: "good", Regex: `token-[0-9]+`}}, + }) + if len(issues) != 0 { + t.Fatalf("issues = %v, want none for a valid config", issues) + } +} + func benchSource() string { const block = "func handler(ctx context.Context, req *Request) (*Response, error) {\n" + "\treturn &Response{Status: 200, Body: req.Payload}, nil\n" @@ -61,7 +105,7 @@ func benchSource() string { } func BenchmarkSecretScanContent(b *testing.B) { - scanner := security.BuildScanner(nil) + scanner, _ := security.BuildScanner(nil) source := benchSource() b.SetBytes(int64(len(source))) b.ReportAllocs() @@ -73,7 +117,7 @@ func BenchmarkSecretScanContent(b *testing.B) { func BenchmarkSecretScanContentEntropy(b *testing.B) { enabled := true - scanner := security.BuildScanner(&core.SecretsRulesConfig{Entropy: &core.SecretsEntropyConfig{Enabled: &enabled}}) + scanner, _ := security.BuildScanner(&core.SecretsRulesConfig{Entropy: &core.SecretsEntropyConfig{Enabled: &enabled}}) source := benchSource() b.SetBytes(int64(len(source))) b.ReportAllocs() diff --git a/tests/codeguard/ai_httpretry_test.go b/tests/codeguard/ai_httpretry_test.go new file mode 100644 index 0000000..34d636c --- /dev/null +++ b/tests/codeguard/ai_httpretry_test.go @@ -0,0 +1,346 @@ +package codeguard_test + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/devr-tools/codeguard/internal/codeguard/ai/httpretry" +) + +func fastRetryConfig(maxRetries int) httpretry.Config { + return httpretry.Config{ + MaxRetries: maxRetries, + BaseDelay: time.Millisecond, + MaxDelay: 5 * time.Millisecond, + } +} + +func buildGet(t *testing.T, url string, builds *atomic.Int64) func() (*http.Request, error) { + t.Helper() + return func() (*http.Request, error) { + if builds != nil { + builds.Add(1) + } + return http.NewRequest(http.MethodGet, url, nil) + } +} + +func TestFromEnvDefaults(t *testing.T) { + t.Setenv("CODEGUARD_AI_MAX_RETRIES", "") + t.Setenv("CODEGUARD_AI_RETRY_BASE_DELAY", "") + + cfg := httpretry.FromEnv() + if cfg.MaxRetries != 3 { + t.Fatalf("MaxRetries = %d, want default 3", cfg.MaxRetries) + } + if cfg.BaseDelay != 250*time.Millisecond { + t.Fatalf("BaseDelay = %v, want default 250ms", cfg.BaseDelay) + } + if cfg.MaxDelay != 8*time.Second { + t.Fatalf("MaxDelay = %v, want default 8s", cfg.MaxDelay) + } +} + +func TestFromEnvOverrides(t *testing.T) { + t.Setenv("CODEGUARD_AI_MAX_RETRIES", " 7 ") + t.Setenv("CODEGUARD_AI_RETRY_BASE_DELAY", "50ms") + + cfg := httpretry.FromEnv() + if cfg.MaxRetries != 7 { + t.Fatalf("MaxRetries = %d, want 7 from env", cfg.MaxRetries) + } + if cfg.BaseDelay != 50*time.Millisecond { + t.Fatalf("BaseDelay = %v, want 50ms from env", cfg.BaseDelay) + } +} + +func TestFromEnvIgnoresInvalidValues(t *testing.T) { + cases := []struct { + name string + maxRetries string + baseDelay string + }{ + {name: "garbage", maxRetries: "not-a-number", baseDelay: "not-a-duration"}, + {name: "negative retries", maxRetries: "-2", baseDelay: "-10ms"}, + {name: "zero delay", maxRetries: "3", baseDelay: "0s"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("CODEGUARD_AI_MAX_RETRIES", tc.maxRetries) + t.Setenv("CODEGUARD_AI_RETRY_BASE_DELAY", tc.baseDelay) + + cfg := httpretry.FromEnv() + if cfg.BaseDelay != 250*time.Millisecond { + t.Fatalf("BaseDelay = %v, want default kept for invalid env", cfg.BaseDelay) + } + if tc.maxRetries == "3" { + if cfg.MaxRetries != 3 { + t.Fatalf("MaxRetries = %d, want 3", cfg.MaxRetries) + } + } else if cfg.MaxRetries != 3 { + t.Fatalf("MaxRetries = %d, want default kept for invalid env", cfg.MaxRetries) + } + }) + } +} + +func TestDoReturnsFirstSuccessWithoutRetry(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(3), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + if got := calls.Load(); got != 1 { + t.Fatalf("expected exactly 1 request, got %d", got) + } +} + +func TestDoDoesNotRetryNonRetryableStatus(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusBadRequest) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(3), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status = %d, want 400 surfaced unchanged", resp.StatusCode) + } + if got := calls.Load(); got != 1 { + t.Fatalf("expected no retries for 400, got %d requests", got) + } +} + +func TestDoRetriesRetryableStatusesUntilSuccess(t *testing.T) { + for _, status := range []int{http.StatusTooManyRequests, http.StatusInternalServerError, http.StatusServiceUnavailable} { + t.Run(http.StatusText(status), func(t *testing.T) { + var calls, builds atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if calls.Add(1) <= 2 { + w.WriteHeader(status) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(3), buildGet(t, server.URL, &builds)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want eventual 200", resp.StatusCode) + } + if got := calls.Load(); got != 3 { + t.Fatalf("expected 3 requests (2 failures then success), got %d", got) + } + if got := builds.Load(); got != 3 { + t.Fatalf("expected a fresh request per attempt (3 builds), got %d", got) + } + }) + } +} + +func TestDoSurfacesFinalResponseWhenRetriesExhausted(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusTooManyRequests) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(2), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusTooManyRequests { + t.Fatalf("status = %d, want final 429 returned unchanged", resp.StatusCode) + } + if got := calls.Load(); got != 3 { + t.Fatalf("expected initial attempt plus 2 retries, got %d requests", got) + } +} + +func TestDoWithZeroRetriesMakesSingleAttempt(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(0), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if got := calls.Load(); got != 1 { + t.Fatalf("expected a single attempt with MaxRetries=0, got %d", got) + } +} + +func TestDoRetriesNetworkErrorAndReturnsItWhenExhausted(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {})) + client := server.Client() + server.Close() // every attempt now fails with a connection error + + var builds atomic.Int64 + resp, err := httpretry.Do(context.Background(), client, fastRetryConfig(2), buildGet(t, server.URL, &builds)) + if err == nil { + resp.Body.Close() + t.Fatal("expected network error after retries are exhausted") + } + if resp != nil { + t.Fatalf("resp = %v, want nil alongside network error", resp) + } + if got := builds.Load(); got != 3 { + t.Fatalf("expected 3 attempts against a dead server, got %d builds", got) + } +} + +func TestDoHonorsRetryAfterSeconds(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if calls.Add(1) == 1 { + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + // BaseDelay is tiny, so a gap of ~1s proves Retry-After won over backoff. + start := time.Now() + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(1), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if elapsed := time.Since(start); elapsed < 900*time.Millisecond { + t.Fatalf("elapsed = %v, want >=~1s honoring Retry-After", elapsed) + } + if got := calls.Load(); got != 2 { + t.Fatalf("expected 2 requests, got %d", got) + } +} + +func TestDoIgnoresMalformedRetryAfter(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if calls.Add(1) == 1 { + w.Header().Set("Retry-After", "soon-ish") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + start := time.Now() + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(1), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("elapsed = %v, want fast fallback to backoff for malformed Retry-After", elapsed) + } + if got := calls.Load(); got != 2 { + t.Fatalf("expected 2 requests, got %d", got) + } +} + +func TestDoStopsWaitingWhenContextCanceled(t *testing.T) { + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + cfg := httpretry.Config{ + MaxRetries: 5, + BaseDelay: 10 * time.Second, + MaxDelay: 10 * time.Second, + } + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + start := time.Now() + resp, err := httpretry.Do(ctx, server.Client(), cfg, buildGet(t, server.URL, nil)) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("err = %v, want context.DeadlineExceeded from backoff wait", err) + } + if resp != nil { + resp.Body.Close() + t.Fatal("expected nil response when canceled during backoff") + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("elapsed = %v, want prompt return on cancellation", elapsed) + } + if got := calls.Load(); got != 1 { + t.Fatalf("expected 1 request before cancellation, got %d", got) + } +} + +func TestDoReturnsBuildErrorImmediately(t *testing.T) { + buildErr := errors.New("bad request template") + var calls atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls.Add(1) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), server.Client(), fastRetryConfig(3), func() (*http.Request, error) { + return nil, buildErr + }) + if !errors.Is(err, buildErr) { + t.Fatalf("err = %v, want build error surfaced", err) + } + if resp != nil { + resp.Body.Close() + t.Fatal("expected nil response for build error") + } + if got := calls.Load(); got != 0 { + t.Fatalf("expected no requests when build fails, got %d", got) + } +} + +func TestDoWithNilClientUsesDefault(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + resp, err := httpretry.Do(context.Background(), nil, fastRetryConfig(0), buildGet(t, server.URL, nil)) + if err != nil { + t.Fatalf("Do with nil client: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } +}