diff --git a/runtime/drivers/duckdb/model_executor_self_sqlite_test.go b/runtime/drivers/duckdb/model_executor_self_sqlite_test.go index a17cdc6d2ba2..3d23df0eb041 100644 --- a/runtime/drivers/duckdb/model_executor_self_sqlite_test.go +++ b/runtime/drivers/duckdb/model_executor_self_sqlite_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "fmt" + "path/filepath" "testing" "github.com/rilldata/rill/runtime/drivers" @@ -18,7 +19,7 @@ import ( func Test_sqliteToDuckDB_Transfer(t *testing.T) { tempDir := t.TempDir() - dbPath := fmt.Sprintf("%s.db", tempDir) + dbPath := filepath.Join(tempDir, "source.db") db, err := sql.Open("sqlite", dbPath) require.NoError(t, err) @@ -41,6 +42,7 @@ func Test_sqliteToDuckDB_Transfer(t *testing.T) { OutputConnector: "duckdb", Env: &drivers.ModelEnv{ AllowHostAccess: false, + RepoRoot: tempDir, StageChanges: true, }, PreliminaryInputProperties: map[string]any{ diff --git a/runtime/drivers/duckdb/model_executor_self_test.go b/runtime/drivers/duckdb/model_executor_self_test.go index db4c531b9eb6..2df789122a2c 100644 --- a/runtime/drivers/duckdb/model_executor_self_test.go +++ b/runtime/drivers/duckdb/model_executor_self_test.go @@ -43,6 +43,7 @@ func TestDuckDBToDuckDBTransfer(t *testing.T) { OutputConnector: "duckdb", Env: &drivers.ModelEnv{ AllowHostAccess: false, + RepoRoot: tempDir, StageChanges: true, }, PreliminaryInputProperties: map[string]any{ diff --git a/runtime/pkg/fileutil/fileutil.go b/runtime/pkg/fileutil/fileutil.go index 803566bde5a4..e838a2acd29c 100644 --- a/runtime/pkg/fileutil/fileutil.go +++ b/runtime/pkg/fileutil/fileutil.go @@ -324,10 +324,18 @@ func ResolveLocalPath(path, root string, allowHostAccess bool) (string, error) { if !filepath.IsAbs(path) { finalPath = filepath.Join(root, path) } - - if !allowHostAccess && !strings.HasPrefix(finalPath, root) { - // path is outside the repo root - return "", fmt.Errorf("path is outside repo root") + // Clean here so that ".." segments are resolved before the sandbox check below; + // filepath.Join already cleans the relative branch, but an absolute path does not pass through it. + finalPath = filepath.Clean(finalPath) + + if !allowHostAccess { + // Ensure the resolved path stays within the repo root. + // The trailing separator prevents a sibling directory whose name shares the root's prefix + // (e.g. root "/data/proj" matching "/data/proj-x") from passing the check. + root = filepath.Clean(root) + if finalPath != root && !strings.HasPrefix(finalPath, root+string(filepath.Separator)) { + return "", fmt.Errorf("path is outside repo root") + } } return finalPath, nil } diff --git a/runtime/pkg/fileutil/fileutil_test.go b/runtime/pkg/fileutil/fileutil_test.go index a40ea0c24d7c..26ebf6b75a2c 100644 --- a/runtime/pkg/fileutil/fileutil_test.go +++ b/runtime/pkg/fileutil/fileutil_test.go @@ -2,6 +2,7 @@ package fileutil import ( "os/user" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -74,6 +75,52 @@ func TestExpandHome(t *testing.T) { } } +func TestResolveLocalPath(t *testing.T) { + sep := string(filepath.Separator) + root := filepath.Clean("/data/proj123") + + variations := []struct { + Name string + Path string + Root string + AllowHostAccess bool + Expected string + ExpectErr bool + }{ + // Relative paths within the root are joined and cleaned. + {"relative within root", "sub/file.csv", root, false, filepath.Join(root, "sub", "file.csv"), false}, + {"relative to root itself", ".", root, false, root, false}, + + // Absolute paths within the root are allowed. + {"absolute within root", filepath.Join(root, "file.csv"), root, false, filepath.Join(root, "file.csv"), false}, + + // Absolute path traversal must be rejected once cleaned. + {"absolute traversal", root + sep + ".." + sep + ".." + sep + "etc" + sep + "passwd", root, false, "", true}, + + // Relative traversal escaping the root must be rejected. + {"relative traversal", ".." + sep + ".." + sep + "etc" + sep + "passwd", root, false, "", true}, + + // Sibling directory sharing the root's prefix must not pass the check. + {"sibling prefix collision", "/data/proj123-x/secret", root, false, "", true}, + {"relative sibling prefix collision", ".." + sep + "proj123-x" + sep + "secret", root, false, "", true}, + + // With host access enabled, any path is allowed (but still cleaned). + {"host access bypasses sandbox", root + sep + ".." + sep + "etc" + sep + "passwd", root, true, filepath.Clean("/data/etc/passwd"), false}, + } + + for _, tt := range variations { + t.Run(tt.Name, func(t *testing.T) { + got, err := ResolveLocalPath(tt.Path, tt.Root, tt.AllowHostAccess) + if tt.ExpectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.Expected, got) + }) + } +} + func TestIsGlob(t *testing.T) { variations := []struct { Path string