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.
This commit is contained in:
Chris Lu
2026-08-27 16:28:02 -07:00
committed by GitHub
parent 0b5fff2ccd
commit ab8b34720a
2 changed files with 36 additions and 0 deletions
@@ -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")
}
+10
View File
@@ -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