Skip to content

Commit dd6c8cc

Browse files
davidslaterGitHub Ace
andauthored
fix(detector): bound and sanitize the open-text reasons in detection_result.json (#803)
* fix(detector): bound the open-text reasons in detection_result.json The three boolean fields of the result contract were already strictly validated (required, correctly typed, no extra fields), but the `reasons` array is model-authored free text and had no bounds at all. A result with 500 reasons of 200 KB each was accepted on both the report and the read side; concluding it read a 100 MB file fully into memory and emitted 200 MB into the job log. Whitespace-only reasons also satisfied the "at least one reason when a threat is reported" requirement, defeating it. Bound the free-text portion symmetrically on write and read, so no result the tool accepts can be rejected later: - at most 20 reasons, each non-blank and at most 1000 runes - cap result-file reads at 1 MiB and reject oversize files as parse errors rather than parsing a truncated prefix Also sanitize and truncate reasons before folding them into the ::error:: annotation. That path joined them raw, so while escapeWorkflowData neutralized newlines, ANSI and other control characters reached the job log unescaped — inconsistent with reportVerdict, which already escaped them. Document the bounds as TD-10b/TD-10c and state them in the detection prompt so the model targets them instead of discovering them by retry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * fix(detector): enforce the reason bounds on write, not just on read WriteResultFile marshaled any Result directly, so a result violating the TD-10b bounds was written successfully and only rejected later by ReadResultFile. That contradicts the documented write/read symmetry. No production caller could reach it — report.go validates via ValidateReportFields first — but WriteResultFile is exported, so a future caller could persist a file that fails much later at conclude time as a misleading ERR_PARSE, reported as a detection outage rather than the malformed verdict it is. Validate the marshaled bytes before creating any file, so what is checked is byte-for-byte what a reader would parse, and also enforce MaxResultFileBytes on the encoded form. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> --------- Co-authored-by: GitHub Ace <githubnext@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com>
1 parent c45d912 commit dd6c8cc

7 files changed

Lines changed: 308 additions & 5 deletions

File tree

README.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,16 @@ explicitly treated as untrusted runtime data.
337337
}
338338
```
339339

340+
The three booleans are fully constrained by the schema: all are required, no
341+
other fields are accepted, and a result that adds, omits, or mistypes a field is
342+
rejected. `reasons` is model-authored free text and is bounded as well — at most
343+
20 entries, each non-blank and at most 1000 characters — and the whole result
344+
file is capped at 1 MiB before it is parsed. The same bounds apply when the model
345+
reports a result and when the file is read back, so a recorded result can never
346+
fail validation later. A rejected report is returned to the model as a
347+
correctable tool error; an oversized or malformed result file is a parse error
348+
that fails the detection closed.
349+
340350
### Replay workflow
341351

342352
Maintainers can manually run **Replay Threat Detection** from the Actions tab to rerun detection against artifacts from a prior workflow run. Provide the source repository and run ID; the workflow downloads the `agent`, `activation`, optional experiment, and optional original `detection` artifacts, normalizes them into the CLI input contract above, runs `threat-detect`, and uploads a sanitized `replay-detection-<run_id>` artifact with the manifest, file inventory, free-form replay log, replay result, and original-result comparison.

cmd/threat-detect/conclude.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,14 @@ func (c *concluder) conclude(resultFile string) int {
231231
}
232232
message := fmt.Sprintf("%s: ❌ Security threats detected: %s", errCodeValidation, strings.Join(threats, ", "))
233233
if len(result.Reasons) > 0 {
234-
message += "\nReasons: " + strings.Join(result.Reasons, "; ")
234+
// Reasons are model-authored open text; sanitize and bound them the
235+
// same way reportVerdict does before folding them into the workflow
236+
// command and the run log.
237+
safe := make([]string, 0, len(result.Reasons))
238+
for _, reason := range result.Reasons {
239+
safe = append(safe, sanitizeLogValue(truncateRunes(reason, maxEchoedLineRunes)))
240+
}
241+
message += "\nReasons: " + strings.Join(safe, "; ")
235242
}
236243
return c.fail(result, detector.ReasonThreatDetected, message)
237244
}

cmd/threat-detect/conclude_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import (
77
"path/filepath"
88
"strings"
99
"testing"
10+
11+
"github.com/github/gh-aw-threat-detection/pkg/detector"
1012
)
1113

1214
// parseKV reads a name=value file (as written to $GITHUB_OUTPUT / $GITHUB_ENV)
@@ -967,3 +969,70 @@ func TestConcludeToolingFailureLogsEngineFailure(t *testing.T) {
967969
t.Errorf("job log missing tooling-failure disclaimer; got:\n%s", stdout.String())
968970
}
969971
}
972+
973+
// TestConcludeThreatMessageSanitizesReasons verifies that model-authored reason
974+
// text cannot inject control characters into the ::error:: annotation. The
975+
// reasons list is open text, so it is escaped the same way reportVerdict
976+
// escapes it before being folded into the workflow command.
977+
func TestConcludeThreatMessageSanitizesReasons(t *testing.T) {
978+
dir := t.TempDir()
979+
resultFile := writeResultFixture(t,
980+
`{"prompt_injection":true,"secret_leak":false,"malicious_patch":false,"reasons":["esc\u001b[31m tab\there"]}`)
981+
982+
var stdout bytes.Buffer
983+
c := &concluder{
984+
runDetection: "true",
985+
githubOutput: filepath.Join(dir, "out"),
986+
githubEnv: filepath.Join(dir, "env"),
987+
stdout: &stdout,
988+
}
989+
if code := c.run(resultFile); code != concludeExitFail {
990+
t.Fatalf("exit code = %d, want %d", code, concludeExitFail)
991+
}
992+
errorLine := ""
993+
for _, line := range strings.Split(stdout.String(), "\n") {
994+
if strings.HasPrefix(line, "::error::") {
995+
errorLine = line
996+
}
997+
}
998+
if errorLine == "" {
999+
t.Fatalf("expected ::error:: command, got: %q", stdout.String())
1000+
}
1001+
if strings.ContainsAny(errorLine, "\x1b\t") {
1002+
t.Fatalf("control characters must be escaped in the annotation, got: %q", errorLine)
1003+
}
1004+
if !strings.Contains(errorLine, `esc\x1b[31m tab\there`) {
1005+
t.Fatalf("expected escaped reason text, got: %q", errorLine)
1006+
}
1007+
}
1008+
1009+
// TestConcludeRejectsOversizeResultFile verifies that an oversized result file
1010+
// is reported as a parse error and fails closed, instead of being read into
1011+
// memory and echoed in full into the job log.
1012+
func TestConcludeRejectsOversizeResultFile(t *testing.T) {
1013+
dir := t.TempDir()
1014+
resultFile := writeResultFixture(t,
1015+
`{"prompt_injection":false,"secret_leak":false,"malicious_patch":false,"reasons":["`+
1016+
strings.Repeat("x", detector.MaxResultFileBytes)+`"]}`)
1017+
1018+
var stdout bytes.Buffer
1019+
c := &concluder{
1020+
runDetection: "true",
1021+
githubOutput: filepath.Join(dir, "out"),
1022+
githubEnv: filepath.Join(dir, "env"),
1023+
stdout: &stdout,
1024+
}
1025+
if code := c.run(resultFile); code != concludeExitFail {
1026+
t.Fatalf("exit code = %d, want %d", code, concludeExitFail)
1027+
}
1028+
got := stdout.String()
1029+
if !strings.Contains(got, errCodeParse) {
1030+
t.Fatalf("expected %s, got: %q", errCodeParse, got)
1031+
}
1032+
if len(got) > 1<<16 {
1033+
t.Fatalf("oversize result file must not be echoed into the job log; got %d bytes", len(got))
1034+
}
1035+
if outputs := parseKV(t, filepath.Join(dir, "out")); outputs["reason"] != detector.ReasonParseError {
1036+
t.Fatalf("reason = %q, want %q", outputs["reason"], detector.ReasonParseError)
1037+
}
1038+
}

pkg/detector/prompts/threat_detection.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,9 @@ Analyze the above content for the following security threats, using the workflow
184184

185185
Pass each `--prompt-injection`, `--secret-leak`, and `--malicious-patch` flag as `true` if you detect that
186186
type of threat, `false` otherwise. Add one `--reason` flag per detected threat explaining it (at least one
187-
`--reason` is required whenever any threat is `true`).
187+
`--reason` is required whenever any threat is `true`). Each `--reason` must be non-empty and at most 1000
188+
characters, and you may pass at most 20 of them — write concise explanations, not transcripts or quoted
189+
artifact dumps.
188190

189191
The command validates your input and prints `THREAT_DETECTION_RESULT_ERROR` with the problem if anything is
190192
wrong — fix it and run the command again. When it prints `THREAT_DETECTION_RESULT_RECORDED`, the analysis is

pkg/detector/result.go

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,25 @@ import (
77
"io"
88
"os"
99
"path/filepath"
10+
"strings"
11+
"unicode/utf8"
12+
)
13+
14+
// Bounds on the free-text portion of the result contract. The three boolean
15+
// fields are fully constrained by their type, but `reasons` is model-authored
16+
// open text that flows into job logs, workflow commands, and the run log, so it
17+
// is bounded on both the reporting and the reading side.
18+
const (
19+
// MaxReasons is the maximum number of entries allowed in `reasons`. Three
20+
// threat categories never need more than a handful of explanations.
21+
MaxReasons = 20
22+
// MaxReasonRunes is the maximum length of a single reason. Reasons are
23+
// human-readable explanations, not transcripts or embedded artifacts.
24+
MaxReasonRunes = 1000
25+
// MaxResultFileBytes caps how much of a result file is read before parsing.
26+
// It is far above any schema-valid result (MaxReasons × MaxReasonRunes plus
27+
// JSON overhead) yet bounds memory for a corrupt or hostile file.
28+
MaxResultFileBytes = 1 << 20 // 1 MiB
1029
)
1130

1231
// Result represents the structured output of threat detection analysis.
@@ -82,16 +101,33 @@ func validateRawResult(raw map[string]any, label string) error {
82101
if !ok {
83102
return fmt.Errorf("invalid type for %q: expected array, got %T (%v)", "reasons", reasons, reasons)
84103
}
104+
if len(reasonsArr) > MaxReasons {
105+
return fmt.Errorf("too many entries in %q: got %d, maximum is %d", "reasons", len(reasonsArr), MaxReasons)
106+
}
85107
for i, reason := range reasonsArr {
86-
if _, ok := reason.(string); !ok {
108+
text, ok := reason.(string)
109+
if !ok {
87110
return fmt.Errorf("invalid type for %q[%d]: expected string, got %T (%v)", "reasons", i, reason, reason)
88111
}
112+
if strings.TrimSpace(text) == "" {
113+
return fmt.Errorf("invalid value for %q[%d]: reason must not be empty or whitespace-only", "reasons", i)
114+
}
115+
if n := utf8.RuneCountInString(text); n > MaxReasonRunes {
116+
return fmt.Errorf("invalid value for %q[%d]: reason is %d characters, maximum is %d", "reasons", i, n, MaxReasonRunes)
117+
}
89118
}
90119
return nil
91120
}
92121

93122
// WriteResultFile atomically writes r as canonical THREAT_DETECTION_RESULT JSON
94123
// to path (temp file in the same dir + rename), with 0o600 permissions.
124+
//
125+
// The marshaled bytes are validated against the same rules ReadResultFile
126+
// applies before any file is created, so a result that would not survive being
127+
// read back is rejected at the source rather than persisted as an unreadable
128+
// file. Validating the canonical JSON — rather than the in-memory struct —
129+
// makes the guarantee exact: what is checked is byte-for-byte what a reader
130+
// would parse.
95131
func WriteResultFile(path string, r *Result) error {
96132
if r == nil {
97133
return fmt.Errorf("cannot write nil result")
@@ -105,6 +141,12 @@ func WriteResultFile(path string, r *Result) error {
105141
if err != nil {
106142
return fmt.Errorf("marshaling result: %w", err)
107143
}
144+
if int64(len(data)) > MaxResultFileBytes {
145+
return fmt.Errorf("refusing to write result: encoded result is %d bytes, exceeding the maximum of %d", len(data), MaxResultFileBytes)
146+
}
147+
if _, err := ParseStructuredResult(data); err != nil {
148+
return fmt.Errorf("refusing to write result that could not be read back: %w", err)
149+
}
108150
dir := filepath.Dir(path)
109151
tmp, err := os.CreateTemp(dir, ".threat-detect-result-*.tmp")
110152
if err != nil {
@@ -133,12 +175,23 @@ func WriteResultFile(path string, r *Result) error {
133175
}
134176

135177
// ReadResultFile reads path and parses it with ParseStructuredResult, returning
136-
// a validated *Result. Returns an error if the file is missing, empty, or invalid.
178+
// a validated *Result. Returns an error if the file is missing, empty, larger
179+
// than MaxResultFileBytes, or invalid.
137180
func ReadResultFile(path string) (*Result, error) {
138-
data, err := os.ReadFile(path)
181+
f, err := os.Open(path)
139182
if err != nil {
140183
return nil, err
141184
}
185+
defer f.Close()
186+
// Read one byte past the cap so an oversized file is rejected rather than
187+
// silently truncated into a parse error that hides the real cause.
188+
data, err := io.ReadAll(io.LimitReader(f, MaxResultFileBytes+1))
189+
if err != nil {
190+
return nil, err
191+
}
192+
if int64(len(data)) > MaxResultFileBytes {
193+
return nil, fmt.Errorf("result file %q exceeds the maximum size of %d bytes", path, MaxResultFileBytes)
194+
}
142195
if len(bytes.TrimSpace(data)) == 0 {
143196
return nil, fmt.Errorf("result file %q is empty", path)
144197
}

pkg/detector/result_test.go

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package detector
22

33
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
47
"testing"
58
)
69

@@ -42,3 +45,148 @@ func TestResult_HasThreats(t *testing.T) {
4245
})
4346
}
4447
}
48+
49+
func TestParseStructuredResult_ReasonBounds(t *testing.T) {
50+
build := func(reasons string) []byte {
51+
return []byte(`{"prompt_injection":false,"secret_leak":false,"malicious_patch":false,"reasons":[` + reasons + `]}`)
52+
}
53+
54+
tests := []struct {
55+
name string
56+
reasons string
57+
wantErr bool
58+
}{
59+
{"single reason", `"looks fine"`, false},
60+
{"max length reason", `"` + strings.Repeat("x", MaxReasonRunes) + `"`, false},
61+
{"max count reasons", strings.TrimSuffix(strings.Repeat(`"r",`, MaxReasons), ","), false},
62+
{"empty reason", `""`, true},
63+
{"whitespace-only reason", `" \t "`, true},
64+
{"over-long reason", `"` + strings.Repeat("x", MaxReasonRunes+1) + `"`, true},
65+
{"too many reasons", strings.TrimSuffix(strings.Repeat(`"r",`, MaxReasons+1), ","), true},
66+
}
67+
68+
for _, tt := range tests {
69+
t.Run(tt.name, func(t *testing.T) {
70+
_, err := ParseStructuredResult(build(tt.reasons))
71+
if tt.wantErr && err == nil {
72+
t.Fatalf("expected error for %s", tt.name)
73+
}
74+
if !tt.wantErr && err != nil {
75+
t.Fatalf("unexpected error for %s: %v", tt.name, err)
76+
}
77+
})
78+
}
79+
}
80+
81+
func TestParseStructuredResult_ReasonRunesCountedAsRunes(t *testing.T) {
82+
// MaxReasonRunes bounds characters, not bytes: a multi-byte reason at the
83+
// limit must be accepted even though its byte length exceeds the limit.
84+
data := []byte(`{"prompt_injection":false,"secret_leak":false,"malicious_patch":false,"reasons":["` +
85+
strings.Repeat("é", MaxReasonRunes) + `"]}`)
86+
if _, err := ParseStructuredResult(data); err != nil {
87+
t.Fatalf("unexpected error for multi-byte reason at the rune limit: %v", err)
88+
}
89+
}
90+
91+
func TestReadResultFile_RejectsOversizeFile(t *testing.T) {
92+
dir := t.TempDir()
93+
path := filepath.Join(dir, "detection_result.json")
94+
// A syntactically valid but oversized file must be rejected on size before
95+
// it is parsed, so a hostile file cannot be read into memory in full.
96+
payload := `{"prompt_injection":false,"secret_leak":false,"malicious_patch":false,"reasons":["` +
97+
strings.Repeat("x", MaxResultFileBytes) + `"]}`
98+
if err := os.WriteFile(path, []byte(payload), 0o600); err != nil {
99+
t.Fatalf("writing oversize result file: %v", err)
100+
}
101+
_, err := ReadResultFile(path)
102+
if err == nil {
103+
t.Fatal("expected oversize result file to be rejected")
104+
}
105+
if !strings.Contains(err.Error(), "exceeds the maximum size") {
106+
t.Fatalf("expected size error, got: %v", err)
107+
}
108+
}
109+
110+
func TestReadResultFile_RoundTripsWrittenResult(t *testing.T) {
111+
dir := t.TempDir()
112+
path := filepath.Join(dir, "detection_result.json")
113+
// Every result the detector writes must pass its own read-side validation.
114+
want := BuildResultFromReport(true, false, false, []string{"injected instruction in issue body"})
115+
if err := WriteResultFile(path, want); err != nil {
116+
t.Fatalf("writing result: %v", err)
117+
}
118+
got, err := ReadResultFile(path)
119+
if err != nil {
120+
t.Fatalf("reading back written result: %v", err)
121+
}
122+
if !got.HasThreats() || len(got.Reasons) != 1 || got.Reasons[0] != want.Reasons[0] {
123+
t.Fatalf("round-trip mismatch: %+v", got)
124+
}
125+
}
126+
127+
func TestValidateReportFields_BoundsReasons(t *testing.T) {
128+
if msg := ValidateReportFields(false, false, false, []any{" "}); msg == "" {
129+
t.Fatal("expected whitespace-only reason to be rejected at report time")
130+
}
131+
if msg := ValidateReportFields(true, false, false, []any{"real reason"}); msg != "" {
132+
t.Fatalf("unexpected rejection: %s", msg)
133+
}
134+
}
135+
136+
// TestWriteResultFile_RejectsUnreadableResult verifies the write/read symmetry
137+
// required by TD-10b: a result that would be rejected on read must be rejected
138+
// on write, and must not leave a file behind. Without this, a caller that
139+
// bypassed the report-time validation could persist a result file that only
140+
// fails much later, at conclude time, as a misleading parse error.
141+
func TestWriteResultFile_RejectsUnreadableResult(t *testing.T) {
142+
tests := []struct {
143+
name string
144+
result *Result
145+
}{
146+
{"over-long reason", &Result{PromptInjection: true, Reasons: []string{strings.Repeat("x", MaxReasonRunes+1)}}},
147+
{"blank reason", &Result{PromptInjection: true, Reasons: []string{" "}}},
148+
{"empty reason", &Result{PromptInjection: true, Reasons: []string{""}}},
149+
{"too many reasons", &Result{PromptInjection: true, Reasons: make([]string, MaxReasons+1)}},
150+
}
151+
152+
for _, tt := range tests {
153+
t.Run(tt.name, func(t *testing.T) {
154+
dir := t.TempDir()
155+
path := filepath.Join(dir, "detection_result.json")
156+
if err := WriteResultFile(path, tt.result); err == nil {
157+
t.Fatal("expected write to be rejected")
158+
}
159+
if _, err := os.Stat(path); !os.IsNotExist(err) {
160+
t.Fatalf("rejected write must not leave a result file behind (stat err = %v)", err)
161+
}
162+
// The atomic-write temp file must be cleaned up too.
163+
entries, err := os.ReadDir(dir)
164+
if err != nil {
165+
t.Fatalf("ReadDir error = %v", err)
166+
}
167+
if len(entries) != 0 {
168+
t.Fatalf("rejected write left %d file(s) behind: %v", len(entries), entries)
169+
}
170+
})
171+
}
172+
}
173+
174+
// TestWriteResultFile_AcceptsBoundaryResult verifies the symmetry does not
175+
// over-reject: a result exactly at the documented limits must round-trip.
176+
func TestWriteResultFile_AcceptsBoundaryResult(t *testing.T) {
177+
path := filepath.Join(t.TempDir(), "detection_result.json")
178+
reasons := make([]string, MaxReasons)
179+
for i := range reasons {
180+
reasons[i] = strings.Repeat("y", MaxReasonRunes)
181+
}
182+
if err := WriteResultFile(path, &Result{SecretLeak: true, Reasons: reasons}); err != nil {
183+
t.Fatalf("result at the documented limits must be writable: %v", err)
184+
}
185+
got, err := ReadResultFile(path)
186+
if err != nil {
187+
t.Fatalf("result at the documented limits must be readable: %v", err)
188+
}
189+
if len(got.Reasons) != MaxReasons {
190+
t.Fatalf("reasons = %d, want %d", len(got.Reasons), MaxReasons)
191+
}
192+
}

0 commit comments

Comments
 (0)