Merge pull request #69 from GoodOlClint/fix/guest-exec-args

fix: deliver Invoke-PveVmGuestExec -Args as argv (#68)
2026-07-26 07:58:14 +00:00 · 2026-05-22 14:41:45 -05:00
parent 5266accfae 56dcf22cea
commit 5cb5c90db8
7 changed files with 239 additions and 14 deletions
@@ -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<string, object?> 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<string,string> 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<KeyValuePair<string,string>>) 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)"
+      }
    }
  ]
 }
@@ -16,6 +16,13 @@ namespace PSProxmoxVE.Core.Client
        /// <summary>Performs a POST request against the specified API resource path.</summary>
        Task<string> PostAsync(string resource, Dictionary<string, string>? data = null);

+        /// <summary>
+        /// 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
+        /// <c>key=value</c> field, so a key may appear multiple times.
+        /// </summary>
+        Task<string> PostAsync(string resource, IEnumerable<KeyValuePair<string, string>> data);
+
        /// <summary>Performs a PUT request against the specified API resource path.</summary>
        Task<string> PutAsync(string resource, Dictionary<string, string>? data = null);

@@ -25,7 +32,7 @@ namespace PSProxmoxVE.Core.Client
        /// <summary>Synchronous wrapper for <see cref="GetAsync"/>.</summary>
        string Get(string resource);

-        /// <summary>Synchronous wrapper for <see cref="PostAsync"/>.</summary>
+        /// <summary>Synchronous wrapper for <see cref="PostAsync(string, Dictionary{string, string})"/>.</summary>
        string Post(string resource, Dictionary<string, string>? data = null);

        /// <summary>Synchronous wrapper for <see cref="PutAsync"/>.</summary>
@@ -111,6 +111,21 @@ namespace PSProxmoxVE.Core.Client
            return await SendAsync(request, resource, "POST").ConfigureAwait(false);
        }

+        /// <summary>
+        /// POST whose form body may contain repeated keys, for PVE array parameters
+        /// (e.g. guest-exec "command"). Each pair becomes one key=value field.
+        /// </summary>
+        /// <param name="resource">Relative resource path</param>
+        /// <param name="data">Form fields; a key may appear more than once</param>
+        /// <returns>Raw JSON response body</returns>
+        public async Task<string> PostAsync(string resource, IEnumerable<KeyValuePair<string, string>> 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);
+        }
+
        /// <summary>Performs a PUT request against the specified API resource path.</summary>
        /// <param name="resource">Relative resource path</param>
        /// <param name="data">Form fields to send as application/x-www-form-urlencoded body</param>
@@ -142,7 +157,7 @@ namespace PSProxmoxVE.Core.Client
        /// <c>:</c> and <c>!</c> which PVE's internal API consumers (e.g. cluster join)
        /// do not properly URL-decode.
        /// </summary>
-        private static StringContent BuildFormContent(Dictionary<string, string> data)
+        private static StringContent BuildFormContent(IEnumerable<KeyValuePair<string, string>> 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();

-        /// <summary>Synchronous wrapper for <see cref="PostAsync"/>.</summary>
+        /// <summary>Synchronous wrapper for <see cref="PostAsync(string, Dictionary{string, string})"/>.</summary>
        public string Post(string resource, Dictionary<string, string>? data = null) =>
            PostAsync(resource, data).GetAwaiter().GetResult();

@@ -599,16 +599,22 @@ namespace PSProxmoxVE.Core.Services
            IPveHttpClient client = _injectedClient ?? new PveHttpClient(session);
            try
            {
-                var data = new Dictionary<string, string>
+                // 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=<exe>&command=<arg1>&...). Do NOT use "input-data" for
+                // arguments — that is the process's STDIN, not argv.
+                var data = new List<KeyValuePair<string, string>>
                {
-                    ["command"] = command
+                    new KeyValuePair<string, string>("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<string, string>("command", arg));
+                    }
                }

                var response = client.PostAsync($"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/agent/exec", data)
@@ -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<KeyValuePair<string, string>>
+                {
+                    new KeyValuePair<string, string>("command", "cmd.exe"),
+                    new KeyValuePair<string, string>("command", "/c"),
+                    new KeyValuePair<string, string>("command", "echo"),
+                    new KeyValuePair<string, string>("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<KeyValuePair<string, string>>
+                {
+                    new KeyValuePair<string, string>("command", "powershell.exe"),
+                    new KeyValuePair<string, string>("command", "-Command"),
+                    new KeyValuePair<string, string>("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;
@@ -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<KeyValuePair<string, string>>? captured = null;
+            var mockClient = new Mock<IPveHttpClient>();
+            mockClient
+                .Setup(c => c.PostAsync(It.IsAny<string>(), It.IsAny<IEnumerable<KeyValuePair<string, string>>>()))
+                .Callback<string, IEnumerable<KeyValuePair<string, string>>>((_, 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<KeyValuePair<string, string>>? captured = null;
+            var mockClient = new Mock<IPveHttpClient>();
+            mockClient
+                .Setup(c => c.PostAsync(It.IsAny<string>(), It.IsAny<IEnumerable<KeyValuePair<string, string>>>()))
+                .Callback<string, IEnumerable<KeyValuePair<string, string>>>((_, 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<KeyValuePair<string, string>>? captured = null;
+            var mockClient = new Mock<IPveHttpClient>();
+            mockClient
+                .Setup(c => c.PostAsync(It.IsAny<string>(), It.IsAny<IEnumerable<KeyValuePair<string, string>>>()))
+                .Callback<string, IEnumerable<KeyValuePair<string, string>>>((_, 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<KeyValuePair<string, string>>? captured = null;
+            var mockClient = new Mock<IPveHttpClient>();
+            mockClient
+                .Setup(c => c.PostAsync(It.IsAny<string>(), It.IsAny<IEnumerable<KeyValuePair<string, string>>>()))
+                .Callback<string, IEnumerable<KeyValuePair<string, string>>>((_, 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<IPveHttpClient>();
+            var service = new VmService(mockClient.Object);
+
+            var ex = Assert.Throws<ArgumentException>(() =>
+                service.ExecuteGuestCommand(CreateSession(), TestNode, TestVmId,
+                    "cmd.exe", new[] { "/c", null!, "echo" }));
+            Assert.Equal("args", ex.ParamName);
+        }
+    }
+}
@@ -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' {