fix: handle the null version id correctly in posix versioning

Fixes #2165

`GetObject` marked an object carrying no `versionIdKey` attribute as the null version and then overwrote that with the empty attribute value, and `HeadObject` never marked it at all, so neither reported a version id for an object put into a versioning-suspended bucket. Both now resolve it through a shared `liveObjVersionId`, which reports `null` only when the bucket has versioning configured so that a bucket that was never versioned keeps reporting no version id at all.

`HeadObject` also resolved an explicit `?versionId=null` into the versioning directory twice, once for the missing attribute and once for the mismatch that followed from it, and answered `404` for a version that `GetObject` served. It now treats the missing attribute as the null version the way `GetObject` does.

`latestObjVersion` took the last version directory entry as the newest, but `os.ReadDir` sorts by name and `null` sorts after every `ulid`. Deleting the current version of an object whose history also held a null version therefore restored `null` rather than the version created immediately before the deleted one. The `ulid` entries keep their name order, which is creation order, and the null version is placed by its modification time, the same rule `fileToObjVersions` already applies.

backend/posix/posix.go

@@ -1226,6 +1226,30 @@ func (p *Posix) isBucketVersioningSuspended(s types.BucketVersioningStatus) bool
 	return s == types.BucketVersioningStatusSuspended
 }
 
+// liveObjVersionId returns the version id to report for the current version
+// of bucket/object. An object with no versionId attribute is the null
+// version, but only a bucket with versioning configured has versions at all:
+// in a bucket that was never versioned the object has no version id.
+func (p *Posix) liveObjVersionId(ctx context.Context, bucket, object string) (string, error) {
+	vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey)
+	if err == nil {
+		return string(vId), nil
+	}
+	if !errors.Is(err, meta.ErrNoSuchKey) {
+		return "", fmt.Errorf("get obj versionId: %w", err)
+	}
+
+	status, err := p.getBucketVersioningStatus(ctx, bucket)
+	if err != nil {
+		return "", fmt.Errorf("get bucket versioning status: %w", err)
+	}
+	if status == "" {
+		return "", nil
+	}
+
+	return nullVersionId, nil
+}
+
 // Generates the object version path in the versioning directory
 func (p *Posix) genObjVersionPath(bucket, key string) string {
 	return filepath.Join(p.versioningDir, bucket, genObjVersionKey(key))
@@ -5155,9 +5179,9 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 					}, nil
 				}
 
-				srcObjVersion, err := latestObjVersion(ents).Info()
+				srcObjVersion, err := latestObjVersion(ents)
 				if err != nil {
-					return nil, fmt.Errorf("get file info: %w", err)
+					return nil, fmt.Errorf("get latest obj version: %w", err)
 				}
 				srcVersionId := srcObjVersion.Name()
 				sf, err := os.Open(filepath.Join(versionPath, srcVersionId))
@@ -5326,11 +5350,45 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (
 	return &s3.DeleteObjectOutput{}, nil
 }
 
-// latestObjVersion returns the entry of the version, among the version
-// directory entries, that becomes the latest one when the latest version
-// of the object is deleted
-func latestObjVersion(ents []fs.DirEntry) fs.DirEntry {
-	return ents[len(ents)-1]
+// latestObjVersion returns the version, among the version directory entries,
+// that becomes the latest one when the latest version of the object is
+// deleted. The entries are named after the version id and os.ReadDir sorts
+// them by name, which puts the ulid version ids in creation order. The null
+// version id doesn't sort with them, so it's placed by its modification time,
+// which is the time the version was created.
+func latestObjVersion(ents []fs.DirEntry) (fs.FileInfo, error) {
+	var latest, nullEnt fs.DirEntry
+	for _, ent := range ents {
+		if ent.Name() == nullVersionId {
+			nullEnt = ent
+			continue
+		}
+		latest = ent
+	}
+
+	switch {
+	case latest == nil && nullEnt == nil:
+		return nil, fs.ErrNotExist
+	case latest == nil:
+		return nullEnt.Info()
+	case nullEnt == nil:
+		return latest.Info()
+	}
+
+	latestInfo, err := latest.Info()
+	if err != nil {
+		return nil, err
+	}
+	nullInfo, err := nullEnt.Info()
+	if err != nil {
+		return nil, err
+	}
+
+	if nullInfo.ModTime().After(latestInfo.ModTime()) {
+		return nullInfo, nil
+	}
+
+	return latestInfo, nil
 }
 
 // deleteDirObjectLatestVersion removes the latest version of the directory
@@ -5371,12 +5429,11 @@ func (p *Posix) deleteDirObjectLatestVersion(bucket, key string) error {
 		return nil
 	}
 
-	srcVersion := latestObjVersion(ents)
-	srcVersionId := srcVersion.Name()
-	srcInfo, err := srcVersion.Info()
+	srcInfo, err := latestObjVersion(ents)
 	if err != nil {
-		return fmt.Errorf("get file info: %w", err)
+		return fmt.Errorf("get latest obj version: %w", err)
 	}
+	srcVersionId := srcInfo.Name()
 
 	// replace the attributes in place, so that the directory keeps its etag
 	for _, attr := range dirObjectAttrs {
@@ -5623,14 +5680,10 @@ func (p *Posix) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.Ge
 
 	// If versioning is configured get the object versionId
 	if p.versioningEnabled() && versionId == "" {
-		vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey)
-		if errors.Is(err, meta.ErrNoSuchKey) {
-			versionId = nullVersionId
-		} else if err != nil {
+		versionId, err = p.liveObjVersionId(ctx, bucket, object)
+		if err != nil {
 			return nil, err
 		}
-
-		versionId = string(vId)
 	}
 
 	if fid.IsDir() {
@@ -5879,8 +5932,8 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3.
 			return nil, fmt.Errorf("get obj versionId: %w", err)
 		}
 		if errors.Is(err, meta.ErrNoSuchKey) {
-			bucket = filepath.Join(p.versioningDir, bucket)
-			object = filepath.Join(genObjVersionKey(object), versionId)
+			// an object without a versionId attribute is the null version
+			vId = []byte(nullVersionId)
 		}
 
 		if string(vId) != versionId {
@@ -5944,12 +5997,10 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3.
 	}
 
 	if p.versioningEnabled() && versionId == "" {
-		vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey)
-		if err != nil && !errors.Is(err, meta.ErrNoSuchKey) {
-			return nil, fmt.Errorf("get object versionId: %v", err)
+		versionId, err = p.liveObjVersionId(ctx, bucket, object)
+		if err != nil {
+			return nil, err
 		}
-
-		versionId = string(vId)
 	}
 
 	objMeta := p.loadObjectMetaProperties(nil, bucket, object, &fi)

tests/integration/group-tests.go

@@ -1972,6 +1972,8 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_HeadObject_success)
 	ts.Run(Versioning_HeadObject_dir_object_versions)
 	ts.Run(Versioning_HeadObject_without_versionId)
+	ts.Run(Versioning_HeadObject_null_version_without_versionId)
+	ts.Run(Versioning_HeadObject_null_versionId_obj)
 	ts.Run(Versioning_HeadObject_delete_marker)
 	// GetObject action
 	ts.Run(Versioning_GetObject_invalid_versionId)
@@ -1980,6 +1982,8 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_GetObject_delete_marker_without_versionId)
 	ts.Run(Versioning_GetObject_delete_marker)
 	ts.Run(Versioning_GetObject_null_versionId_obj)
+	ts.Run(Versioning_GetObject_null_version_without_versionId)
+	ts.Run(Versioning_unversioned_bucket_omits_versionId)
 	// object tagging actions
 	ts.Run(Versioning_PutObjectTagging_invalid_versionId)
 	ts.Run(Versioning_PutObjectTagging_non_existing_object_version)
@@ -1998,6 +2002,7 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_DeleteObject_invalid_versionId)
 	ts.Run(Versioning_DeleteObject_delete_object_version)
 	ts.Run(Versioning_DeleteObject_dir_object_latest_version)
+	ts.Run(Versioning_DeleteObject_latest_version_with_null_version)
 	ts.Run(Versioning_DeleteObject_non_existing_object)
 	ts.Run(Versioning_DeleteObject_implicit_dir)
 	ts.Run(Versioning_DeleteObject_trailing_slash_counterpart)
@@ -3509,12 +3514,16 @@ func GetIntTests() IntTests {
 		"Versioning_HeadObject_success":                                                    Versioning_HeadObject_success,
 		"Versioning_HeadObject_dir_object_versions":                                        Versioning_HeadObject_dir_object_versions,
 		"Versioning_HeadObject_without_versionId":                                          Versioning_HeadObject_without_versionId,
+		"Versioning_HeadObject_null_versionId_obj":                                         Versioning_HeadObject_null_versionId_obj,
+		"Versioning_HeadObject_null_version_without_versionId":                             Versioning_HeadObject_null_version_without_versionId,
 		"Versioning_HeadObject_delete_marker":                                              Versioning_HeadObject_delete_marker,
 		"Versioning_GetObject_invalid_versionId":                                           Versioning_GetObject_invalid_versionId,
 		"Versioning_GetObject_non_existing_object_version":                                 Versioning_GetObject_non_existing_object_version,
 		"Versioning_GetObject_success":                                                     Versioning_GetObject_success,
 		"Versioning_GetObject_delete_marker_without_versionId":                             Versioning_GetObject_delete_marker_without_versionId,
 		"Versioning_GetObject_delete_marker":                                               Versioning_GetObject_delete_marker,
+		"Versioning_unversioned_bucket_omits_versionId":                                    Versioning_unversioned_bucket_omits_versionId,
+		"Versioning_GetObject_null_version_without_versionId":                              Versioning_GetObject_null_version_without_versionId,
 		"Versioning_GetObject_null_versionId_obj":                                          Versioning_GetObject_null_versionId_obj,
 		"Versioning_PutObjectTagging_invalid_versionId":                                    Versioning_PutObjectTagging_invalid_versionId,
 		"Versioning_PutObjectTagging_non_existing_object_version":                          Versioning_PutObjectTagging_non_existing_object_version,
@@ -3530,6 +3539,7 @@ func GetIntTests() IntTests {
 		"Versioning_GetObjectAttributes_delete_marker":                                     Versioning_GetObjectAttributes_delete_marker,
 		"Versioning_DeleteObject_invalid_versionId":                                        Versioning_DeleteObject_invalid_versionId,
 		"Versioning_DeleteObject_delete_object_version":                                    Versioning_DeleteObject_delete_object_version,
+		"Versioning_DeleteObject_latest_version_with_null_version":                         Versioning_DeleteObject_latest_version_with_null_version,
 		"Versioning_DeleteObject_dir_object_latest_version":                                Versioning_DeleteObject_dir_object_latest_version,
 		"Versioning_DeleteObject_non_existing_object":                                      Versioning_DeleteObject_non_existing_object,
 		"Versioning_DeleteObject_implicit_dir":                                             Versioning_DeleteObject_implicit_dir,

tests/integration/versioning.go

@@ -1269,6 +1269,105 @@ func Versioning_HeadObject_without_versionId(s *S3Conf) error {
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+// Versioning_HeadObject_null_version_without_versionId heads an object put
+// into a versioning-suspended bucket, without naming a version id.
+func Versioning_HeadObject_null_versionId_obj(s *S3Conf) error {
+	testName := "Versioning_HeadObject_null_versionId_obj"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		keys, dataLen := []string{"my-obj", "my-dir/"}, int64(321)
+		// the objects are put before versioning is enabled
+		etags := make(map[string]string, len(keys))
+		err := forEachKey(keys, func(obj string) error {
+			out, err := putObjectWithData(objDataLen(obj, dataLen), &s3.PutObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			}, s3client)
+			if err != nil {
+				return err
+			}
+			etags[obj] = getString(out.res.ETag)
+			return nil
+		})
+		if err != nil {
+			return err
+		}
+
+		err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled)
+		if err != nil {
+			return err
+		}
+
+		return forEachKey(keys, func(obj string) error {
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{
+				Bucket:    &bucket,
+				Key:       &obj,
+				VersionId: &nullVersionId,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+
+			if getString(res.VersionId) != nullVersionId {
+				return fmt.Errorf("expected the versionId to be %v, instead got %v",
+					nullVersionId, getString(res.VersionId))
+			}
+			if res.ContentLength == nil || *res.ContentLength != objDataLen(obj, dataLen) {
+				return fmt.Errorf("expected the Content-Length to be %v, instead got %v",
+					objDataLen(obj, dataLen), res.ContentLength)
+			}
+			if getString(res.ETag) != etags[obj] {
+				return fmt.Errorf("expected the ETag to be %v, instead got %v",
+					etags[obj], getString(res.ETag))
+			}
+
+			return nil
+		})
+	})
+}
+
+func Versioning_HeadObject_null_version_without_versionId(s *S3Conf) error {
+	testName := "Versioning_HeadObject_null_version_without_versionId"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error {
+			dataLen := objDataLen(obj, 765)
+			out, err := putObjectWithData(dataLen, &s3.PutObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			}, s3client)
+			if err != nil {
+				return err
+			}
+
+			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) != nullVersionId {
+				return fmt.Errorf("expected the versionId to be %v, instead got %v",
+					nullVersionId, getString(res.VersionId))
+			}
+			if res.ContentLength == nil || *res.ContentLength != dataLen {
+				return fmt.Errorf("expected the Content-Length to be %v, instead got %v",
+					dataLen, res.ContentLength)
+			}
+			if getString(res.ETag) != getString(out.res.ETag) {
+				return fmt.Errorf("expected the ETag to be %v, instead got %v",
+					getString(out.res.ETag), getString(res.ETag))
+			}
+
+			return nil
+		})
+	}, withVersioning(types.BucketVersioningStatusSuspended))
+}
+
 func Versioning_HeadObject_delete_marker(s *S3Conf) error {
 	testName := "Versioning_HeadObject_delete_marker"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -1611,6 +1710,120 @@ func Versioning_GetObject_null_versionId_obj(s *S3Conf) error {
 	})
 }
 
+// Versioning_GetObject_null_version_without_versionId reads an object put
+// into a versioning-suspended bucket, without naming a version id. The object
+// is the null version and the response has to report it as null rather than
+// leave the version id out.
+func Versioning_GetObject_null_version_without_versionId(s *S3Conf) error {
+	testName := "Versioning_GetObject_null_version_without_versionId"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error {
+			dataLen := objDataLen(obj, 543)
+			out, err := putObjectWithData(dataLen, &s3.PutObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			}, s3client)
+			if err != nil {
+				return err
+			}
+			// a put into a versioning-suspended bucket creates the null
+			// version and reports no version id
+			if out.res.VersionId != nil {
+				return fmt.Errorf("expected PutObject response to omit versionId, instead got %v",
+					getString(out.res.VersionId))
+			}
+
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			res, err := s3client.GetObject(ctx, &s3.GetObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+
+			if getString(res.VersionId) != nullVersionId {
+				return fmt.Errorf("expected the versionId to be %v, instead got %v",
+					nullVersionId, getString(res.VersionId))
+			}
+			if res.ContentLength == nil || *res.ContentLength != dataLen {
+				return fmt.Errorf("expected the Content-Length to be %v, instead got %v",
+					dataLen, res.ContentLength)
+			}
+			if getString(res.ETag) != getString(out.res.ETag) {
+				return fmt.Errorf("expected the ETag to be %v, instead got %v",
+					getString(out.res.ETag), getString(res.ETag))
+			}
+
+			return nil
+		})
+	}, withVersioning(types.BucketVersioningStatusSuspended))
+}
+
+// Versioning_unversioned_bucket_omits_versionId reads an object in a bucket
+// that never had versioning configured. Such a bucket has no versions at all,
+// so neither GetObject nor HeadObject reports a version id, not even the null
+// one, even though the gateway itself runs with versioning enabled.
+func Versioning_unversioned_bucket_omits_versionId(s *S3Conf) error {
+	testName := "Versioning_unversioned_bucket_omits_versionId"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		vRes, err := s3client.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+		// guard the premise of the test
+		if vRes.Status != "" {
+			return fmt.Errorf("expected the bucket versioning to be unconfigured, instead got %v",
+				vRes.Status)
+		}
+
+		return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error {
+			_, err := putObjectWithData(objDataLen(obj, 432), &s3.PutObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			}, s3client)
+			if err != nil {
+				return err
+			}
+
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			gRes, err := s3client.GetObject(ctx, &s3.GetObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+			if gRes.VersionId != nil {
+				return fmt.Errorf("expected GetObject to omit the versionId, instead got %v",
+					*gRes.VersionId)
+			}
+
+			ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+			hRes, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+			if hRes.VersionId != nil {
+				return fmt.Errorf("expected HeadObject to omit the versionId, instead got %v",
+					*hRes.VersionId)
+			}
+
+			return nil
+		})
+	})
+}
+
 func Versioning_GetObjectAttributes_invalid_versionId(s *S3Conf) error {
 	testName := "Versioning_GetObjectAttributes_invalid_versionId"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -1961,6 +2174,122 @@ func Versioning_DeleteObject_dir_object_latest_version(s *S3Conf) error {
 	}, withVersioning(types.BucketVersioningStatusEnabled))
 }
 
+// Versioning_DeleteObject_latest_version_with_null_version deletes the current
+// version of an object whose history also holds a null version, created while
+// versioning was suspended. The version that becomes current has to be the one
+// created right before the deleted version, not the older null version.
+func Versioning_DeleteObject_latest_version_with_null_version(s *S3Conf) error {
+	testName := "Versioning_DeleteObject_latest_version_with_null_version"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error {
+			// the versions are told apart by a metadata entry, as a
+			// directory object carries no data
+			put := func(marker string) (*s3.PutObjectOutput, error) {
+				ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+				defer cancel()
+				return s3client.PutObject(ctx, &s3.PutObjectInput{
+					Bucket:   &bucket,
+					Key:      &obj,
+					Metadata: map[string]string{"marker": marker},
+				})
+			}
+
+			if _, err := put("v1"); err != nil {
+				return err
+			}
+			if _, err := put("v2"); err != nil {
+				return err
+			}
+
+			err := putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusSuspended)
+			if err != nil {
+				return err
+			}
+
+			// the null version sits in the middle of the version history
+			if _, err := put("null"); err != nil {
+				return err
+			}
+
+			err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled)
+			if err != nil {
+				return err
+			}
+
+			third, err := put("v3")
+			if err != nil {
+				return err
+			}
+			latest, err := put("v4")
+			if err != nil {
+				return err
+			}
+
+			ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+			out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+				Bucket:    &bucket,
+				Key:       &obj,
+				VersionId: latest.VersionId,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+			if getString(out.VersionId) != getString(latest.VersionId) {
+				return fmt.Errorf("expected the deleted versionId to be %v, instead got %v",
+					getString(latest.VersionId), getString(out.VersionId))
+			}
+
+			ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+			res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{
+				Bucket: &bucket,
+				Key:    &obj,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+
+			expectedMeta := map[string]string{"marker": "v3"}
+			if !areMapsSame(res.Metadata, expectedMeta) {
+				return fmt.Errorf("expected the object metadata to be %v, instead got %v",
+					expectedMeta, res.Metadata)
+			}
+			if getString(res.VersionId) != getString(third.VersionId) {
+				return fmt.Errorf("expected the current versionId to be %v, instead got %v",
+					getString(third.VersionId), getString(res.VersionId))
+			}
+
+			// the null version has to be left untouched by the delete
+			ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+			versions, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+				Bucket: &bucket,
+				Prefix: &obj,
+			})
+			cancel()
+			if err != nil {
+				return err
+			}
+
+			nullFound := false
+			for _, v := range versions.Versions {
+				if getString(v.VersionId) == nullVersionId {
+					nullFound = true
+					if v.IsLatest != nil && *v.IsLatest {
+						return fmt.Errorf("expected the null version not to be the latest")
+					}
+				}
+			}
+			if !nullFound {
+				return fmt.Errorf("expected the null version to be kept in %v",
+					versions.Versions)
+			}
+
+			return nil
+		})
+	}, withVersioning(types.BucketVersioningStatusEnabled))
+}
+
 func Versioning_DeleteObject_non_existing_object(s *S3Conf) error {
 	testName := "Versioning_DeleteObject_non_existing_object"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {