From bc71ed4a123b2254939e4d17eef157836d0c8ac3 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Fri, 22 May 2026 14:32:04 -0500 Subject: [PATCH] fix: deliver Invoke-PveVmGuestExec -Args to the guest as argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExecuteGuestCommand JSON-serialized the args array into the agent/exec 'input-data' field — which is the process's STDIN, not its arguments. So guest commands ran with no argv: cmd.exe started interactively and the JSON blob ['/c','echo',...] arrived at its prompt. PVE's agent/exec 'command' parameter is itself an array (element 0 = the executable, the rest = argv) sent as repeated form keys. The low-level client couldn't express repeated keys (Dictionary only), so: - Add PostAsync(string, IEnumerable>) to IPveHttpClient/PveHttpClient; BuildFormContent now emits one key=value field per pair, so a key may repeat. - ExecuteGuestCommand builds command = [exe] + args as repeated 'command' fields and no longer touches input-data. Tests: form-encoder repeated-key + per-value encoding cases; VmService tests asserting the command array, order, and absence of input-data; an integration regression guard that echoes an arg and checks it round-trips as stdout. Tracked as F091. Closes #68. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/review/findings.json | 32 ++++++++ src/PSProxmoxVE.Core/Client/IPveHttpClient.cs | 9 ++- src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 19 ++++- src/PSProxmoxVE.Core/Services/VmService.cs | 16 ++-- .../Client/PveHttpClientFormEncodingTests.cs | 40 +++++++++ .../Services/VmServiceTests.cs | 81 +++++++++++++++++++ .../Integration/11_LinuxVM.Tests.ps1 | 12 +++ 7 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs diff --git a/docs/review/findings.json b/docs/review/findings.json index a93c804..d783826 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2971,6 +2971,38 @@ "evidence": "Added typed scsihw/efidisk0/tpmstate0 properties plus a private [JsonExtensionData] landing field exposed as AdditionalProperties (Dictionary via JsonHelper.ToNative, so values are native .NET types per D013 — no JToken leakage). Typed keys do not duplicate into the catch-all.", "verified_by": "dotnet build + dotnet test (586 passed, new VmModelTests for scsihw/efidisk/tpm, native-typed AdditionalProperties, and no typed-key leakage)" } + }, + { + "id": "F091", + "title": "Invoke-PveVmGuestExec -Args delivered as JSON STDIN instead of argv", + "category": "api_contract", + "severity": "high", + "status": "resolved", + "first_detected": "2026-05-22", + "github_issue": 68, + "files": [ + "src/PSProxmoxVE.Core/Services/VmService.cs", + "src/PSProxmoxVE.Core/Client/PveHttpClient.cs", + "src/PSProxmoxVE.Core/Client/IPveHttpClient.cs" + ], + "description": "VmService.ExecuteGuestCommand JSON-serialized the args array into the agent/exec 'input-data' field (the process's STDIN) instead of passing them as argv. PVE's agent/exec 'command' parameter is itself an array (element 0 = executable, rest = arguments) and must be sent as repeated form keys. The result: guest commands ran with no/garbage arguments (cmd.exe started interactively, the JSON blob appeared at the prompt). The low-level client also could not express repeated form keys (Dictionary only).", + "scan_history": [ + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "new" + }, + { + "scan_date": "2026-05-22", + "local_id": null, + "status": "fixed" + } + ], + "resolution": { + "scan_date": "2026-05-22", + "evidence": "Added PostAsync(string, IEnumerable>) to IPveHttpClient/PveHttpClient (BuildFormContent now emits repeated keys). ExecuteGuestCommand builds command = [exe] + args as repeated 'command' fields and no longer touches input-data.", + "verified_by": "dotnet build (0 warnings) + dotnet test (594 passed; new PveHttpClientFormEncodingTests for repeated keys + per-value encoding, and VmServiceTests asserting the command array and absence of input-data)" + } } ] } diff --git a/src/PSProxmoxVE.Core/Client/IPveHttpClient.cs b/src/PSProxmoxVE.Core/Client/IPveHttpClient.cs index 7c789f3..4783140 100644 --- a/src/PSProxmoxVE.Core/Client/IPveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/IPveHttpClient.cs @@ -16,6 +16,13 @@ namespace PSProxmoxVE.Core.Client /// Performs a POST request against the specified API resource path. Task PostAsync(string resource, Dictionary? data = null); + /// + /// Performs a POST request whose form body may contain repeated keys, used for + /// PVE array parameters (e.g. guest-exec "command"). Each pair becomes one + /// key=value field, so a key may appear multiple times. + /// + Task PostAsync(string resource, IEnumerable> data); + /// Performs a PUT request against the specified API resource path. Task PutAsync(string resource, Dictionary? data = null); @@ -25,7 +32,7 @@ namespace PSProxmoxVE.Core.Client /// Synchronous wrapper for . string Get(string resource); - /// Synchronous wrapper for . + /// Synchronous wrapper for . string Post(string resource, Dictionary? data = null); /// Synchronous wrapper for . diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 6c61425..4da3040 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -111,6 +111,21 @@ namespace PSProxmoxVE.Core.Client return await SendAsync(request, resource, "POST").ConfigureAwait(false); } + /// + /// POST whose form body may contain repeated keys, for PVE array parameters + /// (e.g. guest-exec "command"). Each pair becomes one key=value field. + /// + /// Relative resource path + /// Form fields; a key may appear more than once + /// Raw JSON response body + public async Task PostAsync(string resource, IEnumerable> data) + { + if (data == null) throw new ArgumentNullException(nameof(data)); + var request = BuildRequest(HttpMethod.Post, resource, mutating: true); + request.Content = BuildFormContent(data); + return await SendAsync(request, resource, "POST").ConfigureAwait(false); + } + /// Performs a PUT request against the specified API resource path. /// Relative resource path /// Form fields to send as application/x-www-form-urlencoded body @@ -142,7 +157,7 @@ namespace PSProxmoxVE.Core.Client /// : and ! which PVE's internal API consumers (e.g. cluster join) /// do not properly URL-decode. /// - private static StringContent BuildFormContent(Dictionary data) + private static StringContent BuildFormContent(IEnumerable> data) { var sb = new StringBuilder(); foreach (var kvp in data) @@ -191,7 +206,7 @@ namespace PSProxmoxVE.Core.Client public string Get(string resource) => GetAsync(resource).GetAwaiter().GetResult(); - /// Synchronous wrapper for . + /// Synchronous wrapper for . public string Post(string resource, Dictionary? data = null) => PostAsync(resource, data).GetAwaiter().GetResult(); diff --git a/src/PSProxmoxVE.Core/Services/VmService.cs b/src/PSProxmoxVE.Core/Services/VmService.cs index a615d03..8e1d5e6 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -599,16 +599,18 @@ namespace PSProxmoxVE.Core.Services IPveHttpClient client = _injectedClient ?? new PveHttpClient(session); try { - var data = new Dictionary + // PVE's agent/exec "command" is an array: element 0 is the executable and + // each subsequent element is one argv entry. It is sent as repeated form + // keys (command=&command=&...). Do NOT use "input-data" for + // arguments — that is the process's STDIN, not argv. + var data = new List> { - ["command"] = command + new KeyValuePair("command", command) }; - - if (args != null && args.Length > 0) + if (args != null) { - // PVE expects input-data for arguments passed as a JSON-encoded string array - var argsJson = Newtonsoft.Json.JsonConvert.SerializeObject(args); - data["input-data"] = argsJson; + foreach (var arg in args) + data.Add(new KeyValuePair("command", arg)); } var response = client.PostAsync($"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/agent/exec", data) diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs index 25be8dd..8264be0 100644 --- a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs @@ -79,6 +79,46 @@ namespace PSProxmoxVE.Core.Tests.Client Assert.Contains("ide2=nas:iso/win.iso,media%3Dcdrom", handler.LastBody); } + [Fact] + public async Task PostAsync_RepeatedKeys_EmitOneFieldPerValue() + { + var (client, handler) = NewCapturingClient(); + using (client) + { + // PVE array params (e.g. guest-exec command) are sent as repeated keys. + var data = new List> + { + new KeyValuePair("command", "cmd.exe"), + new KeyValuePair("command", "/c"), + new KeyValuePair("command", "echo"), + new KeyValuePair("command", "WLMARK42"), + }; + await client.PostAsync("nodes/pve/qemu/100/agent/exec", data); + } + + Assert.Equal("command=cmd.exe&command=/c&command=echo&command=WLMARK42", handler.LastBody); + } + + [Fact] + public async Task PostAsync_RepeatedKeys_EncodeEachValueIndependently() + { + var (client, handler) = NewCapturingClient(); + using (client) + { + var data = new List> + { + new KeyValuePair("command", "powershell.exe"), + new KeyValuePair("command", "-Command"), + new KeyValuePair("command", "echo a=b;c"), + }; + await client.PostAsync("nodes/pve/qemu/100/agent/exec", data); + } + + // The '=' and ';' inside an arg must be encoded so they don't split the body, + // while spaces follow the form convention ('+'). + Assert.Equal("command=powershell.exe&command=-Command&command=echo+a%3Db%3Bc", handler.LastBody); + } + private sealed class CapturingHandler : HttpMessageHandler { public string LastBody { get; private set; } = string.Empty; diff --git a/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs new file mode 100644 index 0000000..74826e5 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -0,0 +1,81 @@ +using System.Collections.Generic; +using System.Linq; +using Moq; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Client; +using PSProxmoxVE.Core.Services; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Services +{ + public class VmServiceTests + { + private const string TestNode = "pve1"; + private const int TestVmId = 100; + + private static PveSession CreateSession() => + new PveSession("pve1.example.com", 8006, true, "PVE:root@pam:TEST_TOKEN"); + + [Fact] + public void ExecuteGuestCommand_SendsCommandAndArgsAsRepeatedCommandArray() + { + List>? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>>())) + .Callback>>((_, data) => captured = data.ToList()) + .ReturnsAsync("{\"data\":{\"pid\":4242}}"); + + var service = new VmService(mockClient.Object); + var pid = service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId, + "cmd.exe", new[] { "/c", "echo", "WLMARK42" }); + + Assert.Equal(4242, pid); + Assert.NotNull(captured); + + // Every element (exe + each arg) is its own "command" entry, in order. + Assert.All(captured!, kvp => Assert.Equal("command", kvp.Key)); + Assert.Equal( + new[] { "cmd.exe", "/c", "echo", "WLMARK42" }, + captured!.Select(kvp => kvp.Value).ToArray()); + } + + [Fact] + public void ExecuteGuestCommand_DoesNotUseInputDataForArgs() + { + List>? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>>())) + .Callback>>((_, data) => captured = data.ToList()) + .ReturnsAsync("{\"data\":{\"pid\":1}}"); + + var service = new VmService(mockClient.Object); + service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId, + "powershell.exe", new[] { "-NoProfile", "-Command", "echo hi" }); + + Assert.NotNull(captured); + // Args are argv, not STDIN — "input-data" must never be emitted. + Assert.DoesNotContain(captured!, kvp => kvp.Key == "input-data"); + } + + [Fact] + public void ExecuteGuestCommand_NoArgs_SendsSingleCommandEntry() + { + List>? captured = null; + var mockClient = new Mock(); + mockClient + .Setup(c => c.PostAsync(It.IsAny(), It.IsAny>>())) + .Callback>>((_, data) => captured = data.ToList()) + .ReturnsAsync("{\"data\":{\"pid\":7}}"); + + var service = new VmService(mockClient.Object); + service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId, "whoami", null); + + Assert.NotNull(captured); + var only = Assert.Single(captured!); + Assert.Equal("command", only.Key); + Assert.Equal("whoami", only.Value); + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Integration/11_LinuxVM.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/11_LinuxVM.Tests.ps1 index e72d5b4..079a652 100644 --- a/tests/PSProxmoxVE.Tests/Integration/11_LinuxVM.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Integration/11_LinuxVM.Tests.ps1 @@ -157,6 +157,18 @@ Describe 'Linux VM — Integration' -Tag 'Integration' { $result.ExitCode | Should -Be 0 $result.Stdout | Should -Not -BeNullOrEmpty } + + It 'Should pass -Args to the guest as argv (Invoke-PveVmGuestExec)' { + if (Skip-IfNoLinuxVm) { return } + + # Regression guard for #68: args must reach the process as argv, not STDIN. + # With the old bug, echo got no arguments and stdout was empty. + $marker = 'PSPROXMOXVE_ARG_TEST' + $result = Invoke-PveVmGuestExec -Node $script:Node -VmId $script:LinuxVmId ` + -Command 'echo' -Args @($marker) + $result.ExitCode | Should -Be 0 + $result.Stdout.Trim() | Should -Be $marker + } } Context 'Guest Agent VM — Lifecycle' {