From ba02c766abf09da68b014366bdc5686211f6101c Mon Sep 17 00:00:00 2001 From: niksis02 Date: Sun, 20 Sep 2026 16:07:40 +0400 Subject: [PATCH] fix: stop posix rewriting the current version on a copy to itself `CopyObject` treated a destination path equal to the source path as an in-place metadata rewrite, regardless of bucket versioning. On a versioned bucket a self copy therefore edited the current version and returned its existing version id instead of creating a new one. This also defeated object lock: `CheckObjectAccess` skips the retention and legal hold checks for overwrites on version-enabled buckets because an overwrite is expected to create a new version, so a `COMPLIANCE` retained or legal held version had its metadata replaced underneath it. A self copy is now rewritten in place only when the bucket is unversioned, and otherwise goes through the regular copy path, which creates a new version and leaves the one it replaces untouched. The rejection of a self copy that replaces nothing moved out of the in-place branch and is now applied only when the copy source carries no version id. Naming a version explicitly makes the request a regular copy, which AWS accepts even with the `COPY` metadata directive, while versitygw answered `InvalidRequest`. Source tagging for a `COPY` tagging directive is now read before `PutObject` writes the destination rather than after. A self copy replaces the source object's attributes, so the later read returned nothing and the tags were dropped from the new version. --- backend/posix/posix.go | 53 +++-- backend/posix/posix_io_helpers.go | 19 ++ backend/posix/posix_io_helpers_test.go | 79 +++++++ tests/integration/group-tests.go | 6 + tests/integration/versioning.go | 275 +++++++++++++++++++++++++ 5 files changed, 416 insertions(+), 16 deletions(-) create mode 100644 backend/posix/posix_io_helpers_test.go diff --git a/backend/posix/posix.go b/backend/posix/posix.go index b3c7eaaf..165a67f9 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -6310,11 +6310,18 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput var chType types.ChecksumType dstObjdPath := joinPathWithTrailer(p.BucketPath(dstBucket), dstObject) - if dstObjdPath == objPath { - if input.MetadataDirective == types.MetadataDirectiveCopy { - return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) - } + // A copy of an object onto itself is rejected unless it replaces the + // object metadata. Naming a source version makes it a regular copy. + selfCopy := dstObjdPath == objPath + if selfCopy && srcVersionId == "" && + input.MetadataDirective == types.MetadataDirectiveCopy { + return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) + } + // In a versioned bucket a self copy creates a new version like any other + // write, so only unversioned buckets are rewritten in place. + versioned := p.versioningEnabled() && vStatus != "" + if selfCopy && !versioned { // Delete the object metadata err = p.meta.DeleteAttribute(dstBucket, dstObject, metadataHdr) if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { @@ -6452,6 +6459,14 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput checksums.Algorithm = input.ChecksumAlgorithm } + // A self copy publishes the new version over the source path, which + // on Windows can't be renamed over while the source is still open. + // PutObject reads the body before publishing, so the handle is + // released as soon as the data has been staged. + if selfCopy { + srcBody = &closeOnEOFReader{r: srcBody, c: f} + } + putObjectInput := s3response.PutObjectInput{ Bucket: &dstBucket, Key: &dstObject, @@ -6488,23 +6503,29 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput putObjectInput.Tagging = input.Tagging } - res, err := p.PutObject(withCtxNoSlot(ctx), putObjectInput) - if err != nil { - return s3response.CopyObjectOutput{}, err - } - - // copy the source object tagging after the destination object - // creation, if tagging directive is "COPY" + // read the source tagging before the destination is written, as a + // self copy replaces the source object's metadata + var srcTagging []byte + var hasSrcTagging bool if input.TaggingDirective == types.TaggingDirectiveCopy { tagging, err := p.meta.RetrieveAttribute(nil, srcBucket, srcObject, tagHdr) if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { return s3response.CopyObjectOutput{}, fmt.Errorf("get source object tagging: %w", err) } - if err == nil { - err := p.meta.StoreAttribute(nil, dstBucket, dstObject, tagHdr, tagging) - if err != nil { - return s3response.CopyObjectOutput{}, fmt.Errorf("set destination object tagging: %w", err) - } + srcTagging, hasSrcTagging = tagging, err == nil + } + + res, err := p.PutObject(withCtxNoSlot(ctx), putObjectInput) + if err != nil { + return s3response.CopyObjectOutput{}, err + } + + // the source tagging is stored after the destination object creation, + // if tagging directive is "COPY" + if hasSrcTagging { + err := p.meta.StoreAttribute(nil, dstBucket, dstObject, tagHdr, srcTagging) + if err != nil { + return s3response.CopyObjectOutput{}, fmt.Errorf("set destination object tagging: %w", err) } } diff --git a/backend/posix/posix_io_helpers.go b/backend/posix/posix_io_helpers.go index 62eed318..76d8fd70 100644 --- a/backend/posix/posix_io_helpers.go +++ b/backend/posix/posix_io_helpers.go @@ -16,6 +16,7 @@ package posix import ( "bufio" + "errors" "io" "log" "sync" @@ -45,6 +46,24 @@ func (b *bufferedReadCloser) Close() error { return b.c.Close() } +// closeOnEOFReader closes c once r is drained, for readers whose source has +// to be released before the caller is done with the reader. +type closeOnEOFReader struct { + r io.Reader + c io.Closer + closed bool +} + +func (e *closeOnEOFReader) Read(p []byte) (int, error) { + n, err := e.r.Read(p) + if errors.Is(err, io.EOF) && !e.closed { + e.closed = true + e.c.Close() + } + + return n, err +} + var odirectUnsupportedWarnByOp sync.Map func warnODirectUnsupportedOnce(op string, err error) { diff --git a/backend/posix/posix_io_helpers_test.go b/backend/posix/posix_io_helpers_test.go new file mode 100644 index 00000000..5beb962f --- /dev/null +++ b/backend/posix/posix_io_helpers_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package posix + +import ( + "bytes" + "io" + "strings" + "testing" +) + +type countingCloser struct { + count int +} + +func (c *countingCloser) Close() error { + c.count++ + return nil +} + +func TestCloseOnEOFReader(t *testing.T) { + c := &countingCloser{} + r := &closeOnEOFReader{r: strings.NewReader("hello"), c: c} + + buf := make([]byte, 2) + n, err := r.Read(buf) + if err != nil { + t.Fatalf("read: %v", err) + } + if n != 2 { + t.Fatalf("expected 2 bytes, got %v", n) + } + if c.count != 0 { + t.Fatalf("expected the source to stay open before EOF, closed %v times", c.count) + } + + data, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read all: %v", err) + } + if !bytes.Equal(data, []byte("llo")) { + t.Fatalf("expected the remaining data to be llo, got %s", data) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once at EOF, closed %v times", c.count) + } + + // reads past EOF don't close the source again + if _, err := r.Read(buf); err != io.EOF { + t.Fatalf("expected io.EOF, got %v", err) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once, closed %v times", c.count) + } +} + +func TestCloseOnEOFReaderEmptySource(t *testing.T) { + c := &countingCloser{} + r := &closeOnEOFReader{r: strings.NewReader(""), c: c} + + if _, err := io.ReadAll(r); err != nil { + t.Fatalf("read all: %v", err) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once at EOF, closed %v times", c.count) + } +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index cbed13f6..6f5d8c79 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1958,6 +1958,8 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_CopyObject_non_existing_version_id) ts.Run(Versioning_CopyObject_from_an_object_version) ts.Run(Versioning_CopyObject_from_a_delete_marker) + ts.Run(Versioning_CopyObject_to_itself) + ts.Run(Versioning_CopyObject_to_itself_from_the_current_version) if !ts.conf.windowsTests { ts.Run(Versioning_CopyObject_special_chars) } @@ -2056,6 +2058,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_WORM_delete_marker_locked_object_compliance_retention) ts.Run(Versioning_WORM_PutObject_overwrite_locked_object) ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object) + ts.Run(Versioning_WORM_CopyObject_to_itself_locked_object) ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object) if !ts.conf.windowsTests { ts.Run(Versioning_WORM_remove_delete_marker_under_bucket_default_retention) @@ -3497,6 +3500,8 @@ func GetIntTests() IntTests { "Versioning_CopyObject_non_existing_version_id": Versioning_CopyObject_non_existing_version_id, "Versioning_CopyObject_from_an_object_version": Versioning_CopyObject_from_an_object_version, "Versioning_CopyObject_from_a_delete_marker": Versioning_CopyObject_from_a_delete_marker, + "Versioning_CopyObject_to_itself": Versioning_CopyObject_to_itself, + "Versioning_CopyObject_to_itself_from_the_current_version": Versioning_CopyObject_to_itself_from_the_current_version, "Versioning_CopyObject_special_chars": Versioning_CopyObject_special_chars, "Versioning_HeadObject_invalid_versionId": Versioning_HeadObject_invalid_versionId, "Versioning_HeadObject_non_existing_object_version": Versioning_HeadObject_non_existing_object_version, @@ -3578,6 +3583,7 @@ func GetIntTests() IntTests { "Versioning_WORM_delete_marker_locked_object_compliance_retention": Versioning_WORM_delete_marker_locked_object_compliance_retention, "Versioning_WORM_PutObject_overwrite_locked_object": Versioning_WORM_PutObject_overwrite_locked_object, "Versioning_WORM_CopyObject_overwrite_locked_object": Versioning_WORM_CopyObject_overwrite_locked_object, + "Versioning_WORM_CopyObject_to_itself_locked_object": Versioning_WORM_CopyObject_to_itself_locked_object, "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object": Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object, "Versioning_WORM_remove_delete_marker_under_bucket_default_retention": Versioning_WORM_remove_delete_marker_under_bucket_default_retention, "Versioning_WORM_trailing_slash_counterpart": Versioning_WORM_trailing_slash_counterpart, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 45f2491a..8e71ecd5 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -742,6 +742,182 @@ func Versioning_CopyObject_from_a_delete_marker(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// A copy of an object onto itself in a versioned bucket is an ordinary +// write: it creates a new version and leaves the one it replaces untouched. +// Without a metadata directive there is nothing to replace, so it's rejected. +func Versioning_CopyObject_to_itself(s *S3Conf) error { + testName := "Versioning_CopyObject_to_itself" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + // directory objects always carry the directory content-type + srcContentType, dstContentType := "text/plain", "application/json" + if strings.HasSuffix(obj, "/") { + srcContentType, dstContentType = directoryContentType, directoryContentType + } + + srcMeta := map[string]string{"key": "value"} + r, err := putObjectWithData(objDataLen(obj, 1234), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + ContentType: getPtr("text/plain"), + Metadata: srcMeta, + }, s3client) + if err != nil { + return err + } + + srcVersionId := getString(r.res.VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidCopyDest)); err != nil { + return err + } + + dstMeta := map[string]string{"new-key": "new-value"} + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirectiveReplace, + ContentType: getPtr("application/json"), + Metadata: dstMeta, + }) + cancel() + if err != nil { + return err + } + + dstVersionId := getString(out.VersionId) + if dstVersionId == "" { + return fmt.Errorf("expected non empty versionId") + } + if dstVersionId == srcVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", dstVersionId) + } + + // the replaced version keeps its own metadata + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &srcVersionId, + }) + cancel() + if err != nil { + return err + } + + if getString(res.ContentType) != srcContentType { + return fmt.Errorf("expected the source version content-type to be %v, instead got %v", + srcContentType, getString(res.ContentType)) + } + if !areMapsSame(res.Metadata, srcMeta) { + return fmt.Errorf("expected the source version metadata to be %v, instead got %v", + srcMeta, res.Metadata) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != dstVersionId { + return fmt.Errorf("expected the current versionId to be %v, instead got %v", + dstVersionId, getString(res.VersionId)) + } + if getString(res.ContentType) != dstContentType { + return fmt.Errorf("expected the new version content-type to be %v, instead got %v", + dstContentType, getString(res.ContentType)) + } + if !areMapsSame(res.Metadata, dstMeta) { + return fmt.Errorf("expected the new version metadata to be %v, instead got %v", + dstMeta, res.Metadata) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + vRes, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + Prefix: &obj, + }) + cancel() + if err != nil { + return err + } + + if len(vRes.Versions) != 2 { + return fmt.Errorf("expected 2 object versions, instead got %v", len(vRes.Versions)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +// Naming the current version in the copy source makes a copy onto the same +// key a regular copy, so it is accepted even without a metadata directive. +func Versioning_CopyObject_to_itself_from_the_current_version(s *S3Conf) error { + testName := "Versioning_CopyObject_to_itself_from_the_current_version" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + srcVersionId := getString(versions[0].VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", bucket, obj, srcVersionId)), + }) + cancel() + if err != nil { + return err + } + + if getString(out.CopySourceVersionId) != srcVersionId { + return fmt.Errorf("expected the copy-source-version-id to be %v, instead got %v", + srcVersionId, getString(out.CopySourceVersionId)) + } + if getString(out.VersionId) == srcVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", getString(out.VersionId)) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &srcVersionId, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != srcVersionId { + return fmt.Errorf("expected the source version to remain, instead got %v", + getString(res.VersionId)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_CopyObject_special_chars(s *S3Conf) error { testName := "Versioning_CopyObject_special_chars" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -4250,6 +4426,105 @@ func Versioning_WORM_CopyObject_overwrite_locked_object(s *S3Conf) error { }, withLock()) } +// A copy of a locked object onto itself creates a new version, leaving the +// locked one and its legal hold in place. +func Versioning_WORM_CopyObject_to_itself_locked_object(s *S3Conf) error { + testName := "Versioning_WORM_CopyObject_to_itself_locked_object" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + v := versions[0] + v.IsLatest = getPtr(false) + lockedVersionId := getString(v.VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOn, + }, + }) + cancel() + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + copyResult, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirectiveReplace, + ContentType: getPtr("application/json"), + }) + cancel() + if err != nil { + return err + } + + if getString(copyResult.VersionId) == lockedVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", + getString(copyResult.VersionId)) + } + + version := types.ObjectVersion{ + ETag: copyResult.CopyObjectResult.ETag, + IsLatest: getPtr(true), + Key: &obj, + Size: v.Size, + VersionId: copyResult.VersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + ChecksumType: copyResult.CopyObjectResult.ChecksumType, + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + Prefix: &obj, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions([]types.ObjectVersion{version, v}, out.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", + []types.ObjectVersion{version, v}, out.Versions) + } + + // the legal hold stays on the version it was set on + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + lhRes, err := s3client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &lockedVersionId, + }) + cancel() + if err != nil { + return err + } + + if lhRes.LegalHold.Status != types.ObjectLockLegalHoldStatusOn { + return fmt.Errorf("expected the legal hold status to be %v, instead got %v", + types.ObjectLockLegalHoldStatusOn, lhRes.LegalHold.Status) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: lockedVersionId, + removeOnlyLeglHold: true, + }, + }) + }) + }, withLock()) +} + func Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object(s *S3Conf) error { testName := "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {