Enforce API-only endpoint restrictions on chart routes - #49477
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49477 +/- ##
==========================================
- Coverage 67.99% 67.82% -0.17%
==========================================
Files 3768 3815 +47
Lines 239077 247093 +8016
Branches 12613 12613
==========================================
+ Hits 162570 167601 +5031
- Misses 61744 64253 +2509
- Partials 14763 15239 +476
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:
|
… file Adds a test case verifying that API-only users with endpoint restrictions are denied access to chart endpoints not in the API catalog. Also updates the changes file to use past tense.
WalkthroughChart endpoint construction now attaches matched route templates to request context. Chart authentication wraps requests with an API-only endpoint check, and tests verify that restricted chart requests return a 🚥 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 |
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 applies the existing API-only endpoint restriction middleware (APIOnlyEndpointCheck) to the chart bounded-context routes so that API-only users with endpoint allow-lists are restricted consistently (matching the activity bounded context).
Changes:
- Wrap chart bounded-context authentication middleware with
auth.APIOnlyEndpointCheck. - Add an auth middleware unit test case covering a chart route template under API-only restrictions.
- Add a changes entry (content excluded from this review).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/service/middleware/auth/api_only_test.go | Adds a new test case for API-only restrictions against a chart route template. |
| cmd/fleet/serve.go | Wraps chart bounded-context auth middleware with APIOnlyEndpointCheck. |
| changes/fix-chart-api-only-enforcement | Changes entry for user-visible behavior (diff content excluded by policy). |
Files excluded by content exclusion policy (1)
- changes/fix-chart-api-only-enforcement
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add RouteTemplateRequestFunc as a ServerBefore option in chart endpoint_utils.go so APIOnlyEndpointCheck can read the matched mux route template from context (matching the activity pattern). - Update test to include charts in the catalog so it exercises the allow-list rejection path rather than the catalog-check path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/middleware/auth/api_only_test.go (1)
298-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a full-slice expression to prevent potential test pollution.
While
testCatalogEndpointsis currently a slice literal (meaning itslenequals itscap) andappendwill safely allocate a new backing array without mutating the global state, you might consider using a full-slice expression here for consistency withendpoint_utils.go. This defensively guarantees that the global slice won't be mutated if it is ever modified to have spare capacity in the future.♻️ Proposed refactor
- catalogWithCharts := append(testCatalogEndpoints, + catalogWithCharts := append(testCatalogEndpoints[:len(testCatalogEndpoints):len(testCatalogEndpoints)], fleet.NewAPIEndpointFromTpl("GET", "/api/v1/fleet/charts/:metric"), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/middleware/auth/api_only_test.go` around lines 298 - 312, Update the catalog construction in the “api-only user with restrictions, chart endpoint not in allow-list is rejected” test to append to a full-slice view of testCatalogEndpoints with capacity limited to its length, preventing accidental mutation if the shared slice later has spare capacity. Keep the existing endpoint set and apiOnlyEndpointCheck behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/service/middleware/auth/api_only_test.go`:
- Around line 298-312: Update the catalog construction in the “api-only user
with restrictions, chart endpoint not in allow-list is rejected” test to append
to a full-slice view of testCatalogEndpoints with capacity limited to its
length, preventing accidental mutation if the shared slice later has spare
capacity. Keep the existing endpoint set and apiOnlyEndpointCheck behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f34ad6d-811b-4a37-9caa-961ed68ec4ea
📒 Files selected for processing (2)
server/chart/internal/service/endpoint_utils.goserver/service/middleware/auth/api_only_test.go
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
LGTM, some minor comments below.
Move chart endpoint into testCatalogEndpoints and use the standard newEndpoint helper instead of building a custom catalog inline.
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/fix-chart-api-only-enforcement
Checklist 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
Summary
Enforced API-only endpoint restrictions on chart endpoints, matching the pattern already used by the activity bounded context. Also added
RouteTemplateRequestFuncto chart route server options so the middleware can read the matched mux route template from context.Testing
Reproduction
Created an API-only user with a restrictive endpoint allow-list (only
GET /api/v1/fleet/hosts). Confirmed that:/api/latest/fleet/hosts) returns 200/api/latest/fleet/users) returns 403/api/latest/fleet/charts/uptime) returned 200 before the fix (the bug)Unit test
Added a test case in
server/service/middleware/auth/api_only_test.gothat verifies an API-only user with endpoint restrictions is denied access to chart endpoints not in their allow-list. The chart endpoint is included in the test catalog (matching production), so the test exercises the allow-list rejection path.All 17 tests in the auth middleware package pass.
Local verification
cmd/fleet/serve.gopreviously calledauth.AuthenticatedUser(svc, next)withoutAPIOnlyEndpointCheckwrappingauth.APIOnlyEndpointCheck(next)as the correct patternRouteTemplateRequestFunctoserver/chart/internal/service/endpoint_utils.goso the route template is available in context (required byAPIOnlyEndpointCheck)go test ./server/service/middleware/auth/ -vwith all 17 tests passingmake lint-go-incrementalwith 0 issues