Repository navigation
Return an error when refetching an Android host - #51119
Conversation
The refetch host API endpoint accepted refetch requests for Android hosts and returned 200, but Android hosts report their data through AMAPI and never run osquery or Apple MDM. Since refetch_requested is only cleared by the osquery detail query path and by the Apple MDM DeviceInformation response, the flag stayed true on the host forever, so the API reported a refetch as pending indefinitely on a host that will never perform one. The Host details page already hides the Refetch button for Android hosts and explains why, so return the same explanation to API callers instead of a success response that never refetches anything. Fixes fleetdm#50882
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. Walkthrough
Merge Risk: ⚪ Minimal · up to This change rejects unsupported Android host refetch requests before they can set a stale pending flag, while preserving supported host behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description identifies issue Full details: Linked Issues checkExplanation The changes address issue Full details: Out of Scope Changes checkResolution Remove the unrelated IDP device-mapping test changes and disk-encryption escrow configuration changes, or provide a clear requirement and dependency showing why they are needed for issue Full details: Docstring CoverageExplanation Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 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 #51119 +/- ##
=======================================
Coverage 76.21% 76.21%
=======================================
Files 4192 4193 +1
Lines 254389 254488 +99
Branches 14822 14825 +3
=======================================
+ Hits 193872 193963 +91
- Misses 60342 60349 +7
- Partials 175 176 +1
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:
|
nilaway flagged the bre.Message access because it cannot see that require.ErrorAs guarantees a non-nil result. BadRequestError.Error() returns Message, so assert on the error text instead, which keeps the same assertion and matches how other tests in this package check typed errors.
|
@MagnusHJensen mind taking a look at this one when you get a chance? |
|
I'll leave this up to the byod group, which is @dantecatalfamo and @ksykulev |
|
|
||
| // Android hosts report their data through AMAPI whenever it changes, so there is | ||
| // nothing to refetch on demand. The Host details page hides the Refetch button for | ||
| // them; reject the request here too so API callers get an explanation instead of a | ||
| // success response that never refetches anything. | ||
| if fleet.IsAndroidPlatform(host.Platform) { | ||
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | ||
| Message: "Refetch is not supported for Android hosts. Android hosts sync data automatically when it changes.", | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if err := svc.ds.UpdateHostRefetchRequested(ctx, id, true); err != nil { |
There was a problem hiding this comment.
Was placing this inside the non-device-auth block intentional, since HostDeviceURL mints a device auth token for any host except iOS/iPadOS and AuthenticateDevice rejects only iOS/iPadOS, so GET /hosts/:id/device_url followed by POST /device/:token/refetch still returns 200 and pins refetch_requested = true on an Android host? authenticatedDevice puts the host it resolved into hostctx for all three device auth modes, so the platform is available without an extra read, and reading it into a local rather than assigning host keeps the iOS MDM-command block below from newly firing on the device route.
| // Android hosts report their data through AMAPI whenever it changes, so there is | |
| // nothing to refetch on demand. The Host details page hides the Refetch button for | |
| // them; reject the request here too so API callers get an explanation instead of a | |
| // success response that never refetches anything. | |
| if fleet.IsAndroidPlatform(host.Platform) { | |
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | |
| Message: "Refetch is not supported for Android hosts. Android hosts sync data automatically when it changes.", | |
| }) | |
| } | |
| } | |
| if err := svc.ds.UpdateHostRefetchRequested(ctx, id, true); err != nil { | |
| } | |
| // Android hosts report their data through AMAPI whenever it changes, so there is | |
| // nothing to refetch on demand. The Host details page hides the Refetch button for | |
| // them; reject the request here too so API callers get an explanation instead of a | |
| // success response that never refetches anything. | |
| // | |
| // This sits outside the block above so it also covers the device-authenticated | |
| // route, which leaves host nil: an admin can mint a device token for an Android | |
| // host with GET /hosts/:id/device_url, and authenticatedDevice always puts the | |
| // host it resolved into the request context. | |
| hostPlatform := "" | |
| if host != nil { | |
| hostPlatform = host.Platform | |
| } else if deviceHost, ok := hostctx.FromContext(ctx); ok { | |
| hostPlatform = deviceHost.Platform | |
| } | |
| if fleet.IsAndroidPlatform(hostPlatform) { | |
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | |
| Message: "Refetch is not supported for Android hosts. Android hosts sync data automatically when they change.", | |
| }) | |
| } | |
| if err := svc.ds.UpdateHostRefetchRequested(ctx, id, true); err != nil { |
The message change also lines the wording up with ANDROID_NO_REFETCH_TOOLTIP_MESSAGE in HostHeader/helpers.tsx, which says "when they change".
There was a problem hiding this comment.
Good catch, you're right. HostDeviceURL mints a token for Android, so the device route still returned 200. Moved the check out of the user-auth block and take the platform from hostctx, holding it in a local so the iOS MDM commands below stay off the device path.
| err := svc.RefetchHost(test.UserContext(ctx, test.UserAdmin), host.ID) | ||
| require.Error(t, err) | ||
| var bre *fleet.BadRequestError | ||
| require.ErrorAs(t, err, &bre) | ||
| require.ErrorContains(t, err, "Refetch is not supported for Android hosts. Android hosts sync data automatically when it changes.") | ||
|
|
||
| // the refetch flag must not be set for a host that can't be refetched | ||
| assert.True(t, ds.HostLiteFuncInvoked) | ||
| assert.False(t, ds.UpdateHostRefetchRequestedFuncInvoked) |
There was a problem hiding this comment.
Since test.HostContext already sets both hostctx and AuthnDeviceToken, a second call would cover the device-authenticated route in two lines, and matching on a substring rather than the whole sentence keeps the assertion from breaking on copy tweaks.
| err := svc.RefetchHost(test.UserContext(ctx, test.UserAdmin), host.ID) | |
| require.Error(t, err) | |
| var bre *fleet.BadRequestError | |
| require.ErrorAs(t, err, &bre) | |
| require.ErrorContains(t, err, "Refetch is not supported for Android hosts. Android hosts sync data automatically when it changes.") | |
| // the refetch flag must not be set for a host that can't be refetched | |
| assert.True(t, ds.HostLiteFuncInvoked) | |
| assert.False(t, ds.UpdateHostRefetchRequestedFuncInvoked) | |
| err := svc.RefetchHost(test.UserContext(ctx, test.UserAdmin), host.ID) | |
| require.Error(t, err) | |
| var bre *fleet.BadRequestError | |
| require.ErrorAs(t, err, &bre) | |
| require.ErrorContains(t, err, "Refetch is not supported for Android hosts") | |
| // the device-authenticated route reaches RefetchHost with no host loaded, so the | |
| // platform has to come from the request context | |
| err = svc.RefetchHost(test.HostContext(ctx, host), host.ID) | |
| require.ErrorAs(t, err, &bre) | |
| // the refetch flag must not be set for a host that can't be refetched | |
| assert.True(t, ds.HostLiteFuncInvoked) | |
| assert.False(t, ds.UpdateHostRefetchRequestedFuncInvoked) |
An integration case in integration_android_software_test.go asserting http.StatusBadRequest would still be worth adding, since neither of these exercises the transport and the status code is the actual contract change.
There was a problem hiding this comment.
Done — added the test.HostContext call and switched both assertions to the substring.
|
|
||
| Flags the host details, labels and policies to be refetched the next time the host checks in. Note that we cannot be certain when the host will actually check in. Further requests to the host APIs will indicate that the refetch has been requested through the `refetch_requested` field on the host object. | ||
|
|
||
| Android hosts can't be refetched on demand because they sync data automatically when it changes. This endpoint returns a `400` for Android hosts. |
There was a problem hiding this comment.
Since "Refetch host by Fleet Desktop token" is documented as "Same as Refetch host", naming both routes here would keep the two in sync once the device path is covered.
| Android hosts can't be refetched on demand because they sync data automatically when it changes. This endpoint returns a `400` for Android hosts. | |
| Android hosts can't be refetched on demand because they sync data automatically when they change. This endpoint and [Refetch host by Fleet Desktop token](#refetch-host-by-fleet-desktop-token) return a `400` for Android hosts. |
There was a problem hiding this comment.
Done, the line now names both and links to Refetch host by Fleet Desktop token.
| } | ||
| } | ||
|
|
||
| if err := svc.ds.UpdateHostRefetchRequested(ctx, id, true); err != nil { |
There was a problem hiding this comment.
Heads up that Android hosts flagged on earlier versions keep refetch_requested: true forever, since it is only ever cleared by the osquery detail path and the Apple MDM DeviceInformation response and the Android insert in server/datastore/mysql/android.go leaves the column at its DEFAULT '0', so a migration doing UPDATE hosts SET refetch_requested = 0 WHERE platform = 'android' would clear the residue this fix otherwise leaves behind.
The guard sat inside the block that only runs for user-authenticated requests, but HostDeviceURL mints a device token for every platform except iOS and iPadOS, so GET /hosts/:id/device_url followed by POST /device/:token/refetch still returned 200 for an Android host and pinned refetch_requested. Take the platform from the host that device authentication already resolved into the request context, keeping it out of `host` so the iOS MDM commands further down stay off the device path. That also settles the TODO asking whether Android belongs in the iOS/iPadOS branch: it can no longer reach it. Nothing clears refetch_requested for an Android host — only the osquery detail path and the Apple MDM DeviceInformation response reset it — so hosts flagged before this keep it set forever. Added a migration to clear the residue.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…etch # Conflicts: # server/datastore/mysql/schema.sql
…etch # Conflicts: # server/datastore/mysql/schema.sql
|
Looks like the schema needs to be re-dumped |
…etch # Conflicts: # server/datastore/mysql/schema.sql
- Adds related issue (if any), and if any of the manual QA checkboxes were selected. - Adds a -v verbose mode that also outputs who is assigned to the PR, a full link, and tags from the related issue Examples: ``` jonathan@jonathans-macbook-pro:~/repos/fleet-2$ ./tools/oncall/oncall.sh prs 51893 2026-08-25T17:29:40Z tested prateek-0490 Batch scheduled query name lookups when processing result logs 51881 2026-08-25T16:25:40Z nonpunctual Include script for printer configuration from CSV 51845 2026-08-25T04:20:44Z 49502 tested rajendrachavan Write a query and its labels in a single transaction 51731 2026-08-21T19:49:52Z tested prateek-0490 Validate software list order keys against an allowlist 51583 2026-08-19T19:34:15Z tested prateek-0490 Require a team selection when validating report run targets 51338 2026-08-17T05:23:53Z 50846 Dhvanit41 Hide the toast details panel when the payload is empty 51265 2026-08-14T17:54:28Z tested prateek-0490 Expand host script variables via the environment 51241 2026-08-14T14:32:34Z 50336 tested andymFleet 50336 apple business stale fleet names 51178 2026-08-13T20:18:48Z moojomoore Add macOS unlock user account action 51119 2026-08-13T05:28:14Z 50882 Dhvanit41 Return an error when refetching an Android host 51020 2026-08-12T00:46:05Z 40092 sjawhar Support credentialProviderPolicy in Android app configuration 50943 2026-08-11T12:31:48Z 46773 tested surajtripathy07 Add software custom targets warning to delete label modal 50898 2026-08-10T19:26:08Z 45100 tianrking Recover vulnerability counts after interrupted table swaps 50836 2026-08-08T19:23:13Z 50300 tested kitarp29 Adding the logic to make controls even when it's empty 50822 2026-08-08T01:35:19Z tested viewer12 Add Nexus Shell as a macOS Fleet-maintained app 50774 2026-08-07T15:14:11Z 50357 tested Dhvanit41 Clear stale failure detail when an Android profile becomes verified 49816 2026-07-23T13:19:19Z tested sleepy-slothh Fix Ubuntu OSV binary package matching 43774 2026-04-20T15:46:23Z BCTBB Helm - Gateway API support ``` ``` jonathan@jonathans-macbook-pro:~/repos/fleet-2$ ./tools/oncall/oncall.sh prs -v 51893 2026-08-25T17:29:40Z tested prateek-0490 #51893 Batch scheduled query name lookups when processing result logs 51881 2026-08-25T16:25:40Z nonpunctual #51881 Include script for printer configuration from CSV 51845 2026-08-25T04:20:44Z 49502 tested rajendrachavan lucasmrod #51845 Write a query and its labels in a single transaction bug, ~backend, ~released bug, #g-orchestration 51731 2026-08-21T19:49:52Z tested prateek-0490 juan-fdz-hawa #51731 Validate software list order keys against an allowlist 51583 2026-08-19T19:34:15Z tested prateek-0490 juan-fdz-hawa #51583 Require a team selection when validating report run targets 51338 2026-08-17T05:23:53Z 50846 Dhvanit41 #51338 Hide the toast details panel when the payload is empty bug, ~frontend, ~released bug, #g-auto-patching 51265 2026-08-14T17:54:28Z tested prateek-0490 georgekarrv #51265 Expand host script variables via the environment 51241 2026-08-14T14:32:34Z 50336 tested andymFleet JordanMontgomery #51241 50336 apple business stale fleet names bug, ~released bug, #g-apple-at-work 51178 2026-08-13T20:18:48Z moojomoore #51178 Add macOS unlock user account action 51119 2026-08-13T05:28:14Z 50882 Dhvanit41 #51119 Return an error when refetching an Android host bug, ~released bug, #g-byod 51020 2026-08-12T00:46:05Z 40092 sjawhar #51020 Support credentialProviderPolicy in Android app configuration ~feature fest, customer-pingali, #g-byod 50943 2026-08-11T12:31:48Z 46773 tested surajtripathy07 lucasmrod #50943 Add software custom targets warning to delete label modal bug, ~released bug, #g-orchestration, customer-susanoo 50898 2026-08-10T19:26:08Z 45100 tianrking juan-fdz-hawa #50898 Recover vulnerability counts after interrupted table swaps bug, ~released bug, ~community, #g-supply-chain, ~aging bug 50836 2026-08-08T19:23:13Z 50300 tested kitarp29 lucasmrod #50836 Adding the logic to make controls even when it's empty bug, ~gitops, #g-orchestration 50822 2026-08-08T01:35:19Z tested viewer12 AdamBaali #50822 Add Nexus Shell as a macOS Fleet-maintained app 50774 2026-08-07T15:14:11Z 50357 tested Dhvanit41 #50774 Clear stale failure detail when an Android profile becomes verified bug, ~released bug, #g-byod 49816 2026-07-23T13:19:19Z tested sleepy-slothh #49816 Fix Ubuntu OSV binary package matching 43774 2026-04-20T15:46:23Z BCTBB #43774 Helm - Gateway API support ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an optional verbose mode for pull request listings, showing assignees, URLs, linked issue labels, and manual QA status. * Improved output formatting with aligned columns. * **Bug Fixes** * Unknown command-line arguments are now rejected with clearer usage guidance. * Improved detection of linked issues in pull request descriptions. * **Documentation** * Updated command usage information to describe the verbose option. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
@Dhvanit41 Sorry, looks like the schema needs to be re-dumped again. I commented on your other issue with the same problem. If you do the other one first, then wait for it to be merged, and do this one, we should be good to go! |
…etch # Conflicts: # server/datastore/mysql/schema.sql
Main gained migrations up to 20260831190535 while this was open, which left this one below the watermark. Goose only picks up versions above the highest already applied, so it would have been skipped on any database carrying the newer ones.
|
Re-dumped, thanks. Also rebased on main and bumped the migration to Will leave #50774 until this one merges, as you suggested. |
|
Cc : @dantecatalfamo |
Main picked up 20260901174312 and 20260901192408 after this branch was stamped, so the migration would have sorted before migrations already applied on existing databases.
…etch Restamp the refetch migration to 20260904204224 so it sorts after 20260904202219, which main picked up after this branch was last stamped, and regenerate schema.sql on top of main's.
Restamp the refetch migration ahead of main's latest and re-dump the schema.
The service-level tests call RefetchHost directly, so the 400 status code - the actual contract change - went untested. Covers both the user- and device-authenticated routes, and that refetch_requested stays unset. The test lives in integration_core_hosts_test.go rather than integration_core_android_test.go because the latter's _android filename suffix is read as a GOOS constraint, so that file never builds.
Re-dump the schema on top of main's latest migration.
…etch # Conflicts: # server/datastore/mysql/schema.sql
main merged 20260921141651_AddExecutableHashesToHostSoftwareInstalledPaths, so 20260917151330 would have run out of order. Re-dumped schema.sql.
…etch # Conflicts: # server/datastore/mysql/schema.sql
main merged the 20260922041731-33 nano/profile index migrations, so 20260921171134 would have run out of order. Re-dumped schema.sql.
Related issue: Resolves #50882
POST /api/v1/fleet/hosts/:id/refetchreturned200for Android hosts and flagged them for refetch, even though an Android host will never act on that flag.Android hosts report their data through AMAPI and run neither osquery nor Apple MDM.
refetch_requestedis only cleared in two places — the osquery detail query path (server/service/osquery.go) and the Apple MDMDeviceInformationresponse (server/service/apple_mdm.go) — so once set on an Android host it stayedtruepermanently. The API therefore reported a refetch as pending indefinitely on a host that will never perform one.The Host details page already disables the Refetch button for Android hosts and explains why (#50001), so this returns the same explanation to API callers:
Changes
server/service/hosts.go— reject refetch for Android hosts inRefetchHost, after the authorization check and before therefetch_requestedwrite, so the flag is never set.server/service/hosts_test.go— regression test asserting the error and thatUpdateHostRefetchRequestedis not called.docs/REST API/rest-api.md— document the Android behavior on the "Refetch host" endpoint.Notes for the reviewer
I used
fleet.BadRequestError(400) to matchfleet.ValidateAndroidWipeRequest, the closest existing precedent for an action that isn't supported on a given platform. Happy to switch toNewInvalidArgumentError(422) or adjust the wording if the group prefers something else.The device-authenticated route (
POST /device/{token}/refetch) is left alone, because none of its three authentication modes can be reached by an Android host:AuthnDeviceURL—AuthenticateIDeviceByURLexplicitly rejects any platform that isn't iOS or iPadOS.AuthnDeviceToken— device auth tokens are only issued by orbit/fleetd and conditional access, and Android hosts run neither.AuthnDeviceCertificate— resolves the host from an MDM SCEP certificate serial, which is part of Apple MDM enrollment.Guarding that path would add a host lookup to a route an Android host can't reach.
The background callers of
refetch_requested(calendar cron,apple_mdm.go,orbit.go) write through the datastore directly rather thanRefetchHost, so they are unaffected.Checklist for submitter
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.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
On the manual QA box: I verified this with automated tests rather than against a live Android host, since I don't have an Android Enterprise environment to enroll one.
TestRefetchHostAndroidNotSupportedfails with the fix reverted (returnsnilinstead of an error, i.e. the reported200), so it reproduces the reported behavior. I also checked that no existing integration test refetches an Android host — the refetch tests that assert200use Linux hosts (createHostsdefaults todebian/rhel/linux) or iOS/iPadOS hosts. Confirmation against a real enrolled Android device from someone with that setup would be welcome.Summary by CodeRabbit