From ecd79c2219b8fdb27c8e82b01a894ffda221e0bc Mon Sep 17 00:00:00 2001 From: Simone Date: Sat, 9 May 2026 00:30:19 +0200 Subject: [PATCH] docs: add FIX-TODO with full project review findings Review post e2e-009: P0 bugs (shallow clone/checkout, artifact size false-fail, upload-artifact@v3 deprecated), P0 security (hardcoded password, net user argv, no installer SHA256), P1 stale Host-Only docs, P2 operational gaps (orphan cleanup, PSScriptAnalyzer, log recursion). Co-Authored-By: Claude Sonnet 4.6 --- FIX-TODO.md | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 FIX-TODO.md diff --git a/FIX-TODO.md b/FIX-TODO.md new file mode 100644 index 0000000..fb78686 --- /dev/null +++ b/FIX-TODO.md @@ -0,0 +1,110 @@ +# FIX-TODO — Local CI/CD System + + + +Lista di fix prioritizzati emersi dalla review. Ordine: P0 (bug/security) → P1 (doc obsolete che possono fuorviare) → P2 (cleanup/operativo). + +--- + +## P0 — Bug funzionali + +- [ ] **Shallow clone vs checkout commit specifico** — `scripts/Invoke-CIJob.ps1:198` + Phase 1 fa `git clone --depth 1 --branch $Branch` e poi `git checkout $Commit` (riga 209). + Se `$Commit` ≠ HEAD del branch (es. commit più vecchio, push in retag) il checkout fallisce: il commit non è nella shallow history. + `gitea/workflows/build-nsis.yml` passa `${{ github.sha }}` esplicito → bug latente. + **Fix**: o `git fetch origin $Commit && git checkout $Commit` dopo clone shallow, + oppure se `$Commit` è valorizzato saltare `--depth 1` e fare clone completo + checkout. + Test: rerun di un workflow su un commit che non è più HEAD del branch. + +- [ ] **Falso negativo size artifact** — `scripts/Get-BuildArtifacts.ps1:112-115` + `$sizeKB = [math]::Round((Get-Item $hostDestPath).Length / 1KB, 1); if ($sizeKB -eq 0) { throw }` + File con dimensione <50 byte arrotondano a 0 → throw spurio. + **Fix**: validare su `.Length -eq 0` (raw byte), poi calcolare `$sizeKB` solo per il messaggio. + +- [ ] **`actions/upload-artifact@v3` deprecato** — `gitea/workflows/build-nsis.yml:42` + `workflow-example.yml` usa già `@v4`. Allineare il workflow live a v4 (sintassi compatibile in questo caso). + +## P0 — Sicurezza + +- [ ] **Password CI hardcoded committata** — `CIBuild!ChangeMe2026` in: + `Setup-Host.ps1:85`, `template/Prepare-TemplateSetup.ps1:72`, + `template/Setup-TemplateVM.ps1:55`, `README.md:117`, `TODO.md:86`, + e anche stampata a console (`Setup-Host.ps1:177`, `Setup-TemplateVM.ps1:657`). + **Fix**: + - rimuovere il default; rendere `-BuildPassword` obbligatorio (o `Read-Host -AsSecureString` se assente) + - mai stamparla nei messaggi di output / esempi nel README + - documentare in BEST-PRACTICES che la password è scelta dall'operatore, non fornita dal repo + +- [ ] **Password in argv (`net user`)** — `template/Setup-TemplateVM.ps1:221` + `& net user $BuildUsername $BuildPassword /add /y` — visibile in process listing/audit log. + **Fix**: tornare a `New-LocalUser` con `ConvertTo-SecureString` (la nota nel commento dice c'era un bug WinRM SecureString — verificare se ancora attuale su Server 2025; in alternativa pipe della password via `stdin` di `net user`). + +- [ ] **Nessuna verifica integrità degli installer scaricati** — `template/Setup-TemplateVM.ps1` + `dotnet-install.ps1` (riga 338), Python (372), `vs_BuildTools.exe` (412) scaricati su HTTPS senza SHA256. + **Fix**: aggiungere `Get-FileHash` + confronto contro hash atteso parametrizzato. Per VS BuildTools (URL stabile `vs_buildtools.exe` ma binario aggiornato) tenere la verifica MZ-header già presente come fallback minimo, ma loggare hash effettivo per audit. + +## P1 — Doc obsolete (residui dell'epoca Host-Only) + +- [ ] **`Setup-TemplateVM.ps1` blocco "NETWORK REQUIREMENT" e istruzioni finali** + Righe 22-32 e 642-651 dicono di passare a `VMnet11 (Host-Only)` prima dello snapshot. + Sistema oggi resta su VMnet8 NAT. Riscrivere blocco + istruzioni finali per riflettere NAT permanente. + +- [ ] **`Setup-TemplateVM.ps1:205-206`** — commento "VM lives on a Host-Only network (VMnet11)" — falso. Sostituire con riferimento a VMnet8 NAT (lab isolato). + +- [ ] **`scripts/Invoke-CIJob.ps1:193`** — commento "build VM has no internet/LAN access (Host-Only network)". Falso. Aggiornare a "build VM su VMnet8 NAT — internet OK; sorgente comunque iniettato via WinRM zip per isolamento dal token gitea". + +- [ ] **`docs/BEST-PRACTICES.md` Step 8 "Network Isolation Verification"** (riga 236-256) + Test pensato per Host-Only: assume VM senza internet. Contraddice il setup. Rimuovere lo step o riscriverlo come "VM raggiunge solo NAT gateway + LAN gitea, non altre subnet host". + +- [ ] **`docs/ARCHITECTURE.md:119`** — "Git (per repo clone inside VM)" — git NON installato in template. Rimuovere finché `-UseGitClone` non implementato (vedi TODO.md sezione "In-VM Git Clone"). + +- [ ] **`docs/ARCHITECTURE.md:194`** — riga "Source code is transferred via zip ... no git clone inside VM" è corretta ma stride col commento successivo su isolamento internet. Riformulare per dichiarare esplicitamente: VM ha NAT internet per pip/nuget; sorgente comunque via zip per evitare di iniettare PAT git. + +## P1 — Bug minori / dead code + +- [ ] **`scripts/Invoke-RemoteBuild.ps1:96-107`** — primo `New-Item` su `$workDir` viene creato e rimosso subito dopo. Sostituire con singolo blocco: `if (Test-Path) { Remove-Item -Recurse -Force }; New-Item ...`. + +- [ ] **`scripts/Wait-VMReady.ps1:119`** — `$elapsed = ($deadline - (Get-Date)).Negate().TotalSeconds + $TimeoutSeconds` calcolato e mai usato. Eliminare o usare nel messaggio. + +- [ ] **`scripts/Wait-VMReady.ps1:80-86`** — `try/catch` attorno a `& vmrun ... | Out-Null`: native exit non lancia eccezioni, `catch` irraggiungibile. Semplificare a controllo `$LASTEXITCODE` puro. + +- [ ] **`template/Setup-TemplateVM.ps1:188,193`** — `MaxMemoryPerShellMB` settato sia via `winrm set winrm/config/winrs` che `Set-Item WSMan:\...\Shell\...`. Tenere uno solo (preferenza: `Set-Item` su PS drive — idempotente in PS). + +- [ ] **`template/Prepare-TemplateSetup.ps1:101-107`** — modifica permanente di `WSMan:\localhost\Client\AllowUnencrypted` sull'host senza ripristino e senza messaggio chiaro. + **Fix**: salvare lo stato pre-modifica e ripristinarlo nel `finally`, oppure documentare esplicitamente che è un cambio permanente di setup (riportare su `BEST-PRACTICES.md` e `Setup-Host.ps1`). + +- [ ] **`runner/Install-Runner.ps1` duplica `Setup-Host.ps1` Step 6** — README cita solo `Setup-Host.ps1`. Decidere: + a) eliminare `Install-Runner.ps1`, o + b) farlo richiamare da `Setup-Host.ps1` (single source of truth). + Drift garantito altrimenti. + +## P2 — Operativo + +- [ ] **Implementare `scripts/Cleanup-OrphanedBuildVMs.ps1`** — descritto in `docs/BEST-PRACTICES.md:155` ma non in repo. Crearlo + scheduled task daily che lo invoca. Soglia default: 4h da `LastWriteTime` (allineato a `runner.timeout: 2h`). + +- [ ] **Includere log build da subdir** — `scripts/Get-BuildArtifacts.ps1:96` + Oggi: `Get-ChildItem -Path 'C:\CI\build' -Filter '*.log'` (NON ricorsivo). + MSBuild scrive log in `obj/*.log`, `bin/*.log`. Aggiungere `-Recurse` (con limite di profondità via filtro path o size cap per evitare archivi enormi). + +- [ ] **`scripts/Invoke-RemoteBuild.ps1:141` — sanitizzazione `BuildCommand`** + `& cmd /c "$cmd 2>&1"` interpola stringa user-controlled. Workflow autore ha già exec, ma è poco igienico. + **Fix**: passare `BuildCommand` come array di args al ScriptBlock e invocare con `Start-Process`/`& $exe $args` quando possibile, oppure documentare che `BuildCommand` è trusted. + +- [ ] **PSScriptAnalyzer in CI** — aggiungere workflow `lint.yml` che esegue `Invoke-ScriptAnalyzer` su `scripts/`, `template/`, `Setup-Host.ps1`. Catturerebbe metà delle voci sopra (dead code, var inutilizzate, native exit pattern). + +- [ ] **Validazione IP più stretta** — `scripts/Invoke-CIJob.ps1:107` `[ValidatePattern('^(\d{1,3}\.){3}\d{1,3}$')]` accetta `999.999.999.999`. Sostituire con `[System.Net.IPAddress]::TryParse(...)` o regex per ottetti 0-255. + +- [ ] **Aggiornare `TODO.md` In-VM Git Clone** — la sezione è ancora aperta. Quando si attacca: + - non distribuire PAT in env globale del runner (`runner/config.yaml` `envs:`); leggerlo da Credential Manager per-job + - mai loggare la URL completa (con PAT inline) — fare `git remote set-url` post-clone con URL pulita prima di qualsiasi log + +--- + +## Note + +Sistema in stato production-ready (e2e-009 verde). Tutti i P0 sopra sono bug latenti che non si manifestano nel caso d'uso testato (`nsis-plugin-nsinnounp` sul branch HEAD), ma scattano al primo: +- rerun di un commit non-HEAD (P0 shallow clone), +- artifact molto piccolo (P0 size==0), +- maintenance (`upload-artifact v3` smetterà di funzionare senza preavviso). + +Le P1 doc sono il debito più visibile: chiunque legga `Setup-TemplateVM.ps1` da zero finisce per spostare la VM su Host-Only e rompere il sistema.