diff --git a/CODE-REVIEW.md b/CODE-REVIEW.md new file mode 100644 index 00000000..43515c5e --- /dev/null +++ b/CODE-REVIEW.md @@ -0,0 +1,497 @@ +# Code Review — Sicherheit & UX + +**Projekt:** plMail · **Stand:** `132c1bf` (v0.1.9) · **Datum:** 2026-08-22 +**Fokus:** Sicherheit und UX · **Umfang:** 143.503 LOC PHP in `src/`, Templates, Stimulus-Controller, Deployment-Konfiguration + +--- + +## 1. Gesamteindruck + +Das ist eine überdurchschnittlich sorgfältig gebaute Codebase. Die sicherheitskritischen Kernentscheidungen sind nicht nur richtig getroffen, sondern im Code auch begründet — der Image-Proxy (`ImageProxyFetcher`) gehört zum Besten, was man an SSRF-Abwehr in einer PHP-Anwendung sieht (DNS-Pinning gegen Rebinding, manuelle Redirect-Behandlung mit Revalidierung pro Hop, Port- und Scheme-Pinning). Die Reading-Frame-Isolierung (`MessageRenderer`) ist ein Lehrbuchbeispiel: opaker Origin, Nonce-CSP, `img-src` auf den eigenen Origin beschränkt, damit Blocker **und** CSP beide versagen müssten, bevor ein Tracking-Pixel feuert. + +Genau deshalb fällt der zentrale Fund umso stärker auf: **die sorgfältig gebaute Sandbox wird beim Antworten auf eine Mail umgangen.** Der Lesepfad ist gehärtet, der Antwortpfad nicht — und beide arbeiten mit demselben, vom Angreifer kontrollierten HTML. + +**Ergebnis in Zahlen:** 1 kritischer, 2 hohe, 2 mittlere, 5 niedrige Sicherheitsbefunde; 4 kleinere UX-/Accessibility-Befunde. Autorisierung (`OwnershipVoter`), Credential-Verschlüsselung (`Encryptor`), 2FA-Throttling, CSRF und SQL-Parametrisierung wurden geprüft und sind sauber. + +### Methodik und Grenzen dieses Reviews + +Geprüft wurde statisch (Code, Templates, JS, Deployment-Config) plus die anonym erreichbaren Antworten der Live-Instanz `https://mail.vpn.cpy-pst.de/`. + +Zwei Einschränkungen, die für die Bewertung der UX relevant sind: + +- **Claude for Chrome stand in dieser Session nicht zur Verfügung.** Es waren keine Browser-Tools geladen, nur `WebFetch`. Der Frontend-Teil beruht daher auf Templates, Stimulus-Controllern und der anonym ausgelieferten Login-Seite — nicht auf tatsächlicher Bedienung. +- **Kein Login möglich** (keine Zugangsdaten). Alles hinter der Anmeldung — Inbox, Compose, Kalender, Admin — ist ausschließlich aus dem Code beurteilt. Aussagen zu Laufzeitverhalten, Ladezeiten und tatsächlicher Tastaturbedienung fehlen deshalb bewusst. + +Die Sicherheitsbefunde sind davon nicht betroffen: alle sind am Code vollständig nachvollzogen und mit Datei- und Zeilenangaben belegt. + +--- + +## 1a. Bearbeitungsstand + +Gepflegt beim Abarbeiten — eine Zeile pro Befund, mit dem Commit, der ihn +schließt. Ein Befund gilt erst als erledigt, wenn ein Test ihn festhält; „behoben" +ohne Test heißt hier „behoben, bis es jemand versehentlich zurückdreht". + +| ID | Stand | Commit | Anmerkung | +|---|---|---|---| +| **S-01** | ✅ behoben | `016d9f3` | Alle drei empfohlenen Stellen, plus ein Sanitizer-Profil fürs Verfassen, das der Review nicht vorgesehen hatte — siehe unten. | +| **S-02** | ⬜ offen | | | +| **S-03** | ⬜ offen | | | +| **S-04** | ⬜ offen | | | +| **S-05** | ⬜ offen | | | +| **S-06** | ⬜ offen | | | +| **S-07** | ⬜ offen | | | +| **S-08** | ⬜ offen | | | +| **S-09** | ⬜ offen | | | +| **S-10** | ⬜ offen | | | +| **U-01** | ⬜ offen | | | +| **U-02** | ⬜ offen | | Sicherheitshälfte ist mit S-01 Punkt 2 erledigt; offen ist die UX-Hälfte (Paste-Normalisierung). | +| **U-03** | ⬜ offen | | | +| **U-04** | ⬜ offen | | | + +### Nachtrag zu S-01 — zwei Fallen, die die empfohlene Behebung so nicht überlebt hätte + +Die Analyse des Reviews stimmt vollständig; die vorgeschlagene Korrektur hätte +in Punkt 2 zwei Dinge kaputt gemacht, die beide von vorhandenen Tests gehalten +wurden. + +**Die Allow-Liste verwirft jedes `data-`-Attribut.** Das ist für fremdes HTML +richtig und für einen selbst verfassten Text zerstörerisch: `data-quoted` ist, +woran das Zitat gefunden wird — zum Einklappen, und um es aus dem Snippet zu +schneiden —, `data-pl-signature` ist, was ein Absenderwechsel ersetzt, und +`data-cid` ist die gesamte Brücke zwischen einem Inline-Bild im Editor und der +`cid:`-Referenz, die auf die Leitung geht. `sanitizeFragment()` auf `bodyHtml` +in `markAsDraft()` hätte den Body sicher und den Composer kaputt hinterlassen. +Deshalb ein eigenes Profil (`sanitizeComposedBody()`) mit genau diesen fünf +Markern — geschlossene Liste, nicht `data-*` pauschal. + +**`HtmlSanitizerConfig` ist unveränderlich.** Jede `allow*`-Methode liefert +einen Klon. Ein `$config->allowAttribute(...)` als Anweisung geschrieben +konfiguriert eine Kopie, die anschließend verfällt — es sieht aus wie eine +funktionierende Allow-Liste und entfernt alles, was sie erlauben sollte. + +**Und der Sanitizer maskiert `@` zu `@`.** In einem Attributwert ist das +korrektes HTML und im Browser folgenlos. Für eine `cid:`-Referenz ist es fatal: +`Symfony\Component\Mime\Email::prepareParts()` verbindet einen eingebetteten +Teil mit dem Body über einen Vergleich der *Zeichenkette* `cid:`, und +`InlineAttachmentDetector` entscheidet beim Ingest genauso. Keiner von beiden +parst HTML. Ohne Gegenmaßnahme wäre jede gesendete Mail mit Inline-Bild mit +einem kaputten Bild rausgegangen — der Befund wäre geschlossen und ein +Sendefehler eingebaut gewesen. `restoreCidReferences()` stellt diese Referenzen +wieder her, und nur dort, wo die dekodierte Fassung noch wie eine Referenz +aussieht: einen Attributwert allgemein zu dekodieren wäre ein Weg aus ihm +heraus. + +--- + +## 2. Befundübersicht + +| ID | Schwere | Titel | Ort | +|---|---|---|---| +| **S-01** | 🔴 **Kritisch** | Stored XSS: Antworten/Weiterleiten zitiert **unsanitisiertes** `bodyHtml` in den App-Origin | `ReplyDraftBuilder.php:213` | +| **S-02** | 🟠 **Hoch** | Stored XSS: SVG-Anhang wird inline ausgeliefert, per `cid:`-Link aus der Mail erreichbar | `AttachmentController.php:68` | +| **S-03** | 🟠 **Hoch** | Keine CSP auf dem Anwendungsdokument — Verstärker für S-01/S-02 | global | +| **S-04** | 🟡 **Mittel** | File-Picker-Preview liefert fremden MIME-Typ ohne `Content-Disposition` inline aus | `FilePickerController.php:134` | +| **S-05** | 🟡 **Mittel** | Anwendung liefert keinerlei Security-Header aus; Schutz hängt am Proxy des Betreibers | `frankenphp/Caddyfile` | +| **S-06** | 🟢 Niedrig | Mercure-Default `subscribe: '*'` als latente Fehlerquelle | `config/packages/mercure.yaml:9` | +| **S-07** | 🟢 Niedrig | Signierte Image-Proxy-URLs laufen nie ab | `ImageProxySigner.php:43` | +| **S-08** | 🟢 Niedrig | Gleiches Inline-Muster wie S-02 im JMAP-Download (latent) | `JmapDownloadController.php:82` | +| **S-09** | 🟢 Niedrig | `/install` — Trust-on-first-use-Fenster beim Erststart | `InstallGuard.php:27` | +| **S-10** | ℹ️ Info | Kein Rate-Limit auf der JMAP-Authentifizierung | `security.yaml` (Firewall `jmap`) | +| **U-01** | 🟡 UX | 4 Accessibility-Lücken (Icon-Buttons ohne Namen, `` ohne `alt`) | Templates | +| **U-02** | 🟢 UX | Eingefügtes HTML im Composer wird nicht bereinigt — geht so auf die Leitung | `compose_controller.js:3077` | +| **U-03** | 🟢 UX | 9 unübersetzte deutsche Strings | `translations/messages.de.yaml` | +| **U-04** | ℹ️ UX | `en_PI` fehlen 218 Keys (Fallback auf Englisch) | `translations/messages.en_PI.yaml` | + +--- + +## 3. Sicherheitsbefunde im Detail + +### 🔴 S-01 — Stored XSS über zitierten Mail-Inhalt beim Antworten + +**Ort:** [`src/Service/Mail/ReplyDraftBuilder.php:213`](src/Service/Mail/ReplyDraftBuilder.php:213), ausgegeben in [`templates/compose/_window.html.twig:487`](templates/compose/_window.html.twig:487) + +#### Das Problem + +`Message` trägt zwei Body-Felder, und der Unterschied ist die gesamte Sicherheitsgrenze der Anwendung: + +- `bodyHtml` — das **rohe** HTML des Absenders, unverändert wie empfangen +- `bodyHtmlSafe` — die von `MailBodySanitizer` bereinigte Fassung + +Der Lesepfad benutzt korrekt `bodyHtmlSafe` und rendert es zusätzlich in einem sandboxed iframe. `ReplyDraftBuilder::quote()` greift jedoch auf das rohe Feld zu: + +```php +// src/Service/Mail/ReplyDraftBuilder.php:213 +$html = trim($original->bodyHtml ?? ''); // ← roh, nicht bodyHtmlSafe +$text = trim($original->bodyText ?? ''); +$body = '' !== $html ? $html : nl2br(htmlspecialchars($text, ENT_QUOTES, 'UTF-8')); +``` + +`$body` wird unverändert in den Entwurf eingebettet (`$draft->bodyHtml = $lead . $this->quote(...)`, Zeile 142). Das Compose-Fenster gibt dieses Feld dann per `|raw` in ein `contenteditable`-Div aus — **im Hauptdokument der Anwendung**, nicht im iframe: + +```twig +{# templates/compose/_window.html.twig:487 #} +empty:before:pointer-events-none">{{ form.bodyHtml.vars.value|raw ?: '' }} +``` + +`DraftPersister::markAsDraft()` ruft zwar `$this->bodySanitizer->sanitize($message)` auf ([`DraftPersister.php:154`](src/Service/Mail/DraftPersister.php:154)) — aber `MailBodySanitizer::sanitize()` schreibt sein Ergebnis nach `bodyHtmlSafe` und lässt `bodyHtml` unangetastet ([`MailBodySanitizer.php:72`](src/Service/Mail/MailBodySanitizer.php:72)). Der Sanitizer läuft also, rettet aber genau das Feld nicht, das ausgegeben wird. + +#### Angriffskette + +1. Angreifer sendet eine Mail mit `` im HTML-Body an eine plMail-Adresse. +2. Ingest speichert das rohe HTML in `bodyHtml` (`MessageSyncer.php:487`, `GmailMessageBuilder.php:159`, `GraphMessageBuilder.php:194`), die bereinigte Fassung in `bodyHtmlSafe`. +3. Das Opfer **liest** die Mail — sicher, die Sandbox greift. +4. Das Opfer klickt **Antworten** (`GET /compose/reply/{id}`, [`ComposeController.php:136`](src/Controller/Mail/ComposeController.php:136)). +5. Das rohe HTML landet über `quote()` im Entwurf und wird per `|raw` ins Hauptdokument geschrieben. +6. `onerror` feuert im Origin der Anwendung — mit Session, ohne CSP (siehe S-03). + +Reply, Reply-All und Forward sind alle betroffen (`ComposeController.php:136`, `:152`, `:168`). + +#### Auswirkung + +Vollständige Übernahme der Sitzung im Kontext der Anwendung. Konkret erreichbar: sämtliche Mail lesen und exfiltrieren, ein **App-Passwort erzeugen** (`ApiToken`) und damit dauerhaften JMAP-Zugriff etablieren, der ein Passwort-Reset überlebt, sowie eine Weiterleitungsregel anlegen. Der Angriff braucht keinen zweiten Klick über „Antworten" hinaus — eine Handlung, die bei einer Mail, die eine Antwort provoziert, der Normalfall ist. + +Das `HttpOnly`-Flag auf `PHPSESSID` verhindert das Auslesen des Cookies, nicht aber Aktionen im Namen des Nutzers. + +#### Behebung + +Drei Stellen, gestaffelt — die erste behebt den Befund, die anderen beiden verhindern die Wiederkehr: + +**1. Zitat aus der bereinigten Fassung bauen** (`ReplyDraftBuilder`): + +```php +public function __construct( + private readonly SignatureProvider $signatures, + private readonly MailBodySanitizer $sanitizer, // neu +) {} + +// in quote(): +$html = $this->sanitizer->sanitizeFragment($original->bodyHtml); +``` + +`sanitizeFragment()` existiert bereits und wird für Signaturen genau zu diesem Zweck verwendet ([`MailBodySanitizer.php:95`](src/Service/Mail/MailBodySanitizer.php:95)). `bodyHtmlSafe` direkt zu nehmen wäre die Alternative, ist aber schlechter: dort sind `cid:`-Referenzen bereits in App-URLs umgeschrieben, was den Umweg über `InlineImageRewriter::toCid()` beim Speichern nötig macht. + +**2. `bodyHtml` beim Persistieren bereinigen** (`DraftPersister::markAsDraft()`) — deckt zusätzlich U-02 ab, also alles, was über Paste oder einen manipulierten POST in den Composer gelangt, und stellt sicher, dass die **versendete** Mail kein Script trägt: + +```php +$message->bodyHtml = $this->bodySanitizer->sanitizeFragment($message->bodyHtml); +$this->bodySanitizer->sanitize($message); +``` + +**3. Ausgabe absichern.** Das `|raw` in `_window.html.twig:487` sollte nicht auf einem Feld stehen, dessen Sicherheit von einem entfernten Aufrufer abhängt. Sauber wäre eine Twig-Funktion analog zu `message_render()` — der Kommentar in `MessageRender` beschreibt genau dieses Prinzip („Exists so that no Twig file has to ask a security question") und sollte auch hier gelten. + +**Zusätzlich:** `JmapDraftWriter` baut Entwürfe für Nicht-Browser-Clients und ist laut eigenem Docblock eine bereits abgedriftete Kopie dieser Logik — bei der Korrektur mitprüfen. + +--- + +### 🟠 S-02 — Stored XSS über SVG-Anhang + +**Ort:** [`src/Controller/Mail/AttachmentController.php:68`](src/Controller/Mail/AttachmentController.php:68) + +#### Das Problem + +```php +$inlineAllowed = true === str_starts_with($contentType, 'image/'); +``` + +`image/svg+xml` erfüllt dieses Prädikat. SVG ist aber kein passives Bildformat: als Top-Level-Dokument geladen führt es `