feat(bitrix24): imbot.v2 migration, 2-way media, openline sender-tag echo, and hardening (#1236)

* refactor(bitrix24): rename "Path B" framing to maintainer-specified naming [B24:2794] Per maintainer hard rule #10 (no generic "Path A/B" framing) from PR #1061 review. The Bitrix24 MCP auto-onboard flow is Bitrix-specific glue ("Bitrix24 OAuth -> existing mcp_user_credentials bridge"), NOT a generic MCP architecture pattern. Naming convention applied consistently: - First mention per file: full "Bitrix24 OAuth -> existing mcp_user_credentials bridge" (matches maintainer comment verbatim). - Subsequent mentions in same file: shortened "mcp_user_credentials bridge". - Test/log context referencing literal endpoint /api/auto-onboard: keep "auto-onboard" reference (it's the actual API endpoint name). Changes are documentation-only: - Rename in code comments + test descriptions + plan docs. - Clarify framing in mcp_client.go + provisioner.go doc comments to emphasize Bitrix-specific glue (not generic MCP infra). - Reuse existing mcp_user_credentials table + MCPServerStore methods (no schema / store / abstraction change). Files: - cmd/gateway.go (factory registration doc) - internal/channels/bitrix24/{channel,factory,mcp_client,provisioner}.go - internal/channels/bitrix24/{mcp_client,provisioner}_test.go - plan/goclaw-mcp-integration.md (21 occurrences) Verified: go build + MCP-related tests pass (TestProvision*, TestInitMCPProvisioner*, TestMCPClient*). Phase 1 of Path C execution per plans/reports/decision-log-260519-1555-bitrix24-pr-fork-decision.md. * fix: confine outbound media paths to agent workspace [B24:2794] Tool MEDIA:<path> output reached channel file-upload sinks (Bitrix imbot.v2.File.upload, Telegram sendDocument, etc.) verbatim via parseMediaResult, with no workspace-boundary check. A malicious or buggy tool emitting MEDIA:/etc/passwd could exfiltrate arbitrary files to chat. Extract the EvalSymlinks+Rel containment from extractMediaFromContent into a shared confineToWorkspace helper and apply it at the parseMediaResult sink in processToolResult. Fixing at the source/egress boundary protects every channel at once rather than per-channel. Paths that escape the workspace are dropped and logged (security.media_path_rejected). Add TestConfineToWorkspace (boundary unit) and TestParseMediaResultConfinedToWorkspace (sink regression for H2). * feat(bitrix24): support inbound + outbound media via imbot.v2 File API [B24:2794] Bitrix24 channel was text-only; attachments were parsed but dropped. - Inbound: download chat files via imbot.v2.File.download (one-time URL), forward to the agent with MIME preserved (internal/channels/bitrix24/download.go). - Outbound: upload agent media to the chat via imbot.v2.File.upload (internal/channels/bitrix24/send_media.go). - Add BaseChannel.HandleMessageMedia to preserve MIME/filename through the bus. - Per-channel media_max_mb cap (default 20) applies to both directions. Tests: 92 pass (internal/channels/bitrix24 + internal/channels), go vet clean (PG + sqliteonly). * refactor(bitrix24): migrate messaging/bot-list/unregister to imbot v2 API [B24:2794] Move outbound REST calls to the imbot v2 family (keeps register on v1): - imbot.message.add -> imbot.v2.Chat.Message.send (fields.message shape, live-verified) - imbot.bot.list (+ legacy imbot.list fallback) -> imbot.v2.Bot.list; add botListRows to normalize the v2 {bots:[...]} envelope, legacy array, and id-keyed map forms - imbot.unregister -> imbot.v2.Bot.unregister Bot registration 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. Tests: bitrix24 package green; go vet ./... clean. * feat(bitrix24): route whisper via v1 SKIP_CONNECTOR + add v2 replyId [B24:2794] Bot was leaking HiddenMessage (whisper) replies to the external Zalo connector because every outbound call went through imbot.v2.Chat.Message.send, which has no equivalent of the v1 SKIP_CONNECTOR flag. Branch the outbound path on inbound visibility: whisper → imbot.message.add + SKIP_CONNECTOR=Y (v1, send_v1.go) public → imbot.v2.Chat.Message.send + fields.replyId (v2, send_v2.go) Pipeline: events.go parse data[PARAMS][PARAMS][COMPONENT_ID]=HiddenMessage into EventParams.IsHiddenMessage (form + JSON variants) handle.go set bitrix_visibility on InboundMessage.Metadata consumer forward visibility + message_id into OutboundMessage send.go resolveSendOptions + sendChunk dispatcher + shared callWithRateLimitRetry helper metadata_keys.go single source of truth for the keys + values Defaults preserve pre-refactor behaviour: callers that don't populate bitrix_visibility still go through v2 public, and replyId is omitted unless a numeric bitrix_message_id arrives in metadata. Tests: TestParseEvent_FormURLEncoded_IsHiddenMessage (3 cases) TestParseEvent_JSON_IsHiddenMessage (3 cases) TestResolveSendOptions (8 cases) TestSend_BranchesOnVisibility (4 cases) * feat(bitrix24): openline sender-tag echo on replies [B24:2794] Openline sender-tag echo (this change): - Capture the connector sender tag ("[name #id]:" or "[name] #id:") from inbound openline group messages, strip it from the body the agent sees, and re-prepend the canonical "[name] #id:" form to the reply so the Open Channel connector routes the answer back to the right external user. - New sender_prefix.go helper (+ test) accepts both inbound layouts and emits one canonical form; scoped to messages carrying the tag, so plain chats are unaffected. - metadata_keys.go: MetaKeySenderPrefix; handle.go capture/strip/stash; gateway_consumer_normal.go forwards the key; send.go prepends it on the first chunk before chunking. Bundled bitrix24 channel-core work already on this branch: - handle.go: @mention is the sole trigger for both staff and connector customers; unmentioned traffic is dropped (was: drop all connector msgs). - isGroupMessageType: treat SONET_GROUP "B" as a group. - handle_test.go, mcp_client_test.go: cover the above. * feat(bitrix24): accept colon-less openline sender tag, echo [name] #id [B24:2794] The Open Channel connector dropped the trailing colon from its sender tag: inbound now arrives as "[Name] #id <msg>" (was "[Name] #id: <msg>"). The id-bearing patterns required the colon, so the tag fell through to the name-only branch and the reply echoed "[Name]" — dropping the #id the connector needs to route the answer back. - sender_prefix.go: make the trailing ":" optional on both id layouts ([name #id] / [name] #id, with or without colon) and echo the canonical "[name] #id" (no colon) to match the connector's current format. Bare "[name]" (no id) still echoes "[name]" for Open Channel only. - handle.go: gate the bare name-only layout to Open Channel (isOpenChannel) so ordinary group chats starting with "[x] ..." are left untouched. - sender_prefix_test.go: cover colon/no-colon x id-inside/id-outside, the name-only openline case, and the non-openline no-op. * fix: security and robustness fixes from the bitrix24 channel review [B24:2794] - download.go: block redirect-based SSRF on inbound media. CheckRedirect re-validates each hop (http(s) only, reject private/loopback/link-local hosts, cap hops); the initial portal-domain pin is no longer bypassable via a 3xx to an internal service. Public-host redirects still allowed. - handle.go: extract/echo the openline sender tag only for Open Channel sessions (was: any group chat), removing bogus prefixes in CRM group chats and narrowing the forged-tag misroute surface. - loop_tools.go + loop_media.go: confine result.Media to the agent / team / tenant-allowed roots (new confineToAnyRoot) before a channel uploads it, so a prompt-injected out-of-workspace path (e.g. /etc/passwd) cannot exfiltrate, while legitimate cross-workspace media (team files, delegatee output) still flows. - send_media.go: bounded outbound read via io.LimitReader replaces the os.Stat + os.ReadFile pair, closing the TOCTOU size-cap bypass; cap a single message's outbound attachments at 10 (mirrors inbound). - register.go: paginate imbot.v2.Bot.list (limit/offset + hasNextPage, capped at 40 pages) so verify/lookup see bots past the first 50. - mcp_client.go: redact access_token / refresh_token / client_secret from an echoed MCP error body before it is logged or returned (+ test). * fix(security): validate resolved dial IP on Bitrix media redirects [B24:2794] The inbound media download redirect guard only string-checked the redirect hostname (isPrivateOrLoopback on req.URL.Hostname()), so a redirect to a public hostname that resolves to 127.0.0.1 / 169.254.169.254 / an RFC1918 address — or a DNS-rebinding swap between check and dial — still passed the guard and the client would connect. Reported in PR review. Add security.NewRedirectFollowingSafeClient: it follows redirects but validates the RESOLVED destination IP of every hop at dial time via net.Dialer.Control, reusing the existing blocked-CIDR list. The IP it checks is the IP actually dialed, so both redirect-to-internal and DNS rebinding are refused, while legitimate public CDN redirects still succeed. download.go now uses it instead of the hostname-string guard. Tests: deterministic dial-control table (loopback / link-local / private / multicast / unspecified / public, v4 + v6), malformed/non-IP addr, test bypass, loopback-dial-blocked client wiring, and redirect cap + scheme checks. * feat(bitrix24): per-participant Zalo openline identity from 3-token sender tag [B24:2794] Parse the connector's "[Name] #uid #msgId" sender tag so each external customer in a shared Open Channel group gets its own contact + USER.md instead of collapsing onto the connector proxy id. Identity minting is gated on IS_CONNECTOR=Y to reject operator forged tags. Echo back the msgId only ("#msgId") on replies; keep the legacy single-number and name-only layouts unchanged. Zero DB migration. - sender_prefix.go: parseOpenlineSenderTag() classifies 3-token / legacy / name-only - handle.go: synthetic senderID "openlines:{instance}:{chat}:{uid}" + participant_user_id metadata, gated on FromIsConnector - gateway_consumer_normal.go: deriveGroupUserID() routes participant -> per-person scope, group fallback otherwise - send.go: buildAddressMention numeric-id guard so synthetic ids don't emit invalid [USER=...] BBCode - MetaKeyMessageID kept as Bitrix MESSAGE_ID (drives v2 fields.replyId); connector msgId surfaced only via echo prefix --------- Co-authored-by: DangTinh311 <dangtinh31193@gmail.com> Co-authored-by: Chinh Dang <chinhdang@192.168.68.104>
2026-10-15 22:15:00 +00:00 · 2026-06-22 14:23:34 +07:00
1 parent c24677272c
commit bd5adc61c8
@@ -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)
+			}
+		},
+	}
+}
@@ -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)
+	}
+}