Windows MDM validation fixes#46029
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Actions performedFull review triggered. |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens validation of uploaded Windows MDM SyncML configuration profiles to prevent server-side acceptance of payloads that will later be rejected by Windows devices.
Changes:
- Reject non-element XML inputs (e.g., plain text or comment-only) by requiring at least one supported SyncML top-level element (
Replace,Add,Exec,Atomic). - Add LocURI format validation to require a
./prefix and disallow../path traversal sequences. - Update and extend unit tests, and add two changelog entries describing the validation fixes.
Reviewed changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/fleet/windows_mdm.go | Adds SyncML “has supported top-level element” check and new LocURI format validation helper. |
| server/fleet/windows_mdm_test.go | Updates existing fixtures for ./-prefixed LocURIs and adds new regression tests for the reported issues. |
| changes/42224-windows-locuri-format-validation | Changelog entry for LocURI validation. |
| changes/42219-windows-profile-syncml-validation | Changelog entry for SyncML top-level element validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR requires uploaded Windows MDM profiles to include at least one supported SyncML top-level element (, , , or ) unless the Fleet secret placeholder is present. It enforces LocURI rules during parsing: an empty is rejected; values starting with '/' are rejected (unless they start with './'); and any path segment containing '..' is rejected. Tests and changelog entries were added and fixtures adjusted to reflect the new validation. 🚥 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)
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: 2
🤖 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/windows_mdm.go`:
- Around line 141-143: The error string returned when v.sawValidTopLevel is
false does not match the required `#42219` contract; update the return value in
the branch that checks v.sawValidTopLevel (the code that currently returns
errors.New(...)) to exactly "The file should include valid SyncML XML with at
least one supported element." so the API/UI error text matches the spec.
- Around line 240-257: The current validateLocURIFormat function accepts any
path beginning with "./"; tighten it to only allow the OMA-DM roots by verifying
that trimmed strings either are empty or do not contain "../" and start with one
of the explicit prefixes "./Device/", "./User/", or "./Vendor/". Update
validateLocURIFormat to first trim and return nil for empty, keep the "../"
check, then replace the generic HasPrefix("./") check with a check that the
value HasPrefix one of the three allowed prefixes and return a specific error if
it doesn't match those roots.
🪄 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: 0f480363-b7dc-42ad-80db-6d141a0de392
📒 Files selected for processing (4)
changes/42219-windows-profile-syncml-validationchanges/42224-windows-locuri-format-validationserver/fleet/windows_mdm.goserver/fleet/windows_mdm_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46029 +/- ##
=======================================
Coverage 66.82% 66.82%
=======================================
Files 2754 2754
Lines 220137 220162 +25
Branches 11025 11025
=======================================
+ Hits 147114 147134 +20
- Misses 59727 59732 +5
Partials 13296 13296
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[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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/fleet/windows_mdm.go (1)
91-93: ⚡ Quick winValidateUserProvided doc comment no longer matches LocURI behavior.
The comment says LocURI “must start with
./”, butvalidateLocURIFormatand tests explicitly allow forms likeDevice/Vendor/.... Please align the comment with the implemented rule to avoid contract confusion.🤖 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/windows_mdm.go` around lines 91 - 93, Update the ValidateUserProvided doc comment in windows_mdm.go to match the implemented LocURI rules in validateLocURIFormat: remove the blanket "must start with './'" statement and state that LocURI may either be a relative OMA-DM path beginning with "./" or a device-style path like "Device/..." (and must not contain "../" traversal), and also note that LocURI values within <Target> must not be reserved Fleet-managed URIs; reference validateLocURIFormat and ValidateUserProvided so readers know where the enforced rules live.
🤖 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/windows_mdm.go`:
- Around line 114-117: The current check uses bytes.Contains(m.SyncML,
[]byte(ServerSecretPrefix)) which is too permissive; change
validator.containsServerSecret to only match the secret placeholder when it
appears with the expected sigil (e.g. b"$"+ServerSecretPrefix) or otherwise
validate the surrounding SyncML token boundary (so plain text "FLEET_SECRET_"
doesn't bypass checks). In practice replace the bytes.Contains call with a
scoped check that searches for []byte("$"+ServerSecretPrefix) or uses a
regex/token parse against m.SyncML to ensure the "$" sigil precedes
ServerSecretPrefix; apply the same stricter check wherever the original
contains-based test appears (e.g. the other occurrence handling
supported-top-level-element verification).
---
Nitpick comments:
In `@server/fleet/windows_mdm.go`:
- Around line 91-93: Update the ValidateUserProvided doc comment in
windows_mdm.go to match the implemented LocURI rules in validateLocURIFormat:
remove the blanket "must start with './'" statement and state that LocURI may
either be a relative OMA-DM path beginning with "./" or a device-style path like
"Device/..." (and must not contain "../" traversal), and also note that LocURI
values within <Target> must not be reserved Fleet-managed URIs; reference
validateLocURIFormat and ValidateUserProvided so readers know where the enforced
rules live.
🪄 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: 15fe56c5-b58f-497a-ae4b-4d222df99774
📒 Files selected for processing (4)
changes/42219-windows-profile-syncml-validationchanges/42224-windows-locuri-format-validationserver/fleet/windows_mdm.goserver/fleet/windows_mdm_test.go
✅ Files skipped from review due to trivial changes (2)
- changes/42219-windows-profile-syncml-validation
- changes/42224-windows-locuri-format-validation
| // Substring match for the secret prefix. A literal "FLEET_SECRET_" appearing in profile data with no "$" sigil would | ||
| // also flip this flag, but the only consequence is skipping the top-level element check on that upload, which is | ||
| // acceptable. | ||
| validator.containsServerSecret = bytes.Contains(m.SyncML, []byte(ServerSecretPrefix)) |
There was a problem hiding this comment.
Secret-placeholder bypass is over-broad and can accept unintended non-SyncML payloads.
Using bytes.Contains(m.SyncML, []byte(ServerSecretPrefix)) means any payload containing FLEET_SECRET_ (including plain text/comments) skips the supported-top-level-element check. That weakens the new #42219 protection and can admit invalid uploads.
Proposed fix
- validator.containsServerSecret = bytes.Contains(m.SyncML, []byte(ServerSecretPrefix))
+ validator.containsServerSecret =
+ bytes.Contains(m.SyncML, []byte("$"+ServerSecretPrefix)) ||
+ bytes.Contains(m.SyncML, []byte("${"+ServerSecretPrefix))Also applies to: 149-153
🤖 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/windows_mdm.go` around lines 114 - 117, The current check uses
bytes.Contains(m.SyncML, []byte(ServerSecretPrefix)) which is too permissive;
change validator.containsServerSecret to only match the secret placeholder when
it appears with the expected sigil (e.g. b"$"+ServerSecretPrefix) or otherwise
validate the surrounding SyncML token boundary (so plain text "FLEET_SECRET_"
doesn't bypass checks). In practice replace the bytes.Contains call with a
scoped check that searches for []byte("$"+ServerSecretPrefix) or uses a
regex/token parse against m.SyncML to ensure the "$" sigil precedes
ServerSecretPrefix; apply the same stricter check wherever the original
contains-based test appears (e.g. the other occurrence handling
supported-top-level-element verification).
Related issue: Resolves #42219
Resolves #42224
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.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.Testing
Summary by CodeRabbit
/, contain..path traversal, or are otherwise malformed are now rejected (whitespace-trimmed), preventing later device deployment failures.