From d62aa7b9794e46ffac838a416207489556408fb8 Mon Sep 17 00:00:00 2001 From: Denis Kirillov Date: Mon, 10 Jul 2023 09:59:12 +0300 Subject: [PATCH] [#146] Fix preconditions: trim quotes in etags Signed-off-by: Denis Kirillov --- api/handler/get.go | 19 +++++++++++++------ api/handler/get_test.go | 7 ++++++- api/handler/head_test.go | 7 +++++++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/api/handler/get.go b/api/handler/get.go index 615f7f0..4c3bf38 100644 --- a/api/handler/get.go +++ b/api/handler/get.go @@ -231,17 +231,24 @@ func (h *handler) GetObjectHandler(w http.ResponseWriter, r *http.Request) { func checkPreconditions(info *data.ObjectInfo, args *conditionalArgs) error { if len(args.IfMatch) > 0 && args.IfMatch != info.HashSum { - return errors.GetAPIError(errors.ErrPreconditionFailed) + return fmt.Errorf("%w: etag mismatched: '%s', '%s'", errors.GetAPIError(errors.ErrPreconditionFailed), args.IfMatch, info.HashSum) } if len(args.IfNoneMatch) > 0 && args.IfNoneMatch == info.HashSum { - return errors.GetAPIError(errors.ErrNotModified) + return fmt.Errorf("%w: etag matched: '%s', '%s'", errors.GetAPIError(errors.ErrNotModified), args.IfNoneMatch, info.HashSum) } if args.IfModifiedSince != nil && info.Created.Before(*args.IfModifiedSince) { - return errors.GetAPIError(errors.ErrNotModified) + return fmt.Errorf("%w: not modified since '%s', last modified '%s'", errors.GetAPIError(errors.ErrNotModified), + args.IfModifiedSince.Format(time.RFC3339), info.Created.Format(time.RFC3339)) } if args.IfUnmodifiedSince != nil && info.Created.After(*args.IfUnmodifiedSince) { + // https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html#API_GetObject_RequestSyntax + // If both of the If-Match and If-Unmodified-Since headers are present in the request as follows: + // If-Match condition evaluates to true, and; + // If-Unmodified-Since condition evaluates to false; + // then, S3 returns 200 OK and the data requested. if len(args.IfMatch) == 0 { - return errors.GetAPIError(errors.ErrPreconditionFailed) + return fmt.Errorf("%w: modified since '%s', last modified '%s'", errors.GetAPIError(errors.ErrPreconditionFailed), + args.IfUnmodifiedSince.Format(time.RFC3339), info.Created.Format(time.RFC3339)) } } @@ -251,8 +258,8 @@ func checkPreconditions(info *data.ObjectInfo, args *conditionalArgs) error { func parseConditionalHeaders(headers http.Header) (*conditionalArgs, error) { var err error args := &conditionalArgs{ - IfMatch: headers.Get(api.IfMatch), - IfNoneMatch: headers.Get(api.IfNoneMatch), + IfMatch: strings.Trim(headers.Get(api.IfMatch), "\""), + IfNoneMatch: strings.Trim(headers.Get(api.IfNoneMatch), "\""), } if args.IfModifiedSince, err = parseHTTPTime(headers.Get(api.IfModifiedSince)); err != nil { diff --git a/api/handler/get_test.go b/api/handler/get_test.go index 22c7784..996dc2e 100644 --- a/api/handler/get_test.go +++ b/api/handler/get_test.go @@ -2,6 +2,7 @@ package handler import ( "bytes" + stderrors "errors" "fmt" "io" "net/http" @@ -149,7 +150,11 @@ func TestPreconditions(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { actual := checkPreconditions(tc.info, tc.args) - require.Equal(t, tc.expected, actual) + if tc.expected == nil { + require.NoError(t, actual) + } else { + require.True(t, stderrors.Is(actual, tc.expected), tc.expected, actual) + } }) } } diff --git a/api/handler/head_test.go b/api/handler/head_test.go index d0c1225..2936ceb 100644 --- a/api/handler/head_test.go +++ b/api/handler/head_test.go @@ -27,10 +27,14 @@ func TestConditionalHead(t *testing.T) { tc.Handler().HeadObjectHandler(w, r) assertStatus(t, w, http.StatusOK) etag := w.Result().Header.Get(api.ETag) + etagQuoted := "\"" + etag + "\"" headers := map[string]string{api.IfMatch: etag} headObject(t, tc, bktName, objName, headers, http.StatusOK) + headers = map[string]string{api.IfMatch: etagQuoted} + headObject(t, tc, bktName, objName, headers, http.StatusOK) + headers = map[string]string{api.IfMatch: "etag"} headObject(t, tc, bktName, objName, headers, http.StatusPreconditionFailed) @@ -50,6 +54,9 @@ func TestConditionalHead(t *testing.T) { headers = map[string]string{api.IfNoneMatch: etag} headObject(t, tc, bktName, objName, headers, http.StatusNotModified) + headers = map[string]string{api.IfNoneMatch: etagQuoted} + headObject(t, tc, bktName, objName, headers, http.StatusNotModified) + headers = map[string]string{api.IfNoneMatch: "etag"} headObject(t, tc, bktName, objName, headers, http.StatusOK)