iOS/iPadOS managed config: validator (#43963) - #44930
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44930 +/- ##
==========================================
+ Coverage 66.69% 66.81% +0.12%
==========================================
Files 2651 2665 +14
Lines 213559 216451 +2892
Branches 9647 9647
==========================================
+ Hits 142424 144621 +2197
- Misses 58169 58667 +498
- Partials 12966 13163 +197
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 |
![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
Adds backend validation support for iOS/iPadOS “managed app configuration” payloads (XML plist) and introduces an allow-list of supported $FLEET_VAR_* tokens for use in Apple app configurations.
Changes:
- Added
ValidateAppleAppConfigurationto parse/validate XML plist<dict>payloads and reject unsupported Fleet variables (including entity-encoded tokens). - Introduced
FleetVarsSupportedInAppleAppConfigallow-list for variables permitted in iOS/iPadOS managed app configuration. - Updated several tests and VPP structs to use
[]byteforConfiguration.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_android_software_test.go | Updates expectations around the configuration field type in integration tests. |
| server/fleet/vpp.go | Adds Apple app configuration validation + allow-list; changes Configuration field types. |
| server/fleet/vpp_test.go | Adds unit tests for ValidateAppleAppConfiguration. |
| server/datastore/mysql/vpp_test.go | Adjusts datastore tests for the new Configuration field type. |
| server/datastore/mysql/android_test.go | Adjusts Android configuration tests for the new Configuration field type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // "Browsers", etc. | ||
| Categories []string `json:"categories"` | ||
| DisplayName string `json:"display_name"` | ||
| // Configuration is a json file used to customize Android app | ||
| // behavior/settings. Applicable to Android apps only. | ||
| Configuration json.RawMessage `json:"configuration,omitempty"` | ||
| // Configuration is the managed app configuration payload. JSON for Android, | ||
| // XML for iOS / iPadOS. | ||
| Configuration []byte `json:"configuration,omitempty"` |
| // Configuration is the managed app configuration payload. JSON for Android, | ||
| // XML for iOS / iPadOS. | ||
| Configuration []byte `json:"configuration,omitempty"` | ||
| AutoUpdateEnabled *bool `json:"-"` | ||
| AutoUpdateStartTime *string `json:"-"` | ||
| AutoUpdateEndTime *string `json:"-"` |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request changes VPP app configuration payload representation from 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🧹 Nitpick comments (1)
server/fleet/vpp.go (1)
255-283: 💤 Low valueHelper is correct; consider a small UX nit on the "not a dict" error path.
findUnsupportedFleetVarcorrectly limits recursion to the three plist value kinds that can carry text (string,map[string]any,[]any) —int64/float64/bool/[]byte/time.Timecannot legally contain a$FLEET_VAR_*reference, so skipping them is the right behavior.One small UX consideration on the validator above: if a user submits an XML plist whose root is
<array>(or any other non-dict),plist.Unmarshal(..., &root)will surface a generic "cannot unmarshal array into Go value of type map[string]interface {}" error to the caller. The function docstring explicitly promises that the root must be a<dict>, so a dedicated error message would read better. If you want to keep the strict-typing approach, you can sniff the root withanyfirst and switch on the concrete type before unmarshaling intomap[string]any. Not blocking — totally fine to keep as-is.🤖 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/fleet/vpp.go` around lines 255 - 283, The validator currently unmarshals plist directly into map[string]any and returns the raw "cannot unmarshal array into Go value of type map[string]interface {}" error for non-dict roots; change that code to first unmarshal into an any (interface{}) value, switch on its concrete type, and if the root is not map[string]any return a clear, user-facing error like "plist root must be a <dict>, got <array>" (or include the actual concrete type), otherwise cast/convert the value to map[string]any and continue (so keep using findUnsupportedFleetVar and other logic 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.
Inline comments:
In `@server/fleet/vpp.go`:
- Around line 62-67: The Configuration field was changed from json.RawMessage to
[]byte in VPPAppTeam, VPPAppStoreApp, and AppStoreAppUpdatePayload which causes
responses to be base64-encoded; either revert Configuration back to
json.RawMessage on those types or implement custom MarshalJSON/UnmarshalJSON on
each type (or a shared wrapper type) to emit/accept raw JSON/XML to preserve the
original wire shape; then ensure request types addAppStoreAppRequest,
updateAppStoreAppRequest, and VPPBatchPayload remain compatible and update API
docs and downstream consumers (UI, fleetctl/gitops, Terraform, REST clients) to
handle the chosen format.
---
Nitpick comments:
In `@server/fleet/vpp.go`:
- Around line 255-283: The validator currently unmarshals plist directly into
map[string]any and returns the raw "cannot unmarshal array into Go value of type
map[string]interface {}" error for non-dict roots; change that code to first
unmarshal into an any (interface{}) value, switch on its concrete type, and if
the root is not map[string]any return a clear, user-facing error like "plist
root must be a <dict>, got <array>" (or include the actual concrete type),
otherwise cast/convert the value to map[string]any and continue (so keep using
findUnsupportedFleetVar and other logic unchanged).
🪄 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: 08ab203f-d961-41d1-a9d6-886926032faf
📒 Files selected for processing (5)
server/datastore/mysql/android_test.goserver/datastore/mysql/vpp_test.goserver/fleet/vpp.goserver/fleet/vpp_test.goserver/service/integration_android_software_test.go
| // Configuration is the managed app configuration payload. JSON for Android, | ||
| // XML for iOS / iPadOS. | ||
| Configuration []byte `json:"configuration,omitempty"` | ||
| AutoUpdateEnabled *bool `json:"-"` | ||
| AutoUpdateStartTime *string `json:"-"` | ||
| AutoUpdateEndTime *string `json:"-"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find every place that reads/writes app_store_app.configuration in non-Go code (UI, gitops, terraform, docs).
fd -t f -e ts -e tsx -e js -e jsx -e py -e go -e md -e yml -e yaml | xargs rg -n --no-messages 'app_store_app[^a-zA-Z0-9_]*\.?configuration|"configuration"\s*:' | rg -v '_test\.go|integration_.*_test\.go' | head -200
# 2) Confirm the request-side types are still json.RawMessage (asymmetry check).
rg -nP --type=go -C2 '\b(addAppStoreAppRequest|updateAppStoreAppRequest|VPPBatchPayload)\b' | rg -nP -C2 'Configuration'
# 3) See whether any place attaches a custom (Un)MarshalJSON to VPPAppStoreApp or VPPAppTeam to neutralize the wire-format change.
ast-grep --pattern $'func ($_ $_VPPAppStoreApp) MarshalJSON() ($_, $_) { $$$ }'
ast-grep --pattern $'func ($_ $_VPPAppTeam) MarshalJSON() ($_, $_) { $$$ }'
ast-grep --pattern $'func ($_ $_AppStoreAppUpdatePayload) MarshalJSON() ($_, $_) { $$$ }'Repository: fleetdm/fleet
Length of output: 9918
🏁 Script executed:
# Get the current state of VPPAppStoreApp and related structures
sed -n '50,80p' server/fleet/vpp.go | cat -nRepository: fleetdm/fleet
Length of output: 1535
🏁 Script executed:
# Get more context around the mentioned lines
sed -n '55,75p' server/fleet/vpp.go | cat -nRepository: fleetdm/fleet
Length of output: 1028
🏁 Script executed:
# Search for the request-side type definitions
rg -n 'type (addAppStoreAppRequest|updateAppStoreAppRequest|VPPBatchPayload)' --type=go -A 15Repository: fleetdm/fleet
Length of output: 6038
🏁 Script executed:
# Look for marshal implementations on the response types
rg -n 'func.*VPPAppStoreApp.*Marshal' --type=goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check the specific test line 419 mentioned in the comment
sed -n '410,430p' server/service/integration_android_software_test.go | cat -nRepository: fleetdm/fleet
Length of output: 995
🏁 Script executed:
# Check the other affected lines mentioned (135-137, 196-205)
sed -n '130,210p' server/fleet/vpp.go | cat -nRepository: fleetdm/fleet
Length of output: 4022
🏁 Script executed:
# Search for any related API documentation or changelog
fd -i -name '*.md' | xargs rg -l 'app.store|vpp|configuration' | head -10Repository: fleetdm/fleet
Length of output: 623
🏁 Script executed:
# Check if there are any changelog or migration notes
find . -type f \( -name 'CHANGELOG*' -o -name 'changelog*' -o -name 'MIGRATION*' \) | head -5Repository: fleetdm/fleet
Length of output: 214
🏁 Script executed:
# Look for TypeScript/JavaScript client code that might handle this
fd -type f \( -e ts -e tsx -e js -e jsx \) | xargs rg -l 'configuration' | grep -i client | head -10Repository: fleetdm/fleet
Length of output: 589
🏁 Script executed:
# Look for recent changes to vpp.go to understand the intent
git log --oneline -20 -- server/fleet/vpp.go 2>/dev/null || echo "Git history not available"Repository: fleetdm/fleet
Length of output: 95
🏁 Script executed:
# Search for any TODOs or FIXMEs related to Configuration
rg -n 'TODO|FIXME' server/fleet/vpp.go server/service/vpp.goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look for any frontend/TypeScript references to configuration in app store app context
rg -n 'appStoreApp|app_store_app' --type=ts --type=tsx --type=js --type=jsx | head -20Repository: fleetdm/fleet
Length of output: 85
🏁 Script executed:
# Check if there are any API integration tests that validate the wire format
rg -n 'Configuration.*base64|base64.*Configuration' --type=goRepository: fleetdm/fleet
Length of output: 513
🏁 Script executed:
# Look for JSON marshaling tests
rg -n 'json.Marshal.*Configuration|Configuration.*json.Marshal' --type=go | head -10Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look at how Configuration is handled in responses - find the handler that returns it
rg -n 'getSoftwareTitle|software/titles' server/service/vpp.go -A 10 | head -40Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check if there's any special handling when retrieving Configuration from DB
rg -n 'Configuration' server/datastore/mysql/vpp.go -B 2 -A 2 | head -50Repository: fleetdm/fleet
Length of output: 1959
🏁 Script executed:
# Look for how the response is constructed
rg -n 'type.*Response.*struct' server/service/vpp.go -A 15 | grep -A 15 'getSoftwareTitleResponse\|getSoftwareTitle'Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check how configuration is serialized when storing/retrieving
rg -n 'db:.*configuration|json:.*configuration' server/fleet/vpp.go --ignore-caseRepository: fleetdm/fleet
Length of output: 181
🏁 Script executed:
# Check git blame for when Configuration changed from json.RawMessage to []byte
git log -p --all -- server/fleet/vpp.go | grep -B 20 -A 5 'Configuration.*\[\]byte' | head -60Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look for PR/commit messages mentioning this change
git log --all --grep='Configuration\|json.RawMessage\|\[\]byte' --oneline | head -20Repository: fleetdm/fleet
Length of output: 1341
🏁 Script executed:
# Check if there's documentation about the wire format
fd -type f -name '*.md' | xargs rg -l 'software.*title\|app.*store' | head -5Repository: fleetdm/fleet
Length of output: 228
🏁 Script executed:
# Search for any UI code consuming the configuration endpoint
find . -type f \( -name '*.ts' -o -name '*.js' \) -path '*/frontend/*' -o -path '*/web/*' | xargs rg -l 'configuration' 2>/dev/null | head -10Repository: fleetdm/fleet
Length of output: 442
🏁 Script executed:
# Check the frontend software interface to see how configuration is handled
cat frontend/interfaces/software.ts | head -150Repository: fleetdm/fleet
Length of output: 4564
🏁 Script executed:
# Look for any handling of app_store_app in the frontend
rg -n 'app_store_app\|appStoreApp' frontend/ | head -30Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check if there's any base64 encoding/decoding in the frontend
rg -n 'btoa\|atob\|base64' frontend/services/entities/software.ts | head -20Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look at the actual git commit that changed the type
git log --all -p -- server/fleet/vpp.go | grep -B 30 -A 5 'Configuration.*\[\]byte.*json' | head -80Repository: fleetdm/fleet
Length of output: 4311
🏁 Script executed:
# Try to find when this change was introduced
git log --all --pretty=format:"%H %s" -- server/fleet/vpp.go | head -30Repository: fleetdm/fleet
Length of output: 534
🏁 Script executed:
# Look at the specific commit that changed Configuration to []byte
git show 1da4c58d --statRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
# Get the full commit message
git show 1da4c58d --no-patch --format=fullerRepository: fleetdm/fleet
Length of output: 317
🏁 Script executed:
# Check if there are any issues or PRs related to this
rg -n 'plist\|xml.*format\|base64' server/fleet/vpp.go server/service/vpp.go | head -20Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look at the actual implementation of how Configuration is stored/retrieved
sed -n '100,150p' server/datastore/mysql/vpp.go | cat -nRepository: fleetdm/fleet
Length of output: 1929
🏁 Script executed:
# Check if the test actually validates the wire format properly
sed -n '410,450p' server/service/integration_android_software_test.go | cat -nRepository: fleetdm/fleet
Length of output: 2032
🏁 Script executed:
# Check what the commit actually changed - look at the diff for the relevant lines
git show 1da4c58d -- server/fleet/vpp.go | grep -A 5 -B 5 'Configuration'Repository: fleetdm/fleet
Length of output: 2787
🏁 Script executed:
# Search for any issues or PRs referencing this change
rg -n 'base64|RawMessage.*byte|byte.*RawMessage' server/fleet/vpp.go server/service/vpp.goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check if there are tests that specifically validate JSON marshaling
rg -n 'json.Marshal.*Configuration\|MarshalJSON' server/service/ --type=go -B 2 -A 2 | head -40Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look at the Android configuration storage to understand how it flows
rg -n 'GetAndroidAppConfiguration\|updateAndroidAppConfigurationTx' server/datastore/mysql/android* --type=go -A 10 | head -60Repository: fleetdm/fleet
Length of output: 39
Confirm this wire format change is intentional and verify downstream consumers have been updated.
Configuration was changed from json.RawMessage to []byte in VPPAppTeam, VPPAppStoreApp, and AppStoreAppUpdatePayload response types. This introduces a breaking wire format change:
- Requests still accept raw JSON via
json.RawMessagein addAppStoreAppRequest, updateAppStoreAppRequest, and VPPBatchPayload - Responses now emit base64-encoded bytes instead of raw JSON/XML, since
encoding/jsonbase64-encodes[]bytefields - No custom
MarshalJSON/UnmarshalJSONimplementations exist to preserve the previous wire shape
Go integration tests pass because Go's decoder accepts both formats transparently. However, non-Go consumers (UI, fleetctl/gitops, Terraform, REST clients) will receive base64-encoded configuration instead of the expected raw JSON/XML structure, requiring code changes.
Ensure this change is intentional and that API documentation, UI code, and any clients consuming the software-titles endpoints have been updated to handle the new base64-encoded format. If backward compatibility is required, implement custom marshal/unmarshal methods to preserve the original wire format.
🤖 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/fleet/vpp.go` around lines 62 - 67, The Configuration field was
changed from json.RawMessage to []byte in VPPAppTeam, VPPAppStoreApp, and
AppStoreAppUpdatePayload which causes responses to be base64-encoded; either
revert Configuration back to json.RawMessage on those types or implement custom
MarshalJSON/UnmarshalJSON on each type (or a shared wrapper type) to emit/accept
raw JSON/XML to preserve the original wire shape; then ensure request types
addAppStoreAppRequest, updateAppStoreAppRequest, and VPPBatchPayload remain
compatible and update API docs and downstream consumers (UI, fleetctl/gitops,
Terraform, REST clients) to handle the chosen format.
jkatz01
left a comment
jkatz01
left a comment
There was a problem hiding this comment.
Thanks for improving this! The validation looks pretty thorough.
Part of #38790. Stacked on top of #44930. Closes #43964. Adds VPP and in-house datastore methods (`GetVPPAppConfiguration`, `BulkGetVPPAppConfigurations`, `DeleteVPPAppConfiguration`, `HasVPPAppConfigurationChanged`, plus in-house equivalents) keyed on the merged `vpp_app_configurations` and `in_house_app_configurations` tables. Wires them into `InsertVPPAppWithTeam`, `SaveInHouseAppUpdates`, and the team / app removal paths. Two follow-up bug fixes folded into this branch: 1. iPadOS in-house apps received no configuration: a single `.ipa` upload creates two `in_house_apps` rows but config was only stored against the iOS row's id, so iPadOS lookups returned NotFound. Now writes to both sibling rows on insert and propagates updates / clears via `installerIDsForInHouseAppSibling`. 2. Single-app PATCH with `"configuration": null` was inserting empty bytes instead of deleting; aligned the iOS/iPadOS branch in `InsertVPPAppWithTeam` with the batch path's `len > 0` upsert / `len == 0` delete semantics. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for managing in-house app configurations including storage, updates, and deletion. * Expanded VPP app configuration support for iOS/iPadOS platforms with configuration storage and change detection. * **Refactor** * Updated Android app configuration handling to improve internal data consistency and streamline configuration management workflows across all app types. [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/44931) <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: jkatz01 <yehonatankatz@gmail.com>
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/44931){kind=link}
Part of #38790 (iOS / iPadOS managed app configuration).
Closes #43963.
Adds
ValidateAppleAppConfigurationand theFleetVarsSupportedInAppleAppConfigallow-list inserver/fleet/vpp.go. Walks the decoded plist (keys + string values) so XML-entity-encoded$FLEET_VAR_*tokens can't slip past the disallow check, and rejects non-XML plist formats (binary, OpenStep, GNUStep) since Apple'sInstallApplicationonly accepts XML.Stacked PRs (review bottom up):
Summary by CodeRabbit
New Features
Bug Fixes
Tests