diff --git a/docs/review/findings.json b/docs/review/findings.json index a93c804..df95a91 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -1,15 +1,15 @@ { "_schema_version": "1.0", "_description": "PSProxmoxVE stable findings ledger. IDs are permanent (F001, F002...). Resolved findings are never deleted — they are marked resolved with evidence. If a finding reappears, it is marked regressed and retains its original ID.", - "last_updated": "2026-03-26", + "last_updated": "2026-05-22", "last_scan_date": "2026-03-26", "counters": { - "next_id": 86, + "next_id": 92, "total_open": 7, - "total_resolved": 77, + "total_resolved": 83, "total_regressed": 0, "open": 7, - "resolved": 77, + "resolved": 83, "regressed": 0, "wont_fix": 1 }, @@ -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..5b7764a 100644 --- a/src/PSProxmoxVE.Core/Services/VmService.cs +++ b/src/PSProxmoxVE.Core/Services/VmService.cs @@ -599,16 +599,22 @@ 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) + { + if (arg == null) + throw new ArgumentException("Args elements must not be null.", nameof(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..136b064 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Services/VmServiceTests.cs @@ -0,0 +1,113 @@ +using System; +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); + } + + [Fact] + public void ExecuteGuestCommand_EmptyArgs_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\":9}}"); + + var service = new VmService(mockClient.Object); + service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId, "whoami", new string[0]); + + Assert.NotNull(captured); + var only = Assert.Single(captured!); + Assert.Equal("command", only.Key); + Assert.Equal("whoami", only.Value); + } + + [Fact] + public void ExecuteGuestCommand_NullArgElement_ThrowsArgumentException() + { + var mockClient = new Mock(); + var service = new VmService(mockClient.Object); + + var ex = Assert.Throws(() => + service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId, + "cmd.exe", new[] { "/c", null!, "echo" })); + Assert.Equal("args", ex.ParamName); + } + } +} 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' {