Enforce ORDER BY allowlist on activity list endpoints - #49624
Closed
lukeheath wants to merge 2 commits into
Closed
Conversation
A global-admin API-only token scoped to an api_endpoints allowlist could still reach every /debug/* route, because the debug handler's middleware only checked for the global-admin role and never consulted the token's endpoint restrictions like the main API path does. Debug routes are not in the public API catalog, so they can never appear in an allowlist. Deny restricted API-only tokens (api_only with a non-empty api_endpoints list) at the debug middleware, matching the least-privilege scoping APIOnlyEndpointCheck enforces elsewhere.
…-2mpw) Migrate ListActivities and ListHostPastActivities in the activity bounded context from the deprecated AppendListOptionsWithParams to the allowlist-based AppendListOptionsWithParamsSecure. These were the two residual call sites left by #44385. Unlisted order keys (including columns not returned by the query, such as details) now return a client error instead of being sorted on.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughActivity listing endpoints now restrict Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49624 +/- ##
==========================================
- Coverage 67.81% 67.03% -0.79%
==========================================
Files 3890 3890
Lines 247635 247641 +6
Branches 12982 12982
==========================================
- Hits 167943 166004 -1939
- Misses 64529 66430 +1901
- Partials 15163 15207 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR tightens sorting validation on the activity list endpoints by enforcing an ORDER BY allowlist (returning 422 for unapproved order_key values), and also prevents restricted API-only tokens (those with a non-empty endpoint allowlist) from accessing /debug/* routes.
Changes:
- Migrate activity datastore list queries to
platform_mysql.AppendListOptionsWithParamsSecurewith explicitOrderKeyAllowlistmappings. - Add datastore + integration tests asserting invalid
order_keyvalues are rejected with422 Unprocessable Entity. - Deny restricted API-only users from debug endpoints and add unit tests for the new auth behavior.
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/debug_handler.go | Denies debug route access for restricted API-only users. |
| server/service/debug_handler_test.go | Adds coverage for restricted vs unrestricted API-only behavior on debug routes. |
| server/activity/internal/tests/integration_test.go | Adds endpoint-level assertions for 422 on invalid order_key. |
| server/activity/internal/service/service.go | Adjusts default order key to match new allowlist keys. |
| server/activity/internal/mysql/activity.go | Introduces and applies order-key allowlists for activity list queries. |
| server/activity/internal/mysql/activity_test.go | Adds datastore-level tests for allowed/rejected order keys. |
| docs/REST API/rest-api.md | (Content excluded) REST API documentation update. |
| changes/16879-debug-api-only-restriction | (Content excluded) Release-note/change entry for debug restriction. |
| changes/15935-activity-order-key-allowlist | (Content excluded) Release-note/change entry for activity order-key allowlist. |
Files excluded by content exclusion policy (3)
- changes/15935-activity-order-key-allowlist
- changes/16879-debug-api-only-restriction
- docs/REST API/rest-api.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+53
| if v.User.APIOnly && len(v.User.APIEndpoints) > 0 { | ||
| http.Error(w, "Unauthorized", http.StatusForbidden) | ||
| return |
Comment on lines
+115
to
+119
| ).Return(&fleet.User{ | ||
| GlobalRole: ptr.String(fleet.RoleAdmin), | ||
| APIOnly: true, | ||
| APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, | ||
| }, nil) |
Comment on lines
+135
to
+137
| "admin session": {GlobalRole: ptr.String(fleet.RoleAdmin)}, | ||
| "unrestricted api-only": {GlobalRole: ptr.String(fleet.RoleAdmin), APIOnly: true}, | ||
| "api-only empty allowlist": {GlobalRole: ptr.String(fleet.RoleAdmin), APIOnly: true, APIEndpoints: []fleet.APIEndpointRef{}}, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Addresses GHSA-rxhg-vcww-2mpw (residual call sites left by #44385)
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).What & why
#44385migrated the deprecatedappendListOptionsWithCursorToSQL(which sanitizes but does not allowlist theorder_key) to the allowlist-based secure variant across the codebase. Two call sites were left behind in the activity bounded context after it was refactored to useplatform_mysql.AppendListOptionsWithParams:ListActivities— powersGET /api/v1/fleet/activitiesListHostPastActivities— powersGET /api/v1/fleet/hosts/{id}/activitiesBoth allowed sorting by any column name (the value was backtick-sanitized, so it was not SQL-injectable, but there was no allowlist). This migrates them to
AppendListOptionsWithParamsSecurewith an explicitOrderKeyAllowlist:id,created_at,activity_type,user_id,user_name,user_email,streamed,fleet_initiated(all columns already returned by the query).id,created_at,activity_type(matches the documented allowed fields).Columns not returned by the query (
details,host_only) are intentionally excluded soorder_keycannot be used as an inference oracle. An unlisted key now returns422 Unprocessable Entityinstead of being sorted on.Testing
Added/updated automated tests
details,host_only,user_emailfor host activities, etc.) returnInvalidOrderKeyError, and documented keys still succeed.422for an unlistedorder_key.QA'd via automated tests (
MYSQL_TEST=1 REDIS_TEST=1 go test ./server/activity/...).Database migrations
N/A — no schema changes.
New Fleet configuration settings
N/A.
fleetd/orbit/Fleet Desktop
N/A.
Summary by CodeRabbit
order_keyvalues now return a client validation error.