From 1e65f4b6e5a797be24aa4f1d38d2f042fb9a0e91 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sun, 28 Jun 2026 13:29:45 -0400 Subject: [PATCH] =?UTF-8?q?fix(run):=20sanitize=20input-file=20names=20?= =?UTF-8?q?=E2=80=94=20path-traversal=20+=20prompt-injection=20hardening?= =?UTF-8?q?=20(gadfly=20#18)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full swarm (5-6 models) flagged that stageInputFiles passed the untrusted attachment filename straight to StageInputFile and inlined it into the [ATTACHED FILES]/`/workspace/` descriptor with no sanitization — a path the byte-cap already treats as a trust boundary. A name like ../../etc/passwd or an absolute/drive path could escape the host store or the sandbox workspace, and newlines in the name/mime could inject text into the prompt block. - sanitizeName: strips control chars/newlines, then reduces to a base name (path.Base after backslash-normalization) so ../, nested dirs, and absolute / drive paths all collapse to their last element; "attachment" fallback for empty/"."/"..". Applied BEFORE staging AND inlining. - sanitizeField: strips control chars from MimeType (also inlined verbatim). - maxInputFiles (32) count cap — defense-in-depth vs a flood of tiny files, independent of the per-file byte cap. Tests: sanitizeName table (traversal/absolute/backslash/control/fallback, + no-separator invariant); traversal staged+described under the base name only; oversize skip; count-cap truncation. Full suite green (-race). Co-Authored-By: Claude Opus 4.8 (1M context) --- run/input_files.go | 62 ++++++++++++++++++++++++++++++------ run/input_files_test.go | 69 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 9 deletions(-) diff --git a/run/input_files.go b/run/input_files.go index f358517..bad3815 100644 --- a/run/input_files.go +++ b/run/input_files.go @@ -6,6 +6,7 @@ import ( "log/slog" "path" "strings" + "unicode" "gitea.stevedudenhoeffer.com/steve/executus/tool" ) @@ -16,6 +17,10 @@ import ( // directly must not write an unbounded blob to the host file store. const maxInputFileBytes = 50_000_000 +// maxInputFiles bounds how many attachments a single run stages, independent of +// the per-file byte cap — defense-in-depth against a flood of tiny files. +const maxInputFiles = 32 + // stageInputFiles persists each non-image input attachment into the host file // store (Ports.InputFiles) under run scope and appends a descriptor block to the // prompt so the agent knows the file_ids it can pass to a worker tool. The bytes @@ -30,12 +35,19 @@ func (e *Executor) stageInputFiles(ctx context.Context, runID, agentID string, f if e.cfg.Ports.InputFiles == nil || len(files) == 0 { return prompt } + // Count cap: bound how many attachments one run can stage, independent of the + // per-file byte cap (defense-in-depth against a flood of tiny files). + if len(files) > maxInputFiles { + slog.Warn("run: too many input files, truncating", + "agent", agentID, "run_id", runID, "count", len(files), "cap", maxInputFiles) + files = files[:maxInputFiles] + } - type staged struct { + type stagedFile struct { name, mime, fileID string size int } - var ok []staged + var staged []stagedFile seenNames := make(map[string]int, len(files)) for _, f := range files { if len(f.Data) == 0 { @@ -49,19 +61,22 @@ func (e *Executor) stageInputFiles(ctx context.Context, runID, agentID string, f "size", len(f.Data), "cap", maxInputFileBytes) continue } - // Disambiguate colliding base names so two attachments with the same - // name don't both map to /workspace/ (the second would clobber the - // first when the agent writes them via code_exec). - name := uniqueName(f.Name, seenNames) + // Reduce the untrusted filename to a safe base name BEFORE staging or + // inlining: strips ../ and absolute-path components (so it can't escape + // the host store or /workspace/) and drops control chars/newlines + // (so a crafted name can't inject text into the descriptor block below). + // Then disambiguate colliding base names so two attachments don't both map + // to /workspace/ (the second would clobber the first). + name := uniqueName(sanitizeName(f.Name), seenNames) fileID, err := e.cfg.Ports.InputFiles.StageInputFile(ctx, runID, agentID, name, f.MimeType, f.Data) if err != nil { slog.Warn("run: failed to stage input file", "agent", agentID, "run_id", runID, "name", name, "error", err) continue } - ok = append(ok, staged{name: name, mime: f.MimeType, fileID: fileID, size: len(f.Data)}) + staged = append(staged, stagedFile{name: name, mime: sanitizeField(f.MimeType), fileID: fileID, size: len(f.Data)}) } - if len(ok) == 0 { + if len(staged) == 0 { return prompt } @@ -71,7 +86,7 @@ func (e *Executor) stageInputFiles(ctx context.Context, runID, agentID string, f b.WriteString("To work with one, call the code_exec tool with a files_in entry — e.g. ") b.WriteString(`files_in: [{"name": "", "file_id": ""}]`) b.WriteString(" — which writes it to /workspace/ inside the Python sandbox. You may also pass a file_id to any other tool that accepts one.\n") - for _, s := range ok { + for _, s := range staged { fmt.Fprintf(&b, "- %s (%s, %s) → file_id: %s\n", s.name, s.mime, humanizeBytes(s.size), s.fileID) } @@ -81,6 +96,35 @@ func (e *Executor) stageInputFiles(ctx context.Context, runID, agentID string, f return prompt + "\n\n" + b.String() } +// sanitizeName reduces an untrusted attachment filename to a safe base name. It +// drops control characters / newlines (which would otherwise let a crafted name +// inject text into the [ATTACHED FILES] descriptor) and strips every directory +// component — defeating ../ traversal, nested dirs, and absolute / drive paths +// both in the host file store and at /workspace/. Returns "attachment" +// when nothing usable remains (empty, ".", ".."). +func sanitizeName(name string) string { + name = sanitizeField(name) + // Normalize backslashes so a Windows-style path also reduces to its base. + base := path.Base(strings.ReplaceAll(name, `\`, "/")) + base = strings.TrimSpace(base) + if base == "" || base == "." || base == ".." { + return "attachment" + } + return base +} + +// sanitizeField strips control characters (incl. newlines/tabs) from a value +// that gets inlined verbatim into the prompt descriptor, so it can't break out +// of its line or inject instructions. +func sanitizeField(s string) string { + return strings.Map(func(r rune) rune { + if r == '\n' || r == '\r' || r == '\t' || unicode.IsControl(r) { + return -1 + } + return r + }, s) +} + // uniqueName returns name unchanged the first time it's seen, then name-2, // name-3, … (suffix inserted before the extension) on repeats, recording each // result in seen so later collisions keep counting up. diff --git a/run/input_files_test.go b/run/input_files_test.go index 0b0be0d..77777e4 100644 --- a/run/input_files_test.go +++ b/run/input_files_test.go @@ -117,3 +117,72 @@ func TestStageInputFilesSkipsBad(t *testing.T) { t.Errorf("save error should drop the file and leave the prompt, got %q", out) } } + +// TestStageInputFilesOversize: a file past the byte cap is skipped (prompt +// unchanged), exercising the size guard directly. +func TestStageInputFilesOversize(t *testing.T) { + st := &stagerFunc{} + ex := newStagerExecutor(st) + big := make([]byte, maxInputFileBytes+1) + out := ex.stageInputFiles(context.Background(), "r", "a", + []tool.InputFile{{Name: "huge.bin", Data: big}}, "p") + if out != "p" || len(st.staged) != 0 { + t.Errorf("oversized file should be skipped: out=%q staged=%d", out, len(st.staged)) + } +} + +// TestStageInputFilesCountCap: more than maxInputFiles attachments are truncated +// to the cap. +func TestStageInputFilesCountCap(t *testing.T) { + st := &stagerFunc{} + ex := newStagerExecutor(st) + files := make([]tool.InputFile, maxInputFiles+5) + for i := range files { + files[i] = tool.InputFile{Name: "f.bin", Data: []byte("x")} + } + ex.stageInputFiles(context.Background(), "r", "a", files, "p") + if len(st.staged) != maxInputFiles { + t.Errorf("count cap: staged %d, want %d", len(st.staged), maxInputFiles) + } +} + +// TestSanitizeName: traversal + absolute + control-char filenames are reduced to +// a safe base name (no path separators, no newlines), with a fallback. +func TestSanitizeName(t *testing.T) { + cases := map[string]string{ + "../../etc/passwd": "passwd", + "/etc/cron.d/x": "x", + `..\..\windows\sys`: "sys", + "clip.mp3": "clip.mp3", + "": "attachment", + "..": "attachment", + ".": "attachment", + "evil\n- injected": "evil- injected", + "a/b/c.wav": "c.wav", + } + for in, want := range cases { + if got := sanitizeName(in); got != want { + t.Errorf("sanitizeName(%q) = %q, want %q", in, got, want) + } + // A sanitized name must never carry a path separator or newline. + got := sanitizeName(in) + if strings.ContainsAny(got, "/\\\n\r") { + t.Errorf("sanitizeName(%q) = %q still contains a separator/newline", in, got) + } + } +} + +// TestStageInputFilesSanitizesTraversal: a traversal filename is staged AND +// described under its safe base name only. +func TestStageInputFilesSanitizesTraversal(t *testing.T) { + st := &stagerFunc{} + ex := newStagerExecutor(st) + out := ex.stageInputFiles(context.Background(), "r", "a", + []tool.InputFile{{Name: "../../../etc/passwd", MimeType: "text/plain", Data: []byte("x")}}, "go") + if len(st.staged) != 1 || st.staged[0].name != "passwd" { + t.Fatalf("staged name = %+v, want passwd", st.staged) + } + if strings.Contains(out, "..") || strings.Contains(out, "/etc/") { + t.Errorf("descriptor leaked the traversal path:\n%s", out) + } +}