fix: reject unknown response-* query parameters in GetObject and HeadObject

getobject and headobject silently ignored any response-* query parameter outside the six overridable headers. they now return 400 InvalidArgument naming the parameter and its value, like s3, and the rest tests skipped for this issue are enabled.

Fixes #2075

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{

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{

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")

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{

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 {

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))
+		})
+	}
+}

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),
+	}
+}

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 {

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)
 }

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,

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 {

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"

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"