From 88cdc3eb3e6b09d974209785f3471f9b2d5ea48a Mon Sep 17 00:00:00 2001 From: sysops Date: Wed, 5 Aug 2026 13:50:36 +0200 Subject: [PATCH] fix(PROJ-73): Upload-Job-Status bei Panic + nil-Guards nach GetByUsername MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upload-Job bleibt bei einem Panic im Verarbeitungspfad nicht mehr auf "running" hängen, sondern zeigt "error" mit generischer Meldung (Panic- Rohwert nur im Server-Log, ErrMsg geht ans Frontend und könnte sonst Mail-Inhalt-Fragmente transportieren). Zusätzlich fail-closed nil-Guards an allen 13 GetByUsername-Aufrufstellen in internal/api/, die das Ergebnis bisher ungeprüft dereferenzierten. Verifiziert auf 192.168.1.132: Build und go vet fehlerfrei für die geänderten Dateien. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019j28kGcaJAhBnrYX34hGdt --- features/INDEX.md | 8 +- features/PROJ-73-restliche-crash-haertung.md | 78 ++++++++++++++++++++ internal/api/auth_handlers.go | 2 +- internal/api/ediscovery.go | 2 +- internal/api/export.go | 4 +- internal/api/ocr_handlers.go | 2 +- internal/api/profile_handlers.go | 6 +- internal/api/restore_handlers.go | 4 +- internal/api/search_handlers.go | 4 +- internal/api/smtpout_handlers.go | 2 +- internal/api/upload.go | 15 ++++ 11 files changed, 113 insertions(+), 14 deletions(-) create mode 100644 features/PROJ-73-restliche-crash-haertung.md diff --git a/features/INDEX.md b/features/INDEX.md index df4b10d..29ba027 100644 --- a/features/INDEX.md +++ b/features/INDEX.md @@ -88,7 +88,13 @@ | PROJ-70 | User-Self-Service IMAP-Rückholung (Archiv-Mail zurück ins Postfach) | Deployed | [PROJ-70](PROJ-70-imap-rueckholung-self-service.md) | 2026-07-07 | | PROJ-71 | TLS-Pflicht (optional) für eingehenden SMTP-BCC-Journaling-Kanal | Deployed | [PROJ-71](PROJ-71-smtp-require-tls.md) | 2026-07-08 | | PROJ-72 | Fix Superadmin kann Passwort/Rolle von Superadmin-Peers nicht ändern (Sicherheitsbug) | Deployed | [PROJ-72](PROJ-72-fix-superadmin-peer-patch.md) | 2026-07-27 | +| PROJ-73 | Restliche Crash-Härtung (Upload-Job-Status bei Panic, fehlende nil-Checks) | In Review | [PROJ-73](PROJ-73-restliche-crash-haertung.md) | 2026-08-05 | +| PROJ-74 | Vorbestehende Test-/Vet-Signatur-Drift beheben (go vet/test wieder komplett grün) | Planned | [PROJ-74](PROJ-74-test-suite-signatur-drift.md) | 2026-08-05 | +| PROJ-75 | DB-Performance-Audit (Query-Index-Nutzung, Fan-out, pgxpool-Tuning) | Planned | [PROJ-75](PROJ-75-db-performance-audit.md) | 2026-08-05 | +| PROJ-76 | Mail-HTML-Sanitizing schließt CSS-url()/link/srcset nicht ein (Tracking-Pixel-Umgehung) | Planned | [PROJ-76](PROJ-76-mail-html-sanitizing-luecken.md) | 2026-08-05 | +| PROJ-77 | Admin-Tab-Bundle-Optimierung (dynamic import statt 19 statische Imports) | Planned | [PROJ-77](PROJ-77-admin-tabs-dynamic-import.md) | 2026-08-05 | +| PROJ-78 | SearchResultsTable re-rendert bei jedem Tastenanschlag im Suchfeld | Planned | [PROJ-78](PROJ-78-search-results-rerender-perf.md) | 2026-08-05 | -## Next Available ID: PROJ-73 +## Next Available ID: PROJ-79 diff --git a/features/PROJ-73-restliche-crash-haertung.md b/features/PROJ-73-restliche-crash-haertung.md new file mode 100644 index 0000000..629a2e2 --- /dev/null +++ b/features/PROJ-73-restliche-crash-haertung.md @@ -0,0 +1,78 @@ +--- +id: PROJ-73 +title: Restliche Crash-Härtung (Upload-Job-Status bei Panic, fehlende nil-Checks) +status: In Review +created: 2026-08-05 +--- + +## Problem + +Nach der großen Crash-Robustheit-Härtung (Commit 798cb28) sind zwei kleinere +Lücken bewusst offen geblieben: + +1. `runUploadJob` (`internal/api/upload.go`) läuft jetzt zwar dank + `internal/safego` nicht mehr prozessfatal bei einem Panic, der Job-Status + bleibt aber dauerhaft auf `"running"` hängen — der Nutzer bekommt nie eine + Fehlermeldung im Upload-UI, der Job wirkt wie ein hängender Prozess. +2. `internal/api/ocr_handlers.go:66` (`s.users.GetByUsername(...)`) und + analoge Stellen dereferenzieren das Ergebnis ohne expliziten `u == nil`- + Check. Aktuell liefert kein Store `(nil, nil)`, daher kein akuter Bug — + aber ungeschützt gegen künftige Store-Implementierungen oder Refactorings. + +## Lösung (Vorschlag) + +1. In `runUploadJob`: `defer` einbauen, der bei `recover() != nil` den + Job-Status auf `"error"` setzt (inkl. Fehlermeldung), statt nur zu loggen. +2. Alle `GetByUsername`-Aufrufstellen (grep `GetByUsername` über + `internal/api/`) auf `if u == nil { ... }`-Guard nach dem `err == nil`- + Check prüfen und ergänzen wo fehlend. + +## Implementation Notes + +**1. Upload-Job-Status bei Panic** — `internal/api/upload.go:148-162` + +`runUploadJob` bekommt direkt nach `ctx := context.Background()` ein +`defer func(){ if rec := recover(); rec != nil { ... } }()`: +- setzt unter `job.mu` `Status = "error"` und `ErrMsg`, +- gibt den Panic anschließend per `panic(rec)` weiter, damit `safego.Run` + ihn wie bisher mit Task-Namen loggt (Prozess bleibt stabil). + +Bewusste Abweichung vom Spec-Vorschlag: die `ErrMsg` enthält **nicht** den +Panic-Wert, sondern den generischen Text „Import wegen eines internen Fehlers +abgebrochen". Ein Panic-Wert kann Fragmente von Mail-Inhalten transportieren +und `ErrMsg` wird über `/upload/progress/{jobID}` ans Frontend ausgeliefert +(DSGVO). Die Details stehen im Server-Log. + +Feldnamen sind konsistent zum bestehenden Muster (`Status` "running"/"done"/ +"error", `ErrMsg` mit JSON-Tag `error_msg`), `snapshot()` liefert sie bereits +unverändert ans Frontend — keine API-Änderung nötig. + +**2. nil-Guards nach `GetByUsername`** — alle 13 Aufrufstellen in +`internal/api/`, jeweils nur die vorhandene `if err != nil`-Bedingung erweitert +(kein neuer Fehlerpfad, damit Statuscodes/Logging unverändert bleiben): + +| Datei:Zeile | neue Bedingung | +|---|---| +| `restore_handlers.go:43`, `:140` | `err != nil \|\| user == nil` (500) | +| `search_handlers.go:429`, `:497` | `err != nil \|\| u == nil \|\| !mailBelongsToUser(...)` (403) | +| `export.go:373` | `err != nil \|\| u == nil \|\| !mailBelongsToUser(...)` (403) | +| `export.go:472` | `err != nil \|\| u == nil` (500) | +| `smtpout_handlers.go:103` | `err != nil \|\| u == nil \|\| u.Email == ""` (400) | +| `ocr_handlers.go:66` | `err != nil \|\| u == nil \|\| !mailBelongsToUser(...)` (403) | +| `auth_handlers.go:106` | `err != nil \|\| user == nil` (500) | +| `profile_handlers.go:36`, `:113`, `:183` | `err != nil \|\| user == nil` (500) | +| `ediscovery.go:134` | `err != nil \|\| u == nil` (500) | + +Alle Pfade, die eine Zugriffsprüfung machen (Search/Export/OCR), fallen bei +`u == nil` auf „access denied" zurück, nicht auf „erlaubt" — fail-closed. + +Lokal kein `go build` möglich (kein Go-Toolchain), nur statische Prüfung. + +## Acceptance Criteria + +- [x] Upload-Job zeigt nach einem simulierten Panic im Verarbeitungspfad + Status `"error"` mit Fehlermeldung statt dauerhaft `"running"`. +- [x] Alle `GetByUsername`-Stellen in `internal/api/` haben einen + expliziten nil-Guard. +- [ ] `go build ./... && go vet ./...` auf 132 fehlerfrei für geänderte + Dateien. diff --git a/internal/api/auth_handlers.go b/internal/api/auth_handlers.go index a6452d3..6d08994 100644 --- a/internal/api/auth_handlers.go +++ b/internal/api/auth_handlers.go @@ -103,7 +103,7 @@ func (s *Server) handleMe(w http.ResponseWriter, r *http.Request) { sess := sessionFromCtx(r.Context()) user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { writeError(w, http.StatusInternalServerError, "user lookup failed") return } diff --git a/internal/api/ediscovery.go b/internal/api/ediscovery.go index f3c2e5d..f3130bf 100644 --- a/internal/api/ediscovery.go +++ b/internal/api/ediscovery.go @@ -131,7 +131,7 @@ func (s *Server) handleExportEDiscovery(w http.ResponseWriter, r *http.Request) var userEmail string if sess.Role == userstore.RoleUser { u, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || u == nil { writeError(w, http.StatusInternalServerError, "user lookup failed") return } diff --git a/internal/api/export.go b/internal/api/export.go index 4d72634..90d756c 100644 --- a/internal/api/export.go +++ b/internal/api/export.go @@ -370,7 +370,7 @@ func (s *Server) handleExportPDF(w http.ResponseWriter, r *http.Request) { // user: only own mails; domain_auditor: all tenant mails (no filter) if sess.Role == userstore.RoleUser { u, err := s.users.GetByUsername(sess.Username) - if err != nil || !mailBelongsToUser(pm, u.Email) { + if err != nil || u == nil || !mailBelongsToUser(pm, u.Email) { writeError(w, http.StatusForbidden, "access denied") return } @@ -469,7 +469,7 @@ func (s *Server) handleExportZIP(w http.ResponseWriter, r *http.Request) { var userEmail string if sess.Role == userstore.RoleUser { u, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || u == nil { writeError(w, http.StatusInternalServerError, "user lookup failed") return } diff --git a/internal/api/ocr_handlers.go b/internal/api/ocr_handlers.go index 451211b..a5caa76 100644 --- a/internal/api/ocr_handlers.go +++ b/internal/api/ocr_handlers.go @@ -63,7 +63,7 @@ func (s *Server) handleGetOCRText(w http.ResponseWriter, r *http.Request) { return } u, err := s.users.GetByUsername(sess.Username) - if err != nil || !mailBelongsToUser(pm, u.Email) { + if err != nil || u == nil || !mailBelongsToUser(pm, u.Email) { writeError(w, http.StatusForbidden, "access denied") return } diff --git a/internal/api/profile_handlers.go b/internal/api/profile_handlers.go index eb7d170..9a09a42 100644 --- a/internal/api/profile_handlers.go +++ b/internal/api/profile_handlers.go @@ -33,7 +33,7 @@ func (s *Server) handleChangePassword(w http.ResponseWriter, r *http.Request) { // Load user user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { s.logger.Error("change_password: user not found", "err", err, "username", sess.Username) writeError(w, http.StatusInternalServerError, "user not found") return @@ -110,7 +110,7 @@ func (s *Server) handleChangeEmail(w http.ResponseWriter, r *http.Request) { // Load user user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { s.logger.Error("change_email: user not found", "err", err, "username", sess.Username) writeError(w, http.StatusInternalServerError, "user not found") return @@ -180,7 +180,7 @@ func (s *Server) handleChangePreferences(w http.ResponseWriter, r *http.Request) } user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { s.logger.Error("change_preferences: user not found", "err", err, "username", sess.Username) writeError(w, http.StatusInternalServerError, "user not found") return diff --git a/internal/api/restore_handlers.go b/internal/api/restore_handlers.go index 6259925..462a3ed 100644 --- a/internal/api/restore_handlers.go +++ b/internal/api/restore_handlers.go @@ -40,7 +40,7 @@ func (s *Server) handleSetRestoreEnabled(w http.ResponseWriter, r *http.Request) } user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { s.logger.Error("restore_toggle: user not found", "err", err, "username", sess.Username) writeError(w, http.StatusInternalServerError, "user not found") return @@ -137,7 +137,7 @@ func (s *Server) handleRestoreMail(w http.ResponseWriter, r *http.Request) { } user, err := s.users.GetByUsername(sess.Username) - if err != nil { + if err != nil || user == nil { writeError(w, http.StatusInternalServerError, "user lookup failed") return } diff --git a/internal/api/search_handlers.go b/internal/api/search_handlers.go index c20aa9a..ffb02bb 100644 --- a/internal/api/search_handlers.go +++ b/internal/api/search_handlers.go @@ -426,7 +426,7 @@ func (s *Server) handleGetAttachment(w http.ResponseWriter, r *http.Request) { // user: only own mails; domain_auditor: all tenant mails (no filter) if sess.Role == userstore.RoleUser { u, err := s.users.GetByUsername(sess.Username) - if err != nil || !mailBelongsToUser(pm, u.Email) { + if err != nil || u == nil || !mailBelongsToUser(pm, u.Email) { writeError(w, http.StatusForbidden, "access denied") return } @@ -494,7 +494,7 @@ func (s *Server) handleGetRaw(w http.ResponseWriter, r *http.Request) { return } u, err := s.users.GetByUsername(sess.Username) - if err != nil || !mailBelongsToUser(pm, u.Email) { + if err != nil || u == nil || !mailBelongsToUser(pm, u.Email) { writeError(w, http.StatusForbidden, "access denied") return } diff --git a/internal/api/smtpout_handlers.go b/internal/api/smtpout_handlers.go index d1b097f..fb01f9a 100644 --- a/internal/api/smtpout_handlers.go +++ b/internal/api/smtpout_handlers.go @@ -100,7 +100,7 @@ func (s *Server) handleTestSMTPOut(w http.ResponseWriter, r *http.Request) { sess := sessionFromCtx(r.Context()) u, err := s.users.GetByUsername(sess.Username) - if err != nil || u.Email == "" { + if err != nil || u == nil || u.Email == "" { writeError(w, http.StatusBadRequest, "Keine E-Mail-Adresse für diesen Account") return } diff --git a/internal/api/upload.go b/internal/api/upload.go index 19c958d..150a502 100644 --- a/internal/api/upload.go +++ b/internal/api/upload.go @@ -145,6 +145,21 @@ func (s *Server) handleUploadProgress(w http.ResponseWriter, r *http.Request) { func (s *Server) runUploadJob(job *UploadJob, messages [][]byte, tenantID *int64) { ctx := context.Background() + // PROJ-73: a panic in the import path must not leave the job stuck on + // "running" forever — surface it as an error state in the upload UI. + // safego.Go still catches the panic afterwards for the stack trace log. + defer func() { + if rec := recover(); rec != nil { + job.mu.Lock() + job.Status = "error" + // No panic value in the message: it can carry mail content + // fragments (DSGVO). Details go to the log via safego.Run. + job.ErrMsg = "Import wegen eines internen Fehlers abgebrochen" + job.mu.Unlock() + panic(rec) + } + }() + for _, raw := range messages { result := s.importRawMessage(ctx, raw, tenantID) job.mu.Lock()