Skip to content

Commit 2963eef

Browse files
jkyberneeesclaude
andcommitted
fix(security): fail-closed Telegram authorization (#3)
An empty allowlist previously meant "allow all": isAllowed returned true when both AllowedChats and AllowedUsers were empty, and DefaultConfig left them empty with no warning. Anyone who found the bot could drive the agent and its shell/file tools. Now authorization is fail-closed: - ValidateConfig refuses to start when no allowlist is configured, unless the operator explicitly sets ODEK_TELEGRAM_ALLOW_ALL=true. - isAllowed denies when both allowlists are empty and AllowAllUsers is unset (defense in depth beyond the startup gate). - Startup logs a loud warning when running open via the opt-in. Adds the AllowAllUsers field + ODEK_TELEGRAM_ALLOW_ALL env var, wires it through HandlerConfig, and updates docs (TELEGRAM.md, SECURITY.md, .env.example). Existing deployments that set an allowlist are unaffected. Tests: TestValidateConfig_noAllowlistFailsClosed / _allowAllOptIn / _allowlistSatisfiesCheck, TestConfigFromEnv_allowAll, TestIsAllowed_* updated for the new semantics. Routing tests opt in via AllowAllUsers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 2770f02 commit 2963eef

14 files changed

Lines changed: 201 additions & 33 deletions

‎FIX_PLAN.md‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ steps. Check items off as they land.
88
## Priority order (fix these first)
99

1010
1. **#1** — Telegram document path traversal (remote arbitrary-write/RCE). ✅ Fixed
11-
2. **#3** — Fail-open Telegram authorization (amplifies #1).
11+
2. **#3** — Fail-open Telegram authorization (amplifies #1). ✅ Fixed
1212
3. **#2** — `trustAll` never resets within a run (one click unlocks the turn). ✅ Fixed
1313
4. **#4 / #5** — WS auth + serve-mode approval deadlock.
1414

15-
> Status: #1 and #2 are implemented and tested in this PR. The remaining items
16-
> are open. #3, #4, #6 involve a behavior/default change (fail-closed auth, WS
17-
> token) and are left for maintainer direction.
15+
> Status: #1, #2, and #3 are implemented and tested in this PR. The remaining
16+
> items are open. #4 and #6 involve a behavior/default change (WS token) and are
17+
> left for maintainer direction.
1818
1919
---
2020

@@ -56,6 +56,13 @@ steps. Check items off as they land.
5656
same run still triggers `PromptCommand`.
5757

5858
## 3. Fail-open Telegram authorization
59+
- **Status:** ✅ Fixed — `ValidateConfig` now refuses to start with no allowlist
60+
unless `ODEK_TELEGRAM_ALLOW_ALL=true` is set; `isAllowed` is fail-closed
61+
(empty lists + no opt-in → deny); startup logs a loud warning when running
62+
open. Tests: `TestValidateConfig_noAllowlistFailsClosed`,
63+
`TestValidateConfig_allowAllOptIn`, `TestConfigFromEnv_allowAll`,
64+
`TestIsAllowed_EmptyAllowlist` (now asserts deny),
65+
`TestIsAllowed_EmptyAllowlistWithAllowAll`.
5966
- **Severity:** High
6067
- **Location:** `internal/telegram/handler.go:505-533`; defaults in
6168
`internal/telegram/config.go`

‎cmd/odek/telegram.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,10 +159,18 @@ func telegramCmd(args []string) error {
159159

160160
// 8. Set handler config from cfg.
161161
handler.Config = telegram.HandlerConfig{
162-
AllowedChats: cfg.AllowedChats,
163-
BotUsername: cfg.BotUsername,
164-
MaxMsgLength: cfg.MaxMsgLength,
165-
AllowedUsers: cfg.AllowedUsers,
162+
AllowedChats: cfg.AllowedChats,
163+
BotUsername: cfg.BotUsername,
164+
MaxMsgLength: cfg.MaxMsgLength,
165+
AllowedUsers: cfg.AllowedUsers,
166+
AllowAllUsers: cfg.AllowAllUsers,
167+
}
168+
169+
// Loud warning when running without an allowlist (only reachable when the
170+
// operator explicitly set ODEK_TELEGRAM_ALLOW_ALL; ValidateConfig blocks
171+
// the accidental case).
172+
if cfg.AllowAllUsers && len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 {
173+
handlerLog.Warn("telegram bot is running with NO allowlist — ANY user can drive the agent (ODEK_TELEGRAM_ALLOW_ALL=true)")
166174
}
167175

168176
// 9. Build system prompt: explicit override > IDENTITY.md > compiled default

‎docker/.env.example‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,13 @@ GIT_COMMITTER_EMAIL=you@example.com
4141

4242
# ── Telegram bot (only for the telegram-* profiles) ──────────────────────
4343
# Get a token from @BotFather; restrict to YOUR chat/user id.
44+
# REQUIRED: set at least one allowlist below — the bot refuses to start with
45+
# none (fail-closed). Any Telegram user who finds an open bot can drive the
46+
# agent and its shell/file tools.
4447
# ODEK_TELEGRAM_BOT_TOKEN=123456:ABC-your-bot-token
4548
# ODEK_TELEGRAM_ALLOWED_CHATS=11111111 # comma-separated chat IDs
4649
# ODEK_TELEGRAM_ALLOWED_USERS=11111111 # comma-separated user IDs (optional)
50+
# ODEK_TELEGRAM_ALLOW_ALL=true # DANGER: run with NO allowlist (any user). Not recommended.
4751
# Chat ID that `odek run --deliver` and scheduled reminders send to.
4852
# Required for Telegram delivery; usually your own chat ID from ALLOWED_CHATS.
4953
# ODEK_TELEGRAM_DEFAULT_CHAT_ID=11111111

‎docs/SECURITY.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ odek audit <session-id> | jq … # programmatic triage
217217

218218
`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.
219219

220-
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.
220+
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.
221221

222222
### 13. Identity anchoring (legacy)
223223

‎docs/TELEGRAM.md‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,9 @@ All configuration flows through `TelegramConfig` and can be set via environment
3131
| Variable | Field | Default |
3232
|---|---|---|
3333
| `ODEK_TELEGRAM_BOT_TOKEN` | Token | — (required) |
34-
| `ODEK_TELEGRAM_ALLOWED_CHATS` | AllowedChats | all |
35-
| `ODEK_TELEGRAM_ALLOWED_USERS` | AllowedUsers | all |
34+
| `ODEK_TELEGRAM_ALLOWED_CHATS` | AllowedChats | — (see below) |
35+
| `ODEK_TELEGRAM_ALLOWED_USERS` | AllowedUsers | — (see below) |
36+
| `ODEK_TELEGRAM_ALLOW_ALL` | AllowAllUsers | false |
3637
| `ODEK_TELEGRAM_BOT_USERNAME` | BotUsername | — |
3738
| `ODEK_TELEGRAM_POLL_INTERVAL` | PollInterval | 1s |
3839
| `ODEK_TELEGRAM_POLL_TIMEOUT` | PollTimeout | 30s |
@@ -181,10 +182,19 @@ All callbacks return a response string (may be empty) and an error. The `Handle`
181182
### Access Control
182183

183184
`HandlerConfig` supports:
184-
- **AllowedChats** — restrict to specific chat IDs (empty = allow all)
185-
- **AllowedUsers** — restrict to specific user IDs (empty = allow all)
185+
- **AllowedChats** — restrict to specific chat IDs
186+
- **AllowedUsers** — restrict to specific user IDs
187+
- **AllowAllUsers** — explicit opt-in to run with no allowlist (see below)
186188
- **BotUsername** — for `@mention` detection in groups
187189

190+
> **Fail-closed authorization.** The bot **refuses to start** if neither
191+
> `ODEK_TELEGRAM_ALLOWED_CHATS` nor `ODEK_TELEGRAM_ALLOWED_USERS` is set, so an
192+
> open bot — where any Telegram user could drive the agent and its shell/file
193+
> tools — can never be deployed by accident. To intentionally run an open bot,
194+
> set `ODEK_TELEGRAM_ALLOW_ALL=true` (logged as a loud warning at startup). At
195+
> runtime, with both allowlists empty and `AllowAllUsers` unset, every update is
196+
> denied.
197+
188198
### Inline Keyboards
189199

190200
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`.

‎internal/telegram/config.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ type TelegramConfig struct {
2424
LogLevel string `json:"log_level"` // "debug","info","warn","error" (default "info")
2525
LogFile string `json:"log_file"` // path or empty for stderr
2626
DefaultChatID int64 `json:"default_chat_id"` // for --deliver and cron delivery
27+
// AllowAllUsers must be explicitly set to true to run the bot with NO
28+
// allowlist (any Telegram user may drive the agent). Without it, an empty
29+
// AllowedChats + AllowedUsers is a fatal misconfiguration (fail-closed) so
30+
// an open bot can never be deployed by accident. Env: ODEK_TELEGRAM_ALLOW_ALL.
31+
AllowAllUsers bool `json:"allow_all_users"`
2732
}
2833

2934
// DefaultConfig returns a TelegramConfig with sensible defaults.
@@ -52,6 +57,9 @@ func ConfigFromEnv(base TelegramConfig) TelegramConfig {
5257
if v := os.Getenv("ODEK_TELEGRAM_ALLOWED_USERS"); v != "" {
5358
cfg.AllowedUsers = parseInt64List(v)
5459
}
60+
if v := os.Getenv("ODEK_TELEGRAM_ALLOW_ALL"); v != "" {
61+
cfg.AllowAllUsers = parseBool(v)
62+
}
5563
if v := os.Getenv("ODEK_TELEGRAM_BOT_USERNAME"); v != "" {
5664
cfg.BotUsername = v
5765
}
@@ -127,9 +135,29 @@ func ValidateConfig(cfg TelegramConfig) error {
127135
if cfg.AgentTimeout < 0 {
128136
return fmt.Errorf("telegram: agent_timeout_seconds must be >= 0, got %d", cfg.AgentTimeout)
129137
}
138+
// Fail-closed authorization: refuse to start an unrestricted bot unless the
139+
// operator explicitly opts in. An empty allowlist would otherwise let ANY
140+
// Telegram user drive the agent (and its shell/file tools). Checked last so
141+
// field-level validation errors surface first.
142+
if len(cfg.AllowedChats) == 0 && len(cfg.AllowedUsers) == 0 && !cfg.AllowAllUsers {
143+
return fmt.Errorf("telegram: no allowlist configured — set ODEK_TELEGRAM_ALLOWED_CHATS " +
144+
"and/or ODEK_TELEGRAM_ALLOWED_USERS to restrict access, or set " +
145+
"ODEK_TELEGRAM_ALLOW_ALL=true to explicitly run an open bot (NOT recommended)")
146+
}
130147
return nil
131148
}
132149

150+
// parseBool parses common truthy string values ("true", "1", "yes", "on",
151+
// case-insensitive). Anything else is false.
152+
func parseBool(s string) bool {
153+
switch strings.ToLower(strings.TrimSpace(s)) {
154+
case "true", "1", "yes", "on":
155+
return true
156+
default:
157+
return false
158+
}
159+
}
160+
133161
// parseInt64List parses a comma-separated string of integers into a slice of int64.
134162
func parseInt64List(s string) []int64 {
135163
parts := splitAndTrim(s)

‎internal/telegram/config_test.go‎

Lines changed: 74 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -366,11 +366,12 @@ func TestConfigFromEnv_multipleOverrides(t *testing.T) {
366366

367367
func TestValidateConfig_valid(t *testing.T) {
368368
cfg := TelegramConfig{
369-
Token: "valid:token",
370-
PollInterval: 1,
371-
PollTimeout: 30,
372-
MaxMsgLength: 4096,
373-
SessionTTL: 24,
369+
Token: "valid:token",
370+
PollInterval: 1,
371+
PollTimeout: 30,
372+
MaxMsgLength: 4096,
373+
SessionTTL: 24,
374+
AllowAllUsers: true,
374375
}
375376
if err := ValidateConfig(cfg); err != nil {
376377
t.Errorf("ValidateConfig() = %v, want nil", err)
@@ -384,22 +385,80 @@ func TestValidateConfig_validMinimal(t *testing.T) {
384385
PollTimeout: 1,
385386
MaxMsgLength: 1,
386387
SessionTTL: 1,
388+
AllowedUsers: []int64{123}, // satisfies fail-closed check via allowlist
387389
}
388390
if err := ValidateConfig(cfg); err != nil {
389391
t.Errorf("ValidateConfig() = %v, want nil", err)
390392
}
391393
}
392394

393395
func TestValidateConfig_validMaximums(t *testing.T) {
396+
cfg := TelegramConfig{
397+
Token: "abc",
398+
PollInterval: 999,
399+
PollTimeout: 60,
400+
MaxMsgLength: 4096,
401+
SessionTTL: 8760,
402+
AllowAllUsers: true,
403+
}
404+
if err := ValidateConfig(cfg); err != nil {
405+
t.Errorf("ValidateConfig() = %v, want nil", err)
406+
}
407+
}
408+
409+
func TestValidateConfig_noAllowlistFailsClosed(t *testing.T) {
410+
// A config with a token but no allowlist and no explicit opt-in must be
411+
// rejected (fail-closed) so an open bot can't be deployed by accident.
412+
cfg := TelegramConfig{
413+
Token: "abc",
414+
PollInterval: 1,
415+
PollTimeout: 30,
416+
MaxMsgLength: 4096,
417+
SessionTTL: 24,
418+
}
419+
err := ValidateConfig(cfg)
420+
if err == nil {
421+
t.Fatal("ValidateConfig() = nil, want error about missing allowlist")
422+
}
423+
if !contains(err.Error(), "no allowlist configured") {
424+
t.Errorf("unexpected error: %q", err.Error())
425+
}
426+
}
427+
428+
func TestValidateConfig_allowAllOptIn(t *testing.T) {
429+
cfg := TelegramConfig{
430+
Token: "abc",
431+
PollInterval: 1,
432+
PollTimeout: 30,
433+
MaxMsgLength: 4096,
434+
SessionTTL: 24,
435+
AllowAllUsers: true,
436+
}
437+
if err := ValidateConfig(cfg); err != nil {
438+
t.Errorf("ValidateConfig() with AllowAllUsers = %v, want nil", err)
439+
}
440+
}
441+
442+
func TestValidateConfig_allowlistSatisfiesCheck(t *testing.T) {
394443
cfg := TelegramConfig{
395444
Token: "abc",
396-
PollInterval: 999,
397-
PollTimeout: 60,
445+
PollInterval: 1,
446+
PollTimeout: 30,
398447
MaxMsgLength: 4096,
399-
SessionTTL: 8760,
448+
SessionTTL: 24,
449+
AllowedChats: []int64{42},
400450
}
401451
if err := ValidateConfig(cfg); err != nil {
402-
t.Errorf("ValidateConfig() = %v, want nil", err)
452+
t.Errorf("ValidateConfig() with allowlist = %v, want nil", err)
453+
}
454+
}
455+
456+
func TestConfigFromEnv_allowAll(t *testing.T) {
457+
unsetAllEnvVars(t)
458+
t.Setenv("ODEK_TELEGRAM_ALLOW_ALL", "true")
459+
cfg := ConfigFromEnv(DefaultConfig())
460+
if !cfg.AllowAllUsers {
461+
t.Error("ConfigFromEnv did not set AllowAllUsers from ODEK_TELEGRAM_ALLOW_ALL=true")
403462
}
404463
}
405464

@@ -732,11 +791,12 @@ func splitAt(s string, sep byte) string {
732791
// validConfig returns a TelegramConfig that passes ValidateConfig.
733792
func validConfig() TelegramConfig {
734793
return TelegramConfig{
735-
Token: "test:token",
736-
PollInterval: 1,
737-
PollTimeout: 30,
738-
MaxMsgLength: 4096,
739-
SessionTTL: 24,
794+
Token: "test:token",
795+
PollInterval: 1,
796+
PollTimeout: 30,
797+
MaxMsgLength: 4096,
798+
SessionTTL: 24,
799+
AllowAllUsers: true, // satisfy fail-closed authorization check
740800
}
741801
}
742802

‎internal/telegram/e2e_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ func TestE2E_FullTextMessageFlow(t *testing.T) {
7373
poller := NewPoller(bot)
7474
poller.Timeout = 0 // don't wait in tests
7575
handler := NewHandler(bot)
76+
handler.Config.AllowAllUsers = true // routing test
7677

7778
// Set up the text message callback.
7879
var (
@@ -198,6 +199,7 @@ func TestE2E_FullCommandFlow(t *testing.T) {
198199
poller := NewPoller(bot)
199200
poller.Timeout = 0
200201
handler := NewHandler(bot)
202+
handler.Config.AllowAllUsers = true // routing test
201203

202204
var (
203205
capturedChatID int64
@@ -328,6 +330,7 @@ func TestE2E_FullCallbackFlow(t *testing.T) {
328330
poller := NewPoller(bot)
329331
poller.Timeout = 0
330332
handler := NewHandler(bot)
333+
handler.Config.AllowAllUsers = true // routing test
331334

332335
var (
333336
capturedChatID int64
@@ -446,6 +449,7 @@ func TestE2E_PollThenHandlerFlow(t *testing.T) {
446449
poller := NewPoller(bot)
447450
poller.Timeout = 0
448451
handler := NewHandler(bot)
452+
handler.Config.AllowAllUsers = true // routing test
449453

450454
// Track how many text messages and their content.
451455
var (
@@ -614,6 +618,7 @@ func TestE2E_MediaFlow(t *testing.T) {
614618
poller := NewPoller(bot)
615619
poller.Timeout = 0
616620
handler := NewHandler(bot)
621+
handler.Config.AllowAllUsers = true // routing test
617622

618623
// OnTextMessage returns a MEDIA:photo response.
619624
handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) {
@@ -715,6 +720,7 @@ func TestE2E_VoiceMediaFlow(t *testing.T) {
715720
poller := NewPoller(bot)
716721
poller.Timeout = 0
717722
handler := NewHandler(bot)
723+
handler.Config.AllowAllUsers = true // routing test
718724

719725
handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) {
720726
return "MEDIA:voice:" + tmpPath, nil
@@ -804,6 +810,7 @@ func TestE2E_PollEmptyThenMessage(t *testing.T) {
804810
poller := NewPoller(bot)
805811
poller.Timeout = 0
806812
handler := NewHandler(bot)
813+
handler.Config.AllowAllUsers = true // routing test
807814

808815
var messagesReceived []string
809816
handler.OnTextMessage = func(chatID int64, messageID int, text string) (string, error) {
@@ -925,6 +932,7 @@ func TestE2E_InlineKeyboardResponse(t *testing.T) {
925932
poller := NewPoller(bot)
926933
poller.Timeout = 0
927934
handler := NewHandler(bot)
935+
handler.Config.AllowAllUsers = true // routing test
928936

929937
handler.OnCommand = func(chatID int64, messageID int, cmd string, args string) (string, error) {
930938
return "Here are your options:", nil

‎internal/telegram/handler.go‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,14 @@ import (
1111

1212
// HandlerConfig controls which messages the Handler processes.
1313
type HandlerConfig struct {
14-
AllowedChats []int64 // empty = allow all
14+
AllowedChats []int64 // restricts by chat ID; empty + AllowAllUsers required to allow any
1515
BotUsername string // for @mention detection in groups (without @)
1616
MaxMsgLength int // default: 4096
17-
AllowedUsers []int64 // empty = allow all users
17+
AllowedUsers []int64 // restricts by user ID; empty + AllowAllUsers required to allow any
18+
// AllowAllUsers must be true to permit access when BOTH allowlists are
19+
// empty. Default false = fail-closed (deny everyone) so an unconfigured
20+
// handler never silently allows all users. See ValidateConfig.
21+
AllowAllUsers bool
1822
}
1923

2024
// ─── Handler ──────────────────────────────────────────────────────────────
@@ -501,8 +505,15 @@ func (h *Handler) sendChunk(chatID int64, chunk string, replyToMessageID int) {
501505
// ─── Access Control ───────────────────────────────────────────────────────
502506

503507
// isAllowed checks if the given chat and user are allowed to interact.
504-
// Returns true if both pass (or their respective allowlists are empty).
508+
// When both allowlists are empty, access is denied unless AllowAllUsers was
509+
// explicitly enabled (fail-closed). When a list is non-empty, the corresponding
510+
// ID must appear in it.
505511
func (h *Handler) isAllowed(chatID int64, userID int64) bool {
512+
// Fail-closed: with no allowlist at all, deny unless explicitly opted in.
513+
if len(h.Config.AllowedChats) == 0 && len(h.Config.AllowedUsers) == 0 {
514+
return h.Config.AllowAllUsers
515+
}
516+
506517
if len(h.Config.AllowedChats) > 0 {
507518
found := false
508519
for _, id := range h.Config.AllowedChats {

‎internal/telegram/handler_document_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ func TestHandleUpdate_Document(t *testing.T) {
7373
defer ts.Close()
7474
bot := testBot(t, ts)
7575
h := NewHandler(bot)
76+
h.Config.AllowAllUsers = true // routing test
7677

7778
h.OnDocumentMessage = func(chatID int64, messageID int, fileID string, fileName string) (string, error) {
7879
capturedFileID = fileID

0 commit comments

Comments
 (0)