From 3fd770d84cf61df25b233faa30485aaf99825610 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Fri, 22 May 2026 14:04:31 -0500 Subject: [PATCH] fix: percent-encode ';' in form values PveHttpClient.EncodeFormValue encoded &, =, +, space, and % but left ';' literal. PVE's application/x-www-form-urlencoded parser treats a raw ';' as a field separator (the historical alternative to '&'), so a value like boot=order=scsi0;ide2 was split into 'boot=order=scsi0' plus an empty 'ide2' field, and PVE rejected the PUT with "ide2: unable to parse drive options". This broke any multi-device boot order set via Set-PveVmConfig -AdditionalConfig, and any other value containing ';'. Encode ';' as %3B. Safe under the existing minimal-encoding policy that keeps ':' and '!' literal for cluster-join: cluster-join payloads never contain ';', and PVE url-decodes config form values. Tracked as F088. Closes #64. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/review/findings.json | 30 ++++++ src/PSProxmoxVE.Core/Client/PveHttpClient.cs | 5 + .../Client/PveHttpClientFormEncodingTests.cs | 99 +++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs diff --git a/docs/review/findings.json b/docs/review/findings.json index 4db20e6..4f4311a 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2881,6 +2881,36 @@ "evidence": "Added PveSession.Timeout (default 100s) and a TimeSpan? override on PveHttpClient. Connect-PveServer exposes -TimeoutSeconds to set the session-default; Send-PveFile and Invoke-PveStorageDownload expose -TimeoutSeconds with a 30-minute implicit default so large uploads/downloads do not trip the 100s HttpClient default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan. PveHttpClient.SendAsync now catches the TimeoutException-wrapped TaskCanceledException and rethrows it as PveApiException(RequestTimeout) with the resource path.", "verified_by": "dotnet build + dotnet test (passed including new SendAsync_TimeoutFires xUnit test) + Pester (-TimeoutSeconds coverage on all three cmdlets)" } + }, + { + "id": "F088", + "title": "Form values containing ';' are split into bogus fields (boot order breaks Set-PveVmConfig)", + "category": "api_contract", + "severity": "high", + "status": "resolved", + "first_detected": "2026-05-22", + "github_issue": 64, + "files": [ + "src/PSProxmoxVE.Core/Client/PveHttpClient.cs" + ], + "description": "PveHttpClient.EncodeFormValue percent-encoded &, =, +, space, and % but not ';'. PVE's application/x-www-form-urlencoded parser treats a raw ';' as a field separator, so a value like boot=order=scsi0;ide2 was split into 'boot=order=scsi0' plus an empty 'ide2' field, which PVE rejected with 'ide2: unable to parse drive options'. Affected any config value containing ';' (boot order, hookscript, some args).", + "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 ';' -> %3B to EncodeFormValue. Safe under the minimal-encoding policy (cluster-join values never contain ';' and PVE url-decodes config form values). Verified the colon/comma minimal-encoding behavior is preserved.", + "verified_by": "dotnet build + dotnet test (3 new PveHttpClientFormEncodingTests: semicolon encoded on POST/PUT, comma/colon remain literal)" + } } ] } diff --git a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs index 3729b10..6c61425 100644 --- a/src/PSProxmoxVE.Core/Client/PveHttpClient.cs +++ b/src/PSProxmoxVE.Core/Client/PveHttpClient.cs @@ -169,6 +169,11 @@ namespace PSProxmoxVE.Core.Client { case '&': sb.Append("%26"); break; case '=': sb.Append("%3D"); break; + // PVE's form parser treats a raw ';' as a field separator (the + // historical alternative to '&'), so an unencoded ';' inside a value + // (e.g. boot=order=scsi0;ide2) splits the value into bogus extra + // fields. Encode it so the value arrives intact. + case ';': sb.Append("%3B"); break; case '+': sb.Append("%2B"); break; case ' ': sb.Append('+'); break; case '%': sb.Append("%25"); break; diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs new file mode 100644 index 0000000..25be8dd --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientFormEncodingTests.cs @@ -0,0 +1,99 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using PSProxmoxVE.Core.Authentication; +using PSProxmoxVE.Core.Client; +using Xunit; + +namespace PSProxmoxVE.Core.Tests.Client +{ + public class PveHttpClientFormEncodingTests + { + private static void SetInnerHttpClient(PveHttpClient client, HttpClient newInner) + { + var field = typeof(PveHttpClient).GetField("_httpClient", + BindingFlags.Instance | BindingFlags.NonPublic)!; + ((HttpClient)field.GetValue(client)!).Dispose(); + field.SetValue(client, newInner); + } + + private static PveSession NewSession() + { + return new PveSession("pve.example.com", 8006, false, + "root@pam!token=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); + } + + private static (PveHttpClient client, CapturingHandler handler) NewCapturingClient() + { + var client = new PveHttpClient(NewSession()); + var handler = new CapturingHandler(); + SetInnerHttpClient(client, new HttpClient(handler)); + return (client, handler); + } + + [Fact] + public async Task PostAsync_SemicolonInValue_IsPercentEncoded() + { + var (client, handler) = NewCapturingClient(); + using (client) + { + await client.PostAsync("nodes/pve/qemu/100/config", + new Dictionary { ["boot"] = "order=scsi0;ide2" }); + } + + // PVE treats a raw ';' as a form-field separator, so it must be encoded. + Assert.Contains("boot=order%3Dscsi0%3Bide2", handler.LastBody); + Assert.DoesNotContain(";", handler.LastBody); + } + + [Fact] + public async Task PutAsync_SemicolonInValue_IsPercentEncoded() + { + var (client, handler) = NewCapturingClient(); + using (client) + { + await client.PutAsync("nodes/pve/qemu/100/config", + new Dictionary { ["boot"] = "order=ide2;virtio0;net0" }); + } + + Assert.Contains("boot=order%3Dide2%3Bvirtio0%3Bnet0", handler.LastBody); + Assert.DoesNotContain(";", handler.LastBody); + } + + [Fact] + public async Task PostAsync_CommaAndColonInValue_RemainLiteral() + { + var (client, handler) = NewCapturingClient(); + using (client) + { + // Drive options use literal commas; cluster-join values use literal colons. + // Minimal-encoding policy must keep both unescaped. + await client.PostAsync("nodes/pve/qemu/100/config", + new Dictionary { ["ide2"] = "nas:iso/win.iso,media=cdrom" }); + } + + Assert.Contains("ide2=nas:iso/win.iso,media%3Dcdrom", handler.LastBody); + } + + private sealed class CapturingHandler : HttpMessageHandler + { + public string LastBody { get; private set; } = string.Empty; + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + LastBody = request.Content == null + ? string.Empty + : await request.Content.ReadAsStringAsync().ConfigureAwait(false); + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"data\":null}") + }; + } + } + } +}