Simone
ecd79c2219
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 <noreply@anthropic.com>
111 lines
8.2 KiB
Markdown
111 lines
8.2 KiB
Markdown
# FIX-TODO — Local CI/CD System
|
|
|
|
<!-- Generato: 2026-05-09 — review post e2e-009 SUCCESS -->
|
|
|
|
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.
|