From df5d48c78dde62d95f91bf45c8e47036c437951f Mon Sep 17 00:00:00 2001 From: viettranx Date: Fri, 24 Apr 2026 20:59:08 +0700 Subject: [PATCH] feat(tools/shell): add runtime-reloadable global shell deny-groups via config.tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds ToolsConfig.ShellDenyGroups (map[string]bool) so operators can toggle shell deny groups (package_install, env_dump, etc.) from the /config Web UI without a process restart. Wiring: - ExecTool gains globalDenyGroups + SetGlobalShellDenyGroups (defensive copy). - effectiveDenyGroups merges per-agent context overrides with global, with per-key agent precedence — multi-tenant invariant preserved. - setupToolRegistry applies the initial config before any request can arrive. - subscribeShellDenyGroupsReload (extracted helper for testability) reacts to bus.TopicConfigChanged and re-applies the new toggles. Regression coverage the original PR #1005 attempt missed: - 6 unit tests for the merge / defensive-copy semantics in internal/tools. - 3 pub/sub dispatch tests in cmd: happy path, wrong event name, wrong payload type. --- cmd/gateway_lifecycle.go | 4 + cmd/gateway_lifecycle_shell_deny_groups.go | 35 ++++++ ...ateway_lifecycle_shell_deny_groups_test.go | 90 ++++++++++++++++ cmd/gateway_setup.go | 3 + internal/config/config_channels.go | 5 +- internal/tools/shell.go | 52 ++++++++- .../tools/shell_global_deny_groups_test.go | 100 ++++++++++++++++++ 7 files changed, 285 insertions(+), 4 deletions(-) create mode 100644 cmd/gateway_lifecycle_shell_deny_groups.go create mode 100644 cmd/gateway_lifecycle_shell_deny_groups_test.go create mode 100644 internal/tools/shell_global_deny_groups_test.go diff --git a/cmd/gateway_lifecycle.go b/cmd/gateway_lifecycle.go index 755c8ba2..bc6c4277 100644 --- a/cmd/gateway_lifecycle.go +++ b/cmd/gateway_lifecycle.go @@ -84,6 +84,10 @@ func (d *gatewayDeps) runLifecycle( deps.webFetchTool.UpdatePolicy(updatedCfg.Tools.WebFetch.Policy, updatedCfg.Tools.WebFetch.AllowedDomains, updatedCfg.Tools.WebFetch.BlockedDomains) }) + // Reload global shell deny-group toggles on config changes via pub/sub + // so /config edits apply without a process restart. + subscribeShellDenyGroupsReload(d.msgBus, d.toolsReg) + // Reload TTS providers on config changes via pub/sub. d.msgBus.Subscribe("tts-config-reload", func(evt bus.Event) { if evt.Name != bus.TopicConfigChanged { diff --git a/cmd/gateway_lifecycle_shell_deny_groups.go b/cmd/gateway_lifecycle_shell_deny_groups.go new file mode 100644 index 00000000..28c8be7f --- /dev/null +++ b/cmd/gateway_lifecycle_shell_deny_groups.go @@ -0,0 +1,35 @@ +package cmd + +import ( + "log/slog" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// subscribeShellDenyGroupsReload wires pub/sub so global shell deny-group +// toggles applied via the /config page take effect without a process restart. +// Extracted from runLifecycle to make the dispatch path unit-testable +// (the regression coverage missing from the original PR #1005 attempt). +func subscribeShellDenyGroupsReload(msgBus *bus.MessageBus, toolsReg *tools.Registry) { + msgBus.Subscribe("shell-deny-groups-config-reload", func(evt bus.Event) { + if evt.Name != bus.TopicConfigChanged { + return + } + updatedCfg, ok := evt.Payload.(*config.Config) + if !ok { + return + } + execTool, ok := toolsReg.Get("exec") + if !ok { + return + } + et, ok := execTool.(*tools.ExecTool) + if !ok { + return + } + et.SetGlobalShellDenyGroups(updatedCfg.Tools.ShellDenyGroups) + slog.Info("shell deny groups reloaded via pub/sub", "groups", len(updatedCfg.Tools.ShellDenyGroups)) + }) +} diff --git a/cmd/gateway_lifecycle_shell_deny_groups_test.go b/cmd/gateway_lifecycle_shell_deny_groups_test.go new file mode 100644 index 00000000..13368dd6 --- /dev/null +++ b/cmd/gateway_lifecycle_shell_deny_groups_test.go @@ -0,0 +1,90 @@ +package cmd + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// TestShellDenyGroupsConfigReload_UpdatesGlobal asserts the pub/sub subscriber +// dispatches a TopicConfigChanged event into ExecTool.SetGlobalShellDenyGroups — +// the regression coverage that the original PR #1005 was missing. +func TestShellDenyGroupsConfigReload_UpdatesGlobal(t *testing.T) { + msgBus := bus.New() + defer msgBus.Unsubscribe("shell-deny-groups-config-reload") + + toolsReg := tools.NewRegistry() + execTool := tools.NewExecTool("/tmp", false) + toolsReg.Register(execTool) + + subscribeShellDenyGroupsReload(msgBus, toolsReg) + + msgBus.Broadcast(bus.Event{ + Name: bus.TopicConfigChanged, + Payload: &config.Config{ + Tools: config.ToolsConfig{ + ShellDenyGroups: map[string]bool{"package_install": true}, + }, + }, + }) + + got := execTool.EffectiveDenyGroupsForTest(context.Background()) + if v, ok := got["package_install"]; !ok || v != true { + t.Fatalf("expected pub/sub to set global package_install=true, got %v", got) + } +} + +// TestShellDenyGroupsConfigReload_IgnoresOtherEvents: subscriber must guard +// on event.Name and ignore non-TopicConfigChanged broadcasts. +func TestShellDenyGroupsConfigReload_IgnoresOtherEvents(t *testing.T) { + msgBus := bus.New() + defer msgBus.Unsubscribe("shell-deny-groups-config-reload") + + toolsReg := tools.NewRegistry() + execTool := tools.NewExecTool("/tmp", false) + execTool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false}) // baseline + toolsReg.Register(execTool) + + subscribeShellDenyGroupsReload(msgBus, toolsReg) + + msgBus.Broadcast(bus.Event{ + Name: bus.TopicAgentDeleted, + Payload: &config.Config{ + Tools: config.ToolsConfig{ + ShellDenyGroups: map[string]bool{"package_install": true}, + }, + }, + }) + + got := execTool.EffectiveDenyGroupsForTest(context.Background()) + if v := got["package_install"]; v != false { + t.Fatalf("expected non-config event to be ignored; package_install changed to %v", v) + } +} + +// TestShellDenyGroupsConfigReload_IgnoresWrongPayload: subscriber must +// type-assert payload to *config.Config and skip mismatched payloads. +func TestShellDenyGroupsConfigReload_IgnoresWrongPayload(t *testing.T) { + msgBus := bus.New() + defer msgBus.Unsubscribe("shell-deny-groups-config-reload") + + toolsReg := tools.NewRegistry() + execTool := tools.NewExecTool("/tmp", false) + execTool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false}) + toolsReg.Register(execTool) + + subscribeShellDenyGroupsReload(msgBus, toolsReg) + + msgBus.Broadcast(bus.Event{ + Name: bus.TopicConfigChanged, + Payload: "not-a-config-pointer", + }) + + got := execTool.EffectiveDenyGroupsForTest(context.Background()) + if v := got["package_install"]; v != false { + t.Fatalf("expected wrong-payload event to be ignored; package_install changed to %v", v) + } +} diff --git a/cmd/gateway_setup.go b/cmd/gateway_setup.go index 0d270f1a..491bd680 100644 --- a/cmd/gateway_setup.go +++ b/cmd/gateway_setup.go @@ -211,6 +211,9 @@ func setupToolRegistry( // Exception: .goclaw/skills-store/ is allowed (skills may contain executable scripts). if execTool, ok := toolsReg.Get("exec"); ok { if et, ok := execTool.(*tools.ExecTool); ok { + // Apply global shell deny-group toggles before any request can arrive. + // Per-agent overrides via store.WithShellDenyGroups still win per-key. + et.SetGlobalShellDenyGroups(cfg.Tools.ShellDenyGroups) et.DenyPaths(dataDir, ".goclaw/") // Allow skills execution: master-tenant skills-store + all tenant-scoped skills-store dirs. et.AllowPathExemptions( diff --git a/internal/config/config_channels.go b/internal/config/config_channels.go index 0d04a672..a93fcdd2 100644 --- a/internal/config/config_channels.go +++ b/internal/config/config_channels.go @@ -369,8 +369,9 @@ type ToolsConfig struct { Allow []string `json:"allow,omitempty"` // global allow list (tool names or "group:xxx") Deny []string `json:"deny,omitempty"` // global deny list AlsoAllow []string `json:"alsoAllow,omitempty"` // additive: adds without removing existing - ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides - ExecApproval ExecApprovalCfg `json:"execApproval"` // exec command approval settings + ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` // per-provider overrides + ShellDenyGroups map[string]bool `json:"shellDenyGroups,omitempty"` // global shell deny-group toggles (group name -> denied); per-agent overrides win per-key + ExecApproval ExecApprovalCfg `json:"execApproval"` // exec command approval settings WebFetch WebFetchPolicyConfig `json:"web_fetch"` // domain policy for URL fetching Browser BrowserToolConfig `json:"browser"` RateLimitPerHour int `json:"rate_limit_per_hour,omitempty"` // max tool executions per hour per session (0 = disabled) diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 6726ea71..5611108b 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -43,6 +43,53 @@ type ExecTool struct { approvalMgr *ExecApprovalManager // nil = no approval needed agentID string // for approval request context secureCLIStore store.SecureCLIStore // nil = no credentialed exec + // globalDenyGroups holds global shell deny-group toggles from config.tools. + // Per-agent overrides from context (store.WithShellDenyGroups) win per-key. + // Updated at startup and via TopicConfigChanged pub/sub for runtime reload. + globalDenyGroups map[string]bool +} + +// SetGlobalShellDenyGroups replaces the global shell deny-group toggles. The +// caller's map is defensively copied so later mutations cannot leak into the +// tool's internal state. Passing nil or an empty map clears the global config +// (per-agent context overrides, if any, still apply on their own). +func (t *ExecTool) SetGlobalShellDenyGroups(groups map[string]bool) { + if len(groups) == 0 { + t.globalDenyGroups = nil + return + } + cp := make(map[string]bool, len(groups)) + for k, v := range groups { + cp[k] = v + } + t.globalDenyGroups = cp +} + +// effectiveDenyGroups merges the per-agent context override with the global +// config. Precedence: per-agent context (per-key) > global. When one side is +// empty, the other is returned directly (no allocation). +func (t *ExecTool) effectiveDenyGroups(ctx context.Context) map[string]bool { + agent := store.ShellDenyGroupsFromContext(ctx) + if len(t.globalDenyGroups) == 0 { + return agent + } + if len(agent) == 0 { + return t.globalDenyGroups + } + merged := make(map[string]bool, len(t.globalDenyGroups)+len(agent)) + for k, v := range t.globalDenyGroups { + merged[k] = v + } + for k, v := range agent { + merged[k] = v // agent wins per-key + } + return merged +} + +// EffectiveDenyGroupsForTest exposes effectiveDenyGroups for cross-package tests +// (e.g. cmd pub/sub regression). Not for production callers. +func (t *ExecTool) EffectiveDenyGroupsForTest(ctx context.Context) map[string]bool { + return t.effectiveDenyGroups(ctx) } // NewExecTool creates an exec tool that runs commands directly on the host. @@ -152,8 +199,9 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result { // Unicode-based pattern bypass while preserving functional command content. normalizedCommand := normalizeCommand(command) - // Resolve deny patterns: per-agent overrides from context, fallback to all defaults. - denyOverrides := store.ShellDenyGroupsFromContext(ctx) + // Resolve deny patterns: merge per-agent context overrides with global + // config (per-key agent precedence), fallback to all registry defaults. + denyOverrides := t.effectiveDenyGroups(ctx) groupPatterns := ResolveDenyPatterns(denyOverrides) // Also resolve package_install patterns separately for approval routing. diff --git a/internal/tools/shell_global_deny_groups_test.go b/internal/tools/shell_global_deny_groups_test.go new file mode 100644 index 00000000..3eff37e5 --- /dev/null +++ b/internal/tools/shell_global_deny_groups_test.go @@ -0,0 +1,100 @@ +package tools + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// TestExecToolEffectiveDenyGroups_GlobalOnly: with an empty agent context, +// effectiveDenyGroups must return the global toggles set via SetGlobalShellDenyGroups. +func TestExecToolEffectiveDenyGroups_GlobalOnly(t *testing.T) { + tool := NewExecTool("/tmp", false) + tool.SetGlobalShellDenyGroups(map[string]bool{"package_install": false}) + + got := tool.effectiveDenyGroups(context.Background()) + if v, ok := got["package_install"]; !ok || v != false { + t.Fatalf("expected global package_install=false in effective map, got %v", got) + } +} + +// TestExecToolEffectiveDenyGroups_AgentOverridesGlobal: per-agent context +// override wins per-key over global; non-overridden global keys must remain. +func TestExecToolEffectiveDenyGroups_AgentOverridesGlobal(t *testing.T) { + tool := NewExecTool("/tmp", false) + tool.SetGlobalShellDenyGroups(map[string]bool{ + "package_install": false, + "env_dump": false, + }) + + ctx := store.WithShellDenyGroups(context.Background(), map[string]bool{"package_install": true}) + + got := tool.effectiveDenyGroups(ctx) + if v, ok := got["package_install"]; !ok || v != true { + t.Errorf("expected agent override package_install=true, got %v (ok=%v)", v, ok) + } + if v, ok := got["env_dump"]; !ok || v != false { + t.Errorf("expected global env_dump=false preserved, got %v (ok=%v)", v, ok) + } +} + +// TestExecToolEffectiveDenyGroups_NilGlobalReturnsAgent: when no global +// is configured, return the agent map (preserving existing per-agent semantics). +func TestExecToolEffectiveDenyGroups_NilGlobalReturnsAgent(t *testing.T) { + tool := NewExecTool("/tmp", false) + + agent := map[string]bool{"foo": true} + ctx := store.WithShellDenyGroups(context.Background(), agent) + + got := tool.effectiveDenyGroups(ctx) + if v, ok := got["foo"]; !ok || v != true { + t.Fatalf("expected agent map returned when global empty, got %v", got) + } +} + +// TestExecToolEffectiveDenyGroups_EmptyAgentReturnsGlobal: when agent +// context has no overrides, return the global map. +func TestExecToolEffectiveDenyGroups_EmptyAgentReturnsGlobal(t *testing.T) { + tool := NewExecTool("/tmp", false) + tool.SetGlobalShellDenyGroups(map[string]bool{"foo": true}) + + got := tool.effectiveDenyGroups(context.Background()) + if v, ok := got["foo"]; !ok || v != true { + t.Fatalf("expected global map returned when agent empty, got %v", got) + } +} + +// TestExecToolSetGlobalShellDenyGroups_DefensiveCopy: mutating the caller's +// map after SetGlobalShellDenyGroups must not affect the tool's internal state. +func TestExecToolSetGlobalShellDenyGroups_DefensiveCopy(t *testing.T) { + tool := NewExecTool("/tmp", false) + + src := map[string]bool{"package_install": true} + tool.SetGlobalShellDenyGroups(src) + + // Mutate the caller's map AFTER passing it in. + src["package_install"] = false + src["env_dump"] = true + + got := tool.effectiveDenyGroups(context.Background()) + if v := got["package_install"]; v != true { + t.Errorf("expected internal copy to be insulated from caller mutation; got package_install=%v", v) + } + if _, ok := got["env_dump"]; ok { + t.Errorf("expected internal copy to be insulated from new caller-side keys; got %v", got) + } +} + +// TestExecToolSetGlobalShellDenyGroups_EmptyClears: passing an empty map +// must clear the internal state, not retain a stale copy. +func TestExecToolSetGlobalShellDenyGroups_EmptyClears(t *testing.T) { + tool := NewExecTool("/tmp", false) + tool.SetGlobalShellDenyGroups(map[string]bool{"foo": true}) + tool.SetGlobalShellDenyGroups(map[string]bool{}) + + got := tool.effectiveDenyGroups(context.Background()) + if len(got) != 0 { + t.Fatalf("expected cleared global to yield empty effective map, got %v", got) + } +}