From 85f04b54fd65aaf4f2f5d796bcb3b76cde2c2db6 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 11 Oct 2026 19:55:58 -0700 Subject: [PATCH] s3tables: check data-path ancestors for catalog markers on DeleteTable The decoupled-delete guard only inspects the resolved data path itself, but the catalog marker lives on a table's name path, so a location that resolves inside another table's directory (its marker-less data subdirectory) slips through and the recursive purge destroys the other table's data. Walk the data path's ancestors within the bucket and refuse when any carries a table or view catalog entry. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../s3tables/handler_delete_decouple_test.go | 48 +++++++++++++++++++ weed/s3api/s3tables/handler_table.go | 21 ++++++++ 2 files changed, 69 insertions(+) diff --git a/weed/s3api/s3tables/handler_delete_decouple_test.go b/weed/s3api/s3tables/handler_delete_decouple_test.go index 3a420f00c..f574caf63 100644 --- a/weed/s3api/s3tables/handler_delete_decouple_test.go +++ b/weed/s3api/s3tables/handler_delete_decouple_test.go @@ -3,6 +3,7 @@ package s3tables import ( "context" "encoding/json" + "path" "testing" "github.com/seaweedfs/seaweedfs/weed/s3api/s3tables/s3tablestest" @@ -120,3 +121,50 @@ func TestDeleteTableRefusesLiveSiblingDataPath(t *testing.T) { assert.NotNil(t, fs.Get(GetTablePath(renameTestBucket, "ns", "t"), "data"), "the targeted table's data must survive") } + +// A location inside another table's directory carries no catalog marker of its +// own — the marker lives on the name path. Dropping a table whose +// MetadataLocation resolves into nsa/vtab/data must be refused by the ancestor +// check, or the victim's marker-less data subdirectory is purged wholesale. +func TestDeleteTableRefusesNestedSiblingDataPath(t *testing.T) { + fs, m := startRenameManager(t) + + nsaMeta, _ := json.Marshal(namespaceMetadata{Namespace: []string{"nsa"}, OwnerAccountID: DefaultAccountID}) + fs.Put(GetTableBucketPath(renameTestBucket), "nsa", map[string][]byte{ExtendedKeyMetadata: nsaMeta}) + + victimMeta, _ := json.Marshal(tableMetadataInternal{ + Name: "vtab", + Namespace: "nsa", + Format: "ICEBERG", + OwnerAccountID: DefaultAccountID, + MetadataLocation: "s3://" + renameTestBucket + "/nsa/vtab/metadata/v1.metadata.json", + }) + victimTablePath := GetTablePath(renameTestBucket, "nsa", "vtab") + fs.Put(GetNamespacePath(renameTestBucket, "nsa"), "vtab", map[string][]byte{ + ExtendedKeyMetadata: victimMeta, + ExtendedKeyEntryType: []byte(EntryTypeTable), + }) + fs.Put(victimTablePath, "data", nil) + fs.Put(path.Join(victimTablePath, "data"), "important-0001.parquet", nil) + + evilMeta, _ := json.Marshal(tableMetadataInternal{ + Name: "evil", + Namespace: "attackerns2", + Format: "ICEBERG", + OwnerAccountID: DefaultAccountID, + MetadataLocation: "s3://" + renameTestBucket + "/nsa/vtab/data/metadata/v1.metadata.json", + }) + nsMeta, _ := json.Marshal(namespaceMetadata{Namespace: []string{"attackerns2"}, OwnerAccountID: DefaultAccountID}) + fs.Put(GetTableBucketPath(renameTestBucket), "attackerns2", map[string][]byte{ExtendedKeyMetadata: nsMeta}) + fs.Put(GetNamespacePath(renameTestBucket, "attackerns2"), "evil", map[string][]byte{ + ExtendedKeyMetadata: evilMeta, + ExtendedKeyEntryType: []byte(EntryTypeTable), + }) + + require.Error(t, runDeleteTable(t, m, fs, "attackerns2", "evil")) + + assert.NotNil(t, fs.Get(path.Join(victimTablePath, "data"), "important-0001.parquet"), + "data inside another table's directory must survive") + assert.NotNil(t, fs.Get(GetNamespacePath(renameTestBucket, "nsa"), "vtab"), + "the victim table's catalog entry must survive") +} diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index d43bca7da..31fdc77fe 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "path" "strconv" "strings" "time" @@ -996,6 +997,26 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque // still a live catalog entry. Only this table's authorization was // checked; a decoupled location has had its catalog attributes // stripped, so a surviving marker means the path belongs elsewhere. + // The marker lives on the entry's name path, so ancestors of the + // data path are checked as well: a location inside another table's + // directory resolves above any marker the data path itself would + // carry. + for dir := path.Dir(dataPath); dir != GetTableBucketPath(bucketName) && strings.HasPrefix(dir, GetTableBucketPath(bucketName)+"/"); dir = path.Dir(dir) { + ancestor, lookupErr := h.lookupEntry(r.Context(), client, dir) + if lookupErr != nil { + if errors.Is(lookupErr, filer_pb.ErrNotFound) { + continue + } + return fmt.Errorf("refusing to delete table %s: cannot read data path %q: %w", tableName, dataPath, lookupErr) + } + if _, ok := ancestor.Extended[ExtendedKeyEntryType]; ok { + return fmt.Errorf("refusing to delete table %s: data path %q is inside another catalog entry", tableName, dataPath) + } + var ownerMeta tableMetadataInternal + if data := ancestor.Extended[ExtendedKeyMetadata]; json.Unmarshal(data, &ownerMeta) == nil && ownerMeta.Name != "" { + return fmt.Errorf("refusing to delete table %s: data path %q is inside another catalog entry", tableName, dataPath) + } + } switch _, markerErr := h.getExtendedAttribute(r.Context(), client, dataPath, ExtendedKeyMetadata); { case markerErr == nil: return fmt.Errorf("refusing to delete table %s: data path %q is another catalog entry", tableName, dataPath)