Skip to content

Commit c8e97af

Browse files
committed
fix: Resource.Search root traversal + Telegram media path allowlist
Finding #12 — Resource.Search root traversal: - Add validateSearchQuery to reject queries containing .., path separators, or absolute paths. - Migrate walkAndMatch from filepath.Walk to filepath.WalkDir. - Skip symlinked directories/files during traversal. - Verify every match is withinRoot before returning it. - Add regression tests for traversal and symlink skipping. Finding #13 — Telegram media upload path traversal: - Add internal/telegram/media_path.go with ResolveMediaPath, which restricts outbound media to cwd, ~/.odek/media, and os.TempDir(). - Reject symlinks and paths outside the allowlist. - Apply ResolveMediaPath in internal/telegram/handler.go sendMedia, cmd/odek/telegram.go sendTelegramMedia, and internal/tool/send_message.go. - send_message file argument must be absolute and pass allowlist checks. - Add regression tests for allowed/rejected/symlink paths. - Update docs/TELEGRAM.md, docs/SECURITY.md, and AGENTS.md.
1 parent e187f8d commit c8e97af

11 files changed

Lines changed: 511 additions & 45 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
116116
- **@-resource / --ctx prompt wrapping** (`cmd/odek/refs.go`, `cmd/odek/serve.go`) — content resolved from `@file` references and `--ctx` files is wrapped as untrusted before being inserted into the prompt.
117117
- **Config file size cap** (`internal/config/loader.go`) — `~/.odek/config.json` and `./odek.json` are rejected if larger than 5 MiB to prevent OOM from a malicious or broken config at startup.
118118
- **Resource resolver size cap** (`internal/resource/resource.go`) — `@-resource` file loads are capped at 1 MiB to prevent OOM from `@hugefile` references.
119-
- **Resource resolver symlink hardening** (`internal/resource/resource.go`) — `FileResolver.Search` uses `os.Lstat` (not `os.Stat`) for search-result metadata, so symlinks cannot leak the size of arbitrary targets outside the workspace.
119+
- **Resource resolver search hardening** (`internal/resource/resource.go`) — `FileResolver.Search` rejects queries containing `..`, path separators, or absolute components before joining them with the workspace root, and uses `filepath.WalkDir` so directory symlinks are not followed during recursive autocomplete. `os.Lstat` (not `os.Stat`) is used for search-result metadata, so symlinks cannot leak the size of arbitrary targets outside the workspace.
120120
- **Sub-agent summary cap + wrapping** (`cmd/odek/subagent_tool.go`) — each sub-agent result included in the `delegate_tasks` summary is truncated to 100 KiB to prevent memory DoS, and the final aggregated summary is wrapped as untrusted content so a compromised sub-agent cannot inject instructions into the parent context.
121121
- **Tree path wrapping** (`cmd/odek/perf_tools.go`) — the `tree` tool wraps every filesystem-derived path as untrusted content.
122122
- **head_tail output cap** (`cmd/odek/perf_tools.go`) — `head_tail` truncates returned lines so total content stays within 1 MiB, preventing multi-file/multi-line memory DoS.
@@ -138,6 +138,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
138138
- **Telegram singleton flock lock** (`cmd/odek/telegram.go` + `internal/flock`) — the Telegram bot now uses an advisory `flock` on `~/.odek/telegram.lock` instead of a PID file probed with signals. This removes the non-Linux path where a planted PID could cause odek to kill an arbitrary process.
139139
- **Telegram photo caption wrapping** (`cmd/odek/telegram.go`) — photo captions cross the Telegram trust boundary, so they are wrapped as untrusted content both when passed to the local vision model and when injected into the main agent's user message.
140140
- **`send_message` callback prefix restriction** (`internal/tool/send_message.go` + `cmd/odek/telegram.go`) — the `send_message` tool rejects any button whose `callback_data` starts with a reserved internal prefix (`apr:`, `den:`, `trs:`, `clarify:`, `skill_save:`, `skill_skip:`); only user-facing `cb:` callbacks are allowed. The Telegram sender closure validates again as defense-in-depth, preventing a forged approval or skill button.
141+
- **Telegram outbound media path allowlist** (`internal/telegram/media_path.go` + `internal/telegram/handler.go` + `internal/tool/send_message.go` + `cmd/odek/telegram.go`) — paths supplied to `MEDIA:...` prefixes or `send_message(file=...)` are resolved to an absolute path and verified against an allowlist (cwd, `~/.odek/media/`, system temp dir). `os.Lstat` rejects symlink final components and `filepath.EvalSymlinks` ensures the resolved path does not escape the allowlist, preventing prompt-injection-driven exfiltration of arbitrary files.
141142
- **Session ID entropy + session-scoped auth tokens** (`internal/session/session.go`, `cmd/odek/serve.go`) — session IDs now carry 128 bits of randomness (16 bytes / 32 hex chars); each session stores a 256-bit `AuthToken` required by `GET/DELETE/POST /api/sessions/<id>` and WebSocket session-resume messages via `X-Session-Token` header, `session_token` cookie, or `auth_token` WS field. Per-IP rate limiting (60/min) on session lookups adds a brute-force backstop.
142143
- **Skill/episode untrusted wrapper** (`internal/loop/loop.go` + `odek.go`) — skill context and retrieved session-episode context are passed through the caller-provided untrusted wrapper (the same nonce'd `<untrusted_content_*>` boundary used for tool output) before being injected into the model's system context. This prevents a compromised or tainted skill/episode from being treated as trusted system instructions.
143144
- **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.

cmd/odek/telegram.go

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1477,12 +1477,12 @@ func handleChatMessage(
14771477
SystemMessage: systemMessage,
14781478
UntrustedWrapper: wrapUntrusted,
14791479
RuntimeContext: odek.BuildRuntimeContext("telegram"),
1480-
InteractionMode: resolved.InteractionMode,
1481-
NoProjectFile: resolved.NoAgents,
1482-
Skills: skillsCfg,
1483-
Thinking: resolved.Thinking,
1484-
Tools: agentTools,
1485-
Renderer: rend,
1480+
InteractionMode: resolved.InteractionMode,
1481+
NoProjectFile: resolved.NoAgents,
1482+
Skills: skillsCfg,
1483+
Thinking: resolved.Thinking,
1484+
Tools: agentTools,
1485+
Renderer: rend,
14861486
ToolEventHandler: func(event string, name string, data string) {
14871487
// Enhance mode: send new messages with narrated descriptions.
14881488
if isEnhance {
@@ -2130,6 +2130,12 @@ func mediaTypeFromExt(path string) string {
21302130
// sendTelegramMedia sends a file as a Telegram media message with caption
21312131
// and optional inline keyboard. Detects the media type from file extension.
21322132
func sendTelegramMedia(bot *telegram.Bot, chatID int64, mediaType, path, caption string, buttons [][]map[string]string) error {
2133+
// Defense-in-depth: validate the path against the media allowlist.
2134+
resolved, err := telegram.ResolveMediaPath(path)
2135+
if err != nil {
2136+
return fmt.Errorf("telegram media: %w", err)
2137+
}
2138+
21332139
var replyMarkup *telegram.InlineKeyboardMarkup
21342140
if len(buttons) > 0 {
21352141
replyMarkup = buttonsToMarkup(buttons)
@@ -2140,13 +2146,13 @@ func sendTelegramMedia(bot *telegram.Bot, chatID int64, mediaType, path, caption
21402146
}
21412147
switch mediaType {
21422148
case "photo":
2143-
_, err := bot.SendPhoto(chatID, path, caption, opts)
2149+
_, err := bot.SendPhoto(chatID, resolved, caption, opts)
21442150
return err
21452151
case "voice":
2146-
_, err := bot.SendVoice(chatID, path, caption, opts)
2152+
_, err := bot.SendVoice(chatID, resolved, caption, opts)
21472153
return err
21482154
default:
2149-
_, err := bot.SendDocument(chatID, path, caption, opts)
2155+
_, err := bot.SendDocument(chatID, resolved, caption, opts)
21502156
return err
21512157
}
21522158
}

docs/SECURITY.md

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ The MCP wrapper guards a tool's **output**. The server-supplied tool **descripti
7474

7575
The model is instructed (via the default system prompt) to treat the wrapped region as data, not instructions. A model trained on prompt-injection resistance (Claude Sonnet 4.6+ does this well) honours the boundary. Older models or aggressively fine-tuned ones may not.
7676

77-
Two additional boundaries keep filesystem-derived metadata from leaking as "trusted" context. First, the `base64` tool wraps encoded output when reading from a file path, so even transformed filesystem bytes stay inside an untrusted boundary. Second, the `@`-resource resolver (`FileResolver.Search`) uses `os.Lstat` when building search-result metadata, which prevents a symlink inside the workspace from leaking the size (or other `stat` metadata) of an arbitrary target outside it.
77+
Two additional boundaries keep filesystem-derived metadata from leaking as "trusted" context. First, the `base64` tool wraps encoded output when reading from a file path, so even transformed filesystem bytes stay inside an untrusted boundary. Second, the `@`-resource resolver (`FileResolver.Search`) rejects queries containing `..`, path separators, or absolute components before joining them with the workspace root, and uses `filepath.WalkDir` (which does not follow symlinks) for recursive autocomplete; `os.Lstat` is used when building search-result metadata, which prevents a symlink inside the workspace from leaking the size (or other `stat` metadata) of an arbitrary target outside it.
7878

7979
### 3. Danger classifier (shell)
8080

@@ -324,7 +324,17 @@ The Telegram bot previously used a PID file at `~/.odek/telegram.pid` to enforce
324324

325325
The `send_message` tool lets the agent send inline keyboard buttons. Each button's `callback_data` is validated by the tool and again by the Telegram sender closure: any value that starts with a reserved internal prefix (`apr:`, `den:`, `trs:`, `clarify:`, `skill_save:`, `skill_skip:`) is rejected. Only user-facing `cb:` callbacks are allowed. This prevents a compromised or prompt-injected agent from presenting a button that, when clicked, would forge an approval decision or trigger a skill action.
326326

327-
### 23. Session ID entropy + session-scoped auth tokens
327+
### 23. Telegram outbound media path allowlist
328+
329+
When the agent emits `MEDIA:photo:/path`, `MEDIA:voice:/path`, `MEDIA:document:/path`, or `send_message` with a `file`, the path is validated by `internal/telegram.ResolveMediaPath` before upload. Only paths inside an allowed base directory are permitted:
330+
331+
- the current working directory,
332+
- `~/.odek/media/`, and
333+
- the system temporary directory.
334+
335+
The path is resolved to an absolute, cleaned form with `filepath.Abs`, symlinks are resolved with `filepath.EvalSymlinks`, and the final component is checked with `os.Lstat`. If the final component is a symlink, or if the resolved path escapes the allowlist, the upload is rejected. This closes the arbitrary-file-read/exfiltration vector where a prompt-injected agent asks the bot to send files such as `/home/user/.ssh/id_rsa`.
336+
337+
### 24. Session ID entropy + session-scoped auth tokens
328338

329339
`odek serve` session endpoints were previously protected only by localhost binding and a short, predictable session ID (`YYYYMMDD-` + 3 random bytes ≈ 16.7 M possibilities). A local attacker who obtained IDs from `GET /api/sessions` could brute-force `GET /api/sessions/<id>` to read transcripts.
330340

@@ -336,7 +346,7 @@ The defense has three layers:
336346

337347
Legacy sessions created before this defense have no `AuthToken`; the first access bootstraps one and returns it to the client, preserving backward compatibility without weakening protection for newly created sessions.
338348

339-
### 24. Skill and episode context wrapped as untrusted
349+
### 25. Skill and episode context wrapped as untrusted
340350

341351
Skill content and retrieved session episodes are externally-sourced data that cross the trust boundary. Before injecting them as `system` messages, the loop passes them through the same nonce'd `<untrusted_content_*>` wrapper used for tool output. The skill manager already gates `NeedsReview`/tainted skills, and the memory manager filters tainted episodes from search, but the wrapper provides defense-in-depth so a compromised skill or episode cannot pose as trusted system instructions.
342352

@@ -389,6 +399,7 @@ Defaults: `FrictionThreshold=3`, `FrictionWindow=60s`. To opt out (TTYApprover o
389399
| Local process brute-forces session IDs to read transcripts | 128-bit IDs + session-scoped auth tokens + per-IP rate limiting |
390400
| Telegram bot scanned by random user | Allowlist enforced before any tool call |
391401
| Agent sends fake approval/skill button via `send_message` | Reserved internal callback prefixes rejected; only `cb:` allowed |
402+
| Agent exfiltrates arbitrary file via Telegram media | Outbound paths restricted to cwd, `~/.odek/media/`, and temp dir; symlinks rejected |
392403
| Auto-saved skill auto-activates on next session | Provenance gate pins NeedsReview skills to Lazy |
393404
| Memory replays a previously-injected episode forever | Tainted episodes filtered from `Search` |
394405
| User reflex-approves a destructive class after many benign ones | Friction mode requires typed `approve` + 1.5 s pause |

docs/TELEGRAM.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,15 @@ All callbacks return a response string (may be empty) and an error. The `Handle`
206206

207207
The handler uses `sync.Map` for `TelegramApprover` instances, keyed by `chatID`. This allows the agent to send inline keyboard approval requests (yes/no) and receive responses via callback queries. The handler intercepts callback queries matching pending approval requests before dispatching to `OnCallbackQuery`.
208208

209+
### Outbound Media
210+
211+
The agent can send files back to the chat either by emitting a `MEDIA:` prefix in its final answer (`MEDIA:photo:/path`, `MEDIA:voice:/path`, `MEDIA:document:/path`) or by calling `send_message` with the `file` parameter. Before any upload, the path is validated by `internal/telegram.ResolveMediaPath`:
212+
213+
- Allowed directories: current working directory, `~/.odek/media/`, and the system temporary directory.
214+
- The path is resolved to an absolute, cleaned form and checked against the allowlist.
215+
- Symlinks are rejected: the final component is verified with `os.Lstat` and the resolved path must not escape the allowlist.
216+
- Files outside the allowlist (e.g. `/home/user/.ssh/id_rsa`) are refused, closing prompt-injection-driven exfiltration.
217+
209218
## Slash Commands (`commands.go`)
210219

211220
### Built-in Commands

internal/resource/resource.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,12 @@ func (f *FileResolver) Search(ctx context.Context, query string, limit int) ([]R
211211
return nil, nil
212212
}
213213

214+
// Reject traversal attempts before touching the filesystem. A query is a
215+
// bare filename/prefix for autocomplete, not a path expression.
216+
if err := validateSearchQuery(query); err != nil {
217+
return nil, err
218+
}
219+
214220
// Try exact match first
215221
pattern := filepath.Join(f.root, query)
216222
matches, err := filepath.Glob(pattern + "*")
@@ -223,11 +229,7 @@ func (f *FileResolver) Search(ctx context.Context, query string, limit int) ([]R
223229
matches = f.walkAndMatch(query)
224230
}
225231

226-
// Resolve the root once so every match can be confined to it. A query
227-
// such as "../../etc/passwd" makes filepath.Join above clean to a path
228-
// outside root, and filepath.Glob would then match files the workspace
229-
// must not expose. Skip any match that escapes root before touching the
230-
// filesystem (closes CodeQL "uncontrolled data in path expression").
232+
// Resolve the root once so every match can be confined to it.
231233
absRoot, err := filepath.Abs(f.root)
232234
if err != nil {
233235
return nil, nil
@@ -261,6 +263,21 @@ func (f *FileResolver) Search(ctx context.Context, query string, limit int) ([]R
261263
return resources, nil
262264
}
263265

266+
// validateSearchQuery rejects queries that could escape the configured root.
267+
// Search queries are autocomplete prefixes, not filesystem paths.
268+
func validateSearchQuery(query string) error {
269+
if filepath.IsAbs(query) {
270+
return fmt.Errorf("resource: search query must not be an absolute path")
271+
}
272+
if strings.Contains(query, "..") {
273+
return fmt.Errorf("resource: search query must not contain parent references")
274+
}
275+
if strings.ContainsAny(query, "/\\") {
276+
return fmt.Errorf("resource: search query must not contain path separators")
277+
}
278+
return nil
279+
}
280+
264281
func (f *FileResolver) Load(ctx context.Context, id string) (string, error) {
265282
// id is the path after @ (e.g. "src/main.go")
266283
target := filepath.Join(f.root, id)
@@ -312,17 +329,21 @@ func (f *FileResolver) walkAndMatch(searchTerm string) []string {
312329
base := f.root
313330

314331
var results []string
315-
filepath.Walk(base, func(path string, info os.FileInfo, err error) error {
332+
filepath.WalkDir(base, func(path string, d os.DirEntry, err error) error {
316333
if err != nil {
317334
return nil
318335
}
319336
// Skip symlinks — resource resolver uses O_NOFOLLOW on Load,
320-
// so symlinks are unreadable anyway.
321-
if info.Mode()&os.ModeSymlink != 0 {
337+
// so symlinks are unreadable anyway. Skip symlinked directories
338+
// entirely so traversal cannot follow them.
339+
if d.Type()&os.ModeSymlink != 0 {
340+
if d.IsDir() {
341+
return filepath.SkipDir
342+
}
322343
return nil
323344
}
324-
if info.IsDir() {
325-
if skipDir(info.Name()) {
345+
if d.IsDir() {
346+
if skipDir(d.Name()) {
326347
return filepath.SkipDir
327348
}
328349
return nil

internal/resource/resource_test.go

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,8 @@ func TestFileResolver_SearchRecursive(t *testing.T) {
224224

225225
func TestFileResolver_SearchOutsideRoot(t *testing.T) {
226226
// Parent holds a sentinel file; root is a subdirectory of it. A traversal
227-
// query must not surface metadata for files outside root.
227+
// query must be rejected outright rather than surface metadata for files
228+
// outside root.
228229
parent := t.TempDir()
229230
if err := os.WriteFile(filepath.Join(parent, "secret.txt"), []byte("top secret"), 0644); err != nil {
230231
t.Fatal(err)
@@ -235,13 +236,51 @@ func TestFileResolver_SearchOutsideRoot(t *testing.T) {
235236
}
236237
res := NewFileResolver(root)
237238

238-
results, err := res.Search(context.Background(), "../secret", 10)
239+
_, err := res.Search(context.Background(), "../secret", 10)
240+
if err == nil {
241+
t.Fatal("expected Search() to reject traversal query")
242+
}
243+
}
244+
245+
func TestFileResolver_Search_AbsoluteQueryRejected(t *testing.T) {
246+
dir := newTestDir(t)
247+
res := NewFileResolver(dir)
248+
249+
_, err := res.Search(context.Background(), "/etc/passwd", 10)
250+
if err == nil {
251+
t.Fatal("expected Search() to reject absolute query")
252+
}
253+
}
254+
255+
func TestFileResolver_Search_PathSeparatorQueryRejected(t *testing.T) {
256+
dir := newTestDir(t)
257+
res := NewFileResolver(dir)
258+
259+
_, err := res.Search(context.Background(), "subdir/deep", 10)
260+
if err == nil {
261+
t.Fatal("expected Search() to reject query containing path separators")
262+
}
263+
}
264+
265+
func TestFileResolver_Search_SymlinkedDirectoryNotFollowed(t *testing.T) {
266+
workspace := t.TempDir()
267+
outside := t.TempDir()
268+
if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0644); err != nil {
269+
t.Fatal(err)
270+
}
271+
link := filepath.Join(workspace, "link")
272+
if err := os.Symlink(outside, link); err != nil {
273+
t.Fatal(err)
274+
}
275+
276+
resolver := NewFileResolver(workspace)
277+
results, err := resolver.Search(context.Background(), "secret", 10)
239278
if err != nil {
240-
t.Fatalf("Search() error: %v", err)
279+
t.Fatalf("Search failed: %v", err)
241280
}
242281
for _, r := range results {
243-
if strings.Contains(r.Label, "secret") || strings.Contains(r.ID, "secret") {
244-
t.Fatalf("traversal query leaked file outside root: %+v", r)
282+
if strings.Contains(r.Label, "secret") {
283+
t.Fatalf("search followed symlinked directory: %+v", r)
245284
}
246285
}
247286
}

0 commit comments

Comments
 (0)