mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
dev
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Conner MoandConner Mo
|
c21499a7f5 |
feat(security): let operators un-block CIDRs behind a transparent proxy (#1465)
SSRF protection resolves a hostname and judges the resulting IP. That model assumes DNS resolution describes where the traffic actually goes, which stops being true behind a TUN/fake-IP proxy: every query is answered with a synthetic address out of a reserved range, and the proxy then routes that address to the real public host. The IP is a handle, not a destination. In that environment web_fetch rejects ordinary public sites — observed with news.sina.cn resolving to 198.18.0.236 — and no configuration can fix it, because the block list is compiled in. The agent then burns iterations retrying URLs that can never succeed. GOCLAW_SSRF_ALLOWED_CIDRS lets an operator name the ranges their proxy hands out. Empty by default, so nothing changes for deployments that do not set it, and the accepted and refused entries are both logged at startup — this widens what LLM- and admin-supplied URLs can reach, so it should be visible. Ranges an SSRF actually targets can never be allowlisted: link-local (including cloud metadata at 169.254.169.254), multicast and unspecified are refused at parse time, in either direction, so neither an exact entry nor a wider range that swallows one gets through. Applied inside isBlocked rather than only in validate() because NewSafeClient re-checks the pinned IP at dial time through the same function — relaxing just the pre-flight check would pass validation and then fail to connect. internal/tools carries its own private-range list for web_fetch and web_search, separate from this package and not identical to it. It has to consult the same setting, or relaxing one gate leaves the other rejecting the very traffic the operator permitted. Unifying the two lists is left alone here; it is a wider change than this one. Co-authored-by: Conner Mo <connermo@ConnerdeMacBook-Pro.local> |
||
|
Thanh_Dang
|
12a0168271 |
fix(security): block 198.18.0.0/15 and 240.0.0.0/4 in SSRF protection (#1269)
Add RFC 2544 benchmarking range (198.18.0.0/15) and reserved range (240.0.0.0/4) to both SSRF blocklists (security package + web_fetch tool) to prevent internal network access via these special-use IPs. Closes #1218 |
||
|
|
bd5adc61c8 |
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> |
||
|
Zezae Oh
|
c31dd21934 |
feat(mcp): opt-in allowlist for trusted private MCP hosts (#1248)
Registering a remote MCP server (sse/streamable-http) whose hostname resolves to a private IP is rejected at config-validation time by the SSRF guard, with no production escape hatch -- the only bypass is a test-only loopback flag. Self-hosted MCP servers on a private network are therefore unregisterable, even though the runtime MCP client connects to them fine (Test Connection succeeds and lists tools; only create/update input validation blocks). Add an opt-in, operator-configured allowlist (GOCLAW_MCP_ALLOWED_HOSTS, empty by default) of trusted hostnames exempt from the private/loopback IP block during MCP server URL validation only: - security.ValidateAllowingHosts(url, allowedHosts): like Validate but skips the private/loopback block for allowlisted hostnames. The cloud-metadata/link-local (169.254/fe80), multicast and unspecified ranges are never exempted, even for allowlisted hosts. - mcp.SetAllowedHosts wires the operator allowlist into ValidateURL / ValidateServerConfig; default empty => no behavior change. - web_fetch / webhook / redirect SSRF paths are unchanged (those stay agent-influenced and fully guarded). Matching is case-insensitive on the pre-resolution hostname. |
||
|
|
43837afca3 |
fix(security): consolidate & enhance batched security fixes (#1155, #967, #972, #974, #989, #973) (#1185)
* fix(sandbox): avoid shell in FsBridge writes Replace sh -c with interpolated path by shell-free 'tee -- <path>' argv form, piping content via stdin. Prevents command injection through filenames containing shell metacharacters inside the sandbox container. Co-authored-by: evgyur <evgyur@gmail.com> * fix(security): fail-closed on pairing DB errors across channels On IsPaired lookup error, deny instead of granting access. Covers the shared CheckDMPolicy/CheckGroupPolicy helpers (Slack/Discord/Feishu/WhatsApp/Zalo) and the four inline Telegram pairing checks. Co-authored-by: Srini <srinis.k@gmail.com> * fix(security): harden provider URL validation against SSRF Enforce scheme check for all provider types; restrict local types (ollama, claude_cli, acp) to an explicit localhost allowlist instead of skipping checks; resolve remote hostnames and reject any IP in a private/reserved range via the shared security.IsBlocked CIDR list (covers loopback, link-local, metadata, multicast, and unspecified 0.0.0.0/::). Closes the wildcard-DNS bypass and the local-type escape hatch. Operator opt-in via GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS. Exports security.IsBlocked as the single source of truth for blocked ranges. Co-authored-by: Linh Vo Van <linh.vo@e-cq.net> * feat(pipeline): add fail-closed tool call authorization gate Gate tool execution against the server-side AllowedTools allowlist built from the RBAC/tenant-aware filtered tool set. Resolve the tool-call prefix before the allowlist lookup so prefixed agents are not wrongly blocked, re-check deny on lazy MCP activation, and expand IsDenied to cover aliased tool names. Co-authored-by: Huy Doan <tui@pm.me> * fix(security): expand file-serve deny-list defense-in-depth Add absolute-path deny prefixes (/home, /Users, /srv, /var/lib, /var/www, /opt) and an explicit fail-closed log when no file-serving boundary is configured. Co-authored-by: Linh Vo Van <linh.vo@e-cq.net> * fix(providers): allow claude cli executable paths Refs: #1185 --------- Co-authored-by: evgyur <evgyur@gmail.com> Co-authored-by: Srini <srinis.k@gmail.com> Co-authored-by: Linh Vo Van <linh.vo@e-cq.net> Co-authored-by: Huy Doan <tui@pm.me> |
||
|
badgerbeesandViet Tran
|
f5917b0e46 |
fix(backup): harden tenant restore preview and lookup handling (#920)
* fix(backup): harden tenant restore preview and lookup handling * test(pg): fix hook test migration path * test(ci): stabilize race coverage tests * fix(hooks): scope GetByID and allow loopback tests * fix(backup): harden tenant restore replace/new contracts + race-safe SSRF flag - Replace mode no longer deletes the tenants row (FK safe vs excluded diagnostic tables: traces, activity_logs, usage_snapshots, spans, embedding_cache, pairing_requests, paired_devices, channel_pending_messages, cron_run_logs). Metadata is preserved in place. - shouldRestoreTable now excludes tenants for both new and replace modes. - CLI: add validateTenantRestoreFlags guardrail. mode=new requires --new-tenant-slug and rejects --tenant/--tenant-id; upsert/replace warn on stray --new-tenant-slug; invalid --mode values rejected. TAB in help text fixed; flag descriptions clarified. - HTTP: resolveRestoreTarget rejects tenant_id for mode=new regardless of tenant_slug (matches CLI contract). New i18n key MsgRestoreNewModeRejectsTenantID (en/vi/zh). - security/ssrf: allowLoopbackForTest switched to atomic.Bool so concurrent reads from outbound dialers are race-safe. - Polish: vi backup.json key order matches en/zh; TenantRestoreOptions.Mode doc comment documents upsert/replace/new semantics including clone behavior for new. - Tests: unit coverage for validator (12 cases), HTTP guardrails (3 cases), shouldRestoreTable replace branch. Integration test tests/integration/tenant_restore_replace_test.go regression-guards the FK fix using activity_logs seed + DeleteTenantDataForTest helper. --------- Co-authored-by: Viet Tran <viettranx@gmail.com> |
||
|
viettranx
|
fb3552f7e5 |
fix(hooks/security): add SSRF-safe http client missed by phase 2 commit
Phase 2 (a587231b) advertised SSRF hardening for the http hook handler but the supporting `internal/security` package was never created, so the production HTTPHandler fell back to a bare http.Client and admin-config webhooks could probe loopback / link-local / RFC1918 / cloud-metadata. - internal/security/ssrf.go: Validate(rawURL) parses + resolves once, rejects loopback/link-local/private/multicast/unspecified + 169.254.169.254; NewSafeClient(timeout) returns an http.Client whose DialContext pins the resolved IP from context (defense-in-depth re-checks the dialed IP) and refuses redirects (CheckRedirect = ErrUseLastResponse) - internal/hooks/handlers/http_handler.go: call Validate before each request, attach pinned IP via security.WithPinnedIP(ctx, ip) - cmd/gateway_managed.go: construct HTTPHandler with Client: security.NewSafeClient(10*time.Second) - tests: 13 new ssrf_test.go cases (every block category + redirect refused + dial pinned); existing http_test.go retrofitted with security.SetAllowLoopbackForTest helper for httptest.NewServer - plan: phase-02 Step 2a marked done with reference to this commit Refs: GitHub Issue #875 |