From 632eb4729a6cbc8d725f9c6519a15d6496e29363 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 2 Sep 2026 14:06:56 +0800 Subject: [PATCH] refactor: serve non-resident buckets with the global CORS policy The pre-authentication CORS lookup stays resident-only, so client-supplied path segments still cause no metadata I/O and no cache growth. The fail-closed states for startup, load failures, and the internal namespace are gone: CORS is a browser response policy rather than an authorization boundary, and failing closed only denied browser clients CORS headers while bucket metadata was still loading. A bucket whose stored CORS document does not parse still gets no CORS headers. This removes the loadFailed bookkeeping and the unused GetCorsConfig, HasAllowedOrigin, and generic Update path for CORS; tests use the CORS-specific writer. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PvgysXDmhPBBimCReYtA8q Signed-off-by: Feng Ruohang --- cmd/api-router.go | 20 ++- cmd/bucket-cors-middleware_test.go | 118 ++---------------- cmd/bucket-cors-site-replication_test.go | 10 +- cmd/bucket-metadata-sys.go | 111 +++------------- ...site-replication-status-accounting_test.go | 4 +- internal/bucket/cors/cors.go | 6 - 6 files changed, 42 insertions(+), 227 deletions(-) diff --git a/cmd/api-router.go b/cmd/api-router.go index 18305e427..0f98159db 100644 --- a/cmd/api-router.go +++ b/cmd/api-router.go @@ -461,15 +461,15 @@ func registerAPIRouter(router *mux.Router) { router.Methods(http.MethodPut). HandlerFunc(s3APIMiddleware(api.PutBucketACLHandler)). Queries("acl", "") - // GetBucketCors - this is a dummy call. + // GetBucketCors router.Methods(http.MethodGet). HandlerFunc(s3APIMiddleware(api.GetBucketCorsHandler)). Queries("cors", "") - // PutBucketCors - this is a dummy call. + // PutBucketCors router.Methods(http.MethodPut). HandlerFunc(s3APIMiddleware(api.PutBucketCorsHandler)). Queries("cors", "") - // DeleteBucketCors - this is a dummy call. + // DeleteBucketCors router.Methods(http.MethodDelete). HandlerFunc(s3APIMiddleware(api.DeleteBucketCorsHandler)). Queries("cors", "") @@ -787,15 +787,11 @@ func corsHandler(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Header.Get("Origin") != "" { if bucket, _ := request2BucketObjectName(r); bucket != "" && globalBucketMetadataSys != nil { - // Resident-only lookup: this runs pre-auth for every - // Origin-bearing request using a client-supplied path segment as - // the bucket name. It must never load or cache metadata for - // arbitrary names (see GetResidentCorsConfig). GetResidentCorsConfig - // is the single decision point: it returns errInvalidArgument for - // the internal .minio.sys namespace (fail closed), a config for a - // resident bucket, errBucketMetadataNotInitialized for a real but - // unloaded bucket (fail closed), and errConfigNotFound otherwise - // (fall back to the global policy below). + // Resident-only lookup: this runs before authentication with a + // client-supplied path segment as the bucket name, so it must + // never load or cache metadata. A bucket with a stored CORS + // document that failed to parse gets no CORS headers; any other + // non-resident name falls back to the global policy below. cfg, _, err := globalBucketMetadataSys.GetResidentCorsConfig(bucket) if err == nil && cfg != nil { if applyBucketCors(w, r, cfg) { diff --git a/cmd/bucket-cors-middleware_test.go b/cmd/bucket-cors-middleware_test.go index dd8a934ad..33ffc7dc6 100644 --- a/cmd/bucket-cors-middleware_test.go +++ b/cmd/bucket-cors-middleware_test.go @@ -261,6 +261,11 @@ func TestBucketCorsMetadataErrorFailsClosed(t *testing.T) { oldMetadataSys := globalBucketMetadataSys setObjectLayer(nil) globalBucketMetadataSys = NewBucketMetadataSys() + // A resident bucket whose stored CORS document does not parse must not be + // answered with the global policy: it has a configuration we cannot honor. + meta := newBucketMetadata("cors-metadata-error") + meta.corsConfigErr = fmt.Errorf("invalid bucket CORS configuration") + globalBucketMetadataSys.Set("cors-metadata-error", meta) defer func() { setObjectLayer(oldObjectAPI) globalBucketMetadataSys = oldMetadataSys @@ -593,15 +598,15 @@ func testBucketCorsUnknownBucketDoesNotGrowMetadata(obj ObjectLayer, _ string, _ } } -func TestBucketCorsStartupMissFailsClosedWithoutIO(t *testing.T) { +func TestBucketCorsStartupMissUsesGlobalFallbackWithoutIO(t *testing.T) { ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ t: t, - objAPITest: testBucketCorsStartupMissFailsClosedWithoutIO, + objAPITest: testBucketCorsStartupMissUsesGlobalFallbackWithoutIO, endpoints: []string{"GetBucketCors"}, }) } -func testBucketCorsStartupMissFailsClosedWithoutIO(obj ObjectLayer, _ string, _ string, _ http.Handler, _ auth.Credentials, t *testing.T) { +func testBucketCorsStartupMissUsesGlobalFallbackWithoutIO(obj ObjectLayer, _ string, _ string, _ http.Handler, _ auth.Credentials, t *testing.T) { oldObjectAPI := newObjectLayerFn() oldMetadataSys := globalBucketMetadataSys counting := &corsLookupCountingObjectLayer{ObjectLayer: obj} @@ -625,8 +630,8 @@ func testBucketCorsStartupMissFailsClosedWithoutIO(obj ObjectLayer, _ string, _ if !innerCalled || rec.Code != http.StatusNoContent { t.Fatalf("startup miss did not reach inner handler: called=%v status=%d", innerCalled, rec.Code) } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { - t.Fatalf("startup miss used permissive global CORS: %q", got) + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://app.example.com" { + t.Fatalf("startup miss did not fall back to the global policy: %q", got) } if got := counting.getObjectNInfoCalls.Load(); got != 0 { t.Fatalf("startup miss performed %d metadata reads", got) @@ -635,106 +640,3 @@ func testBucketCorsStartupMissFailsClosedWithoutIO(obj ObjectLayer, _ string, _ t.Fatalf("startup miss grew metadataMap to %d", got) } } - -// markBucketMetadataLoadFailed records a bucket as one whose metadata failed to -// load at startup while the subsystem is Initialized, modeling the degraded -// state where a real bucket is not resident. Returns a restore function. -func markBucketMetadataLoadFailed(t *testing.T, bucket string) func() { - t.Helper() - sys := globalBucketMetadataSys - if sys == nil { - t.Fatal("globalBucketMetadataSys is nil") - } - sys.Lock() - _, had := sys.loadFailed[bucket] - sys.loadFailed[bucket] = struct{}{} - sys.Unlock() - return func() { - sys.Lock() - if !had { - delete(sys.loadFailed, bucket) - } - sys.Unlock() - } -} - -// TestBucketCorsLoadFailedBucketFailsClosed guards P1: a real bucket whose -// metadata could not be loaded at startup (present in loadFailed, subsystem -// Initialized) must NOT be answered with the permissive global CORS policy. We -// cannot rule out a restrictive per-bucket config for it, so it must fail -// closed — without a synchronous disk read. -func TestBucketCorsLoadFailedBucketFailsClosed(t *testing.T) { - ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ - t: t, - objAPITest: testBucketCorsLoadFailedBucketFailsClosed, - endpoints: []string{"GetBucketCors"}, - }) -} - -func testBucketCorsLoadFailedBucketFailsClosed(obj ObjectLayer, _ string, _ string, _ http.Handler, _ auth.Credentials, t *testing.T) { - restoreInit := markBucketMetadataInitialized(t) - defer restoreInit() - restoreFail := markBucketMetadataLoadFailed(t, "strict-cors-bucket") - defer restoreFail() - - oldObjectAPI := newObjectLayerFn() - counting := &corsLookupCountingObjectLayer{ObjectLayer: obj} - setObjectLayer(counting) - defer setObjectLayer(oldObjectAPI) - - wrapped := corsHandler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - - for _, method := range []string{http.MethodGet, http.MethodOptions} { - rec := httptest.NewRecorder() - req := httptest.NewRequest(method, getGetObjectURL("", "strict-cors-bucket", "object"), nil) - req.Header.Set("Origin", "https://app.example.com") - if method == http.MethodOptions { - req.Header.Set("Access-Control-Request-Method", http.MethodGet) - } - wrapped.ServeHTTP(rec, req) - - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { - t.Fatalf("%s: load-failed bucket fell back to global allow-origin %q", method, got) - } - if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { - t.Fatalf("%s: load-failed bucket fell back to global credentials %q", method, got) - } - } - if got := counting.getObjectNInfoCalls.Load(); got != 0 { - t.Fatalf("load-failed CORS lookup performed %d synchronous bucket metadata reads", got) - } -} - -// TestBucketCorsInternalBucketFailsClosed guards P2: an Origin-bearing request -// whose first path segment is the reserved .minio.sys namespace must preserve -// GetConfig's errInvalidArgument semantics and fail closed, not fall back to -// the permissive global CORS policy. -func TestBucketCorsInternalBucketFailsClosed(t *testing.T) { - ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ - t: t, - objAPITest: testBucketCorsInternalBucketFailsClosed, - endpoints: []string{"GetBucketCors"}, - }) -} - -func testBucketCorsInternalBucketFailsClosed(_ ObjectLayer, _ string, _ string, _ http.Handler, _ auth.Credentials, t *testing.T) { - restoreInit := markBucketMetadataInitialized(t) - defer restoreInit() - - wrapped := corsHandler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - })) - rec := httptest.NewRecorder() - req := httptest.NewRequest(http.MethodGet, getGetObjectURL("", minioMetaBucket, "object"), nil) - req.Header.Set("Origin", "https://app.example.com") - wrapped.ServeHTTP(rec, req) - - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { - t.Fatalf("internal bucket fell back to global allow-origin %q", got) - } - if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { - t.Fatalf("internal bucket fell back to global credentials %q", got) - } -} diff --git a/cmd/bucket-cors-site-replication_test.go b/cmd/bucket-cors-site-replication_test.go index f8d673903..64c37330b 100644 --- a/cmd/bucket-cors-site-replication_test.go +++ b/cmd/bucket-cors-site-replication_test.go @@ -85,7 +85,7 @@ func testPeerBucketCorsReplicationOrdering(_ ObjectLayer, _ string, bucket strin if !meta.CorsConfigUpdatedAt.Equal(putAt) { t.Fatalf("peer PUT timestamp = %v, want source time %v", meta.CorsConfigUpdatedAt, putAt) } - cfg, cfgAt, err := globalBucketMetadataSys.GetCorsConfig(bucket) + cfg, cfgAt, err := globalBucketMetadataSys.GetResidentCorsConfig(bucket) if err != nil { t.Fatalf("peer PUT stored raw XML but no parsed config: %v", err) } @@ -165,10 +165,10 @@ func TestSiteReplicationMetaInfoPreservesCorsTombstone(t *testing.T) { func testSiteReplicationMetaInfoPreservesCorsTombstone(obj ObjectLayer, _ string, bucket string, _ http.Handler, _ auth.Credentials, t *testing.T) { ctx := t.Context() - if _, err := globalBucketMetadataSys.Update(ctx, bucket, bucketCorsConfig, []byte(testSiteReplicationCORSDoc)); err != nil { + if _, err := updateLocalBucketCORSMetadata(ctx, obj, bucket, []byte(testSiteReplicationCORSDoc)); err != nil { t.Fatal(err) } - deleteAt, err := globalBucketMetadataSys.Delete(ctx, bucket, bucketCorsConfig) + deleteAt, err := updateLocalBucketCORSMetadata(ctx, obj, bucket, nil) if err != nil { t.Fatal(err) } @@ -918,8 +918,8 @@ func testLegacyInvalidCorsMetadataCanBeDeleted(obj ObjectLayer, _ string, bucket t.Fatalf("legacy CORS state = (%#v, %v), want fail-closed parse error", loaded.corsConfig, loaded.corsConfigErr) } globalBucketMetadataSys.Set(bucket, loaded) - if _, gotAt, err := globalBucketMetadataSys.GetCorsConfig(bucket); err == nil || !gotAt.Equal(legacyAt) { - t.Fatalf("GetCorsConfig = timestamp %v, error %v; want legacy timestamp and error", gotAt, err) + if _, gotAt, err := globalBucketMetadataSys.GetResidentCorsConfig(bucket); err == nil || !gotAt.Equal(legacyAt) { + t.Fatalf("GetResidentCorsConfig = timestamp %v, error %v; want legacy timestamp and error", gotAt, err) } if _, gotAt, err := globalBucketMetadataSys.GetCorsConfigXML(bucket); err == nil || !gotAt.Equal(legacyAt) { t.Fatalf("GetCorsConfigXML = timestamp %v, error %v; want legacy timestamp and error", gotAt, err) diff --git a/cmd/bucket-metadata-sys.go b/cmd/bucket-metadata-sys.go index ab2c4ea3d..982b86e42 100644 --- a/cmd/bucket-metadata-sys.go +++ b/cmd/bucket-metadata-sys.go @@ -51,14 +51,6 @@ type BucketMetadataSys struct { initialized bool group *singleflight.Group metadataMap map[string]BucketMetadata - // loadFailed tracks real buckets whose metadata could not be loaded at - // startup (concurrentLoad) or during a refresh. Such buckets are NOT - // resident in metadataMap even though the subsystem is Initialized, so a - // plain map miss cannot distinguish "not a bucket" from "known bucket whose - // config we could not read". Callers that must fail closed for a real but - // unreadable bucket (e.g. per-bucket CORS) consult this set. It is bounded - // by the number of load failures and is empty in normal operation. - loadFailed map[string]struct{} } // Count returns number of bucket metadata map entries. @@ -75,7 +67,6 @@ func (sys *BucketMetadataSys) Remove(buckets ...string) { for _, bucket := range buckets { sys.group.Forget(bucket) delete(sys.metadataMap, bucket) - delete(sys.loadFailed, bucket) globalBucketMonitor.DeleteBucket(bucket) } sys.Unlock() @@ -93,11 +84,6 @@ func (sys *BucketMetadataSys) RemoveStaleBuckets(diskBuckets set.StringSet) { delete(sys.metadataMap, bucket) globalBucketMonitor.DeleteBucket(bucket) } - for bucket := range sys.loadFailed { - if !diskBuckets.Contains(bucket) { - delete(sys.loadFailed, bucket) - } - } } // Set - sets a new metadata in-memory. @@ -109,7 +95,6 @@ func (sys *BucketMetadataSys) Set(bucket string, meta BucketMetadata) { if !isMinioMetaBucketName(bucket) { sys.Lock() sys.metadataMap[bucket] = meta - delete(sys.loadFailed, bucket) sys.Unlock() } } @@ -163,9 +148,6 @@ func (sys *BucketMetadataSys) updateAndParse(ctx context.Context, bucket string, case bucketTaggingConfig: meta.TaggingConfigXML = configData meta.TaggingConfigUpdatedAt = updatedAt - case bucketCorsConfig: - meta.CorsConfigXML = configData - meta.CorsConfigUpdatedAt = updatedAt case bucketQuotaConfigFile: meta.QuotaConfigJSON = configData meta.QuotaConfigUpdatedAt = updatedAt @@ -415,12 +397,22 @@ func (sys *BucketMetadataSys) GetSSEConfig(bucket string) (*bucketsse.BucketSSEC return meta.sseConfig, meta.EncryptionConfigUpdatedAt, nil } -// GetCorsConfig returns the CORS configuration for the given bucket. -// The returned object must not be modified. -func (sys *BucketMetadataSys) GetCorsConfig(bucket string) (*cors.Config, time.Time, error) { - meta, _, err := sys.GetConfig(GlobalContext, bucket) - if err != nil { - return nil, time.Time{}, err +// GetResidentCorsConfig returns the CORS configuration of a bucket whose +// metadata is already resident in memory. It runs before authentication for +// every Origin-bearing request with a client-supplied path segment, so it +// never loads or caches metadata. A name that is not resident, including any +// real bucket while startup loading is still in progress, reports +// errConfigNotFound and the caller applies the global CORS policy exactly as +// releases without per-bucket CORS did. +func (sys *BucketMetadataSys) GetResidentCorsConfig(bucket string) (*cors.Config, time.Time, error) { + if isReservedOrInvalidBucket(bucket, true) { + return nil, time.Time{}, errConfigNotFound + } + sys.RLock() + meta, ok := sys.metadataMap[bucket] + sys.RUnlock() + if !ok { + return nil, time.Time{}, errConfigNotFound } if meta.corsConfigErr != nil { return nil, meta.CorsConfigUpdatedAt, meta.corsConfigErr @@ -431,65 +423,6 @@ func (sys *BucketMetadataSys) GetCorsConfig(bucket string) (*cors.Config, time.T return meta.corsConfig, meta.CorsConfigUpdatedAt, nil } -// GetResidentCorsConfig returns the CORS configuration for the given bucket -// using only bucket metadata that is already resident in memory. Unlike -// GetCorsConfig it never loads metadata from disk and never caches a new -// entry. -// -// The per-request CORS middleware runs before authentication, for every -// Origin-bearing request, using the validated first path segment as the bucket -// name. -// Routing that through GetCorsConfig (which loads and caches) let an -// unauthenticated client grow metadataMap without bound and trigger an -// erasure metadata probe for every distinct, attacker-controlled, -// non-existent name it sent with an Origin header (e.g. /minio/... , /api/... , -// or random buckets). Every bucket that can carry a CORS document is made -// resident when the document is written (Set) and when metadata is loaded at -// startup (Init/concurrentLoad), so a resident-only read is complete for real -// buckets while costing only an in-memory map lookup for everything else. -// -// While bucket metadata is still loading (not yet Initialized) a non-resident -// bucket returns errBucketMetadataNotInitialized so the caller fails closed -// rather than answering with the permissive global policy for a bucket whose -// restrictive CORS document may simply not be loaded yet. -func (sys *BucketMetadataSys) GetResidentCorsConfig(bucket string) (*cors.Config, time.Time, error) { - if isMinioMetaBucketName(bucket) { - // Preserve GetConfig's semantics for the internal namespace: this is - // not a real bucket, and returning a non-errConfigNotFound error makes - // the CORS middleware fail closed rather than answer for .minio.sys - // with the permissive global policy. - return nil, time.Time{}, errInvalidArgument - } - if isReservedOrInvalidBucket(bucket, true) { - return nil, time.Time{}, errConfigNotFound - } - sys.RLock() - meta, ok := sys.metadataMap[bucket] - _, failed := sys.loadFailed[bucket] - initialized := sys.initialized - sys.RUnlock() - if ok { - if meta.corsConfigErr != nil { - return nil, meta.CorsConfigUpdatedAt, meta.corsConfigErr - } - if meta.corsConfig == nil { - return nil, time.Time{}, errConfigNotFound - } - return meta.corsConfig, meta.CorsConfigUpdatedAt, nil - } - // Not resident. Two cases must not be conflated: - // - metadata is still loading (!initialized), or this is a real bucket - // whose metadata failed to load: we cannot rule out a restrictive CORS - // config, so fail closed rather than answer with the global policy. - // - a fully initialized subsystem with no record of the name: it is not a - // bucket that can carry CORS, so fall back to the global policy without - // loading or caching metadata for an arbitrary, client-supplied name. - if !initialized || failed { - return nil, time.Time{}, errBucketMetadataNotInitialized - } - return nil, time.Time{}, errConfigNotFound -} - // GetCorsConfigXML returns the raw stored CORS configuration XML for the // given bucket, preserving the document exactly as it was PUT (including // the S3 xmlns and any unmodeled elements). @@ -687,13 +620,8 @@ func (sys *BucketMetadataSys) concurrentLoad(ctx context.Context, buckets []stri sys.Lock() for i, meta := range bucketMetas { if errs[i] != nil { - // Real bucket whose metadata could not be loaded: record it so - // consumers that must fail closed (per-bucket CORS) can tell it - // apart from a name that is not a bucket at all. - sys.loadFailed[buckets[i]] = struct{}{} continue } - delete(sys.loadFailed, buckets[i]) sys.metadataMap[buckets[i]] = meta } sys.Unlock() @@ -743,9 +671,6 @@ func (sys *BucketMetadataSys) refreshBucketsMetadataLoop(ctx context.Context) { meta, err := loadBucketMetadata(ctx, sys.objAPI, bucket) if err != nil { internalLogIf(ctx, err, logger.WarningKind) - sys.Lock() - sys.loadFailed[bucket] = struct{}{} - sys.Unlock() wait() // wait to proceed to next entry. continue } @@ -756,8 +681,6 @@ func (sys *BucketMetadataSys) refreshBucketsMetadataLoop(ctx context.Context) { updated = true sys.metadataMap[bucket] = meta } - // A successful (re)load clears any earlier load failure. - delete(sys.loadFailed, bucket) sys.Unlock() if updated { @@ -805,7 +728,6 @@ func (sys *BucketMetadataSys) init(ctx context.Context, buckets []string) { func (sys *BucketMetadataSys) Reset() { sys.Lock() clear(sys.metadataMap) - clear(sys.loadFailed) sys.Unlock() } @@ -813,7 +735,6 @@ func (sys *BucketMetadataSys) Reset() { func NewBucketMetadataSys() *BucketMetadataSys { return &BucketMetadataSys{ metadataMap: make(map[string]BucketMetadata), - loadFailed: make(map[string]struct{}), group: &singleflight.Group{}, } } diff --git a/cmd/site-replication-status-accounting_test.go b/cmd/site-replication-status-accounting_test.go index bebc4b182..7f5d23e03 100644 --- a/cmd/site-replication-status-accounting_test.go +++ b/cmd/site-replication-status-accounting_test.go @@ -69,12 +69,14 @@ func testSiteReplicationStatusAccountsPerSiteAndSurvivesMalformedConfig(obj Obje bucketSSEConfig: sseXML, bucketQuotaConfigFile: quotaJSON, bucketPolicyConfig: policyJSON, - bucketCorsConfig: []byte(testSiteReplicationCORSDoc), } { if _, err := globalBucketMetadataSys.Update(ctx, localBucket, configFile, data); err != nil { t.Fatalf("%s: update %s: %v", instanceType, configFile, err) } } + if _, err := updateLocalBucketCORSMetadata(ctx, obj, localBucket, []byte(testSiteReplicationCORSDoc)); err != nil { + t.Fatalf("%s: update %s: %v", instanceType, bucketCorsConfig, err) + } encode := func(data []byte) *string { encoded := base64.StdEncoding.EncodeToString(data) diff --git a/internal/bucket/cors/cors.go b/internal/bucket/cors/cors.go index f36432144..7d5eb0c4c 100644 --- a/internal/bucket/cors/cors.go +++ b/internal/bucket/cors/cors.go @@ -290,12 +290,6 @@ func (r Rule) matchAllowedOrigin(origin string) (string, bool) { return "", false } -// HasAllowedOrigin reports whether the rule allows the given origin. -func (r Rule) HasAllowedOrigin(origin string) bool { - _, ok := r.matchAllowedOrigin(origin) - return ok -} - // HasAllowedMethod reports whether the rule allows the given HTTP method. func (r Rule) HasAllowedMethod(method string) bool { for _, m := range r.AllowedMethods {