From 11e5049573dbbdddee52e8a5609cb9ed86049d1f Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Thu, 5 Feb 2026 16:54:46 -0800 Subject: [PATCH] fix: remove duplicate cors headers from options router In the refactor for being able to set global CORS headers, the options router was incorrectly set to use both CORS middlewares casuing duplicate headers to be set. The ApplyBucketCORS() middleware is not needed for options since this is already handled by the CORSOoptions controller. Fixes #1819 --- s3api/router.go | 2 - s3api/router_cors_test.go | 119 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) diff --git a/s3api/router.go b/s3api/router.go index 30f2f6e..8816f49 100644 --- a/s3api/router.go +++ b/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), ), ) diff --git a/s3api/router_cors_test.go b/s3api/router_cors_test.go index 2999e4c..e83ca36 100644 --- a/s3api/router_cors_test.go +++ b/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(` + + http://localhost:3333 + GET + HEAD + PUT + POST + content-length + content-type + + `) + + 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") + } +}