Skip to content

Fix fleetctl apply custom profile path and label validation - #55097

Merged
raju249 merged 3 commits into
fleetdm:mainfrom
Prajwal2246:fix/54690-custom-settings-types
Oct 8, 2026
Merged

raju249 merged 3 commits into
fleetdm:mainfrom
Prajwal2246:fix/54690-custom-settings-types

Conversation

@Prajwal2246

@Prajwal2246 Prajwal2246 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Related issue: Resolves #54690

Follow-up to #54375, which added validation for profile names and descriptions.

Reject incorrectly typed custom profile paths and label values when applying fleet specs. Previously, Fleet silently dropped these values and could report success with incomplete settings. Preserve omitted/null fields, empty label lists, and legacy string profile entries. Missing or empty paths remain unchanged; this fixes incorrect types only.

Checklist for submitter

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/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.

Testing

  • Added/updated automated tests

  • QA'd all new/changed functionality manually

Automated validation:

  • go test ./server/fleet -count=1
  • REDIS_TEST=1 go test ./cmd/fleetctl/fleetctl -run '^TestApplySpecs$' -count=1 -v — all 81 cases passed, including three new CLI regressions.
  • GOMAXPROCS=2 make lint-go LINT_CONCURRENCY=2 — main and custom incremental lint passed with 0 issues.
  • Focused validation tests rerun after the linter-required interface{} to any substitutions.
  • New unit regressions fail against the original implementation.
  • CLI cases exercise apply through the HTTP service with a mocked datastore and real local Redis.

Manual QA on local server at commit 2ed283d (Apple MDM disabled):

  • Numeric path rejected with the field-specific validation error and exit code 1.
  • Scalar labels rejected with the field-specific validation error and exit code 1.
  • A numeric labels_include_any element rejected with the field-specific validation error and exit code 1.
  • YAML exports before and after the label tests matched (diff exit code 0).

Valid-profile installation with Apple MDM enabled has not been manually tested. The path rejection was tested before the baseline export was captured.

AI

AI: Codex (unknown)

Frontend

Not applicable; backend validation and CLI errors only.

Summary by CodeRabbit

  • Bug Fixes
    • fleetctl apply now reports validation errors for custom profile settings with incorrectly typed path or label values, rather than silently dropping them. Label fields must contain arrays of strings; omitted or null label fields remain accepted.
    • Invalid values in deprecated team settings also produce validation errors, and the deprecation warning continues to appear.

@Prajwal2246
Prajwal2246 requested a review from a team as a code owner October 8, 2026 11:00
@raju249 raju249 self-assigned this Oct 8, 2026
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: fleetdm/fleet/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a05e10cd-721c-4911-8326-506c67798c50
📥 Commits

Reviewing files that changed from the base of the PR and between 5894330 and 399b49a.

📒 Files selected for processing (1)
  • changes/54690-validate-custom-settings-types
🚧 Files skipped from review as they are similar to previous changes (1)
  • changes/54690-validate-custom-settings-types

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


Walkthrough

MacOSSettings.FromMap now returns type errors for non-string custom-setting paths, non-array label fields, and non-string label elements. Missing and null label fields remain unset. Tests cover parser behavior and confirm that fleetctl apply reports validation errors for malformed values.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 399b4

No actionable merge blocker is established by the supplied change evidence. Normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: validation of custom profile paths and labels during fleetctl apply.
Description check ✅ Passed The description identifies the related issue, explains the behavior change and compatibility requirements, confirms automated tests and manual QA, includes the required AI section, and marks frontend …
Linked Issues check ✅ Passed Issue #54690 requires rejection of non-string path, non-array label fields, and non-string label elements. MacOSSettings.FromMap now returns field-specific json.UnmarshalTypeError values for the…
Out of Scope Changes check ✅ Passed The production change is limited to validation of custom profile paths and labels for issue #54690. The added unit tests, CLI regression tests, and change note support that behavior. No unrelated prod…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.94%. Comparing base (fc58117) to head (399b49a).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55097      +/-   ##
==========================================
- Coverage   76.97%   76.94%   -0.04%     
==========================================
  Files        4292     4292              
  Lines      264268   264307      +39     
  Branches    15431    15431              
==========================================
- Hits       203432   203372      -60     
- Misses      60655    60754      +99     
  Partials      181      181              
Flag Coverage Δ
backend 78.29% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

  • Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

🟢 Approval recommended

The validation is atomic, preserves documented compatibility behavior, and has focused unit and end-to-end CLI coverage.

0 open findings

What changed in this PR

Validates custom Apple profile paths and labels during fleet spec application, preventing silently dropped invalid values.

Changes:

  • Rejects incorrectly typed paths, label arrays, and label elements.
  • Preserves supported null, empty, and legacy profile forms.
  • Adds unit and CLI regression coverage.
File Description
server/​fleet/​app.go Adds strict profile field validation.
server/​fleet/​app_test.go Tests valid and invalid field types.
cmd/​fleetctl/​fleetctl/​apply_test.go Tests surfaced CLI validation errors.
changes/​54690-validate-custom-settings-types Adds the changes entry; content excluded from review.
Files excluded by content exclusion policy (1)
  • changes/54690-validate-custom-settings-types

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@raju249

raju249 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

QAed it locally, fixes the problem described in the issue. I would have taken the same approach as in this PR.

@raju249
raju249 merged commit d148b9f into fleetdm:main Oct 8, 2026
30 of 31 checks passed
@Prajwal2246
Prajwal2246 deleted the fix/54690-custom-settings-types branch October 23, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fleetctl apply silently drops path and labels of the wrong type in apple_settings.custom_settings

3 participants