diff --git a/s3api/controllers/bucket-post.go b/s3api/controllers/bucket-post.go index 216f04a1..39ed508d 100644 --- a/s3api/controllers/bucket-post.go +++ b/s3api/controllers/bucket-post.go @@ -16,6 +16,7 @@ package controllers import ( "encoding/xml" + "errors" "net/http" "net/url" "slices" @@ -51,11 +52,17 @@ func (c S3ApiController) DeleteObjects(ctx fiber.Ctx) (*Response, error) { err := xml.Unmarshal(ctx.BodyRaw(), &dObj) if err != nil { debuglogger.Logf("error unmarshalling delete objects: %v", err) + // The decoder returns an S3 error for a body that is well-formed + // XML but carries an invalid value. + var s3Err s3err.S3Error + if !errors.As(err, &s3Err) { + s3Err = s3err.GetAPIError(s3err.ErrInvalidRequest) + } return &Response{ MetaOpts: &MetaOptions{ BucketOwner: parsedAcl.Owner, }, - }, s3err.GetAPIError(s3err.ErrInvalidRequest) + }, s3Err } // S3 requires at least one object in a DeleteObjects request and diff --git a/s3api/controllers/bucket-post_test.go b/s3api/controllers/bucket-post_test.go index 06a173bd..a9f3ba8b 100644 --- a/s3api/controllers/bucket-post_test.go +++ b/s3api/controllers/bucket-post_test.go @@ -93,6 +93,13 @@ func TestS3ApiController_DeleteObjects(t *testing.T) { }) assert.NoError(t, err) + // AWS SDKs and the AWS CLI send LastModifiedTime as an HTTP date. + httpDateBody := []byte(`` + + `obj` + + `Wed, 07 Oct 2026 05:00:00 GMT` + + ``) + httpDateLMT := time.Date(2026, time.October, 7, 5, 0, 0, 0, time.UTC) + quietBody, err := xml.Marshal(s3response.DeleteObjects{ Objects: []types.ObjectIdentifier{ {Key: utils.GetStringPtr("ok")}, @@ -166,6 +173,38 @@ func TestS3ApiController_DeleteObjects(t *testing.T) { err: s3err.GetAPIError(s3err.ErrInvalidRequest), }, }, + { + name: "invalid last modified time", + input: testInput{ + locals: defaultLocals, + body: []byte(`obj` + + `2026-10-07T05:00:00Z`), + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidDateAttributeErr("2026-10-07T05:00:00Z"), + }, + }, + { + name: "empty last modified time", + input: testInput{ + locals: defaultLocals, + body: []byte(`obj` + + ``), + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetAPIError(s3err.ErrMalformedXML), + }, + }, { name: "check object access returns error", input: testInput{ @@ -222,6 +261,33 @@ func TestS3ApiController_DeleteObjects(t *testing.T) { }, }, }, + { + name: "last modified time as an HTTP date", + input: testInput{ + locals: defaultLocals, + body: httpDateBody, + extraMockErr: s3err.GetAPIError(s3err.ErrObjectLockConfigurationNotFound), + }, + configureMock: func(be *BackendMock) { + be.DeleteObjectsFunc = func(_ context.Context, input *s3.DeleteObjectsInput) (s3response.DeleteResult, error) { + objs := input.Delete.Objects + if len(objs) != 1 || objs[0].LastModifiedTime == nil || !objs[0].LastModifiedTime.Equal(httpDateLMT) { + return s3response.DeleteResult{}, s3err.GetAPIError(s3err.ErrInternalError) + } + return validRes, nil + } + }, + output: testOutput{ + response: &Response{ + Data: validRes, + MetaOpts: &MetaOptions{ + BucketOwner: "root", + EventName: s3event.EventObjectRemovedDeleteObjects, + ObjectCount: 1, + }, + }, + }, + }, { name: "quiet mode reports only the objects not deleted", input: testInput{ diff --git a/s3err/s3err.go b/s3err/s3err.go index bc5fd1f1..6ffb2020 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -922,6 +922,16 @@ func GetInvalidTrailingChecksumHeaderErr(header string) APIError { } } +// GetInvalidDateAttributeErr returns the error for a date in a request body +// that doesn't parse. value is echoed as the client sent it. +func GetInvalidDateAttributeErr(value string) APIError { + return APIError{ + Code: "InvalidRequest", + Description: fmt.Sprintf("Invalid value for date attribute: %v", value), + HTTPStatusCode: http.StatusBadRequest, + } +} + // Returns checksum type mismatch APIError func GetChecksumTypeMismatchErr(expected, actual types.ChecksumAlgorithm) APIError { return APIError{ diff --git a/s3response/s3response.go b/s3response/s3response.go index a9af33dd..6b87d77d 100644 --- a/s3response/s3response.go +++ b/s3response/s3response.go @@ -17,9 +17,11 @@ package s3response import ( "encoding/xml" "io" + "strings" "time" "github.com/aws/aws-sdk-go-v2/service/s3/types" + smithytime "github.com/aws/smithy-go/time" "github.com/versity/versitygw/s3err" ) @@ -298,6 +300,58 @@ type DeleteObjects struct { Quiet bool `xml:"Quiet,omitempty"` } +// UnmarshalXML decodes LastModifiedTime as an HTTP date, the form the SDKs +// send. The SDK type alone decodes it as RFC 3339, which S3 rejects. +func (d *DeleteObjects) UnmarshalXML(dec *xml.Decoder, start xml.StartElement) error { + var p struct { + Objects []struct { + types.ObjectIdentifier + LastModifiedTime *string + } `xml:"Object"` + Quiet bool + } + if err := dec.DecodeElement(&p, &start); err != nil { + return err + } + + // An empty LastModifiedTime is a schema error, and S3 reports it ahead + // of an unparseable date in any object. Of the unparseable dates, the + // first one in the body is reported, as sent. + var dateErr error + objects := make([]types.ObjectIdentifier, 0, len(p.Objects)) + for _, o := range p.Objects { + obj := o.ObjectIdentifier + if o.LastModifiedTime != nil { + if strings.TrimSpace(*o.LastModifiedTime) == "" { + return s3err.GetAPIError(s3err.ErrMalformedXML) + } + lmt, err := parseLastModifiedTime(*o.LastModifiedTime) + if err != nil && dateErr == nil { + dateErr = s3err.GetInvalidDateAttributeErr(*o.LastModifiedTime) + } + obj.LastModifiedTime = &lmt + } + objects = append(objects, obj) + } + if dateErr != nil { + return dateErr + } + + d.Objects = objects + d.Quiet = p.Quiet + return nil +} + +// parseLastModifiedTime accepts the HTTP date forms and, like S3, a numeric +// zone offset in place of GMT. +func parseLastModifiedTime(s string) (time.Time, error) { + s = strings.TrimSpace(s) + if t, err := smithytime.ParseHTTPDate(s); err == nil { + return t, nil + } + return time.Parse(time.RFC1123Z, s) +} + type DeleteResult struct { Deleted []types.DeletedObject Error []types.Error diff --git a/s3response/s3response_test.go b/s3response/s3response_test.go new file mode 100644 index 00000000..bd9e8dfc --- /dev/null +++ b/s3response/s3response_test.go @@ -0,0 +1,141 @@ +// 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 s3response + +import ( + "encoding/xml" + "errors" + "testing" + "time" + + "github.com/versity/versitygw/s3err" +) + +func TestDeleteObjectsUnmarshalLastModifiedTime(t *testing.T) { + want := time.Date(2026, time.October, 7, 5, 0, 0, 0, time.UTC) + + valid := []struct { + name string + lmt string + }{ + // AWS SDKs and the AWS CLI send this form. + {name: "http date", lmt: "Wed, 07 Oct 2026 05:00:00 GMT"}, + {name: "rfc 850", lmt: "Wednesday, 07-Oct-26 05:00:00 GMT"}, + {name: "ansic", lmt: "Wed Oct 7 05:00:00 2026"}, + {name: "single digit day", lmt: "Wed, 7 Oct 2026 05:00:00 GMT"}, + {name: "zero offset", lmt: "Wed, 07 Oct 2026 05:00:00 +0000"}, + {name: "numeric offset", lmt: "Wed, 07 Oct 2026 07:00:00 +0200"}, + {name: "padded", lmt: " Wed, 07 Oct 2026 05:00:00 GMT "}, + } + + for _, tt := range valid { + t.Run(tt.name, func(t *testing.T) { + body := `` + + `k"abc"` + + `` + tt.lmt + `` + + `5v1` + + `true` + + var d DeleteObjects + if err := xml.Unmarshal([]byte(body), &d); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(d.Objects) != 1 { + t.Fatalf("expected 1 object, got %d", len(d.Objects)) + } + obj := d.Objects[0] + if obj.Key == nil || *obj.Key != "k" { + t.Errorf("unexpected key: %v", obj.Key) + } + if obj.ETag == nil || *obj.ETag != `"abc"` { + t.Errorf("unexpected etag: %v", obj.ETag) + } + if obj.Size == nil || *obj.Size != 5 { + t.Errorf("unexpected size: %v", obj.Size) + } + if obj.VersionId == nil || *obj.VersionId != "v1" { + t.Errorf("unexpected version id: %v", obj.VersionId) + } + if obj.LastModifiedTime == nil || !obj.LastModifiedTime.Equal(want) { + t.Errorf("unexpected last modified time: %v", obj.LastModifiedTime) + } + if !d.Quiet { + t.Errorf("expected quiet to be true") + } + }) + } + + malformed := s3err.GetAPIError(s3err.ErrMalformedXML) + invalid := []struct { + name string + objects string + want s3err.APIError + }{ + // S3 rejects RFC 3339 here. + { + name: "rfc3339", + objects: `k2026-10-07T05:00:00Z`, + want: s3err.GetInvalidDateAttributeErr("2026-10-07T05:00:00Z"), + }, + { + name: "not a date", + objects: `knot a date`, + want: s3err.GetInvalidDateAttributeErr("not a date"), + }, + { + name: "padded value echoed as sent", + objects: `k not a date `, + want: s3err.GetInvalidDateAttributeErr(" not a date "), + }, + { + name: "first invalid date reported", + objects: `afirst` + + `bsecond`, + want: s3err.GetInvalidDateAttributeErr("first"), + }, + { + name: "empty", + objects: `k`, + want: malformed, + }, + { + name: "whitespace only", + objects: `k `, + want: malformed, + }, + { + name: "self-closing", + objects: `k`, + want: malformed, + }, + { + name: "empty wins over an earlier invalid date", + objects: `anot a date` + + `b`, + want: malformed, + }, + } + + for _, tt := range invalid { + t.Run(tt.name, func(t *testing.T) { + var d DeleteObjects + err := xml.Unmarshal([]byte(``+tt.objects+``), &d) + var apiErr s3err.APIError + if !errors.As(err, &apiErr) || apiErr != tt.want { + t.Fatalf("expected %v, got %v", tt.want, err) + } + }) + } +} diff --git a/tests/integration/DeleteObjects.go b/tests/integration/DeleteObjects.go index 3d219e8a..e3df87c4 100644 --- a/tests/integration/DeleteObjects.go +++ b/tests/integration/DeleteObjects.go @@ -16,14 +16,20 @@ package integration import ( "context" + "crypto/md5" + "encoding/base64" + "encoding/xml" "fmt" + "net/http" "slices" "strings" + "time" "github.com/aws/aws-sdk-go-v2/service/iam" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/versity/versitygw/s3err" + "github.com/versity/versitygw/s3response" ) func DeleteObjects_non_existing_bucket(s *S3Conf) error { @@ -321,6 +327,206 @@ func DeleteObjects_conditional(s *S3Conf) error { }) } +// DeleteObjects_conditional_last_modified_time checks the LastModifiedTime +// condition of each object, which the SDKs send as an HTTP date. +func DeleteObjects_conditional_last_modified_time(s *S3Conf) error { + testName := "DeleteObjects_conditional_last_modified_time" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + _, err := putObjects(s3client, []string{"match", "stale"}, bucket) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + head, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: getPtr("match"), + }) + cancel() + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.DeleteObjects(ctx, &s3.DeleteObjectsInput{ + Bucket: &bucket, + Delete: &types.Delete{ + Objects: []types.ObjectIdentifier{ + {Key: getPtr("match"), LastModifiedTime: head.LastModified}, + {Key: getPtr("stale"), LastModifiedTime: getPtr(head.LastModified.AddDate(-1, 0, 0))}, + }, + }, + }) + cancel() + if err != nil { + return err + } + + if len(out.Deleted) != 1 || getString(out.Deleted[0].Key) != "match" { + return fmt.Errorf("expected only match to be deleted, instead got %v", out.Deleted) + } + if len(out.Errors) != 1 { + return fmt.Errorf("expected 1 error, instead got %v", out.Errors) + } + return checkDeleteObjectsErr(out.Errors[0], "stale", + s3err.GetAPIError(s3err.ErrPreconditionFailed)) + }) +} + +// DeleteObjects_last_modified_time_formats checks that each date form S3 +// accepts for LastModifiedTime matches the object it names. The SDK only +// sends IMF-fixdate, so the request is signed and sent by hand. +func DeleteObjects_last_modified_time_formats(s *S3Conf) error { + testName := "DeleteObjects_last_modified_time_formats" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + forms := []struct { + key string + format func(time.Time) string + }{ + {"rfc1123", func(t time.Time) string { return t.UTC().Format(http.TimeFormat) }}, + {"rfc850", func(t time.Time) string { return t.UTC().Format("Monday, 02-Jan-06 15:04:05 GMT") }}, + {"ansic", func(t time.Time) string { return t.UTC().Format(time.ANSIC) }}, + {"numeric-offset", func(t time.Time) string { + return t.In(time.FixedZone("", 2*60*60)).Format(time.RFC1123Z) + }}, + {"padded", func(t time.Time) string { return " " + t.UTC().Format(http.TimeFormat) + " " }}, + } + + var body strings.Builder + body.WriteString("") + for _, form := range forms { + _, err := putObjects(s3client, []string{form.key}, bucket) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + head, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &form.key, + }) + cancel() + if err != nil { + return err + } + fmt.Fprintf(&body, "%v%v", + form.key, form.format(*head.LastModified)) + } + body.WriteString("") + sum := md5.Sum([]byte(body.String())) + + req, err := createSignedReq(http.MethodPost, s.endpoint, bucket+"?delete", + s.awsID, s.awsSecret, "s3", s.awsRegion, "", []byte(body.String()), time.Now(), map[string]string{ + "Content-Md5": base64.StdEncoding.EncodeToString(sum[:]), + }) + if err != nil { + return err + } + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("expected the response status code to be 200, instead got %v", resp.StatusCode) + } + var res s3response.DeleteResult + if err := xml.NewDecoder(resp.Body).Decode(&res); err != nil { + return err + } + if len(res.Error) != 0 { + return fmt.Errorf("expected no errors, instead got %v", res.Error) + } + if len(res.Deleted) != len(forms) { + return fmt.Errorf("expected %v deleted objects, instead got %v", len(forms), len(res.Deleted)) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectsV2(ctx, &s3.ListObjectsV2Input{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + if len(out.Contents) != 0 { + return fmt.Errorf("expected the bucket to be empty, instead got %v objects", len(out.Contents)) + } + + return nil + }) +} + +// DeleteObjects_invalid_last_modified_time checks that a LastModifiedTime S3 +// can't parse fails the whole request and deletes nothing. +func DeleteObjects_invalid_last_modified_time(s *S3Conf) error { + testName := "DeleteObjects_invalid_last_modified_time" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + _, err := putObjects(s3client, []string{obj}, bucket) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + head, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + // the object's own time, so the condition would hold if RFC 3339 + // were accepted + rfc3339 := head.LastModified.UTC().Format(time.RFC3339) + malformed := s3err.GetAPIError(s3err.ErrMalformedXML) + for i, test := range []struct { + lmts []string + err s3err.S3Error + }{ + {[]string{rfc3339}, s3err.GetInvalidDateAttributeErr(rfc3339)}, + {[]string{"not a date"}, s3err.GetInvalidDateAttributeErr("not a date")}, + {[]string{" not a date "}, s3err.GetInvalidDateAttributeErr(" not a date ")}, + {[]string{"first", "second"}, s3err.GetInvalidDateAttributeErr("first")}, + {[]string{""}, malformed}, + {[]string{" "}, malformed}, + // the empty value is reported ahead of an earlier invalid date + {[]string{"not a date", ""}, malformed}, + } { + var body strings.Builder + body.WriteString("") + for _, lmt := range test.lmts { + fmt.Fprintf(&body, "%v%v", obj, lmt) + } + body.WriteString("") + sum := md5.Sum([]byte(body.String())) + + req, err := createSignedReq(http.MethodPost, s.endpoint, bucket+"?delete", + s.awsID, s.awsSecret, "s3", s.awsRegion, "", []byte(body.String()), time.Now(), map[string]string{ + "Content-Md5": base64.StdEncoding.EncodeToString(sum[:]), + }) + if err != nil { + return err + } + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + if err := checkHTTPResponseApiErr(resp, test.err); err != nil { + return fmt.Errorf("test case %d failed: %w", i, err) + } + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + return err + }) +} + // DeleteObjects_iam_mixed_denials_and_success covers a single batch mixing // every DeleteObjects outcome at once: a key the identity policy denies, a // governance-locked key with no bypass, and keys the caller may freely diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 4638b831..95d7a728 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -393,6 +393,9 @@ func TestDeleteObjects(ts *TestState) { ts.Run(DeleteObjects_success) ts.Run(DeleteObjects_quiet_mode) ts.Run(DeleteObjects_conditional) + ts.Run(DeleteObjects_conditional_last_modified_time) + ts.Run(DeleteObjects_last_modified_time_formats) + ts.Run(DeleteObjects_invalid_last_modified_time) ts.Run(DeleteObjects_key_limit) ts.Run(DeleteObjects_invalid_object_keys) } @@ -3240,6 +3243,9 @@ func GetIntTests() IntTests { "DeleteObjects_success": DeleteObjects_success, "DeleteObjects_quiet_mode": DeleteObjects_quiet_mode, "DeleteObjects_conditional": DeleteObjects_conditional, + "DeleteObjects_conditional_last_modified_time": DeleteObjects_conditional_last_modified_time, + "DeleteObjects_last_modified_time_formats": DeleteObjects_last_modified_time_formats, + "DeleteObjects_invalid_last_modified_time": DeleteObjects_invalid_last_modified_time, "DeleteObjects_key_limit": DeleteObjects_key_limit, "DeleteObjects_invalid_object_keys": DeleteObjects_invalid_object_keys, "DeleteObjects_iam_mixed_denials_and_success": DeleteObjects_iam_mixed_denials_and_success,