fix: accept HTTP dates for LastModifiedTime in DeleteObjects

Follow-up to #2486

The AWS SDKs and CLI send `ObjectIdentifier.LastModifiedTime` as an HTTP date, but `s3response.DeleteObjects` decoded it into the SDK's `time.Time` field, which accepts only RFC 3339. Any `DeleteObjects` request that carried a `LastModifiedTime` was therefore rejected as a whole with `InvalidRequest`, and the RFC 3339 form the gateway did accept is one that S3 rejects.

`DeleteObjects` now has its own `UnmarshalXML`. It embeds `types.ObjectIdentifier`, decodes `LastModifiedTime` as a string and parses it with `smithytime.ParseHTTPDate`, falling back to `time.RFC1123Z` for numeric zone offsets. This accepts IMF-fixdate, RFC 850, ANSIC and `+hhmm` offsets and rejects RFC 3339, which matches S3 directory buckets, the only bucket type where S3 evaluates this field.

Errors are reported as S3 reports them. An unparseable date returns `InvalidRequest` with "Invalid value for date attribute: <value>" through the new `s3err.GetInvalidDateAttributeErr`, which echoes the raw value of the first bad date in the body. An empty, whitespace-only or self-closing `LastModifiedTime` returns `MalformedXML`, which takes precedence over an invalid date in any object. The `DeleteObjects` controller now passes S3 errors from the decoder through instead of turning every decode failure into `InvalidRequest`.
This commit is contained in:
niksis02
2026-10-21 02:40:58 +04:00
parent 007a858918
commit 7662573e5e
7 changed files with 491 additions and 1 deletions
+8 -1
View File
@@ -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
+66
View File
@@ -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(`<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">` +
`<Object><Key>obj</Key>` +
`<LastModifiedTime>Wed, 07 Oct 2026 05:00:00 GMT</LastModifiedTime>` +
`</Object></Delete>`)
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(`<Delete><Object><Key>obj</Key>` +
`<LastModifiedTime>2026-10-07T05:00:00Z</LastModifiedTime></Object></Delete>`),
},
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(`<Delete><Object><Key>obj</Key>` +
`<LastModifiedTime></LastModifiedTime></Object></Delete>`),
},
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{
+10
View File
@@ -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{
+54
View File
@@ -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
+141
View File
@@ -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 := `<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">` +
`<Object><Key>k</Key><ETag>"abc"</ETag>` +
`<LastModifiedTime>` + tt.lmt + `</LastModifiedTime>` +
`<Size>5</Size><VersionId>v1</VersionId></Object>` +
`<Quiet>true</Quiet></Delete>`
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: `<Object><Key>k</Key><LastModifiedTime>2026-10-07T05:00:00Z</LastModifiedTime></Object>`,
want: s3err.GetInvalidDateAttributeErr("2026-10-07T05:00:00Z"),
},
{
name: "not a date",
objects: `<Object><Key>k</Key><LastModifiedTime>not a date</LastModifiedTime></Object>`,
want: s3err.GetInvalidDateAttributeErr("not a date"),
},
{
name: "padded value echoed as sent",
objects: `<Object><Key>k</Key><LastModifiedTime> not a date </LastModifiedTime></Object>`,
want: s3err.GetInvalidDateAttributeErr(" not a date "),
},
{
name: "first invalid date reported",
objects: `<Object><Key>a</Key><LastModifiedTime>first</LastModifiedTime></Object>` +
`<Object><Key>b</Key><LastModifiedTime>second</LastModifiedTime></Object>`,
want: s3err.GetInvalidDateAttributeErr("first"),
},
{
name: "empty",
objects: `<Object><Key>k</Key><LastModifiedTime></LastModifiedTime></Object>`,
want: malformed,
},
{
name: "whitespace only",
objects: `<Object><Key>k</Key><LastModifiedTime> </LastModifiedTime></Object>`,
want: malformed,
},
{
name: "self-closing",
objects: `<Object><Key>k</Key><LastModifiedTime/></Object>`,
want: malformed,
},
{
name: "empty wins over an earlier invalid date",
objects: `<Object><Key>a</Key><LastModifiedTime>not a date</LastModifiedTime></Object>` +
`<Object><Key>b</Key><LastModifiedTime></LastModifiedTime></Object>`,
want: malformed,
},
}
for _, tt := range invalid {
t.Run(tt.name, func(t *testing.T) {
var d DeleteObjects
err := xml.Unmarshal([]byte(`<Delete>`+tt.objects+`</Delete>`), &d)
var apiErr s3err.APIError
if !errors.As(err, &apiErr) || apiErr != tt.want {
t.Fatalf("expected %v, got %v", tt.want, err)
}
})
}
}
+206
View File
@@ -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("<Delete>")
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, "<Object><Key>%v</Key><LastModifiedTime>%v</LastModifiedTime></Object>",
form.key, form.format(*head.LastModified))
}
body.WriteString("</Delete>")
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("<Delete>")
for _, lmt := range test.lmts {
fmt.Fprintf(&body, "<Object><Key>%v</Key><LastModifiedTime>%v</LastModifiedTime></Object>", obj, lmt)
}
body.WriteString("</Delete>")
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
+6
View File
@@ -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,