17257 validate bulk ops request filters (#17514)

changes/17257-host-ops-validation

@@ -0,0 +1 @@
+- added validation to the json request for bulk host operations for transfer and delete

server/fleet/hosts.go

@@ -39,6 +39,15 @@ const (
 	OnlineIntervalBuffer = 60
 )
 
+func (s HostStatus) IsValid() bool {
+	switch s {
+	case StatusOnline, StatusOffline, StatusNew, StatusMissing, StatusMIA:
+		return true
+	default:
+		return false
+	}
+}
+
 // MDMEnrollStatus defines the possible MDM enrollment statuses.
 type MDMEnrollStatus string
 

server/fleet/hosts_test.go

@@ -52,6 +52,26 @@ func TestHostStatus(t *testing.T) {
 	}
 }
 
+func TestHostStatusIsValid(t *testing.T) {
+	for _, tt := range []struct {
+		name     string
+		status   HostStatus
+		expected bool
+	}{
+		{"online", StatusOnline, true},
+		{"offline", StatusOffline, true},
+		{"new", StatusNew, true},
+		{"missing", StatusMissing, true},
+		{"mia", StatusMIA, true}, // As of Fleet 4.15, StatusMIA is deprecated in favor of StatusOffline
+		{"empty", HostStatus(""), false},
+		{"invalid", HostStatus("invalid"), false},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, tt.status.IsValid())
+		})
+	}
+}
+
 func TestHostIsNew(t *testing.T) {
 	mockClock := clock.NewMockClock()
 

server/fleet/service.go

@@ -350,8 +350,8 @@ type Service interface {
 	AddHostsToTeam(ctx context.Context, teamID *uint, hostIDs []uint, skipBulkPending bool) error
 	// AddHostsToTeamByFilter adds hosts to an existing team, clearing their team settings if teamID is nil. Hosts are
 	// selected by the label and HostListOptions provided.
-	AddHostsToTeamByFilter(ctx context.Context, teamID *uint, opt HostListOptions, lid *uint) error
-	DeleteHosts(ctx context.Context, ids []uint, opt *HostListOptions, lid *uint) error
+	AddHostsToTeamByFilter(ctx context.Context, teamID *uint, filter *map[string]interface{}) error
+	DeleteHosts(ctx context.Context, ids []uint, filters *map[string]interface{}) error
 	CountHosts(ctx context.Context, labelID *uint, opts HostListOptions) (int, error)
 	// SearchHosts performs a search on the hosts table using the following criteria:
 	//	- matchQuery is the query SQL

server/service/client_hosts.go

@@ -114,21 +114,27 @@ func (c *Client) TransferHosts(hosts []string, label string, status, searchQuery
 		return c.authenticatedRequest(params, verb, path, &responseBody)
 	}
 
-	var labelIDPtr *uint
+	filter := make(map[string]interface{})
+
 	if label != "" {
-		labelIDPtr = &labelID
+		filter["label_id"] = labelID
+	}
+
+	if status != "" {
+		filter["status"] = fleet.HostStatus(status)
+	}
+
+	if searchQuery != "" {
+		filter["query"] = searchQuery
 	}
 
 	verb, path := "POST", "/api/latest/fleet/hosts/transfer/filter"
 	var responseBody addHostsToTeamByFilterResponse
 	params := addHostsToTeamByFilterRequest{
-		TeamID: teamIDPtr, Filters: struct {
-			MatchQuery string           `json:"query"`
-			Status     fleet.HostStatus `json:"status"`
-			LabelID    *uint            `json:"label_id"`
-			TeamID     *uint            `json:"team_id"`
-		}{MatchQuery: searchQuery, Status: fleet.HostStatus(status), LabelID: labelIDPtr},
+		TeamID:  teamIDPtr,
+		Filters: &filter,
 	}
+
 	return c.authenticatedRequest(params, verb, path, &responseBody)
 }
 

server/service/hosts.go

@@ -217,17 +217,10 @@ var (
 	deleteHostsSkipAuthorization = false
 )
 
-type deleteHostsFilters struct {
-	MatchQuery string           `json:"query"`
-	Status     fleet.HostStatus `json:"status"`
-	LabelID    *uint            `json:"label_id"`
-	TeamID     *uint            `json:"team_id"`
-}
-
 type deleteHostsRequest struct {
 	IDs []uint `json:"ids"`
 	// Using a pointer to help determine whether an empty filter was passed, like: "filters":{}
-	Filters *deleteHostsFilters `json:"filters"`
+	Filters *map[string]interface{} `json:"filters"`
 }
 
 type deleteHostsResponse struct {
@@ -242,18 +235,6 @@ func (r deleteHostsResponse) Status() int { return r.StatusCode }
 
 func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) {
 	req := request.(*deleteHostsRequest)
-	var listOpts *fleet.HostListOptions
-	var labelID *uint
-	if req.Filters != nil {
-		listOpts = &fleet.HostListOptions{
-			ListOptions: fleet.ListOptions{
-				MatchQuery: req.Filters.MatchQuery,
-			},
-			StatusFilter: req.Filters.Status,
-			TeamFilter:   req.Filters.TeamID,
-		}
-		labelID = req.Filters.LabelID
-	}
 
 	// Since bulk deletes can take a long time, after DeleteHostsTimeout, we will return a 202 (Accepted) status code
 	// and allow the delete operation to proceed.
@@ -261,7 +242,7 @@ func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Ser
 	deleteDone := make(chan bool, 1)
 	ctx = context.WithoutCancel(ctx) // to make sure DB operations don't get killed after we return a 202
 	go func() {
-		err = svc.DeleteHosts(ctx, req.IDs, listOpts, labelID)
+		err = svc.DeleteHosts(ctx, req.IDs, req.Filters)
 		if err != nil {
 			// logging the error for future debug in case we already sent http.StatusAccepted
 			logging.WithErr(ctx, err)
@@ -283,11 +264,16 @@ func deleteHostsEndpoint(ctx context.Context, request interface{}, svc fleet.Ser
 	}
 }
 
-func (svc *Service) DeleteHosts(ctx context.Context, ids []uint, opts *fleet.HostListOptions, lid *uint) error {
+func (svc *Service) DeleteHosts(ctx context.Context, ids []uint, filter *map[string]interface{}) error {
 	if err := svc.authz.Authorize(ctx, &fleet.Host{}, fleet.ActionList); err != nil {
 		return err
 	}
 
+	opts, lid, err := hostListOptionsFromFilters(filter)
+	if err != nil {
+		return err
+	}
+
 	if len(ids) == 0 && lid == nil && opts == nil {
 		return &fleet.BadRequestError{Message: "list of ids or filters must be specified"}
 	}
@@ -862,13 +848,8 @@ func (svc *Service) createTransferredHostsActivity(ctx context.Context, teamID *
 ////////////////////////////////////////////////////////////////////////////////
 
 type addHostsToTeamByFilterRequest struct {
-	TeamID  *uint `json:"team_id"`
-	Filters struct {
-		MatchQuery string           `json:"query"`
-		Status     fleet.HostStatus `json:"status"`
-		LabelID    *uint            `json:"label_id"`
-		TeamID     *uint            `json:"team_id"`
-	} `json:"filters"`
+	TeamID  *uint                   `json:"team_id"`
+	Filters *map[string]interface{} `json:"filters"`
 }
 
 type addHostsToTeamByFilterResponse struct {
@@ -879,14 +860,7 @@ func (r addHostsToTeamByFilterResponse) error() error { return r.Err }
 
 func addHostsToTeamByFilterEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) {
 	req := request.(*addHostsToTeamByFilterRequest)
-	listOpt := fleet.HostListOptions{
-		ListOptions: fleet.ListOptions{
-			MatchQuery: req.Filters.MatchQuery,
-		},
-		StatusFilter: req.Filters.Status,
-		TeamFilter:   req.Filters.TeamID,
-	}
-	err := svc.AddHostsToTeamByFilter(ctx, req.TeamID, listOpt, req.Filters.LabelID)
+	err := svc.AddHostsToTeamByFilter(ctx, req.TeamID, req.Filters)
 	if err != nil {
 		return addHostsToTeamByFilterResponse{Err: err}, nil
 	}
@@ -894,7 +868,7 @@ func addHostsToTeamByFilterEndpoint(ctx context.Context, request interface{}, sv
 	return addHostsToTeamByFilterResponse{}, err
 }
 
-func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, opt fleet.HostListOptions, lid *uint) error {
+func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, filters *map[string]interface{}) error {
 	// This is currently treated as a "team write". If we ever give users
 	// besides global admins permissions to modify team hosts, we will need to
 	// check that the user has permissions for both the source and destination
@@ -903,7 +877,16 @@ func (svc *Service) AddHostsToTeamByFilter(ctx context.Context, teamID *uint, op
 		return err
 	}
 
-	hostIDs, hostNames, err := svc.hostIDsAndNamesFromFilters(ctx, opt, lid)
+	opt, lid, err := hostListOptionsFromFilters(filters)
+	if err != nil {
+		return err
+	}
+
+	if opt == nil {
+		return &fleet.BadRequestError{Message: "filters must be specified"}
+	}
+
+	hostIDs, hostNames, err := svc.hostIDsAndNamesFromFilters(ctx, *opt, lid)
 	if err != nil {
 		return err
 	}
@@ -2160,3 +2143,55 @@ func (svc *Service) HostLiteByID(ctx context.Context, id uint) (*fleet.HostLite,
 
 	return host, nil
 }
+
+func hostListOptionsFromFilters(filter *map[string]interface{}) (*fleet.HostListOptions, *uint, error) {
+	var labelID *uint
+
+	if filter == nil {
+		return nil, nil, nil
+	}
+
+	opt := fleet.HostListOptions{}
+
+	for k, v := range *filter {
+		switch k {
+		case "label_id":
+			if l, ok := v.(float64); ok { // json unmarshals numbers as float64
+				lid := uint(l)
+				labelID = &lid
+			} else {
+				return nil, nil, badRequest("label_id must be a number")
+			}
+		case "team_id":
+			if teamID, ok := v.(float64); ok { // json unmarshals numbers as float64
+				teamID := uint(teamID)
+				opt.TeamFilter = &teamID
+			} else {
+				return nil, nil, badRequest("team_id must be a number")
+			}
+		case "status":
+			status, ok := v.(string)
+			if !ok {
+				return nil, nil, badRequest("status must be a string")
+			}
+			if !fleet.HostStatus(status).IsValid() {
+				return nil, nil, badRequest("status must be one of: new, online, offline, missing")
+			}
+			opt.StatusFilter = fleet.HostStatus(status)
+		case "query":
+			query, ok := v.(string)
+			if !ok {
+				return nil, nil, badRequest("query must be a string")
+			}
+			if query == "" {
+				return nil, nil, badRequest("query must not be empty")
+			}
+			opt.MatchQuery = query
+
+		default:
+			return nil, nil, badRequest(fmt.Sprintf("unknown filter key: %s", k))
+		}
+	}
+
+	return &opt, labelID, nil
+}

server/service/hosts_test.go

@@ -723,16 +723,17 @@ func TestHostAuth(t *testing.T) {
 			err = svc.DeleteHost(ctx, 2)
 			checkAuthErr(t, tt.shouldFailGlobalWrite, err)
 
-			err = svc.DeleteHosts(ctx, []uint{1}, nil, nil)
+			err = svc.DeleteHosts(ctx, []uint{1}, nil)
 			checkAuthErr(t, tt.shouldFailTeamWrite, err)
 
-			err = svc.DeleteHosts(ctx, []uint{2}, &fleet.HostListOptions{}, nil)
+			err = svc.DeleteHosts(ctx, []uint{2}, nil)
 			checkAuthErr(t, tt.shouldFailGlobalWrite, err)
 
 			err = svc.AddHostsToTeam(ctx, ptr.Uint(1), []uint{1}, false)
 			checkAuthErr(t, tt.shouldFailTeamWrite, err)
 
-			err = svc.AddHostsToTeamByFilter(ctx, ptr.Uint(1), fleet.HostListOptions{}, nil)
+			emptyFilter := make(map[string]interface{})
+			err = svc.AddHostsToTeamByFilter(ctx, ptr.Uint(1), &emptyFilter)
 			checkAuthErr(t, tt.shouldFailTeamWrite, err)
 
 			err = svc.RefetchHost(ctx, 1)
@@ -855,7 +856,9 @@ func TestAddHostsToTeamByFilter(t *testing.T) {
 		return nil
 	}
 
-	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, fleet.HostListOptions{}, nil))
+	emptyRequest := &map[string]interface{}{}
+
+	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, emptyRequest))
 	assert.True(t, ds.ListHostsFuncInvoked)
 	assert.True(t, ds.AddHostsToTeamFuncInvoked)
 }
@@ -866,10 +869,10 @@ func TestAddHostsToTeamByFilterLabel(t *testing.T) {
 
 	expectedHostIDs := []uint{6}
 	expectedTeam := ptr.Uint(1)
-	expectedLabel := ptr.Uint(2)
+	expectedLabel := float64(2)
 
 	ds.ListHostsInLabelFunc = func(ctx context.Context, filter fleet.TeamFilter, lid uint, opt fleet.HostListOptions) ([]*fleet.Host, error) {
-		assert.Equal(t, *expectedLabel, lid)
+		assert.Equal(t, uint(expectedLabel), lid)
 		var hosts []*fleet.Host
 		for _, id := range expectedHostIDs {
 			hosts = append(hosts, &fleet.Host{ID: id})
@@ -893,7 +896,9 @@ func TestAddHostsToTeamByFilterLabel(t *testing.T) {
 		return nil
 	}
 
-	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, fleet.HostListOptions{}, expectedLabel))
+	filter := &map[string]interface{}{"label_id": expectedLabel}
+
+	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), expectedTeam, filter))
 	assert.True(t, ds.ListHostsInLabelFuncInvoked)
 	assert.True(t, ds.AddHostsToTeamFuncInvoked)
 }
@@ -912,7 +917,9 @@ func TestAddHostsToTeamByFilterEmptyHosts(t *testing.T) {
 		return nil
 	}
 
-	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), nil, fleet.HostListOptions{}, nil))
+	emptyFilter := &map[string]interface{}{}
+
+	require.NoError(t, svc.AddHostsToTeamByFilter(test.UserContext(ctx, test.UserAdmin), nil, emptyFilter))
 	assert.True(t, ds.ListHostsFuncInvoked)
 	assert.False(t, ds.AddHostsToTeamFuncInvoked)
 }
@@ -1628,3 +1635,159 @@ func TestLockUnlockWipeHostAuth(t *testing.T) {
 		})
 	}
 }
+
+func TestBulkOperationFilterValidation(t *testing.T) {
+	ds := new(mock.Store)
+	svc, ctx := newTestService(t, ds, nil, nil)
+	viewerCtx := test.UserContext(ctx, test.UserAdmin)
+
+	ds.ListHostsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.HostListOptions) ([]*fleet.Host, error) {
+		return []*fleet.Host{}, nil
+	}
+
+	ds.ListHostsInLabelFunc = func(ctx context.Context, filter fleet.TeamFilter, lid uint, opt fleet.HostListOptions) ([]*fleet.Host, error) {
+		return []*fleet.Host{}, nil
+	}
+
+	tc := []struct {
+		name      string
+		filters   *map[string]interface{}
+		has400Err bool
+	}{
+		{
+			name: "valid status filter",
+			filters: &map[string]interface{}{
+				"status": "new",
+			},
+		},
+		{
+			name: "invalid status",
+			filters: &map[string]interface{}{
+				"status": "invalid",
+			},
+			has400Err: true,
+		},
+		{
+			name: "empty status is invalid",
+			filters: &map[string]interface{}{
+				"status": "",
+			},
+			has400Err: true,
+		},
+
+		{
+			name: "valid team filter",
+			filters: &map[string]interface{}{
+				"team_id": float64(1), // json unmarshals to float64
+			},
+		},
+		{
+			name: "invalid team_id type",
+			filters: &map[string]interface{}{
+				"team_id": "invalid",
+			},
+			has400Err: true,
+		},
+		{
+			name: "valid label_id filter",
+			filters: &map[string]interface{}{
+				"label_id": float64(1),
+			},
+		},
+		{
+			name: "invalid label_id type",
+			filters: &map[string]interface{}{
+				"label_id": "invalid",
+			},
+			has400Err: true,
+		},
+
+		{
+			name: "invalid status type",
+			filters: &map[string]interface{}{
+				"status": float64(1),
+			},
+			has400Err: true,
+		},
+		{
+			name:    "empty filter",
+			filters: &map[string]interface{}{},
+		},
+		{
+			name: "valid query filter",
+			filters: &map[string]interface{}{
+				"query": "test",
+			},
+		},
+		{
+			name: "invalid query type",
+			filters: &map[string]interface{}{
+				"query": float64(1),
+			},
+			has400Err: true,
+		},
+		{
+			name: "empty query is invalid",
+			filters: &map[string]interface{}{
+				"query": "",
+			},
+			has400Err: true,
+		},
+		{
+			name: "multiple valid filters",
+			filters: &map[string]interface{}{
+				"status":  "new",
+				"team_id": float64(1),
+				"query":   "test",
+			},
+		},
+		{
+			name: "mixed valid and invalid filters",
+			filters: &map[string]interface{}{
+				"status":  "new",
+				"team_id": "invalid",
+			},
+			has400Err: true,
+		},
+		{
+			name: "mixed invalid filters and valid filters (different order)",
+			filters: &map[string]interface{}{
+				"status":  "invalid",
+				"team_id": 1,
+			},
+			has400Err: true,
+		},
+		{
+			name: "mixed valid and unknown filters",
+			filters: &map[string]interface{}{
+				"status":  "new",
+				"unknown": "filter",
+			},
+			has400Err: true,
+		},
+		{
+			name: "unknown filter",
+			filters: &map[string]interface{}{
+				"unknown": "filter",
+			},
+			has400Err: true,
+		},
+	}
+
+	checkErr := func(t *testing.T, err error, has400Err bool) {
+		if has400Err {
+			require.Error(t, err)
+			var be *fleet.BadRequestError
+			require.ErrorAs(t, err, &be)
+		} else {
+			require.NoError(t, err)
+		}
+	}
+
+	for _, tt := range tc {
+		t.Run(tt.name, func(t *testing.T) {
+			checkErr(t, svc.AddHostsToTeamByFilter(viewerCtx, nil, tt.filters), tt.has400Err)
+			checkErr(t, svc.DeleteHosts(viewerCtx, nil, tt.filters), tt.has400Err)
+		})
+	}
+}

server/service/integration_core_test.go

@@ -1000,7 +1000,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsFromTeam() {
 	require.NoError(t, s.ds.AddHostsToTeam(context.Background(), &team1.ID, []uint{hosts[0].ID}))
 
 	req := deleteHostsRequest{
-		Filters: &deleteHostsFilters{TeamID: ptr.Uint(team1.ID)},
+		Filters: &map[string]interface{}{"team_id": float64(team1.ID)},
 	}
 	resp := deleteHostsResponse{}
 	s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp)
@@ -1037,7 +1037,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsInLabel() {
 	require.NoError(t, s.ds.RecordLabelQueryExecutions(context.Background(), hosts[2], map[uint]*bool{label.ID: ptr.Bool(true)}, time.Now(), false))
 
 	req := deleteHostsRequest{
-		Filters: &deleteHostsFilters{LabelID: ptr.Uint(label.ID)},
+		Filters: &map[string]interface{}{"label_id": float64(label.ID)},
 	}
 	resp := deleteHostsResponse{}
 	s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp)
@@ -1120,7 +1120,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsAll() {
 
 	// All hosts should be deleted when an empty filter is specified
 	req := deleteHostsRequest{
-		Filters: &deleteHostsFilters{},
+		Filters: &map[string]interface{}{},
 	}
 	resp := deleteHostsResponse{}
 	s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusOK, &resp)
@@ -1163,7 +1163,7 @@ func (s *integrationTestSuite) TestBulkDeleteHostsErrors() {
 
 	req := deleteHostsRequest{
 		IDs:     []uint{hosts[0].ID, hosts[1].ID},
-		Filters: &deleteHostsFilters{LabelID: ptr.Uint(1)},
+		Filters: &map[string]interface{}{"label_id": float64(1)},
 	}
 	resp := deleteHostsResponse{}
 	s.DoJSON("POST", "/api/latest/fleet/hosts/delete", req, http.StatusBadRequest, &resp)
@@ -2814,8 +2814,11 @@ func (s *integrationTestSuite) TestHostsAddToTeam() {
 
 	// assign host to team 2 with filter
 	var addfResp addHostsToTeamByFilterResponse
-	req := addHostsToTeamByFilterRequest{TeamID: &tm2.ID}
-	req.Filters.MatchQuery = hosts[2].Hostname
+	req := addHostsToTeamByFilterRequest{
+		TeamID:  &tm2.ID,
+		Filters: &map[string]interface{}{"query": hosts[2].Hostname},
+	}
+
 	s.DoJSON("POST", "/api/latest/fleet/hosts/transfer/filter", req, http.StatusOK, &addfResp)
 	s.lastActivityOfTypeMatches(
 		fleet.ActivityTypeTransferredHostsToTeam{}.ActivityName(),