From fa361a3691826deca78443ac6230c8ad7f0235d9 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 20 May 2026 18:06:13 -0500 Subject: [PATCH] fix: address PR #60 review feedback - 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) --- docs/review/findings.json | 4 +- src/PSProxmoxVE.Core/Utilities/SizeParser.cs | 8 ++- .../Containers/NewPveContainerCmdlet.cs | 14 ++-- src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs | 12 +++- .../Utilities/SizeParserTests.cs | 10 +++ .../Containers/New-PveContainer.Tests.ps1 | 68 +++++++++++++++++++ .../PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 | 35 ++++++++++ 7 files changed, 140 insertions(+), 11 deletions(-) create mode 100644 tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 diff --git a/docs/review/findings.json b/docs/review/findings.json index aaa8c77..b6bd0a6 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -2843,8 +2843,8 @@ ], "resolution": { "scan_date": "2026-05-20", - "evidence": "Added SizeParser.NormalizeToGibibytes() which strips G/GB/T/TB suffixes (and rejects sub-GB units with a clear error). New-PveVm and New-PveContainer normalize -DiskSize and -RootFsSize through it before constructing the disk spec, so '32G' becomes ':32' on every storage type.", - "verified_by": "dotnet build + dotnet test (576 passed, 31 new SizeParserTests)" + "evidence": "Added SizeParser.NormalizeToGibibytes() which strips G/GB/T/TB suffixes, rejects sub-GB units with a clear error, and converts TB overflows to ArgumentException. New-PveVm and New-PveContainer normalize -DiskSize and -RootFsSize before ShouldProcess so typos are caught with -WhatIf, regardless of whether the matching -DiskStorage/-RootFsStorage was supplied.", + "verified_by": "dotnet build + dotnet test (577 passed, 32 SizeParserTests) + Pester (39 passed, new DiskSize/RootFsSize validation contexts)" } } ] diff --git a/src/PSProxmoxVE.Core/Utilities/SizeParser.cs b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs index 307f569..f3fa34d 100644 --- a/src/PSProxmoxVE.Core/Utilities/SizeParser.cs +++ b/src/PSProxmoxVE.Core/Utilities/SizeParser.cs @@ -59,7 +59,13 @@ namespace PSProxmoxVE.Core.Utilities case "T": case "TB": case "TIB": - gib = checked(num * 1024L); + try { gib = checked(num * 1024L); } + catch (OverflowException) + { + throw new ArgumentException( + $"{parameterName} '{value}' is too large to represent in GiB.", + parameterName); + } break; default: throw new ArgumentException( diff --git a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs index b0213e1..42b4d9d 100644 --- a/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Containers/NewPveContainerCmdlet.cs @@ -130,6 +130,13 @@ namespace PSProxmoxVE.Cmdlets.Containers protected override void ProcessRecord() { + // Validate -RootFsSize before ShouldProcess so typos like "512M" are rejected + // even with -WhatIf, and so the error is raised regardless of whether + // -RootFsStorage is also supplied. + string? rootFsSizeGib = null; + if (!string.IsNullOrEmpty(RootFsSize)) + rootFsSizeGib = SizeParser.NormalizeToGibibytes(RootFsSize!, nameof(RootFsSize)); + if (!ShouldProcess($"Container on node '{Node}'", "New-PveContainer")) return; @@ -165,11 +172,8 @@ namespace PSProxmoxVE.Cmdlets.Containers if (!string.IsNullOrEmpty(RootFsStorage)) { var rootFsValue = RootFsStorage!; - if (!string.IsNullOrEmpty(RootFsSize)) - { - var sizeGib = SizeParser.NormalizeToGibibytes(RootFsSize!, nameof(RootFsSize)); - rootFsValue += $":{sizeGib}"; - } + if (rootFsSizeGib != null) + rootFsValue += $":{rootFsSizeGib}"; config["rootfs"] = rootFsValue; } diff --git a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs index d0e189f..0c90225 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/NewPveVmCmdlet.cs @@ -128,6 +128,13 @@ namespace PSProxmoxVE.Cmdlets.Vms protected override void ProcessRecord() { + // Validate -DiskSize before ShouldProcess so typos like "512M" are rejected + // even with -WhatIf, and so the error is raised regardless of whether + // -DiskStorage is also supplied. + string? diskSizeGib = null; + if (!string.IsNullOrEmpty(DiskSize)) + diskSizeGib = SizeParser.NormalizeToGibibytes(DiskSize!, nameof(DiskSize)); + if (!ShouldProcess($"VM on node '{Node}'", "New-PveVm")) return; @@ -166,10 +173,9 @@ namespace PSProxmoxVE.Cmdlets.Vms if (!string.IsNullOrEmpty(OsType)) config["ostype"] = OsType!; - if (!string.IsNullOrEmpty(DiskStorage) && !string.IsNullOrEmpty(DiskSize)) + if (!string.IsNullOrEmpty(DiskStorage) && diskSizeGib != null) { - var sizeGib = SizeParser.NormalizeToGibibytes(DiskSize!, nameof(DiskSize)); - var diskValue = $"{DiskStorage}:{sizeGib}"; + var diskValue = $"{DiskStorage}:{diskSizeGib}"; if (!string.IsNullOrEmpty(DiskFormat)) diskValue += $",format={DiskFormat}"; config["virtio0"] = diskValue; diff --git a/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs index ba734b6..4961215 100644 --- a/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs +++ b/tests/PSProxmoxVE.Core.Tests/Utilities/SizeParserTests.cs @@ -76,5 +76,15 @@ namespace PSProxmoxVE.Core.Tests.Utilities Assert.Equal("DiskSize", ex.ParamName); Assert.Contains("DiskSize", ex.Message); } + + [Fact] + public void NormalizeToGibibytes_TerabyteOverflow_ThrowsArgumentException() + { + // long.MaxValue with a T suffix overflows when multiplied by 1024. + var input = long.MaxValue.ToString(System.Globalization.CultureInfo.InvariantCulture) + "T"; + var ex = Assert.Throws(() => SizeParser.NormalizeToGibibytes(input, "DiskSize")); + Assert.Equal("DiskSize", ex.ParamName); + Assert.Contains("too large", ex.Message, StringComparison.OrdinalIgnoreCase); + } } } diff --git a/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 b/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 new file mode 100644 index 0000000..8b989f8 --- /dev/null +++ b/tests/PSProxmoxVE.Tests/Containers/New-PveContainer.Tests.ps1 @@ -0,0 +1,68 @@ +#Requires -Module Pester +<# +.SYNOPSIS + Pester 5 tests for New-PveContainer. + All tests are fully offline — no live Proxmox VE target is required. +#> + +BeforeAll { + . $PSScriptRoot/../_TestHelper.ps1 +} + +Describe 'New-PveContainer' { + + Context 'Command existence' { + It 'Should be available after module import' { + Get-Command 'New-PveContainer' -ErrorAction SilentlyContinue | + Should -Not -BeNullOrEmpty + } + + It 'Should be a CmdletInfo (binary cmdlet)' { + (Get-Command 'New-PveContainer').CommandType | Should -Be 'Cmdlet' + } + } + + Context 'ShouldProcess support' { + BeforeAll { + $script:Cmd = Get-Command 'New-PveContainer' + } + + It 'Should support ShouldProcess (WhatIf parameter present)' { + $script:Cmd.Parameters.ContainsKey('WhatIf') | Should -BeTrue + } + + It 'Should support ShouldProcess (Confirm parameter present)' { + $script:Cmd.Parameters.ContainsKey('Confirm') | Should -BeTrue + } + } + + Context 'RootFsSize validation' { + # Validation runs before ShouldProcess so -WhatIf is enough to exercise it + # without an active session. + + It 'Should reject sub-GB units (e.g. 512M)' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject sub-GB units even when -RootFsStorage is omitted' { + { New-PveContainer -Node 'pve-node1' -RootFsSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject malformed input (e.g. 8.5G)' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8.5G' -WhatIf -ErrorAction Stop } | + Should -Throw '*not a valid size*' + } + + It 'Should accept a bare integer with -WhatIf' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "8G" with -WhatIf' { + { New-PveContainer -Node 'pve-node1' -RootFsStorage 'local-lvm' -RootFsSize '8G' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + } +} diff --git a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 index d3da01d..e26116e 100644 --- a/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Vms/New-PveVm.Tests.ps1 @@ -133,4 +133,39 @@ Describe 'New-PveVm' { Should -Throw '*No active Proxmox VE session*' } } + + Context 'DiskSize validation' { + # Validation runs before ShouldProcess so -WhatIf is enough to exercise it + # without an active session. + + It 'Should reject sub-GB units (e.g. 512M)' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject sub-GB units even when -DiskStorage is omitted' { + { New-PveVm -Node 'pve-node1' -DiskSize '512M' -WhatIf -ErrorAction Stop } | + Should -Throw '*unsupported unit*' + } + + It 'Should reject malformed input (e.g. 32.5G)' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32.5G' -WhatIf -ErrorAction Stop } | + Should -Throw '*not a valid size*' + } + + It 'Should accept a bare integer with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "32G" with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '32G' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + + It 'Should accept "1T" with -WhatIf' { + { New-PveVm -Node 'pve-node1' -DiskStorage 'local-lvm' -DiskSize '1T' -WhatIf -ErrorAction Stop } | + Should -Not -Throw + } + } }