Reject path traversal in filesystem storage (#106)

Author: andrew

internal/storage/filesystem.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 )
 
@@ -31,12 +32,19 @@ func NewFilesystem(root string) (*Filesystem, error) {
 	return &Filesystem{root: absRoot}, nil
 }
 
-func (fs *Filesystem) fullPath(path string) string {
-	return filepath.Join(fs.root, filepath.FromSlash(path))
+func (fs *Filesystem) fullPath(path string) (string, error) {
+	full := filepath.Clean(filepath.Join(fs.root, filepath.FromSlash(path)))
+	if full != fs.root && !strings.HasPrefix(full, fs.root+string(filepath.Separator)) {
+		return "", fmt.Errorf("%w: path escapes storage root", ErrNotFound)
+	}
+	return full, nil
 }
 
 func (fs *Filesystem) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) {
-	fullPath := fs.fullPath(path)
+	fullPath, err := fs.fullPath(path)
+	if err != nil {
+		return 0, "", err
+	}
 
 	dir := filepath.Dir(fullPath)
 	if err := os.MkdirAll(dir, dirPermissions); err != nil {
@@ -83,7 +91,10 @@ func (fs *Filesystem) Store(ctx context.Context, path string, r io.Reader) (int6
 }
 
 func (fs *Filesystem) Open(ctx context.Context, path string) (io.ReadCloser, error) {
-	fullPath := fs.fullPath(path)
+	fullPath, err := fs.fullPath(path)
+	if err != nil {
+		return nil, err
+	}
 
 	f, err := os.Open(fullPath)
 	if err != nil {
@@ -97,9 +108,12 @@ func (fs *Filesystem) Open(ctx context.Context, path string) (io.ReadCloser, err
 }
 
 func (fs *Filesystem) Exists(ctx context.Context, path string) (bool, error) {
-	fullPath := fs.fullPath(path)
+	fullPath, err := fs.fullPath(path)
+	if err != nil {
+		return false, err
+	}
 
-	_, err := os.Stat(fullPath)
+	_, err = os.Stat(fullPath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return false, nil
@@ -111,9 +125,12 @@ func (fs *Filesystem) Exists(ctx context.Context, path string) (bool, error) {
 }
 
 func (fs *Filesystem) Delete(ctx context.Context, path string) error {
-	fullPath := fs.fullPath(path)
+	fullPath, err := fs.fullPath(path)
+	if err != nil {
+		return err
+	}
 
-	err := os.Remove(fullPath)
+	err = os.Remove(fullPath)
 	if err != nil && !os.IsNotExist(err) {
 		return fmt.Errorf("removing file: %w", err)
 	}
@@ -135,7 +152,10 @@ func (fs *Filesystem) SignedURL(_ context.Context, _ string, _ time.Duration) (s
 }
 
 func (fs *Filesystem) Size(ctx context.Context, path string) (int64, error) {
-	fullPath := fs.fullPath(path)
+	fullPath, err := fs.fullPath(path)
+	if err != nil {
+		return 0, err
+	}
 
 	info, err := os.Stat(fullPath)
 	if err != nil {
@@ -174,7 +194,8 @@ func (fs *Filesystem) Root() string {
 
 // FullPath returns the full filesystem path for a storage path.
 // Useful for serving files directly or debugging.
-func (fs *Filesystem) FullPath(path string) string {
+// Returns an error if the resulting path would escape the storage root.
+func (fs *Filesystem) FullPath(path string) (string, error) {
 	return fs.fullPath(path)
 }
 

internal/storage/filesystem_test.go

@@ -52,7 +52,10 @@ func TestFilesystemStore(t *testing.T) {
 	}
 
 	// Verify file exists on disk
-	fullPath := fs.FullPath("npm/lodash/4.17.21/lodash.tgz")
+	fullPath, err := fs.FullPath("npm/lodash/4.17.21/lodash.tgz")
+	if err != nil {
+		t.Fatalf("FullPath failed: %v", err)
+	}
 	data, err := os.ReadFile(fullPath)
 	if err != nil {
 		t.Fatalf("reading stored file: %v", err)
@@ -163,7 +166,10 @@ func TestFilesystemDelete(t *testing.T) {
 	}
 
 	// Empty parent directories should be cleaned up
-	nestedDir := fs.FullPath("test/delete/nested")
+	nestedDir, err := fs.FullPath("test/delete/nested")
+	if err != nil {
+		t.Fatalf("FullPath failed: %v", err)
+	}
 	if _, err := os.Stat(nestedDir); !os.IsNotExist(err) {
 		t.Error("empty nested directory not cleaned up")
 	}
@@ -237,6 +243,22 @@ func TestFilesystemLargeFile(t *testing.T) {
 	assertLargeFileRoundTrip(t, createTestFilesystem(t))
 }
 
+func TestFilesystemRejectsTraversal(t *testing.T) {
+	tmp := t.TempDir()
+	fs, err := NewFilesystem(tmp)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, p := range []string{"../etc/passwd", "../../etc/passwd", "a/../../etc/passwd"} {
+		if _, err := fs.Open(context.Background(), p); err == nil {
+			t.Errorf("Open(%q) should reject traversal", p)
+		}
+		if _, _, err := fs.Store(context.Background(), p, strings.NewReader("x")); err == nil {
+			t.Errorf("Store(%q) should reject traversal", p)
+		}
+	}
+}
+
 func TestFilesystemSignedURLUnsupported(t *testing.T) {
 	fs := createTestFilesystem(t)