mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-19 10:15:01 +00:00
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
This commit is contained in:
1 parent
304a94e3b6
commit
e715e43a0b
7 files changed
+239
-5
No files matched your search
@@ -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...)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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"`
|
||||
|
||||
@@ -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})
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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())
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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())
|
||||
|
||||
Reference in new issue
Block a user