Merge pull request #195 from git-pkgs/fix-filesystem-storage-path-validation

Validate filesystem storage paths

Author: andrew

internal/storage/filesystem.go

@@ -34,11 +34,31 @@ func NewFilesystem(root string) (*Filesystem, error) {
 }
 
 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)
+	localPath, err := cleanStoragePath(path)
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(fs.root, localPath), nil
+}
+
+func (fs *Filesystem) prefixPath(prefix string) (string, error) {
+	if prefix == "" {
+		return fs.root, nil
+	}
+	return fs.fullPath(prefix)
+}
+
+func cleanStoragePath(path string) (string, error) {
+	if path == "." || strings.Contains(path, `\`) || !filepath.IsLocal(path) {
+		return "", fmt.Errorf("%w: invalid storage path", ErrNotFound)
 	}
-	return full, nil
+
+	localPath, err := filepath.Localize(path)
+	if err != nil || localPath == "." || !filepath.IsLocal(localPath) {
+		return "", fmt.Errorf("%w: invalid storage path", ErrNotFound)
+	}
+
+	return localPath, nil
 }
 
 func (fs *Filesystem) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) {
@@ -190,7 +210,7 @@ func (fs *Filesystem) UsedSpace(ctx context.Context) (int64, error) {
 
 // ListPrefix returns object metadata for paths under a prefix.
 func (fs *Filesystem) ListPrefix(ctx context.Context, prefix string) ([]ObjectInfo, error) {
-	searchRoot, err := fs.fullPath(prefix)
+	searchRoot, err := fs.prefixPath(prefix)
 	if err != nil {
 		return nil, err
 	}

internal/storage/filesystem_test.go

@@ -243,19 +243,70 @@ func TestFilesystemLargeFile(t *testing.T) {
 	assertLargeFileRoundTrip(t, createTestFilesystem(t))
 }
 
-func TestFilesystemRejectsTraversal(t *testing.T) {
+func TestFilesystemRejectsInvalidPaths(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)
+	for _, p := range []string{
+		"",
+		".",
+		"../etc/passwd",
+		"../../etc/passwd",
+		"a/../../etc/passwd",
+		"/etc/passwd",
+		"test//file.txt",
+		"test/./file.txt",
+		"test/../file.txt",
+		`test\..\file.txt`,
+	} {
+		name := p
+		if name == "" {
+			name = "empty"
 		}
+
+		t.Run(name, func(t *testing.T) {
+			ctx := context.Background()
+
+			if _, err := fs.FullPath(p); !errors.Is(err, ErrNotFound) {
+				t.Errorf("FullPath(%q) = %v, want ErrNotFound", p, err)
+			}
+			if _, err := fs.Open(ctx, p); err == nil {
+				t.Errorf("Open(%q) should reject invalid path", p)
+			}
+			if _, _, err := fs.Store(ctx, p, strings.NewReader("x")); err == nil {
+				t.Errorf("Store(%q) should reject invalid path", p)
+			}
+			if _, err := fs.Exists(ctx, p); err == nil {
+				t.Errorf("Exists(%q) should reject invalid path", p)
+			}
+			if err := fs.Delete(ctx, p); err == nil {
+				t.Errorf("Delete(%q) should reject invalid path", p)
+			}
+			if _, err := fs.Size(ctx, p); err == nil {
+				t.Errorf("Size(%q) should reject invalid path", p)
+			}
+			if _, err := fs.ListPrefix(ctx, p); p != "" && err == nil {
+				t.Errorf("ListPrefix(%q) should reject invalid path", p)
+			}
+		})
+	}
+}
+
+func TestFilesystemListPrefixAllowsEmptyPrefix(t *testing.T) {
+	fs := createTestFilesystem(t)
+	ctx := context.Background()
+
+	_, _, _ = fs.Store(ctx, "a.txt", strings.NewReader("aaaa"))
+	_, _, _ = fs.Store(ctx, "c/d.txt", strings.NewReader("ccccc"))
+
+	objects, err := fs.ListPrefix(ctx, "")
+	if err != nil {
+		t.Fatalf("ListPrefix empty prefix failed: %v", err)
+	}
+	if len(objects) != 2 {
+		t.Fatalf("ListPrefix empty prefix returned %d objects, want 2", len(objects))
 	}
 }