diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 469b2bf6..6585f78a 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -347,6 +347,18 @@ func (p *Posix) versioningEnabled() bool { return p.versioningDir != "" } +// validateVersionId checks if the input versionId is 'ulid' compatible +func (p *Posix) validateVersionId(versionId string) error { + if versionId == "" || versionId == "null" { + return nil + } + _, err := ulid.Parse(versionId) + if err != nil { + return s3err.GetAPIError(s3err.ErrInvalidVersionId) + } + return nil +} + func (p *Posix) doesBucketAndObjectExist(bucket, object string) error { _, err := os.Stat(bucket) if errors.Is(err, fs.ErrNotExist) { @@ -3044,6 +3056,9 @@ func (p *Posix) UploadPartCopy(ctx context.Context, upi *s3.UploadPartCopyInput) if err != nil { return s3response.CopyPartResult{}, err } + if err := p.validateVersionId(srcVersionId); err != nil { + return s3response.CopyPartResult{}, err + } _, err = os.Stat(srcBucket) if errors.Is(err, fs.ErrNotExist) { @@ -3727,6 +3742,10 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, s3err.GetAPIError(s3err.ErrInvalidBucketName) } + if err := p.validateVersionId(backend.GetStringFromPtr(input.VersionId)); err != nil { + return nil, err + } + _, err = os.Stat(bucket) if errors.Is(err, fs.ErrNotExist) { return nil, s3err.GetAPIError(s3err.ErrNoSuchBucket) @@ -4146,6 +4165,10 @@ func (p *Posix) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.Ge versionId = *input.VersionId } + if err := p.validateVersionId(versionId); err != nil { + return nil, err + } + if input.PartNumber != nil { // querying an object by part number is not supported return nil, s3err.GetAPIError(s3err.ErrNotImplemented) @@ -4411,6 +4434,10 @@ func (p *Posix) HeadObject(ctx context.Context, input *s3.HeadObjectInput) (*s3. } versionId := backend.GetStringFromPtr(input.VersionId) + if err := p.validateVersionId(versionId); err != nil { + return nil, err + } + if input.PartNumber != nil { // querying an object by part number is not supported return nil, s3err.GetAPIError(s3err.ErrNotImplemented) @@ -4670,6 +4697,9 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput if err != nil { return s3response.CopyObjectOutput{}, err } + if err := p.validateVersionId(srcVersionId); err != nil { + return s3response.CopyObjectOutput{}, err + } if !p.isBucketValid(srcBucket) { return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidBucketName) } @@ -5381,6 +5411,10 @@ func (p *Posix) GetObjectTagging(ctx context.Context, bucket, object, versionId return nil, fmt.Errorf("stat bucket: %w", err) } + if err := p.validateVersionId(versionId); err != nil { + return nil, err + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? @@ -5454,6 +5488,10 @@ func (p *Posix) PutObjectTagging(ctx context.Context, bucket, object, versionId return fmt.Errorf("stat bucket: %w", err) } + if err := p.validateVersionId(versionId); err != nil { + return err + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? @@ -5785,6 +5823,10 @@ func (p *Posix) PutObjectLegalHold(ctx context.Context, bucket, object, versionI return err } + if err := p.validateVersionId(versionId); err != nil { + return err + } + var statusData []byte if status { statusData = []byte{1} @@ -5849,6 +5891,10 @@ func (p *Posix) GetObjectLegalHold(ctx context.Context, bucket, object, versionI return nil, err } + if err := p.validateVersionId(versionId); err != nil { + return nil, err + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? @@ -5911,6 +5957,10 @@ func (p *Posix) PutObjectRetention(ctx context.Context, bucket, object, versionI return err } + if err := p.validateVersionId(versionId); err != nil { + return err + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? @@ -5962,6 +6012,10 @@ func (p *Posix) GetObjectRetention(ctx context.Context, bucket, object, versionI return nil, err } + if err := p.validateVersionId(versionId); err != nil { + return nil, err + } + if versionId != "" { if !p.versioningEnabled() { //TODO: Maybe we need to return our custom error here? diff --git a/s3api/controllers/object-delete.go b/s3api/controllers/object-delete.go index a374e5ab..43678d2d 100644 --- a/s3api/controllers/object-delete.go +++ b/s3api/controllers/object-delete.go @@ -62,15 +62,6 @@ func (c S3ApiController) DeleteObjectTagging(ctx *fiber.Ctx) (*Response, error) }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - err = c.be.DeleteObjectTagging(ctx.Context(), bucket, key, versionId) return &Response{ Headers: map[string]*string{ @@ -170,15 +161,6 @@ func (c S3ApiController) DeleteObject(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - err = auth.CheckObjectAccess( ctx.Context(), bucket, diff --git a/s3api/controllers/object-delete_test.go b/s3api/controllers/object-delete_test.go index 512954f6..ac09cb22 100644 --- a/s3api/controllers/object-delete_test.go +++ b/s3api/controllers/object-delete_test.go @@ -47,23 +47,6 @@ func TestS3ApiController_DeleteObjectTagging(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "backend returns error", input: testInput{ @@ -238,23 +221,6 @@ func TestS3ApiController_DeleteObject(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "object locked", input: testInput{ diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index 4d212110..b5330d21 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -65,15 +65,6 @@ func (c S3ApiController) GetObjectTagging(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - data, err := c.be.GetObjectTagging(ctx.Context(), bucket, key, versionId) if err != nil { return &Response{ @@ -132,15 +123,6 @@ func (c S3ApiController) GetObjectRetention(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - data, err := c.be.GetObjectRetention(ctx.Context(), bucket, key, versionId) if err != nil { return &Response{ @@ -189,15 +171,6 @@ func (c S3ApiController) GetObjectLegalHold(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - data, err := c.be.GetObjectLegalHold(ctx.Context(), bucket, key, versionId) return &Response{ Data: auth.ParseObjectLegalHoldOutput(data), @@ -351,15 +324,6 @@ func (c S3ApiController) GetObjectAttributes(ctx *fiber.Ctx) (*Response, error) }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - var maxParts *int32 // parse max parts parsed, err := utils.ParseMaxLimiter(maxPartsStr, utils.LimiterTypeMaxParts) @@ -499,15 +463,6 @@ func (c S3ApiController) GetObject(ctx *fiber.Ctx) (*Response, error) { partNumber = &partNumberQuery } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - // validate the checksum mode if checksumMode != "" && checksumMode != types.ChecksumModeEnabled { debuglogger.Logf("invalid x-amz-checksum-mode header value: %v", checksumMode) diff --git a/s3api/controllers/object-get_test.go b/s3api/controllers/object-get_test.go index f8e89d0c..27bbd874 100644 --- a/s3api/controllers/object-get_test.go +++ b/s3api/controllers/object-get_test.go @@ -54,23 +54,6 @@ func TestS3ApiController_GetObjectTagging(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "backend returns error", input: testInput{ @@ -173,23 +156,6 @@ func TestS3ApiController_GetObjectRetention(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "backend returns error", input: testInput{ @@ -293,23 +259,6 @@ func TestS3ApiController_GetObjectLegalHold(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "backend returns error", input: testInput{ @@ -617,23 +566,6 @@ func TestS3ApiController_GetObjectAttributes(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid object attributes", input: testInput{ @@ -756,23 +688,6 @@ func TestS3ApiController_GetObject(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid checksum mode", input: testInput{ diff --git a/s3api/controllers/object-head.go b/s3api/controllers/object-head.go index 62baf3fa..96719285 100644 --- a/s3api/controllers/object-head.go +++ b/s3api/controllers/object-head.go @@ -110,15 +110,6 @@ func (c S3ApiController) HeadObject(ctx *fiber.Ctx) (*Response, error) { partNumber = &partNumberQuery } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - checksumMode := types.ChecksumMode(strings.ToUpper(ctx.Get("x-amz-checksum-mode"))) if checksumMode != "" && checksumMode != types.ChecksumModeEnabled { debuglogger.Logf("invalid x-amz-checksum-mode header value: %v", checksumMode) diff --git a/s3api/controllers/object-head_test.go b/s3api/controllers/object-head_test.go index e975ec53..f7ff245d 100644 --- a/s3api/controllers/object-head_test.go +++ b/s3api/controllers/object-head_test.go @@ -80,23 +80,6 @@ func TestS3ApiController_HeadObject(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAnonymousResponseHeaders), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid part number", input: testInput{ diff --git a/s3api/controllers/object-put.go b/s3api/controllers/object-put.go index 0aa7657d..0685c220 100644 --- a/s3api/controllers/object-put.go +++ b/s3api/controllers/object-put.go @@ -67,15 +67,6 @@ func (c S3ApiController) PutObjectTagging(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - tagging, err := utils.ParseTagging(ctx.Body(), utils.TagLimitObject) if err != nil { return &Response{ @@ -127,15 +118,6 @@ func (c S3ApiController) PutObjectRetention(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - // parse the request body bytes into a go struct and validate retention, err := auth.ParseObjectLockRetentionInput(ctx.Body()) if err != nil { @@ -203,15 +185,6 @@ func (c S3ApiController) PutObjectLegalHold(ctx *fiber.Ctx) (*Response, error) { }, err } - err = utils.ValidateVersionId(versionId) - if err != nil { - return &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: parsedAcl.Owner, - }, - }, err - } - var legalHold types.ObjectLockLegalHold if err := xml.Unmarshal(ctx.Body(), &legalHold); err != nil { debuglogger.Logf("failed to parse request body: %v", err) diff --git a/s3api/controllers/object-put_test.go b/s3api/controllers/object-put_test.go index 171a896a..09648fdb 100644 --- a/s3api/controllers/object-put_test.go +++ b/s3api/controllers/object-put_test.go @@ -67,23 +67,6 @@ func TestS3ApiController_PutObjectTagging(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid request body", input: testInput{ @@ -204,23 +187,6 @@ func TestS3ApiController_PutObjectRetention(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid versionId", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid request body", input: testInput{ @@ -349,23 +315,6 @@ func TestS3ApiController_PutObjectLegalHold(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid request body", - input: testInput{ - locals: defaultLocals, - queries: map[string]string{ - "versionId": "invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid request body", input: testInput{ @@ -649,26 +598,6 @@ func TestS3ApiController_UploadPartCopy(t *testing.T) { err: s3err.GetAPIError(s3err.ErrAccessDenied), }, }, - { - name: "invalid copy source: invalid versionId", - input: testInput{ - locals: defaultLocals, - headers: map[string]string{ - "X-Amz-Copy-Source": "bucket/object?versionId=invalid_versionId", - }, - queries: map[string]string{ - "partNumber": "2", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "invalid copy source", input: testInput{ @@ -929,23 +858,6 @@ func TestS3ApiController_CopyObject(t *testing.T) { err: s3err.GetAPIError(s3err.ErrInvalidCopySourceBucket), }, }, - { - name: "invalid copy source: versionId", - input: testInput{ - locals: defaultLocals, - headers: map[string]string{ - "X-Amz-Copy-Source": "bucket/object?versionId=invalid_versionId", - }, - }, - output: testOutput{ - response: &Response{ - MetaOpts: &MetaOptions{ - BucketOwner: "root", - }, - }, - err: s3err.GetAPIError(s3err.ErrInvalidVersionId), - }, - }, { name: "non empty request body", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index 34618c86..a38adfdd 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -33,7 +33,6 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/gofiber/fiber/v2" - "github.com/oklog/ulid/v2" "github.com/valyala/fasthttp" "github.com/versity/versitygw/debuglogger" "github.com/versity/versitygw/s3err" @@ -1015,7 +1014,7 @@ func ValidateCopySource(copysource string) error { // cut till the versionId as it's the only query param // that is recognized in copy source - object, versionId, _ := strings.Cut(rest, "?versionId=") + object, _, _ := strings.Cut(rest, "?versionId=") // objects containing '../', '...../' ... are considered valid in AWS // but for the security purposes these should be considered as invalid @@ -1025,12 +1024,6 @@ func ValidateCopySource(copysource string) error { return s3err.GetAPIError(s3err.ErrInvalidCopySourceObject) } - // validate the versionId - err = ValidateVersionId(versionId) - if err != nil { - return err - } - return nil } @@ -1051,20 +1044,6 @@ func ApplyOverride(original, override *string) *string { return original } -// ValidateVersionId check if the input versionId is 'ulid' compatible -func ValidateVersionId(versionId string) error { - if versionId == "" || versionId == "null" { - return nil - } - _, err := ulid.Parse(versionId) - if err != nil { - debuglogger.Logf("invalid versionId: %s", versionId) - return s3err.GetAPIError(s3err.ErrInvalidVersionId) - } - - return nil -} - // GenerateObjectLocation generates the object location path-styled or host-styled // depending on the gateway configuration func GenerateObjectLocation(ctx *fiber.Ctx, virtualDomain, bucket, object string) string { diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index 218a5163..3b167ad6 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1389,9 +1389,6 @@ func TestValidateCopySource(t *testing.T) { {"invalid object name 3", "bucket", s3err.GetAPIError(s3err.ErrInvalidCopySourceObject)}, {"invalid object name 4", "bucket/../foo/dir/../../../", s3err.GetAPIError(s3err.ErrInvalidCopySourceObject)}, {"invalid object name 5", "bucket/.?versionId=smth", s3err.GetAPIError(s3err.ErrInvalidCopySourceObject)}, - // invalid versionId - {"invalid versionId 1", "bucket/object?versionId=invalid", s3err.GetAPIError(s3err.ErrInvalidVersionId)}, - {"invalid versionId 2", "bucket/object?versionId=01BX5ZZKBKACTAV9WEVGEMMV", s3err.GetAPIError(s3err.ErrInvalidVersionId)}, // success {"no error 1", "bucket/object", nil}, {"no error 2", "bucket/object/key", nil}, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 80c1bf84..30fd5816 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -2100,7 +2100,7 @@ func Versioning_PutObjectRetention_invalid_versionId(s *S3Conf) error { }) cancel() return checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidVersionId)) - }) + }, withLock()) } func Versioning_PutObjectRetention_non_existing_object_version(s *S3Conf) error { @@ -2149,7 +2149,7 @@ func Versioning_GetObjectRetention_invalid_versionId(s *S3Conf) error { }) cancel() return checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidVersionId)) - }) + }, withLock()) } func Versioning_GetObjectRetention_non_existing_object_version(s *S3Conf) error { @@ -2293,7 +2293,7 @@ func Versioning_PutObjectLegalHold_invalid_versionId(s *S3Conf) error { }) cancel() return checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidVersionId)) - }) + }, withLock()) } func Versioning_PutObjectLegalHold_non_existing_object_version(s *S3Conf) error { @@ -2340,7 +2340,7 @@ func Versioning_GetObjectLegalHold_invalid_versionId(s *S3Conf) error { }) cancel() return checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidVersionId)) - }) + }, withLock()) } func Versioning_GetObjectLegalHold_non_existing_object_version(s *S3Conf) error {