From 6fef9eff24b2b147a9d92d82ce2cae7aad2a7633 Mon Sep 17 00:00:00 2001 From: fds2610-bot Date: Wed, 26 Aug 2026 07:26:28 +0200 Subject: [PATCH 1/2] =?UTF-8?q?Fix=20#202:=20Admin-Pr=C3=BCfung=20bei=20/s?= =?UTF-8?q?ettings/logo=20(Logo-Upload)=20erg=C3=A4nzt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Changes.md | 5 +++++ src/Controllers/SettingsController.php | 2 ++ .../settings-admin-authorization.spec.js | 19 +++++++++++++------ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Changes.md b/Changes.md index 6ea0b22..570a9c4 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,11 @@ --- +## 2026-08-26 – Fehlende Admin-Prüfung bei /settings/logo (Logo-Upload) (#202) + +- **`src/Controllers/SettingsController.php`:** `uploadLogo()` wurde bei Fix #200 übersehen und rief `Auth::requireAdmin()` nicht auf, obwohl `deleteLogo()` bereits abgesichert ist. Da `/settings` nicht im globalen Admin-Gate in `public/index.php` enthalten ist, konnte jeder eingeloggte Nicht-Admin-Benutzer per `POST /settings/logo` das Firmenlogo austauschen. `Auth::requireAdmin()` als erste Zeile ergänzt, analog zu `deleteLogo()`. +- **`tests/e2e/specs/settings-admin-authorization.spec.js`:** Fall für `POST /settings/logo` (Multipart-Upload) ergänzt, der wie die übrigen Endpoints 403 für Nicht-Admins erwartet. + ## 2026-08-24 – Fehlende Admin-Prüfung bei mehreren SettingsController-Endpoints (#200) - **`src/Controllers/SettingsController.php`:** `deleteLogo()`, `llmTest()`, `bankImport()`, `createBankImportFormat()`, `updateBankImportField()`, `deleteBankImportFormat()`, `storeSuggestion()` und `dismissSuggestion()` prüften bislang nur den generellen Login (`Auth::require()` im Router-Gate), nicht die Rolle `admin` – anders als `update()`/`mailTest()`, deren Code-Kommentar diesen Bereich bewusst als admin-pflichtig begründet. Ein eingeloggter Benutzer mit Rolle `user` konnte damit das Firmenlogo löschen, CSV-Bankimport-Formate anlegen/ändern/löschen (Integritätsrisiko für importierte Buchungen) und den LLM-Verbindungstest auslösen. Alle acht Methoden rufen jetzt `Auth::requireAdmin()` konsistent zu Beginn auf. diff --git a/src/Controllers/SettingsController.php b/src/Controllers/SettingsController.php index b980025..4062560 100644 --- a/src/Controllers/SettingsController.php +++ b/src/Controllers/SettingsController.php @@ -94,6 +94,8 @@ class SettingsController extends BaseController public function uploadLogo(array $params): void { + Auth::requireAdmin(); + if (empty($_FILES['logo']) || $_FILES['logo']['error'] !== UPLOAD_ERR_OK) { View::redirect('/settings', 'Kein gültiges Bild übermittelt.', 'error'); return; diff --git a/tests/e2e/specs/settings-admin-authorization.spec.js b/tests/e2e/specs/settings-admin-authorization.spec.js index afbeb48..e10e386 100644 --- a/tests/e2e/specs/settings-admin-authorization.spec.js +++ b/tests/e2e/specs/settings-admin-authorization.spec.js @@ -1,9 +1,10 @@ // @ts-check -// Fix #200: Mehrere SettingsController-Endpoints (Logo, LLM-Test, -// CSV-Bankimport-Formate) prüften Auth::requireAdmin() nicht, obwohl der -// Bereich laut Code-Kommentar bei update() bewusst admin-pflichtig sein -// soll. Ein normaler "user"-Account konnte damit Firmenlogo löschen, -// Bankimport-Formate anlegen/ändern/löschen und den LLM-Test auslösen. +// Fix #200 / #202: Mehrere SettingsController-Endpoints (Logo-Upload, +// Logo-Löschen, LLM-Test, CSV-Bankimport-Formate) prüften +// Auth::requireAdmin() nicht, obwohl der Bereich laut Code-Kommentar bei +// update() bewusst admin-pflichtig sein soll. Ein normaler "user"-Account +// konnte damit das Firmenlogo hochladen/löschen, Bankimport-Formate +// anlegen/ändern/löschen und den LLM-Test auslösen. // Dieser Test prüft die Durchsetzung auf HTTP-Ebene, die die PHP-Suite // nicht erreicht (Router-Guard, Auth::requireAdmin()). @@ -42,6 +43,12 @@ test.describe('Autorisierung: Settings-Verwaltungsendpoints nur für Admins (Fix const bankImportGet = await userPage.request.get('/settings/bankimport') expect(bankImportGet.status()).toBe(403) + const uploadLogo = await userPage.request.post('/settings/logo', { + headers, + multipart: { logo: { name: 'logo.png', mimeType: 'image/png', buffer: Buffer.from([]) } }, + }) + expect(uploadLogo.status()).toBe(403) + const deleteLogo = await userPage.request.post('/settings/logo/delete', { headers, form: {} }) expect(deleteLogo.status()).toBe(403) @@ -72,7 +79,7 @@ test.describe('Autorisierung: Settings-Verwaltungsendpoints nur für Admins (Fix const deleteFormat = await userPage.request.post('/settings/bankimport/1/delete', { headers, form: {} }) expect(deleteFormat.status()).toBe(403) - for (const response of [bankImportGet, deleteLogo, llmTest, createFormat, suggest, dismiss, updateField, deleteFormat]) { + for (const response of [bankImportGet, uploadLogo, deleteLogo, llmTest, createFormat, suggest, dismiss, updateField, deleteFormat]) { await expect(response.text()).resolves.toContain('Kein Zugriff') } From 47ec4ed1b9c97d485b7971ed0d7b6ee061e2702b Mon Sep 17 00:00:00 2001 From: fds2610-bot Date: Wed, 26 Aug 2026 07:31:37 +0200 Subject: [PATCH 2/2] =?UTF-8?q?Fix=20#203:=20Admin-Pr=C3=BCfung=20im=20Ban?= =?UTF-8?q?kinstituteController=20erg=C3=A4nzt?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Changes.md | 5 ++++ src/Controllers/BankinstituteController.php | 7 +++++ .../settings-admin-authorization.spec.js | 28 ++++++++++++++++--- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/Changes.md b/Changes.md index 570a9c4..174a841 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,11 @@ --- +## 2026-08-26 – Fehlende Admin-Prüfung im BankinstituteController (#203) + +- **`src/Controllers/BankinstituteController.php`:** `index()`, `check()` und `import()` prüften bislang keine Rolle, obwohl der globale Admin-Gate in `public/index.php` nur `/useradmin*` abdeckt und `SettingsController` erst seit Fix #200 admin-pflichtig ist – dieser Controller wurde dabei übersehen. Insbesondere `import()` ersetzt über `Bankinstitut::replaceAll()` die komplette BLZ/BIC-Referenztabelle (u. a. für die FinTS-Direktverbindung genutzt) anhand einer hochgeladenen Datei; jeder eingeloggte Nicht-Admin-Nutzer konnte diese systemweite Tabelle beliebig überschreiben. Alle drei Methoden rufen jetzt `Auth::requireAdmin()` zu Beginn auf. `search()` bleibt bewusst offen, da sie unter der eigenen Route `/bankinstitute/search` für die BLZ-Suche beim Anlegen einer FinTS-Verbindung (`bank/fints.php`) von allen eingeloggten Nutzern benötigt wird. +- **`tests/e2e/specs/settings-admin-authorization.spec.js`:** Fälle für `GET /settings/bankinstitute`, `POST /settings/bankinstitute/check` und `POST /settings/bankinstitute/import` ergänzt (403 für Nicht-Admins), sowie ein Check, dass `GET /bankinstitute/search` weiterhin für alle eingeloggten Nutzer erreichbar bleibt. + ## 2026-08-26 – Fehlende Admin-Prüfung bei /settings/logo (Logo-Upload) (#202) - **`src/Controllers/SettingsController.php`:** `uploadLogo()` wurde bei Fix #200 übersehen und rief `Auth::requireAdmin()` nicht auf, obwohl `deleteLogo()` bereits abgesichert ist. Da `/settings` nicht im globalen Admin-Gate in `public/index.php` enthalten ist, konnte jeder eingeloggte Nicht-Admin-Benutzer per `POST /settings/logo` das Firmenlogo austauschen. `Auth::requireAdmin()` als erste Zeile ergänzt, analog zu `deleteLogo()`. diff --git a/src/Controllers/BankinstituteController.php b/src/Controllers/BankinstituteController.php index ac93a40..03f31d2 100644 --- a/src/Controllers/BankinstituteController.php +++ b/src/Controllers/BankinstituteController.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Steuerkram\Controllers; +use Steuerkram\Auth; use Steuerkram\Models\Bankinstitut; use Steuerkram\Services\FintsInstituteImport; use Steuerkram\View; @@ -14,6 +15,8 @@ class BankinstituteController extends BaseController public function index(array $params): void { + Auth::requireAdmin(); + $query = trim((string)($_GET['q'] ?? '')); $rows = $query !== '' ? Bankinstitut::search($query, 200) : []; @@ -31,6 +34,8 @@ class BankinstituteController extends BaseController public function check(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $analysis = $this->analyzeUpload(); @@ -52,6 +57,8 @@ class BankinstituteController extends BaseController public function import(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $analysis = $this->analyzeUpload(); diff --git a/tests/e2e/specs/settings-admin-authorization.spec.js b/tests/e2e/specs/settings-admin-authorization.spec.js index e10e386..77ae021 100644 --- a/tests/e2e/specs/settings-admin-authorization.spec.js +++ b/tests/e2e/specs/settings-admin-authorization.spec.js @@ -1,10 +1,12 @@ // @ts-check -// Fix #200 / #202: Mehrere SettingsController-Endpoints (Logo-Upload, -// Logo-Löschen, LLM-Test, CSV-Bankimport-Formate) prüften +// Fix #200 / #202 / #203: Mehrere SettingsController- und +// BankinstituteController-Endpoints (Logo-Upload, Logo-Löschen, LLM-Test, +// CSV-Bankimport-Formate, Bankinstitute-Verwaltung) prüften // Auth::requireAdmin() nicht, obwohl der Bereich laut Code-Kommentar bei // update() bewusst admin-pflichtig sein soll. Ein normaler "user"-Account // konnte damit das Firmenlogo hochladen/löschen, Bankimport-Formate -// anlegen/ändern/löschen und den LLM-Test auslösen. +// anlegen/ändern/löschen, den LLM-Test auslösen und die systemweite +// Bankinstitute-Referenztabelle (BLZ/BIC) überschreiben. // Dieser Test prüft die Durchsetzung auf HTTP-Ebene, die die PHP-Suite // nicht erreicht (Router-Guard, Auth::requireAdmin()). @@ -79,10 +81,28 @@ test.describe('Autorisierung: Settings-Verwaltungsendpoints nur für Admins (Fix const deleteFormat = await userPage.request.post('/settings/bankimport/1/delete', { headers, form: {} }) expect(deleteFormat.status()).toBe(403) - for (const response of [bankImportGet, uploadLogo, deleteLogo, llmTest, createFormat, suggest, dismiss, updateField, deleteFormat]) { + const bankinstituteGet = await userPage.request.get('/settings/bankinstitute') + expect(bankinstituteGet.status()).toBe(403) + + const bankinstituteCheck = await userPage.request.post('/settings/bankinstitute/check', { + headers, + multipart: { institute_file: { name: 'inst.csv', mimeType: 'text/csv', buffer: Buffer.from('') } }, + }) + expect(bankinstituteCheck.status()).toBe(403) + + const bankinstituteImport = await userPage.request.post('/settings/bankinstitute/import', { + headers, + multipart: { institute_file: { name: 'inst.csv', mimeType: 'text/csv', buffer: Buffer.from('') } }, + }) + expect(bankinstituteImport.status()).toBe(403) + + for (const response of [bankImportGet, uploadLogo, deleteLogo, llmTest, createFormat, suggest, dismiss, updateField, deleteFormat, bankinstituteGet, bankinstituteCheck, bankinstituteImport]) { await expect(response.text()).resolves.toContain('Kein Zugriff') } + const bankinstituteSearch = await userPage.request.get('/bankinstitute/search?q=100') + expect(bankinstituteSearch.status()).toBe(200) + await context.close() } finally { const delForm = row.locator('form[action$="/delete"]')