Merge pull request #1820 from versity/ben/cors

fix: remove duplicate cors headers from options router

s3api/router.go

@@ -1445,7 +1445,6 @@ func (sa *S3ApiRouter) Init(app *fiber.App, be backend.Backend, iam auth.IAMServ
 		middlewares.ApplyBucketCORSPreflightFallback(be, corsAllowOrigin),
 		controllers.ProcessHandlers(ctrl.CORSOptions, metrics.ActionOptions, services,
 			middlewares.BucketObjectNameValidator(),
-			middlewares.ApplyBucketCORS(be, corsAllowOrigin),
 			middlewares.ParseAcl(be),
 		),
 	)
@@ -1454,7 +1453,6 @@ func (sa *S3ApiRouter) Init(app *fiber.App, be backend.Backend, iam auth.IAMServ
 		middlewares.ApplyBucketCORSPreflightFallback(be, corsAllowOrigin),
 		controllers.ProcessHandlers(ctrl.CORSOptions, metrics.ActionOptions, services,
 			middlewares.BucketObjectNameValidator(),
-			middlewares.ApplyBucketCORS(be, corsAllowOrigin),
 			middlewares.ParseAcl(be),
 		),
 	)

s3api/router_cors_test.go

@@ -16,9 +16,11 @@ package s3api
 
 import (
 	"context"
+	"encoding/json"
 	"net/http"
 	"testing"
 
+	"github.com/aws/aws-sdk-go-v2/service/s3"
 	"github.com/gofiber/fiber/v2"
 	"github.com/versity/versitygw/auth"
 	"github.com/versity/versitygw/backend"
@@ -34,6 +36,26 @@ func (b backendWithCorsOnly) GetBucketCors(ctx context.Context, bucket string) (
 	return nil, s3err.GetAPIError(s3err.ErrNoSuchCORSConfiguration)
 }
 
+type backendWithBucketCors struct {
+	backend.BackendUnsupported
+	corsConfig []byte
+}
+
+func (b backendWithBucketCors) GetBucketCors(ctx context.Context, bucket string) ([]byte, error) {
+	if b.corsConfig != nil {
+		return b.corsConfig, nil
+	}
+	return nil, s3err.GetAPIError(s3err.ErrNoSuchCORSConfiguration)
+}
+
+func (b backendWithBucketCors) GetBucketAcl(ctx context.Context, input *s3.GetBucketAclInput) ([]byte, error) {
+	// Return a minimal ACL in JSON format
+	acl := auth.ACL{
+		Owner: "test",
+	}
+	return json.Marshal(acl)
+}
+
 func TestS3ApiRouter_ListBuckets_DefaultCORSAllowOrigin(t *testing.T) {
 	origin := "https://example.com"
 
@@ -251,3 +273,100 @@ func TestS3ApiRouter_PutObject_ErrorStillIncludesFallbackCORS(t *testing.T) {
 		t.Fatalf("expected Access-Control-Allow-Origin %q, got %q", origin, got)
 	}
 }
+
+func TestS3ApiRouter_OptionsWithBucketCORS_NoDuplicateHeaders(t *testing.T) {
+	// This test reproduces the issue from GitHub issue #1819
+	// where CORS headers were duplicated in OPTIONS responses
+	bucketOrigin := "http://localhost:3333"
+	fallbackOrigin := "https://fallback.example.com"
+
+	// CORS configuration matching the issue reproduction steps
+	corsConfig := []byte(`<CORSConfiguration>
+		<CORSRule>
+			<AllowedOrigin>http://localhost:3333</AllowedOrigin>
+			<AllowedMethod>GET</AllowedMethod>
+			<AllowedMethod>HEAD</AllowedMethod>
+			<AllowedMethod>PUT</AllowedMethod>
+			<AllowedMethod>POST</AllowedMethod>
+			<AllowedHeader>content-length</AllowedHeader>
+			<AllowedHeader>content-type</AllowedHeader>
+		</CORSRule>
+	</CORSConfiguration>`)
+
+	app := fiber.New()
+	(&S3ApiRouter{}).Init(
+		app,
+		backendWithBucketCors{corsConfig: corsConfig},
+		&auth.IAMServiceInternal{},
+		nil,
+		nil,
+		nil,
+		nil,
+		false,
+		"us-east-1",
+		"",
+		middlewares.RootUserConfig{},
+		fallbackOrigin,
+	)
+
+	req, err := http.NewRequest(http.MethodOptions, "/xyz/upload/test.txt", nil)
+	if err != nil {
+		t.Fatalf("new request: %v", err)
+	}
+	req.Header.Set("Origin", bucketOrigin)
+	req.Header.Set("Access-Control-Request-Method", "PUT")
+	req.Header.Set("Access-Control-Request-Headers", "content-type")
+
+	resp, err := app.Test(req)
+	if err != nil {
+		t.Fatalf("app.Test: %v", err)
+	}
+
+	// Verify the response has the correct status
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("expected status %d, got %d", http.StatusOK, resp.StatusCode)
+	}
+
+	// Check that headers are not duplicated
+	headers := resp.Header
+
+	// Helper function to count header occurrences
+	countHeader := func(headerName string) int {
+		values := headers.Values(headerName)
+		return len(values)
+	}
+
+	// Verify each CORS header appears exactly once
+	corsHeaders := []string{
+		"Access-Control-Allow-Origin",
+		"Access-Control-Allow-Methods",
+		"Access-Control-Allow-Headers",
+		"Access-Control-Allow-Credentials",
+		"Vary",
+	}
+
+	for _, header := range corsHeaders {
+		count := countHeader(header)
+		if count > 1 {
+			t.Errorf("Header %q appears %d times (expected 1). Values: %v",
+				header, count, headers.Values(header))
+		}
+	}
+
+	// Verify the bucket CORS values are used (not the fallback)
+	if got := resp.Header.Get("Access-Control-Allow-Origin"); got != bucketOrigin {
+		t.Errorf("expected Access-Control-Allow-Origin %q (from bucket CORS), got %q", bucketOrigin, got)
+	}
+
+	// Verify the requested method is in the allowed methods
+	allowedMethods := resp.Header.Get("Access-Control-Allow-Methods")
+	if allowedMethods == "" {
+		t.Error("Access-Control-Allow-Methods header is empty")
+	}
+
+	// Verify the requested header is in the allowed headers
+	allowedHeaders := resp.Header.Get("Access-Control-Allow-Headers")
+	if allowedHeaders == "" {
+		t.Error("Access-Control-Allow-Headers header is empty")
+	}
+}