fix(agent): probe package manager availability instead of hardcoding capabilities - #1901
Conversation
|
Implementation notes: Why per-user probing (not a service-global startup probe): manager executables are resolved from the target user's environment block at execution time (
Testing: new unit tests cover filtering (only probed managers advertised, empty set), and per-user probe caching with a fake executor. |
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull request overview
Updates broker capabilities to report only package managers available to the requesting Windows user.
Changes:
- Adds per-user package-manager probing with a 60-second cache.
- Filters capabilities using detected executables and user environment.
- Adds capability filtering and cache tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
devolutions-agent/src/broker/task.rs |
Initializes the probe cache. |
devolutions-agent/src/broker/server/responses.rs |
Filters supported manager capabilities. |
devolutions-agent/src/broker/server/mod.rs |
Adds cached, per-user capability probing. |
devolutions-agent/src/broker/executor/windows/mod.rs |
Implements Windows manager detection. |
devolutions-agent/src/broker/executor/mod.rs |
Defines supported managers and the probe interface. |
devolutions-agent/src/broker/command_builder/dotnet.rs |
Exposes trusted .NET executable resolution. |
Suppressed comments (1)
devolutions-agent/src/broker/executor/windows/mod.rs:165
- This accepts
scoop.cmd, butbuild_scoop_commandresolves onlyscoop.ps1withGet-Command -CommandType ExternalScript; a PATH containing only the.cmdshim is thus advertised and then guaranteed to fail. Scoop also requires the trusted Windows PowerShell host used byprepare_powershell_script, so include both actual prerequisites.
// Scoop is driven through the user's `scoop.ps1` shim resolved from PATH.
ManagerName::Scoop => path_contains_executable(user_env, &["scoop.ps1", "scoop.cmd"]),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Ok(Err(error)) => { | ||
| // Fail open: advertising the full set preserves the previous behavior when the | ||
| // target user environment cannot be inspected (e.g., no active session). | ||
| warn!( | ||
| error = format!("{error:#}"), | ||
| "Failed to probe package manager availability; advertising all supported managers" | ||
| ); | ||
| BROKER_SUPPORTED_MANAGERS.to_vec() | ||
| } | ||
| Err(error) => { | ||
| warn!(%error, "Package manager availability probe panicked; advertising all supported managers"); | ||
| BROKER_SUPPORTED_MANAGERS.to_vec() | ||
| } |
There was a problem hiding this comment.
Fixed in c111099: the probe now fails closed — both the error and panic branches advertise no managers, and failures are never cached (only successful probes are inserted into the cache).
| // npm runs through the user PATH `npm` shim inside the trusted PowerShell wrapper. | ||
| ManagerName::Npm => path_contains_executable(user_env, &["npm.cmd", "npm.exe"]), |
There was a problem hiding this comment.
Fixed in c111099: npm availability now also requires the trusted Windows PowerShell host. Same for Scoop, which additionally only accepts the scoop.ps1 shim to match build_scoop_command's Get-Command -CommandType ExternalScript resolution.
| cache | ||
| .get(user_key) | ||
| .filter(|(probed_at, _)| probed_at.elapsed() < MANAGER_PROBE_TTL) | ||
| .map(|(_, managers)| managers.clone()) |
There was a problem hiding this comment.
Fixed in c111099: the cache TTL is now injectable (ManagerProbeCache::with_ttl) and a deterministic manager_probe_is_refreshed_after_ttl_expiry test verifies a second probe occurs after expiry.
| let mut process_token = Process::current_process() | ||
| .token(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY) | ||
| .context("failed to open process token for privilege adjustment")?; | ||
| let _priv_tcb = | ||
| ScopedPrivileges::enter(&mut process_token, &[privilege::SE_TCB_NAME]).context("failed to enable SeTcb")?; |
There was a problem hiding this comment.
Fixed in c111099: added a process-wide reference-counted SharedPrivileges guard (executor/windows/privileges.rs). Both SYSTEM-mode execution and the probe now acquire privileges through it; a privilege is only enabled on the first acquisition and disabled when the last guard drops, so concurrent probe/execution requests can no longer disable SeTcb under each other. Enable/disable operations are serialized under the refcount lock.
| error = format!("{error:#}"), | ||
| "Failed to probe package manager availability; advertising all supported managers" | ||
| ); | ||
| BROKER_SUPPORTED_MANAGERS.to_vec() |
There was a problem hiding this comment.
Medium — capability fallback contradicts the new availability contract. When target-user probing fails (including no active WTS session), this fallback advertises all 11 managers. The same session lookup is required for execution, so clients can select managers that were not verified and cannot run in that state. Return no verified managers or surface the probe failure instead of restoring the static list.
There was a problem hiding this comment.
Fixed in c111099: probe failure (including no active WTS session) now advertises no managers instead of the static list, and failed probes are not cached.
…capabilities The broker capabilities endpoint statically claimed all supported package managers (including PowerShell 7) without checking whether they exist on the machine. UniGetUI preflights against capabilities, so wrong capabilities turned into runtime failures instead of clean unsupported errors. Manager executables are now probed against the target user's environment using the same resolution rules the execution path applies, and the advertised capabilities are filtered to the managers actually available. Probe results are cached per user and refreshed after a short TTL. Issue: DGW-435 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Availability must be positively established by a platform executor rather than assumed for unknown platforms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Fail closed when the availability probe fails: advertise no managers instead of the full static list, since execution requires the same session/environment lookup and would fail anyway. - Require the trusted Windows PowerShell host for npm and Scoop availability, and only accept the scoop.ps1 shim (matching build_scoop_command resolution). - Replace ScopedPrivileges with a process-wide reference-counted SharedPrivileges guard in both SYSTEM-mode execution and probing, so concurrent requests cannot disable SeTcb (and friends) out from under one another. - Make the probe cache TTL injectable and add a TTL-expiry refresh test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Convert the JoinError into the same anyhow error path instead of matching on the nested result, mirroring the execute() pattern. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
28ea1bd to
0b0b6ce
Compare
d71fead
into
master
The package broker capabilities endpoint previously advertised all supported package managers unconditionally, even when a manager was not installed on the machine (e.g. PowerShell 7 when
pwsh.exedoes not exist).Since UniGetUI preflights operations against the advertised capabilities, incorrect capabilities surfaced as confusing runtime execution failures instead of clean unsupported errors.
The broker now probes which package managers are actually available for the requesting user and only advertises capabilities for those.
Probe results are refreshed periodically, so managers installed or removed after startup are picked up automatically.
Issue: DGW-435