From 3814818537526845e26442f4d1b68fef636ba8ff Mon Sep 17 00:00:00 2001 From: h5vx Date: Tue, 25 Aug 2026 17:32:23 +0500 Subject: [PATCH] fix: address CORS final-review findings (multi-rule preflight, raw GET, e2e test) Signed-off-by: h5vx --- cmd/api-router.go | 9 ++------ cmd/bucket-cors-handlers.go | 9 +------- cmd/bucket-cors-handlers_test.go | 32 ++++++++++++++++++++++++++ cmd/bucket-metadata-sys.go | 14 ++++++++++++ internal/bucket/cors/cors.go | 20 +++++++++++++++++ internal/bucket/cors/cors_test.go | 37 +++++++++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 15 deletions(-) diff --git a/cmd/api-router.go b/cmd/api-router.go index 48d05f228..bd0af5a65 100644 --- a/cmd/api-router.go +++ b/cmd/api-router.go @@ -646,7 +646,6 @@ func registerAPIRouter(router *mux.Router) { apiRouter.MethodNotAllowedHandler = collectAPIStats("methodnotallowed", httpTraceAll(methodNotAllowedHandler("S3"))) } -// corsHandler handler for CORS (Cross Origin Resource Sharing) // applyBucketCors applies a bucket's CORS configuration to the request. // For an OPTIONS preflight it writes the full CORS response and returns true // (request is complete). For an actual request it adds the applicable @@ -664,13 +663,8 @@ func applyBucketCors(w http.ResponseWriter, r *http.Request, cfg *bktcors.Config if isPreflight { method := r.Header.Get("Access-Control-Request-Method") - rule, ok := cfg.MatchRule(origin, method) - if !ok { - writeResponse(w, http.StatusForbidden, nil, mimeNone) - return true - } reqHeaders := splitAndTrim(r.Header.Get("Access-Control-Request-Headers")) - allowedHeaders, ok := rule.FilterAllowedHeaders(reqHeaders) + rule, allowedHeaders, ok := cfg.MatchPreflight(origin, method, reqHeaders) if !ok { writeResponse(w, http.StatusForbidden, nil, mimeNone) return true @@ -720,6 +714,7 @@ func splitAndTrim(s string) []string { return out } +// corsHandler handler for CORS (Cross Origin Resource Sharing) func corsHandler(handler http.Handler) http.Handler { commonS3Headers := []string{ xhttp.Date, diff --git a/cmd/bucket-cors-handlers.go b/cmd/bucket-cors-handlers.go index d38e5f3be..81525e606 100644 --- a/cmd/bucket-cors-handlers.go +++ b/cmd/bucket-cors-handlers.go @@ -20,7 +20,6 @@ package cmd import ( "bytes" "encoding/base64" - "encoding/xml" "errors" "io" "net/http" @@ -136,7 +135,7 @@ func (api objectAPIHandlers) GetBucketCorsHandler(w http.ResponseWriter, r *http return } - config, _, err := globalBucketMetadataSys.GetCorsConfig(bucket) + configData, _, err := globalBucketMetadataSys.GetCorsConfigXML(bucket) if err != nil { if errors.Is(err, errConfigNotFound) { writeErrorResponse(ctx, w, errorCodes.ToAPIErr(ErrNoSuchCORSConfiguration), r.URL) @@ -146,12 +145,6 @@ func (api objectAPIHandlers) GetBucketCorsHandler(w http.ResponseWriter, r *http return } - configData, err := xml.Marshal(config) - if err != nil { - writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL) - return - } - writeSuccessResponseXML(w, configData) } diff --git a/cmd/bucket-cors-handlers_test.go b/cmd/bucket-cors-handlers_test.go index 22932fdaa..f442c2b79 100644 --- a/cmd/bucket-cors-handlers_test.go +++ b/cmd/bucket-cors-handlers_test.go @@ -97,4 +97,36 @@ func testBucketCorsHandlers(obj ObjectLayer, instanceType, bucketName string, ap if rec.Code != http.StatusBadRequest { t.Fatalf("PUT malformed cors: expected 400, got %d", rec.Code) } + + // Re-PUT the config so the store→GetCorsConfig→enforce seam below has + // something to enforce (the earlier DELETE removed it). + req, err = newTestSignedRequestV4(http.MethodPut, getBucketCorsURL("", bucketName), + int64(len(testCORSDoc)), bytes.NewReader([]byte(testCORSDoc)), creds.AccessKey, creds.SecretKey, nil) + if err != nil { + t.Fatal(err) + } + rec = httptest.NewRecorder() + apiRouter.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("PUT cors (re-put): expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + // End-to-end enforcement: drive an OPTIONS preflight through the real + // corsHandler wrapper (not applyBucketCors in isolation), exercising the + // full store -> globalBucketMetadataSys.GetCorsConfig -> enforce seam. + wrapped := corsHandler(apiRouter) + + preflightURL := getBucketCorsURL("", bucketName) + preflightReq := httptest.NewRequest(http.MethodOptions, preflightURL, nil) + preflightReq.Header.Set("Origin", "http://example.com") + preflightReq.Header.Set("Access-Control-Request-Method", http.MethodGet) + + rec = httptest.NewRecorder() + wrapped.ServeHTTP(rec, preflightReq) + if rec.Code != http.StatusOK { + t.Fatalf("OPTIONS preflight via corsHandler: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "http://example.com" { + t.Fatalf("OPTIONS preflight via corsHandler: expected Access-Control-Allow-Origin echoed, got %q", got) + } } diff --git a/cmd/bucket-metadata-sys.go b/cmd/bucket-metadata-sys.go index 65aed178e..cb442ca5e 100644 --- a/cmd/bucket-metadata-sys.go +++ b/cmd/bucket-metadata-sys.go @@ -376,6 +376,20 @@ func (sys *BucketMetadataSys) GetCorsConfig(bucket string) (*cors.Config, time.T return meta.corsConfig, meta.CorsConfigUpdatedAt, nil } +// 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). +func (sys *BucketMetadataSys) GetCorsConfigXML(bucket string) ([]byte, time.Time, error) { + meta, _, err := sys.GetConfig(GlobalContext, bucket) + if err != nil { + return nil, time.Time{}, err + } + if len(meta.CorsConfigXML) == 0 { + return nil, time.Time{}, errConfigNotFound + } + return meta.CorsConfigXML, meta.CorsConfigUpdatedAt, nil +} + // CreatedAt returns the time of creation of bucket func (sys *BucketMetadataSys) CreatedAt(bucket string) (time.Time, error) { meta, _, err := sys.GetConfig(GlobalContext, bucket) diff --git a/internal/bucket/cors/cors.go b/internal/bucket/cors/cors.go index 91822f13b..5fe0fcce9 100644 --- a/internal/bucket/cors/cors.go +++ b/internal/bucket/cors/cors.go @@ -148,3 +148,23 @@ func (c *Config) MatchRule(origin, method string) (*Rule, bool) { } return nil, false } + +// MatchPreflight returns the first rule whose origin and method match and +// whose AllowedHeaders permit every header in reqHeaders. Unlike MatchRule, +// this keeps evaluating subsequent rules until one fully satisfies the +// preflight request, since an earlier origin/method match with a more +// restrictive header list must not shadow a later, more permissive rule. +func (c *Config) MatchPreflight(origin, method string, reqHeaders []string) (rule *Rule, allowedHeaders []string, ok bool) { + for i := range c.CORSRules { + r := &c.CORSRules[i] + if !r.HasAllowedOrigin(origin) || !r.HasAllowedMethod(method) { + continue + } + allowed, headersOK := r.FilterAllowedHeaders(reqHeaders) + if !headersOK { + continue + } + return r, allowed, true + } + return nil, nil, false +} diff --git a/internal/bucket/cors/cors_test.go b/internal/bucket/cors/cors_test.go index a83cd1cfb..a77745b3f 100644 --- a/internal/bucket/cors/cors_test.go +++ b/internal/bucket/cors/cors_test.go @@ -89,3 +89,40 @@ func TestMatching(t *testing.T) { t.Fatal("did not expect authorization to be allowed") } } + +func TestMatchPreflightFallsThroughToLaterRule(t *testing.T) { + // Rule A matches origin+method but only allows a restrictive header set. + // Rule B, listed after A, matches the same origin+method and allows any + // header. A preflight requesting a header only B permits must not be + // rejected just because A was tried first. + const doc = ` + + A-restrictive + https://app.example.com + GET + x-amz-date + + + B-permissive + https://app.example.com + GET + * + +` + + c, err := ParseBucketCorsConfig(strings.NewReader(doc)) + if err != nil { + t.Fatalf("parse failed: %v", err) + } + + rule, allowed, ok := c.MatchPreflight("https://app.example.com", "GET", []string{"x-custom-header"}) + if !ok { + t.Fatal("expected MatchPreflight to succeed via the later, permissive rule") + } + if rule.ID != "B-permissive" { + t.Fatalf("expected rule B-permissive to be selected, got %q", rule.ID) + } + if len(allowed) != 1 || allowed[0] != "x-custom-header" { + t.Fatalf("unexpected allowed headers: %v", allowed) + } +}