Skip to content

Commit 67e3da7

Browse files
committed
fix: next 5 security vulnerabilities (shell output, browser timeout, batch_patch, transcribe, tree width)
1 parent 9a1eaa6 commit 67e3da7

6 files changed

Lines changed: 264 additions & 10 deletions

File tree

AGENTS.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ System prompt is loaded by priority: `--system` flag > `~/.odek/IDENTITY.md` > c
9191
### Security Architecture
9292
Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECURITY.md](docs/SECURITY.md).
9393

94-
- **Untrusted-content wrapper** (`cmd/odek/untrusted.go`) — every tool whose output sources from outside the trust boundary (`browser`, `read_file`, `shell`, `search_files`, `multi_grep`, `transcribe`, `head_tail`, `diff`, `tr`, `sort`, `json_query`, any MCP tool) wraps results in `<untrusted_content_<nonce> source="...">…</untrusted_content_<nonce>>`. Per-call nonce defeats wrapper-escape via literal close tag.
94+
- **Untrusted-content wrapper** (`cmd/odek/untrusted.go`) — every tool whose output sources from outside the trust boundary (`browser`, `read_file`, `shell`, `search_files`, `multi_grep`, `transcribe`, `head_tail`, `diff`, `tr`, `sort`, `json_query`, `batch_patch`, any MCP tool) wraps results in `<untrusted_content_<nonce> source="...">…</untrusted_content_<nonce>>`. Per-call nonce defeats wrapper-escape via literal close tag.
9595
- **Audit log** (`cmd/odek/audit.go` + `internal/session/audit.go`) — every `wrapUntrusted` call records source + content-hash + turn into `<sessions>/audit/<id>.json`. After each turn a divergence heuristic flags `suspicious_divergence=true` when the agent ingested untrusted content AND its tool calls referenced resources the user did not mention. Inspect with `odek audit <session-id>` / `odek audit --list`.
9696
- **Memory taint** (`internal/memory/provenance.go`) — `EpisodeProvenance` tracks Untrusted/Sources/UserApproved. Tainted episodes are stored but `Search()` filters them out, so a one-shot injection cannot persist via the episode pipeline. User must explicitly promote.
9797
- **Skill provenance gate** (`internal/skills/loader.go` + `cache.go`) — `Skill.Provenance{Untrusted, Sources, NeedsReview}`. NeedsReview skills pin to Lazy regardless of `auto_load`. `odek skill promote <name>` clears the flag after user review.
@@ -103,7 +103,11 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
103103
- **REST API CSRF protection** (`cmd/odek/serve.go::requireLocalOrigin`) — state-changing HTTP endpoints (POST/PUT/PATCH/DELETE) require a localhost origin or no Origin header, and static responses set `X-Frame-Options: DENY` + `Content-Security-Policy: frame-ancestors 'none'` to block clickjacking.
104104
- **Browser history cap** (`cmd/odek/browser_tool.go`) — navigation history is capped at 50 snapshots to prevent memory DoS from repeated `browser_navigate` calls.
105105
- **Search result bounds** (`cmd/odek/file_tool.go`, `cmd/odek/perf_tools.go`) — `search_files` and `multi_grep` enforce a max match limit (500) and a total returned-content cap (1 MiB) to avoid unbounded result JSON.
106-
- **Perf-tool file-size cap** (`cmd/odek/perf_tools.go`) — `diff`, `base64`, `tr`, `sort`, and `json_query` reject files larger than 10 MiB to avoid loading multi-gigabyte files into memory.
106+
- **Perf-tool file-size cap** (`cmd/odek/perf_tools.go`) — `diff`, `base64`, `tr`, `sort`, `json_query`, and `batch_patch` reject files larger than 10 MiB to avoid loading multi-gigabyte files into memory.
107+
- **Shell output cap** (`cmd/odek/shell.go`, `cmd/odek/perf_tools.go`) — `shell` and `parallel_shell` cap captured stdout/stderr at 1 MiB per stream to prevent memory DoS from commands that dump huge files.
108+
- **Browser request timeout** (`cmd/odek/browser_tool.go`) — the browser HTTP client enforces a 30-second request timeout so a slow/malicious server cannot hang the agent turn.
109+
- **Transcribe input guard** (`cmd/odek/transcribe_tool.go`) — rejects audio files larger than 10 MiB and writes ffmpeg output to a temp file so it cannot clobber an existing `.wav` next to the source path.
110+
- **Tree width cap** (`cmd/odek/perf_tools.go`) — the `tree` tool limits each directory listing to 1,000 entries to avoid OOM from directories with millions of files.
107111
- **Serve sandbox default-on**`odek serve` enables `--sandbox` automatically unless `--no-sandbox` is passed.
108112
- **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.
109113

cmd/odek/browser_tool.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"regexp"
1010
"strings"
1111
"sync"
12+
"time"
1213

1314
"github.com/BackendStack21/odek/internal/danger"
1415
)
@@ -66,12 +67,17 @@ type browserTool struct {
6667
trustedClasses map[danger.RiskClass]bool
6768
}
6869

70+
// browserRequestTimeout bounds each browser HTTP request. Tests may lower it to
71+
// verify timeout behavior.
72+
var browserRequestTimeout = 30 * time.Second
73+
6974
func newBrowserTool(dc danger.DangerousConfig) *browserTool {
7075
t := &browserTool{
7176
state: &browserState{nextRef: 1},
7277
dangerousConfig: dc,
7378
}
7479
t.client = &http.Client{
80+
Timeout: browserRequestTimeout,
7581
CheckRedirect: t.checkRedirect,
7682
Transport: ssrfGuardedTransport(),
7783
}
@@ -171,6 +177,7 @@ func (t *browserTool) Call(argsJSON string) (string, error) {
171177
}
172178
if t.client == nil {
173179
t.client = &http.Client{
180+
Timeout: browserRequestTimeout,
174181
CheckRedirect: t.checkRedirect,
175182
Transport: ssrfGuardedTransport(),
176183
}

cmd/odek/next_security_vulnerabilities_test.go

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
package main
22

33
import (
4+
"context"
45
"fmt"
56
"net/http"
67
"net/http/httptest"
78
"os"
89
"path/filepath"
910
"strings"
1011
"testing"
12+
"time"
1113

1214
"github.com/BackendStack21/odek/internal/danger"
1315
)
@@ -316,3 +318,160 @@ func TestJsonQuery_WrapsStringValue(t *testing.T) {
316318
t.Fatalf("json_query string value should be wrapped in untrusted_content, got: %q", r.Value)
317319
}
318320
}
321+
322+
323+
// ── 6. Shell / parallel_shell must cap command output ────────────────────
324+
325+
func TestShell_CapsOutputSize(t *testing.T) {
326+
dir := t.TempDir()
327+
path := filepath.Join(dir, "huge.txt")
328+
os.WriteFile(path, []byte(strings.Repeat("x", 15*1024*1024)), 0644)
329+
330+
tool := &shellTool{}
331+
tool.SetContext(context.Background())
332+
result, err := tool.Call(fmt.Sprintf(`{"command":"cat %s","description":"read huge file"}`, path))
333+
if err != nil {
334+
t.Fatalf("Call() error: %v", err)
335+
}
336+
337+
body := unwrapUntrusted(result)
338+
if len(body) > 1024*1024+200 {
339+
t.Fatalf("shell returned %d bytes, expected cap near 1 MiB", len(body))
340+
}
341+
}
342+
343+
func TestParallelShell_CapsOutputSize(t *testing.T) {
344+
dir := t.TempDir()
345+
path := filepath.Join(dir, "huge.txt")
346+
os.WriteFile(path, []byte(strings.Repeat("x", 15*1024*1024)), 0644)
347+
348+
tool := &parallelShellTool{}
349+
result := callJSON(t, tool, fmt.Sprintf(`{"commands":[{"command":"cat %s"}]}`, path))
350+
var r struct {
351+
Results []struct {
352+
Stdout string `json:"stdout"`
353+
Stderr string `json:"stderr"`
354+
Error string `json:"error,omitempty"`
355+
} `json:"results"`
356+
}
357+
mustUnmarshal(t, result, &r)
358+
if len(r.Results) != 1 {
359+
t.Fatalf("expected 1 result, got %d", len(r.Results))
360+
}
361+
out := r.Results[0].Stdout + r.Results[0].Stderr
362+
if len(out) > 1024*1024+200 {
363+
t.Fatalf("parallel_shell returned %d bytes, expected cap near 1 MiB", len(out))
364+
}
365+
}
366+
367+
// ── 7. Browser must enforce an HTTP request timeout ──────────────────────
368+
369+
func TestBrowser_NavigateTimeout(t *testing.T) {
370+
orig := browserRequestTimeout
371+
browserRequestTimeout = 100 * time.Millisecond
372+
defer func() { browserRequestTimeout = orig }()
373+
374+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
375+
time.Sleep(300 * time.Millisecond)
376+
fmt.Fprint(w, "<html><body>page</body></html>")
377+
}))
378+
defer srv.Close()
379+
380+
tool := newBrowserTool(danger.DangerousConfig{})
381+
result := callJSON(t, tool, fmt.Sprintf(`{"action":"navigate","url":%q}`, srv.URL))
382+
var r struct {
383+
Error string `json:"error,omitempty"`
384+
}
385+
mustUnmarshal(t, result, &r)
386+
if r.Error == "" || !strings.Contains(strings.ToLower(r.Error), "timeout") {
387+
t.Fatalf("browser should time out on a slow server, got: %q", r.Error)
388+
}
389+
}
390+
391+
// ── 8. batch_patch must reject huge files and wrap diff output ───────────
392+
393+
func TestBatchPatch_RejectsHugeFile(t *testing.T) {
394+
dir := t.TempDir()
395+
path := filepath.Join(dir, "huge.txt")
396+
os.WriteFile(path, []byte(strings.Repeat("x", 15*1024*1024)), 0644)
397+
398+
tool := &batchPatchTool{}
399+
result := callJSON(t, tool, fmt.Sprintf(`{"patches":[{"path":%q,"old_string":"xxx","new_string":"yyy"}]}`, path))
400+
var r struct {
401+
Results []struct {
402+
Success bool `json:"success"`
403+
Error string `json:"error,omitempty"`
404+
} `json:"results"`
405+
}
406+
mustUnmarshal(t, result, &r)
407+
if len(r.Results) != 1 {
408+
t.Fatalf("expected 1 result, got %d", len(r.Results))
409+
}
410+
if r.Results[0].Success {
411+
t.Fatal("batch_patch should reject a 15 MiB file")
412+
}
413+
if r.Results[0].Error == "" {
414+
t.Fatal("batch_patch should return an error for a 15 MiB file")
415+
}
416+
}
417+
418+
func TestBatchPatch_WrapsDiff(t *testing.T) {
419+
dir := t.TempDir()
420+
path := filepath.Join(dir, "test.txt")
421+
os.WriteFile(path, []byte("hello world\n"), 0644)
422+
423+
tool := &batchPatchTool{}
424+
result := callJSON(t, tool, fmt.Sprintf(`{"patches":[{"path":%q,"old_string":"hello","new_string":"goodbye"}]}`, path))
425+
var r struct {
426+
Results []struct {
427+
Diff string `json:"diff"`
428+
} `json:"results"`
429+
}
430+
mustUnmarshal(t, result, &r)
431+
if len(r.Results) == 0 || !strings.HasPrefix(r.Results[0].Diff, "<untrusted_content_") {
432+
t.Fatalf("batch_patch diff should be wrapped in untrusted_content, got: %q", r.Results[0].Diff)
433+
}
434+
}
435+
436+
// ── 9. Transcribe must reject huge / symlinked audio inputs ──────────────
437+
438+
func TestTranscribe_RejectsHugeFile(t *testing.T) {
439+
dir := t.TempDir()
440+
path := filepath.Join(dir, "huge.ogg")
441+
os.WriteFile(path, make([]byte, 15*1024*1024), 0644)
442+
443+
tool := &transcribeTool{}
444+
result := callJSON(t, tool, fmt.Sprintf(`{"path":%q}`, path))
445+
var r struct {
446+
Error string `json:"error,omitempty"`
447+
}
448+
mustUnmarshal(t, result, &r)
449+
if !strings.Contains(r.Error, "too large") {
450+
t.Fatalf("transcribe should reject a 15 MiB file with a size error, got: %q", r.Error)
451+
}
452+
}
453+
454+
// ── 10. Tree must cap directory width ────────────────────────────────────
455+
456+
func TestTree_CapsDirectoryWidth(t *testing.T) {
457+
dir := t.TempDir()
458+
for i := 0; i < 1500; i++ {
459+
os.WriteFile(filepath.Join(dir, fmt.Sprintf("file%d.txt", i)), []byte("x"), 0644)
460+
}
461+
462+
tool := &treeTool{dangerousConfig: danger.DangerousConfig{}}
463+
result := callJSON(t, tool, fmt.Sprintf(`{"path":%q}`, dir))
464+
var r struct {
465+
Tree struct {
466+
Children []any `json:"children"`
467+
} `json:"tree"`
468+
Error string `json:"error,omitempty"`
469+
}
470+
mustUnmarshal(t, result, &r)
471+
if r.Error != "" {
472+
t.Fatalf("tree returned error: %s", r.Error)
473+
}
474+
if len(r.Tree.Children) > 1000 {
475+
t.Fatalf("tree did not cap directory width: got %d children", len(r.Tree.Children))
476+
}
477+
}

cmd/odek/perf_tools.go

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"bufio"
5+
"bytes"
56
"crypto/md5"
67
"crypto/sha1"
78
"crypto/sha256"
@@ -34,6 +35,10 @@ import (
3435
// multi-gigabyte logs or core dumps.
3536
const maxFileReadBytes = 10 << 20 // 10 MiB
3637

38+
// maxTreeEntries caps the number of children reported for a single directory
39+
// by the tree tool, preventing OOM from directories with millions of entries.
40+
const maxTreeEntries = 1000
41+
3742
// readFileNoFollow reads a file with O_NOFOLLOW (anti-symlink), rejecting files
3843
// larger than maxFileReadBytes to avoid unbounded memory consumption.
3944
func readFileNoFollow(path string) ([]byte, error) {
@@ -173,6 +178,20 @@ func (t *batchPatchTool) Call(argsJSON string) (result string, err error) {
173178
continue
174179
}
175180

181+
info, err := f.Stat()
182+
if err != nil {
183+
f.Close()
184+
entry.Error = fmt.Sprintf("cannot stat %q: %v", p.Path, err)
185+
results[idx] = entry
186+
continue
187+
}
188+
if info.Size() > maxFileReadBytes {
189+
f.Close()
190+
entry.Error = fmt.Sprintf("file too large (%d bytes, max %d)", info.Size(), maxFileReadBytes)
191+
results[idx] = entry
192+
continue
193+
}
194+
176195
var sb strings.Builder
177196
_, err = io.Copy(&sb, f)
178197
f.Close()
@@ -243,7 +262,7 @@ func (t *batchPatchTool) Call(argsJSON string) (result string, err error) {
243262
}
244263

245264
entry.Success = true
246-
entry.Diff = diff
265+
entry.Diff = wrapUntrusted("batch_patch:"+p.Path, diff)
247266
results[idx] = entry
248267
}
249268

@@ -380,9 +399,11 @@ func (t *parallelShellTool) runOne(cmd parallelShellCmd) parallelShellEntry {
380399
} else {
381400
shCmd = exec.Command("sh", "-c", cmd.Command)
382401
}
383-
var stdout, stderr strings.Builder
384-
shCmd.Stdout = &stdout
385-
shCmd.Stderr = &stderr
402+
var stdout, stderr bytes.Buffer
403+
outW := &limitWriter{buf: &stdout, limit: maxShellOutputBytes}
404+
errW := &limitWriter{buf: &stderr, limit: maxShellOutputBytes}
405+
shCmd.Stdout = outW
406+
shCmd.Stderr = errW
386407

387408
// Kill on timeout via goroutine, with mutex to avoid Process race
388409
var procMu sync.Mutex
@@ -1513,10 +1534,21 @@ func buildTree(root, path string, depth, maxDepth int, includeHidden bool) (tree
15131534
return entry, nil
15141535
}
15151536

1537+
totalEntries := len(entries)
1538+
truncated := false
1539+
if totalEntries > maxTreeEntries {
1540+
entries = entries[:maxTreeEntries]
1541+
truncated = true
1542+
}
1543+
15161544
sort.Slice(entries, func(i, j int) bool {
15171545
return entries[i].Name() < entries[j].Name()
15181546
})
15191547

1548+
if truncated {
1549+
entry.ErrMsg = fmt.Sprintf("directory truncated (%d entries shown, %d total)", maxTreeEntries, totalEntries)
1550+
}
1551+
15201552
entry.Children = make([]treeEntry, 0, len(entries))
15211553
for _, e := range entries {
15221554
if !includeHidden && strings.HasPrefix(e.Name(), ".") {

cmd/odek/shell.go

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,34 @@ import (
2222
// immediately regardless of this backstop.
2323
const defaultShellTimeout = 30 * time.Minute
2424

25+
// maxShellOutputBytes caps the stdout + stderr captured from a single shell
26+
// command to prevent memory DoS from commands that dump huge files.
27+
const maxShellOutputBytes = 1 << 20 // 1 MiB
28+
29+
// limitWriter wraps a bytes.Buffer and drops further writes once the total
30+
// size would exceed limit, recording that output was truncated.
31+
type limitWriter struct {
32+
buf *bytes.Buffer
33+
limit int
34+
truncated bool
35+
}
36+
37+
func (w *limitWriter) Write(p []byte) (int, error) {
38+
if w.truncated {
39+
return len(p), nil
40+
}
41+
if w.buf.Len()+len(p) > w.limit {
42+
w.truncated = true
43+
room := w.limit - w.buf.Len()
44+
if room > 0 {
45+
w.buf.Write(p[:room])
46+
}
47+
w.buf.WriteString("\n... [output truncated]")
48+
return len(p), nil
49+
}
50+
return w.buf.Write(p)
51+
}
52+
2553
// shellTool is odek's built-in tool that lets the agent run shell commands.
2654
//
2755
// This is the only built-in tool — it's enough for reading files, running
@@ -163,8 +191,10 @@ func (t *shellTool) Call(args string) (string, error) {
163191
cmd.WaitDelay = 3 * time.Second
164192

165193
var outBuf, errBuf bytes.Buffer
166-
cmd.Stdout = &outBuf
167-
cmd.Stderr = &errBuf
194+
outW := &limitWriter{buf: &outBuf, limit: maxShellOutputBytes}
195+
errW := &limitWriter{buf: &errBuf, limit: maxShellOutputBytes}
196+
cmd.Stdout = outW
197+
cmd.Stderr = errW
168198

169199
err := cmd.Run()
170200

0 commit comments

Comments
 (0)