Skip to content

Reject shell metacharacters and use -File for PowerShell operations - #5348

Merged
Gabriel Dufresne (GabrielDuf) merged 12 commits into
mainfrom
fix/sec-16522-command-injection
Sep 4, 2026
Merged

Reject shell metacharacters and use -File for PowerShell operations#5348
Gabriel Dufresne (GabrielDuf) merged 12 commits into
mainfrom
fix/sec-16522-command-injection

Conversation

@GabrielDuf

Copy link
Copy Markdown
Contributor

This pull request introduces several important security and robustness improvements to UniGetUI, focusing on safer PowerShell command execution, stricter validation of package fields, and improved handling of bundle imports and script exports. The changes include the addition of a controlled PowerShell launcher script, enhanced validation and sanitization of package identifiers and versions, and refactoring of bundle import filtering to centralize security logic.

Security and Validation Enhancements:

  • Added a new PowerShell launcher script (unigetui_ps_operation.ps1) to securely handle package operations by binding arguments as data and minimizing the risk of command injection. The script is invoked with -File and includes explicit TLS 1.2 selection for compatibility. ([[1]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-f5a2a4e0c90ac87d07c0907a1636d72d9afb1f9e23fd1b892f91deaf2f1d9e0dR1-R83), [[2]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-df264a55a2539d30dd3b1a3b2ba468333e613d2a4a04bf0887a591035f3d0e1eR579-R592))
  • Introduced comprehensive tests for package version and identifier validation, ensuring that only safe, inert values are accepted and potential injection payloads are rejected. ([src/UniGetUI.Core.Tools.Tests/PackageFieldValidationTests.csR1-R87](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-bb3c4e1ec43f9c6ca239a7c1e9aa7531a9c554a6504bbd515ef0fae41bcaed0eR1-R87))
  • Strengthened the CoreTools.EnsureSafeQueryString method to remove a broader set of potentially dangerous characters, including , (), {}, [], #, !, and ^, making query strings safer for use in command lines. Additional test cases were added to verify this behavior. ([1], [2]`)

Bundle Import and Command Construction Improvements:

  • Refactored bundle import filtering logic into the new BundleImportFilter class, centralizing checks for whether CLI arguments and pre/post-operation commands are allowed, and applying these checks consistently during import. ([[1]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cL436-R444), [[2]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cL495-L513))
  • Updated batch script export and manual install command generation to use the safer GetStandaloneParameters method, with error handling to skip packages that cannot produce a safe command line. ([[1]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cL545-R538), [[2]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-9f11ef61491cc877ccb03190fb6d7eb110d0eed8040f2aa8434c95dff4c43998L29-R40), [[3]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-8b4094a887795ba46b76f7e3762a316ab46ee8a463ec8bc26239bd502944fbabL510-R521))

Codebase Maintenance:

  • Cleaned up unused helper methods in PackageBundlesPage.cs and improved PowerShell string escaping in generated scripts for better reliability. ([[1]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cL495-L513), [[2]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cL607-R594))
  • Added missing using directives and minor refactoring for clarity and maintainability. ([[1]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-2559d980295a60506567dcad14f6e7f3ecded5465e2046fa0ffacb26de309e2cR16), [[2]](https://github.com/Devolutions/UniGetUI/pull/5348/files#diff-619abd46a81953ef9167a62636f794b01cdbe6a3373adfc7931ff476eeee3268R1-R3))

These changes collectively make UniGetUI more secure against command injection and improve the reliability of package operations and bundle processing.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical injection, script-export, and process-hang risks block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Hardens package-manager execution through argument-vector PowerShell launching, stricter field validation, and centralized bundle-import filtering.

Changes:

  • Adds a controlled PowerShell -File launcher and vectorized execution.
  • Strengthens package-field validation and command escaping.
  • Expands security, launcher, and command-injection tests.
File summaries
File Description
src/UniGetUI.PackageEngine.Tests/UniGetUI.PackageEngine.Tests.csproj Deploys launcher scripts in tests.
src/UniGetUI.PackageEngine.Tests/ShellManagerLaunchModeTests.cs Tests vector and fallback launch modes.
src/UniGetUI.PackageEngine.Tests/PowerShellOperationLauncherTests.cs Tests the launcher end to end.
src/UniGetUI.PackageEngine.Tests/PipManagerTests.cs Updates Pip escaping expectations.
src/UniGetUI.PackageEngine.Tests/PackageOperationCommandInjectionTests.cs Tests command-injection rejection.
src/UniGetUI.PackageEngine.Tests/OperationCallArgsWiringTests.cs Verifies manager launcher wiring.
src/UniGetUI.PackageEngine.Tests/NpmManagerTests.cs Updates npm specification expectations.
src/UniGetUI.PackageEngine.Tests/Infrastructure/Fakes/TestPackageManager.cs Supports vectorized test managers.
src/UniGetUI.PackageEngine.Tests/CommandLineReachabilityTests.cs Tests final process command lines.
src/UniGetUI.PackageEngine.Tests/CommandInjectionSurfaceTests.cs Covers validation attack surfaces.
src/UniGetUI.PackageEngine.Tests/ChocolateyManagerTests.cs Updates Chocolatey escaping expectations.
src/UniGetUI.PackageEngine.Tests/BundleImportFilterTests.cs Tests centralized import filtering.
src/UniGetUI.PackageEngine.Tests/BrokerRequestBuilderTests.cs Tests broker-side validation.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/PackageManager.cs Adds launch-vector initialization.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs Adds operation-field validation.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgDetailsHelper.cs Validates shell-bound detail lookups.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Classes/NullPackageManager.cs Implements new interface members.
src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Classes/BundleImportFilter.cs Centralizes bundle security filtering.
src/UniGetUI.PackageEngine.Operations/SourceOperations.cs Adds source validation and vectors.
src/UniGetUI.PackageEngine.Operations/PackageOperations.cs Launches operations using vectors.
src/UniGetUI.PackageEngine.Operations/AbstractProcessOperation.cs Adds argument-vector helpers.
src/UniGetUI.PackageEngine.Managers.WinGet/Helpers/WinGetPkgOperationHelper.cs Escapes WinGet versions.
src/UniGetUI.PackageEngine.Managers.Scoop/Scoop.cs Launches Scoop through -File.
src/UniGetUI.PackageEngine.Managers.Scoop/Helpers/ScoopPkgDetailsHelper.cs Applies vectorized Scoop arguments.
src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs Configures the PowerShell 7 launcher.
src/UniGetUI.PackageEngine.Managers.PowerShell7/Helpers/PowerShell7PkgOperationHelper.cs Validates uninstall versions.
src/UniGetUI.PackageEngine.Managers.PowerShell/PowerShell.cs Configures the Windows PowerShell launcher.
src/UniGetUI.PackageEngine.Managers.PowerShell/Helpers/PowerShellPkgOperationHelper.cs Separates launched and standalone parameters.
src/UniGetUI.PackageEngine.Managers.Pip/Helpers/PipPkgOperationHelper.cs Escapes pinned Pip specifications.
src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs Uses npm’s PowerShell entry point.
src/UniGetUI.PackageEngine.Managers.Npm/Helpers/NpmPkgOperationHelper.cs Validates npm specifications.
src/UniGetUI.PackageEngine.Managers.Npm/Helpers/NpmPkgDetailsHelper.cs Applies vectorized npm arguments.
src/UniGetUI.PackageEngine.Managers.Dotnet/Helpers/DotNetPkgOperationHelper.cs Escapes .NET Tool versions.
src/UniGetUI.PackageEngine.Managers.Chocolatey/Helpers/ChocolateyPkgOperationHelper.cs Escapes Chocolatey versions.
src/UniGetUI.PackageEngine.Managers.Cargo/Helpers/CargoPkgOperationHelper.cs Escapes Cargo versions.
src/UniGetUI.PackageEngine.Managers.Bun/Helpers/BunPkgOperationHelper.cs Validates Bun package specifications.
src/UniGetUI.PackageEngine.Interfaces/ManagerHelpers/IPackageOperationHelper.cs Adds standalone parameter generation.
src/UniGetUI.PackageEngine.Interfaces/IPackageManager.cs Exposes shell-interpretation metadata.
src/UniGetUI.PackageEngine.Enums/ManagerStatus.cs Stores and applies argument vectors.
src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs Validates broker package fields.
src/UniGetUI.Interface.IpcApi/IpcPackageApi.cs Sanitizes IPC search queries.
src/UniGetUI.Interface.IpcApi/IpcBundleApi.cs Uses centralized bundle filtering.
src/UniGetUI.Core.Tools/Tools.cs Adds validation, escaping, and launcher probing.
src/UniGetUI.Core.Tools.Tests/ToolsTests.cs Expands query-sanitization tests.
src/UniGetUI.Core.Tools.Tests/PackageFieldValidationTests.cs Tests package-field validators.
src/UniGetUI.Core.Data/CoreData.cs Defines the launcher path.
src/UniGetUI.Avalonia/Views/SoftwarePages/PackageBundlesPage.cs Secures imports and script exports.
src/UniGetUI.Avalonia/ViewModels/DialogPages/InstallOptionsViewModel.cs Uses standalone preview parameters.
src/UniGetUI.Avalonia/Infrastructure/ManualInstallHelper.cs Uses standalone manual commands.
src/SharedAssets/Assets/Utilities/unigetui_ps_operation.ps1 Implements the controlled PowerShell launcher.
Review details
  • Files reviewed: 50/50 changed files
  • Comments generated: 11
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/UniGetUI.Avalonia/Views/SoftwarePages/PackageBundlesPage.cs
Comment thread src/UniGetUI.Core.Tools/Tools.cs Outdated
Comment thread src/UniGetUI.PackageEngine.Operations/SourceOperations.cs Outdated
Comment thread src/UniGetUI.Interface.IpcApi/IpcPackageApi.cs
Comment thread src/UniGetUI.PackageEngine.Managers.Npm/Npm.cs
Comment thread src/UniGetUI.PackageEngine.Operations/SourceOperations.cs Outdated
Comment thread src/UniGetUI.PackageEngine.Operations/SourceOperations.cs Outdated
Comment thread src/UniGetUI.PackageEngine.Tests/PowerShellOperationLauncherTests.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical broker validation gap and multiple moderate correctness and security issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/UniGetUI.PackageEngine.Managers.Cargo/Helpers/CargoPkgOperationHelper.cs:19

  • This still feeds package.VersionString into --version when no version was requested. For an unpinned ImportedPackage, that property is localized “Latest”, so Cargo receives a translated non-version (or its quoted multi-word translation) and the bundle install fails. Omit --version for an unpinned imported package instead of escaping the display placeholder.

src/UniGetUI.Interface.IpcApi/IpcPackageApi.cs:132

  • This sanitizer still permits option-shaped queries. For example, --registry=https://attacker.example survives unchanged and the updated npm path passes it as an argv token, so npm interprets it as a configuration flag rather than search text and sends the request to the supplied registry. Reject option tokens or add a manager-specific option boundary before dispatch instead of relying only on shell-metacharacter removal.
        string safeQuery = CoreTools.EnsureSafeQueryString(query);
        if (safeQuery.Length is 0)
            return [];

        return GetManagers(managerName)
            .SelectMany(manager => manager.FindPackages(safeQuery))

src/UniGetUI.PackageEngine.Operations/SourceOperations.cs:94

  • callVector.Count == 0 is not equivalent to “a shell will parse this command.” Direct managers such as WinGet, Chocolatey, Flatpak, and Homebrew also have no vector, so a valid feed URL containing &, %, or # is rejected here even though those characters are ordinary data when the executable is started directly. Restrict metacharacter rejection to the legacy shell-interpreted fallback (while still handling whitespace safely), or move direct source operations to ArgumentList.
  • Files reviewed: 52/52 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread src/UniGetUI.PackageEngine.AgentBroker/BrokerRequestBuilder.cs
Comment thread src/SharedAssets/Assets/Utilities/unigetui_ps_operation.ps1
Comment thread src/SharedAssets/Assets/Utilities/unigetui_ps_operation.ps1
Comment thread src/UniGetUI.PackageEngine.Managers.Bun/Helpers/BunPkgOperationHelper.cs Outdated
Comment thread src/UniGetUI.PackageEngine.Managers.Npm/Helpers/NpmPkgOperationHelper.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical launcher and Scoop validation defects block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/UniGetUI.PackageEngine.Operations/SourceOperations.cs:86

  • This still rejects valid direct-executable source URLs containing & or % (for example, signed/private feed URLs). Those characters are only shell metacharacters; WinGet, Chocolatey, Flatpak, and Homebrew run with UseShellExecute = false, so they do not alter execution. Restrict the inert-value check to shell-interpreted fallbacks while continuing to reject quote/whitespace splitting on direct concatenated paths.
    src/UniGetUI.PackageEngine.Tests/OperationCallArgsWiringTests.cs:174
  • The timeout is ineffective because ReadToEnd() blocks until PowerShell exits before WaitForExit(30_000) is reached. If process startup hangs, this test can hang the runner indefinitely; begin the asynchronous read first and kill the process when the timed wait fails.

src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs:84

  • The new central validation only checks package.Id, but Scoop’s operation helper later emits ${package.Source.Name}/${package.Id}. When the launcher probe selects the supported -Command fallback, an existing bucket name containing a shell separator is therefore concatenated after these checks and can still alter the script. Validate Scoop’s source-derived package spec before returning the operation parameters; the same source component is also used by its details path.
  • Files reviewed: 56/56 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread src/SharedAssets/Assets/Utilities/unigetui_ps_operation.ps1

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical process hang and unresolved option-injection and source-handling issues block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

src/UniGetUI.Interface.IpcApi/IpcPackageApi.cs:129

  • Sanitization does not prevent option injection: --registry=https://evil.example survives EnsureSafeQueryString unchanged and is then passed by npm as the argument after search, where npm interprets it as a registry option rather than a query. Reject option-shaped sanitized queries (including after leading whitespace), or add a manager-specific end-of-options boundary before dispatch.
        string safeQuery = CoreTools.EnsureSafeQueryString(query);
        if (safeQuery.Length is 0)
            return [];

src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs:61

  • The central validation still omits another imported value that reaches a package command: package.Source.Name. SourceFactory.GetSourceOrDefault creates unknown bundle source names verbatim, and WinGet appends that name raw after --source; a value such as winget --scope machine is therefore split into real options and bypasses the bundle setting that disables imported CLI arguments. Validate/quote source identity at the operation sink, or refuse unresolved imported sources before building parameters.
    src/UniGetUI.PackageEngine.Tests/PowerShellOperationLauncherTests.cs:369
  • This test is not self-contained: Install-Module -WhatIf still resolves the requested package through PSGallery, so an offline runner, gallery outage, or repository configuration change can make the suite fail or consume the full launcher timeout. Exercise named binding with a local temporary command/module (or a built-in cmdlet) instead of contacting an external repository.
  • Files reviewed: 58/58 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/UniGetUI.PackageEngine.Operations/PackageOperations.cs
Comment thread src/UniGetUI.PackageEngine.Operations/SourceOperations.cs
The theory added with the standard input fix flipped
DisableNewProcessLineHandler through the settings engine without redirecting
the data directory first, so it read and wrote the real configuration of
whoever ran the tests. A run interrupted between the write and the restore
would have left that setting turned on, which is exactly the state that makes
npm and Scoop operations hang.

The class now redirects the data directory and the secure settings root in its
constructor and resets both afterwards, as every other settings-touching test
class here does, which also removes the need to save and restore the value.

Re-checked that the theory still catches the fault it was written for: with the
close moved back inside the conditional the disabled case still hangs until the
timeout, so isolating the setting did not make it pass vacuously.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical WinGet validation gap and two moderate command-handling issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/UniGetUI.PackageEngine.Operations/SourceOperations.cs:85

  • This still treats every non-vector launch as shell-interpreted. Direct managers also have an empty OperationCallArgs, so legitimate values parsed by those managers are rejected—for example, Chocolatey preserves the source name internal repo (ChocolateyManagerTests.cs:115-138), but removing that source now throws here; direct feed URLs containing & or % are likewise blocked even though those characters are inert to the executable. Distinguish the legacy shell fallback from direct execution, and pass direct source arguments through an argument vector (or quote each value) so spaces remain one argument without applying shell-metacharacter rejection.
    src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs:49
  • standalone output is later rejoined into shell text (the bundle exporter executes it via cmd.exe /C, and manual install places it in a PowerShell/bash prompt), but this path still validates direct-exec managers using direct-process rules. For example, a Cargo package ID pkg&calc passes IsOptionSafeIdentifier and is emitted raw, so the exported script executes calc; a Pip version containing $(...) is wrapped in double quotes by EscapeCommandLineArgument, which still expands in PowerShell/bash. Standalone generation needs shell-specific escaping or stricter rejection for every package-derived field instead of reusing the live-process argv representation.
  • Files reviewed: 58/58 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/UniGetUI.PackageEngine.Managers.WinGet/Helpers/WinGetPkgOperationHelper.cs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

One critical command-injection flaw and three moderate validation or escaping issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/UniGetUI.PackageEngine.Managers.Npm/Helpers/NpmPkgOperationHelper.cs:25

  • This raw spec is reused for both the -File vector and the legacy Windows -Command fallback. In the fallback, a scoped package such as @babel/core@7.24.0 starts with PowerShell's @ splatting token, so removing the previous PowerShell quoting can make valid scoped installs fail. The standalone/export path has a different requirement again (cmd.exe consumes caret characters in alias/range specs). Format the spec per sink: raw for ArgumentList, PowerShell-single-quoted for -Command, and command-line-escaped for standalone commands.

src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Classes/BundleImportFilter.cs:145

  • This predicate is weaker than the validation applied later by BasePkgOperationHelper. For example, 1/2 and 1,2 are command-line-inert here, so the import report stays empty and preserves the pin, but IsValidPackageVersion rejects it when installation or export is attempted. Use the same package-version predicate here so rejected shell-manager versions are stripped and reported during import as intended.
    src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs:49
  • standalone: true still applies only the direct-execution checks, even though its callers reassemble these values into shell text. For example, an imported Pip, Cargo, .NET Tool, or Chocolatey ID such as safe&calc passes IsOptionSafeIdentifier, is emitted unquoted by its helper, and the exported script runs the resulting string through cmd.exe /C (PackageBundlesPage.cs:554,626-628), so the appended command executes. The manual-install path can likewise expose shell metacharacters to PowerShell. Standalone commands need target-shell-aware escaping/validation (including quoted WinGet values), rather than reusing the ProcessStartInfo argument checks.
  • Files reviewed: 58/58 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/UniGetUI.Avalonia/Views/SoftwarePages/PackageBundlesPage.cs Outdated
The exported installation script collects each command as a PowerShell
single-quoted literal and then runs it with "cmd.exe /C $command". The
single-quoting stops PowerShell from expanding anything, but cmd expands %NAME%
before it parses the command, inside double quotes included, so stripping
quotes and control characters from an imported process name was not enough.
Confirmed against the real thing: with FOO set to a value carrying a quote and
a separator, the emitted taskkill /im "%FOO%" ran an injected command.

Reaching arbitrary code this way needs an environment variable whose value
already carries the payload, since a bundle can only supply the %NAME%
reference, so this is narrower than a bundle-only injection. It is still
imported data reaching a command line unchecked, which is what this branch
exists to stop.

The name is now refused rather than stripped. Encoding is not an option: a
percent sign cannot be escaped in a cmd /C command line the way it can inside a
batch file. Stripping was also wrong on its own terms, because removing a
character silently retargets the kill at whatever process the shortened name
happens to match. Whitespace stays allowed, as image names such as
"My App.exe" are ordinary and the quoting protects them.

The in-app kill path needs no change: KillProcessOperation goes through
Process.GetProcessesByName and Process.Kill and never builds a command line.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two critical and three moderate command-safety and compatibility issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/UniGetUI.PackageEngine.Managers.Npm/Helpers/NpmPkgOperationHelper.cs:25

  • This unquoted spec breaks the legacy Windows fallback. When OperationCallArgs is empty, npm is still invoked through powershell.exe -Command; an npm scope such as @babel/core@7.24.0 is then parsed as PowerShell splatting instead of as a literal npm argument. Keep the raw spec for -File and cmd-export paths, but quote it for the -Command runtime fallback.

src/UniGetUI.PackageEngine.Operations/SourceOperations.cs:85

  • usingArgumentVector == false also describes direct executables, not only a shell fallback. Consequently this now rejects legitimate direct-manager source names containing spaces; Chocolatey already parses and preserves internal repo (ChocolateyManagerTests.cs:115-138), but add/remove reaches this check and throws. Direct executables should receive source values via ArgumentList (including through the elevator), while metacharacter rejection should be reserved for the actual shell-concatenated fallback.
    src/UniGetUI.PackageEngine.PackageManagerClasses/Manager/Helpers/BasePkgOperationHelper.cs:49
  • GetStandaloneParameters reuses validation for the manager's normal process sink, but every standalone consumer turns the result back into a shell command (the exported script runs it through cmd.exe /C, and manual install places it in a terminal). A direct-manager identifier such as pkg&calc passes IsOptionSafeIdentifier and skips the shell validation, then the exported command executes calc as a second command. Standalone command generation must escape/validate package-derived values for the target shell (or avoid producing a command string) rather than relying on the manager's direct-exec safety classification.
  • Files reviewed: 58/58 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/UniGetUI.PackageEngine.Managers.PowerShell7/PowerShell7.cs
…cher everywhere

Two gaps in the shell fallback.

Custom CLI arguments were never validated. InstallOptionsFactory strips
& | ; < > and newlines but leaves $, backticks and parentheses, and PowerShell
evaluates both $(...) and a bare (...) in argument position, so an imported
custom argument became code wherever the parameters are concatenated. They are
now required to be inert on exactly those paths: an operation whose manager has
no argument vector, and the exported script, which joins them whatever the
manager does. With a vector they stay separate arguments and are left alone, so
the power-user feature keeps its freedom where it is safe. Both need the
AllowCLIArguments secure setting to survive import at all, which is why this is
narrower than the version field of the original report.

The launcher was also copied only by the Windows-conditioned item group, while
PowerShell 7 is a manager on every supported platform and its call args are not
platform-gated. Every non-Windows PowerShell 7 operation therefore failed the
probe and stayed on -Command. The copy is now unconditional. The primary defence
was never affected: CommandLineIsShellInterpreted is platform-independent, so
the identifier and version validation applied on Linux regardless.

The new deployment test covers the test host on both frameworks, not the app's
publish layout, which cannot be checked from a unit test on Windows because the
target framework and that item group both key off the build OS. The app side is
inspection plus the Linux CI build.

Npm keeps declaring itself shell-interpreted on Linux, where it is executed
directly. That is conservative rather than wrong, and matches how the identifier
and version checks already behave there; a space or a subexpression in a single
argument is broken by the space join on that path anyway.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The npm fallback has a critical quoting regression, and direct source operations reject valid spaced names.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/UniGetUI.PackageEngine.Operations/SourceOperations.cs:85

  • This condition treats every manager without an operation call vector as shell-interpreted. Direct managers also have an empty vector, so valid existing source names containing spaces are rejected before launch—for example, Chocolatey parses internal repo as a source in ChocolateyManagerTests.cs:115-138, but removing it now always throws here. Direct source operations need an argument-vector/quoting path that preserves one source-name argument; metacharacter rejection should apply only to the actual shell-concatenated fallback.
  • Files reviewed: 60/60 changed files
  • Comments generated: 1
  • Review effort level: Balanced

@randy-but-a-ro randy-but-a-ro Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉

Integration Details
{
	"deliveryId": "218d1290-a88b-11f1-83b1-63d083cf8af0",
	"headSha": "5c39b2bfec6534a6fe478cec64c8947ae8006a79",
	"reviewer": "copilot-pull-request-reviewer[bot]"
}

@GabrielDuf
Gabriel Dufresne (GabrielDuf) merged commit 4f8a820 into main Sep 4, 2026
6 checks passed
@GabrielDuf
Gabriel Dufresne (GabrielDuf) deleted the fix/sec-16522-command-injection branch September 4, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants