diff --git a/pkg/handlers/framework.go b/pkg/handlers/framework.go index e178ff57..743a1634 100755 --- a/pkg/handlers/framework.go +++ b/pkg/handlers/framework.go @@ -3,8 +3,10 @@ package handlers import ( "bytes" "encoding/json" + goerrors "errors" "io" "net/http" + "reflect" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/presenters" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/response" @@ -85,11 +87,19 @@ func handle(w http.ResponseWriter, r *http.Request, cfg *handlerConfig, httpStat dec := json.NewDecoder(bytes.NewReader(bodyBytes)) dec.DisallowUnknownFields() if err := dec.Decode(cfg.MarshalInto); err != nil { + if typeErr := cleanTypeMismatchError(err); typeErr != nil { + handleError(r, w, typeErr) + return + } handleError(r, w, errors.BadRequest("Unknown or immutable field in request body: %s", err)) return } } else { if err := json.Unmarshal(bodyBytes, &cfg.MarshalInto); err != nil { + if typeErr := cleanTypeMismatchError(err); typeErr != nil { + handleError(r, w, typeErr) + return + } handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) return } @@ -138,6 +148,35 @@ func handleSoftDelete(w http.ResponseWriter, r *http.Request, cfg *handlerConfig } +func cleanTypeMismatchError(err error) *errors.ServiceError { + var typeErr *json.UnmarshalTypeError + if !goerrors.As(err, &typeErr) { + return nil + } + return errors.Validation("field '%s' must be %s", typeErr.Field, describeJSONKind(typeErr.Type.Kind())) +} + +func describeJSONKind(k reflect.Kind) string { + switch k { + case reflect.Map, reflect.Struct: + return "an object" + case reflect.Slice, reflect.Array: + return "an array" + case reflect.String: + return "a string" + case reflect.Bool: + return "a boolean" + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return "a number" + default: + // Fallback for kinds json.UnmarshalTypeError.Type should never report + // here (e.g. Ptr, Interface) — avoids mislabeling as a number. + return "the correct type" + } +} + func handleGet(w http.ResponseWriter, r *http.Request, cfg *handlerConfig) { if cfg.ErrorHandler == nil { cfg.ErrorHandler = handleError @@ -180,6 +219,10 @@ func handleForceDelete(w http.ResponseWriter, r *http.Request, cfg *handlerConfi err = json.Unmarshal(bytes, &cfg.MarshalInto) if err != nil { + if typeErr := cleanTypeMismatchError(err); typeErr != nil { + handleError(r, w, typeErr) + return + } handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) return } @@ -217,6 +260,10 @@ func handleCreateWithNoContent(w http.ResponseWriter, r *http.Request, cfg *hand err = json.Unmarshal(bytes, &cfg.MarshalInto) if err != nil { + if typeErr := cleanTypeMismatchError(err); typeErr != nil { + handleError(r, w, typeErr) + return + } handleError(r, w, errors.MalformedRequest("Invalid request format: %s", err)) return } diff --git a/pkg/handlers/resource_handler_test.go b/pkg/handlers/resource_handler_test.go index 3ece8726..e4829091 100644 --- a/pkg/handlers/resource_handler_test.go +++ b/pkg/handlers/resource_handler_test.go @@ -276,6 +276,25 @@ func TestResourceHandler_Patch(t *testing.T) { }, expectedStatusCode: http.StatusOK, }, + { + // References-only patch must be accepted, not rejected as "no field provided" — HYPERFLEET-1376 finding 8. + name: "Success - references only", + id: "ch-123", + body: `{"references":{"parents":[{"kind":"Cluster","id":"cluster-1"}]}}`, + setupMock: func(mock *services.MockResourceService) { + mock.EXPECT().Patch(gomock.Any(), "Channel", "ch-123", gomock.AssignableToTypeOf(&api.ResourcePatch{})). + Return(&api.Resource{ + Meta: api.Meta{ID: "ch-123", CreatedTime: now, UpdatedTime: now}, + Kind: "Channel", + Name: "stable", + Spec: datatypes.JSON(`{"is_default":false}`), + Generation: 2, + CreatedBy: "user@test.com", + UpdatedBy: "user@test.com", + }, nil) + }, + expectedStatusCode: http.StatusOK, + }, { name: "Not found", id: "nonexistent", @@ -728,6 +747,15 @@ func TestResourceHandler_ForceDelete(t *testing.T) { }, expectedStatusCode: http.StatusBadRequest, }, + { + // Wrong JSON type for "reason" must return a clean message without leaking + // the Go struct name (ForceDeleteRequest) — HYPERFLEET-1376. + name: "Error 400 - reason wrong type", + body: `{"reason": 123}`, + setupMock: func(mock *services.MockResourceService) { + }, + expectedStatusCode: http.StatusBadRequest, + }, { name: "Error 400 - empty reason", body: `{"reason": ""}`, @@ -797,6 +825,10 @@ func TestResourceHandler_ForceDelete(t *testing.T) { if tt.expectedStatusCode == http.StatusNoContent { Expect(rr.Body.Len()).To(Equal(0)) } + if tt.name == "Error 400 - reason wrong type" { + Expect(rr.Body.String()).To(ContainSubstring("field 'reason' must be a string")) + Expect(rr.Body.String()).ToNot(ContainSubstring("ForceDeleteRequest")) + } }) } } @@ -933,6 +965,46 @@ func TestResourceHandler_Patch_RejectsUnknownFields(t *testing.T) { } } +// TestResourceHandler_Patch_LabelsTypeMismatch verifies that a wrong JSON type for +// "labels" returns a clean validation message without leaking the Go struct name +// (e.g. "ResourcePatchRequest") or type (e.g. "map[string]string") — HYPERFLEET-1376 +// findings 5 & 6. +func TestResourceHandler_Patch_LabelsTypeMismatch(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + name string + body string + }{ + {"labels as string", `{"labels":"not-an-object"}`}, + {"labels as array", `{"labels":["a","b"]}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + handler, _ := newTestResourceHandler(ctrl) + + reqURL := "/api/hyperfleet/v1/channels/ch-123" + req := httptest.NewRequest(http.MethodPatch, reqURL, strings.NewReader(tt.body)) + req.Header.Set("Content-Type", "application/json") + req = mux.SetURLVars(req, map[string]string{"id": "ch-123"}) + + rr := httptest.NewRecorder() + handler.Patch(rr, req) + + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + Expect(rr.Body.String()).To(ContainSubstring("field 'labels' must be an object")) + Expect(rr.Body.String()).ToNot(ContainSubstring("ResourcePatchRequest")) + Expect(rr.Body.String()).ToNot(ContainSubstring("openapi.")) + }) + } +} + func TestResourceHandler_PatchByOwner_RejectsUnknownFields(t *testing.T) { RegisterTestingT(t) diff --git a/pkg/handlers/resource_status_handler_test.go b/pkg/handlers/resource_status_handler_test.go index 7973fd1b..830d3d89 100644 --- a/pkg/handlers/resource_status_handler_test.go +++ b/pkg/handlers/resource_status_handler_test.go @@ -194,6 +194,31 @@ func TestResourceStatusHandler_Create_MissingAdapter_Returns400(t *testing.T) { Expect(w.Code).To(Equal(http.StatusBadRequest)) } +// TestResourceStatusHandler_Create_ConditionsTypeMismatch verifies that a wrong JSON +// type for "conditions" returns a clean validation message without leaking the Go +// struct/package name (e.g. "AdapterStatusCreateRequest", "openapi.ConditionRequest") +// — HYPERFLEET-1376 finding 7. +func TestResourceStatusHandler_Create_ConditionsTypeMismatch(t *testing.T) { + RegisterTestingT(t) + ctrl := gomock.NewController(t) + + handler, _, _ := newTestResourceStatusHandler(ctrl) + + body := `{"adapter":"x","conditions":"not-an-array"}` + + r := httptest.NewRequest(http.MethodPut, "/channels/ch-1/statuses", strings.NewReader(body)) + r.Header.Set("Content-Type", "application/json") + r = mux.SetURLVars(r, map[string]string{"id": testChannelID}) + w := httptest.NewRecorder() + + handler.Create(w, r) + + Expect(w.Code).To(Equal(http.StatusBadRequest)) + Expect(w.Body.String()).To(ContainSubstring("field 'conditions' must be an array")) + Expect(w.Body.String()).ToNot(ContainSubstring("AdapterStatusCreateRequest")) + Expect(w.Body.String()).ToNot(ContainSubstring("openapi.")) +} + func TestResourceStatusHandler_Create_ResourceNotFound(t *testing.T) { RegisterTestingT(t) ctrl := gomock.NewController(t) diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index 1433752f..02b61878 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -247,18 +247,20 @@ func validateLabels(i interface{}, fieldName string) validate { } } -// validatePatchRequest validates that at least one patchable field (Spec or Labels) is provided +// validatePatchRequest validates that at least one patchable field (Spec, Labels, or References) is provided func validatePatchRequest(i interface{}) validate { return func() *errors.ServiceError { v := reflect.ValueOf(i).Elem() spec := v.FieldByName("Spec") labels := v.FieldByName("Labels") + references := v.FieldByName("References") specPresent := spec.IsValid() && !spec.IsNil() labelsPresent := labels.IsValid() && !labels.IsNil() + referencesPresent := references.IsValid() && !references.IsNil() - if !specPresent && !labelsPresent { + if !specPresent && !labelsPresent && !referencesPresent { return errors.BadRequest("at least one field must be provided for update") } return nil diff --git a/pkg/handlers/validation_test.go b/pkg/handlers/validation_test.go index d7b44fe1..6f0edaa8 100644 --- a/pkg/handlers/validation_test.go +++ b/pkg/handlers/validation_test.go @@ -401,6 +401,56 @@ func TestValidatePatchRequest(t *testing.T) { } } +// TestValidatePatchRequest_References uses openapi.ResourcePatchRequest (rather than +// ClusterPatchRequest, which has no References field) to verify a references-only +// patch is accepted — see HYPERFLEET-1376 finding 8. +func TestValidatePatchRequest_References(t *testing.T) { + references := map[string][]openapi.ObjectReference{ + "parents": {{Kind: "Cluster", Id: strPtr("cluster-1")}}, + } + labels := map[string]string{"env": "prod"} + + testCases := []struct { + req openapi.ResourcePatchRequest + name string + expectError bool + }{ + { + name: "references only is valid", + req: openapi.ResourcePatchRequest{References: &references}, + expectError: false, + }, + { + name: "all nil returns error", + req: openapi.ResourcePatchRequest{}, + expectError: true, + }, + { + name: "references and labels is valid", + req: openapi.ResourcePatchRequest{References: &references, Labels: &labels}, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + RegisterTestingT(t) + validator := validatePatchRequest(&tc.req) + err := validator() + if tc.expectError { + Expect(err).ToNot(BeNil()) + Expect(err.Reason).To(ContainSubstring("at least one field must be provided for update")) + } else { + Expect(err).To(BeNil()) + } + }) + } +} + +func strPtr(s string) *string { + return &s +} + func TestValidateObservedGeneration(t *testing.T) { testCases := []struct { name string