filer: use bind variables for request-controlled values in the arangodb store (#10795)

* arangodb: bind list prefix, start file name and collection into the AQL query Concatenating them into the query text let a caller-supplied prefix or start name close the string literal and append arbitrary AQL, which runs with the filer's ArangoDB credentials against any collection. * arangodb: bind the folder path and collection into the recursive delete query A trailing-slash S3 key reaches DeleteFolderChildren through the directory-marker cleanup, so quotes in the path could turn the filter into a match-everything REMOVE over the whole bucket collection. * arangodb: match the real directory prefix in the recursive delete The prefix was built by re-joining the path segments with commas, so it never matched a stored directory and the subtree sweep did nothing.
2026-08-18 13:17:08 +00:00 · 2026-08-17 15:15:26 -07:00
parent 5d5ea63b3f
commit e383ee47cb
2 changed files with 215 additions and 17 deletions
@@ -5,7 +5,6 @@ import (
 	"crypto/tls"
 	"fmt"
 	"strconv"
-	"strings"
 	"sync"
 	"time"

@@ -265,21 +264,19 @@ func (store *ArangodbStore) DeleteEntry(ctx context.Context, fullpath util.FullP

 // this runs in log time
 func (store *ArangodbStore) DeleteFolderChildren(ctx context.Context, fullpath util.FullPath) (err error) {
-	var query string
 	targetCollection, err := store.extractBucketCollection(ctx, fullpath)
 	if err != nil {
 		return err
 	}
-	query = query + fmt.Sprintf(`
-	for d in %s
-	filter starts_with(d.directory, "%s/")  || d.directory == "%s"
-	remove d._key in %s`,
-		"`"+targetCollection.Name()+"`",
-		strings.Join(strings.Split(string(fullpath), "/"), ","),
-		string(fullpath),
-		"`"+targetCollection.Name()+"`",
-	)
-	cur, err := store.database.Query(ctx, query, nil)
+	query := `
+	for d in @@collection
+	filter starts_with(d.directory, @dirPrefix)  || d.directory == @dir
+	remove d._key in @@collection`
+	cur, err := store.database.Query(ctx, query, map[string]interface{}{
+		"@collection": targetCollection.Name(),
+		"dirPrefix":   string(fullpath) + "/",
+		"dir":         string(fullpath),
+	})
 	if err != nil {
 		return fmt.Errorf("delete %s : %v", fullpath, err)
 	}
@@ -296,14 +293,20 @@ func (store *ArangodbStore) ListDirectoryPrefixedEntries(ctx context.Context, di
 	if err != nil {
 		return lastFileName, err
 	}
-	query := "for d in " + "`" + targetCollection.Name() + "`"
+	bindVars := map[string]interface{}{
+		"@collection": targetCollection.Name(),
+		"dir":         dirPath,
+		"startFile":   startFileName,
+	}
+	query := "for d in @@collection"
 	if includeStartFile {
-		query = query + " filter d.name >= \"" + startFileName + "\" "
+		query = query + " filter d.name >= @startFile "
 	} else {
-		query = query + " filter d.name > \"" + startFileName + "\" "
+		query = query + " filter d.name > @startFile "
 	}
 	if prefix != "" {
-		query = query + fmt.Sprintf(`&& starts_with(d.name, "%s")`, prefix)
+		query = query + "&& starts_with(d.name, @prefix)"
+		bindVars["prefix"] = prefix
 	}
 	query = query + `
 filter d.directory == @dir
@@ -313,7 +316,7 @@ sort d.name asc
 		query = query + "limit " + strconv.Itoa(int(limit))
 	}
 	query = query + "\n return d"
-	cur, err := store.database.Query(ctx, query, map[string]interface{}{"dir": dirPath})
+	cur, err := store.database.Query(ctx, query, bindVars)
 	if err != nil {
 		return lastFileName, fmt.Errorf("failed to list directory entries: find error: %w", err)
 	}
@@ -0,0 +1,195 @@
+package arangodb
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/arangodb/go-driver"
+	"github.com/seaweedfs/seaweedfs/weed/filer"
+	"github.com/seaweedfs/seaweedfs/weed/util"
+)
+
+func newTestStore(t *testing.T) *ArangodbStore {
+	t.Helper()
+
+	if os.Getenv("RUN_ARANGODB_TESTS") != "1" {
+		t.Skip("arangodb tests are disabled. Start an arangodb server and set RUN_ARANGODB_TESTS=1 to enable, ARANGODB_ADDR defaults to http://127.0.0.1:8529.")
+	}
+
+	addr := os.Getenv("ARANGODB_ADDR")
+	if addr == "" {
+		addr = "http://127.0.0.1:8529"
+	}
+	user := os.Getenv("ARANGODB_USER")
+	if user == "" {
+		user = "root"
+	}
+
+	store := &ArangodbStore{databaseName: fmt.Sprintf("seaweed_test_%d", time.Now().UnixNano())}
+	store.buckets = make(map[string]driver.Collection, 3)
+	if err := store.connection([]string{addr}, user, os.Getenv("ARANGODB_PASSWORD"), true); err != nil {
+		t.Fatalf("connect to arangodb at %s: %v", addr, err)
+	}
+	t.Cleanup(func() {
+		if err := store.database.Remove(context.Background()); err != nil {
+			t.Errorf("drop test database: %v", err)
+		}
+	})
+	return store
+}
+
+func insertTestEntry(t *testing.T, store *ArangodbStore, path string) {
+	t.Helper()
+	if err := store.InsertEntry(context.Background(), &filer.Entry{FullPath: util.FullPath(path)}); err != nil {
+		t.Fatalf("insert %s: %v", path, err)
+	}
+}
+
+func countDocuments(t *testing.T, store *ArangodbStore, bucket string) int64 {
+	t.Helper()
+	collection, err := store.ensureBucket(context.Background(), bucket)
+	if err != nil {
+		t.Fatalf("ensure bucket %s: %v", bucket, err)
+	}
+	count, err := collection.Count(context.Background())
+	if err != nil {
+		t.Fatalf("count %s: %v", bucket, err)
+	}
+	return count
+}
+
+func listNames(t *testing.T, store *ArangodbStore, dir util.FullPath, prefix string) []string {
+	t.Helper()
+	var names []string
+	_, err := store.ListDirectoryPrefixedEntries(context.Background(), dir, "", true, 100, prefix,
+		func(entry *filer.Entry) (bool, error) {
+			names = append(names, entry.Name())
+			return true, nil
+		})
+	if err != nil {
+		t.Fatalf("list %s prefix %q: %v", dir, prefix, err)
+	}
+	return names
+}
+
+// AQL operators in a list prefix must be matched literally, never executed.
+func TestListDirectoryPrefixedEntriesAqlInjection(t *testing.T) {
+	store := newTestStore(t)
+
+	insertTestEntry(t, store, "/buckets/victim/secret")
+	insertTestEntry(t, store, "/buckets/tenant/regular")
+
+	injection := "x\" && false || (FOR q IN `victim` LIMIT 1 UPDATE q WITH {injected:\"yes\"} IN `victim` RETURN \"\") || \""
+	if names := listNames(t, store, util.FullPath("/buckets/tenant"), injection); len(names) != 0 {
+		t.Errorf("expected no match for injected prefix, got %v", names)
+	}
+
+	var victim Model
+	collection, err := store.ensureBucket(context.Background(), "victim")
+	if err != nil {
+		t.Fatalf("ensure bucket victim: %v", err)
+	}
+	if _, err := collection.ReadDocument(context.Background(), hashString("/buckets/victim/secret"), &victim); err != nil {
+		t.Fatalf("read victim document: %v", err)
+	}
+	if victim.Name != "secret" {
+		t.Errorf("victim document was modified across buckets: %+v", victim)
+	}
+}
+
+// A prefix full of AQL operators must not be evaluated as a query.
+func TestListDirectoryPrefixedEntriesPrefixNotEvaluated(t *testing.T) {
+	store := newTestStore(t)
+
+	insertTestEntry(t, store, "/buckets/tenant/a")
+	insertTestEntry(t, store, "/buckets/tenant/b")
+	insertTestEntry(t, store, "/buckets/tenant/c")
+
+	start := time.Now()
+	if names := listNames(t, store, util.FullPath("/buckets/tenant"), "x\" && false || TO_STRING(SLEEP(1)) && false || \""); len(names) != 0 {
+		t.Errorf("expected no match for injected prefix, got %v", names)
+	}
+	if elapsed := time.Since(start); elapsed > time.Second {
+		t.Errorf("SLEEP() in prefix was executed, listing took %v", elapsed)
+	}
+}
+
+// Quotes and newlines are ordinary object-name characters and must round-trip.
+func TestListDirectoryPrefixedEntriesQuotedNames(t *testing.T) {
+	store := newTestStore(t)
+
+	names := []string{"plain", "quo\"te", "new\nline", "back\\slash"}
+	for _, name := range names {
+		insertTestEntry(t, store, "/buckets/tenant/"+name)
+	}
+
+	for _, name := range names {
+		if got := listNames(t, store, util.FullPath("/buckets/tenant"), name); len(got) != 1 || got[0] != name {
+			t.Errorf("prefix %q listed %v, want exactly [%q]", name, got, name)
+		}
+	}
+
+	if got := listNames(t, store, util.FullPath("/buckets/tenant"), ""); len(got) != len(names) {
+		t.Errorf("empty prefix listed %v, want %d entries", got, len(names))
+	}
+}
+
+// A directory name full of AQL operators must only drop that directory's own children.
+func TestDeleteFolderChildrenAqlInjection(t *testing.T) {
+	store := newTestStore(t)
+
+	for i := 0; i < 5; i++ {
+		insertTestEntry(t, store, fmt.Sprintf("/buckets/tenant/keep%d", i))
+	}
+	injection := "x\" || true || \""
+	insertTestEntry(t, store, "/buckets/tenant/"+injection+"/child")
+
+	if err := store.DeleteFolderChildren(context.Background(), util.FullPath("/buckets/tenant/"+injection)); err != nil {
+		t.Fatalf("delete folder children: %v", err)
+	}
+
+	if got := countDocuments(t, store, "tenant"); got != 5 {
+		t.Errorf("tenant collection holds %d documents, want the 5 unrelated ones", got)
+	}
+}
+
+func TestDeleteFolderChildrenRemovesSubtree(t *testing.T) {
+	store := newTestStore(t)
+
+	insertTestEntry(t, store, "/buckets/tenant/dir/file")
+	insertTestEntry(t, store, "/buckets/tenant/dir/sub/deep")
+	insertTestEntry(t, store, "/buckets/tenant/dirX/sibling")
+	insertTestEntry(t, store, "/buckets/tenant/other")
+
+	if err := store.DeleteFolderChildren(context.Background(), util.FullPath("/buckets/tenant/dir")); err != nil {
+		t.Fatalf("delete folder children: %v", err)
+	}
+
+	if got := countDocuments(t, store, "tenant"); got != 2 {
+		t.Errorf("tenant collection holds %d documents, want /buckets/tenant/dirX/sibling and /buckets/tenant/other", got)
+	}
+}
+
+// startFileName is request-controlled too and must not break out of the filter.
+func TestListDirectoryEntriesQuotedStartFileName(t *testing.T) {
+	store := newTestStore(t)
+
+	insertTestEntry(t, store, "/buckets/tenant/a")
+	insertTestEntry(t, store, "/buckets/tenant/z")
+
+	var names []string
+	_, err := store.ListDirectoryEntries(context.Background(), util.FullPath("/buckets/tenant"), "z\" || true || \"", false, 100,
+		func(entry *filer.Entry) (bool, error) {
+			names = append(names, entry.Name())
+			return true, nil
+		})
+	if err != nil {
+		t.Fatalf("list with quoted start file: %v", err)
+	}
+	if len(names) != 0 {
+		t.Errorf("listed %v, want nothing sorted after the literal start name", names)
+	}
+}