From fc3a9b7d0a1a777afe5d5a40a534304dcdc5af84 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Wed, 22 Jul 2026 10:14:00 +0530 Subject: [PATCH] Reject paths outside the repo root in repo drivers and AI file tools --- runtime/ai/develop_file.go | 7 ++--- runtime/ai/file_read.go | 7 ++--- runtime/ai/file_read_test.go | 40 +++++++++++++++++++++++++++++ runtime/ai/file_write.go | 6 +++-- runtime/ai/file_write_test.go | 17 +++++++++++++ runtime/ai/util.go | 15 +++++++++++ runtime/drivers/admin/repo.go | 40 +++++++++++++++++++++++------ runtime/drivers/file/repo.go | 48 ++++++++++++++++++++++++++--------- runtime/drivers/repo.go | 13 ++++++++++ runtime/drivers/repo_test.go | 47 ++++++++++++++++++++++++++++++++++ 10 files changed, 212 insertions(+), 28 deletions(-) create mode 100644 runtime/ai/file_read_test.go diff --git a/runtime/ai/develop_file.go b/runtime/ai/develop_file.go index 3616b1ab11df..6b7f9c8a52df 100644 --- a/runtime/ai/develop_file.go +++ b/runtime/ai/develop_file.go @@ -3,7 +3,6 @@ package ai import ( "context" "fmt" - "strings" "github.com/modelcontextprotocol/go-sdk/mcp" aiv1 "github.com/rilldata/rill/proto/gen/rill/ai/v1" @@ -56,9 +55,11 @@ func (t *DevelopFile) Handler(ctx context.Context, args *DevelopFileArgs) (*Deve if args.Path == "" || args.Prompt == "" { return nil, fmt.Errorf("invalid input: path and prompt are required") } - if !strings.HasPrefix(args.Path, "/") { - args.Path = "/" + args.Path + path, err := normalizeFilePath(args.Path) + if err != nil { + return nil, err } + args.Path = path // Prepare the system prompts generalInstructions, err := instructions.Load("development.md", instructions.Options{}) diff --git a/runtime/ai/file_read.go b/runtime/ai/file_read.go index d1f0dd5c5384..fca080520373 100644 --- a/runtime/ai/file_read.go +++ b/runtime/ai/file_read.go @@ -2,7 +2,6 @@ package ai import ( "context" - "strings" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/rilldata/rill/runtime" @@ -49,9 +48,11 @@ func (t *ReadFile) CheckAccess(ctx context.Context) (bool, error) { func (t *ReadFile) Handler(ctx context.Context, args *ReadFileArgs) (*ReadFileResult, error) { s := GetSession(ctx) - if !strings.HasPrefix(args.Path, "/") { - args.Path = "/" + args.Path + path, err := normalizeFilePath(args.Path) + if err != nil { + return nil, err } + args.Path = path blob, _, err := t.Runtime.GetFile(ctx, s.InstanceID(), args.Path) if err != nil { diff --git a/runtime/ai/file_read_test.go b/runtime/ai/file_read_test.go new file mode 100644 index 000000000000..289ebcc56d4e --- /dev/null +++ b/runtime/ai/file_read_test.go @@ -0,0 +1,40 @@ +package ai_test + +import ( + "testing" + + "github.com/rilldata/rill/runtime/ai" + "github.com/rilldata/rill/runtime/testruntime" + "github.com/stretchr/testify/require" +) + +func TestReadFile(t *testing.T) { + // Setup a project with a file and a test session + rt, instanceID := testruntime.NewInstanceWithOptions(t, testruntime.InstanceOptions{ + Files: map[string]string{ + "models/test_model.sql": "SELECT 1 AS val", + }, + }) + s := newSession(t, rt, instanceID) + + // Read file with and without a leading slash + for _, path := range []string{"models/test_model.sql", "/models/test_model.sql"} { + var res *ai.ReadFileResult + _, err := s.CallTool(t.Context(), ai.RoleUser, ai.ReadFileName, &res, &ai.ReadFileArgs{Path: path}) + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, "SELECT 1 AS val", res.Contents) + } + + // Read non-existent file + var res *ai.ReadFileResult + _, err := s.CallTool(t.Context(), ai.RoleUser, ai.ReadFileName, &res, &ai.ReadFileArgs{Path: "models/non_existent.sql"}) + require.Error(t, err) + + // Reject paths that traverse outside the project directory + for _, path := range []string{"../secret.txt", "/../secret.txt", "models/../../secret.txt", "../../../../etc/passwd", "..\\..\\secret.txt"} { + res = nil + _, err := s.CallTool(t.Context(), ai.RoleUser, ai.ReadFileName, &res, &ai.ReadFileArgs{Path: path}) + require.ErrorContains(t, err, "must not contain", "path %q", path) + } +} diff --git a/runtime/ai/file_write.go b/runtime/ai/file_write.go index c5a09fe50ca4..afdbdc66ed71 100644 --- a/runtime/ai/file_write.go +++ b/runtime/ai/file_write.go @@ -58,9 +58,11 @@ func (t *WriteFile) CheckAccess(ctx context.Context) (bool, error) { func (t *WriteFile) Handler(ctx context.Context, args *WriteFileArgs) (*WriteFileResult, error) { s := GetSession(ctx) - if !strings.HasPrefix(args.Path, "/") { - args.Path = "/" + args.Path + path, err := normalizeFilePath(args.Path) + if err != nil { + return nil, err } + args.Path = path // Read existing content before writing (for diff computation) var isNewFile bool diff --git a/runtime/ai/file_write_test.go b/runtime/ai/file_write_test.go index 990f9d0a64e4..9297cd3c92b5 100644 --- a/runtime/ai/file_write_test.go +++ b/runtime/ai/file_write_test.go @@ -80,4 +80,21 @@ func TestWriteFile(t *testing.T) { Remove: true, }) require.Error(t, err) + + // Reject paths that traverse outside the project directory + for _, path := range []string{"../escape.sql", "/../escape.sql", "models/../../escape.sql", "..\\escape.sql"} { + res = nil + _, err = s.CallTool(t.Context(), ai.RoleUser, ai.WriteFileName, &res, &ai.WriteFileArgs{ + Path: path, + Contents: "SELECT 1 AS val", + }) + require.ErrorContains(t, err, "must not contain", "path %q", path) + + res = nil + _, err = s.CallTool(t.Context(), ai.RoleUser, ai.WriteFileName, &res, &ai.WriteFileArgs{ + Path: path, + Remove: true, + }) + require.ErrorContains(t, err, "must not contain", "path %q", path) + } } diff --git a/runtime/ai/util.go b/runtime/ai/util.go index 65e7848ff316..c5cb4b7e51e0 100644 --- a/runtime/ai/util.go +++ b/runtime/ai/util.go @@ -63,6 +63,21 @@ func resolverResultToTabular(res runtime.ResolverResult) ([]SchemaField, [][]any return fields, data, nil } +// normalizeFilePath converts a model-provided file path to an absolute path within the project (e.g. "models/foo.sql" to "/models/foo.sql"). +// It returns an error for paths with ".." segments, which could otherwise traverse outside the project directory. +func normalizeFilePath(p string) (string, error) { + segments := strings.FieldsFunc(p, func(r rune) bool { return r == '/' || r == '\\' }) + for _, segment := range segments { + if segment == ".." { + return "", fmt.Errorf("invalid path %q: must not contain %q segments", p, "..") + } + } + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + return p, nil +} + var templateFuncs = template.FuncMap{ "backticks": func() string { return "```" diff --git a/runtime/drivers/admin/repo.go b/runtime/drivers/admin/repo.go index 4bdbaf879da5..8e37a6dfb6ee 100644 --- a/runtime/drivers/admin/repo.go +++ b/runtime/drivers/admin/repo.go @@ -147,7 +147,10 @@ func (r *repo) Get(ctx context.Context, path string) (string, error) { var readErr error for _, root := range r.roots() { // Search in every underlying file system. - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return "", err + } b, err := os.ReadFile(fp) if err != nil { // Keep searching if it's a not exist error. Otherwise break and return the error immediately. @@ -185,7 +188,10 @@ func (r *repo) Hash(ctx context.Context, paths []string) (string, error) { if drivers.IsIgnored(path, r.ignorePaths) { continue // Skip if file does not exist } - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return "", err + } file, err := os.Open(fp) if err != nil { if os.IsNotExist(err) { @@ -218,7 +224,10 @@ func (r *repo) Stat(ctx context.Context, path string) (*drivers.FileInfo, error) var statErr error for _, root := range r.roots() { // Search in every underlying file system. - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return nil, err + } info, err := os.Stat(fp) if err != nil { // Keep searching if it's a not exist error. Otherwise break and return the error immediately. @@ -254,7 +263,10 @@ func (r *repo) Put(ctx context.Context, path string, reader io.Reader) error { return fmt.Errorf("can't write to ignored path %q", path) } - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return err + } err = os.MkdirAll(filepath.Dir(fp), os.ModePerm) if err != nil { @@ -292,7 +304,10 @@ func (r *repo) MkdirAll(ctx context.Context, path string) error { return fmt.Errorf("can't write to ignored path %q", path) } - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return err + } err = os.MkdirAll(fp, os.ModePerm) if err != nil { @@ -322,8 +337,14 @@ func (r *repo) Rename(ctx context.Context, fromPath, toPath string) error { return fmt.Errorf("can't write to ignored path %q", toPath) } - fromPath = filepath.Join(root, fromPath) - toPath = filepath.Join(root, toPath) + fromPath, err = drivers.ResolveRepoPath(root, fromPath) + if err != nil { + return err + } + toPath, err = drivers.ResolveRepoPath(root, toPath) + if err != nil { + return err + } if _, err := os.Stat(toPath); !strings.EqualFold(fromPath, toPath) && err == nil { return os.ErrExist @@ -358,7 +379,10 @@ func (r *repo) Delete(ctx context.Context, path string, force bool) error { return fmt.Errorf("can't write to ignored path %q", path) } - fp := filepath.Join(root, path) + fp, err := drivers.ResolveRepoPath(root, path) + if err != nil { + return err + } if force { err = os.RemoveAll(fp) diff --git a/runtime/drivers/file/repo.go b/runtime/drivers/file/repo.go index f418bf259d0f..2e9fe23e9841 100644 --- a/runtime/drivers/file/repo.go +++ b/runtime/drivers/file/repo.go @@ -71,7 +71,10 @@ func (c *connection) ListGlob(ctx context.Context, glob string, skipDirs bool) ( // Get implements drivers.RepoStore. func (c *connection) Get(ctx context.Context, filePath string) (string, error) { - fp := filepath.Join(c.root, filePath) + fp, err := drivers.ResolveRepoPath(c.root, filePath) + if err != nil { + return "", err + } b, err := os.ReadFile(fp) if err != nil { @@ -90,8 +93,11 @@ func (c *connection) Get(ctx context.Context, filePath string) (string, error) { func (c *connection) Hash(ctx context.Context, paths []string) (string, error) { hasher := md5.New() for _, path := range paths { - path = filepath.Join(c.root, path) - file, err := os.Open(path) + fp, err := drivers.ResolveRepoPath(c.root, path) + if err != nil { + return "", err + } + file, err := os.Open(fp) if err != nil { if os.IsNotExist(err) { continue @@ -110,7 +116,10 @@ func (c *connection) Hash(ctx context.Context, paths []string) (string, error) { // Stat implements drivers.RepoStore. func (c *connection) Stat(ctx context.Context, filePath string) (*drivers.FileInfo, error) { - filePath = filepath.Join(c.root, filePath) + filePath, err := drivers.ResolveRepoPath(c.root, filePath) + if err != nil { + return nil, err + } info, err := os.Stat(filePath) if err != nil { @@ -125,9 +134,12 @@ func (c *connection) Stat(ctx context.Context, filePath string) (*drivers.FileIn // Put implements drivers.RepoStore. func (c *connection) Put(ctx context.Context, filePath string, reader io.Reader) error { - filePath = filepath.Join(c.root, filePath) + filePath, err := drivers.ResolveRepoPath(c.root, filePath) + if err != nil { + return err + } - err := os.MkdirAll(filepath.Dir(filePath), os.ModePerm) + err = os.MkdirAll(filepath.Dir(filePath), os.ModePerm) if err != nil { return err } @@ -148,9 +160,12 @@ func (c *connection) Put(ctx context.Context, filePath string, reader io.Reader) // MkdirAll implements drivers.RepoStore. func (c *connection) MkdirAll(ctx context.Context, dirPath string) error { - dirPath = filepath.Join(c.root, dirPath) + dirPath, err := drivers.ResolveRepoPath(c.root, dirPath) + if err != nil { + return err + } - err := os.MkdirAll(dirPath, os.ModePerm) + err = os.MkdirAll(dirPath, os.ModePerm) if err != nil { return err } @@ -160,13 +175,19 @@ func (c *connection) MkdirAll(ctx context.Context, dirPath string) error { // Rename implements drivers.RepoStore. func (c *connection) Rename(ctx context.Context, fromPath, toPath string) error { - toPath = filepath.Join(c.root, toPath) + toPath, err := drivers.ResolveRepoPath(c.root, toPath) + if err != nil { + return err + } - fromPath = filepath.Join(c.root, fromPath) + fromPath, err = drivers.ResolveRepoPath(c.root, fromPath) + if err != nil { + return err + } if _, err := os.Stat(toPath); !strings.EqualFold(fromPath, toPath) && err == nil { return os.ErrExist } - err := os.Rename(fromPath, toPath) + err = os.Rename(fromPath, toPath) if err != nil { return err } @@ -175,7 +196,10 @@ func (c *connection) Rename(ctx context.Context, fromPath, toPath string) error // Delete implements drivers.RepoStore. func (c *connection) Delete(ctx context.Context, filePath string, force bool) error { - filePath = filepath.Join(c.root, filePath) + filePath, err := drivers.ResolveRepoPath(c.root, filePath) + if err != nil { + return err + } if force { return os.RemoveAll(filePath) } diff --git a/runtime/drivers/repo.go b/runtime/drivers/repo.go index 18662c98b6c8..1c421753b38e 100644 --- a/runtime/drivers/repo.go +++ b/runtime/drivers/repo.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "strings" "time" @@ -146,6 +147,18 @@ func IsIgnored(path string, additionalIgnoredPaths []string) bool { return false } +// ResolveRepoPath resolves a repo-relative path against the given root directory and returns the resulting file system path. +// It returns an error if the resolved path falls outside the root, which prevents path traversal using ".." segments. +// Repo drivers must use it instead of joining untrusted paths onto the root directly. +func ResolveRepoPath(root, path string) (string, error) { + root = filepath.Clean(root) + fp := filepath.Join(root, path) + if fp != root && !strings.HasPrefix(fp, root+string(filepath.Separator)) { + return "", fmt.Errorf("path %q is outside the repo root", path) + } + return fp, nil +} + type RepoStatus struct { // IsGitRepo indicates if the repo is backed by a Git repository. IsGitRepo bool diff --git a/runtime/drivers/repo_test.go b/runtime/drivers/repo_test.go index e3757bc25302..c25055c0dad0 100644 --- a/runtime/drivers/repo_test.go +++ b/runtime/drivers/repo_test.go @@ -3,6 +3,7 @@ package drivers_test import ( "context" "os" + "path/filepath" "strings" "testing" @@ -157,4 +158,50 @@ func testRepo(t *testing.T, repo drivers.RepoStore) { {"/foo_new.yml", false}, {"/new_folder", true}, }, files) + + // paths that traverse outside the repo root must be rejected + _, err = repo.Get(ctx, "/../secret.txt") + require.ErrorContains(t, err, "outside the repo root") + _, err = repo.Stat(ctx, "../secret.txt") + require.ErrorContains(t, err, "outside the repo root") + err = repo.Put(ctx, "../escape.sql", strings.NewReader("boom")) + require.ErrorContains(t, err, "outside the repo root") + err = repo.MkdirAll(ctx, "/nested/../../escape_dir") + require.ErrorContains(t, err, "outside the repo root") + err = repo.Rename(ctx, "foo.csv", "../escape.csv") + require.ErrorContains(t, err, "outside the repo root") + err = repo.Rename(ctx, "../escape.csv", "foo2.csv") + require.ErrorContains(t, err, "outside the repo root") + err = repo.Delete(ctx, "/../escape.sql", false) + require.ErrorContains(t, err, "outside the repo root") +} + +func TestResolveRepoPath(t *testing.T) { + root := filepath.Join(string(filepath.Separator), "data", "repo") + + valid := map[string]string{ + "/models/foo.sql": filepath.Join(root, "models", "foo.sql"), + "models/foo.sql": filepath.Join(root, "models", "foo.sql"), + "/models/../rill.yaml": filepath.Join(root, "rill.yaml"), + "/": root, + "": root, + } + for path, expected := range valid { + fp, err := drivers.ResolveRepoPath(root, path) + require.NoError(t, err, "path %q", path) + require.Equal(t, expected, fp, "path %q", path) + } + + invalid := []string{ + "..", + "/..", + "../secret.txt", + "/../secret.txt", + "/models/../../secret.txt", + "../../../../etc/passwd", + } + for _, path := range invalid { + _, err := drivers.ResolveRepoPath(root, path) + require.ErrorContains(t, err, "outside the repo root", "path %q", path) + } }