diff --git a/cmd/odek/telegram.go b/cmd/odek/telegram.go index 4771805..f2ab145 100644 --- a/cmd/odek/telegram.go +++ b/cmd/odek/telegram.go @@ -169,7 +169,7 @@ func telegramCmd(args []string) error { // 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 { + if cfg.AllowAllUsers && !cfg.HasAllowlist() { handlerLog.Warn("telegram bot is running with NO allowlist — ANY user can drive the agent (ODEK_TELEGRAM_ALLOW_ALL=true)") } diff --git a/internal/loop/loop.go b/internal/loop/loop.go index b316bbb..adc49b7 100644 --- a/internal/loop/loop.go +++ b/internal/loop/loop.go @@ -520,6 +520,10 @@ func (e *Engine) RunWithMessages(ctx context.Context, messages []llm.Message) (s return e.runLoop(ctx, messages) } +// trustAllSetter is implemented by approvers (wsApprover, TelegramApprover) +// whose tool-level prompts auto-pass while a batch approval grant is active. +type trustAllSetter interface{ SetTrustAll(bool) } + // runLoop is the shared core of Run and RunWithMessages. // It runs the ReAct loop on the given messages and returns the final // answer plus the complete updated message history. @@ -534,7 +538,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // 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 { + if ta, ok := e.approver.(trustAllSetter); ok { defer ta.SetTrustAll(false) } @@ -847,7 +851,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // (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) } + var trustAllApprover trustAllSetter if e.approver != nil && len(result.ToolCalls) > 1 { // Classify each tool call and filter to only those needing approval. type riskyCall struct { @@ -901,7 +905,7 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ // Approved: set trustAll on the approver if supported, so // individual tool-level PromptCommand calls auto-pass. if !batchDenied { - if ta, ok := e.approver.(interface{ SetTrustAll(bool) }); ok { + if ta, ok := e.approver.(trustAllSetter); ok { ta.SetTrustAll(true) trustAllApprover = ta } @@ -939,12 +943,13 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ ctxTool.SetContext(ctx) } // Capture any panic from the tool so it does not kill the agent. - var toolPanicked bool + // The recovered message falls through to results[idx] like any + // other tool error, so the LLM sees it and the consecutive-error + // tracking counts it. func() { defer func() { if r := recover(); r != nil { output = fmt.Sprintf("error: tool %q panicked: %v", tcRef.Function.Name, r) - toolPanicked = true } }() res, err := t.Call(tcRef.Function.Arguments) @@ -954,9 +959,6 @@ func (e *Engine) runLoop(ctx context.Context, messages []llm.Message) (string, [ output = redact.RedactSecrets(res) } }() - if toolPanicked { - return - } } results[idx] = execResult{output: output} }(i, tc) diff --git a/internal/loop/loop_test.go b/internal/loop/loop_test.go index 5c735ad..546056f 100644 --- a/internal/loop/loop_test.go +++ b/internal/loop/loop_test.go @@ -10,6 +10,7 @@ import ( "sort" "strings" "sync" + "sync/atomic" "testing" "time" @@ -2007,8 +2008,12 @@ func (p *panicTool) Call(args string) (string, error) { } // TestToolPanic_DoesNotKillAgent verifies that a panicking tool call -// does not crash the agent. The agent should recover and continue. +// does not crash the agent. The agent should recover and continue, and the +// recovered panic message must reach the LLM as the tool result (regression: +// it used to be discarded, leaving an empty tool result). func TestToolPanic_DoesNotKillAgent(t *testing.T) { + var toolResult atomic.Value // string: content of the tool message the LLM saw + var callNum atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var body struct { Messages []llm.Message `json:"messages"` @@ -2017,11 +2022,16 @@ func TestToolPanic_DoesNotKillAgent(t *testing.T) { t.Fatal(err) } // First call: return tool calls with panic tool - if len(body.Messages) == 2 { + if callNum.Add(1) == 1 { fmt.Fprint(w, `{"choices":[{"index":0,"message":{"role":"assistant","content":"","tool_calls":[{"id":"call_1","type":"function","function":{"name":"panic_tool","arguments":"{}"}}]},"finish_reason":"tool_calls"}]}`) return } - // Second call (after tool panic): return content + // Second call (after tool panic): capture the tool result, return content + for _, m := range body.Messages { + if m.Role == "tool" { + toolResult.Store(m.Content) + } + } fmt.Fprint(w, `{"choices":[{"index":0,"message":{"role":"assistant","content":"Agent survived the panic!"},"finish_reason":"stop"}]}`) })) defer server.Close() @@ -2035,6 +2045,10 @@ func TestToolPanic_DoesNotKillAgent(t *testing.T) { if !strings.Contains(result, "Agent survived") { t.Errorf("agent result = %q, want 'Agent survived'", result) } + tr, _ := toolResult.Load().(string) + if !strings.Contains(tr, "panicked") { + t.Errorf("tool result sent to LLM = %q, want the recovered panic message (containing %q)", tr, "panicked") + } } // ── Context Length Error Detection ──────────────────────────────────── diff --git a/internal/telegram/config.go b/internal/telegram/config.go index 753b089..c085e59 100644 --- a/internal/telegram/config.go +++ b/internal/telegram/config.go @@ -58,7 +58,10 @@ func ConfigFromEnv(base TelegramConfig) TelegramConfig { cfg.AllowedUsers = parseInt64List(v) } if v := os.Getenv("ODEK_TELEGRAM_ALLOW_ALL"); v != "" { - cfg.AllowAllUsers = parseBool(v) + // strconv.ParseBool keeps the truthy grammar consistent with the rest + // of the config surface; a malformed value fails closed (false). + b, err := strconv.ParseBool(strings.TrimSpace(v)) + cfg.AllowAllUsers = err == nil && b } if v := os.Getenv("ODEK_TELEGRAM_BOT_USERNAME"); v != "" { cfg.BotUsername = v @@ -139,7 +142,7 @@ func ValidateConfig(cfg TelegramConfig) error { // 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 { + if !cfg.HasAllowlist() && !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)") @@ -147,15 +150,11 @@ func ValidateConfig(cfg TelegramConfig) error { 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 - } +// HasAllowlist reports whether at least one allowlist (chats or users) is +// configured. With no allowlist the bot is open to every Telegram user, which +// requires the explicit AllowAllUsers opt-in (see ValidateConfig). +func (c TelegramConfig) HasAllowlist() bool { + return len(c.AllowedChats) > 0 || len(c.AllowedUsers) > 0 } // parseInt64List parses a comma-separated string of integers into a slice of int64. diff --git a/internal/telegram/config_test.go b/internal/telegram/config_test.go index 070625a..b000c6f 100644 --- a/internal/telegram/config_test.go +++ b/internal/telegram/config_test.go @@ -462,6 +462,59 @@ func TestConfigFromEnv_allowAll(t *testing.T) { } } +// TestConfigFromEnv_allowAllStrictGrammar pins the strconv.ParseBool grammar: +// only its truthy forms enable the open-bot flag; anything else — including +// the formerly-accepted "yes"/"on" and malformed values — fails closed. +func TestConfigFromEnv_allowAllStrictGrammar(t *testing.T) { + tests := []struct { + value string + want bool + }{ + {"true", true}, + {"TRUE", true}, + {"1", true}, + {" true ", true}, // surrounding whitespace is trimmed + {"false", false}, + {"0", false}, + {"yes", false}, // not part of strconv.ParseBool — fails closed + {"on", false}, // not part of strconv.ParseBool — fails closed + {"junk", false}, + } + for _, tt := range tests { + t.Run(tt.value, func(t *testing.T) { + unsetAllEnvVars(t) + t.Setenv("ODEK_TELEGRAM_ALLOW_ALL", tt.value) + cfg := ConfigFromEnv(DefaultConfig()) + if cfg.AllowAllUsers != tt.want { + t.Errorf("ODEK_TELEGRAM_ALLOW_ALL=%q → AllowAllUsers=%v, want %v", + tt.value, cfg.AllowAllUsers, tt.want) + } + }) + } +} + +func TestHasAllowlist(t *testing.T) { + tests := []struct { + name string + chats []int64 + users []int64 + want bool + }{ + {"both empty", nil, nil, false}, + {"chats only", []int64{1}, nil, true}, + {"users only", nil, []int64{2}, true}, + {"both set", []int64{1}, []int64{2}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := TelegramConfig{AllowedChats: tt.chats, AllowedUsers: tt.users} + if got := cfg.HasAllowlist(); got != tt.want { + t.Errorf("HasAllowlist() = %v, want %v", got, tt.want) + } + }) + } +} + func TestValidateConfig_emptyToken(t *testing.T) { cfg := DefaultConfig() err := ValidateConfig(cfg) diff --git a/internal/telegram/handler.go b/internal/telegram/handler.go index 125a796..7f3742b 100644 --- a/internal/telegram/handler.go +++ b/internal/telegram/handler.go @@ -11,10 +11,10 @@ import ( // HandlerConfig controls which messages the Handler processes. type HandlerConfig struct { - AllowedChats []int64 // restricts by chat ID; empty + AllowAllUsers required to allow any + AllowedChats []int64 // non-empty: only these chat IDs pass; empty: no chat filter BotUsername string // for @mention detection in groups (without @) MaxMsgLength int // default: 4096 - AllowedUsers []int64 // restricts by user ID; empty + AllowAllUsers required to allow any + AllowedUsers []int64 // non-empty: only these user IDs pass; empty: no user filter // 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. @@ -184,6 +184,11 @@ func defaultDocumentHandler(bot *Bot) func(int64, int, string, string) (string, // HandleUpdate routes an incoming Telegram update to the appropriate handler. // Recovers from panics in handler callbacks to prevent a single bad update // from crashing the entire bot loop. +// +// SECURITY: every handler routed here must enforce isAllowed itself before +// acting (handleMessage and handleCallback both do). When adding a new update +// type, gate it the same way — callback queries once bypassed authorization +// because their handler skipped this check. func (h *Handler) HandleUpdate(upd Update) { defer h.recoverFromPanic("HandleUpdate", upd.ID) switch {