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.
2026-01-22 06:04:57 +08:00
parent cd2e93bf2b
commit 52882aed70
4 changed files with 199 additions and 87 deletions
--- 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 != "" {
--- 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",
 			},
 		},
 	}
--- 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 == "" {
+		m.processCORS(w, r, func() {
 			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
-			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)
-
-	// If not a CORS request, return OK
-	if corsReq.Origin == "" {
-		w.WriteHeader(http.StatusOK)
-		return
-	}
-
-	// Extract bucket from request
 	bucket, _ := s3_constants.GetBucketAndObject(r)
+
+	// 1. Basic Validation
 	if bucket == "" {
-		w.WriteHeader(http.StatusOK)
+		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
+	// 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
+	}
+
+	// 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
 		}
+		next()
+		return
+	}
+
+	// 7. Success Case
+	ApplyHeaders(w, corsResp)
+
+	if corsReq.IsPreflightRequest {
 		w.WriteHeader(http.StatusOK)
 		return
 	}

-	// Apply CORS headers and return success
-	ApplyHeaders(w, corsResp)
-	w.WriteHeader(http.StatusOK)
+	next()
 }
--- 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
+}