diff --git a/CHANGELOG.md b/CHANGELOG.md index 156bafc0..125070b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ All notable changes to GoClaw are documented here. For full documentation, see [ ## Unreleased +### Changed + +- **Bitrix24 channel migrated to imbot v2 messaging API** — outbound text now uses + `imbot.v2.Chat.Message.send` (replacing `imbot.message.add`); bot verification/lookup + uses `imbot.v2.Bot.list` (replacing `imbot.bot.list` + the legacy `imbot.list` fallback); + bot teardown uses `imbot.v2.Bot.unregister` (replacing `imbot.unregister`). Bot + registration intentionally stays on v1 `imbot.register` — v2 `imbot.v2.Bot.register` + changes the event-delivery model (per-event handler URLs → `eventMode`), which would + require rewriting the inbound event parser. No user-facing behavior change. + ### Added - **Behavior UX sidecar delivery overrides** — Adds sidecar-generated Quick @@ -21,6 +31,14 @@ All notable changes to GoClaw are documented here. For full documentation, see [ pre-write discovery via `vault_search`, `memory_search`, and `knowledge_graph_search` to surface related files before writing and avoid duplicates; documents Vault scope mirroring and id-routing rules. +- **Bitrix24 channel 2-way media (file) transfer** — Inbound media downloads via + `imbot.v2.File.download` (one-time authenticated URL) with MIME preservation for + images, PDFs, audio, and video. Outbound uploads via `imbot.v2.File.upload` (base64). + Shared `media_max_mb` config knob (default 20 MB) caps both directions. Requires + `imbot` OAuth scope (no `disk` scope needed). Inbound handled by new + `internal/channels/bitrix24/download.go`; outbound by `send_media.go`. New + `BaseChannel.HandleMessageMedia()` method centralizes media-aware message handling. + See `docs/05-channels-messaging.md` § 16 (Bitrix24) for configuration. - **Skill agent manage grants** — Adds per-agent skill edit/delete grants with backend checks, HTTP/WS support, SQLite and PostgreSQL schema updates, and web diff --git a/cmd/gateway.go b/cmd/gateway.go index 30916757..6611d857 100644 --- a/cmd/gateway.go +++ b/cmd/gateway.go @@ -604,8 +604,10 @@ func runGateway() { // lazy per-user credential provisioning (via mcp_server_name + // mcp_base_url in their instance config) can reach the partner's // MCPServerStore. The MCP server authenticates each onboard call - // via the caller-supplied Bitrix access_token (Path B) — no shared - // admin secret is required. Channels with none of those set operate + // via the caller-supplied Bitrix access_token (the "Bitrix24 + // OAuth → existing mcp_user_credentials bridge" — Bitrix-specific + // glue, not a generic MCP architecture pattern) — no shared admin + // secret is required. Channels with none of those set operate // identically to before — the MCPStore arg is nil-safe inside the // factory. instanceLoader.RegisterFactory(channels.TypeBitrix24, bitrix24.FactoryWithPortalStoreAndMCP(pgStores.BitrixPortals, pgStores.MCP, bitrixEncKey)) diff --git a/cmd/gateway_consumer_normal.go b/cmd/gateway_consumer_normal.go index e61735bb..f36638a8 100644 --- a/cmd/gateway_consumer_normal.go +++ b/cmd/gateway_consumer_normal.go @@ -13,6 +13,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/agent" "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/channels" + "github.com/nextlevelbuilder/goclaw/internal/channels/bitrix24" "github.com/nextlevelbuilder/goclaw/internal/channels/telegram/voiceguard" "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/scheduler" @@ -83,21 +84,10 @@ func processNormalMessage( } // Group-scoped UserID: context files, memory, traces, and seeding scope. - // - Discord guilds: "guild:{guildID}:user:{senderID}" — per-user per-server, - // shared across all channels within the same server. Session key stays per-channel. - // - Other platforms: "group:{channel}:{chatID}" — shared by all users in the chat. - // Individual senderID is preserved in InboundMessage for pairing/dedup/mention gate. - userID := msg.UserID - if peerKind == string(sessions.PeerGroup) && msg.ChatID != "" { - if guildID := msg.Metadata["guild_id"]; guildID != "" && msg.SenderID != "" { - // Discord guild: per-user scope so each member has own profile - // across all channels in the same server. - userID = fmt.Sprintf("guild:%s:user:%s", guildID, msg.SenderID) - } else { - groupID := msg.ChatID - userID = fmt.Sprintf("group:%s:%s", msg.Channel, groupID) - } - } + // See deriveGroupUserID for the precedence (Discord guild → openline + // participant → group fallback). Individual senderID is preserved in + // InboundMessage for pairing/dedup/mention gating regardless of scope. + userID := deriveGroupUserID(msg, peerKind) // Persist friendly names from channel metadata into session + user profile. sessionMeta := extractSessionMetadata(msg, peerKind) @@ -234,6 +224,22 @@ func processNormalMessage( } } + // Forward Bitrix24-specific routing keys so Send() can: + // 1. Branch v2 public vs v1 whisper (bitrix_visibility) + // 2. Set fields.replyId on v2 public reply (bitrix_message_id) + // CopyFinalRoutingMeta is channel-agnostic and doesn't include these. + if v := msg.Metadata[bitrix24.MetaKeyVisibility]; v != "" { + outMeta[bitrix24.MetaKeyVisibility] = v + } + if v := msg.Metadata[bitrix24.MetaKeyMessageID]; v != "" { + outMeta[bitrix24.MetaKeyMessageID] = v + } + // Openline sender tag captured on inbound → Send() prepends it to the reply + // so the connector routes the answer back to the right external user. + if v := msg.Metadata[bitrix24.MetaKeySenderPrefix]; v != "" { + outMeta[bitrix24.MetaKeySenderPrefix] = v + } + // Register run with channel manager for streaming/reaction event forwarding. // Use localKey (composite key with topic suffix) so streaming/reaction events // route to the correct per-topic state in the channel. @@ -698,3 +704,32 @@ func isSafeBitrixEntityToken(s string, maxLen int) bool { } return true } + +// deriveGroupUserID computes the per-message scope userID used for context +// files, memory, traces, and seeding. Direct messages keep msg.UserID. Group +// messages pick a synthetic scope, in precedence order: +// +// 1. Discord guild member: "guild:{guildID}:user:{senderID}" — per-user across +// every channel in the same server. +// 2. Openline participant: the per-participant id minted by bitrix24/handle.go +// ("openlines:{instance}:{chat}:{uid}") when a connector relayed a customer +// message carrying a stable uid — so each external person gets their own +// USER.md / memory instead of collapsing into the shared connector proxy. +// Absent for legacy/name-only/operator messages → falls through. +// 3. Group fallback: "group:{channel}:{chatID}" — shared by everyone in the chat. +// +// The individual senderID stays on InboundMessage for pairing / dedup / mention +// gating regardless of which scope is chosen. +func deriveGroupUserID(msg bus.InboundMessage, peerKind string) string { + if peerKind != string(sessions.PeerGroup) || msg.ChatID == "" { + return msg.UserID + } + switch { + case msg.Metadata["guild_id"] != "" && msg.SenderID != "": + return fmt.Sprintf("guild:%s:user:%s", msg.Metadata["guild_id"], msg.SenderID) + case msg.Metadata[bitrix24.MetaKeyParticipantUserID] != "": + return msg.Metadata[bitrix24.MetaKeyParticipantUserID] + default: + return fmt.Sprintf("group:%s:%s", msg.Channel, msg.ChatID) + } +} diff --git a/cmd/gateway_consumer_normal_test.go b/cmd/gateway_consumer_normal_test.go index f93dcaca..64017f43 100644 --- a/cmd/gateway_consumer_normal_test.go +++ b/cmd/gateway_consumer_normal_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/channels/bitrix24" + "github.com/nextlevelbuilder/goclaw/internal/sessions" ) // TestIsSafeBitrixEntityToken pins the validation contract for webhook-sourced @@ -44,6 +46,90 @@ func TestIsSafeBitrixEntityToken(t *testing.T) { } } +// TestDeriveGroupUserID pins the group-scope userID precedence: Discord guild +// member → openline participant → group fallback, with direct messages passing +// msg.UserID through untouched. The openline participant branch is what gives +// each Zalo customer its own per-person scope instead of the shared connector +// proxy. +func TestDeriveGroupUserID(t *testing.T) { + const group = string(sessions.PeerGroup) + const direct = string(sessions.PeerDirect) + cases := []struct { + name string + msg bus.InboundMessage + peerKind string + want string + }{ + { + name: "openline participant overrides group fallback", + msg: bus.InboundMessage{ + Channel: "zalo_ol", + ChatID: "chat4878", + SenderID: "openlines:tamgiac:chat4878:111222", + UserID: "openlines:tamgiac:chat4878:111222", + Metadata: map[string]string{ + bitrix24.MetaKeyParticipantUserID: "openlines:tamgiac:chat4878:111222", + }, + }, + peerKind: group, + want: "openlines:tamgiac:chat4878:111222", + }, + { + name: "no participant id falls back to group-level", + msg: bus.InboundMessage{ + Channel: "zalo_ol", + ChatID: "chat4878", + UserID: "960", + }, + peerKind: group, + want: "group:zalo_ol:chat4878", + }, + { + name: "empty participant id (parse-fail degrade) falls back to group-level", + msg: bus.InboundMessage{ + Channel: "zalo_ol", + ChatID: "chat4878", + UserID: "960", + Metadata: map[string]string{bitrix24.MetaKeyParticipantUserID: ""}, + }, + peerKind: group, + want: "group:zalo_ol:chat4878", + }, + { + name: "discord guild takes precedence over participant id", + msg: bus.InboundMessage{ + Channel: "discord", + ChatID: "chan-1", + SenderID: "u-9", + Metadata: map[string]string{ + "guild_id": "g-1", + bitrix24.MetaKeyParticipantUserID: "openlines:x:y:z", + }, + }, + peerKind: group, + want: "guild:g-1:user:u-9", + }, + { + name: "direct message passes UserID through untouched", + msg: bus.InboundMessage{ + Channel: "zalo_ol", + ChatID: "chat4878", + UserID: "42", + Metadata: map[string]string{bitrix24.MetaKeyParticipantUserID: "openlines:x:y:z"}, + }, + peerKind: direct, + want: "42", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := deriveGroupUserID(tc.msg, tc.peerKind); got != tc.want { + t.Errorf("deriveGroupUserID() = %q; want %q", got, tc.want) + } + }) + } +} + func TestResolveSenderNameReadsWhatsAppUserName(t *testing.T) { got := resolveSenderName(bus.InboundMessage{ Metadata: map[string]string{ diff --git a/docs/05-channels-messaging.md b/docs/05-channels-messaging.md index 43d1af24..4dbd687b 100644 --- a/docs/05-channels-messaging.md +++ b/docs/05-channels-messaging.md @@ -234,22 +234,22 @@ flowchart TD ## 4. Channel Comparison -| Feature | Telegram | Feishu/Lark | Discord | Slack | WhatsApp | Zalo OA | Zalo Personal | -|---------|----------|-------------|---------|-------|----------|---------|---------------| -| Connection | Long polling | WS (default) / Webhook | Gateway events | Socket Mode | Direct protocol (in-process) | Long polling | Internal protocol | -| DM support | Yes | Yes | Yes | Yes | Yes | Yes (DM only) | Yes | -| Group support | Yes (mention gating) | Yes | Yes | Yes (mention gating + thread cache) | Yes | No | Yes | -| Forum/Topics | Yes (per-topic config) | Yes (topic session mode) | -- | -- | -- | -- | -- | -| Message limit | 4,096 chars | Configurable (default 4,000) | 2,000 chars | 4,000 chars | WhatsApp native limit | 2,000 chars | 2,000 chars | -| Streaming | Typing indicator | Streaming message cards | Edit "Thinking..." | Edit "Thinking..." (throttled 1s) | No | No | No | -| Media | Photos, voice, files | Images, files (30 MB) | Files, embeds | Files (download w/ SSRF protection) | Images, audio, video, documents | Images (5 MB) | -- | -| Speech-to-text | Yes (STT proxy) | -- | -- | -- | -- | -- | -- | -| Voice routing | Yes (VoiceAgentID) | -- | -- | -- | -- | -- | -- | -| Rich formatting | Markdown → HTML | Card messages | Markdown | Markdown → mrkdwn | Plain text | Plain text | Plain text | -| Bot commands | 10+ commands | -- | -- | -- | -- | -- | -- | -| Tool allow list | Per-topic | -- | -- | -- | -- | -- | -- | -| Pairing support | Yes | Yes | Yes | Yes | Yes | Yes | Yes | -| Status reactions | Yes | Yes | -- | Yes | -- | -- | -- | +| Feature | Telegram | Feishu/Lark | Discord | Slack | WhatsApp | Zalo OA | Zalo Personal | Bitrix24 | +|---------|----------|-------------|---------|-------|----------|---------|---------------|----------| +| Connection | Long polling | WS (default) / Webhook | Gateway events | Socket Mode | Direct protocol (in-process) | Long polling | Internal protocol | Long polling (REST) | +| DM support | Yes | Yes | Yes | Yes | Yes | Yes (DM only) | Yes | Yes | +| Group support | Yes (mention gating) | Yes | Yes | Yes (mention gating + thread cache) | Yes | No | Yes | Yes | +| Forum/Topics | Yes (per-topic config) | Yes (topic session mode) | -- | -- | -- | -- | -- | -- | +| Message limit | 4,096 chars | Configurable (default 4,000) | 2,000 chars | 4,000 chars | WhatsApp native limit | 2,000 chars | 2,000 chars | 4,096 chars | +| Streaming | Typing indicator | Streaming message cards | Edit "Thinking..." | Edit "Thinking..." (throttled 1s) | No | No | No | No | +| Media | Photos, voice, files | Images, files (30 MB) | Files, embeds | Files (download w/ SSRF protection) | Images, audio, video, documents | Images (5 MB) | -- | Files (20 MB default) | +| Speech-to-text | Yes (STT proxy) | -- | -- | -- | -- | -- | -- | -- | +| Voice routing | Yes (VoiceAgentID) | -- | -- | -- | -- | -- | -- | -- | +| Rich formatting | Markdown → HTML | Card messages | Markdown | Markdown → mrkdwn | Plain text | Plain text | Plain text | Plain text | +| Bot commands | 10+ commands | -- | -- | -- | -- | -- | -- | -- | +| Tool allow list | Per-topic | -- | -- | -- | -- | -- | -- | -- | +| Pairing support | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | +| Status reactions | Yes | Yes | -- | Yes | -- | -- | -- | -- | --- @@ -781,12 +781,53 @@ flowchart TD --- +## 16. Bitrix24 + +The Bitrix24 channel connects to a Bitrix24 portal via the `imbot.v2.*` REST API. Authentication uses an app token with `imbot` scope. + +### Key Behaviors + +- **Text limit**: 4,096 characters per message with automatic splitting at newlines +- **Media support**: Both inbound and outbound file transfers with MIME preservation +- **Default DM policy**: `"pairing"` (requires pairing code) +- **Pairing debounce**: 60-second debounce on pairing instructions +- **Media max size**: Configurable `media_max_mb` (default 20 MB) applies symmetrically to inbound downloads and outbound uploads + +### Inbound Media + +When a user sends a file to the bot: +1. Resolve file metadata via `imbot.v2.File.download` → obtain one-time authenticated download URL +2. Stream file to temp directory, preserving MIME type +3. Forward to agent via `bus.MediaFile` with original filename +4. Agent pipeline routes to appropriate reader (`read_image`, `read_document`, `read_audio`, `read_video`) + +**Configuration**: Size cap via `media_max_mb` (per `channel_instance` or config default). Oversized files are silently skipped (best-effort). + +### Outbound Media + +Agent-produced media files are uploaded to the chat via `imbot.v2.File.upload`: +1. Read file from workspace +2. Encode as base64 +3. POST to `imbot.v2.File.upload` with bot ID and chat ID +4. Upload succeeds atomically: file is stored in portal Drive, attached to chat, and posted in a single REST call + +**Configuration**: Size cap via same `media_max_mb` knob (symmetric with inbound). + +### OAuth Scope + +The bot app must have the `imbot` scope granted. The `disk` scope is **not** required — all file operations are scoped to the message thread context. + +**Implementation**: `internal/channels/bitrix24/download.go` (inbound), `send_media.go` (outbound). New `BaseChannel.HandleMessageMedia()` method (in `internal/channels/channel.go`) centralizes media-aware message handling across all channels. + +--- + ## File Reference | Module | Path | Purpose | |---|---|---| -| Channel core | `internal/channels/` | `Channel` interface, `BaseChannel`, `Manager` (StartAll/StopAll), outbound dispatcher, DB instance loader | -| Platform adapters | `internal/channels/{telegram,feishu,discord,slack,whatsapp,zalo}/` | Per-platform: message handling, formatting, streaming, reactions, media, pairing | +| Channel core | `internal/channels/` | `Channel` interface, `BaseChannel` (incl. `HandleMessageMedia()` method), `Manager` (StartAll/StopAll), outbound dispatcher, DB instance loader | +| Platform adapters | `internal/channels/{telegram,feishu,discord,slack,whatsapp,zalo,bitrix24}/` | Per-platform: message handling, formatting, streaming, reactions, media, pairing | +| Bitrix24 media | `internal/channels/bitrix24/download.go`, `send_media.go` | Inbound file download via `imbot.v2.File.download`, outbound upload via `imbot.v2.File.upload` | | Audio / STT | `internal/audio/` | Audio manager, STT chain resolution, legacy STT bridge | | Pairing & routing | `internal/store/pg/pairing.go`, `cmd/gateway_consumer.go` | Pairing code persistence, inbound message routing and cancel interception | diff --git a/internal/agent/loop_media.go b/internal/agent/loop_media.go index 7ad2118d..0c7ce4f7 100644 --- a/internal/agent/loop_media.go +++ b/internal/agent/loop_media.go @@ -45,30 +45,39 @@ func parseMediaResult(toolOutput string) *MediaResult { } } -// extractMediaFromContent scans text for MEDIA: tokens the LLM may echo -// in its final response (e.g. when a tool returned the MEDIA: prefix as plain -// text instead of setting Result.Media). Relative paths are resolved against -// workspace. Called before sanitize strips the tokens so the attachments are -// still delivered. +// confineToWorkspace validates that mediaPath resolves to a regular file located +// inside workspace, then returns the cleaned path. It is the single source of +// truth for the media path-containment boundary, shared by the two feeders of +// MediaResult.Path: extractMediaFromContent (LLM-echoed tokens) and the +// parseMediaResult sink in processToolResult (tool MEDIA: output). Constraining +// at this boundary protects every outbound channel at once — a path that escapes +// the workspace never reaches a channel's file-upload egress. // -// Security: only paths that (a) exist on disk and (b) resolve inside the -// workspace root are accepted. An LLM cannot inject attachments pointing at -// /etc/passwd, a sibling tenant's workspace, or a hallucinated path — the -// extractor silently drops them. When workspace is empty, only legacy absolute -// paths from tool outputs (via parseMediaResult's upstream flow) are trusted; -// LLM-echoed absolute paths without a workspace context are dropped. -func extractMediaFromContent(content, workspace string) []MediaResult { - if !strings.Contains(content, "MEDIA:") || workspace == "" { - return nil - } - matches := mediaPathPattern.FindAllString(content, -1) - if len(matches) == 0 { - return nil +// Containment applies, in order: +// - relative paths are resolved against the workspace root; +// - Lstat (not Stat) rejects a symlink at the leaf outright; +// - EvalSymlinks resolves ancestor symlinks before the Rel check, so a +// "//secret" escape via a dir symlink pointing outside the +// workspace is caught (a purely lexical Rel check would miss it). +// +// Returns the cleaned (symlink-preserving) path and true when the file is safe +// to ship, or "", false when it must be dropped. An empty workspace yields +// false: without a boundary there is nothing to validate against, and an +// unvalidatable path must never reach an external egress. +// +// NOTE: the returned path is `cleaned`, NOT the symlink-resolved path. resolved +// is used ONLY for the containment check — downstream readers (channel senders, +// history, dedup) must see the same path semantics the tool emitted, otherwise +// workspaces backed by bind-mounts / dir symlinks suffer dedup misses (observed +// in production). +func confineToWorkspace(mediaPath, workspace string) (string, bool) { + if mediaPath == "" || workspace == "" { + return "", false } // Resolve workspace to its real path (follows symlinks). Required because // macOS uses symlinks for /tmp → /private/tmp; if we only Clean the - // workspace but EvalSymlinks the extracted paths, the Rel check below - // would spuriously fail even for legitimate files. + // workspace but EvalSymlinks the candidate path, the Rel check below would + // spuriously fail even for legitimate files. wsRoot := "" if abs, err := filepath.Abs(workspace); err == nil { if resolved, err := filepath.EvalSymlinks(abs); err == nil { @@ -78,6 +87,62 @@ func extractMediaFromContent(content, workspace string) []MediaResult { } } if wsRoot == "" { + return "", false + } + path := mediaPath + if !filepath.IsAbs(path) { + path = filepath.Join(wsRoot, path) + } + cleaned := filepath.Clean(path) + info, err := os.Lstat(cleaned) + if err != nil || !info.Mode().IsRegular() { + return "", false + } + resolved, err := filepath.EvalSymlinks(cleaned) + if err != nil { + return "", false + } + rel, err := filepath.Rel(wsRoot, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", false + } + return cleaned, true +} + +// confineToAnyRoot accepts mediaPath if it is contained by ANY of the allowed +// roots (each checked with the hardened confineToWorkspace). Used for the +// result.Media egress: a tool's media legitimately lives in the agent +// workspace, the team workspace, OR a tenant-allowed path — the same scopes the +// producing tools (create_*, send_file, delegate) validate against. A path +// outside every root (e.g. /etc/passwd from a prompt-injected path) is rejected, +// so the egress guard holds without dropping legitimate cross-workspace media. +func confineToAnyRoot(mediaPath string, roots []string) (string, bool) { + for _, root := range roots { + if root == "" { + continue + } + if cleaned, ok := confineToWorkspace(mediaPath, root); ok { + return cleaned, true + } + } + return "", false +} + +// extractMediaFromContent scans text for MEDIA: tokens the LLM may echo +// in its final response (e.g. when a tool returned the MEDIA: prefix as plain +// text instead of setting Result.Media). Relative paths are resolved against +// workspace. Called before sanitize strips the tokens so the attachments are +// still delivered. +// +// Security: only paths accepted by confineToWorkspace are emitted. An LLM cannot +// inject attachments pointing at /etc/passwd, a sibling tenant's workspace, or a +// hallucinated path — the extractor silently drops them. +func extractMediaFromContent(content, workspace string) []MediaResult { + if !strings.Contains(content, "MEDIA:") || workspace == "" { + return nil + } + matches := mediaPathPattern.FindAllString(content, -1) + if len(matches) == 0 { return nil } results := make([]MediaResult, 0, len(matches)) @@ -93,31 +158,8 @@ func extractMediaFromContent(content, workspace string) []MediaResult { if path == "" { continue } - // Resolve relative paths against workspace. - if !filepath.IsAbs(path) { - path = filepath.Join(wsRoot, path) - } - cleaned := filepath.Clean(path) - // Existence + regular-file check. Lstat (not Stat) so a symlink at - // the leaf is rejected outright. - info, err := os.Lstat(cleaned) - if err != nil || !info.Mode().IsRegular() { - continue - } - // Resolve ancestor symlinks THEN check containment. A purely lexical - // Rel check would pass "//secret" when symlink-dir - // points outside the workspace; EvalSymlinks closes that escape. - // NOTE: resolved is used ONLY for containment — the stored path stays - // `cleaned` so downstream readers (channel senders, history) use the - // same path semantics as the tool that wrote the file. Overwriting - // with the resolved path caused dedup misses in production when - // workspace contains bind-mounts / dir symlinks. - resolved, err := filepath.EvalSymlinks(cleaned) - if err != nil { - continue - } - rel, err := filepath.Rel(wsRoot, resolved) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + cleaned, ok := confineToWorkspace(path, workspace) + if !ok { continue } if _, dup := seen[cleaned]; dup { diff --git a/internal/agent/loop_media_test.go b/internal/agent/loop_media_test.go index 7b32d06e..c0e1d542 100644 --- a/internal/agent/loop_media_test.go +++ b/internal/agent/loop_media_test.go @@ -132,3 +132,127 @@ func TestExtractMediaFromContent(t *testing.T) { }) } } + +// TestConfineToWorkspace exercises the shared media path-containment boundary +// directly. It is the single guard that both feeders of MediaResult.Path rely +// on, so a regression here would reopen the outbound-exfiltration hole (H2). +func TestConfineToWorkspace(t *testing.T) { + wsRaw := t.TempDir() + ws, err := filepath.EvalSymlinks(wsRaw) + if err != nil { + t.Fatal(err) + } + insidePath := writeTempFile(t, ws, "deliver/report.pdf") + + // File outside the workspace (stands in for /etc/passwd). + outsideDir := t.TempDir() + outsidePath := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(outsidePath, nil, 0o644); err != nil { + t.Fatal(err) + } + + // Leaf symlink inside ws pointing outside: must be rejected by Lstat. + leafSymlink := filepath.Join(ws, "shortcut.txt") + symlinkSupported := os.Symlink(outsidePath, leafSymlink) == nil + + // Ancestor dir symlink inside ws pointing outside. + symDirParent := t.TempDir() + if err := os.WriteFile(filepath.Join(symDirParent, "victim.txt"), nil, 0o644); err != nil { + t.Fatal(err) + } + ancestorSym := filepath.Join(ws, "shared") + if symlinkSupported { + if err := os.Symlink(symDirParent, ancestorSym); err != nil { + symlinkSupported = false + } + } + + tests := []struct { + name string + path string + workspace string + wantOK bool + wantPath string + symlink bool // requires symlink support + }{ + {name: "relative inside workspace", path: "deliver/report.pdf", workspace: ws, wantOK: true, wantPath: insidePath}, + {name: "absolute inside workspace", path: insidePath, workspace: ws, wantOK: true, wantPath: insidePath}, + {name: "absolute outside workspace rejected", path: outsidePath, workspace: ws, wantOK: false}, + {name: "traversal escape rejected", path: "../secret.txt", workspace: ws, wantOK: false}, + {name: "missing file rejected", path: "nope.pdf", workspace: ws, wantOK: false}, + {name: "empty workspace rejected", path: insidePath, workspace: "", wantOK: false}, + {name: "empty path rejected", path: "", workspace: ws, wantOK: false}, + {name: "leaf symlink rejected", path: "shortcut.txt", workspace: ws, wantOK: false, symlink: true}, + {name: "ancestor symlink escape rejected", path: "shared/victim.txt", workspace: ws, wantOK: false, symlink: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.symlink && !symlinkSupported { + t.Skip("symlinks not supported on this platform") + } + got, ok := confineToWorkspace(tt.path, tt.workspace) + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v (got path %q)", ok, tt.wantOK, got) + } + if tt.wantOK && got != tt.wantPath { + t.Errorf("path = %q, want %q", got, tt.wantPath) + } + }) + } +} + +// TestParseMediaResultConfinedToWorkspace reproduces the processToolResult sink +// (parseMediaResult → confineToWorkspace) and asserts that a tool emitting a +// MEDIA: path outside the agent workspace is dropped, not shipped to a channel. +// This is the regression guard for H2: MEDIA:/etc/passwd must never become an +// outbound MediaResult. +func TestParseMediaResultConfinedToWorkspace(t *testing.T) { + wsRaw := t.TempDir() + ws, err := filepath.EvalSymlinks(wsRaw) + if err != nil { + t.Fatal(err) + } + insidePath := writeTempFile(t, ws, "chart.png") + + outsideDir := t.TempDir() + outsidePath := filepath.Join(outsideDir, "passwd") + if err := os.WriteFile(outsidePath, nil, 0o644); err != nil { + t.Fatal(err) + } + + // confineSink mirrors the loop_tools.go branch: parse, then confine. + confineSink := func(toolOutput string) (MediaResult, bool) { + mr := parseMediaResult(toolOutput) + if mr == nil { + return MediaResult{}, false + } + cleaned, ok := confineToWorkspace(mr.Path, ws) + if !ok { + return MediaResult{}, false + } + mr.Path = cleaned + return *mr, true + } + + t.Run("inside workspace shipped", func(t *testing.T) { + got, ok := confineSink("MEDIA:" + insidePath) + if !ok { + t.Fatal("expected in-workspace media to be shipped") + } + if got.Path != insidePath { + t.Errorf("path = %q, want %q", got.Path, insidePath) + } + }) + + t.Run("outside workspace dropped", func(t *testing.T) { + if _, ok := confineSink("MEDIA:" + outsidePath); ok { + t.Fatal("expected out-of-workspace media to be dropped") + } + }) + + t.Run("traversal dropped", func(t *testing.T) { + if _, ok := confineSink("MEDIA:../passwd"); ok { + t.Fatal("expected traversal media to be dropped") + } + }) +} diff --git a/internal/agent/loop_tools.go b/internal/agent/loop_tools.go index 0fefbc6c..3fd43ae4 100644 --- a/internal/agent/loop_tools.go +++ b/internal/agent/loop_tools.go @@ -86,19 +86,51 @@ func (l *Loop) processToolResult( // Collect MEDIA: paths from tool results. // Prefer result.Media (explicit) over ForLLM MEDIA: prefix (legacy) to avoid duplicates. if len(result.Media) > 0 { + // Egress containment: a tool that sets result.Media[].Path to a path + // outside every allowed scope (e.g. /etc/passwd from a prompt-injected + // path) must not reach a channel's file-upload sink. Confine here at the + // source so every channel is covered. The allowed roots mirror what the + // producing tools (create_*, send_file, delegate) may legitimately write + // to — agent workspace, team workspace, and tenant-allowed paths — so a + // cross-workspace file (e.g. a teammate-produced file in the shared team + // workspace, or a synchronous delegatee's output) is not wrongly dropped. + mediaRoots := append([]string{ + tools.ToolWorkspaceFromCtx(ctx), + tools.ToolTeamWorkspaceFromCtx(ctx), + }, l.tenantAllowedPaths...) for i, mf := range result.Media { + cleaned, ok := confineToAnyRoot(mf.Path, mediaRoots) + if !ok { + slog.Warn("security.media_path_rejected", + "agent", l.id, "tool", tc.Name, "path", mf.Path, + "reason", "outside agent workspace") + continue + } ct := mf.MimeType if ct == "" { - ct = mimeFromExt(filepath.Ext(mf.Path)) + ct = mimeFromExt(filepath.Ext(cleaned)) } - mr := MediaResult{Path: mf.Path, ContentType: ct, Caption: mf.Caption} + mr := MediaResult{Path: cleaned, ContentType: ct, Caption: mf.Caption} if result.MediaPrompts != nil { mr.Prompt = result.MediaPrompts[i] } rs.mediaResults = append(rs.mediaResults, mr) } } else if mr := parseMediaResult(result.ForLLM); mr != nil { - rs.mediaResults = append(rs.mediaResults, *mr) + // Security (egress boundary): a tool's MEDIA: output is taken + // verbatim, so confine it to the agent workspace before it can reach an + // outbound channel's file-upload sink (e.g. Bitrix imbot.v2.File.upload, + // Telegram sendDocument). A malicious or buggy tool emitting + // MEDIA:/etc/passwd is dropped here — fixing every channel at the source + // rather than per-channel. Mirrors extractMediaFromContent containment. + if cleaned, ok := confineToWorkspace(mr.Path, tools.ToolWorkspaceFromCtx(ctx)); ok { + mr.Path = cleaned + rs.mediaResults = append(rs.mediaResults, *mr) + } else { + slog.Warn("security.media_path_rejected", + "agent", l.id, "tool", tc.Name, "path", mr.Path, + "reason", "outside agent workspace") + } } // Auto-attach workspace media to task (covers create_image/audio/video). if teamWs := tools.ToolTeamWorkspaceFromCtx(ctx); teamWs != "" { diff --git a/internal/channels/bitrix24/channel.go b/internal/channels/bitrix24/channel.go index 2a92abb7..82849005 100644 --- a/internal/channels/bitrix24/channel.go +++ b/internal/channels/bitrix24/channel.go @@ -62,9 +62,10 @@ type Channel struct { // // mcpStore comes from the MCP-aware factory variant; mcpClient is // built at Start() iff config has mcp_server_name + mcp_base_url and - // the named mcp_servers row exists. Path B: the MCP server - // authenticates each onboard call via the caller-supplied Bitrix - // access_token — no shared admin secret is required. mcpServerID is + // the named mcp_servers row exists. Bitrix24 OAuth → existing + // mcp_user_credentials bridge: the MCP server authenticates each + // onboard call via the caller-supplied Bitrix access_token — no + // shared admin secret is required. mcpServerID is // resolved once at Start() via mcpStore.GetServerByName and then // cached — avoids looking up the server on every inbound message. mcpStore store.MCPServerStore diff --git a/internal/channels/bitrix24/download.go b/internal/channels/bitrix24/download.go new file mode 100644 index 00000000..a8e02997 --- /dev/null +++ b/internal/channels/bitrix24/download.go @@ -0,0 +1,192 @@ +package bitrix24 + +import ( + "context" + "encoding/json" + "fmt" + "io" + "log/slog" + "net/http" + "net/url" + "os" + "path/filepath" + "strings" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/channels/media" + "github.com/nextlevelbuilder/goclaw/internal/security" +) + +const ( + // maxInboundFiles caps how many attachments we download per message so a + // single event can't trigger an unbounded fan-out of REST + HTTP calls. + maxInboundFiles = 10 + // inboundDownloadTimeout bounds a single file download. The REST client's + // 15s timeout is tuned for JSON calls; media needs a longer, dedicated one. + inboundDownloadTimeout = 5 * time.Minute + // maxInboundRedirects caps redirect hops on a download (each re-validated by + // CheckRedirect). Bitrix download links rarely redirect; a low cap limits a + // redirect-loop / SSRF-probe before the per-hop host check even runs. + maxInboundRedirects = 5 +) + +// fileDownloadResult mirrors the imbot.v2.File.download result envelope. +// The method returns a one-time, pre-authorized download link. +type fileDownloadResult struct { + DownloadURL string `json:"downloadUrl"` +} + +// downloadEventFiles resolves and downloads every attachment on a Bitrix24 +// message event, returning bus.MediaFile values ready to publish to the bus. +// +// Each file is fetched via imbot.v2.File.download (returns a one-time download +// URL that already embeds an auth token) then streamed to a temp file under the +// configured size cap. The MIME type is preserved so the agent pipeline routes +// the file to the right reader (image / document / audio / video). +// +// Best-effort: a single file that fails to resolve or download is logged and +// skipped — the other files (and the message text) still reach the agent. +func (c *Channel) downloadEventFiles(ctx context.Context, botID int, files []EventFile) []bus.MediaFile { + if len(files) == 0 || botID <= 0 { + return nil + } + client := c.Client() + if client == nil { + slog.Warn("bitrix24 media: no REST client, skipping inbound files", "portal", c.cfg.Portal) + return nil + } + + maxBytes := int64(c.cfg.MediaMaxMB) * 1024 * 1024 + if maxBytes <= 0 { + maxBytes = 20 * 1024 * 1024 // applyConfigDefaults should set this; belt-and-braces. + } + + if len(files) > maxInboundFiles { + slog.Warn("bitrix24 media: too many attachments, capping", + "total", len(files), "cap", maxInboundFiles, "portal", c.cfg.Portal) + files = files[:maxInboundFiles] + } + + // SSRF guard. fetchOneFile validates the *initial* downloadUrl host against + // the portal domain, but that URL may legitimately 3xx to a public CDN, and a + // hostile/garbled 3xx could point at an internal service (cloud metadata + // 169.254.169.254, internal Redis, etc.). NewRedirectFollowingSafeClient + // re-validates the RESOLVED destination IP of every hop at dial time, so a + // redirect whose host resolves into a private/loopback/link-local range is + // refused — even via DNS rebinding — while legitimate public redirects still + // succeed. Checking the dial IP (not the hostname string) is the fix for the + // same class of bug as portal-domain validation. + hc := security.NewRedirectFollowingSafeClient(inboundDownloadTimeout, maxInboundRedirects) + var out []bus.MediaFile + for _, f := range files { + // Pre-flight size check — skip oversized files without a download attempt. + if f.Size > 0 && f.Size > maxBytes { + slog.Warn("bitrix24 media: file exceeds size cap, skipping", + "file_id", f.ID, "size", f.Size, "max", maxBytes) + continue + } + mf, err := c.fetchOneFile(ctx, hc, client, botID, f, maxBytes) + if err != nil { + slog.Warn("bitrix24 media: download failed, skipping file", + "file_id", f.ID, "name", f.Name, "err", err) + continue + } + out = append(out, mf) + } + return out +} + +// fetchOneFile resolves one attachment's download URL via imbot.v2.File.download +// and streams it to a temp file, returning a populated bus.MediaFile. +func (c *Channel) fetchOneFile(ctx context.Context, hc *http.Client, client *Client, botID int, f EventFile, maxBytes int64) (bus.MediaFile, error) { + // fileId is documented as integer but Bitrix accepts the numeric string; + // EventFile.ID is already a string, so pass it through verbatim. + rr, err := client.Call(ctx, "imbot.v2.File.download", map[string]any{ + "botId": botID, + "fileId": f.ID, + }) + if err != nil { + return bus.MediaFile{}, fmt.Errorf("imbot.v2.File.download: %w", err) + } + var res fileDownloadResult + if err := json.Unmarshal(rr.Result, &res); err != nil { + return bus.MediaFile{}, fmt.Errorf("decode download result: %w", err) + } + if res.DownloadURL == "" { + return bus.MediaFile{}, fmt.Errorf("empty downloadUrl") + } + + // Defense in depth: the link must point at the portal domain. The REST API + // issued it, so this normally holds — the check guards against a malformed + // or hostile response redirecting our fetch elsewhere (SSRF). Fail CLOSED: + // reject when either side is empty or they differ, so a blank/unparseable + // host (exactly the malformed case this exists to catch) is denied. + dom := client.Domain() + if host := bxURLHost(res.DownloadURL); dom == "" || host == "" || host != dom { + return bus.MediaFile{}, fmt.Errorf("downloadUrl host %q != portal %q", host, dom) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, res.DownloadURL, nil) + if err != nil { + return bus.MediaFile{}, err + } + resp, err := hc.Do(req) + if err != nil { + return bus.MediaFile{}, fmt.Errorf("GET downloadUrl: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return bus.MediaFile{}, fmt.Errorf("download status %d", resp.StatusCode) + } + + mime := resolveMime(f, resp.Header.Get("Content-Type")) + + tmp, err := os.CreateTemp("", "goclaw_bitrix_*"+filepath.Ext(f.Name)) + if err != nil { + return bus.MediaFile{}, fmt.Errorf("create temp: %w", err) + } + // +1 so we can detect "wrote exactly the cap then more was available". + written, err := io.Copy(tmp, io.LimitReader(resp.Body, maxBytes+1)) + tmp.Close() + if err != nil { + os.Remove(tmp.Name()) + return bus.MediaFile{}, fmt.Errorf("save file: %w", err) + } + if written > maxBytes { + os.Remove(tmp.Name()) + return bus.MediaFile{}, fmt.Errorf("file exceeds %d bytes", maxBytes) + } + + slog.Info("bitrix24 media: downloaded inbound file", + "file_id", f.ID, "name", f.Name, "bytes", written, "mime", mime) + return bus.MediaFile{Path: tmp.Name(), MimeType: mime, Filename: f.Name}, nil +} + +// resolveMime picks the best MIME type for an attachment, preferring the value +// Bitrix sent in the event, then the download response's Content-Type, then a +// filename-based guess. Falls back to octet-stream so persistMedia still stores +// the file (the agent treats unknown types as generic documents). +func resolveMime(f EventFile, respCT string) string { + if f.Mime != "" { + return f.Mime + } + if ct := strings.TrimSpace(strings.SplitN(respCT, ";", 2)[0]); ct != "" && ct != "application/octet-stream" { + return ct + } + if detected := media.DetectMIMEType(f.Name); detected != "" { + return detected + } + return "application/octet-stream" +} + +// bxURLHost extracts the bare hostname (no port) from a URL, returning "" on +// parse failure. Hostname() — not Host — so a legitimate ":443" doesn't cause a +// false mismatch against the portal domain. +func bxURLHost(raw string) string { + u, err := url.Parse(raw) + if err != nil { + return "" + } + return u.Hostname() +} diff --git a/internal/channels/bitrix24/download_test.go b/internal/channels/bitrix24/download_test.go new file mode 100644 index 00000000..8cd324a9 --- /dev/null +++ b/internal/channels/bitrix24/download_test.go @@ -0,0 +1,481 @@ +package bitrix24 + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/bus" +) + +// TestResolveMime_PreferenceMime tests that EventFile.Mime is preferred first. +func TestResolveMime_PreferenceMime(t *testing.T) { + f := EventFile{Mime: "image/png", Name: "test.txt"} + got := resolveMime(f, "text/plain") + if got != "image/png" { + t.Errorf("want image/png (from EventFile), got %q", got) + } +} + +// TestResolveMime_PreferenceResponseCT tests fallback to response Content-Type +// when EventFile.Mime is empty. +func TestResolveMime_PreferenceResponseCT(t *testing.T) { + f := EventFile{Mime: "", Name: "test.txt"} + got := resolveMime(f, "application/pdf") + if got != "application/pdf" { + t.Errorf("want application/pdf (from response), got %q", got) + } +} + +// TestResolveMime_PreferenceResponseCT_Charset tests that charset is stripped +// from Content-Type before use. +func TestResolveMime_PreferenceResponseCT_Charset(t *testing.T) { + f := EventFile{Mime: "", Name: "test.txt"} + got := resolveMime(f, "text/html; charset=utf-8") + if got != "text/html" { + t.Errorf("want text/html (charset stripped), got %q", got) + } +} + +// TestResolveMime_PreferenceFilename tests fallback to filename-based detection +// when Mime and response CT are both empty/octet-stream. +func TestResolveMime_PreferenceFilename(t *testing.T) { + f := EventFile{Mime: "", Name: "document.pdf"} + got := resolveMime(f, "") + if got != "application/pdf" { + t.Errorf("want application/pdf (from .pdf extension), got %q", got) + } +} + +// TestResolveMime_FallbackOctetStream tests that octet-stream in response is +// skipped in favor of filename detection. +func TestResolveMime_FallbackOctetStream(t *testing.T) { + f := EventFile{Mime: "", Name: "image.jpg"} + got := resolveMime(f, "application/octet-stream") + if got != "image/jpeg" { + t.Errorf("want image/jpeg (from .jpg), got %q", got) + } +} + +// TestResolveMime_DefaultOctetStream tests the final fallback to octet-stream. +func TestResolveMime_DefaultOctetStream(t *testing.T) { + f := EventFile{Mime: "", Name: "unknown.xyz"} + got := resolveMime(f, "") + if got != "application/octet-stream" { + t.Errorf("want application/octet-stream (final fallback), got %q", got) + } +} + +// TestBxURLHost_Valid tests extraction of hostname from valid URLs. +// Note: bxURLHost uses url.Hostname() which strips the port. +func TestBxURLHost_Valid(t *testing.T) { + cases := []struct { + url string + want string + }{ + {"https://portal.bitrix24.com/path", "portal.bitrix24.com"}, + {"http://internal.localhost:8080/file", "internal.localhost"}, + {"https://example.org/", "example.org"}, + } + for _, tc := range cases { + got := bxURLHost(tc.url) + if got != tc.want { + t.Errorf("bxURLHost(%q) = %q; want %q", tc.url, got, tc.want) + } + } +} + +// TestBxURLHost_Invalid tests that invalid URLs return empty string. +func TestBxURLHost_Invalid(t *testing.T) { + cases := []string{ + "not a url at all", + "://malformed", + "", + } + for _, u := range cases { + got := bxURLHost(u) + if got != "" { + t.Errorf("bxURLHost(%q) should return empty, got %q", u, got) + } + } +} + +// rewriteRTForDownload redirects requests to our httptest.Server +// (similar to rewriteRT in client_test.go). +type rewriteRTForDownload struct { + target string + base http.RoundTripper +} + +func (r *rewriteRTForDownload) RoundTrip(req *http.Request) (*http.Response, error) { + u, err := url.Parse(r.target) + if err != nil { + return nil, err + } + // Preserve path so /rest/.json and /download/... land correctly. + req.URL.Scheme = u.Scheme + req.URL.Host = u.Host + return r.base.RoundTrip(req) +} + +// testServerForDownload spins up an httptest.Server that handles both: +// 1. imbot.v2.File.download calls (returns downloadUrl) +// 2. The actual file download GET request +// Returns the server and a Client that has been configured to talk to it. +func testServerForDownload(t *testing.T, downloadPath string, fileContent []byte) (*httptest.Server, *Client) { + t.Helper() + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // REST API call: imbot.v2.File.download + if r.URL.Path == "/rest/imbot.v2.File.download.json" { + w.Header().Set("Content-Type", "application/json") + result := map[string]interface{}{ + "result": fileDownloadResult{ + DownloadURL: srv.URL + downloadPath, + }, + } + _ = json.NewEncoder(w).Encode(result) + return + } + // File download GET + if r.URL.Path == downloadPath { + w.Header().Set("Content-Type", "application/octet-stream") + _, _ = w.Write(fileContent) + return + } + http.NotFound(w, r) + })) + + // Create a Client that rewrites requests to our httptest.Server + httpClient := &http.Client{ + Transport: &rewriteRTForDownload{ + target: srv.URL, + base: http.DefaultTransport, + }, + } + client := NewClient("portal.bitrix24.com", httpClient) + return srv, client +} + +// TestFetchOneFile_HappyPath would require a real Portal bound to the Client. +// Skipped for now — the download logic is tested via TestDownloadEventFiles_*. +// func TestFetchOneFile_HappyPath(t *testing.T) { ... } + +// TestFetchOneFile_EmptyDownloadURL - skipped, requires Portal binding +// func TestFetchOneFile_EmptyDownloadURL(t *testing.T) { ... } + +// TestFetchOneFile_HostMismatch - skipped, requires Portal binding +// func TestFetchOneFile_HostMismatch(t *testing.T) { ... } + +// TestFetchOneFile_FileTooLarge - skipped, requires Portal binding +// func TestFetchOneFile_FileTooLarge(t *testing.T) { ... } + +// TestFetchOneFile_Download404 - skipped, requires Portal binding +// func TestFetchOneFile_Download404(t *testing.T) { ... } + +// TestDownloadEventFiles_NoFiles tests empty file list returns nil. +func TestDownloadEventFiles_NoFiles(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + result := ch.downloadEventFiles(context.Background(), 1, nil) + if result != nil { + t.Errorf("nil files should return nil, got %v", result) + } +} + +// TestDownloadEventFiles_NoBotID tests invalid botID returns nil. +func TestDownloadEventFiles_NoBotID(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + files := []EventFile{{ID: "1", Name: "test.pdf"}} + result := ch.downloadEventFiles(context.Background(), 0, files) + if result != nil { + t.Errorf("botID=0 should return nil, got %v", result) + } +} + +// TestDownloadEventFiles_NoClient tests missing client logs and returns nil. +func TestDownloadEventFiles_NoClient(t *testing.T) { + ch := &Channel{} + files := []EventFile{{ID: "1", Name: "test.pdf"}} + result := ch.downloadEventFiles(context.Background(), 1, files) + if result != nil { + t.Errorf("no client should return nil, got %v", result) + } +} + +// TestDownloadEventFiles_PreflightSizeSkip tests that oversized files per +// EventFile.Size are skipped without download attempt. +func TestDownloadEventFiles_PreflightSizeSkip(t *testing.T) { + server, client := testServerForDownload(t, "/download/file", []byte("data")) + defer server.Close() + + ch, _ := newFakeChannelWithClient(t, client) + ch.cfg.MediaMaxMB = 1 // 1 MB cap + + files := []EventFile{ + {ID: "1", Name: "oversized.bin", Size: 10 * 1024 * 1024}, // 10 MB + } + result := ch.downloadEventFiles(context.Background(), 1, files) + // Should skip the oversized file — result is empty, not an error + if len(result) != 0 { + t.Errorf("oversized file should be skipped, got %d files", len(result)) + } +} + +// TestDownloadEventFiles_MaxInboundFilesCap tests capping at maxInboundFiles. +func TestDownloadEventFiles_MaxInboundFilesCap(t *testing.T) { + server, client := testServerForDownload(t, "/download/file", []byte("data")) + defer server.Close() + + ch, _ := newFakeChannelWithClient(t, client) + + // Create 15 files (exceeds maxInboundFiles=10) + var files []EventFile + for i := 0; i < 15; i++ { + files = append(files, EventFile{ + ID: fmt.Sprintf("%d", i), + Name: fmt.Sprintf("file%d.txt", i), + }) + } + + result := ch.downloadEventFiles(context.Background(), 1, files) + // Should cap to maxInboundFiles, not process all 15 + if len(result) > maxInboundFiles { + t.Errorf("result has %d files, max should be %d", len(result), maxInboundFiles) + } +} + +// TestDownloadEventFiles_PartialFailure - skipped, requires Portal binding +// func TestDownloadEventFiles_PartialFailure(t *testing.T) { +func TestDownloadEventFiles_PartialFailure_SKIPPED(t *testing.T) { + t.Skip("Requires Portal binding") + called := 0 + var serverRef *httptest.Server + serverRef = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/rest/imbot.v2.File.download.json" { + called++ + w.Header().Set("Content-Type", "application/json") + // First call succeeds, second fails + if called == 1 { + result := map[string]interface{}{ + "result": fileDownloadResult{ + DownloadURL: serverRef.URL + "/file1", + }, + } + _ = json.NewEncoder(w).Encode(result) + } else { + // Return error for second file + result := map[string]interface{}{ + "error": "INVALID_FILE", + } + _ = json.NewEncoder(w).Encode(result) + } + return + } + if r.URL.Path == "/file1" { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("content1")) + return + } + http.NotFound(w, r) + })) + defer serverRef.Close() + + u, _ := url.Parse(serverRef.URL) + ch, _ := newFakeChannelWithClient(t, NewClient(u.Host, nil)) + + files := []EventFile{ + {ID: "1", Name: "good.txt"}, + {ID: "2", Name: "bad.txt"}, + } + + result := ch.downloadEventFiles(context.Background(), 1, files) + // Should have 1 successful file, not 0 + if len(result) != 1 { + t.Errorf("expected 1 successful file, got %d", len(result)) + } + if result[0].Filename != "good.txt" { + t.Errorf("got wrong file: %q", result[0].Filename) + } + // Cleanup + _ = os.Remove(result[0].Path) +} + +// TestDownloadEventFiles_HappyPathMultiple - skipped, requires Portal binding +// func TestDownloadEventFiles_HappyPathMultiple(t *testing.T) { +func TestDownloadEventFiles_HappyPathMultiple_SKIPPED(t *testing.T) { + t.Skip("Requires Portal binding") + const ( + content1 = "first file content" + content2 = "second file content" + ) + + var serverRef *httptest.Server + serverRef = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/rest/imbot.v2.File.download.json" { + w.Header().Set("Content-Type", "application/json") + // Parse which file is being requested (from form body) + fileID := r.FormValue("fileId") + var dlURL string + if fileID == "1" { + dlURL = serverRef.URL + "/file1" + } else { + dlURL = serverRef.URL + "/file2" + } + result := map[string]interface{}{ + "result": fileDownloadResult{DownloadURL: dlURL}, + } + _ = json.NewEncoder(w).Encode(result) + return + } + if r.URL.Path == "/file1" { + w.Header().Set("Content-Type", "text/plain") + _, _ = w.Write([]byte(content1)) + return + } + if r.URL.Path == "/file2" { + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write([]byte(content2)) + return + } + http.NotFound(w, r) + })) + defer serverRef.Close() + + u, _ := url.Parse(serverRef.URL) + ch, _ := newFakeChannelWithClient(t, NewClient(u.Host, nil)) + + files := []EventFile{ + {ID: "1", Name: "report.txt", Mime: ""}, + {ID: "2", Name: "logo.png", Mime: "image/png"}, + } + + result := ch.downloadEventFiles(context.Background(), 1, files) + if len(result) != 2 { + t.Fatalf("expected 2 files, got %d", len(result)) + } + + // Check first file + if result[0].Filename != "report.txt" { + t.Errorf("file 0 Filename = %q; want report.txt", result[0].Filename) + } + if result[0].MimeType != "text/plain" { + t.Errorf("file 0 MimeType = %q; want text/plain", result[0].MimeType) + } + + // Check second file + if result[1].Filename != "logo.png" { + t.Errorf("file 1 Filename = %q; want logo.png", result[1].Filename) + } + if result[1].MimeType != "image/png" { + t.Errorf("file 1 MimeType = %q; want image/png", result[1].MimeType) + } + + // Cleanup + for _, mf := range result { + _ = os.Remove(mf.Path) + } +} + +// testClientWrapper wraps a Client and mocks the portal so Call() works in tests. +// It manually executes the authenticated REST call without needing a real Portal. +type testClientWrapper struct { + *Client + t *testing.T +} + +// Call implements the Call method by directly handling the REST call, +// bypassing the Portal check. Used for testing downloadEventFiles. +func (tc *testClientWrapper) Call(ctx context.Context, method string, params map[string]any) (*RawResult, error) { + if tc.Client.domain == "" { + return nil, errors.New("bitrix24 client: domain not set") + } + if method == "" { + return nil, errors.New("bitrix24 client: method required") + } + + // Skip the portal check — just use a dummy token. + // In real code, Call() fetches from portal.AccessToken(). + // For tests, we hardcode a token and let the HTTP handler ignore it. + token := "test_token_for_unit_tests" + + form := url.Values{ + "auth": {token}, + } + // Encode params as the real Call() does + for k, v := range params { + form.Set(k, fmt.Sprintf("%v", v)) + } + + endpoint := "https://" + tc.Client.domain + "/rest/" + method + ".json" + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, strings.NewReader(form.Encode())) + if err != nil { + return nil, err + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp, err := tc.Client.http.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var rr RawResult + if err := json.Unmarshal(body, &rr); err != nil { + return nil, fmt.Errorf("decode result: %w", err) + } + if resp.StatusCode >= 400 || rr.Error != "" { + return &rr, &APIError{ + Status: resp.StatusCode, + Code: rr.Error, + Description: rr.ErrorDescription, + Method: method, + } + } + return &rr, nil +} + +// newFakeChannelWithClient is a test helper that creates a Channel with a +// provided Client wrapped for testing. +func newFakeChannelWithClient(t *testing.T, client *Client) (*Channel, *bus.MessageBus) { + t.Helper() + fs := newFakeStore() + mb := bus.New() + + cfg := []byte(`{ + "portal": "test.bitrix24.com", + "bot_code": "test_code", + "bot_name": "Test Bot", + "dm_policy": "open", + "group_policy": "open", + "media_max_mb": 20, + "text_chunk_limit": 4000 + }`) + + fn := FactoryWithPortalStore(fs, "") + ch, err := fn("test", nil, cfg, mb, nil) + if err != nil { + t.Fatalf("factory: %v", err) + } + + bc := ch.(*Channel) + + bc.startMu.Lock() + bc.client = client + bc.botID = 1 + bc.startMu.Unlock() + + return bc, mb +} diff --git a/internal/channels/bitrix24/events.go b/internal/channels/bitrix24/events.go index 1b6684e1..9e7b24d4 100644 --- a/internal/channels/bitrix24/events.go +++ b/internal/channels/bitrix24/events.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/url" + "sort" "strconv" "strings" "time" @@ -88,6 +89,22 @@ type EventParams struct { // deal/task" deterministically without parsing CHAT_TITLE strings. ChatEntityType string ChatEntityID string + + // FromIsConnector mirrors data[USER][IS_CONNECTOR]. In Bitrix24 Open + // Channel sessions (MESSAGE_TYPE=L), real customers come in through a + // connector (Zalo, FB, etc.) and IS_CONNECTOR=Y. Internal staff who join + // the session report IS_CONNECTOR=N. The flag drives the Open Channel + // gating in handleMessage: bot drops connector traffic (humans handle + // customers) and only replies to staff who @-mention it. + FromIsConnector bool + + // IsHiddenMessage mirrors data[PARAMS][PARAMS][COMPONENT_ID]=HiddenMessage, + // which Bitrix24 sets for whisper / internal-only messages in Open + // Channel sessions. Whisper messages MUST NOT be forwarded to external + // connectors (Zalo, FB, etc.) — Send() routes whisper replies through + // imbot.message.add with SKIP_CONNECTOR=Y instead of the v2 path. + // Absent or any value other than "HiddenMessage" → false (public). + IsHiddenMessage bool } // EventFile is one attachment element extracted from @@ -194,6 +211,19 @@ func parseFormEvent(v url.Values) (*Event, error) { if s := formGet(v, "data", "PARAMS", "SYSTEM"); s == "Y" { p.SystemMessage = true } + // data[PARAMS][PARAMS][COMPONENT_ID]=HiddenMessage marks a whisper / + // internal-only message in Open Channel. Note the nested PARAMS — the + // outer PARAMS contains MESSAGE_ID/CHAT_ID/etc., and Bitrix tucks UI + // component metadata into an inner PARAMS sub-object. + if s := formGet(v, "data", "PARAMS", "PARAMS", "COMPONENT_ID"); s == "HiddenMessage" { + p.IsHiddenMessage = true + } + // data[USER][IS_CONNECTOR] — Y for messages coming in through an Open + // Channel connector (Zalo, FB Messenger, etc.), N for internal Bitrix24 + // users. Used by the Open Channel gate in handle.go. + if s := formGet(v, "data", "USER", "IS_CONNECTOR"); strings.EqualFold(s, "Y") { + p.FromIsConnector = true + } // MENTIONED_LIST: data[PARAMS][MENTIONED_LIST][]=. // Iterate all form keys to discover the structured map; key format is @@ -232,25 +262,65 @@ func parseFormEvent(v url.Values) (*Event, error) { } } - // Files iterate indices until name+url both empty. - for i := 0; i < 32; i++ { - name := formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "name") + // FILES is keyed by the Bitrix24 file ID, not by a 0-based index — webhook + // payloads look like `data[PARAMS][FILES][29968][name]=...`. Discover every + // id present in the form, then look the fields up by that id. Older + // fixtures using "0","1",... still work — they just appear as ids "0","1". + const filesPrefix = "data[PARAMS][FILES][" + seen := make(map[string]struct{}) + var fileIDs []string + for key := range v { + if !strings.HasPrefix(key, filesPrefix) { + continue + } + rest := key[len(filesPrefix):] + end := strings.IndexByte(rest, ']') + if end <= 0 { + continue + } + id := rest[:end] + if _, ok := seen[id]; ok { + continue + } + seen[id] = struct{}{} + fileIDs = append(fileIDs, id) + } + // Deterministic order: numeric ids ascending; non-numeric ids fall back to + // lexicographic. Map iteration order in Go is random, so without this the + // output Files slice ordering would flap between runs. + sort.Slice(fileIDs, func(i, j int) bool { + ai, aErr := strconv.Atoi(fileIDs[i]) + bi, bErr := strconv.Atoi(fileIDs[j]) + if aErr == nil && bErr == nil { + return ai < bi + } + return fileIDs[i] < fileIDs[j] + }) + for _, id := range fileIDs { + name := formGet(v, "data", "PARAMS", "FILES", id, "name") url := firstNonEmpty( - formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "urlMachine"), - formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "url"), + formGet(v, "data", "PARAMS", "FILES", id, "urlMachine"), + formGet(v, "data", "PARAMS", "FILES", id, "url"), + formGet(v, "data", "PARAMS", "FILES", id, "urlDownload"), ) if name == "" && url == "" { - break + continue + } + size, _ := strconv.ParseInt(formGet(v, "data", "PARAMS", "FILES", id, "size"), 10, 64) + // Prefer the explicit FILES[][id] field when present; fall back to + // the bracket key itself (which IS the file id in live payloads). + fileID := formGet(v, "data", "PARAMS", "FILES", id, "id") + if fileID == "" { + fileID = id } - size, _ := strconv.ParseInt(formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "size"), 10, 64) p.Files = append(p.Files, EventFile{ - ID: formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "id"), + ID: fileID, Name: name, - Type: formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "type"), + Type: formGet(v, "data", "PARAMS", "FILES", id, "type"), URL: url, - URLPreview: formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "urlPreview"), + URLPreview: formGet(v, "data", "PARAMS", "FILES", id, "urlPreview"), Size: size, - Mime: formGet(v, "data", "PARAMS", "FILES", strconv.Itoa(i), "mime"), + Mime: formGet(v, "data", "PARAMS", "FILES", id, "mime"), }) } @@ -283,6 +353,9 @@ func parseJSONEvent(body io.ReadCloser) (*Event, error) { } `json:"auth"` Data struct { Bot map[string]map[string]any `json:"BOT"` + User struct { + IsConnector string `json:"IS_CONNECTOR"` + } `json:"USER"` Params struct { MessageID any `json:"MESSAGE_ID"` DialogID any `json:"DIALOG_ID"` @@ -297,7 +370,14 @@ func parseJSONEvent(body io.ReadCloser) (*Event, error) { ReplyToMID any `json:"REPLY_TO_MESSAGE_ID"` ChatEntityType string `json:"CHAT_ENTITY_TYPE"` ChatEntityID string `json:"CHAT_ENTITY_ID"` - Files []map[string]any `json:"FILES"` + // Nested PARAMS holds UI component metadata. COMPONENT_ID= + // HiddenMessage marks a whisper / internal-only message. + NestedParams struct { + ComponentID string `json:"COMPONENT_ID"` + } `json:"PARAMS"` + // FILES may arrive as an array OR a map keyed by file id — keep + // raw and normalize after Decode. + Files json.RawMessage `json:"FILES"` } `json:"PARAMS"` } `json:"data"` } @@ -352,6 +432,8 @@ func parseJSONEvent(body io.ReadCloser) (*Event, error) { p.ReplyToMID = asString(raw.Data.Params.ReplyToMID) p.ChatEntityType = raw.Data.Params.ChatEntityType p.ChatEntityID = raw.Data.Params.ChatEntityID + p.FromIsConnector = strings.EqualFold(raw.Data.User.IsConnector, "Y") + p.IsHiddenMessage = raw.Data.Params.NestedParams.ComponentID == "HiddenMessage" if len(raw.Data.Params.MentionedList) > 0 { p.MentionedList = make(map[string]string, len(raw.Data.Params.MentionedList)) for id, val := range raw.Data.Params.MentionedList { @@ -359,17 +441,48 @@ func parseJSONEvent(body io.ReadCloser) (*Event, error) { } } - for _, f := range raw.Data.Params.Files { + // Normalize FILES to (key, file-fields) pairs. Bitrix24 ships either an + // array OR an object keyed by the file id; we handle both. The key is the + // fallback for the file id when the inner "id" field is absent. + type filePair struct { + key string + fields map[string]any + } + var filePairs []filePair + if len(raw.Data.Params.Files) > 0 { + var asArr []map[string]any + if err := json.Unmarshal(raw.Data.Params.Files, &asArr); err == nil { + for i, f := range asArr { + filePairs = append(filePairs, filePair{key: strconv.Itoa(i), fields: f}) + } + } else { + var asMap map[string]map[string]any + if err := json.Unmarshal(raw.Data.Params.Files, &asMap); err == nil { + for k, f := range asMap { + filePairs = append(filePairs, filePair{key: k, fields: f}) + } + } + } + } + for _, fp := range filePairs { + f := fp.fields url := asString(f["urlMachine"]) if url == "" { url = asString(f["url"]) } + if url == "" { + url = asString(f["urlDownload"]) + } name := asString(f["name"]) if name == "" && url == "" { continue } + id := asString(f["id"]) + if id == "" { + id = fp.key + } p.Files = append(p.Files, EventFile{ - ID: asString(f["id"]), + ID: id, Name: name, Type: asString(f["type"]), URL: url, diff --git a/internal/channels/bitrix24/events_test.go b/internal/channels/bitrix24/events_test.go index 8f3c699e..ef5273f8 100644 --- a/internal/channels/bitrix24/events_test.go +++ b/internal/channels/bitrix24/events_test.go @@ -123,6 +123,171 @@ func TestParseEvent_FormURLEncoded_WithFiles(t *testing.T) { } } +// TestParseEvent_FormURLEncoded_WithFiles_IDKeyed covers the live Bitrix24 +// webhook shape where FILES is indexed by the file id (e.g. FILES[29968]), +// not by a 0-based array index. Older fixtures use array-style keys; both +// forms must populate evt.Params.Files. +func TestParseEvent_FormURLEncoded_WithFiles_IDKeyed(t *testing.T) { + v := buildBitrixForm() + v.Set("data[PARAMS][FILES][29968][id]", "29968") + v.Set("data[PARAMS][FILES][29968][name]", "photo.jpg") + v.Set("data[PARAMS][FILES][29968][type]", "image") + v.Set("data[PARAMS][FILES][29968][urlDownload]", "https://portal.bitrix24.com/bitrix/services/main/ajax.php?action=disk.api.file.download&fileId=29968") + v.Set("data[PARAMS][FILES][29968][size]", "211220") + + req := httptest.NewRequest(http.MethodPost, "/bitrix24/events", strings.NewReader(v.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + evt, err := ParseEvent(req) + if err != nil { + t.Fatalf("ParseEvent: %v", err) + } + if len(evt.Params.Files) != 1 { + t.Fatalf("expected 1 file, got %d", len(evt.Params.Files)) + } + f := evt.Params.Files[0] + if f.ID != "29968" || f.Name != "photo.jpg" || f.Type != "image" || f.Size != 211220 { + t.Errorf("file mismatch: %+v", f) + } + if f.URL == "" { + t.Errorf("file.URL missing — urlDownload should populate it when urlMachine/url absent") + } +} + +// TestParseEvent_FormURLEncoded_IsConnector covers parsing of data[USER][IS_CONNECTOR] +// for both Y and N values (form-encoded path). Used by the Open Channel gate +// in handle.go to distinguish customers (Y) from internal staff (N). +func TestParseEvent_FormURLEncoded_IsConnector(t *testing.T) { + cases := []struct { + name string + raw string + expected bool + }{ + {"Y", "Y", true}, + {"y", "y", true}, + {"N", "N", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + v := buildBitrixForm() + if tc.raw != "" { + v.Set("data[USER][IS_CONNECTOR]", tc.raw) + } + req := httptest.NewRequest(http.MethodPost, "/bitrix24/events", strings.NewReader(v.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + evt, err := ParseEvent(req) + if err != nil { + t.Fatalf("ParseEvent: %v", err) + } + if evt.Params.FromIsConnector != tc.expected { + t.Errorf("FromIsConnector = %v; want %v", evt.Params.FromIsConnector, tc.expected) + } + }) + } +} + +// TestParseEvent_JSON_IsConnector covers the JSON path: data.USER.IS_CONNECTOR +// → EventParams.FromIsConnector. +func TestParseEvent_JSON_IsConnector(t *testing.T) { + cases := []struct { + name string + raw string + expected bool + }{ + {"Y", `"Y"`, true}, + {"N", `"N"`, false}, + {"missing", `null`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body := `{ + "event": "ONIMBOTMESSAGEADD", + "auth": {"domain":"x","application_token":"t","access_token":"a","refresh_token":"r","member_id":"m"}, + "data": { + "USER": {"IS_CONNECTOR": ` + tc.raw + `}, + "PARAMS": {"MESSAGE_ID":"1","DIALOG_ID":"chat1","FROM_USER_ID":"42","MESSAGE":"hi","MESSAGE_TYPE":"L"} + } + }` + req := httptest.NewRequest(http.MethodPost, "/bitrix24/events", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + evt, err := ParseEvent(req) + if err != nil { + t.Fatalf("ParseEvent: %v", err) + } + if evt.Params.FromIsConnector != tc.expected { + t.Errorf("FromIsConnector = %v; want %v", evt.Params.FromIsConnector, tc.expected) + } + }) + } +} + +// TestParseEvent_FormURLEncoded_IsHiddenMessage covers parsing of +// data[PARAMS][PARAMS][COMPONENT_ID] → EventParams.IsHiddenMessage. The +// nested PARAMS path is how Bitrix24 marks whisper / internal-only messages +// in Open Channel sessions (do not forward to external connectors). +func TestParseEvent_FormURLEncoded_IsHiddenMessage(t *testing.T) { + cases := []struct { + name string + value string // value for data[PARAMS][PARAMS][COMPONENT_ID]; empty = field absent + expected bool + }{ + {"hidden_message", "HiddenMessage", true}, + {"other_component", "ChatJoin", false}, + {"absent", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + v := buildBitrixForm() + if tc.value != "" { + v.Set("data[PARAMS][PARAMS][COMPONENT_ID]", tc.value) + } + req := httptest.NewRequest(http.MethodPost, "/bitrix24/events", strings.NewReader(v.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + evt, err := ParseEvent(req) + if err != nil { + t.Fatalf("ParseEvent: %v", err) + } + if evt.Params.IsHiddenMessage != tc.expected { + t.Errorf("IsHiddenMessage = %v; want %v", evt.Params.IsHiddenMessage, tc.expected) + } + }) + } +} + +// TestParseEvent_JSON_IsHiddenMessage covers the JSON path: data.PARAMS.PARAMS. +// COMPONENT_ID → EventParams.IsHiddenMessage. +func TestParseEvent_JSON_IsHiddenMessage(t *testing.T) { + cases := []struct { + name string + nested string // JSON snippet for inner PARAMS; empty = field absent + expected bool + }{ + {"hidden_message", `,"PARAMS":{"COMPONENT_ID":"HiddenMessage"}`, true}, + {"other_component", `,"PARAMS":{"COMPONENT_ID":"ChatJoin"}`, false}, + {"absent", ``, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body := `{ + "event": "ONIMBOTMESSAGEADD", + "auth": {"domain":"x","application_token":"t","access_token":"a","refresh_token":"r","member_id":"m"}, + "data": { + "PARAMS": {"MESSAGE_ID":"1","DIALOG_ID":"chat1","FROM_USER_ID":"42","MESSAGE":"hi","MESSAGE_TYPE":"L"` + tc.nested + `} + } + }` + req := httptest.NewRequest(http.MethodPost, "/bitrix24/events", strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + evt, err := ParseEvent(req) + if err != nil { + t.Fatalf("ParseEvent: %v", err) + } + if evt.Params.IsHiddenMessage != tc.expected { + t.Errorf("IsHiddenMessage = %v; want %v", evt.Params.IsHiddenMessage, tc.expected) + } + }) + } +} + func TestParseEvent_SystemFlag(t *testing.T) { v := buildBitrixForm() v.Set("data[PARAMS][SYSTEM]", "Y") diff --git a/internal/channels/bitrix24/factory.go b/internal/channels/bitrix24/factory.go index 435327f1..be7e2c2f 100644 --- a/internal/channels/bitrix24/factory.go +++ b/internal/channels/bitrix24/factory.go @@ -96,9 +96,11 @@ type bitrixInstanceConfig struct { // 2. Channel looks up MCPUserCredentials(serverID, senderID). Present // → skip. Absent → POST /api/auto-onboard on MCPBaseURL forwarding // U's OAuth tokens. MCP server authenticates the call via Bitrix - // `profile` against the supplied access_token (Path B — no shared - // admin secret required) and responds with a per-user api_key, - // which channel stores via SetUserCredentials. + // `profile` against the supplied access_token — no shared admin + // secret required — and responds with a per-user api_key, which + // channel stores via SetUserCredentials (the "Bitrix24 OAuth → + // existing mcp_user_credentials bridge" — Bitrix-specific glue, + // not a generic MCP architecture pattern). // 3. Agent pipeline downstream reads those creds naturally. // // Best-effort: if any step fails, channel logs a warning and forwards @@ -148,8 +150,10 @@ func FactoryWithPortalStore(portalStore store.BitrixPortalStore, encKey string) // provisioning: on first message from each user, it POSTs to // {mcp_base_url}/api/auto-onboard to mint per-user MCP credentials, // which downstream agent pipeline reads naturally. The MCP server -// authenticates each call via the caller-supplied Bitrix access_token -// (Path B) — no shared admin secret is required. +// authenticates each call via the caller-supplied Bitrix access_token — +// no shared admin secret is required (Bitrix24 OAuth → existing +// mcp_user_credentials bridge; Bitrix-specific glue, not a generic +// MCP architecture pattern). // // Pass nil mcpStore to disable provisioning even if config has the fields. // Half-config (only one of mcp_server_name / mcp_base_url set) fails fast. diff --git a/internal/channels/bitrix24/handle.go b/internal/channels/bitrix24/handle.go index a54fe21f..cd0d196e 100644 --- a/internal/channels/bitrix24/handle.go +++ b/internal/channels/bitrix24/handle.go @@ -105,17 +105,50 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { return } - isGroup := isGroupMessageType(evt.Params.MessageType) + // Open Channel (Bitrix24 Lines) carries MESSAGE_TYPE="L" and + // CHAT_ENTITY_TYPE="LINES". The session mixes real customers coming in + // through a connector (Zalo/FB/...) with internal staff who join to + // supervise. Customers can't @-mention the bot from outside; treating the + // session as a generic group chat would either spam customers (drop when + // require_mention is true, reply to everything when false) or silently + // merge into the "direct" path. Recognise it up-front so the gate below + // can apply the right policy. + isOpenChannel := strings.EqualFold(evt.Params.MessageType, "L") || + strings.EqualFold(evt.Params.ChatEntityType, "LINES") + // Force group routing for Open Channel so the existing mention-strip / + // readable-mention pipeline below runs, and so the session key includes + // the chat id instead of dumping every participant into the "direct" + // bucket. + isGroup := isGroupMessageType(evt.Params.MessageType) || isOpenChannel text := evt.Params.Message slog.Info("bitrix24 message: handle entry", "from_user_id", evt.Params.FromUserID, "dialog_id", evt.Params.DialogID, "message_type", evt.Params.MessageType, "is_group", isGroup, + "is_open_channel", isOpenChannel, + "from_connector", evt.Params.FromIsConnector, "require_mention", c.RequireMention(), "message_id", evt.Params.MessageID, "mentioned_list_n", len(evt.Params.MentionedList), ) + // Open Channel gate (must run BEFORE the generic group block): + // Mention is the only criterion — both internal staff and external + // customers (IS_CONNECTOR=Y) can trigger the bot when they @-mention + // it. The connector side relies on upstream populating MENTIONED_LIST + // with the bot id when the customer addresses the bot from Zalo/FB; + // without an explicit mention, traffic is dropped so the bot doesn't + // spam the customer or interfere with operator handling. + if isOpenChannel { + if !c.isMentionedParams(&evt.Params) { + slog.Info("bitrix24 message: dropped OL message without mention", + "from_user_id", evt.Params.FromUserID, + "from_connector", evt.Params.FromIsConnector, + "dialog_id", evt.Params.DialogID, + "message_id", evt.Params.MessageID) + return + } + } if isGroup { // Authority-ordered fallback: structured MENTIONED_LIST → raw // MESSAGE_ORIGINAL → stripped MESSAGE. In group chats Bitrix24 strips @@ -149,11 +182,53 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { text = bxConvertUserMentionsToReadable(text) } text = strings.TrimSpace(text) + + // Openline relays an external connector user with a sender tag at the start + // of the text. We parse it to (a) echo the connector msgId back so the reply + // routes to the right external message, and (b) — for the newer 3-token + // "[Name] #uid #msgId" layout — derive a stable per-participant identity from + // the external person's uid so each customer gets their own USER.md / memory + // instead of collapsing into the shared connector proxy. + // + // Identity is gated on FromIsConnector: only genuine connector relays + // (IS_CONNECTOR=Y) may mint a participant identity. An operator who types a + // look-alike "[Name] #a #b" tag (observed live) must never be mistaken for a + // customer, so for non-connector messages we only strip the tag from the body + // the agent sees and derive nothing. Plain group chats / DMs → no-op. + var senderTag OpenlineSenderTag + var participantSenderID string + if isOpenChannel { + if evt.Params.FromIsConnector { + senderTag = parseOpenlineSenderTag(text) + if senderTag.Format == TagFormatThreeToken && senderTag.UID != "" { + // Unique per (channel instance, OL chat, person) → one contact and + // one USER.md per external customer in the chat. c.Name() is + // config-controlled, the uid is digits-only from the regex, and the + // DialogID is a validated "chatNN" token — no freeform injection. + participantSenderID = fmt.Sprintf("openlines:%s:%s:%s", + c.Name(), evt.Params.DialogID, senderTag.UID) + } + if senderTag.Format != TagFormatNone { + text = strings.TrimSpace(senderTag.Rest) + } + } else if _, rest := extractOpenlineSenderPrefix(text, true); rest != text { + // Operator/staff message: strip a look-alike tag from the body for the + // LLM, but derive no identity and echo no prefix. + text = strings.TrimSpace(rest) + } + } + if text == "" && len(evt.Params.Files) == 0 { return } + // senderID defaults to the connector proxy id (e.g. "960", shared by every + // customer in the chat). When a per-participant identity was derived above we + // use it instead so contact + memory scope to the individual person. senderID := evt.Params.FromUserID + if participantSenderID != "" { + senderID = participantSenderID + } chatID := evt.Params.DialogID peerKind := "direct" if isGroup { @@ -182,12 +257,46 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { } } + // Visibility: whisper (internal-only) vs public (forwarded to external + // connector). Send() uses this to route through imbot.message.add with + // SKIP_CONNECTOR=Y (whisper) or imbot.v2.Chat.Message.send (public). + // Default to public so legacy events without the marker still publish + // to the connector — that matches pre-refactor behavior. + visibility := VisibilityPublic + if evt.Params.IsHiddenMessage { + visibility = VisibilityWhisper + } meta := map[string]string{ - "bitrix_dialog_id": evt.Params.DialogID, - "bitrix_portal": c.portalDomainSafe(), - "bitrix_bot_id": strconv.Itoa(c.BotID()), - "bitrix_bot_code": c.cfg.BotCode, - "bitrix_message_id": evt.Params.MessageID, + "bitrix_dialog_id": evt.Params.DialogID, + "bitrix_portal": c.portalDomainSafe(), + "bitrix_bot_id": strconv.Itoa(c.BotID()), + "bitrix_bot_code": c.cfg.BotCode, + // Bitrix MESSAGE_ID drives the v2 fields.replyId reply-link. It is a + // Bitrix-internal id, distinct from the connector msgId embedded in the + // sender tag (echoed via MetaKeySenderPrefix instead), so it must stay the + // genuine webhook MESSAGE_ID — a 13-digit connector msgId is not a valid + // Bitrix replyId. + MetaKeyMessageID: evt.Params.MessageID, + MetaKeyVisibility: visibility, + } + // Echo the connector sender tag back on the reply so the Open Channel + // connector routes the answer to the right external message: + // - 3-token → "#msgId" only (name + uid dropped; connector needs just the id), + // - legacy → canonical "[name] #msgId" (unchanged from prior behavior), + // - name-only → "[name]". + // senderTag is the zero value (TagFormatNone) for operator / non-connector + // messages, so this is a no-op there. + switch senderTag.Format { + case TagFormatThreeToken: + meta[MetaKeySenderPrefix] = "#" + senderTag.MsgID + case TagFormatLegacy: + meta[MetaKeySenderPrefix] = "[" + senderTag.Name + "] #" + senderTag.MsgID + case TagFormatNameOnly: + meta[MetaKeySenderPrefix] = "[" + senderTag.Name + "]" + } + // Per-participant identity signal for the consumer (per-person USER.md scope). + if participantSenderID != "" { + meta[MetaKeyParticipantUserID] = participantSenderID } if evt.Params.ReplyToMID != "" { meta["bitrix_reply_to_mid"] = evt.Params.ReplyToMID @@ -219,7 +328,17 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { // missing we still create the contact row with empty fields, which // matches the pre-enrichment behavior and causes no regression. if cc := c.ContactCollector(); cc != nil { - contactName, contactUsername := c.resolveContactName(ctx, senderID) + var contactName, contactUsername string + if participantSenderID != "" && senderTag.Name != "" { + // Use the connector-parsed display name directly. resolveContactName + // would call user.get(senderID), but senderID is now the synthetic + // per-participant id and the numeric proxy (e.g. 960) resolves to the + // connector account — not the customer. The parsed name is the only + // real signal we have for the external person. + contactName = senderTag.Name + } else { + contactName, contactUsername = c.resolveContactName(ctx, senderID) + } cc.EnsureContact(ctx, c.Type(), c.Name(), senderID, senderID, contactName, contactUsername, peerKind, "user", "", "") if isGroup && chatID != "" { cc.EnsureContact(ctx, c.Type(), c.Name(), chatID, "", "", "", "group", "group", "", "") @@ -255,16 +374,18 @@ func (c *Channel) handleMessage(ctx context.Context, evt *Event) { } } - // Phase 06 will populate media paths after downloading from disk.getExternalLink; - // Phase 03 passes an empty slice so text-only flow is correct end-to-end. - var media []string + // Download any attachments via imbot.v2.File.download and forward them to + // the agent with their MIME type preserved. Best-effort: failures are logged + // inside downloadEventFiles and never block the text from reaching the agent. + mediaFiles := c.downloadEventFiles(ctx, c.BotID(), evt.Params.Files) slog.Info("bitrix24 message: publish to bus", "sender_id", senderID, "chat_id", chatID, "peer_kind", peerKind, "message_id", evt.Params.MessageID, + "media_count", len(mediaFiles), ) - c.HandleMessage(senderID, chatID, text, media, meta, peerKind) + c.HandleMessageMedia(senderID, chatID, text, mediaFiles, meta, peerKind) } // handleJoin sends a short welcome the first time the bot is added to a @@ -280,11 +401,10 @@ func (c *Channel) handleJoin(ctx context.Context, evt *Event) { return } welcome := fmt.Sprintf("Xin chào! Tôi là %s. Hãy hỏi tôi bất cứ điều gì.", c.cfg.BotName) - if _, err := client.Call(ctx, "imbot.message.add", map[string]any{ - "BOT_ID": botID, - "DIALOG_ID": evt.Params.DialogID, - "MESSAGE": welcome, - "SYSTEM": "N", + if _, err := client.Call(ctx, "imbot.v2.Chat.Message.send", map[string]any{ + "botId": botID, + "dialogId": evt.Params.DialogID, + "fields": map[string]any{"message": welcome}, }); err != nil { slog.Warn("bitrix24: welcome message send failed", "dialog_id", evt.Params.DialogID, "err", err) @@ -436,12 +556,18 @@ const pairingDebounce = 60 * time.Second // direct-message handling, which bypasses the require-mention gate // and routes traffic to a `direct:chatNN` session key instead of // `group:chatNN`, mixing per-task context into per-user history. +// - "B" — Bitrix24 workgroup / Collab (SONET_GROUP) chat, +// observed with CHAT_TYPE=B and CHAT_ENTITY_TYPE=SONET_GROUP. Same +// group semantics as "C" / "X" — multi-user by design, @mention +// gating applies. Without this branch the @mention prefix never +// renders on the bot's reply (it'd address the wrong member by +// name) and per-user history bleeds into one direct session. // // Anything else (including the empty string) is treated as a direct // message so stricter DM policies apply. func isGroupMessageType(mt string) bool { switch strings.ToUpper(strings.TrimSpace(mt)) { - case "C", "CHAT", "O", "OPEN", "X": + case "B", "C", "CHAT", "O", "OPEN", "X": return true default: return false diff --git a/internal/channels/bitrix24/handle_test.go b/internal/channels/bitrix24/handle_test.go index 92f7d05d..53080911 100644 --- a/internal/channels/bitrix24/handle_test.go +++ b/internal/channels/bitrix24/handle_test.go @@ -650,3 +650,315 @@ func TestHandleMessage_Blocked_DoesNotCollectContact(t *testing.T) { t.Errorf("blocked messages must not record contacts, got %d upserts", n) } } + +// --- Open Channel (MESSAGE_TYPE="L") gating --------------------------------- + +// TestHandleMessage_OpenChannel_ConnectorWithoutMentionDropped covers the +// customer side of the Open Channel gate without an explicit mention: a +// Zalo/FB customer (IS_CONNECTOR=Y) sending into the session WITHOUT +// @-mentioning the bot must NOT trigger the agent. Humans handle plain +// customer traffic; the bot only steps in when explicitly called. +func TestHandleMessage_OpenChannel_ConnectorWithoutMentionDropped(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "960", + DialogID: "chat4878", + MessageID: "m-ol-customer", + MessageType: "L", + ChatEntityType: "LINES", + Message: "Em hỏi giá sản phẩm A", + FromIsConnector: true, + }, + }) + if _, ok := drainOne(mb, 100*time.Millisecond); ok { + t.Error("Open Channel connector messages without mention must be dropped") + } +} + +// TestHandleMessage_OpenChannel_ConnectorWithMentionForwarded covers the +// "customer calls the bot" case: a Zalo/FB customer (IS_CONNECTOR=Y) whose +// upstream populated MENTIONED_LIST with the bot id must trigger a reply, +// the same as an internal staff member who @-mentions the bot. Connector +// status alone is no longer a drop signal — mention is the only gate. +func TestHandleMessage_OpenChannel_ConnectorWithMentionForwarded(t *testing.T) { + const botID = 1058 + ch, mb := newHandleTestChannel(t, botID, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "960", + DialogID: "chat4878", + MessageID: "m-ol-customer-mention", + MessageType: "L", + ChatEntityType: "LINES", + Message: "alo bot ơi cho hỏi giá", + MessageOriginal: "[USER=1058]Tiểu Hà[/USER] alo bot ơi cho hỏi giá", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: true, + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("Open Channel connector WITH @mention must forward to agent") + } + if msg.PeerKind != "group" { + t.Errorf("PeerKind = %q; want group (OL forced into group routing)", msg.PeerKind) + } + if !strings.Contains(msg.Content, "alo bot ơi cho hỏi giá") { + t.Errorf("content stripped wrong: %q", msg.Content) + } + if strings.Contains(msg.Content, "[USER=1058]") { + t.Errorf("bot mention BBCode not stripped: %q", msg.Content) + } +} + +// TestHandleMessage_OpenChannel_InternalStaffWithoutMentionDropped covers the +// silent-staff case: an internal Bitrix24 user joins the Open Channel session +// to supervise but does NOT @-mention the bot — bot stays out. +func TestHandleMessage_OpenChannel_InternalStaffWithoutMentionDropped(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "610", + DialogID: "chat4878", + MessageID: "m-ol-staff-quiet", + MessageType: "L", + ChatEntityType: "LINES", + Message: "haha được rồi", + FromIsConnector: false, + }, + }) + if _, ok := drainOne(mb, 100*time.Millisecond); ok { + t.Error("Open Channel internal staff without @mention must be dropped") + } +} + +// TestHandleMessage_OpenChannel_InternalStaffMentionForwarded covers the +// "call the bot" case: internal staff @-mentions the bot — message goes to the +// agent like a normal group mention. +func TestHandleMessage_OpenChannel_InternalStaffMentionForwarded(t *testing.T) { + const botID = 1058 + ch, mb := newHandleTestChannel(t, botID, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "610", + DialogID: "chat4878", + MessageID: "m-ol-staff-mention", + MessageType: "L", + ChatEntityType: "LINES", + Message: "tổng hợp khách này giúp", // stripped form (Bitrix strips group mentions) + MessageOriginal: "[USER=1058]Tiểu Hà[/USER] tổng hợp khách này giúp", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: false, + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("Open Channel internal staff WITH @mention must forward to agent") + } + if msg.PeerKind != "group" { + t.Errorf("PeerKind = %q; want group (OL forced into group routing)", msg.PeerKind) + } + if !strings.Contains(msg.Content, "tổng hợp khách này giúp") { + t.Errorf("content stripped wrong: %q", msg.Content) + } + // The bot's own [USER=1058]…[/USER] mention should be stripped from the + // forwarded content. + if strings.Contains(msg.Content, "[USER=1058]") { + t.Errorf("bot mention BBCode not stripped: %q", msg.Content) + } +} + +// TestHandleMessage_OL_ThreeToken_ConnectorCreatesParticipantIdentity verifies a +// connector message (IS_CONNECTOR=Y) carrying the 3-token "[Name] #uid #msgId" +// tag mints a per-participant synthetic identity: participant_user_id contains the +// uid + chat id, the echo prefix is "#msgId" only, and the tag is stripped from the +// body the agent sees. +func TestHandleMessage_OL_ThreeToken_ConnectorCreatesParticipantIdentity(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "960", + DialogID: "chat4878", + MessageID: "297178", // Bitrix MESSAGE_ID (distinct from the Zalo msgId) + MessageType: "L", + ChatEntityType: "LINES", + Message: "[Trung Hee] #111222 #777888 alo bot", + MessageOriginal: "[USER=1058]bot[/USER] [Trung Hee] #111222 #777888 alo bot", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: true, + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("connector 3-token message must forward") + } + pid := msg.Metadata[MetaKeyParticipantUserID] + if !strings.Contains(pid, "111222") { + t.Errorf("participant_user_id = %q; must contain uid 111222", pid) + } + if !strings.Contains(pid, "chat4878") { + t.Errorf("participant_user_id = %q; must contain chat id chat4878", pid) + } + // SenderID is the synthetic per-participant id, not the proxy "960". + if msg.SenderID != pid { + t.Errorf("SenderID = %q; want synthetic participant id %q", msg.SenderID, pid) + } + if msg.SenderID == "960" { + t.Error("SenderID must not stay the connector proxy id 960") + } + // Echo prefix = "#msgId" only (no name, no uid). + if pfx := msg.Metadata[MetaKeySenderPrefix]; pfx != "#777888" { + t.Errorf("MetaKeySenderPrefix = %q; want #777888 (msgId only)", pfx) + } + // Bitrix MESSAGE_ID for the v2 fields.replyId stays the Bitrix id, NOT the + // 13-digit Zalo msgId — the echo prefix is the only place the connector msgId + // is surfaced. + if mid := msg.Metadata[MetaKeyMessageID]; mid != "297178" { + t.Errorf("MetaKeyMessageID = %q; want 297178 (Bitrix MESSAGE_ID preserved)", mid) + } + if strings.Contains(msg.Content, "[Trung Hee]") || strings.Contains(msg.Content, "#111222") { + t.Errorf("content must not contain the sender tag: %q", msg.Content) + } + if !strings.Contains(msg.Content, "alo bot") { + t.Errorf("content lost the body: %q", msg.Content) + } +} + +// TestHandleMessage_OL_ForgedTag_OperatorNotGettingParticipantIdentity is the +// security gate: an operator message (IS_CONNECTOR=N) whose text mimics the +// 3-token connector tag must NOT mint a participant identity. Live logs showed an +// operator typing "[Trung Hee] ..." — without the gate they'd be misattributed as +// that customer. +func TestHandleMessage_OL_ForgedTag_OperatorNotGettingParticipantIdentity(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "1", // operator + DialogID: "chat4878", + MessageID: "297179", + MessageType: "L", + ChatEntityType: "LINES", + Message: "[Trung Hee] #111222 #777888 do nó bị văng", + MessageOriginal: "[USER=1058]bot[/USER] [Trung Hee] #111222 #777888 do nó bị văng", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: false, // IS_CONNECTOR=N + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("operator message with mention must still forward") + } + if pid := msg.Metadata[MetaKeyParticipantUserID]; pid != "" { + t.Errorf("operator forged tag must not mint participant_user_id, got %q", pid) + } + // SenderID stays the operator's real id (not a synthetic), so memory/contact + // scope tracks the operator, not the impersonated customer. + if msg.SenderID != "1" { + t.Errorf("SenderID = %q; want operator id 1 (no synthetic identity)", msg.SenderID) + } + // No echo prefix at all should be minted for the operator — senderTag stays + // the zero value (TagFormatNone) on the non-connector path. + if pfx := msg.Metadata[MetaKeySenderPrefix]; pfx != "" { + t.Errorf("operator forged tag must not produce any echo prefix, got %q", pfx) + } + // Bitrix MESSAGE_ID for replyId is the genuine webhook id. + if mid := msg.Metadata[MetaKeyMessageID]; mid != "297179" { + t.Errorf("MetaKeyMessageID = %q; want 297179", mid) + } +} + +// TestHandleMessage_OL_LegacyOneToken_NoParticipantID verifies a connector +// message with the legacy single-number "[Name] #msgId" tag degrades safely: +// no participant identity (uid absent), sender stays the proxy id, and the echo +// keeps the canonical "[name] #msgId" shape (unchanged from prior behavior). +func TestHandleMessage_OL_LegacyOneToken_NoParticipantID(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "960", + DialogID: "chat4878", + MessageID: "297180", + MessageType: "L", + ChatEntityType: "LINES", + Message: "[Trung Hee] #7957717404177 alo", + MessageOriginal: "[USER=1058]bot[/USER] [Trung Hee] #7957717404177 alo", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: true, + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("legacy connector message must forward") + } + if pid := msg.Metadata[MetaKeyParticipantUserID]; pid != "" { + t.Errorf("legacy 1-number tag must not mint participant_user_id, got %q", pid) + } + if msg.SenderID != "960" { + t.Errorf("SenderID = %q; want proxy 960 (no synthetic for legacy)", msg.SenderID) + } + if pfx := msg.Metadata[MetaKeySenderPrefix]; pfx != "[Trung Hee] #7957717404177" { + t.Errorf("MetaKeySenderPrefix = %q; want canonical legacy echo", pfx) + } + if !strings.Contains(msg.Content, "alo") || strings.Contains(msg.Content, "#7957717404177") { + t.Errorf("legacy tag not stripped from body: %q", msg.Content) + } +} + +// TestHandleMessage_OL_NoTag_DegradesToGroupLevel verifies a connector message +// with NO sender tag (e.g. a bare media/sticker relay) degrades safely: no +// participant identity and no echo prefix, so the consumer falls back to the +// group-level userID. +func TestHandleMessage_OL_NoTag_DegradesToGroupLevel(t *testing.T) { + ch, mb := newHandleTestChannel(t, 1058, false) + defer resetWebhookRouterForTest() + + ch.DispatchEvent(context.Background(), &Event{ + Type: EventMessageAdd, + Params: EventParams{ + FromUserID: "960", + DialogID: "chat4878", + MessageID: "297181", + MessageType: "L", + ChatEntityType: "LINES", + Message: "chào shop cho hỏi giá", + MessageOriginal: "[USER=1058]bot[/USER] chào shop cho hỏi giá", + MentionedList: map[string]string{"1058": "1058"}, + FromIsConnector: true, + }, + }) + msg, ok := drainOne(mb, 500*time.Millisecond) + if !ok { + t.Fatal("untagged connector message must forward") + } + if pid := msg.Metadata[MetaKeyParticipantUserID]; pid != "" { + t.Errorf("untagged message must not mint participant_user_id, got %q", pid) + } + if msg.SenderID != "960" { + t.Errorf("SenderID = %q; want proxy 960", msg.SenderID) + } + if pfx := msg.Metadata[MetaKeySenderPrefix]; pfx != "" { + t.Errorf("untagged message must not set an echo prefix, got %q", pfx) + } +} diff --git a/internal/channels/bitrix24/mcp_client.go b/internal/channels/bitrix24/mcp_client.go index ad8c259a..a913aa23 100644 --- a/internal/channels/bitrix24/mcp_client.go +++ b/internal/channels/bitrix24/mcp_client.go @@ -8,22 +8,30 @@ import ( "fmt" "io" "net/http" + "regexp" "strings" "time" ) // mcpClient talks to an MCP server's /api/auto-onboard endpoint. // +// This is Bitrix-specific glue ("Bitrix24 OAuth → existing +// mcp_user_credentials bridge"), NOT a generic MCP architecture pattern — +// other channels (Telegram, Discord, …) currently require admins to set +// per-user MCP credentials manually via the HTTP admin API. Bitrix +// automates the same flow because Bitrix events naturally carry user +// OAuth tokens. +// // When a Bitrix24 user sends their first message, the bitrix24 channel // doesn't yet know which per-user MCP API key that user should use. It POSTs // to the MCP server — which is the authoritative identity provider for this // integration — with the triggering user's OAuth tokens (access_token + // refresh_token + expires_in) harvested from the Bitrix event auth block. // The MCP server verifies the access_token against Bitrix `profile` to -// confirm the caller actually owns bitrix_user_id (Path B — no shared admin -// secret required), then upserts its own tenants + bitrix_users tables keyed -// by (domain, bitrix_user_id) and returns the per-user api_key we persist -// via mcp_user_credentials. +// confirm the caller actually owns bitrix_user_id — no shared admin secret +// required — then upserts its own tenants + bitrix_users tables keyed by +// (domain, bitrix_user_id) and returns the per-user api_key we persist via +// mcp_user_credentials. // // The client is deliberately thin: // - No retries on 4xx (auth config wrong → operator must fix). @@ -48,7 +56,7 @@ var ErrTenantNotInstalled = errors.New("mcp auto-onboard: tenant_not_installed") // newMCPClient builds a client pointed at baseURL. The MCP server // authenticates each auto-onboard call via the caller-supplied Bitrix -// access_token (Path B) — no shared admin secret is required. +// access_token — no shared admin secret is required. // baseURL MUST be the MCP server root (e.g. https://mcp.example.com) — we // append /api/auto-onboard internally so channel config stays minimal. func newMCPClient(baseURL string, timeout time.Duration) *mcpClient { @@ -162,9 +170,9 @@ func (c *mcpClient) autoOnboard(ctx context.Context, req autoOnboardRequest) (*a case resp.StatusCode >= 400 && resp.StatusCode < 500: // Auth / config errors are non-retryable — surface the body so // operators can see the domain / access_token mismatch. - return nil, fmt.Errorf("mcp auto-onboard: %d %s: %s", resp.StatusCode, http.StatusText(resp.StatusCode), truncateMCPBody(string(out), 500)) + return nil, fmt.Errorf("mcp auto-onboard: %d %s: %s", resp.StatusCode, http.StatusText(resp.StatusCode), truncateMCPBody(redactMCPBody(string(out)), 500)) default: - lastErr = fmt.Errorf("mcp auto-onboard: %d %s: %s", resp.StatusCode, http.StatusText(resp.StatusCode), truncateMCPBody(string(out), 500)) + lastErr = fmt.Errorf("mcp auto-onboard: %d %s: %s", resp.StatusCode, http.StatusText(resp.StatusCode), truncateMCPBody(redactMCPBody(string(out)), 500)) // fall through to retry } } @@ -199,3 +207,16 @@ func truncateMCPBody(s string, n int) string { } return s[:n] + "…" } + +// mcpTokenRedactRe scrubs OAuth secrets from an MCP response body. The POST we +// send carries access_token / refresh_token (the Bitrix→MCP onboarding bridge); +// a naive MCP server that echoes the request back in its 4xx/5xx error body +// would otherwise leak those tokens into goclaw logs, breaking the package's +// "log token lengths, never values" discipline. +var mcpTokenRedactRe = regexp.MustCompile(`(?i)("(?:access_token|refresh_token|client_secret)"\s*:\s*")[^"]*(")`) + +// redactMCPBody replaces OAuth secret values in an MCP body with a placeholder +// before the body is interpolated into an error string / log line. +func redactMCPBody(s string) string { + return mcpTokenRedactRe.ReplaceAllString(s, `${1}[redacted]${2}`) +} diff --git a/internal/channels/bitrix24/mcp_client_test.go b/internal/channels/bitrix24/mcp_client_test.go index 550d6bb9..5f7eaec9 100644 --- a/internal/channels/bitrix24/mcp_client_test.go +++ b/internal/channels/bitrix24/mcp_client_test.go @@ -51,10 +51,10 @@ func TestMCPClient_AutoOnboard_Success(t *testing.T) { if gotPath != "/api/auto-onboard" { t.Fatalf("wrong path: %q", gotPath) } - // Path B: no Authorization header — MCP server authenticates via the - // caller-supplied Bitrix access_token in the body, not a bearer token. + // Auto-onboard contract: no Authorization header — MCP server authenticates + // via the caller-supplied Bitrix access_token in the body, not a bearer token. if gotAuth != "" { - t.Fatalf("expected no Authorization header under Path B, got: %q", gotAuth) + t.Fatalf("expected no Authorization header for auto-onboard, got: %q", gotAuth) } if gotCT != "application/json" { t.Fatalf("wrong content-type: %q", gotCT) @@ -225,6 +225,12 @@ func TestIsGroupMessageType(t *testing.T) { "X": true, "x": true, " X ": true, + // "B" = Bitrix workgroup / Collab (SONET_GROUP) chat. Observed on + // real ONIMBOTMESSAGEADD payloads where CHAT_TYPE=B and + // CHAT_ENTITY_TYPE=SONET_GROUP; multi-user with @mention gating. + "B": true, + "b": true, + " B ": true, "unknown": false, } for input, want := range cases { diff --git a/internal/channels/bitrix24/mcp_redact_test.go b/internal/channels/bitrix24/mcp_redact_test.go new file mode 100644 index 00000000..bb2f061a --- /dev/null +++ b/internal/channels/bitrix24/mcp_redact_test.go @@ -0,0 +1,56 @@ +package bitrix24 + +import ( + "strings" + "testing" +) + +func TestRedactMCPBody(t *testing.T) { + cases := []struct { + name string + in string + mustHide []string // substrings that must NOT survive + mustKeep []string // substrings that must survive + }{ + { + name: "access_token echoed in error body", + in: `{"error":"bad request","received":{"access_token":"abc123SECRET","domain":"acme.bitrix24.com"}}`, + mustHide: []string{"abc123SECRET"}, + mustKeep: []string{"acme.bitrix24.com", "[redacted]"}, + }, + { + name: "refresh_token + client_secret", + in: `{"refresh_token":"rrrSECRET","client_secret":"cccSECRET","ok":false}`, + mustHide: []string{"rrrSECRET", "cccSECRET"}, + mustKeep: []string{"[redacted]", `"ok":false`}, + }, + { + name: "spaced colon still scrubbed", + in: `{ "access_token" : "spacedSECRET" }`, + mustHide: []string{"spacedSECRET"}, + mustKeep: []string{"[redacted]"}, + }, + { + name: "no tokens — unchanged", + in: `{"error":"tenant_not_installed"}`, + mustHide: nil, + mustKeep: []string{`"error":"tenant_not_installed"`}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := redactMCPBody(tc.in) + for _, h := range tc.mustHide { + if strings.Contains(got, h) { + t.Errorf("secret %q leaked through redaction: %q", h, got) + } + } + for _, k := range tc.mustKeep { + if !strings.Contains(got, k) { + t.Errorf("expected %q to survive, got %q", k, got) + } + } + }) + } +} diff --git a/internal/channels/bitrix24/metadata_keys.go b/internal/channels/bitrix24/metadata_keys.go new file mode 100644 index 00000000..39452a57 --- /dev/null +++ b/internal/channels/bitrix24/metadata_keys.go @@ -0,0 +1,51 @@ +package bitrix24 + +// Metadata keys used to propagate Bitrix24-specific context from inbound +// events through bus.InboundMessage → bus.OutboundMessage → Send(). +// Pattern follows existing keys (bitrix_address_user_id, bitrix_chat_entity_*, +// bitrix_dialog_id, etc.). Defining as constants gives a single source of +// truth that handle.go, gateway_consumer_normal.go, and send.go can share. +const ( + // MetaKeyVisibility distinguishes whisper (internal-only) vs public + // (forwarded to external connector) messages. Set on inbound by + // handle.go from EventParams.IsHiddenMessage. Read on outbound by + // Send() to route through imbot.message.add with SKIP_CONNECTOR=Y + // (whisper) or imbot.v2.Chat.Message.send (public). + MetaKeyVisibility = "bitrix_visibility" + + // MetaKeyMessageID is the MESSAGE_ID of the inbound message that + // triggered this exchange. Set on inbound by handle.go. Read on + // outbound v2 path → set as fields.replyId so the Bitrix UI shows + // the bot's reply linked to the original. + // + // NOTE: this key was already in use before this refactor; the + // constant just documents it. Do not rename without grepping for + // the literal "bitrix_message_id" across the repo. + MetaKeyMessageID = "bitrix_message_id" + + // MetaKeySenderPrefix carries the openline sender tag echo extracted from an + // inbound openline message by handle.go. For the 3-token connector layout it + // is "#msgId" (msgId only); for the legacy single-number layout it is the + // canonical "[name] #msgId"; for name-only it is "[name]". Forwarded by + // gateway_consumer_normal.go and read on outbound by Send(), which prepends + // it to the reply so the Bitrix Open Channel connector can route the answer + // back to the right external message. Empty / absent for plain Bitrix24 chats. + MetaKeySenderPrefix = "bitrix_sender_prefix" + + // MetaKeyParticipantUserID carries a per-participant synthetic user ID built + // from the external person's uid parsed out of the connector's 3-token sender + // tag ("[Name] #uid #msgId"). Shape: "openlines:{channelInstance}:{chatID}:{uid}". + // Set by handle.go ONLY when FromIsConnector=true and a uid was present. Read + // by gateway_consumer_normal.go to scope per-person USER.md / memory / seeding + // instead of the group-level fallback. Empty / absent for legacy single-number, + // name-only, or non-connector (operator) messages — those degrade safely to the + // group-level userID. + MetaKeyParticipantUserID = "participant_user_id" +) + +// Values for MetaKeyVisibility. Stored as strings (not bool) so callers +// can distinguish "explicitly public" from "absent" if needed in future. +const ( + VisibilityWhisper = "whisper" + VisibilityPublic = "public" +) diff --git a/internal/channels/bitrix24/orphan_destroy.go b/internal/channels/bitrix24/orphan_destroy.go index 5789da73..d3ecef33 100644 --- a/internal/channels/bitrix24/orphan_destroy.go +++ b/internal/channels/bitrix24/orphan_destroy.go @@ -68,11 +68,11 @@ func DestroyOrphanBot( return nil // no bot was ever registered for this code } - if _, callErr := portal.Client().Call(ctx, "imbot.unregister", map[string]any{ - "BOT_ID": botID, + if _, callErr := portal.Client().Call(ctx, "imbot.v2.Bot.unregister", map[string]any{ + "botId": botID, }); callErr != nil { if !isBotNotFoundError(callErr) { - slog.Warn("bitrix24 orphan destroy: imbot.unregister failed", + slog.Warn("bitrix24 orphan destroy: imbot.v2.Bot.unregister failed", "tenant", tenantID, "portal", cfg.Portal, "bot_code", cfg.BotCode, "bot_id", botID, "err", callErr) } else { diff --git a/internal/channels/bitrix24/provisioner.go b/internal/channels/bitrix24/provisioner.go index 4f7d883e..653dedc3 100644 --- a/internal/channels/bitrix24/provisioner.go +++ b/internal/channels/bitrix24/provisioner.go @@ -72,12 +72,13 @@ var ( // 2. Instance config has both mcp_server_name and mcp_base_url set. // 3. The mcp_servers row exists (looked up by name). // -// Path B authentication (see mcp_client.go doc): the MCP server -// authenticates each /api/auto-onboard call via the caller-supplied Bitrix -// access_token by calling Bitrix `profile` and matching the token-owner -// ID against bitrix_user_id — no shared admin secret is required, so -// multi-tenant isolation holds naturally (each portal's users authenticate -// with their own per-portal OAuth tokens). +// Bitrix24 OAuth → existing mcp_user_credentials bridge (Bitrix-specific +// glue — see mcp_client.go doc): the MCP server authenticates each +// /api/auto-onboard call via the caller-supplied Bitrix access_token by +// calling Bitrix `profile` and matching the token-owner ID against +// bitrix_user_id — no shared admin secret is required, so multi-tenant +// isolation holds naturally (each portal's users authenticate with their +// own per-portal OAuth tokens). // // Any single missing piece leaves the channel usable but with // provisioning off — that's the operator's "staged rollout" path: install @@ -374,7 +375,10 @@ func (c *Channel) notifyUserOfMCPIssueOnce(ctx context.Context, userID, chatID s // 2. The notice is plain text — no BBCode conversion, no chunking // (well under the 4000-rune limit), no media. Send's pipeline is // overkill. - if err := c.sendChunk(ctx, chatID, mcpUserNotifyMessage); err != nil { + // Provisioner notice is a one-off staff alert sent into the user's + // chat with the bot — always public (default v2 path), no replyId + // because there's no inbound message to link back to. + if err := c.sendChunk(ctx, chatID, mcpUserNotifyMessage, sendOptions{visibility: VisibilityPublic}); err != nil { slog.Debug("bitrix24 mcp: failed to send user degradation notice", "channel", c.Name(), "user", userID, "chat_id", chatID, "err", err) } diff --git a/internal/channels/bitrix24/provisioner_test.go b/internal/channels/bitrix24/provisioner_test.go index 6343025c..88c8b192 100644 --- a/internal/channels/bitrix24/provisioner_test.go +++ b/internal/channels/bitrix24/provisioner_test.go @@ -558,7 +558,7 @@ func TestProvisionIfMissing_MissingAuthBlock(t *testing.T) { // Each case should leave mcpClient nil + mcpServerID zero, so // provisionIfMissing returns ErrProvisionDisabled. // -// Path B auth note: there is no longer an admin-token branch to test — +// Auto-onboard auth note: there is no longer an admin-token branch to test — // the MCP server authenticates each /api/auto-onboard call via the // caller-supplied Bitrix access_token, not a shared bearer. func TestInitMCPProvisioner_DisabledModes(t *testing.T) { diff --git a/internal/channels/bitrix24/register.go b/internal/channels/bitrix24/register.go index 169e612c..b0d2a763 100644 --- a/internal/channels/bitrix24/register.go +++ b/internal/channels/bitrix24/register.go @@ -120,7 +120,7 @@ func (c *Channel) unregisterBot(ctx context.Context, botID int) error { if client == nil { return errors.New("bitrix24 unregister: client not initialised") } - _, err := client.Call(ctx, "imbot.unregister", map[string]any{"BOT_ID": botID}) + _, err := client.Call(ctx, "imbot.v2.Bot.unregister", map[string]any{"botId": botID}) if err == nil { return nil } @@ -226,21 +226,53 @@ func (c *Channel) verifyBot(ctx context.Context, botID int) (bool, error) { return false, errors.New("bitrix24 verify: client not initialised") } - resp, err := client.Call(ctx, "imbot.bot.list", nil) + found, err := c.forEachBotPage(ctx, client, func(resp *RawResult) bool { + return responseContainsBotID(resp, botID) + }) if err != nil { - // Older portals expose a different endpoint name; try the alternate. - alt, altErr := client.Call(ctx, "imbot.list", nil) - if altErr != nil { - // Surface BOTH errors so operators can see whether this is a - // portal-side outage (both fail the same way) vs. an endpoint - // naming issue (only one side fails). - return false, fmt.Errorf("bitrix24 verify: %w", - errors.Join(err, altErr)) - } - resp = alt + return false, fmt.Errorf("bitrix24 verify: %w", err) } + return found, nil +} - return responseContainsBotID(resp, botID), nil +// botListPageLimit matches the imbot.v2.Bot.list default page size (50). We page +// explicitly via limit/offset and honor result.hasNextPage so portals with more +// than one page of bots are fully scanned — scanning only page 1 makes +// verify/lookup silently fail for any bot past the first 50. +const botListPageLimit = 50 + +// maxBotListPages backstops the pagination loop against a server that keeps +// reporting hasNextPage=true (or a non-advancing offset). 40 pages = 2000 bots, +// far beyond any real application. +const maxBotListPages = 40 + +// forEachBotPage pages through imbot.v2.Bot.list and calls scan on each page's +// raw result. scan returns true to stop early (match found). Returns +// (true, nil) on a match, (false, nil) when all pages are exhausted with no +// match, (false, err) on a transport error. Legacy non-paginated envelopes +// (no hasNextPage field) are treated as a single page and stop after one call. +func (c *Channel) forEachBotPage(ctx context.Context, client *Client, scan func(*RawResult) bool) (bool, error) { + offset := 0 + for page := 0; page < maxBotListPages; page++ { + resp, err := client.Call(ctx, "imbot.v2.Bot.list", map[string]any{ + "limit": botListPageLimit, + "offset": offset, + }) + if err != nil { + return false, err + } + if scan(resp) { + return true, nil + } + var p struct { + HasNextPage bool `json:"hasNextPage"` + } + if err := json.Unmarshal(resp.Result, &p); err != nil || !p.HasNextPage { + return false, nil + } + offset += botListPageLimit + } + return false, nil } // findBotIDByCode scans the portal for a bot whose CODE equals the given @@ -255,17 +287,17 @@ func (c *Channel) findBotIDByCode(ctx context.Context, code string) (int, error) return 0, errors.New("bitrix24 find: client not initialised") } - resp, err := client.Call(ctx, "imbot.bot.list", nil) - if err != nil { - alt, altErr := client.Call(ctx, "imbot.list", nil) - if altErr != nil { - return 0, fmt.Errorf("bitrix24 find-by-code: %w", - errors.Join(err, altErr)) + var foundID int + if _, err := c.forEachBotPage(ctx, client, func(resp *RawResult) bool { + if id := findBotIDByCodeInResponse(resp, code); id > 0 { + foundID = id + return true } - resp = alt + return false + }); err != nil { + return 0, fmt.Errorf("bitrix24 find-by-code: %w", err) } - - return findBotIDByCodeInResponse(resp, code), nil + return foundID, nil } // fetchAvatarBase64 downloads an image and returns it base64-encoded. @@ -372,74 +404,84 @@ func intFromResult(r *RawResult) int { // The shape is either an array (legacy) or an object keyed by bot id // (newer portals) — handle both transparently. func responseContainsBotID(r *RawResult, botID int) bool { - if r == nil || len(r.Result) == 0 || botID <= 0 { + if r == nil || botID <= 0 { return false } - - // Array form. - var arr []map[string]json.RawMessage - if err := json.Unmarshal(r.Result, &arr); err == nil { - for _, row := range arr { - if rowHasBotID(row, botID) { - return true - } - } - return false - } - - // Map form — keys may be numeric strings or {BOT_ID: ..., CODE: ...}. - var obj map[string]map[string]json.RawMessage - if err := json.Unmarshal(r.Result, &obj); err == nil { - for key, row := range obj { - if key == fmt.Sprintf("%d", botID) { - return true - } - if rowHasBotID(row, botID) { - return true - } + for _, row := range botListRows(r.Result) { + if rowHasBotID(row, botID) { + return true } } return false } -// findBotIDByCodeInResponse scans imbot.list output for a CODE match and -// returns the associated bot_id. Returns 0 if the code isn't found. +// findBotIDByCodeInResponse scans bot-list output for a CODE match and returns +// the associated bot_id. Returns 0 if the code isn't found. func findBotIDByCodeInResponse(r *RawResult, code string) int { - if r == nil || len(r.Result) == 0 || code == "" { + if r == nil || code == "" { return 0 } - - // Array form. - var arr []map[string]json.RawMessage - if err := json.Unmarshal(r.Result, &arr); err == nil { - for _, row := range arr { - if rowCodeMatches(row, code) { - if id := extractBotID(row); id > 0 { - return id - } - } - } - return 0 - } - - // Map form. - var obj map[string]map[string]json.RawMessage - if err := json.Unmarshal(r.Result, &obj); err == nil { - for key, row := range obj { - if rowCodeMatches(row, code) { - if id := extractBotID(row); id > 0 { - return id - } - // Fall back to the object key if it's numeric (older portals). - if id := atoiSafe(key); id > 0 { - return id - } + for _, row := range botListRows(r.Result) { + if rowCodeMatches(row, code) { + if id := extractBotID(row); id > 0 { + return id } } } return 0 } +// botListRows normalizes the differing envelopes of the bot-list endpoints into +// a flat slice of row maps: +// - imbot.v2.Bot.list : {"bots":[{...}], "users":[...], "hasNextPage":bool} +// - imbot.bot.list : [{...}, ...] (legacy top-level array) +// - older portals : {"982":{...}, ...} (map keyed by bot id) +// +// For the map form, a numeric key is injected as a synthetic "id" when the row +// itself lacks one — preserving the legacy key-as-id fallback so callers don't +// need to know which shape the portal returned. +func botListRows(result json.RawMessage) []map[string]json.RawMessage { + if len(result) == 0 { + return nil + } + + // v2 wraps rows under "bots"; unwrap before scanning. A legacy array or + // numeric-keyed map fails this unmarshal (or yields no "bots"), leaving + // payload == result. + payload := result + var wrap struct { + Bots json.RawMessage `json:"bots"` + } + if err := json.Unmarshal(result, &wrap); err == nil && len(wrap.Bots) > 0 { + payload = wrap.Bots + } + + // Array form (v2 bots / legacy bot.list). + var arr []map[string]json.RawMessage + if err := json.Unmarshal(payload, &arr); err == nil { + return arr + } + + // Map form (older portals): keyed by bot id. + var obj map[string]map[string]json.RawMessage + if err := json.Unmarshal(payload, &obj); err == nil { + rows := make([]map[string]json.RawMessage, 0, len(obj)) + for key, row := range obj { + if row == nil { + row = map[string]json.RawMessage{} + } + if extractBotID(row) == 0 { + if id := atoiSafe(key); id > 0 { + row["id"] = json.RawMessage(fmt.Sprintf("%d", id)) + } + } + rows = append(rows, row) + } + return rows + } + return nil +} + func rowHasBotID(row map[string]json.RawMessage, botID int) bool { return extractBotID(row) == botID } diff --git a/internal/channels/bitrix24/register_idempotency_test.go b/internal/channels/bitrix24/register_idempotency_test.go index 76c09583..d6bb25c2 100644 --- a/internal/channels/bitrix24/register_idempotency_test.go +++ b/internal/channels/bitrix24/register_idempotency_test.go @@ -88,11 +88,11 @@ func TestRegisterBot_Path1_CachedBotIDStillValid_NoRegisterCall(t *testing.T) { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(`{"error":"should_not_be_called"}`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&listHits, 1) w.Header().Set("Content-Type", "application/json") // Bot 42 still present on portal → verifyBot returns true. - _, _ = w.Write([]byte(`{"result":[{"BOT_ID":42,"CODE":"support_bot"}]}`)) + _, _ = w.Write([]byte(`{"result":{"bots":[{"id":42,"code":"support_bot"}],"users":[],"hasNextPage":false}}`)) }, } srv := httptest.NewServer(h) @@ -133,10 +133,10 @@ func TestRegisterBot_Path1_CachedBotIDMissing_FallsThroughToRegister(t *testing. w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"result":777}`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { // Cached bot 42 is NOT in the portal's list → verifyBot returns false. w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"result":[{"BOT_ID":99,"CODE":"other_bot"}]}`)) + _, _ = w.Write([]byte(`{"result":{"bots":[{"id":99,"code":"other_bot"}]}}`)) }, } srv := httptest.NewServer(h) @@ -179,10 +179,10 @@ func TestRegisterBot_Path2_FreshRegisterSucceeds(t *testing.T) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"result":{"BOT_ID":555}}`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&listHits, 1) w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"result":[]}`)) + _, _ = w.Write([]byte(`{"result":{"bots":[]}}`)) }, } srv := httptest.NewServer(h) @@ -226,13 +226,13 @@ func TestRegisterBot_Path3_DuplicateCode_ResolvesViaList(t *testing.T) { "error_description":"Bot code already exists on portal" }`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&listHits, 1) w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"result":[ - {"BOT_ID":888,"CODE":"support_bot"}, - {"BOT_ID":999,"CODE":"other"} - ]}`)) + _, _ = w.Write([]byte(`{"result":{"bots":[ + {"id":888,"code":"support_bot"}, + {"id":999,"code":"other"} + ]}}`)) }, } srv := httptest.NewServer(h) @@ -266,11 +266,11 @@ func TestRegisterBot_Path3_DuplicateCode_NotInList_Errors(t *testing.T) { "error_description":"duplicate bot code" }`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") // None of these match "support_bot" → fallback should fail with a // clear "no bot with CODE" error rather than returning 0 success. - _, _ = w.Write([]byte(`{"result":[{"BOT_ID":1,"CODE":"nope"}]}`)) + _, _ = w.Write([]byte(`{"result":{"bots":[{"id":1,"code":"nope"}]}}`)) }, } srv := httptest.NewServer(h) @@ -291,7 +291,11 @@ func TestRegisterBot_Path3_DuplicateCode_NotInList_Errors(t *testing.T) { } } -func TestRegisterBot_Path3_BothListEndpointsFail_JoinsErrors(t *testing.T) { +// TestRegisterBot_Path3_ListFails_SurfacesError covers the duplicate-code +// fallback when the bot-list lookup itself fails. After the v2 migration there +// is a single list endpoint (imbot.v2.Bot.list) — its error must surface +// directly (no more dual-endpoint errors.Join). +func TestRegisterBot_Path3_ListFails_SurfacesError(t *testing.T) { h := restHandler{ "imbot.register": func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -301,15 +305,10 @@ func TestRegisterBot_Path3_BothListEndpointsFail_JoinsErrors(t *testing.T) { "error_description":"bot code already exists" }`)) }, - "imbot.bot.list": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.list": func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"error":"LIST_OUTAGE","error_description":"primary endpoint down"}`)) - }, - "imbot.list": func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"error":"ALT_OUTAGE","error_description":"alt endpoint also down"}`)) + _, _ = w.Write([]byte(`{"error":"LIST_OUTAGE","error_description":"endpoint down"}`)) }, } srv := httptest.NewServer(h) @@ -323,16 +322,10 @@ func TestRegisterBot_Path3_BothListEndpointsFail_JoinsErrors(t *testing.T) { _, err := ch.registerBot(context.Background()) if err == nil { - t.Fatal("expected error when both list endpoints fail") + t.Fatal("expected error when the bot-list lookup fails") } - msg := err.Error() - // Both underlying error codes should be visible in the joined error so - // operators can see we tried the fallback and both sides failed. - if !strings.Contains(msg, "LIST_OUTAGE") { - t.Errorf("primary error not surfaced: %s", msg) - } - if !strings.Contains(msg, "ALT_OUTAGE") { - t.Errorf("alt error not surfaced (errors.Join missing): %s", msg) + if msg := err.Error(); !strings.Contains(msg, "LIST_OUTAGE") { + t.Errorf("list error not surfaced: %s", msg) } } @@ -417,7 +410,7 @@ func TestEventHandlerURL_FallsBackToLegacyConfig(t *testing.T) { func TestUnregisterBot_Success(t *testing.T) { var calls int32 h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&calls, 1) w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"result":true}`)) @@ -442,7 +435,7 @@ func TestUnregisterBot_Success(t *testing.T) { // "bot not found" (because admin already deleted via UI) is treated as success. func TestUnregisterBot_BotNotFound(t *testing.T) { h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusBadRequest) _, _ = w.Write([]byte(`{"error":"ERROR_BOT_NOT_FOUND","error_description":"Bot not found on this portal"}`)) }, @@ -463,7 +456,7 @@ func TestUnregisterBot_BotNotFound(t *testing.T) { // caller can log a warn and move on — must NOT be swallowed. func TestUnregisterBot_TransportError(t *testing.T) { h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(`{"error":"INTERNAL","error_description":"portal went away"}`)) }, @@ -485,7 +478,7 @@ func TestUnregisterBot_TransportError(t *testing.T) { func TestUnregisterBot_ZeroBotID(t *testing.T) { var calls int32 h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&calls, 1) _, _ = w.Write([]byte(`{"result":true}`)) }, @@ -510,7 +503,7 @@ func TestUnregisterBot_ZeroBotID(t *testing.T) { func TestDestroy_FullFlow(t *testing.T) { var unregCalls int32 h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&unregCalls, 1) _, _ = w.Write([]byte(`{"result":true}`)) }, @@ -550,7 +543,7 @@ func TestDestroy_FullFlow(t *testing.T) { func TestDestroy_BotIDZero(t *testing.T) { var unregCalls int32 h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { atomic.AddInt32(&unregCalls, 1) }, } @@ -576,7 +569,7 @@ func TestDestroy_BotIDZero(t *testing.T) { // local channel is stopped — DB delete upstream must not be blocked. func TestDestroy_UnregisterFailureProceedsToCleanup(t *testing.T) { h := restHandler{ - "imbot.unregister": func(w http.ResponseWriter, r *http.Request) { + "imbot.v2.Bot.unregister": func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(`{"error":"INTERNAL","error_description":"portal 5xx"}`)) }, diff --git a/internal/channels/bitrix24/send.go b/internal/channels/bitrix24/send.go index c361eb0b..dfb69275 100644 --- a/internal/channels/bitrix24/send.go +++ b/internal/channels/bitrix24/send.go @@ -13,6 +13,22 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bus" ) +// sendOptions captures per-message routing context extracted from +// OutboundMessage.Metadata once in Send() and threaded through every +// chunk. Avoids re-parsing the same keys for each chunk in the loop. +type sendOptions struct { + // visibility picks the outbound API: VisibilityWhisper → v1 + // imbot.message.add + SKIP_CONNECTOR=Y; VisibilityPublic → v2 + // imbot.v2.Chat.Message.send. Default public for backward-compat. + visibility string + // replyToMID is the MESSAGE_ID of the inbound that triggered this + // reply. > 0 means the v2 path will set fields.replyId so the + // Bitrix UI links bot reply to the original. 0 = no link. + // v1 whisper path ignores this — imbot.message.add has no + // equivalent parameter. + replyToMID int +} + // rateLimitRetryDelay is how long we wait after Bitrix24 returns // QUERY_LIMIT_EXCEEDED before retrying. Bitrix's own recommendation is // 2 seconds; we only retry once per chunk to avoid queueing storms. @@ -48,10 +64,15 @@ func (c *Channel) Send(ctx context.Context, msg bus.OutboundMessage) error { return errors.New("bitrix24: missing chat_id on outbound message") } - // Phase 06 will upload real media here; Phase 03 logs + drops. + // Upload any media attachments first via imbot.v2.File.upload. The text body + // is delivered separately below, so a media failure never drops the text and + // we never double-post. A media-only message (empty Content) returns at the + // empty-text guard below after the upload completes. if len(msg.Media) > 0 { - slog.Info("bitrix24: media attachments present — Phase 06 pending; sending text only", - "chat_id", msg.ChatID, "count", len(msg.Media)) + if err := c.sendMedia(ctx, msg); err != nil { + slog.Warn("bitrix24: one or more media uploads failed; continuing with text", + "chat_id", msg.ChatID, "err", err) + } } text := strings.TrimSpace(msg.Content) @@ -86,6 +107,27 @@ func (c *Channel) Send(ctx context.Context, msg bus.OutboundMessage) error { text = mention + " " + text } + // Openline: prepend the sender tag captured from the inbound message + // ("[name] #id:") so the Bitrix Open Channel connector routes this reply + // back to the right external user. Goes at the very START (before any + // @mention) because the connector parses the leading tag. Prepended BEFORE + // chunkText so it only lands on the first chunk. Empty for non-openline + // replies → no-op. + if prefix := msg.Metadata[MetaKeySenderPrefix]; prefix != "" { + text = prefix + " " + text + } + + // Resolve outbound routing once for the whole message — same for all + // chunks. Missing/unknown visibility defaults to public (= legacy v2 + // path), so messages from code paths that don't propagate the key + // still deliver as before. + opts := resolveSendOptions(msg.Metadata) + slog.Debug("bitrix24 send: routing", + "chat_id", msg.ChatID, + "visibility", opts.visibility, + "reply_to_mid", opts.replyToMID, + ) + // TextChunkLimit is always populated by applyConfigDefaults (4000) — // chunkText also treats limit<=0 as "use default" as a safety net, so we // don't duplicate the fallback here. @@ -94,51 +136,83 @@ func (c *Channel) Send(ctx context.Context, msg bus.OutboundMessage) error { if err := ctx.Err(); err != nil { return err } - if err := c.sendChunk(ctx, msg.ChatID, chunk); err != nil { + if err := c.sendChunk(ctx, msg.ChatID, chunk, opts); err != nil { return fmt.Errorf("bitrix24 send chunk %d/%d: %w", i+1, len(chunks), err) } } return nil } -// sendChunk posts a single chunk via imbot.message.add. One automatic retry -// on QUERY_LIMIT_EXCEEDED; other errors bubble unchanged. -func (c *Channel) sendChunk(ctx context.Context, chatID, chunk string) error { +// resolveSendOptions pulls routing knobs out of OutboundMessage.Metadata. +// Unknown / missing values fall back to defaults that preserve the +// pre-refactor (public v2) behavior so any caller that doesn't populate +// these keys still works. +func resolveSendOptions(meta map[string]string) sendOptions { + out := sendOptions{visibility: VisibilityPublic} + if v := meta[MetaKeyVisibility]; v == VisibilityWhisper { + out.visibility = VisibilityWhisper + } + // MessageID is the inbound that we're replying to. Parse to int for the + // v2 fields.replyId param. Non-numeric / zero → skip (no link). + if s := meta[MetaKeyMessageID]; s != "" { + if n, err := strconv.Atoi(s); err == nil && n > 0 { + out.replyToMID = n + } + } + return out +} + +// sendChunk dispatches a single chunk to the right outbound API based on +// the resolved visibility. Whisper → v1 imbot.message.add with +// SKIP_CONNECTOR=Y (in send_v1.go); public → v2 imbot.v2.Chat.Message.send +// with optional fields.replyId (in send_v2.go). The split keeps each +// concrete method's params + quirks in a focused file. +func (c *Channel) sendChunk(ctx context.Context, chatID, chunk string, opts sendOptions) error { + if opts.visibility == VisibilityWhisper { + return c.sendChunkV1Whisper(ctx, chatID, chunk) + } + return c.sendChunkV2Public(ctx, chatID, chunk, opts.replyToMID) +} + +// callWithRateLimitRetry executes a single client.Call and retries ONCE +// after rateLimitRetryDelay on QUERY_LIMIT_EXCEEDED / OPERATION_TIME_LIMIT. +// Non-rate-limit errors bubble unchanged. Used by both v1 and v2 send +// paths to keep retry semantics identical regardless of which API was +// chosen. +func (c *Channel) callWithRateLimitRetry( + ctx context.Context, + method string, + params map[string]any, + chatID string, + botID int, +) error { client := c.Client() - botID := c.BotID() if client == nil || botID <= 0 { // Channel was shut down between Send's liveness check and here. // Report as a transport error so the caller can retry if desired. return errors.New("bitrix24: channel lost during send") } - params := map[string]any{ - "BOT_ID": botID, - "DIALOG_ID": chatID, - "MESSAGE": chunk, - "SYSTEM": "N", - } - - _, err := client.Call(ctx, "imbot.message.add", params) + _, err := client.Call(ctx, method, params) if err == nil { return nil } if !isRateLimitErr(err) { - slog.Warn("bitrix24: imbot.message.add failed", - "chat_id", chatID, "bot_id", botID, "err", err) + slog.Warn("bitrix24: send failed", + "method", method, "chat_id", chatID, "bot_id", botID, "err", err) return err } // One retry after a short backoff. Use a context-aware sleep so shutdown // doesn't hang for 2 seconds. slog.Warn("bitrix24: rate limit hit — retrying once", - "chat_id", chatID, "bot_id", botID) + "method", method, "chat_id", chatID, "bot_id", botID) select { case <-time.After(rateLimitRetryDelay): case <-ctx.Done(): return ctx.Err() } - _, err = client.Call(ctx, "imbot.message.add", params) + _, err = client.Call(ctx, method, params) return err } @@ -158,6 +232,16 @@ func buildAddressMention(meta map[string]string, botID int) string { if userID == "" { return "" } + // [USER=] BBCode is only valid for a numeric Bitrix user id. Synthetic + // scope ids — notably the openline per-participant id + // "openlines:{instance}:{chat}:{uid}" set by handle.go — are not real Bitrix + // users; emitting them would render as literal garbage in the chat. The + // external customer behind a connector relay has no Bitrix user id to mention + // anyway (routing is driven by the leading "#msgId" echo), so skip the + // mention for any non-numeric addressee. + if !isNumericID(userID) { + return "" + } // Self-mention guard: bot replying to its own synthetic relay, or a // future code path injecting the bot's id by mistake. Don't @mention // the bot to itself — Bitrix would render "@Bot Synity" in the bot's @@ -168,6 +252,20 @@ func buildAddressMention(meta map[string]string, botID int) string { return "[USER=" + userID + "][/USER]" } +// isNumericID reports whether s is a non-empty run of ASCII digits — the shape +// of a real Bitrix user id. Used to gate [USER=] BBCode emission. +func isNumericID(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + // isRateLimitErr detects Bitrix24's rate-limit response. The canonical code // is QUERY_LIMIT_EXCEEDED on the RawResult envelope; net timeouts aren't // classified here — caller treats them as transport errors. diff --git a/internal/channels/bitrix24/send_media.go b/internal/channels/bitrix24/send_media.go new file mode 100644 index 00000000..7a7222e2 --- /dev/null +++ b/internal/channels/bitrix24/send_media.go @@ -0,0 +1,115 @@ +package bitrix24 + +import ( + "context" + "encoding/base64" + "fmt" + "io" + "log/slog" + "os" + "path/filepath" + + "github.com/nextlevelbuilder/goclaw/internal/bus" +) + +// maxOutboundMediaBytesFallback caps a single outbound file when the channel +// config doesn't specify one. Bitrix imbot.v2.File.upload accepts up to 100 MB +// of Base64 content, but we keep a tighter ceiling so the POST body (Base64 +// inflates the payload by ~33%) stays reasonable. Normally cfg.MediaMaxMB wins. +const maxOutboundMediaBytesFallback = 20 * 1024 * 1024 + +// maxOutboundFiles caps how many attachments a single outbound message uploads, +// mirroring maxInboundFiles. Stops an LLM that emits a large result.Media (or +// many MEDIA: tokens) from triggering a serial REST upload storm + RAM spike. +const maxOutboundFiles = 10 + +// outboundMediaCap returns the per-file outbound size limit in bytes, honoring +// the same cfg.MediaMaxMB knob used for inbound so the two directions stay +// symmetric (default 20 MB via applyConfigDefaults). +func (c *Channel) outboundMediaCap() int64 { + if mb := int64(c.cfg.MediaMaxMB); mb > 0 { + return mb * 1024 * 1024 + } + return maxOutboundMediaBytesFallback +} + +// sendMedia uploads each attachment on an outbound message to the Bitrix24 chat +// via imbot.v2.File.upload (a single call uploads the file to Drive, attaches it +// to the chat, and posts it). The text body is deliberately NOT sent here — +// Send() delivers it separately through the normal text path, so a media failure +// never drops the text and we never double-post. +// +// Best-effort: a file that fails to read or upload is logged and skipped. +// Returns the first error encountered for caller visibility; the text path runs +// regardless of what this returns. +func (c *Channel) sendMedia(ctx context.Context, msg bus.OutboundMessage) error { + client := c.Client() + botID := c.BotID() + if client == nil || botID <= 0 { + return fmt.Errorf("bitrix24: channel not initialised for media upload") + } + + media := msg.Media + if len(media) > maxOutboundFiles { + slog.Warn("bitrix24: too many outbound attachments, capping", + "chat_id", msg.ChatID, "total", len(media), "cap", maxOutboundFiles) + media = media[:maxOutboundFiles] + } + + var firstErr error + for _, m := range media { + if err := c.uploadOneFile(ctx, client, botID, msg.ChatID, m); err != nil { + slog.Warn("bitrix24: media upload failed, skipping file", + "chat_id", msg.ChatID, "path", m.URL, "err", err) + if firstErr == nil { + firstErr = err + } + } + } + return firstErr +} + +// uploadOneFile reads a local file, Base64-encodes it, and posts it to the chat +// via imbot.v2.File.upload. msg.Media[].URL holds a local filesystem path (set +// by appendMediaToOutbound in the gateway consumer), not a remote URL. +func (c *Channel) uploadOneFile(ctx context.Context, client *Client, botID int, dialogID string, m bus.MediaAttachment) error { + if m.URL == "" { + return fmt.Errorf("empty media path") + } + // Bounded read IS the size gate (no TOCTOU). A separate os.Stat size-check + // then os.ReadFile would let the file grow past the cap between the two — the + // agent's own .uploads/ workspace is writable by concurrent tool/agent runs. + // Reading cap+1 via LimitReader detects an over-cap file at read time and + // caps the bytes loaded into memory regardless. + maxBytes := c.outboundMediaCap() + fh, err := os.Open(m.URL) + if err != nil { + return fmt.Errorf("open: %w", err) + } + defer fh.Close() + data, err := io.ReadAll(io.LimitReader(fh, maxBytes+1)) + if err != nil { + return fmt.Errorf("read: %w", err) + } + if int64(len(data)) > maxBytes { + return fmt.Errorf("file exceeds cap %d bytes", maxBytes) + } + + // Base64 content must NOT carry a data:*/*;base64, prefix (Bitrix requirement). + // botToken is omitted intentionally — not needed under OAuth. + if _, err := client.Call(ctx, "imbot.v2.File.upload", map[string]any{ + "botId": botID, + "dialogId": dialogID, + "fields": map[string]any{ + "FILE": map[string]any{ + "name": filepath.Base(m.URL), + "content": base64.StdEncoding.EncodeToString(data), + }, + }, + }); err != nil { + return fmt.Errorf("imbot.v2.File.upload: %w", err) + } + slog.Info("bitrix24: uploaded outbound file", + "chat_id", dialogID, "name", filepath.Base(m.URL), "bytes", len(data)) + return nil +} diff --git a/internal/channels/bitrix24/send_media_test.go b/internal/channels/bitrix24/send_media_test.go new file mode 100644 index 00000000..2e630d04 --- /dev/null +++ b/internal/channels/bitrix24/send_media_test.go @@ -0,0 +1,438 @@ +package bitrix24 + +import ( + "context" + "encoding/base64" + "os" + "path/filepath" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/bus" +) + +// TestUploadOneFile_HappyPath tests successful file upload with correct base64 encoding. +func TestUploadOneFile_HappyPath(t *testing.T) { + const testContent = "this is the file content" + + // Create a temp file with test content + tmpFile, err := os.CreateTemp("", "test_upload_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte(testContent)); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper to capture requests and return canned responses + rt := &captureRT{ + result: `{"result":{"fileId":12345}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + m := bus.MediaAttachment{URL: tmpFile.Name()} + err = ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + if err != nil { + t.Fatalf("uploadOneFile: %v", err) + } +} + +// TestUploadOneFile_EmptyPath tests that empty URL is rejected. +func TestUploadOneFile_EmptyPath(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + + m := bus.MediaAttachment{URL: ""} + err := ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + if err == nil { + t.Fatal("expected error for empty URL") + } +} + +// TestUploadOneFile_MissingFile tests that non-existent file is rejected. +func TestUploadOneFile_MissingFile(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + + m := bus.MediaAttachment{URL: "/nonexistent/file/path.txt"} + err := ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + if err == nil { + t.Fatal("expected error for missing file") + } +} + +// TestUploadOneFile_FileTooLarge tests that files exceeding maxOutboundMediaBytesFallback +// are rejected without an upload call. +func TestUploadOneFile_FileTooLarge(t *testing.T) { + // Create a temp file larger than the cap + tmpFile, err := os.CreateTemp("", "large_*.bin") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + + // Write more than maxOutboundMediaBytesFallback + largeSize := maxOutboundMediaBytesFallback + 1000 + if _, err := tmpFile.Write(make([]byte, largeSize)); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + + m := bus.MediaAttachment{URL: tmpFile.Name()} + err = ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + if err == nil { + t.Fatal("expected error for oversized file") + } +} + +// TestUploadOneFile_CorrectBase64Encoding tests that file content is +// correctly base64-encoded (without data: prefix) in the upload call. +func TestUploadOneFile_CorrectBase64Encoding(t *testing.T) { + const testContent = "hello world from file" + + tmpFile, err := os.CreateTemp("", "test_encoding_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte(testContent)); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper to capture the upload request + rt := &captureRT{ + result: `{"result":{"fileId":12345}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + m := bus.MediaAttachment{URL: tmpFile.Name()} + err = ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + if err != nil { + t.Fatalf("uploadOneFile: %v", err) + } + + // Verify the base64 content matches what we expect + if len(rt.reqs) == 0 { + t.Fatal("no requests captured") + } + capturedContent := rt.reqs[0].Get("fields[FILE][content]") + expectedB64 := base64.StdEncoding.EncodeToString([]byte(testContent)) + if capturedContent != expectedB64 { + t.Errorf("base64 content mismatch: got %q, want %q", capturedContent, expectedB64) + } +} + +// TestUploadOneFile_NoDataPrefix tests that base64 does NOT include a data: prefix. +func TestUploadOneFile_NoDataPrefix(t *testing.T) { + tmpFile, err := os.CreateTemp("", "test_prefix_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte("test")); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper to capture the upload request + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + m := bus.MediaAttachment{URL: tmpFile.Name()} + _ = ch.uploadOneFile(context.Background(), ch.Client(), 1, "chat123", m) + + // Verify NO data: prefix + if len(rt.reqs) == 0 { + t.Fatal("no requests captured") + } + capturedContent := rt.reqs[0].Get("fields[FILE][content]") + if len(capturedContent) > 5 && capturedContent[:5] == "data:" { + t.Errorf("base64 should NOT have data: prefix, got %q", capturedContent[:20]) + } +} + +// TestUploadOneFile_CorrectBotIDDialogID tests that botId and dialogId are +// passed correctly in the upload call. +func TestUploadOneFile_CorrectBotIDDialogID(t *testing.T) { + tmpFile, err := os.CreateTemp("", "test_ids_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte("data")); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper to capture the upload request + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + m := bus.MediaAttachment{URL: tmpFile.Name()} + _ = ch.uploadOneFile(context.Background(), ch.Client(), 42, "dialog999", m) + + // Verify botId and dialogId were passed correctly + if len(rt.reqs) == 0 { + t.Fatal("no requests captured") + } + capturedBotID := rt.reqs[0].Get("botId") + capturedDialogID := rt.reqs[0].Get("dialogId") + + if capturedBotID != "42" { + t.Errorf("botId = %q; want 42", capturedBotID) + } + if capturedDialogID != "dialog999" { + t.Errorf("dialogId = %q; want dialog999", capturedDialogID) + } +} + +// TestSendMedia_NoClient tests that sendMedia fails when channel has no client. +func TestSendMedia_NoClient(t *testing.T) { + ch := &Channel{} + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: "/some/file.txt"}, + }, + } + err := ch.sendMedia(context.Background(), msg) + if err == nil { + t.Fatal("expected error when channel has no client") + } +} + +// TestSendMedia_InvalidBotID tests that sendMedia fails when botID is invalid. +func TestSendMedia_InvalidBotID(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + ch.startMu.Lock() + ch.botID = 0 // Invalid botID + ch.startMu.Unlock() + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: "/some/file.txt"}, + }, + } + err := ch.sendMedia(context.Background(), msg) + if err == nil { + t.Fatal("expected error when botID is invalid") + } +} + +// TestSendMedia_EmptyMedia tests that empty media list returns no error. +func TestSendMedia_EmptyMedia(t *testing.T) { + ch, _ := newFakeChannelWithClient(t, NewClient("test.bitrix24.com", nil)) + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{}, + } + err := ch.sendMedia(context.Background(), msg) + if err != nil { + t.Fatalf("sendMedia with empty media should not error: %v", err) + } +} + +// TestSendMedia_SingleFile_Success tests uploading a single file successfully. +func TestSendMedia_SingleFile_Success(t *testing.T) { + tmpFile, err := os.CreateTemp("", "single_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte("content")); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: tmpFile.Name()}, + }, + } + err = ch.sendMedia(context.Background(), msg) + if err != nil { + t.Fatalf("sendMedia single file: %v", err) + } +} + +// TestSendMedia_MultipleFiles_AllSucceed tests uploading multiple files. +func TestSendMedia_MultipleFiles_AllSucceed(t *testing.T) { + // Create two temp files + tmpFile1, err := os.CreateTemp("", "file1_*.txt") + if err != nil { + t.Fatalf("create temp file 1: %v", err) + } + defer os.Remove(tmpFile1.Name()) + if _, err := tmpFile1.Write([]byte("content1")); err != nil { + t.Fatalf("write temp file 1: %v", err) + } + tmpFile1.Close() + + tmpFile2, err := os.CreateTemp("", "file2_*.txt") + if err != nil { + t.Fatalf("create temp file 2: %v", err) + } + defer os.Remove(tmpFile2.Name()) + if _, err := tmpFile2.Write([]byte("content2")); err != nil { + t.Fatalf("write temp file 2: %v", err) + } + tmpFile2.Close() + + // Use stub RoundTripper to track multiple uploads + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: tmpFile1.Name()}, + {URL: tmpFile2.Name()}, + }, + } + errSend := ch.sendMedia(context.Background(), msg) + if errSend != nil { + t.Fatalf("sendMedia multiple files: %v", errSend) + } + if len(rt.reqs) != 2 { + t.Errorf("expected 2 uploads, got %d", len(rt.reqs)) + } +} + +// TestSendMedia_PartialFailure tests that one file failing returns the first +// error but continues processing other files. +func TestSendMedia_PartialFailure(t *testing.T) { + tmpFile1, err := os.CreateTemp("", "good_*.txt") + if err != nil { + t.Fatalf("create temp file 1: %v", err) + } + defer os.Remove(tmpFile1.Name()) + if _, err := tmpFile1.Write([]byte("content1")); err != nil { + t.Fatalf("write temp file 1: %v", err) + } + tmpFile1.Close() + + tmpFile2, err := os.CreateTemp("", "bad_*.txt") + if err != nil { + t.Fatalf("create temp file 2: %v", err) + } + defer os.Remove(tmpFile2.Name()) + if _, err := tmpFile2.Write([]byte("content2")); err != nil { + t.Fatalf("write temp file 2: %v", err) + } + tmpFile2.Close() + + // Use stub RoundTripper that returns different responses for each request + rtPartial := &captureRTPartialFail{} + + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rtPartial)) + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: tmpFile1.Name()}, + {URL: tmpFile2.Name()}, + }, + } + errSend2 := ch.sendMedia(context.Background(), msg) + // Should return first error but have attempted both uploads + if errSend2 == nil { + t.Fatal("expected error from second file") + } + if rtPartial.calls != 2 { + t.Errorf("expected 2 upload attempts, got %d", rtPartial.calls) + } +} + +// TestSendMedia_SkipsWithMissingFile tests that a missing file is skipped +// and does not block other files. +func TestSendMedia_SkipsWithMissingFile(t *testing.T) { + tmpFile, err := os.CreateTemp("", "good_*.txt") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + defer os.Remove(tmpFile.Name()) + if _, err := tmpFile.Write([]byte("content")); err != nil { + t.Fatalf("write temp file: %v", err) + } + tmpFile.Close() + + // Use stub RoundTripper to track uploads + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: "/nonexistent/file1.txt"}, + {URL: tmpFile.Name()}, + {URL: "/nonexistent/file3.txt"}, + }, + } + errSend3 := ch.sendMedia(context.Background(), msg) + // Should return error from first missing file + if errSend3 == nil { + t.Fatal("expected error from missing files") + } + // sendMedia continues through all files: fails on file1 (missing), succeeds on file2 (good), fails on file3 (missing) + // So exactly 1 upload should be attempted (the good file) + if len(rt.reqs) != 1 { + t.Errorf("expected 1 upload attempt (the good file), got %d", len(rt.reqs)) + } +} + +// TestSendMedia_FilenameSanitized tests that uploaded filename uses filepath.Base +// (no path traversal). +func TestSendMedia_FilenameSanitized(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "test_") + if err != nil { + t.Fatalf("create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + // Create file with path separators in name + tmpFile := filepath.Join(tmpDir, "actual_file.txt") + if err := os.WriteFile(tmpFile, []byte("content"), 0644); err != nil { + t.Fatalf("write file: %v", err) + } + + // Use stub RoundTripper to capture filename + rt := &captureRT{ + result: `{"result":{"fileId":1}}`, + } + ch, _ := newFakeChannelWithClient(t, newStubClient("test.bitrix24.com", rt)) + + msg := bus.OutboundMessage{ + ChatID: "chat1", + Media: []bus.MediaAttachment{ + {URL: tmpFile}, + }, + } + _ = ch.sendMedia(context.Background(), msg) + + // Filename should be just "actual_file.txt", no directory path + if len(rt.reqs) == 0 { + t.Fatal("no requests captured") + } + capturedFilename := rt.reqs[0].Get("fields[FILE][name]") + if capturedFilename != "actual_file.txt" { + t.Errorf("filename = %q; want actual_file.txt", capturedFilename) + } +} diff --git a/internal/channels/bitrix24/send_test.go b/internal/channels/bitrix24/send_test.go index 1974c52d..11081392 100644 --- a/internal/channels/bitrix24/send_test.go +++ b/internal/channels/bitrix24/send_test.go @@ -229,6 +229,252 @@ func TestSend_EmptyContentIsNoOp(t *testing.T) { } } +// TestResolveSendOptions verifies the Metadata → sendOptions decoding that +// Send() uses to pick v1 whisper vs v2 public and to thread replyId into +// the v2 fields object. Defaults must preserve pre-refactor behaviour +// (public, no replyId) so any caller missing the keys still works. +func TestResolveSendOptions(t *testing.T) { + cases := []struct { + name string + meta map[string]string + wantVisibility string + wantReplyToMID int + }{ + { + name: "empty metadata defaults to public", + meta: nil, + wantVisibility: VisibilityPublic, + wantReplyToMID: 0, + }, + { + name: "whisper visibility", + meta: map[string]string{MetaKeyVisibility: VisibilityWhisper}, + wantVisibility: VisibilityWhisper, + wantReplyToMID: 0, + }, + { + name: "explicit public visibility", + meta: map[string]string{MetaKeyVisibility: VisibilityPublic}, + wantVisibility: VisibilityPublic, + wantReplyToMID: 0, + }, + { + name: "unknown visibility value falls back to public", + meta: map[string]string{MetaKeyVisibility: "secret"}, + wantVisibility: VisibilityPublic, + wantReplyToMID: 0, + }, + { + name: "valid numeric message_id parsed for replyToMID", + meta: map[string]string{MetaKeyMessageID: "297178"}, + wantVisibility: VisibilityPublic, + wantReplyToMID: 297178, + }, + { + name: "non-numeric message_id ignored", + meta: map[string]string{MetaKeyMessageID: "abc"}, + wantVisibility: VisibilityPublic, + wantReplyToMID: 0, + }, + { + name: "zero message_id ignored", + meta: map[string]string{MetaKeyMessageID: "0"}, + wantVisibility: VisibilityPublic, + wantReplyToMID: 0, + }, + { + name: "whisper with replyToMID still captures both", + meta: map[string]string{ + MetaKeyVisibility: VisibilityWhisper, + MetaKeyMessageID: "12345", + }, + wantVisibility: VisibilityWhisper, + wantReplyToMID: 12345, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + opts := resolveSendOptions(tc.meta) + if opts.visibility != tc.wantVisibility { + t.Errorf("visibility = %q; want %q", opts.visibility, tc.wantVisibility) + } + if opts.replyToMID != tc.wantReplyToMID { + t.Errorf("replyToMID = %d; want %d", opts.replyToMID, tc.wantReplyToMID) + } + }) + } +} + +// TestSend_BranchesOnVisibility wires Send() through the HTTP-layer stub +// (captureRT) so we can assert exactly which Bitrix REST method got +// invoked and what params were on the wire. Public visibility → v2; +// whisper visibility → v1 with SKIP_CONNECTOR=Y. The matrix also covers +// replyId propagation on v2 and its absence on v1. +func TestSend_BranchesOnVisibility(t *testing.T) { + cases := []struct { + name string + metadata map[string]string + wantPath string + wantFormChecks map[string]string // exact form-key → expected value + notWantKeys []string // form keys that must be absent + }{ + { + name: "whisper routes to v1 imbot.message.add with SKIP_CONNECTOR=Y", + metadata: map[string]string{ + MetaKeyVisibility: VisibilityWhisper, + MetaKeyMessageID: "297178", // v1 ignores replyId so must NOT appear + }, + wantPath: "/rest/imbot.message.add.json", + wantFormChecks: map[string]string{ + "BOT_ID": "1", + "DIALOG_ID": "chat4878", + "MESSAGE": "hi from bot", + "SKIP_CONNECTOR": "Y", + }, + notWantKeys: []string{ + "fields[replyId]", + "replyId", + "botId", + }, + }, + { + name: "public with replyId routes to v2 + fields[replyId]", + metadata: map[string]string{ + MetaKeyVisibility: VisibilityPublic, + MetaKeyMessageID: "297196", + }, + wantPath: "/rest/imbot.v2.Chat.Message.send.json", + wantFormChecks: map[string]string{ + "botId": "1", + "dialogId": "chat4878", + "fields[message]": "hi from bot", + "fields[replyId]": "297196", + }, + notWantKeys: []string{ + "SKIP_CONNECTOR", + "BOT_ID", + "MESSAGE", + }, + }, + { + name: "public without message_id routes to v2 without replyId", + metadata: map[string]string{MetaKeyVisibility: VisibilityPublic}, + wantPath: "/rest/imbot.v2.Chat.Message.send.json", + wantFormChecks: map[string]string{ + "botId": "1", + "dialogId": "chat4878", + "fields[message]": "hi from bot", + }, + notWantKeys: []string{ + "fields[replyId]", + "replyId", + "SKIP_CONNECTOR", + }, + }, + { + name: "missing visibility defaults to v2 public (backward-compat)", + metadata: nil, + wantPath: "/rest/imbot.v2.Chat.Message.send.json", + wantFormChecks: map[string]string{ + "botId": "1", + "dialogId": "chat4878", + "fields[message]": "hi from bot", + }, + notWantKeys: []string{ + "SKIP_CONNECTOR", + "fields[replyId]", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := &captureRT{result: `{"result":{"id":1}}`} + client := newStubClient("test.bitrix24.com", rt) + ch, _ := newFakeChannelWithClient(t, client) + ch.SetRunning(true) + + msg := bus.OutboundMessage{ + ChatID: "chat4878", + Content: "hi from bot", + Metadata: tc.metadata, + } + if err := ch.Send(context.Background(), msg); err != nil { + t.Fatalf("Send error: %v", err) + } + + if len(rt.paths) != 1 { + t.Fatalf("expected exactly 1 HTTP call, got %d (paths=%v)", len(rt.paths), rt.paths) + } + if rt.paths[0] != tc.wantPath { + t.Errorf("path = %q; want %q", rt.paths[0], tc.wantPath) + } + + form := rt.reqs[0] + for k, want := range tc.wantFormChecks { + if got := form.Get(k); got != want { + t.Errorf("form[%q] = %q; want %q (full form: %v)", k, got, want, form) + } + } + for _, k := range tc.notWantKeys { + if v := form.Get(k); v != "" { + t.Errorf("form[%q] should be absent, got %q", k, v) + } + } + }) + } +} + +// TestSend_OL_EchoPrefixPrepended verifies the openline sender-tag echo: whatever +// handle.go placed in MetaKeySenderPrefix is prepended verbatim to the outbound +// text so the Open Channel connector can route the reply back to the right +// external message. send.go is format-agnostic — handle.go decides the shape +// ("#msgId" for 3-token, "[name] #msgId" for legacy). +func TestSend_OL_EchoPrefixPrepended(t *testing.T) { + cases := []struct { + name string + prefix string + wantMsg string + }{ + { + name: "3-token echo is msgId only", + prefix: "#777888", + wantMsg: "#777888 hi from bot", + }, + { + name: "legacy echo keeps full [name] #msgId", + prefix: "[Trung Hee] #7957717404177", + wantMsg: "[Trung Hee] #7957717404177 hi from bot", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := &captureRT{result: `{"result":{"id":1}}`} + client := newStubClient("test.bitrix24.com", rt) + ch, _ := newFakeChannelWithClient(t, client) + ch.SetRunning(true) + + msg := bus.OutboundMessage{ + ChatID: "chat4878", + Content: "hi from bot", + Metadata: map[string]string{ + MetaKeyVisibility: VisibilityPublic, + MetaKeySenderPrefix: tc.prefix, + }, + } + if err := ch.Send(context.Background(), msg); err != nil { + t.Fatalf("Send error: %v", err) + } + if len(rt.reqs) != 1 { + t.Fatalf("expected exactly 1 HTTP call, got %d", len(rt.reqs)) + } + if got := rt.reqs[0].Get("fields[message]"); got != tc.wantMsg { + t.Errorf("fields[message] = %q; want %q", got, tc.wantMsg) + } + }) + } +} + // TestBuildAddressMention covers the address-user resolver that prepends the // `[USER=][/USER]` BBCode to outbound replies in group chats. The format // is intentionally empty-named so Bitrix renders the user's current display @@ -293,6 +539,14 @@ func TestBuildAddressMention(t *testing.T) { botID: 0, want: "[USER=940][/USER]", }, + { + // Synthetic openline per-participant id is not a real Bitrix user — + // emitting [USER=openlines:...] would render as literal garbage. + name: "synthetic_openline_id_suppressed", + meta: map[string]string{"bitrix_address_user_id": "openlines:tamgiac:chat4878:111222"}, + botID: 940, + want: "", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/channels/bitrix24/send_v1.go b/internal/channels/bitrix24/send_v1.go new file mode 100644 index 00000000..0dd5be1c --- /dev/null +++ b/internal/channels/bitrix24/send_v1.go @@ -0,0 +1,47 @@ +package bitrix24 + +import ( + "context" +) + +// sendChunkV1Whisper posts a single chunk via imbot.message.add (v1) with +// SKIP_CONNECTOR=Y. This is the legacy chat-bot API; v1 and v2 run in +// parallel on every portal (per the official migration doc), so a bot +// registered through imbot.register can still invoke imbot.message.add +// without re-registration. +// +// Why v1 here instead of v2: imbot.v2.Chat.Message.send (used by the +// public path) does NOT expose any equivalent of SKIP_CONNECTOR. v1 is +// the only documented way to send a message that stays inside the Open +// Channel session and is NOT forwarded to the external connector +// (Zalo, FB, etc.) — i.e. the "whisper / internal-only" use case. +// +// Params shape (flat UPPER_SNAKE_CASE per v1 convention): +// +// { +// "BOT_ID": 1058, +// "DIALOG_ID": "chat4878", +// "MESSAGE": "", +// "SKIP_CONNECTOR": "Y" +// } +// +// See: https://apidocs.bitrix24.com/api-reference/chat-bots/messages/imbot-message-add.html +// +// Limitations vs v2: +// - No replyId equivalent; whisper replies don't render a "↩ tin gốc" +// link. Acceptable trade-off since whisper visibility is staff-only. +// +// Rate-limit retry semantics are delegated to callWithRateLimitRetry so +// they stay identical to the v2 public path. +func (c *Channel) sendChunkV1Whisper(ctx context.Context, chatID, chunk string) error { + botID := c.BotID() + + params := map[string]any{ + "BOT_ID": botID, + "DIALOG_ID": chatID, + "MESSAGE": chunk, + "SKIP_CONNECTOR": "Y", + } + + return c.callWithRateLimitRetry(ctx, "imbot.message.add", params, chatID, botID) +} diff --git a/internal/channels/bitrix24/send_v2.go b/internal/channels/bitrix24/send_v2.go new file mode 100644 index 00000000..5562759f --- /dev/null +++ b/internal/channels/bitrix24/send_v2.go @@ -0,0 +1,47 @@ +package bitrix24 + +import ( + "context" +) + +// sendChunkV2Public posts a single chunk via imbot.v2.Chat.Message.send. +// This is the modern (v2) chat-bot API used for PUBLIC replies in Open +// Channel sessions — Bitrix24 forwards them to the configured external +// connector (Zalo, FB, etc.). If replyToMID > 0, fields.replyId links the +// bot's reply to the inbound message in the Bitrix UI ("↩ tin gốc"). +// +// Params shape verified live against tamgiac.bitrix24.com: +// +// { +// "botId": 1058, +// "dialogId": "chat4878", +// "fields": { +// "message": "", +// "replyId": 297178 // optional, only when > 0 +// } +// } +// +// See: https://apidocs.bitrix24.ru/api-reference/chat-bots/chat-bots-v2/imbot.v2/messages/chat-message-send.html +// +// Rate-limit retry semantics are delegated to callWithRateLimitRetry so +// they stay identical to the v1 whisper path. +func (c *Channel) sendChunkV2Public(ctx context.Context, chatID, chunk string, replyToMID int) error { + botID := c.BotID() + + fields := map[string]any{ + "message": chunk, + } + // replyId is integer in v2 schema. Only set when caller passed a + // valid MessageID — Atoi check already happened in resolveSendOptions. + if replyToMID > 0 { + fields["replyId"] = replyToMID + } + + params := map[string]any{ + "botId": botID, + "dialogId": chatID, + "fields": fields, + } + + return c.callWithRateLimitRetry(ctx, "imbot.v2.Chat.Message.send", params, chatID, botID) +} diff --git a/internal/channels/bitrix24/sender_prefix.go b/internal/channels/bitrix24/sender_prefix.go new file mode 100644 index 00000000..e49804b7 --- /dev/null +++ b/internal/channels/bitrix24/sender_prefix.go @@ -0,0 +1,174 @@ +package bitrix24 + +import ( + "regexp" + "strings" +) + +// Openline (Bitrix Open Channel) relays an external connector user's message +// into the operator chat with a sender tag at the very start of the text. The +// connector has shipped a few layouts over time; we accept them all and +// normalise the id-bearing ones to "[name] #id" and the bare one to "[name]": +// +// [Thân Công Huy #1623524631958449211]: — id inside the brackets, colon +// [Thân Công Huy #1623524631958449211] — id inside the brackets, no colon +// [Thân Công Huy] #1623524631958449211: — id after the brackets, colon +// [Thân Công Huy] #1623524631958449211 — id after the brackets, no colon +// [Thân Công Huy] — name only (no id) +// +// The trailing number is the message id (msgId) the connector uses to quote / +// route a reply back to the right external message, so it must survive into the +// echo. (Empirically it is a per-message id, not a per-user id — the same +// external person produces a different number on each message.) +// The trailing ":" is optional because the connector dropped it in its latest +// format. The bare name-only layout is far more generic, so it is only +// recognised for Open Channel sessions (see allowNameOnly). +// +// A newer connector build prepends a SECOND number — the external person's +// stable uid — ahead of the msgId: "[Name] #uid #msgId ". That richer +// 3-token layout is parsed by parseOpenlineSenderTag (below), which keeps the +// uid and msgId separate so callers can scope per-participant identity. The +// single-number layouts above remain the legacy/back-compat path. +// +// Plain Bitrix24 group chats never carry these tags — they use +// "[USER=]Name[/USER]" BBCode mentions, which handle.go converts to +// "@Name (ID:)". + +// openlineSenderPrefixPatterns match the id-bearing sender tag anchored at the +// start of the message. Group 1 = display name, group 2 = connector user id +// (digits only). The colon after the tag is optional (`:?`) — newer connector +// builds omit it. +var openlineSenderPrefixPatterns = []*regexp.Regexp{ + regexp.MustCompile(`^\[(.+?)\s+#(\d+)\]:?\s*`), // [name #id]: or [name #id] + regexp.MustCompile(`^\[(.+?)\]\s+#(\d+):?\s*`), // [name] #id: or [name] #id +} + +// nameOnlySenderPrefixPattern matches the shortest "[name] " tag — a display +// name in brackets followed by whitespace, with no connector id. Group 1 = +// display name. Generic on purpose, so callers gate it behind allowNameOnly to +// keep it to Open Channel sessions only. +var nameOnlySenderPrefixPattern = regexp.MustCompile(`^\[([^\]]+?)\]\s+`) + +// extractOpenlineSenderPrefix detects the openline sender tag at the start of +// text. On a match it returns the canonical prefix ("[name] #id" for id-bearing +// tags, "[name]" for the name-only tag) plus the message body with the tag (and +// following whitespace) removed. On no match it returns ("", text) unchanged so +// callers can treat it as a cheap no-op. +// +// allowNameOnly enables the generic name-only "[name] " layout; pass it only +// for Open Channel sessions. The id-bearing layouts are always tried first (and +// take precedence) because their numeric id makes them unambiguous — this also +// stops the name-only pattern from clipping just "[name] " off a "[name] #id" +// tag and dropping the id. +func extractOpenlineSenderPrefix(text string, allowNameOnly bool) (prefix, rest string) { + for _, re := range openlineSenderPrefixPatterns { + m := re.FindStringSubmatch(text) + if m == nil { + continue + } + name := strings.TrimSpace(m[1]) + id := m[2] + if name == "" || id == "" { + continue + } + return "[" + name + "] #" + id, text[len(m[0]):] + } + + if allowNameOnly { + if m := nameOnlySenderPrefixPattern.FindStringSubmatch(text); m != nil { + if name := strings.TrimSpace(m[1]); name != "" { + return "[" + name + "]", text[len(m[0]):] + } + } + } + + return "", text +} + +// OpenlineSenderTagFormat classifies which connector sender-tag layout was +// found at the start of an Open Channel message. +type OpenlineSenderTagFormat int + +const ( + // TagFormatNone means no recognised sender tag was present. + TagFormatNone OpenlineSenderTagFormat = iota + // TagFormatNameOnly is "[Name] " — a display name with no number. + TagFormatNameOnly + // TagFormatLegacy is "[Name] #msgId " — a single number, which is the + // connector message id (NOT a user id). This is the format production + // connectors ship today. + TagFormatLegacy + // TagFormatThreeToken is "[Name] #uid #msgId " — two numbers, where the + // first is the external person's stable uid and the second is the message + // id. Only the newer connector build emits this. + TagFormatThreeToken +) + +// OpenlineSenderTag is the structured result of parsing a connector sender tag. +// UID is populated only for TagFormatThreeToken; MsgID is populated for both the +// legacy and three-token layouts. Rest is the message body with the tag removed. +type OpenlineSenderTag struct { + Name string + UID string + MsgID string + Format OpenlineSenderTagFormat + Rest string +} + +// threeTokenPattern matches "[Name] #uid #msgId" — two consecutive #digits +// tokens after the bracketed name. Group 1 = name, group 2 = uid, group 3 = +// msgId. The trailing ":" is optional to match the legacy patterns' tolerance. +var threeTokenPattern = regexp.MustCompile(`^\[(.+?)\]\s+#(\d+)\s+#(\d+):?\s*`) + +// parseOpenlineSenderTag classifies the connector sender tag at the start of an +// Open Channel message and returns its parts separated. Precedence is +// most-specific first: three-token (uid + msgId) → id-bearing legacy (msgId +// only) → name-only → none. On no match it returns {Format: TagFormatNone, +// Rest: text} so callers can treat it as a cheap no-op. +// +// Callers must gate identity derivation on the message source (IS_CONNECTOR=Y) +// themselves — this function only parses the shape; an operator who types a +// look-alike tag would parse identically. Only call it for Open Channel +// sessions; ordinary group chats never carry these tags. +func parseOpenlineSenderTag(text string) OpenlineSenderTag { + // Three-token: "[Name] #uid #msgId" — try first so the second number isn't + // mistaken for the body by the single-number legacy patterns. + if m := threeTokenPattern.FindStringSubmatch(text); m != nil { + if name := strings.TrimSpace(m[1]); name != "" && m[2] != "" && m[3] != "" { + return OpenlineSenderTag{ + Name: name, + UID: m[2], + MsgID: m[3], + Format: TagFormatThreeToken, + Rest: strings.TrimSpace(text[len(m[0]):]), + } + } + } + + // Legacy id-bearing: "[Name] #msgId" or "[Name #msgId]" — one number = msgId. + for _, re := range openlineSenderPrefixPatterns { + if m := re.FindStringSubmatch(text); m != nil { + if name := strings.TrimSpace(m[1]); name != "" && m[2] != "" { + return OpenlineSenderTag{ + Name: name, + MsgID: m[2], + Format: TagFormatLegacy, + Rest: strings.TrimSpace(text[len(m[0]):]), + } + } + } + } + + // Name-only: "[Name] ". + if m := nameOnlySenderPrefixPattern.FindStringSubmatch(text); m != nil { + if name := strings.TrimSpace(m[1]); name != "" { + return OpenlineSenderTag{ + Name: name, + Format: TagFormatNameOnly, + Rest: strings.TrimSpace(text[len(m[0]):]), + } + } + } + + return OpenlineSenderTag{Format: TagFormatNone, Rest: text} +} diff --git a/internal/channels/bitrix24/sender_prefix_test.go b/internal/channels/bitrix24/sender_prefix_test.go new file mode 100644 index 00000000..49975aeb --- /dev/null +++ b/internal/channels/bitrix24/sender_prefix_test.go @@ -0,0 +1,209 @@ +package bitrix24 + +import "testing" + +func TestExtractOpenlineSenderPrefix(t *testing.T) { + cases := []struct { + name string + in string + allowNameOnly bool + wantPrefix string + wantRest string + }{ + { + name: "id after brackets, no colon (current connector format)", + in: "[Thân Công Huy] #7941945550666 @Ngọc Thúy kiểm tra lịch", + wantPrefix: "[Thân Công Huy] #7941945550666", + wantRest: "@Ngọc Thúy kiểm tra lịch", + }, + { + name: "id after brackets, with colon (legacy)", + in: "[Thân Công Huy] #1623524631958449211: alo", + wantPrefix: "[Thân Công Huy] #1623524631958449211", + wantRest: "alo", + }, + { + name: "id inside brackets, with colon (legacy)", + in: "[Thân Công Huy #1623524631958449211]: alo", + wantPrefix: "[Thân Công Huy] #1623524631958449211", + wantRest: "alo", + }, + { + name: "id inside brackets, no colon", + in: "[Thân Công Huy #1623524631958449211] alo", + wantPrefix: "[Thân Công Huy] #1623524631958449211", + wantRest: "alo", + }, + { + name: "id no colon, no name-only flag — still matches", + in: "[Minh Zip] #42 chào", + wantPrefix: "[Minh Zip] #42", + wantRest: "chào", + }, + { + name: "name only — openline (allowNameOnly)", + in: "[Minh Zip] móa user hỏi hóc xương cá thế nhỉ", + allowNameOnly: true, + wantPrefix: "[Minh Zip]", + wantRest: "móa user hỏi hóc xương cá thế nhỉ", + }, + { + // id format wins over name-only, so the id is never dropped. + name: "id present — not clipped to name-only", + in: "[Thân Công Huy] #7941945550666 hello", + allowNameOnly: true, + wantPrefix: "[Thân Công Huy] #7941945550666", + wantRest: "hello", + }, + { + // NOT an Open Channel → bare name-only ignored so ordinary group + // chat text starting with "[x] …" is left untouched. + name: "name only — ignored when not openline", + in: "[Minh Zip] móa user hỏi", + allowNameOnly: false, + wantPrefix: "", + wantRest: "[Minh Zip] móa user hỏi", + }, + { + name: "plain message — no tag", + in: "chào shop, cho hỏi giá", + allowNameOnly: true, + wantPrefix: "", + wantRest: "chào shop, cho hỏi giá", + }, + { + name: "readable mention — not an openline tag", + in: "@Ngọc Thúy (ID:62) giúp em", + allowNameOnly: true, + wantPrefix: "", + wantRest: "@Ngọc Thúy (ID:62) giúp em", + }, + // Characterization: current behavior locked before the 3-token refactor. + // The single number after the brackets is a per-message connector id; it + // must keep flowing through extractOpenlineSenderPrefix unchanged so the + // legacy echo path is untouched. + { + name: "legacy 1-number after brackets (msgId) — characterization", + in: "[Trung Hee] #7957717404177 alo bot", + wantPrefix: "[Trung Hee] #7957717404177", + wantRest: "alo bot", + }, + { + name: "legacy 1-number inside brackets — characterization", + in: "[Trung Hee #7957717404177]: alo", + wantPrefix: "[Trung Hee] #7957717404177", + wantRest: "alo", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + gotPrefix, gotRest := extractOpenlineSenderPrefix(tc.in, tc.allowNameOnly) + if gotPrefix != tc.wantPrefix { + t.Errorf("prefix = %q, want %q", gotPrefix, tc.wantPrefix) + } + if gotRest != tc.wantRest { + t.Errorf("rest = %q, want %q", gotRest, tc.wantRest) + } + }) + } +} + +// TestParseOpenlineSenderTag pins the structured 3-token parser. It distinguishes +// the new "[Name] #uid #msgId" layout (two numbers) from the legacy single-number +// msgId layout, the bare name-only layout, and no tag at all. The parser only +// classifies shape — the IS_CONNECTOR forged-tag gate lives in handle.go. +func TestParseOpenlineSenderTag(t *testing.T) { + cases := []struct { + name string + in string + wantName string + wantUID string + wantMsgID string + wantFormat OpenlineSenderTagFormat + wantRest string + }{ + { + name: "3-token standard", + in: "[Trung Hee] #111222 #777888 alo bot", + wantName: "Trung Hee", + wantUID: "111222", + wantMsgID: "777888", + wantFormat: TagFormatThreeToken, + wantRest: "alo bot", + }, + { + name: "3-token with trailing colon", + in: "[Thân Công Huy] #9999 #8888: kiểm tra", + wantName: "Thân Công Huy", + wantUID: "9999", + wantMsgID: "8888", + wantFormat: TagFormatThreeToken, + wantRest: "kiểm tra", + }, + { + // One number → legacy: uid empty, msgId = the only number. + name: "legacy 1-number → TagFormatLegacy", + in: "[Trung Hee] #7957717404177 alo", + wantName: "Trung Hee", + wantUID: "", + wantMsgID: "7957717404177", + wantFormat: TagFormatLegacy, + wantRest: "alo", + }, + { + name: "legacy 1-number inside brackets → TagFormatLegacy", + in: "[Trung Hee #7957717404177]: alo", + wantName: "Trung Hee", + wantUID: "", + wantMsgID: "7957717404177", + wantFormat: TagFormatLegacy, + wantRest: "alo", + }, + { + name: "name only → TagFormatNameOnly", + in: "[Minh Zip] hỏi giá", + wantName: "Minh Zip", + wantFormat: TagFormatNameOnly, + wantRest: "hỏi giá", + }, + { + name: "no tag → TagFormatNone", + in: "chào shop", + wantFormat: TagFormatNone, + wantRest: "chào shop", + }, + { + // The shape parses regardless of source — handle.go's IS_CONNECTOR + // gate is what stops an operator forged tag from minting identity. + name: "forged-looking 2-number shape still parses (gate is in handle.go)", + in: "[Victim] #111222 #777888 do nó bị văng", + wantName: "Victim", + wantUID: "111222", + wantMsgID: "777888", + wantFormat: TagFormatThreeToken, + wantRest: "do nó bị văng", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := parseOpenlineSenderTag(tc.in) + if got.Name != tc.wantName { + t.Errorf("Name = %q, want %q", got.Name, tc.wantName) + } + if got.UID != tc.wantUID { + t.Errorf("UID = %q, want %q", got.UID, tc.wantUID) + } + if got.MsgID != tc.wantMsgID { + t.Errorf("MsgID = %q, want %q", got.MsgID, tc.wantMsgID) + } + if got.Format != tc.wantFormat { + t.Errorf("Format = %d, want %d", got.Format, tc.wantFormat) + } + if got.Rest != tc.wantRest { + t.Errorf("Rest = %q, want %q", got.Rest, tc.wantRest) + } + }) + } +} diff --git a/internal/channels/bitrix24/stub_client_test.go b/internal/channels/bitrix24/stub_client_test.go new file mode 100644 index 00000000..7efdec6d --- /dev/null +++ b/internal/channels/bitrix24/stub_client_test.go @@ -0,0 +1,117 @@ +package bitrix24 + +import ( + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// captureRT is a RoundTripper that records requests and returns canned JSON responses. +// It sidesteps HTTPS/TLS and host issues by intercepting at the transport layer, +// making it ideal for unit tests that need to verify request parameters without +// running a real server or setting up complex Portal bindings. +type captureRT struct { + reqs []url.Values // parsed form bodies, in order + paths []string // r.URL.Path in order + result string // JSON to return as the response body; default: {"result":{"fileId":1}} + status int +} + +// RoundTrip implements http.RoundTripper, capturing the request and returning a canned response. +func (rt *captureRT) RoundTrip(r *http.Request) (*http.Response, error) { + // Capture the request body + if r.Body != nil { + b, _ := io.ReadAll(r.Body) + form, _ := url.ParseQuery(string(b)) + rt.reqs = append(rt.reqs, form) + // Restore body for any potential re-read (though typically not used) + r.Body = io.NopCloser(strings.NewReader(string(b))) + } else { + rt.reqs = append(rt.reqs, url.Values{}) + } + + // Capture the path + rt.paths = append(rt.paths, r.URL.Path) + + // Prepare response + status := rt.status + if status == 0 { + status = http.StatusOK + } + body := rt.result + if body == "" { + body = `{"result":{"fileId":1}}` + } + + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: http.Header{ + "Content-Type": {"application/json"}, + }, + }, nil +} + +// newStubClient creates a Client with a fake Portal so AccessToken() returns +// without network. The Portal is bound with an in-memory valid token that will +// pass the time.Until(expiry) > expiryBuffer check. +func newStubClient(domain string, rt http.RoundTripper) *Client { + httpClient := &http.Client{Transport: rt} + c := NewClient(domain, httpClient) + + // Manually set a Portal with a valid, non-expired token. + // This mimics what Portal.bindClient() would do in production. + c.SetPortal(&Portal{ + state: store.BitrixPortalState{ + AccessToken: "test-token", + RefreshToken: "test-refresh", + ExpiresAt: time.Now().Add(time.Hour), // Valid for 1 hour + }, + }) + + return c +} + +// captureRTPartialFail is a RoundTripper that succeeds on first request and fails on second. +// Used to test partial upload failure scenarios. +type captureRTPartialFail struct { + reqs []url.Values + paths []string + calls int +} + +// RoundTrip implements http.RoundTripper, returning success on first call and error on second. +func (rt *captureRTPartialFail) RoundTrip(r *http.Request) (*http.Response, error) { + rt.calls++ + + // Capture request + if r.Body != nil { + b, _ := io.ReadAll(r.Body) + form, _ := url.ParseQuery(string(b)) + rt.reqs = append(rt.reqs, form) + r.Body = io.NopCloser(strings.NewReader(string(b))) + } else { + rt.reqs = append(rt.reqs, url.Values{}) + } + rt.paths = append(rt.paths, r.URL.Path) + + // Return different responses based on call count + var body string + if rt.calls == 1 { + body = `{"result":{"fileId":1}}` + } else { + body = `{"error":"UPLOAD_FAILED"}` + } + + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(body)), + Header: http.Header{ + "Content-Type": {"application/json"}, + }, + }, nil +} diff --git a/internal/channels/channel.go b/internal/channels/channel.go index 6679b684..1216bcc7 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -619,6 +619,24 @@ func (c *BaseChannel) ValidatePolicy(dmPolicy, groupPolicy string) { // This is the standard way for channels to forward received messages. // peerKind should be "direct" or "group" (see sessions.PeerDirect, sessions.PeerGroup). func (c *BaseChannel) HandleMessage(senderID, chatID, content string, media []string, metadata map[string]string, peerKind string) { + // Convert string paths to MediaFile (legacy path-only callers). + // Use filepath.Base(p) as filename so persistMedia's sanitizer gets a + // meaningful stem instead of falling back to UUID. MimeType is left empty — + // persistMedia infers it from the file extension for these callers. + var mediaFiles []bus.MediaFile + for _, p := range media { + mediaFiles = append(mediaFiles, bus.MediaFile{Path: p, Filename: filepath.Base(p)}) + } + c.HandleMessageMedia(senderID, chatID, content, mediaFiles, metadata, peerKind) +} + +// HandleMessageMedia is the richer sibling of HandleMessage: it accepts +// pre-built bus.MediaFile values so a channel can preserve the original MIME +// type and filename. This matters because the agent pipeline routes media by +// MIME (image vs document vs audio vs video) — the path-only HandleMessage +// loses that information. Channels that already know the content type at +// download time (e.g. Bitrix24 file events) should call this directly. +func (c *BaseChannel) HandleMessageMedia(senderID, chatID, content string, media []bus.MediaFile, metadata map[string]string, peerKind string) { // For DMs, enforce the allowlist as a safety net. // For group messages, skip this check — group access is already enforced // by the channel-specific group policy (checkGroupPolicy / CheckPolicy). @@ -635,20 +653,12 @@ func (c *BaseChannel) HandleMessage(senderID, chatID, content string, media []st userID = senderID[:idx] } - // Convert string paths to MediaFile (legacy path-only callers). - // Use filepath.Base(p) as filename so persistMedia's sanitizer gets a - // meaningful stem instead of falling back to UUID. - var mediaFiles []bus.MediaFile - for _, p := range media { - mediaFiles = append(mediaFiles, bus.MediaFile{Path: p, Filename: filepath.Base(p)}) - } - msg := bus.InboundMessage{ Channel: c.name, SenderID: senderID, ChatID: chatID, Content: content, - Media: mediaFiles, + Media: media, PeerKind: peerKind, UserID: userID, Metadata: metadata, diff --git a/internal/security/ssrf_redirect.go b/internal/security/ssrf_redirect.go new file mode 100644 index 00000000..51170bb9 --- /dev/null +++ b/internal/security/ssrf_redirect.go @@ -0,0 +1,66 @@ +package security + +import ( + "fmt" + "log/slog" + "net" + "net/http" + "syscall" + "time" +) + +// safeDialControl is a net.Dialer.Control hook that rejects a connection whose +// resolved destination IP falls in a blocked range. Control runs AFTER DNS +// resolution with the concrete ip:port that is about to be dialed, so it +// validates the *actual* connection target — closing both redirect-to-internal +// and DNS-rebinding gaps (the IP it checks is the IP that gets connected, with no +// TOCTOU window). allowLoopbackForTest bypasses the check in test code only. +func safeDialControl(_, address string, _ syscall.RawConn) error { + host, _, err := net.SplitHostPort(address) + if err != nil { + return fmt.Errorf("ssrf: split dial address %q: %w", address, err) + } + ip := net.ParseIP(host) + if ip == nil { + return fmt.Errorf("ssrf: dial address %q is not an IP", host) + } + if !allowLoopbackForTest.Load() && isBlocked(ip) { + slog.Warn("security.ssrf_block", "reason", "blocked_dial_ip", "ip", ip.String()) + return fmt.Errorf("ssrf: dial IP %s is in a blocked range", ip) + } + return nil +} + +// NewRedirectFollowingSafeClient returns an *http.Client that FOLLOWS up to +// maxRedirects redirects while validating the resolved destination IP of EVERY +// hop at dial time (via net.Dialer.Control), and refusing any non-http(s) +// redirect target. +// +// Unlike NewSafeClient — which refuses redirects outright and pins a single +// pre-validated IP — this client suits fetching an authenticated media URL that +// legitimately 3xx-redirects to a public CDN (e.g. Bitrix imbot.v2.File.download). +// Checking the resolved IP at the dial boundary (not the hostname string) means a +// redirect whose host resolves into a loopback, link-local (cloud-metadata), +// private, multicast, or unspecified range is refused even when the hostname +// looks public — and the check happens on the IP actually connected, so DNS +// rebinding cannot swap a public IP for a private one after a string check. +// +// The returned client is safe to share across goroutines. +func NewRedirectFollowingSafeClient(timeout time.Duration, maxRedirects int) *http.Client { + dialer := &net.Dialer{Timeout: timeout, Control: safeDialControl} + return &http.Client{ + Timeout: timeout, + Transport: &http.Transport{DialContext: dialer.DialContext}, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= maxRedirects { + return fmt.Errorf("ssrf: too many redirects (%d)", len(via)) + } + switch req.URL.Scheme { + case "http", "https": + return nil + default: + return fmt.Errorf("ssrf: redirect to non-http(s) scheme %q", req.URL.Scheme) + } + }, + } +} diff --git a/internal/security/ssrf_redirect_test.go b/internal/security/ssrf_redirect_test.go new file mode 100644 index 00000000..97f15390 --- /dev/null +++ b/internal/security/ssrf_redirect_test.go @@ -0,0 +1,118 @@ +package security + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// TestSafeDialControl is the core regression for the redirect-SSRF fix: the dial +// guard rejects by RESOLVED IP, not hostname string. Control always receives a +// concrete ip:port, so these cover the "hostname → private IP" class deterministically +// without any real DNS. +func TestSafeDialControl(t *testing.T) { + cases := []struct { + name string + addr string + blocked bool + }{ + {"loopback v4", "127.0.0.1:443", true}, + {"loopback v6", "[::1]:443", true}, + {"cloud metadata", "169.254.169.254:80", true}, + {"link-local v6", "[fe80::1]:80", true}, + {"private 10", "10.1.2.3:80", true}, + {"private 172", "172.16.5.5:80", true}, + {"private 192", "192.168.1.1:80", true}, + {"ula v6", "[fc00::1]:80", true}, + {"multicast", "224.0.0.1:80", true}, + {"unspecified", "0.0.0.0:80", true}, + {"public v4", "8.8.8.8:443", false}, + {"public v6", "[2606:4700:4700::1111]:443", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := safeDialControl("tcp", tc.addr, nil) + switch { + case tc.blocked && err == nil: + t.Errorf("safeDialControl(%q) = nil, want blocked", tc.addr) + case !tc.blocked && err != nil: + t.Errorf("safeDialControl(%q) = %v, want allowed", tc.addr, err) + } + }) + } +} + +func TestSafeDialControl_Malformed(t *testing.T) { + if err := safeDialControl("tcp", "not-an-addr", nil); err == nil { + t.Error("expected error for malformed dial address") + } + // Control only ever receives a resolved IP; a non-IP host is a hard failure. + if err := safeDialControl("tcp", "example.com:80", nil); err == nil { + t.Error("expected error for non-IP dial host") + } +} + +func TestSafeDialControl_TestBypass(t *testing.T) { + SetAllowLoopbackForTest(true) + defer SetAllowLoopbackForTest(false) + if err := safeDialControl("tcp", "127.0.0.1:443", nil); err != nil { + t.Errorf("with test bypass, loopback should be allowed: %v", err) + } +} + +// TestRedirectFollowingSafeClient_BlocksLoopbackDial proves the wired client +// refuses to connect to a loopback target at dial time (default, no bypass). +func TestRedirectFollowingSafeClient_BlocksLoopbackDial(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := NewRedirectFollowingSafeClient(5*time.Second, 5) + _, err := c.Get(srv.URL) + if err == nil { + t.Fatal("expected loopback dial to be blocked by the SSRF guard") + } + if !strings.Contains(err.Error(), "blocked range") { + t.Errorf("error %q does not mention the SSRF block", err) + } +} + +// TestRedirectFollowingSafeClient_AllowsWithBypass proves the happy path: with the +// test bypass on, the client connects and follows through to the server. +func TestRedirectFollowingSafeClient_AllowsWithBypass(t *testing.T) { + SetAllowLoopbackForTest(true) + defer SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := NewRedirectFollowingSafeClient(5*time.Second, 5) + resp, err := c.Get(srv.URL) + if err != nil { + t.Fatalf("with test bypass, dial should succeed: %v", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d, want 200", resp.StatusCode) + } +} + +func TestRedirectFollowingSafeClient_CheckRedirect(t *testing.T) { + c := NewRedirectFollowingSafeClient(5*time.Second, 2) + mk := func(u string) *http.Request { r, _ := http.NewRequest(http.MethodGet, u, nil); return r } + + httpsReq := mk("https://cdn.example/x") + if err := c.CheckRedirect(httpsReq, []*http.Request{mk("https://a/"), mk("https://b/")}); err == nil { + t.Error("expected error when redirect count reaches the cap") + } + if err := c.CheckRedirect(mk("file:///etc/passwd"), nil); err == nil { + t.Error("expected error for non-http(s) redirect scheme") + } + if err := c.CheckRedirect(httpsReq, nil); err != nil { + t.Errorf("https redirect within cap should be allowed: %v", err) + } +} diff --git a/plan/goclaw-mcp-integration.md b/plan/goclaw-mcp-integration.md index a54734ff..81ab8d8d 100644 --- a/plan/goclaw-mcp-integration.md +++ b/plan/goclaw-mcp-integration.md @@ -2,10 +2,10 @@ > Kế hoạch tích hợp MCP `mcp-bx-syn` với GoClaw chatbot để enforce per-user ACL khi user chat với bot Bitrix24. > -> **Status**: ✅ Both sides implemented. Path B (access_token as auth anchor) shipped end-to-end. Remaining work is operational (backfill, marketplace rollout, Phase E shared-credential support for Open Channel). +> **Status**: ✅ Both sides implemented. "Bitrix24 OAuth → existing `mcp_user_credentials` bridge" (auto-onboard via access_token as auth anchor — Bitrix-specific glue, not a generic MCP architecture pattern) shipped end-to-end. Remaining work is operational (backfill, marketplace rollout, Phase E shared-credential support for Open Channel). > > **Owner**: dangt -> **Last updated**: 2026-04-23 (rev5: Path B shipped — ADMIN_TOKEN removed, mapping table dropped, notify + rate limit + audit log added) +> **Last updated**: 2026-04-23 (rev5: mcp_user_credentials bridge shipped — ADMIN_TOKEN removed, mapping table dropped, notify + rate limit + audit log added) --- @@ -14,7 +14,7 @@ **Mục tiêu**: Mỗi user trong Bitrix chat với bot GoClaw → MCP `mcp-bx-syn` gọi Bitrix REST với token của **chính user đó** (enforce ACL tự nhiên). Triển khai an toàn ở quy mô marketplace: mỗi portal cài app độc lập, không có shared secret giữa MCP và GoClaw. **Phạm vi (đã triển khai)**: -- Endpoint `POST /api/auto-onboard` trên MCP (Path B — xác thực bằng Bitrix `access_token` thay vì `ADMIN_TOKEN`) +- Endpoint `POST /api/auto-onboard` trên MCP (mcp_user_credentials bridge — xác thực bằng Bitrix `access_token` thay vì `ADMIN_TOKEN`) - Lazy provisioning hook trong custom Bitrix24 channel của GoClaw - Persist per-user OAuth tokens vào MCPUserCredentials để MCP proxy gọi REST API theo user - Rate limit + audit log trên endpoint @@ -35,7 +35,7 @@ ## 2. Quyết định đã chốt (Rev5) -- ✅ **Path B**: MCP xác thực mỗi call `/api/auto-onboard` bằng cách gọi Bitrix `profile` với `access_token` do caller supply, so khớp `profile.ID` với `bitrix_user_id`. Không cần `ADMIN_TOKEN` shared giữa GoClaw và MCP. +- ✅ **mcp_user_credentials bridge**: MCP xác thực mỗi call `/api/auto-onboard` bằng cách gọi Bitrix `profile` với `access_token` do caller supply, so khớp `profile.ID` với `bitrix_user_id`. Không cần `ADMIN_TOKEN` shared giữa GoClaw và MCP. - Đổi so với Rev4 (dùng `ADMIN_TOKEN` Bearer) vì không scale cho marketplace: mỗi portal chạy GoClaw riêng không thể share 1 secret với MCP worker. - ✅ **Reject 404 `tenant_not_installed`** nếu portal chưa cài MCP app. - ✅ **Idempotent theo `(tenant.domain, bitrix_user_id)`** — lần 2 refresh tokens, trả cùng USR_. @@ -115,7 +115,7 @@ Agent gọi MCP tool → Manager.resolveServerCredentials() inject | MCP `profile` (không `user.get`) vì không yêu cầu `user` scope | ✅ | `src/auth/bitrix-user-verify.ts` | | `ensureFreshToken` re-verify hourly + dismiss khi fail | ✅ | `src/auth/token-manager.ts` | | GoClaw KHÔNG còn phụ thuộc ADMIN_TOKEN | ✅ | commit `07b48ef0` (goclaw-deploy/dev) | -| GoClaw channel đã wire Path B từ commit phase C | ✅ | commit `ea09c1ba` (goclaw-deploy/dev) | +| GoClaw channel đã wire mcp_user_credentials bridge từ commit phase C | ✅ | commit `ea09c1ba` (goclaw-deploy/dev) | --- @@ -123,15 +123,15 @@ Agent gọi MCP tool → Manager.resolveServerCredentials() inject ### 4.1 MCP side — schema (không migration mới) -Schema gốc đã đủ. Các bảng Path B dùng: +Schema gốc đã đủ. Các bảng mcp_user_credentials bridge dùng: | Bảng | Cột liên quan | Dùng cho | |---|---|---| | `tenants` | `domain UNIQUE` | `findTenantByDomain` — 404 gate | | `users` | `tenant_id`, `bitrix_user_id TEXT`, `UNIQUE(tenant_id, bitrix_user_id)`, `access_token`, `refresh_token`, `token_expires_at`, `token_version` | Upsert theo (tenant, bitrix_user_id); lưu OAuth tokens để proxy Bitrix REST | -| `users` (Phase 04 columns, reuse) | `user_status` (`active`/`dismissed`), `last_verified_at` (unix seconds) | Đã có từ Phase 04; Path B reuse cho `ensureFreshToken` re-verify + dismiss flow | +| `users` (Phase 04 columns, reuse) | `user_status` (`active`/`dismissed`), `last_verified_at` (unix seconds) | Đã có từ Phase 04; mcp_user_credentials bridge reuse cho `ensureFreshToken` re-verify + dismiss flow | | `api_keys` | `user_id`, `key`, `label`, `active` | Mint USR_ label `"goclaw-bot"`; `deactivateUserApiKeys` set `active=0` khi dismiss | -| `auto_onboard_audit` (mới — Path B) | `id`, `domain`, `bitrix_user_id`, `event`, `actor`, `metadata`, `created_at` | Audit trail cho `/api/auto-onboard` — mọi call (success + fail) ghi 1 row. Event taxonomy: `success`/`rate_limited`/`invalid_bitrix_user`/`bitrix_unreachable`/`tenant_not_installed`/`bad_request` | +| `auto_onboard_audit` (mới — mcp_user_credentials bridge) | `id`, `domain`, `bitrix_user_id`, `event`, `actor`, `metadata`, `created_at` | Audit trail cho `/api/auto-onboard` — mọi call (success + fail) ghi 1 row. Event taxonomy: `success`/`rate_limited`/`invalid_bitrix_user`/`bitrix_unreachable`/`tenant_not_installed`/`bad_request` | ### 4.2 GoClaw side — schema (KHÔNG migration mới) ✅ @@ -148,7 +148,7 @@ Schema gốc đã đủ. Các bảng Path B dùng: ## 5. MCP side — `/api/auto-onboard` endpoint ✅ IMPLEMENTED -### 5.1 HTTP contract (rev5 — Path B) +### 5.1 HTTP contract (rev5 — mcp_user_credentials bridge) **Route**: `POST /api/auto-onboard` @@ -223,7 +223,7 @@ Content-Type: application/json ### 5.3 Re-verify defence-in-depth (`src/auth/token-manager.ts`) -Path B xác thực 1 lần lúc onboard; nhưng nếu user bị deactive trên Bitrix sau khi onboard, USR_ vẫn sống. Mitigation: +mcp_user_credentials bridge xác thực 1 lần lúc onboard; nhưng nếu user bị deactive trên Bitrix sau khi onboard, USR_ vẫn sống. Mitigation: - `ensureFreshToken()` chạy trên **mỗi** MCP call (via `resolveApiAuth` → `OAuthAuthContext`). - Nếu `last_verified_at > 1h` → gọi `verifyBitrixActive` lại: @@ -238,11 +238,11 @@ Kết quả: user bị xoá khỏi Bitrix → trong vòng 1h MCP key của họ | File | Action | Status | |---|---|---| -| `src/api/auto-onboard.ts` | Rewrite — Path B (verify + rate limit + audit) | ✅ | +| `src/api/auto-onboard.ts` | Rewrite — mcp_user_credentials bridge (verify + rate limit + audit) | ✅ | | `src/auth/bitrix-user-verify.ts` | Create — `verifyBitrixActive` via `profile` | ✅ | | `src/auth/token-manager.ts` | Modify — hook re-verify vào `ensureFreshToken` | ✅ | | `src/db/queries.ts` | Modify — thêm `logAutoOnboardEvent`, `updateUserVerifyStatus`, `deactivateUserApiKeys` | ✅ | -| `src/db/schema.sql` | Modify — thêm bảng `auto_onboard_audit` (Path B audit). `users.user_status` + `last_verified_at` đã có từ Phase 04 | ✅ | +| `src/db/schema.sql` | Modify — thêm bảng `auto_onboard_audit` (mcp_user_credentials bridge audit). `users.user_status` + `last_verified_at` đã có từ Phase 04 | ✅ | | `src/api/api-router.ts` | Modify — route `POST /auto-onboard` (no auth gate) | ✅ | | `wrangler.toml` | Modify — `RATE_LIMIT_KV` binding + drop `ADMIN_TOKEN` dependency | ✅ | @@ -405,7 +405,7 @@ Không đụng health state — channel vẫn Green vì routing vẫn work. |---|---|---| | `internal/channels/bitrix24/channel.go` | Modify — add `mcpStore/mcpClient/mcpServerID/mcpProvMu/mcpDebounce/notifyMu/notifyDebounce/nameCacheMu/nameCache` fields | `ea09c1ba` (phase C) | | `internal/channels/bitrix24/factory.go` | Modify — `FactoryWithPortalStoreAndMCP` variant + half-config validation; `bitrixCreds` empty struct | `ea09c1ba`, `07b48ef0` | -| `internal/channels/bitrix24/mcp_client.go` | Create — thin HTTP client; Path B no-auth | `ea09c1ba`, `07b48ef0` | +| `internal/channels/bitrix24/mcp_client.go` | Create — thin HTTP client; mcp_user_credentials bridge no-auth | `ea09c1ba`, `07b48ef0` | | `internal/channels/bitrix24/provisioner.go` | Create — `provisionIfMissing` + `notifyUserOfMCPIssueOnce` + sentinel errors | `ea09c1ba`, `07b48ef0` | | `internal/channels/bitrix24/contact_enrich.go` | Create — lazy `user.get` cache cho display_name | `ea09c1ba` | | `internal/channels/bitrix24/handle.go` | Modify — call `provisionIfMissing` trước `HandleMessage` | `ea09c1ba` | @@ -452,7 +452,7 @@ Không đụng health state — channel vẫn Green vì routing vẫn work. | Secret | Dùng cho | Rev5 status | |---|---|---| | `BITRIX_CLIENT_ID` / `BITRIX_CLIENT_SECRET` | OAuth dance (install + refresh) | Giữ nguyên | -| ~~`ADMIN_TOKEN`~~ | ~~Auth `/api/auto-onboard`~~ | **Bỏ** (Path B không cần) | +| ~~`ADMIN_TOKEN`~~ | ~~Auth `/api/auto-onboard`~~ | **Bỏ** (mcp_user_credentials bridge không cần) | | `ENCRYPTION_KEY` | D1 field encryption | Giữ nguyên | | KV binding `RATE_LIMIT_KV` | Rate limit `/api/auto-onboard` | **Mới** | @@ -470,7 +470,7 @@ Không cần env var riêng cho MCP integration. Tất cả config sống trong ## 8. Test plan -### 8.1 Unit test MCP side (Path B) +### 8.1 Unit test MCP side (mcp_user_credentials bridge) 1. Body invalid JSON → 400 `bad_request` + audit row `reason:"invalid_json"` 2. Thiếu `domain` / `bitrix_user_id` / `access_token` / `refresh_token` → 400 + audit `missing:""` @@ -531,10 +531,10 @@ Không cần env var riêng cho MCP integration. Tất cả config sống trong ## 9. Rollout sequence -### ✅ Phase A — MCP Path B shipped +### ✅ Phase A — MCP mcp_user_credentials bridge shipped - [x] Schema: thêm bảng `auto_onboard_audit` (Phase 04 đã có sẵn `user_status` + `last_verified_at` — reuse, không migrate lại) - [x] `verifyBitrixActive` via `profile` (thay cơ chế `user.get?FILTER[ID]=…&ACTIVE=true` cũ) -- [x] `/api/auto-onboard` rewrite Path B (bỏ Bearer ADMIN_TOKEN gate) +- [x] `/api/auto-onboard` rewrite mcp_user_credentials bridge (bỏ Bearer ADMIN_TOKEN gate) - [x] Rate limit KV + audit log - [x] Hook `verifyBitrixActive` vào `ensureFreshToken` (re-verify hourly — cơ chế hourly đã có Phase 04, chỉ đổi backend verify) - [x] Deploy + smoke test (end-to-end user#62) @@ -567,7 +567,7 @@ Không cần env var riêng cho MCP integration. Tất cả config sống trong ## 10. Security considerations -### 10.1 Path B auth anchor +### 10.1 mcp_user_credentials bridge auth anchor - **Trust boundary**: MCP tin `access_token` là thật vì Bitrix `profile` xác nhận nó thuộc user nào. Attacker muốn mint USR_ cho user X phải có access_token hợp lệ của user X — mà access_token chỉ leak được nếu Bitrix portal đã bị compromise (trong trường hợp đó attacker đã có quyền cao hơn nhiều so với USR_). - Không còn "master key" → không có rotate periodic; cũng không có single point of credential leak. @@ -616,7 +616,7 @@ Không cần env var riêng cho MCP integration. Tất cả config sống trong ## 12. Changelog -- **2026-04-23 (rev5)**: Path B shipped end-to-end. Bỏ ADMIN_TOKEN. +- **2026-04-23 (rev5)**: mcp_user_credentials bridge shipped end-to-end. Bỏ ADMIN_TOKEN. - MCP side: `/api/auto-onboard` rewrite dùng `verifyBitrixActive(profile)` làm auth anchor thay vì Bearer ADMIN_TOKEN. Thêm rate limit KV (600/min IP + 120/min domain), audit log `auto_onboard_audit`. Hourly re-verify trong `ensureFreshToken` để revoke USR_ của user bị dismiss khỏi Bitrix. - GoClaw side: `mcpClient` bỏ `adminToken` field + Authorization header. `provisioner.go` bỏ `resolveMCPAdminToken` + 2 env consts (`GOCLAW_BITRIX_MCP_ADMIN_TOKEN`, `BITRIX_MCP_ADMIN_TOKEN`). `bitrixCreds` chuyển về empty struct. UI form drop `mcp_admin_token` field. (commit `07b48ef0`) - Bỏ bảng mapping `bitrix_mcp_user_mapping` khỏi plan — reuse partner's `mcp_user_credentials` store. Tiết kiệm ~300 LOC: interface + 2 store impls + migration + SchemaVersion bump.