From 52882aed70b8b882e7cfd569e57ad7dab9b7dd01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=B2=92=E7=B2=92=E6=A9=99?= Date: Thu, 22 Jan 2026 06:04:57 +0800 Subject: [PATCH] fix(s3api): missing `Vary: Origin` header on non-CORS and `OPTIONS` requests (#8072) * fix: Refactor CORS middleware to consistently apply the `Vary: Origin` header when a configuration exists and streamline request processing logic. * fix: Add Vary: Origin header to CORS OPTIONS responses and refactor request handling for clarity and correctness. * fix: update CORS middleware tests to correctly parse and check for 'Origin' in Vary header. * refactor: extract `hasVaryOrigin` helper function to simplify Vary header checks in tests. * test: Remove `Vary: Origin` header from CORS test expectations. * refactor: consolidate CORS request handling into a new `processCORS` method using a `next` callback. --- weed/s3api/cors/cors.go | 4 - weed/s3api/cors/cors_test.go | 2 - weed/s3api/cors/middleware.go | 117 +++++++-------------- weed/s3api/cors/middleware_test.go | 159 +++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 85 deletions(-) diff --git a/weed/s3api/cors/cors.go b/weed/s3api/cors/cors.go index ac9e7cca3..d6eb520af 100644 --- a/weed/s3api/cors/cors.go +++ b/weed/s3api/cors/cors.go @@ -361,10 +361,6 @@ func ApplyHeaders(w http.ResponseWriter, corsResp *CORSResponse) { if corsResp.AllowOrigin != "" { w.Header().Set("Access-Control-Allow-Origin", corsResp.AllowOrigin) - - if corsResp.AllowOrigin != "*" { - w.Header().Add("Vary", "Origin") - } } if corsResp.AllowMethods != "" { diff --git a/weed/s3api/cors/cors_test.go b/weed/s3api/cors/cors_test.go index 7b72ee482..33d7b11b8 100644 --- a/weed/s3api/cors/cors_test.go +++ b/weed/s3api/cors/cors_test.go @@ -523,7 +523,6 @@ func TestApplyHeaders(t *testing.T) { "Access-Control-Allow-Headers": "Content-Type", "Access-Control-Expose-Headers": "ETag", "Access-Control-Max-Age": "3600", - "Vary": "Origin", }, }, { @@ -537,7 +536,6 @@ func TestApplyHeaders(t *testing.T) { "Access-Control-Allow-Origin": "http://example.com", "Access-Control-Allow-Methods": "GET", "Access-Control-Allow-Credentials": "true", - "Vary": "Origin", }, }, } diff --git a/weed/s3api/cors/middleware.go b/weed/s3api/cors/middleware.go index c3542b1f0..8ecac8b81 100644 --- a/weed/s3api/cors/middleware.go +++ b/weed/s3api/cors/middleware.go @@ -69,103 +69,56 @@ func (m *Middleware) getCORSConfig(bucket string) (*CORSConfiguration, bool) { // Handler returns the CORS middleware handler func (m *Middleware) Handler(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Parse CORS request - corsReq := ParseRequest(r) - - // If not a CORS request, continue normally - if corsReq.Origin == "" { - next.ServeHTTP(w, r) - return - } - - // Extract bucket from request - bucket, _ := s3_constants.GetBucketAndObject(r) - if bucket == "" { - next.ServeHTTP(w, r) - return - } - - // Get CORS configuration (bucket-specific or fallback) BEFORE checking bucket existence - // This ensures CORS headers are applied consistently regardless of bucket existence, - // preventing information disclosure about whether a bucket exists - config, found := m.getCORSConfig(bucket) - if !found { - // No CORS configuration at all, handle based on request type - if corsReq.IsPreflightRequest { - // Preflight request without CORS config should fail - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - // Non-preflight request, continue normally - next.ServeHTTP(w, r) - return - } - - // Evaluate CORS request - corsResp, err := EvaluateRequest(config, corsReq) - if err != nil { - glog.V(3).Infof("CORS evaluation failed for bucket %s: %v", bucket, err) - if corsReq.IsPreflightRequest { - // Preflight request that doesn't match CORS rules should fail - s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) - return - } - // Non-preflight request, continue normally but without CORS headers + m.processCORS(w, r, func() { next.ServeHTTP(w, r) - return - } - - // Apply CORS headers early, before bucket existence check - // This ensures consistent CORS behavior and prevents information disclosure - ApplyHeaders(w, corsResp) - - // Handle preflight requests - return success immediately without checking bucket existence - // This matches AWS S3 behavior where preflight requests succeed even for non-existent buckets - if corsReq.IsPreflightRequest { - // Preflight request should return 200 OK with just CORS headers - w.WriteHeader(http.StatusOK) - return - } - - // For actual requests, continue with normal processing - // The handler will check bucket existence and return appropriate errors (e.g., NoSuchBucket) - // but CORS headers have already been applied above - next.ServeHTTP(w, r) + }) }) } // HandleOptionsRequest handles OPTIONS requests for CORS preflight func (m *Middleware) HandleOptionsRequest(w http.ResponseWriter, r *http.Request) { - // Parse CORS request + m.processCORS(w, r, func() { + w.WriteHeader(http.StatusOK) + }) +} + +// processCORS handles the common CORS logic for both regular and preflight requests. +// It takes a next callback which is executed if the request flow proceeds (i.e., not short-circuited by CORS errors or preflight responses). +func (m *Middleware) processCORS(w http.ResponseWriter, r *http.Request, next func()) { corsReq := ParseRequest(r) + bucket, _ := s3_constants.GetBucketAndObject(r) - // If not a CORS request, return OK - if corsReq.Origin == "" { - w.WriteHeader(http.StatusOK) + // 1. Basic Validation + if bucket == "" { + next() return } - // Extract bucket from request - bucket, _ := s3_constants.GetBucketAndObject(r) - if bucket == "" { - w.WriteHeader(http.StatusOK) + // 2. Load Configuration + config, hasConfig := m.getCORSConfig(bucket) + + // 3. Apply Vary Header (Always applied if config exists) + if hasConfig { + w.Header().Add("Vary", "Origin") + } + + // 4. Handle Non-CORS Requests + if corsReq.Origin == "" { + next() return } - // Get CORS configuration (bucket-specific or fallback) BEFORE checking bucket existence - // This ensures CORS headers are applied consistently regardless of bucket existence - config, found := m.getCORSConfig(bucket) - if !found { - // No CORS configuration at all for OPTIONS request should return access denied + // 5. Handle Missing Configuration + if !hasConfig { if corsReq.IsPreflightRequest { s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) return } - w.WriteHeader(http.StatusOK) + next() return } - // Evaluate CORS request + // 6. Evaluate CORS Request corsResp, err := EvaluateRequest(config, corsReq) if err != nil { glog.V(3).Infof("CORS evaluation failed for bucket %s: %v", bucket, err) @@ -173,11 +126,17 @@ func (m *Middleware) HandleOptionsRequest(w http.ResponseWriter, r *http.Request s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) return } - w.WriteHeader(http.StatusOK) + next() return } - // Apply CORS headers and return success + // 7. Success Case ApplyHeaders(w, corsResp) - w.WriteHeader(http.StatusOK) + + if corsReq.IsPreflightRequest { + w.WriteHeader(http.StatusOK) + return + } + + next() } diff --git a/weed/s3api/cors/middleware_test.go b/weed/s3api/cors/middleware_test.go index 98f7940be..59d00f447 100644 --- a/weed/s3api/cors/middleware_test.go +++ b/weed/s3api/cors/middleware_test.go @@ -3,6 +3,7 @@ package cors import ( "net/http" "net/http/httptest" + "strings" "testing" "github.com/gorilla/mux" @@ -403,3 +404,161 @@ func TestMiddlewareFallbackWithError(t *testing.T) { }) } } + +// TestMiddlewareVaryHeader tests that the Vary: Origin header is correctly applied in various scenarios +func TestMiddlewareVaryHeader(t *testing.T) { + tests := []struct { + name string + bucketConfig *CORSConfiguration + shouldHaveVaryHeader bool + description string + }{ + { + name: "Specific allowed origin", + bucketConfig: &CORSConfiguration{ + CORSRules: []CORSRule{ + { + AllowedOrigins: []string{"https://example.com"}, + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"*"}, + }, + }, + }, + shouldHaveVaryHeader: true, + description: "Should have Vary: Origin header when CORS config exists with specific origin", + }, + { + name: "Wildcard allowed origin", + bucketConfig: &CORSConfiguration{ + CORSRules: []CORSRule{ + { + AllowedOrigins: []string{"*"}, + AllowedMethods: []string{"GET"}, + AllowedHeaders: []string{"*"}, + }, + }, + }, + shouldHaveVaryHeader: true, + description: "Should have Vary: Origin header even when CORS config has wildcard origin", + }, + { + name: "No CORS configuration", + bucketConfig: nil, + shouldHaveVaryHeader: false, + description: "Should NOT have Vary: Origin header when no CORS config exists", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup mocks + bucketChecker := &mockBucketChecker{bucketExists: true} + + var errCode s3err.ErrorCode + if tt.bucketConfig == nil { + errCode = s3err.ErrNoSuchCORSConfiguration + } else { + errCode = s3err.ErrNone + } + + configGetter := &mockCORSConfigGetter{ + config: tt.bucketConfig, + errCode: errCode, + } + + // Create middleware + middleware := NewMiddleware(bucketChecker, configGetter, nil) + + // Create request WITHOUT Origin header + req := httptest.NewRequest("GET", "/testbucket/testobject", nil) + req = mux.SetURLVars(req, map[string]string{ + "bucket": "testbucket", + "object": "testobject", + }) + + // Create response recorder + w := httptest.NewRecorder() + + // Create a simple handler that returns 200 OK + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + // Execute middleware + middleware.Handler(nextHandler).ServeHTTP(w, req) + + // Check Vary header + hasVaryOrigin := hasVaryOrigin(w.Header()) + + if tt.shouldHaveVaryHeader && !hasVaryOrigin { + t.Errorf("%s: expected Vary: Origin header, but got %v", tt.description, w.Header()["Vary"]) + } else if !tt.shouldHaveVaryHeader && hasVaryOrigin { + t.Errorf("%s: expected NO Vary: Origin header, but got it", tt.description) + } + }) + } +} + +// TestHandleOptionsRequestVaryHeader reproduces the issue where HandleOptionsRequest misses Vary: Origin +func TestHandleOptionsRequestVaryHeader(t *testing.T) { + // Setup mocks + bucketChecker := &mockBucketChecker{bucketExists: true} + + config := &CORSConfiguration{ + CORSRules: []CORSRule{ + { + AllowedOrigins: []string{"https://example.com"}, + AllowedMethods: []string{"GET", "POST"}, + AllowedHeaders: []string{"*"}, + }, + }, + } + + configGetter := &mockCORSConfigGetter{ + config: config, + errCode: s3err.ErrNone, + } + + // Create middleware + middleware := NewMiddleware(bucketChecker, configGetter, nil) + + // Create OPTIONS request + req := httptest.NewRequest("OPTIONS", "/testbucket/testobject", nil) + req = mux.SetURLVars(req, map[string]string{ + "bucket": "testbucket", + "object": "testobject", + }) + + // Set valid CORS headers + req.Header.Set("Origin", "https://example.com") + req.Header.Set("Access-Control-Request-Method", "GET") + + // Create response recorder + w := httptest.NewRecorder() + + // Execute HandleOptionsRequest + middleware.HandleOptionsRequest(w, req) + + // Check Response Status + if w.Code != http.StatusOK { + t.Errorf("Expected status 200, got %d", w.Code) + } + + // Check Vary header - verified to FAIL without fix + if !hasVaryOrigin(w.Header()) { + t.Errorf("Expected Vary: Origin header in OPTIONS response, but got %v", w.Header()["Vary"]) + } +} + +// hasVaryOrigin checks if the "Vary" header contains "Origin" +func hasVaryOrigin(header http.Header) bool { + varyHeaders := header["Vary"] + for _, h := range varyHeaders { + for _, v := range strings.Split(h, ",") { + if strings.TrimSpace(v) == "Origin" { + return true + } + } + } + return false +}