iOS/iPadOS managed config: send-paths audit (#43968) - #44936
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.
…thods # Conflicts: # server/fleet/vpp.go
![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 |
jkatz01
previously approved these changes
May 11, 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)
WalkthroughThe changes add documentation comments to two database functions— 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 coverage and documentation to ensure managed app configuration is fetched and $FLEET_VAR_* substitution happens at the single enqueue “fan-in” points for Apple InstallApplication commands, and that macOS omits the Configuration field.
Changes:
- Add MySQL integration tests verifying iOS VPP installs include
<key>Configuration</key>with host-specific substitution and that macOS omits the field. - Document invariants that
nanoEnqueueVPPInstall/activateNextInHouseAppInstallActivityare the single fan-in points for enqueueingInstallApplication. - Minor whitespace cleanup in the Apple InstallApplication builder file.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/mdm/apple/install_application.go | Removes an extraneous blank/whitespace line. |
| server/datastore/mysql/vpp_test.go | Adds integration tests for Configuration dict presence/absence across iOS vs macOS enqueueing. |
| server/datastore/mysql/vpp.go | Adds documentation on nanoEnqueueVPPInstall as the VPP InstallApplication fan-in point. |
| server/datastore/mysql/activities.go | Adds documentation on in-house app install fan-in and substitution behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3264
to
+3265
| _, err = ds.InsertVPPAppWithTeam(ctx, vpp, nil) | ||
| require.NoError(t, err) |
Comment on lines
+3305
to
+3315
| const adamID = "macos-app-1" | ||
| setupTestVPPApp(t, ds, adamID, fleet.MacOSPlatform) | ||
| vpp := &fleet.VPPApp{ | ||
| Name: "MacApp", | ||
| VPPAppTeam: fleet.VPPAppTeam{ | ||
| VPPAppID: fleet.VPPAppID{AdamID: adamID, Platform: fleet.MacOSPlatform}, | ||
| }, | ||
| BundleIdentifier: adamID, | ||
| } | ||
| _, err = ds.InsertVPPAppWithTeam(ctx, vpp, nil) | ||
| require.NoError(t, err) |
Comment on lines
+3287
to
+3291
| // testVPPInstallOmitsConfigurationOnMacOS confirms that when the configuration | ||
| // is set up under iOS but a macOS install runs, the macOS InstallApplication | ||
| // command does not carry a Configuration dict. Apple's MDM does not accept | ||
| // the same Configuration shape on macOS, so this is both a correctness and a | ||
| // safety check. |
3 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 #44935.
Closes #43968.
The two activate functions are the only
InstallApplicationcommand 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.Summary by CodeRabbit
Documentation
Tests