From ab8b34720ace7488fab6c190a8c370e121231b2f Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 27 Aug 2026 16:28:02 -0700 Subject: [PATCH] s3tables: delete only the location the dropped table owns (#10986) DeleteTable authorizes the named table, then recursively purges the data path derived from its stored MetadataLocation. That location is supplied by the caller at create/register time and never bound to the table, so a tenant allowed to drop one table could point it at a table in a sibling namespace and have the delete destroy that table's catalog entry and data files. A legitimately decoupled location -- a rename source, or a leftover the name was reused over -- has had its catalog attributes stripped, so a surviving metadata marker identifies a path that belongs to another entry. Refuse those, alongside the existing ancestor refusal. --- .../s3tables/handler_delete_decouple_test.go | 26 +++++++++++++++++++ weed/s3api/s3tables/handler_table.go | 10 +++++++ 2 files changed, 36 insertions(+) diff --git a/weed/s3api/s3tables/handler_delete_decouple_test.go b/weed/s3api/s3tables/handler_delete_decouple_test.go index b26966c86..3a420f00c 100644 --- a/weed/s3api/s3tables/handler_delete_decouple_test.go +++ b/weed/s3api/s3tables/handler_delete_decouple_test.go @@ -94,3 +94,29 @@ func TestDeleteTableColocatedRemovesData(t *testing.T) { assert.Nil(t, fs.Get(GetNamespacePath(renameTestBucket, "ns"), "t"), "colocated table entry must be deleted") assert.Nil(t, fs.Get(GetTablePath(renameTestBucket, "ns", "t"), "metadata"), "colocated table data must be deleted") } + +// A table whose MetadataLocation points at a sibling that is still a live +// catalog entry must not be deleted: only the named table was authorized, so a +// recursive purge of that location would destroy another tenant's table. +func TestDeleteTableRefusesLiveSiblingDataPath(t *testing.T) { + fs, m := startRenameManager(t) + + nsMeta, _ := json.Marshal(namespaceMetadata{Namespace: []string{"attackerns"}, OwnerAccountID: DefaultAccountID}) + fs.Put(GetTableBucketPath(renameTestBucket), "attackerns", map[string][]byte{ExtendedKeyMetadata: nsMeta}) + + decoyMeta, _ := json.Marshal(tableMetadataInternal{ + Name: "decoy", + Namespace: "attackerns", + Format: "ICEBERG", + OwnerAccountID: DefaultAccountID, + MetadataLocation: "s3://" + renameTestBucket + "/ns/t/metadata/v3.metadata.json", + }) + fs.Put(GetNamespacePath(renameTestBucket, "attackerns"), "decoy", map[string][]byte{ExtendedKeyMetadata: decoyMeta}) + + require.Error(t, runDeleteTable(t, m, fs, "attackerns", "decoy")) + + assert.NotNil(t, fs.Get(GetNamespacePath(renameTestBucket, "ns"), "t"), + "the targeted table's catalog entry must survive") + assert.NotNil(t, fs.Get(GetTablePath(renameTestBucket, "ns", "t"), "data"), + "the targeted table's data must survive") +} diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index 4d92a2ae8..b0cef424b 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -986,6 +986,16 @@ func (h *S3TablesHandler) handleDeleteTable(w http.ResponseWriter, r *http.Reque if strings.HasPrefix(tablePath+"/", dataPath+"/") { return fmt.Errorf("refusing to delete table %s: data path %q is an ancestor of catalog path %q", tableName, dataPath, tablePath) } + // The location is caller-supplied, so it may name a sibling that is + // 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. + 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) + case !errors.Is(markerErr, ErrAttributeNotFound) && !errors.Is(markerErr, filer_pb.ErrNotFound): + return fmt.Errorf("refusing to delete table %s: cannot read data path %q: %w", tableName, dataPath, markerErr) + } // Decoupled table (renamed, or created over a leftover): its data // lives elsewhere. Purge the data, then clear the catalog marker // without deleting the name path -- it may still hold another