diff --git a/s3api/controllers/object-get.go b/s3api/controllers/object-get.go index 5e49076..e0a9298 100644 --- a/s3api/controllers/object-get.go +++ b/s3api/controllers/object-get.go @@ -451,6 +451,14 @@ func (c S3ApiController) GetObject(ctx fiber.Ctx) (*Response, error) { isPublicBucketRequest := utils.ContextKeyPublicBucket.IsSet(ctx) utils.ContextKeySkipResBodyLog.Set(ctx, true) + if err := utils.ValidateResponseOverrides(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + // Validate that response override parameters are not used with anonymous requests if hasResponseOverrides && isPublicBucketRequest { return &Response{ diff --git a/s3api/controllers/object-get_test.go b/s3api/controllers/object-get_test.go index a9b7d70..15a9621 100644 --- a/s3api/controllers/object-get_test.go +++ b/s3api/controllers/object-get_test.go @@ -770,6 +770,23 @@ func TestS3ApiController_GetObject(t *testing.T) { err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), }, }, + { + name: "invalid response override query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "response-invalid": "dummy", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgResponseOverride("response-invalid", "dummy"), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/controllers/object-head.go b/s3api/controllers/object-head.go index 2f023d3..a1f1dc6 100644 --- a/s3api/controllers/object-head.go +++ b/s3api/controllers/object-head.go @@ -61,6 +61,14 @@ func (c S3ApiController) HeadObject(ctx fiber.Ctx) (*Response, error) { } } + if err := utils.ValidateResponseOverrides(ctx); err != nil { + return &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: parsedAcl.Owner, + }, + }, err + } + // Validate that response override parameters are not used with anonymous requests if hasResponseOverrides && isPublicBucket { debuglogger.Logf("anonymous access is denied with response override params") diff --git a/s3api/controllers/object-head_test.go b/s3api/controllers/object-head_test.go index ed168ce..0d965e7 100644 --- a/s3api/controllers/object-head_test.go +++ b/s3api/controllers/object-head_test.go @@ -56,6 +56,23 @@ func TestS3ApiController_HeadObject(t *testing.T) { err: s3err.GetInvalidArgumentErr(s3err.InvalidArgEmptyVersionId, ""), }, }, + { + name: "invalid response override query param", + input: testInput{ + locals: defaultLocals, + queries: map[string]string{ + "response-invalid": "dummy", + }, + }, + output: testOutput{ + response: &Response{ + MetaOpts: &MetaOptions{ + BucketOwner: "root", + }, + }, + err: s3err.GetInvalidArgResponseOverride("response-invalid", "dummy"), + }, + }, { name: "verify access fails", input: testInput{ diff --git a/s3api/utils/utils.go b/s3api/utils/utils.go index a3faa6c..3063ddf 100644 --- a/s3api/utils/utils.go +++ b/s3api/utils/utils.go @@ -999,6 +999,36 @@ func ValidateVersionId(ctx fiber.Ctx) error { return nil } +// overridableResponseHeaders lists the response-* query parameters that +// GetObject and HeadObject accept to override response headers. +var overridableResponseHeaders = map[string]struct{}{ + "response-cache-control": {}, + "response-content-disposition": {}, + "response-content-encoding": {}, + "response-content-language": {}, + "response-content-type": {}, + "response-expires": {}, +} + +// ValidateResponseOverrides rejects the first response-* query parameter that +// isn't an overridable response header. S3 answers such a request with 400 +// InvalidArgument instead of ignoring the parameter. +func ValidateResponseOverrides(ctx fiber.Ctx) error { + for key, value := range ctx.Request().URI().QueryArgs().All() { + name := string(key) + if !strings.HasPrefix(name, "response-") { + continue + } + if _, ok := overridableResponseHeaders[name]; ok { + continue + } + debuglogger.Logf("invalid response override query parameter: %s", name) + return s3err.GetInvalidArgResponseOverride(name, string(value)) + } + + return nil +} + // ApplyOverride returns the override value if it exists and status is 200, otherwise returns original func ApplyOverride(original, override *string) *string { if override != nil { diff --git a/s3api/utils/utils_test.go b/s3api/utils/utils_test.go index ea5f9c4..b469f90 100644 --- a/s3api/utils/utils_test.go +++ b/s3api/utils/utils_test.go @@ -1677,3 +1677,52 @@ func TestSetRegionMismatchHeader(t *testing.T) { }) } } + +func TestValidateResponseOverrides(t *testing.T) { + tests := []struct { + name string + query string + want error + }{ + {"no query", "", nil}, + {"no response override", "versionId=id&partNumber=1", nil}, + { + "every overridable response header", + "response-cache-control=a&response-content-disposition=b&response-content-encoding=c&response-content-language=d&response-content-type=e&response-expires=f", + nil, + }, + { + "unknown response header", + "response-fdsjkljj=dummy", + s3err.GetInvalidArgResponseOverride("response-fdsjkljj", "dummy"), + }, + { + "unknown response header without a value", + "response-invalid", + s3err.GetInvalidArgResponseOverride("response-invalid", ""), + }, + { + "bare prefix", + "response-=dummy", + s3err.GetInvalidArgResponseOverride("response-", "dummy"), + }, + { + "unknown response header next to a valid one", + "response-content-type=text%2Fplain&response-invalid=dummy", + s3err.GetInvalidArgResponseOverride("response-invalid", "dummy"), + }, + { + "first of several unknown response headers", + "response-one=1&response-two=2", + s3err.GetInvalidArgResponseOverride("response-one", "1"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := fiber.New().AcquireCtx(&fasthttp.RequestCtx{}) + ctx.Request().URI().SetQueryString(tt.query) + + assert.Equal(t, tt.want, ValidateResponseOverrides(ctx)) + }) + } +} diff --git a/s3err/invalid-argument.go b/s3err/invalid-argument.go index 27f992f..200546d 100644 --- a/s3err/invalid-argument.go +++ b/s3err/invalid-argument.go @@ -401,3 +401,13 @@ func GetInvalidArgBucketRegion(value string) InvalidArgumentError { Description: description, } } + +// GetInvalidArgResponseOverride is returned for a response-* query parameter +// that isn't one of the response headers GetObject and HeadObject can override. +func GetInvalidArgResponseOverride(name, value string) InvalidArgumentError { + return InvalidArgumentError{ + ArgumentName: name, + ArgumentValue: value, + Description: fmt.Sprintf("%s is not in the set of overridable response headers. Please refer to the S3 API documentation for a complete list of overridable response headers.", name), + } +} diff --git a/tests/integration/GetObject.go b/tests/integration/GetObject.go index ab24187..3f875a6 100644 --- a/tests/integration/GetObject.go +++ b/tests/integration/GetObject.go @@ -1305,6 +1305,10 @@ func GetObject_overrides_fail_public(s *S3Conf) error { }, withAnonymousClient()) } +func GetObject_overrides_fail_invalid_header(s *S3Conf) error { + return testInvalidResponseOverride(s, "GetObject_overrides_fail_invalid_header", http.MethodGet) +} + func GetObject_invalid_part_number(s *S3Conf) error { testName := "GetObject_invalid_part_number" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { diff --git a/tests/integration/HeadObject.go b/tests/integration/HeadObject.go index 8a283fe..430b834 100644 --- a/tests/integration/HeadObject.go +++ b/tests/integration/HeadObject.go @@ -1511,6 +1511,10 @@ func HeadObject_overrides_fail_public(s *S3Conf) error { }, withAnonymousClient()) } +func HeadObject_overrides_fail_invalid_header(s *S3Conf) error { + return testInvalidResponseOverride(s, "HeadObject_overrides_fail_invalid_header", http.MethodHead) +} + func HeadObject_empty_version_id(s *S3Conf) error { return testEmptyVersionId(s, "HeadObject_empty_version_id", http.MethodHead, "", nil) } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 328683b..95501ad 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -246,6 +246,7 @@ func TestHeadObject(ts *TestState) { ts.Run(HeadObject_overrides_success) ts.Run(HeadObject_overrides_presign_success) ts.Run(HeadObject_overrides_fail_public) + ts.Run(HeadObject_overrides_fail_invalid_header) ts.Run(HeadObject_range_and_part_number) ts.Run(HeadObject_mp_part_number_exceeds_parts_count) ts.Run(HeadObject_mp_part_number_success) @@ -294,6 +295,7 @@ func TestGetObject(ts *TestState) { ts.Run(GetObject_overrides_success) ts.Run(GetObject_overrides_presign_success) ts.Run(GetObject_overrides_fail_public) + ts.Run(GetObject_overrides_fail_invalid_header) ts.Run(GetObject_invalid_part_number) ts.Run(GetObject_range_and_part_number) ts.Run(GetObject_mp_part_number_exceeds_parts_count) @@ -3064,6 +3066,7 @@ func GetIntTests() IntTests { "HeadObject_overrides_success": HeadObject_overrides_success, "HeadObject_overrides_presign_success": HeadObject_overrides_presign_success, "HeadObject_overrides_fail_public": HeadObject_overrides_fail_public, + "HeadObject_overrides_fail_invalid_header": HeadObject_overrides_fail_invalid_header, "HeadObject_range_and_part_number": HeadObject_range_and_part_number, "HeadObject_mp_part_number_exceeds_parts_count": HeadObject_mp_part_number_exceeds_parts_count, "HeadObject_mp_part_number_success": HeadObject_mp_part_number_success, @@ -3100,6 +3103,7 @@ func GetIntTests() IntTests { "GetObject_overrides_success": GetObject_overrides_success, "GetObject_overrides_presign_success": GetObject_overrides_presign_success, "GetObject_overrides_fail_public": GetObject_overrides_fail_public, + "GetObject_overrides_fail_invalid_header": GetObject_overrides_fail_invalid_header, "GetObject_invalid_part_number": GetObject_invalid_part_number, "GetObject_range_and_part_number": GetObject_range_and_part_number, "GetObject_mp_part_number_exceeds_parts_count": GetObject_mp_part_number_exceeds_parts_count, diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 429f1a8..fc42cbc 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -612,6 +612,63 @@ func testEmptyVersionId(s *S3Conf, testName, method, subresource string, body [] }) } +// testInvalidResponseOverride verifies GetObject or HeadObject rejects a +// response-* query parameter that isn't an overridable response header. The +// SDK only sends the known overrides, so the request is signed and sent by +// hand. +func testInvalidResponseOverride(s *S3Conf, testName, method string) error { + 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 + } + + for _, test := range []struct { + query string + expected s3err.InvalidArgumentError + }{ + { + query: "response-fdsjkljj=dummy", + expected: s3err.GetInvalidArgResponseOverride("response-fdsjkljj", "dummy"), + }, + // a valid override doesn't let an unknown one through + { + query: "response-content-type=text%2Fplain&response-invalid=dummy", + expected: s3err.GetInvalidArgResponseOverride("response-invalid", "dummy"), + }, + } { + req, err := createSignedReq(method, s.endpoint, + fmt.Sprintf("%v/%v?%v", bucket, obj, test.query), s.awsID, s.awsSecret, + "s3", s.awsRegion, "", nil, time.Now(), nil) + if err != nil { + return err + } + + resp, err := s.httpClient.Do(req) + if err != nil { + return err + } + + // HEAD responses carry no body: only the status code is verifiable + if method == http.MethodHead { + resp.Body.Close() + if resp.StatusCode != test.expected.StatusCode() { + return fmt.Errorf("%v: expected response status code to be %v, instead got %v", + test.query, test.expected.StatusCode(), resp.StatusCode) + } + continue + } + + if err := checkHTTPResponseApiErr(resp, test.expected); err != nil { + return fmt.Errorf("%v: %w", test.query, err) + } + } + + return nil + }) +} + func checkIAMAuthRequest(s *S3Conf, req *http.Request, expected iamerr.APIError) error { resp, err := s.httpClient.Do(req) if err != nil { diff --git a/tests/test_rest_get_object.sh b/tests/test_rest_get_object.sh index c6133dd..d39ba42 100644 --- a/tests/test_rest_get_object.sh +++ b/tests/test_rest_get_object.sh @@ -119,9 +119,6 @@ source ./tests/setup.sh # tags: curl,GetObject,invalid-header,response-headers @test "REST - GetObject - response query - invalid response type" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/2075" - fi run setup_bucket_and_file_v3 "$BUCKET_ONE_NAME" assert_success read -r bucket_name file_name <<< "$output" diff --git a/tests/test_rest_head_object.sh b/tests/test_rest_head_object.sh index 2758b9d..a96e6fd 100644 --- a/tests/test_rest_head_object.sh +++ b/tests/test_rest_head_object.sh @@ -103,9 +103,6 @@ source ./tests/drivers/string.sh # tags: curl,HeadObject,invalid-query @test "REST - HeadObject - invalid request header type" { - if [ "$DIRECT" != "true" ]; then - skip "https://github.com/versity/versitygw/issues/2075" - fi run setup_bucket_and_add_file_v3 "$BUCKET_ONE_NAME" assert_success read -r bucket_name file_name <<< "$output"