diff --git a/CHANGELOG.md b/CHANGELOG.md index f6a1247..e198dc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 - 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 -- `Remove-PveRole` now has `ConfirmImpact.High` for safety -- URL-encode snapshot names in API paths for defense-in-depth -- Extract auth header magic strings to named constants in PveHttpClient +- `ConfirmImpact.High` added to all destructive cmdlets (Stop, Reset, Restart, Suspend, Remove, Restore, New-PveTemplate) +- Hardcoded test password moved from CI workflow to GitHub Actions secret +- Terraform variable default password removed (requires env var) ## [0.1.0-preview] - 2026-03-19 diff --git a/CLAUDE.md b/CLAUDE.md index 35ef393..09558e8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,7 @@ This repo uses a structured review system to track findings and prevent regressi ### Key files - `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...). 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 any new code.** It documents patterns that were deliberately chosen or changed and must not be reintroduced. diff --git a/README.md b/README.md index 76aff96..200ca0f 100644 --- a/README.md +++ b/README.md @@ -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 | | `Invoke-PveStorageDownload` | Download a URL to storage (server-side) | | `New-PveStorage` | Create a storage pool | +| `Set-PveStorage` | Update a storage pool configuration | | `Remove-PveStorage` | Remove a storage pool | ### 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 | | `Remove-PveDomain` | Delete an authentication realm | | `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-PveRole` | Update a role's privileges | ## Known Limitations diff --git a/SECURITY.md b/SECURITY.md index 80e8eab..e9d301f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -32,14 +32,20 @@ Instead, please email the maintainer directly or use [GitHub's private vulnerabi ### Credential Handling - 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. ### TLS/HTTPS -- All API communication uses HTTPS. -- The `-SkipCertificateCheck` parameter disables TLS certificate validation. Use only in trusted networks or test environments. +- All API communication uses HTTPS. There is no HTTP fallback. +- 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 -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. diff --git a/docs/review/REVIEW_REPORT.md b/docs/review/REVIEW_REPORT.md index 1074c94..be3beb3 100644 --- a/docs/review/REVIEW_REPORT.md +++ b/docs/review/REVIEW_REPORT.md @@ -1,27 +1,27 @@ -# PSProxmoxVE Module Review Report — Scan 6 +# PSProxmoxVE Module Review Report — Scan 7 ``` 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 SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030 PVE version hint: N/A (not set in spec) Total API endpoints: 646 Findings DB: docs/review/findings.json (F001–F083) -Open findings: 11 (before scan) → 15 (after scan) -New this scan: 4 Resolved this scan: 0 Regressed: 0 -Last CI run: integration-tests.yml | failure | 2026-03-23T18:51:46Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23454616043 +Open findings: 12 (before scan) → 11 open + 1 regressed (after scan) +New this scan: 0 Resolved this scan: 1 (F082) Regressed: 1 (F050) +Last CI run: integration-tests.yml | success | 2026-03-23T21:42:02Z | https://github.com/goodolclint/PSProxmoxVE/actions/runs/23461565300 ``` ## 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) -- **CI**: Last integration run: **FAILED** | 2 test failures on PVE 9 + 1 infrastructure failure on PVE 8 -- **Code quality**: All 12 DECISIONS.md entries upheld — zero regressions detected -- Module has 169 cmdlets, 170 markdown docs, netstandard2.0 targeting, ~800+ total tests (374 xUnit + ~400-500 Pester + 107 integration) +- **CI**: Last integration run: **PASSED** | 0 test failures (both PVE 8 and PVE 9) +- **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, 1,749 total tests (374 xUnit + 1,268 Pester + 107 integration) - 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 | | README.md | Yes | With badges (Build, Unit Tests, License, PSGallery) | | 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 | -| CODE_OF_CONDUCT.md | Yes | Contributor Covenant | +| CODE_OF_CONDUCT.md | Yes | Contributor Covenant 2.1 | | SECURITY.md | Yes | Vulnerability disclosure policy | | DECISIONS.md | Yes | 12 active decisions (D001–D012) | -| .editorconfig | Yes | Present | -| .gitignore | Yes | Present | -| .gitattributes | Yes | Line ending normalization | -| CODEOWNERS | Yes | Present | -| Issue templates | Yes | Bug report (YAML), feature request (YAML), config.yml | -| PR template | Yes | .github/pull_request_template.md | -| dependabot.yml | Yes | NuGet + GitHub Actions (weekly) | -| CI: Build | Yes | .github/workflows/build.yml (3-matrix) | -| CI: Unit Tests | Yes | .github/workflows/unit-tests.yml (Pester multi-platform) | -| CI: Integration | Yes | .github/workflows/integration-tests.yml (PVE 8+9) | -| CI: Publish | Yes | .github/workflows/publish.yml (tag-triggered, PS 5.1 smoke test) | -| Findings DB | Yes | docs/review/findings.json (F001–F083) | +| .editorconfig | Yes | Root config with per-filetype overrides | +| .gitignore | Yes | Comprehensive | +| .gitattributes | Yes | LF enforcement, binary handling, diff drivers | +| CODEOWNERS | Yes | `* @goodolclint` | +| dependabot.yml | Yes | Automated dependency updates | +| Issue templates | Yes | Bug report + feature request (YAML forms) + config.yml | +| PR template | Yes | Summary, type, checklist, API endpoints, testing | +| CI workflows | Yes | build.yml, unit-tests.yml, integration-tests.yml, publish.yml | +| Review system | Yes | docs/review/findings.json, docs/review/REVIEW_REPORT.md | +| Cmdlet docs | Yes | ~170 markdown files in docs/cmdlets/ | ### 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 -| 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 | -| Containers (lxc) | ~45 | 20 | ~44% | Full lifecycle + snapshots + templates | -| Storage | ~30 | 11 | ~37% | CRUD, content, upload, download, disk alloc | -| Network | ~15 | 5 | ~33% | CRUD + apply | -| 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%** | | +| F061 | HA rules | New `/cluster/ha/rules` subsystem | 9.0 | Additive | +| F061 | Bulk actions | New `/cluster/bulk-action` | 9.0 | Additive | +| F061 | SDN fabrics | New `/cluster/sdn/fabrics` | 9.0 | Additive | +| — | Various | 77 additive parameter changes across implemented endpoints | 8.x–9.x | Additive | -### 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 | -|---|---|---| -| HA Rules | 5 | Affinity/anti-affinity rules for HA resources | -| Bulk Actions | 5 | Cluster-wide guest start/shutdown/suspend/migrate | -| SDN Fabrics | 13 | Fabric management, node assignments | -| SDN Lock/Rollback | 3 | Transactional SDN changes | -| 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. +**Priority 2 — Frequently useful:** +- `GET /nodes/{node}/syslog` / `journal` — Log access +- `GET /nodes/{node}/hardware` — PCI passthrough info +- `GET /nodes/{node}/scan/*` — Storage discovery +- `POST /access/domains/{realm}/sync` — LDAP/AD sync +- APT package management --- ## 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 -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 -### Test Infrastructure +### Test Summary -| Component | Framework | Count | Notes | -|---|---|---|---| -| xUnit tests | net10.0;net48 | 374 cases | 25 files: services (214), models (133), auth (27) | -| Pester unit tests | PS 5.1, 7.5 | ~400-500 cases | 46 files: parameter validation, metadata, ShouldProcess | -| Pester integration | PS 7.x | 107 cases | 1 file: live PVE 8 + PVE 9 end-to-end | -| **Total** | | **~800+** | Three-tier: unit (xUnit) + cmdlet (Pester) + integration | +| Category | Files | Tests | +|---|---|---| +| xUnit (C# unit) | 25 | 374 | +| Pester (PS unit) | 48 | 1,268 | +| Pester (integration) | 1 | 107 | +| **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 -- **All 169 cmdlets have Pester tests**: Parameter validation, mandatory params, ShouldProcess support, OutputType -- **Arrange/Act/Assert**: xUnit tests consistently structured with Moq for IPveHttpClient mocking -- **Offline-first**: 45/46 Pester test files work without network; xUnit uses mock fixtures -- **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 +- **100% cmdlet unit test coverage** — all 169 cmdlets have Pester parameter/metadata tests +- **xUnit service tests**: UserService (78), StorageService (40), FirewallModel (24), BackupService (23), SdnModel (21) +- **32 JSON fixture files** from real PVE 8/9 API responses for model deserialization tests +- **Integration tests**: 107 tests across connection, nodes, VMs, containers, snapshots, storage, users, firewall, SDN, backup, templates, cloud-init, guest agent ### 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: -- Connection, nodes, users, roles, API tokens, permissions -- VMs (CRUD, lifecycle, config, resize, clone, snapshots) -- Containers (CRUD, lifecycle, config, snapshots) -- Storage (CRUD, content, upload, download) -- Network (CRUD, apply), SDN (zones, VNets, subnets, IPAM, DNS, controllers) -- Templates (convert, clone, remove), cloud-init, tasks -- 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. +| Gap | Severity | Detail | +|---|---|---| +| No VmService xUnit tests | Medium | VM ops (clone, migrate, import) have complex logic untested at service layer | +| No ContainerService xUnit tests | Medium | Similar complexity to VmService | +| No NetworkService xUnit tests | Low | Thin CRUD wrappers | +| No FirewallService xUnit tests | Low | Thin CRUD wrappers | +| Pool/Cluster cmdlets lack integration tests | Low | Simple read-only operations | --- ## Phase 4b — CI Integration Test Results -### Last Run Summary +### Last CI Run | Field | Value | |---|---| -| Run ID | 23454616043 | +| Run ID | 23461565300 | +| Conclusion | **SUCCESS** | +| Date | 2026-03-23T21:42:02Z | +| SHA | `1faaba0` | | 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 | -|---|---|---|---|---|---| -| 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 | +### CI Findings -**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. - -**Fix**: Add `-Confirm:$false` to the Restart-PveContainer call in the integration test. - -### PVE 8: Infrastructure failure - -| 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. +| Finding ID | Test / Cmdlet | Status | Notes | +|---|---|---|---| +| F080 | Restart-PveContainer | Resolved (scan-6) | ConfirmImpact.High fix applied | +| F081 | Copy-PveContainer | Resolved (scan-6) | Cascade from F080 | +| F082 | PVE 8 Docker image | **Resolved (this scan)** | Container-image job now succeeds | --- ## Phase 5 — Security Review -| Finding ID | Area | Severity | Status | Description | -|---|---|---|---|---| -| F031 | Secret scanning | Low | Open | Hardcoded test password in CI workflow — masked, disposable VM | -| — | Credential handling | — | Pass | PSCredential, SecureString with Marshal cleanup (D002) | -| — | TLS/HTTPS | — | Pass | Enforced by default, SkipCertificateCheck opt-in with warning | -| — | URL encoding | — | Pass | Uri.EscapeDataString on all path params (D003) | -| — | Dependencies | — | Pass | Newtonsoft.Json 13.0.3, SharpCompress 0.38.0 — current | -| — | Debug scripts | — | Pass | Placeholder tokens only (F052 resolved) | -| — | Verbose logging | — | Pass | No credentials in WriteVerbose/WriteDebug output | - -No new security findings. All security-related DECISIONS.md entries (D002, D003) upheld. +| Finding ID | Area | File(s) | Severity | Status | Description | +|---|---|---|---|---|---| +| F050 | URL encoding | 5 service files | Medium | **Regressed** | 20 instances of unescaped path params (see Phase 3) | +| F031 | Secrets | terraform.tfvars, CI | Low | Open | Lab IPs in terraform.tfvars (gitignored); CI uses ${{ secrets.* }} properly | +| — | Credential handling | All password cmdlets | Pass | — | SecureString with Marshal try/finally throughout | +| — | TLS/HTTPS | PveHttpClient.cs | Pass | — | HTTPS-only, SkipCertificateCheck opt-in with warning | +| — | CSRF | PveHttpClient.cs | Pass | — | CSRFPreventionToken on all mutating requests | +| — | Bare catches | All .cs files | Pass | — | No D004 violations | +| — | Boundary generation | PveHttpClient.cs | Pass | — | Uses RandomNumberGenerator (CSPRNG) | +| — | Dependencies | Both .csproj | Pass | — | Newtonsoft.Json 13.0.3, SharpCompress 0.47.3 (current) | --- @@ -261,26 +237,20 @@ No new security findings. All security-related DECISIONS.md entries (D002, D003) | Finding ID | Check | Pass/Fail | Notes | |---|---|---|---| -| — | ModuleVersion | Pass | 0.1.0 | -| — | GUID | Pass | a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b | -| — | Author | Pass | goodolclint | -| — | Description | Pass | Descriptive | -| — | PowerShellVersion | Pass | 5.1 | -| — | CompatiblePSEditions | Pass | Desktop, Core | -| — | LicenseUri | Pass | MIT license linked | -| — | ProjectUri | Pass | GitHub repo linked | -| — | Tags | Pass | 8 relevant tags | +| — | ModuleVersion | Pass | 0.1.0, dynamically set from git tag | +| — | GUID | Pass | Present and unique | +| — | Author/CompanyName | Pass | goodolclint / Worklab | +| — | Description | Pass | Descriptive, mentions PVE 8.x/9.x | +| — | PowerShellVersion | Pass | 5.1 minimum | +| — | CompatiblePSEditions | Pass | Desktop + Core | +| — | CmdletsToExport | Pass | Explicit list of 169 cmdlets | +| — | Tags | Pass | 8 tags for discoverability | +| — | LicenseUri/ProjectUri | Pass | GitHub links | +| F021 | IconUri | **Fail** | Not set — PSGallery shows placeholder | | — | ReleaseNotes | Pass | Present in PSData | -| F021 | IconUri | Fail | Missing — cosmetic only | -| — | Prerelease | Pass | 'preview' — appropriate for current state | -| — | CmdletsToExport | Pass | 169 cmdlets listed | -| — | 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) | +| — | Framework targets | Pass | netstandard2.0 for both publishable projects | +| — | Publish workflow | Pass | Tag-triggered, 3-job pipeline with PS 5.1 smoke test | +| — | CHANGELOG | Pass | Keep a Changelog format | --- @@ -288,55 +258,51 @@ No new security findings. All security-related DECISIONS.md entries (D002, D003) | Finding ID | Check | Pass/Fail | Notes | |---|---|---|---| -| — | README quality | Pass | Comprehensive with examples, badges, version table | -| — | CONTRIBUTING.md | Pass | Dev setup, coding standards, PR process | -| F083 | CONTRIBUTING.md SDK version | Fail | References .NET SDK 9.0+ (should be 10.0+) | -| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant | -| — | SECURITY.md | Pass | Vulnerability disclosure policy | -| — | DECISIONS.md | Pass | 12 active decisions, linked from CLAUDE.md | -| — | Issue templates | Pass | Bug report + feature request (YAML) + config.yml | -| — | PR template | Pass | Present | -| — | CODEOWNERS | Pass | Present | -| — | .gitattributes | Pass | Line ending normalization | -| — | .editorconfig | Pass | Present | -| — | dependabot.yml | Pass | NuGet + GitHub Actions (weekly) | -| — | CHANGELOG.md | Pass | Keep a Changelog format | -| — | Commit conventions | Pass | Conventional Commits | +| — | Bug report template | Pass | YAML form with version fields | +| — | Feature request template | Pass | YAML form with use case | +| — | Issue template config | Pass | Disables blank issues, links to Discussions | +| — | PR template | Pass | Checklist with build, tests, psd1, changelog | +| — | CONTRIBUTING.md | Pass | Complete dev setup and coding standards | +| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant 2.1 | +| — | SECURITY.md | Pass | Responsible disclosure instructions | +| — | DECISIONS.md | Pass | 12 decisions, linked from CLAUDE.md | +| — | CODEOWNERS | Pass | `* @goodolclint` | +| — | .editorconfig | Pass | Per-filetype overrides | +| — | .gitattributes | Pass | LF enforcement, binary handling | +| — | Commit conventions | Pass | Conventional Commits consistently used | +| — | Release process | Pass | Tag-based publish with GitHub Release | +| — | Dependabot | Pass | Automated dependency PRs | --- ## Phase 9 — Prioritized Recommendations -### Critical +### 🔴 Critical -*(None)* +No critical findings. -### High +### 🟠 High + +No high findings. + +### 🟡 Medium | 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 | |---|---|---|---|---| -| F046 | Integration test coverage gaps | tests/ | ~64 cmdlets untested end-to-end | Add tests incrementally | -| F059 | VM/CT-level firewall 0% | — | ~44 endpoints for per-VM/CT firewall | Implement VM/CT firewall cmdlets | -| F060 | Cluster config 0% | — | 10 endpoints for cluster management | Implement cluster config cmdlets | -| F061 | PVE 9.0 endpoints 0% | — | 42 new endpoints | Prioritize HA rules and bulk actions | -| F081 | Copy-PveContainer CI failure | Integration.Tests.ps1:998 | Cascading from F080 | Resolves when F080 is fixed | - -### Low - -| 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+" | +| F021 | No IconUri in manifest | PSProxmoxVE.psd1 | PSGallery listing looks less professional | Add 128x128 PNG icon and set IconUri | +| F031 | Hardcoded test IPs | terraform.tfvars, CI | Lab IPs committed (gitignored locally) | Verify tfvars not tracked in git | +| F053 | HA subsystem 0% coverage | — | No HA cmdlets | Implement when HA rules (PVE 9.0) are prioritized | +| F054 | Ceph subsystem 0% coverage | — | Large specialized surface | Low priority unless Ceph users identified | +| 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 | +| F069 | ACME/Certificates 0% coverage | — | Certificate management | Low priority | diff --git a/docs/review/findings.json b/docs/review/findings.json index 9fe67d5..75c1cd5 100644 --- a/docs/review/findings.json +++ b/docs/review/findings.json @@ -5,11 +5,12 @@ "last_scan_date": "2026-03-23", "counters": { "next_id": 84, - "total_open": 12, - "total_resolved": 70, + "total_open": 10, + "total_resolved": 72, "total_regressed": 0, - "open": 12, - "resolved": 70, + "open": 10, + "resolved": 72, + "regressed": 0, "wont_fix": 1 }, "findings": [ @@ -923,13 +924,13 @@ "title": "Hardcoded test password in CI workflow and Terraform", "category": "security", "severity": "low", - "status": "open", + "status": "resolved", "first_detected": "2026-03-19", "files": [ ".github/workflows/integration-tests.yml", "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_date": "2026-03-19", @@ -955,8 +956,19 @@ "scan_date": "2026-03-23", "local_id": null, "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 placeholder. No hardcoded passwords remain in tracked files." + } }, { "id": "F032", @@ -1545,14 +1557,17 @@ "id": "F050", "title": "URL-encode snapshot/node names in API paths", "category": "security", - "severity": "low", + "severity": "medium", "status": "resolved", "first_detected": "2026-03-21", "files": [ - "src/PSProxmoxVE.Core/Services/SnapshotService.cs", - "src/PSProxmoxVE.Core/Services/ContainerService.cs" + "src/PSProxmoxVE.Core/Services/NodeService.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_date": "2026-03-21", @@ -1563,14 +1578,33 @@ "scan_date": "2026-03-22", "local_id": null, "status": "fixed" + }, + { + "scan_date": "2026-03-23", + "local_id": null, + "status": "regressed" + }, + { + "scan_date": "2026-03-23", + "local_id": null, + "status": "fixed" } ], "resolution": { - "scan_date": "2026-03-22", - "evidence": "Uri.EscapeDataString() now used consistently across all services", - "verified_by": "self-reported" + "scan_date": "2026-03-23", + "report_id": "scan-7", + "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", @@ -2555,7 +2589,7 @@ "title": "PVE 8 integration: Docker container image not found", "category": "ci_integration", "severity": "low", - "status": "open", + "status": "resolved", "first_detected": "2026-03-23", "files": [ ".github/workflows/integration-tests.yml" @@ -2576,8 +2610,19 @@ "scan_date": "2026-03-23", "local_id": null, "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",