From 73b46c36343904364f3fe569c1d3933ba050e29a Mon Sep 17 00:00:00 2001 From: viettranx Date: Sat, 28 Feb 2026 16:52:37 +0700 Subject: [PATCH] feat(security): apply upstream TS OpenClaw security and core engine fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port 8 fixes from upstream TypeScript OpenClaw (4 CRITICAL + 4 HIGH): CRITICAL: 1. Tool call name trimming — add strings.TrimSpace() to all provider response parsers (Anthropic 2 locations, OpenAI 3 locations) to prevent registry lookup failures from LLM whitespace-padded names 2. Shell env injection deny patterns — block GIT_EXTERNAL_DIFF, GIT_DIFF_OPTS, BASH_ENV, and ENV=.*sh to prevent code execution via environment variable injection during git/shell operations 3. Broken symlink escape — recursive target resolution via resolveThroughExistingAncestors() to catch chained symlinks that escape workspace (e.g. link1→link2→/etc/passwd) 4. Mutable parent-symlink TOCTOU check — hasMutableSymlinkParent() detects symlinks in writable parent dirs that could be rebound between path validation and file operation HIGH: 5. Model fallback thinking preservation — add ThinkingCapable interface to providers/types.go, implement on Anthropic/OpenAI/DashScope, check before injecting thinking_level in agent loop, warn on fallback when thinking is configured 6. Cron session-key double-prefix guard — prevent agent:X:cron:agent:X duplication in BuildCronSessionKey() 7. Webhook rate limiter — bounded WebhookRateLimiter (4096 keys max, 60s window, 30 hits/window) to prevent memory exhaustion DoS 8. DM policy allowlist validation — warn at startup when dmPolicy=allowlist with empty allowFrom (silent message drop) Co-Authored-By: Claude Opus 4.6 --- cmd/gateway_consumer.go | 4 +- internal/agent/loop.go | 7 ++- internal/agent/resolver.go | 5 ++ internal/channels/channel.go | 14 +++++ internal/channels/discord/discord.go | 1 + internal/channels/feishu/feishu.go | 1 + internal/channels/ratelimit.go | 70 +++++++++++++++++++++ internal/channels/telegram/channel.go | 1 + internal/channels/whatsapp/whatsapp.go | 1 + internal/channels/zalo/zalo.go | 1 + internal/providers/anthropic.go | 9 +-- internal/providers/dashscope.go | 3 +- internal/providers/openai.go | 11 ++-- internal/providers/types.go | 6 ++ internal/sessions/key.go | 7 +++ internal/tools/filesystem.go | 84 +++++++++++++++++++++++++- internal/tools/shell.go | 4 ++ 17 files changed, 213 insertions(+), 16 deletions(-) create mode 100644 internal/channels/ratelimit.go diff --git a/cmd/gateway_consumer.go b/cmd/gateway_consumer.go index 5c64dde0..c6d17590 100644 --- a/cmd/gateway_consumer.go +++ b/cmd/gateway_consumer.go @@ -655,7 +655,7 @@ func makeCronJobHandler(agents *agent.Router, msgBus *bus.MessageBus, cfg *confi return nil, fmt.Errorf("agent %s not found: %w", agentID, err) } - sessionKey := sessions.BuildCronSessionKey(agentID, job.ID, fmt.Sprintf("cron-%s", job.ID)) + sessionKey := sessions.BuildCronSessionKey(agentID, job.ID, job.ID) channel := job.Payload.Channel if channel == "" { channel = "cron" @@ -667,7 +667,7 @@ func makeCronJobHandler(agents *agent.Router, msgBus *bus.MessageBus, cfg *confi Channel: channel, ChatID: job.Payload.To, UserID: job.UserID, - RunID: fmt.Sprintf("cron-%s", job.ID), + RunID: fmt.Sprintf("cron:%s", job.ID), Stream: false, TraceName: fmt.Sprintf("Cron [%s] - %s", job.Name, agentID), TraceTags: []string{"cron"}, diff --git a/internal/agent/loop.go b/internal/agent/loop.go index ad6413aa..4037c5a6 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -552,7 +552,12 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) }, } if l.thinkingLevel != "" && l.thinkingLevel != "off" { - chatReq.Options[providers.OptThinkingLevel] = l.thinkingLevel + if tc, ok := l.provider.(providers.ThinkingCapable); ok && tc.SupportsThinking() { + chatReq.Options[providers.OptThinkingLevel] = l.thinkingLevel + } else { + slog.Debug("thinking_level ignored: provider does not support thinking", + "provider", l.provider.Name(), "level", l.thinkingLevel) + } } // Call LLM (streaming or non-streaming) diff --git a/internal/agent/resolver.go b/internal/agent/resolver.go index 0b57909c..ec253535 100644 --- a/internal/agent/resolver.go +++ b/internal/agent/resolver.go @@ -90,6 +90,11 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { provider, _ = deps.ProviderReg.Get(names[0]) slog.Warn("agent provider not found, using fallback", "agent", agentKey, "wanted", ag.Provider, "using", names[0]) + if tl := ag.ParseThinkingLevel(); tl != "" && tl != "off" { + slog.Warn("agent thinking may not be supported by fallback provider", + "agent", agentKey, "thinking_level", tl, + "wanted_provider", ag.Provider, "fallback_provider", names[0]) + } } if provider == nil { diff --git a/internal/channels/channel.go b/internal/channels/channel.go index 2730116a..ba03eb94 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -10,6 +10,7 @@ package channels import ( "context" + "log/slog" "strings" "github.com/nextlevelbuilder/goclaw/internal/bus" @@ -203,6 +204,19 @@ func (c *BaseChannel) CheckPolicy(peerKind, dmPolicy, groupPolicy, senderID stri } } +// ValidatePolicy logs warnings for common policy misconfigurations. +// Should be called during channel initialization. +func (c *BaseChannel) ValidatePolicy(dmPolicy, groupPolicy string) { + if dmPolicy == "allowlist" && !c.HasAllowList() { + slog.Warn("channel policy misconfiguration: dmPolicy=allowlist but allowFrom is empty — all DMs will be rejected", + "channel", c.name) + } + if groupPolicy == "allowlist" && !c.HasAllowList() { + slog.Warn("channel policy misconfiguration: groupPolicy=allowlist but allowFrom is empty — all group messages will be rejected", + "channel", c.name) + } +} + // HandleMessage creates an InboundMessage and publishes it to the bus. // This is the standard way for channels to forward received messages. // peerKind should be "direct" or "group" (see sessions.PeerDirect, sessions.PeerGroup). diff --git a/internal/channels/discord/discord.go b/internal/channels/discord/discord.go index 2f3cd8a4..76942ac6 100644 --- a/internal/channels/discord/discord.go +++ b/internal/channels/discord/discord.go @@ -47,6 +47,7 @@ func New(cfg config.DiscordConfig, msgBus *bus.MessageBus, pairingSvc store.Pair discordgo.IntentsMessageContent base := channels.NewBaseChannel("discord", msgBus, cfg.AllowFrom) + base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy) requireMention := true if cfg.RequireMention != nil { diff --git a/internal/channels/feishu/feishu.go b/internal/channels/feishu/feishu.go index c8e81ff5..69cb24b1 100644 --- a/internal/channels/feishu/feishu.go +++ b/internal/channels/feishu/feishu.go @@ -63,6 +63,7 @@ func New(cfg config.FeishuConfig, msgBus *bus.MessageBus, pairingSvc store.Pairi client := NewLarkClient(cfg.AppID, cfg.AppSecret, domain) base := channels.NewBaseChannel("feishu", msgBus, cfg.AllowFrom) + base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy) historyLimit := cfg.HistoryLimit if historyLimit == 0 { diff --git a/internal/channels/ratelimit.go b/internal/channels/ratelimit.go new file mode 100644 index 00000000..2bedb737 --- /dev/null +++ b/internal/channels/ratelimit.go @@ -0,0 +1,70 @@ +package channels + +import ( + "sync" + "time" +) + +const ( + // maxTrackedKeys caps the number of tracked rate-limit keys to prevent + // memory exhaustion from attackers rotating source IPs/keys. + maxTrackedKeys = 4096 + + // rateLimitWindow is the sliding window duration for rate counting. + rateLimitWindow = 60 * time.Second + + // rateLimitMaxHits is the max requests per key within a window. + rateLimitMaxHits = 30 +) + +type rateLimitEntry struct { + windowStart time.Time + count int +} + +// WebhookRateLimiter bounds the number of tracked rate-limit keys +// to prevent memory exhaustion from rotating source keys (DoS). +// Safe for concurrent use. +type WebhookRateLimiter struct { + mu sync.Mutex + entries map[string]*rateLimitEntry +} + +// NewWebhookRateLimiter creates a bounded webhook rate limiter. +func NewWebhookRateLimiter() *WebhookRateLimiter { + return &WebhookRateLimiter{entries: make(map[string]*rateLimitEntry)} +} + +// Allow returns true if the key is within rate limits. +// Automatically prunes stale entries and enforces a hard cap on tracked keys. +func (r *WebhookRateLimiter) Allow(key string) bool { + r.mu.Lock() + defer r.mu.Unlock() + + now := time.Now() + + // Prune stale entries when approaching the cap + if len(r.entries) >= maxTrackedKeys { + for k, e := range r.entries { + if now.Sub(e.windowStart) >= rateLimitWindow { + delete(r.entries, k) + } + } + // Hard eviction if still at cap (FIFO-ish via map iteration) + for len(r.entries) >= maxTrackedKeys { + for k := range r.entries { + delete(r.entries, k) + break + } + } + } + + e, ok := r.entries[key] + if !ok || now.Sub(e.windowStart) >= rateLimitWindow { + r.entries[key] = &rateLimitEntry{windowStart: now, count: 1} + return true + } + + e.count++ + return e.count <= rateLimitMaxHits +} diff --git a/internal/channels/telegram/channel.go b/internal/channels/telegram/channel.go index 006a61be..2ff33a18 100644 --- a/internal/channels/telegram/channel.go +++ b/internal/channels/telegram/channel.go @@ -76,6 +76,7 @@ func New(cfg config.TelegramConfig, msgBus *bus.MessageBus, pairingSvc store.Pai } base := channels.NewBaseChannel("telegram", msgBus, cfg.AllowFrom) + base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy) requireMention := true if cfg.RequireMention != nil { diff --git a/internal/channels/whatsapp/whatsapp.go b/internal/channels/whatsapp/whatsapp.go index 53ad2024..279d1d4a 100644 --- a/internal/channels/whatsapp/whatsapp.go +++ b/internal/channels/whatsapp/whatsapp.go @@ -41,6 +41,7 @@ func New(cfg config.WhatsAppConfig, msgBus *bus.MessageBus, pairingSvc store.Pai } base := channels.NewBaseChannel("whatsapp", msgBus, cfg.AllowFrom) + base.ValidatePolicy(cfg.DMPolicy, cfg.GroupPolicy) return &Channel{ BaseChannel: base, diff --git a/internal/channels/zalo/zalo.go b/internal/channels/zalo/zalo.go index 48385598..1bd1984f 100644 --- a/internal/channels/zalo/zalo.go +++ b/internal/channels/zalo/zalo.go @@ -51,6 +51,7 @@ func New(cfg config.ZaloConfig, msgBus *bus.MessageBus, pairingSvc store.Pairing } base := channels.NewBaseChannel("zalo", msgBus, cfg.AllowFrom) + base.ValidatePolicy(cfg.DMPolicy, "") dmPolicy := cfg.DMPolicy if dmPolicy == "" { diff --git a/internal/providers/anthropic.go b/internal/providers/anthropic.go index 965c7186..ea12dba9 100644 --- a/internal/providers/anthropic.go +++ b/internal/providers/anthropic.go @@ -56,8 +56,9 @@ func WithAnthropicBaseURL(baseURL string) AnthropicOption { } } -func (p *AnthropicProvider) Name() string { return "anthropic" } -func (p *AnthropicProvider) DefaultModel() string { return p.defaultModel } +func (p *AnthropicProvider) Name() string { return "anthropic" } +func (p *AnthropicProvider) DefaultModel() string { return p.defaultModel } +func (p *AnthropicProvider) SupportsThinking() bool { return true } func (p *AnthropicProvider) Chat(ctx context.Context, req ChatRequest) (*ChatResponse, error) { model := req.Model @@ -150,7 +151,7 @@ func (p *AnthropicProvider) ChatStream(ctx context.Context, req ChatRequest, onC if ev.ContentBlock.Type == "tool_use" { result.ToolCalls = append(result.ToolCalls, ToolCall{ ID: ev.ContentBlock.ID, - Name: ev.ContentBlock.Name, + Name: strings.TrimSpace(ev.ContentBlock.Name), Arguments: make(map[string]interface{}), }) } @@ -529,7 +530,7 @@ func (p *AnthropicProvider) parseResponse(resp *anthropicResponse) *ChatResponse _ = json.Unmarshal(block.Input, &args) result.ToolCalls = append(result.ToolCalls, ToolCall{ ID: block.ID, - Name: block.Name, + Name: strings.TrimSpace(block.Name), Arguments: args, }) } diff --git a/internal/providers/dashscope.go b/internal/providers/dashscope.go index ae33f75a..68203a3b 100644 --- a/internal/providers/dashscope.go +++ b/internal/providers/dashscope.go @@ -29,7 +29,8 @@ func NewDashScopeProvider(apiKey, apiBase, defaultModel string) *DashScopeProvid } } -func (p *DashScopeProvider) Name() string { return "dashscope" } +func (p *DashScopeProvider) Name() string { return "dashscope" } +func (p *DashScopeProvider) SupportsThinking() bool { return true } // ChatStream handles DashScope's limitation: tools + streaming cannot coexist. // When tools are present, falls back to non-streaming Chat() and synthesizes diff --git a/internal/providers/openai.go b/internal/providers/openai.go index 05c507c4..7b903808 100644 --- a/internal/providers/openai.go +++ b/internal/providers/openai.go @@ -47,8 +47,9 @@ func (p *OpenAIProvider) WithChatPath(path string) *OpenAIProvider { return p } -func (p *OpenAIProvider) Name() string { return p.name } -func (p *OpenAIProvider) DefaultModel() string { return p.defaultModel } +func (p *OpenAIProvider) Name() string { return p.name } +func (p *OpenAIProvider) DefaultModel() string { return p.defaultModel } +func (p *OpenAIProvider) SupportsThinking() bool { return true } func (p *OpenAIProvider) APIKey() string { return p.apiKey } func (p *OpenAIProvider) APIBase() string { return p.apiBase } @@ -140,12 +141,12 @@ func (p *OpenAIProvider) ChatStream(ctx context.Context, req ChatRequest, onChun acc, ok := accumulators[tc.Index] if !ok { acc = &toolCallAccumulator{ - ToolCall: ToolCall{ID: tc.ID, Name: tc.Function.Name}, + ToolCall: ToolCall{ID: tc.ID, Name: strings.TrimSpace(tc.Function.Name)}, } accumulators[tc.Index] = acc } if tc.Function.Name != "" { - acc.Name = tc.Function.Name + acc.Name = strings.TrimSpace(tc.Function.Name) } acc.rawArgs += tc.Function.Arguments if tc.Function.ThoughtSignature != "" { @@ -356,7 +357,7 @@ func (p *OpenAIProvider) parseResponse(resp *openAIResponse) *ChatResponse { _ = json.Unmarshal([]byte(tc.Function.Arguments), &args) call := ToolCall{ ID: tc.ID, - Name: tc.Function.Name, + Name: strings.TrimSpace(tc.Function.Name), Arguments: args, } if tc.Function.ThoughtSignature != "" { diff --git a/internal/providers/types.go b/internal/providers/types.go index e108d4d4..929b9519 100644 --- a/internal/providers/types.go +++ b/internal/providers/types.go @@ -32,6 +32,12 @@ type Provider interface { Name() string } +// ThinkingCapable is optionally implemented by providers that support extended thinking. +// Used to gate thinking_level injection so it's not sent to providers that ignore it. +type ThinkingCapable interface { + SupportsThinking() bool +} + // ChatRequest contains the input for a Chat/ChatStream call. type ChatRequest struct { Messages []Message `json:"messages"` diff --git a/internal/sessions/key.go b/internal/sessions/key.go index ce7d665a..049e5635 100644 --- a/internal/sessions/key.go +++ b/internal/sessions/key.go @@ -60,7 +60,14 @@ func BuildSubagentSessionKey(agentID, label string) string { // BuildCronSessionKey builds the session key for a cron job run. // // agent:{agentId}:cron:{jobID}:run:{runID} +// +// Guards against double-prefixing: if jobID is already a canonical session key +// (e.g. "agent:X:..."), only the rest part is used to prevent +// "agent:X:cron:agent:X:cron:..." duplication. func BuildCronSessionKey(agentID, jobID, runID string) string { + if _, rest := ParseSessionKey(jobID); rest != "" { + jobID = rest + } return fmt.Sprintf("agent:%s:cron:%s:run:%s", agentID, jobID, runID) } diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index 40ebcf71..e26ee6a0 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -247,11 +247,19 @@ func resolvePath(path, workspace string, restrict bool) (string, error) { target = filepath.Join(filepath.Dir(absResolved), target) } target = filepath.Clean(target) - if !isPathInside(target, wsReal) { - slog.Warn("security.broken_symlink_escape", "path", path, "target", target, "workspace", wsReal) + + // Resolve through existing ancestors to catch chained symlinks + // (e.g. link1 → link2 → /outside) where intermediate targets escape. + resolved, resolveErr := resolveThroughExistingAncestors(target) + if resolveErr != nil { + slog.Warn("security.broken_symlink_resolve_failed", "path", path, "target", target) + return "", fmt.Errorf("access denied: cannot resolve broken symlink target") + } + if !isPathInside(resolved, wsReal) { + slog.Warn("security.broken_symlink_escape", "path", path, "target", resolved, "workspace", wsReal) return "", fmt.Errorf("access denied: broken symlink target outside workspace") } - real = target + real = resolved } else { // Truly non-existent file (not a symlink): resolve parent and re-validate. parentReal, parentErr := filepath.EvalSymlinks(filepath.Dir(absResolved)) @@ -273,6 +281,14 @@ func resolvePath(path, workspace string, restrict bool) (string, error) { return "", fmt.Errorf("access denied: path outside workspace") } + // Reject paths with mutable symlink components (TOCTOU symlink rebind risk). + // A symlink in the path whose parent directory is writable could be replaced + // between resolution time and actual file operation. + if hasMutableSymlinkParent(real) { + slog.Warn("security.mutable_symlink_parent", "path", path, "resolved", real) + return "", fmt.Errorf("access denied: path contains mutable symlink component") + } + // Reject hardlinked files (nlink > 1) to prevent hardlink-based escapes. if err := checkHardlink(real); err != nil { return "", err @@ -289,6 +305,68 @@ func isPathInside(child, parent string) bool { return strings.HasPrefix(child, parent+string(filepath.Separator)) } +// resolveThroughExistingAncestors resolves a path by finding the deepest +// existing ancestor, canonicalizing it with EvalSymlinks, then appending +// the remaining non-existent components. This handles broken symlinks +// whose targets contain intermediate symlinks that escape the workspace. +func resolveThroughExistingAncestors(target string) (string, error) { + // Try full resolution first (target exists and all symlinks resolve) + if real, err := filepath.EvalSymlinks(target); err == nil { + return real, nil + } + + // Walk up to find the deepest existing ancestor + current := target + var tail []string + for { + parent := filepath.Dir(current) + if parent == current { + // Reached filesystem root without finding existing dir + break + } + tail = append([]string{filepath.Base(current)}, tail...) + current = parent + + if realParent, err := filepath.EvalSymlinks(current); err == nil { + // Found existing ancestor — canonicalize and rebuild + result := realParent + for _, component := range tail { + result = filepath.Join(result, component) + } + return result, nil + } + } + return filepath.Clean(target), nil +} + +// hasMutableSymlinkParent checks if any component of the resolved path is a symlink +// whose parent directory is writable by the current process. A writable parent means +// the symlink could be replaced between path resolution and actual file operation +// (TOCTOU symlink rebind attack). +func hasMutableSymlinkParent(path string) bool { + clean := filepath.Clean(path) + components := strings.Split(clean, string(filepath.Separator)) + current := string(filepath.Separator) + for _, comp := range components { + if comp == "" { + continue + } + current = filepath.Join(current, comp) + info, err := os.Lstat(current) + if err != nil { + break // non-existent — stop checking + } + if info.Mode()&os.ModeSymlink != 0 { + // Symlink found — check if its parent dir is writable + parentDir := filepath.Dir(current) + if syscall.Access(parentDir, 0x2 /* W_OK */) == nil { + return true + } + } + } + return false +} + // checkHardlink rejects regular files with nlink > 1 (hardlink attack prevention). // Directories naturally have nlink > 1 and are exempt. func checkHardlink(path string) error { diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 985b20b8..852fd123 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -75,6 +75,10 @@ var defaultDenyPatterns = []*regexp.Regexp{ regexp.MustCompile(`\bDYLD_INSERT_LIBRARIES\s*=`), regexp.MustCompile(`\bLD_LIBRARY_PATH\s*=`), regexp.MustCompile(`/etc/ld\.so\.preload`), + regexp.MustCompile(`\bGIT_EXTERNAL_DIFF\s*=`), // git diff arbitrary code exec + regexp.MustCompile(`\bGIT_DIFF_OPTS\s*=`), // git diff behavior injection + regexp.MustCompile(`\bBASH_ENV\s*=`), // shell init injection + regexp.MustCompile(`\bENV\s*=.*\bsh\b`), // sh init injection // ── Container escape ── regexp.MustCompile(`/var/run/docker\.sock|docker\.(sock|socket)`),