extra error message checks and correct escaping in error message - #50136
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughUpdated mobileconfig parsing to classify additional plist unmarshal failures as unescaped special-character errors and revised the escaping examples in the returned message. Extended Apple MDM tests with cases for unescaped payload content, identifiers, and names, including assertions for expected error text. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
This PR improves the error experience when parsing Apple MDM configuration profiles that contain unescaped XML special characters, and adds test coverage to ensure the intended friendly error is returned for common failure cases.
Changes:
- Expanded
ParseConfigProfileerror detection to recognize additional XML parsing failures that are likely caused by unescaped characters. - Updated the user-facing error message examples for escaping special characters.
- Added test cases that validate the friendly error message is returned for unescaped characters in the payload, identifier, and name.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/mdm/apple/mobileconfig/mobileconfig.go | Broadened parsing error matching and updated the friendly error message returned on parse failures. |
| server/fleet/apple_mdm_test.go | Added new test cases and assertions for the friendly error message when profiles include unescaped XML characters. |
Comments suppressed due to low confidence (1)
server/mdm/apple/mobileconfig/mobileconfig.go:172
payloadSummaryreturns the same friendly message but only triggers it for"illegal base64 data". SinceParseConfigProfilenow also detects other XML parse errors for unescaped characters, this function can still surface confusing raw parser errors for the same underlying issue. Consider matching the same additional error substrings here, and update the example escaping to the correct XML entities.
if strings.Contains(err.Error(), "illegal base64 data") {
return nil, errors.New("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → \\&, < → \\<) and try again.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 1
🤖 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/mdm/apple/mobileconfig/mobileconfig.go`:
- Around line 169-174: Update payloadSummary’s plist.Unmarshal error handling to
map “illegal base64 data”, “invalid character entity”, and “expected attribute
name in element” to the same XML-escaping user-facing error used by
ParseConfigProfile. Prefer extracting and reusing a shared helper so both
parsing paths remain consistent, while returning unrelated errors 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 Plus
Run ID: b6839441-72f3-4cd8-8f09-8fd52bc9ca81
📒 Files selected for processing (2)
server/fleet/apple_mdm_test.goserver/mdm/apple/mobileconfig/mobileconfig.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
server/fleet/apple_mdm_test.go:112
- These new test cases assert an error message that suggests escaping as
\\&/\\<, which is not valid XML escaping. If the backend message is corrected to use&/<, update these expected strings accordingly to avoid locking in incorrect guidance.
testName: "TestParseConfigProfileUnescapedCharsInPayload",
mobileconfig: MobileconfigForTest("ValidName", "ValidIdentifier", uuid.NewString(), `<string>Unescaped & < > ' "</string>`),
shouldFail: true,
errString: new("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again."),
},
{
testName: "TestParseConfigProfileUnescapedCharsInIdentifier",
mobileconfig: MobileconfigForTest("ValidName", "Valid<Identifier", uuid.NewString(), `<string>Valid</string>`),
shouldFail: true,
errString: new("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again."),
},
{
testName: "TestParseConfigProfileUnescapedCharsInName",
mobileconfig: MobileconfigForTest("Valid<Name", "ValidIdentifier", uuid.NewString(), `<string>Valid</string>`),
shouldFail: true,
errString: new("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again."),
},
server/mdm/apple/mobileconfig/mobileconfig.go:118
- The error message’s escape examples are incorrect for XML.
\&and\<are not valid XML escaping; the examples should show XML entities like&and<so admins can apply the correct fix.
if strings.Contains(err.Error(), "illegal base64 data") || strings.Contains(err.Error(), "invalid character entity") || strings.Contains(err.Error(), "expected attribute name in element") {
return nil, errors.New("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again.")
}
server/mdm/apple/mobileconfig/mobileconfig.go:173
- Same as above: the escape examples in this error message are not valid XML escaping. Use XML entities like
&and<so the guidance is accurate.
if strings.Contains(err.Error(), "illegal base64 data") || strings.Contains(err.Error(), "invalid character entity") || strings.Contains(err.Error(), "expected attribute name in element") {
return nil, errors.New("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again.")
}
georgekarrv
approved these changes
Jul 29, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
server/fleet/apple_mdm_test.go:105
- Same issue here:
new("...")is invalid in Go and will not compile; useptr.String(...)to create the*string.
errString: new("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again."),
server/fleet/apple_mdm_test.go:111
- Same issue here:
new("...")is invalid in Go and will not compile; useptr.String(...)to create the*string.
errString: new("The configuration profile contains special characters (&, <, >, ', \") that must be XML-escaped. Please escape them (e.g. & → &, < → <) and try again."),
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50136 +/- ##
=======================================
Coverage 68.08% 68.08%
=======================================
Files 3936 3936
Lines 250647 250674 +27
Branches 13275 13275
=======================================
+ Hits 170642 170673 +31
+ Misses 64693 64690 -3
+ Partials 15312 15311 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6 tasks
MagnusHJensen
added a commit
that referenced
this pull request
Jul 29, 2026
…50136) (#50140) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #40074 unreleased bug Cherry picks: #50136 # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#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. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved configuration profile validation for unescaped special characters in Apple payloads. * Error messages now consistently indicate when characters like `&` and `<` must be XML-escaped. * Updated error examples to show properly escaped guidance. * Expanded test coverage to verify the standardized XML-escaping error for additional failing scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Related issue: Resolves #40074 unreleased bug
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
&and<must be XML-escaped.