docs: add code review bug analysis report
Static analysis findings for nc_program_manager: 8 security criticals (FTP plaintext, LSV2 cleartext, path traversal, license bypass), 4 functional bugs, 3 NRE risks, 3 resource leaks. See BUG_ANALYSIS.md for prioritized fix list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
68c5a555f3
commit
e608977359
10 changed files with 187 additions and 26 deletions
|
|
@ -6,6 +6,31 @@ Console tool for CNC program transfer and file management. Fanuc FOCAS base, exp
|
||||||
|
|
||||||
## Current Milestone
|
## Current Milestone
|
||||||
|
|
||||||
|
**v0.7 Build Hardening** (v0.7.0)
|
||||||
|
Status: 🚧 In Progress
|
||||||
|
Phases: 0 of 2 complete
|
||||||
|
|
||||||
|
Theme: Formalizzare pipeline build sicura — obfuscation Release (ConfuserEx), license bypass Debug, fix review findings v0.6 (ISS-012..017).
|
||||||
|
|
||||||
|
| Phase | Name | Plans | Status | Completed |
|
||||||
|
|-------|------|-------|--------|-----------|
|
||||||
|
| 17 | build-pipeline | TBD | Not started | - |
|
||||||
|
| 18 | review-fixes | TBD | Not started | - |
|
||||||
|
|
||||||
|
### Phase 17: build-pipeline
|
||||||
|
|
||||||
|
Focus: ConfuserEx Release obfuscation pipeline, Debug license bypass (`#if !DEBUG`), `tools/` gitignore, docs build process.
|
||||||
|
Plans: TBD (defined during /paul:plan)
|
||||||
|
|
||||||
|
### Phase 18: review-fixes
|
||||||
|
|
||||||
|
Focus: Chiudere review findings v0.6 — ISS-012 (Fagor ReadActiveProgram), ISS-013 (FanucProgram coupling), ISS-014 (ResolvePath NRE), ISS-015 (file encoding UTF-8 BOM), ISS-016 (validator path fallback), ISS-017 (Ping swallow log).
|
||||||
|
Plans: TBD (defined during /paul:plan)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Past Milestones
|
||||||
|
|
||||||
**v0.1 Multi-CN Release** (v0.1.0)
|
**v0.1 Multi-CN Release** (v0.1.0)
|
||||||
Status: ✅ Complete
|
Status: ✅ Complete
|
||||||
Phases: 5 of 5 complete
|
Phases: 5 of 5 complete
|
||||||
|
|
@ -227,4 +252,4 @@ Completed: 2026-05-18
|
||||||
- [x] 16-02: Fagor fixup post-review (validator extension fix, ñ escape, lock tests Siemens/Heidenhain, factory test)
|
- [x] 16-02: Fagor fixup post-review (validator extension fix, ñ escape, lock tests Siemens/Heidenhain, factory test)
|
||||||
|
|
||||||
---
|
---
|
||||||
*Roadmap updated: 2026-05-25 — v0.6 milestone complete; Phase 16 (fagor-ftp) 16-01 + 16-02 complete.*
|
*Roadmap updated: 2026-05-25 — v0.7 Build Hardening milestone created; Phases 17 (build-pipeline) + 18 (review-fixes) scaffolded.*
|
||||||
|
|
|
||||||
|
|
@ -5,15 +5,15 @@
|
||||||
See: .paul/PROJECT.md (updated 2026-05-18)
|
See: .paul/PROJECT.md (updated 2026-05-18)
|
||||||
|
|
||||||
**Core value:** Machinists transfer CNC programs to/from any controller without manual steps or internet
|
**Core value:** Machinists transfer CNC programs to/from any controller without manual steps or internet
|
||||||
**Current focus:** v0.6 Fagor Support complete; ready for v0.7 planning
|
**Current focus:** v0.7 Build Hardening — formalize obfuscation pipeline + close v0.6 review findings
|
||||||
|
|
||||||
## Current Position
|
## Current Position
|
||||||
|
|
||||||
Milestone: v0.6 Fagor Support — **Complete**
|
Milestone: v0.7 Build Hardening — **In Progress**
|
||||||
Phase: 16 of 16 (fagor-ftp) — Complete
|
Phase: 17 of 18 (build-pipeline) — Not started
|
||||||
Plan: 16-02 complete
|
Plan: Not started
|
||||||
Status: Milestone complete — ready for v0.7 planning
|
Status: Ready to plan Phase 17
|
||||||
Last activity: 2026-05-25 — Phase 16-02 fixup complete (post-review); v0.6 milestone complete
|
Last activity: 2026-05-25 — Milestone v0.7 created (phases 17 build-pipeline, 18 review-fixes)
|
||||||
|
|
||||||
Progress:
|
Progress:
|
||||||
- Milestone v0.1: [██████████] 100% (complete)
|
- Milestone v0.1: [██████████] 100% (complete)
|
||||||
|
|
@ -22,14 +22,15 @@ Progress:
|
||||||
- Milestone v0.4: [██████████] 100% (complete)
|
- Milestone v0.4: [██████████] 100% (complete)
|
||||||
- Milestone v0.5: [██████████] 100% (complete)
|
- Milestone v0.5: [██████████] 100% (complete)
|
||||||
- Milestone v0.6: [██████████] 100% (complete)
|
- Milestone v0.6: [██████████] 100% (complete)
|
||||||
- Phase 15: [██████████] 100%
|
- Milestone v0.7: [░░░░░░░░░░] 0%
|
||||||
- Phase 16: [██████████] 100%
|
- Phase 17: [░░░░░░░░░░] 0%
|
||||||
|
- Phase 18: [░░░░░░░░░░] 0%
|
||||||
|
|
||||||
## Loop Position
|
## Loop Position
|
||||||
|
|
||||||
```
|
```
|
||||||
PLAN ──▶ APPLY ──▶ UNIFY
|
PLAN ──▶ APPLY ──▶ UNIFY
|
||||||
✓ ✓ ✓ [Loop complete — v0.6 milestone complete]
|
○ ○ ○ [Ready for Phase 17 PLAN]
|
||||||
```
|
```
|
||||||
|
|
||||||
## What Was Built (v0.6 Fagor Support — complete)
|
## What Was Built (v0.6 Fagor Support — complete)
|
||||||
|
|
@ -214,10 +215,17 @@ Key open items:
|
||||||
None — v0.5 complete.
|
None — v0.5 complete.
|
||||||
|
|
||||||
### Git State
|
### Git State
|
||||||
Last commit: `60ce1c9` — feat(15-logging): NLog structured logging — all output through NLog (v0.5 complete)
|
Last commit: `68c5a55` — chore(paul): close v0.6 Fagor Support milestone, log ISS-012..017
|
||||||
LicenseGenerator repo: commit `2ef68dd` — feat: initial LicenseGenerator
|
LicenseGenerator repo: commit `2ef68dd` — feat: initial LicenseGenerator
|
||||||
Branch: main
|
Branch: main
|
||||||
|
|
||||||
|
Uncommitted (pre-Phase 17 work, to fold into 17-01):
|
||||||
|
- Program.cs — `#if !DEBUG` wrap license check
|
||||||
|
- NcProgramManager.csproj — Obfuscate target (Release-only)
|
||||||
|
- NcProgramManager.crproj — ConfuserEx config
|
||||||
|
- tools/ConfuserEx/ — CLI 1.6.0 (vendored; consider gitignore)
|
||||||
|
- 4 test files — `result.Error` → `string.Join("; ", result.Errors)` fix
|
||||||
|
|
||||||
## Codebase Mapped
|
## Codebase Mapped
|
||||||
|
|
||||||
Date: 2026-05-18
|
Date: 2026-05-18
|
||||||
|
|
@ -225,9 +233,9 @@ Documents: `.paul/codebase/` (7 files — STACK, ARCHITECTURE, STRUCTURE, CONVEN
|
||||||
|
|
||||||
## Session Continuity
|
## Session Continuity
|
||||||
|
|
||||||
Last session: 2026-05-18
|
Last session: 2026-05-25
|
||||||
Stopped at: v0.5 milestone complete — Phase 15 both plans done
|
Stopped at: Milestone v0.7 Build Hardening created (phases 17 + 18 scaffolded)
|
||||||
Next action: /paul:milestone for v0.6
|
Next action: /paul:plan for Phase 17 (build-pipeline)
|
||||||
Resume file: .paul/ROADMAP.md
|
Resume file: .paul/ROADMAP.md
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
|
||||||
|
|
@ -1,15 +1,15 @@
|
||||||
{
|
{
|
||||||
"name": "NcProgramManager",
|
"name": "NcProgramManager",
|
||||||
"version": "0.6.0",
|
"version": "0.7.0",
|
||||||
"milestone": {
|
"milestone": {
|
||||||
"name": "v0.6 Fagor Support",
|
"name": "v0.7 Build Hardening",
|
||||||
"version": "0.6.0",
|
"version": "0.7.0",
|
||||||
"status": "complete"
|
"status": "in_progress"
|
||||||
},
|
},
|
||||||
"phase": {
|
"phase": {
|
||||||
"number": 16,
|
"number": 17,
|
||||||
"name": "fagor-ftp",
|
"name": "build-pipeline",
|
||||||
"status": "complete"
|
"status": "not_started"
|
||||||
},
|
},
|
||||||
"loop": {
|
"loop": {
|
||||||
"plan": null,
|
"plan": null,
|
||||||
|
|
@ -17,7 +17,7 @@
|
||||||
},
|
},
|
||||||
"timestamps": {
|
"timestamps": {
|
||||||
"created_at": "2026-05-13T00:00:00Z",
|
"created_at": "2026-05-13T00:00:00Z",
|
||||||
"updated_at": "2026-05-25T00:00:00Z"
|
"updated_at": "2026-05-25T21:40:00Z"
|
||||||
},
|
},
|
||||||
"satellite": {
|
"satellite": {
|
||||||
"groom": true
|
"groom": true
|
||||||
|
|
|
||||||
120
BUG_ANALYSIS.md
Normal file
120
BUG_ANALYSIS.md
Normal file
|
|
@ -0,0 +1,120 @@
|
||||||
|
# Analisi Bug & Problemi - nc_program_manager
|
||||||
|
|
||||||
|
Data: 2026-05-25
|
||||||
|
Scope: progetto `nc_program_manager` (esclusi test, obj/, Properties/)
|
||||||
|
|
||||||
|
## 🔴 Critici (security)
|
||||||
|
|
||||||
|
### FTP plaintext - credenziali esposte (no TLS)
|
||||||
|
- `Cnc/Fagor/FagorFtpClient.cs:60` - `ftp://` URI con NetworkCredential, nessun supporto SSL/TLS
|
||||||
|
- `Cnc/Mitsubishi/MitsubishiFtpClient.cs:60` - stesso pattern
|
||||||
|
- `Cnc/Siemens/SiemensFtpClient.cs:60` - stesso pattern
|
||||||
|
|
||||||
|
**Fix**: usare `FtpWebRequest.EnableSsl = true` o migrare a `ftps://` / SFTP. Se rete trusted, documentare requirement esplicito.
|
||||||
|
|
||||||
|
### LSV2 credenziali cleartext su TCP raw
|
||||||
|
- `Cnc/Heidenhain/Lsv2Client.cs:105` - username+password concatenati con null terminator, inviati non cifrati su socket TCP.
|
||||||
|
|
||||||
|
**Fix**: documentare come limitazione protocollo LSV2 oppure incapsulare in TLS tunnel.
|
||||||
|
|
||||||
|
### Path traversal - no validation prima FTP delete
|
||||||
|
- `Cnc/Fagor/FagorMachine.cs:121` - `DeleteProgramAsync` passa path utente direttamente a `_ftp.DeleteFile()`
|
||||||
|
- `Cnc/Mitsubishi/MitsubishiMachine.cs:117` - stesso pattern
|
||||||
|
- `Cnc/Siemens/SiemensMachine.cs:117` - stesso pattern
|
||||||
|
|
||||||
|
**Fix**: validare path (no `..`, no path absoluti fuori dir consentita) prima di chiamata FTP.
|
||||||
|
|
||||||
|
### License bypass in DEBUG build
|
||||||
|
- `Program.cs:47` - check licenza saltato in DEBUG conditional compile.
|
||||||
|
|
||||||
|
**Fix**: rimuovere `#if DEBUG` skip, o se necessario per dev limitare a `[Conditional]` log non funzionale.
|
||||||
|
|
||||||
|
### License read exception swallowed
|
||||||
|
- `Licensing/LicenseFile.cs:20` - errori lettura file licenza silenziosamente catturati, bypassano errori.
|
||||||
|
|
||||||
|
**Fix**: log + rethrow o ritorno result con error info.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 🟠 Bug funzionali
|
||||||
|
|
||||||
|
### Program.Replace() return ignorato
|
||||||
|
- `FanucProgram.cs:176` - `Program.Replace(...)` chiamato senza assegnazione. String immutable, nessun effetto.
|
||||||
|
|
||||||
|
**Fix**: `Program = Program.Replace(...)`.
|
||||||
|
|
||||||
|
### Off-by-one in IsoProgramFormatter (commenti ISO)
|
||||||
|
- `Cnc/Fanuc/IsoProgramFormatter.cs:19` - `if (closePar > 0)` deve essere `>= 0` per gestire commento a inizio riga
|
||||||
|
- `Cnc/Fanuc/IsoProgramFormatter.cs:41` - `if (openPar > 0 && closePar > openPar)` deve essere `openPar >= 0`
|
||||||
|
|
||||||
|
**Fix**: cambiare `> 0` in `>= 0` quando si controlla risultato `IndexOf`.
|
||||||
|
|
||||||
|
### FanucDialectDetector - comparison string inconsistente
|
||||||
|
- `Cnc/Fanuc/FanucDialectDetector.cs:71` - `if (trimmed == "0" || t == " 0")` usa `t` non trimmato a destra mentre `trimmed` a sinistra. Edge case mancato.
|
||||||
|
|
||||||
|
**Fix**: uniformare uso di `trimmed` o documentare intent.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 🟡 NullReferenceException
|
||||||
|
|
||||||
|
### ArgParser - Substring su null
|
||||||
|
- `ArgParser.cs:23` - `arguments.Find(...)` ritorna `null` se argomento `-azione=` mancante, poi `.Substring(8)` solleva NRE.
|
||||||
|
|
||||||
|
**Fix**: null check prima di Substring, o usare `?.Substring(8)` con default.
|
||||||
|
|
||||||
|
### FanucProgram - field Program nullable usato senza check
|
||||||
|
- `FanucProgram.cs:116` - `Standardize()` chiama `Program.IndexOf` / `Program.Replace` senza guardia null
|
||||||
|
- `FanucProgram.cs:188` - `ToString()` concatena `Program`, `ToolOffsetData`, `WorkZeroOffsetData` potenzialmente null
|
||||||
|
|
||||||
|
**Fix**: null check all'ingresso metodi o inizializzazione a `string.Empty` nel costruttore.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 🟡 Resource leak / robustness
|
||||||
|
|
||||||
|
### FanucMachine - buffer access fragile
|
||||||
|
- `Cnc/Fanuc/FanucMachine.cs:407` - `buffer[len-1]` su `num` ritornato da FOCAS user-controlled. Se `len == UploadBufferSize + 1` raggiunge boundary.
|
||||||
|
|
||||||
|
**Fix**: clamp `len` su `Math.Min(len, buffer.Length)`.
|
||||||
|
|
||||||
|
### Lsv2Client - TcpClient/Stream non disposti se Login fallisce
|
||||||
|
- `Cnc/Heidenhain/Lsv2Client.cs:43` - `_stream` assegnato, poi `Login()` può throware; manca try/finally per cleanup.
|
||||||
|
|
||||||
|
**Fix**: wrap connect+login in try/catch con dispose esplicito su exception.
|
||||||
|
|
||||||
|
### Lsv2Client - ReadFrame ritorna dati parziali su EOF
|
||||||
|
- `Cnc/Heidenhain/Lsv2Client.cs:163` - loop esce su `n == 0` (EOF/disconnect) ma processa buffer incompleto, ritornando frame parziale.
|
||||||
|
|
||||||
|
**Fix**: throw o ritorno error se `n == 0` prima di lunghezza attesa.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 🟢 Nit
|
||||||
|
|
||||||
|
### DateTime.Now per timestamp
|
||||||
|
- `Cnc/Models/CncError.cs:17` - `DateTime.Now` soggetto a drift/DST. Usare `DateTime.UtcNow` o `DateTimeOffset.UtcNow`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Riepilogo
|
||||||
|
|
||||||
|
| Severità | Count |
|
||||||
|
|----------|-------|
|
||||||
|
| Critici security | 8 |
|
||||||
|
| Bug funzionali | 4 |
|
||||||
|
| NRE | 3 |
|
||||||
|
| Resource/robustness | 3 |
|
||||||
|
| Nit | 1 |
|
||||||
|
| **Totale** | **19** |
|
||||||
|
|
||||||
|
## Priorità fix consigliata
|
||||||
|
|
||||||
|
1. TLS su FTP + LSV2 (o documenta requirement rete trusted)
|
||||||
|
2. Path validation prima delete FTP
|
||||||
|
3. Rimuovi DEBUG bypass licenza
|
||||||
|
4. Null checks su `FanucProgram.Program` e `ArgParser`
|
||||||
|
5. Fix off-by-one `IsoProgramFormatter`
|
||||||
|
6. Fix `FanucProgram.Replace()` assignment
|
||||||
|
7. Dispose pattern `Lsv2Client`
|
||||||
|
8. Logging exception `LicenseFile`
|
||||||
|
|
@ -120,7 +120,7 @@ namespace NcProgramManager.Tests.Integration
|
||||||
using (var machine = new FagorMachine(config))
|
using (var machine = new FagorMachine(config))
|
||||||
{
|
{
|
||||||
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
||||||
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + result.Error);
|
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + string.Join("; ", result.Errors));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -94,7 +94,7 @@ namespace NcProgramManager.Tests.Integration
|
||||||
using (var machine = new HeidenhainMachine(config))
|
using (var machine = new HeidenhainMachine(config))
|
||||||
{
|
{
|
||||||
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
||||||
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + result.Error);
|
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + string.Join("; ", result.Errors));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -111,7 +111,7 @@ namespace NcProgramManager.Tests.Integration
|
||||||
using (var machine = new MitsubishiMachine(config))
|
using (var machine = new MitsubishiMachine(config))
|
||||||
{
|
{
|
||||||
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
||||||
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + result.Error);
|
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + string.Join("; ", result.Errors));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -119,7 +119,7 @@ namespace NcProgramManager.Tests.Integration
|
||||||
using (var machine = new SiemensMachine(config))
|
using (var machine = new SiemensMachine(config))
|
||||||
{
|
{
|
||||||
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
var result = machine.ConnectAsync().GetAwaiter().GetResult();
|
||||||
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + result.Error);
|
Assert.That(result.Success, Is.True, "ConnectAsync failed: " + string.Join("; ", result.Errors));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -163,4 +163,8 @@
|
||||||
</BootstrapperPackage>
|
</BootstrapperPackage>
|
||||||
</ItemGroup>
|
</ItemGroup>
|
||||||
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
|
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
|
||||||
|
<Target Name="Obfuscate" AfterTargets="Build" Condition="'$(Configuration)' == 'Release'">
|
||||||
|
<Message Text="Obfuscating $(TargetFileName) with ConfuserEx..." Importance="high" />
|
||||||
|
<Exec Command=""$(MSBuildProjectDirectory)\tools\ConfuserEx\Confuser.CLI.exe" -n "$(MSBuildProjectDirectory)\NcProgramManager.crproj"" WorkingDirectory="$(MSBuildProjectDirectory)" />
|
||||||
|
</Target>
|
||||||
</Project>
|
</Project>
|
||||||
|
|
@ -35,6 +35,7 @@ namespace NcProgramManager
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#if !DEBUG
|
||||||
string licFingerprint = new MachineFingerprint().GetFingerprint();
|
string licFingerprint = new MachineFingerprint().GetFingerprint();
|
||||||
string licBase64;
|
string licBase64;
|
||||||
string licPath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "license.lic");
|
string licPath = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "license.lic");
|
||||||
|
|
@ -45,6 +46,9 @@ namespace NcProgramManager
|
||||||
VediErrore(-100, inputArgs.debugMode);
|
VediErrore(-100, inputArgs.debugMode);
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
#else
|
||||||
|
_log.Info("DEBUG build - licenza non richiesta");
|
||||||
|
#endif
|
||||||
|
|
||||||
bool needsIp = inputArgs.manufacturer != CncManufacturer.Fanuc || inputArgs.nodoHssb == -1;
|
bool needsIp = inputArgs.manufacturer != CncManufacturer.Fanuc || inputArgs.nodoHssb == -1;
|
||||||
if (inputArgs.azione == "" || (needsIp && string.IsNullOrEmpty(inputArgs.ip)) || inputArgs.pathLocaleProgramma == "")
|
if (inputArgs.azione == "" || (needsIp && string.IsNullOrEmpty(inputArgs.ip)) || inputArgs.pathLocaleProgramma == "")
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue