From 9c6bb597d2860f796c2ef03486fe7f207f081ab7 Mon Sep 17 00:00:00 2001 From: fds2610-bot Date: Mon, 24 Aug 2026 22:52:00 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20#200:=20Admin-Pr=C3=BCfung=20f=C3=BCr=20a?= =?UTF-8?q?lle=20Settings-Verwaltungsendpoints=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 | 16 ++++ .../settings-admin-authorization.spec.js | 93 +++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100644 tests/e2e/specs/settings-admin-authorization.spec.js diff --git a/Changes.md b/Changes.md index 0be8b8d..6ea0b22 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,11 @@ --- +## 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. +- **`tests/e2e/specs/settings-admin-authorization.spec.js`** (neu): prüft auf HTTP-Ebene, dass ein Nicht-Admin-Account von den o.g. Endpoints mit 403 abgewiesen wird, während ein Admin weiterhin Zugriff hat. + ## 2026-08-21 – scripts/ nach Ausführungsort aufgeräumt: Host / Container / intern (#196) - **`scripts/`:** Bislang lagen 16 Skripte mit vier verschiedenen Aufrufkonventionen flach nebeneinander – von außen war nicht erkennbar, was direkt vom Host aufrufbar ist, was `docker compose exec` braucht und was nie manuell laufen darf. Jetzt gibt es genau ein Ordnungskriterium: `scripts/` (Host, ausnahmslos direkt aufrufbar), `scripts/container/` (`docker compose exec web ...`) und `scripts/intern/` (Entrypoint/Container-Start, nie manuell). diff --git a/src/Controllers/SettingsController.php b/src/Controllers/SettingsController.php index 6906c51..b980025 100644 --- a/src/Controllers/SettingsController.php +++ b/src/Controllers/SettingsController.php @@ -192,6 +192,8 @@ class SettingsController extends BaseController public function deleteLogo(array $params): void { + Auth::requireAdmin(); + Setting::deleteLogo(); View::redirect('/settings', 'Logo gelöscht.'); } @@ -204,6 +206,8 @@ class SettingsController extends BaseController public function llmTest(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $result = (new \Steuerkram\Services\LlmService())->testConnection(); echo json_encode($result); @@ -211,6 +215,8 @@ class SettingsController extends BaseController public function bankImport(array $params): void { + Auth::requireAdmin(); + $formats = BankSource::all('csv'); $suggestion = $_SESSION['bank_import_suggestion'] ?? null; @@ -226,6 +232,8 @@ class SettingsController extends BaseController public function deleteBankImportFormat(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $id = (int)($params['id'] ?? 0); @@ -242,6 +250,8 @@ class SettingsController extends BaseController public function storeSuggestion(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $data = json_decode(file_get_contents('php://input'), true); if (is_array($data)) { @@ -255,6 +265,8 @@ class SettingsController extends BaseController public function dismissSuggestion(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); unset($_SESSION['bank_import_suggestion']); echo json_encode(['ok' => true]); @@ -262,6 +274,8 @@ class SettingsController extends BaseController public function createBankImportFormat(array $params): void { + Auth::requireAdmin(); + $dateFormats = ['d.m.Y', 'd.m.y', 'Y-m-d', 'd/m/Y', 'm/d/Y', 'Y.m.d', 'd-m-Y']; $name = trim($_POST['name'] ?? ''); @@ -366,6 +380,8 @@ class SettingsController extends BaseController public function updateBankImportField(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $id = (int)($params['id'] ?? 0); diff --git a/tests/e2e/specs/settings-admin-authorization.spec.js b/tests/e2e/specs/settings-admin-authorization.spec.js new file mode 100644 index 0000000..afbeb48 --- /dev/null +++ b/tests/e2e/specs/settings-admin-authorization.spec.js @@ -0,0 +1,93 @@ +// @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. +// Dieser Test prüft die Durchsetzung auf HTTP-Ebene, die die PHP-Suite +// nicht erreicht (Router-Guard, Auth::requireAdmin()). + +const { test, expect } = require('@playwright/test') +const { uniqueTag } = require('../helpers/app') + +test.describe('Autorisierung: Settings-Verwaltungsendpoints nur für Admins (Fix #200)', () => { + test('Nicht-Admin wird von Logo-, Bankimport- und LLM-Test-Endpoints ausgesperrt', async ({ page, browser }) => { + const username = uniqueTag('noadmin').toLowerCase().replace(/[^a-z0-9-]/g, '').slice(0, 64) + const password = 'TempPass123!' + + await page.goto('/useradmin') + await page.fill('#field-username', username) + await page.fill('#field-password', password) + await page.click('#submit-btn') + await page.waitForURL('/useradmin') + + const row = page.locator('tr', { hasText: username }) + await expect(row).toBeVisible() + + try { + const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }) + const userPage = await context.newPage() + await userPage.goto('/login') + await userPage.fill('#username', username) + await userPage.fill('#password', password) + await userPage.click('form[action="/login"] button[type="submit"]') + await userPage.waitForURL(url => !url.pathname.startsWith('/login')) + + await userPage.goto('/settings') + const csrfToken = await userPage.evaluate(() => document.querySelector('meta[name="csrf-token"]')?.content || '') + expect(csrfToken).not.toBe('') + + const headers = { 'X-CSRF-Token': csrfToken } + + const bankImportGet = await userPage.request.get('/settings/bankimport') + expect(bankImportGet.status()).toBe(403) + + const deleteLogo = await userPage.request.post('/settings/logo/delete', { headers, form: {} }) + expect(deleteLogo.status()).toBe(403) + + const llmTest = await userPage.request.post('/settings/llm-test', { headers, form: {} }) + expect(llmTest.status()).toBe(403) + + const createFormat = await userPage.request.post('/settings/bankimport/create', { + headers, + form: { name: 'X', kuerzel: 'X' }, + }) + expect(createFormat.status()).toBe(403) + + const suggest = await userPage.request.post('/settings/bankimport/suggest', { + headers, + data: {}, + }) + expect(suggest.status()).toBe(403) + + const dismiss = await userPage.request.post('/settings/bankimport/dismiss-suggestion', { headers, form: {} }) + expect(dismiss.status()).toBe(403) + + const updateField = await userPage.request.post('/settings/bankimport/1/field', { + headers, + form: { field: 'name', value: 'X' }, + }) + expect(updateField.status()).toBe(403) + + 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]) { + await expect(response.text()).resolves.toContain('Kein Zugriff') + } + + await context.close() + } finally { + const delForm = row.locator('form[action$="/delete"]') + if (await delForm.count() > 0) { + await delForm.locator('button[type="submit"]').click() + await page.waitForLoadState('networkidle') + } + } + }) + + test('Admin darf /settings/bankimport weiterhin aufrufen (kein 403)', async ({ page }) => { + const response = await page.goto('/settings/bankimport') + expect(response.status()).not.toBe(403) + }) +})