From 21aa6e39e1766c2b3cff5526bd98d96a27f40ca6 Mon Sep 17 00:00:00 2001 From: DonVoo Date: Tue, 14 Jul 2026 05:22:26 +0200 Subject: [PATCH] Document review fixes and harden mail actions --- REVIEW-FIXES.md | 57 +++++++++++++++++++++++++++++++++++++++++ backend/00-router.go | 3 +++ backend/08-viewer.go | 3 +++ backend/09-smtp.go | 29 ++++++++++++++++++--- backend/09-smtp_test.go | 12 +++++++++ 5 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 REVIEW-FIXES.md create mode 100644 backend/09-smtp_test.go diff --git a/REVIEW-FIXES.md b/REVIEW-FIXES.md new file mode 100644 index 0000000..c04844a --- /dev/null +++ b/REVIEW-FIXES.md @@ -0,0 +1,57 @@ +# Mail-Graveyard Review-Fixes + +Stand: 2026-07-14 + +## Richtung + +Der Migrationskern soll auf `emersion/go-imap/v2` umgestellt werden. Der aktuell handgeschriebene `simpleIMAP`-Parser ist zwar schlank, aber fuer ein Beweis-Archiv zu riskant: Ordnernamen, Flags, INTERNALDATE und FETCH-Varianten muessen korrekt und serverunabhaengig verarbeitet werden. + +## P0 - Migrationsintegritaet + +1. `simpleIMAP` durch `emersion/go-imap/v2` ersetzen. + - Betrifft `backend/04-imap-source.go` und `backend/05-imap-target.go`. + - Ziel: IMAP UTF-7 korrekt dekodieren, `FETCH` robust parsen, Flags korrekt normalisieren, APPEND RFC-konform senden. + +2. IMAP UTF-7 Ordnernamen korrekt behandeln. + - Aktuelles Risiko: Rollen-Erkennung fuer Papierkorb/Geloescht/Gesendet scheitert bei Alt-Providern. + - Folge: Zielordner werden doppelt angelegt. + +3. `\Recent` niemals per APPEND setzen. + - `\Recent` ist serververwaltet und darf nicht vom Client appended werden. + +4. INTERNALDATE robust parsen. + - Ein- und zweistellige Tage muessen funktionieren. + - Bei Parse-Fehlern nicht still auf `time.Now()` fallen, sondern melden oder kontrolliert degradieren. + +5. FETCH-Reihenfolge unabhaengig parsen. + - FLAGS, INTERNALDATE und BODY koennen serverseitig in anderer Reihenfolge kommen. + +6. Ordnerweise Streaming statt kompletter Ordner im RAM. + - Ziel: grosse Postfaecher nicht in den Speicher laden. + +7. POP3-Fallback verdrahten. + - `src_proto=pop3` darf nicht weiter in IMAP laufen. + +## P1 - Sicherheit und Rollen + +1. SMTP-Header-Injection verhindern. + - Erledigt: Betreff, From-Header und To-Header werden auf CR/LF geprueft. + +2. Schreibende Viewer-Aktionen rollenbegrenzen. + - Erledigt: `/mail/send` und `/view/manual-copy` sind vorerst nur fuer Verwalter/Admins erreichbar. + - Offene Produktentscheidung: Wenn normale Benutzer weiterhin eingeschraenkt weiterleiten duerfen, braucht es einen separaten, begrenzten Forward-Endpunkt statt freiem Compose-SMTP. + +## P2 - Lose Enden + +1. `/view/forward` und `ForwardMessage()` entfernen oder sauber in den neuen Forward-Flow integrieren. +2. Sync-Dropdowns entweder implementieren oder sichtbar als "noch nicht aktiv" kennzeichnen. +3. Stale `TODO Codex`-Kommentare an fertigen Handlern bereinigen. +4. Tote Render-/Helper-Funktionen entfernen, sobald die Transfer-Workbench stabil ist. + +## Verifikation pro Fix-Serie + +- `go test ./...` +- Live-Test gegen einen kleinen IMAP-Account mit Umlautordnern. +- Live-Test gegen ein Alt-Provider-Postfach ohne SPECIAL-USE. +- Grossordner-Test mit Speicherbeobachtung. +- Rollen-Test: User darf nur Archiv lesen; Verwalter/Admin duerfen senden, kopieren und verwalten. diff --git a/backend/00-router.go b/backend/00-router.go index bed369c..72984b7 100644 --- a/backend/00-router.go +++ b/backend/00-router.go @@ -65,6 +65,9 @@ func RegisterRoutes(mux *http.ServeMux) { } func mailSendHandler(w http.ResponseWriter, r *http.Request) { + if !requireManager(w, r) { + return + } if r.Method != http.MethodPost { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return diff --git a/backend/08-viewer.go b/backend/08-viewer.go index 7a97217..bbda7d1 100644 --- a/backend/08-viewer.go +++ b/backend/08-viewer.go @@ -271,6 +271,9 @@ func archiveFolderSortKey(name string) string { } func manualCopyHandler(w http.ResponseWriter, r *http.Request) { + if !requireManager(w, r) { + return + } if r.Method != http.MethodPost { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return diff --git a/backend/09-smtp.go b/backend/09-smtp.go index 25d6540..585201f 100644 --- a/backend/09-smtp.go +++ b/backend/09-smtp.go @@ -17,13 +17,25 @@ func SendPlainMail(to []string, subject, body string) error { if strings.TrimSpace(cfg.Host) == "" || cfg.Port == 0 || strings.TrimSpace(cfg.From) == "" { return fmt.Errorf("forward_smtp ist nicht vollstaendig konfiguriert") } + fromHeader, err := cleanMailHeader(cfg.From) + if err != nil { + return fmt.Errorf("ungueltiger Absender: %w", err) + } + subjectHeader, err := cleanMailHeader(subject) + if err != nil { + return fmt.Errorf("ungueltiger Betreff: %w", err) + } + toHeader, err := cleanMailHeader(strings.Join(to, ", ")) + if err != nil { + return fmt.Errorf("ungueltiger Empfaenger: %w", err) + } from := cfg.From if strings.TrimSpace(cfg.User) != "" { from = cfg.User } - message := []byte("From: " + cfg.From + "\r\n" + - "To: " + strings.Join(to, ", ") + "\r\n" + - "Subject: " + subject + "\r\n" + + message := []byte("From: " + fromHeader + "\r\n" + + "To: " + toHeader + "\r\n" + + "Subject: " + subjectHeader + "\r\n" + "Content-Type: text/plain; charset=utf-8\r\n" + "\r\n" + body) addr := net.JoinHostPort(cfg.Host, fmt.Sprint(cfg.Port)) @@ -37,6 +49,17 @@ func SendPlainMail(to []string, subject, body string) error { return sendStartTLS(addr, cfg.Host, auth, from, to, message) } +func cleanMailHeader(value string) (string, error) { + cleaned := strings.TrimSpace(value) + if cleaned == "" { + return "", nil + } + if strings.ContainsAny(cleaned, "\r\n") { + return "", fmt.Errorf("Header darf keine Zeilenumbrueche enthalten") + } + return cleaned, nil +} + func sendStartTLS(addr, host string, auth smtp.Auth, from string, to []string, message []byte) error { c, err := smtp.Dial(addr) if err != nil { diff --git a/backend/09-smtp_test.go b/backend/09-smtp_test.go new file mode 100644 index 0000000..699d51e --- /dev/null +++ b/backend/09-smtp_test.go @@ -0,0 +1,12 @@ +package backend + +import "testing" + +func TestCleanMailHeaderRejectsLineBreaks(t *testing.T) { + if _, err := cleanMailHeader("Betreff\r\nBcc: attacker@example.com"); err == nil { + t.Fatal("expected CRLF header injection to be rejected") + } + if got, err := cleanMailHeader(" Normaler Betreff "); err != nil || got != "Normaler Betreff" { + t.Fatalf("cleanMailHeader returned %q, %v", got, err) + } +}