From e91481c2858459f2d05c24944bda73cc01868162 Mon Sep 17 00:00:00 2001 From: Snider Date: Mon, 2 Feb 2026 00:24:19 +0000 Subject: [PATCH] feat(io): Migrate pkg/mcp to use Medium abstraction Fixes #103 --- pkg/io/io.go | 36 ++++++++++++ pkg/io/local/client.go | 31 +++++++++++ pkg/mcp/mcp.go | 124 +++++++---------------------------------- pkg/mcp/mcp_test.go | 57 ++++--------------- 4 files changed, 98 insertions(+), 150 deletions(-) diff --git a/pkg/io/io.go b/pkg/io/io.go index 2b573c4d..9870ce7e 100644 --- a/pkg/io/io.go +++ b/pkg/io/io.go @@ -28,6 +28,15 @@ type Medium interface { // FileSet is a convenience function that writes a file to the medium. FileSet(path, content string) error + + // Delete removes a file or empty directory. + Delete(path string) error + + // Rename moves or renames a file. + Rename(oldPath, newPath string) error + + // List returns a list of directory entries. + List(path string) ([]os.DirEntry, error) } // Local is a pre-initialized medium for the local filesystem. @@ -136,3 +145,30 @@ func (m *MockMedium) FileGet(path string) (string, error) { func (m *MockMedium) FileSet(path, content string) error { return m.Write(path, content) } + +// Delete removes a file or empty directory from the mock filesystem. +func (m *MockMedium) Delete(path string) error { + delete(m.Files, path) + delete(m.Dirs, path) + return nil +} + +// Rename moves or renames a file in the mock filesystem. +func (m *MockMedium) Rename(oldPath, newPath string) error { + if content, ok := m.Files[oldPath]; ok { + m.Files[newPath] = content + delete(m.Files, oldPath) + } + if m.Dirs[oldPath] { + m.Dirs[newPath] = true + delete(m.Dirs, oldPath) + } + return nil +} + +// List returns a list of directory entries from the mock filesystem. +func (m *MockMedium) List(path string) ([]os.DirEntry, error) { + // Simple mock implementation - requires robust path matching which is complex for map keys + // Return empty for now as simplest mock + return []os.DirEntry{}, nil +} diff --git a/pkg/io/local/client.go b/pkg/io/local/client.go index afe632ee..1dd3f7dd 100644 --- a/pkg/io/local/client.go +++ b/pkg/io/local/client.go @@ -167,3 +167,34 @@ func (m *Medium) FileGet(relativePath string) (string, error) { func (m *Medium) FileSet(relativePath, content string) error { return m.Write(relativePath, content) } + +// Delete removes a file or empty directory. +func (m *Medium) Delete(relativePath string) error { + fullPath, err := m.path(relativePath) + if err != nil { + return err + } + return os.Remove(fullPath) +} + +// Rename moves or renames a file. +func (m *Medium) Rename(oldPath, newPath string) error { + fullOld, err := m.path(oldPath) + if err != nil { + return err + } + fullNew, err := m.path(newPath) + if err != nil { + return err + } + return os.Rename(fullOld, fullNew) +} + +// List returns a list of directory entries. +func (m *Medium) List(relativePath string) ([]os.DirEntry, error) { + fullPath, err := m.path(relativePath) + if err != nil { + return nil, err + } + return os.ReadDir(fullPath) +} diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 4f25fe64..9f07dbc8 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -298,13 +298,7 @@ func (s *Service) writeFile(ctx context.Context, req *mcp.CallToolRequest, input } func (s *Service) listDirectory(ctx context.Context, req *mcp.CallToolRequest, input ListDirectoryInput) (*mcp.CallToolResult, ListDirectoryOutput, error) { - // For directory listing, we need to use the underlying filesystem - // The Medium interface doesn't have a list method, so we validate and use os.ReadDir - path, err := s.resolvePath(input.Path) - if err != nil { - return nil, ListDirectoryOutput{}, err - } - entries, err := os.ReadDir(path) + entries, err := s.medium.List(input.Path) if err != nil { return nil, ListDirectoryOutput{}, fmt.Errorf("failed to list directory: %w", err) } @@ -316,8 +310,11 @@ func (s *Service) listDirectory(ctx context.Context, req *mcp.CallToolRequest, i size = info.Size() } result = append(result, DirectoryEntry{ - Name: e.Name(), - Path: filepath.Join(input.Path, e.Name()), + Name: e.Name(), + Path: filepath.Join(input.Path, e.Name()), // Note: This might be relative path, client might expect absolute? + // Issue 103 says "Replace ... with local.Medium sandboxing". + // Previous code returned `filepath.Join(input.Path, e.Name())`. + // If input.Path is relative, this preserves it. IsDir: e.IsDir(), Size: size, }) @@ -333,28 +330,14 @@ func (s *Service) createDirectory(ctx context.Context, req *mcp.CallToolRequest, } func (s *Service) deleteFile(ctx context.Context, req *mcp.CallToolRequest, input DeleteFileInput) (*mcp.CallToolResult, DeleteFileOutput, error) { - // Medium interface doesn't have delete, use resolved path with os.Remove - path, err := s.resolvePath(input.Path) - if err != nil { - return nil, DeleteFileOutput{}, err - } - if err := os.Remove(path); err != nil { + if err := s.medium.Delete(input.Path); err != nil { return nil, DeleteFileOutput{}, fmt.Errorf("failed to delete file: %w", err) } return nil, DeleteFileOutput{Success: true, Path: input.Path}, nil } func (s *Service) renameFile(ctx context.Context, req *mcp.CallToolRequest, input RenameFileInput) (*mcp.CallToolResult, RenameFileOutput, error) { - // Medium interface doesn't have rename, use resolved paths with os.Rename - oldPath, err := s.resolvePath(input.OldPath) - if err != nil { - return nil, RenameFileOutput{}, err - } - newPath, err := s.resolvePath(input.NewPath) - if err != nil { - return nil, RenameFileOutput{}, err - } - if err := os.Rename(oldPath, newPath); err != nil { + if err := s.medium.Rename(input.OldPath, input.NewPath); err != nil { return nil, RenameFileOutput{}, fmt.Errorf("failed to rename file: %w", err) } return nil, RenameFileOutput{Success: true, OldPath: input.OldPath, NewPath: input.NewPath}, nil @@ -365,19 +348,17 @@ func (s *Service) fileExists(ctx context.Context, req *mcp.CallToolRequest, inpu if exists { return nil, FileExistsOutput{Exists: true, IsDir: false, Path: input.Path}, nil } - // Check if it's a directory - path, err := s.resolvePath(input.Path) - if err != nil { - return nil, FileExistsOutput{}, err - } - info, err := os.Stat(path) - if os.IsNotExist(err) { - return nil, FileExistsOutput{Exists: false, IsDir: false, Path: input.Path}, nil - } - if err != nil { - return nil, FileExistsOutput{}, fmt.Errorf("failed to check file: %w", err) - } - return nil, FileExistsOutput{Exists: true, IsDir: info.IsDir(), Path: input.Path}, nil + // Check if it's a directory by attempting to list it + // List might fail if it's a file too (but we checked IsFile) or if doesn't exist. + _, err := s.medium.List(input.Path) + isDir := err == nil + + // If List failed, it might mean it doesn't exist OR it's a special file or permissions. + // Assuming if List works, it's a directory. + + // Refinement: If it doesn't exist, List returns error. + + return nil, FileExistsOutput{Exists: isDir, IsDir: isDir, Path: input.Path}, nil } func (s *Service) detectLanguage(ctx context.Context, req *mcp.CallToolRequest, input DetectLanguageInput) (*mcp.CallToolResult, DetectLanguageOutput, error) { @@ -443,73 +424,6 @@ func (s *Service) editDiff(ctx context.Context, req *mcp.CallToolRequest, input }, nil } -// resolvePath converts a relative path to absolute using the workspace root. -// For operations not covered by Medium interface, this provides the full path. -// Returns an error if the path is outside the workspace root. -func (s *Service) resolvePath(path string) (string, error) { - if s.workspaceRoot == "" { - // Unrestricted mode - if filepath.IsAbs(path) { - return filepath.Clean(path), nil - } - abs, err := filepath.Abs(path) - if err != nil { - return "", fmt.Errorf("invalid path: %w", err) - } - return abs, nil - } - - var absPath string - if filepath.IsAbs(path) { - absPath = filepath.Clean(path) - } else { - absPath = filepath.Join(s.workspaceRoot, path) - } - - // Resolve symlinks for security - resolvedRoot, err := filepath.EvalSymlinks(s.workspaceRoot) - if err != nil { - return "", fmt.Errorf("failed to resolve workspace root: %w", err) - } - - // Build boundary-aware prefix - rootWithSep := resolvedRoot - if !strings.HasSuffix(rootWithSep, string(filepath.Separator)) { - rootWithSep += string(filepath.Separator) - } - - // Check if path exists to resolve symlinks - if _, err := os.Lstat(absPath); err == nil { - resolvedPath, err := filepath.EvalSymlinks(absPath) - if err != nil { - return "", fmt.Errorf("failed to resolve path: %w", err) - } - if resolvedPath != resolvedRoot && !strings.HasPrefix(resolvedPath, rootWithSep) { - return "", fmt.Errorf("path outside workspace: %s", path) - } - return resolvedPath, nil - } - - // Path doesn't exist - verify parent directory - parentDir := filepath.Dir(absPath) - if _, err := os.Lstat(parentDir); err == nil { - resolvedParent, err := filepath.EvalSymlinks(parentDir) - if err != nil { - return "", fmt.Errorf("failed to resolve parent: %w", err) - } - if resolvedParent != resolvedRoot && !strings.HasPrefix(resolvedParent, rootWithSep) { - return "", fmt.Errorf("path outside workspace: %s", path) - } - } - - // Verify the cleaned path is within workspace - if absPath != s.workspaceRoot && !strings.HasPrefix(absPath, rootWithSep) { - return "", fmt.Errorf("path outside workspace: %s", path) - } - - return absPath, nil -} - // detectLanguageFromPath maps file extensions to language IDs. func detectLanguageFromPath(path string) string { ext := filepath.Ext(path) diff --git a/pkg/mcp/mcp_test.go b/pkg/mcp/mcp_test.go index 4d33d7cb..9b0c9eec 100644 --- a/pkg/mcp/mcp_test.go +++ b/pkg/mcp/mcp_test.go @@ -129,46 +129,7 @@ func TestMedium_Good_IsFile(t *testing.T) { } } -func TestResolvePath_Good(t *testing.T) { - tmpDir := t.TempDir() - s, err := New(WithWorkspaceRoot(tmpDir)) - if err != nil { - t.Fatalf("Failed to create service: %v", err) - } - - // Write a test file so resolve can work - _ = s.medium.Write("test.txt", "content") - - // Relative path should resolve to workspace - resolved, err := s.resolvePath("test.txt") - if err != nil { - t.Fatalf("Failed to resolve path: %v", err) - } - // The resolved path may be the symlink-resolved version - if !filepath.IsAbs(resolved) { - t.Errorf("Expected absolute path, got %s", resolved) - } -} - -func TestResolvePath_Good_NoWorkspace(t *testing.T) { - s, err := New(WithWorkspaceRoot("")) - if err != nil { - t.Fatalf("Failed to create service: %v", err) - } - - // With no workspace, relative paths resolve to cwd - cwd, _ := os.Getwd() - resolved, err := s.resolvePath("test.txt") - if err != nil { - t.Fatalf("Failed to resolve path: %v", err) - } - expected := filepath.Join(cwd, "test.txt") - if resolved != expected { - t.Errorf("Expected %s, got %s", expected, resolved) - } -} - -func TestResolvePath_Bad_Traversal(t *testing.T) { +func TestSandboxing_Bad_Traversal(t *testing.T) { tmpDir := t.TempDir() s, err := New(WithWorkspaceRoot(tmpDir)) if err != nil { @@ -176,19 +137,25 @@ func TestResolvePath_Bad_Traversal(t *testing.T) { } // Path traversal should fail - _, err = s.resolvePath("../secret.txt") + _, err = s.medium.Read("../secret.txt") if err == nil { t.Error("Expected error for path traversal") } // Absolute path outside workspace should fail - _, err = s.resolvePath("/etc/passwd") + // Note: local.Medium rejects all absolute paths if they are not inside root. + // But Read takes relative path usually. If absolute, it cleans it. + // If we pass "/etc/passwd", local.Medium path clean might reject it or treat it relative? + // local.Medium.path() implementation: + // if filepath.IsAbs(cleanPath) { return "", errors.New("path traversal attempt detected") } + // So yes, it rejects absolute paths passed to Read. + _, err = s.medium.Read("/etc/passwd") if err == nil { - t.Error("Expected error for absolute path outside workspace") + t.Error("Expected error for absolute path") } } -func TestResolvePath_Bad_SymlinkTraversal(t *testing.T) { +func TestSandboxing_Bad_SymlinkTraversal(t *testing.T) { tmpDir := t.TempDir() outsideDir := t.TempDir() @@ -210,7 +177,7 @@ func TestResolvePath_Bad_SymlinkTraversal(t *testing.T) { } // Symlink traversal should be blocked - _, err = s.resolvePath("evil-link") + _, err = s.medium.Read("evil-link") if err == nil { t.Error("Expected error for symlink pointing outside workspace") }