Merge pull request #2433 from dgageot/fix6

fix: prevent symlink-based path traversal in ACP filesystem toolset

Author: dgageot

pkg/acp/filesystem.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 	"path/filepath"
 	"strings"
 
@@ -71,6 +72,8 @@ func (t *FilesystemToolset) Tools(ctx context.Context) ([]tools.Tool, error) {
 
 // resolvePath resolves a user-supplied path relative to the working directory
 // and validates that the resulting path does not escape the working directory.
+// It follows symlinks to prevent a symlink inside the working directory from
+// pointing outside it.
 func (t *FilesystemToolset) resolvePath(userPath string) (string, error) {
 	resolved := filepath.Clean(filepath.Join(t.workingDir, userPath))
 	absWorkingDir, err := filepath.Abs(t.workingDir)
@@ -81,14 +84,52 @@ func (t *FilesystemToolset) resolvePath(userPath string) (string, error) {
 	if err != nil {
 		return "", fmt.Errorf("failed to resolve path: %w", err)
 	}
+
+	// Resolve symlinks. For paths that don't exist yet (e.g. a new file
+	// being created), walk up to the nearest existing ancestor, resolve
+	// symlinks on that, then re-append the remaining components.
+	realResolved, err := evalSymlinksAllowMissing(absResolved)
+	if err != nil {
+		return "", fmt.Errorf("failed to evaluate symlinks: %w", err)
+	}
+	realWorkingDir, err := filepath.EvalSymlinks(absWorkingDir)
+	if err != nil {
+		return "", fmt.Errorf("failed to evaluate symlinks for working directory: %w", err)
+	}
+
 	// Normalize paths for comparison to prevent bypasses on case-insensitive
 	// filesystems (macOS, Windows) where differing case could defeat the check.
-	normResolved := normalizePathForComparison(absResolved)
-	normWorkingDir := normalizePathForComparison(absWorkingDir)
+	normResolved := normalizePathForComparison(realResolved)
+	normWorkingDir := normalizePathForComparison(realWorkingDir)
 	if !strings.HasPrefix(normResolved, normWorkingDir+string(filepath.Separator)) && normResolved != normWorkingDir {
 		return "", fmt.Errorf("path %q escapes the working directory", userPath)
 	}
-	return absResolved, nil
+	return realResolved, nil
+}
+
+// evalSymlinksAllowMissing resolves symlinks for a path that may not fully
+// exist. It walks up from the given path until it finds an existing ancestor,
+// resolves symlinks on that ancestor, then re-appends the missing tail.
+func evalSymlinksAllowMissing(path string) (string, error) {
+	resolved, err := filepath.EvalSymlinks(path)
+	if err == nil {
+		return resolved, nil
+	}
+	if !os.IsNotExist(err) {
+		return "", err
+	}
+
+	// Walk up to find the nearest existing ancestor.
+	parent := filepath.Dir(path)
+	if parent == path {
+		// Reached filesystem root without finding an existing path.
+		return path, nil
+	}
+	realParent, err := evalSymlinksAllowMissing(parent)
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(realParent, filepath.Base(path)), nil
 }
 
 func (t *FilesystemToolset) handleReadFile(ctx context.Context, toolCall tools.ToolCall) (*tools.ToolCallResult, error) {

pkg/acp/filesystem_test.go

@@ -1,7 +1,9 @@
 package acp
 
 import (
+	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -17,7 +19,7 @@ func TestResolvePath(t *testing.T) {
 		workingDir: workingDir,
 	}
 
-	absWorkingDir, err := filepath.Abs(workingDir)
+	absWorkingDir, err := filepath.EvalSymlinks(workingDir)
 	require.NoError(t, err)
 
 	tests := []struct {
@@ -85,3 +87,81 @@ func TestNormalizePathForComparison(t *testing.T) {
 	// The exact behavior depends on the platform.
 	assert.NotEmpty(t, result)
 }
+
+func TestResolvePath_SymlinkEscape(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("symlink test not reliable on Windows")
+	}
+
+	workingDir := t.TempDir()
+	outsideDir := t.TempDir()
+
+	// Create a secret file outside the working directory.
+	secretFile := filepath.Join(outsideDir, "secret.txt")
+	require.NoError(t, os.WriteFile(secretFile, []byte("secret"), 0o644))
+
+	// Create a symlink inside the working directory pointing outside.
+	symlink := filepath.Join(workingDir, "escape")
+	require.NoError(t, os.Symlink(outsideDir, symlink))
+
+	ts := &FilesystemToolset{workingDir: workingDir}
+
+	// Accessing a file through the symlink should be blocked.
+	_, err := ts.resolvePath("escape/secret.txt")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "escapes the working directory")
+
+	// The symlink target itself should also be blocked.
+	_, err = ts.resolvePath("escape")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "escapes the working directory")
+}
+
+func TestResolvePath_SymlinkWithinWorkingDir(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("symlink test not reliable on Windows")
+	}
+
+	workingDir := t.TempDir()
+
+	// Create a subdirectory and a symlink to it within the working dir.
+	subdir := filepath.Join(workingDir, "real")
+	require.NoError(t, os.Mkdir(subdir, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(subdir, "file.txt"), []byte("ok"), 0o644))
+
+	link := filepath.Join(workingDir, "link")
+	require.NoError(t, os.Symlink(subdir, link))
+
+	ts := &FilesystemToolset{workingDir: workingDir}
+
+	// Symlink within working dir should be allowed.
+	resolved, err := ts.resolvePath("link/file.txt")
+	require.NoError(t, err)
+	assert.Contains(t, resolved, "real/file.txt")
+}
+
+func TestResolvePath_NonExistentPathWithSymlinkAncestor(t *testing.T) {
+	t.Parallel()
+
+	if runtime.GOOS == "windows" {
+		t.Skip("symlink test not reliable on Windows")
+	}
+
+	workingDir := t.TempDir()
+	outsideDir := t.TempDir()
+
+	// Symlink inside working dir pointing outside.
+	symlink := filepath.Join(workingDir, "escape")
+	require.NoError(t, os.Symlink(outsideDir, symlink))
+
+	ts := &FilesystemToolset{workingDir: workingDir}
+
+	// Even for a non-existent file under the symlink, traversal should be blocked.
+	_, err := ts.resolvePath("escape/nonexistent.txt")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "escapes the working directory")
+}