iOS/iPadOS managed config: Fleet variable expansion (#43967) - #44935
Merged
Conversation
…o []byte Replaces the stub from the datastore-methods branch (#43964) with a real plist validator. Lives in server/fleet/vpp.go alongside the rest of VPP because validation is part of the InstallApplication command flow. - ValidateAppleAppConfiguration parses the payload via howett.net/plist into a map[string]any (which naturally rejects non-dict roots), then walks string-typed leaves checking Fleet variable tokens against the app-config allow-list. - FleetVarsSupportedInAppleAppConfig: host-scoped subset of the variables permitted in Apple configuration profiles. Excludes credential / SCEP / NDES variables that don't fit the InstallApplication shape. - Configuration field type: json.RawMessage -> []byte across VPPAppTeam, VPPAppStoreApp, AppStoreAppUpdatePayload. The Apple payload is plist XML, not JSON, so the field is just opaque bytes. Existing Android callers pass and receive []byte transparently (json.RawMessage is []byte-underlying, assignable in both directions). - Updated comments on the three Configuration fields to reflect Apple support. - No size cap in the validator — caps belong at the API layer. - Empty input is allowed: callers (e.g. the gitops change-detector in vpp.go) decide whether to store or clear. Issue: #43963
Replaces the ad-hoc walker over the parsed plist tree with a raw-bytes scan using server/variables.Find — the same helper validateConfigProfileFleetVariables in apple_mdm.go uses for profile validation. Drops the local fleetVarTokenRegexp, the per-call allowed-set map, the walkAppleAppConfigStrings recursive walker, and the higher-order-function callback style. The plist.Unmarshal step stays as the structural check (must be valid plist with a <dict> root). Token allow-list check is now a flat loop with slices.Contains.
…figuration Mirrors the Android datastore methods for the new vpp_app_configurations and in_house_app_configurations tables (subtask 01). Wires VPP methods into the existing add/update/delete code paths in vpp.go alongside the Android branches. In-house methods land here but aren't wired yet — fleet.InHouseAppPayload doesn't carry a Configuration field until subtask 02. ValidateAppleAppConfiguration is a temporary stub (TODO #43963) replaced by the real plist validator in subtask 02. Issue: #43964
Migration #44435 dropped team_id from in_house_app_configurations (the parent in_house_apps row already pins team and platform). Updates the five in-house datastore methods, the four interface signatures, and the regenerated mocks to match.
- Test helpers use real datastore APIs (insertInHouseApp, InsertVPPAppWithTeam, DeleteInHouseApp) instead of raw SQL where a public function exists. - Replace fleetdm.com sample with example.com in test plist. - HasChanged methods early-return false when incoming config is empty — null/empty incoming is a no-op; clearing requires explicit Delete*Configuration. Destructive-on-omit semantics for gitops will live in #43969 (TODO added).
- HasVPPAppConfigurationChanged / HasInHouseAppConfigurationChanged: empty incoming against existing config now returns true (= delete intent), matching Android's semantic. Removing a config from gitops YAML clears it. - SetTeamVPPApps batch upsert path: when iOS/iPadOS Configuration is empty, DELETE the existing config row instead of skipping the write. Implements the destructive-on-omit behavior at the wiring layer. - Get*AppConfiguration return type changed from *[]byte to []byte (nil = not found, returned alongside notFound error). Drops a non-idiomatic pointer. - Updated stale comment on VPPAppTeam.Configuration / VPPAppStoreApp.Configuration to reflect that Apple platforms now use the field (plist payload). - Added documentation on Datastore interface that in-house config methods do not enforce team-scoped auth — callers must validate. - Added cross-team isolation assertions to the VPP CRUDFlow test.
…byte Now that VPPAppTeam.Configuration is []byte (subtask 02), align the Android datastore surface to match. Drops the now-redundant pointer wrapping on Get* returns at the same time. Method signatures: - GetAndroidAppConfiguration -> ([]byte, error) (was *json.RawMessage) - GetAndroidAppConfigurationByAppTeamID -> ([]byte, error) - BulkGetAndroidAppConfigurations -> (map[string][]byte, error) - HasAndroidAppConfigurationChanged -> takes []byte - updateAndroidAppConfigurationTx -> takes []byte Callers updated: - vpp.go app-store fetch (drops *config deref) - worker/software_worker.go (configByAppID maps + helper signature) - Tests in android_test.go, vpp_test.go, software_worker_test.go Mocks regenerated. Also fixes a side-effect in TestAndroid/AddDeleteAndroidAppWithConfiguration: the test previously sent JSON-shaped Configuration to an iOS app (vestigial "ios shouldn't have configuration" wording). The real iOS validator from subtask 02 now rejects non-plist input, so the test seeds a valid plist fragment instead.
Adds Configuration on InHouseAppPayload and the SoftwareInstaller upload/update payloads, and threads it through MatchOrCreateSoftwareInstaller, insertInHouseApp, and SaveInHouseAppUpdates inside their existing transactions. GetInHouseAppMetadataByTeamAndTitleID now hydrates the Configuration field.
Implements subtask 04: AddAppStoreApp, UpdateAppStoreApp, and
BatchAssociateVPPApps now persist the configuration field for iOS and
iPadOS VPP apps (previously silently dropped). Each iOS/iPadOS branch
JSON-decodes the wire-format string, calls fleet.ValidateAppleAppConfiguration,
and forwards the decoded plist bytes to the datastore.
The configuration wipe at the top of the Apple-platform branch is scoped
to macOS (which doesn't support managed configuration on App Store apps).
Activity emission keeps the JSON-encoded form so ActivityEditedAppStoreApp
/ ActivityAddedAppStoreApp's Configuration json.RawMessage field continues
to marshal as valid JSON for both Android (inline JSON object) and Apple
(JSON-quoted plist string).
decodeAppleAppConfiguration helper translates the wire format
("configuration": "<plist XML>") into raw bytes for validator and
datastore consumption.
New integration test TestVPPAppleManagedAppConfiguration covers add with
valid plist, update with allowed Fleet variable, omit-field no-op,
malformed XML rejection, and disallowed-variable rejection.
Premium gate and authorization unchanged (existing entry-point checks
already cover iOS/iPadOS — same fleet.VPPApp authz + EE-only service
implementation).
Issue: #43965
- Add iOS update path validator coverage (malformed plist + disallowed Fleet variable) - Add macOS invalid-plist cases on add, update, and batch paths to lock down the silent-drop ordering (validation must run after macOS configuration is cleared, not before)
Cover platform-specific Configuration encoding (iOS/iPadOS as JSON string, Android as base64), field omission for nil/empty Configuration, and JSON null handling. Also tightens the early-exit in both UnmarshalJSON sites to treat the literal `null` token as nil instead of decoding it through the platform-specific path.
Default []byte JSON encoding produced base64 for Android responses, which is an artifact of Go's defaults rather than a deliberate choice. Override MarshalJSON to wrap the field as json.RawMessage and update UnmarshalJSON to mirror it, so requests and responses both use a JSON object for Android (symmetric with iOS / iPadOS using a JSON string of plist).
The custom UnmarshalJSON was localizing wire-format knowledge to the request type, but with the symmetric Android wire format the service can take the configuration as raw JSON directly and unwrap iOS plist strings inline. The activity emission also goes back to using the wire form directly without a re-encode round trip.
VPPAppStoreApp.Configuration is now json.RawMessage (the form returned in API responses) and the type-level MarshalJSON / UnmarshalJSON methods are gone. UpdateAppStoreApp (EE) and the GET software title endpoint each wrap iOS / iPadOS plist bytes as a JSON string after fetching from the datastore, before assigning to the response. Android passes through as raw JSON.
- Configuration is a multipart form field (raw XML bytes for iOS / iPadOS). - Upload validates with ValidateAppleAppConfiguration; for non-.ipa extensions the configuration is silently dropped. - Update treats nil as "leave unchanged", non-nil empty as "clear", non-nil non-empty as "validate + set". - Response sites (UploadSoftwareInstaller, UpdateSoftwareInstaller, GET software title) wrap the iOS configuration as a JSON string so it appears as the same wire form clients send on input. - Integration test covers upload + update + clear + validation errors.
GitOps apply now reads `configuration.path` from disk for iOS / iPadOS app store apps (validated with `ValidateAppleAppConfiguration`, sent on the wire as a JSON-encoded string of XML to match the API shape), and `fleetctl generate-gitops` emits stored configurations under `lib/<team>/software/<slug>-<platform>-config.xml` alongside the existing Android `*-config.json` artifacts.
… gitops Adds a `configuration.path` field to `software.packages` entries. When present, gitops apply reads the file (validated with `ValidateAppleAppConfiguration`), threads it through the batch upload pipeline, and `BatchSetInHouseAppsInstallers` upserts or clears the in-house app configuration declaratively. Configuration is silently dropped for non-.ipa packages. `fleetctl generate-gitops` emits the stored configuration as `<slug>-<platform>-config.xml` alongside the in-house app's other artifacts.
cdcme
marked this pull request as ready for review
May 9, 2026 00:21
![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.
Member
Author
|
@claude review once |
There was a problem hiding this comment.
MDM device-command code touching a security-sensitive XML/plist path — substitution looks correct (XML escaping via the shared , explicit error on unresolvable vars rather than silent empty values, decent test coverage including an escape round-trip) but worth a human pass given the surface area.
Extended reasoning...
Overview
Adds SubstituteFleetVarsInAppConfig in server/mdm/apple/install_application.go and wires it into two call sites in the datastore (nanoEnqueueVPPInstall in vpp.go, activateNextInHouseAppInstallActivity in activities.go). The function expands the host-scoped $FLEET_VAR_* tokens allow-listed by FleetVarsSupportedInAppleAppConfig directly before the InstallApplication plist is built and inserted into nano_commands. The two SELECTs on hosts were extended to pull hardware_serial so it's available without a round-trip.
Security risks
This is the path that produces XML-plist bytes sent to managed Apple devices, so the main risk is XML/plist injection via substituted values. The implementation reuses profiles.ReplaceFleetVariableInXML, which runs the replacement through xml.EscapeText before substitution, and there is a dedicated test (XML special chars in substituted value are escaped) that round-trips through the plist parser. Unresolvable variables return ErrUnresolvableAppConfigVar so the caller fails the install rather than shipping a blank value. The validator (fleet.ValidateAppleAppConfiguration) already restricts stored bytes to the allow-list, and the default arm of the switch fails closed if validator/switch drift.
Level of scrutiny
Medium-high — this is MDM device-command code, not config or cosmetics. It's also a non-trivial feature change (~150 net lines across datastore, mdm/apple, and tests) and is part of a stacked PR series (iosmac/*) where the base is an earlier feature branch rather than main. Per the approval guidelines, MDM/security-adjacent feature code should get a human pass even when the implementation looks right.
Other factors
- Bug-hunting system flagged nothing.
- Codecov reports ~66% patch coverage (23 uncovered lines), mostly in the datastore wiring; the core substitution function is well-covered.
- The author explicitly requested a review (
@claude review once) on 2026-05-11. - No outstanding reviewer comments to address.
jkatz01
reviewed
May 11, 2026
…urationsTx variants
jkatz01
approved these changes
May 11, 2026
jkatz01
previously approved these changes
May 12, 2026
jkatz01
approved these changes
May 12, 2026
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR implements runtime substitution of Possibly related PRs
✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Fleet variable expansion for iOS/iPadOS managed app configuration, enabling per-host substitution of $FLEET_VAR_* tokens before building InstallApplication commands.
Changes:
- Introduces
SubstituteFleetVarsInAppConfigwith support for the Apple app-config allow-list and “unresolvable variable” error signaling. - Invokes substitution in VPP and in-house install enqueue paths and pulls
hardware_serialin host queries. - Adds unit tests validating substitution behavior and plist round-tripping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/mdm/apple/install_application.go | Adds substitution logic and error type for unresolved managed-app-config variables. |
| server/mdm/apple/install_application_test.go | Adds tests for substitution behavior and round-trip safety with plist generation. |
| server/datastore/mysql/vpp.go | Fetches hardware_serial and performs substitution before generating VPP InstallApplication commands. |
| server/datastore/mysql/activities.go | Fetches hardware_serial and performs substitution before generating in-house InstallApplication commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+41
to
+71
| func SubstituteFleetVarsInAppConfig( | ||
| ctx context.Context, | ||
| ds fleet.Datastore, | ||
| config []byte, | ||
| host AppConfigSubstitutionHost, | ||
| ) ([]byte, error) { | ||
| if len(config) == 0 { | ||
| return config, nil | ||
| } | ||
| used := variables.Find(string(config)) | ||
| if len(used) == 0 { | ||
| return config, nil | ||
| } | ||
|
|
||
| contents := string(config) | ||
| idpUUIDCache := map[string]uint{} | ||
|
|
||
| for _, name := range used { | ||
| switch fleet.FleetVarName(name) { | ||
| case fleet.FleetVarHostUUID: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostUUIDRegexp, contents, host.UUID) | ||
| case fleet.FleetVarHostHardwareSerial: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostHardwareSerialRegexp, contents, host.HardwareSerial) | ||
| case fleet.FleetVarHostPlatform: | ||
| platform := host.Platform | ||
| if platform == "darwin" { | ||
| platform = "macos" | ||
| } | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostPlatformRegexp, contents, platform) | ||
| case fleet.FleetVarHostEndUserEmailIDP: | ||
| emails, err := ds.GetHostEmails(ctx, host.UUID, fleet.DeviceMappingMDMIdpAccounts) |
Comment on lines
+60
to
+69
| case fleet.FleetVarHostUUID: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostUUIDRegexp, contents, host.UUID) | ||
| case fleet.FleetVarHostHardwareSerial: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostHardwareSerialRegexp, contents, host.HardwareSerial) | ||
| case fleet.FleetVarHostPlatform: | ||
| platform := host.Platform | ||
| if platform == "darwin" { | ||
| platform = "macos" | ||
| } | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostPlatformRegexp, contents, platform) |
Comment on lines
+58
to
+70
| for _, name := range used { | ||
| switch fleet.FleetVarName(name) { | ||
| case fleet.FleetVarHostUUID: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostUUIDRegexp, contents, host.UUID) | ||
| case fleet.FleetVarHostHardwareSerial: | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostHardwareSerialRegexp, contents, host.HardwareSerial) | ||
| case fleet.FleetVarHostPlatform: | ||
| platform := host.Platform | ||
| if platform == "darwin" { | ||
| platform = "macos" | ||
| } | ||
| contents = profiles.ReplaceFleetVariableInXML(fleet.FleetVarHostPlatformRegexp, contents, platform) | ||
| case fleet.FleetVarHostEndUserEmailIDP: |
Comment on lines
+181
to
+187
| func TestSubstituteFleetVarsInAppConfig(t *testing.T) { | ||
| ctx := context.Background() | ||
| host := AppConfigSubstitutionHost{ | ||
| UUID: "host-uuid-1", | ||
| HardwareSerial: "ABC123", | ||
| Platform: "ios", | ||
| } |
cdcme
added a commit
that referenced
this pull request
May 12, 2026
Part of #38790. Stacked on top of #44935. Closes #43968. The two activate functions are the only `InstallApplication` command builders, so fetching configuration and substituting `$FLEET_VAR_*` inside them covers every enqueue path (manual, self-service, policy auto-install, scheduled auto-update, setup experience, retry, admin reinstall) by construction. Documents this invariant on each function. Adds integration tests asserting an iOS install enqueues `<key>Configuration>` with the host UUID substituted in, and macOS installs omit the field. --------- Co-authored-by: jkatz01 <yehonatankatz@gmail.com>
This was referenced May 14, 2026
4 tasks
4 tasks
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.
Part of #38790. Stacked on top of #44934.
Closes #43967.
Adds
SubstituteFleetVarsInAppConfiginserver/mdm/apple, called fromnanoEnqueueVPPInstallandactivateNextInHouseAppInstallActivityright before theInstallApplicationplist is built. Reusesprofiles.ReplaceFleetVariableInXML(XML-escapes substituted values) andprofiles.ReplaceHostEndUserIDPVariablesfor IDP fields.Supports the full
FleetVarsSupportedInAppleAppConfigallow-list:HOST_UUID,HOST_HARDWARE_SERIAL,HOST_PLATFORM,HOST_END_USER_EMAIL_IDP,HOST_END_USER_IDP_USERNAME/_LOCAL_PART/_GROUPS/_DEPARTMENT/_FULLNAME. ReturnsErrUnresolvableAppConfigVarwhen the host can't supply a referenced variable (e.g. end-user IDP not enrolled) so the caller can fail the install rather than send an empty value to the device.Pulls
hardware_serialonto the host SELECT soHOST_HARDWARE_SERIALis available without a second round-trip.Summary by CodeRabbit
New Features
Tests