From b6fbedde4ad33709faf8fb1d12d277b322d24dc3 Mon Sep 17 00:00:00 2001 From: chrislu Date: Mon, 14 Jul 2025 22:48:37 -0700 Subject: [PATCH] address comments --- test/s3/cors/s3_cors_http_test.go | 67 +++++++++++++++++++++++++++++++ test/s3/cors/test_config.json | 9 ----- weed/s3api/s3api_bucket_config.go | 5 +++ weed/s3api/s3api_server.go | 3 +- 4 files changed, 74 insertions(+), 10 deletions(-) delete mode 100644 test/s3/cors/test_config.json diff --git a/test/s3/cors/s3_cors_http_test.go b/test/s3/cors/s3_cors_http_test.go index dc9aa44ad..b94caef27 100644 --- a/test/s3/cors/s3_cors_http_test.go +++ b/test/s3/cors/s3_cors_http_test.go @@ -467,3 +467,70 @@ func TestCORSMultipleRulesMatching(t *testing.T) { assert.Contains(t, resp2.Header.Get("Access-Control-Allow-Headers"), "Authorization", "Should allow Authorization header") assert.Equal(t, "7200", resp2.Header.Get("Access-Control-Max-Age"), "Should have second rule's max age") } + +// TestServiceLevelCORS tests that service-level endpoints (like /status) get proper CORS headers +func TestServiceLevelCORS(t *testing.T) { + assert := assert.New(t) + + endpoints := []string{ + "/", + "/status", + "/healthz", + } + + for _, endpoint := range endpoints { + t.Run(fmt.Sprintf("endpoint_%s", strings.ReplaceAll(endpoint, "/", "_")), func(t *testing.T) { + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s%s", getDefaultConfig().Endpoint, endpoint), nil) + assert.NoError(err) + + // Add Origin header to trigger CORS + req.Header.Set("Origin", "http://example.com") + + client := &http.Client{} + resp, err := client.Do(req) + assert.NoError(err) + defer resp.Body.Close() + + // Should return 200 OK + assert.Equal(http.StatusOK, resp.StatusCode) + + // Should have CORS headers set + assert.Equal("*", resp.Header.Get("Access-Control-Allow-Origin")) + assert.Equal("*", resp.Header.Get("Access-Control-Expose-Headers")) + assert.Equal("*", resp.Header.Get("Access-Control-Allow-Methods")) + assert.Equal("*", resp.Header.Get("Access-Control-Allow-Headers")) + }) + } +} + +// TestServiceLevelCORSWithoutOrigin tests that service-level endpoints without Origin header don't get CORS headers +func TestServiceLevelCORSWithoutOrigin(t *testing.T) { + assert := assert.New(t) + + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/status", getDefaultConfig().Endpoint), nil) + assert.NoError(err) + + // No Origin header + + client := &http.Client{} + resp, err := client.Do(req) + assert.NoError(err) + defer resp.Body.Close() + + // Should return 200 OK + assert.Equal(http.StatusOK, resp.StatusCode) + + // Should not have CORS headers set (or have empty values) + corsHeaders := []string{ + "Access-Control-Allow-Origin", + "Access-Control-Expose-Headers", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + } + + for _, header := range corsHeaders { + value := resp.Header.Get(header) + // Headers should either be empty or not present + assert.True(value == "" || value == "*", "Header %s should be empty or wildcard, got: %s", header, value) + } +} diff --git a/test/s3/cors/test_config.json b/test/s3/cors/test_config.json deleted file mode 100644 index b9b8f2e15..000000000 --- a/test/s3/cors/test_config.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "endpoint": "http://localhost:8333", - "access_key": "some_access_key1", - "secret_key": "some_secret_key1", - "region": "us-east-1", - "bucket_prefix": "test-cors-", - "use_ssl": false, - "skip_verify_ssl": true -} \ No newline at end of file diff --git a/weed/s3api/s3api_bucket_config.go b/weed/s3api/s3api_bucket_config.go index 61f0f7f62..2cd4e9a18 100644 --- a/weed/s3api/s3api_bucket_config.go +++ b/weed/s3api/s3api_bucket_config.go @@ -273,26 +273,31 @@ func (s3a *S3ApiServer) loadCORSFromMetadata(bucket string) (*cors.CORSConfigura entry, err := s3a.getEntry("", bucketMetadataPath) if err != nil || entry == nil { + glog.V(3).Infof("loadCORSFromMetadata: no metadata found for bucket %s: %v", bucket, err) return nil, fmt.Errorf("no metadata found") } if len(entry.Content) == 0 { + glog.V(3).Infof("loadCORSFromMetadata: empty metadata content for bucket %s", bucket) return nil, fmt.Errorf("no metadata content") } var metadata map[string]json.RawMessage if err := json.Unmarshal(entry.Content, &metadata); err != nil { + glog.Errorf("loadCORSFromMetadata: failed to unmarshal metadata for bucket %s: %v", bucket, err) return nil, fmt.Errorf("failed to unmarshal metadata: %v", err) } corsData, exists := metadata["cors"] if !exists { + glog.V(3).Infof("loadCORSFromMetadata: no CORS configuration found for bucket %s", bucket) return nil, fmt.Errorf("no CORS configuration found") } // Directly unmarshal the raw JSON to CORSConfiguration to avoid round-trip allocations var config cors.CORSConfiguration if err := json.Unmarshal(corsData, &config); err != nil { + glog.Errorf("loadCORSFromMetadata: failed to unmarshal CORS configuration for bucket %s: %v", bucket, err) return nil, fmt.Errorf("failed to unmarshal CORS configuration: %v", err) } diff --git a/weed/s3api/s3api_server.go b/weed/s3api/s3api_server.go index bd2a31e6a..9e9adcfca 100644 --- a/weed/s3api/s3api_server.go +++ b/weed/s3api/s3api_server.go @@ -186,7 +186,8 @@ func (s3a *S3ApiServer) registerRouter(router *mux.Router) { bucket.Use(corsMiddleware.Handler) // Bucket-specific OPTIONS handler for CORS preflight requests - bucket.Methods(http.MethodOptions).HandlerFunc(corsMiddleware.HandleOptionsRequest) + // Use PathPrefix to catch all bucket-level preflight routes including /bucket/object + bucket.PathPrefix("/").Methods(http.MethodOptions).HandlerFunc(corsMiddleware.HandleOptionsRequest) // each case should follow the next rule: // - requesting object with query must precede any other methods