From de3ecaf0de03a7d2ee1f8cf7516291b3b3b56122 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 10 Dec 2025 23:42:58 -0800 Subject: [PATCH] s3: fix presigned POST upload missing slash between bucket and key (#7714) * s3: fix presigned POST upload missing slash between bucket and key When uploading a file using presigned POST (e.g., boto3.generate_presigned_post), the file was saved with the bucket name and object key concatenated without a slash (e.g., 'my-bucketfilename' instead of 'my-bucket/filename'). The issue was that PostPolicyBucketHandler retrieved the object key from form values without ensuring it had a leading slash, unlike GetBucketAndObject() which normalizes the key. Fixes #7713 * s3: add tests for presigned POST key normalization Add comprehensive tests for PostPolicyBucketHandler to ensure: - Object keys without leading slashes are properly normalized - ${filename} substitution works correctly with normalization - Path construction correctly separates bucket and key - Form value extraction works properly These tests would have caught the bug fixed in the previous commit where keys like 'test_image.png' were concatenated with bucket without a separator, resulting in 'my-buckettest_image.png'. * s3: create normalizeObjectKey function for robust key normalization Address review feedback by creating a reusable normalizeObjectKey function that both adds a leading slash and removes duplicate slashes, aligning with how other handlers process paths (e.g., toFilerPath uses removeDuplicateSlashes). The function handles edge cases like: - Keys without leading slashes (the original bug) - Keys with duplicate slashes (e.g., 'a//b' -> '/a/b') - Keys with leading duplicate slashes (e.g., '///a' -> '/a') Updated tests to use the new function and added TestNormalizeObjectKey for comprehensive coverage of the new function. * s3: move NormalizeObjectKey to s3_constants for shared use Move the NormalizeObjectKey function to the s3_constants package so it can be reused by: - GetBucketAndObject() - now normalizes all object keys from URL paths - GetPrefix() - now normalizes prefix query parameters - PostPolicyBucketHandler - normalizes keys from form values This ensures consistent object key normalization across all S3 API handlers, handling both missing leading slashes and duplicate slashes. Benefits: - Single source of truth for key normalization - GetBucketAndObject now removes duplicate slashes (previously only added leading slash) - All handlers benefit from the improved normalization automatically --- weed/s3api/s3_constants/header.go | 31 +- weed/s3api/s3api_object_handlers.go | 1 + .../s3api/s3api_object_handlers_postpolicy.go | 2 +- .../s3api_object_handlers_postpolicy_test.go | 383 ++++++++++++++++++ 4 files changed, 414 insertions(+), 3 deletions(-) create mode 100644 weed/s3api/s3api_object_handlers_postpolicy_test.go diff --git a/weed/s3api/s3_constants/header.go b/weed/s3api/s3_constants/header.go index 377f355f6..b7e1be9e5 100644 --- a/weed/s3api/s3_constants/header.go +++ b/weed/s3api/s3_constants/header.go @@ -140,17 +140,44 @@ const ( func GetBucketAndObject(r *http.Request) (bucket, object string) { vars := mux.Vars(r) bucket = vars["bucket"] - object = vars["object"] + object = NormalizeObjectKey(vars["object"]) + return +} + +// NormalizeObjectKey ensures the object key has a leading slash and no duplicate slashes. +// This normalizes keys from various sources (URL path, form values, etc.) to a consistent format. +func NormalizeObjectKey(object string) string { + object = removeDuplicateSlashes(object) if !strings.HasPrefix(object, "/") { object = "/" + object } + return object +} - return +// removeDuplicateSlashes removes consecutive slashes from a path +func removeDuplicateSlashes(s string) string { + var result strings.Builder + result.Grow(len(s)) + + lastWasSlash := false + for _, r := range s { + if r == '/' { + if !lastWasSlash { + result.WriteRune(r) + } + lastWasSlash = true + } else { + result.WriteRune(r) + lastWasSlash = false + } + } + return result.String() } func GetPrefix(r *http.Request) string { query := r.URL.Query() prefix := query.Get("prefix") + prefix = removeDuplicateSlashes(prefix) if !strings.HasPrefix(prefix, "/") { prefix = "/" + prefix } diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index bdd16195a..93d857cdc 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -201,6 +201,7 @@ func removeDuplicateSlashes(object string) string { return result.String() } + // hasChildren checks if a path has any child objects (is a directory with contents) // // This helper function is used to distinguish implicit directories from regular files or empty directories. diff --git a/weed/s3api/s3api_object_handlers_postpolicy.go b/weed/s3api/s3api_object_handlers_postpolicy.go index 3ec6147f5..e6e885848 100644 --- a/weed/s3api/s3api_object_handlers_postpolicy.go +++ b/weed/s3api/s3api_object_handlers_postpolicy.go @@ -56,7 +56,7 @@ func (s3a *S3ApiServer) PostPolicyBucketHandler(w http.ResponseWriter, r *http.R if fileName != "" && strings.Contains(formValues.Get("Key"), "${filename}") { formValues.Set("Key", strings.Replace(formValues.Get("Key"), "${filename}", fileName, -1)) } - object := formValues.Get("Key") + object := s3_constants.NormalizeObjectKey(formValues.Get("Key")) successRedirect := formValues.Get("success_action_redirect") successStatus := formValues.Get("success_action_status") diff --git a/weed/s3api/s3api_object_handlers_postpolicy_test.go b/weed/s3api/s3api_object_handlers_postpolicy_test.go new file mode 100644 index 000000000..357fb9c7c --- /dev/null +++ b/weed/s3api/s3api_object_handlers_postpolicy_test.go @@ -0,0 +1,383 @@ +package s3api + +import ( + "bytes" + "io" + "mime/multipart" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gorilla/mux" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/stretchr/testify/assert" +) + +// TestPostPolicyKeyNormalization tests that object keys from presigned POST +// are properly normalized with a leading slash and duplicate slashes removed. +// This addresses issue #7713 where keys without leading slashes caused +// bucket and key to be concatenated without a separator. +func TestPostPolicyKeyNormalization(t *testing.T) { + tests := []struct { + name string + key string + expectedPrefix string // Expected path prefix after bucket + }{ + { + name: "key without leading slash", + key: "test_image.png", + expectedPrefix: "/test_image.png", + }, + { + name: "key with leading slash", + key: "/test_image.png", + expectedPrefix: "/test_image.png", + }, + { + name: "key with path without leading slash", + key: "folder/subfolder/test_image.png", + expectedPrefix: "/folder/subfolder/test_image.png", + }, + { + name: "key with path with leading slash", + key: "/folder/subfolder/test_image.png", + expectedPrefix: "/folder/subfolder/test_image.png", + }, + { + name: "simple filename", + key: "file.txt", + expectedPrefix: "/file.txt", + }, + { + name: "key with duplicate slashes", + key: "folder//subfolder///file.txt", + expectedPrefix: "/folder/subfolder/file.txt", + }, + { + name: "key with leading duplicate slashes", + key: "//folder/file.txt", + expectedPrefix: "/folder/file.txt", + }, + { + name: "key with trailing slash", + key: "folder/", + expectedPrefix: "/folder/", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Use the actual NormalizeObjectKey function + object := s3_constants.NormalizeObjectKey(tt.key) + + // Verify the normalized object has the expected prefix + assert.Equal(t, tt.expectedPrefix, object, + "Key should be normalized correctly") + + // Verify path construction would be correct + bucket := "my-bucket" + bucketsPath := "/buckets" + expectedPath := bucketsPath + "/" + bucket + tt.expectedPrefix + actualPath := bucketsPath + "/" + bucket + object + + assert.Equal(t, expectedPath, actualPath, + "File path should be correctly constructed with slash between bucket and key") + + // Verify we don't have double slashes (except at the start which is fine) + assert.NotContains(t, actualPath[1:], "//", + "Path should not contain double slashes") + }) + } +} + +// TestNormalizeObjectKey tests the NormalizeObjectKey function directly +func TestNormalizeObjectKey(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + {"empty string", "", "/"}, + {"simple file", "file.txt", "/file.txt"}, + {"with leading slash", "/file.txt", "/file.txt"}, + {"path without slash", "a/b/c.txt", "/a/b/c.txt"}, + {"path with slash", "/a/b/c.txt", "/a/b/c.txt"}, + {"duplicate slashes", "a//b///c.txt", "/a/b/c.txt"}, + {"leading duplicates", "///a/b.txt", "/a/b.txt"}, + {"all duplicates", "//a//b//", "/a/b/"}, + {"just slashes", "///", "/"}, + {"trailing slash", "folder/", "/folder/"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := s3_constants.NormalizeObjectKey(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} + +// TestPostPolicyFilenameSubstitution tests the ${filename} substitution in keys +func TestPostPolicyFilenameSubstitution(t *testing.T) { + tests := []struct { + name string + keyTemplate string + uploadedFilename string + expectedKey string + }{ + { + name: "filename at end", + keyTemplate: "uploads/${filename}", + uploadedFilename: "photo.jpg", + expectedKey: "/uploads/photo.jpg", + }, + { + name: "filename in middle", + keyTemplate: "user/files/${filename}/original", + uploadedFilename: "document.pdf", + expectedKey: "/user/files/document.pdf/original", + }, + { + name: "no substitution needed", + keyTemplate: "static/file.txt", + uploadedFilename: "ignored.txt", + expectedKey: "/static/file.txt", + }, + { + name: "filename only", + keyTemplate: "${filename}", + uploadedFilename: "myfile.png", + expectedKey: "/myfile.png", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Simulate the substitution logic from PostPolicyBucketHandler + key := tt.keyTemplate + if tt.uploadedFilename != "" && strings.Contains(key, "${filename}") { + key = strings.Replace(key, "${filename}", tt.uploadedFilename, -1) + } + + // Normalize using the actual function + object := s3_constants.NormalizeObjectKey(key) + + assert.Equal(t, tt.expectedKey, object, + "Key should be correctly substituted and normalized") + }) + } +} + +// TestExtractPostPolicyFormValues tests the form value extraction +func TestExtractPostPolicyFormValues(t *testing.T) { + tests := []struct { + name string + key string + contentType string + fileContent string + fileName string + expectSuccess bool + }{ + { + name: "basic upload", + key: "test.txt", + contentType: "text/plain", + fileContent: "hello world", + fileName: "upload.txt", + expectSuccess: true, + }, + { + name: "upload with path key", + key: "folder/subfolder/test.txt", + contentType: "application/octet-stream", + fileContent: "binary data", + fileName: "data.bin", + expectSuccess: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create multipart form + var buf bytes.Buffer + writer := multipart.NewWriter(&buf) + + // Add form fields + writer.WriteField("key", tt.key) + writer.WriteField("Content-Type", tt.contentType) + + // Add file + part, err := writer.CreateFormFile("file", tt.fileName) + assert.NoError(t, err) + _, err = io.WriteString(part, tt.fileContent) + assert.NoError(t, err) + + err = writer.Close() + assert.NoError(t, err) + + // Parse the form + reader := multipart.NewReader(&buf, writer.Boundary()) + form, err := reader.ReadForm(5 * 1024 * 1024) + assert.NoError(t, err) + defer form.RemoveAll() + + // Extract values using the actual function + filePart, fileName, fileContentType, fileSize, formValues, err := extractPostPolicyFormValues(form) + + if tt.expectSuccess { + assert.NoError(t, err) + assert.NotNil(t, filePart) + assert.Equal(t, tt.fileName, fileName) + assert.NotEmpty(t, fileContentType) + assert.Greater(t, fileSize, int64(0)) + assert.Equal(t, tt.key, formValues.Get("Key")) + + filePart.Close() + } + }) + } +} + +// TestPostPolicyPathConstruction is an integration-style test that verifies +// the complete path construction logic +func TestPostPolicyPathConstruction(t *testing.T) { + s3a := &S3ApiServer{ + option: &S3ApiServerOption{ + BucketsPath: "/buckets", + }, + } + + tests := []struct { + name string + bucket string + formKey string // Key as it would come from form (may not have leading slash) + expectedPath string + }{ + { + name: "simple key without slash - the bug case", + bucket: "my-bucket", + formKey: "test_image.png", + expectedPath: "/buckets/my-bucket/test_image.png", + }, + { + name: "simple key with slash", + bucket: "my-bucket", + formKey: "/test_image.png", + expectedPath: "/buckets/my-bucket/test_image.png", + }, + { + name: "nested path without leading slash", + bucket: "uploads", + formKey: "2024/01/photo.jpg", + expectedPath: "/buckets/uploads/2024/01/photo.jpg", + }, + { + name: "nested path with leading slash", + bucket: "uploads", + formKey: "/2024/01/photo.jpg", + expectedPath: "/buckets/uploads/2024/01/photo.jpg", + }, + { + name: "key with duplicate slashes", + bucket: "my-bucket", + formKey: "folder//file.txt", + expectedPath: "/buckets/my-bucket/folder/file.txt", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Use the actual NormalizeObjectKey function + object := s3_constants.NormalizeObjectKey(tt.formKey) + + // Construct path as done in PostPolicyBucketHandler + filePath := s3a.option.BucketsPath + "/" + tt.bucket + object + + assert.Equal(t, tt.expectedPath, filePath, + "File path should be correctly constructed") + + // Verify bucket and key are properly separated + assert.Contains(t, filePath, tt.bucket+"/", + "Bucket should be followed by a slash") + }) + } +} + +// TestPostPolicyBucketHandlerKeyExtraction tests that the handler correctly +// extracts and normalizes the key from a POST request +func TestPostPolicyBucketHandlerKeyExtraction(t *testing.T) { + // Create a minimal S3ApiServer for testing + s3a := &S3ApiServer{ + option: &S3ApiServerOption{ + BucketsPath: "/buckets", + }, + iam: &IdentityAccessManagement{}, + } + + tests := []struct { + name string + bucket string + key string + wantPathHas string // substring that must be in the constructed path + }{ + { + name: "key without leading slash", + bucket: "test-bucket", + key: "simple-file.txt", + wantPathHas: "/test-bucket/simple-file.txt", + }, + { + name: "key with leading slash", + bucket: "test-bucket", + key: "/prefixed-file.txt", + wantPathHas: "/test-bucket/prefixed-file.txt", + }, + { + name: "key with duplicate slashes", + bucket: "test-bucket", + key: "folder//nested///file.txt", + wantPathHas: "/test-bucket/folder/nested/file.txt", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create multipart form body + var buf bytes.Buffer + writer := multipart.NewWriter(&buf) + + // Add required fields + writer.WriteField("key", tt.key) + writer.WriteField("Policy", "") // Empty policy for this test + + // Add file + part, _ := writer.CreateFormFile("file", "test.txt") + part.Write([]byte("test content")) + writer.Close() + + // Create request + req := httptest.NewRequest(http.MethodPost, "/"+tt.bucket, &buf) + req.Header.Set("Content-Type", writer.FormDataContentType()) + + // Set up mux vars (simulating router) + req = mux.SetURLVars(req, map[string]string{"bucket": tt.bucket}) + + // Parse form to extract key + reader, _ := req.MultipartReader() + form, _ := reader.ReadForm(5 * 1024 * 1024) + defer form.RemoveAll() + + _, _, _, _, formValues, _ := extractPostPolicyFormValues(form) + + // Apply the same normalization as PostPolicyBucketHandler + object := s3_constants.NormalizeObjectKey(formValues.Get("Key")) + + // Construct path + filePath := s3a.option.BucketsPath + "/" + tt.bucket + object + + assert.Contains(t, filePath, tt.wantPathHas, + "Path should contain properly separated bucket and key") + }) + } +}