diff --git a/FIX_PLAN.md b/FIX_PLAN.md new file mode 100644 index 0000000..23787f6 --- /dev/null +++ b/FIX_PLAN.md @@ -0,0 +1,236 @@ +# FIX_PLAN.md + +Remediation plan for bugs and security vulnerabilities identified in the +security/correctness audit (2026-06-10). Findings are ordered by priority. +Each entry lists the location, the flaw, the concrete fix, and verification +steps. Check items off as they land. + +## Priority order (fix these first) + +1. **#1** — Telegram document path traversal (remote arbitrary-write/RCE). ✅ Fixed +2. **#3** — Fail-open Telegram authorization (amplifies #1). ✅ Fixed +3. **#2** — `trustAll` never resets within a run (one click unlocks the turn). ✅ Fixed +4. **#4 / #5** — WS auth + serve-mode approval deadlock. + +> Status: #1, #2, and #3 are implemented and tested in this PR. The remaining +> items are open. #4 and #6 involve a behavior/default change (WS token) and are +> left for maintainer direction. + +--- + +## 1. Path traversal → arbitrary file write via Telegram document filename +- **Status:** ✅ Fixed — `sanitizeDocName` strips directory components; tests in + `download_test.go` (`TestSanitizeDocName`, `TestDownloadDocument_NoTraversal`). +- **Severity:** Critical +- **Location:** `internal/telegram/download.go:156-164` +- **Flaw:** `DownloadDocument` uses the sender-controlled `Document.FileName` + directly: `safeName := fileName; localPath := filepath.Join(dir, safeName)`. + `filepath.Join` collapses `../`, so a document named + `../../../home//.odek/config.json` (or `../../.ssh/authorized_keys`) + writes outside `~/.odek/media/`. Both path and contents are attacker- + controlled → RCE/persistence. Photo/voice paths are safe (hashed names). +- **Fix:** + - `safeName = filepath.Base(filepath.Clean(fileName))`. + - Reject if the result is empty, `.`, `..`, or still contains a path + separator; fall back to the generated `doc_` name. + - Confirm the final `localPath` is still within `dir` (defense in depth): + resolve both with `filepath.Abs` and check `strings.HasPrefix`. +- **Verify:** unit test in `internal/telegram` sending a `FileName` of + `../../evil.txt` and asserting the write stays under `~/.odek/media/`. + +## 2. Approval bypass: `trustAll` never resets within a run +- **Status:** ✅ Fixed — grant is now reset at the end of each iteration's tool + phase (not via `defer`). Regression test: + `TestBatchApprovalTrustAllNotLeakedAcrossIterations` in `loop_test.go`. +- **Severity:** High +- **Location:** `internal/loop/loop.go:885-891` +- **Flaw:** `defer ta.SetTrustAll(false)` sits inside the + `for i := 0; i < e.maxIter` loop, so it fires at function return, not at + iteration end. After one approved batch, all later dangerous ops auto-approve + for the rest of the turn (incl. destructive/blocked classes). +- **Fix:** scope the reset to the iteration — set `trustAll` true before tool + execution and `false` immediately after (e.g. wrap the phase in a closure, or + add an explicit `ta.SetTrustAll(false)` at the end of the iteration body). + Do not rely on `defer` inside the loop. +- **Verify:** test that a second dangerous tool call in a later iteration of the + same run still triggers `PromptCommand`. + +## 3. Fail-open Telegram authorization +- **Status:** ✅ Fixed — `ValidateConfig` now refuses to start with no allowlist + unless `ODEK_TELEGRAM_ALLOW_ALL=true` is set; `isAllowed` is fail-closed + (empty lists + no opt-in → deny); startup logs a loud warning when running + open. **Also closed a callback-query authorization bypass** found while + challenging the fix: `handleCallback` (inline-button presses) did not call + `isAllowed` — it now does, so callbacks are gated like messages. Tests: + `TestValidateConfig_noAllowlistFailsClosed`, `TestValidateConfig_allowAllOptIn`, + `TestConfigFromEnv_allowAll`, `TestIsAllowed_EmptyAllowlist` (now asserts deny), + `TestIsAllowed_EmptyAllowlistWithAllowAll`, `TestHandleCallback_RespectsAllowlist`, + `TestHandleUpdate_CallbackQueryNotAllowed` (now asserts deny). +- **Note (not fixed — pre-existing, lower priority):** in a *group* chat where + only `AllowedChats` is set, any group member can press another user's + approval/clarify buttons (callback passes the chat-level allowlist). Tightening + this requires binding an approval to the specific user who triggered the run — + out of scope for this PR. +- **Severity:** High +- **Location:** `internal/telegram/handler.go:505-533`; defaults in + `internal/telegram/config.go` +- **Flaw:** `isAllowed` returns `true` when both `AllowedChats` and + `AllowedUsers` are empty, and the default config leaves both empty with no + warning. Anyone who finds the bot gets full (possibly godmode) access. +- **Fix:** make the bot fail-closed — if no allow-list is configured, either + refuse to start (log a fatal/clear error) or run with tools disabled. + Emit a loud startup warning when the allow-list is empty. +- **Verify:** start the bot with no `ODEK_TELEGRAM_ALLOWED_*` env vars and + confirm it refuses / restricts rather than serving all users. + +## 4. WebSocket `/ws` (and `/api/*`) has no authentication +- **Severity:** High +- **Location:** `cmd/odek/serve.go:148,874-888` (and the `/api/*` handlers + ~904-1057) +- **Flaw:** the only gate is an Origin check that returns `nil` for empty + Origin, so any non-browser client (no Origin header) gets full agent control. + `/api/*` handlers have no Origin check at all. Safety depends solely on the + loopback bind; `--addr 0.0.0.0` removes it. +- **Fix:** + - Add a per-process bearer/session token required on the WS handshake and all + `/api/*` requests (generate on startup, print to the operator). + - Reject non-loopback `RemoteAddr` unless a token is explicitly configured. + - Apply the same origin/token gate to every `/api/*` handler. +- **Verify:** connect with `websocat` (no Origin) and confirm rejection without + a token; confirm `--addr 0.0.0.0` still requires the token. + +## 5. Web UI approvals deadlock and always time out +- **Severity:** High +- **Location:** `cmd/odek/serve.go:494-590`; `cmd/odek/wsapprover.go:153-184` +- **Flaw:** `handleWS` is the only goroutine reading the socket and calls + `RunWithMessages` synchronously. The approver blocks on `HandleResponse`, + which is only called from that same (now-blocked) receive loop, so + `approval_response` is never read → 60s timeout → denial. Approval UI in serve + mode is dead. +- **Fix:** read the WebSocket on a dedicated goroutine (channel-feed the receive + loop), or run the prompt on a separate goroutine from the socket reader so + responses can be processed while a prompt is in flight. +- **Verify:** in serve mode, trigger a dangerous tool and confirm the browser + approval is received and honored without timing out. + +## 6. `write_file`/`patch` `~/.odek/` carve-out enables privilege escalation +- **Severity:** High +- **Location:** `cmd/odek/file_tool.go:760-768`; + `internal/danger/classifier.go:177-186`; `patch` wiring in `cmd/odek/main.go` +- **Flaw:** `confineToCWD` allows any path under `~/.odek/`, and + `~/.odek/config.json` classifies as auto-allowed `LocalWrite`. A confined/ + untrusted sub-agent can rewrite its own config to disable the sandbox / enable + YOLO on the next run, or drop an auto-loaded `SKILL.md`. Shell rc files + (`~/.bashrc`, `~/.zshrc`, `~/.profile`) also fall through to `LocalWrite`. + `patch` is created without `restrictToCWD`. +- **Fix:** + - Classify `~/.odek/config.json`, `~/.odek/skills/`, and the shell rc/profile + files (and crontab paths) as `SystemWrite` (prompt/deny). + - Exclude those paths from the `confineToCWD` carve-out. + - Give `patch` the same `restrictToCWD` confinement as `write_file`. +- **Verify:** confined agent attempt to write `~/.odek/config.json` and + `~/.bashrc` must hit an approval/deny path, not auto-allow. + +## 7. Data race in `wsApprover` can fatally crash serve +- **Severity:** Medium +- **Location:** `cmd/odek/wsapprover.go:117,175` +- **Flaw:** `a.approveAll[cls] || a.trustAll` read unlocked at :117 while + `a.approveAll[cls] = true` writes unlocked at :175. Parallel tool goroutines + share one approver → concurrent map read/write is a fatal Go runtime error. +- **Fix:** guard all `approveAll`/`trustAll` access with the existing mutex + (mirror `internal/telegram/approver.go`, which locks correctly). +- **Verify:** run the serve approval path under `go test -race` with parallel + tool calls. + +## 8. SSRF: URL gate is string-only, no dial-time IP check +- **Severity:** Medium +- **Location:** `internal/danger/classifier.go:195-244`; + `cmd/odek/browser_tool.go`; `cmd/odek/perf_tools.go` +- **Flaw:** `ClassifyURL` inspects only the literal hostname. A domain whose A + record resolves to `169.254.169.254` / `10.x` / `192.168.x` classifies as + plain `NetworkEgress`, and the HTTP clients use a default transport with no + post-resolution IP guard → SSRF / DNS-rebinding to cloud metadata & internal + services when egress is allowed. +- **Fix:** install a custom `DialContext` on the browser/http_batch clients that + re-checks the resolved `net.IP` against loopback/private/link-local/ULA ranges + and refuses them. This also closes the redirect-hop and rebinding window. +- **Verify:** request a host that resolves to `169.254.169.254` and confirm the + dial is refused. + +## 9. Sub-agent results lost on >64KB NDJSON line, then full-timeout hang +- **Severity:** Medium +- **Location:** `cmd/odek/subagent_tool.go:247-285` +- **Flaw:** `runTask` reads child stdout with a default `bufio.Scanner` (64KB + token cap). Streamed `tool_call` events embedding full tool args (e.g. a large + `write_file`) exceed 64KB → `ErrTooLong` → reader stops → child blocks on a + full pipe → `cmd.Wait` hangs until the 120s timeout kills a task that actually + succeeded. +- **Fix:** call `scanner.Buffer(make([]byte, 0, 64*1024), maxCap)` with a large + cap, or switch to a `bufio.Reader` with `ReadString('\n')`. +- **Verify:** sub-agent task whose streamed event line exceeds 64KB returns its + result instead of timing out. + +## 10. `/new` deletes the per-chat mutex while a run holds it +- **Severity:** High (correctness/data-integrity) +- **Location:** `cmd/odek/telegram.go:263` +- **Flaw:** `/new` runs `chatMu.Delete(chatID)` on the update loop, + unsynchronized with in-flight `handleChatMessage` goroutines. The next message + `LoadOrStore`s a fresh mutex and `TryLock`s it → two concurrent runs for the + same chat corrupt interleaved `sessionManager.Save` writes and clobber each + other's approver. +- **Fix:** do not delete the mutex on `/new` (a small per-chat leak is + acceptable), or only delete it while holding the lock and with no active run. +- **Verify:** send `/new` during an active run, then a message; confirm the two + runs do not execute concurrently. + +--- + +## Secondary findings (track, lower priority) + +- **Telegram bot token leaked into logs** — `internal/telegram/bot.go:117-120`. + `*url.Error` includes the token-bearing URL. Strip credentials from logged + URLs (log method only). +- **`currentPromptCancel` single global slot** — `cmd/odek/serve.go:37,581`. + `/api/cancel` cancels the wrong prompt with multiple tabs; A finishing clobbers + B's cancel. Track per-connection / per-prompt-ID. +- **`/stop` cannot interrupt a pending `clarify`** — `cmd/odek/telegram.go:1353`. + Circular wait until the 10-minute timeout. Make clarify observe `agentCtx`. +- **Sub-agent / panic edge cases in loop** — + `internal/loop/loop.go:927-945`: a panicked tool returns an empty result + (early `return` before `results[idx]` is set); the model gets no error. +- **UTF-8 byte-slicing in Telegram previews** — + `cmd/odek/telegram.go:305-327`. `taskPreview[:50]`/`[:80]` slice by byte and + can emit invalid UTF-8 (Telegram 400). Also `Messages[0]` can index an empty + slice and previews the system prompt. Use rune-aware truncation (see + `truncate()` in `subagent.go`). +- **`activeTaskWG.Add` races `gracefulRestart`'s `Wait`** — + `cmd/odek/telegram.go:1063-1076` vs `:879-961`. Invert to Add-first, + re-check-flag, Done+return on bail. +- **`serve --open` never opens a browser** — + `cmd/odek/serve.go:1104-1113`. `os.StartProcess` with a bare name does no PATH + lookup. Use `exec.LookPath` / `exec.Command`. +- **SearXNG `secret_key` weak sentinel** — + `docker/searxng/settings.yml` ships `ultrasecretkey` with a matching compose + default. Mitigated (no host port) but should hard-fail on the sentinel. +- **Sessions dir created `0755`** — `internal/session/session.go:71`. Tighten to + `0700` (files are already `0600`). +- **`ffprobe` leading-dash arg injection** — `cmd/odek/vision_tool.go:101-106`. + Low impact; add a `--` separator before positional paths. + +## Operational follow-up + +- **Rotate the credentials** in the working-tree `docker/.env` + (`ODEK_API_KEY`, `ODEK_TELEGRAM_BOT_TOKEN`) if this tree was ever shared, + imaged, or backed up. (`docker/.env` is gitignored and was never committed — + only `.env.example` is tracked.) + +## Confirmed NOT vulnerable (do not re-investigate) + +- `shell.go` — argv-form `exec.Command` + token-based fail-closed danger + classifier. +- File tools — `O_NOFOLLOW` + atomic temp-write/rename (symlink TOCTOU closed). +- `subagent_key.go` — API key handed off via an unlinked FD, not env/args. +- `mcp.go` — MCP servers come from local resolved config, not remote-supplied + definitions; tool descriptions are injection-scanned and outputs wrapped. +- `docker/.env` — gitignored, never committed (see operational follow-up for the + on-disk copy). diff --git a/cmd/odek/telegram.go b/cmd/odek/telegram.go index 4dbbb70..4771805 100644 --- a/cmd/odek/telegram.go +++ b/cmd/odek/telegram.go @@ -159,10 +159,18 @@ func telegramCmd(args []string) error { // 8. Set handler config from cfg. handler.Config = telegram.HandlerConfig{ - AllowedChats: cfg.AllowedChats, - BotUsername: cfg.BotUsername, - MaxMsgLength: cfg.MaxMsgLength, - AllowedUsers: cfg.AllowedUsers, + AllowedChats: cfg.AllowedChats, + BotUsername: cfg.BotUsername, + MaxMsgLength: cfg.MaxMsgLength, + AllowedUsers: cfg.AllowedUsers, + AllowAllUsers: cfg.AllowAllUsers, + } + + // Loud warning when running without an allowlist (only reachable when the + // operator explicitly set ODEK_TELEGRAM_ALLOW_ALL; ValidateConfig blocks + // the accidental case). + if cfg.AllowAllUsers && len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 { + handlerLog.Warn("telegram bot is running with NO allowlist — ANY user can drive the agent (ODEK_TELEGRAM_ALLOW_ALL=true)") } // 9. Build system prompt: explicit override > IDENTITY.md > compiled default diff --git a/docker/.env.example b/docker/.env.example index 8665ea0..3a42c7f 100644 --- a/docker/.env.example +++ b/docker/.env.example @@ -41,9 +41,13 @@ GIT_COMMITTER_EMAIL=you@example.com # ── Telegram bot (only for the telegram-* profiles) ────────────────────── # Get a token from @BotFather; restrict to YOUR chat/user id. +# REQUIRED: set at least one allowlist below — the bot refuses to start with +# none (fail-closed). Any Telegram user who finds an open bot can drive the +# agent and its shell/file tools. # ODEK_TELEGRAM_BOT_TOKEN=123456:ABC-your-bot-token # ODEK_TELEGRAM_ALLOWED_CHATS=11111111 # comma-separated chat IDs # ODEK_TELEGRAM_ALLOWED_USERS=11111111 # comma-separated user IDs (optional) +# ODEK_TELEGRAM_ALLOW_ALL=true # DANGER: run with NO allowlist (any user). Not recommended. # Chat ID that `odek run --deliver` and scheduled reminders send to. # Required for Telegram delivery; usually your own chat ID from ALLOWED_CHATS. # ODEK_TELEGRAM_DEFAULT_CHAT_ID=11111111 diff --git a/docs/SECURITY.md b/docs/SECURITY.md index bc2407d..942c1dd 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -217,7 +217,7 @@ odek audit | jq … # programmatic triage `AllowedChats` and `AllowedUsers` are loaded from `[telegram]` config or `ODEK_TELEGRAM_ALLOWED_CHATS` / `…_USERS` env vars. When non-empty, the handler rejects any update whose `chat.id` / `user.id` is not in the list **before** any tool call is reached. Denied attempts are logged so you can notice scanning. -If you run the bot at all, **set the allowlist**. The bot is the only internet-exposed surface, and the agent it drives has full host access. +Authorization is **fail-closed**: if neither allowlist is configured, the bot refuses to start (`ValidateConfig` returns an error), and at runtime `isAllowed` denies every update. The bot is the only internet-exposed surface and the agent it drives has full host access, so an empty allowlist must never silently mean "allow everyone". To intentionally run an open bot you must explicitly set `ODEK_TELEGRAM_ALLOW_ALL=true`, which logs a loud warning at startup. ### 13. Identity anchoring (legacy) diff --git a/docs/TELEGRAM.md b/docs/TELEGRAM.md index b944c4a..6a6cfb8 100644 --- a/docs/TELEGRAM.md +++ b/docs/TELEGRAM.md @@ -31,8 +31,9 @@ All configuration flows through `TelegramConfig` and can be set via environment | Variable | Field | Default | |---|---|---| | `ODEK_TELEGRAM_BOT_TOKEN` | Token | — (required) | -| `ODEK_TELEGRAM_ALLOWED_CHATS` | AllowedChats | all | -| `ODEK_TELEGRAM_ALLOWED_USERS` | AllowedUsers | all | +| `ODEK_TELEGRAM_ALLOWED_CHATS` | AllowedChats | — (see below) | +| `ODEK_TELEGRAM_ALLOWED_USERS` | AllowedUsers | — (see below) | +| `ODEK_TELEGRAM_ALLOW_ALL` | AllowAllUsers | false | | `ODEK_TELEGRAM_BOT_USERNAME` | BotUsername | — | | `ODEK_TELEGRAM_POLL_INTERVAL` | PollInterval | 1s | | `ODEK_TELEGRAM_POLL_TIMEOUT` | PollTimeout | 30s | @@ -181,10 +182,19 @@ All callbacks return a response string (may be empty) and an error. The `Handle` ### Access Control `HandlerConfig` supports: -- **AllowedChats** — restrict to specific chat IDs (empty = allow all) -- **AllowedUsers** — restrict to specific user IDs (empty = allow all) +- **AllowedChats** — restrict to specific chat IDs +- **AllowedUsers** — restrict to specific user IDs +- **AllowAllUsers** — explicit opt-in to run with no allowlist (see below) - **BotUsername** — for `@mention` detection in groups +> **Fail-closed authorization.** The bot **refuses to start** if neither +> `ODEK_TELEGRAM_ALLOWED_CHATS` nor `ODEK_TELEGRAM_ALLOWED_USERS` is set, so an +> open bot — where any Telegram user could drive the agent and its shell/file +> tools — can never be deployed by accident. To intentionally run an open bot, +> set `ODEK_TELEGRAM_ALLOW_ALL=true` (logged as a loud warning at startup). At +> runtime, with both allowlists empty and `AllowAllUsers` unset, every update is +> denied. + ### Inline Keyboards 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`. diff --git a/internal/loop/loop.go b/internal/loop/loop.go index 9d89e72..b316bbb 100644 --- a/internal/loop/loop.go +++ b/internal/loop/loop.go @@ -529,6 +529,15 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // Reset per-session tool error tracking e.maxConsecutiveToolErrors = make(map[string]int) + // Backstop: clear any batch trustAll grant when this run returns, even on + // early exit or panic, so it never leaks into a later prompt that reuses + // the same approver (wsApprover is per-connection, TelegramApprover is + // per-chat). The per-iteration reset below is the primary mechanism; this + // defer only covers abnormal exits mid-iteration. + if ta, ok := e.approver.(interface{ SetTrustAll(bool) }); ok { + defer ta.SetTrustAll(false) + } + for i := 0; i < e.maxIter; i++ { select { case <-ctx.Done(): @@ -832,6 +841,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // anything. If approved, the approver's trustAll flag is set so // individual tool-level PromptCommand calls auto-pass. batchDenied := false + // trustAllApprover holds the approver whose trustAll flag was set by an + // approved batch this iteration, so it can be reset once this + // iteration's tools have executed. It MUST be reset per-iteration + // (not via defer, which only fires when runLoop returns) — otherwise a + // single approved batch would auto-approve every dangerous tool for the + // remainder of the run. + var trustAllApprover interface{ SetTrustAll(bool) } if e.approver != nil && len(result.ToolCalls) > 1 { // Classify each tool call and filter to only those needing approval. type riskyCall struct { @@ -887,7 +903,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ if !batchDenied { if ta, ok := e.approver.(interface{ SetTrustAll(bool) }); ok { ta.SetTrustAll(true) - defer ta.SetTrustAll(false) + trustAllApprover = ta } } } @@ -951,6 +967,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ } } + // Reset the batch trustAll grant now that this iteration's tools have + // run. Scoping it to the iteration (rather than deferring to function + // return) ensures a later iteration's dangerous tools still prompt. + if trustAllApprover != nil { + trustAllApprover.SetTrustAll(false) + } + // Phase 3: process results in order (render, compress, append to messages) const maxOutput = 4096 for i, tc := range result.ToolCalls { diff --git a/internal/loop/loop_test.go b/internal/loop/loop_test.go index 4063a96..5c735ad 100644 --- a/internal/loop/loop_test.go +++ b/internal/loop/loop_test.go @@ -1626,18 +1626,100 @@ func TestBatchApprovalApproved(t *testing.T) { approver.mu.Lock() cc := approver.callCount - approxAfter := approver.trustAll // should be false (reset by defer) + approxAfter := approver.trustAll // should be false (reset after the iteration) approver.mu.Unlock() if cc != 1 { t.Errorf("approver.PromptCommand called %d times, want 1 (batch gate only)", cc) } if approxAfter { - t.Error("SetTrustAll should have been reset to false after iteration (defer)") + t.Error("SetTrustAll should have been reset to false after the iteration") } t.Logf("Batch approved: PromptCommand called %d time(s), elapsed=%v ✓", cc, elapsed) } +// recordingApprover mimics the real approvers (wsApprover / TelegramApprover): +// PromptCommand auto-approves while trustAll is set, and it records the +// trustAll state observed at each call so a test can assert the batch grant +// does not leak across loop iterations. +type recordingApprover struct { + mu sync.Mutex + trustAll bool + trustAtCall []bool +} + +func (a *recordingApprover) PromptCommand(cls danger.RiskClass, cmd, description string) error { + a.mu.Lock() + defer a.mu.Unlock() + a.trustAtCall = append(a.trustAtCall, a.trustAll) + return nil // always approve so the run proceeds to the next iteration +} + +func (a *recordingApprover) PromptOperation(op danger.ToolOperation) error { + return a.PromptCommand(op.Risk, op.Resource, op.Name) +} + +func (a *recordingApprover) SetTrustAll(enabled bool) { + a.mu.Lock() + defer a.mu.Unlock() + a.trustAll = enabled +} + +// TestBatchApprovalTrustAllNotLeakedAcrossIterations is a regression test for a +// bug where an approved batch's trustAll grant was reset via `defer` inside the +// iteration loop — so it only fired at function return, leaving trustAll set for +// every subsequent iteration. That let a single approved batch auto-approve all +// later dangerous tools in the run. The batch gate must see trustAll=false at +// the start of every iteration. +func TestBatchApprovalTrustAllNotLeakedAcrossIterations(t *testing.T) { + approver := &recordingApprover{} + + shellTool := &mockTool{name: "shell", result: "done"} + registry := tool.NewRegistry([]tool.Tool{shellTool}) + + // Server returns a 2-tool destructive batch on the first TWO LLM calls + // (two iterations that both hit the batch gate), then a final answer. + callNum := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callNum++ + if callNum <= 2 { + fmt.Fprint(w, `{"choices":[{"message":{"content":"","tool_calls":[`+ + `{"id":"call_a","function":{"name":"shell","arguments":"{\"command\":\"rm -rf /etc/a\"}"}},`+ + `{"id":"call_b","function":{"name":"shell","arguments":"{\"command\":\"rm -rf /etc/b\"}"}}`+ + `]}}]}`) + return + } + fmt.Fprintf(w, `{"choices":[{"message":{"content":%q}}]}`, "done") + })) + defer server.Close() + + client := llm.New(server.URL, "sk-test", "test-model", "", 0, 0) + engine := New(client, registry, 10, "", nil, 0) + engine.SetApprover(approver) + engine.SetMaxToolParallel(2) + allow := "allow" + engine.SetDangerousConfig(&danger.DangerousConfig{ + DefaultAction: &allow, + Classes: map[danger.RiskClass]danger.Action{danger.Destructive: danger.Prompt}, + }) + + if _, err := engine.Run(context.Background(), "two dangerous batches"); err != nil { + t.Fatalf("Run() error: %v", err) + } + + approver.mu.Lock() + defer approver.mu.Unlock() + if len(approver.trustAtCall) != 2 { + t.Fatalf("batch gate invoked %d times, want 2 (one per iteration): %v", + len(approver.trustAtCall), approver.trustAtCall) + } + for i, observed := range approver.trustAtCall { + if observed { + t.Errorf("iteration %d: batch gate saw trustAll=true (grant leaked from a prior batch)", i+1) + } + } +} + // TestBatchApprovalSingleTool verifies that single tool calls skip the batch gate. func TestBatchApprovalSingleTool(t *testing.T) { approver := &mockApprover{approved: false} // would deny, but should never be called diff --git a/internal/telegram/config.go b/internal/telegram/config.go index 7ee13db..753b089 100644 --- a/internal/telegram/config.go +++ b/internal/telegram/config.go @@ -24,6 +24,11 @@ type TelegramConfig struct { LogLevel string `json:"log_level"` // "debug","info","warn","error" (default "info") LogFile string `json:"log_file"` // path or empty for stderr DefaultChatID int64 `json:"default_chat_id"` // for --deliver and cron delivery + // AllowAllUsers must be explicitly set to true to run the bot with NO + // allowlist (any Telegram user may drive the agent). Without it, an empty + // AllowedChats + AllowedUsers is a fatal misconfiguration (fail-closed) so + // an open bot can never be deployed by accident. Env: ODEK_TELEGRAM_ALLOW_ALL. + AllowAllUsers bool `json:"allow_all_users"` } // DefaultConfig returns a TelegramConfig with sensible defaults. @@ -52,6 +57,9 @@ func ConfigFromEnv(base TelegramConfig) TelegramConfig { if v := os.Getenv("ODEK_TELEGRAM_ALLOWED_USERS"); v != "" { cfg.AllowedUsers = parseInt64List(v) } + if v := os.Getenv("ODEK_TELEGRAM_ALLOW_ALL"); v != "" { + cfg.AllowAllUsers = parseBool(v) + } if v := os.Getenv("ODEK_TELEGRAM_BOT_USERNAME"); v != "" { cfg.BotUsername = v } @@ -127,9 +135,29 @@ func ValidateConfig(cfg TelegramConfig) error { if cfg.AgentTimeout < 0 { return fmt.Errorf("telegram: agent_timeout_seconds must be >= 0, got %d", cfg.AgentTimeout) } + // Fail-closed authorization: refuse to start an unrestricted bot unless the + // operator explicitly opts in. An empty allowlist would otherwise let ANY + // Telegram user drive the agent (and its shell/file tools). Checked last so + // field-level validation errors surface first. + if len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 && !cfg.AllowAllUsers { + return fmt.Errorf("telegram: no allowlist configured — set ODEK_TELEGRAM_ALLOWED_CHATS " + + "and/or ODEK_TELEGRAM_ALLOWED_USERS to restrict access, or set " + + "ODEK_TELEGRAM_ALLOW_ALL=true to explicitly run an open bot (NOT recommended)") + } return nil } +// parseBool parses common truthy string values ("true", "1", "yes", "on", +// case-insensitive). Anything else is false. +func parseBool(s string) bool { + switch strings.ToLower(strings.TrimSpace(s)) { + case "true", "1", "yes", "on": + return true + default: + return false + } +} + // parseInt64List parses a comma-separated string of integers into a slice of int64. func parseInt64List(s string) []int64 { parts := splitAndTrim(s) diff --git a/internal/telegram/config_test.go b/internal/telegram/config_test.go index 02be936..070625a 100644 --- a/internal/telegram/config_test.go +++ b/internal/telegram/config_test.go @@ -366,11 +366,12 @@ func TestConfigFromEnv_multipleOverrides(t *testing.T) { func TestValidateConfig_valid(t *testing.T) { cfg := TelegramConfig{ - Token: "valid:token", - PollInterval: 1, - PollTimeout: 30, - MaxMsgLength: 4096, - SessionTTL: 24, + Token: "valid:token", + PollInterval: 1, + PollTimeout: 30, + MaxMsgLength: 4096, + SessionTTL: 24, + AllowAllUsers: true, } if err := ValidateConfig(cfg); err != nil { t.Errorf("ValidateConfig() = %v, want nil", err) @@ -384,6 +385,7 @@ func TestValidateConfig_validMinimal(t *testing.T) { PollTimeout: 1, MaxMsgLength: 1, SessionTTL: 1, + AllowedUsers: []int64{123}, // satisfies fail-closed check via allowlist } if err := ValidateConfig(cfg); err != nil { t.Errorf("ValidateConfig() = %v, want nil", err) @@ -391,15 +393,72 @@ func TestValidateConfig_validMinimal(t *testing.T) { } func TestValidateConfig_validMaximums(t *testing.T) { + cfg := TelegramConfig{ + Token: "abc", + PollInterval: 999, + PollTimeout: 60, + MaxMsgLength: 4096, + SessionTTL: 8760, + AllowAllUsers: true, + } + if err := ValidateConfig(cfg); err != nil { + t.Errorf("ValidateConfig() = %v, want nil", err) + } +} + +func TestValidateConfig_noAllowlistFailsClosed(t *testing.T) { + // A config with a token but no allowlist and no explicit opt-in must be + // rejected (fail-closed) so an open bot can't be deployed by accident. + cfg := TelegramConfig{ + Token: "abc", + PollInterval: 1, + PollTimeout: 30, + MaxMsgLength: 4096, + SessionTTL: 24, + } + err := ValidateConfig(cfg) + if err == nil { + t.Fatal("ValidateConfig() = nil, want error about missing allowlist") + } + if !contains(err.Error(), "no allowlist configured") { + t.Errorf("unexpected error: %q", err.Error()) + } +} + +func TestValidateConfig_allowAllOptIn(t *testing.T) { + cfg := TelegramConfig{ + Token: "abc", + PollInterval: 1, + PollTimeout: 30, + MaxMsgLength: 4096, + SessionTTL: 24, + AllowAllUsers: true, + } + if err := ValidateConfig(cfg); err != nil { + t.Errorf("ValidateConfig() with AllowAllUsers = %v, want nil", err) + } +} + +func TestValidateConfig_allowlistSatisfiesCheck(t *testing.T) { cfg := TelegramConfig{ Token: "abc", - PollInterval: 999, - PollTimeout: 60, + PollInterval: 1, + PollTimeout: 30, MaxMsgLength: 4096, - SessionTTL: 8760, + SessionTTL: 24, + AllowedChats: []int64{42}, } if err := ValidateConfig(cfg); err != nil { - t.Errorf("ValidateConfig() = %v, want nil", err) + t.Errorf("ValidateConfig() with allowlist = %v, want nil", err) + } +} + +func TestConfigFromEnv_allowAll(t *testing.T) { + unsetAllEnvVars(t) + t.Setenv("ODEK_TELEGRAM_ALLOW_ALL", "true") + cfg := ConfigFromEnv(DefaultConfig()) + if !cfg.AllowAllUsers { + t.Error("ConfigFromEnv did not set AllowAllUsers from ODEK_TELEGRAM_ALLOW_ALL=true") } } @@ -732,11 +791,12 @@ func splitAt(s string, sep byte) string { // validConfig returns a TelegramConfig that passes ValidateConfig. func validConfig() TelegramConfig { return TelegramConfig{ - Token: "test:token", - PollInterval: 1, - PollTimeout: 30, - MaxMsgLength: 4096, - SessionTTL: 24, + Token: "test:token", + PollInterval: 1, + PollTimeout: 30, + MaxMsgLength: 4096, + SessionTTL: 24, + AllowAllUsers: true, // satisfy fail-closed authorization check } } diff --git a/internal/telegram/download.go b/internal/telegram/download.go index 82105e3..95e2905 100644 --- a/internal/telegram/download.go +++ b/internal/telegram/download.go @@ -153,14 +153,7 @@ func DownloadDocument(bot *Bot, fileID, fileName string) (string, error) { } // Use original filename or generate one from file ID. - safeName := fileName - if safeName == "" { - ext := filepath.Ext(f.FilePath) - if ext == "" { - ext = ".bin" - } - safeName = "doc_" + fileID[:min(16, len(fileID))] + ext - } + safeName := sanitizeDocName(fileName, fileID, f.FilePath) localPath := filepath.Join(dir, safeName) if err := os.WriteFile(localPath, data, 0600); err != nil { @@ -170,6 +163,26 @@ func DownloadDocument(bot *Bot, fileID, fileName string) (string, error) { return localPath, nil } +// sanitizeDocName derives a safe, single-component filename for a downloaded +// Telegram document. The supplied fileName comes from attacker-controlled +// Document metadata, so directory components must be stripped to prevent path +// traversal outside the media directory (e.g. "../../.ssh/authorized_keys"). +// When the name is empty or degenerate ("", ".", "..") a deterministic name is +// generated from the file ID instead. +func sanitizeDocName(fileName, fileID, filePath string) string { + base := filepath.Base(filepath.Clean(fileName)) + // filepath.Base never returns a path containing a separator, but it can + // still yield "." (empty/relative input) or ".." which must be rejected. + if base == "" || base == "." || base == ".." || base == string(filepath.Separator) { + ext := filepath.Ext(filePath) + if ext == "" { + ext = ".bin" + } + return "doc_" + fileID[:min(16, len(fileID))] + ext + } + return base +} + // ── Media Cleanup ────────────────────────────────────────────────────────── // CleanupMedia removes media files older than maxAge from the downloaded diff --git a/internal/telegram/download_test.go b/internal/telegram/download_test.go index ee306ad..fe57c56 100644 --- a/internal/telegram/download_test.go +++ b/internal/telegram/download_test.go @@ -492,3 +492,69 @@ func TestDownloadPhoto_MediaDirError(t *testing.T) { t.Fatal("expected error when MediaDir fails") } } + +// ── Test sanitizeDocName (path-traversal hardening) ───────────────────────── + +func TestSanitizeDocName(t *testing.T) { + const fileID = "AgACAgIAAxkBAAIabcdef0123456789" + tests := []struct { + name string + fileName string + want string + }{ + {"plain name", "report.pdf", "report.pdf"}, + {"parent traversal", "../../.ssh/authorized_keys", "authorized_keys"}, + {"absolute path", "/etc/passwd", "passwd"}, + {"deep traversal to config", "../../../home/user/.odek/config.json", "config.json"}, + {"nested subdir", "a/b/c.txt", "c.txt"}, + {"dot-dot only", "..", "doc_" + fileID[:16] + ".bin"}, + {"empty falls back", "", "doc_" + fileID[:16] + ".bin"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeDocName(tt.fileName, fileID, "documents/file.bin") + if got != tt.want { + t.Errorf("sanitizeDocName(%q) = %q, want %q", tt.fileName, got, tt.want) + } + // The result must never contain a path separator. + if strings.ContainsRune(got, filepath.Separator) { + t.Errorf("sanitizeDocName(%q) = %q contains a path separator", tt.fileName, got) + } + }) + } +} + +// TestDownloadDocument_NoTraversal verifies the full download path keeps a +// malicious filename confined to the media directory. +func TestDownloadDocument_NoTraversal(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.String(), "getFile"): + fmt.Fprintf(w, `{"ok":true,"result":{"file_id":"doc123","file_path":"documents/file.bin"}}`) + case strings.HasSuffix(r.URL.String(), "documents/file.bin"): + w.Write([]byte("pwned")) + default: + http.Error(w, `{"ok":false}`, 404) + } + } + ts := httptest.NewServer(http.HandlerFunc(handler)) + defer ts.Close() + bot := testBot(t, ts) + + dir, err := MediaDir() + if err != nil { + t.Fatalf("MediaDir: %v", err) + } + defer os.RemoveAll(dir) + + localPath, err := DownloadDocument(bot, "doc123", "../../../evil.txt") + if err != nil { + t.Fatalf("DownloadDocument: %v", err) + } + if filepath.Dir(localPath) != filepath.Clean(dir) { + t.Errorf("file written to %q, want it inside %q", localPath, dir) + } + if _, err := os.Stat(filepath.Join(filepath.Dir(dir), "evil.txt")); err == nil { + t.Error("traversal succeeded: evil.txt written outside media dir") + } +} diff --git a/internal/telegram/e2e_test.go b/internal/telegram/e2e_test.go index ed2c9de..9544e3a 100644 --- a/internal/telegram/e2e_test.go +++ b/internal/telegram/e2e_test.go @@ -73,6 +73,7 @@ func TestE2E_FullTextMessageFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 // don't wait in tests handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test // Set up the text message callback. var ( @@ -198,6 +199,7 @@ func TestE2E_FullCommandFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test var ( capturedChatID int64 @@ -328,6 +330,7 @@ func TestE2E_FullCallbackFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test var ( capturedChatID int64 @@ -446,6 +449,7 @@ func TestE2E_PollThenHandlerFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test // Track how many text messages and their content. var ( @@ -614,6 +618,7 @@ func TestE2E_MediaFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test // OnTextMessage returns a MEDIA:photo response. handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) { @@ -715,6 +720,7 @@ func TestE2E_VoiceMediaFlow(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) { return "MEDIA:voice:" + tmpPath, nil @@ -804,6 +810,7 @@ func TestE2E_PollEmptyThenMessage(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test var messagesReceived []string handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) { @@ -925,6 +932,7 @@ func TestE2E_InlineKeyboardResponse(t *testing.T) { poller := NewPoller(bot) poller.Timeout = 0 handler := NewHandler(bot) + handler.Config.AllowAllUsers = true // routing test handler.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { return "Here are your options:", nil diff --git a/internal/telegram/handler.go b/internal/telegram/handler.go index cb24a17..125a796 100644 --- a/internal/telegram/handler.go +++ b/internal/telegram/handler.go @@ -11,10 +11,14 @@ import ( // HandlerConfig controls which messages the Handler processes. type HandlerConfig struct { - AllowedChats []int64 // empty = allow all + AllowedChats []int64 // restricts by chat ID; empty + AllowAllUsers required to allow any BotUsername string // for @mention detection in groups (without @) MaxMsgLength int // default: 4096 - AllowedUsers []int64 // empty = allow all users + AllowedUsers []int64 // restricts by user ID; empty + AllowAllUsers required to allow any + // AllowAllUsers must be true to permit access when BOTH allowlists are + // empty. Default false = fail-closed (deny everyone) so an unconfigured + // handler never silently allows all users. See ValidateConfig. + AllowAllUsers bool } // ─── Handler ────────────────────────────────────────────────────────────── @@ -330,6 +334,18 @@ func (h *Handler) handleCallback(cq *CallbackQuery) { return } + // Apply the same allowlist as messages. Callback queries (inline-button + // presses) otherwise bypass authorization — they can drive per-chat + // approval/clarify state and trigger outbound API calls. Without a From + // user, treat the user ID as 0 (matches no AllowedUsers entry). + var userID int64 + if cq.From != nil { + userID = cq.From.ID + } + if !h.isAllowed(cq.Message.Chat.ID, userID) { + return + } + // Route approval callbacks to the per-chat TelegramApprover. if a := h.GetApprover(cq.Message.Chat.ID); a != nil && a.HandleCallback(cq.Data) { // Show a toast acknowledging the user's choice. @@ -501,8 +517,15 @@ func (h *Handler) sendChunk(chatID int64, chunk string, replyToMessageID int) { // ─── Access Control ─────────────────────────────────────────────────────── // isAllowed checks if the given chat and user are allowed to interact. -// Returns true if both pass (or their respective allowlists are empty). +// When both allowlists are empty, access is denied unless AllowAllUsers was +// explicitly enabled (fail-closed). When a list is non-empty, the corresponding +// ID must appear in it. func (h *Handler) isAllowed(chatID int64, userID int64) bool { + // Fail-closed: with no allowlist at all, deny unless explicitly opted in. + if len(h.Config.AllowedChats) == 0 && len(h.Config.AllowedUsers) == 0 { + return h.Config.AllowAllUsers + } + if len(h.Config.AllowedChats) > 0 { found := false for _, id := range h.Config.AllowedChats { diff --git a/internal/telegram/handler_document_test.go b/internal/telegram/handler_document_test.go index e4931d0..de9ffdc 100644 --- a/internal/telegram/handler_document_test.go +++ b/internal/telegram/handler_document_test.go @@ -73,6 +73,7 @@ func TestHandleUpdate_Document(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnDocumentMessage = func(chatID int64, messageID int, fileID string, fileName string) (string, error) { capturedFileID = fileID diff --git a/internal/telegram/handler_edited_test.go b/internal/telegram/handler_edited_test.go index 7e1feb2..6653bf2 100644 --- a/internal/telegram/handler_edited_test.go +++ b/internal/telegram/handler_edited_test.go @@ -16,6 +16,7 @@ func TestHandleUpdate_EditedMessage(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) { capturedChatID = chatID capturedText = text @@ -59,6 +60,7 @@ func TestHandleUpdate_EditedMessageWithCommand(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { capturedCmd = cmd capturedArgs = args diff --git a/internal/telegram/handler_error_test.go b/internal/telegram/handler_error_test.go index 25bf097..9af506f 100644 --- a/internal/telegram/handler_error_test.go +++ b/internal/telegram/handler_error_test.go @@ -14,6 +14,7 @@ func TestHandleCommand_ErrorSentToUser(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { return "", fmt.Errorf("simulated command failure: %s", cmd) @@ -57,6 +58,7 @@ func TestHandleCallback_ErrorSentToUser(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCallbackQuery = func(chatID int64, data string) (string, error) { return "", fmt.Errorf("simulated callback failure: %s", data) @@ -99,6 +101,7 @@ func TestHandleCommand_ErrorNotSentOnSuccess(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { return "ok response", nil diff --git a/internal/telegram/handler_recover_test.go b/internal/telegram/handler_recover_test.go index 2c7dc4f..fc2e98f 100644 --- a/internal/telegram/handler_recover_test.go +++ b/internal/telegram/handler_recover_test.go @@ -12,6 +12,7 @@ func TestHandleUpdate_RecoverFromPanic(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test // Set up a text handler that panics. var panicCaught atomic.Bool @@ -62,6 +63,7 @@ func TestHandleUpdate_RecoverFromPanicCallback(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCallbackQuery = func(chatID int64, data string) (string, error) { panic("simulated callback panic") @@ -108,6 +110,7 @@ func TestHandleUpdate_RecoverFromPanicCommand(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test h.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { panic("simulated command panic") diff --git a/internal/telegram/handler_test.go b/internal/telegram/handler_test.go index c1ce942..fad5cac 100644 --- a/internal/telegram/handler_test.go +++ b/internal/telegram/handler_test.go @@ -202,6 +202,7 @@ func TestHandleUpdate_TextMessage(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* h.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) { capturedChatID = chatID capturedMessageID = messageID @@ -241,6 +242,7 @@ func TestHandleUpdate_CallbackQuery(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // callback routing test h.OnCallbackQuery = func(chatID int64, data string) (string, error) { capturedChatID = chatID capturedCallbackID = data @@ -279,6 +281,7 @@ func TestHandleUpdate_Command(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* h.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) { capturedChatID = chatID capturedCmd = cmd @@ -320,6 +323,7 @@ func TestHandleUpdate_VoiceMessage(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* h.OnVoiceMessage = func(chatID int64, messageID int, fileID string) (string, error) { capturedChatID = chatID capturedFileID = fileID @@ -359,6 +363,7 @@ func TestHandleUpdate_PhotoMessage(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* h.OnPhotoMessage = func(chatID int64, messageID int, fileIDs []string, caption string) (string, error) { capturedChatID = chatID capturedFileIDs = fileIDs @@ -726,12 +731,66 @@ func TestIsAllowed_EmptyAllowlist(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) - // Both AllowedChats and AllowedUsers are empty → allow all. + // Fail-closed: both allowlists empty and AllowAllUsers not set → deny. + if h.isAllowed(999, 888) { + t.Error("isAllowed(999, 888) = true, want false (fail-closed, no opt-in)") + } + if h.isAllowed(0, 0) { + t.Error("isAllowed(0, 0) = true, want false (fail-closed, no opt-in)") + } +} + +func TestIsAllowed_EmptyAllowlistWithAllowAll(t *testing.T) { + ts := testServer(t, nil) + defer ts.Close() + bot := testBot(t, ts) + h := NewHandler(bot) + h.Config.AllowAllUsers = true + // Explicit opt-in: empty allowlists + AllowAllUsers → allow anyone. if !h.isAllowed(999, 888) { - t.Error("isAllowed(999, 888) = false, want true (empty allowlists)") + t.Error("isAllowed(999, 888) = false, want true (AllowAllUsers opt-in)") } if !h.isAllowed(0, 0) { - t.Error("isAllowed(0, 0) = false, want true (empty allowlists)") + t.Error("isAllowed(0, 0) = false, want true (AllowAllUsers opt-in)") + } +} + +// TestHandleCallback_RespectsAllowlist verifies callback queries (inline-button +// presses) are gated by the same allowlist as messages — a non-allowlisted +// chat must not reach OnCallbackQuery. +func TestHandleCallback_RespectsAllowlist(t *testing.T) { + ts := testServer(t, nil) + defer ts.Close() + bot := testBot(t, ts) + h := NewHandler(bot) + h.Config.AllowedChats = []int64{100} // only chat 100 allowed + + var called bool + h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + called = true + return "", nil + } + + // Callback from a NON-allowlisted chat → must be dropped. + h.handleCallback(&CallbackQuery{ + ID: "cb1", + From: &User{ID: 999}, + Message: &Message{Chat: &Chat{ID: 999}}, + Data: "clarify:yes", + }) + if called { + t.Error("OnCallbackQuery was called for a non-allowlisted chat (callback bypassed authorization)") + } + + // Callback from the allowlisted chat → must be processed. + h.handleCallback(&CallbackQuery{ + ID: "cb2", + From: &User{ID: 1}, + Message: &Message{Chat: &Chat{ID: 100}}, + Data: "clarify:yes", + }) + if !called { + t.Error("OnCallbackQuery was NOT called for an allowlisted chat") } } @@ -1190,6 +1249,7 @@ func TestHandleUpdate_OnErrorCalled(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* h.OnTextMessage = func(_ int64, _ int, _ string) (string, error) { return "", assertError("simulated error") } @@ -1372,6 +1432,7 @@ func TestHandler_HandleCallback_RouteToApprover(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // callback routing test chatID := int64(789) approver := NewTelegramApprover(bot, chatID) @@ -1438,6 +1499,7 @@ func TestHandler_HandleCallback_ApproverAnswerError(t *testing.T) { log: NewNopLogger(), } h := NewHandler(bot) + h.Config.AllowAllUsers = true // callback routing test chatID := int64(789) approver := NewTelegramApprover(bot, chatID) @@ -1482,6 +1544,7 @@ func TestHandler_HandleCallback_FallbackToOnCallbackQuery(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // callback routing test // No approver registered. var ( @@ -1570,6 +1633,7 @@ func TestHandler_HandleMessage_OnErrorCalledOnVoiceFailure(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* chatID := int64(333) expectedErr := assertError("voice processing failed") @@ -1611,6 +1675,7 @@ func TestHandler_HandleMessage_OnErrorCalledOnPhotoFailure(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* chatID := int64(555) expectedErr := assertError("photo processing failed") @@ -1650,6 +1715,7 @@ func TestHandler_HandleMessage_OnErrorCalledOnTextFailure(t *testing.T) { defer ts.Close() bot := testBot(t, ts) h := NewHandler(bot) + h.Config.AllowAllUsers = true // routing test — access control covered by TestIsAllowed_* chatID := int64(777) expectedErr := assertError("text processing failed") @@ -1749,10 +1815,9 @@ func TestHandleUpdate_CallbackQueryNotAllowed(t *testing.T) { return "", nil } - // Note: handleCallback does NOT check isAllowed — it only routes. - // The isAllowed check is only in handleMessage. - // So callback queries from any user are processed regardless. - // This is the current behavior — document it. + // Callback queries are gated by the same allowlist as messages: a press + // from a user not in AllowedUsers must be dropped before reaching + // OnCallbackQuery (otherwise inline buttons bypass authorization). upd := Update{ ID: 40, CallbackQuery: &CallbackQuery{ @@ -1767,9 +1832,8 @@ func TestHandleUpdate_CallbackQueryNotAllowed(t *testing.T) { h.HandleUpdate(upd) - // Callback queries are currently processed without isAllowed check - if !called { - t.Error("OnCallbackQuery was not called for user 999 (callback queries may not check isAllowed)") + if called { + t.Error("OnCallbackQuery was called for non-allowlisted user 999 (callback bypassed isAllowed)") } }