From 1bf7483a4e3c8aa0e0f8ab0b40ad7dd0bee00f52 Mon Sep 17 00:00:00 2001
From: "goodolclint-claude[bot]"
<323206664+goodolclint-claude[bot]@users.noreply.github.com>
Date: Wed, 2 Sep 2026 16:59:57 +0000
Subject: [PATCH] fix: escape dynamic path segments in three Remove-* cmdlets
(#161)
* fix: escape dynamic path segments in three Remove-* cmdlets
Wrap Storage, Vnet, and Zone parameters with Uri.EscapeDataString() in
RemovePveStorageCmdlet, RemovePveSdnVnetCmdlet, and RemovePveSdnZoneCmdlet
to prevent path traversal attacks via the API path.
Add xUnit test demonstrating that escaped paths preserve percent-encoding
(preventing path collapse) while unescaped paths allow segment traversal.
Fixes #145
* fix: route Remove-Pve{Storage,SdnVnet,SdnZone} through their services
Delete the private PveHttpClient construction and inline
Uri.EscapeDataString call in RemovePveStorageCmdlet,
RemovePveSdnVnetCmdlet and RemovePveSdnZoneCmdlet; call
StorageService.RemoveStorage / NetworkService.RemoveSdnZone /
NetworkService.RemoveSdnVnet instead, which already escape the
identifier identically and are now the single place doing so.
Add ValidatePattern on the Storage/Vnet/Zone parameters as defense
in depth, anchored with \A/\z so a trailing newline cannot slip a
disallowed character past the gate.
Replace PveHttpClientPathEscapingTests with a version that actually
regression-tests the real Uri parser (asserts both that %2F survives
and that the unescaped form is absent), and add
StorageServiceTests/NetworkServiceTests cases that mock
IPveHttpClient and verify the exact escaped DELETE path for a
traversal-attempt name.
---------
Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
---
.../Cmdlets/Network/RemovePveSdnVnetCmdlet.cs | 7 +-
.../Cmdlets/Network/RemovePveSdnZoneCmdlet.cs | 7 +-
.../Cmdlets/Storage/RemovePveStorageCmdlet.cs | 7 +-
.../Client/PveHttpClientPathEscapingTests.cs | 78 +++++++++++++++++++
.../Services/NetworkServiceTests.cs | 62 +++++++++++++++
.../Services/StorageServiceTests.cs | 14 ++++
6 files changed, 166 insertions(+), 9 deletions(-)
create mode 100644 tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientPathEscapingTests.cs
create mode 100644 tests/PSProxmoxVE.Core.Tests/Services/NetworkServiceTests.cs
diff --git a/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnVnetCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnVnetCmdlet.cs
index 08d4721..82092b8 100644
--- a/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnVnetCmdlet.cs
+++ b/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnVnetCmdlet.cs
@@ -1,5 +1,5 @@
using System.Management.Automation;
-using PSProxmoxVE.Core.Client;
+using PSProxmoxVE.Core.Services;
namespace PSProxmoxVE.Cmdlets.Network
{
@@ -15,6 +15,7 @@ namespace PSProxmoxVE.Cmdlets.Network
{
/// The VNet identifier to remove.
[Parameter(Mandatory = true, Position = 0, ValueFromPipelineByPropertyName = true, HelpMessage = "The SDN VNet name.")]
+ [ValidatePattern(@"\A[A-Za-z0-9][A-Za-z0-9._-]*\z")]
public string Vnet { get; set; } = string.Empty;
protected override void ProcessRecord()
@@ -24,10 +25,10 @@ namespace PSProxmoxVE.Cmdlets.Network
var session = GetSession();
RequireVersion(session, "SDN", 6, 2, 8, 0);
- using var client = new PveHttpClient(session);
+ var service = new NetworkService();
WriteVerbose($"Removing SDN VNet '{Vnet}'...");
- client.DeleteAsync($"cluster/sdn/vnets/{Vnet}").GetAwaiter().GetResult();
+ service.RemoveSdnVnet(session, Vnet);
}
}
}
diff --git a/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnZoneCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnZoneCmdlet.cs
index 73aa216..785277c 100644
--- a/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnZoneCmdlet.cs
+++ b/src/PSProxmoxVE/Cmdlets/Network/RemovePveSdnZoneCmdlet.cs
@@ -1,5 +1,5 @@
using System.Management.Automation;
-using PSProxmoxVE.Core.Client;
+using PSProxmoxVE.Core.Services;
namespace PSProxmoxVE.Cmdlets.Network
{
@@ -16,6 +16,7 @@ namespace PSProxmoxVE.Cmdlets.Network
{
/// The zone identifier to remove.
[Parameter(Mandatory = true, Position = 0, ValueFromPipelineByPropertyName = true, HelpMessage = "The SDN zone name.")]
+ [ValidatePattern(@"\A[A-Za-z0-9][A-Za-z0-9._-]*\z")]
public string Zone { get; set; } = string.Empty;
protected override void ProcessRecord()
@@ -25,10 +26,10 @@ namespace PSProxmoxVE.Cmdlets.Network
var session = GetSession();
RequireVersion(session, "SDN", 6, 2, 8, 0);
- using var client = new PveHttpClient(session);
+ var service = new NetworkService();
WriteVerbose($"Removing SDN zone '{Zone}'...");
- client.DeleteAsync($"cluster/sdn/zones/{Zone}").GetAwaiter().GetResult();
+ service.RemoveSdnZone(session, Zone);
}
}
}
diff --git a/src/PSProxmoxVE/Cmdlets/Storage/RemovePveStorageCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Storage/RemovePveStorageCmdlet.cs
index abb9546..6e347cf 100644
--- a/src/PSProxmoxVE/Cmdlets/Storage/RemovePveStorageCmdlet.cs
+++ b/src/PSProxmoxVE/Cmdlets/Storage/RemovePveStorageCmdlet.cs
@@ -1,5 +1,5 @@
using System.Management.Automation;
-using PSProxmoxVE.Core.Client;
+using PSProxmoxVE.Core.Services;
namespace PSProxmoxVE.Cmdlets.Storage
{
@@ -18,6 +18,7 @@ namespace PSProxmoxVE.Cmdlets.Storage
{
/// The storage identifier to remove.
[Parameter(Mandatory = true, Position = 0, ValueFromPipelineByPropertyName = true, HelpMessage = "The storage pool name.")]
+ [ValidatePattern(@"\A[A-Za-z0-9][A-Za-z0-9._-]*\z")]
public string Storage { get; set; } = string.Empty;
protected override void ProcessRecord()
@@ -26,10 +27,10 @@ namespace PSProxmoxVE.Cmdlets.Storage
return;
var session = GetSession();
- using var client = new PveHttpClient(session);
+ var service = new StorageService();
WriteVerbose($"Removing storage '{Storage}'...");
- client.DeleteAsync($"storage/{Storage}").GetAwaiter().GetResult();
+ service.RemoveStorage(session, Storage);
}
}
}
diff --git a/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientPathEscapingTests.cs b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientPathEscapingTests.cs
new file mode 100644
index 0000000..79b85c6
--- /dev/null
+++ b/tests/PSProxmoxVE.Core.Tests/Client/PveHttpClientPathEscapingTests.cs
@@ -0,0 +1,78 @@
+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 PveHttpClientPathEscapingTests
+ {
+ 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 (PveHttpClient client, ScriptedHandler handler) NewClient(
+ params (HttpStatusCode status, string body)[] responses)
+ {
+ var session = new PveSession("pve.example.com", 8006, false,
+ "root@pam!token=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee");
+ var client = new PveHttpClient(session);
+ var handler = new ScriptedHandler(responses);
+ SetInnerHttpClient(client, new HttpClient(handler));
+ return (client, handler);
+ }
+
+ [Fact]
+ public async Task DeleteAsync_WithEscapedPathSegment_DoesNotCollapseAcrossTheRealUriParser()
+ {
+ var maliciousName = "../access/users/root@pam!t";
+ var (client, handler) = NewClient(
+ (HttpStatusCode.OK, "{\"data\":null}"));
+
+ using (client)
+ {
+ await client.DeleteAsync($"storage/{Uri.EscapeDataString(maliciousName)}");
+ }
+
+ Assert.Single(handler.Uris);
+ var uri = handler.Uris[0];
+ Assert.Contains("storage/..%2Faccess", uri);
+ Assert.DoesNotContain("storage/../", uri);
+ }
+
+ private sealed class ScriptedHandler : HttpMessageHandler
+ {
+ private readonly (HttpStatusCode status, string body)[] _responses;
+ private int _index;
+
+ public List Uris { get; } = new List();
+
+ public ScriptedHandler((HttpStatusCode status, string body)[] responses)
+ {
+ _responses = responses;
+ }
+
+ protected override async Task SendAsync(
+ HttpRequestMessage request, CancellationToken cancellationToken)
+ {
+ Uris.Add(request.RequestUri!.ToString());
+
+ if (_index >= _responses.Length)
+ throw new InvalidOperationException("ScriptedHandler ran out of responses.");
+
+ var (status, body) = _responses[_index++];
+ return new HttpResponseMessage(status) { Content = new StringContent(body) };
+ }
+ }
+ }
+}
diff --git a/tests/PSProxmoxVE.Core.Tests/Services/NetworkServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/NetworkServiceTests.cs
new file mode 100644
index 0000000..ff8db6a
--- /dev/null
+++ b/tests/PSProxmoxVE.Core.Tests/Services/NetworkServiceTests.cs
@@ -0,0 +1,62 @@
+using Moq;
+using Xunit;
+using PSProxmoxVE.Core.Authentication;
+using PSProxmoxVE.Core.Client;
+using PSProxmoxVE.Core.Services;
+
+namespace PSProxmoxVE.Core.Tests.Services
+{
+ public class NetworkServiceTests
+ {
+ private readonly Mock _mockClient;
+ private readonly NetworkService _service;
+ private readonly PveSession _session;
+
+ public NetworkServiceTests()
+ {
+ _mockClient = new Mock();
+ _service = new NetworkService(_mockClient.Object);
+ _session = new PveSession(
+ "pve.example.com",
+ 8006,
+ skipCertificateCheck: true,
+ apiToken: "root@pam!test=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee");
+ }
+
+ // -----------------------------------------------------------------
+ // RemoveSdnZone
+ // -----------------------------------------------------------------
+
+ [Fact]
+ public void RemoveSdnZone_EscapesPathTraversalInName()
+ {
+ // Arrange
+ _mockClient.Setup(c => c.DeleteAsync("cluster/sdn/zones/..%2Faccess%2Fusers%2Fx"))
+ .ReturnsAsync(@"{""data"":null}");
+
+ // Act
+ _service.RemoveSdnZone(_session, "../access/users/x");
+
+ // Assert
+ _mockClient.Verify(c => c.DeleteAsync("cluster/sdn/zones/..%2Faccess%2Fusers%2Fx"), Times.Once);
+ }
+
+ // -----------------------------------------------------------------
+ // RemoveSdnVnet
+ // -----------------------------------------------------------------
+
+ [Fact]
+ public void RemoveSdnVnet_EscapesPathTraversalInName()
+ {
+ // Arrange
+ _mockClient.Setup(c => c.DeleteAsync("cluster/sdn/vnets/..%2Faccess%2Fusers%2Fx"))
+ .ReturnsAsync(@"{""data"":null}");
+
+ // Act
+ _service.RemoveSdnVnet(_session, "../access/users/x");
+
+ // Assert
+ _mockClient.Verify(c => c.DeleteAsync("cluster/sdn/vnets/..%2Faccess%2Fusers%2Fx"), Times.Once);
+ }
+ }
+}
diff --git a/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs b/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs
index 97d5a8c..f2c78c1 100644
--- a/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs
+++ b/tests/PSProxmoxVE.Core.Tests/Services/StorageServiceTests.cs
@@ -246,6 +246,20 @@ namespace PSProxmoxVE.Core.Tests.Services
_mockClient.Verify(c => c.DeleteAsync("storage/nfs-backup"), Times.Once);
}
+ [Fact]
+ public void RemoveStorage_EscapesPathTraversalInName()
+ {
+ // Arrange
+ _mockClient.Setup(c => c.DeleteAsync("storage/..%2Faccess%2Fusers%2Fx"))
+ .ReturnsAsync(@"{""data"":null}");
+
+ // Act
+ _service.RemoveStorage(_session, "../access/users/x");
+
+ // Assert
+ _mockClient.Verify(c => c.DeleteAsync("storage/..%2Faccess%2Fusers%2Fx"), Times.Once);
+ }
+
[Fact]
public void RemoveStorage_NullSession_ThrowsArgumentNullException()
{