Improve filtering on commands endpoints - #44426
Conversation
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
WalkthroughThis pull request adds validation for the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves the /api/v1/fleet/commands, /api/v1/fleet/mdm/commands, and /api/v1/fleet/mdm/apple/commands listing behavior by validating order_key values against explicit allowlists so invalid sort keys return clearer client-facing errors instead of falling through to unexpected SQL behavior.
Changes:
- Add explicit
order_keyallowlists for MDM commands and Apple MDM commands list queries. - Switch affected MySQL datastore list queries to
appendListOptionsWithCursorToSQLSecureto enforce allowlisted sorting keys. - Add datastore tests covering allowed and rejected
order_keyvalues (including cursor pagination).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/datastore/mysql/mdm.go | Introduces allowlisted order keys and enforces them in MDM commands listing queries. |
| server/datastore/mysql/apple_mdm.go | Adds allowlisted order keys and enforces them in Apple MDM commands listing query. |
| server/datastore/mysql/mdm_test.go | Adds test coverage for allowed/rejected order keys and cursor pagination behavior. |
| changes/fix-mdm-commands-filtering | Adds a user-visible change note for the commands endpoints behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "request_type": "request_type", | ||
| "status": "status", | ||
| "updated_at": "updated_at", | ||
| "hostname": "hostname", |
There was a problem hiding this comment.
This is existing breakage so I am leaving it in.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/datastore/mysql/mdm_test.go (1)
625-634: Assert validation intent, not just generic failure.Both unknown-key tests only check
require.Error(t, err). A non-validation failure would still pass. Consider asserting a stable substring (for example,order_key) so the tests prove the intended contract.Also applies to: 708-717
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm_test.go` around lines 625 - 634, The test for ListMDMCommands currently only asserts a generic error; change the assertion to verify the error is a validation error about the unknown order key by checking the error message contains a stable substring (e.g., "order_key" or "unknown order key"). Update the test case "rejects_unknown_key" (and the similar case around lines 708-717) to call ListMDMCommands with MDMCommandListOptions{ListOptions: fleet.ListOptions{OrderKey: "not_a_real_column"}} and replace require.Error(t, err) with an assertion that err != nil and strings.Contains(err.Error(), "order_key") (or the exact validation token returned by the datastore) so the test asserts the intended validation contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/mdm_test.go`:
- Around line 694-706: The test loop in testListMDMAppleCommandsOrderKeys is
missing the "name" order key that exists in mdmAppleCommandsAllowedOrderKeys;
update the slice of keys in the for loop (currently
[]string{"command_uuid","request_type","status","updated_at","hostname","device_id"})
to include "name" so the test exercises order_key="name" when calling
ListMDMAppleCommands and asserting the results.
In `@server/datastore/mysql/mdm.go`:
- Around line 22-30: The current shared allowlist mdmCommandsAllowedOrderKeys
includes "hostname" but the host-identifier listing query (the host-identifier
listing path) does not project a hostname column, so order_key=hostname can pass
validation then fail at execution; split the allowlist: keep
mdmCommandsAllowedOrderKeys as-is for command-listing queries and add a new
allowlist (e.g., mdmHostIdentifiersAllowedOrderKeys) that omits "hostname", then
update the host-identifier listing code to validate order_key against
mdmHostIdentifiersAllowedOrderKeys instead of mdmCommandsAllowedOrderKeys so
invalid keys are rejected at validation time.
---
Nitpick comments:
In `@server/datastore/mysql/mdm_test.go`:
- Around line 625-634: The test for ListMDMCommands currently only asserts a
generic error; change the assertion to verify the error is a validation error
about the unknown order key by checking the error message contains a stable
substring (e.g., "order_key" or "unknown order key"). Update the test case
"rejects_unknown_key" (and the similar case around lines 708-717) to call
ListMDMCommands with MDMCommandListOptions{ListOptions:
fleet.ListOptions{OrderKey: "not_a_real_column"}} and replace require.Error(t,
err) with an assertion that err != nil and strings.Contains(err.Error(),
"order_key") (or the exact validation token returned by the datastore) so the
test asserts the intended validation contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5156ab2-b329-48aa-b98b-0f0a585eff56
📒 Files selected for processing (4)
changes/fix-mdm-commands-filteringserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.go
| 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) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add missing Apple allowlisted key test (name).
testListMDMAppleCommandsOrderKeys does not exercise order_key=name, even though server/datastore/mysql/apple_mdm.go includes "name" in mdmAppleCommandsAllowedOrderKeys. This leaves a regression gap.
🔧 Proposed patch
- for _, key := range []string{"command_uuid", "request_type", "status", "updated_at", "hostname", "device_id"} {
+ for _, key := range []string{"command_uuid", "request_type", "status", "updated_at", "hostname", "device_id", "name"} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) | |
| }) | |
| } | |
| for _, key := range []string{"command_uuid", "request_type", "status", "updated_at", "hostname", "device_id", "name"} { | |
| 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) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/mdm_test.go` around lines 694 - 706, The test loop in
testListMDMAppleCommandsOrderKeys is missing the "name" order key that exists in
mdmAppleCommandsAllowedOrderKeys; update the slice of keys in the for loop
(currently
[]string{"command_uuid","request_type","status","updated_at","hostname","device_id"})
to include "name" so the test exercises order_key="name" when calling
ListMDMAppleCommands and asserting the results.
| 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", | ||
| } |
There was a problem hiding this comment.
Split order-key allowlists per query path.
"hostname" is allowlisted here, but the host-identifier listing path (used at Line 374) does not project a hostname column. That means order_key=hostname can pass validation and then fail at query execution instead of returning the intended invalid-order-key error.
Proposed fix
-var mdmCommandsAllowedOrderKeys = common_mysql.OrderKeyAllowlist{
+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",
}
+
+var mdmCommandsByHostIdentifierAllowedOrderKeys = common_mysql.OrderKeyAllowlist{
+ "command_uuid": "command_uuid",
+ "request_type": "request_type",
+ "status": "status",
+ "updated_at": "updated_at",
+ "host_uuid": "host_uuid",
+ "name": "name",
+}-listStmt, params, err = appendListOptionsWithCursorToSQLSecure(listStmt, params, &listOpts.ListOptions, mdmCommandsAllowedOrderKeys)
+listStmt, params, err = appendListOptionsWithCursorToSQLSecure(listStmt, params, &listOpts.ListOptions, mdmCommandsByHostIdentifierAllowedOrderKeys)As per coding guidelines, SQL behavior should be precisely scoped to avoid unintended/non-deterministic results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/mdm.go` around lines 22 - 30, The current shared
allowlist mdmCommandsAllowedOrderKeys includes "hostname" but the
host-identifier listing query (the host-identifier listing path) does not
project a hostname column, so order_key=hostname can pass validation then fail
at execution; split the allowlist: keep mdmCommandsAllowedOrderKeys as-is for
command-listing queries and add a new allowlist (e.g.,
mdmHostIdentifiersAllowedOrderKeys) that omits "hostname", then update the
host-identifier listing code to validate order_key against
mdmHostIdentifiersAllowedOrderKeys instead of mdmCommandsAllowedOrderKeys so
invalid keys are rejected at validation time.
There was a problem hiding this comment.
This is pre-existing breakage we'll fix in a followup(this whole endpoint needs some additional validation)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44426 +/- ##
=======================================
Coverage 66.75% 66.76%
=======================================
Files 2633 2633
Lines 211736 211747 +11
Branches 9424 9506 +82
=======================================
+ Hits 141354 141363 +9
- Misses 57540 57544 +4
+ Partials 12842 12840 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> 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 Cherry-pick of #44426 # 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Provides better errors on invalid/unexpected sort keys passed to
/api/v1/fleet/commands,/api/v1/fleet/mdm/commandsand/api/v1/fleet/mdm/apple/commandsendpointsChecklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
order_keyvalues 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.