Android profile content checksums - #46276
Conversation
![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.
There was a problem hiding this comment.
Pull request overview
Fixes #43456 by switching the "should we re-send this Android profile" decision in ListMDMAndroidProfilesToSend from a policy-version comparison to a content-checksum comparison, so that unrelated AMAPI policy bumps (e.g. cert template work) no longer revert verified profiles to pending. Adds a generated checksum column on mdm_android_configuration_profiles (MD5 of raw_json) and a mirrored checksum column on host_mdm_android_profiles, plus plumbs the checksum through the reconciler and verify paths. Also tightens verifyDevicePolicy to pick the most-recent applied profile PATCH UUID via <= rather than ==.
Changes:
- New migration adds
checksum BINARY(16)to both Android profile tables and backfills host rows. ListMDMAndroidProfilesToSendselects onhmap.checksum != ds.checksum(or missing row / NULL status) instead ofincluded_in_policy_version.- Reconciler, bulk upsert, and
verifyDevicePolicyupdated to carry the checksum and handle the case where the previously-applied policy version is older than the device's applied version.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| changes/43456-android-profile-checksum | Changelog entry. |
| server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.go | Adds checksum columns and backfills host rows from config-profile checksum. |
| server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles_test.go | Migration test exercising config profile + matching/orphan host rows. |
| server/datastore/mysql/schema.sql | Regenerated schema reflecting both new columns. |
| server/datastore/mysql/android.go | Adds checksum to applicable-profiles CTEs, switches re-send predicate to checksum comparison, and threads checksum through bulk upsert. |
| server/datastore/mysql/android_test.go | Adds getAndroidProfileChecksum helper and updates expected payloads to include checksum. |
| server/fleet/android.go | Adds Checksum []byte field to MDMAndroidProfilePayload. |
| server/mdm/android/service/profiles.go | Propagates prof.Checksum into install/remove/failed payloads; intentionally omits it for withheld profiles. |
| server/mdm/android/service/profiles_test.go | Updates verify-path test payloads to include checksum; adds helper. |
| server/mdm/android/service/pubsub.go | Adds nil-check for IncludedInPolicyVersion and picks the highest applied profile-PATCH UUID using <=. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis pull request implements content-based checksum tracking for Android MDM profiles to decouple profile re-sync from unrelated policy version bumps. It adds a Checksum field to profile payloads, adds/generated checksum columns and a backfill migration, updates datastore queries and bulk upsert to select and persist checksums, changes change-detection logic to compare checksums (row missing / checksum mismatch / NULL status), and propagates checksums through service and pubsub verification paths. It also fixes policyRequestUUID selection to pick the highest IncludedInPolicyVersion ≤ device.AppliedPolicyVersion. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/mdm/android/service/pubsub.go (1)
963-972:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an early guard when no applicable policy request UUID is found.
If no profile satisfies the selection (or UUID/version fields are nil),
policyRequestUUIDstays empty and triggers a lookup/log path that looks like a real NotFound. Please short-circuit before querying.Suggested fix
for _, profile := range pendingInstallProfiles { if profile.PolicyRequestUUID != nil && profile.IncludedInPolicyVersion != nil { v := int64(*profile.IncludedInPolicyVersion) if v <= device.AppliedPolicyVersion && v > maxVersion { maxVersion = v policyRequestUUID = *profile.PolicyRequestUUID } } } + + if policyRequestUUID == "" { + svc.logger.DebugContext(ctx, "no applicable policy request UUID found for non-compliance verification", + "host_uuid", hostUUID, "applied_policy_version", device.AppliedPolicyVersion) + return + } // Iterate over all policy request uuids, fetch them and unmarshal the payload into the type.🤖 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/mdm/android/service/pubsub.go` around lines 963 - 972, When selecting a profile leaves policyRequestUUID empty, short-circuit before calling svc.ds.GetAndroidPolicyRequestByUUID: add an early guard that checks if policyRequestUUID == "" (or nil-equivalent) and return after logging a clear debug/info message (including hostUUID) instead of performing the lookup; update the code around the policyRequestUUID usage in pubsub.go so the GetAndroidPolicyRequestByUUID call only runs when policyRequestUUID is non-empty to avoid treating an empty UUID as a NotFound error.
🧹 Nitpick comments (2)
server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles_test.go (1)
55-61: ⚡ Quick winAssert the orphan checksum exact value, not just inequality.
At Line 61,
NotEqualis too weak and won’t catch sentinel drift. Assert the exact fallback checksum bytes expected by schema/migration.💡 Proposed fix
- // Orphan checksum should NOT equal the real profile's checksum - assert.NotEqual(t, fmt.Sprintf("%x", expectedChecksum), fmt.Sprintf("%x", orphanChecksum)) + // Orphan checksum should be the explicit zero-checksum sentinel. + assert.Equal(t, "00000000000000000000000000000000", fmt.Sprintf("%x", orphanChecksum))🤖 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/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles_test.go` around lines 55 - 61, Change the weak NotEqual assertion to assert the exact expected fallback checksum value for the orphan profile: compute the expected fallback checksum bytes (the migration uses COALESCE(…) which yields the sentinel 0 stored as MySQL BINARY(16) representation, i.e. 0x30 followed by zeros) and assert orphanChecksum equals that expected byte slice (or its hex string via fmt.Sprintf("%x", ...)). Update the assertion that currently compares fmt.Sprintf("%x", expectedChecksum) vs fmt.Sprintf("%x", orphanChecksum) to instead compare orphanChecksum (or its hex) to the concrete expected fallback checksum so sentinel drift is caught; locate this change around the db.QueryRow/select of checksum and the orphanChecksum variable in the test.server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.go (1)
15-15: ⚡ Quick winMatch the generated checksum expression to schema.sql exactly.
Line 15 omits
charset utf8mb4inCAST(raw_json AS CHAR), while schema.sql includes it. Keeping these byte-identical avoids schema/migration drift.💡 Proposed fix
- ADD COLUMN checksum BINARY(16) AS (UNHEX(MD5(CAST(raw_json AS CHAR)))) STORED; + ADD COLUMN checksum BINARY(16) AS (UNHEX(MD5(CAST(raw_json AS CHAR CHARSET utf8mb4)))) STORED;🤖 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/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.go` at line 15, Update the generated CHECKSUM column expression to match schema.sql exactly by including the utf8mb4 charset in the CAST; specifically modify the column definition for checksum (the expression AS (UNHEX(MD5(CAST(raw_json AS CHAR ...)))) STORED) so the CAST uses CHARACTER SET utf8mb4 (e.g. CAST(raw_json AS CHAR CHARACTER SET utf8mb4) or the equivalent syntax used in schema.sql), ensuring the binary checksum expression is byte-identical to schema.sql.
🤖 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.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.go`:
- Around line 18-21: The migration AddChecksumToAndroidProfiles.go currently
uses numeric 0 for the BINARY(16) checksum default and COALESCE fallback;
replace those with the schema's 16-null-byte literal so the ADD COLUMN and the
UPDATE use the exact 16-byte-zero sentinel (the same literal used in schema.sql)
to avoid byte-sequence drift for host_mdm_android_profiles.checksum and the
COALESCE fallback when selecting from
mdm_android_configuration_profiles.checksum.
---
Outside diff comments:
In `@server/mdm/android/service/pubsub.go`:
- Around line 963-972: When selecting a profile leaves policyRequestUUID empty,
short-circuit before calling svc.ds.GetAndroidPolicyRequestByUUID: add an early
guard that checks if policyRequestUUID == "" (or nil-equivalent) and return
after logging a clear debug/info message (including hostUUID) instead of
performing the lookup; update the code around the policyRequestUUID usage in
pubsub.go so the GetAndroidPolicyRequestByUUID call only runs when
policyRequestUUID is non-empty to avoid treating an empty UUID as a NotFound
error.
---
Nitpick comments:
In
`@server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles_test.go`:
- Around line 55-61: Change the weak NotEqual assertion to assert the exact
expected fallback checksum value for the orphan profile: compute the expected
fallback checksum bytes (the migration uses COALESCE(…) which yields the
sentinel 0 stored as MySQL BINARY(16) representation, i.e. 0x30 followed by
zeros) and assert orphanChecksum equals that expected byte slice (or its hex
string via fmt.Sprintf("%x", ...)). Update the assertion that currently compares
fmt.Sprintf("%x", expectedChecksum) vs fmt.Sprintf("%x", orphanChecksum) to
instead compare orphanChecksum (or its hex) to the concrete expected fallback
checksum so sentinel drift is caught; locate this change around the
db.QueryRow/select of checksum and the orphanChecksum variable in the test.
In
`@server/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.go`:
- Line 15: Update the generated CHECKSUM column expression to match schema.sql
exactly by including the utf8mb4 charset in the CAST; specifically modify the
column definition for checksum (the expression AS (UNHEX(MD5(CAST(raw_json AS
CHAR ...)))) STORED) so the CAST uses CHARACTER SET utf8mb4 (e.g. CAST(raw_json
AS CHAR CHARACTER SET utf8mb4) or the equivalent syntax used in schema.sql),
ensuring the binary checksum expression is byte-identical to schema.sql.
🪄 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: 8dc360d0-d6d6-4f6b-a7da-5731eb7944ff
📒 Files selected for processing (10)
changes/43456-android-profile-checksumserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles.goserver/datastore/mysql/migrations/tables/20260526231538_AddChecksumToAndroidProfiles_test.goserver/datastore/mysql/schema.sqlserver/fleet/android.goserver/mdm/android/service/profiles.goserver/mdm/android/service/profiles_test.goserver/mdm/android/service/pubsub.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46276 +/- ##
==========================================
- Coverage 66.87% 66.84% -0.04%
==========================================
Files 2786 2764 -22
Lines 222256 221418 -838
Branches 11469 10895 -574
==========================================
- Hits 148642 148012 -630
+ Misses 60160 59954 -206
+ Partials 13454 13452 -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:
|
|
@claude review once |
There was a problem hiding this comment.
This is a meaningful re-sync decision change in Android MDM that replaces policy-version comparison with content checksums across schema, queries, reconciler, and pubsub verification — worth a human look given the cross-layer invariants (intentional zero-checksum for withheld ONC profiles, NULL-status safety net for callers that don't propagate checksums).
Extended reasoning...
Overview\nThis PR adds content-based checksum tracking for Android MDM profiles, replacing the prior policy-version-based logic in ListMDMAndroidProfilesToSend. The change touches a migration (new checksum columns on mdm_android_configuration_profiles as a STORED generated column from raw_json, and on host_mdm_android_profiles with backfill from the config profile), the applicable-profiles SQL and bulk upsert in server/datastore/mysql/android.go, the reconciler in server/mdm/android/service/profiles.go, and the verification path in server/mdm/android/service/pubsub.go. Tests were updated to assert the new Checksum field flows through, including a new testONCWithheldUntilCertVerified test.\n\n### Security risks\nNo direct security risk surface: no new auth boundaries, no new untrusted input handling, no crypto change (MD5 is used purely as a content fingerprint, not for security). SQL uses placeholders throughout. The migration generated column derives from already-validated profile JSON.\n\n### Level of scrutiny\nMedium-high. This is not a config or mechanical change — it modifies the core decision logic for when Fleet re-sends Android profiles to managed devices, with multiple subtle invariants that must hold for the system to behave correctly. Examples: (a) withheld ONC profiles intentionally retain a zero checksum so they re-trigger when a blocking cert verifies, (b) the new change-detection query relies on hmap.status IS NULL as a safety net for callers of BulkUpsertMDMAndroidHostProfiles that don't propagate checksum (e.g. bulkSetPendingMDMAndroidHostProfilesDB), (c) verifyDevicePolicy now picks the highest IncludedInPolicyVersion <= AppliedPolicyVersion instead of equality — a deliberate change to handle non-profile PATCHes bumping the device version.\n\n### Other factors\nPrior reviewer feedback from Copilot about checksum clobbering on nil values was addressed by the author by propagating profile.Checksum through both the verify and send paths; CodeRabbit's migration-default concern was correctly rebutted as following the Windows-profile convention. No bugs were surfaced by the bug hunting system on this run. Patch coverage is 86%, and the new tests exercise the key checksum-propagation paths. The PR author has explicitly requested a review on the latest revision, which signals they want eyes on it before merging.
|
@getvictor the checksum is an MD5 hash of the template. Which means that the placeholders will sit there without replacement. So if the variable changes the content hash won't change. This PR doesn't add a I could preemptively add |
Related issue: Resolves #43456
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.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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Migrations
Tests