From f4fa62d2d6488ede648e58ec1887b6862e4015e5 Mon Sep 17 00:00:00 2001 From: GitHub Ace Date: Tue, 4 Aug 2026 18:44:47 +0000 Subject: [PATCH 1/2] feat(conclude): emit verbose diagnostics at parity with gh-aw inline step `threat-detect conclude` previously emitted at most a single line, making detection triage on the external path require downloading artifacts. It now mirrors gh-aw's parse_threat_detection_results.cjs job-log output: - Opening/closing banners framing the conclusion section - Echo of RUN_DETECTION, continue-on-error, execution outcome, and the resolved result-file / detection-log paths - Per-field verdict breakdown on both the safe and threat paths, with an indexed reasons list (or "(none)") - Recursive listing of the result directory plus detection-log stats and echoed THREAT_DETECTION_* marker lines when the result is missing or unusable (bounded so job logs stay readable) - New --detection-log and --log-file flags; the conclusion, verdict, and diagnostics are mirrored into the JSONL run log The detection log remains diagnostic-only and is never parsed for a verdict. Spec (TD-20c, U-18a) and README updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> --- README.md | 17 ++ cmd/threat-detect/conclude.go | 336 ++++++++++++++++++++++++++++- cmd/threat-detect/conclude_test.go | 247 +++++++++++++++++++++ specs/threat-detection-spec.md | 15 ++ specs/usage-spec.md | 11 +- 5 files changed, 614 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 970dfd2..511c430 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,23 @@ at: "), a malformed file reports `parse_error`, and detected threats repor `threat_detected`. There is no log-scraping fallback: if the file is absent, the step fails loudly. +`conclude` writes a verbose, self-contained diagnostic section to the job log: +banners framing the section, the environment inputs and resolved paths, and the +per-field verdict breakdown (`prompt_injection`/`secret_leak`/`malicious_patch`) +with an indexed reasons list. When the result file is missing or unusable it also +prints a recursive listing of the result directory plus detection-log statistics +and every line carrying a `THREAT_DETECTION_STATUS:`/`THREAT_DETECTION_RESULT:` +marker, so a failed run can be diagnosed without downloading artifacts. + +Additional flags: + +- `--detection-log ` — detection run log used for diagnostics (defaults to + `detection.log` beside the result file); it is never parsed for a verdict +- `--log-file ` — mirror the conclusion into a JSONL run log (env: + `THREAT_DETECTION_LOG_FILE`), emitting `conclude_start`, `conclude_verdict`, + `conclude_directory_listing`, `conclude_detection_log`, and `conclude_outcome` + events + ### AI Credits and Token Usage The threat-detection pass is a **separate agentic engine invocation** from the main diff --git a/cmd/threat-detect/conclude.go b/cmd/threat-detect/conclude.go index e2b5f82..3eabe5f 100644 --- a/cmd/threat-detect/conclude.go +++ b/cmd/threat-detect/conclude.go @@ -7,9 +7,12 @@ import ( "io" "io/fs" "os" + "path/filepath" + "sort" "strings" "github.com/github/gh-aw-threat-detection/pkg/detector" + "github.com/github/gh-aw-threat-detection/pkg/runlog" ) // Exit codes for the conclude subcommand. These map directly onto whether the @@ -33,6 +36,29 @@ const ( // the detector via --output and surfaced through the AWF read-write mount. const defaultConcludeResultFile = "/tmp/gh-aw/threat-detection/detection_result.json" +// defaultDetectionLogName is the conventional name of the detection run's +// captured stdout/stderr log, which sits alongside the result file. The +// conclude subcommand never parses it for a verdict — it is read only for +// diagnostics when the structured result is missing or unusable. +const defaultDetectionLogName = "detection.log" + +// Diagnostic bounds. Job logs are a shared resource, so the directory listing +// and echoed log lines are capped rather than dumped in full. +const ( + maxListedFiles = 200 + maxEchoedLogLines = 50 + maxEchoedLineRunes = 500 + diagnosticLogMaxSize = 8 << 20 // 8 MiB: read no more of the detection log +) + +// bannerRule is the horizontal rule framing the conclude banners. It mirrors +// gh-aw's inline conclusion step so both paths read identically in job logs. +const bannerRule = "════════════════════════════════════════════════════════" + +// Prefixes echoed from the detection log for focused diagnosis. These are the +// two machine-readable markers the detector emits. +var detectionLogMarkers = []string{"THREAT_DETECTION_STATUS:", "THREAT_DETECTION_RESULT:"} + // runConclude implements the "conclude" subcommand. It reads the structured // detection verdict produced by the detection run, evaluates it against the // gh-aw job-output contract (conclusion/success/reason plus the exported @@ -41,8 +67,14 @@ const defaultConcludeResultFile = "/tmp/gh-aw/threat-detection/detection_result. func runConclude(args []string) int { fs := flag.NewFlagSet("conclude", flag.ContinueOnError) fs.SetOutput(os.Stderr) - var resultFile string + var ( + resultFile string + detectionLog string + logFile string + ) fs.StringVar(&resultFile, "result-file", defaultConcludeResultFile, "Path to the structured detection_result.json verdict file") + fs.StringVar(&detectionLog, "detection-log", "", "Path to the detection run log used for diagnostics (defaults to detection.log beside the result file)") + fs.StringVar(&logFile, "log-file", os.Getenv("THREAT_DETECTION_LOG_FILE"), "Path to write JSONL run logs (env: THREAT_DETECTION_LOG_FILE)") if err := fs.Parse(args); err != nil { if errors.Is(err, flag.ErrHelp) { return concludeExitProceed @@ -50,13 +82,33 @@ func runConclude(args []string) int { return concludeExitFail } + // The JSONL log is an additive observability sink: failing to open it must + // not change the conclusion, so it degrades to a warning on stderr. + var logger *runlog.Logger + if logFile != "" { + opened, err := runlog.Open(logFile) + if err != nil { + fmt.Fprintf(os.Stderr, "conclude: failed to open log file: %v\n", err) + } else { + logger = opened + defer func() { + if err := logger.Close(); err != nil { + fmt.Fprintf(os.Stderr, "conclude: failed to close log file: %v\n", err) + } + }() + } + } + c := &concluder{ - runDetection: os.Getenv("RUN_DETECTION"), - warnMode: os.Getenv("GH_AW_DETECTION_CONTINUE_ON_ERROR") != "false", - executionFailed: os.Getenv("DETECTION_AGENTIC_EXECUTION_OUTCOME") == "failure", - githubOutput: os.Getenv("GITHUB_OUTPUT"), - githubEnv: os.Getenv("GITHUB_ENV"), - stdout: os.Stdout, + runDetection: os.Getenv("RUN_DETECTION"), + warnMode: os.Getenv("GH_AW_DETECTION_CONTINUE_ON_ERROR") != "false", + executionFailed: os.Getenv("DETECTION_AGENTIC_EXECUTION_OUTCOME") == "failure", + executionOutcome: os.Getenv("DETECTION_AGENTIC_EXECUTION_OUTCOME"), + githubOutput: os.Getenv("GITHUB_OUTPUT"), + githubEnv: os.Getenv("GITHUB_ENV"), + detectionLog: detectionLog, + logger: logger, + stdout: os.Stdout, } return c.run(resultFile) } @@ -65,35 +117,83 @@ func runConclude(args []string) int { // constructed from the environment by runConclude and exercised directly by // tests with temp files. type concluder struct { - runDetection string // RUN_DETECTION; "true" means a verdict is expected - warnMode bool // GH_AW_DETECTION_CONTINUE_ON_ERROR != "false" - executionFailed bool // DETECTION_AGENTIC_EXECUTION_OUTCOME == "failure" + runDetection string // RUN_DETECTION; "true" means a verdict is expected + warnMode bool // GH_AW_DETECTION_CONTINUE_ON_ERROR != "false" + executionFailed bool // DETECTION_AGENTIC_EXECUTION_OUTCOME == "failure" + executionOutcome string // raw DETECTION_AGENTIC_EXECUTION_OUTCOME, echoed for diagnosis githubOutput string // path of $GITHUB_OUTPUT (may be empty) githubEnv string // path of $GITHUB_ENV (may be empty) + detectionLog string // detection run log path; empty means derive from the result file + logger *runlog.Logger stdout io.Writer } +// run emits the conclusion banner, evaluates the verdict, and always closes +// with the completion banner so the section is unambiguously delimited in the +// job log regardless of which path was taken. func (c *concluder) run(resultFile string) int { + code := c.conclude(resultFile) + c.banner("🛡️ Threat detection conclusion complete.") + return code +} + +func (c *concluder) conclude(resultFile string) int { + detectionLog := c.detectionLogPath(resultFile) + detectionDir := filepath.Dir(resultFile) + + c.banner("🛡️ Threat Detection: Parse Results & Conclude") + c.info(fmt.Sprintf("📋 RUN_DETECTION env: %q", c.runDetection)) + c.info(fmt.Sprintf("📋 continue-on-error: %t", c.warnMode)) + c.info(fmt.Sprintf("📋 detection execution outcome: %q", c.executionOutcome)) + c.info(fmt.Sprintf("📁 Threat detection directory: %s", detectionDir)) + c.info(fmt.Sprintf("📄 Detection log path: %s", detectionLog)) + c.info(fmt.Sprintf("📄 Structured result path: %s", resultFile)) + c.logger.Info("conclude_start", map[string]any{ + "run_detection": c.runDetection, + "continue_on_error": c.warnMode, + "execution_outcome": c.executionOutcome, + "detection_dir": detectionDir, + "detection_log": detectionLog, + "result_file": resultFile, + }) + // Step 1 — detection not required: skip without reading any verdict. if c.runDetection != "true" { c.info("⏭️ Detection not required (RUN_DETECTION != \"true\"); conclusion=skipped.") + c.info(" Reason: no agent output types or patch files were produced.") + c.info(" Setting conclusion=skipped, success=true.") c.setOutput("conclusion", "skipped") c.exportVariable("GH_AW_DETECTION_CONCLUSION", "skipped") c.setOutput("success", "true") c.setOutput("reason", "") c.exportVariable("GH_AW_DETECTION_REASON", "") + c.info("✅ Detection skipped — no threats to evaluate.") + c.logger.Info("conclude_outcome", map[string]any{ + "conclusion": "skipped", + "reason": "", + "success": true, + "exit": concludeExitProceed, + }) return concludeExitProceed } + c.info("🔍 Detection is required. Proceeding to parse detection result...") + // Step 2 — locate and read the structured verdict file. A missing or // otherwise unreadable file (not-exist, permission/IO error, or a TOCTOU // where the file is removed mid-read) means the detection run never produced // a readable verdict, which is a system-side agent failure (ERR_SYSTEM). Only // a file that is readable but whose contents are empty or malformed is a // parse error (ERR_PARSE). + c.info(fmt.Sprintf("🔎 Checking for structured result file: %s", resultFile)) result, err := detector.ReadResultFile(resultFile) if err != nil { + // The structured result is the only verdict source, so dump everything + // that helps explain its absence before failing. + c.listDirectory(detectionDir) + c.reportDetectionLog(detectionLog) + if errors.Is(err, fs.ErrNotExist) { return c.fail("agent_failure", fmt.Sprintf("%s: ❌ Detection result file not found at: %s", errCodeSystem, resultFile)) } @@ -101,10 +201,15 @@ func (c *concluder) run(resultFile string) int { if errors.As(err, &pathErr) { return c.fail("agent_failure", fmt.Sprintf("%s: ❌ Detection result file unreadable at %s: %v", errCodeSystem, resultFile, err)) } + c.info("💡 This usually means the AI engine did not record a verdict in the expected format.") + c.info(` Expected content: {"prompt_injection":bool,"secret_leak":bool,"malicious_patch":bool,"reasons":[...]}`) return c.fail("parse_error", fmt.Sprintf("%s: ❌ Failed to parse detection result file %s: %v", errCodeParse, resultFile, err)) } - // Step 3 — evaluate the verdict. + c.info("✔️ Structured result file found and parsed successfully.") + + // Step 3 — report and evaluate the verdict. + c.reportVerdict(result) if result.HasThreats() { threats := make([]string, 0, 3) if result.PromptInjection { @@ -129,9 +234,196 @@ func (c *concluder) run(resultFile string) int { c.setOutput("success", "true") c.setOutput("reason", "") c.exportVariable("GH_AW_DETECTION_REASON", "") + c.logger.Info("conclude_outcome", map[string]any{ + "conclusion": "success", + "reason": "", + "success": true, + "exit": concludeExitProceed, + }) return concludeExitProceed } +// detectionLogPath resolves the detection log location, defaulting to +// detection.log alongside the structured result file. +func (c *concluder) detectionLogPath(resultFile string) string { + if c.detectionLog != "" { + return c.detectionLog + } + return filepath.Join(filepath.Dir(resultFile), defaultDetectionLogName) +} + +// reportVerdict prints the per-field verdict breakdown and the indexed reasons +// list. It runs on both the safe and threat paths so the job log always shows +// exactly what the detector concluded. +func (c *concluder) reportVerdict(result *detector.Result) { + c.info("📋 Threat detection verdict (from structured result file):") + c.info(fmt.Sprintf(" prompt_injection : %t", result.PromptInjection)) + c.info(fmt.Sprintf(" secret_leak : %t", result.SecretLeak)) + c.info(fmt.Sprintf(" malicious_patch : %t", result.MaliciousPatch)) + if len(result.Reasons) > 0 { + c.info(fmt.Sprintf(" reasons (%d):", len(result.Reasons))) + for i, reason := range result.Reasons { + c.info(fmt.Sprintf(" [%d] %s", i+1, reason)) + } + } else { + c.info(" reasons : (none)") + } + c.logger.Info("conclude_verdict", map[string]any{ + "prompt_injection": result.PromptInjection, + "secret_leak": result.SecretLeak, + "malicious_patch": result.MaliciousPatch, + "reasons": result.Reasons, + "has_threats": result.HasThreats(), + "source": "structured_result_file", + }) +} + +// listDirectory prints a recursive listing of dir so a missing or unusable +// result file can be diagnosed from the job log alone. +func (c *concluder) listDirectory(dir string) { + c.info(fmt.Sprintf("📁 Listing all files in artifact directory for diagnosis: %s", dir)) + files, err := listFilesRecursively(dir) + if err != nil { + c.info(fmt.Sprintf(" ⚠️ Could not list files in %s: %v", dir, err)) + c.logger.Info("conclude_directory_listing", map[string]any{"dir": dir, "error": err.Error()}) + return + } + if len(files) == 0 { + c.info(fmt.Sprintf(" ⚠️ No files found in %s", dir)) + c.logger.Info("conclude_directory_listing", map[string]any{"dir": dir, "count": 0, "files": []string{}}) + return + } + shown := files + if len(shown) > maxListedFiles { + shown = shown[:maxListedFiles] + } + c.info(fmt.Sprintf(" Found %d file(s):", len(files))) + for _, file := range shown { + c.info(" - " + file) + } + if len(shown) < len(files) { + c.info(fmt.Sprintf(" ... %d more file(s) omitted", len(files)-len(shown))) + } + c.logger.Info("conclude_directory_listing", map[string]any{ + "dir": dir, + "count": len(files), + "files": shown, + }) +} + +// reportDetectionLog prints size statistics for the detection log plus every +// line carrying a THREAT_DETECTION_* marker. The log is never used to derive a +// verdict — this is diagnostics only. +func (c *concluder) reportDetectionLog(path string) { + data, err := readBounded(path, diagnosticLogMaxSize) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + c.info(fmt.Sprintf("📄 No detection log found at: %s", path)) + } else { + c.info(fmt.Sprintf(" ⚠️ Could not read detection log %s: %v", path, err)) + } + c.logger.Info("conclude_detection_log", map[string]any{"path": path, "error": err.Error()}) + return + } + + lines := strings.Split(string(data), "\n") + c.info(fmt.Sprintf("📊 Detection log stats: %d lines, %d bytes", len(lines), len(data))) + + matches := make([]string, 0, 8) + for i, line := range lines { + if !containsMarker(line) { + continue + } + matches = append(matches, fmt.Sprintf("[%d] %s", i+1, truncateRunes(strings.TrimRight(line, "\r"), maxEchoedLineRunes))) + } + if len(matches) == 0 { + c.info(fmt.Sprintf("📄 No lines containing THREAT_DETECTION markers found in %d lines", len(lines))) + } else { + shown := matches + if len(shown) > maxEchoedLogLines { + shown = shown[:maxEchoedLogLines] + } + c.info(fmt.Sprintf("📄 Lines containing THREAT_DETECTION markers (%d of %d):", len(matches), len(lines))) + for _, m := range shown { + c.info(" " + m) + } + if len(shown) < len(matches) { + c.info(fmt.Sprintf(" ... %d more matching line(s) omitted", len(matches)-len(shown))) + } + matches = shown + } + c.logger.Info("conclude_detection_log", map[string]any{ + "path": path, + "lines": len(lines), + "bytes": len(data), + "marker_lines": matches, + "marker_count": len(matches), + }) +} + +func containsMarker(line string) bool { + for _, marker := range detectionLogMarkers { + if strings.Contains(line, marker) { + return true + } + } + return false +} + +// listFilesRecursively returns the paths of all regular files under dir, +// relative to dir and sorted for deterministic output. Unreadable +// subdirectories are skipped rather than aborting the whole listing. +func listFilesRecursively(dir string) ([]string, error) { + if _, err := os.Stat(dir); err != nil { + return nil, err + } + var files []string + err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + if d != nil && d.IsDir() { + return filepath.SkipDir + } + return nil + } + if d.IsDir() { + return nil + } + rel, relErr := filepath.Rel(dir, path) + if relErr != nil { + rel = path + } + files = append(files, rel) + return nil + }) + if err != nil { + return nil, err + } + sort.Strings(files) + return files, nil +} + +// readBounded reads at most limit bytes from path. Detection logs can be very +// large; the diagnostics only need the beginning to be useful. +func readBounded(path string, limit int64) ([]byte, error) { + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() + return io.ReadAll(io.LimitReader(f, limit)) +} + +// truncateRunes shortens s to at most max runes, appending an ellipsis marker +// when content was dropped, so a single pathological log line cannot flood the +// job log. +func truncateRunes(s string, max int) string { + runes := []rune(s) + if len(runes) <= max { + return s + } + return string(runes[:max]) + "… (truncated)" +} + // fail records a failure verdict and decides whether to fail closed. It mirrors // gh-aw's setDetectionFailure exactly: // @@ -149,12 +441,26 @@ func (c *concluder) fail(reason, message string) int { c.setOutput("conclusion", "warning") c.exportVariable("GH_AW_DETECTION_CONCLUSION", "warning") c.setOutput("success", "false") + c.logger.Error("conclude_outcome", map[string]any{ + "conclusion": "warning", + "reason": reason, + "success": false, + "exit": concludeExitProceed, + "message": message, + }) return concludeExitProceed } c.command("error", message) c.setOutput("conclusion", "failure") c.exportVariable("GH_AW_DETECTION_CONCLUSION", "failure") c.setOutput("success", "false") + c.logger.Error("conclude_outcome", map[string]any{ + "conclusion": "failure", + "reason": reason, + "success": false, + "exit": concludeExitFail, + "message": message, + }) return concludeExitFail } @@ -188,6 +494,14 @@ func (c *concluder) info(message string) { fmt.Fprintln(c.stdout, message) } +// banner prints a title framed by horizontal rules, delimiting the conclusion +// section in the job log. +func (c *concluder) banner(title string) { + c.info(bannerRule) + c.info(title) + c.info(bannerRule) +} + // command emits a GitHub Actions workflow command (::error:: / ::warning::) // with the message data properly escaped. func (c *concluder) command(kind, message string) { diff --git a/cmd/threat-detect/conclude_test.go b/cmd/threat-detect/conclude_test.go index 8287041..f61eca5 100644 --- a/cmd/threat-detect/conclude_test.go +++ b/cmd/threat-detect/conclude_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "fmt" "os" "path/filepath" "strings" @@ -288,3 +289,249 @@ func TestRunConcludeReadsEnv(t *testing.T) { t.Fatalf("conclusion = %q, want success", outputs["conclusion"]) } } + +// TestConcludeDiagnosticOutput verifies the verbose job-log rendering: the +// banners, the echoed environment inputs, and the per-field verdict breakdown +// with an indexed reasons list. This is the parity contract with gh-aw's inline +// conclusion step — debugging must be possible from the job log alone. +func TestConcludeDiagnosticOutput(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + if err := os.WriteFile(resultFile, []byte(threatVerdict), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + warnMode: true, + executionOutcome: "success", + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + if code := c.run(resultFile); code != concludeExitProceed { + t.Fatalf("exit code = %d, want %d", code, concludeExitProceed) + } + + got := stdout.String() + for _, want := range []string{ + bannerRule, + "🛡️ Threat Detection: Parse Results & Conclude", + `📋 RUN_DETECTION env: "true"`, + "📋 continue-on-error: true", + `📋 detection execution outcome: "success"`, + "📁 Threat detection directory: " + dir, + "📄 Detection log path: " + filepath.Join(dir, defaultDetectionLogName), + "📄 Structured result path: " + resultFile, + "🔎 Checking for structured result file: " + resultFile, + "✔️ Structured result file found and parsed successfully.", + "📋 Threat detection verdict (from structured result file):", + " prompt_injection : true", + " secret_leak : false", + " malicious_patch : false", + " reasons (1):", + " [1] jailbreak attempt", + "🛡️ Threat detection conclusion complete.", + } { + if !strings.Contains(got, want) { + t.Errorf("stdout missing %q\n--- got ---\n%s", want, got) + } + } +} + +// TestConcludeSafeVerdictBreakdown verifies the verdict breakdown is emitted on +// the success path too, with the "(none)" reasons rendering. +func TestConcludeSafeVerdictBreakdown(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + if err := os.WriteFile(resultFile, []byte(safeVerdict), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + if code := c.run(resultFile); code != concludeExitProceed { + t.Fatalf("exit code = %d, want %d", code, concludeExitProceed) + } + got := stdout.String() + for _, want := range []string{ + " prompt_injection : false", + " reasons : (none)", + "✅ No security threats detected. Safe outputs may proceed.", + "🛡️ Threat detection conclusion complete.", + } { + if !strings.Contains(got, want) { + t.Errorf("stdout missing %q\n--- got ---\n%s", want, got) + } + } +} + +// TestConcludeSkippedOutput verifies the skip path is framed by both banners and +// explains itself, rather than emitting a single bare line. +func TestConcludeSkippedOutput(t *testing.T) { + dir := t.TempDir() + var stdout bytes.Buffer + c := &concluder{ + runDetection: "", + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + if code := c.run(filepath.Join(dir, "detection_result.json")); code != concludeExitProceed { + t.Fatalf("exit code = %d, want %d", code, concludeExitProceed) + } + got := stdout.String() + for _, want := range []string{ + "🛡️ Threat Detection: Parse Results & Conclude", + "✅ Detection skipped — no threats to evaluate.", + "🛡️ Threat detection conclusion complete.", + } { + if !strings.Contains(got, want) { + t.Errorf("stdout missing %q\n--- got ---\n%s", want, got) + } + } + // The skip path must not read or report on any verdict. + if strings.Contains(got, "Threat detection verdict") { + t.Errorf("skip path must not report a verdict, got:\n%s", got) + } +} + +// TestConcludeMissingResultDiagnostics verifies that a missing result file +// triggers a recursive directory listing plus detection-log stats and an echo of +// the THREAT_DETECTION marker lines. +func TestConcludeMissingResultDiagnostics(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "nested"), 0o755); err != nil { + t.Fatalf("MkdirAll error = %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "nested", "stray.txt"), []byte("x"), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + logPath := filepath.Join(dir, defaultDetectionLogName) + logBody := "starting\nTHREAT_DETECTION_STATUS: reason=engine_error exit=2\ndone\n" + if err := os.WriteFile(logPath, []byte(logBody), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + warnMode: true, + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + code := c.run(filepath.Join(dir, "detection_result.json")) + if code != concludeExitProceed { + t.Fatalf("exit code = %d, want %d", code, concludeExitProceed) + } + + got := stdout.String() + for _, want := range []string{ + "📁 Listing all files in artifact directory for diagnosis: " + dir, + " - " + filepath.Join("nested", "stray.txt"), + " - " + defaultDetectionLogName, + fmt.Sprintf("📊 Detection log stats: 4 lines, %d bytes", len(logBody)), + "📄 Lines containing THREAT_DETECTION markers (1 of 4):", + " [2] THREAT_DETECTION_STATUS: reason=engine_error exit=2", + } { + if !strings.Contains(got, want) { + t.Errorf("stdout missing %q\n--- got ---\n%s", want, got) + } + } +} + +// TestConcludeMissingDetectionLogIsReported verifies the diagnostics degrade +// gracefully when the detection log itself is absent. +func TestConcludeMissingDetectionLogIsReported(t *testing.T) { + dir := t.TempDir() + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + c.run(filepath.Join(dir, "detection_result.json")) + if want := "📄 No detection log found at: " + filepath.Join(dir, defaultDetectionLogName); !strings.Contains(stdout.String(), want) { + t.Errorf("stdout missing %q\n--- got ---\n%s", want, stdout.String()) + } +} + +// TestConcludeDetectionLogOverride verifies --detection-log points the +// diagnostics at an explicit path instead of the default sibling file. +func TestConcludeDetectionLogOverride(t *testing.T) { + dir := t.TempDir() + custom := filepath.Join(dir, "custom.log") + if err := os.WriteFile(custom, []byte("THREAT_DETECTION_RESULT: {}\n"), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + detectionLog: custom, + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + c.run(filepath.Join(dir, "detection_result.json")) + got := stdout.String() + if !strings.Contains(got, "📄 Detection log path: "+custom) { + t.Errorf("stdout missing custom detection log path\n--- got ---\n%s", got) + } + if !strings.Contains(got, "[1] THREAT_DETECTION_RESULT: {}") { + t.Errorf("stdout missing echoed marker line\n--- got ---\n%s", got) + } +} + +// TestConcludeRunLog verifies the conclusion is mirrored into the JSONL run log +// when --log-file is set. +func TestConcludeRunLog(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + if err := os.WriteFile(resultFile, []byte(threatVerdict), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + logPath := filepath.Join(dir, "run.jsonl") + + t.Setenv("RUN_DETECTION", "true") + t.Setenv("GH_AW_DETECTION_CONTINUE_ON_ERROR", "false") + t.Setenv("DETECTION_AGENTIC_EXECUTION_OUTCOME", "success") + t.Setenv("GITHUB_OUTPUT", filepath.Join(dir, "out")) + t.Setenv("GITHUB_ENV", filepath.Join(dir, "env")) + + if code := runConclude([]string{"--result-file", resultFile, "--log-file", logPath}); code != concludeExitFail { + t.Fatalf("runConclude() = %d, want %d", code, concludeExitFail) + } + + records := readJSONLRecords(t, logPath) + start := findRecord(records, "conclude_start") + if start == nil { + t.Fatal("missing conclude_start record") + } + if start["run_detection"] != "true" || start["execution_outcome"] != "success" || start["result_file"] != resultFile { + t.Errorf("conclude_start fields = %v", start) + } + + verdict := findRecord(records, "conclude_verdict") + if verdict == nil { + t.Fatal("missing conclude_verdict record") + } + if verdict["prompt_injection"] != true || verdict["secret_leak"] != false || verdict["has_threats"] != true { + t.Errorf("conclude_verdict fields = %v", verdict) + } + + outcome := findRecord(records, "conclude_outcome") + if outcome == nil { + t.Fatal("missing conclude_outcome record") + } + if outcome["conclusion"] != "failure" || outcome["reason"] != "threat_detected" || outcome["level"] != "error" { + t.Errorf("conclude_outcome fields = %v", outcome) + } +} diff --git a/specs/threat-detection-spec.md b/specs/threat-detection-spec.md index 54a4c64..ec4987c 100644 --- a/specs/threat-detection-spec.md +++ b/specs/threat-detection-spec.md @@ -192,6 +192,21 @@ non-mandatory failures MUST surface as warnings without failing the job, except that `agent_failure` and `parse_error` MUST hard-fail when the detection execution step itself failed. +**TD-20c**: The `conclude` subcommand MUST write a human-readable diagnostic +section to standard output that is sufficient, on its own, to explain the +conclusion. It MUST report the environment inputs it consumed (`RUN_DETECTION`, +`GH_AW_DETECTION_CONTINUE_ON_ERROR`, `DETECTION_AGENTIC_EXECUTION_OUTCOME`) and +the resolved result-file and detection-log paths, and — on both the safe and the +threat path — the per-field verdict (`prompt_injection`, `secret_leak`, +`malicious_patch`) together with an indexed list of reasons. When the result file +is missing or unusable it MUST additionally emit a recursive listing of the +result directory and, when a detection log is present, its line and byte counts +plus every line containing a `THREAT_DETECTION_STATUS:` or +`THREAT_DETECTION_RESULT:` marker. The detection log MUST NOT be used to derive a +verdict; it is diagnostic input only. Diagnostic output MAY be bounded to keep +job logs readable. When `--log-file` is set, `conclude` MUST mirror these +diagnostics and the final conclusion into the JSONL run log (TD-20a). + ### 8.3 Exit Codes **TD-21**: The detector MUST use the following exit codes: diff --git a/specs/usage-spec.md b/specs/usage-spec.md index e437677..af1b834 100644 --- a/specs/usage-spec.md +++ b/specs/usage-spec.md @@ -190,9 +190,18 @@ threat-detect conclude --result-file /detection_result.json `GITHUB_OUTPUT` and export `GH_AW_DETECTION_CONCLUSION` and `GH_AW_DETECTION_REASON` to `GITHUB_ENV` (per TD-20b). +**U-18a**: `conclude` emits a self-contained diagnostic section on standard +output (per TD-20c): the environment inputs and resolved paths, the per-field +verdict breakdown with indexed reasons, and — when the result file is missing or +unusable — a recursive listing of the result directory plus detection-log stats +and any `THREAT_DETECTION_*` marker lines. A conforming host SHOULD surface this +output in its job log so a detection run can be triaged without downloading +artifacts, and MAY pass `--detection-log ` when the detection run's log is +not stored beside the result file. A host MAY pass `--log-file ` to mirror +the conclusion into a JSONL run log. + **U-19**: A conforming host MAY control conclusion behavior through these environment inputs consumed by `conclude`: - | Variable | Effect | |----------|--------| | `RUN_DETECTION` | When not `"true"`, the verdict is `skipped`/`success` | From 3151165841e27808ce9b9062456dda5fe4d84c81 Mon Sep 17 00:00:00 2001 From: GitHub Ace Date: Tue, 4 Aug 2026 21:32:10 +0000 Subject: [PATCH 2/2] fix(conclude): harden diagnostics against log injection and false stats Addresses review feedback on #749: - Reject --log-file colliding with --result-file or the resolved detection log. runlog.Open truncates, so a collision erased the verdict (producing a bogus parse_error) or the failure diagnostics. Uses the existing samePath helper and checks the default sibling detection-log path too. - Treat an unopenable --log-file as a configuration error (TD-20a) instead of silently concluding without the required JSONL mirroring. - Escape control characters in untrusted values echoed to the job log (model-authored reasons, artifact filenames, detection-log lines). An embedded newline previously let a reason such as "text\n::add-mask::x" emit a live GitHub Actions workflow command; regression tests confirm this. - Report the detection log's true byte size and explicitly label truncation rather than presenting the scanned 8 MiB prefix as the whole file. Spec TD-20c and README updated to require truncation labelling and escaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> --- README.md | 11 +- cmd/threat-detect/conclude.go | 142 ++++++++++++++++---- cmd/threat-detect/conclude_test.go | 208 ++++++++++++++++++++++++++++- specs/threat-detection-spec.md | 7 +- 4 files changed, 337 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 511c430..27819a4 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,16 @@ Additional flags: - `--log-file ` — mirror the conclusion into a JSONL run log (env: `THREAT_DETECTION_LOG_FILE`), emitting `conclude_start`, `conclude_verdict`, `conclude_directory_listing`, `conclude_detection_log`, and `conclude_outcome` - events + events. It must not resolve to the same file as `--result-file` or the + detection log — the log is opened truncating, so a collision would destroy the + input it is meant to describe. Collisions and unopenable log paths are + configuration errors that fail the step. + +Diagnostic output is bounded so a pathological run cannot flood the job log, and +truncation is always labelled rather than passed off as a complete reading. +Untrusted values (model-authored reasons, artifact filenames, detection-log +lines) have control characters escaped so each stays on one line and cannot +inject a workflow command into the host job log. ### AI Credits and Token Usage diff --git a/cmd/threat-detect/conclude.go b/cmd/threat-detect/conclude.go index 3eabe5f..46db81f 100644 --- a/cmd/threat-detect/conclude.go +++ b/cmd/threat-detect/conclude.go @@ -10,6 +10,7 @@ import ( "path/filepath" "sort" "strings" + "unicode" "github.com/github/gh-aw-threat-detection/pkg/detector" "github.com/github/gh-aw-threat-detection/pkg/runlog" @@ -82,21 +83,49 @@ func runConclude(args []string) int { return concludeExitFail } - // The JSONL log is an additive observability sink: failing to open it must - // not change the conclusion, so it degrades to a warning on stderr. + // The JSONL log is opened with O_TRUNC, so it must not alias an input this + // command reads. Aliasing --result-file would erase the verdict (yielding a + // bogus parse_error) and aliasing the detection log would erase the failure + // diagnostics; both are configuration errors, checked before anything is + // opened. The detection-log path is resolved first so the default sibling + // path participates in the check. + resolvedDetectionLog := detectionLog + if resolvedDetectionLog == "" { + resolvedDetectionLog = filepath.Join(filepath.Dir(resultFile), defaultDetectionLogName) + } + if logFile != "" { + for _, other := range []struct{ path, flag string }{ + {resultFile, "--result-file"}, + {resolvedDetectionLog, "--detection-log"}, + } { + same, err := samePath(logFile, other.path) + if err != nil { + fmt.Fprintf(os.Stderr, "conclude: error resolving log paths: %v\n", err) + return concludeExitFail + } + if same { + fmt.Fprintf(os.Stderr, "conclude: --log-file and %s must not point to the same file (%q)\n", other.flag, logFile) + return concludeExitFail + } + } + } + + // A failure to open the requested JSONL log is a configuration error + // (TD-20a): the caller explicitly asked for logs, and concluding without + // them would report success while the required mirroring is absent. var logger *runlog.Logger if logFile != "" { opened, err := runlog.Open(logFile) if err != nil { - fmt.Fprintf(os.Stderr, "conclude: failed to open log file: %v\n", err) - } else { - logger = opened - defer func() { - if err := logger.Close(); err != nil { - fmt.Fprintf(os.Stderr, "conclude: failed to close log file: %v\n", err) - } - }() + fmt.Fprintf(os.Stderr, "conclude: error opening log file: %v\n", err) + return concludeExitFail } + logger = opened + defer func() { + if err := logger.Close(); err != nil { + fmt.Fprintf(os.Stderr, "conclude: failed to close log file: %v\n", err) + } + }() } c := &concluder{ @@ -263,7 +292,7 @@ func (c *concluder) reportVerdict(result *detector.Result) { if len(result.Reasons) > 0 { c.info(fmt.Sprintf(" reasons (%d):", len(result.Reasons))) for i, reason := range result.Reasons { - c.info(fmt.Sprintf(" [%d] %s", i+1, reason)) + c.info(fmt.Sprintf(" [%d] %s", i+1, sanitizeLogValue(reason))) } } else { c.info(" reasons : (none)") @@ -299,7 +328,7 @@ func (c *concluder) listDirectory(dir string) { } c.info(fmt.Sprintf(" Found %d file(s):", len(files))) for _, file := range shown { - c.info(" - " + file) + c.info(" - " + sanitizeLogValue(file)) } if len(shown) < len(files) { c.info(fmt.Sprintf(" ... %d more file(s) omitted", len(files)-len(shown))) @@ -313,9 +342,11 @@ func (c *concluder) listDirectory(dir string) { // reportDetectionLog prints size statistics for the detection log plus every // line carrying a THREAT_DETECTION_* marker. The log is never used to derive a -// verdict — this is diagnostics only. +// verdict — this is diagnostics only. Only a bounded prefix of the file is read, +// so the output distinguishes the true file size from the scanned prefix rather +// than reporting the prefix as if it were the whole log. func (c *concluder) reportDetectionLog(path string) { - data, err := readBounded(path, diagnosticLogMaxSize) + data, totalBytes, err := readBounded(path, diagnosticLogMaxSize) if err != nil { if errors.Is(err, fs.ErrNotExist) { c.info(fmt.Sprintf("📄 No detection log found at: %s", path)) @@ -326,24 +357,35 @@ func (c *concluder) reportDetectionLog(path string) { return } + truncated := totalBytes > int64(len(data)) lines := strings.Split(string(data), "\n") - c.info(fmt.Sprintf("📊 Detection log stats: %d lines, %d bytes", len(lines), len(data))) + if truncated { + c.info(fmt.Sprintf("📊 Detection log stats: %d bytes total; scanned first %d bytes (%d lines) — log truncated for diagnostics", + totalBytes, len(data), len(lines))) + } else { + c.info(fmt.Sprintf("📊 Detection log stats: %d lines, %d bytes", len(lines), len(data))) + } + + scope := "lines" + if truncated { + scope = "scanned lines" + } matches := make([]string, 0, 8) for i, line := range lines { if !containsMarker(line) { continue } - matches = append(matches, fmt.Sprintf("[%d] %s", i+1, truncateRunes(strings.TrimRight(line, "\r"), maxEchoedLineRunes))) + matches = append(matches, fmt.Sprintf("[%d] %s", i+1, sanitizeLogValue(truncateRunes(strings.TrimRight(line, "\r"), maxEchoedLineRunes)))) } if len(matches) == 0 { - c.info(fmt.Sprintf("📄 No lines containing THREAT_DETECTION markers found in %d lines", len(lines))) + c.info(fmt.Sprintf("📄 No lines containing THREAT_DETECTION markers found in %d %s", len(lines), scope)) } else { shown := matches if len(shown) > maxEchoedLogLines { shown = shown[:maxEchoedLogLines] } - c.info(fmt.Sprintf("📄 Lines containing THREAT_DETECTION markers (%d of %d):", len(matches), len(lines))) + c.info(fmt.Sprintf("📄 Lines containing THREAT_DETECTION markers (%d of %d %s):", len(matches), len(lines), scope)) for _, m := range shown { c.info(" " + m) } @@ -353,11 +395,13 @@ func (c *concluder) reportDetectionLog(path string) { matches = shown } c.logger.Info("conclude_detection_log", map[string]any{ - "path": path, - "lines": len(lines), - "bytes": len(data), - "marker_lines": matches, - "marker_count": len(matches), + "path": path, + "bytes_total": totalBytes, + "bytes_scanned": len(data), + "lines_scanned": len(lines), + "truncated": truncated, + "marker_lines": matches, + "marker_count": len(matches), }) } @@ -402,15 +446,57 @@ func listFilesRecursively(dir string) ([]string, error) { return files, nil } -// readBounded reads at most limit bytes from path. Detection logs can be very -// large; the diagnostics only need the beginning to be useful. -func readBounded(path string, limit int64) ([]byte, error) { +// readBounded reads at most limit bytes from path and also reports the file's +// total size, so callers can tell how much of the file they actually scanned. +func readBounded(path string, limit int64) ([]byte, int64, error) { f, err := os.Open(path) if err != nil { - return nil, err + return nil, 0, err } defer f.Close() - return io.ReadAll(io.LimitReader(f, limit)) + var totalBytes int64 + if info, err := f.Stat(); err == nil { + totalBytes = info.Size() + } + data, err := io.ReadAll(io.LimitReader(f, limit)) + if err != nil { + return nil, 0, err + } + // A growing or non-regular file can report a size smaller than what was + // read; never claim a total below the bytes actually scanned. + if totalBytes < int64(len(data)) { + totalBytes = int64(len(data)) + } + return data, totalBytes, nil +} + +// sanitizeLogValue renders an untrusted string (a model-authored reason, an +// artifact filename, or a detection-log line) so it cannot break out of its +// single physical log line. Embedded newlines would otherwise let the value +// emit a line of its own beginning with "::", which the Actions runner would +// interpret as a workflow command. Control characters are escaped rather than +// dropped so the original content stays visible and diagnosable. +func sanitizeLogValue(s string) string { + if !strings.ContainsFunc(s, unicode.IsControl) { + return s + } + var b strings.Builder + b.Grow(len(s)) + for _, r := range s { + switch { + case r == '\n': + b.WriteString(`\n`) + case r == '\r': + b.WriteString(`\r`) + case r == '\t': + b.WriteString(`\t`) + case unicode.IsControl(r): + fmt.Fprintf(&b, `\x%02x`, r) + default: + b.WriteRune(r) + } + } + return b.String() } // truncateRunes shortens s to at most max runes, appending an ellipsis marker diff --git a/cmd/threat-detect/conclude_test.go b/cmd/threat-detect/conclude_test.go index f61eca5..922a315 100644 --- a/cmd/threat-detect/conclude_test.go +++ b/cmd/threat-detect/conclude_test.go @@ -438,7 +438,7 @@ func TestConcludeMissingResultDiagnostics(t *testing.T) { " - " + filepath.Join("nested", "stray.txt"), " - " + defaultDetectionLogName, fmt.Sprintf("📊 Detection log stats: 4 lines, %d bytes", len(logBody)), - "📄 Lines containing THREAT_DETECTION markers (1 of 4):", + "📄 Lines containing THREAT_DETECTION markers (1 of 4 lines):", " [2] THREAT_DETECTION_STATUS: reason=engine_error exit=2", } { if !strings.Contains(got, want) { @@ -535,3 +535,209 @@ func TestConcludeRunLog(t *testing.T) { t.Errorf("conclude_outcome fields = %v", outcome) } } + +// TestSanitizeLogValue verifies untrusted values cannot break out of their log +// line. An embedded newline would otherwise let a model-authored reason or an +// artifact filename emit a line of its own beginning with "::", which the +// Actions runner interprets as a workflow command. +func TestSanitizeLogValue(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"plain text is unchanged", "jailbreak attempt", "jailbreak attempt"}, + {"unicode is preserved", "emoji 🚨 and accents é", "emoji 🚨 and accents é"}, + {"newline is escaped", "text\n::add-mask::secret", `text\n::add-mask::secret`}, + {"carriage return is escaped", "text\r::error::boom", `text\r::error::boom`}, + {"tab is escaped", "a\tb", `a\tb`}, + {"other control chars are escaped", "a\x00b\x1bc", `a\x00b\x1bc`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := sanitizeLogValue(tt.in); got != tt.want { + t.Errorf("sanitizeLogValue(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestConcludeReasonCannotInjectWorkflowCommand verifies a malicious reason in +// the verdict cannot emit an unprefixed workflow command into the job log. +func TestConcludeReasonCannotInjectWorkflowCommand(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + verdict := `{"prompt_injection":true,"secret_leak":false,"malicious_patch":false,` + + `"reasons":["benign looking\n::add-mask::injected"]}` + if err := os.WriteFile(resultFile, []byte(verdict), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + warnMode: true, + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + c.run(resultFile) + + for _, line := range strings.Split(stdout.String(), "\n") { + trimmed := strings.TrimSpace(line) + // The only workflow command this run may emit is the threat warning. + if strings.HasPrefix(trimmed, "::") && !strings.HasPrefix(trimmed, "::warning::") { + t.Errorf("unexpected workflow command emitted: %q", line) + } + } + if !strings.Contains(stdout.String(), `[1] benign looking\n::add-mask::injected`) { + t.Errorf("escaped reason not rendered on a single line:\n%s", stdout.String()) + } +} + +// TestConcludeFilenameCannotInjectWorkflowCommand verifies the directory +// listing escapes newlines in artifact filenames, which are untrusted. +func TestConcludeFilenameCannotInjectWorkflowCommand(t *testing.T) { + dir := t.TempDir() + // Linux permits newlines in filenames; a crafted artifact name must not be + // able to start a new log line. + evil := filepath.Join(dir, "evil\n::add-mask::injected.txt") + if err := os.WriteFile(evil, []byte("x"), 0o600); err != nil { + t.Skipf("filesystem rejects newline in filename: %v", err) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + warnMode: true, + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + c.run(filepath.Join(dir, "detection_result.json")) + + for _, line := range strings.Split(stdout.String(), "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "::") && !strings.HasPrefix(trimmed, "::warning::") { + t.Errorf("unexpected workflow command emitted: %q", line) + } + } + if !strings.Contains(stdout.String(), `- evil\n::add-mask::injected.txt`) { + t.Errorf("escaped filename not rendered on a single line:\n%s", stdout.String()) + } +} + +// TestConcludeTruncatedDetectionLogStats verifies that when only a prefix of a +// large detection log is scanned, the reported stats say so instead of passing +// the prefix off as the whole file. +func TestConcludeTruncatedDetectionLogStats(t *testing.T) { + dir := t.TempDir() + logPath := filepath.Join(dir, defaultDetectionLogName) + // Comfortably larger than the read bound so truncation is guaranteed. + body := strings.Repeat("padding line to fill the detection log\n", 300000) + if err := os.WriteFile(logPath, []byte(body), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + if int64(len(body)) <= diagnosticLogMaxSize { + t.Fatalf("fixture too small: %d bytes, want > %d", len(body), diagnosticLogMaxSize) + } + + var stdout bytes.Buffer + c := &concluder{ + runDetection: "true", + warnMode: true, + githubOutput: filepath.Join(dir, "out"), + githubEnv: filepath.Join(dir, "env"), + stdout: &stdout, + } + c.run(filepath.Join(dir, "detection_result.json")) + + got := stdout.String() + if !strings.Contains(got, fmt.Sprintf("%d bytes total", len(body))) { + t.Errorf("stats must report the true file size (%d bytes):\n%s", len(body), got) + } + for _, want := range []string{ + fmt.Sprintf("scanned first %d bytes", diagnosticLogMaxSize), + "log truncated for diagnostics", + "scanned lines", + } { + if !strings.Contains(got, want) { + t.Errorf("stdout missing %q:\n%s", want, got) + } + } +} + +// TestConcludeRejectsLogFileCollisions verifies --log-file (opened O_TRUNC) is +// refused when it aliases an input this command reads: truncating the result +// file would erase the verdict, and truncating the detection log would erase +// the failure diagnostics. +func TestConcludeRejectsLogFileCollisions(t *testing.T) { + t.Run("collides with result file", func(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + if err := os.WriteFile(resultFile, []byte(safeVerdict), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + t.Setenv("RUN_DETECTION", "true") + t.Setenv("GITHUB_OUTPUT", filepath.Join(dir, "out")) + t.Setenv("GITHUB_ENV", filepath.Join(dir, "env")) + + if code := runConclude([]string{"--result-file", resultFile, "--log-file", resultFile}); code != concludeExitFail { + t.Fatalf("runConclude() = %d, want %d", code, concludeExitFail) + } + // The verdict must survive: rejection happens before the log is opened. + data, err := os.ReadFile(resultFile) + if err != nil { + t.Fatalf("ReadFile error = %v", err) + } + if string(data) != safeVerdict { + t.Errorf("result file was modified: %q", string(data)) + } + }) + + t.Run("collides with detection log", func(t *testing.T) { + dir := t.TempDir() + resultFile := filepath.Join(dir, "detection_result.json") + logPath := filepath.Join(dir, defaultDetectionLogName) + if err := os.WriteFile(logPath, []byte("THREAT_DETECTION_STATUS: reason=engine_error exit=2\n"), 0o600); err != nil { + t.Fatalf("WriteFile error = %v", err) + } + t.Setenv("RUN_DETECTION", "true") + t.Setenv("GITHUB_OUTPUT", filepath.Join(dir, "out")) + t.Setenv("GITHUB_ENV", filepath.Join(dir, "env")) + + // The default detection-log path must participate in the check even + // though --detection-log was not passed explicitly. + if code := runConclude([]string{"--result-file", resultFile, "--log-file", logPath}); code != concludeExitFail { + t.Fatalf("runConclude() = %d, want %d", code, concludeExitFail) + } + data, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("ReadFile error = %v", err) + } + if len(data) == 0 { + t.Error("detection log was truncated") + } + }) +} + +// TestConcludeUnopenableLogFileIsConfigError verifies that failing to open an +// explicitly requested --log-file fails the step (TD-20a) rather than silently +// concluding without the required JSONL mirroring. +func TestConcludeUnopenableLogFileIsConfigError(t *testing.T) { + dir := t.TempDir() + resultFile := writeResultFixture(t, safeVerdict) + // A directory cannot be opened for writing. + logPath := filepath.Join(dir, "logdir") + if err := os.Mkdir(logPath, 0o755); err != nil { + t.Fatalf("Mkdir error = %v", err) + } + + t.Setenv("RUN_DETECTION", "true") + t.Setenv("GITHUB_OUTPUT", filepath.Join(dir, "out")) + t.Setenv("GITHUB_ENV", filepath.Join(dir, "env")) + + if code := runConclude([]string{"--result-file", resultFile, "--log-file", logPath}); code != concludeExitFail { + t.Fatalf("runConclude() = %d, want %d", code, concludeExitFail) + } +} diff --git a/specs/threat-detection-spec.md b/specs/threat-detection-spec.md index ec4987c..ed2ceb0 100644 --- a/specs/threat-detection-spec.md +++ b/specs/threat-detection-spec.md @@ -204,7 +204,12 @@ result directory and, when a detection log is present, its line and byte counts plus every line containing a `THREAT_DETECTION_STATUS:` or `THREAT_DETECTION_RESULT:` marker. The detection log MUST NOT be used to derive a verdict; it is diagnostic input only. Diagnostic output MAY be bounded to keep -job logs readable. When `--log-file` is set, `conclude` MUST mirror these +job logs readable, but when it is bounded the output MUST indicate that it was +truncated and MUST NOT report a bounded prefix as though it were the whole +input. Untrusted values echoed into the diagnostics (model-authored reasons, +artifact filenames, and detection-log lines) MUST be escaped so that each is +confined to a single physical output line and cannot emit a host workflow +command. When `--log-file` is set, `conclude` MUST mirror these diagnostics and the final conclusion into the JSONL run log (TD-20a). ### 8.3 Exit Codes