Add support for fleet vars in scripts(controls scripts, software scripts/script-only packages and setup experience scripts) - #49781
Conversation
Adds fleet.FleetVarsSupportedInScripts (the eight host-scoped built-in variables) and fleet.ValidateFleetVariablesInScript, wired into every entry point that accepts script content: ad-hoc run, saved script create/edit, GitOps batch set (before the dry-run return so --dry-run reports it), and the setup experience script. Any variable use requires a Premium license; unsupported variables are rejected naming the variable. Variables do not expand yet; that ships separately. Part of #46837, resolves #49511
GetHostScript now resolves the supported $FLEET_VAR_* references for the fetching host, after secrets and custom host vitals expansion so end-user-influenced values (IdP data) are never re-scanned by those passes. Stored contents stay unexpanded and deduplicated. When a variable can't resolve (e.g. no linked IdP user), the server records a failed result through SaveHostScriptResult with the new sentinel exit code -5 and the reasons in the output (one line per failing variable), then returns the marked script. fleetd skips a script whose exit code is already set and continues its queue, so a failure never wedges the remaining executions. (-5 because script results already use -1/-2 and software install results use -2 to -4.) Part of #46837, resolves #49512
Validates $FLEET_VAR_* usage in install, post-install, and uninstall scripts at every installer entry point (upload, update, GitOps batch including dry runs, and Fleet-maintained apps), naming the offending script. Script-only packages flow through the same entry points. GetSoftwareInstallDetails resolves the variables for the fetching host across all three scripts in one pass. On an unresolvable variable the server records the failed install through SaveHostSoftwareInstallResult (exit code -3, reason as install script output) and returns not-found, which fleetd already tolerates; the recorded result advances the host's queue. Uninstall executions are delivered as regular script executions, so their expansion is covered by the GetHostScript path. Part of #46837, resolves #49513
# Conflicts: # ee/server/service/setup_experience_test.go
GitHub Actions
Error
There was a problem hiding this comment.
⚠️ Not ready to approve
The newly added service test contains compile-time issues (new(fleet.RoleAdmin) / new("abc")), and RunHostScript validation does not cover saved scripts loaded via script_id/script_name.
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.
Pull request overview
Adds server-side validation for $FLEET_VAR_... usage in scripts, enforcing a Premium-only gate and an allow-list of supported variables so unsupported variables are rejected early across script-related endpoints.
Changes:
- Introduces
fleet.ValidateFleetVariablesInScriptwith an allow-list for script-safe Fleet variables and Premium-only enforcement. - Applies validation in script creation/update, ad-hoc run, batch set scripts, and setup experience script flows.
- Adds unit + integration test coverage for supported/unsupported variables and Free-tier license behavior.
File summaries
| File | Description |
|---|---|
| server/service/scripts.go | Calls Fleet-variable validation in multiple script service methods (run/create/update/batch). |
| server/service/scripts_test.go | Adds service-level tests for Fleet-variable validation behavior (Premium vs Free). |
| server/service/integration_enterprise_test.go | Adds enterprise integration coverage for supported/unsupported Fleet variables in scripts. |
| server/service/integration_core_test.go | Verifies Free-tier endpoints reject scripts containing Fleet variables with license error. |
| server/fleet/script_variables.go | Adds allow-list + validation function for Fleet variables in scripts. |
| server/fleet/script_variables_test.go | Unit tests for Fleet-variable script validation logic. |
| ee/server/service/setup_experience.go | Adds Fleet-variable validation for setup experience script upload. |
| ee/server/service/setup_experience_test.go | Tests setup experience script rejection/acceptance based on Fleet variables. |
| changes/46837-fleet-variables-in-scripts | Change log entry (content excluded by policy; not reviewed). |
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
- Files reviewed: 8/9 changed files
- Comments generated: 4
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49781 +/- ##
==========================================
+ Coverage 67.92% 67.94% +0.01%
==========================================
Files 3905 3894 -11
Lines 249693 249224 -469
Branches 13350 12931 -419
==========================================
- Hits 169598 169324 -274
+ Misses 64823 64624 -199
- Partials 15272 15276 +4
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:
|
GitHub Actions
Inactive
JordanMontgomery
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
✅ Ready to approve
The changes appear correct, consistently enforce validation/licensing across upload paths, and include strong unit and integration test coverage for both script execution and software install flows.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Fleet built-in variable support for scripts, software installers, maintained apps, and setup experience scripts. Variable usage is validated against an allow-list and requires Fleet Premium. Supported variables are expanded per host when Orbit fetches scripts or installer details. Unresolved values produce dedicated failure codes, messages, persisted results, and queue handling. Unit and integration tests cover validation, expansion, licensing, installer flows, and execution failures. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)server/service/integration_enterprise_test.goast-grep timed out on this file 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: 6
🤖 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 `@ee/server/service/maintained_apps.go`:
- Around line 74-76: In the maintained-app update flow, resolve empty install
and uninstall script inputs to the maintained app defaults before calling
validateFleetVariablesOnInstallerScripts. Ensure validation receives the final
effective install, post-install, and uninstall contents, preserving the existing
allow-list and Premium-license checks; add a regression test covering requests
that rely on maintained-app defaults.
In `@ee/server/service/software_installers.go`:
- Around line 424-426: Move or repeat validation in UpdateSoftwareInstaller
after the uploaded script package has been processed and its derived content
assigned to payload.InstallScript, using
validateFleetVariablesOnInstallerScripts for the replacement scripts. Ensure
unsupported Fleet variables and all Fleet variables on Free are rejected, and
add a regression test covering script-package updates.
In `@server/service/integration_enterprise_test.go`:
- Around line 34645-34648: Update the comment immediately above the script
results request to refer to the -5 user message, matching the
ScriptFleetVarResolutionFailedExitCode assertion used by this test; do not
change the response request or assertion.
- Around line 34705-34713: Update the condition function passed to
require.Eventually around ListPendingHostScriptExecutions to avoid
require.NoError on the separate goroutine; use assert.NoError and return false
when an error occurs, while preserving the existing pending-execution polling
behavior.
In `@server/service/orbit.go`:
- Around line 1606-1625: Update the failure result recorded in the failures
block of the software-install result flow to use the software-installer Fleet
variable resolution failure exit code (the installer-specific -3 value) instead
of fleet.ScriptFleetVarResolutionFailedExitCode. Keep the existing
duplicate-result checks and failure message handling unchanged.
In `@server/service/scripts.go`:
- Around line 135-138: Revalidate the final request.ScriptContents in the
saved-script resolution flow after resolving ScriptID or ScriptName, while
retaining the existing early validation for ad-hoc contents; use the same
license-aware fleet validator and authorization-skip behavior. In
server/service/scripts.go lines 135-138, update the relevant script execution
method, and in server/service/scripts_test.go lines 1317-1324, add an ID-backed
saved-script test covering both unsupported variables and Free-tier variable
usage failures.
🪄 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: ec28d115-f38e-472d-86bd-23c03a030d47
📒 Files selected for processing (20)
changes/46837-fleet-variables-in-scriptsee/server/service/maintained_apps.goee/server/service/setup_experience.goee/server/service/setup_experience_test.goee/server/service/software_installers.goee/server/service/software_installers_test.goserver/fleet/errors.goserver/fleet/script_variables.goserver/fleet/script_variables_test.goserver/fleet/scripts.goserver/fleet/software_installer.goserver/fleet/software_test.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/integration_install_test.goserver/service/orbit.goserver/service/script_variables.goserver/service/script_variables_test.goserver/service/scripts.goserver/service/scripts_test.go
…ripts.go Both sentinel namespaces now live side by side in one file so the next magic exit code gets picked with the full picture in view: script results (-1 timeout, -2 scripts disabled, -5 fleet var resolution failed) and software install results (-2 scripts disabled, -3 download failed, -4 installer not found, -5 fleet var resolution failed). Values shared across the two namespaces are deliberately defined once per namespace. Names the previously bare -1/-2 literals in UserMessage and fleetd's disabled-scripts result, and moves the ExitCode* constants out of software_installer.go unchanged.
GitHub Actions
Inactive
There was a problem hiding this comment.
⚠️ Not ready to approve
Script variable expansion uses naive string replacement that can partially mutate unsupported variable tokens when names share prefixes, breaking the “unsupported vars left untouched” guarantee.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
- Files reviewed: 20/21 changed files
- Comments generated: 3
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
GitHub Actions
Inactive
There was a problem hiding this comment.
⚠️ Human review recommended
It changes core script execution and software install fetch behavior across multiple services and tiers, so it warrants final human review despite strong test coverage.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
Comments suppressed due to low confidence (1)
server/fleet/script_variables.go:26
- The comment says this returns the "first" $FLEET_VAR_* reference, but
variables.Findsorts/dedupes matches (it’s not ordered by first occurrence), so the return value isn’t necessarily the first reference in the script. This doc can mislead future callers/tests.
// FindUnsupportedScriptFleetVar returns the name of the first $FLEET_VAR_*
// reference in contents that is not supported in scripts, or "" if all are
// supported.
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| // ExitCodeScriptTimeout is reported when a script did not terminate | ||
| // normally, e.g. fleetd killed it at the execution timeout (Go reports -1 | ||
| // for a process that did not exit cleanly). Script results only. | ||
| ExitCodeScriptTimeout = -1 |
There was a problem hiding this comment.
Note that all except -5 were existing usages but they were defined in multiple places and often just as literals and not consts, so part of this change was unifying all of them(after I ran into the footgun of having a collision between the two by using -3 which I thought was free)
GitHub Actions
Failure
There was a problem hiding this comment.
⚠️ Not ready to approve
A confirmed variable-expansion bug can mutate unsupported $FLEET_VAR_* references (prefix-collision) and BatchSetScripts reports inconsistent invalid-argument field naming for fleet-variable validation errors.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
Comments suppressed due to low confidence (1)
server/service/script_variables.go:84
- variables.Replace uses strings.ReplaceAll on "$FLEET_VAR_"+name, which can accidentally expand a supported variable inside an unsupported variable reference when the unsupported name has the supported name as a prefix (e.g.
$FLEET_VAR_HOST_UUID_SUFFIX+$FLEET_VAR_HOST_UUIDwould expand theHOST_UUIDsubstring, violating the “unsupported variable names are left untouched” guarantee). This can change behavior for legacy scripts containing unsupported variables that should remain literal.
contents = variables.Replace(contents, v, value)
}
- Files reviewed: 21/22 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
GitHub Actions
Failure
There was a problem hiding this comment.
⚠️ Human review recommended
It changes multiple execution-time and result-recording paths across scripts and software installs (including Orbit fetch behavior), so it warrants final human review despite strong test coverage.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
GitHub Actions
Inactive
There was a problem hiding this comment.
✅ Ready to approve
The implementation matches the referenced acceptance criteria, includes Premium gating at all script-content entry points, expands variables per host at fetch time, and is covered by targeted unit and integration tests.
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.
Review details
Files excluded by content exclusion policy (1)
- changes/46837-fleet-variables-in-scripts
- Files reviewed: 21/22 changed files
- Comments generated: 0 new
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Related issue: Resolves #49511 and #46837 as a whole
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
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit