* 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>
D001-D021 become ADR 0001-0021 in docs/decisions/, one decision per file.
D017's PESTER_VERSION amendment was a second decision in one entry and becomes
ADR 0022. ADR 0023 records the migration and reverses the lane2-change-plan
ruling that deliberately kept DECISIONS.md until the CI lane work landed.
DECISIONS.md is reduced to a stub with a D-to-ADR redirect table, so the four
released CHANGELOG entries and older issue bodies that cite it degrade to a
redirect rather than a dead reference.
docs/review/ and docs/lane2-change-plan.md are deleted (ADR 0024). Of 91
findings, 83 were resolved and six of the seven still open were already GitHub
issues; F021 was the exception and is now #130.
CLAUDE.md's Key Conventions list gains the two rules it was missing and becomes
the checklist, with the ADRs carrying rationale.
PveNetwork already deserialised bridge_vlan_aware as BridgeVlanAware, so a
VLAN-aware bridge could be read back but never created or changed. Both write
paths now take a -BridgeVlanAware switch.
Clearing the flag does not use bridge_vlan_aware=0. PVE merges the supplied
keys onto the stored stanza and accepts that 0 without acting on it, so the
obvious form is a silent no-op: an integration run against PVE 9 issued it and
Get-PveNetwork still reported 1. The endpoint's delete list is what actually
removes the key. The API schema advertises a plain boolean and gives no hint of
this, which is why the behaviour is pinned by an integration test rather than
inferred.
Set-PveNetwork guards the switch on BoundParameters so an update that omits it
leaves the flag alone; the create path follows the existing -Autostart form.
Only bridge_vlan_aware is added. bridge_vids is an independent parameter that
PVE defaults to 2-4094, and the issue asks only for the flag.
Coverage: the integration suite pins create, disable, re-enable, and that an
unrelated Set leaves the flag alone -- that last one kills a mutant that drops
the BoundParameters guard, which every other test survives. A model test pins
the read path the assertions depend on. The Pester unit tests assert only
parameter metadata; the defect is server-side, so nothing offline can catch it.
Closes#92
Two non-blocking review observations.
A 45s retry is indistinguishable from a hang with nothing on the wire, so
GuestLockRetry.Execute takes an onRetry hook and InvokeGuestTask reports
each reissue through WriteVerbose.
The gap between attempts now scales with the budget, capped at the 2s
production value. A caller passing a short window wants a fast answer
rather than one long sleep, which also takes the retrying unit tests off
a real 2s sleep each: the xUnit run drops from 8s to 4s. PveHttpClient's
window becomes a field so those tests can shorten it too.
WaitForStatusTransition refused to return while snapshot.Locked, and its
comment quoted the exact error it was meant to prevent. Locked reads the
guest config's lock: property; the failure is the flock on
/var/lock/qemu-server/lock-<vmid>.conf, which PVE exposes nowhere.
The flock cannot be observed, so it is retried. GuestLockRetry reissues an
operation for a bounded 45s while PVE reports failing to enter lock_config
for a guest, which it raises before doing any work.
Two seams, because the failure has two surfaces. PveHttpClient.SendAsync
retries the request for operations PVE serialises in the API handler; it
takes a request factory because an HttpRequestMessage cannot be resent.
PveCmdletBase.InvokeGuestTask reissues the call and re-waits its task for
operations serialised in the forked worker, where the POST returns 200 and
only the task fails. WaitForStatusTransition routes through the latter,
hence Func<PveTask>.
The predicate is path-specific and anchored at the start of what PVE said:
lock_file uses identical wording for storage, LVM and HA locks, and a
qmclone that fails after allocating disks must not be reissued into
"VM already exists". That requires the raw text, so it reads
PveTaskFailedException.ExitStatus and PveApiException.ApiMessage.
The Locked check stays — it is correct for the config lock — with a comment
that says so.
Closes#113
PVE publishes a guest's new status while the operation still holds
/var/lock/qemu-server/lock-<vmid>.conf, so WaitForStatusTransition could return
while the guest was still locked and the caller's next request would fail with
"can't lock file ... got timeout".
Integration run 183 failed four tests from this one cause: Restart-PveVm -Wait
returned after 4.1s having seen "running", the following Stop-PveVm spent exactly
10.0s failing to take the lock, and that cascaded into the template convert,
clone, and remove tests. Run 184 - same commit, re-run - passed because its status
poll happened to take 10.1s, by which point the lock had cleared. The same
settling happens either way; the only variable is whether the wait absorbs it or
the next caller does.
The check goes in WaitForStatusTransition because all nine lifecycle call sites
(Start/Stop/Restart/Reset/Resume across VMs and containers) route through it.
`lock` comes from the status/current response the poll already fetches - present
on both qemu and lxc since PVE 5.4, below the module's 7.0 floor - so it costs no
extra request.
If the status is reached but the lock outlasts -Timeout the cmdlet still returns
success, so a call that succeeded before this change cannot become an exception
after it.
Recorded as D015, the guest-lock sibling of D014.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Returns once quorate, throws TimeoutException when quorum never arrives,
and keeps polling through a PveApiException from the restarting API.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- VmService.ExecuteGuestCommand: guard against null elements in -Args.
The old JSON-serialization tolerated nulls (as "null"); the repeated-key
path would NRE in EncodeFormValue. Throw a clear ArgumentException instead.
- findings.json: refresh the stale counters block (untouched since F085) to
the actual ledger state — next_id 92, resolved 83 — and bump last_updated
to 2026-05-22. last_scan_date stays 2026-03-26 (F086–F091 came from issue
triage, not a formal review scan).
- VmServiceTests: add empty-array (single command entry) and null-element
(throws) cases.
Note: F091 is the correct next ID — F086–F090 already exist from prior
merged PRs (#60/#61/#66/#67); only the counters were lagging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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<string,string> only),
so:
- Add PostAsync(string, IEnumerable<KeyValuePair<string,string>>) 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) <noreply@anthropic.com>
New-PveVm (F089):
- Add -DiskBus (virtio/scsi/sata/ide, default virtio), -ScsiHardware (scsihw),
-DiskIoThread, -DiskAio, -DiskSsd, -DiskDiscard, -DiskCache so a tuned disk
(e.g. virtio-scsi-single + scsi0,iothread=1,aio=native,ssd=1,discard=on) can
be created in one call instead of diskless + a hand-built Set-PveVmConfig string.
- Disk spec built via BuildDiskSpec; ValidateDiskOptions runs before ShouldProcess
and rejects ssd on virtio and iothread on sata/ide or scsi-without-virtio-scsi-single
with clear errors, instead of letting PVE fail at VM start.
Get-PveVmConfig (F090):
- PveVmConfig was a fixed allow-list, silently dropping keys like scsihw, efidisk0,
tpmstate0, hostpci0. Add typed scsihw/efidisk0/tpmstate0 plus a [JsonExtensionData]
catch-all exposed as AdditionalProperties (native types via JsonHelper.ToNative,
per D013 — no JToken leakage). Makes the disk tuning above verifiable by reading
the config back.
Closes#65.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
When HttpClient.Timeout elapses, .NET throws TaskCanceledException — not
HttpRequestException — so the existing catch in PveHttpClient.SendAsync
missed it and callers got a raw stack trace. With -TimeoutSeconds now
configurable and documented, this gap became user-visible.
In .NET 5+ HttpClient surfaces transport timeouts as TaskCanceledException
with a TimeoutException inner; user-driven token cancellation does not.
Catch by that inner-type signature and rethrow as PveApiException with
HttpStatusCode.RequestTimeout, the resource path, and a message that
reports the configured timeout.
Adds SendAsync_TimeoutFires_ThrowsPveApiExceptionWithRequestTimeout which
swaps in a delaying HttpMessageHandler with a 50ms timeout to exercise
the path deterministically. Drops the redundant
DefaultSessionTimeoutIs100Seconds test (covered by
PveSessionTests.Timeout_DefaultIs100Seconds and the existing flow-through
test).
Addresses PR #61 review feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves findings.json conflict — F086 (from #60, merged into main) and
F087 (this branch) both append to the trailing findings array. Kept both
entries, in numeric order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PveHttpClient was constructed without setting HttpClient.Timeout, so
.NET's 100s default applied to every request. Multi-GB ISO uploads via
Send-PveFile on a real LAN reliably tripped this with TaskCanceledException
after 100 seconds, and there was no way to override it.
- PveSession gains a Timeout (TimeSpan) property, defaulting to 100s.
- PveHttpClient accepts an optional per-instance timeout override that
takes precedence over the session timeout.
- 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 default. -TimeoutSeconds 0 means Timeout.InfiniteTimeSpan.
Tracked as F087. Closes#59.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- SizeParser: wrap TB-suffix overflow in try/catch so callers get
ArgumentException with the parameter name rather than OverflowException.
- New-PveVm/New-PveContainer: validate -DiskSize/-RootFsSize before
ShouldProcess so typos like "512M" are rejected even with -WhatIf and
even when the matching -DiskStorage/-RootFsStorage is omitted.
- Add Pester tests for the new DiskSize and RootFsSize validation paths,
including a new New-PveContainer.Tests.ps1.
- Add SizeParserTests coverage for the TB overflow path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Disk and rootfs size strings were interpolated directly into the disk
spec as "<storage>:<size>", so "60G" produced "local-lvm:60G". On
LVM/LVM-thin storages PVE parses the value after the colon as a volume
name unless it is a bare integer, returning "unable to parse lvm volume
name '60G'". File-backed storages mask this by accepting either form.
SizeParser.NormalizeToGibibytes() now strips G/GB/T/TB suffixes and
returns a bare GiB integer string, so the documented "32G" call shape
works on every storage type. Sub-GB units are rejected with a clear
error rather than being silently truncated.
Tracked as F086. Closes#58.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace single-version fixture with per-version specs from pve-api.
Tests now validate enum values against PVE 7 (best-effort), 8, and 9.
Key findings from version-specific specs:
- VM.Monitor: valid in PVE 7+8, removed in PVE 9
- VM.Replicate: added in PVE 9 only
- VM.GuestAgent.*: added in PVE 9 only
- Mapping.*: added in PVE 8+
- glusterfs: not in any version (removed before PVE 7)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 70 xUnit tests that validate every ValidateSet in the module
against the PVE OpenAPI spec. Three bugs found and fixed:
- Storage: remove `glusterfs` (dropped in PVE 9), add `btrfs`, `esxi`
- Backup compression: `none` → `0` (PVE uses "0" not "none")
- Cluster resources: remove `lxc` filter (PVE uses `vm` for both)
The pve-api-enums.json fixture (199KB) is extracted from the full
OpenAPI spec and contains parameter enum values for 302 API paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test was still expecting GetAsync on cloudinit/dump but the service
now calls PutAsync on cloudinit (regenerate endpoint). Updated mock
setup and assertions to match.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- xUnit test project now targets net10.0 instead of net9.0 (EOL Nov 2026)
- Updated GuestAgentExtCmdlets tests for SecureString Password parameter
- Added Timeout parameter tests to GuestAgentCmdlets
- Added ConfirmImpact.High assertions for Suspend/Restart-PveVm
- Added -Confirm:$false to Suspend-PveVm integration test call
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 35 new cmdlets bringing the total to 118:
- Firewall (21): rules, groups, aliases, IP sets, options at
cluster/node/VM/container levels
- Backup (5): ad-hoc vzdump and scheduled backup job CRUD
- SDN IPAM/DNS/Controller (9): plugin management for SDN subsystem
Also includes:
- Fix: Remove-PveRole now has ConfirmImpact.High
- Fix: URL-encode snapshot names in API paths
- Refactor: extract auth header strings to constants in PveHttpClient
- Add PSGallery version badge to README
- Full test coverage: 11 JSON fixtures, xUnit model tests, Pester
unit tests, and integration tests for firewall/backup/OVA import
- Updated manifest, format file, CHANGELOG, README, API coverage docs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- CS8601: Add null-forgiving operator on guarded dictionary assignments
in cmdlets where IsNullOrEmpty check precedes the assignment
- CS8602: Add Assert.NotNull after JObject["data"] in xUnit model tests
(JToken indexer returns nullable on net48)
- CS8604: Add null-forgiving on guarded arguments in NodeService,
WaitPveTaskCmdlet, GetPveTemplateCmdlet, and auth test parameters
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- CS8604: Use cmdlet's own Node property instead of nullable task.Node
in WaitForTask calls across all VM cmdlets
- CS8618: Make PveSession.ServerVersion nullable (PveVersion?) since it
is set post-construction by PveAuthenticator
- CS8603: Add null-forgiving operator where IsNullOrWhiteSpace guards
guarantee non-null in PveHttpClient
- CS1573: Add XML param documentation to all public service methods
- xUnit1012: Make test parameters nullable (string?) for null test cases
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Change test projects from net10.0 to net9.0 to match source projects
- Update build workflow from net8.0 to net9.0
- Upgrade checkout to v5, setup-dotnet to v5 across all workflows
- Remove outdated PS 7.2 test matrix entries
- Restructure integration tests: create test keeps VM alive so start/stop,
snapshot, and cloud-init tests can use it instead of skipping. AfterAll
handles cleanup.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- BaseUrl trailing slash and resource path cleanup: PveSession.BaseUrl now
ends with '/', all cmdlet resource strings stripped of leading slashes and
/api2/json/ prefixes so URLs compose correctly
- PveNodeStatus: rename Name -> Node to match JSON field and test expectations
- NewPveVmCmdlet: auto-allocate vmid via GET cluster/nextid when -VmId omitted
- UploadFileAsync (BZ 7389 workarounds):
* Unquoted multipart boundary in Content-Type header
* Quoted name= values in Content-Disposition (embedded double-quotes)
* Content-Disposition set before Content-Type on file part — PVE's parser
closes the connection if Content-Type appears first
- SendPveIsoCmdlet: run upload in Task.Run, track progress atomically with
Interlocked, poll and call WriteProgress only from pipeline thread to avoid
InvalidOperationException from PSCmdlet thread-affinity requirements
- Add debug/Capture-UploadDiff.ps1: mitmproxy capture script used to isolate
the header-ordering bug by diffing raw multipart bytes from PS vs C# module
Integration test result: 11 passed, 0 failed, 4 skipped (expected — no
template/stopped VMs on bare nested PVE test node).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
144 tests covering authentication (version parsing, session expiry, token
format validation), model deserialization from real PVE 8.x/9.x API JSON
fixtures, and service layer behavior. Includes TestHelper with mock
HttpMessageHandler factory and 21 JSON fixture files.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set up the PSProxmoxVE repository with solution file, project files for
Core library (net10.0+net48), PowerShell module, and xUnit test project.
Includes .gitignore, .editorconfig, LICENSE (MIT), CHANGELOG, and README
with supported version matrix and full cmdlet reference.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goodolclint-claude[bot]