mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 12:16:54 +00:00
fix(config): MCP env: resolution, channel field filter, orphan provider event, workspace fallback (#348, #297, #295, #431)
- manager.go: add resolveEnvVars() to expand env:VARNAME in MCP headers (copy-safe)
- manager_env_test.go: 3 test cases for env resolution
- validate.go: add "name" to channelInstanceAllowedFields
- validate_test.go: test channel instance name field retention
- bus/types.go: add TopicAgentDeleted event + AgentDeletedPayload
- agents_delete.go: emit agent:deleted event with provider name for async cleanup
- gateway.go: log-only subscriber for orphaned provider warning (no auto-delete — FK safety)
- resolver.go: fallback to deps.Workspace for master tenant agents with empty ag.Workspace
(fixes filesystem escape where filepath.Join("","system") resolved to /system)
This commit is contained in:
1 parent
8eb4ce6d6f
commit
bdfc0fadfb
7 files changed
+98
-3
No files matched your search
@@ -240,6 +240,10 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc {
|
||||
workspace = config.TenantWorkspace(deps.Workspace, ag.TenantID, tenantSlug)
|
||||
}
|
||||
}
|
||||
// Fallback to global workspace if per-agent workspace is empty
|
||||
if workspace == "" && deps.Workspace != "" {
|
||||
workspace = deps.Workspace
|
||||
}
|
||||
if workspace != "" {
|
||||
if err := os.MkdirAll(workspace, 0755); err != nil {
|
||||
slog.Warn("failed to create agent workspace directory", "workspace", workspace, "agent", agentKey, "error", err)
|
||||
|
||||
@@ -99,6 +99,7 @@ const (
|
||||
TopicSystemConfigChanged = "system_config:changed"
|
||||
TopicPairingRevoked = "pairing:revoked"
|
||||
TopicAgentStatusChanged = "agent:status_changed"
|
||||
TopicAgentDeleted = "agent:deleted"
|
||||
)
|
||||
|
||||
// EventPairingRevoked is the event name broadcast when a paired device is revoked.
|
||||
@@ -120,6 +121,13 @@ type AgentStatusChangedPayload struct {
|
||||
NewStatus string `json:"new_status"`
|
||||
}
|
||||
|
||||
// AgentDeletedPayload carries agent deletion info for async cleanup (e.g. orphaned provider removal).
|
||||
type AgentDeletedPayload struct {
|
||||
AgentKey string `json:"agent_key"`
|
||||
Provider string `json:"provider,omitempty"` // provider name for orphan cleanup
|
||||
TenantID uuid.UUID `json:"tenant_id,omitempty"`
|
||||
}
|
||||
|
||||
// AuditEventPayload carries audit log data emitted by handlers.
|
||||
// A single subscriber persists these to the activity_logs table.
|
||||
type AuditEventPayload struct {
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"fmt"
|
||||
"os"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/bus"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/config"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/gateway"
|
||||
"github.com/nextlevelbuilder/goclaw/internal/i18n"
|
||||
@@ -54,6 +55,18 @@ func (m *AgentsMethods) handleDelete(ctx context.Context, client *gateway.Client
|
||||
m.agents.InvalidateAgent(params.AgentID)
|
||||
m.agents.Remove(params.AgentID)
|
||||
|
||||
// Emit agent:deleted event for async cleanup (e.g. orphaned provider removal)
|
||||
if m.eventBus != nil {
|
||||
m.eventBus.Broadcast(bus.Event{
|
||||
Name: bus.TopicAgentDeleted,
|
||||
Payload: bus.AgentDeletedPayload{
|
||||
AgentKey: params.AgentID,
|
||||
Provider: ag.Provider,
|
||||
TenantID: store.TenantIDFromContext(ctx),
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
// Best-effort delete workspace
|
||||
if params.DeleteFiles && ag.Workspace != "" {
|
||||
os.RemoveAll(ag.Workspace)
|
||||
|
||||
@@ -64,7 +64,7 @@ var mcpServerAllowedFields = map[string]bool{
|
||||
}
|
||||
|
||||
var channelInstanceAllowedFields = map[string]bool{
|
||||
"channel_type": true, "credentials": true, "agent_id": true,
|
||||
"name": true, "channel_type": true, "credentials": true, "agent_id": true,
|
||||
"enabled": true, "group_policy": true, "allow_from": true,
|
||||
"metadata": true, "webhook_secret": true, "config": true,
|
||||
"display_name": true,
|
||||
|
||||
@@ -4,6 +4,20 @@ import (
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestFilterAllowedKeys_ChannelInstance_IncludesName(t *testing.T) {
|
||||
input := map[string]any{"name": "my-bot", "channel_type": "telegram", "evil_field": "hack"}
|
||||
result := filterAllowedKeys(input, channelInstanceAllowedFields)
|
||||
if result["name"] != "my-bot" {
|
||||
t.Error("expected 'name' to be retained")
|
||||
}
|
||||
if _, ok := result["evil_field"]; ok {
|
||||
t.Error("expected 'evil_field' to be stripped")
|
||||
}
|
||||
if result["channel_type"] != "telegram" {
|
||||
t.Error("expected 'channel_type' to be retained")
|
||||
}
|
||||
}
|
||||
|
||||
func TestFilterAllowedKeys(t *testing.T) {
|
||||
allowed := map[string]bool{"name": true, "status": true}
|
||||
|
||||
|
||||
+17
-2
@@ -5,6 +5,8 @@ import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"os"
|
||||
"strings"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
@@ -141,7 +143,7 @@ func (m *Manager) Start(ctx context.Context) error {
|
||||
continue
|
||||
}
|
||||
|
||||
if err := m.connectServer(ctx, name, cfg.Transport, cfg.Command, cfg.Args, cfg.Env, cfg.URL, cfg.Headers, cfg.ToolPrefix, cfg.TimeoutSec); err != nil {
|
||||
if err := m.connectServer(ctx, name, cfg.Transport, cfg.Command, cfg.Args, cfg.Env, cfg.URL, resolveEnvVars(cfg.Headers), cfg.ToolPrefix, cfg.TimeoutSec); err != nil {
|
||||
slog.Warn("mcp.server.connect_failed", "server", name, "error", err)
|
||||
errs = append(errs, fmt.Sprintf("%s: %v", name, err))
|
||||
}
|
||||
@@ -184,7 +186,7 @@ func (m *Manager) resolveServerCredentials(ctx context.Context, info store.MCPAc
|
||||
|
||||
args := jsonBytesToStringSlice(srv.Args)
|
||||
env := jsonBytesToStringMap(srv.Env)
|
||||
headers := jsonBytesToStringMap(srv.Headers)
|
||||
headers := resolveEnvVars(jsonBytesToStringMap(srv.Headers))
|
||||
|
||||
// Inject APIKey into headers if present (bug fix: was never passed to connections)
|
||||
if srv.APIKey != "" && headers["Authorization"] == "" {
|
||||
@@ -508,6 +510,19 @@ func (m *Manager) ServerStatus() []ServerStatus {
|
||||
return statuses
|
||||
}
|
||||
|
||||
// resolveEnvVars returns a copy of m with "env:VARNAME" values resolved to os.Getenv("VARNAME").
|
||||
func resolveEnvVars(m map[string]string) map[string]string {
|
||||
out := make(map[string]string, len(m))
|
||||
for k, v := range m {
|
||||
if after, ok := strings.CutPrefix(v, "env:"); ok {
|
||||
out[k] = os.Getenv(after)
|
||||
} else {
|
||||
out[k] = v
|
||||
}
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// requireUserCreds checks if an MCP server's settings mandate per-user credentials.
|
||||
func requireUserCreds(settings json.RawMessage) bool {
|
||||
if len(settings) == 0 {
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestResolveEnvVars(t *testing.T) {
|
||||
t.Setenv("TEST_MCP_TOKEN", "secret123")
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
input map[string]string
|
||||
want map[string]string
|
||||
}{
|
||||
{
|
||||
"resolves env prefix",
|
||||
map[string]string{"Authorization": "env:TEST_MCP_TOKEN", "X-Custom": "literal"},
|
||||
map[string]string{"Authorization": "secret123", "X-Custom": "literal"},
|
||||
},
|
||||
{
|
||||
"missing env var resolves to empty",
|
||||
map[string]string{"X-Missing": "env:NONEXISTENT_VAR_XYZ"},
|
||||
map[string]string{"X-Missing": ""},
|
||||
},
|
||||
{
|
||||
"nil map",
|
||||
nil,
|
||||
map[string]string{},
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got := resolveEnvVars(tt.input)
|
||||
for k, v := range tt.want {
|
||||
if got[k] != v {
|
||||
t.Errorf("key %q: got %q, want %q", k, got[k], v)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user