mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 12:16:54 +00:00
fix(gateway): re-apply allowed_paths to filesystem tools after system_configs overlay (#1274)
setupToolRegistry wires the filesystem tools' AllowPaths from the config/JSON5 default before ApplySystemConfigs overlays system_configs['allowed_paths'], so DB-configured allowed paths never reached read_file / list_files / write_file / edit / send_file. Only the rate limiter was re-applied after the overlay (#1111); the AllowPaths analogue was missing, so agents were denied access to configured shared directories outside their workspace even though the DB value was present (visible as a read_file "access denied" log whose allowedPrefixes omit the configured path). Extract the user-allowed-path application into applyUserAllowedPaths and re-run it from runGateway after the overlay, mirroring the rate-limiter re-apply. The helper is idempotent (AllowPaths is additive and the prefix check is membership-based), so the initial wiring call plus the re-apply is safe. Test: cmd/gateway_tools_wiring_test.go asserts a path outside the workspace is denied before the grant and readable after, that an unrelated path stays denied, that repeated application is safe, and that an empty list is a no-op.
This commit is contained in:
1 parent
12a0168271
commit
ce0472b580
3 files changed
+138
-24
No files matched your search
@@ -240,6 +240,17 @@ func runGateway() {
|
||||
} else {
|
||||
toolsReg.SetRateLimiter(nil)
|
||||
}
|
||||
|
||||
// Re-apply user-configured allowed paths for the same reason as the rate
|
||||
// limiter above: setupToolRegistry wired the filesystem tools' AllowPaths
|
||||
// from the JSON5 default before ApplySystemConfigs overlaid
|
||||
// system_configs['allowed_paths'], so DB-driven paths never reached the tools.
|
||||
// Re-run now that cfg reflects the DB value. Safe: server has not started, no
|
||||
// in-flight tool calls.
|
||||
if paths := cfg.Agents.Defaults.AllowedPaths; len(paths) > 0 {
|
||||
applyUserAllowedPaths(toolsReg, paths)
|
||||
slog.Info("filesystem allowed paths reapplied from system_configs", "paths", len(paths))
|
||||
}
|
||||
setupMemoryEmbeddings(pgStores, providerRegistry)
|
||||
usageCapSvc := usagecaps.NewService(pgStores.UsageCaps, pgStores.Providers)
|
||||
|
||||
|
||||
+41
-24
@@ -97,47 +97,31 @@ func wireExtraTools(
|
||||
if pgStores.Skills != nil {
|
||||
skillsAllowPaths = append(skillsAllowPaths, pgStores.Skills.Dirs()...)
|
||||
}
|
||||
// Expand user-configured allowed paths (for cross-drive access on Windows).
|
||||
// These paths are validated per-request in resolvePath for tenant isolation.
|
||||
var userAllowPaths []string
|
||||
for _, p := range agentCfg.AllowedPaths {
|
||||
expanded := config.ExpandHome(p)
|
||||
if expanded != "" {
|
||||
userAllowPaths = append(userAllowPaths, expanded)
|
||||
}
|
||||
}
|
||||
|
||||
if readTool, ok := toolsReg.Get("read_file"); ok {
|
||||
if pa, ok := readTool.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(skillsAllowPaths...)
|
||||
pa.AllowPaths(filepath.Join(dataDir, "cli-workspaces"))
|
||||
pa.AllowPaths(userAllowPaths...)
|
||||
}
|
||||
}
|
||||
if listTool, ok := toolsReg.Get("list_files"); ok {
|
||||
if pa, ok := listTool.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(skillsAllowPaths...)
|
||||
pa.AllowPaths(userAllowPaths...)
|
||||
}
|
||||
}
|
||||
// Write and edit tools also get user-configured allowed paths for cross-drive access.
|
||||
if writeTool, ok := toolsReg.Get("write_file"); ok {
|
||||
if pa, ok := writeTool.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(userAllowPaths...)
|
||||
}
|
||||
}
|
||||
if editTool, ok := toolsReg.Get("edit"); ok {
|
||||
if pa, ok := editTool.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(userAllowPaths...)
|
||||
}
|
||||
}
|
||||
if sendFileTool, ok := toolsReg.Get("send_file"); ok {
|
||||
if pa, ok := sendFileTool.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(skillsAllowPaths...)
|
||||
pa.AllowPaths(userAllowPaths...)
|
||||
}
|
||||
}
|
||||
|
||||
// User-configured allowed paths (config agents.defaults.allowed_paths, for
|
||||
// cross-drive access on Windows and shared dirs outside the workspace).
|
||||
// Applied via a helper so cmd/gateway.go can re-apply it after
|
||||
// ApplySystemConfigs overlays system_configs['allowed_paths'] — see
|
||||
// applyUserAllowedPaths. Paths are validated per-request in resolvePath for
|
||||
// tenant isolation.
|
||||
applyUserAllowedPaths(toolsReg, agentCfg.AllowedPaths)
|
||||
|
||||
// Memory tools are PG-backed; always available.
|
||||
hasMemory = true
|
||||
|
||||
@@ -162,6 +146,39 @@ func wireExtraTools(
|
||||
return heartbeatTool, hasMemory
|
||||
}
|
||||
|
||||
// fsAllowPathTools are the filesystem tools that honour user-configured allowed
|
||||
// paths beyond the agent workspace (config agents.defaults.allowed_paths).
|
||||
var fsAllowPathTools = []string{"read_file", "list_files", "write_file", "edit", "send_file"}
|
||||
|
||||
// applyUserAllowedPaths grants the user-configured allowed paths to the
|
||||
// filesystem tools. ExpandHome resolves a leading "~".
|
||||
//
|
||||
// Called twice during startup: once while tools are wired (from config.json),
|
||||
// and again from cmd/gateway.go after ApplySystemConfigs overlays
|
||||
// system_configs['allowed_paths']. Tool wiring runs before that overlay, so
|
||||
// without the re-apply DB-driven allowed paths never reach the tools — the
|
||||
// AllowPaths analogue of the rate-limiter re-apply (#1111). Safe to call
|
||||
// repeatedly: AllowPaths is additive and the prefix check is membership-based,
|
||||
// so a duplicated prefix is harmless.
|
||||
func applyUserAllowedPaths(toolsReg *tools.Registry, allowedPaths []string) {
|
||||
var paths []string
|
||||
for _, p := range allowedPaths {
|
||||
if expanded := config.ExpandHome(p); expanded != "" {
|
||||
paths = append(paths, expanded)
|
||||
}
|
||||
}
|
||||
if len(paths) == 0 {
|
||||
return
|
||||
}
|
||||
for _, name := range fsAllowPathTools {
|
||||
if t, ok := toolsReg.Get(name); ok {
|
||||
if pa, ok := t.(tools.PathAllowable); ok {
|
||||
pa.AllowPaths(paths...)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// wireWorkstationTools registers workstation_exec and claude_remote tools (Standard edition only).
|
||||
// Phase 6: wires the real AllowlistChecker permission check replacing the deny-all sentinel.
|
||||
// Phase 7: wires the activity sink for exec audit logging.
|
||||
|
||||
@@ -0,0 +1,86 @@
|
||||
package cmd
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"github.com/nextlevelbuilder/goclaw/internal/tools"
|
||||
)
|
||||
|
||||
// applyUserAllowedPaths must grant the filesystem tools access to paths outside
|
||||
// the agent workspace. cmd/gateway.go relies on this to re-apply
|
||||
// system_configs['allowed_paths'] after the overlay (tool wiring runs first) —
|
||||
// the AllowPaths analogue of the rate-limiter re-apply in #1111.
|
||||
func TestApplyUserAllowedPaths_GrantsExternalPathToReadFile(t *testing.T) {
|
||||
ws := t.TempDir()
|
||||
ext := t.TempDir()
|
||||
extFile := filepath.Join(ext, "kb.md")
|
||||
if err := os.WriteFile(extFile, []byte("knowledge"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
reg := tools.NewRegistry()
|
||||
reg.Register(tools.NewReadFileTool(ws, true))
|
||||
read, ok := reg.Get("read_file")
|
||||
if !ok {
|
||||
t.Fatal("read_file not registered")
|
||||
}
|
||||
|
||||
// Before granting: a path outside the workspace is denied.
|
||||
if res := read.Execute(context.Background(), map[string]any{"path": extFile}); !res.IsError {
|
||||
t.Fatalf("expected access denied before allow, got: %s", res.ForLLM)
|
||||
}
|
||||
|
||||
applyUserAllowedPaths(reg, []string{ext})
|
||||
|
||||
// After granting: the external path is readable.
|
||||
if res := read.Execute(context.Background(), map[string]any{"path": extFile}); res.IsError {
|
||||
t.Fatalf("expected success after allow, got error: %s", res.ForLLM)
|
||||
}
|
||||
|
||||
// The grant is scoped — an unrelated external path stays denied.
|
||||
other := t.TempDir()
|
||||
otherFile := filepath.Join(other, "secret.md")
|
||||
if err := os.WriteFile(otherFile, []byte("nope"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if res := read.Execute(context.Background(), map[string]any{"path": otherFile}); !res.IsError {
|
||||
t.Fatalf("expected unrelated external path to stay denied, got: %s", res.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
// Applying twice (initial wiring + the gateway re-apply after ApplySystemConfigs)
|
||||
// must keep the grant working and must not error.
|
||||
func TestApplyUserAllowedPaths_RepeatedApplyIsSafe(t *testing.T) {
|
||||
ws := t.TempDir()
|
||||
ext := t.TempDir()
|
||||
extFile := filepath.Join(ext, "kb.md")
|
||||
if err := os.WriteFile(extFile, []byte("knowledge"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
reg := tools.NewRegistry()
|
||||
reg.Register(tools.NewReadFileTool(ws, true))
|
||||
read, ok := reg.Get("read_file")
|
||||
if !ok {
|
||||
t.Fatal("read_file not registered")
|
||||
}
|
||||
|
||||
applyUserAllowedPaths(reg, []string{ext}) // initial wiring (from config.json)
|
||||
applyUserAllowedPaths(reg, []string{ext}) // re-apply after system_configs overlay
|
||||
|
||||
if res := read.Execute(context.Background(), map[string]any{"path": extFile}); res.IsError {
|
||||
t.Fatalf("expected success after repeated allow, got error: %s", res.ForLLM)
|
||||
}
|
||||
}
|
||||
|
||||
// An empty allow list is a no-op and must not panic (the common case when no
|
||||
// allowed_paths are configured).
|
||||
func TestApplyUserAllowedPaths_EmptyIsNoop(t *testing.T) {
|
||||
reg := tools.NewRegistry()
|
||||
reg.Register(tools.NewReadFileTool(t.TempDir(), true))
|
||||
applyUserAllowedPaths(reg, nil)
|
||||
applyUserAllowedPaths(reg, []string{})
|
||||
}
|
||||
Reference in new issue
Block a user