From e81df453821a680cfc7865c1fbf6737ffaef4695 Mon Sep 17 00:00:00 2001 From: fds2610-bot Date: Sun, 2 Aug 2026 16:12:47 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20#170:=20Tests=20laufen=20als=20www-data?= =?UTF-8?q?=20und=20r=C3=A4umen=20angelegte=20Dateien=20selbst=20auf?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 16 ++- Changes.md | 10 ++ README.md | 8 +- docs/automatisierung.md | 2 +- scripts/run-tests.sh | 16 ++- scripts/update.sh | 4 +- src/Controllers/BelegController.php | 63 +++++--- tests/19_ImageCropTest.php | 17 +-- tests/33_BankImportParserTest.php | 7 +- tests/34_BankImportDkbTest.php | 7 +- tests/35_BankImportSuggestTest.php | 6 +- tests/49_BelegDeleteUnlinkFailTest.php | 6 +- tests/66_BankUploadPreviewXssTest.php | 3 +- tests/67_BelegXmlXsltXssTest.php | 13 +- tests/70_SettingsLogoUploadTest.php | 2 +- tests/76_DatenverzeichnisUebernahmeTest.php | 4 +- tests/79_TestAufraeumenTest.php | 150 ++++++++++++++++++++ tests/TestCase.php | 106 ++++++++++++++ tests/e2e/README.md | 7 +- tests/run.php | 59 +++++++- 20 files changed, 421 insertions(+), 85 deletions(-) create mode 100644 tests/79_TestAufraeumenTest.php diff --git a/CLAUDE.md b/CLAUDE.md index e33429e..db95548 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,17 +17,31 @@ Tests liegen in `tests/` und folgen dem Namensschema `NN_FunktionsnameTest.php`. Jede Testklasse erweitert `TestCase` und implementiert `run()`. ```bash -php tests/run.php # alle Tests ausführen +docker compose exec -u www-data web php tests/run.php # alle Tests ausführen ./scripts/run-tests.sh # Tests + automatische Issue-Erstellung bei Fehlern ./scripts/run-tests.sh 42 # Tests; bei Fehlern Kommentar in Issue #42 ``` +**Immer `-u www-data`:** ohne die Option laufen die Tests als root und legen +unterhalb von `data/` Dateien an, die Apache/PHP später nicht mehr beschreiben +kann – ein echter Beleg-Upload scheitert dann mit HTTP 500. + ### Neuen Test erstellen 1. Datei `tests/NN_MeinFeatureTest.php` anlegen (NN = nächste freie Nummer) 2. Klasse `MeinFeatureTest extends TestCase` 3. `protected string $testName = 'Test NN: Beschreibung';` 4. Methode `public function run(): void` mit Assertions implementieren 5. Transaktions-Isolation ist automatisch (setUp/tearDown in TestCase) +6. Dateien und Verzeichnisse **immer** über `TestCase` anlegen – der Rollback + greift nur in der Datenbank, nicht im Dateisystem: + +```php +$pfad = $this->tempPath('.csv'); // Pfad in /tmp, wird aufgeräumt +$dir = $this->makeTempDir($basis . '/2026/BLG-...'); // Verzeichnis anlegen + aufräumen +$this->registerTempPath($schonVorhandenerPfad); // nachträglich anmelden +``` + +Bleibt trotzdem etwas liegen, meldet `tests/run.php` das am Ende des Laufs. ### Assertion-Methoden ```php diff --git a/Changes.md b/Changes.md index 9b59c73..aad5de9 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,16 @@ --- +## 2026-08-02 – Testläufe hinterlassen keine Verzeichnisse mehr, die echte Uploads blockieren + +- Die Testsuite lief über `docker compose exec` als *root*, Apache/PHP dagegen als `www-data`. Legte ein Test unterhalb von `data/` ein Verzeichnis an, gehörte es root und war für die Anwendung nicht beschreibbar. Weil der Rollback nur die Datenbank zurückdreht, wurde die im Test vergebene Belegnummer danach erneut vergeben – der nächste echte Upload traf auf das bestehende Verzeichnis, übersprang das `mkdir` und scheiterte in `move_uploaded_file()` mit HTTP 500. In #168 war nur der konkrete Verursacher entschärft worden, nicht die Ursache. +- **Aufrufkonvention:** Die PHP-Suite läuft jetzt mit `docker compose exec -u www-data web php tests/run.php`, also unter derselben Kennung wie die Anwendung. Zurückbleibende Dateien hätten damit die richtigen Rechte und wären unschädlich. Angepasst in `CLAUDE.md`, `README.md`, `docs/automatisierung.md`, `tests/e2e/README.md`, `scripts/update.sh` sowie `scripts/run-tests.sh` – letzteres wechselt selbst per `setpriv` auf `www-data`, wenn es als root im Container startet. +- **`tests/TestCase.php`:** Neue Methoden `registerTempPath()`, `makeTempDir()` und `tempPath()`. Angemeldete Pfade entfernt `tearDown()` rekursiv – auch nach einer Ausnahme, auch bei schreibgeschützten Verzeichnissen und inklusive der von `mkdir` nebenbei angelegten Elternverzeichnisse. Als Sicherheitsnetz wird ausschließlich unterhalb des Temp-, Beleg- und Logoverzeichnisses gelöscht und nie eines dieser Verzeichnisse selbst; alles andere bleibt liegen und wird gemeldet. +- **`tests/run.php`:** Vergleicht den Bestand der Datenverzeichnisse vor und nach dem Lauf und meldet am Ende jeden Rückstand, den ein Test nicht angemeldet hat. Damit fällt ein künftiger Test auf, statt still Leichen zu hinterlassen. +- **`tests/19`, `33`, `34`, `35`, `49`, `66`, `67`, `70`, `76`:** Alle Tests mit Dateisystem-Zugriff auf die zentrale Registrierung umgestellt; die verstreuten `unlink()`/`rmdir()`-Aufrufe und der eigene `tearDown()` in Test 19 entfallen. Test 66 hinterließ zusätzlich die von `tempnam()` angelegte Datei, weil das Suffix erst danach angehängt wurde. +- **`src/Controllers/BelegController.php`:** `upload()` prüft vor dem Speichern, ob das Zielverzeichnis existiert und beschreibbar ist, und wertet den Rückgabewert von `move_uploaded_file()` aus. Statt einer Ausnahme mit HTTP 500 erscheint jetzt eine verständliche Meldung im Formular (bzw. als JSON bei AJAX-Uploads), und die bereits vergebene Belegnummer wird über `Beleg::releaseBelegNr()` wieder freigegeben. +- **`tests/79_TestAufraeumenTest.php`** (neu): Sichert das Aufräumen samt Sicherheitsnetz und die Härtung des Uploads ab. + ## 2026-08-02 – Nummernkreise geben die zuletzt vergebene Nummer beim Löschen wieder frei - Der E2E-Finalisierungstest verbrauchte bei jedem Lauf eine Rechnungs- und eine Belegnummer: Die Testrechnung wurde zwar wieder gelöscht, die Nummern blieben aber vergeben und rissen dauerhaft Lücken in den laufenden Nummernkreis. diff --git a/README.md b/README.md index 8341c17..d093bdd 100644 --- a/README.md +++ b/README.md @@ -355,11 +355,13 @@ Anschließend im Browser unter `http://localhost:8080` (oder dem konfigurierten ## Tests ```bash -docker compose exec web php tests/run.php # PHP-Suite (Models, Services, Berechnungen) -tests/e2e/run.sh # Browser-Tests (Routing, Views, Formular-JS) +docker compose exec -u www-data web php tests/run.php # PHP-Suite (Models, Services, Berechnungen) +tests/e2e/run.sh # Browser-Tests (Routing, Views, Formular-JS) ``` -Die PHP-Suite läuft ohne weitere Voraussetzungen. Die E2E-Tests brauchen Docker, +Die PHP-Suite läuft ohne weitere Voraussetzungen. Das `-u www-data` gehört dazu: +sonst laufen die Tests als root und hinterlassen in `data/` Dateien, die Apache/PHP +später nicht mehr beschreiben kann. Die E2E-Tests brauchen Docker, das [e2e-runner-Projekt](../e2e-runner/) und einen Testnutzer (`docker compose exec web php scripts/e2e-user.php`); fehlt etwas davon, werden sie mit einer Warnung übersprungen. Details in [tests/e2e/README.md](tests/e2e/README.md). diff --git a/docs/automatisierung.md b/docs/automatisierung.md index 995543c..a09bc61 100644 --- a/docs/automatisierung.md +++ b/docs/automatisierung.md @@ -11,7 +11,7 @@ Issues mit dem Label **`automate`** im [Codeberg-Repo](https://codeberg.org/fds2 3. **Klarheitscheck**: Sind die Anforderungen eindeutig? Falls nicht → Rückfragen als Kommentar posten, `automate`-Label entfernen, warten 4. Analyse (Ursache, Lösungsplan, Risiken) 5. Lösung implementieren -6. Tests ausführen (`php tests/run.php`), Fehler beheben +6. Tests ausführen (`docker compose exec -u www-data web php tests/run.php`), Fehler beheben 7. Commit & Push: `Fix #NR: Kurzbeschreibung` 8. Kommentar mit Zusammenfassung im Issue posten 9. Labels aktualisieren, Issue offen lassen diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh index 3417fed..5bc2120 100755 --- a/scripts/run-tests.sh +++ b/scripts/run-tests.sh @@ -26,14 +26,24 @@ API_BASE="https://codeberg.org/api/v1" cd "$PROJECT_DIR" +# ── PHP als www-data ausführen ─────────────────────────────── +# Als root angelegte Dateien unterhalb von data/ sind für Apache/PHP (www-data) +# nicht beschreibbar und blockieren später echte Uploads. Deshalb laufen die +# Tests mit derselben Kennung wie die Anwendung. +if [ "$(id -u)" = "0" ] && id www-data >/dev/null 2>&1; then + PHP="setpriv --reuid=www-data --regid=www-data --clear-groups php" +else + PHP="php" +fi + # ── Tests ausführen ────────────────────────────────────────── echo "" echo "Führe Tests aus..." -JSON_OUTPUT=$(php tests/run.php --json 2>&1) || true +JSON_OUTPUT=$($PHP tests/run.php --json 2>&1) || true EXIT_CODE=$? # Auch die lesbare Ausgabe zeigen -php tests/run.php || true +$PHP tests/run.php || true # ── Ergebnis auswerten ─────────────────────────────────────── FAILED=$(echo "$JSON_OUTPUT" | python3 -c "import json,sys; d=json.load(sys.stdin); print(d.get('failed',0))" 2>/dev/null || echo "0") @@ -80,7 +90,7 @@ while IFS= read -r failure; do fi BODY+="### Schritte zur Reproduktion\n" - BODY+="1. \`php tests/run.php\` ausführen\n" + BODY+="1. \`docker compose exec -u www-data web php tests/run.php\` ausführen\n" BODY+="2. Fehler tritt im oben genannten Test auf\n\n" BODY+="_Dieser Issue wurde automatisch durch den Test-Runner erstellt._\n" BODY+="_Das Label 'automate' wurde bewusst NICHT gesetzt (Human-in-the-loop)._" diff --git a/scripts/update.sh b/scripts/update.sh index 48ed885..a485895 100755 --- a/scripts/update.sh +++ b/scripts/update.sh @@ -359,7 +359,7 @@ if [[ "$OPT_NO_TESTS" == "true" ]]; then warn "Tests übersprungen (--no-tests)" else info "Führe Test-Suite aus..." - if TEST_OUTPUT=$(docker compose exec -T web php /var/www/html/tests/run.php 2>&1); then + if TEST_OUTPUT=$(docker compose exec -T -u www-data web php /var/www/html/tests/run.php 2>&1); then TEST_EXIT=0 else TEST_EXIT=$? @@ -369,7 +369,7 @@ else if [[ "$TEST_EXIT" -ne 0 ]]; then warn "Einige Tests sind fehlgeschlagen – bitte prüfen" - warn "Vollständiger Testlauf: docker compose exec web php tests/run.php" + warn "Vollständiger Testlauf: docker compose exec -u www-data web php tests/run.php" else ok "Alle Tests bestanden" fi diff --git a/src/Controllers/BelegController.php b/src/Controllers/BelegController.php index 2f962ed..aa6a92f 100644 --- a/src/Controllers/BelegController.php +++ b/src/Controllers/BelegController.php @@ -133,20 +133,7 @@ class BelegController extends BaseController } if ($errors) { - if (($_SERVER['HTTP_X_REQUESTED_WITH'] ?? '') === 'XMLHttpRequest') { - http_response_code(422); - header('Content-Type: application/json'); - echo json_encode(['error' => implode(' ', $errors)]); - return; - } - $view = new View(); - $view->render('layout/base.php', [ - 'pageTitle' => 'Beleg hochladen', - 'content' => $this->renderView('belege/upload', [ - 'errors' => $errors, - 'old' => $_POST, - ]), - ]); + $this->renderUploadErrors($errors); return; } @@ -163,14 +150,33 @@ class BelegController extends BaseController // Store under uploads/{year}/{belegNr}/original.{ext} $relDir = "{$year}/{$belegNr}"; $absDir = Config::get('upload_dir') . "/{$relDir}"; - if (!is_dir($absDir)) mkdir($absDir, 0755, true); + if (!is_dir($absDir)) @mkdir($absDir, 0755, true); + + // Ein bereits vorhandenes, aber nicht beschreibbares Zielverzeichnis (etwa + // ein Rückstand aus einem Testlauf mit anderem Nutzer) ließ move_uploaded_file() + // sonst mit einer Ausnahme und HTTP 500 auflaufen. + if (!is_dir($absDir) || !is_writable($absDir)) { + Beleg::releaseBelegNr($belegNr); + $this->renderUploadErrors([ + 'file' => "Das Zielverzeichnis {$relDir} ist nicht beschreibbar. " + . 'Bitte die Schreibrechte im Belegverzeichnis prüfen.', + ]); + return; + } $filename = pathinfo($file['name'], PATHINFO_BASENAME); $filenameWithoutExt = pathinfo($file['name'], PATHINFO_FILENAME); $storagePath = "{$relDir}/original.{$ext}"; $absPath = Config::get('upload_dir') . "/{$storagePath}"; - move_uploaded_file($file['tmp_name'], $absPath); + if (!move_uploaded_file($file['tmp_name'], $absPath)) { + Beleg::releaseBelegNr($belegNr); + $this->renderUploadErrors([ + 'file' => 'Die hochgeladene Datei konnte nicht gespeichert werden. ' + . 'Bitte die Schreibrechte im Belegverzeichnis prüfen.', + ]); + return; + } // Weißen Rand bei JPEG/PNG automatisch wegschneiden if (in_array($mime, ['image/jpeg', 'image/png'], true)) { @@ -202,6 +208,31 @@ class BelegController extends BaseController View::redirect("/belege/{$id}", "Beleg {$belegNr} erfolgreich hochgeladen."); } + /** + * Zeigt das Upload-Formular erneut mit Fehlermeldungen an – als JSON, wenn + * der Upload per AJAX kam. + * + * @param array $errors + */ + private function renderUploadErrors(array $errors): void + { + if (($_SERVER['HTTP_X_REQUESTED_WITH'] ?? '') === 'XMLHttpRequest') { + http_response_code(422); + header('Content-Type: application/json'); + echo json_encode(['error' => implode(' ', $errors)]); + return; + } + + $view = new View(); + $view->render('layout/base.php', [ + 'pageTitle' => 'Beleg hochladen', + 'content' => $this->renderView('belege/upload', [ + 'errors' => $errors, + 'old' => $_POST, + ]), + ]); + } + public function show(array $params): void { $beleg = Beleg::find((int)$params['id']); diff --git a/tests/19_ImageCropTest.php b/tests/19_ImageCropTest.php index 85337c6..e52962e 100644 --- a/tests/19_ImageCropTest.php +++ b/tests/19_ImageCropTest.php @@ -13,9 +13,6 @@ class ImageCropTest extends TestCase { protected string $testName = 'Test 19: ImageCropService – Weißrand-Zuschnitt'; - /** @var string[] */ - private array $tmpFiles = []; - public function run(): void { $this->testCropJpeg(); @@ -118,9 +115,8 @@ class ImageCropTest extends TestCase private function testUnsupportedMimeReturnsFalse(): void { // Erstellt eine Dummy-Textdatei – MIME-Typ wird nicht als JPEG/PNG erkannt - $path = sys_get_temp_dir() . '/crop_test_' . uniqid() . '.txt'; + $path = $this->tempPath('.txt'); file_put_contents($path, 'kein Bild'); - $this->tmpFiles[] = $path; $service = new \Steuerkram\Services\ImageCropService(); $result = $service->cropWhitespace($path); @@ -132,8 +128,7 @@ class ImageCropTest extends TestCase private function saveTmp(\GdImage $img, string $type): string { - $path = sys_get_temp_dir() . '/crop_test_' . uniqid() . '.' . $type; - $this->tmpFiles[] = $path; + $path = $this->tempPath('.' . $type); if ($type === 'jpeg') { imagejpeg($img, $path, 92); @@ -143,12 +138,4 @@ class ImageCropTest extends TestCase return $path; } - - public function tearDown(): void - { - foreach ($this->tmpFiles as $f) { - if (file_exists($f)) unlink($f); - } - parent::tearDown(); - } } diff --git a/tests/33_BankImportParserTest.php b/tests/33_BankImportParserTest.php index f773705..eeb277d 100644 --- a/tests/33_BankImportParserTest.php +++ b/tests/33_BankImportParserTest.php @@ -14,7 +14,7 @@ class BankImportParserTest extends TestCase private function makeTempCsv(string $content): string { - $path = sys_get_temp_dir() . '/test33_' . uniqid() . '.csv'; + $path = $this->tempPath('.csv'); file_put_contents($path, $content); return $path; } @@ -53,7 +53,6 @@ class BankImportParserTest extends TestCase $tmp = $this->makeTempCsv($csv); $rows = $parser->parse($tmp, 'csv', $fmt); - unlink($tmp); $this->assertEquals(2, count($rows), 'Normales CSV: 2 Buchungen geparsed'); $this->assertEquals('2026-05-15', $rows[0]['buchungstag'], 'Buchungstag korrekt geparst'); @@ -72,7 +71,6 @@ class BankImportParserTest extends TestCase $tmp2 = $this->makeTempCsv($csvMitLeeren); $rows2 = $parser->parse($tmp2, 'csv', $fmt); - unlink($tmp2); $this->assertEquals(2, count($rows2), 'Leerzeilen werden übersprungen (2 von 3 Zeilen)'); @@ -83,7 +81,6 @@ class BankImportParserTest extends TestCase $tmp3 = $this->makeTempCsv($csvLeer); $rows3 = $parser->parse($tmp3, 'csv', $fmt); - unlink($tmp3); $this->assertEquals(0, count($rows3), 'Nur-Header-CSV: leeres Ergebnis'); @@ -103,7 +100,6 @@ class BankImportParserTest extends TestCase $tmp4 = $this->makeTempCsv($csvBom); $rows4 = $parser->parse($tmp4, 'csv', $fmt); - unlink($tmp4); $this->assertEquals(1, count($rows4), 'CSV mit UTF-8-BOM: 1 Buchung geparst'); $this->assertEquals(-99.00, $rows4[0]['wert'], 'Betrag trotz BOM korrekt geparst'); @@ -115,7 +111,6 @@ class BankImportParserTest extends TestCase $tmp5 = $this->makeTempCsv($csvTausend); $rows5 = $parser->parse($tmp5, 'csv', $fmt); - unlink($tmp5); $this->assertEquals(1, count($rows5), 'CSV mit Tausendertrenner: 1 Buchung'); $this->assertEquals(-1234.56, $rows5[0]['wert'], 'Betrag -1.234,56 korrekt geparst'); diff --git a/tests/34_BankImportDkbTest.php b/tests/34_BankImportDkbTest.php index 92537e9..5ae1678 100644 --- a/tests/34_BankImportDkbTest.php +++ b/tests/34_BankImportDkbTest.php @@ -11,7 +11,7 @@ class BankImportDkbTest extends TestCase private function makeTempCsv(string $content): string { - $path = sys_get_temp_dir() . '/test34_' . uniqid() . '.csv'; + $path = $this->tempPath('.csv'); file_put_contents($path, $content); return $path; } @@ -55,7 +55,6 @@ class BankImportDkbTest extends TestCase $tmp = $this->makeTempCsv($csv); $rows = $parser->parse($tmp, 'csv', $fmt); - unlink($tmp); $this->assertEquals(1, count($rows), 'DKB negativer Betrag: 1 Buchung geparst'); $this->assertEquals('2026-05-15', $rows[0]['buchungstag'], 'Buchungstag korrekt'); @@ -75,7 +74,6 @@ class BankImportDkbTest extends TestCase $tmp2 = $this->makeTempCsv($csv2); $rows2 = $parser->parse($tmp2, 'csv', $fmt); - unlink($tmp2); $this->assertEquals(1, count($rows2), 'DKB positiver Betrag: 1 Buchung geparst'); $this->assertEquals(1500.00, $rows2[0]['wert'], 'Betrag 1.500,00 korrekt'); @@ -92,7 +90,6 @@ class BankImportDkbTest extends TestCase $tmp3 = $this->makeTempCsv($csv3); $rows3 = $parser->parse($tmp3, 'csv', $fmt); - unlink($tmp3); $this->assertEquals(1, count($rows3), 'DKB Betrag 0: 1 Buchung geparst'); $this->assertEquals( @@ -110,7 +107,6 @@ class BankImportDkbTest extends TestCase $tmp5 = $this->makeTempCsv($csv5); $rows5 = $parser->parse($tmp5, 'csv', $fmtUnknown); - unlink($tmp5); $this->assertEquals('Test: Ende', $rows5[0]['buchungstext'], 'Unbekannter Placeholder wird als leerer String aufgelöst'); @@ -137,7 +133,6 @@ class BankImportDkbTest extends TestCase $tmp4 = $this->makeTempCsv($csv4); $rows4 = $parser->parse($tmp4, 'csv', $fmtStd); - unlink($tmp4); $this->assertEquals('Normaler Text', $rows4[0]['buchungstext'], 'Format ohne Template: col_buchungstext direkt genutzt'); diff --git a/tests/35_BankImportSuggestTest.php b/tests/35_BankImportSuggestTest.php index aeeae35..c97bf50 100644 --- a/tests/35_BankImportSuggestTest.php +++ b/tests/35_BankImportSuggestTest.php @@ -13,7 +13,7 @@ class BankImportSuggestTest extends TestCase private function makeTempCsv(string $content): string { - $path = sys_get_temp_dir() . '/test35_' . uniqid() . '.csv'; + $path = $this->tempPath('.csv'); file_put_contents($path, $content); return $path; } @@ -45,8 +45,6 @@ class BankImportSuggestTest extends TestCase $this->assert(isset($result['mapping']['col_wertstellung']), 'col_wertstellung gemappt'); $this->assert(isset($result['mapping']['col_wert']), 'col_wert gemappt (Betrag)'); - unlink($file); - // CSV ohne Banking-Keywords → kein Vorschlag $csvOhneKeywords = implode("\n", [ 'Foo;Bar;Baz', @@ -55,7 +53,6 @@ class BankImportSuggestTest extends TestCase $file2 = $this->makeTempCsv($csvOhneKeywords); $result2 = $parser->suggestFormat($file2, 'csv'); $this->assert($result2 === null, 'suggestFormat() gibt null zurück wenn kein Banking-Header erkannt'); - unlink($file2); // Comdirect-ähnliche CSV (kein skip_rows) $csvComdirect = implode("\n", [ @@ -66,6 +63,5 @@ class BankImportSuggestTest extends TestCase $result3 = $parser->suggestFormat($file3, 'csv'); $this->assert($result3 !== null, 'suggestFormat() erkennt Comdirect-ähnliche CSV'); $this->assertEquals(0, $result3['skip_rows'], 'skip_rows=0 bei direktem Header'); - unlink($file3); } } diff --git a/tests/49_BelegDeleteUnlinkFailTest.php b/tests/49_BelegDeleteUnlinkFailTest.php index fb7947f..c8d5511 100644 --- a/tests/49_BelegDeleteUnlinkFailTest.php +++ b/tests/49_BelegDeleteUnlinkFailTest.php @@ -10,10 +10,9 @@ class BelegDeleteUnlinkFailTest extends TestCase public function run(): void { - $dir = \Steuerkram\Config::get('upload_dir') . '/test_49_unlink_fail'; + $dir = $this->makeTempDir(\Steuerkram\Config::get('upload_dir') . '/test_49_unlink_fail'); $file = $dir . '/original.pdf'; - mkdir($dir, 0755, true); file_put_contents($file, 'dummy'); // Schreibschutz auf das Verzeichnis: unlink() der Datei schlägt fehl (Permission denied), // genau wie im gemeldeten Fehler auf dem Produktiv-Server. @@ -34,9 +33,8 @@ class BelegDeleteUnlinkFailTest extends TestCase $this->assert($result === true, 'delete() liefert true, obwohl unlink() fehlschlägt'); $this->assert(Beleg::find($id) === false, 'Beleg-Datensatz wurde trotz Datei-Fehler aus der DB gelöscht'); } finally { + // Der Schreibschutz muss weg, bevor tearDown() das Verzeichnis abräumt chmod($dir, 0755); - if (file_exists($file)) unlink($file); - if (is_dir($dir)) rmdir($dir); } } } diff --git a/tests/66_BankUploadPreviewXssTest.php b/tests/66_BankUploadPreviewXssTest.php index 553d695..b045db3 100644 --- a/tests/66_BankUploadPreviewXssTest.php +++ b/tests/66_BankUploadPreviewXssTest.php @@ -43,10 +43,9 @@ class BankUploadPreviewXssTest extends TestCase } $script = $m[0] . "\nconsole.log(esc('Spalte\" onmouseover=\"alert(1)'));"; - $tmp = tempnam(sys_get_temp_dir(), 'esc_test_') . '.js'; + $tmp = $this->tempPath('.js'); file_put_contents($tmp, $script); $output = shell_exec(escapeshellarg($node) . ' ' . escapeshellarg($tmp) . ' 2>&1'); - @unlink($tmp); $this->assert( $output !== null && !str_contains($output, '" onmouseover="'), diff --git a/tests/67_BelegXmlXsltXssTest.php b/tests/67_BelegXmlXsltXssTest.php index 22cefd7..c63141c 100644 --- a/tests/67_BelegXmlXsltXssTest.php +++ b/tests/67_BelegXmlXsltXssTest.php @@ -15,10 +15,7 @@ class BelegXmlXsltXssTest extends TestCase $belegNr = \Steuerkram\Models\Beleg::generateBelegNr(); $year = date('Y'); $relDir = "{$year}/{$belegNr}"; - $absDir = \Steuerkram\Config::get('upload_dir') . "/{$relDir}"; - if (!is_dir($absDir)) { - mkdir($absDir, 0755, true); - } + $absDir = $this->makeTempDir(\Steuerkram\Config::get('upload_dir') . "/{$relDir}"); $storagePath = "{$relDir}/original.xml"; $absPath = \Steuerkram\Config::get('upload_dir') . "/{$storagePath}"; @@ -88,13 +85,5 @@ class BelegXmlXsltXssTest extends TestCase (bool)preg_match("/Content-Disposition:\s*'\s*\.\s*\\\$disposition/", $serveFile), 'Der Content-Disposition-Header nutzt die ermittelte Disposition (nicht fest inline)' ); - - // Datei UND Verzeichnis entfernen: Die Transaktion rollt die Belegnummer - // zurück, das Dateisystem aber nicht. Ein zurückbleibendes Verzeichnis - // gehört – weil die Suite via `docker compose exec` als root läuft – dem - // Nutzer root und blockiert später den echten Upload mit derselben - // Nummer (move_uploaded_file: Permission denied → HTTP 500). - @unlink($absPath); - @rmdir($absDir); } } diff --git a/tests/70_SettingsLogoUploadTest.php b/tests/70_SettingsLogoUploadTest.php index e968a7e..f6a8c7c 100644 --- a/tests/70_SettingsLogoUploadTest.php +++ b/tests/70_SettingsLogoUploadTest.php @@ -49,7 +49,7 @@ class SettingsLogoUploadTest extends TestCase // Bewusst ein eigener Testdateiname – deleteLogo() löscht die Datei // physisch, ein echtes logo.png darf davon nie getroffen werden. $testFile = 'logo-test-' . getmypid() . '.png'; - $testPath = Config::get('logo_dir') . '/' . $testFile; + $testPath = $this->registerTempPath(Config::get('logo_dir') . '/' . $testFile); Setting::setLogo($testFile); $this->assertEquals($testFile, Setting::get()['logo_path'], 'setLogo() speichert nur den Dateinamen'); diff --git a/tests/76_DatenverzeichnisUebernahmeTest.php b/tests/76_DatenverzeichnisUebernahmeTest.php index f7e8703..ae52ebf 100644 --- a/tests/76_DatenverzeichnisUebernahmeTest.php +++ b/tests/76_DatenverzeichnisUebernahmeTest.php @@ -44,7 +44,7 @@ class DatenverzeichnisUebernahmeTest extends TestCase ); // ── Funktionaler Lauf: Ziel enthält bereits ein Jahresverzeichnis ────── - $basis = sys_get_temp_dir() . '/test76_' . bin2hex(random_bytes(6)); + $basis = $this->tempPath(); $alt = $basis . '/uploads'; $neu = $basis . '/belege'; $logo = $basis . '/logo'; @@ -109,7 +109,5 @@ class DatenverzeichnisUebernahmeTest extends TestCase exec('sh ' . escapeshellarg($skript) . ' ' . escapeshellarg($leer) . ' ' . escapeshellarg($neu) . ' ' . escapeshellarg($logo) . ' 2>&1', $zweite, $zweiterExit); $this->assertEquals(0, $zweiterExit, 'Lauf ohne Altbestand endet mit Exitcode 0'); - - exec('rm -rf ' . escapeshellarg($basis)); } } diff --git a/tests/79_TestAufraeumenTest.php b/tests/79_TestAufraeumenTest.php new file mode 100644 index 0000000..ba7cf47 --- /dev/null +++ b/tests/79_TestAufraeumenTest.php @@ -0,0 +1,150 @@ +testAufraeumen(); + $this->testFremdePfadeBleiben(); + $this->testTearDownRaeumtAuf(); + $this->testUploadHaertung(); + } + + private function testAufraeumen(): void + { + $probe = new AufraeumProbe(); + + $tmpDatei = $probe->tempPathOeffentlich('.csv'); + file_put_contents($tmpDatei, 'inhalt'); + + $belegDir = $probe->makeTempDirOeffentlich( + \Steuerkram\Config::get('upload_dir') . '/2099/BLG-2099-TEST79' + ); + file_put_contents($belegDir . '/original.pdf', 'dummy'); + mkdir($belegDir . '/unterordner'); + file_put_contents($belegDir . '/unterordner/datei.txt', 'tief'); + + // Schreibgeschütztes Verzeichnis (wie in Test 49) muss trotzdem verschwinden + $gesperrt = $probe->makeTempDirOeffentlich( + \Steuerkram\Config::get('upload_dir') . '/test_79_gesperrt' + ); + file_put_contents($gesperrt . '/original.pdf', 'dummy'); + chmod($gesperrt, 0500); + + $this->assert(is_file($tmpDatei), 'Temporäre Datei wurde angelegt'); + $this->assert(is_dir($belegDir), 'Belegverzeichnis wurde angelegt'); + + $probe->cleanupOeffentlich(); + + $this->assert(!file_exists($tmpDatei), 'Temporäre Datei ist nach dem Aufräumen weg'); + $this->assert(!is_dir($belegDir), 'Belegverzeichnis ist samt Inhalt weg'); + $this->assert(!is_dir($gesperrt), 'Auch ein schreibgeschütztes Verzeichnis wird entfernt'); + $this->assert( + is_dir((string)\Steuerkram\Config::get('upload_dir')), + 'Das Belegverzeichnis selbst bleibt bestehen' + ); + $this->assert( + !is_dir((string)\Steuerkram\Config::get('upload_dir') . '/2099'), + 'Das leere Jahresverzeichnis bleibt nicht als Rückstand liegen' + ); + } + + private function testFremdePfadeBleiben(): void + { + // Sicherheitsnetz: außerhalb von Temp-, Beleg- und Logoverzeichnis wird + // nichts gelöscht – ein Tippfehler im Testpfad darf keine Daten kosten. + $probe = new AufraeumProbe(); + $fremd = dirname(__DIR__) . '/test79_fremd_' . bin2hex(random_bytes(4)) . '.txt'; + file_put_contents($fremd, 'nicht anfassen'); + $probe->registerTempPathOeffentlich($fremd); + + ob_start(); + $probe->cleanupOeffentlich(); + $meldung = (string)ob_get_clean(); + + $this->assert(is_file($fremd), 'Pfad außerhalb der Testverzeichnisse bleibt unangetastet'); + $this->assert(str_contains($meldung, 'Nicht aufgeräumt'), 'Der übersprungene Pfad wird gemeldet'); + unlink($fremd); + } + + private function testTearDownRaeumtAuf(): void + { + $quelltext = (string)file_get_contents(__DIR__ . '/TestCase.php'); + $this->assert( + (bool)preg_match('/function tearDown\(\).*?cleanupTempPaths\(\)/s', $quelltext), + 'tearDown() ruft das Aufräumen auf – auch wenn der Test mit einer Ausnahme abbricht' + ); + + $runner = (string)file_get_contents(__DIR__ . '/run.php'); + $this->assert( + str_contains($runner, 'datenBestand()') && str_contains($runner, 'registerTempPath'), + 'run.php meldet Rückstände, die ein Test nicht angemeldet hat' + ); + } + + private function testUploadHaertung(): void + { + $controller = (string)file_get_contents(dirname(__DIR__) . '/src/Controllers/BelegController.php'); + + $this->assert( + str_contains($controller, 'is_writable($absDir)'), + 'upload() prüft, ob das Zielverzeichnis beschreibbar ist' + ); + $this->assert( + (bool)preg_match('/if \(!move_uploaded_file\(/', $controller), + 'Ein fehlgeschlagenes move_uploaded_file() wird ausgewertet statt ignoriert' + ); + $this->assert( + substr_count($controller, 'Beleg::releaseBelegNr($belegNr)') >= 2, + 'Bei einem Speicherfehler wird die vergebene Belegnummer wieder freigegeben' + ); + $this->assert( + str_contains($controller, 'Bitte die Schreibrechte im Belegverzeichnis prüfen.'), + 'Der Nutzer bekommt eine verständliche Meldung statt einer Ausnahme' + ); + } +} + +/** + * Hilfsklasse: macht die geschützten Aufräum-Methoden von TestCase prüfbar. + * tearDown() wird bewusst nicht aufgerufen – das würde die Transaktion des + * laufenden Tests zurückrollen. + */ +class AufraeumProbe extends TestCase +{ + public function run(): void + { + } + + public function tempPathOeffentlich(string $suffix = ''): string + { + return $this->tempPath($suffix); + } + + public function makeTempDirOeffentlich(string $pfad): string + { + return $this->makeTempDir($pfad); + } + + public function registerTempPathOeffentlich(string $pfad): string + { + return $this->registerTempPath($pfad); + } + + public function cleanupOeffentlich(): void + { + $this->cleanupTempPaths(); + } +} diff --git a/tests/TestCase.php b/tests/TestCase.php index fd46558..0b4ac15 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -19,6 +19,9 @@ abstract class TestCase protected array $failures = []; protected string $testName = ''; + /** @var string[] Im Test angelegte Pfade, die tearDown() wieder entfernt. */ + private array $tempPaths = []; + abstract public function run(): void; protected function setUp(): void @@ -33,6 +36,109 @@ abstract class TestCase } catch (\Throwable) { // Transaktion war evtl. nicht aktiv } + $this->cleanupTempPaths(); + } + + /** + * Meldet einen Pfad zum automatischen Aufräumen an; tearDown() entfernt ihn + * anschließend rekursiv, auch wenn der Test mit einer Ausnahme abbricht. + * + * Nötig, weil der Rollback nur die Datenbank zurückdreht: eine im Test + * vergebene Belegnummer wird danach erneut vergeben, ein liegengebliebenes + * Verzeichnis blockiert dann den echten Upload. + */ + protected function registerTempPath(string $path): string + { + $this->tempPaths[] = $path; + return $path; + } + + /** + * Legt ein Verzeichnis an und meldet es zum Aufräumen an. + */ + protected function makeTempDir(string $path, int $mode = 0755): string + { + if (is_dir($path)) { + return $this->registerTempPath($path); + } + + // Auch die von mkdir() nebenbei angelegten Elternverzeichnisse anmelden – + // sonst bleibt etwa ein leeres Jahresverzeichnis zurück. + $oberstesNeue = $path; + while (!is_dir(dirname($oberstesNeue)) && dirname($oberstesNeue) !== $oberstesNeue) { + $oberstesNeue = dirname($oberstesNeue); + } + + mkdir($path, $mode, true); + $this->registerTempPath($path); + if ($oberstesNeue !== $path) { + $this->registerTempPath($oberstesNeue); + } + return $path; + } + + /** + * Liefert einen eindeutigen, noch nicht existierenden Pfad im System-Temp- + * Verzeichnis und meldet ihn zum Aufräumen an. + */ + protected function tempPath(string $suffix = ''): string + { + $name = 'steuerkram_test_' . bin2hex(random_bytes(6)) . $suffix; + return $this->registerTempPath(sys_get_temp_dir() . '/' . $name); + } + + protected function cleanupTempPaths(): void + { + // Umgekehrte Reihenfolge: verschachtelte Pfade vor ihren Elternverzeichnissen + foreach (array_reverse($this->tempPaths) as $path) { + if (!file_exists($path) && !is_link($path)) { + continue; + } + if (!self::isInsideCleanupRoot($path)) { + echo " \e[33m!\e[0m Nicht aufgeräumt (außerhalb der Testverzeichnisse): {$path}\n"; + continue; + } + self::removeRecursive($path); + } + $this->tempPaths = []; + } + + /** + * Sicherheitsnetz: entfernt wird ausschließlich unterhalb der Temp- und + * Datenverzeichnisse – und nie eines dieser Verzeichnisse selbst. + */ + private static function isInsideCleanupRoot(string $path): bool + { + $real = realpath($path); + if ($real === false) { + $real = realpath(dirname($path)); + if ($real === false) return false; + $real .= '/' . basename($path); + } + + foreach ([sys_get_temp_dir(), \Steuerkram\Config::get('upload_dir'), \Steuerkram\Config::get('logo_dir')] as $root) { + $rootReal = is_string($root) && $root !== '' ? realpath($root) : false; + if ($rootReal !== false && str_starts_with($real, $rootReal . '/')) { + return true; + } + } + return false; + } + + private static function removeRecursive(string $path): void + { + if (is_link($path) || !is_dir($path)) { + @unlink($path); + return; + } + + // Tests setzen Verzeichnisse teils schreibgeschützt, um Fehlerfälle zu prüfen + @chmod($path, 0755); + foreach (scandir($path) ?: [] as $entry) { + if ($entry === '.' || $entry === '..') continue; + self::removeRecursive($path . '/' . $entry); + } + @rmdir($path); } protected function assert(bool $condition, string $message): void diff --git a/tests/e2e/README.md b/tests/e2e/README.md index d4397a5..cc7182d 100644 --- a/tests/e2e/README.md +++ b/tests/e2e/README.md @@ -122,9 +122,10 @@ absteigend, damit möglichst viele Nummern zurückwandern. deshalb **nach** dem Konto setzen. - **Ein Redirect auf `/login` endet mit Status 200.** `gotoOk()` prüft deshalb zusätzlich den Pfad — sonst schlagen Tests mit irreführenden Meldungen fehl. -- **Belegverzeichnisse aus PHP-Testläufen:** `docker compose exec web php tests/run.php` - läuft als *root* und kann in `data/belege//` ein Verzeichnis anlegen, das +- **Belegverzeichnisse aus PHP-Testläufen:** Die PHP-Suite immer mit + `docker compose exec -u www-data web php tests/run.php` starten. Ohne `-u www-data` + läuft sie als *root* und kann in `data/belege//` ein Verzeichnis anlegen, das root gehört, während die Transaktion die Belegnummer zurückrollt. Trifft später ein echter Upload auf dieselbe Nummer, scheitert `move_uploaded_file()` mit - HTTP 500. Solche Leichen finden: + HTTP 500. Solche Leichen aus früheren Läufen finden: `find data/belege -maxdepth 2 -user root` diff --git a/tests/run.php b/tests/run.php index 2a1b4de..7646dea 100644 --- a/tests/run.php +++ b/tests/run.php @@ -29,6 +29,49 @@ require __DIR__ . '/TestCase.php'; $jsonMode = in_array('--json', $argv ?? [], true); +/** + * Erfasst den Bestand unterhalb der Datenverzeichnisse. + * + * Ein Vergleich vor/nach dem Lauf deckt Rückstände auf, die ein Test nicht über + * TestCase::registerTempPath() angemeldet hat. Solche Reste sind gefährlich, + * weil der Rollback die Belegnummer wieder freigibt: ein späterer echter Upload + * trifft dann auf ein bereits existierendes Verzeichnis. + */ +function datenBestand(): array +{ + $bestand = []; + foreach ([\Steuerkram\Config::get('upload_dir'), \Steuerkram\Config::get('logo_dir')] as $root) { + if (!is_string($root) || !is_dir($root)) continue; + $iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($root, FilesystemIterator::SKIP_DOTS), + RecursiveIteratorIterator::SELF_FIRST + ); + foreach ($iterator as $entry) { + $bestand[$entry->getPathname()] = true; + } + } + return $bestand; +} + +/** + * Liefert die neu hinzugekommenen Pfade, ohne deren Unterpfade (sonst meldet + * ein neues Verzeichnis auch jede darin liegende Datei einzeln). + */ +function neuePfade(array $vorher, array $nachher): array +{ + $neu = array_keys(array_diff_key($nachher, $vorher)); + sort($neu); + + $oberste = []; + foreach ($neu as $pfad) { + foreach ($oberste as $bekannt) { + if (str_starts_with($pfad, $bekannt . '/')) continue 2; + } + $oberste[] = $pfad; + } + return $oberste; +} + // Alle Test-Dateien laden $testFiles = glob(__DIR__ . '/[0-9][0-9]_*.php'); sort($testFiles); @@ -57,6 +100,8 @@ if (!$jsonMode) { $allFailures = []; +$dataVorher = datenBestand(); + foreach ($testClasses as $class) { /** @var TestCase $test */ $test = new $class(); @@ -64,7 +109,8 @@ foreach ($testClasses as $class) { $allFailures = array_merge($allFailures, $failures); } -$summary = TestCase::globalSummary(); +$summary = TestCase::globalSummary(); +$rueckstaende = neuePfade($dataVorher, datenBestand()); if (!$jsonMode) { $total = $summary['passed'] + $summary['failed']; @@ -77,7 +123,16 @@ if (!$jsonMode) { echo " • {$failure}\n"; } } - echo "\e[1m══════════════════════════════════════════\e[0m\n\n"; + echo "\e[1m══════════════════════════════════════════\e[0m\n"; + + if ($rueckstaende) { + echo "\n\e[33mHinweis: Der Testlauf hat Dateien in den Datenverzeichnissen hinterlassen:\e[0m\n"; + foreach ($rueckstaende as $pfad) { + echo " • {$pfad}\n"; + } + echo " Der zuständige Test sollte den Pfad über \e[1m\$this->registerTempPath()\e[0m anmelden.\n"; + } + echo "\n"; } else { echo json_encode([ 'passed' => $summary['passed'],