From a35a3aea75431dc15e764995709aa2ea32ba34ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 2 Oct 2026 23:54:26 +0900 Subject: [PATCH 01/17] Align package broker requests with the broker's request validation Send the resolved package version to the package broker: the version the user selected, else the listed version for installs and the target version for updates, omitting versions the broker does not accept for the manager and never sending one for uninstalls or for managers that pin their own. Validate package broker request fields before sending them (install location, versions, package identifiers, source names, custom parameters, architecture) and explain each problem in the operation failure and in the installation options dialog. Warn in the installation options dialog when WinGet custom installer arguments go through the Devolutions Agent. Explain package broker policy decisions and errors: show the denying rule and reason, validation messages, paused and busy states, and authorization failures, including when the broker refuses the capabilities request and when a policy change requires an elevated administrator. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 48 +- .../DialogPages/InstallOptionsViewModel.cs | 79 +++ .../PolicyEditorDialogViewModel.cs | 6 + .../DialogPages/InstallOptionsControl.axaml | 23 + .../BrokerFailureDescriber.cs | 161 ++++++ .../BrokerRequestBuilder.cs | 57 ++- .../BrokerRequestValidator.cs | 466 ++++++++++++++++++ .../PackageOperations.cs | 108 +++- .../BrokerFailureDescriberTests.cs | 119 +++++ .../BrokerRequestBuilderTests.cs | 289 ++++++++++- .../PackageOperationsTests.cs | 171 ++++++- .../PolicyEditorStructuredInputGuardTests.cs | 14 + 12 files changed, 1496 insertions(+), 45 deletions(-) create mode 100644 src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs create mode 100644 src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs create mode 100644 src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 9f7e6d6655..1de24a6061 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1206,6 +1206,52 @@ "Automatic updates": "Automatic updates", "Here you can change UniGetUI's behaviour regarding the following shortcuts. Checking a shortcut will make UniGetUI delete it if if gets created on a future upgrade. Unchecking it will keep the shortcut intact": "Here you can change UniGetUI's behaviour regarding the following shortcuts. Checking a shortcut will make UniGetUI delete it if gets created on a future upgrade. Unchecking it will keep the shortcut intact", "Agent broker unavailable": "Agent broker unavailable", + "These options cannot be used through the Devolutions Agent, which will refuse the operation:": "These options cannot be used through the Devolutions Agent, which will refuse the operation:", + "--override and --custom pass arbitrary arguments to the package installer. Through the Devolutions Agent, the installer can run with administrator rights, so your organization's policy may block these arguments.": "--override and --custom pass arbitrary arguments to the package installer. Through the Devolutions Agent, the installer can run with administrator rights, so your organization's policy may block these arguments.", + "Custom arguments are passed to WinGet by the Devolutions Agent, which runs this operation with administrator rights. Your organization's policy may block custom arguments.": "Custom arguments are passed to WinGet by the Devolutions Agent, which runs this operation with administrator rights. Your organization's policy may block custom arguments.", + "Only an administrator running with elevated rights can change the package broker policy. No changes were saved.": "Only an administrator running with elevated rights can change the package broker policy. No changes were saved.", + "Devolutions Agent only accepts policy changes from signed, unmodified copies of UniGetUI. No changes were saved.": "Devolutions Agent only accepts policy changes from signed, unmodified copies of UniGetUI. No changes were saved.", + "Your organization's package policy does not allow this operation.": "Your organization's package policy does not allow this operation.", + "Reason: {0}": "Reason: {0}", + "Policy rule: {0}": "Policy rule: {0}", + "Contact your administrator if you need this operation to be allowed.": "Contact your administrator if you need this operation to be allowed.", + "The package broker cannot accept this request": "The package broker cannot accept this request", + "The Devolutions Agent would reject the following options of this operation:": "The Devolutions Agent would reject the following options of this operation:", + "Change the installation options of this package, then try again.": "Change the installation options of this package, then try again.", + "UniGetUI is not authorized to use the Devolutions Agent": "UniGetUI is not authorized to use the Devolutions Agent", + "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.": "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.", + "Administrator rights are required": "Administrator rights are required", + "The Devolutions Agent only accepts this request from an administrator running with elevated rights.": "The Devolutions Agent only accepts this request from an administrator running with elevated rights.", + "The Devolutions Agent is busy": "The Devolutions Agent is busy", + "The Devolutions Agent is handling too many requests right now. Wait a moment, then try again.": "The Devolutions Agent is handling too many requests right now. Wait a moment, then try again.", + "Package operations are paused": "Package operations are paused", + "The Devolutions Agent is not accepting package operations, usually because no valid package policy is installed. Contact your administrator.": "The Devolutions Agent is not accepting package operations, usually because no valid package policy is installed. Contact your administrator.", + "The package broker rejected the request": "The package broker rejected the request", + "The Devolutions Agent did not accept one of the options of this operation.": "The Devolutions Agent did not accept one of the options of this operation.", + "The request is too large": "The request is too large", + "The options of this operation exceed the size the Devolutions Agent accepts. Shorten the custom arguments or commands, then try again.": "The options of this operation exceed the size the Devolutions Agent accepts. Shorten the custom arguments or commands, then try again.", + "Operation unsupported by broker": "Operation unsupported by broker", + "The Devolutions Agent on this computer does not support this operation or one of its options.": "The Devolutions Agent on this computer does not support this operation or one of its options.", + "Broker communication error": "Broker communication error", + "The Devolutions Agent did not respond in time. Try again in a moment.": "The Devolutions Agent did not respond in time. Try again in a moment.", + "The Devolutions Agent returned an unexpected response.": "The Devolutions Agent returned an unexpected response.", + "Details: {0}": "Details: {0}", + "The {0} architecture cannot be requested through the Devolutions Agent.": "The {0} architecture cannot be requested through the Devolutions Agent.", + "{0} packages always install the version provided by their source when the Devolutions Agent is used, so a specific version cannot be selected.": "{0} packages always install the version provided by their source when the Devolutions Agent is used, so a specific version cannot be selected.", + "Version ranges such as \"{0}\" cannot be sent to the Devolutions Agent for {1} packages. Select a specific version instead.": "Version ranges such as \"{0}\" cannot be sent to the Devolutions Agent for {1} packages. Select a specific version instead.", + "{0} package versions must be complete semantic versions, such as 1.2.3.": "{0} package versions must be complete semantic versions, such as 1.2.3.", + "The version \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The version \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + "The Chocolatey package identifier \"{0}\" is not accepted by the Devolutions Agent. Identifiers can only contain letters, digits, periods, hyphens and underscores, and cannot name a package file or every package.": "The Chocolatey package identifier \"{0}\" is not accepted by the Devolutions Agent. Identifiers can only contain letters, digits, periods, hyphens and underscores, and cannot name a package file or every package.", + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: it must name a single package, without wildcard characters (* ? [ ]) or a leading hyphen.": "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: it must name a single package, without wildcard characters (* ? [ ]) or a leading hyphen.", + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: module names cannot contain wildcard characters (* ? [ ]).": "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: module names cannot contain wildcard characters (* ? [ ]).", + "The source name \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The source name \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + "The install location \"{0}\" is not accepted by the Devolutions Agent. Use a full path on a local drive, such as C:\\Apps\\MyApp, without \".\" or \"..\" segments, names ending with a period or a space, or special characters.": "The install location \"{0}\" is not accepted by the Devolutions Agent. Use a full path on a local drive, such as C:\\Apps\\MyApp, without \".\" or \"..\" segments, names ending with a period or a space, or special characters.", + "The install location \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}": "The install location \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}", + "The custom argument \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}": "The custom argument \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}", + "The {0} custom argument \"{1}\" is not accepted by the Devolutions Agent. Only single options are allowed, and options that select every app, the architecture or the global scope are not.": "The {0} custom argument \"{1}\" is not accepted by the Devolutions Agent. Only single options are allowed, and options that select every app, the architecture or the global scope are not.", + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: packages must be referenced by their registry name, such as \"name\" or \"@scope/name\", and not by a URL, a Git repository or a local path.": "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: packages must be referenced by their registry name, such as \"name\" or \"@scope/name\", and not by a URL, a Git repository or a local path.", + "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + "The package manager run by the Devolutions Agent reported an error (exit code {0}).": "The package manager run by the Devolutions Agent reported an error (exit code {0}).", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", @@ -1542,12 +1588,10 @@ "Overwrite the policy that changed elsewhere": "Overwrite the policy that changed elsewhere", "Save policy": "Save policy", "Close policy editor": "Close policy editor", - "No reason provided": "No reason provided", "Operation denied by policy": "Operation denied by policy", "Operation failed via broker": "Operation failed via broker", "The broker accepted the request but did not report an operation to track.": "The broker accepted the request but did not report an operation to track.", "The operation did not finish within the allotted time. It may still be running on the agent.": "The operation did not finish within the allotted time. It may still be running on the agent.", - "Operation denied or failed via broker": "Operation denied or failed via broker", "The Devolutions Agent broker is not available. The operation cannot be performed. Please ensure the Devolutions Agent is installed and running.": "The Devolutions Agent broker is not available. The operation cannot be performed. Please ensure the Devolutions Agent is installed and running.", "A boolean match must be omitted, true, or false; mixed arrays are invalid.": "A boolean match must be omitted, true, or false; mixed arrays are invalid.", "A policy field has an invalid value.": "A policy field has an invalid value.", diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 4048fc823e..beccb31b2c 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -12,6 +12,7 @@ using UniGetUI.Core.SettingsEngine.SecureSettings; using UniGetUI.Core.Tools; using UniGetUI.Core.Tools.Scheduling; +using UniGetUI.PackageEngine.AgentBroker; using UniGetUI.PackageEngine.Classes.Packages.Classes; using UniGetUI.PackageEngine.Enums; using UniGetUI.PackageEngine.Interfaces; @@ -194,6 +195,25 @@ partial void OnSelectedProfileChanged(string? value) partial void OnSelectedArchChanged(string? value) => Refresh(); partial void OnSelectedScopeChanged(string? value) => Refresh(); + partial void OnLocationTextChanged(string value) => Refresh(); + + // ── Package broker notices ──────────────────────────────────────────────── + /// Why the Devolutions Agent would reject the current options, one item per line. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(BrokerIssuesVisible))] + private string _brokerIssuesText = ""; + + public bool BrokerIssuesVisible => BrokerIssuesText.Length > 0; + + public string BrokerIssuesHeaderLabel { get; } = CoreTools.Translate( + "These options cannot be used through the Devolutions Agent, which will refuse the operation:"); + + /// Warning about custom WinGet arguments when the operation goes through the Devolutions Agent. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(BrokerCustomArgumentsWarningVisible))] + private string _brokerCustomArgumentsWarning = ""; + + public bool BrokerCustomArgumentsWarningVisible => BrokerCustomArgumentsWarning.Length > 0; // ── CLI params tab ──────────────────────────────────────────────────────── [ObservableProperty] private string _paramsInstall = ""; @@ -499,8 +519,67 @@ private async Task RefreshCommandPreviewAsync() { if (!_uiLoaded) return; CommandPreview = await BuildCurrentCommandAsync(); + await RefreshBrokerNoticesAsync(); } + /// + /// When the operation goes through the Devolutions Agent, explains the options it would + /// reject and warns about custom WinGet installer arguments, using the same rules as the + /// request builder so the user can fix them before starting the operation. + /// + private async Task RefreshBrokerNoticesAsync() + { + if (!IsBrokered(_package)) + { + BrokerIssuesText = ""; + BrokerCustomArgumentsWarning = ""; + return; + } + + var op = CurrentOp(); + try + { + var applied = await InstallOptionsFactory.LoadApplicableAsync(_package, overridePackageOptions: SnapshotOptions()); + string? location = op is OperationType.Uninstall ? null : applied.CustomInstallLocation; + var issues = BrokerRequestValidator.Validate(_package, applied, op, location); + BrokerIssuesText = string.Join(Environment.NewLine, issues.Select(issue => "• " + issue)); + BrokerCustomArgumentsWarning = DescribeBrokerCustomArgumentsRisk(applied, op); + } + catch (Exception ex) + { + Logger.Warn($"[InstallOptionsViewModel] Could not check the options against the package broker rules: {ex.Message}"); + BrokerIssuesText = ""; + BrokerCustomArgumentsWarning = ""; + } + } + + private string DescribeBrokerCustomArgumentsRisk(InstallOptions applied, OperationType op) + { + if (!_package.Manager.Name.Equals("Winget", StringComparison.OrdinalIgnoreCase)) + return ""; + + if (BrokerRequestValidator.UsesWinGetInstallerArguments(_package, applied, op)) + return CoreTools.Translate( + "--override and --custom pass arbitrary arguments to the package installer. Through the Devolutions Agent, the installer can run with administrator rights, so your organization's policy may block these arguments."); + + List parameters = op switch + { + OperationType.Update => applied.CustomParameters_Update, + OperationType.Uninstall => applied.CustomParameters_Uninstall, + _ => applied.CustomParameters_Install, + }; + + return parameters.Count > 0 && applied.RunAsAdministrator + ? CoreTools.Translate( + "Custom arguments are passed to WinGet by the Devolutions Agent, which runs this operation with administrator rights. Your organization's policy may block custom arguments.") + : ""; + } + + private static bool IsBrokered(IPackage package) => + Settings.Get(Settings.K.UseAgentBroker) + && BrokerRequestBuilder.SupportsManager(package.Manager.Name) + && !package.Source.IsVirtualManager; + /// Builds the CLI command for the currently selected operation and options, /// identical to the live preview. Used by the Copy / Open-in-terminal actions. public async Task BuildCurrentCommandAsync() diff --git a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs index 5fdc767ec0..30a4ece0e7 100644 --- a/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/Pages/SettingsPages/PolicyEditor/PolicyEditorDialogViewModel.cs @@ -467,6 +467,12 @@ internal static string DescribeWriteFailure( PolicyWriteFailureKind.BrokerRejected when errorCode == ErrorCode.MalformedDraft => CoreTools.Translate("Devolutions Agent rejected the policy draft as malformed. Refresh policy management state, then review the policy before retrying."), + PolicyWriteFailureKind.BrokerRejected + when errorCode is ErrorCode.AdministratorRequired or ErrorCode.Forbidden => + CoreTools.Translate("Only an administrator running with elevated rights can change the package broker policy. No changes were saved."), + PolicyWriteFailureKind.BrokerRejected + when errorCode is ErrorCode.Unauthorized or ErrorCode.Unauthenticated => + CoreTools.Translate("Devolutions Agent only accepts policy changes from signed, unmodified copies of UniGetUI. No changes were saved."), PolicyWriteFailureKind.BrokerRejected => CoreTools.Translate("Devolutions Agent rejected the policy replacement."), PolicyWriteFailureKind.WriteResultUnknown => diff --git a/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml b/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml index 15561b69b2..e838fb75da 100644 --- a/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml +++ b/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml @@ -225,6 +225,29 @@ + + + + + + + + + + + + A localized title and explanation for a package operation the broker did not run. +public sealed record BrokerFailureDescription(string Title, string Message); + +/// +/// Turns package broker outcomes (policy denials, structured errors and client failures) +/// into specific, localized explanations for the operation failure dialog. +/// +public static class BrokerFailureDescriber +{ + /// Explains a request that the organization's policy denied. + public static BrokerFailureDescription DescribeDenial(DecisionInfo decision) + { + List lines = + [ + CoreTools.Translate("Your organization's package policy does not allow this operation."), + ]; + + if (!string.IsNullOrWhiteSpace(decision.Reason)) + lines.Add(CoreTools.Translate("Reason: {0}", decision.Reason.Trim())); + + if (!string.IsNullOrWhiteSpace(decision.RuleId)) + lines.Add(CoreTools.Translate("Policy rule: {0}", decision.RuleId.Trim())); + + lines.Add(CoreTools.Translate("Contact your administrator if you need this operation to be allowed.")); + + return new(CoreTools.Translate("Operation denied by policy"), string.Join(Environment.NewLine, lines)); + } + + /// Explains a request that UniGetUI did not send because the broker would reject it. + public static BrokerFailureDescription DescribeValidation(IReadOnlyList issues) => + new( + CoreTools.Translate("The package broker cannot accept this request"), + string.Join( + Environment.NewLine, + [ + CoreTools.Translate("The Devolutions Agent would reject the following options of this operation:"), + .. issues.Select(issue => "• " + issue), + CoreTools.Translate("Change the installation options of this package, then try again."), + ])); + + /// + /// Explains why the broker did not accept the client's identity. Returns null when the + /// failure is not an authentication or authorization failure. + /// + public static BrokerFailureDescription? DescribeAccessFailure(BrokerClientException exception) + { + ErrorCode? code = exception.BrokerError?.Code; + if (exception.StatusCode is 401 || code is ErrorCode.Unauthorized or ErrorCode.Unauthenticated) + { + return new( + CoreTools.Translate("UniGetUI is not authorized to use the Devolutions Agent"), + CoreTools.Translate( + "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.")); + } + + if (exception.StatusCode is 403 || code is ErrorCode.Forbidden or ErrorCode.AdministratorRequired) + { + return new( + CoreTools.Translate("Administrator rights are required"), + CoreTools.Translate( + "The Devolutions Agent only accepts this request from an administrator running with elevated rights.")); + } + + return null; + } + + /// Explains a failed broker request. + public static BrokerFailureDescription Describe(BrokerClientException exception) + { + if (DescribeAccessFailure(exception) is { } accessFailure) + return accessFailure; + + ErrorResponse? error = exception.BrokerError; + string? brokerMessage = string.IsNullOrWhiteSpace(error?.Message) ? null : error.Message.Trim(); + + if (exception.StatusCode is 503 && error?.Code is null or ErrorCode.BrokerPaused) + { + return new( + CoreTools.Translate("The Devolutions Agent is busy"), + CoreTools.Translate( + "The Devolutions Agent is handling too many requests right now. Wait a moment, then try again.")); + } + + switch (error?.Code) + { + case ErrorCode.BrokerPaused: + return new( + CoreTools.Translate("Package operations are paused"), + CoreTools.Translate( + "The Devolutions Agent is not accepting package operations, usually because no valid package policy is installed. Contact your administrator.")); + + case ErrorCode.ValidationFailed or ErrorCode.BadRequest: + return new( + CoreTools.Translate("The package broker rejected the request"), + WithDetails( + CoreTools.Translate("The Devolutions Agent did not accept one of the options of this operation."), + DescribeErrorDetails(error, brokerMessage))); + + case ErrorCode.PayloadTooLarge: + return new( + CoreTools.Translate("The request is too large"), + CoreTools.Translate( + "The options of this operation exceed the size the Devolutions Agent accepts. Shorten the custom arguments or commands, then try again.")); + } + + return exception.Kind switch + { + BrokerClientErrorKind.PolicyDenied => new( + CoreTools.Translate("Operation denied by policy"), + CoreTools.Translate("Your organization's package policy does not allow this operation.")), + BrokerClientErrorKind.UnsupportedCapability => new( + CoreTools.Translate("Operation unsupported by broker"), + WithDetails( + CoreTools.Translate("The Devolutions Agent on this computer does not support this operation or one of its options."), + exception.Message)), + BrokerClientErrorKind.RequestTooLarge => new( + CoreTools.Translate("The request is too large"), + CoreTools.Translate( + "The options of this operation exceed the size the Devolutions Agent accepts. Shorten the custom arguments or commands, then try again.")), + BrokerClientErrorKind.Timeout => new( + CoreTools.Translate("Broker communication error"), + CoreTools.Translate("The Devolutions Agent did not respond in time. Try again in a moment.")), + BrokerClientErrorKind.BrokerUnavailable => new( + CoreTools.Translate("Agent broker unavailable"), + CoreTools.Translate( + "The Devolutions Agent broker is not available. The operation cannot be performed. Please ensure the Devolutions Agent is installed and running.")), + BrokerClientErrorKind.EmptyResponse or BrokerClientErrorKind.InvalidResponse => new( + CoreTools.Translate("Broker communication error"), + CoreTools.Translate("The Devolutions Agent returned an unexpected response.")), + _ => new( + CoreTools.Translate("Operation failed via broker"), + brokerMessage ?? exception.Message), + }; + } + + private static string? DescribeErrorDetails(ErrorResponse error, string? brokerMessage) + { + List lines = []; + if (brokerMessage is not null) + lines.Add(brokerMessage); + + foreach (ErrorDetail detail in error.Details ?? []) + { + if (!string.IsNullOrWhiteSpace(detail.Message) && detail.Message.Trim() != brokerMessage) + lines.Add("• " + detail.Message.Trim()); + } + + return lines.Count == 0 ? null : string.Join(Environment.NewLine, lines); + } + + private static string WithDetails(string summary, string? details) => + string.IsNullOrWhiteSpace(details) + ? summary + : summary + Environment.NewLine + CoreTools.Translate("Details: {0}", details); +} diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 0786b4d645..dc56047d0b 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -65,6 +65,14 @@ public static PackageOperationRequest Build( ); } + IReadOnlyList issues = BrokerRequestValidator.Validate( + package, + options, + role, + effectiveInstallLocation); + if (issues.Count > 0) + throw new BrokerRequestValidationException(issues); + List customParameters = GetCustomParameters(options, role); if ( manager is ManagerName.PowerShell @@ -88,7 +96,7 @@ manager is ManagerName.PowerShell Package = new RequestPackage { Id = package.Id, - Version = string.IsNullOrEmpty(options.Version) ? null : options.Version, + Version = ResolveVersion(manager, package, options, role), Architecture = dropArchAndScope ? null : MapArchitecture(options.Architecture), }, Options = new RequestOptions @@ -147,7 +155,52 @@ or ManagerName.PowerShell7 or ManagerName.Scoop or ManagerName.Npm; - private static bool TryMapManagerName(string managerName, out ManagerName mapped) + /// + /// The concrete version the operation installs, when UniGetUI knows it. + /// + /// + /// The broker evaluates version conditions against the version sent in the request: a Deny + /// rule with a version condition matches a request whose version is unknown. So the version + /// is resolved here instead of letting the package manager pick "the latest": the one the + /// user selected, else the one shown for an install, else the one an update moves to. + /// A known version the broker would not accept for the manager is omitted rather than sent, + /// and uninstalls never carry a version, since version conditions do not apply to them. + /// + internal static string? ResolveVersion( + ManagerName manager, + IPackage package, + InstallOptions options, + OperationType role) + { + if (role is OperationType.Uninstall || !BrokerRequestValidator.ManagerAcceptsVersion(manager)) + return null; + + if (role is OperationType.Install && options.Version.Length > 0) + return options.Version; + + // A pre-release install may resolve to a newer version than the one listed. + if (options.PreRelease || !BrokerRequestValidator.ManagerHasKnownVersionRules(manager)) + return null; + + string? candidate = role switch + { + OperationType.Install when package.HasConcreteVersion => package.VersionString, + OperationType.Update when package.IsUpgradable => package.NewVersionString, + _ => null, + }; + + if ( + string.IsNullOrWhiteSpace(candidate) + || candidate.Equals("Unknown", StringComparison.OrdinalIgnoreCase) + || !CoreTools.IsOptionSafeValue(candidate) + || BrokerRequestValidator.CheckVersion(manager, package.Manager.DisplayName, candidate) is not null + ) + return null; + + return candidate; + } + + internal static bool TryMapManagerName(string managerName, out ManagerName mapped) { ManagerName? result = managerName.ToLowerInvariant() switch { diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs new file mode 100644 index 0000000000..c7c5d60ce7 --- /dev/null +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -0,0 +1,466 @@ +using System.Text.RegularExpressions; +using Devolutions.Now.Policy.Api; +using UniGetUI.Core.Tools; +using UniGetUI.PackageEngine.Enums; +using UniGetUI.PackageEngine.Interfaces; +using UniGetUI.PackageEngine.Serializable; +using UniGetUIArchitecture = UniGetUI.PackageEngine.Enums.Architecture; + +namespace UniGetUI.PackageEngine.AgentBroker; + +/// +/// Thrown when a package operation cannot be sent to the package broker because the broker +/// would reject one of its fields. holds one localized explanation per problem. +/// +public sealed class BrokerRequestValidationException(IReadOnlyList issues) + : InvalidOperationException(string.Join(Environment.NewLine, issues)) +{ + public IReadOnlyList Issues { get; } = issues; +} + +/// +/// Client-side checks that mirror the field rules the package broker applies to package +/// operation requests, so that UniGetUI can explain a rejection before sending the request. +/// The broker remains the authority: these checks only cover rules that are known to be +/// enforced, and an accepted request can still be rejected or denied by the broker. +/// +public static partial class BrokerRequestValidator +{ + /// Characters that the broker refuses in values written to a batch script. + private static readonly char[] BatchMetacharacters = ['"', '%', '!', '^', '&', '|', '<', '>', '\r', '\n', '\0']; + + private const string BatchMetacharactersDisplay = "\" % ! ^ & | < >"; + private const int MaxVersionLength = 128; + private const int MaxSourceNameLength = 128; + + /// + /// Returns a localized explanation for every field of the given operation that the package + /// broker is known to reject. An empty list means no known rule is violated. + /// + /// The install location that would be sent, or null. + public static IReadOnlyList Validate( + IPackage package, + InstallOptions options, + OperationType role, + string? installLocation) + { + if (!BrokerRequestBuilder.TryMapManagerName(package.Manager.Name, out ManagerName manager)) + { + return []; + } + + string managerName = package.Manager.DisplayName; + List issues = []; + + AddIssue(issues, CheckPackageId(manager, managerName, package.Id)); + AddIssue(issues, CheckSourceName(manager, managerName, package.Source.Name)); + + if (role is OperationType.Install && options.Version.Length > 0) + { + AddIssue(issues, CheckVersion(manager, managerName, options.Version)); + } + + if (role is not OperationType.Uninstall + && !package.OverridenOptions.WinGet_DropArchAndScope + && string.Equals(options.Architecture, UniGetUIArchitecture.arm32, StringComparison.OrdinalIgnoreCase)) + { + issues.Add(CoreTools.Translate( + "The {0} architecture cannot be requested through the Devolutions Agent.", + UniGetUIArchitecture.arm32)); + } + + if (!string.IsNullOrWhiteSpace(installLocation)) + { + AddIssue(issues, CheckInstallLocation(manager, installLocation)); + } + + foreach (string parameter in GetCustomParameters(options, role)) + { + AddIssue(issues, CheckCustomParameter(manager, managerName, parameter)); + } + + return issues; + } + + /// + /// Whether the custom parameters of the operation pass WinGet --override or + /// --custom, which hand arbitrary arguments to the package installer. + /// + public static bool UsesWinGetInstallerArguments(IPackage package, InstallOptions options, OperationType role) => + BrokerRequestBuilder.TryMapManagerName(package.Manager.Name, out ManagerName manager) + && manager is ManagerName.Winget + && HasWinGetInstallerArguments(GetCustomParameters(options, role)); + + /// + /// Whether any parameter is WinGet's --override or --custom option, matched + /// the way WinGet matches long options: ignoring letter case, with or without an attached value. + /// + public static bool HasWinGetInstallerArguments(IEnumerable parameters) => + parameters.Any(parameter => + { + string option = parameter.Trim(); + if (!option.StartsWith("--", StringComparison.Ordinal)) + { + return false; + } + + string name = option[2..].Split('=', 2)[0].Trim(); + return name.Equals("override", StringComparison.OrdinalIgnoreCase) + || name.Equals("custom", StringComparison.OrdinalIgnoreCase); + }); + + /// + /// Whether the broker accepts a package version for this manager. Scoop and vcpkg always + /// install the version their manifest or port describes. + /// + internal static bool ManagerAcceptsVersion(ManagerName manager) => + manager is not (ManagerName.Scoop or ManagerName.Vcpkg); + + /// + /// Whether the broker applies a version allowlist to this manager, in which case a version + /// UniGetUI already knows can be checked before it is sent. + /// + internal static bool ManagerHasKnownVersionRules(ManagerName manager) => + manager + is ManagerName.Winget + or ManagerName.Chocolatey + or ManagerName.PowerShell + or ManagerName.PowerShell7 + or ManagerName.Npm + or ManagerName.Bun + or ManagerName.Cargo + or ManagerName.Dotnet + or ManagerName.Pip; + + /// Returns a localized explanation when the broker would reject the version. + internal static string? CheckVersion(ManagerName manager, string managerName, string version) + { + if (!ManagerAcceptsVersion(manager)) + { + return CoreTools.Translate( + "{0} packages always install the version provided by their source when the Devolutions Agent is used, so a specific version cannot be selected.", + managerName); + } + + if (manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo + && version.IndexOfAny(['^', '<', '>']) >= 0) + { + return CoreTools.Translate( + "Version ranges such as \"{0}\" cannot be sent to the Devolutions Agent for {1} packages. Select a specific version instead.", + version, + managerName); + } + + if (manager is ManagerName.Bun) + { + return SemanticVersionRegex().IsMatch(version) + ? null + : CoreTools.Translate( + "{0} package versions must be complete semantic versions, such as 1.2.3.", + managerName); + } + + char[]? extraCharacters = manager switch + { + ManagerName.Winget or ManagerName.Chocolatey or ManagerName.PowerShell => [], + ManagerName.Npm => ['~', '*'], + ManagerName.Cargo => ['=', '~', '*'], + ManagerName.Dotnet => ['[', ']', '(', ')', ',', '*'], + ManagerName.PowerShell7 => ['[', ']', '(', ')', ',', '*', ' '], + ManagerName.Pip => ['!'], + _ => null, + }; + + if (extraCharacters is null) + { + return null; + } + + bool valid = version.Length is > 0 and <= MaxVersionLength + && !version.StartsWith('-') + && version.All(c => + char.IsAsciiLetterOrDigit(c) + || c is '.' or '-' or '+' or '_' + || extraCharacters.Contains(c)); + + return valid + ? null + : CoreTools.Translate( + "The version \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + version, + managerName); + } + + /// Returns a localized explanation when the broker would reject the package identifier. + internal static string? CheckPackageId(ManagerName manager, string managerName, string id) + { + string? issue = manager switch + { + ManagerName.Chocolatey when !IsNuGetStylePackageId(id) => CoreTools.Translate( + "The Chocolatey package identifier \"{0}\" is not accepted by the Devolutions Agent. Identifiers can only contain letters, digits, periods, hyphens and underscores, and cannot name a package file or every package.", + id), + ManagerName.Scoop when !IsSinglePackageName(id) => CoreTools.Translate( + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: it must name a single package, without wildcard characters (* ? [ ]) or a leading hyphen.", + managerName, + id), + ManagerName.PowerShell or ManagerName.PowerShell7 when id.IndexOfAny(['*', '?', '[', ']']) >= 0 => + CoreTools.Translate( + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: module names cannot contain wildcard characters (* ? [ ]).", + managerName, + id), + ManagerName.Npm when !IsNpmPackageId(id) => RegistryNameIssue(managerName, id), + ManagerName.Bun when !IsNpmRegistryName(id) => RegistryNameIssue(managerName, id), + ManagerName.Cargo when !IsCrateName(id) => InvalidIdIssue(managerName, id), + ManagerName.Pip when !IsPythonDistributionName(id) => InvalidIdIssue(managerName, id), + _ => null, + }; + + if (issue is null && RunsThroughBatchScript(manager) && id.IndexOfAny(BatchMetacharacters) >= 0) + { + issue = InvalidIdIssue(managerName, id); + } + + return issue; + } + + /// Returns a localized explanation when the broker would reject the source name. + internal static string? CheckSourceName(ManagerName manager, string managerName, string sourceName) + { + if (manager is not (ManagerName.Winget or ManagerName.Chocolatey or ManagerName.PowerShell or ManagerName.PowerShell7)) + { + return null; + } + + string name = manager is ManagerName.PowerShell or ManagerName.PowerShell7 ? sourceName.Trim() : sourceName; + bool valid = name.Length is > 0 and <= MaxSourceNameLength + && char.IsAsciiLetterOrDigit(name[0]) + && !name.EndsWith(' ') + && name.All(c => char.IsAsciiLetterOrDigit(c) || c is '.' or '-' or '_' or ' '); + + return valid + ? null + : CoreTools.Translate( + "The source name \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + sourceName, + managerName); + } + + /// Returns a localized explanation when the broker would reject the install location. + internal static string? CheckInstallLocation(ManagerName manager, string location) + { + if (!IsPlainLocalDrivePath(location)) + { + return CoreTools.Translate( + "The install location \"{0}\" is not accepted by the Devolutions Agent. Use a full path on a local drive, such as C:\\Apps\\MyApp, without \".\" or \"..\" segments, names ending with a period or a space, or special characters.", + location); + } + + if (RunsThroughBatchScript(manager) && location.IndexOfAny(BatchMetacharacters) >= 0) + { + return CoreTools.Translate( + "The install location \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}", + location, + BatchMetacharactersDisplay); + } + + return null; + } + + /// Returns a localized explanation when the broker would reject a custom parameter. + internal static string? CheckCustomParameter(ManagerName manager, string managerName, string parameter) + { + if (RunsThroughBatchScript(manager) && parameter.IndexOfAny(BatchMetacharacters) >= 0) + { + return CoreTools.Translate( + "The custom argument \"{0}\" cannot contain any of the following characters when the Devolutions Agent is used: {1}", + parameter, + BatchMetacharactersDisplay); + } + + if (manager is ManagerName.Scoop && !IsAcceptedScoopParameter(parameter)) + { + return CoreTools.Translate( + "The {0} custom argument \"{1}\" is not accepted by the Devolutions Agent. Only single options are allowed, and options that select every app, the architecture or the global scope are not.", + managerName, + parameter); + } + + return null; + } + + /// + /// Whether the broker runs this manager through a generated batch script, where it + /// rejects batch metacharacters in every argument instead of escaping them. + /// + private static bool RunsThroughBatchScript(ManagerName manager) => + manager + is ManagerName.Winget + or ManagerName.Chocolatey + or ManagerName.Cargo + or ManagerName.Vcpkg + or ManagerName.Bun; + + /// + /// A plain absolute path on a local drive letter: no relative, UNC or device paths, no + /// ./.. segments, no segment starting with a space or ending with a period or a + /// space, no alternate data stream, and no wildcard or control characters. + /// + internal static bool IsPlainLocalDrivePath(string location) + { + string path = location.Replace('/', '\\'); + if (path.Length < 3 || !char.IsAsciiLetter(path[0]) || path[1] != ':' || path[2] != '\\') + { + return false; + } + + string rest = path[3..]; + if (rest.Any(c => char.IsControl(c) || c is ':' or '*' or '?' or '"' or '<' or '>' or '|')) + { + return false; + } + + return rest + .Split('\\', StringSplitOptions.RemoveEmptyEntries) + .All(segment => !segment.EndsWith('.') && !segment.EndsWith(' ') && !segment.StartsWith(' ')); + } + + private static bool IsNuGetStylePackageId(string id) + { + if (id.Length is 0 or > 100 || id[0] is '-' or '.') + { + return false; + } + + if (!id.All(c => char.IsAsciiLetterOrDigit(c) || c is '.' or '-' or '_')) + { + return false; + } + + return !id.EndsWith(".config", StringComparison.OrdinalIgnoreCase) + && !id.EndsWith(".nupkg", StringComparison.OrdinalIgnoreCase) + && !id.Equals("all", StringComparison.OrdinalIgnoreCase); + } + + private static bool IsSinglePackageName(string id) => + id.Trim().Length > 0 + && id.IndexOfAny(['*', '?', '[', ']']) < 0 + && !id.StartsWith('-') + && id.IndexOfAny(['\0', '\r', '\n']) < 0; + + /// A registry name, or an alias whose local and target names are both registry names. + private static bool IsNpmPackageId(string id) + { + if (id.Contains('%')) + { + return false; + } + + int colonIndex = id.IndexOf(':'); + if (colonIndex <= 0) + { + return IsNpmRegistryName(id); + } + + string targetSpec = id[(colonIndex + 1)..]; + int atIndex = targetSpec.LastIndexOf('@'); + string targetName = atIndex > 0 ? targetSpec[..atIndex] : targetSpec; + return IsNpmRegistryName(id[..colonIndex]) && IsNpmRegistryName(targetName); + } + + /// An npm registry package name: name or @scope/name. + private static bool IsNpmRegistryName(string name) + { + if (name.Length is 0 or > 214) + { + return false; + } + + bool shapeIsValid; + if (name.StartsWith('@')) + { + string[] parts = name[1..].Split('/'); + shapeIsValid = parts.Length == 2 && IsNpmNamePart(parts[0]) && IsNpmNamePart(parts[1]); + } + else + { + shapeIsValid = IsNpmNamePart(name); + } + + return shapeIsValid + && !name.EndsWith(".tgz", StringComparison.OrdinalIgnoreCase) + && !name.EndsWith(".tar", StringComparison.OrdinalIgnoreCase) + && !name.EndsWith(".tar.gz", StringComparison.OrdinalIgnoreCase); + } + + private static bool IsNpmNamePart(string part) => + part.Length > 0 + && part[0] is not ('.' or '_' or '-') + && part.All(c => char.IsAsciiLetterOrDigit(c) || c is '-' or '.' or '_' or '~'); + + private static bool IsCrateName(string id) => + id.Length is > 0 and <= 64 + && !id.StartsWith('-') + && id.All(c => char.IsAsciiLetterOrDigit(c) || c is '-' or '_'); + + private static bool IsPythonDistributionName(string id) => + id.Length > 0 + && char.IsAsciiLetterOrDigit(id[0]) + && char.IsAsciiLetterOrDigit(id[^1]) + && id.All(c => char.IsAsciiLetterOrDigit(c) || c is '.' or '-' or '_'); + + /// + /// A single Scoop option that names one app: no positional argument or option terminator, + /// no whitespace, and no option selecting every app (--all), the architecture + /// (--arch, or a in a short-option cluster) or the global scope. + /// + private static bool IsAcceptedScoopParameter(string parameter) + { + if (parameter == "--" || !parameter.StartsWith('-') || parameter.Any(char.IsWhiteSpace)) + { + return false; + } + + string option = parameter.ToLowerInvariant(); + if (option.StartsWith("--", StringComparison.Ordinal)) + { + string name = option[2..].Split('=', 2)[0]; + return name is not ("all" or "arch" or "global"); + } + + string shortOptions = option[1..]; + return !shortOptions.Contains('a') && !shortOptions.Contains('g'); + } + + private static string RegistryNameIssue(string managerName, string id) => + CoreTools.Translate( + "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: packages must be referenced by their registry name, such as \"name\" or \"@scope/name\", and not by a URL, a Git repository or a local path.", + managerName, + id); + + private static string InvalidIdIssue(string managerName, string id) => + CoreTools.Translate( + "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + id, + managerName); + + private static IReadOnlyList GetCustomParameters(InstallOptions options, OperationType role) => role switch + { + OperationType.Install => options.CustomParameters_Install ?? [], + OperationType.Update => options.CustomParameters_Update ?? [], + OperationType.Uninstall => options.CustomParameters_Uninstall ?? [], + _ => [], + }; + + private static void AddIssue(List issues, string? issue) + { + if (issue is not null) + { + issues.Add(issue); + } + } + + [GeneratedRegex( + @"^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?\z", + RegexOptions.CultureInvariant)] + private static partial Regex SemanticVersionRegex(); +} diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index cb3be98a8d..9f2bf42e6f 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -28,6 +28,7 @@ using BrokerStatusResponse = Devolutions.Now.Policy.Api.StatusResponse; using OperationCancelQuery = Devolutions.Now.Policy.Client.OperationCancelQuery; using OperationStatusQuery = Devolutions.Now.Policy.Client.OperationStatusQuery; +using PackageOperationRequest = Devolutions.Now.Policy.Client.PackageOperationRequest; #if WINDOWS using UniGetUI.PackageEngine.Managers.WingetManager; #endif @@ -405,12 +406,42 @@ private async Task PerformBrokerOperation() return HandleBrokerUnavailable(); } + // Fetch the capabilities up front: the broker requires an authenticated client for + // them, so this is where an unsigned or modified build is turned away. The client + // caches the response for the request below. + if (await ProbeBrokerCapabilities(client) is { } capabilitiesFailure) + { + return capabilitiesFailure; + } + // Resolve the install location the same way the local WinGet path does, so the // portable-install safeguard (registry-detected location) is not bypassed. string? effectiveInstallLocation = GetBrokerEffectiveInstallLocation(); // Build the broker request. - var request = BrokerRequestBuilder.Build(Package, Options, Role, effectiveInstallLocation); + PackageOperationRequest request; + try + { + request = BrokerRequestBuilder.Build(Package, Options, Role, effectiveInstallLocation); + } + catch (BrokerRequestValidationException ex) + { + foreach (string issue in ex.Issues) + { + Line(issue, LineType.Error); + } + + Logger.Warn($"[AgentBroker] Request for {Package.Id} not sent: {ex.Message}"); + return FailWith(BrokerFailureDescriber.DescribeValidation(ex.Issues)); + } + catch (InvalidOperationException ex) + { + // A value that would be read as a command-line option or split into further + // arguments; the broker would reject it as well. + Line(ex.Message, LineType.Error); + Logger.Warn($"[AgentBroker] Request for {Package.Id} not sent: {ex.Message}"); + return FailWith(BrokerFailureDescriber.DescribeValidation([ex.Message])); + } Line($"Sending request to broker: {request.RequestId}", LineType.VerboseDetails); Line($" Package: {request.Package.Id} ({request.Operation})", LineType.VerboseDetails); @@ -426,11 +457,10 @@ private async Task PerformBrokerOperation() if (execution.Decision.Decision != BrokerDecision.Allow) { - string denialReason = execution.Decision.Reason ?? CoreTools.Translate("No reason provided"); - Line($"Operation denied by policy: {denialReason}", LineType.Error); - Metadata.FailureTitle = CoreTools.Translate("Operation denied by policy"); - Metadata.FailureMessage = denialReason; - return OperationVeredict.Failure; + Line( + $"Operation denied by policy: rule={execution.Decision.RuleId}, reason={execution.Decision.Reason}", + LineType.Error); + return FailWith(BrokerFailureDescriber.DescribeDenial(execution.Decision)); } if (execution.Operation is null) @@ -503,10 +533,55 @@ private async Task PerformBrokerOperation() { Line($"Broker operation failed: {ex.Message}", LineType.Error); Logger.Error($"[AgentBroker] Broker operation failed: {ex}"); - Metadata.FailureTitle = CoreTools.Translate(GetBrokerFailureTitle(ex.Kind)); - Metadata.FailureMessage = ex.Message; + return FailWith(BrokerFailureDescriber.Describe(ex)); + } + } + + /// + /// Fetches the broker capabilities before the request is built. Returns the failure + /// veredict when the broker cannot be used, or null to continue. + /// + private async Task ProbeBrokerCapabilities(BrokerClient client) + { + try + { + await client.GetCapabilities(CancellationToken); + return null; + } + catch (OperationCanceledException) when (CancellationToken.IsCancellationRequested) + { + Line("Broker operation was canceled.", LineType.Information); + return OperationVeredict.Canceled; + } + catch (BrokerClientException ex) when (BrokerFailureDescriber.DescribeAccessFailure(ex) is { } accessFailure) + { + // The broker is running but does not accept this client: report it as + // unavailable for this copy of UniGetUI, with the reason, instead of a + // generic failure on the operation request. + Line($"The agent broker did not accept this client: {ex.Message}", LineType.Error); + Logger.Error($"[AgentBroker] Capabilities request was refused (HTTP {ex.StatusCode}, {ex.BrokerError?.Code}): {ex.Message}"); + FailWith(accessFailure); + BrokerUnavailable?.Invoke(this, accessFailure.Message); return OperationVeredict.Failure; } + catch (BrokerClientException ex) when (ex.Kind is BrokerClientErrorKind.BrokerUnavailable) + { + Logger.Error($"[AgentBroker] Broker became unavailable while reading its capabilities: {ex}"); + return HandleBrokerUnavailable(); + } + catch (BrokerClientException ex) + { + Line($"Could not read the broker capabilities: {ex.Message}", LineType.Error); + Logger.Error($"[AgentBroker] Capabilities request failed: {ex}"); + return FailWith(BrokerFailureDescriber.Describe(ex)); + } + } + + private OperationVeredict FailWith(BrokerFailureDescription description) + { + Metadata.FailureTitle = description.Title; + Metadata.FailureMessage = description.Message; + return OperationVeredict.Failure; } /// @@ -820,9 +895,13 @@ private async Task InterpretBrokerTerminalStatus(BrokerStatus } // Operation failed — surface a user-visible error. - string reason = status.Message ?? $"Exit code: {status.ExitCode}"; + string reason = string.IsNullOrWhiteSpace(status.Message) + ? CoreTools.Translate( + "The package manager run by the Devolutions Agent reported an error (exit code {0}).", + status.ExitCode?.ToString() ?? "?") + : status.Message; Line($"Operation failed via broker: {reason}", LineType.Error); - Metadata.FailureTitle = CoreTools.Translate("Operation denied or failed via broker"); + Metadata.FailureTitle = CoreTools.Translate("Operation failed via broker"); Metadata.FailureMessage = reason; return OperationVeredict.Failure; } @@ -950,15 +1029,6 @@ private static string GetEffectiveUser() return $"{Environment.UserDomainName}\\{Environment.UserName}"; } - private static string GetBrokerFailureTitle(BrokerClientErrorKind kind) => - kind switch - { - BrokerClientErrorKind.PolicyDenied => "Operation denied by policy", - BrokerClientErrorKind.UnsupportedCapability => "Operation unsupported by broker", - BrokerClientErrorKind.Timeout => "Broker communication error", - _ => "Operation failed via broker", - }; - protected sealed override Task GetProcessVeredict( int ReturnCode, List Output diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs new file mode 100644 index 0000000000..1a19d5d485 --- /dev/null +++ b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs @@ -0,0 +1,119 @@ +using Devolutions.Now.Policy.Api; +using Devolutions.Now.Policy.Client; +using UniGetUI.Core.Tools; +using UniGetUI.PackageEngine.AgentBroker; + +namespace UniGetUI.PackageEngine.Tests; + +public class BrokerFailureDescriberTests +{ + private static BrokerClientException BrokerError(int statusCode, ErrorCode code, string message = "", params string[] details) => + new( + BrokerClientErrorKind.BrokerError, + $"Broker returned HTTP {statusCode} ({code})", + "/v1/package-operations/execute", + statusCode, + new ErrorResponse + { + Code = code, + Message = message, + Details = [.. details.Select(detail => new ErrorDetail { Message = detail })], + }); + + [Fact] + public void DescribeDenial_IncludesReasonAndRule() + { + var description = BrokerFailureDescriber.DescribeDenial(new DecisionInfo + { + Decision = Decision.Deny, + RuleId = "block-beta", + Reason = "Pre-release versions are blocked", + }); + + Assert.Equal(CoreTools.Translate("Operation denied by policy"), description.Title); + Assert.Contains("block-beta", description.Message); + Assert.Contains("Pre-release versions are blocked", description.Message); + } + + [Fact] + public void DescribeDenial_WithoutDetailsStillExplainsThePolicy() + { + var description = BrokerFailureDescriber.DescribeDenial(new DecisionInfo { Decision = Decision.Deny }); + + Assert.Contains(CoreTools.Translate("Your organization's package policy does not allow this operation."), description.Message); + Assert.DoesNotContain(CoreTools.Translate("Policy rule: {0}", ""), description.Message); + } + + [Fact] + public void Describe_ValidationFailureShowsTheBrokerMessage() + { + var description = BrokerFailureDescriber.Describe( + BrokerError(400, ErrorCode.ValidationFailed, "custom install location is not a plain local drive path", "path detail")); + + Assert.Equal(CoreTools.Translate("The package broker rejected the request"), description.Title); + Assert.Contains("custom install location is not a plain local drive path", description.Message); + Assert.Contains("path detail", description.Message); + } + + [Fact] + public void Describe_BusyBrokerAfterRetries() + { + var description = BrokerFailureDescriber.Describe( + BrokerError(503, ErrorCode.BrokerPaused, "package broker is busy; retry later")); + + Assert.Equal(CoreTools.Translate("The Devolutions Agent is busy"), description.Title); + } + + [Fact] + public void Describe_PausedBrokerWithoutPolicy() + { + var description = BrokerFailureDescriber.Describe( + BrokerError(409, ErrorCode.BrokerPaused, "no valid policy")); + + Assert.Equal(CoreTools.Translate("Package operations are paused"), description.Title); + } + + [Theory] + [InlineData(401, ErrorCode.Unauthorized, "UniGetUI is not authorized to use the Devolutions Agent")] + [InlineData(401, ErrorCode.Unauthenticated, "UniGetUI is not authorized to use the Devolutions Agent")] + [InlineData(403, ErrorCode.AdministratorRequired, "Administrator rights are required")] + [InlineData(403, ErrorCode.Forbidden, "Administrator rights are required")] + public void Describe_AuthorizationFailures(int statusCode, ErrorCode code, string expectedTitle) + { + var exception = BrokerError(statusCode, code); + + Assert.NotNull(BrokerFailureDescriber.DescribeAccessFailure(exception)); + Assert.Equal(CoreTools.Translate(expectedTitle), BrokerFailureDescriber.Describe(exception).Title); + } + + [Fact] + public void DescribeAccessFailure_IgnoresOtherFailures() + { + Assert.Null(BrokerFailureDescriber.DescribeAccessFailure(BrokerError(400, ErrorCode.ValidationFailed))); + Assert.Null(BrokerFailureDescriber.DescribeAccessFailure( + new BrokerClientException(BrokerClientErrorKind.BrokerUnavailable, "pipe not found"))); + } + + [Theory] + [InlineData(BrokerClientErrorKind.UnsupportedCapability, "Operation unsupported by broker")] + [InlineData(BrokerClientErrorKind.Timeout, "Broker communication error")] + [InlineData(BrokerClientErrorKind.RequestTooLarge, "The request is too large")] + [InlineData(BrokerClientErrorKind.BrokerUnavailable, "Agent broker unavailable")] + [InlineData(BrokerClientErrorKind.InvalidResponse, "Broker communication error")] + public void Describe_ClientFailures(BrokerClientErrorKind kind, string expectedTitle) + { + var description = BrokerFailureDescriber.Describe(new BrokerClientException(kind, "client failure")); + + Assert.Equal(CoreTools.Translate(expectedTitle), description.Title); + Assert.False(string.IsNullOrWhiteSpace(description.Message)); + } + + [Fact] + public void DescribeValidation_ListsEveryIssue() + { + var description = BrokerFailureDescriber.DescribeValidation(["first issue", "second issue"]); + + Assert.Contains("first issue", description.Message); + Assert.Contains("second issue", description.Message); + } +} diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index c77a5f90bf..01456108f3 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -54,6 +54,7 @@ public void Build_MapsSupportedManagers(string managerName, ManagerName expected { var package = new PackageBuilder() .WithManager(new PackageManagerBuilder().WithName(managerName).Build()) + .WithId("contoso-test") .Build(); var request = BrokerRequestBuilder.Build(package, new InstallOptions(), OperationType.Install); @@ -297,7 +298,7 @@ public void Build_RefusesAnInjectedIdentifierForShellInterpretedManagers() } [Fact] - public void Build_KeepsWinGetVersionsThatAreNotPlainVersions() + public void Build_RefusesWinGetVersionsThatTheBrokerRejects() { var package = new PackageBuilder() .WithManager(new PackageManagerBuilder().WithName("Winget").Build()) @@ -305,9 +306,291 @@ public void Build_KeepsWinGetVersionsThatAreNotPlainVersions() .Build(); var options = new InstallOptions { Version = "2021 Update" }; - var request = BrokerRequestBuilder.Build(package, options, OperationType.Install); + var exception = Assert.Throws( + () => BrokerRequestBuilder.Build(package, options, OperationType.Install)); + + Assert.Contains(exception.Issues, issue => issue.Contains("2021 Update")); + } + + private static UniGetUI.PackageEngine.PackageClasses.Package BuildPackage( + string managerName, + string id = "contoso-tool", + string version = "1.2.3", + string? newVersion = null) + { + var builder = new PackageBuilder() + .WithManager(new PackageManagerBuilder().WithName(managerName).Build()) + .WithId(id) + .WithVersion(version); + if (newVersion is not null) + builder = builder.WithNewVersion(newVersion); + return builder.Build(); + } + + [Theory] + [InlineData("Winget")] + [InlineData("Chocolatey")] + [InlineData("Npm")] + [InlineData("Cargo")] + [InlineData(".NET Tool")] + [InlineData("PowerShell")] + [InlineData("PowerShell7")] + [InlineData("Pip")] + [InlineData("Bun")] + public void Build_SendsTheListedVersionForInstallsWithoutAnExplicitVersion(string managerName) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName), new InstallOptions(), OperationType.Install); + + Assert.Equal("1.2.3", request.Package.Version); + } + + [Fact] + public void Build_SendsTheTargetVersionForUpdates() + { + var package = BuildPackage("Winget", "Contoso.Test", "1.0.0", "2.0.0"); + + var request = BrokerRequestBuilder.Build(package, new InstallOptions(), OperationType.Update); + + Assert.Equal("2.0.0", request.Package.Version); + } + + [Fact] + public void Build_PrefersTheExplicitlySelectedVersion() + { + var request = BrokerRequestBuilder.Build( + BuildPackage("Npm"), new InstallOptions { Version = "1.0.0" }, OperationType.Install); + + Assert.Equal("1.0.0", request.Package.Version); + } + + [Theory] + [InlineData("Winget")] + [InlineData("Npm")] + [InlineData("Pip")] + public void Build_NeverSendsAVersionForUninstalls(string managerName) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName), new InstallOptions(), OperationType.Uninstall); + + Assert.Null(request.Package.Version); + } + + [Theory] + [InlineData("Scoop")] + [InlineData("vcpkg")] + public void Build_NeverSendsAVersionForManagersThatPinTheirOwn(string managerName) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName), new InstallOptions(), OperationType.Install); + + Assert.Null(request.Package.Version); + } + + [Fact] + public void Build_LeavesTheVersionUnsetForPreReleaseInstalls() + { + var request = BrokerRequestBuilder.Build( + BuildPackage("Winget", "Contoso.Test"), new InstallOptions { PreRelease = true }, OperationType.Install); + + Assert.Null(request.Package.Version); + } + + [Theory] + [InlineData("Winget", "Unknown")] + [InlineData("Winget", "< 1.2")] + [InlineData("Winget", "2021 Update")] + [InlineData("Bun", "1.2")] + [InlineData("Npm", "^1.2.0")] + public void Build_OmitsAListedVersionTheBrokerWouldNotAccept(string managerName, string listedVersion) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName, version: listedVersion), new InstallOptions(), OperationType.Install); - Assert.Equal("2021 Update", request.Package.Version); + Assert.Null(request.Package.Version); } + [Theory] + [InlineData("Npm", "^1.2.0")] + [InlineData("Npm", ">=1.0.0")] + [InlineData("Cargo", "<2")] + [InlineData("Bun", "1.2")] + [InlineData("PowerShell", "[1.0,2.0)")] + [InlineData("Scoop", "1.2.3")] + public void Build_RefusesAnExplicitVersionTheBrokerWouldReject(string managerName, string version) + { + Assert.ThrowsAny(() => BrokerRequestBuilder.Build( + BuildPackage(managerName), new InstallOptions { Version = version }, OperationType.Install)); + } + + [Theory] + [InlineData("Npm", "1.2.x")] + [InlineData("Npm", "1.x")] + [InlineData("Cargo", "=1.2.3")] + [InlineData(".NET Tool", "[1.0,2.0)")] + [InlineData("Pip", "1!2.0")] + public void Build_KeepsVersionSyntaxTheBrokerAccepts(string managerName, string version) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName), new InstallOptions { Version = version }, OperationType.Install); + + Assert.Equal(version, request.Package.Version); + } + + [Theory] + [InlineData("C:\\Tools\\..\\Windows")] + [InlineData("C:\\Tools\\.")] + [InlineData("C:\\Tools\\App.")] + [InlineData("C:\\Tools\\App ")] + [InlineData("C:\\Tools\\file.txt:stream")] + [InlineData("Tools\\App")] + [InlineData("\\\\server\\share\\App")] + [InlineData("\\\\?\\C:\\Tools")] + [InlineData("C:Tools")] + [InlineData("C:\\Tools\\%APPDATA%")] + public void Build_RefusesInstallLocationsTheBrokerRejects(string location) + { + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), new InstallOptions(), OperationType.Install, location)); + } + + [Theory] + [InlineData("C:\\Program Files\\App v1.2")] + [InlineData("d:/Tools//App/")] + [InlineData("D:\\")] + public void Build_KeepsPlainLocalInstallLocations(string location) + { + var request = BrokerRequestBuilder.Build( + BuildWinGetPackage(), new InstallOptions(), OperationType.Install, location); + + Assert.Equal(location, request.Options.CustomInstallLocation); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Build_SendsNoInstallLocationForBlankValues(string location) + { + var request = BrokerRequestBuilder.Build( + BuildWinGetPackage(), new InstallOptions(), OperationType.Install, location); + + Assert.Null(request.Options.CustomInstallLocation); + } + + [Theory] + [InlineData("Chocolatey", "git;7zip")] + [InlineData("Chocolatey", "all")] + [InlineData("Chocolatey", "packages.config")] + [InlineData("Chocolatey", "git.nupkg")] + [InlineData("Chocolatey", "C:\\pkgs\\git")] + [InlineData("Scoop", "*")] + [InlineData("Scoop", "7z*")] + [InlineData("Scoop", "--all")] + [InlineData("PowerShell", "Pester*")] + [InlineData("PowerShell7", "[Pp]ester")] + [InlineData("Npm", "user/repo")] + [InlineData("Npm", "github:user/repo")] + [InlineData("Npm", "git+https://example.test/repo.git")] + [InlineData("Npm", "file:../pkg")] + [InlineData("Npm", "pkg.tgz")] + [InlineData("Npm", "contoso%PATH%")] + [InlineData("Bun", "github:user/repo")] + [InlineData("Cargo", "my crate")] + [InlineData("Pip", "requests[security]")] + [InlineData("Winget", "Contoso.App&Other")] + public void Build_RefusesPackageIdentifiersTheBrokerRejects(string managerName, string id) + { + Assert.ThrowsAny(() => BrokerRequestBuilder.Build( + BuildPackage(managerName, id), new InstallOptions(), OperationType.Install)); + } + + [Theory] + [InlineData("Chocolatey", "notepadplusplus.install")] + [InlineData("Chocolatey", "allure")] + [InlineData("Npm", "@contoso/tool")] + [InlineData("Npm", "eslint-v9:eslint@^9.x")] + [InlineData("Bun", "@contoso/tool")] + [InlineData("PowerShell", "Az.Accounts")] + [InlineData("Scoop", "7zip")] + public void Build_KeepsPackageIdentifiersTheBrokerAccepts(string managerName, string id) + { + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName, id), new InstallOptions(), OperationType.Install); + + Assert.Equal(id, request.Package.Id); + } + + [Theory] + [InlineData("--all")] + [InlineData("--arch=64bit")] + [InlineData("-a")] + [InlineData("-qa")] + [InlineData("--global")] + [InlineData("-g")] + [InlineData("extras/app")] + [InlineData("--")] + public void Build_RefusesScoopCustomParametersTheBrokerRejects(string parameter) + { + var options = new InstallOptions { CustomParameters_Update = [parameter] }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Scoop", "7zip"), options, OperationType.Update)); + } + + [Theory] + [InlineData("--no-cache")] + [InlineData("--quiet")] + [InlineData("-fq")] + public void Build_KeepsScoopCustomParametersTheBrokerAccepts(string parameter) + { + var options = new InstallOptions { CustomParameters_Update = [parameter] }; + + var request = BrokerRequestBuilder.Build(BuildPackage("Scoop", "7zip"), options, OperationType.Update); + + Assert.Equal([parameter], request.Options.CustomParameters); + } + + [Theory] + [InlineData("\"/S")] + [InlineData("%TEMP%")] + [InlineData("/D=a!b")] + [InlineData("a^b")] + public void Build_RefusesBatchMetacharactersInWinGetCustomParameters(string parameter) + { + var options = new InstallOptions { CustomParameters_Install = ["--override", parameter] }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Install)); + } + + [Fact] + public void Build_RefusesTheArm32Architecture() + { + var options = new InstallOptions { Architecture = UniGetUIArchitecture.arm32 }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Install)); + } + + [Theory] + [InlineData("--override", true)] + [InlineData("--OVERRIDE=/S", true)] + [InlineData("--custom", true)] + [InlineData("--custom=/quiet", true)] + [InlineData("--silent", false)] + [InlineData("override", false)] + public void HasWinGetInstallerArguments_MatchesOverrideAndCustom(string parameter, bool expected) + { + Assert.Equal(expected, BrokerRequestValidator.HasWinGetInstallerArguments([parameter])); + } + + [Fact] + public void UsesWinGetInstallerArguments_IsWinGetOnly() + { + var options = new InstallOptions { CustomParameters_Install = ["--override"] }; + + Assert.True(BrokerRequestValidator.UsesWinGetInstallerArguments(BuildWinGetPackage(), options, OperationType.Install)); + Assert.False(BrokerRequestValidator.UsesWinGetInstallerArguments(BuildWinGetPackage(), options, OperationType.Update)); + Assert.False(BrokerRequestValidator.UsesWinGetInstallerArguments(BuildPackage("Scoop", "7zip"), options, OperationType.Install)); + } } diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index bdb099dbfd..056008136a 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -26,6 +26,8 @@ using BrokerApiDecision = Devolutions.Now.Policy.Api.Decision; using BrokerApiDecisionInfo = Devolutions.Now.Policy.Api.DecisionInfo; using BrokerApiElevation = Devolutions.Now.Policy.Api.Elevation; +using BrokerApiErrorCode = Devolutions.Now.Policy.Api.ErrorCode; +using BrokerApiErrorResponse = Devolutions.Now.Policy.Api.ErrorResponse; using BrokerApiEventChannel = Devolutions.Now.Policy.Api.EventChannel; using BrokerApiEventChannelKind = Devolutions.Now.Policy.Api.EventChannelKind; using BrokerApiExecutionResponse = Devolutions.Now.Policy.Api.ExecutionResponse; @@ -762,6 +764,107 @@ public async Task BrokerOperationFailsWithoutLocalFallbackWhenBrokerDropsAfterPr Assert.Contains(transport.RequestedPaths, path => path != "/v1/health"); } + private static async Task<(OperationVeredict Veredict, string? Title, string? Message)> RunBrokeredOperationCapturingFailure( + ScriptedBrokerTransport transport, + string? packageId = null) + { + AbstractOperation? created = null; + var result = await RunBrokeredOperationWithOutput( + transport, + onOperationCreated: operation => created = operation, + packageId: packageId); + return (result.Veredict, created?.Metadata.FailureTitle, created?.Metadata.FailureMessage); + } + + [Fact] + public async Task BrokerPolicyDenialExplainsTheReasonAndRule() + { + var transport = new ScriptedBrokerTransport + { + Denial = new BrokerApiDecisionInfo + { + Decision = BrokerApiDecision.Deny, + RuleId = "deny-old-versions", + Reason = "Versions before 2.0 are not allowed", + }, + }; + + var (veredict, title, message) = await RunBrokeredOperationCapturingFailure(transport); + + Assert.Equal(OperationVeredict.Failure, veredict); + Assert.Equal(CoreTools.Translate("Operation denied by policy"), title); + Assert.Contains("deny-old-versions", message); + Assert.Contains("Versions before 2.0 are not allowed", message); + } + + [Fact] + public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizationFailure() + { + bool originalSetting = Settings.Get(Settings.K.UseAgentBroker); + string? notifiedMessage = null; + EventHandler onBrokerUnavailable = (_, message) => notifiedMessage = message; + PackageOperation.BrokerUnavailable += onBrokerUnavailable; + try + { + var transport = new ScriptedBrokerTransport + { + CapabilitiesError = (401, BrokerApiErrorCode.Unauthorized), + }; + + var (veredict, title, message) = await RunBrokeredOperationCapturingFailure(transport); + + Assert.Equal(OperationVeredict.Failure, veredict); + Assert.Equal(CoreTools.Translate("UniGetUI is not authorized to use the Devolutions Agent"), title); + Assert.Contains("signed", message); + Assert.Equal(message, notifiedMessage); + Assert.DoesNotContain("/v1/package-operations/execute", transport.RequestedPaths); + } + finally + { + PackageOperation.BrokerUnavailable -= onBrokerUnavailable; + Settings.Set(Settings.K.UseAgentBroker, originalSetting); + } + } + + [Fact] + public async Task BrokerPausedWithoutPolicyIsExplained() + { + var transport = new ScriptedBrokerTransport + { + CapabilitiesError = (409, BrokerApiErrorCode.BrokerPaused), + }; + + var (veredict, title, _) = await RunBrokeredOperationCapturingFailure(transport); + + Assert.Equal(OperationVeredict.Failure, veredict); + Assert.Equal(CoreTools.Translate("Package operations are paused"), title); + } + + [Fact] + public async Task BrokerRequestThatTheBrokerWouldRejectIsNotSent() + { + var transport = new ScriptedBrokerTransport(); + + var (veredict, title, message) = await RunBrokeredOperationCapturingFailure(transport, packageId: "git;7zip"); + + Assert.Equal(OperationVeredict.Failure, veredict); + Assert.Equal(CoreTools.Translate("The package broker cannot accept this request"), title); + Assert.Contains("git;7zip", message); + Assert.DoesNotContain("/v1/package-operations/execute", transport.RequestedPaths); + } + + [Fact] + public async Task BrokeredInstallSendsTheResolvedPackageVersion() + { + var transport = new ScriptedBrokerTransport(); + + await RunBrokeredOperation(transport); + + Assert.NotNull(transport.LastExecuteRequestBody); + var request = BrokerSerializer.Deserialize(transport.LastExecuteRequestBody!); + Assert.Equal("1.0.0", request!.Package.Version); + } + /// /// Runs an install operation against a broker whose transport simulates an outage and /// asserts the policy-enforcement contract: the operation fails with the @@ -842,7 +945,8 @@ private static async Task RunBrokeredOperationWithOutput( Action? configureCancellation = null, Action? configureOperationHelper = null, Action? onOperationCreated = null, - TimeSpan? operationTimeout = null) + TimeSpan? operationTimeout = null, + string? packageId = null) { bool originalSetting = Settings.Get(Settings.K.UseAgentBroker); int originalPollInterval = PackageOperation.BrokerStatusPollIntervalMs; @@ -858,7 +962,7 @@ private static async Task RunBrokeredOperationWithOutput( }) .ConfigureOperation(helper => configureOperationHelper?.Invoke(helper)) .Build(); - var package = new PackageBuilder().WithManager(manager).Build(); + var package = new PackageBuilder().WithManager(manager).WithId(packageId ?? "Contoso.Test").Build(); PackageOperation.BrokerTransportFactory = () => transport; PackageOperation.BrokerStatusPollIntervalMs = 5; PackageOperation.BrokerCancelRequestTimeout = TimeSpan.FromSeconds(2); @@ -1332,6 +1436,12 @@ private sealed class ScriptedBrokerTransport : IBrokerTransport /// public string? LastExecuteRequestBody { get; private set; } + /// When set, the capabilities endpoint answers with this structured error. + public (int StatusCode, BrokerApiErrorCode Code)? CapabilitiesError { get; set; } + + /// When set, the execute endpoint reports this policy decision without an operation. + public BrokerApiDecisionInfo? Denial { get; set; } + private bool cancelReceived; public BrokerTransportKind Kind => BrokerTransportKind.HttpNamedPipe; @@ -1350,6 +1460,16 @@ public Task Send( return request.Path switch { "/v1/health" => Json(BrokerSerializer.Serialize(BuildHealthResponse())), + "/v1/capabilities" when CapabilitiesError is { } error => Task.FromResult( + new BrokerTransportResponse + { + StatusCode = error.StatusCode, + Body = BrokerSerializer.Serialize(new BrokerApiErrorResponse + { + Code = error.Code, + Message = "capabilities refused", + }), + }), "/v1/capabilities" => Json(BrokerSerializer.Serialize(BuildCapabilities())), "/v1/package-operations/execute" => Json(BrokerSerializer.Serialize(BuildExecutionResponse())), "/v1/package-operations/get-status" => HandleStatusQuery(), @@ -1438,25 +1558,38 @@ private Task HandleCancelRequest() ], }; - private BrokerApiExecutionResponse BuildExecutionResponse() => new() + private BrokerApiExecutionResponse BuildExecutionResponse() { - ResponseKind = BrokerApiConstants.ExecutionResponseKind, - ResponseVersion = BrokerApiConstants.Version, - Decision = new BrokerApiDecisionInfo { Decision = BrokerApiDecision.Allow }, - Operation = new BrokerApiOperationSubmission + if (Denial is { } denial) { - OperationId = OperationId, - Status = BrokerApiOperationStatus.Starting, - SubmittedAt = DateTimeOffset.UtcNow, - EventChannel = EventChannelPipeName is null - ? null - : new BrokerApiEventChannel - { - Kind = BrokerApiEventChannelKind.LocalPipe, - Path = EventChannelPipeName, - }, - }, - }; + return new() + { + ResponseKind = BrokerApiConstants.ExecutionResponseKind, + ResponseVersion = BrokerApiConstants.Version, + Decision = denial, + }; + } + + return new() + { + ResponseKind = BrokerApiConstants.ExecutionResponseKind, + ResponseVersion = BrokerApiConstants.Version, + Decision = new BrokerApiDecisionInfo { Decision = BrokerApiDecision.Allow }, + Operation = new BrokerApiOperationSubmission + { + OperationId = OperationId, + Status = BrokerApiOperationStatus.Starting, + SubmittedAt = DateTimeOffset.UtcNow, + EventChannel = EventChannelPipeName is null + ? null + : new BrokerApiEventChannel + { + Kind = BrokerApiEventChannelKind.LocalPipe, + Path = EventChannelPipeName, + }, + }, + }; + } } /// diff --git a/src/UniGetUI.Tests/PolicyEditor/PolicyEditorStructuredInputGuardTests.cs b/src/UniGetUI.Tests/PolicyEditor/PolicyEditorStructuredInputGuardTests.cs index 99cce2cd6a..87250c7046 100644 --- a/src/UniGetUI.Tests/PolicyEditor/PolicyEditorStructuredInputGuardTests.cs +++ b/src/UniGetUI.Tests/PolicyEditor/PolicyEditorStructuredInputGuardTests.cs @@ -1232,6 +1232,20 @@ public void UnknownWriteResult_WithoutDiagnosticDoesNotClaimAuthenticationFailur Assert.DoesNotContain("authenticate", message, StringComparison.OrdinalIgnoreCase); } + [Theory] + [InlineData(ErrorCode.AdministratorRequired, "administrator")] + [InlineData(ErrorCode.Forbidden, "administrator")] + [InlineData(ErrorCode.Unauthorized, "signed")] + [InlineData(ErrorCode.Unauthenticated, "signed")] + public void AuthorizationRejection_ExplainsWhoCanChangeThePolicy(ErrorCode code, string expected) + { + string message = PolicyEditorDialogViewModel.DescribeWriteFailure( + PolicyWriteFailureKind.BrokerRejected, + code); + + Assert.Contains(expected, message, StringComparison.OrdinalIgnoreCase); + } + [Fact] public void MalformedDraftRejection_ExplainsSafeRecovery() { From 538d753c98d23e69f1d1ae21fa30b39f768c12db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 00:12:18 +0900 Subject: [PATCH 02/17] Check the same broker request values in the options dialog and the operation Share the brokered install location resolver with the installation options dialog, validate versions of managers with broker version rules only against those rules, and bound Bun version length. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogPages/InstallOptionsViewModel.cs | 10 ++++++++-- .../BrokerRequestBuilder.cs | 8 +++++++- .../BrokerRequestValidator.cs | 2 +- .../PackageOperations.cs | 19 +++++++++++++------ .../BrokerRequestBuilderTests.cs | 14 +++++++++++++- 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index beccb31b2c..4c38bc0fb9 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -16,6 +16,7 @@ using UniGetUI.PackageEngine.Classes.Packages.Classes; using UniGetUI.PackageEngine.Enums; using UniGetUI.PackageEngine.Interfaces; +using UniGetUI.PackageEngine.Operations; using UniGetUI.PackageEngine.PackageClasses; using UniGetUI.PackageEngine.Serializable; @@ -540,8 +541,13 @@ private async Task RefreshBrokerNoticesAsync() try { var applied = await InstallOptionsFactory.LoadApplicableAsync(_package, overridePackageOptions: SnapshotOptions()); - string? location = op is OperationType.Uninstall ? null : applied.CustomInstallLocation; - var issues = BrokerRequestValidator.Validate(_package, applied, op, location); + // Resolve the location exactly as the brokered operation will (for WinGet updates + // this may be the registry-detected location rather than the configured one). + var issues = await Task.Run(() => BrokerRequestValidator.Validate( + _package, + applied, + op, + PackageOperation.GetBrokerInstallLocation(_package, applied, op))); BrokerIssuesText = string.Join(Environment.NewLine, issues.Select(issue => "• " + issue)); BrokerCustomArgumentsWarning = DescribeBrokerCustomArgumentsRisk(applied, op); } diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index dc56047d0b..0a36cc9e47 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -59,7 +59,13 @@ public static PackageOperationRequest Build( $"Refusing to build a {manager} broker request for the package identifier \"{package.Id}\": it is not a valid package identifier." ); - if (options.Version.Length > 0 && !CoreTools.IsValidPackageVersion(options.Version)) + // Managers with known broker version rules are checked against those (stricter, and + // aware of each manager's range syntax) by BrokerRequestValidator below. + if ( + options.Version.Length > 0 + && !BrokerRequestValidator.ManagerHasKnownVersionRules(manager) + && !CoreTools.IsValidPackageVersion(options.Version) + ) throw new InvalidOperationException( $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{options.Version}\" is not a valid package version." ); diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index c7c5d60ce7..be297c4fdc 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -153,7 +153,7 @@ or ManagerName.Dotnet if (manager is ManagerName.Bun) { - return SemanticVersionRegex().IsMatch(version) + return version.Length <= MaxVersionLength && SemanticVersionRegex().IsMatch(version) ? null : CoreTools.Translate( "{0} package versions must be complete semantic versions, such as 1.2.3.", diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index 9f2bf42e6f..a2be6cbbeb 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -1246,22 +1246,29 @@ private static bool IsWinGetManager(IPackageManager manager) /// for installs (and non-WinGet updates) the configured custom location; for /// uninstalls nothing. /// - private string? GetBrokerEffectiveInstallLocation() + private string? GetBrokerEffectiveInstallLocation() => + GetBrokerInstallLocation(Package, Options, Role); + + /// + /// The install location a brokered operation sends for the given package, options and role. + /// Shared with the installation options dialog so that it checks the same value. + /// + public static string? GetBrokerInstallLocation(IPackage package, InstallOptions options, OperationType role) { - switch (Role) + switch (role) { case OperationType.Update: #if WINDOWS - if (IsWinGetManager(Package.Manager)) + if (IsWinGetManager(package.Manager)) { - return WinGetPkgOperationHelper.GetEffectiveUpdateLocation(Package, Options); + return WinGetPkgOperationHelper.GetEffectiveUpdateLocation(package, options); } #endif goto case OperationType.Install; case OperationType.Install: - return string.IsNullOrWhiteSpace(Options.CustomInstallLocation) + return string.IsNullOrWhiteSpace(options.CustomInstallLocation) ? null - : Options.CustomInstallLocation; + : options.CustomInstallLocation; default: return null; } diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 01456108f3..b3a87a08f4 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -279,7 +279,7 @@ public void Build_RefusesAnInjectedVersionForShellInterpretedManagers(string man .Build(); var options = new InstallOptions { Version = "1.2.3; Start-Process calc" }; - Assert.Throws( + Assert.ThrowsAny( () => BrokerRequestBuilder.Build(package, options, OperationType.Install) ); } @@ -425,6 +425,9 @@ public void Build_RefusesAnExplicitVersionTheBrokerWouldReject(string managerNam [Theory] [InlineData("Npm", "1.2.x")] + [InlineData("Npm", "~1.2")] + [InlineData("Npm", "1.2.*")] + [InlineData("PowerShell7", "[1.0, 2.0)")] [InlineData("Npm", "1.x")] [InlineData("Cargo", "=1.2.3")] [InlineData(".NET Tool", "[1.0,2.0)")] @@ -437,6 +440,15 @@ public void Build_KeepsVersionSyntaxTheBrokerAccepts(string managerName, string Assert.Equal(version, request.Package.Version); } + [Fact] + public void Build_RefusesBunVersionsLongerThanTheBrokerAccepts() + { + string version = "1.2.3+" + new string('a', 200); + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Bun"), new InstallOptions { Version = version }, OperationType.Install)); + } + [Theory] [InlineData("C:\\Tools\\..\\Windows")] [InlineData("C:\\Tools\\.")] From 1e2b9afea36d23ce59ed8d42526cfb927e1c3b1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 00:30:44 +0900 Subject: [PATCH 03/17] Keep broker notices and notifications accurate Publish only the latest options-dialog broker check, use the operation's elevation predicate for the WinGet custom arguments warning, keep the target version for pre-release updates, and pass the specific failure title to the broker notification dialog. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Infrastructure/AvaloniaBootstrapper.cs | 6 ++--- .../DialogPages/InstallOptionsViewModel.cs | 27 +++++++++++++++---- .../BrokerRequestBuilder.cs | 3 ++- .../PackageOperations.cs | 11 ++++---- .../BrokerRequestBuilderTests.cs | 10 +++++++ .../PackageOperationsTests.cs | 10 ++++--- 6 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/UniGetUI.Avalonia/Infrastructure/AvaloniaBootstrapper.cs b/src/UniGetUI.Avalonia/Infrastructure/AvaloniaBootstrapper.cs index 83687461b6..77301d3172 100644 --- a/src/UniGetUI.Avalonia/Infrastructure/AvaloniaBootstrapper.cs +++ b/src/UniGetUI.Avalonia/Infrastructure/AvaloniaBootstrapper.cs @@ -137,7 +137,7 @@ private static Task InitializeSharedServicesAsync() Secrets.GetOpenSearchUsername(), Secrets.GetOpenSearchPassword()); AbstractOperation.QueueDrained += (_, _) => _ = TelemetryHandler.FlushPackageEventsAsync(); - PackageOperation.BrokerUnavailable += (_, message) => + PackageOperation.BrokerUnavailable += (_, failure) => Dispatcher.UIThread.Post(async void () => { // Runs on the UI thread, so the flag needs no synchronization. @@ -146,8 +146,8 @@ private static Task InitializeSharedServicesAsync() try { await new SimpleErrorDialog( - CoreTools.Translate("Agent broker unavailable"), - message).ShowDialog(owner); + failure.Title, + failure.Message).ShowDialog(owner); } catch (Exception ex) { diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 4c38bc0fb9..ea0155b2de 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -528,8 +528,11 @@ private async Task RefreshCommandPreviewAsync() /// reject and warns about custom WinGet installer arguments, using the same rules as the /// request builder so the user can fix them before starting the operation. /// + private int _brokerNoticesGeneration; + private async Task RefreshBrokerNoticesAsync() { + int generation = Interlocked.Increment(ref _brokerNoticesGeneration); if (!IsBrokered(_package)) { BrokerIssuesText = ""; @@ -538,6 +541,8 @@ private async Task RefreshBrokerNoticesAsync() } var op = CurrentOp(); + string issuesText; + string customArgumentsWarning; try { var applied = await InstallOptionsFactory.LoadApplicableAsync(_package, overridePackageOptions: SnapshotOptions()); @@ -548,15 +553,22 @@ private async Task RefreshBrokerNoticesAsync() applied, op, PackageOperation.GetBrokerInstallLocation(_package, applied, op))); - BrokerIssuesText = string.Join(Environment.NewLine, issues.Select(issue => "• " + issue)); - BrokerCustomArgumentsWarning = DescribeBrokerCustomArgumentsRisk(applied, op); + issuesText = string.Join(Environment.NewLine, issues.Select(issue => "• " + issue)); + customArgumentsWarning = DescribeBrokerCustomArgumentsRisk(applied, op); } catch (Exception ex) { Logger.Warn($"[InstallOptionsViewModel] Could not check the options against the package broker rules: {ex.Message}"); - BrokerIssuesText = ""; - BrokerCustomArgumentsWarning = ""; + issuesText = ""; + customArgumentsWarning = ""; } + + // Edits start overlapping refreshes; only the latest one may publish its result. + if (generation != _brokerNoticesGeneration) + return; + + BrokerIssuesText = issuesText; + BrokerCustomArgumentsWarning = customArgumentsWarning; } private string DescribeBrokerCustomArgumentsRisk(InstallOptions applied, OperationType op) @@ -575,7 +587,12 @@ private string DescribeBrokerCustomArgumentsRisk(InstallOptions applied, Operati _ => applied.CustomParameters_Install, }; - return parameters.Count > 0 && applied.RunAsAdministrator + // Same elevation predicate as the brokered operation: the package's own requirement + // (e.g. a WinGet installer that needs elevation) counts as well as the checkbox. + bool runsElevated = !Settings.Get(Settings.K.ProhibitElevation) + && (_package.OverridenOptions.RunAsAdministrator is true || applied.RunAsAdministrator); + + return parameters.Count > 0 && runsElevated ? CoreTools.Translate( "Custom arguments are passed to WinGet by the Devolutions Agent, which runs this operation with administrator rights. Your organization's policy may block custom arguments.") : ""; diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 0a36cc9e47..757fba082c 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -185,7 +185,8 @@ or ManagerName.Scoop return options.Version; // A pre-release install may resolve to a newer version than the one listed. - if (options.PreRelease || !BrokerRequestValidator.ManagerHasKnownVersionRules(manager)) + if ((role is OperationType.Install && options.PreRelease) + || !BrokerRequestValidator.ManagerHasKnownVersionRules(manager)) return null; string? candidate = role switch diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index a2be6cbbeb..a0785537c9 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -39,10 +39,11 @@ public abstract class PackageOperation : AbstractProcessOperation { /// /// Raised when an operation that must be routed through the Devolutions Agent broker - /// cannot proceed because the broker is not available. The payload is a user-facing - /// error message. The UI layer subscribes to this to show an error message box. + /// cannot proceed because the broker is not available, or does not accept this client. + /// The payload is the user-facing title and message. The UI layer subscribes to this to + /// show an error message box. /// - public static event EventHandler? BrokerUnavailable; + public static event EventHandler? BrokerUnavailable; /// /// Test seam: substitutes the transport used to reach the agent broker so tests can @@ -561,7 +562,7 @@ private async Task PerformBrokerOperation() Line($"The agent broker did not accept this client: {ex.Message}", LineType.Error); Logger.Error($"[AgentBroker] Capabilities request was refused (HTTP {ex.StatusCode}, {ex.BrokerError?.Code}): {ex.Message}"); FailWith(accessFailure); - BrokerUnavailable?.Invoke(this, accessFailure.Message); + BrokerUnavailable?.Invoke(this, accessFailure); return OperationVeredict.Failure; } catch (BrokerClientException ex) when (ex.Kind is BrokerClientErrorKind.BrokerUnavailable) @@ -920,7 +921,7 @@ private OperationVeredict HandleBrokerUnavailable() "The Devolutions Agent broker is not available. The operation cannot be performed. Please ensure the Devolutions Agent is installed and running."); Metadata.FailureTitle = CoreTools.Translate("Agent broker unavailable"); Metadata.FailureMessage = message; - BrokerUnavailable?.Invoke(this, message); + BrokerUnavailable?.Invoke(this, new BrokerFailureDescription(Metadata.FailureTitle, message)); return OperationVeredict.Failure; } diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index b3a87a08f4..632e5c40aa 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -396,6 +396,16 @@ public void Build_LeavesTheVersionUnsetForPreReleaseInstalls() Assert.Null(request.Package.Version); } + [Fact] + public void Build_SendsTheTargetVersionForUpdatesEvenWithPreReleaseEnabled() + { + var package = BuildPackage("Winget", "Contoso.Test", "1.0.0", "2.0.0-beta.1"); + + var request = BrokerRequestBuilder.Build(package, new InstallOptions { PreRelease = true }, OperationType.Update); + + Assert.Equal("2.0.0-beta.1", request.Package.Version); + } + [Theory] [InlineData("Winget", "Unknown")] [InlineData("Winget", "< 1.2")] diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index 056008136a..4357d38deb 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -8,6 +8,7 @@ using UniGetUI.Core.SettingsEngine; using UniGetUI.Core.Tools; using UniGetUI.Interface.Enums; +using UniGetUI.PackageEngine.AgentBroker; using UniGetUI.PackageEngine.Enums; using UniGetUI.PackageEngine.Interfaces; using UniGetUI.PackageEngine.Managers.NpmManager; @@ -801,8 +802,8 @@ public async Task BrokerPolicyDenialExplainsTheReasonAndRule() public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizationFailure() { bool originalSetting = Settings.Get(Settings.K.UseAgentBroker); - string? notifiedMessage = null; - EventHandler onBrokerUnavailable = (_, message) => notifiedMessage = message; + BrokerFailureDescription? notified = null; + EventHandler onBrokerUnavailable = (_, failure) => notified = failure; PackageOperation.BrokerUnavailable += onBrokerUnavailable; try { @@ -816,7 +817,8 @@ public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizatio Assert.Equal(OperationVeredict.Failure, veredict); Assert.Equal(CoreTools.Translate("UniGetUI is not authorized to use the Devolutions Agent"), title); Assert.Contains("signed", message); - Assert.Equal(message, notifiedMessage); + Assert.Equal(title, notified?.Title); + Assert.Equal(message, notified?.Message); Assert.DoesNotContain("/v1/package-operations/execute", transport.RequestedPaths); } finally @@ -891,7 +893,7 @@ private static async Task AssertBrokerUnavailableFailure(FakeBrokerTransport tra .Build(); var package = new PackageBuilder().WithManager(manager).Build(); string? notifiedMessage = null; - EventHandler onBrokerUnavailable = (_, message) => notifiedMessage = message; + EventHandler onBrokerUnavailable = (_, failure) => notifiedMessage = failure.Message; PackageOperation.BrokerUnavailable += onBrokerUnavailable; PackageOperation.BrokerTransportFactory = () => transport; Settings.Set(Settings.K.UseAgentBroker, true); From 75688c347f323ed9d05bbead5edc21ef20e898fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 00:50:11 +0900 Subject: [PATCH 04/17] Send only request fields the broker accepts Omit the source URL for managers that identify sources by name, validate the custom parameters that are actually sent (including the PowerShell AllowClobber retry) and refuse them for managers that accept none, and tell a busy broker apart from a paused one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 1 + .../BrokerFailureDescriber.cs | 14 ++++- .../BrokerRequestBuilder.cs | 23 +++++--- .../BrokerRequestValidator.cs | 55 +++++++++++++++++- .../BrokerFailureDescriberTests.cs | 11 +++- .../BrokerRequestBuilderTests.cs | 56 +++++++++++++++++-- .../PackageOperationsTests.cs | 2 +- 7 files changed, 139 insertions(+), 23 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 1de24a6061..71276b1076 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1252,6 +1252,7 @@ "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: packages must be referenced by their registry name, such as \"name\" or \"@scope/name\", and not by a URL, a Git repository or a local path.": "The {0} package identifier \"{1}\" is not accepted by the Devolutions Agent: packages must be referenced by their registry name, such as \"name\" or \"@scope/name\", and not by a URL, a Git repository or a local path.", "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", "The package manager run by the Devolutions Agent reported an error (exit code {0}).": "The package manager run by the Devolutions Agent reported an error (exit code {0}).", + "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.": "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs index 212d3e6e0e..5ad2d3accf 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs @@ -79,7 +79,13 @@ public static BrokerFailureDescription Describe(BrokerClientException exception) ErrorResponse? error = exception.BrokerError; string? brokerMessage = string.IsNullOrWhiteSpace(error?.Message) ? null : error.Message.Trim(); - if (exception.StatusCode is 503 && error?.Code is null or ErrorCode.BrokerPaused) + // The broker answers both "busy" (connection limit, retried by the client) and "no valid + // policy" with BrokerPaused over HTTP 503; only the busy reply says so in its message. + bool busy = error is null + ? exception.StatusCode is 503 + : error.Code is ErrorCode.BrokerPaused + && brokerMessage?.Contains("busy", StringComparison.OrdinalIgnoreCase) is true; + if (busy) { return new( CoreTools.Translate("The Devolutions Agent is busy"), @@ -92,8 +98,10 @@ public static BrokerFailureDescription Describe(BrokerClientException exception) case ErrorCode.BrokerPaused: return new( CoreTools.Translate("Package operations are paused"), - CoreTools.Translate( - "The Devolutions Agent is not accepting package operations, usually because no valid package policy is installed. Contact your administrator.")); + WithDetails( + CoreTools.Translate( + "The Devolutions Agent is not accepting package operations, usually because no valid package policy is installed. Contact your administrator."), + brokerMessage)); case ErrorCode.ValidationFailed or ErrorCode.BadRequest: return new( diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 757fba082c..4549cabd23 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -71,14 +71,6 @@ public static PackageOperationRequest Build( ); } - IReadOnlyList issues = BrokerRequestValidator.Validate( - package, - options, - role, - effectiveInstallLocation); - if (issues.Count > 0) - throw new BrokerRequestValidationException(issues); - List customParameters = GetCustomParameters(options, role); if ( manager is ManagerName.PowerShell @@ -87,6 +79,16 @@ manager is ManagerName.PowerShell ) customParameters = [.. customParameters, "-AllowClobber"]; + // Validate what will actually be sent, including parameters added by a retry. + IReadOnlyList issues = BrokerRequestValidator.Validate( + package, + options, + role, + effectiveInstallLocation, + customParameters); + if (issues.Count > 0) + throw new BrokerRequestValidationException(issues); + return new PackageOperationRequest { RequestId = BrokerClient.GenerateRequestId(), @@ -97,7 +99,10 @@ manager is ManagerName.PowerShell Source = new RequestSource { Name = package.Source.Name, - Url = package.Source.Url?.ToString(), + // Most managers identify their source by name and refuse a URL. + Url = BrokerRequestValidator.ManagerAcceptsSourceUrl(manager) + ? package.Source.Url?.ToString() + : null, }, Package = new RequestPackage { diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index be297c4fdc..ad325501b1 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -38,11 +38,16 @@ public static partial class BrokerRequestValidator /// broker is known to reject. An empty list means no known rule is violated. /// /// The install location that would be sent, or null. + /// + /// The custom parameters that would be sent, when they differ from the ones saved in + /// for the role (for example a parameter added by a retry). + /// public static IReadOnlyList Validate( IPackage package, InstallOptions options, OperationType role, - string? installLocation) + string? installLocation, + IReadOnlyList? customParameters = null) { if (!BrokerRequestBuilder.TryMapManagerName(package.Manager.Name, out ManagerName manager)) { @@ -74,14 +79,58 @@ public static IReadOnlyList Validate( AddIssue(issues, CheckInstallLocation(manager, installLocation)); } - foreach (string parameter in GetCustomParameters(options, role)) + IReadOnlyList parameters = customParameters ?? GetCustomParameters(options, role); + string[] nonEmptyParameters = [.. parameters.Where(parameter => parameter.Trim().Length > 0)]; + if (ManagerRejectsCustomParameters(manager) && nonEmptyParameters.Length > 0) { - AddIssue(issues, CheckCustomParameter(manager, managerName, parameter)); + issues.Add(CoreTools.Translate( + "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.", + managerName, + string.Join(' ', nonEmptyParameters))); + } + else + { + foreach (string parameter in parameters) + { + AddIssue(issues, CheckCustomParameter(manager, managerName, parameter)); + } } return issues; } + /// + /// Whether the broker refuses every custom parameter for this manager: only WinGet and + /// Scoop pass custom parameters to the package manager. + /// + internal static bool ManagerRejectsCustomParameters(ManagerName manager) => + manager + is ManagerName.Chocolatey + or ManagerName.PowerShell + or ManagerName.PowerShell7 + or ManagerName.Npm + or ManagerName.Bun + or ManagerName.Cargo + or ManagerName.Dotnet + or ManagerName.Pip + or ManagerName.Vcpkg; + + /// + /// Whether the broker accepts a package source URL for this manager. The other managers + /// identify their source by name only and refuse a URL, or only accept their default one. + /// + internal static bool ManagerAcceptsSourceUrl(ManagerName manager) => + manager + is not (ManagerName.Chocolatey + or ManagerName.PowerShell + or ManagerName.PowerShell7 + or ManagerName.Npm + or ManagerName.Bun + or ManagerName.Cargo + or ManagerName.Dotnet + or ManagerName.Pip + or ManagerName.Vcpkg); + /// /// Whether the custom parameters of the operation pass WinGet --override or /// --custom, which hand arbitrary arguments to the package installer. diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs index 1a19d5d485..f80323d7f0 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs @@ -64,11 +64,20 @@ public void Describe_BusyBrokerAfterRetries() Assert.Equal(CoreTools.Translate("The Devolutions Agent is busy"), description.Title); } + [Fact] + public void Describe_UnstructuredServiceUnavailableIsBusy() + { + var description = BrokerFailureDescriber.Describe(new BrokerClientException( + BrokerClientErrorKind.BrokerError, "Broker returned HTTP 503", "/v1/capabilities", 503)); + + Assert.Equal(CoreTools.Translate("The Devolutions Agent is busy"), description.Title); + } + [Fact] public void Describe_PausedBrokerWithoutPolicy() { var description = BrokerFailureDescriber.Describe( - BrokerError(409, ErrorCode.BrokerPaused, "no valid policy")); + BrokerError(503, ErrorCode.BrokerPaused, "active policy is unavailable")); Assert.Equal(CoreTools.Translate("Package operations are paused"), description.Title); } diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 632e5c40aa..baed953fcf 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -119,18 +119,18 @@ public void Build_DropArchAndScopeRetry_OmitsScopeAndArchitecture() } [Fact] - public void Build_AllowClobberRetry_AddsTheParameterForPowerShell5Installs() + public void Build_AllowClobberRetry_IsRefusedBecauseTheBrokerAcceptsNoPowerShellParameters() { var package = BuildPowerShellPackage(); package.OverridenOptions.PowerShell_AllowClobber = true; - var request = BrokerRequestBuilder.Build( + var exception = Assert.Throws(() => BrokerRequestBuilder.Build( package, - new InstallOptions { CustomParameters_Install = ["-Proxy", "http://proxy"] }, + new InstallOptions(), OperationType.Install - ); + )); - Assert.Equal(["-Proxy", "http://proxy", "-AllowClobber"], request.Options.CustomParameters); + Assert.Contains(exception.Issues, issue => issue.Contains("-AllowClobber")); } [Fact] @@ -140,7 +140,8 @@ public void Build_AllowClobberRetry_LeavesTheSavedCustomParametersUntouched() package.OverridenOptions.PowerShell_AllowClobber = true; var options = new InstallOptions { CustomParameters_Install = ["-Proxy"] }; - BrokerRequestBuilder.Build(package, options, OperationType.Install); + Assert.Throws( + () => BrokerRequestBuilder.Build(package, options, OperationType.Install)); Assert.Equal(["-Proxy"], options.CustomParameters_Install); } @@ -585,6 +586,49 @@ public void Build_RefusesBatchMetacharactersInWinGetCustomParameters(string para BuildWinGetPackage(), options, OperationType.Install)); } + [Theory] + [InlineData("Chocolatey")] + [InlineData("PowerShell")] + [InlineData("PowerShell7")] + [InlineData("Npm")] + [InlineData("Bun")] + [InlineData("Cargo")] + [InlineData(".NET Tool")] + [InlineData("Pip")] + [InlineData("vcpkg")] + public void Build_SendsNoSourceUrlForManagersThatIdentifySourcesByName(string managerName) + { + var request = BrokerRequestBuilder.Build(BuildPackage(managerName), new InstallOptions(), OperationType.Install); + + Assert.Null(request.Source.Url); + Assert.False(string.IsNullOrEmpty(request.Source.Name)); + } + + [Theory] + [InlineData("Winget")] + [InlineData("Scoop")] + public void Build_KeepsTheSourceUrlForManagersThatAcceptOne(string managerName) + { + var package = BuildPackage(managerName, managerName == "Winget" ? "Contoso.Test" : "7zip"); + + var request = BrokerRequestBuilder.Build(package, new InstallOptions(), OperationType.Install); + + Assert.Equal(package.Source.Url?.ToString(), request.Source.Url); + } + + [Theory] + [InlineData("Chocolatey")] + [InlineData("Npm")] + [InlineData("Pip")] + [InlineData(".NET Tool")] + public void Build_RefusesCustomParametersForManagersThatAcceptNone(string managerName) + { + var options = new InstallOptions { CustomParameters_Install = ["--force"] }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage(managerName), options, OperationType.Install)); + } + [Fact] public void Build_RefusesTheArm32Architecture() { diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index 4357d38deb..3150061038 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -833,7 +833,7 @@ public async Task BrokerPausedWithoutPolicyIsExplained() { var transport = new ScriptedBrokerTransport { - CapabilitiesError = (409, BrokerApiErrorCode.BrokerPaused), + CapabilitiesError = (503, BrokerApiErrorCode.BrokerPaused), }; var (veredict, title, _) = await RunBrokeredOperationCapturingFailure(transport); From 716d22673c381ad6430ae48825768b89931070f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 01:13:48 +0900 Subject: [PATCH 05/17] Announce broker notices through the shared live region Route changed broker validation problems (assertive) and custom-argument warnings (polite) through AccessibilityAnnouncementService, and describe the managers that refuse custom parameters as an explicit list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ViewModels/DialogPages/InstallOptionsViewModel.cs | 11 +++++++++++ .../Views/DialogPages/InstallOptionsControl.axaml | 2 -- .../BrokerRequestValidator.cs | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index ea0155b2de..60d79bec50 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -1,10 +1,12 @@ using System.Collections.ObjectModel; using System.Net.Http; using System.Windows.Input; +using Avalonia.Automation; using Avalonia.Media.Imaging; using Avalonia.Platform; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; +using UniGetUI.Avalonia.Infrastructure; using UniGetUI.Avalonia.Views; using UniGetUI.Core.Language; using UniGetUI.Core.Logging; @@ -567,6 +569,15 @@ private async Task RefreshBrokerNoticesAsync() if (generation != _brokerNoticesGeneration) return; + // Assigning bound text is not reliably announced, so route changes through the app's + // live region. Assigning an unchanged value is a no-op, which suppresses repeats. + if (issuesText != BrokerIssuesText && issuesText.Length > 0) + AccessibilityAnnouncementService.Announce( + $"{BrokerIssuesHeaderLabel} {issuesText}", + AutomationLiveSetting.Assertive); + if (customArgumentsWarning != BrokerCustomArgumentsWarning && customArgumentsWarning.Length > 0) + AccessibilityAnnouncementService.Announce(customArgumentsWarning, AutomationLiveSetting.Polite); + BrokerIssuesText = issuesText; BrokerCustomArgumentsWarning = customArgumentsWarning; } diff --git a/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml b/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml index e838fb75da..d6225a9391 100644 --- a/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml +++ b/src/UniGetUI.Avalonia/Views/DialogPages/InstallOptionsControl.axaml @@ -235,7 +235,6 @@ TextWrapping="Wrap"/> @@ -243,7 +242,6 @@ diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index ad325501b1..9c9f6e784b 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -100,8 +100,8 @@ public static IReadOnlyList Validate( } /// - /// Whether the broker refuses every custom parameter for this manager: only WinGet and - /// Scoop pass custom parameters to the package manager. + /// The managers whose broker command builder refuses every custom parameter. This is an + /// explicit list: managers not listed here (WinGet, Scoop and others) pass them through. /// internal static bool ManagerRejectsCustomParameters(ManagerName manager) => manager From b26f7c3ab9003efa0161936213224276c7f38f43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 01:30:26 +0900 Subject: [PATCH 06/17] Apply the broker API's universal field bounds before sending requests Check package identifiers, custom parameters and versions against the API-wide length and character rules, use canonical SemVer rules for Bun, drop empty custom parameters, and ignore a saved install version for updates and uninstalls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 3 + .../BrokerRequestBuilder.cs | 17 +++-- .../BrokerRequestValidator.cs | 59 ++++++++++++++++- .../BrokerRequestBuilderTests.cs | 65 ++++++++++++++++++- 4 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 71276b1076..72e569b742 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1253,6 +1253,9 @@ "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", "The package manager run by the Devolutions Agent reported an error (exit code {0}).": "The package manager run by the Devolutions Agent reported an error (exit code {0}).", "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.": "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.", + "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 79ab4a69a9..e358fe2b1f 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -47,9 +47,13 @@ public static PackageOperationRequest Build( $"Refusing to build a {manager} broker request for the package identifier \"{package.Id}\": it would be read as a command-line option or split into further arguments." ); - if (!CoreTools.IsOptionSafeValue(options.Version)) + // Only installs send the saved version (see ResolveVersion), so a saved value never + // blocks an update or an uninstall. + string requestedVersion = role is OperationType.Install ? options.Version : ""; + + if (!CoreTools.IsOptionSafeValue(requestedVersion)) throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{options.Version}\" would be read as a command-line option." + $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{requestedVersion}\" would be read as a command-line option." ); if (ManagerCommandLineIsShellInterpreted(manager)) @@ -62,16 +66,17 @@ public static PackageOperationRequest Build( // Managers with known broker version rules are checked against those (stricter, and // aware of each manager's range syntax) by BrokerRequestValidator below. if ( - options.Version.Length > 0 + requestedVersion.Length > 0 && !BrokerRequestValidator.ManagerHasKnownVersionRules(manager) - && !CoreTools.IsValidPackageVersion(options.Version) + && !CoreTools.IsValidPackageVersion(requestedVersion) ) throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{options.Version}\" is not a valid package version." + $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{requestedVersion}\" is not a valid package version." ); } - List customParameters = GetCustomParameters(options, role); + // The broker refuses empty custom parameters; they carry nothing, so they are dropped. + List customParameters = [.. GetCustomParameters(options, role).Where(parameter => parameter.Length > 0)]; if ( manager is ManagerName.PowerShell && role is OperationType.Install diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index 9c9f6e784b..d8e1668bff 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -1,3 +1,5 @@ +using System.Globalization; +using System.Text; using System.Text.RegularExpressions; using Devolutions.Now.Policy.Api; using UniGetUI.Core.Tools; @@ -32,6 +34,8 @@ public static partial class BrokerRequestValidator private const string BatchMetacharactersDisplay = "\" % ! ^ & | < >"; private const int MaxVersionLength = 128; private const int MaxSourceNameLength = 128; + private const int MaxPackageIdLength = 256; + private const int MaxCustomParameterLength = 512; /// /// Returns a localized explanation for every field of the given operation that the package @@ -90,7 +94,8 @@ public static IReadOnlyList Validate( } else { - foreach (string parameter in parameters) + // Empty entries are dropped from the request rather than sent. + foreach (string parameter in parameters.Where(parameter => parameter.Length > 0)) { AddIssue(issues, CheckCustomParameter(manager, managerName, parameter)); } @@ -200,9 +205,17 @@ or ManagerName.Dotnet managerName); } + if (Encoding.UTF8.GetByteCount(version) > MaxVersionLength) + { + return CoreTools.Translate( + "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + version, + MaxVersionLength); + } + if (manager is ManagerName.Bun) { - return version.Length <= MaxVersionLength && SemanticVersionRegex().IsMatch(version) + return IsCanonicalSemanticVersion(version) ? null : CoreTools.Translate( "{0} package versions must be complete semantic versions, such as 1.2.3.", @@ -243,6 +256,20 @@ or ManagerName.Dotnet /// Returns a localized explanation when the broker would reject the package identifier. internal static string? CheckPackageId(ManagerName manager, string managerName, string id) { + // Every request is refused while it is read when its identifier breaks the API-wide rules. + if (Encoding.UTF8.GetByteCount(id) > MaxPackageIdLength) + { + return CoreTools.Translate( + "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + id, + MaxPackageIdLength); + } + + if (id.Length == 0 || !id.All(IsPackageIdCharacter)) + { + return InvalidIdIssue(managerName, id); + } + string? issue = manager switch { ManagerName.Chocolatey when !IsNuGetStylePackageId(id) => CoreTools.Translate( @@ -318,6 +345,14 @@ ManagerName.PowerShell or ManagerName.PowerShell7 when id.IndexOfAny(['*', '?', /// Returns a localized explanation when the broker would reject a custom parameter. internal static string? CheckCustomParameter(ManagerName manager, string managerName, string parameter) { + if (Encoding.UTF8.GetByteCount(parameter) > MaxCustomParameterLength) + { + return CoreTools.Translate( + "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + parameter, + MaxCustomParameterLength); + } + if (RunsThroughBatchScript(manager) && parameter.IndexOfAny(BatchMetacharacters) >= 0) { return CoreTools.Translate( @@ -508,8 +543,26 @@ private static void AddIssue(List issues, string? issue) } } + /// + /// A SemVer 2.0 version with the canonical numeric rules the broker parser applies: no + /// leading zeros in numeric identifiers, and release components that fit 64 bits. + /// + internal static bool IsCanonicalSemanticVersion(string version) + { + Match match = SemanticVersionRegex().Match(version); + return match.Success + && ulong.TryParse(match.Groups["major"].ValueSpan, NumberStyles.None, CultureInfo.InvariantCulture, out _) + && ulong.TryParse(match.Groups["minor"].ValueSpan, NumberStyles.None, CultureInfo.InvariantCulture, out _) + && ulong.TryParse(match.Groups["patch"].ValueSpan, NumberStyles.None, CultureInfo.InvariantCulture, out _); + } + + /// Characters the broker API accepts in any package identifier. + private static bool IsPackageIdCharacter(char c) => + char.IsAsciiLetterOrDigit(c) + || c is '.' or '-' or '_' or '+' or '@' or '/' or ':' or '[' or ']' or ',' or '#' or '$' or '%' or '{' or '}'; + [GeneratedRegex( - @"^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?\z", + @"^(?0|[1-9][0-9]*)\.(?0|[1-9][0-9]*)\.(?0|[1-9][0-9]*)(-(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*)(\.(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*))*)?(\+[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?\z", RegexOptions.CultureInvariant)] private static partial Regex SemanticVersionRegex(); } diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 7601f9828b..2d0c1dd1ec 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -523,6 +523,66 @@ public void Build_KeepsVersionSyntaxTheBrokerAccepts(string managerName, string Assert.Equal(version, request.Package.Version); } + [Theory] + [InlineData("1.2.3-01")] + [InlineData("01.2.3")] + [InlineData("99999999999999999999.0.0")] + public void Build_RefusesBunVersionsThatAreNotCanonicalSemanticVersions(string version) + { + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Bun"), new InstallOptions { Version = version }, OperationType.Install)); + } + + [Theory] + [InlineData("1.2.3-rc.1")] + [InlineData("1.2.3-0a")] + [InlineData("1.2.3+build.01")] + public void Build_KeepsCanonicalBunVersions(string version) + { + var request = BrokerRequestBuilder.Build( + BuildPackage("Bun"), new InstallOptions { Version = version }, OperationType.Install); + + Assert.Equal(version, request.Package.Version); + } + + [Theory] + [InlineData(OperationType.Update)] + [InlineData(OperationType.Uninstall)] + public void Build_IgnoresASavedInstallVersionForOtherOperations(OperationType role) + { + var options = new InstallOptions { Version = "/latest" }; + + var request = BrokerRequestBuilder.Build(BuildWinGetPackage(), options, role); + + Assert.Null(request.Package.Version); + } + + [Fact] + public void Build_RefusesPackageIdentifiersLongerThanTheBrokerAccepts() + { + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Winget", "Contoso." + new string('a', 260)), new InstallOptions(), OperationType.Install)); + } + + [Fact] + public void Build_RefusesCustomParametersLongerThanTheBrokerAccepts() + { + var options = new InstallOptions { CustomParameters_Install = ["--log=" + new string('a', 520)] }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Install)); + } + + [Fact] + public void Build_DropsEmptyCustomParameters() + { + var options = new InstallOptions { CustomParameters_Install = ["", "--silent", ""] }; + + var request = BrokerRequestBuilder.Build(BuildWinGetPackage(), options, OperationType.Install); + + Assert.Equal(["--silent"], request.Options.CustomParameters); + } + [Fact] public void Build_RefusesBunVersionsLongerThanTheBrokerAccepts() { @@ -593,6 +653,9 @@ public void Build_SendsNoInstallLocationForBlankValues(string location) [InlineData("Cargo", "my crate")] [InlineData("Pip", "requests[security]")] [InlineData("Winget", "Contoso.App&Other")] + [InlineData("Npm", "eslint-v9:eslint@^9.x")] + [InlineData("Winget", "Contoso App")] + [InlineData("Winget", "Contoso.Äpp")] public void Build_RefusesPackageIdentifiersTheBrokerRejects(string managerName, string id) { Assert.ThrowsAny(() => BrokerRequestBuilder.Build( @@ -603,7 +666,7 @@ public void Build_RefusesPackageIdentifiersTheBrokerRejects(string managerName, [InlineData("Chocolatey", "notepadplusplus.install")] [InlineData("Chocolatey", "allure")] [InlineData("Npm", "@contoso/tool")] - [InlineData("Npm", "eslint-v9:eslint@^9.x")] + [InlineData("Npm", "eslint-v9:eslint@9.0.0")] [InlineData("Bun", "@contoso/tool")] [InlineData("PowerShell", "Az.Accounts")] [InlineData("Scoop", "7zip")] From a91d1710eef76aa0880139692f92bc79b1645b81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 01:42:38 +0900 Subject: [PATCH 07/17] Drop blank custom parameters from broker requests Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../BrokerRequestBuilder.cs | 4 ++-- .../BrokerRequestValidator.cs | 6 +++--- .../BrokerRequestBuilderTests.cs | 12 +++++++++++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index e358fe2b1f..e83d7340de 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -75,8 +75,8 @@ public static PackageOperationRequest Build( ); } - // The broker refuses empty custom parameters; they carry nothing, so they are dropped. - List customParameters = [.. GetCustomParameters(options, role).Where(parameter => parameter.Length > 0)]; + // The broker refuses empty custom parameters; blank ones carry nothing, so they are dropped. + List customParameters = [.. GetCustomParameters(options, role).Where(parameter => !string.IsNullOrWhiteSpace(parameter))]; if ( manager is ManagerName.PowerShell && role is OperationType.Install diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index d8e1668bff..9a6db04398 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -84,7 +84,7 @@ public static IReadOnlyList Validate( } IReadOnlyList parameters = customParameters ?? GetCustomParameters(options, role); - string[] nonEmptyParameters = [.. parameters.Where(parameter => parameter.Trim().Length > 0)]; + string[] nonEmptyParameters = [.. parameters.Where(parameter => !string.IsNullOrWhiteSpace(parameter))]; if (ManagerRejectsCustomParameters(manager) && nonEmptyParameters.Length > 0) { issues.Add(CoreTools.Translate( @@ -94,8 +94,8 @@ public static IReadOnlyList Validate( } else { - // Empty entries are dropped from the request rather than sent. - foreach (string parameter in parameters.Where(parameter => parameter.Length > 0)) + // Blank entries are dropped from the request rather than sent. + foreach (string parameter in nonEmptyParameters) { AddIssue(issues, CheckCustomParameter(manager, managerName, parameter)); } diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 2d0c1dd1ec..8e7a899fa6 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -573,10 +573,20 @@ public void Build_RefusesCustomParametersLongerThanTheBrokerAccepts() BuildWinGetPackage(), options, OperationType.Install)); } + [Fact] + public void Build_DropsBlankCustomParametersForManagersThatAcceptNone() + { + var options = new InstallOptions { CustomParameters_Install = [" "] }; + + var request = BrokerRequestBuilder.Build(BuildPackage("Chocolatey"), options, OperationType.Install); + + Assert.Empty(request.Options.CustomParameters); + } + [Fact] public void Build_DropsEmptyCustomParameters() { - var options = new InstallOptions { CustomParameters_Install = ["", "--silent", ""] }; + var options = new InstallOptions { CustomParameters_Install = ["", "--silent", " "] }; var request = BrokerRequestBuilder.Build(BuildWinGetPackage(), options, OperationType.Install); From 02632017c3d0ea05449ff1945d7c51002ffb84bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 02:04:09 +0900 Subject: [PATCH 08/17] Point validation failures at every way to fix them Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 2 +- .../BrokerFailureDescriber.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 72e569b742..3dac16880b 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1217,7 +1217,7 @@ "Contact your administrator if you need this operation to be allowed.": "Contact your administrator if you need this operation to be allowed.", "The package broker cannot accept this request": "The package broker cannot accept this request", "The Devolutions Agent would reject the following options of this operation:": "The Devolutions Agent would reject the following options of this operation:", - "Change the installation options of this package, then try again.": "Change the installation options of this package, then try again.", + "Adjust the installation options of this package, or choose another package or source, then try again.": "Adjust the installation options of this package, or choose another package or source, then try again.", "UniGetUI is not authorized to use the Devolutions Agent": "UniGetUI is not authorized to use the Devolutions Agent", "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.": "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.", "Administrator rights are required": "Administrator rights are required", diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs index 5ad2d3accf..049c8e847e 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs @@ -41,7 +41,7 @@ public static BrokerFailureDescription DescribeValidation(IReadOnlyList [ CoreTools.Translate("The Devolutions Agent would reject the following options of this operation:"), .. issues.Select(issue => "• " + issue), - CoreTools.Translate("Change the installation options of this package, then try again."), + CoreTools.Translate("Adjust the installation options of this package, or choose another package or source, then try again."), ])); /// From ffe4304a67379a85ef677f7db507633be276ce06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 02:16:53 +0900 Subject: [PATCH 09/17] Preview every request-blocking problem in the options dialog The dialog now dry-runs the request builder, so the command-line safety guards are reported alongside the field rules. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogPages/InstallOptionsViewModel.cs | 2 +- .../BrokerRequestBuilder.cs | 29 +++++++++++++++++++ .../BrokerRequestBuilderTests.cs | 24 +++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 60d79bec50..1f009dab66 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -550,7 +550,7 @@ private async Task RefreshBrokerNoticesAsync() var applied = await InstallOptionsFactory.LoadApplicableAsync(_package, overridePackageOptions: SnapshotOptions()); // Resolve the location exactly as the brokered operation will (for WinGet updates // this may be the registry-detected location rather than the configured one). - var issues = await Task.Run(() => BrokerRequestValidator.Validate( + var issues = await Task.Run(() => BrokerRequestBuilder.FindProblems( _package, applied, op, diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index e83d7340de..5243d30868 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -140,6 +140,35 @@ manager is ManagerName.PowerShell }; } + /// + /// Every problem that would stop for these values, without building a + /// request: the field rules of as well as the + /// command-line safety guards. Used to preview a brokered operation before it starts. + /// + public static IReadOnlyList FindProblems( + IPackage package, + InstallOptions options, + OperationType role, + string? effectiveInstallLocation = null) + { + if (!SupportsManager(package.Manager.Name)) + return []; + + try + { + Build(package, options, role, effectiveInstallLocation); + return []; + } + catch (BrokerRequestValidationException ex) + { + return ex.Issues; + } + catch (InvalidOperationException ex) + { + return [ex.Message]; + } + } + private static Operation MapOperation(OperationType role) => role switch { OperationType.Install => Operation.Install, diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 8e7a899fa6..33947bc6b2 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -583,6 +583,30 @@ public void Build_DropsBlankCustomParametersForManagersThatAcceptNone() Assert.Empty(request.Options.CustomParameters); } + [Fact] + public void FindProblems_ReportsTheCommandLineSafetyGuardsToo() + { + var problems = BrokerRequestBuilder.FindProblems( + BuildPackage("Winget", "-foo"), new InstallOptions(), OperationType.Install); + + Assert.NotEmpty(problems); + } + + [Fact] + public void FindProblems_IsEmptyForAValidRequest() + { + Assert.Empty(BrokerRequestBuilder.FindProblems(BuildWinGetPackage(), new InstallOptions(), OperationType.Install)); + } + + [Fact] + public void FindProblems_ReportsFieldValidationIssues() + { + var problems = BrokerRequestBuilder.FindProblems( + BuildWinGetPackage(), new InstallOptions(), OperationType.Install, "Tools\\App"); + + Assert.Single(problems); + } + [Fact] public void Build_DropsEmptyCustomParameters() { From ebe85cf48f1e2930b6dadacc4c32d1550b5d86ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 02:31:31 +0900 Subject: [PATCH 10/17] Localize the command-line safety guards of broker requests Move the option-safety and shell-identifier guards into the request validator so they are reported as localized issues, in the operation failure and in the options dialog. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 2 + .../BrokerRequestBuilder.cs | 48 ------------ .../BrokerRequestValidator.cs | 76 ++++++++++++++++--- .../BrokerRequestBuilderTests.cs | 4 +- 4 files changed, 70 insertions(+), 60 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 3dac16880b..e1b860ed62 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1256,6 +1256,8 @@ "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.": "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.", + "The version \"{0}\" would be read as a command-line option.": "The version \"{0}\" would be read as a command-line option.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 5243d30868..b8cf0296d2 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -34,47 +34,6 @@ public static PackageOperationRequest Build( ManagerName manager = MapManagerName(package.Manager.Name); - // This path does not go through BasePkgOperationHelper, so the checks that apply to every - // manager have to be repeated here: the broker builds a command line from these values, - // and an identifier such as "requests --index-url https://host" would become real options. - if ( - !CoreTools.IsOptionSafeIdentifier( - package.Id, - package.Manager.IdentifiersAreQuotedOnCommandLine - ) - ) - throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for the package identifier \"{package.Id}\": it would be read as a command-line option or split into further arguments." - ); - - // Only installs send the saved version (see ResolveVersion), so a saved value never - // blocks an update or an uninstall. - string requestedVersion = role is OperationType.Install ? options.Version : ""; - - if (!CoreTools.IsOptionSafeValue(requestedVersion)) - throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{requestedVersion}\" would be read as a command-line option." - ); - - if (ManagerCommandLineIsShellInterpreted(manager)) - { - if (!CoreTools.IsValidPackageIdentifier(package.Id)) - throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for the package identifier \"{package.Id}\": it is not a valid package identifier." - ); - - // Managers with known broker version rules are checked against those (stricter, and - // aware of each manager's range syntax) by BrokerRequestValidator below. - if ( - requestedVersion.Length > 0 - && !BrokerRequestValidator.ManagerHasKnownVersionRules(manager) - && !CoreTools.IsValidPackageVersion(requestedVersion) - ) - throw new InvalidOperationException( - $"Refusing to build a {manager} broker request for package {package.Id}: the requested version \"{requestedVersion}\" is not a valid package version." - ); - } - // The broker refuses empty custom parameters; blank ones carry nothing, so they are dropped. List customParameters = [.. GetCustomParameters(options, role).Where(parameter => !string.IsNullOrWhiteSpace(parameter))]; if ( @@ -193,13 +152,6 @@ private static ManagerName MapManagerName(string managerName) => ? mapped : throw new ArgumentException($"Unsupported manager for the broker: {managerName}"); - private static bool ManagerCommandLineIsShellInterpreted(ManagerName manager) => - manager - is ManagerName.PowerShell - or ManagerName.PowerShell7 - or ManagerName.Scoop - or ManagerName.Npm; - /// /// The concrete version the operation installs, when UniGetUI knows it. /// diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index 9a6db04398..b0ab89bbf7 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -61,12 +61,58 @@ public static IReadOnlyList Validate( string managerName = package.Manager.DisplayName; List issues = []; - AddIssue(issues, CheckPackageId(manager, managerName, package.Id)); + // Brokered operations do not go through BasePkgOperationHelper, so the command-line + // safety guards that apply to every manager are repeated here: the broker builds a + // command line from these values, and an identifier such as + // "requests --index-url https://host" would become real options. + // Only installs send the saved version (see BrokerRequestBuilder.ResolveVersion), so a + // saved value never blocks an update or an uninstall. + string requestedVersion = role is OperationType.Install ? options.Version : ""; + bool idIsOptionSafe = CoreTools.IsOptionSafeIdentifier( + package.Id, + package.Manager.IdentifiersAreQuotedOnCommandLine); + if (!idIsOptionSafe) + { + issues.Add(CoreTools.Translate( + "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.", + package.Id)); + } + + if (!CoreTools.IsOptionSafeValue(requestedVersion)) + { + issues.Add(CoreTools.Translate( + "The version \"{0}\" would be read as a command-line option.", + requestedVersion)); + } + + if (ManagerCommandLineIsShellInterpreted(manager)) + { + if (idIsOptionSafe && !CoreTools.IsValidPackageIdentifier(package.Id)) + { + issues.Add(InvalidIdIssue(managerName, package.Id)); + } + + // Managers with known broker version rules are checked against those below + // (stricter, and aware of each manager's range syntax). + if (requestedVersion.Length > 0 + && !ManagerHasKnownVersionRules(manager) + && CoreTools.IsOptionSafeValue(requestedVersion) + && !CoreTools.IsValidPackageVersion(requestedVersion)) + { + issues.Add(InvalidVersionIssue(managerName, requestedVersion)); + } + } + + if (idIsOptionSafe) + { + AddIssue(issues, CheckPackageId(manager, managerName, package.Id)); + } + AddIssue(issues, CheckSourceName(manager, managerName, package.Source.Name)); - if (role is OperationType.Install && options.Version.Length > 0) + if (requestedVersion.Length > 0 && CoreTools.IsOptionSafeValue(requestedVersion)) { - AddIssue(issues, CheckVersion(manager, managerName, options.Version)); + AddIssue(issues, CheckVersion(manager, managerName, requestedVersion)); } if (role is not OperationType.Uninstall @@ -101,7 +147,7 @@ public static IReadOnlyList Validate( } } - return issues; + return [.. issues.Distinct()]; } /// @@ -245,12 +291,8 @@ or ManagerName.Dotnet || c is '.' or '-' or '+' or '_' || extraCharacters.Contains(c)); - return valid - ? null - : CoreTools.Translate( - "The version \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", - version, - managerName); + return valid ? null : InvalidVersionIssue(managerName, version); + } /// Returns a localized explanation when the broker would reject the package identifier. @@ -521,6 +563,20 @@ private static string RegistryNameIssue(string managerName, string id) => managerName, id); + private static string InvalidVersionIssue(string managerName, string version) => + CoreTools.Translate( + "The version \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", + version, + managerName); + + /// Managers whose broker command runs through a PowerShell script. + private static bool ManagerCommandLineIsShellInterpreted(ManagerName manager) => + manager + is ManagerName.PowerShell + or ManagerName.PowerShell7 + or ManagerName.Scoop + or ManagerName.Npm; + private static string InvalidIdIssue(string managerName, string id) => CoreTools.Translate( "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 33947bc6b2..bdf012ecbd 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -365,7 +365,7 @@ public void Build_RefusesAnInjectedIdentifierForShellInterpretedManagers() .WithId("powershell-yaml; Start-Process calc") .Build(); - Assert.Throws( + Assert.Throws( () => BrokerRequestBuilder.Build(package, new InstallOptions(), OperationType.Install) ); } @@ -589,7 +589,7 @@ public void FindProblems_ReportsTheCommandLineSafetyGuardsToo() var problems = BrokerRequestBuilder.FindProblems( BuildPackage("Winget", "-foo"), new InstallOptions(), OperationType.Install); - Assert.NotEmpty(problems); + Assert.Contains(problems, problem => problem.Contains("-foo") && !problem.StartsWith("Refusing")); } [Fact] From 63eb30d9e1c0f6ca36675c9f30b231a312d1854d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 02:48:30 +0900 Subject: [PATCH 11/17] Validate elevation-dependent broker rules and keep forbidden reasons Report elevated operations for managers that run per user and pre/post commands on elevated or machine-scope operations before sending them, sharing the elevation predicate with the operation. Describe a generic Forbidden response with the broker's reason instead of asking for administrator rights. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 4 ++ .../DialogPages/InstallOptionsViewModel.cs | 3 +- .../BrokerFailureDescriber.cs | 10 +++- .../BrokerRequestBuilder.cs | 10 ++-- .../BrokerRequestValidator.cs | 48 +++++++++++++++++++ .../PackageOperations.cs | 3 +- .../BrokerFailureDescriberTests.cs | 12 ++++- .../BrokerRequestBuilderTests.cs | 40 ++++++++++++++++ 8 files changed, 121 insertions(+), 9 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index e1b860ed62..a8f46b3bea 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1258,6 +1258,10 @@ "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.": "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.", "The version \"{0}\" would be read as a command-line option.": "The version \"{0}\" would be read as a command-line option.", + "The Devolutions Agent refused this operation": "The Devolutions Agent refused this operation", + "The Devolutions Agent does not allow this operation at the moment. Contact your administrator if the problem persists.": "The Devolutions Agent does not allow this operation at the moment. Contact your administrator if the problem persists.", + "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.": "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.", + "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.": "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 1f009dab66..03f152d319 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -600,8 +600,7 @@ private string DescribeBrokerCustomArgumentsRisk(InstallOptions applied, Operati // Same elevation predicate as the brokered operation: the package's own requirement // (e.g. a WinGet installer that needs elevation) counts as well as the checkbox. - bool runsElevated = !Settings.Get(Settings.K.ProhibitElevation) - && (_package.OverridenOptions.RunAsAdministrator is true || applied.RunAsAdministrator); + bool runsElevated = BrokerRequestValidator.RequestsElevation(_package, applied); return parameters.Count > 0 && runsElevated ? CoreTools.Translate( diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs index 049c8e847e..263c48a3bf 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs @@ -59,7 +59,7 @@ .. issues.Select(issue => "• " + issue), "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.")); } - if (exception.StatusCode is 403 || code is ErrorCode.Forbidden or ErrorCode.AdministratorRequired) + if (code is ErrorCode.AdministratorRequired) { return new( CoreTools.Translate("Administrator rights are required"), @@ -110,6 +110,14 @@ public static BrokerFailureDescription Describe(BrokerClientException exception) CoreTools.Translate("The Devolutions Agent did not accept one of the options of this operation."), DescribeErrorDetails(error, brokerMessage))); + case ErrorCode.Forbidden: + // Also used when the active policy is outside its validity period. + return new( + CoreTools.Translate("The Devolutions Agent refused this operation"), + WithDetails( + CoreTools.Translate("The Devolutions Agent does not allow this operation at the moment. Contact your administrator if the problem persists."), + DescribeErrorDetails(error, brokerMessage))); + case ErrorCode.PayloadTooLarge: return new( CoreTools.Translate("The request is too large"), diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index b8cf0296d2..e127913004 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -78,9 +78,7 @@ manager is ManagerName.PowerShell { // The per-package scope override takes precedence over the saved options, // matching the local WinGet execution path. - Scope = dropArchAndScope - ? null - : MapScope(manager, package.OverridenOptions.Scope ?? options.InstallationScope), + Scope = ResolveScope(manager, package, options), Interactive = options.InteractiveInstallation, SkipHashCheck = options.SkipHashCheck, PreRelease = options.PreRelease, @@ -226,6 +224,12 @@ internal static bool TryMapManagerName(string managerName, out ManagerName mappe return result is not null; } + /// The scope sent to the broker for these options, or null to let it decide. + internal static Scope? ResolveScope(ManagerName manager, IPackage package, InstallOptions options) => + package.OverridenOptions.WinGet_DropArchAndScope + ? null + : MapScope(manager, package.OverridenOptions.Scope ?? options.InstallationScope); + private static Scope? MapScope(ManagerName manager, string? scope) { if (string.IsNullOrEmpty(scope)) diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index b0ab89bbf7..2413868ce7 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -2,6 +2,7 @@ using System.Text; using System.Text.RegularExpressions; using Devolutions.Now.Policy.Api; +using UniGetUI.Core.SettingsEngine; using UniGetUI.Core.Tools; using UniGetUI.PackageEngine.Enums; using UniGetUI.PackageEngine.Interfaces; @@ -129,6 +130,23 @@ public static IReadOnlyList Validate( AddIssue(issues, CheckInstallLocation(manager, installLocation)); } + bool elevated = RequestsElevation(package, options); + if (elevated && ManagerRejectsElevation(manager)) + { + issues.Add(CoreTools.Translate( + "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.", + managerName)); + } + + // Pre/post commands would run with the elevated token, so the broker refuses them for + // elevated and machine-scope operations whatever the policy says. + if (HasPrePostCommands(options, role) + && (elevated || BrokerRequestBuilder.ResolveScope(manager, package, options) is Scope.Machine)) + { + issues.Add(CoreTools.Translate( + "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.")); + } + IReadOnlyList parameters = customParameters ?? GetCustomParameters(options, role); string[] nonEmptyParameters = [.. parameters.Where(parameter => !string.IsNullOrWhiteSpace(parameter))]; if (ManagerRejectsCustomParameters(manager) && nonEmptyParameters.Length > 0) @@ -150,6 +168,36 @@ public static IReadOnlyList Validate( return [.. issues.Distinct()]; } + /// + /// Whether the operation asks the broker for elevation: the package's own requirement (for + /// example a WinGet installer that needs it) or the "Run as admin" option, unless elevation + /// is prohibited. Shared by the brokered operation and the installation options dialog. + /// + public static bool RequestsElevation(IPackage package, InstallOptions options) => + !Settings.Get(Settings.K.ProhibitElevation) + && (package.OverridenOptions.RunAsAdministrator is true || options.RunAsAdministrator); + + /// Managers whose broker command builder refuses elevated operations. + private static bool ManagerRejectsElevation(ManagerName manager) => + manager + is ManagerName.Npm + or ManagerName.Scoop + or ManagerName.Bun + or ManagerName.Cargo + or ManagerName.Pip + or ManagerName.Vcpkg; + + private static bool HasPrePostCommands(InstallOptions options, OperationType role) => role switch + { + OperationType.Install => !string.IsNullOrWhiteSpace(options.PreInstallCommand) + || !string.IsNullOrWhiteSpace(options.PostInstallCommand), + OperationType.Update => !string.IsNullOrWhiteSpace(options.PreUpdateCommand) + || !string.IsNullOrWhiteSpace(options.PostUpdateCommand), + OperationType.Uninstall => !string.IsNullOrWhiteSpace(options.PreUninstallCommand) + || !string.IsNullOrWhiteSpace(options.PostUninstallCommand), + _ => false, + }; + /// /// The managers whose broker command builder refuses every custom parameter. This is an /// explicit list: managers not listed here (WinGet, Scoop and others) pass them through. diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index a0785537c9..4919e8baf4 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -168,8 +168,7 @@ public static bool HasPendingOperation(IPackage package, OperationType role) } private bool RequiresAdminRights() => - !Settings.Get(Settings.K.ProhibitElevation) - && (Package.OverridenOptions.RunAsAdministrator is true || Options.RunAsAdministrator); + BrokerRequestValidator.RequestsElevation(Package, Options); private volatile int _ranElevated = -1; diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs index f80323d7f0..ddb8ced7f0 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerFailureDescriberTests.cs @@ -86,7 +86,6 @@ public void Describe_PausedBrokerWithoutPolicy() [InlineData(401, ErrorCode.Unauthorized, "UniGetUI is not authorized to use the Devolutions Agent")] [InlineData(401, ErrorCode.Unauthenticated, "UniGetUI is not authorized to use the Devolutions Agent")] [InlineData(403, ErrorCode.AdministratorRequired, "Administrator rights are required")] - [InlineData(403, ErrorCode.Forbidden, "Administrator rights are required")] public void Describe_AuthorizationFailures(int statusCode, ErrorCode code, string expectedTitle) { var exception = BrokerError(statusCode, code); @@ -95,6 +94,17 @@ public void Describe_AuthorizationFailures(int statusCode, ErrorCode code, strin Assert.Equal(CoreTools.Translate(expectedTitle), BrokerFailureDescriber.Describe(exception).Title); } + [Fact] + public void Describe_ForbiddenKeepsTheBrokerReason() + { + var exception = BrokerError(403, ErrorCode.Forbidden, "policy is not valid until 2026-11-01"); + + Assert.Null(BrokerFailureDescriber.DescribeAccessFailure(exception)); + var description = BrokerFailureDescriber.Describe(exception); + Assert.Equal(CoreTools.Translate("The Devolutions Agent refused this operation"), description.Title); + Assert.Contains("policy is not valid until 2026-11-01", description.Message); + } + [Fact] public void DescribeAccessFailure_IgnoresOtherFailures() { diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index bdf012ecbd..72a3698b04 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -798,6 +798,46 @@ public void Build_RefusesCustomParametersForManagersThatAcceptNone(string manage BuildPackage(managerName), options, OperationType.Install)); } + [Theory] + [InlineData("Npm")] + [InlineData("Scoop")] + [InlineData("Pip")] + public void Build_RefusesElevatedOperationsForManagersThatRunPerUser(string managerName) + { + var options = new InstallOptions { RunAsAdministrator = true }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage(managerName, managerName == "Scoop" ? "7zip" : "contoso-tool"), options, OperationType.Install)); + } + + [Fact] + public void Build_RefusesPrePostCommandsForElevatedOperations() + { + var options = new InstallOptions { RunAsAdministrator = true, PreInstallCommand = "echo before" }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Install)); + } + + [Fact] + public void Build_RefusesPrePostCommandsForMachineScopeOperations() + { + var options = new InstallOptions { InstallationScope = PackageScope.Machine, PostUpdateCommand = "echo after" }; + + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Update)); + } + + [Fact] + public void Build_KeepsPrePostCommandsForStandardUserOperations() + { + var options = new InstallOptions { InstallationScope = PackageScope.User, PreInstallCommand = "echo before" }; + + var request = BrokerRequestBuilder.Build(BuildWinGetPackage(), options, OperationType.Install); + + Assert.Equal("echo before", request.Options.PreOperationCommand); + } + [Fact] public void Build_RefusesTheArm32Architecture() { From 8e48d9449a8aeb0ee22a6b2dba510b4d8de8fcad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 03:01:38 +0900 Subject: [PATCH 12/17] Treat refused capabilities as an authorization failure and refresh on command edits A 401 or 403 on the capabilities request now reports that the broker does not accept this client, without reclassifying execution-time Forbidden responses. The options dialog re-checks broker rules when a pre/post command changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogPages/InstallOptionsViewModel.cs | 6 ++++++ .../BrokerFailureDescriber.cs | 20 +++++++++++++++---- .../PackageOperations.cs | 2 +- .../PackageOperationsTests.cs | 10 +++++++--- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 03f152d319..c2f119ec7c 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -240,6 +240,12 @@ partial void OnSelectedProfileChanged(string? value) [ObservableProperty] private string _postUninstallText = ""; [ObservableProperty] private bool _abortUninstall; + partial void OnPreInstallTextChanged(string value) => Refresh(); + partial void OnPostInstallTextChanged(string value) => Refresh(); + partial void OnPreUpdateTextChanged(string value) => Refresh(); + partial void OnPostUpdateTextChanged(string value) => Refresh(); + partial void OnPreUninstallTextChanged(string value) => Refresh(); + partial void OnPostUninstallTextChanged(string value) => Refresh(); // ── Close apps tab ──────────────────────────────────────────────────────── public ObservableCollection KillProcessEntries { get; } = []; diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs index 263c48a3bf..1aaa797f9d 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerFailureDescriber.cs @@ -53,10 +53,7 @@ .. issues.Select(issue => "• " + issue), ErrorCode? code = exception.BrokerError?.Code; if (exception.StatusCode is 401 || code is ErrorCode.Unauthorized or ErrorCode.Unauthenticated) { - return new( - CoreTools.Translate("UniGetUI is not authorized to use the Devolutions Agent"), - CoreTools.Translate( - "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.")); + return DescribeClientNotAuthorized(); } if (code is ErrorCode.AdministratorRequired) @@ -70,6 +67,21 @@ .. issues.Select(issue => "• " + issue), return null; } + /// + /// Explains a refused capabilities request. The broker only requires an authenticated client + /// there, so any 401 or 403 means it does not accept this copy of UniGetUI. Returns null + /// for other failures. + /// + public static BrokerFailureDescription? DescribeCapabilitiesAccessFailure(BrokerClientException exception) => + DescribeAccessFailure(exception) + ?? (exception.StatusCode is 403 ? DescribeClientNotAuthorized() : null); + + private static BrokerFailureDescription DescribeClientNotAuthorized() => + new( + CoreTools.Translate("UniGetUI is not authorized to use the Devolutions Agent"), + CoreTools.Translate( + "The Devolutions Agent only accepts requests from signed, unmodified copies of UniGetUI. If you are running a development or self-built version, install an official release of UniGetUI and try again.")); + /// Explains a failed broker request. public static BrokerFailureDescription Describe(BrokerClientException exception) { diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index 4919e8baf4..8e0727f183 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -553,7 +553,7 @@ private async Task PerformBrokerOperation() Line("Broker operation was canceled.", LineType.Information); return OperationVeredict.Canceled; } - catch (BrokerClientException ex) when (BrokerFailureDescriber.DescribeAccessFailure(ex) is { } accessFailure) + catch (BrokerClientException ex) when (BrokerFailureDescriber.DescribeCapabilitiesAccessFailure(ex) is { } accessFailure) { // The broker is running but does not accept this client: report it as // unavailable for this copy of UniGetUI, with the reason, instead of a diff --git a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs index 3150061038..c7235e4e5d 100644 --- a/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/PackageOperationsTests.cs @@ -798,8 +798,12 @@ public async Task BrokerPolicyDenialExplainsTheReasonAndRule() Assert.Contains("Versions before 2.0 are not allowed", message); } - [Fact] - public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizationFailure() + [Theory] + [InlineData(401, BrokerApiErrorCode.Unauthorized)] + [InlineData(403, BrokerApiErrorCode.Forbidden)] + public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizationFailure( + int statusCode, + BrokerApiErrorCode code) { bool originalSetting = Settings.Get(Settings.K.UseAgentBroker); BrokerFailureDescription? notified = null; @@ -809,7 +813,7 @@ public async Task BrokerRefusingTheCapabilitiesRequestIsReportedAsAnAuthorizatio { var transport = new ScriptedBrokerTransport { - CapabilitiesError = (401, BrokerApiErrorCode.Unauthorized), + CapabilitiesError = (statusCode, code), }; var (veredict, title, message) = await RunBrokeredOperationCapturingFailure(transport); From 137755672cb7af88841e172d888cbdeb21a99bc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 03:15:45 +0900 Subject: [PATCH 13/17] Report PowerShell command conflicts instead of an unsupported retry The broker accepts no PowerShell custom parameter, so the AllowClobber retry is no longer sent: the operation explains the command conflict instead. The options dialog orders whole refreshes (command preview included), and length limits are reported in bytes, as the broker counts them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 8 ++++--- .../DialogPages/InstallOptionsViewModel.cs | 16 +++++++------ .../BrokerRequestBuilder.cs | 19 +++++++++------ .../BrokerRequestValidator.cs | 6 ++--- .../PackageOperations.cs | 10 ++++++++ .../BrokerRequestBuilderTests.cs | 23 ++++++++----------- 6 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index a8f46b3bea..7a2830c6e8 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1253,15 +1253,17 @@ "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.": "The package identifier \"{0}\" contains characters that the Devolutions Agent does not accept for {1} packages.", "The package manager run by the Devolutions Agent reported an error (exit code {0}).": "The package manager run by the Devolutions Agent reported an error (exit code {0}).", "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.": "The Devolutions Agent does not accept custom arguments for {0} packages ({1}). Remove them from the installation options of this package.", - "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", - "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", - "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.": "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The version \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).": "The version \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", + "The package identifier \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).": "The package identifier \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", + "The custom argument \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).": "The custom argument \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.": "The package identifier \"{0}\" would be read as a command-line option or split into several arguments.", "The version \"{0}\" would be read as a command-line option.": "The version \"{0}\" would be read as a command-line option.", "The Devolutions Agent refused this operation": "The Devolutions Agent refused this operation", "The Devolutions Agent does not allow this operation at the moment. Contact your administrator if the problem persists.": "The Devolutions Agent does not allow this operation at the moment. Contact your administrator if the problem persists.", "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.": "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.", "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.": "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.", + "The module conflicts with installed commands": "The module conflicts with installed commands", + "{0} provides commands that another installed module already provides. Installing it anyway requires the -AllowClobber option, which cannot be used through the Devolutions Agent.": "{0} provides commands that another installed module already provides. Installing it anyway requires the -AllowClobber option, which cannot be used through the Devolutions Agent.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", "Policy management is unsupported": "Policy management is unsupported", diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index c2f119ec7c..24038b6198 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -527,8 +527,12 @@ private void ApplyProfileEnableState() private async Task RefreshCommandPreviewAsync() { if (!_uiLoaded) return; - CommandPreview = await BuildCurrentCommandAsync(); - await RefreshBrokerNoticesAsync(); + // Edits start overlapping refreshes; only the latest one may publish its results. + int generation = Interlocked.Increment(ref _refreshGeneration); + string command = await BuildCurrentCommandAsync(); + if (generation != _refreshGeneration) return; + CommandPreview = command; + await RefreshBrokerNoticesAsync(generation); } /// @@ -536,11 +540,10 @@ private async Task RefreshCommandPreviewAsync() /// reject and warns about custom WinGet installer arguments, using the same rules as the /// request builder so the user can fix them before starting the operation. /// - private int _brokerNoticesGeneration; + private int _refreshGeneration; - private async Task RefreshBrokerNoticesAsync() + private async Task RefreshBrokerNoticesAsync(int generation) { - int generation = Interlocked.Increment(ref _brokerNoticesGeneration); if (!IsBrokered(_package)) { BrokerIssuesText = ""; @@ -571,8 +574,7 @@ private async Task RefreshBrokerNoticesAsync() customArgumentsWarning = ""; } - // Edits start overlapping refreshes; only the latest one may publish its result. - if (generation != _brokerNoticesGeneration) + if (generation != _refreshGeneration) return; // Assigning bound text is not reliably announced, so route changes through the app's diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index e127913004..3b8acdef7d 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -36,14 +36,8 @@ public static PackageOperationRequest Build( // The broker refuses empty custom parameters; blank ones carry nothing, so they are dropped. List customParameters = [.. GetCustomParameters(options, role).Where(parameter => !string.IsNullOrWhiteSpace(parameter))]; - if ( - manager is ManagerName.PowerShell - && role is OperationType.Install - && package.OverridenOptions.PowerShell_AllowClobber - ) - customParameters = [.. customParameters, "-AllowClobber"]; - // Validate what will actually be sent, including parameters added by a retry. + // Validate what will actually be sent. IReadOnlyList issues = BrokerRequestValidator.Validate( package, options, @@ -126,6 +120,17 @@ public static IReadOnlyList FindProblems( } } + /// + /// Whether the operation is the local PowerShell 5 retry that adds -AllowClobber + /// after a command conflict. The broker accepts no PowerShell custom parameter, so the retry + /// cannot be sent and the conflict has to be reported instead. + /// + public static bool IsUnsupportedAllowClobberRetry(IPackage package, OperationType role) => + role is OperationType.Install + && package.OverridenOptions.PowerShell_AllowClobber + && TryMapManagerName(package.Manager.Name, out ManagerName manager) + && manager is ManagerName.PowerShell; + private static Operation MapOperation(OperationType role) => role switch { OperationType.Install => Operation.Install, diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index 2413868ce7..c9744c03f8 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -302,7 +302,7 @@ or ManagerName.Dotnet if (Encoding.UTF8.GetByteCount(version) > MaxVersionLength) { return CoreTools.Translate( - "The version \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The version \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", version, MaxVersionLength); } @@ -350,7 +350,7 @@ or ManagerName.Dotnet if (Encoding.UTF8.GetByteCount(id) > MaxPackageIdLength) { return CoreTools.Translate( - "The package identifier \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The package identifier \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", id, MaxPackageIdLength); } @@ -438,7 +438,7 @@ ManagerName.PowerShell or ManagerName.PowerShell7 when id.IndexOfAny(['*', '?', if (Encoding.UTF8.GetByteCount(parameter) > MaxCustomParameterLength) { return CoreTools.Translate( - "The custom argument \"{0}\" is longer than the {1} characters the Devolutions Agent accepts.", + "The custom argument \"{0}\" is longer than the Devolutions Agent accepts (at most {1} bytes).", parameter, MaxCustomParameterLength); } diff --git a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs index 8e0727f183..8294e8d94a 100644 --- a/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs +++ b/src/UniGetUI.PackageEngine.Operations/PackageOperations.cs @@ -389,6 +389,16 @@ private async Task PerformBrokerOperation() _brokerStreamedOutput = null; Line("Routing operation through Devolutions Agent broker...", LineType.Information); + if (BrokerRequestBuilder.IsUnsupportedAllowClobberRetry(Package, Role)) + { + Line("The module conflicts with commands that are already installed; the broker cannot retry with -AllowClobber.", LineType.Error); + return FailWith(new BrokerFailureDescription( + CoreTools.Translate("The module conflicts with installed commands"), + CoreTools.Translate( + "{0} provides commands that another installed module already provides. Installing it anyway requires the -AllowClobber option, which cannot be used through the Devolutions Agent.", + Package.Name))); + } + // Apply manager-specific elevation requirements (e.g. WinGet's detection of // machine-scope or elevation-requiring installers) before deciding the requested // elevation, mirroring the local execution path where this runs as part of diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 72a3698b04..ead727690f 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -191,31 +191,26 @@ public void Build_DropArchAndScopeRetry_OmitsScopeAndArchitecture() } [Fact] - public void Build_AllowClobberRetry_IsRefusedBecauseTheBrokerAcceptsNoPowerShellParameters() + public void Build_AllowClobberRetry_IsNotSentToTheBroker() { var package = BuildPowerShellPackage(); package.OverridenOptions.PowerShell_AllowClobber = true; - var exception = Assert.Throws(() => BrokerRequestBuilder.Build( - package, - new InstallOptions(), - OperationType.Install - )); + var request = BrokerRequestBuilder.Build(package, new InstallOptions(), OperationType.Install); - Assert.Contains(exception.Issues, issue => issue.Contains("-AllowClobber")); + Assert.DoesNotContain("-AllowClobber", request.Options.CustomParameters); + Assert.True(BrokerRequestBuilder.IsUnsupportedAllowClobberRetry(package, OperationType.Install)); } - [Fact] - public void Build_AllowClobberRetry_LeavesTheSavedCustomParametersUntouched() + [Theory] + [InlineData(OperationType.Update)] + [InlineData(OperationType.Uninstall)] + public void IsUnsupportedAllowClobberRetry_IsInstallOnly(OperationType role) { var package = BuildPowerShellPackage(); package.OverridenOptions.PowerShell_AllowClobber = true; - var options = new InstallOptions { CustomParameters_Install = ["-Proxy"] }; - - Assert.Throws( - () => BrokerRequestBuilder.Build(package, options, OperationType.Install)); - Assert.Equal(["-Proxy"], options.CustomParameters_Install); + Assert.False(BrokerRequestBuilder.IsUnsupportedAllowClobberRetry(package, role)); } [Theory] From 72852d09047da5dcad319a171edc0849e5f77d3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 03:27:39 +0900 Subject: [PATCH 14/17] Apply manager elevation requirements before previewing broker rules Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogPages/InstallOptionsViewModel.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 24038b6198..a4ab3386d8 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -559,11 +559,17 @@ private async Task RefreshBrokerNoticesAsync(int generation) var applied = await InstallOptionsFactory.LoadApplicableAsync(_package, overridePackageOptions: SnapshotOptions()); // Resolve the location exactly as the brokered operation will (for WinGet updates // this may be the registry-detected location rather than the configured one). - var issues = await Task.Run(() => BrokerRequestBuilder.FindProblems( - _package, - applied, - op, - PackageOperation.GetBrokerInstallLocation(_package, applied, op))); + var issues = await Task.Run(() => + { + // Apply the manager's own elevation requirements first, as the brokered + // operation does (e.g. a WinGet installer that must run elevated). + _package.Manager.OperationHelper.ApplyElevationRequirements(_package, applied, op); + return BrokerRequestBuilder.FindProblems( + _package, + applied, + op, + PackageOperation.GetBrokerInstallLocation(_package, applied, op)); + }); issuesText = string.Join(Environment.NewLine, issues.Select(issue => "• " + issue)); customArgumentsWarning = DescribeBrokerCustomArgumentsRisk(applied, op); } From 50d8e6f9b562ff6c3d3ae5bd243ce89a7746e1e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 03:40:37 +0900 Subject: [PATCH 15/17] Ignore blank arguments in the WinGet warning and isolate elevation in tests Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DialogPages/InstallOptionsViewModel.cs | 2 +- .../BrokerRequestBuilderTests.cs | 24 +++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index a4ab3386d8..7c24799952 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -616,7 +616,7 @@ private string DescribeBrokerCustomArgumentsRisk(InstallOptions applied, Operati // (e.g. a WinGet installer that needs elevation) counts as well as the checkbox. bool runsElevated = BrokerRequestValidator.RequestsElevation(_package, applied); - return parameters.Count > 0 && runsElevated + return parameters.Any(parameter => !string.IsNullOrWhiteSpace(parameter)) && runsElevated ? CoreTools.Translate( "Custom arguments are passed to WinGet by the Devolutions Agent, which runs this operation with administrator rights. Your organization's policy may block custom arguments.") : ""; diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index ead727690f..3e148987ab 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -1,5 +1,6 @@ using Devolutions.Now.Policy.Api; using Devolutions.Now.Policy.Client; +using UniGetUI.Core.SettingsEngine; using UniGetUI.PackageEngine.AgentBroker; using UniGetUI.PackageEngine.Serializable; using UniGetUI.PackageEngine.Tests.Infrastructure.Builders; @@ -801,8 +802,23 @@ public void Build_RefusesElevatedOperationsForManagersThatRunPerUser(string mana { var options = new InstallOptions { RunAsAdministrator = true }; - Assert.Throws(() => BrokerRequestBuilder.Build( - BuildPackage(managerName, managerName == "Scoop" ? "7zip" : "contoso-tool"), options, OperationType.Install)); + WithElevationAllowed(() => Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage(managerName, managerName == "Scoop" ? "7zip" : "contoso-tool"), options, OperationType.Install))); + } + + /// Runs an assertion with elevation allowed, whatever the machine's settings are. + private static void WithElevationAllowed(Action assertion) + { + bool original = Settings.Get(Settings.K.ProhibitElevation); + Settings.Set(Settings.K.ProhibitElevation, false); + try + { + assertion(); + } + finally + { + Settings.Set(Settings.K.ProhibitElevation, original); + } } [Fact] @@ -810,8 +826,8 @@ public void Build_RefusesPrePostCommandsForElevatedOperations() { var options = new InstallOptions { RunAsAdministrator = true, PreInstallCommand = "echo before" }; - Assert.Throws(() => BrokerRequestBuilder.Build( - BuildWinGetPackage(), options, OperationType.Install)); + WithElevationAllowed(() => Assert.Throws(() => BrokerRequestBuilder.Build( + BuildWinGetPackage(), options, OperationType.Install))); } [Fact] From e8fc724addf4236edb77f41277d893b9788af599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 03:55:21 +0900 Subject: [PATCH 16/17] Mirror per-manager broker option rules Report options the broker's command builder refuses for the manager (machine or user scope, architecture, pre-release, interactive, hash-check skip, install location, uninstall-previous, Cargo actions), stop sending pre-release, hash-check skip and architecture where the role ignores them, and check source names as sent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/Languages/lang_en.json | 10 +++ .../BrokerRequestBuilder.cs | 34 ++++++-- .../BrokerRequestValidator.cs | 78 ++++++++++++++++++- .../BrokerRequestBuilderTests.cs | 73 +++++++++++++++++ 4 files changed, 185 insertions(+), 10 deletions(-) diff --git a/src/Languages/lang_en.json b/src/Languages/lang_en.json index 7a2830c6e8..3af0e8526d 100644 --- a/src/Languages/lang_en.json +++ b/src/Languages/lang_en.json @@ -1263,6 +1263,16 @@ "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.": "{0} operations cannot run with administrator rights through the Devolutions Agent. Turn off \"Run as admin\" for this package.", "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.": "Pre-operation and post-operation commands cannot be used through the Devolutions Agent when the operation runs with administrator rights or for all users. Remove these commands, or turn off \"Run as admin\" and the machine-wide scope.", "The module conflicts with installed commands": "The module conflicts with installed commands", + "The Devolutions Agent does not support these options for {0} packages: {1}.": "The Devolutions Agent does not support these options for {0} packages: {1}.", + "installing for all users": "installing for all users", + "installing for the current user only": "installing for the current user only", + "choosing the architecture": "choosing the architecture", + "pre-release versions": "pre-release versions", + "skipping the hash check": "skipping the hash check", + "a custom install location": "a custom install location", + "uninstalling previous versions": "uninstalling previous versions", + "closing apps before the operation": "closing apps before the operation", + "pre-operation and post-operation commands": "pre-operation and post-operation commands", "{0} provides commands that another installed module already provides. Installing it anyway requires the -AllowClobber option, which cannot be used through the Devolutions Agent.": "{0} provides commands that another installed module already provides. Installing it anyway requires the -AllowClobber option, which cannot be used through the Devolutions Agent.", "Loading policy management state": "Loading policy management state", "Your organization": "Your organization", diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs index 3b8acdef7d..2b9700ae3c 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs @@ -27,10 +27,6 @@ public static PackageOperationRequest Build( OperationType role, string? effectiveInstallLocation = null) { - // WinGet_DropArchAndScope is set after an "update not applicable" result to retry - // without the scope/architecture constraints; mirror the local WinGet behavior so - // the AutoRetry does not rebuild the same constrained request indefinitely. - bool dropArchAndScope = package.OverridenOptions.WinGet_DropArchAndScope; ManagerName manager = MapManagerName(package.Manager.Name); @@ -66,7 +62,7 @@ public static PackageOperationRequest Build( { Id = package.Id, Version = ResolveVersion(manager, package, options, role), - Architecture = dropArchAndScope ? null : MapArchitecture(options.Architecture), + Architecture = ResolveArchitecture(manager, package, options, role), }, Options = new RequestOptions { @@ -74,8 +70,9 @@ public static PackageOperationRequest Build( // matching the local WinGet execution path. Scope = ResolveScope(manager, package, options), Interactive = options.InteractiveInstallation, - SkipHashCheck = options.SkipHashCheck, - PreRelease = options.PreRelease, + // Neither flag means anything for an uninstall, matching the local path. + SkipHashCheck = role is not OperationType.Uninstall && options.SkipHashCheck, + PreRelease = role is not OperationType.Uninstall && options.PreRelease, CustomParameters = customParameters, CustomInstallLocation = NullIfEmpty(effectiveInstallLocation), // Kill/pre/post actions are owned by the broker for brokered operations: @@ -229,6 +226,29 @@ internal static bool TryMapManagerName(string managerName, out ManagerName mappe return result is not null; } + /// + /// The architecture sent to the broker, or null to let the manager decide. Uninstalls never + /// select one, and Scoop only selects one on install, like the local execution paths. + /// + /// + /// WinGet_DropArchAndScope is set after an "update not applicable" result to retry without + /// the scope/architecture constraints; mirror the local WinGet behavior so the AutoRetry + /// does not rebuild the same constrained request indefinitely. + /// + internal static BrokerArchitecture? ResolveArchitecture( + ManagerName manager, + IPackage package, + InstallOptions options, + OperationType role) + { + return ArchitectureApplies(manager, package, role) ? MapArchitecture(options.Architecture) : null; + } + + internal static bool ArchitectureApplies(ManagerName manager, IPackage package, OperationType role) => + !package.OverridenOptions.WinGet_DropArchAndScope + && role is not OperationType.Uninstall + && (manager is not ManagerName.Scoop || role is OperationType.Install); + /// The scope sent to the broker for these options, or null to let it decide. internal static Scope? ResolveScope(ManagerName manager, IPackage package, InstallOptions options) => package.OverridenOptions.WinGet_DropArchAndScope diff --git a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs index c9744c03f8..d44c2c80b3 100644 --- a/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs +++ b/src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestValidator.cs @@ -116,8 +116,7 @@ public static IReadOnlyList Validate( AddIssue(issues, CheckVersion(manager, managerName, requestedVersion)); } - if (role is not OperationType.Uninstall - && !package.OverridenOptions.WinGet_DropArchAndScope + if (BrokerRequestBuilder.ArchitectureApplies(manager, package, role) && string.Equals(options.Architecture, UniGetUIArchitecture.arm32, StringComparison.OrdinalIgnoreCase)) { issues.Add(CoreTools.Translate( @@ -125,6 +124,15 @@ public static IReadOnlyList Validate( UniGetUIArchitecture.arm32)); } + string[] unsupportedOptions = [.. FindUnsupportedOptions(manager, package, options, role, installLocation)]; + if (unsupportedOptions.Length > 0) + { + issues.Add(CoreTools.Translate( + "The Devolutions Agent does not support these options for {0} packages: {1}.", + managerName, + string.Join(", ", unsupportedOptions))); + } + if (!string.IsNullOrWhiteSpace(installLocation)) { AddIssue(issues, CheckInstallLocation(manager, installLocation)); @@ -177,6 +185,69 @@ public static bool RequestsElevation(IPackage package, InstallOptions options) = !Settings.Get(Settings.K.ProhibitElevation) && (package.OverridenOptions.RunAsAdministrator is true || options.RunAsAdministrator); + /// + /// The options the broker's command builder for this manager refuses, as localized names, + /// for the values that the request would actually carry for this role. + /// + private static IEnumerable FindUnsupportedOptions( + ManagerName manager, + IPackage package, + InstallOptions options, + OperationType role, + string? installLocation) + { + bool isUninstall = role is OperationType.Uninstall; + Scope? scope = BrokerRequestBuilder.ResolveScope(manager, package, options); + bool architecture = BrokerRequestBuilder.ResolveArchitecture(manager, package, options, role) is not null; + bool perUserOnly = manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo or ManagerName.Scoop + or ManagerName.Vcpkg or ManagerName.Dotnet or ManagerName.Pip; + bool packageManagerPicksTheBuild = manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo + or ManagerName.Pip or ManagerName.Vcpkg; + + if (scope is Scope.Machine && perUserOnly) + yield return CoreTools.Translate("installing for all users"); + + if (scope is Scope.User && manager is ManagerName.Chocolatey) + yield return CoreTools.Translate("installing for the current user only"); + + if (architecture + && (packageManagerPicksTheBuild + || (manager is ManagerName.Chocolatey + && string.Equals(options.Architecture, UniGetUIArchitecture.arm64, StringComparison.OrdinalIgnoreCase)))) + yield return CoreTools.Translate("choosing the architecture"); + + if (!isUninstall && options.PreRelease + && manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo or ManagerName.Scoop or ManagerName.Vcpkg) + yield return CoreTools.Translate("pre-release versions"); + + if (options.InteractiveInstallation + && manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo or ManagerName.Scoop + or ManagerName.Vcpkg or ManagerName.Dotnet or ManagerName.Pip) + yield return CoreTools.Translate("interactive installation"); + + if (!isUninstall && options.SkipHashCheck + && manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Cargo or ManagerName.Vcpkg + or ManagerName.Dotnet or ManagerName.Pip) + yield return CoreTools.Translate("skipping the hash check"); + + if (!string.IsNullOrWhiteSpace(installLocation) + && manager is ManagerName.Npm or ManagerName.Bun or ManagerName.Scoop or ManagerName.Chocolatey + or ManagerName.Vcpkg or ManagerName.Pip) + yield return CoreTools.Translate("a custom install location"); + + if (role is OperationType.Update && options.UninstallPreviousVersionsOnUpdate + && manager is not (ManagerName.Winget or ManagerName.PowerShell or ManagerName.PowerShell7)) + yield return CoreTools.Translate("uninstalling previous versions"); + + if (manager is ManagerName.Cargo) + { + if ((options.KillBeforeOperation ?? []).Count > 0) + yield return CoreTools.Translate("closing apps before the operation"); + if (HasPrePostCommands(options, role)) + yield return CoreTools.Translate("pre-operation and post-operation commands"); + } + } + /// Managers whose broker command builder refuses elevated operations. private static bool ManagerRejectsElevation(ManagerName manager) => manager @@ -397,7 +468,8 @@ ManagerName.PowerShell or ManagerName.PowerShell7 when id.IndexOfAny(['*', '?', return null; } - string name = manager is ManagerName.PowerShell or ManagerName.PowerShell7 ? sourceName.Trim() : sourceName; + // The broker refuses surrounding whitespace, so the name is checked as it is sent. + string name = sourceName; bool valid = name.Length is > 0 and <= MaxSourceNameLength && char.IsAsciiLetterOrDigit(name[0]) && !name.EndsWith(' ') diff --git a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs index 3e148987ab..a431e6bbc5 100644 --- a/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs +++ b/src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs @@ -1,6 +1,7 @@ using Devolutions.Now.Policy.Api; using Devolutions.Now.Policy.Client; using UniGetUI.Core.SettingsEngine; +using UniGetUI.Core.Tools; using UniGetUI.PackageEngine.AgentBroker; using UniGetUI.PackageEngine.Serializable; using UniGetUI.PackageEngine.Tests.Infrastructure.Builders; @@ -849,6 +850,78 @@ public void Build_KeepsPrePostCommandsForStandardUserOperations() Assert.Equal("echo before", request.Options.PreOperationCommand); } + [Theory] + [InlineData("Npm")] + [InlineData("Scoop")] + [InlineData("Cargo")] + public void Build_RefusesMachineScopeForManagersThatInstallPerUser(string managerName) + { + var options = new InstallOptions { InstallationScope = PackageScope.Machine }; + + var exception = Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage(managerName, managerName == "Scoop" ? "7zip" : "contoso-tool"), options, OperationType.Install)); + + Assert.Contains(exception.Issues, issue => issue.Contains(CoreTools.Translate("installing for all users"))); + } + + [Fact] + public void Build_RefusesPreReleaseInstallsForNpm() + { + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Npm"), new InstallOptions { PreRelease = true }, OperationType.Install)); + } + + [Theory] + [InlineData("Npm")] + [InlineData("Chocolatey")] + [InlineData("Winget")] + public void Build_IgnoresPreReleaseSkipHashAndArchitectureForUninstalls(string managerName) + { + var options = new InstallOptions + { + PreRelease = true, + SkipHashCheck = true, + Architecture = UniGetUIArchitecture.arm64, + }; + + var request = BrokerRequestBuilder.Build( + BuildPackage(managerName, managerName == "Winget" ? "Contoso.Test" : "contoso-tool"), options, OperationType.Uninstall); + + Assert.False(request.Options.PreRelease); + Assert.False(request.Options.SkipHashCheck); + Assert.Null(request.Package.Architecture); + } + + [Fact] + public void Build_SendsAScoopArchitectureOnlyForInstalls() + { + var options = new InstallOptions { Architecture = UniGetUIArchitecture.x64 }; + var package = BuildPackage("Scoop", "7zip"); + + Assert.Equal(Architecture.X64, BrokerRequestBuilder.Build(package, options, OperationType.Install).Package.Architecture); + Assert.Null(BrokerRequestBuilder.Build(package, options, OperationType.Update).Package.Architecture); + } + + [Fact] + public void Build_RefusesArm64ForChocolatey() + { + Assert.Throws(() => BrokerRequestBuilder.Build( + BuildPackage("Chocolatey"), new InstallOptions { Architecture = UniGetUIArchitecture.arm64 }, OperationType.Install)); + } + + [Fact] + public void Build_RefusesPowerShellSourceNamesWithSurroundingWhitespace() + { + var package = new PackageBuilder() + .WithManager(new PackageManagerBuilder().WithName("PowerShell").Build()) + .WithSource(new SourceBuilder().WithName(" PSGallery").Build()) + .WithId("Pester") + .Build(); + + Assert.Throws(() => BrokerRequestBuilder.Build( + package, new InstallOptions(), OperationType.Install)); + } + [Fact] public void Build_RefusesTheArm32Architecture() { From 2cd4ff17333bc7f09310c1b59495dcec3228af6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 3 Oct 2026 04:07:29 +0900 Subject: [PATCH 17/17] Check close-app entries in the live broker preview Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ViewModels/DialogPages/InstallOptionsViewModel.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs index 7c24799952..e2dd9b2104 100644 --- a/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs +++ b/src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs @@ -379,6 +379,8 @@ public InstallOptionsViewModel(IPackage package, OperationType operation, Instal // Close apps foreach (var proc in options.KillBeforeOperation) KillProcessEntries.Add(new KillProcessEntry(proc, e => KillProcessEntries.Remove(e))); + // Close-app entries are part of the brokered request, so changes re-check its rules. + KillProcessEntries.CollectionChanged += (_, _) => Refresh(); ForceKillChecked = Settings.Get(Settings.K.KillProcessesThatRefuseToDie); // Show fallback immediately, then replace with real icon if available @@ -735,6 +737,7 @@ private InstallOptions SnapshotOptions() o.PreUninstallCommand = PreUninstallText; o.PostUninstallCommand = PostUninstallText; o.AbortOnPreUninstallFail = AbortUninstall; + o.KillBeforeOperation = KillProcessEntries.Select(e => e.Name).ToList(); return o; }