Skip to content

Commit 09b2818

Browse files
davidslaterGitHub Ace
andauthored
fix(engine): frame forwarded engine output so it cannot forge diagnostics (#851)
* fix(engine): frame forwarded engine output so it cannot forge diagnostics Engine subprocess stdout/stderr were teed to the detector's own stderr unframed. Since the engine analyzes attacker-controlled artifacts, model-authored text could emit a line impersonating a detector THREAT_DETECTION_* marker or a GitHub Actions workflow command. Forwarding now goes through a line framer that prefixes every forwarded line with "[engine] ", treats both LF and CR as terminators, flushes a trailing partial line, bounds unterminated output, and neutralizes a leading "::". Real-time streaming and the verbatim capture buffers used for error reporting are unchanged. Consumers are hardened independently: lastDetectionStatusReason and the replay workflow's THREAT_DETECTION_RESULT scan both skip framed lines. TD-20a's carve-out is replaced with a positive framing requirement. Closes #796 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Slater <12449447+davidslater@users.noreply.github.com> * style(test): restore line break in malformed doc comment 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 3581a67 commit 09b2818

9 files changed

Lines changed: 424 additions & 19 deletions

File tree

.github/workflows/replay-detection.yml

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -484,13 +484,26 @@ jobs:
484484
485485
# Diagnostic-only: parses the *original* gh-aw run's log, which may
486486
# carry the legacy marker wrapped in Markdown emphasis (**/__/*/_).
487+
#
488+
# Lines prefixed with "[engine] " are engine subprocess output the
489+
# detector forwarded — untrusted, model-authored text. They
490+
# are skipped so an injected marker cannot be promoted to the
491+
# "original" verdict this replay is compared against.
487492
marker = re.compile(r'THREAT_DETECTION_RESULT:\s*(\{.*\})')
493+
engine_prefix = '[engine] '
488494
for path in candidates:
489495
for line in path.read_text(errors='replace').splitlines():
496+
if line.lstrip(' \t').startswith(engine_prefix):
497+
continue
490498
match = marker.search(line)
491-
if match:
492-
output.write_text(json.dumps(json.loads(match.group(1)), indent=2) + '\n')
493-
raise SystemExit(0)
499+
if not match:
500+
continue
501+
try:
502+
parsed = json.loads(match.group(1))
503+
except ValueError:
504+
continue
505+
output.write_text(json.dumps(parsed, indent=2) + '\n')
506+
raise SystemExit(0)
494507
raise SystemExit(0)
495508
PY
496509

README.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,11 @@ line. The rendered prompt itself is never echoed.
130130
Untrusted values interpolated into these detector-authored lines are escaped to a
131131
single physical line and listings are bounded, so neither a model-authored string
132132
nor a hostile filename can forge a workflow command or flood the job log. The
133-
engine subprocess's own stdout/stderr are a separate stream: they are forwarded
134-
verbatim (so harness output and engine errors appear in real time) and are not
135-
detector-attested.
133+
engine subprocess's own stdout/stderr are a separate, untrusted stream: they are
134+
forwarded line by line in real time (so harness output and engine errors stay
135+
visible), each line prefixed with `[engine] ` and stripped of its ability to open
136+
a workflow command. Forwarded lines are not detector-attested — log consumers
137+
must ignore any `THREAT_DETECTION_*` marker that carries the `[engine] ` prefix.
136138

137139
```text
138140
[threat-detect] run start: version=1.2.3 engine=copilot model=(none; using engine default) retries=1

cmd/threat-detect/conclude.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"unicode"
1414

1515
"github.com/github/gh-aw-threat-detection/pkg/detector"
16+
"github.com/github/gh-aw-threat-detection/pkg/engine"
1617
)
1718

1819
// Exit codes for the conclude subcommand. These map directly onto whether the
@@ -490,6 +491,10 @@ func (c *concluder) detectionFailureReason() (reason, errCode string) {
490491
// "" if the file cannot be read or no such line is present. Only the last
491492
// occurrence is used because a retried run may have emitted earlier lines from
492493
// unrelated invocations captured in the same file.
494+
//
495+
// Lines carrying engine.PassthroughPrefix are forwarded engine subprocess output
496+
// — untrusted, model-authored text — and are skipped so a status line the
497+
// detector never emitted cannot drive the reported failure reason.
493498
func lastDetectionStatusReason(path string) string {
494499
if path == "" {
495500
return ""
@@ -500,6 +505,9 @@ func lastDetectionStatusReason(path string) string {
500505
}
501506
reason := ""
502507
for _, line := range strings.Split(string(data), "\n") {
508+
if isForwardedEngineLine(line) {
509+
continue
510+
}
503511
idx := strings.Index(line, statusPrefix)
504512
if idx < 0 {
505513
continue
@@ -518,6 +526,14 @@ func lastDetectionStatusReason(path string) string {
518526
return reason
519527
}
520528

529+
// isForwardedEngineLine reports whether a captured-log line is engine subprocess
530+
// output the detector forwarded, rather than a diagnostic the detector authored.
531+
// Leading whitespace is tolerated so a line indented by an outer log capture is
532+
// still recognized.
533+
func isForwardedEngineLine(line string) bool {
534+
return strings.HasPrefix(strings.TrimLeft(line, " \t"), engine.PassthroughPrefix)
535+
}
536+
521537
// fail records a failure verdict and decides whether to fail closed. It mirrors
522538
// gh-aw's setDetectionFailure exactly:
523539
//

cmd/threat-detect/conclude_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"testing"
1010

1111
"github.com/github/gh-aw-threat-detection/pkg/detector"
12+
"github.com/github/gh-aw-threat-detection/pkg/engine"
1213
)
1314

1415
// parseKV reads a name=value file (as written to $GITHUB_OUTPUT / $GITHUB_ENV)
@@ -518,6 +519,41 @@ func TestDetectionFailureReasonTerminalLineWithoutReasonResetsCandidate(t *testi
518519
}
519520
}
520521

522+
// TestDetectionFailureReasonIgnoresForgedStatusInEngineOutput verifies that a
523+
// THREAT_DETECTION_STATUS: line appearing on forwarded engine output (which is
524+
// model-authored and therefore untrusted) cannot drive the reported failure
525+
// reason. This matters when the detector is killed before emitting its own
526+
// terminal status line, leaving the forged line as the last match.
527+
func TestDetectionFailureReasonIgnoresForgedStatusInEngineOutput(t *testing.T) {
528+
dir := t.TempDir()
529+
logPath := filepath.Join(dir, "detection.log")
530+
content := "[threat-detect] detection attempt 1 of 1\n" +
531+
engine.PassthroughPrefix + "THREAT_DETECTION_STATUS: reason=invalid_report_exhausted exit=2\n" +
532+
" " + engine.PassthroughPrefix + "THREAT_DETECTION_STATUS: reason=output_write_error exit=2\n"
533+
if err := os.WriteFile(logPath, []byte(content), 0o600); err != nil {
534+
t.Fatalf("WriteFile error = %v", err)
535+
}
536+
537+
outPath := filepath.Join(dir, "out")
538+
envPath := filepath.Join(dir, "env")
539+
resultFile := filepath.Join(dir, "detection_result.json")
540+
541+
var stdout bytes.Buffer
542+
c := &concluder{
543+
runDetection: "true",
544+
warnMode: false,
545+
githubOutput: outPath,
546+
githubEnv: envPath,
547+
detectionLog: logPath,
548+
stdout: &stdout,
549+
}
550+
c.run(resultFile)
551+
outputs := parseKV(t, outPath)
552+
if got := outputs["reason"]; got != "agent_failure" {
553+
t.Errorf("reason output = %q, want %q (forged status in engine output must be ignored)", got, "agent_failure")
554+
}
555+
}
556+
521557
// TestDetectionFailureReasonWithoutLogFallsBackToAgentFailure verifies that a
522558
// missing or absent detection log preserves the pre-existing agent_failure
523559
// default rather than erroring.

cmd/threat-detect/main.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,9 @@ func run() (code int) {
354354

355355
result, err := analyzeWithRetries(ctx, eng, prompt, sinkPath, retries)
356356
if err != nil {
357-
fmt.Fprintf(os.Stderr, "Error running detection: %v\n", err)
357+
// The message can embed captured engine output (engineExitError), which
358+
// is untrusted: sanitize it so it cannot break out of this line.
359+
fmt.Fprintf(os.Stderr, "Error running detection: %s\n", sanitizeLogValue(err.Error()))
358360
switch {
359361
case ctx.Err() != nil:
360362
reason = reasonCancelled

pkg/engine/engine.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -455,13 +455,19 @@ func runCLIEnvWithSink(ctx context.Context, name string, args []string, stdinDat
455455
}
456456

457457
var stdout, stderr bytes.Buffer
458-
// Tee both stdout and stderr to os.Stderr so that harness lifecycle output
459-
// ([copilot-harness], [claude-harness], [codex-harness]) and engine errors
460-
// appear in the GitHub Actions job log in real-time, mirroring the agent
461-
// job's "2>&1 | tee" pattern. The buffers are still populated for error
458+
// Tee both stdout and stderr to the detector's stderr so that harness
459+
// lifecycle output ([copilot-harness], [claude-harness], [codex-harness])
460+
// and engine errors appear in the GitHub Actions job log in real-time,
461+
// mirroring the agent job's "2>&1 | tee" pattern. Forwarding goes through a
462+
// framer: the engine is analyzing attacker-controlled artifacts, so each
463+
// forwarded line is prefixed (PassthroughPrefix) and stripped of its ability
464+
// to open a workflow command, keeping it distinguishable from
465+
// detector-attested diagnostics. The buffers are still populated for error
462466
// reporting and sink-result checking.
463-
cmd.Stdout = io.MultiWriter(&stdout, os.Stderr)
464-
cmd.Stderr = io.MultiWriter(&stderr, os.Stderr)
467+
framer := newPassthroughFramer(enginePassthroughStderr)
468+
defer framer.Close()
469+
cmd.Stdout = io.MultiWriter(&stdout, framer.writer())
470+
cmd.Stderr = io.MultiWriter(&stderr, framer.writer())
465471

466472
if err := cmd.Run(); err != nil {
467473
if sinkPath != "" {
@@ -522,6 +528,10 @@ func tailTruncate(s string, max int) string {
522528
// production it is os.Stderr, matching the GitHub Actions job log.
523529
var engineInvokeStderr io.Writer = os.Stderr
524530

531+
// enginePassthroughStderr is the destination for forwarded engine subprocess
532+
// output. It is a package variable for the same reason as engineInvokeStderr.
533+
var enginePassthroughStderr io.Writer = os.Stderr
534+
525535
// logEngineInvoke logs the engine subprocess invocation details to stderr. It is
526536
// called immediately before the engine process starts so that silent engine
527537
// failures (exit with no output) leave enough context in the job log to diagnose

pkg/engine/passthrough.go

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
package engine
2+
3+
import (
4+
"io"
5+
"strings"
6+
"sync"
7+
)
8+
9+
// PassthroughPrefix is prepended to every line of engine subprocess output that
10+
// the detector forwards to its own standard error. The engine analyzes
11+
// attacker-controlled artifacts, so its output is untrusted: without a frame,
12+
// model-authored text containing a newline could impersonate a detector
13+
// diagnostic (a THREAT_DETECTION_* marker) or a host workflow command. The
14+
// prefix makes forwarded bytes distinguishable — for humans reading the job log
15+
// and for tooling that scans it — while preserving real-time streaming.
16+
//
17+
// Consumers that scan the captured log for detector-attested markers MUST
18+
// ignore lines carrying this prefix.
19+
const PassthroughPrefix = "[engine] "
20+
21+
// maxPassthroughLineBytes bounds how much engine output is buffered while
22+
// waiting for a line terminator. An engine that never emits a newline would
23+
// otherwise grow the buffer without limit; past this many bytes the pending
24+
// content is flushed as its own framed line.
25+
const maxPassthroughLineBytes = 8192
26+
27+
// passthroughFramer forwards engine subprocess output to a destination writer,
28+
// one framed line at a time. A single framer serves both the stdout and stderr
29+
// streams of one subprocess: it owns the lock that keeps their interleaved
30+
// writes from splicing into each other's lines.
31+
type passthroughFramer struct {
32+
mu sync.Mutex
33+
dst io.Writer
34+
writers []*passthroughWriter
35+
}
36+
37+
func newPassthroughFramer(dst io.Writer) *passthroughFramer {
38+
return &passthroughFramer{dst: dst}
39+
}
40+
41+
// writer returns a new stream writer bound to this framer. It must be called
42+
// before the subprocess starts, since the writer list is not itself guarded.
43+
func (f *passthroughFramer) writer() io.Writer {
44+
w := &passthroughWriter{framer: f}
45+
f.writers = append(f.writers, w)
46+
return w
47+
}
48+
49+
// Close flushes any partial line each stream left behind — output that ended
50+
// without a trailing newline, which would otherwise be lost.
51+
func (f *passthroughFramer) Close() {
52+
f.mu.Lock()
53+
defer f.mu.Unlock()
54+
for _, w := range f.writers {
55+
w.flushLocked()
56+
}
57+
}
58+
59+
// passthroughWriter accumulates one stream's bytes and emits a framed line each
60+
// time a terminator is seen.
61+
type passthroughWriter struct {
62+
framer *passthroughFramer
63+
buf []byte
64+
pendingCR bool
65+
}
66+
67+
// Write never reports an error: forwarding is a diagnostic convenience, and
68+
// failing the subprocess because the job log could not be written would turn a
69+
// cosmetic problem into a detection outage.
70+
func (w *passthroughWriter) Write(p []byte) (int, error) {
71+
f := w.framer
72+
f.mu.Lock()
73+
defer f.mu.Unlock()
74+
for _, b := range p {
75+
if w.pendingCR {
76+
w.pendingCR = false
77+
// A CR already terminated the line; swallow the LF of a CRLF pair
78+
// rather than emitting an empty line for it.
79+
if b == '\n' {
80+
continue
81+
}
82+
}
83+
switch b {
84+
case '\n':
85+
w.emitLocked()
86+
case '\r':
87+
// A bare CR is treated as a terminator too: the Actions runner
88+
// splits process output on CR as well as LF, so leaving one inline
89+
// would let engine output start a line the runner then parses.
90+
w.emitLocked()
91+
w.pendingCR = true
92+
default:
93+
w.buf = append(w.buf, b)
94+
if len(w.buf) >= maxPassthroughLineBytes {
95+
w.emitLocked()
96+
}
97+
}
98+
}
99+
return len(p), nil
100+
}
101+
102+
func (w *passthroughWriter) emitLocked() {
103+
line := neutralizeWorkflowCommand(string(w.buf))
104+
w.buf = w.buf[:0]
105+
_, _ = io.WriteString(w.framer.dst, PassthroughPrefix+line+"\n")
106+
}
107+
108+
func (w *passthroughWriter) flushLocked() {
109+
if len(w.buf) > 0 {
110+
w.emitLocked()
111+
}
112+
}
113+
114+
// neutralizeWorkflowCommand defuses a line that tries to open a GitHub Actions
115+
// workflow command (::error::, ::stop-commands::, ...). The frame prefix already
116+
// prevents the runner from seeing one, since a framed line no longer starts with
117+
// "::"; this is defense in depth for consumers that strip the prefix, and it
118+
// keeps the intent visible in the log rather than silently dropping it.
119+
func neutralizeWorkflowCommand(line string) string {
120+
trimmed := strings.TrimLeft(line, " \t")
121+
if !strings.HasPrefix(trimmed, "::") {
122+
return line
123+
}
124+
indent := line[:len(line)-len(trimmed)]
125+
return indent + "%3A%3A" + trimmed[2:]
126+
}

0 commit comments

Comments
 (0)