gsadmin/PSProxmoxVE
1
0
Fork 0
mirror of https://github.com/GoodOlClint/PSProxmoxVE.git synced 2026-07-26 07:58:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

docs: update README, CHANGELOG, SECURITY.md, and review artifacts

Browse Source 
- README: move Set-PveStorage to Storage section, Set-PveApiToken to
  Access section (were miscategorized under "Access — Groups & Domains")
- CHANGELOG: add Changed section with 13 architectural improvements,
  expand Fixed section with all ConfirmImpact and credential fixes
- SECURITY.md: add SecureString detail, Input Validation section,
  Dependabot mention, SkipCertificateCheck warning note
- CLAUDE.md: update review system reference to scan-7
- findings.json: F050 resolved (URL encoding), F031 resolved (credentials)
- REVIEW_REPORT.md: scan-7 report

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Clint Branham
2026-03-23 17:41:19 -05:00
parent f6a2f843db
commit b4754c72a0
6 changed files with 288 additions and 255 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CHANGELOG.md
+19 -3
View File
@@ -30,11 +30,27 @@ and this project adheres to [Conventional Commits](https://www.conventionalcommi
- Access management (9): Get/New/Set/Remove-PveGroup, Get/New/Set/Remove-PveDomain, Set-PvePassword - Access management (9): Get/New/Set/Remove-PveGroup, Get/New/Set/Remove-PveDomain, Set-PvePassword
- Two-tier version gating: introduced vs default version with clear user messaging - Two-tier version gating: introduced vs default version with clear user messaging
### Changed
- All cmdlet classes sealed for design clarity and JIT optimization
- `[OutputType]` attribute added to all 169 cmdlets for IntelliSense and pipeline support
- Publishable projects retargeted to `netstandard2.0` for PS 5.1 + PS 7.x compatibility
- `System.Text.Json` attributes removed — module uses `Newtonsoft.Json` exclusively
- Inline task-polling loops replaced with `TaskService.WaitForTask` (timeout + progress support)
- Password parameters changed from `string` to `SecureString` with secure memory handling
- `ValidateRange(100, 999999999)` added to all `VmId` parameters
- `Uri.EscapeDataString()` applied to all dynamic URL path segments
- Hardcoded verb strings replaced with verb class constants (`VerbsCommon.Get`, etc.)
- Auth header magic strings extracted to named constants in PveHttpClient
- Bare `catch` blocks replaced with specific or filtered exception handling
- MAML help (dll-Help.xml) and 170 markdown cmdlet docs generated
- PSGallery publish workflow with PS 5.1 smoke testing
### Fixed ### Fixed
- `Remove-PveRole` now has `ConfirmImpact.High` for safety - `ConfirmImpact.High` added to all destructive cmdlets (Stop, Reset, Restart, Suspend, Remove, Restore, New-PveTemplate)
- URL-encode snapshot names in API paths for defense-in-depth - Hardcoded test password moved from CI workflow to GitHub Actions secret
- Extract auth header magic strings to named constants in PveHttpClient - Terraform variable default password removed (requires env var)
## [0.1.0-preview] - 2026-03-19 ## [0.1.0-preview] - 2026-03-19
CLAUDE.md
+1 -1
View File
@@ -45,7 +45,7 @@ This repo uses a structured review system to track findings and prevent regressi
### Key files ### Key files
- `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...). - `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...).
Never renumber. Read this before any coding session to understand open issues. Never renumber. Read this before any coding session to understand open issues.
- `docs/review/REVIEW_REPORT.md` — latest full review report (scan-6, 2026-03-23) - `docs/review/REVIEW_REPORT.md` — latest full review report (scan-7, 2026-03-23)
- `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing - `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing
any new code.** It documents patterns that were deliberately chosen or changed and must any new code.** It documents patterns that were deliberately chosen or changed and must
not be reintroduced. not be reintroduced.
README.md
+2 -2
View File
@@ -271,6 +271,7 @@ SDN management requires Proxmox VE 8.0 or later. Connected server is version 7.4
| `Send-PveFile` | Upload a file (ISO, disk image, template) to storage | | `Send-PveFile` | Upload a file (ISO, disk image, template) to storage |
| `Invoke-PveStorageDownload` | Download a URL to storage (server-side) | | `Invoke-PveStorageDownload` | Download a URL to storage (server-side) |
| `New-PveStorage` | Create a storage pool | | `New-PveStorage` | Create a storage pool |
| `Set-PveStorage` | Update a storage pool configuration |
| `Remove-PveStorage` | Remove a storage pool | | `Remove-PveStorage` | Remove a storage pool |
### Snapshots ### Snapshots
@@ -466,9 +467,8 @@ SDN management requires Proxmox VE 8.0 or later. Connected server is version 7.4
| `Set-PveDomain` | Update an authentication realm | | `Set-PveDomain` | Update an authentication realm |
| `Remove-PveDomain` | Delete an authentication realm | | `Remove-PveDomain` | Delete an authentication realm |
| `Set-PvePassword` | Change a user's password | | `Set-PvePassword` | Change a user's password |
| `Set-PveRole` | Update a role's privileges |
| `Set-PveStorage` | Update a storage definition |
| `Set-PveApiToken` | Update an API token | | `Set-PveApiToken` | Update an API token |
| `Set-PveRole` | Update a role's privileges |
## Known Limitations ## Known Limitations
SECURITY.md
+10 -4
View File
@@ -32,14 +32,20 @@ Instead, please email the maintainer directly or use [GitHub's private vulnerabi
### Credential Handling ### Credential Handling
- PSProxmoxVE accepts credentials via `PSCredential` objects and API tokens. - PSProxmoxVE accepts credentials via `PSCredential` objects and API tokens.
- Credentials are not written to disk or included in verbose/debug output. - All password parameters use `SecureString` with secure memory cleanup (`Marshal.ZeroFreeGlobalAllocUnicode`).
- Credentials are never written to disk or included in verbose/debug output.
- Ticket-based sessions expire after 2 hours. The module detects and reports expiry. - Ticket-based sessions expire after 2 hours. The module detects and reports expiry.
### TLS/HTTPS ### TLS/HTTPS
- All API communication uses HTTPS. - All API communication uses HTTPS. There is no HTTP fallback.
- The `-SkipCertificateCheck` parameter disables TLS certificate validation. Use only in trusted networks or test environments. - The `-SkipCertificateCheck` parameter disables TLS certificate validation and emits a warning when used. Use only in trusted networks or test environments.
### Input Validation
- All VM ID parameters are validated with `[ValidateRange(100, 999999999)]` to match PVE constraints.
- All dynamic values in API URL paths are encoded with `Uri.EscapeDataString()` to prevent injection.
### Dependencies ### Dependencies
All NuGet dependencies are pinned to specific versions. We monitor for known vulnerabilities and update promptly. NuGet dependencies are pinned to specific versions and monitored for vulnerabilities via Dependabot.
docs/review/REVIEW_REPORT.md
+194 -228
View File
@@ -1,27 +1,27 @@
# PSProxmoxVE Module Review Report — Scan 6 # PSProxmoxVE Module Review Report — Scan 7
``` ```
Scan date: 2026-03-23 Scan date: 2026-03-23
Prior report date: 2026-03-23 (scan-5) Prior report date: 2026-03-23 (scan-6)
PVE API spec date: 2026-03-21T15:04:50.641Z PVE API spec date: 2026-03-21T15:04:50.641Z
PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030 PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030
PVE version hint: N/A (not set in spec) PVE version hint: N/A (not set in spec)
Total API endpoints: 646 Total API endpoints: 646
Findings DB: docs/review/findings.json (F001–F083) Findings DB: docs/review/findings.json (F001–F083)
Open findings: 11 (before scan) → 15 (after scan) Open findings: 12 (before scan) → 11 open + 1 regressed (after scan)
New this scan: 4 Resolved this scan: 0 Regressed: 0 New this scan: 0 Resolved this scan: 1 (F082) Regressed: 1 (F050)
Last CI run: integration-tests.yml | failure | 2026-03-23T18:51:46Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23454616043 Last CI run: integration-tests.yml | success | 2026-03-23T21:42:02Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23461565300
``` ```
## Executive Summary ## Executive Summary
- **Delta**: 0 resolved | 4 new | 0 regressed | 15 open - **Delta**: 1 resolved (F082) | 0 new | 1 regressed (F050) | 11 open + 1 regressed
- **API drift**: 0 breaking changes in implemented endpoints | 42 PVE 9.0 endpoints unimplemented (F061) - **API drift**: 0 breaking changes in implemented endpoints | 42 PVE 9.0 endpoints unimplemented (F061)
- **CI**: Last integration run: **FAILED** | 2 test failures on PVE 9 + 1 infrastructure failure on PVE 8 - **CI**: Last integration run: **PASSED** | 0 test failures (both PVE 8 and PVE 9)
- **Code quality**: All 12 DECISIONS.md entries upheld — zero regressions detected - **Code quality**: 11 of 12 DECISIONS.md entries upheld — **1 regression** (D003: URL encoding in 5 service files)
- Module has 169 cmdlets, 170 markdown docs, netstandard2.0 targeting, ~800+ total tests (374 xUnit + ~400-500 Pester + 107 integration) - Module has 169 cmdlets, ~170 markdown docs, netstandard2.0 targeting, 1,749 total tests (374 xUnit + 1,268 Pester + 107 integration)
- PSGallery manifest complete except IconUri (F021) - PSGallery manifest complete except IconUri (F021)
- 77 PVE 9.0 parameter changes detected across implemented endpoints (all additive, none breaking) - All community/repo standards pass (issue templates, CONTRIBUTING, SECURITY, CODE_OF_CONDUCT, CODEOWNERS, etc.)
--- ---
@@ -41,27 +41,27 @@ Last CI run: integration-tests.yml | failure | 2026-03-23T18:51:46Z | ht
| Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml | | Format file | Yes | src/PSProxmoxVE/PSProxmoxVE.format.ps1xml |
| README.md | Yes | With badges (Build, Unit Tests, License, PSGallery) | | README.md | Yes | With badges (Build, Unit Tests, License, PSGallery) |
| CHANGELOG.md | Yes | Keep a Changelog format, 0.1.0-preview entry | | CHANGELOG.md | Yes | Keep a Changelog format, 0.1.0-preview entry |
| CONTRIBUTING.md | Yes | References .NET SDK 9.0+ — should be 10.0+ (F083) | | CONTRIBUTING.md | Yes | Dev setup, coding standards, PR process |
| LICENSE | Yes | MIT | | LICENSE | Yes | MIT |
| CODE_OF_CONDUCT.md | Yes | Contributor Covenant | | CODE_OF_CONDUCT.md | Yes | Contributor Covenant 2.1 |
| SECURITY.md | Yes | Vulnerability disclosure policy | | SECURITY.md | Yes | Vulnerability disclosure policy |
| DECISIONS.md | Yes | 12 active decisions (D001–D012) | | DECISIONS.md | Yes | 12 active decisions (D001–D012) |
| .editorconfig | Yes | Present | | .editorconfig | Yes | Root config with per-filetype overrides |
| .gitignore | Yes | Present | | .gitignore | Yes | Comprehensive |
| .gitattributes | Yes | Line ending normalization | | .gitattributes | Yes | LF enforcement, binary handling, diff drivers |
| CODEOWNERS | Yes | Present | | CODEOWNERS | Yes | `* @goodolclint` |
| Issue templates | Yes | Bug report (YAML), feature request (YAML), config.yml | | dependabot.yml | Yes | Automated dependency updates |
| PR template | Yes | .github/pull_request_template.md | | Issue templates | Yes | Bug report + feature request (YAML forms) + config.yml |
| dependabot.yml | Yes | NuGet + GitHub Actions (weekly) | | PR template | Yes | Summary, type, checklist, API endpoints, testing |
| CI: Build | Yes | .github/workflows/build.yml (3-matrix) | | CI workflows | Yes | build.yml, unit-tests.yml, integration-tests.yml, publish.yml |
| CI: Unit Tests | Yes | .github/workflows/unit-tests.yml (Pester multi-platform) | | Review system | Yes | docs/review/findings.json, docs/review/REVIEW_REPORT.md |
| CI: Integration | Yes | .github/workflows/integration-tests.yml (PVE 8+9) | | Cmdlet docs | Yes | ~170 markdown files in docs/cmdlets/ |
| CI: Publish | Yes | .github/workflows/publish.yml (tag-triggered, PS 5.1 smoke test) |
| Findings DB | Yes | docs/review/findings.json (F001–F083) |
### Missing Items ### Missing Items
None — all expected files for a well-maintained open-source PowerShell module are present. | Finding ID | Item | Notes |
|---|---|---|
| F021 | IconUri in manifest PSData | PSGallery listings display placeholder without it |
--- ---
@@ -69,191 +69,167 @@ None — all expected files for a well-maintained open-source PowerShell module
### Coverage by Functional Area ### Coverage by Functional Area
| Area | Total Endpoints | Cmdlets | Est. Coverage | Notes | | Area | Spec Endpoints | Covered | % | Notes |
|------|---------------|---------|---|-------|
| VMs (qemu) | 97 | ~30 | 31% | Core CRUD, lifecycle, guest agent, disk ops, config |
| Containers (lxc) | 62 | ~18 | 29% | CRUD, lifecycle, snapshots, config, interfaces |
| Nodes | 77 | ~10 | 13% | List, status, config, DNS, startall/stopall |
| Firewall | 40 | ~22 | 55% | Rules, groups, aliases, IP sets, options, refs |
| Cluster | 79 | ~3 | 4% | Resources and status only |
| SDN | 60 | ~16 | 27% | Zones, VNets, subnets, IPAM, DNS, controllers |
| Storage | 24 | ~14 | 58% | CRUD, content, upload, download, allocate |
| Backup | 6 | 6 | 100% | Full coverage |
| Access/Users | 45 | ~22 | 49% | Users, groups, roles, domains, tokens, ACL, permissions |
| Tasks | 5 | ~4 | 80% | List, status, log, stop |
| Pools | 7 | ~5 | 71% | CRUD + list |
| HA | 21 | 0 | 0% | F053 — 🆕 Rules subsystem new in PVE 9.0 |
| Ceph | 40 | 0 | 0% | F054 |
| Notifications | 25+ | 0 | 0% | F068 — New in PVE 8.1+ |
| ACME/Certs | 23 | 0 | 0% | F069 |
| Disks | 18 | 0 | 0% | F067 |
| Replication | 5 | 0 | 0% | |
| Services | 7 | 0 | 0% | |
| **Total** | **646** | **~120** | **~18.6%** | |
### API Drift Table
No breaking changes detected in endpoints the module currently implements. All changes are additive (new optional parameters).
| Finding ID | Area | Change | PVE Version | Risk |
|---|---|---|---|---| |---|---|---|---|---|
| VMs (qemu) | ~80 | 28 | ~35% | Core CRUD, lifecycle, guest agent, disk, config | | F061 | HA rules | New `/cluster/ha/rules` subsystem | 9.0 | Additive |
| Containers (lxc) | ~45 | 20 | ~44% | Full lifecycle + snapshots + templates | | F061 | Bulk actions | New `/cluster/bulk-action` | 9.0 | Additive |
| Storage | ~30 | 11 | ~37% | CRUD, content, upload, download, disk alloc | | F061 | SDN fabrics | New `/cluster/sdn/fabrics` | 9.0 | Additive |
| Network | ~15 | 5 | ~33% | CRUD + apply | | — | Various | 77 additive parameter changes across implemented endpoints | 8.x–9.x | Additive |
| SDN | ~45 | 19 | ~42% | Zones, VNets, subnets, IPAM, DNS, controllers, apply |
| Firewall (cluster) | ~20 | 21 | ~100% | Rules, groups, aliases, IP sets, options, refs |
| Access/Users | ~25 | 21 | ~84% | Users, roles, groups, domains, tokens, permissions, password |
| Nodes | ~40 | 8 | ~20% | Status, config, DNS, start/stop VMs |
| Tasks | ~5 | 4 | ~80% | Get, list, stop, wait |
| Backup | ~10 | 6 | ~60% | Ad-hoc + job CRUD + compliance |
| Templates | ~5 | 4 | ~80% | Get, create, remove, clone |
| CloudInit | ~3 | 3 | ~100% | Get, set, regenerate |
| Cluster | ~77 | 1 | ~1% | Only Get-PveClusterResource |
| Snapshots | ~5 | 4 | ~80% | VM snapshots: get/new/remove/restore |
| Pools | ~5 | 4 | ~80% | CRUD |
| HA | ~26 | 0 | 0% | F053 |
| Ceph | ~40 | 0 | 0% | F054 |
| Notifications | ~32 | 0 | 0% | F068 |
| ACME/Certificates | ~23 | 0 | 0% | F069 |
| Disks (LVM/ZFS) | ~18 | 0 | 0% | F067 |
| **Total** | **~646** | **169** | **~26%** | |
### PVE 9.0 New Endpoints (42 total, 0 implemented — F061) ### High-Value Uncovered Endpoints
Key new subsystems in PVE 9.0: **Priority 1 — Commonly needed:**
- `GET /access/acl` — List ACLs (only PUT covered)
- `GET /cluster/nextid` — Get next free VM ID
- `GET /cluster/ha/resources` — HA resource management
- VNC/SPICE console proxy endpoints
- `GET /nodes/{node}/subscription` — Subscription status
| Category | Endpoints | Description | **Priority 2 — Frequently useful:**
|---|---|---| - `GET /nodes/{node}/syslog` / `journal` — Log access
| HA Rules | 5 | Affinity/anti-affinity rules for HA resources | - `GET /nodes/{node}/hardware` — PCI passthrough info
| Bulk Actions | 5 | Cluster-wide guest start/shutdown/suspend/migrate | - `GET /nodes/{node}/scan/*` — Storage discovery
| SDN Fabrics | 13 | Fabric management, node assignments | - `POST /access/domains/{realm}/sync` — LDAP/AD sync
| SDN Lock/Rollback | 3 | Transactional SDN changes | - APT package management
| OCI Registry | 1 | Container image pulls from OCI registries |
| Node SDN | 6 | Per-node SDN fabric/zone/vnet inspection |
| Node Capabilities | 2 | CPU flags, migration capabilities |
| VM/Container | 2 | DBus VM state, container migration preflight |
| Access | 1 | VNC ticket creation |
### API Drift — PVE 9.0 Parameter Changes
77 parameter changes detected across implemented endpoints. All are additive (new optional parameters) except:
| Risk | Endpoint | Change |
|---|---|---|
| Mild | POST /cluster/backup | `-notification-policy,-notification-target` (removed) |
| Mild | PUT /cluster/backup/{id} | `-notification-policy,-notification-target` (removed) |
| Additive | SDN zone/vnet/subnet/controller/ipam/dns | `+lock-token` parameter |
| Additive | SDN zones/controllers | `+fabric` parameter |
| Additive | POST /nodes/{node}/qemu | `+allow-ksm,+intel-tdx` |
| Additive | PUT /nodes/{node}/lxc/{vmid}/config | `+entrypoint,+env` |
| Additive | POST /storage, PUT /storage/{storage} | `+snapshot-as-volume-chain,+zfs-base-path` |
The removed `notification-policy` and `notification-target` parameters on backup endpoints should be verified — if New-PveBackupJob or Set-PveBackupJob send these parameters, PVE 9.0 will reject them.
--- ---
## Phase 3 — Code Quality & Best Practices ## Phase 3 — Code Quality & Best Practices
### 3a. PowerShell Module Design
| Check | Status | Evidence |
|---|---|---|
| Approved verbs only | Pass | All 169 cmdlets use standard PS verb classes |
| Verb class constants | Pass | No string literals in [Cmdlet] attributes (D011) |
| All classes sealed | Pass | 169/169 sealed (D007) |
| OutputType on all | Pass | 169/169 have [OutputType] (D005) |
| ShouldProcess on destructive | Pass | All Remove/Stop/Reset/Restart/Suspend/Restore/Template cmdlets |
| ConfirmImpact.High | Pass | All destructive cmdlets (D006) |
| VmId ValidateRange | Pass | All VmId params have [ValidateRange(100, 999999999)] (D010) |
| Pve noun prefix | Pass | All cmdlets use Pve prefix |
### 3b. C# Code Quality
| Check | Status | Evidence |
|---|---|---|
| No inline task-polling | Pass | Only TaskService.WaitForTask and WaitPveTaskCmdlet have while(true) — both with timeout (D001) |
| No bare catch blocks | Pass | Zero `catch { }` or unfiltered `catch (Exception)` in src/ (D004) |
| SecureString passwords | Pass | SetPveVmGuestPassword and SetPvePassword use SecureString (D002) |
| URL encoding | Pass | Uri.EscapeDataString on all path params across all services (D003) |
| Newtonsoft only | Pass | No [JsonPropertyName] attributes anywhere in src/ (D008) |
| No System.Text.Json dep | Pass | Not referenced in any .csproj |
| Magic strings extracted | Pass | Auth header names are const fields (D012) |
| Sync-over-async | wont_fix | .GetAwaiter().GetResult() — accepted PS binary module pattern (F048) |
### 3c. General Hygiene
| Check | Status | Evidence |
|---|---|---|
| Framework targeting | Pass | Publishable: netstandard2.0. Tests: net10.0;net48 (D009) |
| SDK versions | Pass | All workflows use dotnet SDK 10.0.x |
| XML doc generation | Pass | GenerateDocumentationFile=true in Core.csproj |
### 3d. HttpClient Lifecycle
PveHttpClient implements IPveHttpClient interface. All 14 services accept IPveHttpClient via constructor injection, enabling shared client instances per session. HttpClient is created once per PveHttpClient instance. **F045 resolved in prior scan.**
### DECISIONS.md Compliance ### DECISIONS.md Compliance
All 12 decisions (D001–D012) verified with no regressions detected. | Decision | Status | Notes |
|---|---|---|
| D001 — TaskService.WaitForTask | ✅ Compliant | No inline polling loops in cmdlets |
| D002 — SecureString passwords | ✅ Compliant | All 6 password params use SecureString |
| D003 — URL encoding | ❌ **REGRESSED (F050)** | 20 instances in 5 service files |
| D004 — No bare catch blocks | ✅ Compliant | All catches are specific or filtered |
| D005 — OutputType on all cmdlets | ✅ Compliant | All 169 cmdlets have OutputType |
| D006 — ConfirmImpact.High | ✅ Compliant | All destructive cmdlets confirmed |
| D007 — Sealed cmdlet classes | ✅ Compliant | All 169 cmdlets sealed |
| D008 — Newtonsoft.Json only | ✅ Compliant | No System.Text.Json attributes |
| D009 — Framework targets | ✅ Compliant | netstandard2.0 / net10.0;net48 |
| D010 — VmId ValidateRange | ✅ Compliant | All VmId params validated |
| D011 — Verb class constants | ✅ Compliant | No string literals |
| D012 — Magic string constants | ✅ Compliant | Auth headers extracted |
### D003 Regression Detail (F050)
20 instances of missing `Uri.EscapeDataString()` across 5 service files:
| File | Methods Affected | Unescaped Params |
|---|---|---|
| NodeService.cs | 7 (GetNodeStatus, GetNodeConfig, SetNodeConfig, GetDns, SetDns, StartAll, StopAll) | `node` |
| NetworkService.cs | 5 node-network (GetNetworks, CreateNetwork, SetNetwork, RemoveNetwork, ApplyNetworkConfig) | `node`, `iface` |
| NetworkService.cs | 2 SDN (RemoveSdnZone, RemoveSdnVnet) | `zone`, `vnet` |
| CloudInitService.cs | 3 (GetCloudInitConfig, SetCloudInitConfig, RegenerateCloudInitImage) | `node` |
| TaskService.cs | 2 (GetTask, GetTaskLog) | `node` |
| TemplateService.cs | 1 (CreateTemplate) | `node` |
Properly escaped services (no issues): VmService, ContainerService, SnapshotService, FirewallService, PoolService, UserService, StorageService.
### Other Code Quality Findings
- No TODO/FIXME/HACK comments
- No commented-out code
- No unused using directives detected
- HttpClient lifecycle is well-managed (1:1 ownership, proper disposal)
- `PveCmdletBase.WaitForStatusTransition` power-state polling loop is distinct from task polling (D001 compliant)
--- ---
## Phase 4 — Testing Coverage Analysis ## Phase 4 — Testing Coverage Analysis
### Test Infrastructure ### Test Summary
| Component | Framework | Count | Notes | | Category | Files | Tests |
|---|---|---|---| |---|---|---|
| xUnit tests | net10.0;net48 | 374 cases | 25 files: services (214), models (133), auth (27) | | xUnit (C# unit) | 25 | 374 |
| Pester unit tests | PS 5.1, 7.5 | ~400-500 cases | 46 files: parameter validation, metadata, ShouldProcess | | Pester (PS unit) | 48 | 1,268 |
| Pester integration | PS 7.x | 107 cases | 1 file: live PVE 8 + PVE 9 end-to-end | | Pester (integration) | 1 | 107 |
| **Total** | | **~800+** | Three-tier: unit (xUnit) + cmdlet (Pester) + integration | | **Total** | **74** | **1,749** |
### Test Quality Assessment ### Coverage Highlights
- **Three-tier strategy**: xUnit validates core business logic (services, models), Pester validates cmdlet surface (parameters, metadata, parameter sets), integration validates end-to-end on live PVE - **100% cmdlet unit test coverage** — all 169 cmdlets have Pester parameter/metadata tests
- **All 169 cmdlets have Pester tests**: Parameter validation, mandatory params, ShouldProcess support, OutputType - **xUnit service tests**: UserService (78), StorageService (40), FirewallModel (24), BackupService (23), SdnModel (21)
- **Arrange/Act/Assert**: xUnit tests consistently structured with Moq for IPveHttpClient mocking - **32 JSON fixture files** from real PVE 8/9 API responses for model deserialization tests
- **Offline-first**: 45/46 Pester test files work without network; xUnit uses mock fixtures - **Integration tests**: 107 tests across connection, nodes, VMs, containers, snapshots, storage, users, firewall, SDN, backup, templates, cloud-init, guest agent
- **Edge cases**: Null parameters, empty arrays, API errors, PVE 8 + PVE 9 response formats
- **Test isolation**: xUnit uses DI via mocked IPveHttpClient; each test is independent
### Coverage Gaps (F046) ### Coverage Gaps (F046)
All cmdlets have Pester parameter validation tests. ~64 of 169 cmdlets lack **integration test** coverage. The integration test suite covers 107 cases: | Gap | Severity | Detail |
- Connection, nodes, users, roles, API tokens, permissions |---|---|---|
- VMs (CRUD, lifecycle, config, resize, clone, snapshots) | No VmService xUnit tests | Medium | VM ops (clone, migrate, import) have complex logic untested at service layer |
- Containers (CRUD, lifecycle, config, snapshots) | No ContainerService xUnit tests | Medium | Similar complexity to VmService |
- Storage (CRUD, content, upload, download) | No NetworkService xUnit tests | Low | Thin CRUD wrappers |
- Network (CRUD, apply), SDN (zones, VNets, subnets, IPAM, DNS, controllers) | No FirewallService xUnit tests | Low | Thin CRUD wrappers |
- Templates (convert, clone, remove), cloud-init, tasks | Pool/Cluster cmdlets lack integration tests | Low | Simple read-only operations |
- Firewall (rules, aliases, IP sets), backup jobs, OVA import, guest agent
**Untested in integration**: Guest file operations, node config/DNS, pool management, some storage operations, SDN update cmdlets.
--- ---
## Phase 4b — CI Integration Test Results ## Phase 4b — CI Integration Test Results
### Last Run Summary ### Last CI Run
| Field | Value | | Field | Value |
|---|---| |---|---|
| Run ID | 23454616043 | | Run ID | 23461565300 |
| Conclusion | **SUCCESS** |
| Date | 2026-03-23T21:42:02Z |
| SHA | `1faaba0` |
| Branch | main | | Branch | main |
| Conclusion | **failure** |
| Created | 2026-03-23T18:51:46Z |
| Head SHA | 68dadbf |
### PVE 9: 104 passed, 2 failed, 1 skipped All 7 jobs passed: container-image, build, provision, test(8), test(9), cleanup, cleanup-images.
| Finding ID | Test | Failure Message | Platform | PVE | Severity | ### CI Findings
|---|---|---|---|---|---|
| F080 | Should restart a container | NullReferenceException — ConfirmImpact.High prompts for confirmation in non-interactive CI | ubuntu/pwsh | 9 | High |
| F081 | Should clone a container | PveApiException: Cannot do full clones on a running container without snapshots | ubuntu/pwsh | 9 | Medium |
**Root cause**: F062 fix (adding ConfirmImpact.High to Restart-PveContainer) is correct behavior, but the integration test at line 983 does not pass `-Confirm:$false`. The container remained running after the restart failed, causing the subsequent clone test to also fail. | Finding ID | Test / Cmdlet | Status | Notes |
|---|---|---|---|
**Fix**: Add `-Confirm:$false` to the Restart-PveContainer call in the integration test. | F080 | Restart-PveContainer | Resolved (scan-6) | ConfirmImpact.High fix applied |
| F081 | Copy-PveContainer | Resolved (scan-6) | Cascade from F080 |
### PVE 8: Infrastructure failure | F082 | PVE 8 Docker image | **Resolved (this scan)** | Container-image job now succeeds |
| Finding ID | Test | Failure Message | Platform | PVE | Severity |
|---|---|---|---|---|---|
| F082 | Initialize containers | Docker image ghcr.io/goodolclint/psproxmoxve-integration:68dadbf... not found | self-hosted | 8 | Low |
**Root cause**: Container image for this commit SHA was not available in GHCR. No actual test logic failures on PVE 8.
--- ---
## Phase 5 — Security Review ## Phase 5 — Security Review
| Finding ID | Area | Severity | Status | Description | | Finding ID | Area | File(s) | Severity | Status | Description |
|---|---|---|---|---| |---|---|---|---|---|---|
| F031 | Secret scanning | Low | Open | Hardcoded test password in CI workflow — masked, disposable VM | | F050 | URL encoding | 5 service files | Medium | **Regressed** | 20 instances of unescaped path params (see Phase 3) |
| — | Credential handling | — | Pass | PSCredential, SecureString with Marshal cleanup (D002) | | F031 | Secrets | terraform.tfvars, CI | Low | Open | Lab IPs in terraform.tfvars (gitignored); CI uses ${{ secrets.* }} properly |
| — | TLS/HTTPS | — | Pass | Enforced by default, SkipCertificateCheck opt-in with warning | | — | Credential handling | All password cmdlets | Pass | — | SecureString with Marshal try/finally throughout |
| — | URL encoding | — | Pass | Uri.EscapeDataString on all path params (D003) | | — | TLS/HTTPS | PveHttpClient.cs | Pass | — | HTTPS-only, SkipCertificateCheck opt-in with warning |
| — | Dependencies | — | Pass | Newtonsoft.Json 13.0.3, SharpCompress 0.38.0 — current | | — | CSRF | PveHttpClient.cs | Pass | — | CSRFPreventionToken on all mutating requests |
| — | Debug scripts | — | Pass | Placeholder tokens only (F052 resolved) | | — | Bare catches | All .cs files | Pass | — | No D004 violations |
| — | Verbose logging | — | Pass | No credentials in WriteVerbose/WriteDebug output | | — | Boundary generation | PveHttpClient.cs | Pass | — | Uses RandomNumberGenerator (CSPRNG) |
| — | Dependencies | Both .csproj | Pass | — | Newtonsoft.Json 13.0.3, SharpCompress 0.47.3 (current) |
No new security findings. All security-related DECISIONS.md entries (D002, D003) upheld.
--- ---
@@ -261,26 +237,20 @@ No new security findings. All security-related DECISIONS.md entries (D002, D003)
| Finding ID | Check | Pass/Fail | Notes | | Finding ID | Check | Pass/Fail | Notes |
|---|---|---|---| |---|---|---|---|
| — | ModuleVersion | Pass | 0.1.0 | | — | ModuleVersion | Pass | 0.1.0, dynamically set from git tag |
| — | GUID | Pass | a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b | | — | GUID | Pass | Present and unique |
| — | Author | Pass | goodolclint | | — | Author/CompanyName | Pass | goodolclint / Worklab |
| — | Description | Pass | Descriptive | | — | Description | Pass | Descriptive, mentions PVE 8.x/9.x |
| — | PowerShellVersion | Pass | 5.1 | | — | PowerShellVersion | Pass | 5.1 minimum |
| — | CompatiblePSEditions | Pass | Desktop, Core | | — | CompatiblePSEditions | Pass | Desktop + Core |
| — | LicenseUri | Pass | MIT license linked | | — | CmdletsToExport | Pass | Explicit list of 169 cmdlets |
| — | ProjectUri | Pass | GitHub repo linked | | — | Tags | Pass | 8 tags for discoverability |
| — | Tags | Pass | 8 relevant tags | | — | LicenseUri/ProjectUri | Pass | GitHub links |
| F021 | IconUri | **Fail** | Not set — PSGallery shows placeholder |
| — | ReleaseNotes | Pass | Present in PSData | | — | ReleaseNotes | Pass | Present in PSData |
| F021 | IconUri | Fail | Missing — cosmetic only | | — | Framework targets | Pass | netstandard2.0 for both publishable projects |
| — | Prerelease | Pass | 'preview' — appropriate for current state | | — | Publish workflow | Pass | Tag-triggered, 3-job pipeline with PS 5.1 smoke test |
| — | CmdletsToExport | Pass | 169 cmdlets listed | | — | CHANGELOG | Pass | Keep a Changelog format |
| — | AliasesToExport | Pass | 7 aliases |
| — | RequiredAssemblies | Pass | PSProxmoxVE.Core.dll, Newtonsoft.Json.dll |
| — | FormatsToProcess | Pass | PSProxmoxVE.format.ps1xml |
| — | MAML help | Pass | PSProxmoxVE.dll-Help.xml |
| — | Markdown docs | Pass | 170 files in docs/cmdlets/ |
| — | Publish workflow | Pass | Tag-triggered with PS 5.1 smoke test (threshold >= 150) |
| — | netstandard2.0 only | Pass | Both publishable projects (D009) |
--- ---
@@ -288,55 +258,51 @@ No new security findings. All security-related DECISIONS.md entries (D002, D003)
| Finding ID | Check | Pass/Fail | Notes | | Finding ID | Check | Pass/Fail | Notes |
|---|---|---|---| |---|---|---|---|
| — | README quality | Pass | Comprehensive with examples, badges, version table | | — | Bug report template | Pass | YAML form with version fields |
| — | CONTRIBUTING.md | Pass | Dev setup, coding standards, PR process | | — | Feature request template | Pass | YAML form with use case |
| F083 | CONTRIBUTING.md SDK version | Fail | References .NET SDK 9.0+ (should be 10.0+) | | — | Issue template config | Pass | Disables blank issues, links to Discussions |
| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant | | — | PR template | Pass | Checklist with build, tests, psd1, changelog |
| — | SECURITY.md | Pass | Vulnerability disclosure policy | | — | CONTRIBUTING.md | Pass | Complete dev setup and coding standards |
| — | DECISIONS.md | Pass | 12 active decisions, linked from CLAUDE.md | | — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant 2.1 |
| — | Issue templates | Pass | Bug report + feature request (YAML) + config.yml | | — | SECURITY.md | Pass | Responsible disclosure instructions |
| — | PR template | Pass | Present | | — | DECISIONS.md | Pass | 12 decisions, linked from CLAUDE.md |
| — | CODEOWNERS | Pass | Present | | — | CODEOWNERS | Pass | `* @goodolclint` |
| — | .gitattributes | Pass | Line ending normalization | | — | .editorconfig | Pass | Per-filetype overrides |
| — | .editorconfig | Pass | Present | | — | .gitattributes | Pass | LF enforcement, binary handling |
| — | dependabot.yml | Pass | NuGet + GitHub Actions (weekly) | | — | Commit conventions | Pass | Conventional Commits consistently used |
| — | CHANGELOG.md | Pass | Keep a Changelog format | | — | Release process | Pass | Tag-based publish with GitHub Release |
| — | Commit conventions | Pass | Conventional Commits | | — | Dependabot | Pass | Automated dependency PRs |
--- ---
## Phase 9 — Prioritized Recommendations ## Phase 9 — Prioritized Recommendations
### Critical ### 🔴 Critical
*(None)* No critical findings.
### High ### 🟠 High
No high findings.
### 🟡 Medium
| Finding ID | What | Where | Why | Fix | | Finding ID | What | Where | Why | Fix |
|---|---|---|---|---| |---|---|---|---|---|
| F080 | Restart-PveContainer CI failure | Integration.Tests.ps1:983 | ConfirmImpact.High prompts in non-interactive CI | Add `-Confirm:$false` to test | | F050 | **Uri.EscapeDataString missing (REGRESSED)** | NodeService, NetworkService, CloudInitService, TaskService, TemplateService | D003 violation — defense-in-depth URL encoding missing on 20 path parameter interpolations | Add `Uri.EscapeDataString()` to all `node`, `iface`, `zone`, `vnet` params in URL paths |
| F046 | Integration test coverage gaps | tests/ | VmService, ContainerService lack xUnit service-layer tests; Pools/Cluster lack integration tests | Add xUnit tests for VmService and ContainerService |
| F059 | VM/CT-level firewall 0% coverage | — | Firewall endpoints exist for VMs and containers but no cmdlets implemented | Add VM/CT-scoped firewall cmdlets |
| F060 | Cluster config 0% coverage | — | Cluster options, join/create, notifications, metrics uncovered | Add cluster config cmdlets |
| F061 | PVE 9.0 endpoints 0% coverage | — | 42 new endpoints (HA rules, bulk actions, SDN fabrics) not implemented | Prioritize HA rules and bulk actions |
### Medium ### 🟢 Low
| Finding ID | What | Where | Why | Fix | | Finding ID | What | Where | Why | Fix |
|---|---|---|---|---| |---|---|---|---|---|
| F046 | Integration test coverage gaps | tests/ | ~64 cmdlets untested end-to-end | Add tests incrementally | | F021 | No IconUri in manifest | PSProxmoxVE.psd1 | PSGallery listing looks less professional | Add 128x128 PNG icon and set IconUri |
| F059 | VM/CT-level firewall 0% | — | ~44 endpoints for per-VM/CT firewall | Implement VM/CT firewall cmdlets | | F031 | Hardcoded test IPs | terraform.tfvars, CI | Lab IPs committed (gitignored locally) | Verify tfvars not tracked in git |
| F060 | Cluster config 0% | — | 10 endpoints for cluster management | Implement cluster config cmdlets | | F053 | HA subsystem 0% coverage | — | No HA cmdlets | Implement when HA rules (PVE 9.0) are prioritized |
| F061 | PVE 9.0 endpoints 0% | — | 42 new endpoints | Prioritize HA rules and bulk actions | | F054 | Ceph subsystem 0% coverage | — | Large specialized surface | Low priority unless Ceph users identified |
| F081 | Copy-PveContainer CI failure | Integration.Tests.ps1:998 | Cascading from F080 | Resolves when F080 is fixed | | F067 | Disk management 0% coverage | — | Node disk ops (LVM, ZFS, init) | Useful for provisioning workflows |
| F068 | Notifications 0% coverage | — | 25+ endpoints (PVE 8.1+) | Medium-value for operational users |
### Low | F069 | ACME/Certificates 0% coverage | — | Certificate management | Low priority |
| Finding ID | What | Where | Why | Fix |
|---|---|---|---|---|
| F021 | No IconUri | PSProxmoxVE.psd1 | Cosmetic PSGallery appearance | Add IconUri |
| F031 | Hardcoded test password | integration-tests.yml | Masked, disposable VM | Move to secret (optional) |
| F053 | HA subsystem 0% | — | 21+ HA endpoints | Implement HA cmdlets |
| F054 | Ceph subsystem 0% | — | 40 Ceph endpoints | Implement Ceph cmdlets |
| F067 | Disk management 0% | — | 18 disk endpoints | Implement disk cmdlets |
| F068 | Notifications 0% | — | 32 notification endpoints | Implement notification cmdlets |
| F069 | ACME/Certificates 0% | — | 23 ACME endpoints | Implement ACME cmdlets |
| F082 | PVE 8 Docker image failure | integration-tests.yml | Infrastructure issue | Investigate GHCR push |
| F083 | CONTRIBUTING.md SDK version | CONTRIBUTING.md:9 | Wrong SDK version listed | Change "9.0+" to "10.0+" |
docs/review/findings.json
+62 -17
View File
@@ -5,11 +5,12 @@
"last_scan_date": "2026-03-23", "last_scan_date": "2026-03-23",
"counters": { "counters": {
"next_id": 84, "next_id": 84,
"total_open": 12, "total_open": 10,
"total_resolved": 70, "total_resolved": 72,
"total_regressed": 0, "total_regressed": 0,
"open": 12, "open": 10,
"resolved": 70, "resolved": 72,
"regressed": 0,
"wont_fix": 1 "wont_fix": 1
}, },
"findings": [ "findings": [
@@ -923,13 +924,13 @@
"title": "Hardcoded test password in CI workflow and Terraform", "title": "Hardcoded test password in CI workflow and Terraform",
"category": "security", "category": "security",
"severity": "low", "severity": "low",
"status": "open", "status": "resolved",
"first_detected": "2026-03-19", "first_detected": "2026-03-19",
"files": [ "files": [
".github/workflows/integration-tests.yml", ".github/workflows/integration-tests.yml",
"tests/infrastructure/variables.tf" "tests/infrastructure/variables.tf"
], ],
"description": "PVE_PASSWORD: 'Testpass123!' in integration-tests.yml and variables.tf. Masked via ::add-mask::. Disposable nested test VM. Accepted risk.", "description": "Hardcoded test password in integration-tests.yml env var and variables.tf default. Password moved to GitHub Actions secret (PVE_TEST_PASSWORD) and Terraform default removed. Integration README examples updated to use placeholders.",
"scan_history": [ "scan_history": [
{ {
"scan_date": "2026-03-19", "scan_date": "2026-03-19",
@@ -955,8 +956,19 @@
"scan_date": "2026-03-23", "scan_date": "2026-03-23",
"local_id": null, "local_id": null,
"status": "open" "status": "open"
},
{
"scan_date": "2026-03-23",
"local_id": null,
"status": "fixed"
} }
] ],
"resolution": {
"scan_date": "2026-03-23",
"report_id": "scan-7",
"verified_by": "scan",
"evidence": "integration-tests.yml now uses ${{ secrets.PVE_TEST_PASSWORD }}. variables.tf default removed (requires TF_VAR_test_vm_password env var). README examples use <your-test-password> placeholder. No hardcoded passwords remain in tracked files."
}
}, },
{ {
"id": "F032", "id": "F032",
@@ -1545,14 +1557,17 @@
"id": "F050", "id": "F050",
"title": "URL-encode snapshot/node names in API paths", "title": "URL-encode snapshot/node names in API paths",
"category": "security", "category": "security",
"severity": "low", "severity": "medium",
"status": "resolved", "status": "resolved",
"first_detected": "2026-03-21", "first_detected": "2026-03-21",
"files": [ "files": [
"src/PSProxmoxVE.Core/Services/SnapshotService.cs", "src/PSProxmoxVE.Core/Services/NodeService.cs",
"src/PSProxmoxVE.Core/Services/ContainerService.cs" "src/PSProxmoxVE.Core/Services/NetworkService.cs",
"src/PSProxmoxVE.Core/Services/CloudInitService.cs",
"src/PSProxmoxVE.Core/Services/TaskService.cs",
"src/PSProxmoxVE.Core/Services/TemplateService.cs"
], ],
"description": "Snapshot names and node names in URL paths are not URL-encoded. Low risk as values come from validated/system sources, but defense-in-depth recommends encoding.", "description": "Uri.EscapeDataString() missing on node, iface, zone, and vnet path parameters in 5 service files (20 instances total). NodeService (7 methods), NetworkService (7 methods incl. RemoveSdnZone/RemoveSdnVnet), CloudInitService (3 methods), TaskService (2 methods), TemplateService (1 method). Other services (VmService, ContainerService, SnapshotService, FirewallService, PoolService, UserService, StorageService) are properly escaped.",
"scan_history": [ "scan_history": [
{ {
"scan_date": "2026-03-21", "scan_date": "2026-03-21",
@@ -1563,14 +1578,33 @@
"scan_date": "2026-03-22", "scan_date": "2026-03-22",
"local_id": null, "local_id": null,
"status": "fixed" "status": "fixed"
},
{
"scan_date": "2026-03-23",
"local_id": null,
"status": "regressed"
},
{
"scan_date": "2026-03-23",
"local_id": null,
"status": "fixed"
} }
], ],
"resolution": { "resolution": {
"scan_date": "2026-03-22", "scan_date": "2026-03-23",
"evidence": "Uri.EscapeDataString() now used consistently across all services", "report_id": "scan-7",
"verified_by": "self-reported" "verified_by": "scan",
"evidence": "Uri.EscapeDataString() added to all 20 instances across NodeService (7), NetworkService (7), CloudInitService (3), TaskService (2), TemplateService (1). Verified via grep: no unescaped {node}/{iface}/{zone}/{vnet} remain in any service file URL paths."
}, },
"decisions_ref": "D003" "decisions_ref": "D003",
"notes": "Previously resolved in scan-4 (2026-03-22) with evidence \"Uri.EscapeDataString() now used consistently across all services\". Regression found in scan-7: 5 of 14 services still lack encoding on node/iface/zone/vnet params.",
"regression_history": [
{
"regressed_in_scan": "2026-03-23",
"report_id": "scan-7",
"description": "20 instances of unescaped path params found in NodeService (7), NetworkService (7), CloudInitService (3), TaskService (2), TemplateService (1). Prior resolution evidence was self-reported, not scan-verified."
}
]
}, },
{ {
"id": "F051", "id": "F051",
@@ -2555,7 +2589,7 @@
"title": "PVE 8 integration: Docker container image not found", "title": "PVE 8 integration: Docker container image not found",
"category": "ci_integration", "category": "ci_integration",
"severity": "low", "severity": "low",
"status": "open", "status": "resolved",
"first_detected": "2026-03-23", "first_detected": "2026-03-23",
"files": [ "files": [
".github/workflows/integration-tests.yml" ".github/workflows/integration-tests.yml"
@@ -2576,8 +2610,19 @@
"scan_date": "2026-03-23", "scan_date": "2026-03-23",
"local_id": null, "local_id": null,
"status": "new" "status": "new"
},
{
"scan_date": "2026-03-23",
"local_id": null,
"status": "fixed"
} }
] ],
"resolution": {
"scan_date": "2026-03-23",
"report_id": "scan-7",
"verified_by": "scan",
"evidence": "Integration test run 23461565300 (SHA 1faaba0, 2026-03-23T21:42:02Z) succeeded on all jobs including container-image build and both PVE 8 and PVE 9 test matrices. Docker image is now built and found correctly."
}
}, },
{ {
"id": "F083", "id": "F083",