From 4070d62367df784959fc29d1fd1918f459a55f0c Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 26 Nov 2025 09:48:55 -0800 Subject: [PATCH] Fix hardcoded http scheme and add panic recovery Address CodeRabbit review #3512114811: 1. **Major: Fix hardcoded http:// scheme in Location URLs** - Location URLs always used http:// regardless of client connection - HTTPS clients got http:// URLs (incorrect) - Fixed: Detect scheme from request - Check X-Forwarded-Proto header (for proxies) first - Check r.TLS != nil for direct HTTPS - Fallback to http for plain connections - Applied to all 4 CompleteMultipartUploadResult locations 2. **Major: Add panic recovery to discovery goroutine** - Long-running background goroutine could crash entire process - Panic in refreshFilerList would terminate program - Fixed: Add defer recover() with error logging - Goroutine failures now logged, not fatal 3. **Note: Close() idempotency already implemented** - Review flagged as duplicate issue - Already fixed in commit 3d7a65c7e - sync.Once (closeDiscoveryOnce) prevents double-close panic - Safe to call Close() multiple times Changes: - filer_multipart.go: - Add getRequestScheme() helper function - Update all 4 Location URLs to use dynamic scheme - Format: scheme://host/bucket/key (was: http://...) - filer_client.go: - Add panic recovery to discoverFilers() - Log panics instead of crashing Benefits: - Correct scheme (https/http) in Location URLs - Works behind proxies (X-Forwarded-Proto) - No process crashes from discovery failures - Production-hardened background goroutine - Proper AWS S3 API compliance --- weed/s3api/filer_multipart.go | 24 +++++++++++++++++++----- weed/wdclient/filer_client.go | 6 ++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go index 274135230..1e4635ead 100644 --- a/weed/s3api/filer_multipart.go +++ b/weed/s3api/filer_multipart.go @@ -43,6 +43,20 @@ type InitiateMultipartUploadResult struct { s3.CreateMultipartUploadOutput } +// getRequestScheme determines the URL scheme (http or https) from the request +// Checks X-Forwarded-Proto header first (for proxies), then TLS state +func getRequestScheme(r *http.Request) string { + // Check X-Forwarded-Proto header for proxied requests + if proto := r.Header.Get("X-Forwarded-Proto"); proto != "" { + return proto + } + // Check if connection is TLS + if r.TLS != nil { + return "https" + } + return "http" +} + func (s3a *S3ApiServer) createMultipartUpload(r *http.Request, input *s3.CreateMultipartUploadInput) (output *InitiateMultipartUploadResult, code s3err.ErrorCode) { glog.V(2).Infof("createMultipartUpload input %v", input) @@ -185,9 +199,9 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl if entry, _ := s3a.getEntry(dirName, entryName); entry != nil && entry.Extended != nil { if uploadId, ok := entry.Extended[s3_constants.SeaweedFSUploadId]; ok && *input.UploadId == string(uploadId) { // Location uses the S3 endpoint that the client connected to - // Format: http://s3-endpoint/bucket/object (following AWS S3 API) + // Format: scheme://s3-endpoint/bucket/object (following AWS S3 API) return &CompleteMultipartUploadResult{ - Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), + Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), Bucket: input.Bucket, ETag: aws.String("\"" + filer.ETagChunks(entry.GetChunks()) + "\""), Key: objectKey(input.Key), @@ -404,7 +418,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl // The latest version information is tracked in the .versions directory metadata output = &CompleteMultipartUploadResult{ - Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), + Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), Bucket: input.Bucket, ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""), Key: objectKey(input.Key), @@ -457,7 +471,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl // Note: Suspended versioning should NOT return VersionId field according to AWS S3 spec output = &CompleteMultipartUploadResult{ - Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), + Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), Bucket: input.Bucket, ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""), Key: objectKey(input.Key), @@ -514,7 +528,7 @@ func (s3a *S3ApiServer) completeMultipartUpload(r *http.Request, input *s3.Compl // For non-versioned buckets, return response without VersionId output = &CompleteMultipartUploadResult{ - Location: aws.String(fmt.Sprintf("http://%s/%s/%s", r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), + Location: aws.String(fmt.Sprintf("%s://%s/%s/%s", getRequestScheme(r), r.Host, url.PathEscape(*input.Bucket), urlPathEscape(*input.Key))), Bucket: input.Bucket, ETag: aws.String("\"" + filer.ETagChunks(finalParts) + "\""), Key: objectKey(input.Key), diff --git a/weed/wdclient/filer_client.go b/weed/wdclient/filer_client.go index f3082d970..0c6719c6f 100644 --- a/weed/wdclient/filer_client.go +++ b/weed/wdclient/filer_client.go @@ -218,6 +218,12 @@ func (fc *FilerClient) Close() { // discoverFilers periodically queries the master to discover filers in the same group // and updates the filer list. This runs in a background goroutine. func (fc *FilerClient) discoverFilers() { + defer func() { + if r := recover(); r != nil { + glog.Errorf("FilerClient: panic in filer discovery goroutine for group '%s': %v", fc.filerGroup, r) + } + }() + // Do an initial discovery fc.refreshFilerList()