From 6f93f874462e3bc9d50ce1363c76063769633576 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 11 Oct 2026 19:55:28 -0700 Subject: [PATCH] s3tables: confine caller-supplied metadataLocation to the table's namespace CreateTable/RegisterTable/UpdateTable (and the view equivalents) validate the location only for bucket and traversal, so a tenant can register a table whose location resolves into another namespace's directory and DeleteTable will purge it. Require the location to live under the caller's namespace subtree, the boundary per-namespace delegation uses. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/s3api/s3tables/handler_table.go | 6 +-- weed/s3api/s3tables/handler_view.go | 4 +- weed/s3api/s3tables/utils.go | 15 +++++--- weed/s3api/s3tables/utils_location_test.go | 43 +++++++++++++--------- 4 files changed, 40 insertions(+), 28 deletions(-) diff --git a/weed/s3api/s3tables/handler_table.go b/weed/s3api/s3tables/handler_table.go index c4c5253cf3..d43bca7da2 100644 --- a/weed/s3api/s3tables/handler_table.go +++ b/weed/s3api/s3tables/handler_table.go @@ -100,7 +100,7 @@ func (h *S3TablesHandler) handleCreateTable(w http.ResponseWriter, r *http.Reque return err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName); err != nil { + if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return err } @@ -233,7 +233,7 @@ func (h *S3TablesHandler) handleRegisterTable(w http.ResponseWriter, r *http.Req } bucketName, namespaceName, tableName := target.bucketName, target.namespaceName, target.tableName - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName); err != nil { + if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return err } @@ -1494,7 +1494,7 @@ func (h *S3TablesHandler) handleUpdateTable(w http.ResponseWriter, r *http.Reque return err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName); err != nil { + if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return err } diff --git a/weed/s3api/s3tables/handler_view.go b/weed/s3api/s3tables/handler_view.go index df430cfab2..cd8144a0a2 100644 --- a/weed/s3api/s3tables/handler_view.go +++ b/weed/s3api/s3tables/handler_view.go @@ -121,7 +121,7 @@ func (h *S3TablesHandler) handleCreateView(w http.ResponseWriter, r *http.Reques return err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName); err != nil { + if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return err } @@ -410,7 +410,7 @@ func (h *S3TablesHandler) handleUpdateView(w http.ResponseWriter, r *http.Reques return err } - if err := ValidateMetadataLocation(req.MetadataLocation, bucketName); err != nil { + if err := ValidateMetadataLocation(req.MetadataLocation, bucketName, namespaceName); err != nil { h.writeError(w, http.StatusBadRequest, ErrCodeInvalidRequest, err.Error()) return err } diff --git a/weed/s3api/s3tables/utils.go b/weed/s3api/s3tables/utils.go index 6a2e6980ef..d83e498830 100644 --- a/weed/s3api/s3tables/utils.go +++ b/weed/s3api/s3tables/utils.go @@ -127,11 +127,13 @@ func TableDataDirFromMetadataLocation(metadataLocation string) string { } // ValidateMetadataLocation checks that an s3:// metadata location stays within -// the authorized table bucket and rejects traversal segments that path.Join -// would collapse to escape the bucket directory. Empty locations are allowed -// (the catalog derives one). A non-empty location must include a table path so -// its metadata directory is table-specific, not shared at the bucket level. -func ValidateMetadataLocation(metadataLocation, bucketName string) error { +// the authorized table bucket and inside the caller's own namespace subtree, +// the boundary per-tenant policies are delegated on. Without the namespace +// check a caller could register a location inside another tenant's data +// directory and DeleteTable would purge it. Empty locations are allowed (the +// catalog derives one). A non-empty location must include a table path so its +// metadata directory is table-specific, not shared at the bucket level. +func ValidateMetadataLocation(metadataLocation, bucketName, namespace string) error { if metadataLocation == "" { return nil } @@ -155,6 +157,9 @@ func ValidateMetadataLocation(metadataLocation, bucketName string) error { if !hasSegment { return fmt.Errorf("metadata location must include a table path") } + if !strings.HasPrefix(tablePath, namespace+"/") { + return fmt.Errorf("metadata location must be within namespace %s", namespace) + } return nil } diff --git a/weed/s3api/s3tables/utils_location_test.go b/weed/s3api/s3tables/utils_location_test.go index 7f80365eac..756808a796 100644 --- a/weed/s3api/s3tables/utils_location_test.go +++ b/weed/s3api/s3tables/utils_location_test.go @@ -4,29 +4,36 @@ import "testing" func TestValidateMetadataLocation(t *testing.T) { tests := []struct { - name string - loc string - bucket string - wantErr bool + name string + loc string + bucket string + namespace string + wantErr bool }{ - {"empty allowed", "", "bkt", false}, - {"same bucket", "s3://bkt/ns/t/metadata/v1.metadata.json", "bkt", false}, - {"cross bucket rejected", "s3://other/ns/t/metadata/v1.metadata.json", "bkt", true}, - {"bucket only rejected", "s3://bkt", "bkt", true}, - {"bucket with trailing slash rejected", "s3://bkt/", "bkt", true}, - {"slash-only path rejected", "s3://bkt///", "bkt", true}, - {"dotdot in path rejected", "s3://bkt/../victim/metadata/v1.metadata.json", "bkt", true}, - {"dotdot mid path rejected", "s3://bkt/ns/../../victim/metadata/v1.metadata.json", "bkt", true}, - {"dot segment rejected", "s3://bkt/ns/./t/metadata/v1.metadata.json", "bkt", true}, - {"backslash segment rejected", "s3://bkt/ns/\\t/metadata/v1.metadata.json", "bkt", true}, - {"non-s3 scheme rejected", "file:///bkt/ns/t", "bkt", true}, - {"empty location string", "s3://", "bkt", true}, + {"empty allowed", "", "bkt", "ns", false}, + {"own namespace", "s3://bkt/ns/t/metadata/v1.metadata.json", "bkt", "ns", false}, + {"own namespace arbitrary layout", "s3://bkt/ns/warehouse/t/metadata/v1.metadata.json", "bkt", "ns", false}, + {"cross bucket rejected", "s3://other/ns/t/metadata/v1.metadata.json", "bkt", "ns", true}, + {"cross namespace rejected", "s3://bkt/nsa/t/metadata/v1.metadata.json", "bkt", "ns", true}, + {"cross namespace data dir rejected", "s3://bkt/nsa/vtab/data/metadata/v1.metadata.json", "bkt", "ns", true}, + {"namespace prefix rejected", "s3://bkt/nsa/vtab/metadata/v1.metadata.json", "bkt", "ns", true}, + {"outside namespace tree rejected", "s3://bkt/shared/t/metadata/v1.metadata.json", "bkt", "ns", true}, + {"namespace dir without table path rejected", "s3://bkt/ns", "bkt", "ns", true}, + {"bucket only rejected", "s3://bkt", "bkt", "ns", true}, + {"bucket with trailing slash rejected", "s3://bkt/", "bkt", "ns", true}, + {"slash-only path rejected", "s3://bkt///", "bkt", "ns", true}, + {"dotdot in path rejected", "s3://bkt/../victim/metadata/v1.metadata.json", "bkt", "ns", true}, + {"dotdot mid path rejected", "s3://bkt/ns/../../victim/metadata/v1.metadata.json", "bkt", "ns", true}, + {"dot segment rejected", "s3://bkt/ns/./t/metadata/v1.metadata.json", "bkt", "ns", true}, + {"backslash segment rejected", "s3://bkt/ns/\\t/metadata/v1.metadata.json", "bkt", "ns", true}, + {"non-s3 scheme rejected", "file:///bkt/ns/t", "bkt", "ns", true}, + {"empty location string", "s3://", "bkt", "ns", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidateMetadataLocation(tt.loc, tt.bucket) + err := ValidateMetadataLocation(tt.loc, tt.bucket, tt.namespace) if (err != nil) != tt.wantErr { - t.Fatalf("ValidateMetadataLocation(%q, %q) err = %v, wantErr = %v", tt.loc, tt.bucket, err, tt.wantErr) + t.Fatalf("ValidateMetadataLocation(%q, %q, %q) err = %v, wantErr = %v", tt.loc, tt.bucket, tt.namespace, err, tt.wantErr) } }) }