From 7c22a2396443a68034ce1876cd9646684a620044 Mon Sep 17 00:00:00 2001 From: chrislu Date: Mon, 14 Jul 2025 22:03:40 -0700 Subject: [PATCH] address some comments --- test/s3/cors/s3_cors_http_test.go | 16 ++++++++-------- test/s3/cors/s3_cors_test.go | 23 ++++++++++++++--------- weed/s3api/cors/middleware.go | 21 --------------------- weed/s3api/s3api_bucket_config.go | 14 ++++++++++++++ weed/s3api/s3api_object_handlers.go | 20 ++++++++++++-------- 5 files changed, 48 insertions(+), 46 deletions(-) diff --git a/test/s3/cors/s3_cors_http_test.go b/test/s3/cors/s3_cors_http_test.go index ea7cf3438..dc9aa44ad 100644 --- a/test/s3/cors/s3_cors_http_test.go +++ b/test/s3/cors/s3_cors_http_test.go @@ -44,7 +44,7 @@ func TestCORSPreflightRequest(t *testing.T) { httpClient := &http.Client{Timeout: 10 * time.Second} // Create OPTIONS request - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") // Add CORS preflight headers @@ -104,7 +104,7 @@ func TestCORSActualRequest(t *testing.T) { // Test GET request with CORS headers using raw HTTP httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("GET", fmt.Sprintf("%s/%s/%s", defaultConfig.Endpoint, bucketName, objectKey), nil) + req, err := http.NewRequest("GET", fmt.Sprintf("%s/%s/%s", getDefaultConfig().Endpoint, bucketName, objectKey), nil) require.NoError(t, err, "Should be able to create GET request") // Add Origin header to simulate CORS request @@ -189,7 +189,7 @@ func TestCORSOriginMatching(t *testing.T) { // Test preflight request httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req.Header.Set("Origin", tc.requestOrigin) @@ -282,7 +282,7 @@ func TestCORSHeaderMatching(t *testing.T) { // Test preflight request httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req.Header.Set("Origin", "https://example.com") @@ -319,7 +319,7 @@ func TestCORSWithoutConfiguration(t *testing.T) { // Test preflight request without CORS configuration httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req.Header.Set("Origin", "https://example.com") @@ -375,7 +375,7 @@ func TestCORSMethodMatching(t *testing.T) { t.Run(fmt.Sprintf("method_%s", tc.method), func(t *testing.T) { httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req.Header.Set("Origin", "https://example.com") @@ -434,7 +434,7 @@ func TestCORSMultipleRulesMatching(t *testing.T) { // Test first rule httpClient := &http.Client{Timeout: 10 * time.Second} - req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req.Header.Set("Origin", "https://example.com") @@ -451,7 +451,7 @@ func TestCORSMultipleRulesMatching(t *testing.T) { assert.Equal(t, "3600", resp.Header.Get("Access-Control-Max-Age"), "Should have first rule's max age") // Test second rule - req2, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", defaultConfig.Endpoint, bucketName), nil) + req2, err := http.NewRequest("OPTIONS", fmt.Sprintf("%s/%s/test-object", getDefaultConfig().Endpoint, bucketName), nil) require.NoError(t, err, "Should be able to create OPTIONS request") req2.Header.Set("Origin", "https://api.example.com") diff --git a/test/s3/cors/s3_cors_test.go b/test/s3/cors/s3_cors_test.go index eeb3c1d7b..8f1745adf 100644 --- a/test/s3/cors/s3_cors_test.go +++ b/test/s3/cors/s3_cors_test.go @@ -28,19 +28,23 @@ type S3TestConfig struct { SkipVerifySSL bool } -// Default test configuration - should match test_config.json -var defaultConfig = &S3TestConfig{ - Endpoint: "http://localhost:8333", // Default SeaweedFS S3 port - AccessKey: "some_access_key1", - SecretKey: "some_secret_key1", - Region: "us-east-1", - BucketPrefix: "test-cors-", - UseSSL: false, - SkipVerifySSL: true, +// getDefaultConfig returns a fresh instance of the default test configuration +// to avoid parallel test issues with global mutable state +func getDefaultConfig() *S3TestConfig { + return &S3TestConfig{ + Endpoint: "http://localhost:8333", // Default SeaweedFS S3 port + AccessKey: "some_access_key1", + SecretKey: "some_secret_key1", + Region: "us-east-1", + BucketPrefix: "test-cors-", + UseSSL: false, + SkipVerifySSL: true, + } } // getS3Client creates an AWS S3 client for testing func getS3Client(t *testing.T) *s3.Client { + defaultConfig := getDefaultConfig() cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithRegion(defaultConfig.Region), config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider( @@ -66,6 +70,7 @@ func getS3Client(t *testing.T) *s3.Client { // createTestBucket creates a test bucket with a unique name func createTestBucket(t *testing.T, client *s3.Client) string { + defaultConfig := getDefaultConfig() bucketName := fmt.Sprintf("%s%d", defaultConfig.BucketPrefix, time.Now().UnixNano()) _, err := client.CreateBucket(context.TODO(), &s3.CreateBucketInput{ diff --git a/weed/s3api/cors/middleware.go b/weed/s3api/cors/middleware.go index eb67dba2d..49cdaae5c 100644 --- a/weed/s3api/cors/middleware.go +++ b/weed/s3api/cors/middleware.go @@ -2,7 +2,6 @@ package cors import ( "net/http" - "strings" "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" @@ -149,23 +148,3 @@ func (m *Middleware) HandleOptionsRequest(w http.ResponseWriter, r *http.Request ApplyHeaders(w, corsResp) w.WriteHeader(http.StatusOK) } - -// ShouldApplyCORS checks if CORS should be applied to the current request -func ShouldApplyCORS(r *http.Request) bool { - // Apply CORS to all bucket and object operations - path := r.URL.Path - - // Skip CORS for service-level operations - if path == "/" || path == "" { - return false - } - - // Skip CORS for bucket listing - if strings.HasPrefix(path, "/") && !strings.Contains(path[1:], "/") { - // This is a bucket-level operation - return true - } - - // This is an object-level operation - return true -} diff --git a/weed/s3api/s3api_bucket_config.go b/weed/s3api/s3api_bucket_config.go index e77e58cfa..57fcafb2e 100644 --- a/weed/s3api/s3api_bucket_config.go +++ b/weed/s3api/s3api_bucket_config.go @@ -3,6 +3,8 @@ package s3api import ( "encoding/json" "fmt" + "path/filepath" + "strings" "sync" "time" @@ -255,6 +257,18 @@ func (s3a *S3ApiServer) removeBucketConfigKey(bucket, key string) s3err.ErrorCod // loadCORSFromMetadata loads CORS configuration from bucket metadata func (s3a *S3ApiServer) loadCORSFromMetadata(bucket string) (*cors.CORSConfiguration, error) { + // Validate bucket name to prevent path traversal attacks + if bucket == "" || strings.Contains(bucket, "/") || strings.Contains(bucket, "\\") || + strings.Contains(bucket, "..") || strings.Contains(bucket, "~") { + return nil, fmt.Errorf("invalid bucket name: %s", bucket) + } + + // Clean the bucket name further to prevent any potential path traversal + bucket = filepath.Clean(bucket) + if bucket == "." || bucket == ".." { + return nil, fmt.Errorf("invalid bucket name: %s", bucket) + } + bucketMetadataPath := fmt.Sprintf("%s/%s/.s3metadata", s3a.option.BucketsPath, bucket) entry, err := s3a.getEntry("", bucketMetadataPath) diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 887886009..5bc1ba899 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -20,6 +20,17 @@ import ( util_http "github.com/seaweedfs/seaweedfs/weed/util/http" ) +// corsHeaders defines the CORS headers that need to be preserved +// Package-level constant to avoid repeated allocations +var corsHeaders = []string{ + "Access-Control-Allow-Origin", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Headers", + "Access-Control-Expose-Headers", + "Access-Control-Max-Age", + "Access-Control-Allow-Credentials", +} + func mimeDetect(r *http.Request, dataReader io.Reader) io.ReadCloser { mimeBuffer := make([]byte, 512) size, _ := dataReader.Read(mimeBuffer) @@ -384,14 +395,7 @@ func setUserMetadataKeyToLowercase(resp *http.Response) { func passThroughResponse(proxyResponse *http.Response, w http.ResponseWriter) (statusCode int, bytesTransferred int64) { // Preserve existing CORS headers that may have been set by middleware existingCORSHeaders := make(map[string]string) - for _, corsHeader := range []string{ - "Access-Control-Allow-Origin", - "Access-Control-Allow-Methods", - "Access-Control-Allow-Headers", - "Access-Control-Expose-Headers", - "Access-Control-Max-Age", - "Access-Control-Allow-Credentials", - } { + for _, corsHeader := range corsHeaders { if value := w.Header().Get(corsHeader); value != "" { existingCORSHeaders[corsHeader] = value }