From ab8fe45074b164943144c172e26afca6cb3d0db7 Mon Sep 17 00:00:00 2001 From: Dmitry Kolesnikov Date: Sat, 1 Nov 2025 14:18:17 +0200 Subject: [PATCH] (fix) Inconsistent file path behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Key Changes: Updated shared validation functions (filename.go): IsValidFile(), IsValidPath(), and IsValidDir() now accept paths with or without leading / Both /file.txt and file.txt are treated as equivalent paths relative to the mount point Relative paths with .. are still rejected by fs.ValidPath() Both filesystems now behave identically: S3 filesystem (stream): Already had path normalization in s3Key() that strips leading / Local filesystem (lfs): Uses trim() function to normalize paths before filesystem operations Both accept the same path formats and treat them consistently Updated documentation: Both Create() and Open() methods in both filesystems now document that paths can be with or without leading / Clear that both forms are treated as paths relative to the mount point Behavior: Before: Paths required leading / → fsys.Create("/file.txt", nil) ✅ Paths without / failed → fsys.Create("file.txt", nil) ❌ After: Both forms work equally → fsys.Create("/file.txt", nil) ✅ → fsys.Create("file.txt", nil) ✅ Both are treated as the same file at the mount root No support for relative traversal with ../ (correctly rejected) --- filename.go | 53 +++++++++++++++++++++++++++++------- filesystem.go | 26 ++++++++++-------- filesystem_test.go | 6 ++--- lfs/filesystem.go | 20 +++++++------- lfs/filesystem_test.go | 61 ++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 132 insertions(+), 34 deletions(-) diff --git a/filename.go b/filename.go index d507c54..691ca23 100644 --- a/filename.go +++ b/filename.go @@ -12,10 +12,26 @@ import ( "io/fs" ) -// The file system requires absolute path starting from "/" +// The file system accepts paths with or without leading "/" +// Both are treated as paths relative to the mount point. // The file should not end with "/" func IsValidFile(path string) bool { - return len(path) > 0 && path[0] == '/' && fs.ValidPath(path[1:]) + if len(path) == 0 { + return false + } + + // Files must not end with / + if path[len(path)-1] == '/' { + return false + } + + // Accept paths with leading slash - strip it for validation + if path[0] == '/' { + return fs.ValidPath(path[1:]) + } + + // Accept paths without leading slash + return fs.ValidPath(path) } // Validate file path @@ -31,21 +47,30 @@ func RequireValidFile(ctx, path string) error { } } -// The file system requires absolute path starting from "/" +// The file system accepts paths with or without leading "/" +// Both are treated as paths relative to the mount point. func IsValidPath(path string) bool { if path == "/" { return true } - if len(path) != 0 && path[len(path)-1] == '/' { - path = path[:len(path)-1] + // Handle trailing slash + p := path + if len(p) != 0 && p[len(p)-1] == '/' { + p = p[:len(p)-1] } - if len(path) == 0 || path[0] != '/' || !fs.ValidPath(path[1:]) { + if len(p) == 0 { return false } - return true + // Accept paths with leading slash - strip it for validation + if p[0] == '/' { + return fs.ValidPath(p[1:]) + } + + // Accept paths without leading slash + return fs.ValidPath(p) } // Validate Path @@ -62,16 +87,26 @@ func RequireValidPath(ctx, path string) error { } // The file system emulates "dirs" as any valid path ending with "/" +// Accepts paths with or without leading "/" func IsValidDir(path string) bool { if path == "/" { return true } - if len(path) == 0 || path[0] != '/' || path[len(path)-1] != '/' { + if len(path) == 0 || path[len(path)-1] != '/' { return false } - return fs.ValidPath(path[1 : len(path)-1]) + // Strip trailing slash + p := path[:len(path)-1] + + // Accept paths with leading slash - strip it for validation + if len(p) > 0 && p[0] == '/' { + return fs.ValidPath(p[1:]) + } + + // Accept paths without leading slash + return fs.ValidPath(p) } // Validate Dir diff --git a/filesystem.go b/filesystem.go index e4e3b9d..3747183 100644 --- a/filesystem.go +++ b/filesystem.go @@ -84,13 +84,15 @@ func NewFS(bucket string, opts ...Option) (*FileSystem[struct{}], error) { return New[struct{}](bucket, opts...) } -// To open the file for writing use `Create` function giving the absolute path -// starting with `/`, the returned file descriptor is a composite of -// `io.Writer`, `io.Closer` and `stream.Stat`. Utilize Golang's convenient -// streaming methods to update S3 object seamlessly. Once all bytes are written, -// it's crucial to close the stream. Failure to do so would cause data loss. -// The object is considered successfully created on S3 only if all `Write` -// operations and subsequent `Close` actions are successful. +// To open the file for writing use `Create` function giving the path. +// The path can be with or without a leading `/` - both are treated as paths +// relative to the mount point (bucket or bucket/prefix). +// The returned file descriptor is a composite of `io.Writer`, `io.Closer` and +// `stream.Stat`. Utilize Golang's convenient streaming methods to update S3 +// object seamlessly. Once all bytes are written, it's crucial to close the stream. +// Failure to do so would cause data loss. The object is considered successfully +// created on S3 only if all `Write` operations and subsequent `Close` actions +// are successful. func (fsys *FileSystem[T]) Create(path string, attr *T) (File, error) { if err := RequireValidFile("create", path); err != nil { return nil, err @@ -99,10 +101,12 @@ func (fsys *FileSystem[T]) Create(path string, attr *T) (File, error) { return newWriter(fsys, path, attr), nil } -// To open the file for reading use `Open` function giving the absolute path -// starting with `/`, the returned file descriptor is a composite of -// `io.Reader`, `io.Closer` and `stream.Stat`. Utilize Golang's convenient -// streaming methods to consume S3 object seamlessly. +// To open the file for reading use `Open` function giving the path. +// The path can be with or without a leading `/` - both are treated as paths +// relative to the mount point (bucket or bucket/prefix). +// The returned file descriptor is a composite of `io.Reader`, `io.Closer` and +// `stream.Stat`. Utilize Golang's convenient streaming methods to consume S3 +// object seamlessly. func (fsys *FileSystem[T]) Open(path string) (fs.File, error) { if err := RequireValidPath("open", path); err != nil { return nil, err diff --git a/filesystem_test.go b/filesystem_test.go index ae14e30..f89905b 100644 --- a/filesystem_test.go +++ b/filesystem_test.go @@ -307,7 +307,7 @@ func TestReadWrite(t *testing.T) { it.Then(t).Should(it.Nil(err)) it.Then(t).Should( - it.Error(s3fs.Open("invalid..key/")), + it.Error(s3fs.Open("../invalid")), ) }) @@ -419,7 +419,7 @@ func TestWalk(t *testing.T) { it.Then(t).Should(it.Nil(err)) it.Then(t).Should( - it.Error(s3fs.ReadDir("invalid..key/")), + it.Error(s3fs.ReadDir("../invalid/")), ) }) @@ -659,7 +659,7 @@ func TestStat(t *testing.T) { it.Then(t).Should(it.Nil(err)) it.Then(t).Should( - it.Error(s3fs.Stat("invalid..key/")), + it.Error(s3fs.Stat("../invalid")), ) }) diff --git a/lfs/filesystem.go b/lfs/filesystem.go index ff576be..df03e85 100644 --- a/lfs/filesystem.go +++ b/lfs/filesystem.go @@ -65,15 +65,16 @@ func NewTempFS(root string, pattern string) (*FileSystem, error) { return New(dir) } -// To open the file for writing use `Create` function giving the absolute path -// starting with `/`, the returned file descriptor is a composite of -// `io.Writer`, `io.Closer` and `stream.Stat`. +// To open the file for writing use `Create` function giving the path. +// The path can be with or without a leading `/` - both are treated as paths +// relative to the mount point. +// The returned file descriptor is a composite of `io.Writer`, `io.Closer` and `stream.Stat`. func (fsys *FileSystem) Create(path string, attr *struct{}) (stream.File, error) { if err := stream.RequireValidFile("create", path); err != nil { return nil, err } - file := filepath.Join(fsys.Root, path) + file := filepath.Join(fsys.Root, trim(path)) return fsys.osCreate("create", file) } @@ -103,9 +104,10 @@ type nopCanceler struct{ *os.File } // Cancel effect of file i/o func (f nopCanceler) Cancel() error { return f.Close() } -// To open the file for reading use `Open` function giving the absolute path -// starting with `/`, the returned file descriptor is a composite of -// `io.Reader`, `io.Closer` and `stream.Stat`. +// To open the file for reading use `Open` function giving the path. +// The path can be with or without a leading `/` - both are treated as paths +// relative to the mount point. +// The returned file descriptor is a composite of `io.Reader`, `io.Closer` and `stream.Stat`. func (fsys *FileSystem) Open(path string) (fs.File, error) { if err := stream.RequireValidPath("open", path); err != nil { return nil, err @@ -181,7 +183,7 @@ func (fsys *FileSystem) Remove(path string) error { return err } - file := filepath.Join(fsys.Root, path) + file := filepath.Join(fsys.Root, trim(path)) return os.Remove(file) } @@ -202,7 +204,7 @@ func (fsys *FileSystem) Copy(source, target string) (err error) { } defer r.Close() - w, err := fsys.osCreate("copy", target) + w, err := fsys.osCreate("copy", filepath.Join(fsys.Root, trim(target))) if err != nil { return err } diff --git a/lfs/filesystem_test.go b/lfs/filesystem_test.go index a8dc63d..660814a 100644 --- a/lfs/filesystem_test.go +++ b/lfs/filesystem_test.go @@ -129,6 +129,28 @@ func TestReadWrite(t *testing.T) { it.Then(t).Must(it.Nil(err)) }) + t.Run("File/Write/RelativePath", func(t *testing.T) { + s3fs, err := lfs.NewTempFS("", "lfs") + it.Then(t).Should(it.Nil(err)) + + // Test with relative path (no leading slash) + fd, err := s3fs.Create("file.txt", nil) + it.Then(t).Must(it.Nil(err)) + + n, err := io.WriteString(fd, content) + it.Then(t).Should( + it.Nil(err), + it.Equal(n, len(content)), + ) + + err = fd.Close() + it.Then(t).Must(it.Nil(err)) + + // Verify file was created + _, err = s3fs.Stat("file.txt") + it.Then(t).Must(it.Nil(err)) + }) + t.Run("File/Write/Error/Dir", func(t *testing.T) { s3fs, err := lfs.NewTempFS("", "lfs") it.Then(t).Should( @@ -354,6 +376,20 @@ func TestRemove(t *testing.T) { it.Then(t).Must(it.Nil(err)) }) + t.Run("Remove/RelativePath", func(t *testing.T) { + s3fs, err := lfs.NewTempFS("", "lfs") + it.Then(t).Should(it.Nil(err)) + + // Create with relative path + fd, err := s3fs.Create("test.txt", nil) + it.Then(t).Must(it.Nil(err)) + fd.Close() + + // Remove with relative path + err = s3fs.Remove("test.txt") + it.Then(t).Must(it.Nil(err)) + }) + t.Run("Remove/Error", func(t *testing.T) { s3fs, err := lfs.NewTempFS("", "lfs") it.Then(t).Should(it.Nil(err)) @@ -385,7 +421,28 @@ func TestCopy(t *testing.T) { it.Nil(createFile(s3fs)), ) - err = s3fs.Copy(file, filepath.Join(s3fs.Root, "test/file")) + err = s3fs.Copy(file, "/test/file") + it.Then(t).Must(it.Nil(err)) + }) + + t.Run("Copy/RelativePath", func(t *testing.T) { + s3fs, err := lfs.NewTempFS("", "lfs") + it.Then(t).Should(it.Nil(err)) + + // Create source with relative path + fd, err := s3fs.Create("source.txt", nil) + it.Then(t).Must(it.Nil(err)) + fd.Write([]byte(content)) + fd.Close() + + // Copy with relative paths + err = s3fs.Copy("source.txt", "target.txt") + it.Then(t).Must(it.Nil(err)) + + // Verify both files exist + _, err = s3fs.Stat("source.txt") + it.Then(t).Must(it.Nil(err)) + _, err = s3fs.Stat("target.txt") it.Then(t).Must(it.Nil(err)) }) @@ -410,7 +467,7 @@ func TestCopy(t *testing.T) { it.Then(t).Should( it.Fail(func() error { - return s3fs.Copy(file, filepath.Join(s3fs.Root, "the/file")) + return s3fs.Copy(file, "/the/file") }), ) })