From 1dffd4588c593d91356cc84db414c17fb656dc17 Mon Sep 17 00:00:00 2001 From: Solomon Neas Date: Sat, 1 Aug 2026 18:07:46 -0400 Subject: [PATCH 1/2] fix(notify): bound hook input and provider payload sizes Cap stdin/hook reads at 256 KiB and truncate or refuse Discord, Telegram, and Signal payloads against documented ceilings so oversized events fail with a bounded payload_limit diagnostic instead of silent non-delivery. Closes #618 Co-authored-by: Cursor --- stations/notify/cmd/agent-notify/main.go | 6 +- stations/notify/cmd/agent-notify/main_test.go | 96 ++++++++++ stations/notify/internal/adapter/adapter.go | 4 +- stations/notify/internal/adapter/bound.go | 37 ++++ .../notify/internal/adapter/bound_test.go | 92 ++++++++++ .../adapter/claude_code_notification.go | 4 +- .../internal/adapter/claude_code_stop.go | 4 +- .../notify/internal/adapter/codex_notify.go | 7 +- stations/notify/internal/channels/discord.go | 79 ++++++-- stations/notify/internal/channels/errors.go | 9 + stations/notify/internal/channels/limits.go | 52 ++++++ .../notify/internal/channels/limits_test.go | 173 ++++++++++++++++++ stations/notify/internal/channels/signal.go | 28 ++- stations/notify/internal/channels/telegram.go | 40 +++- 14 files changed, 606 insertions(+), 25 deletions(-) create mode 100644 stations/notify/internal/adapter/bound.go create mode 100644 stations/notify/internal/adapter/bound_test.go create mode 100644 stations/notify/internal/channels/limits.go create mode 100644 stations/notify/internal/channels/limits_test.go diff --git a/stations/notify/cmd/agent-notify/main.go b/stations/notify/cmd/agent-notify/main.go index 71bac3b8..6665da52 100644 --- a/stations/notify/cmd/agent-notify/main.go +++ b/stations/notify/cmd/agent-notify/main.go @@ -303,7 +303,11 @@ func buildMessage(hook string, posArgs []string, stdin io.Reader) (canonical.Mes case "", "custom": // Prefer positional arg; otherwise read stdin. if len(posArgs) > 0 { - return adapter.FromString(strings.Join(posArgs, " ")), nil + joined := strings.Join(posArgs, " ") + if err := adapter.CheckSize([]byte(joined)); err != nil { + return canonical.Message{}, err + } + return adapter.FromString(joined), nil } return adapter.AutoDetect(stdin) default: diff --git a/stations/notify/cmd/agent-notify/main_test.go b/stations/notify/cmd/agent-notify/main_test.go index 5fa39fe9..1d54fde0 100644 --- a/stations/notify/cmd/agent-notify/main_test.go +++ b/stations/notify/cmd/agent-notify/main_test.go @@ -775,3 +775,99 @@ func TestRun_CodexNotifyResolvesModelIdentity(t *testing.T) { t.Errorf("embed title = %#v, want OpenAI · gpt-5.6-sol", embed["title"]) } } + +func TestRun_OversizedStdinRejected(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("provider must not be called for oversized input") + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + oversized := strings.Repeat("x", adapterMaxInputBytes()+1) + code, _, stderr := runMain(t, + []string{"agent-notify"}, + oversized, + map[string]string{"DISCORD_WEBHOOK_URL": srv.URL}, + ) + if code != exitConfig { + t.Fatalf("exit = %d, want %d (stderr=%s)", code, exitConfig, stderr) + } + if !strings.Contains(stderr, "input exceeds") { + t.Fatalf("stderr should mention input limit, got %q", stderr) + } + if strings.Contains(stderr, oversized[:32]) { + t.Fatalf("stderr must not echo oversized payload: %q", stderr) + } +} + +func TestRun_PartialFailurePayloadLimitVisible(t *testing.T) { + // Discord truncates titles safely; Telegram MarkdownV2 escaping doubles + // "." so an oversize title cannot fit. One channel succeeds, one fails + // with a bounded payload_limit diagnostic and exitFailures. + var discordHits int + discordSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + discordHits++ + w.WriteHeader(http.StatusNoContent) + })) + defer discordSrv.Close() + + var telegramHits int + telegramSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + telegramHits++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer telegramSrv.Close() + + cfgPath := filepath.Join(t.TempDir(), "config.toml") + if err := os.WriteFile(cfgPath, []byte(` +[channels.discord-main] +type = "discord" +webhook_url_env = "DISCORD_WEBHOOK_URL" + +[channels.telegram-personal] +type = "telegram" +bot_token_env = "TELEGRAM_BOT_TOKEN" +chat_id_env = "TELEGRAM_CHAT_ID" +`), 0o600); err != nil { + t.Fatal(err) + } + + body, _ := json.Marshal(map[string]string{ + "title": strings.Repeat(".", channels.TelegramTextMax+10), + "body": "done", + }) + code, _, stderr := runMain(t, + []string{"agent-notify", "--config", cfgPath, "--to", "discord-main,telegram-personal"}, + string(body), + map[string]string{ + "DISCORD_WEBHOOK_URL": discordSrv.URL, + "TELEGRAM_BOT_TOKEN": "SECRET-BOT-TOKEN", + "TELEGRAM_CHAT_ID": "12345", + }, + ) + if code != exitFailures { + t.Fatalf("exit = %d, want %d (stderr=%s)", code, exitFailures, stderr) + } + if discordHits != 1 { + t.Fatalf("discord hits = %d, want 1", discordHits) + } + if telegramHits != 0 { + t.Fatalf("telegram hits = %d, want 0 (payload rejected before send)", telegramHits) + } + if !strings.Contains(stderr, "FAIL channel=telegram-personal") { + t.Fatalf("stderr missing telegram FAIL line: %q", stderr) + } + if !strings.Contains(stderr, "payload_limit") { + t.Fatalf("stderr missing payload_limit cause: %q", stderr) + } + if strings.Contains(stderr, "SECRET-BOT-TOKEN") { + t.Fatalf("stderr leaked bot token: %q", stderr) + } +} + +// adapterMaxInputBytes mirrors adapter.MaxInputBytes without importing the +// adapter package into every CLI assertion helper. +func adapterMaxInputBytes() int { + return 256 * 1024 +} diff --git a/stations/notify/internal/adapter/adapter.go b/stations/notify/internal/adapter/adapter.go index 2269be10..65023969 100644 --- a/stations/notify/internal/adapter/adapter.go +++ b/stations/notify/internal/adapter/adapter.go @@ -26,9 +26,9 @@ func FromString(s string) canonical.Message { // - JSON arrays, scalars, and other non-object inputs are treated as // plain string bodies. func AutoDetect(r io.Reader) (canonical.Message, error) { - raw, err := io.ReadAll(r) + raw, err := ReadBounded(r) if err != nil { - return canonical.Message{}, fmt.Errorf("read input: %w", err) + return canonical.Message{}, err } trimmed := strings.TrimSpace(string(raw)) if trimmed == "" { diff --git a/stations/notify/internal/adapter/bound.go b/stations/notify/internal/adapter/bound.go new file mode 100644 index 00000000..f7d2905b --- /dev/null +++ b/stations/notify/internal/adapter/bound.go @@ -0,0 +1,37 @@ +package adapter + +import ( + "fmt" + "io" +) + +// MaxInputBytes is the hard ceiling for hook and stdin payloads. Reading stops +// one byte past this limit so oversized input is detected without unbounded +// allocation. +const MaxInputBytes = 256 * 1024 + +// ErrInputTooLarge is returned when hook or stdin input exceeds MaxInputBytes. +var ErrInputTooLarge = fmt.Errorf("input exceeds %d-byte limit", MaxInputBytes) + +// ReadBounded reads from r up to MaxInputBytes. If more data is available it +// returns ErrInputTooLarge without retaining the excess. +func ReadBounded(r io.Reader) ([]byte, error) { + limited := io.LimitReader(r, int64(MaxInputBytes)+1) + raw, err := io.ReadAll(limited) + if err != nil { + return nil, fmt.Errorf("read input: %w", err) + } + if len(raw) > MaxInputBytes { + return nil, ErrInputTooLarge + } + return raw, nil +} + +// CheckSize refuses a pre-buffered payload (for example a Codex argv JSON +// blob) that exceeds MaxInputBytes. +func CheckSize(raw []byte) error { + if len(raw) > MaxInputBytes { + return ErrInputTooLarge + } + return nil +} diff --git a/stations/notify/internal/adapter/bound_test.go b/stations/notify/internal/adapter/bound_test.go new file mode 100644 index 00000000..0d0dc49d --- /dev/null +++ b/stations/notify/internal/adapter/bound_test.go @@ -0,0 +1,92 @@ +package adapter + +import ( + "bytes" + "errors" + "io" + "strings" + "testing" +) + +func TestReadBounded_BelowLimit(t *testing.T) { + in := bytes.Repeat([]byte("a"), MaxInputBytes-1) + got, err := ReadBounded(bytes.NewReader(in)) + if err != nil { + t.Fatalf("ReadBounded: %v", err) + } + if len(got) != MaxInputBytes-1 { + t.Fatalf("len = %d, want %d", len(got), MaxInputBytes-1) + } +} + +func TestReadBounded_AtLimit(t *testing.T) { + in := bytes.Repeat([]byte("b"), MaxInputBytes) + got, err := ReadBounded(bytes.NewReader(in)) + if err != nil { + t.Fatalf("ReadBounded: %v", err) + } + if len(got) != MaxInputBytes { + t.Fatalf("len = %d, want %d", len(got), MaxInputBytes) + } +} + +func TestReadBounded_AboveLimit(t *testing.T) { + in := bytes.Repeat([]byte("c"), MaxInputBytes+1) + got, err := ReadBounded(bytes.NewReader(in)) + if !errors.Is(err, ErrInputTooLarge) { + t.Fatalf("err = %v, want ErrInputTooLarge", err) + } + if got != nil { + t.Fatalf("got %d bytes, want nil on oversized input", len(got)) + } +} + +func TestReadBounded_AboveLimitDoesNotDrainUnbounded(t *testing.T) { + // A reader that can produce far more than the limit must still stop after + // MaxInputBytes+1 so memory stays bounded. + r := &countingReader{limit: MaxInputBytes * 4} + _, err := ReadBounded(r) + if !errors.Is(err, ErrInputTooLarge) { + t.Fatalf("err = %v, want ErrInputTooLarge", err) + } + if r.read > MaxInputBytes+1 { + t.Fatalf("read %d bytes, want at most %d", r.read, MaxInputBytes+1) + } +} + +func TestAutoDetect_AboveLimit(t *testing.T) { + in := strings.Repeat("x", MaxInputBytes+1) + _, err := AutoDetect(strings.NewReader(in)) + if !errors.Is(err, ErrInputTooLarge) { + t.Fatalf("err = %v, want ErrInputTooLarge", err) + } +} + +func TestCheckSize_Boundaries(t *testing.T) { + if err := CheckSize(bytes.Repeat([]byte("d"), MaxInputBytes)); err != nil { + t.Fatalf("at limit: %v", err) + } + if err := CheckSize(bytes.Repeat([]byte("e"), MaxInputBytes+1)); !errors.Is(err, ErrInputTooLarge) { + t.Fatalf("above limit: %v", err) + } +} + +type countingReader struct { + limit int + read int +} + +func (c *countingReader) Read(p []byte) (int, error) { + if c.read >= c.limit { + return 0, io.EOF + } + n := len(p) + if c.read+n > c.limit { + n = c.limit - c.read + } + for i := 0; i < n; i++ { + p[i] = 'z' + } + c.read += n + return n, nil +} diff --git a/stations/notify/internal/adapter/claude_code_notification.go b/stations/notify/internal/adapter/claude_code_notification.go index ea727210..a3e21b49 100644 --- a/stations/notify/internal/adapter/claude_code_notification.go +++ b/stations/notify/internal/adapter/claude_code_notification.go @@ -11,9 +11,9 @@ import ( // ClaudeCodeNotification reads a Claude Code Notification hook event JSON // and produces a canonical message. func ClaudeCodeNotification(r io.Reader) (canonical.Message, error) { - raw, err := io.ReadAll(r) + raw, err := ReadBounded(r) if err != nil { - return canonical.Message{}, fmt.Errorf("read input: %w", err) + return canonical.Message{}, err } var ev map[string]interface{} if err := json.Unmarshal(raw, &ev); err != nil { diff --git a/stations/notify/internal/adapter/claude_code_stop.go b/stations/notify/internal/adapter/claude_code_stop.go index 47f86cea..3a585798 100644 --- a/stations/notify/internal/adapter/claude_code_stop.go +++ b/stations/notify/internal/adapter/claude_code_stop.go @@ -15,9 +15,9 @@ import ( // to sensible defaults when fields are missing. Survives most schema // additions and aliased renames without changes. func ClaudeCodeStop(r io.Reader) (canonical.Message, error) { - raw, err := io.ReadAll(r) + raw, err := ReadBounded(r) if err != nil { - return canonical.Message{}, fmt.Errorf("read input: %w", err) + return canonical.Message{}, err } var ev map[string]interface{} if err := json.Unmarshal(raw, &ev); err != nil { diff --git a/stations/notify/internal/adapter/codex_notify.go b/stations/notify/internal/adapter/codex_notify.go index 1ab909c3..e7869a43 100644 --- a/stations/notify/internal/adapter/codex_notify.go +++ b/stations/notify/internal/adapter/codex_notify.go @@ -12,9 +12,9 @@ import ( // message. Codex's notify schema is younger than Claude Code's, so this // adapter is especially defensive about field name variations. func CodexNotify(r io.Reader) (canonical.Message, error) { - raw, err := io.ReadAll(r) + raw, err := ReadBounded(r) if err != nil { - return canonical.Message{}, fmt.Errorf("read input: %w", err) + return canonical.Message{}, err } return CodexNotifyFromBytes(raw) } @@ -23,6 +23,9 @@ func CodexNotify(r io.Reader) (canonical.Message, error) { // Codex passes the event JSON as the last positional argv argument rather than // on stdin, so the command layer can reach this directly with the arg payload. func CodexNotifyFromBytes(raw []byte) (canonical.Message, error) { + if err := CheckSize(raw); err != nil { + return canonical.Message{}, err + } var ev map[string]interface{} if err := json.Unmarshal(raw, &ev); err != nil { return canonical.Message{}, fmt.Errorf("parse codex event: %w", err) diff --git a/stations/notify/internal/channels/discord.go b/stations/notify/internal/channels/discord.go index 05c9841f..3153470e 100644 --- a/stations/notify/internal/channels/discord.go +++ b/stations/notify/internal/channels/discord.go @@ -62,20 +62,9 @@ type discordRequest struct { } func (d *Discord) Send(ctx context.Context, m canonical.Message) error { - embed := discordEmbed{ - Title: titleFor(m), - Description: m.Body, - Color: colorFor(m.Level), - } - if m.Source != "" { - embed.Footer = &discordFooter{Text: m.Source} - } - if len(m.Tags) > 0 { - embed.Fields = []discordField{{ - Name: "tags", - Value: strings.Join(m.Tags, ", "), - Inline: true, - }} + embed, err := fitDiscordEmbed(m) + if err != nil { + return err } payload := discordRequest{Embeds: []discordEmbed{embed}} @@ -114,3 +103,65 @@ func colorFor(level string) int { return colorInfo } } + +func fitDiscordEmbed(m canonical.Message) (discordEmbed, error) { + title, ok := truncateRunes(titleFor(m), DiscordTitleMax) + if !ok { + return discordEmbed{}, payloadLimitError("discord") + } + desc, ok := truncateRunes(m.Body, DiscordDescriptionMax) + if !ok { + return discordEmbed{}, payloadLimitError("discord") + } + embed := discordEmbed{ + Title: title, + Description: desc, + Color: colorFor(m.Level), + } + if m.Source != "" { + footer, ok := truncateRunes(m.Source, DiscordFooterMax) + if !ok { + return discordEmbed{}, payloadLimitError("discord") + } + embed.Footer = &discordFooter{Text: footer} + } + if len(m.Tags) > 0 { + tags, ok := truncateRunes(strings.Join(m.Tags, ", "), DiscordFieldValueMax) + if !ok { + return discordEmbed{}, payloadLimitError("discord") + } + embed.Fields = []discordField{{ + Name: "tags", + Value: tags, + Inline: true, + }} + } + if embedCharCount(embed) > DiscordEmbedTotalMax { + // Shrink description until the documented total embed ceiling fits. + overhead := embedCharCount(embed) - len(embed.Description) + budget := DiscordEmbedTotalMax - overhead + if budget < len(truncateSuffix) { + return discordEmbed{}, payloadLimitError("discord") + } + desc, ok = truncateRunes(m.Body, budget) + if !ok { + return discordEmbed{}, payloadLimitError("discord") + } + embed.Description = desc + if embedCharCount(embed) > DiscordEmbedTotalMax { + return discordEmbed{}, payloadLimitError("discord") + } + } + return embed, nil +} + +func embedCharCount(e discordEmbed) int { + n := len(e.Title) + len(e.Description) + if e.Footer != nil { + n += len(e.Footer.Text) + } + for _, f := range e.Fields { + n += len(f.Name) + len(f.Value) + } + return n +} diff --git a/stations/notify/internal/channels/errors.go b/stations/notify/internal/channels/errors.go index 9f554061..0ddf8cef 100644 --- a/stations/notify/internal/channels/errors.go +++ b/stations/notify/internal/channels/errors.go @@ -32,6 +32,7 @@ func (e *DeliveryError) Error() string { cause := boundedValue( e.Cause, "encoding", + "payload_limit", "invalid_request", "dns", "tls", @@ -85,6 +86,14 @@ func encodingError(provider string) error { } } +func payloadLimitError(provider string) error { + return &DeliveryError{ + Provider: provider, + Stage: "encode", + Cause: "payload_limit", + } +} + func statusError(provider string, status int) error { return &DeliveryError{ Provider: provider, diff --git a/stations/notify/internal/channels/limits.go b/stations/notify/internal/channels/limits.go new file mode 100644 index 00000000..32ff4088 --- /dev/null +++ b/stations/notify/internal/channels/limits.go @@ -0,0 +1,52 @@ +package channels + +import ( + "unicode/utf8" +) + +// Provider payload ceilings from each vendor's documented API limits. +const ( + // Discord embed field ceilings: + // https://discord.com/developers/docs/resources/channel#embed-object + DiscordTitleMax = 256 + DiscordDescriptionMax = 4096 + DiscordFooterMax = 2048 + DiscordFieldValueMax = 1024 + DiscordEmbedTotalMax = 6000 + + // Telegram Bot API sendMessage text ceiling: + // https://core.telegram.org/bots/api#sendmessage + TelegramTextMax = 4096 + + // Signal has no Telegram-style documented char ceiling in signal-cli's + // send endpoint; keep a practical bound under the DataMessage size class + // so a single notification cannot push multi-megabyte JSON bodies. + SignalMessageMax = 64 * 1024 +) + +const truncateSuffix = "…" + +// truncateRunes shortens s to at most max bytes without splitting a UTF-8 +// rune. When truncation is required the Unicode ellipsis is appended and +// counted toward max. Returns ok=false when max is too small to hold the +// ellipsis alone (message contract cannot be preserved). +func truncateRunes(s string, max int) (string, bool) { + if max < 0 { + return "", false + } + if len(s) <= max { + return s, true + } + if max < len(truncateSuffix) { + return "", false + } + budget := max - len(truncateSuffix) + if budget <= 0 { + return truncateSuffix, true + } + cut := budget + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + return s[:cut] + truncateSuffix, true +} diff --git a/stations/notify/internal/channels/limits_test.go b/stations/notify/internal/channels/limits_test.go new file mode 100644 index 00000000..6aae6ca4 --- /dev/null +++ b/stations/notify/internal/channels/limits_test.go @@ -0,0 +1,173 @@ +package channels + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/escoffier-labs/agent-notify/internal/canonical" +) + +func TestTruncateRunes_Boundaries(t *testing.T) { + got, ok := truncateRunes("hello", 5) + if !ok || got != "hello" { + t.Fatalf("exact fit: got %q ok=%v", got, ok) + } + got, ok = truncateRunes("hello world", 8) + if !ok || !strings.HasSuffix(got, truncateSuffix) || len(got) > 8 { + t.Fatalf("truncate: got %q ok=%v", got, ok) + } + _, ok = truncateRunes("hello", 0) + if ok { + t.Fatal("expected refusal when max cannot hold ellipsis") + } +} + +func TestDiscord_Send_TruncatesDescriptionToLimit(t *testing.T) { + var got discordPayload + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("invalid JSON: %v", err) + } + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + d := NewDiscord("discord-main", srv.URL, 5*time.Second) + msg := canonical.Message{ + Title: "T", + Body: strings.Repeat("d", DiscordDescriptionMax+50), + Level: "info", + } + if err := d.Send(context.Background(), msg); err != nil { + t.Fatalf("Send: %v", err) + } + if len(got.Embeds) != 1 { + t.Fatalf("embeds = %d", len(got.Embeds)) + } + desc := got.Embeds[0].Description + if len(desc) > DiscordDescriptionMax { + t.Fatalf("description len %d exceeds %d", len(desc), DiscordDescriptionMax) + } + if !strings.HasSuffix(desc, truncateSuffix) { + t.Fatalf("expected truncation marker, got len=%d", len(desc)) + } +} + +func TestDiscord_Send_PayloadLimitErrorHasNoSecrets(t *testing.T) { + // Title alone cannot exceed DiscordTitleMax after truncateRunes; force the + // total-embed failure path by stuffing title, footer, tags, and body so + // the remaining description budget collapses below the ellipsis size. + msg := canonical.Message{ + Title: strings.Repeat("t", DiscordTitleMax), + Body: "x", + Source: strings.Repeat("s", DiscordFooterMax), + Tags: []string{strings.Repeat("g", DiscordFieldValueMax)}, + } + // Directly exercise fitDiscordEmbed with a crafted embed that cannot fit: + // zero description budget after overhead. + embed := discordEmbed{ + Title: strings.Repeat("t", DiscordTitleMax), + Description: "", + Footer: &discordFooter{Text: strings.Repeat("s", DiscordFooterMax)}, + Fields: []discordField{{ + Name: "tags", + Value: strings.Repeat("g", DiscordFieldValueMax), + }}, + } + if embedCharCount(embed) <= DiscordEmbedTotalMax { + t.Skip("fixture does not exceed total embed ceiling on this platform") + } + _ = msg + d := NewDiscord("discord-main", "http://127.0.0.1:9/secret-webhook-token", time.Second) + // Oversized title after identity cannot happen; instead verify SafeError + // never echoes webhook URLs when payload_limit is returned. + err := payloadLimitError("discord") + safe := SafeError(err) + if strings.Contains(safe, "secret-webhook") || strings.Contains(safe, "http") { + t.Fatalf("SafeError leaked credential material: %q", safe) + } + if !strings.Contains(safe, "payload_limit") { + t.Fatalf("SafeError = %q, want payload_limit", safe) + } + _ = d +} + +func TestTelegram_Send_TruncatesToTextMax(t *testing.T) { + var got tgPayload + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &got) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true}`)) + })) + defer srv.Close() + + tg := NewTelegram("tg", srv.URL, "TOKEN", "1", 5*time.Second) + msg := canonical.Message{ + Title: "Build", + Body: strings.Repeat("b", TelegramTextMax+200), + Level: "info", + } + if err := tg.Send(context.Background(), msg); err != nil { + t.Fatalf("Send: %v", err) + } + if len(got.Text) > TelegramTextMax { + t.Fatalf("text len %d exceeds %d", len(got.Text), TelegramTextMax) + } +} + +func TestTelegram_Send_PayloadLimitWhenTitleAloneExceeds(t *testing.T) { + // A title that escapes to more than TelegramTextMax cannot be repaired by + // shrinking the body. + tg := NewTelegram("tg", "http://127.0.0.1:9", "SECRETTOKEN", "1", time.Second) + msg := canonical.Message{ + Title: strings.Repeat(".", TelegramTextMax+10), // each "." escapes to "\." + Body: "ok", + } + err := tg.Send(context.Background(), msg) + if err == nil { + t.Fatal("expected payload_limit error") + } + var de *DeliveryError + if !errors.As(err, &de) || de.Cause != "payload_limit" { + t.Fatalf("err = %v, want payload_limit DeliveryError", err) + } + safe := SafeError(err) + if strings.Contains(safe, "SECRETTOKEN") || strings.Contains(safe, msg.Title[:16]) { + t.Fatalf("SafeError leaked secrets or payload: %q", safe) + } +} + +func TestSignal_Send_TruncatesToMessageMax(t *testing.T) { + var got signalPayload + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &got) + w.WriteHeader(http.StatusCreated) + })) + defer srv.Close() + + s := NewSignal("sig", srv.URL, "+15550001111", "uuid", 5*time.Second) + msg := canonical.Message{ + Title: "Alert", + Body: strings.Repeat("s", SignalMessageMax+100), + Level: "error", + } + if err := s.Send(context.Background(), msg); err != nil { + t.Fatalf("Send: %v", err) + } + if len(got.Message) > SignalMessageMax { + t.Fatalf("message len %d exceeds %d", len(got.Message), SignalMessageMax) + } + if !strings.HasSuffix(got.Message, truncateSuffix) { + t.Fatalf("expected truncation marker in message") + } +} diff --git a/stations/notify/internal/channels/signal.go b/stations/notify/internal/channels/signal.go index 7ec3c2c6..58f19475 100644 --- a/stations/notify/internal/channels/signal.go +++ b/stations/notify/internal/channels/signal.go @@ -42,7 +42,10 @@ type signalRequest struct { } func (s *Signal) Send(ctx context.Context, m canonical.Message) error { - text := formatSignal(m) + text, err := fitSignalText(m) + if err != nil { + return err + } payload := signalRequest{ Message: text, Number: s.from, @@ -88,3 +91,26 @@ func formatSignal(m canonical.Message) string { } return sb.String() } + +func fitSignalText(m canonical.Message) (string, error) { + text := formatSignal(m) + if len(text) <= SignalMessageMax { + return text, nil + } + overhead := len(text) - len(m.Body) + budget := SignalMessageMax - overhead + if budget < len(truncateSuffix) { + return "", payloadLimitError("signal") + } + body, ok := truncateRunes(m.Body, budget) + if !ok { + return "", payloadLimitError("signal") + } + trial := m + trial.Body = body + out := formatSignal(trial) + if len(out) > SignalMessageMax { + return "", payloadLimitError("signal") + } + return out, nil +} diff --git a/stations/notify/internal/channels/telegram.go b/stations/notify/internal/channels/telegram.go index 12bbd32e..fbf82ca3 100644 --- a/stations/notify/internal/channels/telegram.go +++ b/stations/notify/internal/channels/telegram.go @@ -45,7 +45,10 @@ type tgRequest struct { } func (t *Telegram) Send(ctx context.Context, m canonical.Message) error { - text := formatTelegram(m) + text, err := fitTelegramText(m) + if err != nil { + return err + } payload := tgRequest{ ChatID: t.chatID, Text: text, @@ -94,6 +97,41 @@ func formatTelegram(m canonical.Message) string { return sb.String() } +// fitTelegramText formats m and ensures the MarkdownV2 payload stays within +// TelegramTextMax. Oversized bodies are truncated before escaping so a cut +// cannot land inside an escape sequence; if the title/tags alone still exceed +// the ceiling the send fails with a bounded payload_limit error. +func fitTelegramText(m canonical.Message) (string, error) { + text := formatTelegram(m) + if len(text) <= TelegramTextMax { + return text, nil + } + // Shrink the body until the escaped payload fits. + lo, hi := 0, len(m.Body) + best := "" + for lo <= hi { + mid := (lo + hi) / 2 + trial := m + body, ok := truncateRunes(m.Body, mid) + if !ok { + hi = mid - 1 + continue + } + trial.Body = body + candidate := formatTelegram(trial) + if len(candidate) <= TelegramTextMax { + best = candidate + lo = mid + 1 + } else { + hi = mid - 1 + } + } + if best == "" { + return "", payloadLimitError("telegram") + } + return best, nil +} + // escapeMDV2 escapes the characters Telegram MarkdownV2 requires escaping // when they appear in text (per Bot API docs). func escapeMDV2(s string) string { From c6e4c95979405c44dd92866d5762374704df6ed8 Mon Sep 17 00:00:00 2001 From: Solomon Neas Date: Sat, 1 Aug 2026 20:14:28 -0400 Subject: [PATCH 2/2] fix(notify): address payload review findings Co-Authored-By: Codex --- stations/notify/cmd/agent-notify/main_test.go | 9 +-- .../notify/internal/channels/limits_test.go | 75 +++++++++++-------- stations/notify/internal/channels/telegram.go | 3 +- 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/stations/notify/cmd/agent-notify/main_test.go b/stations/notify/cmd/agent-notify/main_test.go index 1d54fde0..f53b3f1c 100644 --- a/stations/notify/cmd/agent-notify/main_test.go +++ b/stations/notify/cmd/agent-notify/main_test.go @@ -14,6 +14,7 @@ import ( "testing" "time" + "github.com/escoffier-labs/agent-notify/internal/adapter" "github.com/escoffier-labs/agent-notify/internal/canonical" "github.com/escoffier-labs/agent-notify/internal/channels" "github.com/escoffier-labs/agent-notify/internal/config" @@ -783,7 +784,7 @@ func TestRun_OversizedStdinRejected(t *testing.T) { })) defer srv.Close() - oversized := strings.Repeat("x", adapterMaxInputBytes()+1) + oversized := strings.Repeat("x", adapter.MaxInputBytes+1) code, _, stderr := runMain(t, []string{"agent-notify"}, oversized, @@ -865,9 +866,3 @@ chat_id_env = "TELEGRAM_CHAT_ID" t.Fatalf("stderr leaked bot token: %q", stderr) } } - -// adapterMaxInputBytes mirrors adapter.MaxInputBytes without importing the -// adapter package into every CLI assertion helper. -func adapterMaxInputBytes() int { - return 256 * 1024 -} diff --git a/stations/notify/internal/channels/limits_test.go b/stations/notify/internal/channels/limits_test.go index 6aae6ca4..004381e1 100644 --- a/stations/notify/internal/channels/limits_test.go +++ b/stations/notify/internal/channels/limits_test.go @@ -61,43 +61,32 @@ func TestDiscord_Send_TruncatesDescriptionToLimit(t *testing.T) { } } -func TestDiscord_Send_PayloadLimitErrorHasNoSecrets(t *testing.T) { - // Title alone cannot exceed DiscordTitleMax after truncateRunes; force the - // total-embed failure path by stuffing title, footer, tags, and body so - // the remaining description budget collapses below the ellipsis size. - msg := canonical.Message{ - Title: strings.Repeat("t", DiscordTitleMax), - Body: "x", - Source: strings.Repeat("s", DiscordFooterMax), - Tags: []string{strings.Repeat("g", DiscordFieldValueMax)}, - } - // Directly exercise fitDiscordEmbed with a crafted embed that cannot fit: - // zero description budget after overhead. - embed := discordEmbed{ - Title: strings.Repeat("t", DiscordTitleMax), - Description: "", - Footer: &discordFooter{Text: strings.Repeat("s", DiscordFooterMax)}, - Fields: []discordField{{ - Name: "tags", - Value: strings.Repeat("g", DiscordFieldValueMax), - }}, - } - if embedCharCount(embed) <= DiscordEmbedTotalMax { - t.Skip("fixture does not exceed total embed ceiling on this platform") - } - _ = msg - d := NewDiscord("discord-main", "http://127.0.0.1:9/secret-webhook-token", time.Second) - // Oversized title after identity cannot happen; instead verify SafeError - // never echoes webhook URLs when payload_limit is returned. +func TestDiscord_OverheadCeilingLeavesDescriptionBudget(t *testing.T) { + // Guards the arithmetic that makes payload_limit unreachable from + // Discord.Send: if per-field ceilings ever grow so the description budget + // can collapse below the ellipsis, Send must be tested through Send. + maxOverhead := DiscordTitleMax + DiscordFooterMax + len("tags") + DiscordFieldValueMax + if budget := DiscordEmbedTotalMax - maxOverhead; budget < len(truncateSuffix) { + t.Fatalf("description budget %d < ellipsis %d: payload_limit reachable via Send", budget, len(truncateSuffix)) + } +} + +func TestSafeError_PayloadLimitHasNoSecrets(t *testing.T) { + // payload_limit is not reachable from Discord.Send under the current + // per-field ceilings: maximum non-description overhead is + // DiscordTitleMax + DiscordFooterMax + len("tags") + DiscordFieldValueMax + // = 3332, leaving a description budget of 6000-3332 = 2668, far above + // len(truncateSuffix), so fitDiscordEmbed always truncates and sends (a + // large-body Send fixture only truncates). The payload_limit + // sanitization contract is therefore tested directly here. err := payloadLimitError("discord") safe := SafeError(err) - if strings.Contains(safe, "secret-webhook") || strings.Contains(safe, "http") { + if strings.Contains(safe, "http") || strings.Contains(safe, "webhook") { t.Fatalf("SafeError leaked credential material: %q", safe) } if !strings.Contains(safe, "payload_limit") { t.Fatalf("SafeError = %q, want payload_limit", safe) } - _ = d } func TestTelegram_Send_TruncatesToTextMax(t *testing.T) { @@ -124,6 +113,32 @@ func TestTelegram_Send_TruncatesToTextMax(t *testing.T) { } } +func TestTelegram_FitShortBodyNearLimitOverhead(t *testing.T) { + // Regression: fitTelegramText's binary search shrank hi when truncateRunes + // rejected a too-small mid, discarding every larger candidate. A short + // body whose fixed overhead lands just under TelegramTextMax then failed + // with payload_limit even though an ellipsis-only body fits. + msg := canonical.Message{Level: "info", Body: "abcde"} + // Grow a plain (escape-free) title until the full text just exceeds + // TelegramTextMax; collapsing the 5-byte body to the ellipsis then fits. + fixed := len(formatTelegram(msg)) // indicator + " " + body, no title + titleLen := TelegramTextMax + 2 - fixed - 3 // 3 = "*", title, "*\n" + msg.Title = strings.Repeat("x", titleLen) + if got := len(formatTelegram(msg)); got <= TelegramTextMax { + t.Fatalf("fixture must exceed TelegramTextMax, got %d", got) + } + text, err := fitTelegramText(msg) + if err != nil { + t.Fatalf("fitTelegramText: %v; short body near-limit overhead must truncate, not fail", err) + } + if len(text) > TelegramTextMax { + t.Fatalf("text len %d exceeds %d", len(text), TelegramTextMax) + } + if !strings.Contains(text, truncateSuffix) { + t.Fatal("expected truncation marker in fitted text") + } +} + func TestTelegram_Send_PayloadLimitWhenTitleAloneExceeds(t *testing.T) { // A title that escapes to more than TelegramTextMax cannot be repaired by // shrinking the body. diff --git a/stations/notify/internal/channels/telegram.go b/stations/notify/internal/channels/telegram.go index fbf82ca3..e1b19ec9 100644 --- a/stations/notify/internal/channels/telegram.go +++ b/stations/notify/internal/channels/telegram.go @@ -114,7 +114,8 @@ func fitTelegramText(m canonical.Message) (string, error) { trial := m body, ok := truncateRunes(m.Body, mid) if !ok { - hi = mid - 1 + // mid is too small to hold the ellipsis; only larger cuts can work. + lo = mid + 1 continue } trial.Body = body