From ec261c5fbcc23ce5fea6643d75a8fdaa45b9c4e8 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 6 Oct 2026 18:05:15 +0800 Subject: [PATCH] S3: quote ETag in CopyObject and UploadPartCopy XML responses (#11624) * s3api: extract quoteETag from setEtag Consolidate ETag quoting so the XML response builders can share it. * s3api: quote ETag in CopyObjectResult XML AWS returns the ETag quoted in the copy result body, matching the ETag header. Fixes seaweedfs/seaweedfs#11622 * s3api: quote ETag in CopyPartResult XML UploadPartCopy returned the raw ETag in the XML body while the response header and other APIs return it quoted. Fixes seaweedfs/seaweedfs#11622 * s3api: test ETag quoting in copy responses * s3api: assert quoted ETag on the wire in copy response tests * s3api: use strconv.Quote in quoteETag Addresses CodeQL 'potentially unsafe quoting' on string concatenation. --- weed/s3api/s3api_object_handlers_copy.go | 8 ++-- ...3api_object_handlers_copy_checksum_test.go | 12 +++--- .../s3api_object_handlers_copy_etag_test.go | 37 +++++++++++++++++++ weed/s3api/s3api_object_handlers_put.go | 13 ++++--- 4 files changed, 55 insertions(+), 15 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_copy.go b/weed/s3api/s3api_object_handlers_copy.go index 4855b29bb..76d380695 100644 --- a/weed/s3api/s3api_object_handlers_copy.go +++ b/weed/s3api/s3api_object_handlers_copy.go @@ -374,7 +374,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request } setEtag(w, etag) writeSuccessResponseXML(w, r, CopyObjectResult{ - ETag: etag, + ETag: quoteETag(etag), LastModified: t, }) return @@ -546,7 +546,7 @@ func (s3a *S3ApiServer) CopyObjectHandler(w http.ResponseWriter, r *http.Request setEtag(w, etag) response := CopyObjectResult{ - ETag: etag, + ETag: quoteETag(etag), LastModified: t, } @@ -863,7 +863,7 @@ func (r CopyPartResult) MarshalXML(e *xml.Encoder, start xml.StartElement) error func buildCopyPartResult(etag string, lastModified time.Time, metadata SSEResponseMetadata) CopyPartResult { result := CopyPartResult{ - ETag: etag, + ETag: quoteETag(etag), LastModified: lastModified, } result.SetChecksum(metadata.ChecksumHeaderName, metadata.ChecksumValue) @@ -1086,7 +1086,7 @@ func (s3a *S3ApiServer) CopyObjectPartHandler(w http.ResponseWriter, r *http.Req if !s3a.checkUploadStillOpen(w, r, dstBucket, dstObject, uploadID) { return } - setEtag(w, "\""+strings.Trim(etag, "\"")+"\"") + setEtag(w, etag) // Mirror PutObjectPartHandler: write x-amz-server-side-encryption / // x-amz-server-side-encryption-aws-kms-key-id headers on the response // so clients can see the destination's encryption state. diff --git a/weed/s3api/s3api_object_handlers_copy_checksum_test.go b/weed/s3api/s3api_object_handlers_copy_checksum_test.go index 705a6ada3..8d08ee078 100644 --- a/weed/s3api/s3api_object_handlers_copy_checksum_test.go +++ b/weed/s3api/s3api_object_handlers_copy_checksum_test.go @@ -75,7 +75,7 @@ func TestBuildCopyPartResult(t *testing.T) { header: s3_constants.AmzChecksumCRC32, element: "value", expected: CopyPartResult{ - ETag: "etag", LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC32: "value"}, + ETag: `"etag"`, LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC32: "value"}, }, }, { @@ -83,7 +83,7 @@ func TestBuildCopyPartResult(t *testing.T) { header: s3_constants.AmzChecksumCRC32C, element: "value", expected: CopyPartResult{ - ETag: "etag", LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC32C: "value"}, + ETag: `"etag"`, LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC32C: "value"}, }, }, { @@ -91,7 +91,7 @@ func TestBuildCopyPartResult(t *testing.T) { header: s3_constants.AmzChecksumCRC64NVME, element: "value", expected: CopyPartResult{ - ETag: "etag", LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC64NVME: "value"}, + ETag: `"etag"`, LastModified: modified, ChecksumResult: ChecksumResult{ChecksumCRC64NVME: "value"}, }, }, { @@ -99,7 +99,7 @@ func TestBuildCopyPartResult(t *testing.T) { header: s3_constants.AmzChecksumSHA1, element: "value", expected: CopyPartResult{ - ETag: "etag", LastModified: modified, ChecksumResult: ChecksumResult{ChecksumSHA1: "value"}, + ETag: `"etag"`, LastModified: modified, ChecksumResult: ChecksumResult{ChecksumSHA1: "value"}, }, }, { @@ -107,14 +107,14 @@ func TestBuildCopyPartResult(t *testing.T) { header: s3_constants.AmzChecksumSHA256, element: "value", expected: CopyPartResult{ - ETag: "etag", LastModified: modified, ChecksumResult: ChecksumResult{ChecksumSHA256: "value"}, + ETag: `"etag"`, LastModified: modified, ChecksumResult: ChecksumResult{ChecksumSHA256: "value"}, }, }, { name: "Unknown", header: "x-amz-checksum-unknown", element: "", - expected: CopyPartResult{ETag: "etag", LastModified: modified}, + expected: CopyPartResult{ETag: `"etag"`, LastModified: modified}, }, } diff --git a/weed/s3api/s3api_object_handlers_copy_etag_test.go b/weed/s3api/s3api_object_handlers_copy_etag_test.go index e49559e86..8aeda9ccd 100644 --- a/weed/s3api/s3api_object_handlers_copy_etag_test.go +++ b/weed/s3api/s3api_object_handlers_copy_etag_test.go @@ -4,6 +4,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" @@ -64,6 +65,42 @@ func newCopyETagTestEntry(t *testing.T, storedETag, computedETag string) *filer_ return entry } +func TestQuoteETag(t *testing.T) { + testCases := []struct { + etag string + want string + }{ + {etag: "b1946ac92492d2347c6235b4d2611184", want: `"b1946ac92492d2347c6235b4d2611184"`}, + {etag: `"b1946ac92492d2347c6235b4d2611184"`, want: `"b1946ac92492d2347c6235b4d2611184"`}, + {etag: "", want: ""}, + } + for _, tc := range testCases { + if got := quoteETag(tc.etag); got != tc.want { + t.Errorf("quoteETag(%q) = %q, want %q", tc.etag, got, tc.want) + } + } +} + +func TestBuildCopyPartResultQuotesETag(t *testing.T) { + result := buildCopyPartResult("b1946ac92492d2347c6235b4d2611184", time.Now(), SSEResponseMetadata{}) + if result.ETag != `"b1946ac92492d2347c6235b4d2611184"` { + t.Fatalf("buildCopyPartResult().ETag = %q, want quoted", result.ETag) + } + if encoded := string(s3err.EncodeXMLResponse(result)); !strings.Contains(encoded, `"b1946ac92492d2347c6235b4d2611184"`) { + t.Fatalf("response %q does not contain a quoted ETag", encoded) + } +} + +func TestCopyObjectResultXMLQuotesETag(t *testing.T) { + encoded := string(s3err.EncodeXMLResponse(CopyObjectResult{ + ETag: quoteETag("b1946ac92492d2347c6235b4d2611184"), + LastModified: time.Now(), + })) + if !strings.Contains(encoded, `"b1946ac92492d2347c6235b4d2611184"`) { + t.Fatalf("response %q does not contain a quoted ETag", encoded) + } +} + // A part copy has no way to report a short part, so an unsatisfiable // x-amz-copy-source-range has to be rejected rather than clamped like a GET — // the re-encrypting path would otherwise pad the part out with zeros. diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index b92d5b095..deb968074 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -1397,14 +1397,17 @@ func (s3a *S3ApiServer) resolveFileMode(r *http.Request) uint32 { func setEtag(w http.ResponseWriter, etag string) { if etag != "" { - if strings.HasPrefix(etag, "\"") { - w.Header()["ETag"] = []string{etag} - } else { - w.Header()["ETag"] = []string{"\"" + etag + "\""} - } + w.Header()["ETag"] = []string{quoteETag(etag)} } } +func quoteETag(etag string) string { + if etag == "" || strings.HasPrefix(etag, "\"") { + return etag + } + return strconv.Quote(etag) +} + // setSSEResponseHeaders sets appropriate SSE response headers based on encryption type func (s3a *S3ApiServer) setSSEResponseHeaders(w http.ResponseWriter, r *http.Request, sseMetadata SSEResponseMetadata) { switch sseMetadata.SSEType {