Validate activities order_key against an allowlist

[prateek-0490]

Related issue: N/A

Description

The activities list endpoints (GET /api/v1/fleet/activities and GET /api/v1/fleet/hosts/{id}/activities) applied the order_key parameter through the deprecated AppendListOptionsWithParams helper, which only regex-filters the column name and does not check it against an allowlist. This let a caller sort by (and cursor-paginate over) arbitrary columns, including ones not in the SELECT.

This routes both queries through AppendListOptionsWithParamsSecure with a per-query order-key allowlist, so only known sort keys are accepted and anything else returns a 422. It mirrors how the other list endpoints (hosts, labels, software, certificates, MDM commands) already validate their sort keys. This was the last remaining caller of the deprecated helper.

Checklist for submitter

• Changes file added for user-visible changes in changes/.
• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements) — the sort key is validated against an allowlist and mapped to a fixed column expression; all values remain parameterized.

Testing

• Added/updated automated tests — per-query negative tests assert off-allowlist keys are rejected on both endpoints; existing ordering/cursor tests confirm legitimate id/created_at sorting and pagination still work.
• QA'd all new/changed functionality manually — verified on a local instance: order_key=a.details now returns a 422 invalid order_key, while id/created_at/the default still return 200 with correct ordering.

cc @lukeheath for review.

Summary by CodeRabbit

• Bug Fixes
  • Improved validation for activity sorting parameters.
  • Unsupported sort fields are now rejected for fleet and host activity requests.
  • Fixed host activity sorting behavior for the created_at field.
  • Prevented invalid sort inputs from affecting activity query processing.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 79d24126-8697-491d-a922-f9e0c88f8ba1

📥 Commits

Reviewing files that changed from the base of the PR and between 9a729da and 278f969.

⛔ Files ignored due to path filters (1)

• docs/REST API/rest-api.md is excluded by !**/*.md

📒 Files selected for processing (5)

• server/service/integration_apple_vpp_config_test.go
• server/service/integration_core_test.go
• server/service/integration_enterprise_test.go
• server/service/integration_mdm_test.go
• server/service/testing_client_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

The fleet and host activity endpoints now validate order_key values against approved allowlists. MySQL list methods map valid keys to SQL expressions and reject invalid pagination options before query execution. Host activity ordering now uses the user-facing created_at key. Tests cover invalid keys and updated activity API requests use id.

Possibly related PRs

• fleetdm/fleet#49624: Modifies the same activity ordering allowlists, pagination handling, service default, and validation tests.

Merge Risk: ⚪ Minimal · up to 278f9

The activities endpoints now reject unsupported sort keys while preserving valid ordering and pagination, and no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly explains the security fix, affected endpoints, testing, manual QA, and user-visible changes.
Title check	✅ Passed	The title clearly and concisely describes the main change: validating activity order_key values against an allowlist.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.45.1)

server/service/integration_enterprise_test.go

ast-grep timed out on this file

server/service/integration_mdm_test.go

ast-grep timed out on this file

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[prateek-0490]

/agentic_review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.94%. Comparing base (6c470e7) to head (278f969).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #51556      +/-   ##
==========================================
+ Coverage   68.92%   68.94%   +0.01%     
==========================================
  Files        4009     4010       +1     
  Lines      259778   260145     +367     
  Branches    13727    13727              
==========================================
+ Hits       179061   179366     +305     
- Misses      64863    64919      +56     
- Partials    15854    15860       +6     

Flag	Coverage Δ
backend	69.94% <50.00%> (+0.02%)	⬆️
backend-activity	83.79% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Qualified ID key rejected 🐞 Bug ≡ Correctness

Description

The new exact allowlist accepts id but rejects the previously supported order_key=a.id, causing
existing global activity requests to return 422 instead of results. Repository integration tests and
shared testing helpers still issue a.id while expecting HTTP 200, so these suites will fail.

Code

server/activity/internal/mysql/activity.go[R46-47]

+var activitiesAllowedOrderKeys = platform_mysql.OrderKeyAllowlist{
+	"id":              "a.id",

Evidence

The secure helper performs an exact lookup and returns InvalidOrderKeyError for absent keys, while
the new map contains only id. Multiple checked-in callers still request a.id and assert HTTP
200, including the shared activity test client and a cursor-pagination integration test.

server/platform/mysql/list_options.go[101-115]
server/service/testing_client_test.go[732-761]
server/service/integration_core_test.go[3997-4005]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The activities allowlist rejects the previously accepted `a.id` order key, although existing callers and integration tests depend on it. Preserve this safe legacy alias while retaining exact allowlist validation.
## Issue Context
`AppendListOptionsWithParamsSecure` performs an exact map lookup. Adding `a.id` as an explicit alias remains safe because it maps to a fixed SQL expression; the host activity allowlist should receive equivalent compatibility treatment.
## Fix Focus Areas
- server/activity/internal/mysql/activity.go[46-67]
- server/activity/internal/mysql/activity_test.go[253-320]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Sorting contract remains outdated ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The endpoint now accepts only allowlisted sort keys, but its public REST documentation still
promises that order_key can be any activities-table column. API consumers following that contract
will unexpectedly receive 422 responses without documentation identifying the supported keys.

Code

server/activity/internal/mysql/activity.go[R131-132]

+	// Apply pagination, validating the order key against an allowlist.
+	activitiesQ, args, err := platform_mysql.AppendListOptionsWithParamsSecure(activitiesQ, args, &opt, activitiesAllowedOrderKeys)

Evidence

The changed datastore call enforces the new allowlist, whereas the published parameter description
explicitly says any activities-table column is accepted.

server/activity/internal/mysql/activity.go[42-55]
server/platform/mysql/list_options.go[87-115]
docs/REST API/rest-api.md[503-521]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Update the activities endpoint documentation to reflect that `order_key` is restricted to an explicit set rather than accepting any table column.
## Issue Context
The datastore now rejects every key outside its allowlist, while the REST API reference retains the old arbitrary-column contract. Document the accepted user-facing keys and corresponding 422 behavior for both affected endpoints where applicable.
## Fix Focus Areas
- docs/REST API/rest-api.md[503-521]
- server/activity/internal/mysql/activity.go[42-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[juan-fdz-hawa]

Could you also please remove the AppendListOptionsWithParams helper? I think this end-point was the last one using it.