From 62b60fef240dc99b4b74549160b4ebcbb066f899 Mon Sep 17 00:00:00 2001 From: Jordan Montgomery Date: Wed, 29 Apr 2026 16:08:49 -0400 Subject: [PATCH] Improve filtering on commands endpoints (#44426) Provides better errors on invalid/unexpected sort keys passed to `/api/v1/fleet/commands`, `/api/v1/fleet/mdm/commands` and `/api/v1/fleet/mdm/apple/commands` endpoints # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Improved validation for invalid `order_key` values on MDM command endpoints (`/api/v1/fleet/commands`, `/api/v1/fleet/mdm/commands`, and `/api/v1/fleet/mdm/apple/commands`), ensuring only approved sorting parameters are accepted. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- changes/fix-mdm-commands-filtering | 1 + server/datastore/mysql/apple_mdm.go | 15 ++- server/datastore/mysql/mdm.go | 21 +++- server/datastore/mysql/mdm_test.go | 155 ++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 changes/fix-mdm-commands-filtering diff --git a/changes/fix-mdm-commands-filtering b/changes/fix-mdm-commands-filtering new file mode 100644 index 0000000000..ec0d2d9ff1 --- /dev/null +++ b/changes/fix-mdm-commands-filtering @@ -0,0 +1 @@ +* Improved validation for invalid `order_key` values in `/api/v1/fleet/commands`, `/api/v1/fleet/mdm/commands` and `/api/v1/fleet/mdm/apple/commands` endpoints. diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index 2e3747a33b..991e02d0fb 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -1117,6 +1117,16 @@ WHERE return results, nil } +var mdmAppleCommandsAllowedOrderKeys = common_mysql.OrderKeyAllowlist{ + "command_uuid": "nvq.command_uuid", + "request_type": "nvq.request_type", + "status": "COALESCE(NULLIF(nvq.status, ''), 'Pending')", + "updated_at": "COALESCE(nvq.result_updated_at, nvq.created_at)", + "hostname": "h.hostname", + "device_id": "ne.device_id", + "name": "nvq.name", +} + func (ds *Datastore) ListMDMAppleCommands( ctx context.Context, tmFilter fleet.TeamFilter, @@ -1148,7 +1158,10 @@ WHERE nvq.active = 1 AND %s `, ds.whereFilterHostsByTeams(tmFilter, "h")) - stmt, params := appendListOptionsWithCursorToSQL(stmt, nil, &listOpts.ListOptions) + stmt, params, err := appendListOptionsWithCursorToSQLSecure(stmt, nil, &listOpts.ListOptions, mdmAppleCommandsAllowedOrderKeys) + if err != nil { + return nil, ctxerr.Wrap(ctx, err, "list commands") + } var results []*fleet.MDMAppleCommand if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, stmt, params...); err != nil { diff --git a/server/datastore/mysql/mdm.go b/server/datastore/mysql/mdm.go index e78438612c..333284f7ae 100644 --- a/server/datastore/mysql/mdm.go +++ b/server/datastore/mysql/mdm.go @@ -14,10 +14,21 @@ import ( "github.com/fleetdm/fleet/v4/server/mdm" "github.com/fleetdm/fleet/v4/server/mdm/apple/mobileconfig" microsoft_mdm "github.com/fleetdm/fleet/v4/server/mdm/microsoft" + common_mysql "github.com/fleetdm/fleet/v4/server/platform/mysql" "github.com/google/go-cmp/cmp" "github.com/jmoiron/sqlx" ) +var mdmCommandsAllowedOrderKeys = common_mysql.OrderKeyAllowlist{ + "command_uuid": "command_uuid", + "request_type": "request_type", + "status": "status", + "updated_at": "updated_at", + "hostname": "hostname", + "host_uuid": "host_uuid", + "name": "name", +} + func (ds *Datastore) GetMDMCommandPlatform(ctx context.Context, commandUUID string) (string, error) { stmt := ` SELECT CASE @@ -103,7 +114,10 @@ func (ds *Datastore) ListMDMCommands( jointStmt, params := getCombinedMDMCommandsQuery(ds, listOpts.Filters.HostIdentifier) jointStmt += ds.whereFilterHostsByTeams(tmFilter, "combined_commands") jointStmt, params = addRequestTypeFilter(jointStmt, &listOpts.Filters, params) - jointStmt, params = appendListOptionsWithCursorToSQL(jointStmt, params, &listOpts.ListOptions) + jointStmt, params, err := appendListOptionsWithCursorToSQLSecure(jointStmt, params, &listOpts.ListOptions, mdmCommandsAllowedOrderKeys) + if err != nil { + return nil, nil, nil, ctxerr.Wrap(ctx, err, "list commands") + } var results []*fleet.MDMCommand if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, jointStmt, params...); err != nil { return nil, nil, nil, ctxerr.Wrap(ctx, err, "list commands") @@ -357,7 +371,10 @@ WHERE if listOpts.PerPage == 0 { listOpts.PerPage = 10 } - listStmt, params = appendListOptionsWithCursorToSQL(listStmt, params, &listOpts.ListOptions) + listStmt, params, err = appendListOptionsWithCursorToSQLSecure(listStmt, params, &listOpts.ListOptions, mdmCommandsAllowedOrderKeys) + if err != nil { + return nil, nil, nil, ctxerr.Wrap(ctx, err, "list commands") + } var results []*fleet.MDMCommand if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, listStmt, params...); err != nil { diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index 6acfc3982a..35ab1ed255 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -37,6 +37,8 @@ func TestMDMShared(t *testing.T) { }{ {"TestMDMCommands", testMDMCommands}, {"TestListMDMCommandsWithTeamFilter", testListMDMCommandsWithTeamFilter}, + {"TestListMDMCommandsOrderKeys", testListMDMCommandsOrderKeys}, + {"TestListMDMAppleCommandsOrderKeys", testListMDMAppleCommandsOrderKeys}, {"TestBatchSetMDMProfiles", testBatchSetMDMProfiles}, {"TestListMDMConfigProfiles", testListMDMConfigProfiles}, {"TestBulkSetPendingMDMHostProfiles", testBulkSetPendingMDMHostProfiles}, @@ -586,6 +588,159 @@ func testListMDMCommandsWithTeamFilter(t *testing.T, ds *Datastore) { require.ElementsMatch(t, []string{teamCmdUUID, globalCmdUUID}, got) } +func testListMDMCommandsOrderKeys(t *testing.T, ds *Datastore) { + ctx := t.Context() + + macH, err := ds.NewHost(ctx, &fleet.Host{ + Hostname: "ord-host", + OsqueryHostID: ptr.String("ord-osq"), + NodeKey: ptr.String("ord-nk"), + UUID: uuid.NewString(), + Platform: "darwin", + HardwareSerial: "ORDABC", + }) + require.NoError(t, err) + nanoEnroll(t, ds, macH, false) + + commander, _ := createMDMAppleCommanderAndStorage(t, ds) + for range 3 { + err = commander.EnqueueCommand(ctx, []string{macH.UUID}, createRawAppleCmd("ProfileList", uuid.NewString())) + require.NoError(t, err) + } + + for _, key := range []string{"command_uuid", "request_type", "status", "updated_at", "hostname", "host_uuid", "name"} { + t.Run("order_"+key, func(t *testing.T) { + cmds, _, _, err := ds.ListMDMCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: key, PerPage: 5}, + }, + ) + require.NoError(t, err) + require.Len(t, cmds, 3) + }) + } + + t.Run("rejects_unknown_key", func(t *testing.T) { + _, _, _, err := ds.ListMDMCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "not_a_real_column"}, + }, + ) + require.Error(t, err) + }) + + // the host-identifier branch uses a separate query; confirm it shares the allowlist + t.Run("rejects_unknown_key_host_identifier", func(t *testing.T) { + _, _, _, err := ds.ListMDMCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "not_a_real_column"}, + Filters: fleet.MDMCommandFilters{HostIdentifier: macH.UUID}, + }, + ) + require.Error(t, err) + }) + + t.Run("after_pagination_with_allowed_key", func(t *testing.T) { + cmds, _, _, err := ds.ListMDMCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "command_uuid", PerPage: 1}, + }, + ) + require.NoError(t, err) + require.Len(t, cmds, 1) + afterCursor := cmds[0].CommandUUID + + next, _, _, err := ds.ListMDMCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "command_uuid", PerPage: 1, After: afterCursor}, + }, + ) + require.NoError(t, err) + require.Len(t, next, 1) + require.NotEqual(t, afterCursor, next[0].CommandUUID) + }) +} + +func testListMDMAppleCommandsOrderKeys(t *testing.T, ds *Datastore) { + ctx := t.Context() + + macH, err := ds.NewHost(ctx, &fleet.Host{ + Hostname: "ord-apple-host", + OsqueryHostID: ptr.String("ord-apple-osq"), + NodeKey: ptr.String("ord-apple-nk"), + UUID: uuid.NewString(), + Platform: "darwin", + HardwareSerial: "ORDA1", + }) + require.NoError(t, err) + nanoEnroll(t, ds, macH, false) + + commander, _ := createMDMAppleCommanderAndStorage(t, ds) + for range 2 { + err = commander.EnqueueCommand(ctx, []string{macH.UUID}, createRawAppleCmd("ProfileList", uuid.NewString())) + require.NoError(t, err) + } + + for _, key := range []string{"command_uuid", "request_type", "status", "updated_at", "hostname", "device_id"} { + t.Run("order_"+key, func(t *testing.T) { + cmds, err := ds.ListMDMAppleCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: key, PerPage: 5}, + }, + ) + require.NoError(t, err) + require.Len(t, cmds, 2) + }) + } + + t.Run("rejects_unknown_key", func(t *testing.T) { + _, err := ds.ListMDMAppleCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "not_a_real_column"}, + }, + ) + require.Error(t, err) + }) + + t.Run("after_pagination_with_allowed_key", func(t *testing.T) { + cmds, err := ds.ListMDMAppleCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "command_uuid", PerPage: 1}, + }, + ) + require.NoError(t, err) + require.Len(t, cmds, 1) + afterCursor := cmds[0].CommandUUID + + next, err := ds.ListMDMAppleCommands( + ctx, + fleet.TeamFilter{User: test.UserAdmin}, + &fleet.MDMCommandListOptions{ + ListOptions: fleet.ListOptions{OrderKey: "command_uuid", PerPage: 1, After: afterCursor}, + }, + ) + require.NoError(t, err) + require.Len(t, next, 1) + require.NotEqual(t, afterCursor, next[0].CommandUUID) + }) +} + func testBatchSetMDMProfiles(t *testing.T, ds *Datastore) { ctx := context.Background()