From 42162a4e5f6ca73d076a185d79b7b59c05af4aae Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 19 Jul 2026 10:43:59 +0200 Subject: [PATCH] L11: bind Telegram clarify callbacks to request and originating user - cmd/odek/telegram.go: replace global per-chat clarify channel with pendingClarifyReqs keyed by random request ID; embed reqID in callback data; verify the callback comes from the originating user; reject expired/unknown requests. - internal/telegram/handler.go: pass callback user's userID into OnCallbackQuery so callers can enforce per-user binding. - cmd/odek/telegram_clarify_test.go + internal/telegram/*_test.go: regression tests and signature updates. - docs/SECURITY.md + AGENTS.md: document L11 mitigation. --- AGENTS.md | 1 + cmd/odek/telegram.go | 132 +++++++++++++++++----- cmd/odek/telegram_clarify_test.go | 121 ++++++++++++++++++++ docs/SECURITY.md | 6 + internal/telegram/e2e_test.go | 2 +- internal/telegram/handler.go | 14 ++- internal/telegram/handler_error_test.go | 2 +- internal/telegram/handler_recover_test.go | 2 +- internal/telegram/handler_test.go | 10 +- 9 files changed, 245 insertions(+), 45 deletions(-) create mode 100644 cmd/odek/telegram_clarify_test.go diff --git a/AGENTS.md b/AGENTS.md index 4d080c9..f470ff5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -181,6 +181,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **Telegram plan file size cap** (`internal/telegram/plan.go`) — plan files larger than 1 MiB are rejected by `ReadPlan` and `MostRecentPlan`, and `ListPlans` only reads the first 8 KiB for preview. This prevents a prompt-injected agent from causing an OOM via a multi-hundred-megabyte plan file. - **Telegram log file permissions** (`internal/telegram/log.go`) — Telegram log files are created with `0600`, and `os.Chmod` hardens existing files. Chat IDs and task snippets are no longer world-readable by default. - **Telegram chat-scoped sessions and plans** (`internal/telegram/session.go`, `internal/telegram/plan.go`, `cmd/odek/telegram.go`) — `/sessions`, `/resume`, `/prune`, `/plans`, `/plan_view`, `/plan_delete`, and `/plan_resume` are scoped to the requesting chat. Sessions are filtered by the `tg-` prefix, `ResumeSession` rejects cross-chat IDs, and plans live under `~/.odek/plans/chat/` so one chat cannot list, read, delete, or resume another chat's sessions/plans. +- **Telegram clarify callback binding** (`cmd/odek/telegram.go`, `internal/telegram/handler.go`) — each `clarify` prompt now uses a random request ID in its inline-keyboard callback data (`clarify::yes/no`). The handler validates the request ID and binds the answer to the originating user, so a group member or stale keyboard cannot answer someone else's clarify prompt. `Handler.OnCallbackQuery` now receives the `userID` for consistent callback binding. - **odek self-invocation gate** (`internal/danger/classifier.go`) — any shell stage whose program basename is `odek` is classified as `system_write`. This prevents a prompt-injected agent from reaching the human-gated trust mutations (`odek memory promote`, `odek memory extended confirm`, `odek skill promote --force`) through the shell tool and flipping its own taint gates. - **MCP inputSchema hardening** (`cmd/odek/mcp_approval.go`) — every string in an MCP tool's `inputSchema` is recursively guard-scanned for injection patterns; schemas larger than 256 KiB are rejected; the interactive approval prompt shows a SHA-256 hash and byte size of the schema so operators can detect changes. - **MCP tool batch classification** (`internal/loop/loop.go`) — MCP tools (`__`) are classified as `unknown` by `classifyToolCall`, so the batch approval gate shows them and untrusted sub-agents force them to `deny`. diff --git a/cmd/odek/telegram.go b/cmd/odek/telegram.go index 87e6e65..1eac80d 100644 --- a/cmd/odek/telegram.go +++ b/cmd/odek/telegram.go @@ -2,6 +2,8 @@ package main import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -47,10 +49,89 @@ var chatCancels sync.Map // map[int64]context.CancelFunc // interrupted task. var chatRunInfos sync.Map // map[int64]loop.IterationInfo -// clarifyMsgIDs stores the message ID of the active clarify prompt -// per chat, so the callback handler can edit/remove it after the user -// responds. Cleared when the clarify tool completes or times out. -var clarifyMsgIDs sync.Map // map[int64]int +// pendingClarifyReqs stores active clarify prompts keyed by a random +// request ID embedded in the callback data. Each entry records the +// originating user, the response channel, and the prompt message ID so +// callback queries can be validated against the user who triggered the +// clarify and rejected if the prompt has expired or was answered by +// someone else. Cleared when the clarify tool completes, times out, or +// the user responds. +type pendingClarifyReq struct { + userID int64 + ch chan string + msgID int +} + +var pendingClarifyReqs sync.Map // map[string]*pendingClarifyReq + +// generateClarifyReqID returns a random request ID for a clarify prompt. +// The ID is embedded in the callback data so each prompt is independent +// and stale keyboards cannot answer later clarifies. +func generateClarifyReqID() string { + buf := make([]byte, 8) + if _, err := rand.Read(buf); err != nil { + // crypto/rand.Read only fails on catastrophic system failure. Fail + // closed rather than minting a predictable ID, which would let an + // attacker pre-compute callback data. + panic(fmt.Sprintf("telegram: crypto/rand unavailable: %v", err)) + } + return hex.EncodeToString(buf) +} + +// parseClarifyCallback parses callback data of the form +// "clarify::". It returns the request ID, the answer, and +// true if the data is a valid clarify callback. +func parseClarifyCallback(data string) (reqID, answer string, ok bool) { + rest, ok := strings.CutPrefix(data, "clarify:") + if !ok { + return "", "", false + } + idx := strings.LastIndex(rest, ":") + if idx < 0 { + return "", "", false + } + reqID = rest[:idx] + answer = rest[idx+1:] + if reqID == "" || (answer != "yes" && answer != "no") { + return "", "", false + } + return reqID, answer, true +} + +// handleClarifyCallback routes a Telegram inline-button press for a clarify +// prompt. It validates the embedded request ID, binds the answer to the +// originating user, and unblocks the waiting agent goroutine. The returned +// bool is true when the callback data was a clarify callback (whether valid +// or expired); callers should not process it further. +func handleClarifyCallback(chatID, userID int64, data string, bot *telegram.Bot) (string, bool) { + reqID, answer, ok := parseClarifyCallback(data) + if !ok { + return "", false + } + v, ok := pendingClarifyReqs.Load(reqID) + if !ok { + return "⚠️ This clarify prompt has expired or already been answered.", true + } + req := v.(*pendingClarifyReq) + // Bind the callback to the user who triggered the clarify. + if req.userID != 0 && req.userID != userID { + return "⚠️ This button was meant for another user.", true + } + // Accept the answer and remove the request atomically. + pendingClarifyReqs.Delete(reqID) + select { + case req.ch <- answer: + default: + // Channel full or closed — clarify already resolved. + } + // Remove the inline keyboard and update text to show the answer. + if req.msgID != 0 && bot != nil { + bot.EditMessageText(chatID, req.msgID, + "✅ *User answered:* "+answer, + &telegram.SendOpts{ParseMode: telegram.ParseModeMarkdownV2, ReplyMarkup: &telegram.InlineKeyboardMarkup{InlineKeyboard: [][]telegram.InlineKeyboardButton{}}}) + } + return "", true +} // pendingSuggestions stores SkillSuggestion values keyed by skill name, // awaiting user approval via inline keyboard callbacks. @@ -594,24 +675,10 @@ func telegramCmd(args []string) error { return cmd.Handler(argsStr) } - handler.OnCallbackQuery = func(chatID int64, data string) (string, error) { + handler.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { // Route clarify callbacks — the user clicked Yes/No on a clarify question. - if answer, ok := strings.CutPrefix(data, "clarify:"); ok { - if ch, ok := sessionManager.GetClarifyChannel(chatID); ok { - select { - case ch <- answer: - default: - // Channel full or closed — clarify already resolved. - } - } - // Remove the inline keyboard and update text to show the answer. - if msgID, ok := clarifyMsgIDs.Load(chatID); ok { - bot.EditMessageText(chatID, msgID.(int), - "✅ *User answered:* "+answer, - &telegram.SendOpts{ParseMode: telegram.ParseModeMarkdownV2, ReplyMarkup: &telegram.InlineKeyboardMarkup{InlineKeyboard: [][]telegram.InlineKeyboardButton{}}}) - clarifyMsgIDs.Delete(chatID) - } - return "", nil + if resp, ok := handleClarifyCallback(chatID, userID, data, bot); ok { + return resp, nil } // Route skill suggestion callbacks — Save or Skip. @@ -1514,26 +1581,29 @@ func handleChatMessage( // keyboard message and blocks until the user responds. agentTools := append([]odek.Tool{}, tools...) agentTools = append(agentTools, toolpkg.NewClarifyTool(func(question string) (string, error) { + reqID := generateClarifyReqID() ch := make(chan string, 1) - sessionManager.SetClarifyChannel(chatID, ch) - defer sessionManager.DeleteClarifyChannel(chatID) + req := &pendingClarifyReq{userID: userID, ch: ch} + pendingClarifyReqs.Store(reqID, req) + defer pendingClarifyReqs.Delete(reqID) - // Send the question with Yes/No buttons. + // Send the question with Yes/No buttons. Each prompt gets a unique + // request ID so stale keyboards cannot answer later clarifies and + // the callback can be bound to the originating user. replyMarkup := &telegram.InlineKeyboardMarkup{ InlineKeyboard: [][]telegram.InlineKeyboardButton{ { - {Text: "Yes", CallbackData: "clarify:yes"}, - {Text: "No", CallbackData: "clarify:no"}, + {Text: "Yes", CallbackData: fmt.Sprintf("clarify:%s:yes", reqID)}, + {Text: "No", CallbackData: fmt.Sprintf("clarify:%s:no", reqID)}, }, }, } - if msg, err := bot.SendMessage(chatID, "❓ "+question, - &telegram.SendOpts{ReplyMarkup: replyMarkup, ParseMode: "Markdown", ReplyToMessageID: messageID}); err != nil { + msg, err := bot.SendMessage(chatID, "❓ "+question, + &telegram.SendOpts{ReplyMarkup: replyMarkup, ParseMode: "Markdown", ReplyToMessageID: messageID}) + if err != nil { return "", fmt.Errorf("clarify: send message: %w", err) - } else { - clarifyMsgIDs.Store(chatID, msg.ID) - defer clarifyMsgIDs.Delete(chatID) } + req.msgID = msg.ID // Wait for the user to click a button (or timeout). select { diff --git a/cmd/odek/telegram_clarify_test.go b/cmd/odek/telegram_clarify_test.go new file mode 100644 index 0000000..1e6dec8 --- /dev/null +++ b/cmd/odek/telegram_clarify_test.go @@ -0,0 +1,121 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/BackendStack21/odek/internal/telegram" +) + +func TestParseClarifyCallback_Valid(t *testing.T) { + reqID, answer, ok := parseClarifyCallback("clarify:abcd1234:yes") + if !ok { + t.Fatal("expected valid clarify callback") + } + if reqID != "abcd1234" { + t.Errorf("reqID = %q, want abcd1234", reqID) + } + if answer != "yes" { + t.Errorf("answer = %q, want yes", answer) + } +} + +func TestParseClarifyCallback_Invalid(t *testing.T) { + invalid := []string{ + "clarify:yes", + "clarify:", + "clarify:abcd1234:maybe", + "skill_save:foo", + "apr:123", + "", + } + for _, data := range invalid { + if _, _, ok := parseClarifyCallback(data); ok { + t.Errorf("expected %q to be invalid", data) + } + } +} + +func TestGenerateClarifyReqID_Unique(t *testing.T) { + seen := make(map[string]bool) + for i := 0; i < 100; i++ { + id := generateClarifyReqID() + if seen[id] { + t.Fatalf("duplicate request ID: %q", id) + } + seen[id] = true + } +} + +// TestHandleClarifyCallback_BindsToOriginatingUser verifies that a clarify +// callback from the correct user unblocks the channel and rejects a +// callback from a different user. +func TestHandleClarifyCallback_BindsToOriginatingUser(t *testing.T) { + // Start a fake Telegram server so EditMessageText does not fail. + var edits sync.Map + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "/editMessageText") { + edits.Store(1, true) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"ok":true,"result":true}`)) + })) + defer srv.Close() + + bot := telegram.NewBot("test-token") + bot.BaseURL = srv.URL + "/bot" + bot.Token + + reqID := generateClarifyReqID() + ch := make(chan string, 1) + pendingClarifyReqs.Store(reqID, &pendingClarifyReq{userID: 42, ch: ch, msgID: 123}) + defer pendingClarifyReqs.Delete(reqID) + + // Wrong user should be rejected without sending an answer. + resp, ok := handleClarifyCallback(1, 99, "clarify:"+reqID+":yes", bot) + if !ok { + t.Fatal("expected clarify callback to be recognized") + } + if !strings.Contains(resp, "meant for another user") { + t.Errorf("expected cross-user rejection, got: %q", resp) + } + select { + case <-ch: + t.Fatal("answer should not have been sent for wrong user") + default: + } + + // Correct user should unblock the channel and update the message. + resp, ok = handleClarifyCallback(1, 42, "clarify:"+reqID+":yes", bot) + if !ok { + t.Fatal("expected clarify callback to be recognized") + } + if resp != "" { + t.Errorf("expected empty response for accepted callback, got: %q", resp) + } + select { + case ans := <-ch: + if ans != "yes" { + t.Errorf("answer = %q, want yes", ans) + } + default: + t.Fatal("expected answer to be delivered to channel") + } + if _, ok := edits.Load(1); !ok { + t.Error("expected EditMessageText to be called") + } +} + +// TestHandleClarifyCallback_ExpiredRequest verifies that a callback for an +// unknown/expired request returns a friendly expiration message. +func TestHandleClarifyCallback_ExpiredRequest(t *testing.T) { + resp, ok := handleClarifyCallback(1, 42, "clarify:"+generateClarifyReqID()+":no", nil) + if !ok { + t.Fatal("expected clarify callback to be recognized") + } + if !strings.Contains(resp, "expired") { + t.Errorf("expected expiration message, got: %q", resp) + } +} diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 9935733..b8e5a57 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -667,6 +667,12 @@ Now: `persist()` now hardens any existing file with `os.Chmod(path, 0600)` and opens the file with `os.O_WRONLY|os.O_CREATE|os.O_TRUNC` and mode `0600`, matching the permission model already applied to sessions, audit logs, and Telegram logs. +### 39n. Telegram clarify callback binding + +The Telegram bot's `clarify` tool sent inline keyboard buttons with literal callback data `clarify:yes` and `clarify:no`. Because the data carried no request identifier or user binding, any group member (or a stale keyboard from an earlier task) could answer a clarify prompt intended for someone else, and a later clarify could be answered by a stale button from an earlier one. + +Each clarify prompt now generates a random request ID (embedded in callback data as `clarify::yes/no`). The handler validates the request ID, rejects callbacks from a different user than the one who triggered the prompt, and ignores callbacks for expired or already-answered prompts. The `Handler.OnCallbackQuery` signature now receives the originating `userID` so other callback handlers can apply the same binding. + ### 40. `/api/resources` result limit cap The `/api/resources?q=...&limit=N` autocomplete endpoint previously accepted any positive `limit` value. It is now capped to 100 results both in the HTTP handler and in `Registry.Search`. This prevents a prompt-injected or attacker-forged request from forcing an unbounded directory walk and returning a multi-megabyte JSON response. diff --git a/internal/telegram/e2e_test.go b/internal/telegram/e2e_test.go index f4be011..32630f3 100644 --- a/internal/telegram/e2e_test.go +++ b/internal/telegram/e2e_test.go @@ -336,7 +336,7 @@ func TestE2E_FullCallbackFlow(t *testing.T) { capturedChatID int64 capturedData string ) - handler.OnCallbackQuery = func(chatID int64, callbackData string) (string, error) { + handler.OnCallbackQuery = func(chatID int64, callbackData string, userID int64) (string, error) { capturedChatID = chatID capturedData = callbackData return "You selected option 1!", nil diff --git a/internal/telegram/handler.go b/internal/telegram/handler.go index 531e04d..c4357b5 100644 --- a/internal/telegram/handler.go +++ b/internal/telegram/handler.go @@ -57,9 +57,11 @@ type Handler struct { OnTextMessage func(chatID int64, messageID int, text string, forwarded bool, userID int64) (string, error) // OnCallbackQuery is called when a callback query is received and - // it was NOT handled by the TelegramApprover. Returns the response - // text (may be empty). - OnCallbackQuery func(chatID int64, callbackData string) (string, error) + // it was NOT handled by the TelegramApprover. userID is the Telegram + // user who pressed the button, so callers can bind callbacks to the + // originating user and reject stale or cross-user button presses. + // Returns the response text (may be empty). + OnCallbackQuery func(chatID int64, callbackData string, userID int64) (string, error) // OnCommand is called when a bot command (e.g. /start) is received. // userID is the Telegram user who sent the command. @@ -149,8 +151,8 @@ func defaultTextHandler() func(int64, int, string, bool, int64) (string, error) } // defaultCallbackHandler returns a default OnCallbackQuery callback. -func defaultCallbackHandler() func(int64, string) (string, error) { - return func(_ int64, _ string) (string, error) { +func defaultCallbackHandler() func(int64, string, int64) (string, error) { + return func(_ int64, _ string, _ int64) (string, error) { return "Not implemented yet: callback query", nil } } @@ -409,7 +411,7 @@ func (h *Handler) handleCallback(cq *CallbackQuery) { } if h.OnCallbackQuery != nil { - resp, err := h.OnCallbackQuery(cq.Message.Chat.ID, cq.Data) + resp, err := h.OnCallbackQuery(cq.Message.Chat.ID, cq.Data, userID) if err != nil { h.log.Error("callback query handler failed", "chat_id", cq.Message.Chat.ID, "data", cq.Data, "error", err) if h.OnError != nil { diff --git a/internal/telegram/handler_error_test.go b/internal/telegram/handler_error_test.go index 13a8efe..cb9a470 100644 --- a/internal/telegram/handler_error_test.go +++ b/internal/telegram/handler_error_test.go @@ -60,7 +60,7 @@ func TestHandleCallback_ErrorSentToUser(t *testing.T) { h := NewHandler(bot) h.Config.AllowAllUsers = true // routing test - h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + h.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { return "", fmt.Errorf("simulated callback failure: %s", data) } diff --git a/internal/telegram/handler_recover_test.go b/internal/telegram/handler_recover_test.go index de1876c..4f4816f 100644 --- a/internal/telegram/handler_recover_test.go +++ b/internal/telegram/handler_recover_test.go @@ -65,7 +65,7 @@ func TestHandleUpdate_RecoverFromPanicCallback(t *testing.T) { h := NewHandler(bot) h.Config.AllowAllUsers = true // routing test - h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + h.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { panic("simulated callback panic") } diff --git a/internal/telegram/handler_test.go b/internal/telegram/handler_test.go index dc9ead3..bc12ca6 100644 --- a/internal/telegram/handler_test.go +++ b/internal/telegram/handler_test.go @@ -167,7 +167,7 @@ func TestNewHandler_defaults(t *testing.T) { t.Errorf("default OnTextMessage = %q, want %q", textResp, "Not implemented yet: text") } - cbResp, _ := h.OnCallbackQuery(1, "data") + cbResp, _ := h.OnCallbackQuery(1, "data", 100) if cbResp != "Not implemented yet: callback query" { t.Errorf("default OnCallbackQuery = %q, want %q", cbResp, "Not implemented yet: callback query") } @@ -311,7 +311,7 @@ func TestHandleUpdate_CallbackQuery(t *testing.T) { bot := testBot(t, ts) h := NewHandler(bot) h.Config.AllowAllUsers = true // callback routing test - h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + h.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { capturedChatID = chatID capturedCallbackID = data return "callback response", nil @@ -834,7 +834,7 @@ func TestHandleCallback_RespectsAllowlist(t *testing.T) { h.Config.AllowedChats = []int64{100} // only chat 100 allowed var called bool - h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + h.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { called = true return "", nil } @@ -1634,7 +1634,7 @@ func TestHandler_HandleCallback_FallbackToOnCallbackQuery(t *testing.T) { capturedChatID int64 capturedData string ) - h.OnCallbackQuery = func(chatID int64, data string) (string, error) { + h.OnCallbackQuery = func(chatID int64, data string, userID int64) (string, error) { capturedChatID = chatID capturedData = data return "fallback response", nil @@ -1893,7 +1893,7 @@ func TestHandleUpdate_CallbackQueryNotAllowed(t *testing.T) { h.Config.AllowedUsers = []int64{100} called := false - h.OnCallbackQuery = func(_ int64, _ string) (string, error) { + h.OnCallbackQuery = func(_ int64, _ string, _ int64) (string, error) { called = true return "", nil }