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") + } +}