From cfd867bcd7722b7eddc62b2eb23081bfb7dde75a Mon Sep 17 00:00:00 2001 From: fs Date: Fri, 24 Apr 2026 17:59:57 +0200 Subject: [PATCH] refactor(actions): centralize POST/CSRF guard flow --- core/Support/helpers/request.php | 89 +++++++++++++++++++ pages/admin/settings/favicon().php | 7 +- pages/admin/settings/favicon-delete().php | 10 +-- pages/admin/settings/logo().php | 7 +- pages/admin/settings/logo-delete().php | 10 +-- pages/admin/settings/run-user-lifecycle().php | 8 +- pages/admin/tenants/avatar($id).php | 7 +- pages/admin/tenants/avatar-delete($id).php | 7 +- pages/admin/tenants/favicon($id).php | 7 +- pages/admin/tenants/favicon-delete($id).php | 7 +- pages/admin/users/activate($id).php | 33 +++---- pages/admin/users/avatar($id).php | 7 +- pages/admin/users/avatar-delete($id).php | 7 +- pages/admin/users/deactivate($id).php | 20 +++-- pages/admin/users/delete($id).php | 20 +++-- .../PostEndpointCsrfContractTest.php | 4 +- 16 files changed, 175 insertions(+), 75 deletions(-) diff --git a/core/Support/helpers/request.php b/core/Support/helpers/request.php index 7c6da35..20e5281 100644 --- a/core/Support/helpers/request.php +++ b/core/Support/helpers/request.php @@ -4,6 +4,9 @@ use MintyPHP\Http\Input\FormErrors; use MintyPHP\Http\Input\RequestInput; use MintyPHP\Http\Input\RequestInputFactory; use MintyPHP\Http\Request; +use MintyPHP\Router; +use MintyPHP\Service\Access\AuthorizationDecision; +use MintyPHP\Session; use MintyPHP\Support\Flash; function requestInput(): RequestInput @@ -38,6 +41,92 @@ function flashFormErrors(FormErrors $errors, ?string $scope = null, string $keyP } } +/** + * Enforce POST request method for mutation actions. + * + * Browser requests are redirected to $redirect. + * Optional JSON mode returns 405 with a JSON error payload. + */ +function actionRequirePost( + string $redirect, + bool $jsonAware = false, + string $jsonError = 'method_not_allowed' +): bool { + if (requestInput()->isMethod('POST')) { + return true; + } + + if ($jsonAware && Request::wantsJson()) { + header('Allow: POST'); + http_response_code(405); + Router::json(['error' => trim($jsonError) !== '' ? trim($jsonError) : 'method_not_allowed']); + return false; + } + + Router::redirect($redirect); + return false; +} + +/** + * Enforce CSRF protection for POST actions. + * + * Returns false after responding/redirecting when token validation fails. + */ +function actionRequireCsrf( + string $redirect, + ?string $flashScope = null, + string $flashKeyPrefix = 'csrf_expired', + bool $jsonAware = false, + string $jsonError = 'csrf', + bool $flashOnFailure = true +): bool { + if (Session::checkCsrfToken()) { + return true; + } + + if ($jsonAware && Request::wantsJson()) { + http_response_code(400); + Router::json(['error' => trim($jsonError) !== '' ? trim($jsonError) : 'csrf']); + return false; + } + + if ($flashOnFailure) { + $errorBag = formErrors(); + $errorBag->addGlobal(t('Form expired, please try again')); + flashFormErrors($errorBag, $flashScope ?? $redirect, $flashKeyPrefix); + } + + Router::redirect($redirect); + return false; +} + +/** + * Handle authorization decisions consistently for JSON and browser requests. + */ +function actionAllowOrDeny( + AuthorizationDecision $decision, + string $redirect = 'error/forbidden', + bool $jsonAware = true +): bool { + if ($decision->isAllowed()) { + return true; + } + + if ($jsonAware && Request::wantsJson()) { + http_response_code($decision->status()); + Router::json(['error' => $decision->error()]); + return false; + } + + if ($redirect === 'error/forbidden') { + Router::redirect('error/forbidden?url=' . urlencode(Request::pathWithQuery())); + return false; + } + + Router::redirect($redirect); + return false; +} + /** * Consume and remove flash messages matching any provided scope. * diff --git a/pages/admin/settings/favicon().php b/pages/admin/settings/favicon().php index 894a014..76837fc 100644 --- a/pages/admin/settings/favicon().php +++ b/pages/admin/settings/favicon().php @@ -18,8 +18,11 @@ if (!$decision->isAllowed()) { Guard::deny(); } -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/settings'); +if (!actionRequirePost('admin/settings')) { + return; +} + +if (!actionRequireCsrf('admin/settings', 'admin/settings', 'favicon_upload')) { return; } diff --git a/pages/admin/settings/favicon-delete().php b/pages/admin/settings/favicon-delete().php index b3882cd..7f4acc4 100644 --- a/pages/admin/settings/favicon-delete().php +++ b/pages/admin/settings/favicon-delete().php @@ -3,7 +3,6 @@ use MintyPHP\Http\SessionStoreInterface; use MintyPHP\Router; use MintyPHP\Service\Access\SettingsAuthorizationPolicy; -use MintyPHP\Session; use MintyPHP\Support\Flash; use MintyPHP\Support\Guard; @@ -19,16 +18,11 @@ if (!$decision->isAllowed()) { Guard::deny(); } -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/settings'); +if (!actionRequirePost('admin/settings')) { return; } -$errorBag = formErrors(); -if (!Session::checkCsrfToken()) { - $errorBag->addGlobal(t('Form expired, please try again')); - flashFormErrors($errorBag, 'admin/settings', 'settings_favicon_delete'); - Router::redirect('admin/settings'); +if (!actionRequireCsrf('admin/settings', 'admin/settings', 'settings_favicon_delete')) { return; } diff --git a/pages/admin/settings/logo().php b/pages/admin/settings/logo().php index f466b82..426b54d 100644 --- a/pages/admin/settings/logo().php +++ b/pages/admin/settings/logo().php @@ -18,8 +18,11 @@ if (!$decision->isAllowed()) { Guard::deny(); } -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/settings'); +if (!actionRequirePost('admin/settings')) { + return; +} + +if (!actionRequireCsrf('admin/settings', 'admin/settings', 'logo_upload')) { return; } diff --git a/pages/admin/settings/logo-delete().php b/pages/admin/settings/logo-delete().php index 60269ef..0005174 100644 --- a/pages/admin/settings/logo-delete().php +++ b/pages/admin/settings/logo-delete().php @@ -3,7 +3,6 @@ use MintyPHP\Http\SessionStoreInterface; use MintyPHP\Router; use MintyPHP\Service\Access\SettingsAuthorizationPolicy; -use MintyPHP\Session; use MintyPHP\Support\Flash; use MintyPHP\Support\Guard; @@ -19,16 +18,11 @@ if (!$decision->isAllowed()) { Guard::deny(); } -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/settings'); +if (!actionRequirePost('admin/settings')) { return; } -$errorBag = formErrors(); -if (!Session::checkCsrfToken()) { - $errorBag->addGlobal(t('Form expired, please try again')); - flashFormErrors($errorBag, 'admin/settings', 'settings_logo_delete'); - Router::redirect('admin/settings'); +if (!actionRequireCsrf('admin/settings', 'admin/settings', 'settings_logo_delete')) { return; } diff --git a/pages/admin/settings/run-user-lifecycle().php b/pages/admin/settings/run-user-lifecycle().php index 0aa7a92..f4e4249 100644 --- a/pages/admin/settings/run-user-lifecycle().php +++ b/pages/admin/settings/run-user-lifecycle().php @@ -18,8 +18,12 @@ if (!$decision->isAllowed()) { Guard::deny(); } -if (requestInput()->method() !== 'POST') { - Router::redirect('admin/settings'); +if (!actionRequirePost('admin/settings')) { + return; +} + +if (!actionRequireCsrf('admin/settings', 'admin/settings', 'user_lifecycle_run')) { + return; } $errorBag = formErrors(); diff --git a/pages/admin/tenants/avatar($id).php b/pages/admin/tenants/avatar($id).php index 09c5197..523d283 100644 --- a/pages/admin/tenants/avatar($id).php +++ b/pages/admin/tenants/avatar($id).php @@ -11,8 +11,11 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $tenantAvatarService = app(\MintyPHP\Service\Tenant\TenantAvatarService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/tenants'); +if (!actionRequirePost('admin/tenants')) { + return; +} + +if (!actionRequireCsrf('admin/tenants', 'admin/tenants', 'tenant_avatar')) { return; } diff --git a/pages/admin/tenants/avatar-delete($id).php b/pages/admin/tenants/avatar-delete($id).php index b2aebb3..b883dd8 100644 --- a/pages/admin/tenants/avatar-delete($id).php +++ b/pages/admin/tenants/avatar-delete($id).php @@ -11,8 +11,11 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $tenantAvatarService = app(\MintyPHP\Service\Tenant\TenantAvatarService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/tenants'); +if (!actionRequirePost('admin/tenants')) { + return; +} + +if (!actionRequireCsrf('admin/tenants', 'admin/tenants', 'tenant_avatar_delete')) { return; } diff --git a/pages/admin/tenants/favicon($id).php b/pages/admin/tenants/favicon($id).php index d7b7b05..f53b11a 100644 --- a/pages/admin/tenants/favicon($id).php +++ b/pages/admin/tenants/favicon($id).php @@ -11,8 +11,11 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $tenantFaviconService = app(\MintyPHP\Service\Tenant\TenantFaviconService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/tenants'); +if (!actionRequirePost('admin/tenants')) { + return; +} + +if (!actionRequireCsrf('admin/tenants', 'admin/tenants', 'tenant_favicon')) { return; } diff --git a/pages/admin/tenants/favicon-delete($id).php b/pages/admin/tenants/favicon-delete($id).php index 9af91ce..d9e6115 100644 --- a/pages/admin/tenants/favicon-delete($id).php +++ b/pages/admin/tenants/favicon-delete($id).php @@ -11,8 +11,11 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $tenantFaviconService = app(\MintyPHP\Service\Tenant\TenantFaviconService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/tenants'); +if (!actionRequirePost('admin/tenants')) { + return; +} + +if (!actionRequireCsrf('admin/tenants', 'admin/tenants', 'tenant_favicon_delete')) { return; } diff --git a/pages/admin/users/activate($id).php b/pages/admin/users/activate($id).php index 288ba25..e440f56 100644 --- a/pages/admin/users/activate($id).php +++ b/pages/admin/users/activate($id).php @@ -4,7 +4,6 @@ use MintyPHP\Http\Request; use MintyPHP\Http\SessionStoreInterface; use MintyPHP\Router; use MintyPHP\Service\Access\UserAuthorizationPolicy; -use MintyPHP\Session; use MintyPHP\Support\Guard; $session = app(SessionStoreInterface::class)->all(); @@ -12,24 +11,20 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $userAccountService = app(\MintyPHP\Service\User\UserAccountService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/users'); +if (!actionRequirePost('admin/users')) { + return; +} + +if (!actionRequireCsrf( + 'admin/users', + 'admin/users', + 'user_activate', + jsonAware: true +)) { return; } $errorBag = formErrors(); -if (!Session::checkCsrfToken()) { - if (Request::wantsJson()) { - http_response_code(400); - Router::json(['error' => 'csrf']); - return; - } - $errorBag->addGlobal(t('Form expired, please try again')); - flashFormErrors($errorBag, 'admin/users', 'user_activate'); - Router::redirect('admin/users'); - return; -} - $uuid = trim((string) ($id ?? '')); $currentUserId = (int) ($session['user']['id'] ?? 0); $user = $uuid !== '' ? $userAccountService->findByUuid($uuid) : null; @@ -38,13 +33,7 @@ $decision = $authorizationService->authorize(UserAuthorizationPolicy::ABILITY_AD 'actor_user_id' => $currentUserId, 'target_user_id' => $userId, ]); -if (!$decision->isAllowed()) { - if (Request::wantsJson()) { - http_response_code($decision->status()); - Router::json(['error' => $decision->error()]); - return; - } - Router::redirect('error/forbidden?url=' . urlencode(Request::pathWithQuery())); +if (!actionAllowOrDeny($decision)) { return; } $result = $userAccountService->setActiveByUuid($uuid, true, $currentUserId); diff --git a/pages/admin/users/avatar($id).php b/pages/admin/users/avatar($id).php index e733c1f..e782d16 100644 --- a/pages/admin/users/avatar($id).php +++ b/pages/admin/users/avatar($id).php @@ -12,8 +12,11 @@ $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class $userAccountService = app(\MintyPHP\Service\User\UserAccountService::class); $userAvatarService = app(\MintyPHP\Service\User\UserAvatarService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin'); +if (!actionRequirePost('admin')) { + return; +} + +if (!actionRequireCsrf('admin', 'admin', 'user_avatar_upload')) { return; } diff --git a/pages/admin/users/avatar-delete($id).php b/pages/admin/users/avatar-delete($id).php index b9c870b..24658a5 100644 --- a/pages/admin/users/avatar-delete($id).php +++ b/pages/admin/users/avatar-delete($id).php @@ -12,8 +12,11 @@ $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class $userAccountService = app(\MintyPHP\Service\User\UserAccountService::class); $userAvatarService = app(\MintyPHP\Service\User\UserAvatarService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin'); +if (!actionRequirePost('admin')) { + return; +} + +if (!actionRequireCsrf('admin', 'admin', 'user_avatar_delete')) { return; } diff --git a/pages/admin/users/deactivate($id).php b/pages/admin/users/deactivate($id).php index 2308d3a..195ab3a 100644 --- a/pages/admin/users/deactivate($id).php +++ b/pages/admin/users/deactivate($id).php @@ -11,8 +11,16 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $userAccountService = app(\MintyPHP\Service\User\UserAccountService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/users'); +if (!actionRequirePost('admin/users')) { + return; +} + +if (!actionRequireCsrf( + 'admin/users', + 'admin/users', + 'user_deactivate', + jsonAware: true +)) { return; } @@ -25,13 +33,7 @@ $decision = $authorizationService->authorize(UserAuthorizationPolicy::ABILITY_AD 'actor_user_id' => $currentUserId, 'target_user_id' => $userId, ]); -if (!$decision->isAllowed()) { - if (Request::wantsJson()) { - http_response_code($decision->status()); - Router::json(['error' => $decision->error()]); - return; - } - Router::redirect('error/forbidden?url=' . urlencode(Request::pathWithQuery())); +if (!actionAllowOrDeny($decision)) { return; } $result = $userAccountService->setActiveByUuid($uuid, false, $currentUserId); diff --git a/pages/admin/users/delete($id).php b/pages/admin/users/delete($id).php index 881bd79..a2e1d60 100644 --- a/pages/admin/users/delete($id).php +++ b/pages/admin/users/delete($id).php @@ -12,8 +12,16 @@ Guard::requireLogin(); $authorizationService = app(\MintyPHP\Service\Access\AuthorizationService::class); $userAccountService = app(\MintyPHP\Service\User\UserAccountService::class); -if ((requestInput()->method()) !== 'POST') { - Router::redirect('admin/users'); +if (!actionRequirePost('admin/users')) { + return; +} + +if (!actionRequireCsrf( + 'admin/users', + 'admin/users', + 'user_delete', + jsonAware: true +)) { return; } @@ -26,13 +34,7 @@ $decision = $authorizationService->authorize(UserAuthorizationPolicy::ABILITY_AD 'actor_user_id' => $currentUserId, 'target_user_id' => $userId, ]); -if (!$decision->isAllowed()) { - if (Request::wantsJson()) { - http_response_code($decision->status()); - Router::json(['error' => $decision->error()]); - return; - } - Router::redirect('error/forbidden?url=' . urlencode(Request::pathWithQuery())); +if (!actionAllowOrDeny($decision)) { return; } $result = $userAccountService->deleteByUuid($uuid, $currentUserId); diff --git a/tests/Architecture/PostEndpointCsrfContractTest.php b/tests/Architecture/PostEndpointCsrfContractTest.php index 9e35c6a..39d6da4 100644 --- a/tests/Architecture/PostEndpointCsrfContractTest.php +++ b/tests/Architecture/PostEndpointCsrfContractTest.php @@ -44,7 +44,7 @@ class PostEndpointCsrfContractTest extends TestCase continue; } - if (!str_contains($content, 'checkCsrfToken')) { + if (!str_contains($content, 'checkCsrfToken') && !str_contains($content, 'actionRequireCsrf(')) { $missing[] = $relativePath; } } @@ -72,6 +72,8 @@ class PostEndpointCsrfContractTest extends TestCase private function handlesPostData(string $content): bool { return str_contains($content, "isMethod('POST')") + || str_contains($content, "requestInput()->method()") + || str_contains($content, "(requestInput()->method())") || str_contains($content, 'hasBody(') || str_contains($content, 'bodyAll()') || preg_match('/->body\(/', $content) === 1;