macOS managed local account foundations - #43381
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43381 +/- ##
==========================================
+ Coverage 65.07% 65.09% +0.01%
==========================================
Files 2603 2603
Lines 253139 253641 +502
Branches 9242 9218 -24
==========================================
+ Hits 164740 165115 +375
- Misses 75638 75727 +89
- Partials 12761 12799 +38
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:
|
16 tasks
cdcme
marked this pull request as ready for review
April 21, 2026 00:46
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
server/service/hosts.go (1)
4048-4080: The verified status check is properly implemented in the EE override.The
GetHostManagedAccountPasswordimplementation inee/server/service/hosts.go(lines 740–742) correctly enforces the "verified" status gate, returning an error when the managed account status is not yet verified. Authorization is handled at line 713.Add a test case for the non-verified status path. The existing
TestGetHostManagedAccountPasswordAuthtest covers authorization scenarios but always mocks a verified status. A test case should be added to verify that the function returns the error message "Host's managed account password is not yet verified." when the status is pending or failed, ensuring callers receive an empty/error payload until the account is confirmed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/hosts.go` around lines 4048 - 4080, Add a unit test case to TestGetHostManagedAccountPasswordAuth that simulates a non-verified managed account (pending or failed) and asserts that calling GetHostManagedAccountPassword returns an error with message "Host's managed account password is not yet verified." and does not return a ManagedLocalAccount payload; specifically, update the test to mock the datastore/EE override behavior so GetHostManagedAccountPassword on the Service under test exercises the non-verified branch and validate both the error string and that the returned password is nil/empty.server/fleet/datastore.go (1)
1649-1652: Consider renaming for clarity: method returns a*Host, not an account.
GetManagedLocalAccountByCommandUUIDreturns*Hostbut the name suggests it returns a managed-local-account entity. Something likeGetHostByManagedLocalAccountCommandUUIDwould better reflect the return type and match the surrounding naming style (e.g.,GetHostManagedLocalAccount*). Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/fleet/datastore.go` around lines 1649 - 1652, The method name GetManagedLocalAccountByCommandUUID is misleading because it returns *Host; rename it to GetHostByManagedLocalAccountCommandUUID (or similar consistent with GetHostManagedLocalAccount* naming) and update all declarations, implementations, and call sites to use the new name (e.g., interface definition, concrete datastore struct method, any tests, and consumers) so the signature remains the same but the name reflects the returned Host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/mdm.go`:
- Around line 275-291: These two macOS setup fields
(payload.EnableManagedLocalAccount and payload.EndUserLocalAccountType) must be
blocked when MDM is not enabled; update the validation in
ee/server/service/mdm.go to check the MDM enabled flag (use the existing
ac.MDM.Enabled or equivalent) before applying changes to ac.MDM.MacOSSetup, and
return the same kind of validation error used elsewhere (e.g.,
fleet.NewInvalidArgumentError) if MDM is not enabled; ensure both the
EnableManagedLocalAccount and EndUserLocalAccountType branches perform this
MDM-on guard before mutating ac.MDM.MacOSSetup and before setting
didUpdate/didUpdateManagedLocalAccount.
In `@ee/server/service/teams.go`:
- Around line 2085-2091: The current change detection treats nil → false as a
state change and triggers svc.updateMacOSSetupEnableManagedLocalAccount (which
emits ActivityTypeDisabledManagedLocalAccount); modify the conditional around
tm.Config.MDM.MacOSSetup.EnableManagedLocalAccount and
payload.EnableManagedLocalAccount so that you only consider it an update if the
previous value is non‑nil and different or the new value is true (i.e. skip
treating nil → false as a change). Update the logic that sets
didUpdateManagedLocalAccount/didUpdate accordingly and mirror the same nil→false
no‑op behavior in the corresponding app‑config path (see
ee/server/service/mdm.go) to avoid emitting a “disabled” activity for a feature
that was never enabled.
In `@server/fleet/service.go`:
- Around line 1348-1350: Update the Go doc comment for
GetHostManagedAccountPassword to refer to "host ID" instead of "host UUID":
locate the comment above the GetHostManagedAccountPassword(ctx context.Context,
hostID uint) signature and replace any mention of "host UUID" or "UUID" with
"host ID" (and ensure wording still reads naturally, e.g., "for the given host
ID only if it has a verified status").
In `@server/service/integration_mdm_test.go`:
- Around line 23388-23390: The test reuses pwdResp across two s.DoJSON calls and
can pass due to stale data; reinitialize/reset pwdResp to its zero value (the
same response type used earlier) before the second s.DoJSON call that fetches
"/api/latest/fleet/hosts/{id}/managed_account_password", then perform
require.NotNil(t, pwdResp.ManagedLocalAccount) to assert the password is still
present.
In `@server/worker/apple_mdm_test.go`:
- Around line 1549-1592: The test's current assertions are too loose: they can
be satisfied by unrelated <true /> or generic substrings and don't verify proper
plist structure or XML-escaping; update the assertions in the test around
mdmWorker.sendManagedAccounts / rawCommand to (1) assert the exact key/value
plist structure for LockPrimaryAccountInfo by checking the sequence
"<key>LockPrimaryAccountInfo</key>" immediately followed by "<true/>" or "<true
/>" (ensure adjacency, not just separate contains), (2) assert the
AutoSetupAdminAccounts entry as a complete <array><dict>…</dict></array> for the
admin account by checking the specific keys in order (e.g.,
"<key>Username</key><string>_fleetadmin</string>",
"<key>FullName</key><string>Fleet Admin</string>",
"<key>PasswordHash</key><data>…</data>"), and (3) include tests using
XML-escaped names or assert against xml.EscapeString(username) /
xml.EscapeString(fullname) to ensure names with &, <, > are emitted correctly;
reference the sendManagedAccounts call, appleMDMArgs, mdmWorker, and rawCommand
to locate the assertions to tighten.
In `@server/worker/apple_mdm.go`:
- Line 5: AccountConfiguration currently interpolates raw strings (fullName,
Username, admin account names) into plist XML; escape those values before
inserting into the XML to prevent broken/ malicious plist keys. Update the code
that builds AccountConfiguration (where fullName/Username/admin are concatenated
into the plist) to call an escaping function such as html.EscapeString on each
user-supplied value (or use encoding/xml EscapeText when writing via an
io.Writer) and use the escaped variables in the template; apply the same fix to
the other AccountConfiguration assembly site referenced in the review.
- Around line 221-235: The SSO payload is currently conditioned only on
ssoAccount being non-nil, which causes
PrimaryAccountFullName/PrimaryAccountUserName/LockPrimaryAccountInfo to be
emitted even when SSO is disabled; update the logic in apple_mdm.go (the block
that sets ssoAccount, ssoEnabled, lockPrimaryAccountInfo and any downstream
callers such as sendManagedAccounts or the function that builds the SSO payload)
to require both ssoAccount != nil AND ssoEnabled == true before populating or
emitting SSO-related fields; ensure you apply the same gating fix in the other
occurrences mentioned (the blocks around lines 256-278 and 923-937) so SSO
fields are only included when the feature is enabled for the team/org via
getTeamConfig and appCfg checks.
---
Nitpick comments:
In `@server/fleet/datastore.go`:
- Around line 1649-1652: The method name GetManagedLocalAccountByCommandUUID is
misleading because it returns *Host; rename it to
GetHostByManagedLocalAccountCommandUUID (or similar consistent with
GetHostManagedLocalAccount* naming) and update all declarations,
implementations, and call sites to use the new name (e.g., interface definition,
concrete datastore struct method, any tests, and consumers) so the signature
remains the same but the name reflects the returned Host.
In `@server/service/hosts.go`:
- Around line 4048-4080: Add a unit test case to
TestGetHostManagedAccountPasswordAuth that simulates a non-verified managed
account (pending or failed) and asserts that calling
GetHostManagedAccountPassword returns an error with message "Host's managed
account password is not yet verified." and does not return a ManagedLocalAccount
payload; specifically, update the test to mock the datastore/EE override
behavior so GetHostManagedAccountPassword on the Service under test exercises
the non-verified branch and validate both the error string and that the returned
password is nil/empty.
🪄 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: cfa96d66-5ae2-4481-937f-03e8df7457a1
📒 Files selected for processing (24)
cmd/fleetctl/fleetctl/testing_utils/testing_utils.goee/server/service/hosts.goee/server/service/hosts_test.goee/server/service/mdm.goee/server/service/teams.goserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/service.goserver/mdm/apple/commander.goserver/mdm/apple/commander_test.goserver/mdm/apple/util.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/handler.goserver/service/hosts.goserver/service/hosts_test.goserver/service/integration_mdm_test.goserver/worker/apple_mdm.goserver/worker/apple_mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/mdm/apple/commander.go
- server/datastore/mysql/managed_local_account.go
- server/datastore/mysql/managed_local_account_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/worker/apple_mdm.go (1)
929-961:⚠️ Potential issue | 🟠 MajorEscape plist string values before interpolating XML.
This still interpolates
fullName,ssoAccount.Username, and admin account names directly into XML. Values containing&,<, or</string>can break or inject plist content. Use XML escaping before formatting string values into the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/worker/apple_mdm.go` around lines 929 - 961, The XML payloads interpolate unescaped user-controlled strings (fullName, ssoAccount.Username, adminAccount.ShortName, adminAccount.FullName) which can break or inject plist XML; before building ssoAccountPayload and managedAccountPayload (the fmt.Sprintf calls in apple_mdm.go and the getIdPDisplayName usage), escape those values for XML (e.g. add a helper escapeXML(s string) that uses encoding/xml.EscapeText or a small replacer for & < > ' " and call it on fullName, ssoAccount.Username, adminAccount.ShortName, and adminAccount.FullName) and use the escaped values in the fmt.Sprintf arguments.
🧹 Nitpick comments (1)
ee/server/service/mdm.go (1)
283-291: Consider normalizing/validatingEndUserLocalAccountTypemore defensively.A couple of small concerns worth confirming:
- The check is case-sensitive;
"Admin"or"ADMIN"will be rejected. Considerstrings.EqualFold(or lower-casing before compare/store) if the API should be tolerant.EndUserLocalAccountTypecan be set whileEnableManagedLocalAccountisnil/false, which stores a value that has no effect. If the intent is that the type only applies when managed local account is enabled, either couple the validation (require enabled) or document that the type is persisted independently.- No activity is emitted when this field changes, unlike the adjacent managed-local-account toggle. Confirm this is intentional (e.g., covered by the enable/disable activity alone).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/server/service/mdm.go` around lines 283 - 291, Normalize and validate EndUserLocalAccountType more defensively: treat payload.EndUserLocalAccountType case-insensitively (e.g., use strings.EqualFold or lower-case before comparing/storing) and only accept the supported value "admin"; additionally, if the intent is that this field only applies when EnableManagedLocalAccount is true, enforce that by validating that payload.EnableManagedLocalAccount (or ac.MDM.MacOSSetup.EnableManagedLocalAccount) is true before persisting payload.EndUserLocalAccountType (return an InvalidArgumentError otherwise) or document/persist intentionally; finally, when you update ac.MDM.MacOSSetup.EndUserLocalAccountType set didUpdate = true as now and also emit the same activity/event used for the managed-local-account toggle so changes to EndUserLocalAccountType are captured (reference payload.EndUserLocalAccountType, ac.MDM.MacOSSetup.EndUserLocalAccountType, EnableManagedLocalAccount, didUpdate and the existing activity-emission path for managed local account).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/server/service/mdm.go`:
- Around line 275-281: When comparing the current stored value
ac.MDM.MacOSSetup.EnableManagedLocalAccount to
payload.EnableManagedLocalAccount, treat a nil stored value as false so a
first-time write of explicit false does not count as a change; compute oldBool
:= (ac.MDM.MacOSSetup.EnableManagedLocalAccount != nil &&
*ac.MDM.MacOSSetup.EnableManagedLocalAccount) and newBool :=
*payload.EnableManagedLocalAccount, then only set
ac.MDM.MacOSSetup.EnableManagedLocalAccount, didUpdateManagedLocalAccount and
didUpdate when oldBool != newBool; use these symbols to find and update the
logic in the existing block that currently checks pointers for inequality.
In `@server/worker/apple_mdm.go`:
- Around line 256-287: Before generating a new managed-admin password in
runPostDEPEnrollment, first check for an existing pending escrow record for this
host and reuse it if present: call a Datastore method (e.g.,
GetHostManagedLocalAccount or add one) to fetch an existing entry for
args.HostUUID and if that entry has a pending status/NULL status and contains a
password and cmd UUID, set password and cmdUUID from that record instead of
calling apple_mdm.GenerateManagedAccountPassword(); only when no pending record
exists generate the password, hash it via
apple_mdm.GenerateSaltedSHA512PBKDF2Hash, create the AdminAccount, and call a
SaveHostManagedLocalAccount path that inserts the new record without clobbering
an existing pending record (or use an upsert that preserves status if
duplicate). Ensure sendManagedAccounts is then enqueued using the reused or
newly created cmdUUID so the escrowed secret remains consistent across retries.
---
Duplicate comments:
In `@server/worker/apple_mdm.go`:
- Around line 929-961: The XML payloads interpolate unescaped user-controlled
strings (fullName, ssoAccount.Username, adminAccount.ShortName,
adminAccount.FullName) which can break or inject plist XML; before building
ssoAccountPayload and managedAccountPayload (the fmt.Sprintf calls in
apple_mdm.go and the getIdPDisplayName usage), escape those values for XML (e.g.
add a helper escapeXML(s string) that uses encoding/xml.EscapeText or a small
replacer for & < > ' " and call it on fullName, ssoAccount.Username,
adminAccount.ShortName, and adminAccount.FullName) and use the escaped values in
the fmt.Sprintf arguments.
---
Nitpick comments:
In `@ee/server/service/mdm.go`:
- Around line 283-291: Normalize and validate EndUserLocalAccountType more
defensively: treat payload.EndUserLocalAccountType case-insensitively (e.g., use
strings.EqualFold or lower-case before comparing/storing) and only accept the
supported value "admin"; additionally, if the intent is that this field only
applies when EnableManagedLocalAccount is true, enforce that by validating that
payload.EnableManagedLocalAccount (or
ac.MDM.MacOSSetup.EnableManagedLocalAccount) is true before persisting
payload.EndUserLocalAccountType (return an InvalidArgumentError otherwise) or
document/persist intentionally; finally, when you update
ac.MDM.MacOSSetup.EndUserLocalAccountType set didUpdate = true as now and also
emit the same activity/event used for the managed-local-account toggle so
changes to EndUserLocalAccountType are captured (reference
payload.EndUserLocalAccountType, ac.MDM.MacOSSetup.EndUserLocalAccountType,
EnableManagedLocalAccount, didUpdate and the existing activity-emission path for
managed local account).
🪄 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: fcaa5dcb-24bb-453f-bff5-039aea1a997e
📒 Files selected for processing (4)
ee/server/service/mdm.goserver/fleet/service.goserver/service/integration_mdm_test.goserver/worker/apple_mdm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/service/integration_mdm_test.go
9 tasks
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/hosts.go (1)
4059-4063: Nit: field name doesn't reflect the value it holds.
ManagedLocalAccount *fleet.HostManagedLocalAccountPasswordwith JSON tagmanaged_account_passwordis a bit misleading — the field holds a password payload, not an account struct. Consider renaming the Go field toManagedAccountPassword(orPassword) to align with both the JSON tag and the underlying type. Purely cosmetic; doesn't affect the wire contract.✏️ Suggested rename
type getHostManagedAccountPasswordResponse struct { HostID uint `json:"host_id"` - ManagedLocalAccount *fleet.HostManagedLocalAccountPassword `json:"managed_account_password"` + ManagedAccountPassword *fleet.HostManagedLocalAccountPassword `json:"managed_account_password"` Err error `json:"error,omitempty"` }And update the corresponding field initializer in
getHostManagedAccountPasswordEndpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/hosts.go` around lines 4059 - 4063, Rename the struct field ManagedLocalAccount in getHostManagedAccountPasswordResponse to a clearer name such as ManagedAccountPassword (or Password) to match the JSON tag "managed_account_password" and the underlying type *fleet.HostManagedLocalAccountPassword; update the field reference and initializer in getHostManagedAccountPasswordEndpoint so it populates the renamed field (and any other places that construct or access getHostManagedAccountPasswordResponse) to avoid build errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/hosts.go`:
- Around line 4059-4063: Rename the struct field ManagedLocalAccount in
getHostManagedAccountPasswordResponse to a clearer name such as
ManagedAccountPassword (or Password) to match the JSON tag
"managed_account_password" and the underlying type
*fleet.HostManagedLocalAccountPassword; update the field reference and
initializer in getHostManagedAccountPasswordEndpoint so it populates the renamed
field (and any other places that construct or access
getHostManagedAccountPasswordResponse) to avoid build errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1dc14be1-fe69-465e-8e2d-32d3246bd540
📒 Files selected for processing (2)
server/service/hosts.goserver/service/hosts_test.go
This was referenced Apr 21, 2026
JordanMontgomery
left a comment
JordanMontgomery
left a comment
Member
There was a problem hiding this comment.
Overall looks great. Some questions/comments
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/worker/apple_mdm.go (1)
255-286:⚠️ Potential issue | 🟠 MajorMake the managed-admin escrow path retry-safe.
Line 257 generates a new command UUID and Line 272 saves a new password before
AccountConfigurationenqueue/push has fully completed. If the job retries after the command was enqueued or after push fails, the upsert inserver/datastore/mysql/managed_local_account.gooverwrites the escrowed password and command UUID, while the device may still process the earlier command/hash. Reuse an existing pending escrow record for the host, or make the save/enqueue path insert-only for pending records so retries keep the same password, hash, and command UUID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/worker/apple_mdm.go` around lines 255 - 286, The current flow generates a new cmdUUID and saves a new managed-admin password/hash with SaveHostManagedLocalAccount before the AccountConfiguration push, which allows retries to overwrite an existing pending escrow; change this so retries reuse an existing pending escrow or make the save operation insert-only. Concretely: before calling apple_mdm.GenerateManagedAccountPassword / GenerateSaltedSHA512PBKDF2Hash and before assigning cmdUUID in the function that calls sendManagedAccounts, query the datastore for an existing pending managed-local-account record for args.HostUUID and if found reuse its plaintext/passwordHash/cmdUUID; if none exists, generate password/hash and persist using a new insert-only datastore method (or modify SaveHostManagedLocalAccount to return a conflict instead of upserting) so subsequent retries do not overwrite the original escrow. Ensure sendManagedAccounts is still passed the reused or newly created cmdUUID.server/mdm/apple/commander.go (1)
359-386:⚠️ Potential issue | 🟠 MajorEscape account string fields before interpolating XML.
Lines 360-386 insert SSO/admin names directly into plist XML. IdP/SCIM values containing
&,<, or</string>can break the command or inject plist keys. Escape these fields, or marshal a typed plist payload instead of concatenating XML.Proposed direction
import ( "context" "encoding/base64" + "encoding/xml" "fmt" "net/http" "sort" "strings" @@ ) + +func escapeXMLText(s string) (string, error) { + var b strings.Builder + if err := xml.EscapeText(&b, []byte(s)); err != nil { + return "", err + } + return b.String(), nil +} @@ if ssoAccount != nil { + fullName, err := escapeXMLText(ssoAccount.FullName) + if err != nil { + return ctxerr.Wrap(ctx, err, "escaping SSO full name") + } + userName, err := escapeXMLText(ssoAccount.UserName) + if err != nil { + return ctxerr.Wrap(ctx, err, "escaping SSO user name") + } payload += fmt.Sprintf(` <key>PrimaryAccountFullName</key> <string>%s</string> @@ <key>LockPrimaryAccountInfo</key> <%t /> -`, ssoAccount.FullName, ssoAccount.UserName, ssoAccount.LockPrimaryAccountInfo) +`, fullName, userName, ssoAccount.LockPrimaryAccountInfo) } if adminAccount != nil { passwordHashEncoded := base64.StdEncoding.EncodeToString(adminAccount.PasswordHash) + shortName, err := escapeXMLText(adminAccount.ShortName) + if err != nil { + return ctxerr.Wrap(ctx, err, "escaping admin short name") + } + fullName, err := escapeXMLText(adminAccount.FullName) + if err != nil { + return ctxerr.Wrap(ctx, err, "escaping admin full name") + } payload += fmt.Sprintf(` @@ <key>fullName</key> <string>%s</string> </dict> </array> -`, adminAccount.Hidden, passwordHashEncoded, adminAccount.ShortName, adminAccount.FullName) +`, adminAccount.Hidden, passwordHashEncoded, shortName, fullName) }Please also verify with a unit case containing XML metacharacters, e.g.
FullName: "A & B <admin>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/mdm/apple/commander.go` around lines 359 - 386, The code concatenates raw account strings into plist XML in commander.go (the blocks that reference ssoAccount.FullName, ssoAccount.UserName, ssoAccount.LockPrimaryAccountInfo and the adminAccount block with adminAccount.ShortName, adminAccount.FullName), which allows XML metacharacters like & and < to break or inject XML; fix by escaping these values before interpolation (e.g., run the string fields through an XML-escaping function such as xml.EscapeText or equivalent) or, better, construct a typed plist structure and marshal it with a plist encoder instead of string concatenation; add a unit test that sets FullName to a value with metacharacters (e.g., "A & B <admin>") and asserts the produced plist is well-formed and contains the escaped/encoded value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/mdm/apple/commander.go`:
- Around line 359-386: The code concatenates raw account strings into plist XML
in commander.go (the blocks that reference ssoAccount.FullName,
ssoAccount.UserName, ssoAccount.LockPrimaryAccountInfo and the adminAccount
block with adminAccount.ShortName, adminAccount.FullName), which allows XML
metacharacters like & and < to break or inject XML; fix by escaping these values
before interpolation (e.g., run the string fields through an XML-escaping
function such as xml.EscapeText or equivalent) or, better, construct a typed
plist structure and marshal it with a plist encoder instead of string
concatenation; add a unit test that sets FullName to a value with metacharacters
(e.g., "A & B <admin>") and asserts the produced plist is well-formed and
contains the escaped/encoded value.
In `@server/worker/apple_mdm.go`:
- Around line 255-286: The current flow generates a new cmdUUID and saves a new
managed-admin password/hash with SaveHostManagedLocalAccount before the
AccountConfiguration push, which allows retries to overwrite an existing pending
escrow; change this so retries reuse an existing pending escrow or make the save
operation insert-only. Concretely: before calling
apple_mdm.GenerateManagedAccountPassword / GenerateSaltedSHA512PBKDF2Hash and
before assigning cmdUUID in the function that calls sendManagedAccounts, query
the datastore for an existing pending managed-local-account record for
args.HostUUID and if found reuse its plaintext/passwordHash/cmdUUID; if none
exists, generate password/hash and persist using a new insert-only datastore
method (or modify SaveHostManagedLocalAccount to return a conflict instead of
upserting) so subsequent retries do not overwrite the original escrow. Ensure
sendManagedAccounts is still passed the reused or newly created cmdUUID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aadeae2c-a952-4f54-81f3-57b6e0d7beae
📒 Files selected for processing (6)
server/datastore/mysql/managed_local_account.goserver/mdm/apple/commander.goserver/mdm/apple/commander_test.goserver/service/apple_mdm.goserver/worker/apple_mdm.goserver/worker/apple_mdm_test.go
✅ Files skipped from review due to trivial changes (1)
- server/datastore/mysql/managed_local_account.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/worker/apple_mdm_test.go
Member
Author
|
@JordanMontgomery Thanks for the feedback! I knocked out all those items and answered the one question there that didn't require a code change. |
JordanMontgomery
approved these changes
Apr 22, 2026
This was referenced Apr 22, 2026
This was referenced Apr 30, 2026
Merged
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.
Implements both #42942 and #42943
Parent: #37141
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests