From e715e43a0b71c76ba32f94863a874fd998555153 Mon Sep 17 00:00:00 2001 From: viettranx Date: Sun, 12 Apr 2026 19:56:15 +0700 Subject: [PATCH] fix(tools): Windows multi-drive workspace isolation (#836) - Add case-insensitive path comparison on Windows in isPathInside() - Add allowed_paths config for cross-drive access on Windows - Wire allowed_paths to read/write/edit/list file tools - Add POST /v1/agents/sync-workspace endpoint to propagate workspace changes - Add comprehensive tests for cross-drive, tenant isolation, symlink escape --- cmd/gateway_tools_wiring.go | 23 ++++++ internal/config/config.go | 1 + internal/http/agents.go | 63 ++++++++++++++ internal/tools/boundary_test.go | 128 ++++++++++++++++++++++++++++- internal/tools/edit.go | 11 ++- internal/tools/filesystem.go | 7 ++ internal/tools/filesystem_write.go | 11 ++- 7 files changed, 239 insertions(+), 5 deletions(-) diff --git a/cmd/gateway_tools_wiring.go b/cmd/gateway_tools_wiring.go index 80258ed5..aa0e856c 100644 --- a/cmd/gateway_tools_wiring.go +++ b/cmd/gateway_tools_wiring.go @@ -79,15 +79,38 @@ 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...) } } diff --git a/internal/config/config.go b/internal/config/config.go index b1639d41..a9cf93ae 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -111,6 +111,7 @@ type AgentsConfig struct { // AgentDefaults are default settings for all agents. type AgentDefaults struct { Workspace string `json:"workspace"` + AllowedPaths []string `json:"allowed_paths,omitempty"` // extra paths agents can access (cross-drive on Windows) RestrictToWorkspace bool `json:"restrict_to_workspace"` Provider string `json:"provider"` Model string `json:"model"` diff --git a/internal/http/agents.go b/internal/http/agents.go index 48bc3e67..8e96dc27 100644 --- a/internal/http/agents.go +++ b/internal/http/agents.go @@ -7,6 +7,8 @@ import ( "fmt" "log/slog" "net/http" + "path/filepath" + "strconv" "strings" "github.com/google/uuid" @@ -134,6 +136,8 @@ func (h *AgentsHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("GET /v1/agents/{id}", h.authMiddleware(h.handleGet)) mux.HandleFunc("PUT /v1/agents/{id}", h.adminMiddleware(h.handleUpdate)) mux.HandleFunc("DELETE /v1/agents/{id}", h.adminMiddleware(h.handleDelete)) + // Bulk operations (admin+) + mux.HandleFunc("POST /v1/agents/sync-workspace", h.adminMiddleware(h.handleSyncWorkspace)) // Sharing (admin+) mux.HandleFunc("GET /v1/agents/{id}/shares", h.authMiddleware(h.handleListShares)) mux.HandleFunc("POST /v1/agents/{id}/shares", h.adminMiddleware(h.handleShare)) @@ -529,3 +533,62 @@ func (h *AgentsHandler) handleDelete(w http.ResponseWriter, r *http.Request) { emitAudit(h.msgBus, r, "agent.deleted", "agent", id.String()) writeJSON(w, http.StatusOK, map[string]string{"ok": "true"}) } + +// handleSyncWorkspace updates all agents to use the new workspace root. +// POST /v1/agents/sync-workspace +// Body: {"workspace": "E:\\project\\workspace"} +// Requires admin role. +func (h *AgentsHandler) handleSyncWorkspace(w http.ResponseWriter, r *http.Request) { + tenantID := store.TenantIDFromContext(r.Context()) + + var req struct { + Workspace string `json:"workspace"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + writeError(w, http.StatusBadRequest, protocol.ErrInvalidRequest, "invalid JSON body") + return + } + if req.Workspace == "" { + writeError(w, http.StatusBadRequest, protocol.ErrInvalidRequest, "workspace is required") + return + } + // Path sanity check: reject traversal attempts + if strings.Contains(req.Workspace, "..") { + writeError(w, http.StatusBadRequest, protocol.ErrInvalidRequest, "workspace path cannot contain '..'") + return + } + + // List all agents (empty ownerID = all agents) + agents, err := h.agents.List(r.Context(), "") + if err != nil { + slog.Error("agents.sync_workspace: list failed", "error", err) + writeError(w, http.StatusInternalServerError, protocol.ErrInternal, "failed to list agents") + return + } + + // Update each agent's workspace to use the new root + newWorkspace := config.ExpandHome(req.Workspace) + var updated int + for _, ag := range agents { + // Skip agents from other tenants + if ag.TenantID != tenantID { + continue + } + // Build new workspace path: {newWorkspace}/{agentKey} + newPath := filepath.Join(newWorkspace, ag.AgentKey) + if ag.Workspace == newPath { + continue // already using correct path + } + // Use Update with map[string]any + if err := h.agents.Update(r.Context(), ag.ID, map[string]any{"workspace": newPath}); err != nil { + slog.Warn("agents.sync_workspace: update failed", "agent", ag.AgentKey, "error", err) + continue + } + h.emitCacheInvalidate(bus.CacheKindAgent, ag.AgentKey) + updated++ + } + + slog.Info("agents.sync_workspace: completed", "updated", updated, "total", len(agents), "workspace", newWorkspace) + emitAudit(h.msgBus, r, "agents.workspace_synced", "updated", strconv.Itoa(updated)) + writeJSON(w, http.StatusOK, map[string]any{"ok": true, "updated": updated}) +} diff --git a/internal/tools/boundary_test.go b/internal/tools/boundary_test.go index 9001e50c..42be520f 100644 --- a/internal/tools/boundary_test.go +++ b/internal/tools/boundary_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -326,7 +327,7 @@ func TestIsPathInside(t *testing.T) { }{ {"/a/b/c", "/a/b", true}, {"/a/b", "/a/b", true}, - {"/a/bc", "/a/b", false}, // not a child, just prefix match + {"/a/bc", "/a/b", false}, // not a child, just prefix match {"/a", "/a/b", false}, {"/x/y", "/a/b", false}, } @@ -337,3 +338,128 @@ func TestIsPathInside(t *testing.T) { } } } + +func TestIsPathInside_WindowsCaseInsensitive(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Windows-specific test") + } + tests := []struct { + child, parent string + want bool + }{ + {`C:\Workspace\file.txt`, `c:\workspace`, true}, // case mismatch + {`c:\workspace\file.txt`, `C:\Workspace`, true}, // reverse case + {`C:\WORKSPACE\SUB\FILE`, `c:\workspace`, true}, // all caps child + {`D:\other`, `C:\workspace`, false}, // different drive + {`C:\workspaceX\file.txt`, `C:\workspace`, false}, // prefix but not child + } + for _, tt := range tests { + got := isPathInside(tt.child, tt.parent) + if got != tt.want { + t.Errorf("isPathInside(%q, %q) = %v, want %v", tt.child, tt.parent, got, tt.want) + } + } +} + +func TestResolvePathWithAllowed_CrossDriveAccess(t *testing.T) { + // Simulates cross-drive access on Windows using separate temp directories + // (on Unix these are just separate paths, but the test logic is the same). + workspace := t.TempDir() + crossDrive := t.TempDir() // simulates a different drive (e.g., F:\ vs E:\) + + // Create files in both locations + if err := os.WriteFile(filepath.Join(workspace, "local.txt"), []byte("local"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(crossDrive, "remote.txt"), []byte("remote"), 0644); err != nil { + t.Fatal(err) + } + + // Without allowed paths: cross-drive should be BLOCKED + _, err := resolvePathWithAllowed(filepath.Join(crossDrive, "remote.txt"), workspace, true, nil) + if err == nil { + t.Fatal("expected error for cross-drive access without allowed paths") + } + + // With allowed paths: cross-drive should be ALLOWED + _, err = resolvePathWithAllowed(filepath.Join(crossDrive, "remote.txt"), workspace, true, []string{crossDrive}) + if err != nil { + t.Fatalf("expected success with cross-drive in allowed paths, got: %v", err) + } + + // Allowed paths should not allow escaping to arbitrary locations + outsideAll := t.TempDir() + if err := os.WriteFile(filepath.Join(outsideAll, "secret.txt"), []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + _, err = resolvePathWithAllowed(filepath.Join(outsideAll, "secret.txt"), workspace, true, []string{crossDrive}) + if err == nil { + t.Fatal("expected error for path outside both workspace and allowed paths") + } +} + +func TestResolvePathWithAllowed_TenantIsolation(t *testing.T) { + // Ensure that allowed paths cannot be used to escape tenant boundaries. + // Scenario: tenant A's workspace, tenant B's workspace, and a shared allowed path. + tenantA := t.TempDir() + tenantB := t.TempDir() + sharedSkills := t.TempDir() + + // Create files + if err := os.WriteFile(filepath.Join(tenantA, "a.txt"), []byte("A"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(tenantB, "b.txt"), []byte("B"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(sharedSkills, "skill.md"), []byte("skill"), 0644); err != nil { + t.Fatal(err) + } + + // Tenant A can access their workspace + _, err := resolvePathWithAllowed(filepath.Join(tenantA, "a.txt"), tenantA, true, []string{sharedSkills}) + if err != nil { + t.Fatalf("tenant A should access own workspace: %v", err) + } + + // Tenant A can access shared skills + _, err = resolvePathWithAllowed(filepath.Join(sharedSkills, "skill.md"), tenantA, true, []string{sharedSkills}) + if err != nil { + t.Fatalf("tenant A should access shared skills: %v", err) + } + + // Tenant A CANNOT access tenant B's workspace (even with shared skills allowed) + _, err = resolvePathWithAllowed(filepath.Join(tenantB, "b.txt"), tenantA, true, []string{sharedSkills}) + if err == nil { + t.Fatal("tenant A should NOT access tenant B's workspace") + } + + // Verify the error message is about access denied + if !strings.Contains(err.Error(), "access denied") { + t.Errorf("expected 'access denied' error, got: %v", err) + } +} + +func TestResolvePathWithAllowed_SymlinkEscapeBlocked(t *testing.T) { + // Ensure symlinks in allowed paths cannot be used to escape boundaries. + workspace := t.TempDir() + allowedDir := t.TempDir() + secretDir := t.TempDir() + + // Create a secret file outside both workspace and allowed + if err := os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + + // Create a symlink in allowed dir pointing to secret dir + evilLink := filepath.Join(allowedDir, "escape") + if err := os.Symlink(secretDir, evilLink); err != nil { + t.Skip("cannot create symlinks (permissions or OS limitation)") + } + + // Attempt to access secret file via symlink escape + _, err := resolvePathWithAllowed(filepath.Join(evilLink, "secret.txt"), workspace, true, []string{allowedDir}) + if err == nil { + t.Fatal("expected error for symlink escape attempt") + } +} diff --git a/internal/tools/edit.go b/internal/tools/edit.go index 17a696d5..f7370066 100644 --- a/internal/tools/edit.go +++ b/internal/tools/edit.go @@ -16,7 +16,8 @@ import ( type EditTool struct { workspace string restrict bool - deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) + allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) + deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) sandboxMgr sandbox.Manager contextFileIntc *ContextFileInterceptor memIntc *MemoryInterceptor @@ -26,6 +27,12 @@ type EditTool struct { func (t *EditTool) SetVaultInterceptor(v *VaultInterceptor) { t.vaultIntc = v } +// AllowPaths adds extra path prefixes that edit is allowed to access +// even when restrict_to_workspace is true (e.g. cross-drive on Windows). +func (t *EditTool) AllowPaths(prefixes ...string) { + t.allowedPrefixes = append(t.allowedPrefixes, prefixes...) +} + // DenyPaths adds path prefixes that edit must reject. func (t *EditTool) DenyPaths(prefixes ...string) { t.deniedPrefixes = append(t.deniedPrefixes, prefixes...) @@ -163,7 +170,7 @@ func (t *EditTool) Execute(ctx context.Context, args map[string]any) *Result { if workspace == "" { workspace = t.workspace } - allowed := allowedWithTeamWorkspace(ctx, nil) + allowed := allowedWithTeamWorkspace(ctx, t.allowedPrefixes) resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error()) diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index 4653ba0a..3c4e8060 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -6,6 +6,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime" "strings" "github.com/nextlevelbuilder/goclaw/internal/bootstrap" @@ -488,7 +489,13 @@ func resolvePath(path, workspace string, restrict bool) (string, error) { } // isPathInside checks whether child is inside or equal to parent directory. +// On Windows, comparison is case-insensitive since NTFS paths are case-insensitive. func isPathInside(child, parent string) bool { + // Windows paths are case-insensitive; normalize to lowercase for comparison. + if runtime.GOOS == "windows" { + child = strings.ToLower(child) + parent = strings.ToLower(parent) + } if child == parent { return true } diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index 98fc027a..c902006d 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -15,7 +15,8 @@ import ( type WriteFileTool struct { workspace string restrict bool - deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) + allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) + deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) sandboxMgr sandbox.Manager contextFileIntc *ContextFileInterceptor // nil = no virtual FS routing memIntc *MemoryInterceptor // nil = no memory routing @@ -24,6 +25,12 @@ type WriteFileTool struct { vaultIntc *VaultInterceptor // nil = no vault registration } +// AllowPaths adds extra path prefixes that write_file is allowed to access +// even when restrict_to_workspace is true (e.g. cross-drive on Windows). +func (t *WriteFileTool) AllowPaths(prefixes ...string) { + t.allowedPrefixes = append(t.allowedPrefixes, prefixes...) +} + // DenyPaths adds path prefixes that write_file must reject. func (t *WriteFileTool) DenyPaths(prefixes ...string) { t.deniedPrefixes = append(t.deniedPrefixes, prefixes...) @@ -162,7 +169,7 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul if workspace == "" { workspace = t.workspace } - allowed := allowedWithTeamWorkspace(ctx, nil) + allowed := allowedWithTeamWorkspace(ctx, t.allowedPrefixes) resolved, err := resolvePathWithAllowed(path, workspace, effectiveRestrict(ctx, t.restrict), allowed) if err != nil { return ErrorResult(err.Error())