Skip to content

HYPERFLEET-649 - fix: Reject cluster creation with missing spec field #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Feb 23, 2026
Merged

HYPERFLEET-649 - fix: Reject cluster creation with missing spec field #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/handlers/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func (h clusterHandler) Create(w http.ResponseWriter, r *http.Request) {
validateEmpty(&req, "Id", "id"),
validateName(&req, "Name", "name", 3, 53),
validateKind(&req, "Kind", "kind", "Cluster"),
validateSpec(&req, "Spec", "spec"),
Copy link
Contributor

@rafabene rafabene Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validateSpec check was added to the cluster creation handler, but the nodepool creation
handler in cluster_nodepools.go has the same gap. The NodePoolCreateRequest schema also
defines spec as a required field, and the SchemaValidationMiddleware skips validation when the
spec key is entirely absent from the JSON body (it only catches spec: null). This means a
nodepool can still be created without a spec field.

Consider adding validateSpec(&req, "Spec", "spec") to the validators list in
clusterNodePoolsHandler.Create:

  []validate{
      validateEmpty(&req, "Id", "id"),
      validateName(&req, "Name", "name", 3, 15),
      validateKind(&req, "Kind", "kind", "NodePool"),
      validateSpec(&req, "Spec", "spec"),
  },

This may be intentionally out of scope for this ticket, but worth noting since it's the same
vulnerability

Copy link
Contributor Author

@Mischulee Mischulee Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! I will include it in this PR since its same bug with same solution both violating OpenAPI specification.

},
func() (interface{}, *errors.ServiceError) {
ctx := r.Context()
Expand Down
1 change: 1 addition & 0 deletions pkg/handlers/cluster_nodepools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ func (h clusterNodePoolsHandler) Create(w http.ResponseWriter, r *http.Request)
validateEmpty(&req, "Id", "id"),
validateName(&req, "Name", "name", 3, 15),
validateKind(&req, "Kind", "kind", "NodePool"),
validateSpec(&req, "Spec", "spec"),
},
func() (interface{}, *errors.ServiceError) {
ctx := r.Context()
Expand Down
20 changes: 20 additions & 0 deletions pkg/handlers/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,23 @@ func validateKind(i interface{}, fieldName string, field string, expectedKind st
return nil
}
}

// validateSpec validates that the spec field is not nil
func validateSpec(i interface{}, fieldName string, field string) validate {
Copy link
Contributor

@rafabene rafabene Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validateSpec function doesn't follow the same defensive pattern used by all other
validators in this file. Every other validator checks value.Kind() == reflect.Ptr before
calling methods like IsNil() and dereferences the pointer when needed. Consider adding the
same guard for consistency and safety:

  func validateSpec(i interface{}, fieldName string, field string) validate {
        return func() *errors.ServiceError {
                value := reflect.ValueOf(i).Elem().FieldByName(fieldName)
                if value.Kind() == reflect.Ptr {
                        if value.IsNil() {
                                return errors.Validation("%s is required", field)
                        }
                        value = value.Elem()
                }

                if !value.IsValid() || value.IsNil() {
                        return errors.Validation("%s is required", field)
                }

                return nil
        }
  }

This makes the function safe for reuse if the Spec type ever changes (e.g., to a pointer to a
struct) and keeps the codebase consistent.

Copy link
Contributor Author

@Mischulee Mischulee Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. validateSpec now follows the same defensive pattern.

return func() *errors.ServiceError {
value := reflect.ValueOf(i).Elem().FieldByName(fieldName)

if value.Kind() == reflect.Ptr {
if value.IsNil() {
return errors.Validation("%s is required", field)
}
value = value.Elem()
}

if !value.IsValid() || value.IsNil() {
return errors.Validation("%s is required", field)
}

return nil
}
}
48 changes: 41 additions & 7 deletions pkg/handlers/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,13 @@ func TestValidateNodePoolName_InvalidCharacters(t *testing.T) {
RegisterTestingT(t)

invalidNames := []string{
"TEST", // uppercase
"Test", // mixed case
"test_pool", // underscore
"test.pool", // dot
"test pool", // space
"-test", // starts with hyphen
"test-", // ends with hyphen
"TEST", // uppercase
"Test", // mixed case
"test_pool", // underscore
"test.pool", // dot
"test pool", // space
"-test", // starts with hyphen
"test-", // ends with hyphen
}

for _, name := range invalidNames {
Expand All @@ -231,3 +231,37 @@ func TestValidateNodePoolName_InvalidCharacters(t *testing.T) {
Expect(err.Reason).To(ContainSubstring("lowercase letters, numbers, and hyphens"))
}
}

func TestValidateSpec_Valid(t *testing.T) {
RegisterTestingT(t)

req := openapi.ClusterCreateRequest{
Spec: map[string]interface{}{"test": "value"},
}
validator := validateSpec(&req, "Spec", "spec")
err := validator()
Expect(err).To(BeNil(), "Expected existing spec to be valid")
}

func TestValidateSpec_EmptyMap(t *testing.T) {
RegisterTestingT(t)

req := openapi.ClusterCreateRequest{
Spec: map[string]interface{}{},
}
validator := validateSpec(&req, "Spec", "spec")
err := validator()
Expect(err).To(BeNil(), "Expected empty map spec to be valid")
}

func TestValidateSpec_Nil(t *testing.T) {
RegisterTestingT(t)

req := openapi.ClusterCreateRequest{
Spec: nil,
}
validator := validateSpec(&req, "Spec", "spec")
err := validator()
Expect(err).ToNot(BeNil(), "Expected nil spec to be invalid")
Expect(err.Reason).To(ContainSubstring("spec is required"))
}
77 changes: 75 additions & 2 deletions test/integration/clusters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,8 +555,8 @@ func TestClusterList_DefaultSorting(t *testing.T) {
}

resp, err := client.PostClusterWithResponse(
ctx, openapi.PostClusterJSONRequestBody(clusterInput), test.WithAuthToken(ctx),
)
ctx, openapi.PostClusterJSONRequestBody(clusterInput), test.WithAuthToken(ctx),
)
Expect(err).NotTo(HaveOccurred(), "Failed to create cluster %d", i)
createdClusters = append(createdClusters, *resp.JSON201)

Expand Down Expand Up @@ -778,3 +778,76 @@ func TestClusterPost_WrongKind(t *testing.T) {
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("kind must be 'Cluster'"))
}

// TestClusterPost_NullSpec tests that null spec field returns 400
func TestClusterPost_NullSpec(t *testing.T) {
h, _ := test.RegisterIntegration(t)

account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)
jwtToken := test.GetAccessTokenFromContext(ctx)

// Send request with null spec
invalidInput := `{
"kind": "Cluster",
"name": "test-cluster",
"spec": null
}`

restyResp, err := resty.R().
SetHeader("Content-Type", "application/json").
SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
SetBody(invalidInput).
Post(h.RestURL("/clusters"))

Expect(err).ToNot(HaveOccurred())
Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))

var errorResponse map[string]interface{}
err = json.Unmarshal(restyResp.Body(), &errorResponse)
Expect(err).ToNot(HaveOccurred())

code, ok := errorResponse["code"].(string)
Expect(ok).To(BeTrue())
Expect(code).To(Equal("HYPERFLEET-VAL-000"))

detail, ok := errorResponse["detail"].(string)
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("spec field must be an object"))
}

// TestClusterPost_MissingSpec tests that missing spec field returns 400
func TestClusterPost_MissingSpec(t *testing.T) {
h, _ := test.RegisterIntegration(t)

account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)
jwtToken := test.GetAccessTokenFromContext(ctx)

// Send request without spec field
invalidInput := `{
"kind": "Cluster",
"name": "test-cluster"
}`

restyResp, err := resty.R().
SetHeader("Content-Type", "application/json").
SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
SetBody(invalidInput).
Post(h.RestURL("/clusters"))

Expect(err).ToNot(HaveOccurred())
Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))

var errorResponse map[string]interface{}
err = json.Unmarshal(restyResp.Body(), &errorResponse)
Expect(err).ToNot(HaveOccurred())

code, ok := errorResponse["code"].(string)
Expect(ok).To(BeTrue())
Expect(code).To(Equal("HYPERFLEET-VAL-000"))

detail, ok := errorResponse["detail"].(string)
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("spec is required"))
}
79 changes: 79 additions & 0 deletions test/integration/node_pools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,82 @@ func TestNodePoolDuplicateNames(t *testing.T) {
Expect(*problemDetail.Detail).To(ContainSubstring("already exists"),
"Expected error detail to mention that resource already exists")
}

// TestNodePoolPost_NullSpec tests that null spec field returns 400
func TestNodePoolPost_NullSpec(t *testing.T) {
h, _ := test.RegisterIntegration(t)

account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)
jwtToken := test.GetAccessTokenFromContext(ctx)

cluster, err := h.Factories.NewClusters(h.NewID())
Expect(err).NotTo(HaveOccurred())

// Send request with null spec
invalidInput := `{
"kind": "NodePool",
"name": "test-nodepool",
"spec": null
}`

restyResp, err := resty.R().
SetHeader("Content-Type", "application/json").
SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
SetBody(invalidInput).
Post(h.RestURL(fmt.Sprintf("/clusters/%s/nodepools", cluster.ID)))

Expect(err).ToNot(HaveOccurred())
Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))

var errorResponse map[string]interface{}
err = json.Unmarshal(restyResp.Body(), &errorResponse)
Expect(err).ToNot(HaveOccurred())

code, ok := errorResponse["code"].(string)
Expect(ok).To(BeTrue())
Expect(code).To(Equal("HYPERFLEET-VAL-000"))

detail, ok := errorResponse["detail"].(string)
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("spec field must be an object"))
}

// TestNodePoolPost_MissingSpec tests that missing spec field returns 400
func TestNodePoolPost_MissingSpec(t *testing.T) {
h, _ := test.RegisterIntegration(t)

account := h.NewRandAccount()
ctx := h.NewAuthenticatedContext(account)
jwtToken := test.GetAccessTokenFromContext(ctx)

cluster, err := h.Factories.NewClusters(h.NewID())
Expect(err).NotTo(HaveOccurred())

// Send request without spec field
invalidInput := `{
"kind": "NodePool",
"name": "test-nodepool"
}`

restyResp, err := resty.R().
SetHeader("Content-Type", "application/json").
SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)).
SetBody(invalidInput).
Post(h.RestURL(fmt.Sprintf("/clusters/%s/nodepools", cluster.ID)))

Expect(err).ToNot(HaveOccurred())
Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))

var errorResponse map[string]interface{}
err = json.Unmarshal(restyResp.Body(), &errorResponse)
Expect(err).ToNot(HaveOccurred())

code, ok := errorResponse["code"].(string)
Expect(ok).To(BeTrue())
Expect(code).To(Equal("HYPERFLEET-VAL-000"))

detail, ok := errorResponse["detail"].(string)
Expect(ok).To(BeTrue())
Expect(detail).To(ContainSubstring("spec is required"))
}